Re: [PATCH] arm64: dts: rockchip: Fix the DCDC_REG2 minimum voltage on Quartz64 Model B

[Heiko Stübner]

Am Freitag, 31. Mai 2024, 00:48:45 CEST schrieb Dragan Simic:
> Hello Chen-Yu,
> 
> On 2024-05-29 18:27, Chen-Yu Tsai wrote:
> > On Tue, May 21, 2024 at 1:20 AM Dragan Simic <dsimic@xxxxxxxxxxx> 
> > wrote:
> >> 
> >> Correct the specified regulator-min-microvolt value for the buck 
> >> DCDC_REG2
> >> regulator, which is part of the Rockchip RK809 PMIC, in the Pine64 
> >> Quartz64
> >> Model B board dts.  According to the RK809 datasheet, version 1.01, 
> >> this
> >> regulator is capable of producing voltages as low as 0.5 V on its 
> >> output,
> >> instead of going down to 0.9 V only, which is additionally confirmed 
> >> by the
> >> regulator-min-microvolt values found in the board dts files for the 
> >> other
> >> supported boards that use the same RK809 PMIC.
> >> 
> >> This allows the DVFS to clock the GPU on the Quartz64 Model B below 
> >> 700 MHz,
> >> all the way down to 200 MHz, which saves some power and reduces the 
> >> amount of
> >> generated heat a bit, improving the thermal headroom and possibly 
> >> improving
> >> the bursty CPU and GPU performance on this board.
> >> 
> >> This also eliminates the following warnings in the kernel log:
> >> 
> >>   core: _opp_supported_by_regulators: OPP minuV: 825000 maxuV: 825000, 
> >> not supported by regulator
> >>   panfrost fde60000.gpu: _opp_add: OPP not supported by regulators 
> >> (200000000)
> >>   core: _opp_supported_by_regulators: OPP minuV: 825000 maxuV: 825000, 
> >> not supported by regulator
> >>   panfrost fde60000.gpu: _opp_add: OPP not supported by regulators 
> >> (300000000)
> >>   core: _opp_supported_by_regulators: OPP minuV: 825000 maxuV: 825000, 
> >> not supported by regulator
> >>   panfrost fde60000.gpu: _opp_add: OPP not supported by regulators 
> >> (400000000)
> >>   core: _opp_supported_by_regulators: OPP minuV: 825000 maxuV: 825000, 
> >> not supported by regulator
> >>   panfrost fde60000.gpu: _opp_add: OPP not supported by regulators 
> >> (600000000)
> >> 
> >> Fixes: dcc8c66bef79 ("arm64: dts: rockchip: add Pine64 Quartz64-B 
> >> device tree")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Reported-By: Diederik de Haas <didi.debian@xxxxxxxxx>
> >> Signed-off-by: Dragan Simic <dsimic@xxxxxxxxxxx>
> >> ---
> >>  arch/arm64/boot/dts/rockchip/rk3566-quartz64-b.dts | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-b.dts 
> >> b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-b.dts
> >> index 26322a358d91..b908ce006c26 100644
> >> --- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-b.dts
> >> +++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-b.dts
> >> @@ -289,7 +289,7 @@ vdd_gpu: DCDC_REG2 {
> >>                                 regulator-name = "vdd_gpu";
> >>                                 regulator-always-on;
> >>                                 regulator-boot-on;
> >> -                               regulator-min-microvolt = <900000>;
> >> +                               regulator-min-microvolt = <500000>;
> > 
> > The constraints here are supposed to be the constraints of the 
> > consumer,
> > not the provider. The latter is already known by the implementation.
> > 
> > So if the GPU can go down to 0.825V or 0.81V even (based on the 
> > datasheet),
> > this should say the corresponding value. Surely the GPU can't go down 
> > to
> > 0.5V?
> > 
> > Can you send another fix for it?
> 
> I can confirm that the voltage of the power supply of GPU found inside
> the RK3566 can be as low as 0.81 V, according to the datasheet, or as
> low as 0.825 V, according to the GPU OPPs found in rk356x.dtsi.
> 
> If we want the regulator-min-microvolt parameter to reflect the 
> contraint
> of the GPU as the consumer, which I agree with, we should do that for 
> other
> RK3566-based boards as well, and almost surely for the boards based on 
> the
> RK3568, too.

Hmm, I'm not so sure about that.

The binding does define:
	regulator-min-microvolt:
	    description: smallest voltage consumers may set

This does not seem to describe it as a constraint solely of the consumer.
At least the wording sounds way more flexible there.

Also any regulator _could_ have multiple consumers, whose value would
it need then.


While true, setting it to the lowest the regulator can do in the original
fix patch, might've been a bit much and a saner value might be better.



> This would ensure consistency, but I'd like to know are all those 
> resulting
> patches going to be accepted before starting to prepare them?  There 
> will
> be a whole bunch of small patches.

Hmm, though I'd say that would be one patch per soc?

I.e. you're setting the min-voltage of _one_ regulator used
on each board to a value to support the defined OPPs.

I.e. in my mind you'd end up with:
	arm64: dts: rockchip: set better min voltage for vdd_gpu on rk356x boards

And setting the lower voltage to reach that lower OPP on all affected
rk356x boards.


Heiko

> 
> >>                                 regulator-max-microvolt = <1350000>;
> >>                                 regulator-ramp-delay = <6001>;
>