| Message ID | 20220617071411.187542-1-francesco.dolcini@toradex.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | imx: thermal: Allow trip point configuration from DT | expand |
> Subject: [RESEND PATCH v2 0/9] imx: thermal: Allow trip point configuration > from DT > > This series allows to specify the imx thermal drivers trip point from the device > tree, without this change the threshold are hard-coded and this might not be > correct given the thermal design of the final system. > The threshold is set dynamically based on the part market temp grade. I am little confused why need to specify it in DT? I saw in 'PATCH 5/9', you provide a threshold table based temp grade, why not use the threshold from ' imx_init_temp_grade'? BR Jacky Bai > This change is backward compatible with the existing device tree, and even > with this change in by default the thresholds are the same as before. > > Toradex board are also updated to use a system-specific thresholds. > > Discussion on the current design is here: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke > rnel.org%2Fall%2F4ba1d7d2-3e8c-ba60-37fd-9598f415c076%40linaro.org%2 > F&data=05%7C01%7Cping.bai%40nxp.com%7C3bd9173c93184270acee > 08da5030fdfb%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6379 > 10468593014918%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi > LCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C& > amp;sdata=L0T4514vcK0Nl1Vv9EVNdhDJMpBGqIRP68GFKVeBvwg%3D& > ;reserved=0 > > One side note, after this change the dtbs checker starts complaining with this > message > > ``` > linux/arch/arm/boot/dts/imx6dl-alti6p.dtb: tempmon: > '#thermal-sensor-cells' does not match any of the regexes: > '^(automotive|commercial|extended-commercial|industrial)-thermal$', > 'pinctrl-[0-9]+' > From schema: > linux/Documentation/devicetree/bindings/thermal/imx-thermal.yaml > ``` > > to my understanding this is just a side effect, '#thermal-sensor-cells' is not > changed in any way by this series. I can fix that, I wonder if I should remove > the property from the imx dtsi files or add it to the binding yaml definition, not > sure about it. > Anybody can advise? > > Changes in v2: > - fix build error without CONFIG_THERMAL_OF > - more verbose error reporting in case the dts is not correct > - additional comment on the threshold fixup in case the passive threshold is > higher than critical > - while parsing the dts thermal, return immediately if the node is not there > > > Francesco Dolcini (9): > dt-bindings: thermal: Define trips node in $defs > thermal: thermal: Export OF trip helper function > dt-bindings: thermal: imx: Add trips point > imx: thermal: Configure trip point from DT > ARM: dts: imx[67]: Add trips points > ARM: dts: imx6qdl-apalis: Set CPU critical trip point > ARM: dts: imx7-colibri: Set CPU critical trip point > ARM: dts: imx6ull-colibri: Set CPU critical trip point > ARM: dts: imx6qdl-colibri: Set CPU critical trip point > > .../bindings/thermal/imx-thermal.yaml | 27 ++++ > .../bindings/thermal/thermal-zones.yaml | 130 +++++++++--------- > arch/arm/boot/dts/imx-thermal.dtsi | 61 ++++++++ > arch/arm/boot/dts/imx6qdl-apalis.dtsi | 12 ++ > arch/arm/boot/dts/imx6qdl-colibri.dtsi | 12 ++ > arch/arm/boot/dts/imx6qdl.dtsi | 2 + > arch/arm/boot/dts/imx6sl.dtsi | 2 + > arch/arm/boot/dts/imx6sll.dtsi | 2 + > arch/arm/boot/dts/imx6sx.dtsi | 2 + > arch/arm/boot/dts/imx6ul.dtsi | 2 + > arch/arm/boot/dts/imx6ull-colibri.dtsi | 12 ++ > arch/arm/boot/dts/imx7-colibri.dtsi | 12 ++ > arch/arm/boot/dts/imx7s.dtsi | 2 + > drivers/thermal/imx_thermal.c | 58 ++++++++ > drivers/thermal/thermal_core.h | 7 + > drivers/thermal/thermal_of.c | 5 +- > 16 files changed, 283 insertions(+), 65 deletions(-) create mode 100644 > arch/arm/boot/dts/imx-thermal.dtsi > > -- > 2.25.1
Hello Jacky, On Fri, Jun 17, 2022 at 07:31:25AM +0000, Jacky Bai wrote: > > Subject: [RESEND PATCH v2 0/9] imx: thermal: Allow trip point configuration > > from DT > > > > This series allows to specify the imx thermal drivers trip point from the device > > tree, without this change the threshold are hard-coded and this might not be > > correct given the thermal design of the final system. > > > > The threshold is set dynamically based on the part market temp grade. > I am little confused why need to specify it in DT? I saw in 'PATCH > 5/9', you provide a threshold table based temp grade, why not use the > threshold from ' imx_init_temp_grade'? The problem with the existing temperature thresholds is that they are hard-coded into the driver, there is no way to change those to match the actual final system thermal design. After various discussions [1][2] with Marco, Daniel and Lucas it was agreed that the actual trip is indeed a system property and therefore should be described in the device tree. This series make it possible in a backward compatible way, this new possibility is than used to override the threshold in some system-specific dts (see patches 6,7,8 and 9). Francesco [1] https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/ [2] https://lore.kernel.org/all/20220516190001.147919-1-francesco.dolcini@toradex.com/
Hi Francesco, > Subject: Re: [RESEND PATCH v2 0/9] imx: thermal: Allow trip point > configuration from DT > > Hello Jacky, > > On Fri, Jun 17, 2022 at 07:31:25AM +0000, Jacky Bai wrote: > > > Subject: [RESEND PATCH v2 0/9] imx: thermal: Allow trip point > > > configuration from DT > > > > > > This series allows to specify the imx thermal drivers trip point > > > from the device tree, without this change the threshold are > > > hard-coded and this might not be correct given the thermal design of the > final system. > > > > > > > The threshold is set dynamically based on the part market temp grade. > > I am little confused why need to specify it in DT? I saw in 'PATCH > > 5/9', you provide a threshold table based temp grade, why not use the > > threshold from ' imx_init_temp_grade'? > > The problem with the existing temperature thresholds is that they are > hard-coded into the driver, there is no way to change those to match the > actual final system thermal design. > > After various discussions [1][2] with Marco, Daniel and Lucas it was agreed > that the actual trip is indeed a system property and therefore should be > described in the device tree. This series make it possible in a backward > compatible way, this new possibility is than used to override the threshold in > some system-specific dts (see patches 6,7,8 and 9). > Thx. Yes, in current driver design, the critical trip is fixed at 5 Celsius lower than the MAX limited. So the purpose of this patch is just want to override this limitation to adjust it based on system need? BR Jacky Bai > Francesco >
On Fri, Jun 17, 2022 at 07:51:53AM +0000, Jacky Bai wrote: > > On Fri, Jun 17, 2022 at 07:31:25AM +0000, Jacky Bai wrote: > > > > Subject: [RESEND PATCH v2 0/9] imx: thermal: Allow trip point > > > > configuration from DT > > > > > > > > This series allows to specify the imx thermal drivers trip point > > > > from the device tree, without this change the threshold are > > > > hard-coded and this might not be correct given the thermal design of the > > final system. > > > > > > > > > > The threshold is set dynamically based on the part market temp grade. > > > I am little confused why need to specify it in DT? I saw in 'PATCH > > > 5/9', you provide a threshold table based temp grade, why not use the > > > threshold from ' imx_init_temp_grade'? > > > > The problem with the existing temperature thresholds is that they are > > hard-coded into the driver, there is no way to change those to match the > > actual final system thermal design. > > > > After various discussions [1][2] with Marco, Daniel and Lucas it was agreed > > that the actual trip is indeed a system property and therefore should be > > described in the device tree. This series make it possible in a backward > > compatible way, this new possibility is than used to override the threshold in > > some system-specific dts (see patches 6,7,8 and 9). > > > > Thx. Yes, in current driver design, the critical trip is fixed at 5 Celsius lower than the MAX limited. > So the purpose of this patch is just want to override this limitation to adjust it based on system need? Correct (including the passive trip, for completeness). Francesco
Hi Francesco, On 17. 06. 22 9:55, Francesco Dolcini wrote: > On Fri, Jun 17, 2022 at 07:51:53AM +0000, Jacky Bai wrote: >>> On Fri, Jun 17, 2022 at 07:31:25AM +0000, Jacky Bai wrote: >>>>> Subject: [RESEND PATCH v2 0/9] imx: thermal: Allow trip point >>>>> configuration from DT >>>>> >>>>> This series allows to specify the imx thermal drivers trip point >>>>> from the device tree, without this change the threshold are >>>>> hard-coded and this might not be correct given the thermal design of the >>> final system. >>>>> >>>> >>>> The threshold is set dynamically based on the part market temp grade. >>>> I am little confused why need to specify it in DT? I saw in 'PATCH >>>> 5/9', you provide a threshold table based temp grade, why not use the >>>> threshold from ' imx_init_temp_grade'? >>> >>> The problem with the existing temperature thresholds is that they are >>> hard-coded into the driver, there is no way to change those to match the >>> actual final system thermal design. AFAIK you can change the trip point from user space if you build with THERMAL_WRITABLE_TRIPS so in fact you can adapt it to the final system design. We did exactly that when we swapped imx6 dual for imx6 quad but did not have enough space for adequate heat sink. I do not want to question usefulness of this series though, I agree with the idea. Michal
Hello Michal, On Fri, Jun 17, 2022 at 10:36:17AM +0200, Michal Vokáč wrote: > On 17. 06. 22 9:55, Francesco Dolcini wrote: > > On Fri, Jun 17, 2022 at 07:51:53AM +0000, Jacky Bai wrote: > > > > On Fri, Jun 17, 2022 at 07:31:25AM +0000, Jacky Bai wrote: > > > > > > Subject: [RESEND PATCH v2 0/9] imx: thermal: Allow trip point > > > > > > configuration from DT > > > > > > > > > > > > This series allows to specify the imx thermal drivers trip point > > > > > > from the device tree, without this change the threshold are > > > > > > hard-coded and this might not be correct given the thermal design of the > > > > final system. > > > > > > > > > > > > > > > > The threshold is set dynamically based on the part market temp grade. > > > > > I am little confused why need to specify it in DT? I saw in 'PATCH > > > > > 5/9', you provide a threshold table based temp grade, why not use the > > > > > threshold from ' imx_init_temp_grade'? > > > > > > > > The problem with the existing temperature thresholds is that they are > > > > hard-coded into the driver, there is no way to change those to match the > > > > actual final system thermal design. > > AFAIK you can change the trip point from user space if you build > with THERMAL_WRITABLE_TRIPS so in fact you can adapt it to the final > system design. > > We did exactly that when we swapped imx6 dual for imx6 quad but did not > have enough space for adequate heat sink. When I investigated this only the passive threshold was writeable from user-space/sysfs, and Daniel was against any patch to allow changing also the critical one [0]. Francesco [0] https://lore.kernel.org/all/4de41b5e-1fa6-ece4-9d9a-2656d399b452@linaro.org/
On 17/06/2022 00:14, Francesco Dolcini wrote: > This series allows to specify the imx thermal drivers trip point from the device tree, > without this change the threshold are hard-coded and this might not be correct given the > thermal design of the final system. Why resending? What was wrong with your v2 to which I replied? Best regards, Krzysztof
On 17/06/2022 00:42, Francesco Dolcini wrote: > Hello Jacky, > > On Fri, Jun 17, 2022 at 07:31:25AM +0000, Jacky Bai wrote: >>> Subject: [RESEND PATCH v2 0/9] imx: thermal: Allow trip point configuration >>> from DT >>> >>> This series allows to specify the imx thermal drivers trip point from the device >>> tree, without this change the threshold are hard-coded and this might not be >>> correct given the thermal design of the final system. >>> >> >> The threshold is set dynamically based on the part market temp grade. >> I am little confused why need to specify it in DT? I saw in 'PATCH >> 5/9', you provide a threshold table based temp grade, why not use the >> threshold from ' imx_init_temp_grade'? > > The problem with the existing temperature thresholds is that they are > hard-coded into the driver, there is no way to change those to match the > actual final system thermal design. > > After various discussions [1][2] with Marco, Daniel and Lucas it was agreed > that the actual trip is indeed a system property and therefore should be > described in the device tree. This series make it possible in a > backward compatible way, this new possibility is than used to override the > threshold in some system-specific dts (see patches 6,7,8 and 9). > > Francesco > > [1] https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/ > [2] https://lore.kernel.org/all/20220516190001.147919-1-francesco.dolcini@toradex.com/ > As I responded to your v2, thermal is not really special and other types of devices and other SoCs might be affected as well. Best regards, Krzysztof
On Fri, Jun 17, 2022 at 06:06:40PM -0700, Krzysztof Kozlowski wrote: > On 17/06/2022 00:14, Francesco Dolcini wrote: > > This series allows to specify the imx thermal drivers trip point from the device tree, > > without this change the threshold are hard-coded and this might not be correct given the > > thermal design of the final system. > > Why resending? What was wrong with your v2 to which I replied? Wrong subject in the cover letter, I had v1 instead of v2. I thought this was going to confuse people, therefore I resent. My mistake to not have explicitly written the reason of the resend. Francesco