Received-SPF: pass (google.com: domain of linux-kernel+bounces-32480-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
Date: Mon, 22 Jan 2024 08:57:54 +0100
From: Dragan Simic <dsimic@manjaro.org>
To: Alexey Charkov <alchark@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>,
 Heiko Stuebner <heiko@sntech.de>, Sebastian Reichel
 <sebastian.reichel@collabora.com>, Cristian Ciocaltea
 <cristian.ciocaltea@collabora.com>, Christopher Obbard
 <chris.obbard@collabora.com>, =?UTF-8?Q?Tam=C3=A1s_Sz=C5=B1cs?=
 <szucst@iit.uni-miskolc.hu>, Shreeya Patel <shreeya.patel@collabora.com>,
 Kever Yang <kever.yang@rock-chips.com>, Chris Morgan
 <macromorgan@hotmail.com>, devicetree@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm64: dts: rockchip: enable built-in thermal monitoring
 on rk3588
In-Reply-To: <CABjd4YwxoPrKRPmjn-eWsU1N5fXCjAbnRwfKD9DLKgeL2=vHrA@mail.gmail.com>
References: <20240106222357.23835-1-alchark@gmail.com>
 <e0302da12345e5539583b2c96d747592@manjaro.org>
 <CABjd4Yw5wTLyK5OPw2S-ipPVCw7RTUeF2J0RgH-Vyis-ng8rTw@mail.gmail.com>
 <d7f2f25071a4d7c72bd286b11836dce7@manjaro.org>
 <CABjd4Yz11D8ThcT-oCWsQf9jL2idChFYSRYVVu3KNnzwoOwkKQ@mail.gmail.com>
 <f5c05015e042b11a51a9af26c35f18ed@manjaro.org>
 <CABjd4Yy91MAd2wALp4KQiEub9OyxU+MR+ti5KA_c+yvZT5xaqQ@mail.gmail.com>
 <81a5410c3dbedbd4fe9ce60ab236700c@manjaro.org>
 <CABjd4YwxoPrKRPmjn-eWsU1N5fXCjAbnRwfKD9DLKgeL2=vHrA@mail.gmail.com>
Message-ID: <a15edb98c32ca79b75bd4eaf64734561@manjaro.org>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit

On 2024-01-22 08:36, Alexey Charkov wrote:
> On Mon, Jan 22, 2024 at 10:22 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-01-22 07:03, Alexey Charkov wrote:
>> > On Mon, Jan 22, 2024 at 8:55 AM Dragan Simic <dsimic@manjaro.org> wrote:
>> >> On 2024-01-21 19:56, Alexey Charkov wrote:
>> >> > I think I'll also add a board-specific active cooling mechanism on the
>> >> > package level in the next iteration, given that Rock 5B has a PWM fan
>> >> > defined as a cooling device. That will go in the separate patch that
>> >> > updates rk3588-rock-5b.dts (your feedback to v2 of this patch is also
>> >> > duly noted, thank you!)
>> >>
>> >> Great, thanks.  Sure, making use of the Rock 5B's support for attaching
>> >> a PWM-controlled cooling fan is the way to go.
>> >>
>> >> Just to reiterate a bit, any "active" trip points belong to the board
>> >> dts file(s), because having a cooling fan is a board-specific feature.
>> >> As a note, you may also want to have a look at the RockPro64 dts(i)
>> >> files, for example;  the RockPro64 also comes with a cooling fan
>> >> connector and the associated PWM fan control logic.
>> >
>> > Thanks for the pointer! There is also a helpful doc within devicetree
>> > bindings descriptions, although it sits under hwmon which was a bit
>> > confusing to me. I've already tested it locally (by adding to the
>> > board dts), and it spins up and down quite nicely, and even modulates
>> > the fan speed swiftly when the load changes - yay!
>> 
>> Nice!  Also, isn't it like magic? :)  To me, turning LEDs on/off and
>> controlling fans acts as some kind of a "bridge" between the virtual
>> and the real world. :)
> 
> Oh yes! I also keep admiring how one can add just a couple of lines of
> text here and there that's not even real code, and the whole kernel
> machinery starts crunching numbers, analyzing temperatures, running
> PID loops, etc etc so that I could enjoy the satisfying whistle of a
> small fan when I type `make -j8` :-D

Yes, it's very satisfying, :) and it also demonstrates the true power
of the device trees as hardware definitions.  Just a few more lines and
the cooling works! :)

>> As a suggestion, it would be good to test with a couple of different
>> fans, to make sure that the PWM values work well for more that one fan
>> model.  The Rock 5B requires a 5 V fan, if I'm not mistaken?
> 
> It is 5V, yes. I only have one fan to try though, and I simply relied
> on the PWM values that are already defined in the upstream
> rk3588-rock-5b.dts. They don't look ideal for my particular fan,
> because the lowest non-zero cooling state currently uses a PWM value
> of 95, which doesn't always make it spin up. But in the end it doesn't
> seem to matter that much, because that tiny fan needs to spin at full
> 255 whenever all eight cores are loaded (and even then it can only
> balance the temperature at around 60.5С), and when the load is lighter
> (such as during various ./configure runs) it just switches off
> completely as the temperature goes down to 46C even with the fan not
> spinning.

I see, 5 V fans unfortunately aren't very common.  I'm not sure why
Radxa opted for 5 V there;  maybe the goal was to use Raspberry Pi 5 V
fans, but using those tiny fans doesn't make much sense, IMHO.

I think you can freely adjust the PWM values a bit to make your fan
start reliably at the lowest state, regardless of how rarely that state
will be used.  See, if your fan doesn't spin up reliably with the 
current
lowest state, chances for other fan models not to spin up are quite 
high.
IOW, it's better to play safe there, if you agree.

What kind of heatsink are you using with your Rock 5B?  Ah yes, and
what's the actual model of the fan you're using?

> I don't currently use the GPU/NPU/VPU though - maybe those would
> produce more moderate load which could benefit from spinning the fan
> at medium speeds.

Perhaps, but it will need to be tested at some point.  Have you tried
loading only one or two CPU cores?