Re: [PATCH V1 06/10] of: Add bindings of hw-trips for soctherm

Re: [PATCH V1 06/10] of: Add bindings of hw-trips for soctherm

[Thierry Reding]

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

Cc'ing devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, please make sure to always Cc that
list on device tree binding patches.

On Wed, Jan 13, 2016 at 04:00:22PM +0800, Wei Ni wrote:
> Add hw-trips sub-node for soctherm, which is
> used to describe the hardware trip points for
> each soctherm sensors.

Perhaps you should mention Tegra in the subject and the commit message?

Also please use all of the 72 columns that are at your disposal when
writing commit messages.

> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/thermal/tegra-soctherm.txt | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt b/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
> index 6b68cd150405..77aef0b4968b 100644
> --- a/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
> +++ b/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
> @@ -26,6 +26,18 @@ Required properties :
>      of this property. See <dt-bindings/thermal/tegra124-soctherm.h> for a
>      list of valid values when referring to thermal sensors.
>  
> +Optional properties:
> +- hw-trips : A sub-node which is a container of hardware trip points
> +    for each sensors.

Perhaps call the node "trip-points"? "hw-" seems kind of redundant since
DT is a hardware description, and "trips" isn't a very good abbreviation
for "trip-points".

> +  * sensors: Sub-nodes which are used to describe the HW trip points info,
> +        must be named as "cpu", "gpu", "mem", "pll".
> +      Properties:
> +      - therm-temp : Shutdown temperature in millicelsius, once the
> +        temperature of this sensor is higher than the therm-temp, the system
> +        will be reset or shutdown.

Why not call the property "shutdown-temperature" if that's what it is.

> +      - throt-temp : HW throttle temperature in millicelsius, once the
> +        temperature of this sensor is higher than the throt-temp, the HW
> +        throttle will be triggered in soctherm. (Not implement yet)

"throttle-temperature" would be a better name in my opinion. More typing
perhaps but less chance of getting the abbreviations wrong. Also there's
no need for the "(Not implement yet)" because the binding isn't about an
implementation, it's about the hardware description. If you have no code
that uses this, a safer option might be to just drop it for now.

Thierry

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

Re: [PATCH V1 06/10] of: Add bindings of hw-trips for soctherm

[Wei Ni]

On 2016年01月13日 23:25, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> Cc'ing devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, please make sure to always Cc that
> list on device tree binding patches.
> 
> On Wed, Jan 13, 2016 at 04:00:22PM +0800, Wei Ni wrote:
>> Add hw-trips sub-node for soctherm, which is
>> used to describe the hardware trip points for
>> each soctherm sensors.
> 
> Perhaps you should mention Tegra in the subject and the commit message?
> 
> Also please use all of the 72 columns that are at your disposal when
> writing commit messages.
> 
>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  .../devicetree/bindings/thermal/tegra-soctherm.txt | 29 ++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt b/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
>> index 6b68cd150405..77aef0b4968b 100644
>> --- a/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
>> +++ b/Documentation/devicetree/bindings/thermal/tegra-soctherm.txt
>> @@ -26,6 +26,18 @@ Required properties :
>>      of this property. See <dt-bindings/thermal/tegra124-soctherm.h> for a
>>      list of valid values when referring to thermal sensors.
>>  
>> +Optional properties:
>> +- hw-trips : A sub-node which is a container of hardware trip points
>> +    for each sensors.
> 
> Perhaps call the node "trip-points"? "hw-" seems kind of redundant since
> DT is a hardware description, and "trips" isn't a very good abbreviation
> for "trip-points".

In the linux thermal framework, it has the node definition "trips" and have
sysfs interface named as "trip-points", these "trip" are set for SW. If we also
use the "trip-points" in here, it will make confusion.
I used "hw-", because it's different with the SW trip-points, the trip-points
under it are for the Tegra to shutdown/reset from HW, or trigger the
HW-throttle. (not SW throttle, see below)
How about "hw-trip-points"?

> 
>> +  * sensors: Sub-nodes which are used to describe the HW trip points info,
>> +        must be named as "cpu", "gpu", "mem", "pll".
>> +      Properties:
>> +      - therm-temp : Shutdown temperature in millicelsius, once the
>> +        temperature of this sensor is higher than the therm-temp, the system
>> +        will be reset or shutdown.
> 
> Why not call the property "shutdown-temperature" if that's what it is.

This is the shutdown/reset temperature. The Tegra's SPEC calls it as thermtrip
event, when the temperature touch this value, it will trigger the thermtrip
event, and then will cause the system reset or shutdown, it depends on the HW
platform design. So I named it as "therm-temp".
I think it's better to change to "thermtrip-temperature".

> 
>> +      - throt-temp : HW throttle temperature in millicelsius, once the
>> +        temperature of this sensor is higher than the throt-temp, the HW
>> +        throttle will be triggered in soctherm. (Not implement yet)
> 
> "throttle-temperature" would be a better name in my opinion. More typing
> perhaps but less chance of getting the abbreviations wrong. Also there's
> no need for the "(Not implement yet)" because the binding isn't about an
> implementation, it's about the hardware description. If you have no code
> that uses this, a safer option might be to just drop it for now.

The Tegra soctherm support throttle function, when the temperature touch the
trip point, the HW will skip the CPU/GPU' pulse, so I called it as HW throttle.
For the SW throttle, the SW will cap the CPU/GPU or other clocks frequencies.
Anyway, since we didn't support it yet, I will drop it for now.

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