[sorry if you get this twice: I messed up and sent the previous one in HTML instead of plain text - resending now] On Tue, May 28, 2024 at 7:16 PM Heiko Stuebner <heiko@xxxxxxxxx> 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@xxxxxxxxx> > > >> > 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@xxxxxxxxxxxxxx/ > > >> >> [2] > > >> >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ > > >> >> > > >> >> Signed-off-by: Alexey Charkov <alchark@xxxxxxxxx> > > >> >> --- > > >> > > > >> > 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. FWIW, I think the latest exchange we had with Quentin regarding the OPPs concluded in “false alarm”, given that this version of the series only introduces a subset of them which should apply to all RK3588(s) Performance binning here is more geared towards how low the voltages can go for a given frequency, and right now we’re only introducing the highest-voltage setting for each OPP. Thus the binning and/or intermediate frequencies should be possible to introduce at a later stage in a backwards compatible way (if deemed relevant). > > > 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 ;-) Indeed, those are self-contained and can be merged independently of the OPPs. Having OPPs without thermal is more risky though, but that doesn’t sound like what we’re after here :) Best regards, Alexey
