[PATCH 0/3] Thermal reset support in PMC

[PATCH 0/3] Thermal reset support in PMC

[Mikko Perttunen]

Hi,

this series adds support for hardware-triggered thermal reset to the PMC
driver. Namely, it adds device tree properties for specifying the I2C
command to be sent when thermtrip is triggered. It is to be noted
that thermtrip won't be ever triggered without a soctherm driver to
calibrate the sensors, but I'll follow up with that patch.

pmc.c required some juggling around to make the match data usable in
probe, since I didn't want to put the code into the initcall either, since
the soctherm driver won't be initialized by that point anyway.

Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.

Thanks,
Mikko

Mikko Perttunen (3):
  of: Add descriptions of thermtrip properties to Tegra PMC bindings
  ARM: tegra: Add PMC thermtrip programming to Jetson TK1 device tree
  ARM: tegra: Add thermal reset (thermtrip) support to PMC

 .../bindings/arm/tegra/nvidia,tegra20-pmc.txt      |  13 +
 arch/arm/boot/dts/tegra124-jetson-tk1.dts          |   5 +
 drivers/soc/tegra/pmc.c                            | 348 +++++++++++++--------
 3 files changed, 234 insertions(+), 132 deletions(-)

-- 
1.8.1.5

[PATCH 1/3] of: Add descriptions of thermtrip properties to Tegra PMC bindings

[Mikko Perttunen]

Hardware-triggered thermal reset requires configuring the I2C
reset procedure. This configuration is read from the device tree,
so document the relevant properties in the binding documentation.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 .../devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt    | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
index 68ac65f..140e2aa 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
@@ -38,6 +38,19 @@ Required properties when nvidia,suspend-mode is specified:
 			      Core power good time in uS.
 - nvidia,core-pwr-off-time : Core power off time in uS.
 
+Required properties for hardware-triggered thermal reset:
+    (only tegra30, tegra114, tegra124)
+- nvidia,thermtrip-pmu-i2c-addr : I2C address of the power management unit.
+- nvidia,thermtrip-i2c-controller : Index of the I2C controller the PMU is
+                                    attached to.
+- nvidia,thermtrip-reg-addr : Address (byte) to send reset command to.
+- nvidia,thermtrip-reg-data : Data (byte) to use as reset command.
+
+Optional properties for hardware-triggered thermal reset:
+    (only tegra30, tegra114, tegra124)
+- nvidia,thermtrip-pinmux : Pinmux ID used for I2C access.
+- nvidia,thermtrip-pmu-16bit-ops : Use 16-bit operations.
+
 Required properties when nvidia,suspend-mode=<0>:
 - nvidia,lp0-vec : <start length> Starting address and length of LP0 vector
   The LP0 vector contains the warm boot code that is executed by AVP when
-- 
1.8.1.5

Re: [PATCH 1/3] of: Add descriptions of thermtrip properties to Tegra PMC bindings

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

On Tue, Aug 05, 2014 at 11:12:58AM +0300, Mikko Perttunen wrote:
> Hardware-triggered thermal reset requires configuring the I2C
> reset procedure. This configuration is read from the device tree,
> so document the relevant properties in the binding documentation.
> 
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt    | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> index 68ac65f..140e2aa 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
> @@ -38,6 +38,19 @@ Required properties when nvidia,suspend-mode is specified:
>  			      Core power good time in uS.
>  - nvidia,core-pwr-off-time : Core power off time in uS.
>  
> +Required properties for hardware-triggered thermal reset:
> +    (only tegra30, tegra114, tegra124)
> +- nvidia,thermtrip-pmu-i2c-addr : I2C address of the power management unit.
> +- nvidia,thermtrip-i2c-controller : Index of the I2C controller the PMU is
> +                                    attached to.

This duplicates information already associated with the PMU device. Can
this be turned into something like:

	nvidia,thermtrip-pmu: phandle to Power Management Unit

Then we can query the relevant information from the I2C client resolved
from the phandle.

One problem with that might be that the I2C controller index may not
match the hardware ID.

> +- nvidia,thermtrip-reg-addr : Address (byte) to send reset command to.
> +- nvidia,thermtrip-reg-data : Data (byte) to use as reset command.
> +
> +Optional properties for hardware-triggered thermal reset:
> +    (only tegra30, tegra114, tegra124)
> +- nvidia,thermtrip-pinmux : Pinmux ID used for I2C access.

I suppose this takes a phandle? If so the description should probably
say so.

> +- nvidia,thermtrip-pmu-16bit-ops : Use 16-bit operations.

What exactly does "16-bit operations" mean? And isn't this a property of
the I2C device, therefore could be queried from the I2C slave via the
phandle?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] of: Add descriptions of thermtrip properties to Tegra PMC bindings

[Mikko Perttunen]

On 13/08/14 10:35, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Aug 05, 2014 at 11:12:58AM +0300, Mikko Perttunen wrote:
>> Hardware-triggered thermal reset requires configuring the I2C
>> reset procedure. This configuration is read from the device tree,
>> so document the relevant properties in the binding documentation.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   .../devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt    | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> index 68ac65f..140e2aa 100644
>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.txt
>> @@ -38,6 +38,19 @@ Required properties when nvidia,suspend-mode is specified:
>>   			      Core power good time in uS.
>>   - nvidia,core-pwr-off-time : Core power off time in uS.
>>
>> +Required properties for hardware-triggered thermal reset:
>> +    (only tegra30, tegra114, tegra124)
>> +- nvidia,thermtrip-pmu-i2c-addr : I2C address of the power management unit.
>> +- nvidia,thermtrip-i2c-controller : Index of the I2C controller the PMU is
>> +                                    attached to.
>
> This duplicates information already associated with the PMU device. Can
> this be turned into something like:
>
> 	nvidia,thermtrip-pmu: phandle to Power Management Unit
>
> Then we can query the relevant information from the I2C client resolved
> from the phandle.

True, that would look nicer.

>
> One problem with that might be that the I2C controller index may not
> match the hardware ID.

Maybe we could resort to checking the controller address in this case. 
This is a safety feature, so programming the wrong controller index 
accidentally would be bad.

>
>> +- nvidia,thermtrip-reg-addr : Address (byte) to send reset command to.
>> +- nvidia,thermtrip-reg-data : Data (byte) to use as reset command.
>> +
>> +Optional properties for hardware-triggered thermal reset:
>> +    (only tegra30, tegra114, tegra124)
>> +- nvidia,thermtrip-pinmux : Pinmux ID used for I2C access.
>
> I suppose this takes a phandle? If so the description should probably
> say so.

No, it takes a pinmux ID, described in the boot process (non-public in 
T124 I guess..) section of the TRM. It seems, though, that all platforms 
supported by the downstream kernel have pinmux == 0.

>
>> +- nvidia,thermtrip-pmu-16bit-ops : Use 16-bit operations.
>
> What exactly does "16-bit operations" mean? And isn't this a property of
> the I2C device, therefore could be queried from the I2C slave via the
> phandle?

That's how it's described in the TRM, but I just found a comment in the 
downstream kernel about it. Apparently it controls the amount of data 
sent to the PMIC. The downstream kernel says, though, that the option is 
not supported and must always be left as zero, so I guess it could be 
dropped.

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

Cheers,
Mikko

Re: [PATCH 1/3] of: Add descriptions of thermtrip properties to Tegra PMC bindings

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2773 bytes --]

On Wed, Aug 13, 2014 at 10:51:02AM +0300, Mikko Perttunen wrote:
> On 13/08/14 10:35, Thierry Reding wrote:
> >On Tue, Aug 05, 2014 at 11:12:58AM +0300, Mikko Perttunen wrote:
[...]
> >>+Required properties for hardware-triggered thermal reset:
> >>+    (only tegra30, tegra114, tegra124)
> >>+- nvidia,thermtrip-pmu-i2c-addr : I2C address of the power management unit.
> >>+- nvidia,thermtrip-i2c-controller : Index of the I2C controller the PMU is
> >>+                                    attached to.
> >
> >This duplicates information already associated with the PMU device. Can
> >this be turned into something like:
> >
> >	nvidia,thermtrip-pmu: phandle to Power Management Unit
> >
> >Then we can query the relevant information from the I2C client resolved
> >from the phandle.
> 
> True, that would look nicer.
> 
> >
> >One problem with that might be that the I2C controller index may not
> >match the hardware ID.
> 
> Maybe we could resort to checking the controller address in this case. This
> is a safety feature, so programming the wrong controller index accidentally
> would be bad.

What we've done in the past (for the display controllers) is to add a
special property that has the "hardware index" of the controller. I
think in this case it would be appropriate to add one as well. That way
we could reach into the I2C adapter of the I2C client and parse it from
the associated device tree node (client->adapter->dev.of_node).

> >>+- nvidia,thermtrip-reg-addr : Address (byte) to send reset command to.
> >>+- nvidia,thermtrip-reg-data : Data (byte) to use as reset command.
> >>+
> >>+Optional properties for hardware-triggered thermal reset:
> >>+    (only tegra30, tegra114, tegra124)
> >>+- nvidia,thermtrip-pinmux : Pinmux ID used for I2C access.
> >
> >I suppose this takes a phandle? If so the description should probably
> >say so.
> 
> No, it takes a pinmux ID, described in the boot process (non-public in T124
> I guess..) section of the TRM. It seems, though, that all platforms
> supported by the downstream kernel have pinmux == 0.

Ugh... we'll need to get this documented publicly.

> >What exactly does "16-bit operations" mean? And isn't this a property of
> >the I2C device, therefore could be queried from the I2C slave via the
> >phandle?
> 
> That's how it's described in the TRM, but I just found a comment in the
> downstream kernel about it. Apparently it controls the amount of data sent
> to the PMIC. The downstream kernel says, though, that the option is not
> supported and must always be left as zero, so I guess it could be dropped.

Indeed. If ever it becomes necessary we can add it then (and fall back
to 8-bit "operations" if it's not present).

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/3] ARM: tegra: Add PMC thermtrip programming to Jetson TK1 device tree

[Mikko Perttunen]

This adds the required information to reset the board during an overheating
situation to the Jetson TK1 device tree. The thermal reset is handled by the
PMC by sending an I2C message to the PMIC. The entries specify the I2C
message to be sent.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm/boot/dts/tegra124-jetson-tk1.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
index 624b0fb..ee178c4 100644
--- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
+++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
@@ -1617,6 +1617,11 @@
 		nvidia,core-pwr-off-time = <61036>;
 		nvidia,core-power-req-active-high;
 		nvidia,sys-clock-req-active-high;
+
+		nvidia,thermtrip-pmu-i2c-addr = <0x40>;
+		nvidia,thermtrip-i2c-controller = <4>;
+		nvidia,thermtrip-reg-addr = <0x36>;
+		nvidia,thermtrip-reg-data = <0x2>;
 	};
 
 	padctl@0,7009f000 {
-- 
1.8.1.5

Re: [PATCH 2/3] ARM: tegra: Add PMC thermtrip programming to Jetson TK1 device tree

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On Tue, Aug 05, 2014 at 11:12:59AM +0300, Mikko Perttunen wrote:
> This adds the required information to reset the board during an overheating
> situation to the Jetson TK1 device tree. The thermal reset is handled by the
> PMC by sending an I2C message to the PMIC. The entries specify the I2C
> message to be sent.
> 
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra124-jetson-tk1.dts | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> index 624b0fb..ee178c4 100644
> --- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> +++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> @@ -1617,6 +1617,11 @@
>  		nvidia,core-pwr-off-time = <61036>;
>  		nvidia,core-power-req-active-high;
>  		nvidia,sys-clock-req-active-high;
> +
> +		nvidia,thermtrip-pmu-i2c-addr = <0x40>;
> +		nvidia,thermtrip-i2c-controller = <4>;
> +		nvidia,thermtrip-reg-addr = <0x36>;
> +		nvidia,thermtrip-reg-data = <0x2>;

Given the number of properties that this can potentially have, I wonder
if perhaps it would be a good idea to put this into a subnode, like
this:

		thermtrip {
			pmu-i2c-addr = <0x40>;
			i2c-controller = <4>;
			...
		};

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] ARM: tegra: Add PMC thermtrip programming to Jetson TK1 device tree

[Mikko Perttunen]

On 13/08/14 10:37, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Aug 05, 2014 at 11:12:59AM +0300, Mikko Perttunen wrote:
>> This adds the required information to reset the board during an overheating
>> situation to the Jetson TK1 device tree. The thermal reset is handled by the
>> PMC by sending an I2C message to the PMIC. The entries specify the I2C
>> message to be sent.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   arch/arm/boot/dts/tegra124-jetson-tk1.dts | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>> index 624b0fb..ee178c4 100644
>> --- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>> +++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>> @@ -1617,6 +1617,11 @@
>>   		nvidia,core-pwr-off-time = <61036>;
>>   		nvidia,core-power-req-active-high;
>>   		nvidia,sys-clock-req-active-high;
>> +
>> +		nvidia,thermtrip-pmu-i2c-addr = <0x40>;
>> +		nvidia,thermtrip-i2c-controller = <4>;
>> +		nvidia,thermtrip-reg-addr = <0x36>;
>> +		nvidia,thermtrip-reg-data = <0x2>;
>
> Given the number of properties that this can potentially have, I wonder
> if perhaps it would be a good idea to put this into a subnode, like
> this:
>
> 		thermtrip {
> 			pmu-i2c-addr = <0x40>;
> 			i2c-controller = <4>;
> 			...
> 		};

I guess that'd look a bit nicer.

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

Mikko

Re: [PATCH 2/3] ARM: tegra: Add PMC thermtrip programming to Jetson TK1 device tree

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

On Wed, Aug 13, 2014 at 10:52:18AM +0300, Mikko Perttunen wrote:
> On 13/08/14 10:37, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Tue, Aug 05, 2014 at 11:12:59AM +0300, Mikko Perttunen wrote:
> >>This adds the required information to reset the board during an overheating
> >>situation to the Jetson TK1 device tree. The thermal reset is handled by the
> >>PMC by sending an I2C message to the PMIC. The entries specify the I2C
> >>message to be sent.
> >>
> >>Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> >>---
> >>  arch/arm/boot/dts/tegra124-jetson-tk1.dts | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >>diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> >>index 624b0fb..ee178c4 100644
> >>--- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> >>+++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
> >>@@ -1617,6 +1617,11 @@
> >>  		nvidia,core-pwr-off-time = <61036>;
> >>  		nvidia,core-power-req-active-high;
> >>  		nvidia,sys-clock-req-active-high;
> >>+
> >>+		nvidia,thermtrip-pmu-i2c-addr = <0x40>;
> >>+		nvidia,thermtrip-i2c-controller = <4>;
> >>+		nvidia,thermtrip-reg-addr = <0x36>;
> >>+		nvidia,thermtrip-reg-data = <0x2>;
> >
> >Given the number of properties that this can potentially have, I wonder
> >if perhaps it would be a good idea to put this into a subnode, like
> >this:
> >
> >		thermtrip {
> >			pmu-i2c-addr = <0x40>;
> >			i2c-controller = <4>;
> >			...
> >		};
> 
> I guess that'd look a bit nicer.

Mind you, this may be somewhat on the edge of what's considered good
practice for DT content, so maybe wait for a second opinion before going
ahead with this.

I don't have any objections to a set of nvidia,thermtrip-* properties, I
just think it's fairly redundant to keep repeating that prefix (and it
causes ugliness in drivers, too).

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] ARM: tegra: Add PMC thermtrip programming to Jetson TK1 device tree

[Mikko Perttunen]

On 13/08/14 11:03, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Aug 13, 2014 at 10:52:18AM +0300, Mikko Perttunen wrote:
>> On 13/08/14 10:37, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Tue, Aug 05, 2014 at 11:12:59AM +0300, Mikko Perttunen wrote:
>>>> This adds the required information to reset the board during an overheating
>>>> situation to the Jetson TK1 device tree. The thermal reset is handled by the
>>>> PMC by sending an I2C message to the PMIC. The entries specify the I2C
>>>> message to be sent.
>>>>
>>>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>   arch/arm/boot/dts/tegra124-jetson-tk1.dts | 5 +++++
>>>>   1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>>>> index 624b0fb..ee178c4 100644
>>>> --- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>>>> +++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>>>> @@ -1617,6 +1617,11 @@
>>>>   		nvidia,core-pwr-off-time = <61036>;
>>>>   		nvidia,core-power-req-active-high;
>>>>   		nvidia,sys-clock-req-active-high;
>>>> +
>>>> +		nvidia,thermtrip-pmu-i2c-addr = <0x40>;
>>>> +		nvidia,thermtrip-i2c-controller = <4>;
>>>> +		nvidia,thermtrip-reg-addr = <0x36>;
>>>> +		nvidia,thermtrip-reg-data = <0x2>;
>>>
>>> Given the number of properties that this can potentially have, I wonder
>>> if perhaps it would be a good idea to put this into a subnode, like
>>> this:
>>>
>>> 		thermtrip {
>>> 			pmu-i2c-addr = <0x40>;
>>> 			i2c-controller = <4>;
>>> 			...
>>> 		};
>>
>> I guess that'd look a bit nicer.
>
> Mind you, this may be somewhat on the edge of what's considered good
> practice for DT content, so maybe wait for a second opinion before going
> ahead with this.
>
> I don't have any objections to a set of nvidia,thermtrip-* properties, I
> just think it's fairly redundant to keep repeating that prefix (and it
> causes ugliness in drivers, too).

Hmm, ok. I'll fix the other stuff first.

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

[PATCH 3/3] ARM: tegra: Add thermal reset (thermtrip) support to PMC

[Mikko Perttunen]

This adds a device tree controlled option to enable PMC-based
thermal reset in overheating situations. Thermtrip is supported on
Tegra114 and Tegra124. The thermal reset only works when the thermal
sensors are calibrated, so a soctherm driver is also required.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 348 ++++++++++++++++++++++++++++++------------------
 1 file changed, 216 insertions(+), 132 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index a2c0ceb..2c617d7 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -83,11 +83,28 @@
 
 #define GPU_RG_CNTRL			0x2d4
 
+#define PMC_SENSOR_CTRL			0x1b0
+#define PMC_SENSOR_CTRL_SCRATCH_WRITE	(1 << 2)
+#define PMC_SENSOR_CTRL_ENABLE_RST	(1 << 1)
+
+#define PMC_SCRATCH54			0x258
+#define PMC_SCRATCH54_DATA_SHIFT	8
+#define PMC_SCRATCH54_ADDR_SHIFT	0
+
+#define PMC_SCRATCH55			0x25c
+#define PMC_SCRATCH55_RESET_TEGRA	(1 << 31)
+#define PMC_SCRATCH55_CNTRL_ID_SHIFT	27
+#define PMC_SCRATCH55_PINMUX_SHIFT	24
+#define PMC_SCRATCH55_16BITOP		(1 << 15)
+#define PMC_SCRATCH55_CHECKSUM_SHIFT	16
+#define PMC_SCRATCH55_I2CSLV1_SHIFT	0
+
 struct tegra_pmc_soc {
 	unsigned int num_powergates;
 	const char *const *powergates;
 	unsigned int num_cpu_powergates;
 	const u8 *cpu_powergates;
+	bool has_thermal_reset;
 };
 
 /**
@@ -606,6 +623,203 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
 }
 #endif
 
+static const char * const tegra20_powergates[] = {
+	[TEGRA_POWERGATE_CPU] = "cpu",
+	[TEGRA_POWERGATE_3D] = "3d",
+	[TEGRA_POWERGATE_VENC] = "venc",
+	[TEGRA_POWERGATE_VDEC] = "vdec",
+	[TEGRA_POWERGATE_PCIE] = "pcie",
+	[TEGRA_POWERGATE_L2] = "l2",
+	[TEGRA_POWERGATE_MPE] = "mpe",
+};
+
+static const struct tegra_pmc_soc tegra20_pmc_soc = {
+	.num_powergates = ARRAY_SIZE(tegra20_powergates),
+	.powergates = tegra20_powergates,
+	.num_cpu_powergates = 0,
+	.cpu_powergates = NULL,
+};
+
+static const char * const tegra30_powergates[] = {
+	[TEGRA_POWERGATE_CPU] = "cpu0",
+	[TEGRA_POWERGATE_3D] = "3d0",
+	[TEGRA_POWERGATE_VENC] = "venc",
+	[TEGRA_POWERGATE_VDEC] = "vdec",
+	[TEGRA_POWERGATE_PCIE] = "pcie",
+	[TEGRA_POWERGATE_L2] = "l2",
+	[TEGRA_POWERGATE_MPE] = "mpe",
+	[TEGRA_POWERGATE_HEG] = "heg",
+	[TEGRA_POWERGATE_SATA] = "sata",
+	[TEGRA_POWERGATE_CPU1] = "cpu1",
+	[TEGRA_POWERGATE_CPU2] = "cpu2",
+	[TEGRA_POWERGATE_CPU3] = "cpu3",
+	[TEGRA_POWERGATE_CELP] = "celp",
+	[TEGRA_POWERGATE_3D1] = "3d1",
+};
+
+static const u8 tegra30_cpu_powergates[] = {
+	TEGRA_POWERGATE_CPU,
+	TEGRA_POWERGATE_CPU1,
+	TEGRA_POWERGATE_CPU2,
+	TEGRA_POWERGATE_CPU3,
+};
+
+static const struct tegra_pmc_soc tegra30_pmc_soc = {
+	.num_powergates = ARRAY_SIZE(tegra30_powergates),
+	.powergates = tegra30_powergates,
+	.num_cpu_powergates = ARRAY_SIZE(tegra30_cpu_powergates),
+	.cpu_powergates = tegra30_cpu_powergates,
+	.has_thermal_reset = true,
+};
+
+static const char * const tegra114_powergates[] = {
+	[TEGRA_POWERGATE_CPU] = "crail",
+	[TEGRA_POWERGATE_3D] = "3d",
+	[TEGRA_POWERGATE_VENC] = "venc",
+	[TEGRA_POWERGATE_VDEC] = "vdec",
+	[TEGRA_POWERGATE_MPE] = "mpe",
+	[TEGRA_POWERGATE_HEG] = "heg",
+	[TEGRA_POWERGATE_CPU1] = "cpu1",
+	[TEGRA_POWERGATE_CPU2] = "cpu2",
+	[TEGRA_POWERGATE_CPU3] = "cpu3",
+	[TEGRA_POWERGATE_CELP] = "celp",
+	[TEGRA_POWERGATE_CPU0] = "cpu0",
+	[TEGRA_POWERGATE_C0NC] = "c0nc",
+	[TEGRA_POWERGATE_C1NC] = "c1nc",
+	[TEGRA_POWERGATE_DIS] = "dis",
+	[TEGRA_POWERGATE_DISB] = "disb",
+	[TEGRA_POWERGATE_XUSBA] = "xusba",
+	[TEGRA_POWERGATE_XUSBB] = "xusbb",
+	[TEGRA_POWERGATE_XUSBC] = "xusbc",
+};
+
+static const u8 tegra114_cpu_powergates[] = {
+	TEGRA_POWERGATE_CPU0,
+	TEGRA_POWERGATE_CPU1,
+	TEGRA_POWERGATE_CPU2,
+	TEGRA_POWERGATE_CPU3,
+};
+
+static const struct tegra_pmc_soc tegra114_pmc_soc = {
+	.num_powergates = ARRAY_SIZE(tegra114_powergates),
+	.powergates = tegra114_powergates,
+	.num_cpu_powergates = ARRAY_SIZE(tegra114_cpu_powergates),
+	.cpu_powergates = tegra114_cpu_powergates,
+	.has_thermal_reset = true,
+};
+
+static const char * const tegra124_powergates[] = {
+	[TEGRA_POWERGATE_CPU] = "crail",
+	[TEGRA_POWERGATE_3D] = "3d",
+	[TEGRA_POWERGATE_VENC] = "venc",
+	[TEGRA_POWERGATE_PCIE] = "pcie",
+	[TEGRA_POWERGATE_VDEC] = "vdec",
+	[TEGRA_POWERGATE_L2] = "l2",
+	[TEGRA_POWERGATE_MPE] = "mpe",
+	[TEGRA_POWERGATE_HEG] = "heg",
+	[TEGRA_POWERGATE_SATA] = "sata",
+	[TEGRA_POWERGATE_CPU1] = "cpu1",
+	[TEGRA_POWERGATE_CPU2] = "cpu2",
+	[TEGRA_POWERGATE_CPU3] = "cpu3",
+	[TEGRA_POWERGATE_CELP] = "celp",
+	[TEGRA_POWERGATE_CPU0] = "cpu0",
+	[TEGRA_POWERGATE_C0NC] = "c0nc",
+	[TEGRA_POWERGATE_C1NC] = "c1nc",
+	[TEGRA_POWERGATE_SOR] = "sor",
+	[TEGRA_POWERGATE_DIS] = "dis",
+	[TEGRA_POWERGATE_DISB] = "disb",
+	[TEGRA_POWERGATE_XUSBA] = "xusba",
+	[TEGRA_POWERGATE_XUSBB] = "xusbb",
+	[TEGRA_POWERGATE_XUSBC] = "xusbc",
+	[TEGRA_POWERGATE_VIC] = "vic",
+	[TEGRA_POWERGATE_IRAM] = "iram",
+};
+
+static const u8 tegra124_cpu_powergates[] = {
+	TEGRA_POWERGATE_CPU0,
+	TEGRA_POWERGATE_CPU1,
+	TEGRA_POWERGATE_CPU2,
+	TEGRA_POWERGATE_CPU3,
+};
+
+static const struct tegra_pmc_soc tegra124_pmc_soc = {
+	.num_powergates = ARRAY_SIZE(tegra124_powergates),
+	.powergates = tegra124_powergates,
+	.num_cpu_powergates = ARRAY_SIZE(tegra124_cpu_powergates),
+	.cpu_powergates = tegra124_cpu_powergates,
+	.has_thermal_reset = true,
+};
+
+static const struct of_device_id tegra_pmc_match[] = {
+	{ .compatible = "nvidia,tegra124-pmc", .data = &tegra124_pmc_soc },
+	{ .compatible = "nvidia,tegra114-pmc", .data = &tegra114_pmc_soc },
+	{ .compatible = "nvidia,tegra30-pmc", .data = &tegra30_pmc_soc },
+	{ .compatible = "nvidia,tegra20-pmc", .data = &tegra20_pmc_soc },
+	{ }
+};
+
+void tegra_pmc_init_thermal_reset(struct device_node *np)
+{
+	u32 pmu_i2c_addr, i2c_ctrl_id, reg_addr, reg_data, pinmux;
+	bool pmu_16bit_ops;
+	u32 val, checksum;
+	const struct of_device_id *match = of_match_node(tegra_pmc_match, np);
+	const struct tegra_pmc_soc *data = match->data;
+
+	if (!data->has_thermal_reset)
+		return;
+
+	pmu_16bit_ops =
+		of_property_read_bool(np, "nvidia,thermtrip-pmu-16bit-ops");
+	if (of_property_read_u32(
+		np, "nvidia,thermtrip-pmu-i2c-addr", &pmu_i2c_addr))
+		goto disabled;
+	if (of_property_read_u32(
+		np, "nvidia,thermtrip-i2c-controller", &i2c_ctrl_id))
+		goto disabled;
+	if (of_property_read_u32(
+		np, "nvidia,thermtrip-reg-addr", &reg_addr))
+		goto disabled;
+	if (of_property_read_u32(
+		np, "nvidia,thermtrip-reg-data", &reg_data))
+		goto disabled;
+	if (of_property_read_u32(
+		np, "nvidia,thermtrip-pinmux", &pinmux))
+		pinmux = 0;
+
+	val = tegra_pmc_readl(PMC_SENSOR_CTRL);
+	val |= PMC_SENSOR_CTRL_SCRATCH_WRITE | PMC_SENSOR_CTRL_ENABLE_RST;
+	tegra_pmc_writel(val, PMC_SENSOR_CTRL);
+
+	val = (reg_data << PMC_SCRATCH54_DATA_SHIFT) |
+	      (reg_addr << PMC_SCRATCH54_ADDR_SHIFT);
+	tegra_pmc_writel(val, PMC_SCRATCH54);
+
+	val = 0;
+	val |= PMC_SCRATCH55_RESET_TEGRA;
+	val |= i2c_ctrl_id << PMC_SCRATCH55_CNTRL_ID_SHIFT;
+	val |= pinmux << PMC_SCRATCH55_PINMUX_SHIFT;
+	if (pmu_16bit_ops)
+		val |= PMC_SCRATCH55_16BITOP;
+	val |= pmu_i2c_addr << PMC_SCRATCH55_I2CSLV1_SHIFT;
+
+	checksum = reg_addr + reg_data + (val & 0xFF) + ((val >> 8) & 0xFF) +
+		((val >> 24) & 0xFF);
+	checksum &= 0xFF;
+	checksum = 0x100 - checksum;
+
+	val |= checksum << PMC_SCRATCH55_CHECKSUM_SHIFT;
+
+	tegra_pmc_writel(val, PMC_SCRATCH55);
+
+	pr_info("Tegra: PMC thermal reset enabled\n");
+
+	return;
+
+disabled:
+	pr_warn("Tegra: PMC thermal reset disabled\n");
+}
+
 static int tegra_pmc_parse_dt(struct tegra_pmc *pmc, struct device_node *np)
 {
 	u32 value, values[2];
@@ -730,6 +944,8 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 	tegra_pmc_init(pmc);
 
+	tegra_pmc_init_thermal_reset(pdev->dev.of_node);
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_powergate_debugfs_init();
 		if (err < 0)
@@ -757,138 +973,6 @@ static int tegra_pmc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(tegra_pmc_pm_ops, tegra_pmc_suspend, tegra_pmc_resume);
 
-static const char * const tegra20_powergates[] = {
-	[TEGRA_POWERGATE_CPU] = "cpu",
-	[TEGRA_POWERGATE_3D] = "3d",
-	[TEGRA_POWERGATE_VENC] = "venc",
-	[TEGRA_POWERGATE_VDEC] = "vdec",
-	[TEGRA_POWERGATE_PCIE] = "pcie",
-	[TEGRA_POWERGATE_L2] = "l2",
-	[TEGRA_POWERGATE_MPE] = "mpe",
-};
-
-static const struct tegra_pmc_soc tegra20_pmc_soc = {
-	.num_powergates = ARRAY_SIZE(tegra20_powergates),
-	.powergates = tegra20_powergates,
-	.num_cpu_powergates = 0,
-	.cpu_powergates = NULL,
-};
-
-static const char * const tegra30_powergates[] = {
-	[TEGRA_POWERGATE_CPU] = "cpu0",
-	[TEGRA_POWERGATE_3D] = "3d0",
-	[TEGRA_POWERGATE_VENC] = "venc",
-	[TEGRA_POWERGATE_VDEC] = "vdec",
-	[TEGRA_POWERGATE_PCIE] = "pcie",
-	[TEGRA_POWERGATE_L2] = "l2",
-	[TEGRA_POWERGATE_MPE] = "mpe",
-	[TEGRA_POWERGATE_HEG] = "heg",
-	[TEGRA_POWERGATE_SATA] = "sata",
-	[TEGRA_POWERGATE_CPU1] = "cpu1",
-	[TEGRA_POWERGATE_CPU2] = "cpu2",
-	[TEGRA_POWERGATE_CPU3] = "cpu3",
-	[TEGRA_POWERGATE_CELP] = "celp",
-	[TEGRA_POWERGATE_3D1] = "3d1",
-};
-
-static const u8 tegra30_cpu_powergates[] = {
-	TEGRA_POWERGATE_CPU,
-	TEGRA_POWERGATE_CPU1,
-	TEGRA_POWERGATE_CPU2,
-	TEGRA_POWERGATE_CPU3,
-};
-
-static const struct tegra_pmc_soc tegra30_pmc_soc = {
-	.num_powergates = ARRAY_SIZE(tegra30_powergates),
-	.powergates = tegra30_powergates,
-	.num_cpu_powergates = ARRAY_SIZE(tegra30_cpu_powergates),
-	.cpu_powergates = tegra30_cpu_powergates,
-};
-
-static const char * const tegra114_powergates[] = {
-	[TEGRA_POWERGATE_CPU] = "crail",
-	[TEGRA_POWERGATE_3D] = "3d",
-	[TEGRA_POWERGATE_VENC] = "venc",
-	[TEGRA_POWERGATE_VDEC] = "vdec",
-	[TEGRA_POWERGATE_MPE] = "mpe",
-	[TEGRA_POWERGATE_HEG] = "heg",
-	[TEGRA_POWERGATE_CPU1] = "cpu1",
-	[TEGRA_POWERGATE_CPU2] = "cpu2",
-	[TEGRA_POWERGATE_CPU3] = "cpu3",
-	[TEGRA_POWERGATE_CELP] = "celp",
-	[TEGRA_POWERGATE_CPU0] = "cpu0",
-	[TEGRA_POWERGATE_C0NC] = "c0nc",
-	[TEGRA_POWERGATE_C1NC] = "c1nc",
-	[TEGRA_POWERGATE_DIS] = "dis",
-	[TEGRA_POWERGATE_DISB] = "disb",
-	[TEGRA_POWERGATE_XUSBA] = "xusba",
-	[TEGRA_POWERGATE_XUSBB] = "xusbb",
-	[TEGRA_POWERGATE_XUSBC] = "xusbc",
-};
-
-static const u8 tegra114_cpu_powergates[] = {
-	TEGRA_POWERGATE_CPU0,
-	TEGRA_POWERGATE_CPU1,
-	TEGRA_POWERGATE_CPU2,
-	TEGRA_POWERGATE_CPU3,
-};
-
-static const struct tegra_pmc_soc tegra114_pmc_soc = {
-	.num_powergates = ARRAY_SIZE(tegra114_powergates),
-	.powergates = tegra114_powergates,
-	.num_cpu_powergates = ARRAY_SIZE(tegra114_cpu_powergates),
-	.cpu_powergates = tegra114_cpu_powergates,
-};
-
-static const char * const tegra124_powergates[] = {
-	[TEGRA_POWERGATE_CPU] = "crail",
-	[TEGRA_POWERGATE_3D] = "3d",
-	[TEGRA_POWERGATE_VENC] = "venc",
-	[TEGRA_POWERGATE_PCIE] = "pcie",
-	[TEGRA_POWERGATE_VDEC] = "vdec",
-	[TEGRA_POWERGATE_L2] = "l2",
-	[TEGRA_POWERGATE_MPE] = "mpe",
-	[TEGRA_POWERGATE_HEG] = "heg",
-	[TEGRA_POWERGATE_SATA] = "sata",
-	[TEGRA_POWERGATE_CPU1] = "cpu1",
-	[TEGRA_POWERGATE_CPU2] = "cpu2",
-	[TEGRA_POWERGATE_CPU3] = "cpu3",
-	[TEGRA_POWERGATE_CELP] = "celp",
-	[TEGRA_POWERGATE_CPU0] = "cpu0",
-	[TEGRA_POWERGATE_C0NC] = "c0nc",
-	[TEGRA_POWERGATE_C1NC] = "c1nc",
-	[TEGRA_POWERGATE_SOR] = "sor",
-	[TEGRA_POWERGATE_DIS] = "dis",
-	[TEGRA_POWERGATE_DISB] = "disb",
-	[TEGRA_POWERGATE_XUSBA] = "xusba",
-	[TEGRA_POWERGATE_XUSBB] = "xusbb",
-	[TEGRA_POWERGATE_XUSBC] = "xusbc",
-	[TEGRA_POWERGATE_VIC] = "vic",
-	[TEGRA_POWERGATE_IRAM] = "iram",
-};
-
-static const u8 tegra124_cpu_powergates[] = {
-	TEGRA_POWERGATE_CPU0,
-	TEGRA_POWERGATE_CPU1,
-	TEGRA_POWERGATE_CPU2,
-	TEGRA_POWERGATE_CPU3,
-};
-
-static const struct tegra_pmc_soc tegra124_pmc_soc = {
-	.num_powergates = ARRAY_SIZE(tegra124_powergates),
-	.powergates = tegra124_powergates,
-	.num_cpu_powergates = ARRAY_SIZE(tegra124_cpu_powergates),
-	.cpu_powergates = tegra124_cpu_powergates,
-};
-
-static const struct of_device_id tegra_pmc_match[] = {
-	{ .compatible = "nvidia,tegra124-pmc", .data = &tegra124_pmc_soc },
-	{ .compatible = "nvidia,tegra114-pmc", .data = &tegra114_pmc_soc },
-	{ .compatible = "nvidia,tegra30-pmc", .data = &tegra30_pmc_soc },
-	{ .compatible = "nvidia,tegra20-pmc", .data = &tegra20_pmc_soc },
-	{ }
-};
-
 static struct platform_driver tegra_pmc_driver = {
 	.driver = {
 		.name = "tegra-pmc",
-- 
1.8.1.5

Re: [PATCH 3/3] ARM: tegra: Add thermal reset (thermtrip) support to PMC

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3306 bytes --]

On Tue, Aug 05, 2014 at 11:13:00AM +0300, Mikko Perttunen wrote:
> This adds a device tree controlled option to enable PMC-based
> thermal reset in overheating situations. Thermtrip is supported on
> Tegra114 and Tegra124. The thermal reset only works when the thermal

The binding updates in patch 1/3 say that Tegra30 supports thermtrip as
well.

> +void tegra_pmc_init_thermal_reset(struct device_node *np)

It would be good for this to take a struct device * so that dev_*() can
be used instead of pr_*().

> +{
> +	u32 pmu_i2c_addr, i2c_ctrl_id, reg_addr, reg_data, pinmux;
> +	bool pmu_16bit_ops;
> +	u32 val, checksum;

Nit: All other register accesses use "value" instead of "val" as the
name for this variable.

> +	const struct of_device_id *match = of_match_node(tegra_pmc_match, np);
> +	const struct tegra_pmc_soc *data = match->data;
> +
> +	if (!data->has_thermal_reset)
> +		return;
> +
> +	pmu_16bit_ops =
> +		of_property_read_bool(np, "nvidia,thermtrip-pmu-16bit-ops");

The formatting here (and below) is weird. I think this could be made
more readable by shortening both property name and/or variable name:

	pmu_16bit = of_property_read_bool(np, "nvidia,thermtrip-pmu-16bit");

And similarily for below.

> +	if (of_property_read_u32(
> +		np, "nvidia,thermtrip-pmu-i2c-addr", &pmu_i2c_addr))
> +		goto disabled;
> +	if (of_property_read_u32(
> +		np, "nvidia,thermtrip-i2c-controller", &i2c_ctrl_id))
> +		goto disabled;
> +	if (of_property_read_u32(
> +		np, "nvidia,thermtrip-reg-addr", &reg_addr))
> +		goto disabled;
> +	if (of_property_read_u32(
> +		np, "nvidia,thermtrip-reg-data", &reg_data))
> +		goto disabled;
> +	if (of_property_read_u32(
> +		np, "nvidia,thermtrip-pinmux", &pinmux))
> +		pinmux = 0;
> +
> +	val = tegra_pmc_readl(PMC_SENSOR_CTRL);
> +	val |= PMC_SENSOR_CTRL_SCRATCH_WRITE | PMC_SENSOR_CTRL_ENABLE_RST;
> +	tegra_pmc_writel(val, PMC_SENSOR_CTRL);

It's not immediately clear to me what this does (therefore it would be
good to annotate it with a comment), but if this enables thermal
tripping, shouldn't this be done *after* the registers below have been
set up?

> +
> +	val = (reg_data << PMC_SCRATCH54_DATA_SHIFT) |
> +	      (reg_addr << PMC_SCRATCH54_ADDR_SHIFT);
> +	tegra_pmc_writel(val, PMC_SCRATCH54);
> +
> +	val = 0;
> +	val |= PMC_SCRATCH55_RESET_TEGRA;
> +	val |= i2c_ctrl_id << PMC_SCRATCH55_CNTRL_ID_SHIFT;
> +	val |= pinmux << PMC_SCRATCH55_PINMUX_SHIFT;
> +	if (pmu_16bit_ops)
> +		val |= PMC_SCRATCH55_16BITOP;
> +	val |= pmu_i2c_addr << PMC_SCRATCH55_I2CSLV1_SHIFT;
> +
> +	checksum = reg_addr + reg_data + (val & 0xFF) + ((val >> 8) & 0xFF) +
> +		((val >> 24) & 0xFF);
> +	checksum &= 0xFF;

I'd prefer lower-case hexadecimals. Also, what about bits 23:16? Are
they not needed for the checksum? Again, a comment may help to explain
this.

> +	checksum = 0x100 - checksum;
> +
> +	val |= checksum << PMC_SCRATCH55_CHECKSUM_SHIFT;
> +
> +	tegra_pmc_writel(val, PMC_SCRATCH55);
> +
> +	pr_info("Tegra: PMC thermal reset enabled\n");
> +
> +	return;
> +
> +disabled:
> +	pr_warn("Tegra: PMC thermal reset disabled\n");

You're not giving anyone a clue about what went wrong, so when they see
this warning they don't know what to do about it. Maybe all paths
leading here should have a more specific warning message themselves.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] ARM: tegra: Add thermal reset (thermtrip) support to PMC

[Mikko Perttunen]

On 13/08/14 10:53, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Aug 05, 2014 at 11:13:00AM +0300, Mikko Perttunen wrote:
>> This adds a device tree controlled option to enable PMC-based
>> thermal reset in overheating situations. Thermtrip is supported on
>> Tegra114 and Tegra124. The thermal reset only works when the thermal
>
> The binding updates in patch 1/3 say that Tegra30 supports thermtrip as
> well.

Oops, forgot to update the commit message when adding Tegra30. 
Originally I wasn't sure if it was supported.

>
>> +void tegra_pmc_init_thermal_reset(struct device_node *np)
>
> It would be good for this to take a struct device * so that dev_*() can
> be used instead of pr_*().

Good point.

>
>> +{
>> +	u32 pmu_i2c_addr, i2c_ctrl_id, reg_addr, reg_data, pinmux;
>> +	bool pmu_16bit_ops;
>> +	u32 val, checksum;
>
> Nit: All other register accesses use "value" instead of "val" as the
> name for this variable.

Will fix.

>
>> +	const struct of_device_id *match = of_match_node(tegra_pmc_match, np);
>> +	const struct tegra_pmc_soc *data = match->data;
>> +
>> +	if (!data->has_thermal_reset)
>> +		return;
>> +
>> +	pmu_16bit_ops =
>> +		of_property_read_bool(np, "nvidia,thermtrip-pmu-16bit-ops");
>
> The formatting here (and below) is weird. I think this could be made
> more readable by shortening both property name and/or variable name:
>
> 	pmu_16bit = of_property_read_bool(np, "nvidia,thermtrip-pmu-16bit");
>
> And similarily for below.

Yeah, it's suboptimal. The problem will probably go away once I add the
thermtrip subnode.

>
>> +	if (of_property_read_u32(
>> +		np, "nvidia,thermtrip-pmu-i2c-addr", &pmu_i2c_addr))
>> +		goto disabled;
>> +	if (of_property_read_u32(
>> +		np, "nvidia,thermtrip-i2c-controller", &i2c_ctrl_id))
>> +		goto disabled;
>> +	if (of_property_read_u32(
>> +		np, "nvidia,thermtrip-reg-addr", &reg_addr))
>> +		goto disabled;
>> +	if (of_property_read_u32(
>> +		np, "nvidia,thermtrip-reg-data", &reg_data))
>> +		goto disabled;
>> +	if (of_property_read_u32(
>> +		np, "nvidia,thermtrip-pinmux", &pinmux))
>> +		pinmux = 0;
>> +
>> +	val = tegra_pmc_readl(PMC_SENSOR_CTRL);
>> +	val |= PMC_SENSOR_CTRL_SCRATCH_WRITE | PMC_SENSOR_CTRL_ENABLE_RST;
>> +	tegra_pmc_writel(val, PMC_SENSOR_CTRL);
>
> It's not immediately clear to me what this does (therefore it would be
> good to annotate it with a comment), but if this enables thermal
> tripping, shouldn't this be done *after* the registers below have been
> set up?

I'll move this.

>
>> +
>> +	val = (reg_data << PMC_SCRATCH54_DATA_SHIFT) |
>> +	      (reg_addr << PMC_SCRATCH54_ADDR_SHIFT);
>> +	tegra_pmc_writel(val, PMC_SCRATCH54);
>> +
>> +	val = 0;
>> +	val |= PMC_SCRATCH55_RESET_TEGRA;
>> +	val |= i2c_ctrl_id << PMC_SCRATCH55_CNTRL_ID_SHIFT;
>> +	val |= pinmux << PMC_SCRATCH55_PINMUX_SHIFT;
>> +	if (pmu_16bit_ops)
>> +		val |= PMC_SCRATCH55_16BITOP;
>> +	val |= pmu_i2c_addr << PMC_SCRATCH55_I2CSLV1_SHIFT;
>> +
>> +	checksum = reg_addr + reg_data + (val & 0xFF) + ((val >> 8) & 0xFF) +
>> +		((val >> 24) & 0xFF);
>> +	checksum &= 0xFF;
>
> I'd prefer lower-case hexadecimals. Also, what about bits 23:16? Are
> they not needed for the checksum? Again, a comment may help to explain
> this.

Yes, bits 23:16 will contain the checksum itself. I'll add a comment.

>
>> +	checksum = 0x100 - checksum;
>> +
>> +	val |= checksum << PMC_SCRATCH55_CHECKSUM_SHIFT;
>> +
>> +	tegra_pmc_writel(val, PMC_SCRATCH55);
>> +
>> +	pr_info("Tegra: PMC thermal reset enabled\n");
>> +
>> +	return;
>> +
>> +disabled:
>> +	pr_warn("Tegra: PMC thermal reset disabled\n");
>
> You're not giving anyone a clue about what went wrong, so when they see
> this warning they don't know what to do about it. Maybe all paths
> leading here should have a more specific warning message themselves.

I'll add better error messages. Adding the thermtrip node will make this 
look nicer as well, since I can just error out if the thermtrip node 
exists but a required property doesn't.

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

Thanks,
Mikko

Re: [PATCH 3/3] ARM: tegra: Add thermal reset (thermtrip) support to PMC

[Wei Ni]

On 08/05/2014 04:13 PM, Mikko Perttunen wrote:
> This adds a device tree controlled option to enable PMC-based
> thermal reset in overheating situations. Thermtrip is supported on
> Tegra114 and Tegra124. The thermal reset only works when the thermal
> sensors are calibrated, so a soctherm driver is also required.
> 
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/soc/tegra/pmc.c | 348 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 216 insertions(+), 132 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index a2c0ceb..2c617d7 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -83,11 +83,28 @@
> 
>  #define GPU_RG_CNTRL                   0x2d4
> 
> +#define PMC_SENSOR_CTRL                        0x1b0
> +#define PMC_SENSOR_CTRL_SCRATCH_WRITE  (1 << 2)
> +#define PMC_SENSOR_CTRL_ENABLE_RST     (1 << 1)
> +
> +#define PMC_SCRATCH54                  0x258
> +#define PMC_SCRATCH54_DATA_SHIFT       8
> +#define PMC_SCRATCH54_ADDR_SHIFT       0
> +
> +#define PMC_SCRATCH55                  0x25c
> +#define PMC_SCRATCH55_RESET_TEGRA      (1 << 31)
> +#define PMC_SCRATCH55_CNTRL_ID_SHIFT   27
> +#define PMC_SCRATCH55_PINMUX_SHIFT     24
> +#define PMC_SCRATCH55_16BITOP          (1 << 15)
> +#define PMC_SCRATCH55_CHECKSUM_SHIFT   16
> +#define PMC_SCRATCH55_I2CSLV1_SHIFT    0
> +
>  struct tegra_pmc_soc {
>         unsigned int num_powergates;
>         const char *const *powergates;
>         unsigned int num_cpu_powergates;
>         const u8 *cpu_powergates;
> +       bool has_thermal_reset;

According to the DRM, the scratch54/55 is used for TSENSE_RESET, so I
prefer to change the name to has_tsense_reset().

>  };
> 
>  /**
> @@ -606,6 +623,203 @@ void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>  }

> +
> +void tegra_pmc_init_thermal_reset(struct device_node *np)

I prefer to change the name to tegra_pmc_init_tsense_reset().

> +{
> +       u32 pmu_i2c_addr, i2c_ctrl_id, reg_addr, reg_data, pinmux;
> +       bool pmu_16bit_ops;
> +       u32 val, checksum;
> +       const struct of_device_id *match = of_match_node(tegra_pmc_match, np);
> +       const struct tegra_pmc_soc *data = match->data;
> +

> --
> 1.8.1.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/3] Thermal reset support in PMC

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Tue, Aug 05, 2014 at 11:12:57AM +0300, Mikko Perttunen wrote:
> Hi,
> 
> this series adds support for hardware-triggered thermal reset to the PMC
> driver. Namely, it adds device tree properties for specifying the I2C
> command to be sent when thermtrip is triggered. It is to be noted
> that thermtrip won't be ever triggered without a soctherm driver to
> calibrate the sensors, but I'll follow up with that patch.
> 
> pmc.c required some juggling around to make the match data usable in
> probe, since I didn't want to put the code into the initcall either, since
> the soctherm driver won't be initialized by that point anyway.
> 
> Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.

Can you describe the procedure used to test this? We currently have a
bunch of features in Tegra that some people have tested at some point
during development but the test procedures never got documented. That
means whenever we want to test something we need to go and reinvent a
bunch of tests after the fact.

So what I'd like to start doing is collect tests (preferably in some
scripted way) so that they can be kept in a repository that people can
easily clone and run on devices.

Could you provide something like that for thermtrip?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/3] Thermal reset support in PMC

[Mikko Perttunen]

On 13/08/14 11:07, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Aug 05, 2014 at 11:12:57AM +0300, Mikko Perttunen wrote:
>> Hi,
>>
>> this series adds support for hardware-triggered thermal reset to the PMC
>> driver. Namely, it adds device tree properties for specifying the I2C
>> command to be sent when thermtrip is triggered. It is to be noted
>> that thermtrip won't be ever triggered without a soctherm driver to
>> calibrate the sensors, but I'll follow up with that patch.
>>
>> pmc.c required some juggling around to make the match data usable in
>> probe, since I didn't want to put the code into the initcall either, since
>> the soctherm driver won't be initialized by that point anyway.
>>
>> Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.
>
> Can you describe the procedure used to test this? We currently have a
> bunch of features in Tegra that some people have tested at some point
> during development but the test procedures never got documented. That
> means whenever we want to test something we need to go and reinvent a
> bunch of tests after the fact.
>
> So what I'd like to start doing is collect tests (preferably in some
> scripted way) so that they can be kept in a repository that people can
> easily clone and run on devices.
>
> Could you provide something like that for thermtrip?

Sure. I'll see if I can make a just a test script or if a local patch is 
needed to test. Btw, I also have a pretty nice test script for EMC 
ready, and I agree that such a repository would be very nice.

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

Cheers, Mikko

Re: [PATCH 0/3] Thermal reset support in PMC

[Mikko Perttunen]

On 13/08/14 11:12, Mikko Perttunen wrote:
> On 13/08/14 11:07, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Tue, Aug 05, 2014 at 11:12:57AM +0300, Mikko Perttunen wrote:
>>> Hi,
>>>
>>> this series adds support for hardware-triggered thermal reset to the PMC
>>> driver. Namely, it adds device tree properties for specifying the I2C
>>> command to be sent when thermtrip is triggered. It is to be noted
>>> that thermtrip won't be ever triggered without a soctherm driver to
>>> calibrate the sensors, but I'll follow up with that patch.
>>>
>>> pmc.c required some juggling around to make the match data usable in
>>> probe, since I didn't want to put the code into the initcall either, since
>>> the soctherm driver won't be initialized by that point anyway.
>>>
>>> Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.
>>
>> Can you describe the procedure used to test this? We currently have a
>> bunch of features in Tegra that some people have tested at some point
>> during development but the test procedures never got documented. That
>> means whenever we want to test something we need to go and reinvent a
>> bunch of tests after the fact.
>>
>> So what I'd like to start doing is collect tests (preferably in some
>> scripted way) so that they can be kept in a repository that people can
>> easily clone and run on devices.
>>
>> Could you provide something like that for thermtrip?
>
> Sure. I'll see if I can make a just a test script or if a local patch is
> needed to test. Btw, I also have a pretty nice test script for EMC
> ready, and I agree that such a repository would be very nice.

Here is a test program. It it works, the device with immediately shut down.

https://gist.github.com/cyndis/66126c9c176b5f94a76f

>
>>
>> Thierry
>>
>> * Unknown Key
>> * 0x7F3EB3A1
>>
>
> Cheers, Mikko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/3] Thermal reset support in PMC

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

On Wed, Aug 13, 2014 at 11:42:53AM +0300, Mikko Perttunen wrote:
> 
> 
> On 13/08/14 11:12, Mikko Perttunen wrote:
> >On 13/08/14 11:07, Thierry Reding wrote:
> >>* PGP Signed by an unknown key
> >>
> >>On Tue, Aug 05, 2014 at 11:12:57AM +0300, Mikko Perttunen wrote:
> >>>Hi,
> >>>
> >>>this series adds support for hardware-triggered thermal reset to the PMC
> >>>driver. Namely, it adds device tree properties for specifying the I2C
> >>>command to be sent when thermtrip is triggered. It is to be noted
> >>>that thermtrip won't be ever triggered without a soctherm driver to
> >>>calibrate the sensors, but I'll follow up with that patch.
> >>>
> >>>pmc.c required some juggling around to make the match data usable in
> >>>probe, since I didn't want to put the code into the initcall either, since
> >>>the soctherm driver won't be initialized by that point anyway.
> >>>
> >>>Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.
> >>
> >>Can you describe the procedure used to test this? We currently have a
> >>bunch of features in Tegra that some people have tested at some point
> >>during development but the test procedures never got documented. That
> >>means whenever we want to test something we need to go and reinvent a
> >>bunch of tests after the fact.
> >>
> >>So what I'd like to start doing is collect tests (preferably in some
> >>scripted way) so that they can be kept in a repository that people can
> >>easily clone and run on devices.
> >>
> >>Could you provide something like that for thermtrip?
> >
> >Sure. I'll see if I can make a just a test script or if a local patch is
> >needed to test. Btw, I also have a pretty nice test script for EMC
> >ready, and I agree that such a repository would be very nice.
> 
> Here is a test program. It it works, the device with immediately shut down.
> 
> https://gist.github.com/cyndis/66126c9c176b5f94a76f

Is there a way to set the trip temperature without going through
/dev/mem? I'd expect the device to have a sysfs interface of some
sort.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/3] Thermal reset support in PMC

[Mikko Perttunen]

On 13/08/14 11:57, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Aug 13, 2014 at 11:42:53AM +0300, Mikko Perttunen wrote:
>>
>>
>> On 13/08/14 11:12, Mikko Perttunen wrote:
>>> On 13/08/14 11:07, Thierry Reding wrote:
>>>>> Old Signed by an unknown key
>>>>
>>>> On Tue, Aug 05, 2014 at 11:12:57AM +0300, Mikko Perttunen wrote:
>>>>> Hi,
>>>>>
>>>>> this series adds support for hardware-triggered thermal reset to the PMC
>>>>> driver. Namely, it adds device tree properties for specifying the I2C
>>>>> command to be sent when thermtrip is triggered. It is to be noted
>>>>> that thermtrip won't be ever triggered without a soctherm driver to
>>>>> calibrate the sensors, but I'll follow up with that patch.
>>>>>
>>>>> pmc.c required some juggling around to make the match data usable in
>>>>> probe, since I didn't want to put the code into the initcall either, since
>>>>> the soctherm driver won't be initialized by that point anyway.
>>>>>
>>>>> Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.
>>>>
>>>> Can you describe the procedure used to test this? We currently have a
>>>> bunch of features in Tegra that some people have tested at some point
>>>> during development but the test procedures never got documented. That
>>>> means whenever we want to test something we need to go and reinvent a
>>>> bunch of tests after the fact.
>>>>
>>>> So what I'd like to start doing is collect tests (preferably in some
>>>> scripted way) so that they can be kept in a repository that people can
>>>> easily clone and run on devices.
>>>>
>>>> Could you provide something like that for thermtrip?
>>>
>>> Sure. I'll see if I can make a just a test script or if a local patch is
>>> needed to test. Btw, I also have a pretty nice test script for EMC
>>> ready, and I agree that such a repository would be very nice.
>>
>> Here is a test program. It it works, the device with immediately shut down.
>>
>> https://gist.github.com/cyndis/66126c9c176b5f94a76f
>
> Is there a way to set the trip temperature without going through
> /dev/mem? I'd expect the device to have a sysfs interface of some
> sort.

The thermtrip "device" isn't currently exposed in any way. If it were 
exposed, I suppose it would be exposed as thermal zone devices, each 
with one trip point. Even then, the thermal framework doesn't really 
support this properly; none of the trip point types really apply to this 
kind of trip point, and x86 systems don't expose their trips either. 
Anyway, since debugging is pretty much the only use case for modifying 
the trip temperature, I thought adding the tz_devices would be a bit 
overkill.

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

Re: [PATCH 0/3] Thermal reset support in PMC

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3091 bytes --]

On Wed, Aug 13, 2014 at 12:52:22PM +0300, Mikko Perttunen wrote:
> On 13/08/14 11:57, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Wed, Aug 13, 2014 at 11:42:53AM +0300, Mikko Perttunen wrote:
> >>
> >>
> >>On 13/08/14 11:12, Mikko Perttunen wrote:
> >>>On 13/08/14 11:07, Thierry Reding wrote:
> >>>>>Old Signed by an unknown key
> >>>>
> >>>>On Tue, Aug 05, 2014 at 11:12:57AM +0300, Mikko Perttunen wrote:
> >>>>>Hi,
> >>>>>
> >>>>>this series adds support for hardware-triggered thermal reset to the PMC
> >>>>>driver. Namely, it adds device tree properties for specifying the I2C
> >>>>>command to be sent when thermtrip is triggered. It is to be noted
> >>>>>that thermtrip won't be ever triggered without a soctherm driver to
> >>>>>calibrate the sensors, but I'll follow up with that patch.
> >>>>>
> >>>>>pmc.c required some juggling around to make the match data usable in
> >>>>>probe, since I didn't want to put the code into the initcall either, since
> >>>>>the soctherm driver won't be initialized by that point anyway.
> >>>>>
> >>>>>Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.
> >>>>
> >>>>Can you describe the procedure used to test this? We currently have a
> >>>>bunch of features in Tegra that some people have tested at some point
> >>>>during development but the test procedures never got documented. That
> >>>>means whenever we want to test something we need to go and reinvent a
> >>>>bunch of tests after the fact.
> >>>>
> >>>>So what I'd like to start doing is collect tests (preferably in some
> >>>>scripted way) so that they can be kept in a repository that people can
> >>>>easily clone and run on devices.
> >>>>
> >>>>Could you provide something like that for thermtrip?
> >>>
> >>>Sure. I'll see if I can make a just a test script or if a local patch is
> >>>needed to test. Btw, I also have a pretty nice test script for EMC
> >>>ready, and I agree that such a repository would be very nice.
> >>
> >>Here is a test program. It it works, the device with immediately shut down.
> >>
> >>https://gist.github.com/cyndis/66126c9c176b5f94a76f
> >
> >Is there a way to set the trip temperature without going through
> >/dev/mem? I'd expect the device to have a sysfs interface of some
> >sort.
> 
> The thermtrip "device" isn't currently exposed in any way. If it were
> exposed, I suppose it would be exposed as thermal zone devices, each with
> one trip point. Even then, the thermal framework doesn't really support this
> properly; none of the trip point types really apply to this kind of trip
> point, and x86 systems don't expose their trips either.

Okay. The reason why I asked is because I'm not sure yet that having C
programs in the test suite would be good, so having something that's
easily scriptable would be preferred.

> Anyway, since debugging is pretty much the only use case for modifying
> the trip temperature, I thought adding the tz_devices would be a bit
> overkill.

An alternative could be a file in debugfs.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/3] Thermal reset support in PMC

[Mikko Perttunen]

On 13/08/14 13:36, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Aug 13, 2014 at 12:52:22PM +0300, Mikko Perttunen wrote:
>> On 13/08/14 11:57, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Wed, Aug 13, 2014 at 11:42:53AM +0300, Mikko Perttunen wrote:
>>>>
>>>>
>>>> On 13/08/14 11:12, Mikko Perttunen wrote:
>>>>> On 13/08/14 11:07, Thierry Reding wrote:
>>>>>>> Old Signed by an unknown key
>>>>>>
>>>>>> On Tue, Aug 05, 2014 at 11:12:57AM +0300, Mikko Perttunen wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> this series adds support for hardware-triggered thermal reset to the PMC
>>>>>>> driver. Namely, it adds device tree properties for specifying the I2C
>>>>>>> command to be sent when thermtrip is triggered. It is to be noted
>>>>>>> that thermtrip won't be ever triggered without a soctherm driver to
>>>>>>> calibrate the sensors, but I'll follow up with that patch.
>>>>>>>
>>>>>>> pmc.c required some juggling around to make the match data usable in
>>>>>>> probe, since I didn't want to put the code into the initcall either, since
>>>>>>> the soctherm driver won't be initialized by that point anyway.
>>>>>>>
>>>>>>> Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.
>>>>>>
>>>>>> Can you describe the procedure used to test this? We currently have a
>>>>>> bunch of features in Tegra that some people have tested at some point
>>>>>> during development but the test procedures never got documented. That
>>>>>> means whenever we want to test something we need to go and reinvent a
>>>>>> bunch of tests after the fact.
>>>>>>
>>>>>> So what I'd like to start doing is collect tests (preferably in some
>>>>>> scripted way) so that they can be kept in a repository that people can
>>>>>> easily clone and run on devices.
>>>>>>
>>>>>> Could you provide something like that for thermtrip?
>>>>>
>>>>> Sure. I'll see if I can make a just a test script or if a local patch is
>>>>> needed to test. Btw, I also have a pretty nice test script for EMC
>>>>> ready, and I agree that such a repository would be very nice.
>>>>
>>>> Here is a test program. It it works, the device with immediately shut down.
>>>>
>>>> https://gist.github.com/cyndis/66126c9c176b5f94a76f
>>>
>>> Is there a way to set the trip temperature without going through
>>> /dev/mem? I'd expect the device to have a sysfs interface of some
>>> sort.
>>
>> The thermtrip "device" isn't currently exposed in any way. If it were
>> exposed, I suppose it would be exposed as thermal zone devices, each with
>> one trip point. Even then, the thermal framework doesn't really support this
>> properly; none of the trip point types really apply to this kind of trip
>> point, and x86 systems don't expose their trips either.
>
> Okay. The reason why I asked is because I'm not sure yet that having C
> programs in the test suite would be good, so having something that's
> easily scriptable would be preferred.

I agree. Looks like Python's stdlib has mmap support, so maybe we could 
use that.

>
>> Anyway, since debugging is pretty much the only use case for modifying
>> the trip temperature, I thought adding the tz_devices would be a bit
>> overkill.
>
> An alternative could be a file in debugfs.

True, though there's the "don't expose dangerous stuff in debugfs" 
argument against that. I don't really have problem with it, though.
Although in this case I think the script approach would be good enough.

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

Re: [PATCH 0/3] Thermal reset support in PMC

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4207 bytes --]

On Wed, Aug 13, 2014 at 01:41:52PM +0300, Mikko Perttunen wrote:
> 
> 
> On 13/08/14 13:36, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Wed, Aug 13, 2014 at 12:52:22PM +0300, Mikko Perttunen wrote:
> >>On 13/08/14 11:57, Thierry Reding wrote:
> >>>>Old Signed by an unknown key
> >>>
> >>>On Wed, Aug 13, 2014 at 11:42:53AM +0300, Mikko Perttunen wrote:
> >>>>
> >>>>
> >>>>On 13/08/14 11:12, Mikko Perttunen wrote:
> >>>>>On 13/08/14 11:07, Thierry Reding wrote:
> >>>>>>>Old Signed by an unknown key
> >>>>>>
> >>>>>>On Tue, Aug 05, 2014 at 11:12:57AM +0300, Mikko Perttunen wrote:
> >>>>>>>Hi,
> >>>>>>>
> >>>>>>>this series adds support for hardware-triggered thermal reset to the PMC
> >>>>>>>driver. Namely, it adds device tree properties for specifying the I2C
> >>>>>>>command to be sent when thermtrip is triggered. It is to be noted
> >>>>>>>that thermtrip won't be ever triggered without a soctherm driver to
> >>>>>>>calibrate the sensors, but I'll follow up with that patch.
> >>>>>>>
> >>>>>>>pmc.c required some juggling around to make the match data usable in
> >>>>>>>probe, since I didn't want to put the code into the initcall either, since
> >>>>>>>the soctherm driver won't be initialized by that point anyway.
> >>>>>>>
> >>>>>>>Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.
> >>>>>>
> >>>>>>Can you describe the procedure used to test this? We currently have a
> >>>>>>bunch of features in Tegra that some people have tested at some point
> >>>>>>during development but the test procedures never got documented. That
> >>>>>>means whenever we want to test something we need to go and reinvent a
> >>>>>>bunch of tests after the fact.
> >>>>>>
> >>>>>>So what I'd like to start doing is collect tests (preferably in some
> >>>>>>scripted way) so that they can be kept in a repository that people can
> >>>>>>easily clone and run on devices.
> >>>>>>
> >>>>>>Could you provide something like that for thermtrip?
> >>>>>
> >>>>>Sure. I'll see if I can make a just a test script or if a local patch is
> >>>>>needed to test. Btw, I also have a pretty nice test script for EMC
> >>>>>ready, and I agree that such a repository would be very nice.
> >>>>
> >>>>Here is a test program. It it works, the device with immediately shut down.
> >>>>
> >>>>https://gist.github.com/cyndis/66126c9c176b5f94a76f
> >>>
> >>>Is there a way to set the trip temperature without going through
> >>>/dev/mem? I'd expect the device to have a sysfs interface of some
> >>>sort.
> >>
> >>The thermtrip "device" isn't currently exposed in any way. If it were
> >>exposed, I suppose it would be exposed as thermal zone devices, each with
> >>one trip point. Even then, the thermal framework doesn't really support this
> >>properly; none of the trip point types really apply to this kind of trip
> >>point, and x86 systems don't expose their trips either.
> >
> >Okay. The reason why I asked is because I'm not sure yet that having C
> >programs in the test suite would be good, so having something that's
> >easily scriptable would be preferred.
> 
> I agree. Looks like Python's stdlib has mmap support, so maybe we could use
> that.

Yes, possibly. It still means that somebody needs to have some kind of
advanced scripting available to test this. While having a testsuite is
useful, it would still be nice to occasionally be able to test things
with just simple shell commands.

> >>Anyway, since debugging is pretty much the only use case for modifying
> >>the trip temperature, I thought adding the tz_devices would be a bit
> >>overkill.
> >
> >An alternative could be a file in debugfs.
> 
> True, though there's the "don't expose dangerous stuff in debugfs" argument
> against that. I don't really have problem with it, though.
> Although in this case I think the script approach would be good enough.

I've said this before and I don't think "dangerous stuff in debugfs" is
a valid argument. You need superuser privileges to access debugfs, and
if you have such privileges you can do pretty much what you want (run
the test program you posted for example).

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/3] Thermal reset support in PMC

[Mikko Perttunen]

On 13/08/14 13:53, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Aug 13, 2014 at 01:41:52PM +0300, Mikko Perttunen wrote:
>>
>>
>> On 13/08/14 13:36, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Wed, Aug 13, 2014 at 12:52:22PM +0300, Mikko Perttunen wrote:
>>>> On 13/08/14 11:57, Thierry Reding wrote:
>>>>>> Old Signed by an unknown key
>>>>>
>>>>> On Wed, Aug 13, 2014 at 11:42:53AM +0300, Mikko Perttunen wrote:
>>>>>>
>>>>>>
>>>>>> On 13/08/14 11:12, Mikko Perttunen wrote:
>>>>>>> On 13/08/14 11:07, Thierry Reding wrote:
>>>>>>>>> Old Signed by an unknown key
>>>>>>>>
>>>>>>>> On Tue, Aug 05, 2014 at 11:12:57AM +0300, Mikko Perttunen wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> this series adds support for hardware-triggered thermal reset to the PMC
>>>>>>>>> driver. Namely, it adds device tree properties for specifying the I2C
>>>>>>>>> command to be sent when thermtrip is triggered. It is to be noted
>>>>>>>>> that thermtrip won't be ever triggered without a soctherm driver to
>>>>>>>>> calibrate the sensors, but I'll follow up with that patch.
>>>>>>>>>
>>>>>>>>> pmc.c required some juggling around to make the match data usable in
>>>>>>>>> probe, since I didn't want to put the code into the initcall either, since
>>>>>>>>> the soctherm driver won't be initialized by that point anyway.
>>>>>>>>>
>>>>>>>>> Series tested on Jetson-TK1. Should work on Tegra30 and Tegra114 too.
>>>>>>>>
>>>>>>>> Can you describe the procedure used to test this? We currently have a
>>>>>>>> bunch of features in Tegra that some people have tested at some point
>>>>>>>> during development but the test procedures never got documented. That
>>>>>>>> means whenever we want to test something we need to go and reinvent a
>>>>>>>> bunch of tests after the fact.
>>>>>>>>
>>>>>>>> So what I'd like to start doing is collect tests (preferably in some
>>>>>>>> scripted way) so that they can be kept in a repository that people can
>>>>>>>> easily clone and run on devices.
>>>>>>>>
>>>>>>>> Could you provide something like that for thermtrip?
>>>>>>>
>>>>>>> Sure. I'll see if I can make a just a test script or if a local patch is
>>>>>>> needed to test. Btw, I also have a pretty nice test script for EMC
>>>>>>> ready, and I agree that such a repository would be very nice.
>>>>>>
>>>>>> Here is a test program. It it works, the device with immediately shut down.
>>>>>>
>>>>>> https://gist.github.com/cyndis/66126c9c176b5f94a76f
>>>>>
>>>>> Is there a way to set the trip temperature without going through
>>>>> /dev/mem? I'd expect the device to have a sysfs interface of some
>>>>> sort.
>>>>
>>>> The thermtrip "device" isn't currently exposed in any way. If it were
>>>> exposed, I suppose it would be exposed as thermal zone devices, each with
>>>> one trip point. Even then, the thermal framework doesn't really support this
>>>> properly; none of the trip point types really apply to this kind of trip
>>>> point, and x86 systems don't expose their trips either.
>>>
>>> Okay. The reason why I asked is because I'm not sure yet that having C
>>> programs in the test suite would be good, so having something that's
>>> easily scriptable would be preferred.
>>
>> I agree. Looks like Python's stdlib has mmap support, so maybe we could use
>> that.
>
> Yes, possibly. It still means that somebody needs to have some kind of
> advanced scripting available to test this. While having a testsuite is
> useful, it would still be nice to occasionally be able to test things
> with just simple shell commands.
>
>>>> Anyway, since debugging is pretty much the only use case for modifying
>>>> the trip temperature, I thought adding the tz_devices would be a bit
>>>> overkill.
>>>
>>> An alternative could be a file in debugfs.
>>
>> True, though there's the "don't expose dangerous stuff in debugfs" argument
>> against that. I don't really have problem with it, though.
>> Although in this case I think the script approach would be good enough.
>
> I've said this before and I don't think "dangerous stuff in debugfs" is
> a valid argument. You need superuser privileges to access debugfs, and
> if you have such privileges you can do pretty much what you want (run
> the test program you posted for example).

I agree with you completely. I guess I can add a debugfs entry to the 
soctherm driver, probably in a separate patch once the original series 
is in. It is then up to the thermal maintainers whether they agree with 
us or not.

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>