Message-ID: <6230150.aeNJFYEL58@phil>
Date: Tue, 28 May 2024 17:16:20 +0200
From: Heiko Stuebner <heiko@...ech.de>
To: Dragan Simic <dsimic@...jaro.org>
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

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 ;-) .

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


> > 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 ;-)


Heiko

Powered by blists - more mailing lists