Re: [PATCH 01/10] dt-bindings: thermal: tegra: Document throttle temperature

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 17/04/2023 10:59, Thierry Reding wrote:
> On Fri, Apr 14, 2023 at 11:47:56PM +0200, Krzysztof Kozlowski wrote:
>> On 14/04/2023 14:57, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Each throttling configuration needs to specify the temperature threshold
>>> at which it should start throttling. Previously this was tied to a given
>>> trip point as a cooling device and used the temperature specified for
>>> that trip point. This doesn't work well because the throttling mechanism
>>> is not a cooling device in the traditional sense.
>>>
>>> Instead, allow device trees to specify the throttle temperature in the
>>> throttle configuration directly so that the throttle doesn't need to be
>>> exposed as a cooling device.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  .../bindings/thermal/nvidia,tegra124-soctherm.yaml         | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/nvidia,tegra124-soctherm.yaml b/Documentation/devicetree/bindings/thermal/nvidia,tegra124-soctherm.yaml
>>> index 4677ad6645a5..37dac851f486 100644
>>> --- a/Documentation/devicetree/bindings/thermal/nvidia,tegra124-soctherm.yaml
>>> +++ b/Documentation/devicetree/bindings/thermal/nvidia,tegra124-soctherm.yaml
>>
>> File does not exist in next and no dependency is mentioned, so tricky to
>> review and figure out context. Without context the comment is:
> 
> Apologies, I have a conversion series for these thermal bindings. I'll
> send those out first.
> 
>>> @@ -120,6 +120,13 @@ properties:
>>>                # high (85%, TEGRA_SOCTHERM_THROT_LEVEL_HIGH)
>>>                - 3
>>>  
>>> +          temperature:
>>> +            $ref: /schemas/types.yaml#/definitions/int32
>>
>> Use -millicelsius suffix instead:
>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> Okay.
> 
>>> +            minimum: -273000
>>> +            maximum: 200000
>>> +            description: The temperature threshold (in millicelsius) that,
>>> +              when crossed, will trigger the configured automatic throttling.
>>
>> Don't you want some hysteresis? Or is it already using trips binding?
>> But in that case you should skip the $ref and maximum - they come from
>> thermal-zones, don't they?
> 
> We don't use a hysteresis at the moment, but checking the register
> documentation, there's indeed "up" and "down" thresholds, so we can add
> another property for that.
> 
> This doesn't use the trips binding and in fact, one of the reasons for
> this change is because we want to make this separate from trip points.
> Trip points are usually associated with cooling devices and this
> throttling mechanism doesn't really fit that concept because it is an
> automatic mechanism that is triggered when a given temperature threshold
> is crossed, rather than a manually activated mechanism, which is what a
> cooling device would be.

OK, I just wasn't sure if the binding already includes trips, which
would mean you should use existing 'temperature' property.

In such case, I think it's better to switch to property with common unit
- millicelsius, either low/high ranges or with hysteresis.

Best regards,
Krzysztof