Re: [PATCH v2] arm64: dts: rockchip: Add GPU OPP voltage ranges to RK356x SoC dtsi

["Heiko Stübner" <heiko@sntech.de>]

Am Sonntag, 30. Juni 2024, 00:01:41 CEST schrieb Diederik de Haas:
> On Saturday, 29 June 2024 18:39:02 CEST Dragan Simic wrote:
> > Add support for voltage ranges to the GPU OPPs defined in the SoC dtsi for
> > RK356x.  These voltage ranges are useful for RK356x-based boards that are
> > designed to use the same power supply for the GPU and NPU portions of the
> > SoC, which is described further in the following documents:
> > 
> >   - Rockchip RK3566 Hardware Design Guide, version 1.1.0, page 37
> >   - Rockchip RK3568 Hardware Design Guide, version 1.2, page 78
> 
> That was interesting to read, thanks.
> Now I understand the difference between rk809(-5) and rk817(-5).
> 
> But AFAIUI the above description described why there were separate tables for 
> rk809 and rk817 in v1. But that was dropped in v2. So it seems to me the 
> (commit) message should be updated accordingly?
> 
> I also expected that (for v1) there would be a similar construct as was 
> recently added for rk3588. But I should interpret Heiko's comments as that 
> strategy should not be applied to rk356x?

The issue I had was more about the #ifdef'ery and then having a board define
a constant to enable one or the other.

As far as I understood the description, the OPP itself is the same in
terms of frequency and voltage, just the regulator can't fully realize
that target voltage, so the solution is to allow a voltage range, to
also support the less-exact regulator.

On the rk3588 on the other hand the soc variants have different OPP
tables themselfs, because the soc itself only supports different
frequencies+voltages. So the solution here is the split of the OPPs so
that we don't mess around with /delete-node/ edits of one OPP table.

So TL;DR separate OPP tables are the way to go if the user needs different
freq+voltage values and voltage ranges allows boards to use less-adapted
regulators.


> > The values for the exact GPU OPP voltages and the lower limits for the GPU
> > OPP voltage ranges differ from the values found in the vendor kernel source
> > (cf. downstream commit f8b9431ee38e ("arm64: dts: rockchip: rk3568: support
> > adjust opp-table by otp")). [1][2]  
> 
> Why? In their latest update Rockchip changed it to the values as specified in 
> the links. My assumption is that based on extensive testing they did and/or 
> the feedback they got from the client/customers, they felt the need to change 
> it to the values they did.
> 
> I think we should follow their values unless we have an explicit and very good 
> reason to deviate from that.

Correct.
Values from some "random" Radxa kernel would also not be my
selection of choice.

In the mainline-kernel we always want the save choice - which in for me
is Rockchip's. If people want to experiment with other values on their own
boards to sort of overclock their chips, that's their prerogative.


Heiko


> > However, our values have served us well so far, so let's keep them for now,
> 
> And I don't think that qualifies as a (very) good reason.
> I think it's reasonable to assume that far more (stress) testing has been done 
> with the downstream code, then has happened with the upstream code.
> Hopefully that'll change in the future, but I don't think we're there yet.
> 
> When we/upstream adds npu support, I think we should also follow downstream's 
> OPP values, unless we have a very good reason to deviate from that.
> 
> > until we actually start supporting the CPU and GPU binning, together with
> > the related voltage adjustments.
> 
> I may not fully understand what you mean by that, but I think it's (again) 
> reasonable to assume that Rockchip has far more insight into this then we do.
> 
> Cheers,
>   Diederik
> 
> > [1]
> > https://github.com/rockchip-linux/kernel/commit/f8b9431ee38ed561650be7092ab
> > 93f564598daa9 [2]
> > https://raw.githubusercontent.com/rockchip-linux/kernel/f8b9431ee38ed561650
> > be7092ab93f564598daa9/arch/arm64/boot/dts/rockchip/rk3568.dtsi
>