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@xxxxxxxxxxxxxxxxxxxxx>
Cc: Klaus Goger <klaus.goger@xxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
Reviewed-by: Quentin Schulz <quentin.schulz@xxxxxxxxx>
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
