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

From: Thierry Reding <thierry.reding@gmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Amit Kucheria <amitk@kernel.org>, Zhang Rui <rui.zhang@intel.com>,
	Jon Hunter <jonathanh@nvidia.com>,
	linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-tegra@vger.kernel.org
Subject: Re: [PATCH 01/10] dt-bindings: thermal: tegra: Document throttle temperature
Date: Mon, 17 Apr 2023 10:59:32 +0200	[thread overview]
Message-ID: <ZD0KdAXyi0Ex1JOU@orome> (raw)
In-Reply-To: <187d51b3-6fec-7a25-e472-3d9020c12db5@linaro.org>

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

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.

Thierry

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