Re: [PATCH v2 06/14] arm64: dts: rockchip: Remove #cooling-cells from fan on Theobroma boards

[Quentin Schulz <quentin.schulz@cherry.de>]

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