Message-ID: <e67ee8b92ff657a3cf4a1046450639e7@manjaro.org>
Date: Tue, 28 May 2024 17:41:17 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Heiko Stuebner <heiko@...ech.de>
Cc: Alexey Charkov <alchark@...il.com>, Quentin Schulz
 <quentin.schulz@...rry.de>, Rob Herring <robh+dt@...nel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
 <conor+dt@...nel.org>, Daniel Lezcano <daniel.lezcano@...aro.org>, Viresh
 Kumar <viresh.kumar@...aro.org>, Chen-Yu Tsai <wens@...nel.org>, Diederik de
 Haas <didi.debian@...ow.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 0/6] RK3588 and Rock 5B dts additions: thermal, OPP and
 fan

On 2024-05-28 17:16, Heiko Stuebner wrote:
> Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic:
>> On 2024-05-28 16:34, Heiko Stuebner wrote:
>> > Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic:
>> >> On 2024-05-28 11:49, Alexey Charkov wrote:
>> >> > On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@...il.com>
>> >> > wrote:
>> >> >>
>> >> >> This enables thermal monitoring and CPU DVFS on RK3588(s), as well as
>> >> >> active cooling on Radxa Rock 5B via the provided PWM fan.
>> >> >>
>> >> >> Some RK3588 boards use separate regulators to supply CPUs and their
>> >> >> respective memory interfaces, so this is handled by coupling those
>> >> >> regulators in affected boards' device trees to ensure that their
>> >> >> voltage is adjusted in step.
>> >> >>
>> >> >> This also enables the built-in thermal sensor (TSADC) for all boards
>> >> >> that don't currently have it enabled, using the default CRU based
>> >> >> emergency thermal reset. This default configuration only uses on-SoC
>> >> >> devices and doesn't rely on any external wiring, thus it should work
>> >> >> for all devices (tested only on Rock 5B though).
>> >> >>
>> >> >> The boards that have TSADC_SHUT signal wired to the PMIC reset line
>> >> >> can choose to override the default reset logic in favour of GPIO
>> >> >> driven (PMIC assisted) reset, but in my testing it didn't work on
>> >> >> Radxa Rock 5B - maybe I'm reading the schematic wrong and it doesn't
>> >> >> support PMIC assisted reset after all.
>> >> >>
>> >> >> Fan control on Rock 5B has been split into two intervals: let it spin
>> >> >> at the minimum cooling state between 55C and 65C, and then accelerate
>> >> >> if the system crosses the 65C mark - thanks to Dragan for suggesting.
>> >> >> This lets some cooling setups with beefier heatsinks and/or larger
>> >> >> fan fins to stay in the quietest non-zero fan state while still
>> >> >> gaining potential benefits from the airflow it generates, and
>> >> >> possibly avoiding noisy speeds altogether for some workloads.
>> >> >>
>> >> >> OPPs help actually scale CPU frequencies up and down for both cooling
>> >> >> and performance - tested on Rock 5B under varied loads. I've dropped
>> >> >> those OPPs that cause frequency reductions without accompanying
>> >> >> decrease
>> >> >> in CPU voltage, as they don't seem to be adding much benefit in day to
>> >> >> day use, while the kernel log gets a number of "OPP is inefficient"
>> >> >> lines.
>> >> >>
>> >> >> Note that this submission doesn't touch the SRAM read margin updates
>> >> >> or
>> >> >> the OPP calibration based on silicon quality which the downstream
>> >> >> driver
>> >> >> does and which were mentioned in [1]. It works as it is (also
>> >> >> confirmed by
>> >> >> Sebastian in his follow-up message [2]), and it is stable in my
>> >> >> testing on
>> >> >> Rock 5B, so it sounds better to merge a simple version first and then
>> >> >> extend when/if required.
>> >> >>
>> >> >> [1]
>> >> >> https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/
>> >> >> [2]
>> >> >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/
>> >> >>
>> >> >> Signed-off-by: Alexey Charkov <alchark@...il.com>
>> >> >> ---
>> >> >
>> >> > Hi Heiko,
>> >> >
>> >> > Do you think this can be merged for 6.11? Looks like there hasn't been
>> >> > any new feedback in a while, and it would be good to have frequency
>> >> > scaling in place for RK3588.
>> >> >
>> >> > Please let me know if you have any reservations or if we need any
>> >> > broader discussion.
>> >
>> > not really reservations, more like there was still discussion going on
>> > around the OPPs. Meanwhile we had more discussions regarding the whole
>> > speed binning Rockchip seems to do for rk3588 variants.
>> >
>> > And waiting for the testing Dragan wanted to do ;-) .
>> 
>> I'm sorry for the delays.
> 
> Was definitly _not_ meant as blame ;-) .

Thanks, but I do blame myself a bit for not doing it earlier. :)

> The series has just too many discussions threads to unravel on half
> an afternoon.

Yes, we've touched quite a few areas, for some of which I've already
started working on the associated patches.  That was one of the reasons
for the delays.

>> > So this should definitly make it into 6.11 though, as there is still
>> > a lot of time.
>> >
>> >> As I promised earlier, I was going to test this patch series in
>> >> detail.
>> >> Alas, I haven't managed to do that yet, :/ due to many reasons, but
>> >> I still remain firmly committed to doing that.
>> >>
>> >> Is -rc4 the cutoff for 6.11?  If so, there's still time and I'll do my
>> >> best to test and review these patches as soon as possible.
>> >
>> > As early as possible, the hard cutoff would be -rc6 though.
>> > I guess I'll just start picking the easy patches from the series.
>> 
>> I'll do my best to have the patches tested and reviewed in detail 
>> ASAP.
>> As a suggestion, perhaps it would be better to take the series as a
>> whole,
>> so we don't bring partial merging into the mix.
> 
> Patches need to work individually anyway (in correct order of course),
> so like starting at the top with the general thermal stuff should not
> create issues ;-)

Of course, but I might actually have some comments and suggestions
for some of the patches, and addressing those suggestions, if we end up
agreeing on them, would be a bit messy if some of the patches in the
series become merged first.

Powered by blists - more mailing lists