| Message ID | 20230414125721.1043589-2-thierry.reding@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | thermal: tegra: Do not register cooling device | expand |
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: > @@ -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 > + 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? Best regards, Krzysztof
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
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
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 @@ -120,6 +120,13 @@ properties: # high (85%, TEGRA_SOCTHERM_THROT_LEVEL_HIGH) - 3 + temperature: + $ref: /schemas/types.yaml#/definitions/int32 + minimum: -273000 + maximum: 200000 + description: The temperature threshold (in millicelsius) that, + when crossed, will trigger the configured automatic throttling. + # optional # Tegra210 specific and valid only for OCx throttle events nvidia,count-threshold: