Message ID | 20241008203940.2573684-7-heiko@sntech.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Fixing some dtbscheck warnings | expand |
Hello Heiko, On 2024-10-08 22:39, Heiko Stuebner wrote: > All Theobroma boards use a ti,amc6821 as fan controller. > It normally runs in an automatically controlled way and while it may be > possible to use it as part of a dt-based thermal management, this is > not yet specified in the binding, nor implemented in any kernel. > > Newer boards already don't contain that #cooling-cells property, but > older ones do. So remove them for now, they can be re-added if thermal > integration gets implemented in the future. > > Fixes: c484cf93f61b ("arm64: dts: rockchip: add PX30-µQ7 (Ringneck) > SoM with Haikou baseboard") > Fixes: d99a02bcfa81 ("arm64: dts: rockchip: add RK3368-uQ7 (Lion) SoM") > Fixes: 2c66fc34e945 ("arm64: dts: rockchip: add RK3399-Q7 (Puma) SoM") > Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com> > Cc: Klaus Goger <klaus.goger@theobroma-systems.com> > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> Looking good to me, thanks for the patch. In addition to the amc6821 driver currently not supporting full integration into the thermal framework, the "fan" DT node also isn't referenced in any cooling map, so having it define the "cooling-cells" property is of no use. By the way, it would be nice to see the amc6821 driver supporting fan speed regulation, and test it to check who does a better job when it comes to cooling and fan speed regulation, the thermal framework or the chip's built-in logic. :) Reviewed-by: Dragan Simic <dsimic@manjaro.org> > --- > arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi | 1 - > arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi | 1 - > arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi | 1 - > 3 files changed, 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi > b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi > index b7163ed74232..a6ea9d8c785c 100644 > --- a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi > +++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi > @@ -293,7 +293,6 @@ &i2c1 { > fan: fan@18 { > compatible = "ti,amc6821"; > reg = <0x18>; > - #cooling-cells = <2>; > }; > > rtc_twi: rtc@6f { > diff --git a/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi > b/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi > index 8ac8acf4082d..ab3fda69a1fb 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi > @@ -61,7 +61,6 @@ i2c_lvds_blc: i2c@0 { > fan: fan@18 { > compatible = "ti,amc6821"; > reg = <0x18>; > - #cooling-cells = <2>; > }; > > rtc_twi: rtc@6f { > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi > b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi > index 650b1ba9c192..6de3128556b7 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi > @@ -395,7 +395,6 @@ &i2c7 { > fan: fan@18 { > compatible = "ti,amc6821"; > reg = <0x18>; > - #cooling-cells = <2>; > }; > > rtc_twi: rtc@6f {
Hi Dragan, On 10/9/24 9:16 AM, Dragan Simic wrote: > Hello Heiko, > > On 2024-10-08 22:39, Heiko Stuebner wrote: >> All Theobroma boards use a ti,amc6821 as fan controller. >> It normally runs in an automatically controlled way and while it may be >> possible to use it as part of a dt-based thermal management, this is >> not yet specified in the binding, nor implemented in any kernel. >> >> Newer boards already don't contain that #cooling-cells property, but >> older ones do. So remove them for now, they can be re-added if thermal >> integration gets implemented in the future. >> >> Fixes: c484cf93f61b ("arm64: dts: rockchip: add PX30-µQ7 (Ringneck) >> SoM with Haikou baseboard") >> Fixes: d99a02bcfa81 ("arm64: dts: rockchip: add RK3368-uQ7 (Lion) SoM") >> Fixes: 2c66fc34e945 ("arm64: dts: rockchip: add RK3399-Q7 (Puma) SoM") >> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> Cc: Klaus Goger <klaus.goger@theobroma-systems.com> >> Signed-off-by: Heiko Stuebner <heiko@sntech.de> >> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> > > Looking good to me, thanks for the patch. In addition to the amc6821 > driver currently not supporting full integration into the thermal > framework, the "fan" DT node also isn't referenced in any cooling map, > so having it define the "cooling-cells" property is of no use. > > By the way, it would be nice to see the amc6821 driver supporting fan > speed regulation, and test it to check who does a better job when it > comes to cooling and fan speed regulation, the thermal framework or > the chip's built-in logic. :) > Wasn't this feature added this summer by Guenter? c.f. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/hwmon/amc6821.c?id=becbd16ed2f8f427239ffda66b5d894008bc56af Mode 4 is https://elixir.bootlin.com/linux/v6.11.3/source/drivers/hwmon/amc6821.c#L367 ([FDRC1:FDRC0] = [01] -> Software-RPM Control Mode (Fan Speed Regulator) according to the datasheet). In any case, we cannot compare those for our products as we do not have a genuine AMC6821 but a handmade simulation of the IP we run in an MCU. But that'd be an interesting data point indeed :) Cheers, Quentin
Hello Quentin, On 2024-10-14 17:39, Quentin Schulz wrote: > On 10/9/24 9:16 AM, Dragan Simic wrote: >> On 2024-10-08 22:39, Heiko Stuebner wrote: >>> All Theobroma boards use a ti,amc6821 as fan controller. >>> It normally runs in an automatically controlled way and while it may >>> be >>> possible to use it as part of a dt-based thermal management, this is >>> not yet specified in the binding, nor implemented in any kernel. >>> >>> Newer boards already don't contain that #cooling-cells property, but >>> older ones do. So remove them for now, they can be re-added if >>> thermal >>> integration gets implemented in the future. >>> >>> Fixes: c484cf93f61b ("arm64: dts: rockchip: add PX30-µQ7 (Ringneck) >>> SoM with Haikou baseboard") >>> Fixes: d99a02bcfa81 ("arm64: dts: rockchip: add RK3368-uQ7 (Lion) >>> SoM") >>> Fixes: 2c66fc34e945 ("arm64: dts: rockchip: add RK3399-Q7 (Puma) >>> SoM") >>> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>> Cc: Klaus Goger <klaus.goger@theobroma-systems.com> >>> Signed-off-by: Heiko Stuebner <heiko@sntech.de> >>> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> >> >> Looking good to me, thanks for the patch. In addition to the amc6821 >> driver currently not supporting full integration into the thermal >> framework, the "fan" DT node also isn't referenced in any cooling map, >> so having it define the "cooling-cells" property is of no use. >> >> By the way, it would be nice to see the amc6821 driver supporting fan >> speed regulation, and test it to check who does a better job when it >> comes to cooling and fan speed regulation, the thermal framework or >> the chip's built-in logic. :) > > Wasn't this feature added this summer by Guenter? > > c.f. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/hwmon/amc6821.c?id=becbd16ed2f8f427239ffda66b5d894008bc56af > > Mode 4 is > https://elixir.bootlin.com/linux/v6.11.3/source/drivers/hwmon/amc6821.c#L367 > ([FDRC1:FDRC0] = [01] -> Software-RPM Control Mode (Fan Speed > Regulator) according to the datasheet). Ah, SENSOR_DEVICE_ATTR_RW(fan1_target, fan, IDX_FAN1_TARGET)... How did I miss that? Hmm... Maybe I was looking at some older local branch, which happened not to include that commit. Anywyay, good to know, thanks. > In any case, we cannot compare those for our products as we do not > have a genuine AMC6821 but a handmade simulation of the IP we run in > an MCU. I seem to remember your MCU that performs a few tasks, back from some related discussions. I wonder what was the reason to implement it in software, instead of using actual fan controller chip? > But that'd be an interesting data point indeed :) I'm glad that you agree. :)
Hi Dragan, On 10/14/24 5:49 PM, Dragan Simic wrote: > Hello Quentin, > > On 2024-10-14 17:39, Quentin Schulz wrote: >> On 10/9/24 9:16 AM, Dragan Simic wrote: >>> On 2024-10-08 22:39, Heiko Stuebner wrote: >>>> All Theobroma boards use a ti,amc6821 as fan controller. >>>> It normally runs in an automatically controlled way and while it may be >>>> possible to use it as part of a dt-based thermal management, this is >>>> not yet specified in the binding, nor implemented in any kernel. >>>> >>>> Newer boards already don't contain that #cooling-cells property, but >>>> older ones do. So remove them for now, they can be re-added if thermal >>>> integration gets implemented in the future. >>>> >>>> Fixes: c484cf93f61b ("arm64: dts: rockchip: add PX30-µQ7 (Ringneck) >>>> SoM with Haikou baseboard") >>>> Fixes: d99a02bcfa81 ("arm64: dts: rockchip: add RK3368-uQ7 (Lion) SoM") >>>> Fixes: 2c66fc34e945 ("arm64: dts: rockchip: add RK3399-Q7 (Puma) SoM") >>>> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>>> Cc: Klaus Goger <klaus.goger@theobroma-systems.com> >>>> Signed-off-by: Heiko Stuebner <heiko@sntech.de> >>>> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> >>> >>> Looking good to me, thanks for the patch. In addition to the amc6821 >>> driver currently not supporting full integration into the thermal >>> framework, the "fan" DT node also isn't referenced in any cooling map, >>> so having it define the "cooling-cells" property is of no use. >>> >>> By the way, it would be nice to see the amc6821 driver supporting fan >>> speed regulation, and test it to check who does a better job when it >>> comes to cooling and fan speed regulation, the thermal framework or >>> the chip's built-in logic. :) >> >> Wasn't this feature added this summer by Guenter? >> >> c.f. https://eur02.safelinks.protection.outlook.com/? >> url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2Fdrivers%2Fhwmon%2Famc6821.c%3Fid%3Dbecbd16ed2f8f427239ffda66b5d894008bc56af&data=05%7C02%7Cquentin.schulz%40cherry.de%7C6df77e4e73434d36a6fd08dcec67c21c%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638645177611948235%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=4VaZrAKxDUTdEf7avUM1ewHLl9PIgBple841dE55o4w%3D&reserved=0 >> >> Mode 4 is >> https://eur02.safelinks.protection.outlook.com/? >> url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.11.3%2Fsource%2Fdrivers%2Fhwmon%2Famc6821.c%23L367&data=05%7C02%7Cquentin.schulz%40cherry.de%7C6df77e4e73434d36a6fd08dcec67c21c%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638645177611979168%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=uNnWR0Oux0BlNhpe20Xj4%2FEtGQJv%2FsU1hapm4fGn7Qk%3D&reserved=0 >> ([FDRC1:FDRC0] = [01] -> Software-RPM Control Mode (Fan Speed >> Regulator) according to the datasheet). > > Ah, SENSOR_DEVICE_ATTR_RW(fan1_target, fan, IDX_FAN1_TARGET)... > How did I miss that? Hmm... Maybe I was looking at some older > local branch, which happened not to include that commit. > > Anywyay, good to know, thanks. > >> In any case, we cannot compare those for our products as we do not >> have a genuine AMC6821 but a handmade simulation of the IP we run in >> an MCU. > > I seem to remember your MCU that performs a few tasks, back from > some related discussions. I wonder what was the reason to implement > it in software, instead of using actual fan controller chip? > This predates my joining the company, so... I don't know. What I can say is, we have the following emulated in the MCU: - custom CAN over USB (UCAN; upstreamed already) - ISL1208 RTC - AMC6821 FAN controller - custom PWM controller (upstreaming pending) - a few bytes of NVRAM (AT24-based; upstreaming pending) - uncontrollable (from SoC PoV) watchdog, allows another MCU/system to trigger a full system reset - possibly, custom HW watchdog controllable over I2C (required to fix a very odd corner case in HW on PX30 Ringneck) Possibly more if we have the need for it and it fits into the MCU flash :) I assume this was born out of necessity to add support for CAN on RK3399 Puma since there's no CAN controller inside the SoC? I also think ISL1208 and AMC6821 aren't that easy to source anymore (RK3399 Puma has that MCU and its support started in ~2018 I seem to recall?). Considering the quantities and prices we get for the two MCUs flavors we have and how space constrained we are on some products, especially the uQ7 (PX30 Ringneck), it was probably I wise decision. The second MCU flavor came because STM32 was impossible to source at reasonable prices during the shortage 2-4 years ago. This also means we can expand the set of features over time (which we are for example, with the custom PWM controller, NVRAM and I2C watchdog) since the MCU can be flashed once in the field too. Obviously, you replace component cost and footprint with MCU FW development, so it's not necessarily cost-efficient but I'm not the one running the numbers so wouldn't be able to tell you ;) Cheers, Quentin
Hello Quentin, On 2024-10-14 18:29, Quentin Schulz wrote: > On 10/14/24 5:49 PM, Dragan Simic wrote: >> On 2024-10-14 17:39, Quentin Schulz wrote: >>> On 10/9/24 9:16 AM, Dragan Simic wrote: >>>> On 2024-10-08 22:39, Heiko Stuebner wrote: >>>>> All Theobroma boards use a ti,amc6821 as fan controller. >>>>> It normally runs in an automatically controlled way and while it >>>>> may be >>>>> possible to use it as part of a dt-based thermal management, this >>>>> is >>>>> not yet specified in the binding, nor implemented in any kernel. >>>>> >>>>> Newer boards already don't contain that #cooling-cells property, >>>>> but >>>>> older ones do. So remove them for now, they can be re-added if >>>>> thermal >>>>> integration gets implemented in the future. >>>>> >>>>> Fixes: c484cf93f61b ("arm64: dts: rockchip: add PX30-µQ7 (Ringneck) >>>>> SoM with Haikou baseboard") >>>>> Fixes: d99a02bcfa81 ("arm64: dts: rockchip: add RK3368-uQ7 (Lion) >>>>> SoM") >>>>> Fixes: 2c66fc34e945 ("arm64: dts: rockchip: add RK3399-Q7 (Puma) >>>>> SoM") >>>>> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>>>> Cc: Klaus Goger <klaus.goger@theobroma-systems.com> >>>>> Signed-off-by: Heiko Stuebner <heiko@sntech.de> >>>>> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> >>>> >>>> Looking good to me, thanks for the patch. In addition to the >>>> amc6821 >>>> driver currently not supporting full integration into the thermal >>>> framework, the "fan" DT node also isn't referenced in any cooling >>>> map, >>>> so having it define the "cooling-cells" property is of no use. >>>> >>>> By the way, it would be nice to see the amc6821 driver supporting >>>> fan >>>> speed regulation, and test it to check who does a better job when it >>>> comes to cooling and fan speed regulation, the thermal framework or >>>> the chip's built-in logic. :) >>> >>> Wasn't this feature added this summer by Guenter? >>> >>> c.f. https://eur02.safelinks.protection.outlook.com/? >>> url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2Fdrivers%2Fhwmon%2Famc6821.c%3Fid%3Dbecbd16ed2f8f427239ffda66b5d894008bc56af&data=05%7C02%7Cquentin.schulz%40cherry.de%7C6df77e4e73434d36a6fd08dcec67c21c%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638645177611948235%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=4VaZrAKxDUTdEf7avUM1ewHLl9PIgBple841dE55o4w%3D&reserved=0 >>> >>> Mode 4 is >>> https://eur02.safelinks.protection.outlook.com/? >>> url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.11.3%2Fsource%2Fdrivers%2Fhwmon%2Famc6821.c%23L367&data=05%7C02%7Cquentin.schulz%40cherry.de%7C6df77e4e73434d36a6fd08dcec67c21c%7C5e0e1b5221b54e7b83bb514ec460677e%7C0%7C0%7C638645177611979168%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=uNnWR0Oux0BlNhpe20Xj4%2FEtGQJv%2FsU1hapm4fGn7Qk%3D&reserved=0 >>> ([FDRC1:FDRC0] = [01] -> Software-RPM Control Mode (Fan Speed >>> Regulator) according to the datasheet). >> >> Ah, SENSOR_DEVICE_ATTR_RW(fan1_target, fan, IDX_FAN1_TARGET)... >> How did I miss that? Hmm... Maybe I was looking at some older >> local branch, which happened not to include that commit. >> >> Anywyay, good to know, thanks. >> >>> In any case, we cannot compare those for our products as we do not >>> have a genuine AMC6821 but a handmade simulation of the IP we run in >>> an MCU. >> >> I seem to remember your MCU that performs a few tasks, back from >> some related discussions. I wonder what was the reason to implement >> it in software, instead of using actual fan controller chip? > > This predates my joining the company, so... I don't know. > > What I can say is, we have the following emulated in the MCU: > - custom CAN over USB (UCAN; upstreamed already) > - ISL1208 RTC > - AMC6821 FAN controller > - custom PWM controller (upstreaming pending) > - a few bytes of NVRAM (AT24-based; upstreaming pending) > - uncontrollable (from SoC PoV) watchdog, allows another MCU/system to > trigger a full system reset > - possibly, custom HW watchdog controllable over I2C (required to fix > a very odd corner case in HW on PX30 Ringneck) Nice, that's quite a lot of emulated stuff. > Possibly more if we have the need for it and it fits into the MCU flash > :) That's one of the benefits that come with an approach like this. It's like some kind of PaaS (or whatever is the "cool" thing these days) for hardware design. :) > I assume this was born out of necessity to add support for CAN on > RK3399 Puma since there's no CAN controller inside the SoC? Could be, and the additional functionality, also required for the board, was then just "offloaded" to the same MCU. > I also think ISL1208 and AMC6821 aren't that easy to source anymore > (RK3399 Puma has that MCU and its support started in ~2018 I seem to > recall?). Considering the quantities and prices we get for the two > MCUs flavors we have and how space constrained we are on some > products, especially the uQ7 (PX30 Ringneck), it was probably I wise > decision. The second MCU flavor came because STM32 was impossible to > source at reasonable prices during the shortage 2-4 years ago. Makes sense. Instead of two or more separate additional chips, whose availability can change at virtually any point, you now depend on a single additional chip, which is also, presumably, more widely used, so should be easier to source. > This also means we can expand the set of features over time (which we > are for example, with the custom PWM controller, NVRAM and I2C > watchdog) since the MCU can be flashed once in the field too. Yup, just like PaaS, SaaS or whatever. :) > Obviously, you replace component cost and footprint with MCU FW > development, so it's not necessarily cost-efficient but I'm not the > one running the numbers so wouldn't be able to tell you ;) Also good point. Additional standalone chips are sometimes less expensive than the equivalent manpower. :)
diff --git a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi index b7163ed74232..a6ea9d8c785c 100644 --- a/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi +++ b/arch/arm64/boot/dts/rockchip/px30-ringneck.dtsi @@ -293,7 +293,6 @@ &i2c1 { fan: fan@18 { compatible = "ti,amc6821"; reg = <0x18>; - #cooling-cells = <2>; }; rtc_twi: rtc@6f { diff --git a/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi b/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi index 8ac8acf4082d..ab3fda69a1fb 100644 --- a/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi @@ -61,7 +61,6 @@ i2c_lvds_blc: i2c@0 { fan: fan@18 { compatible = "ti,amc6821"; reg = <0x18>; - #cooling-cells = <2>; }; rtc_twi: rtc@6f { diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi index 650b1ba9c192..6de3128556b7 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi @@ -395,7 +395,6 @@ &i2c7 { fan: fan@18 { compatible = "ti,amc6821"; reg = <0x18>; - #cooling-cells = <2>; }; rtc_twi: rtc@6f {