Re: [PATCH v4 0/6] RK3588 and Rock 5B dts additions: thermal, OPP and fan

From: Dragan Simic <dsimic@manjaro.org>
To: Heiko Stuebner <heiko@sntech.de>
Cc: Alexey Charkov <alchark@gmail.com>,
	Quentin Schulz <quentin.schulz@cherry.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Chen-Yu Tsai <wens@kernel.org>,
	Diederik de Haas <didi.debian@cknow.org>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 0/6] RK3588 and Rock 5B dts additions: thermal, OPP and fan
Date: Tue, 28 May 2024 17:01:48 +0200	[thread overview]
Message-ID: <8727e1c29bd6f562a7fc3de0ddac62cf@manjaro.org> (raw)
In-Reply-To: <5122636.irdbgypaU6@phil>

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@gmail.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@gmail.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.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel