2024-05-20 17:20:49

by Dragan Simic

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

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: [email protected]
Reported-By: Diederik de Haas <[email protected]>
Signed-off-by: Dragan Simic <[email protected]>
---
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>;
regulator-max-microvolt = <1350000>;
regulator-ramp-delay = <6001>;



2024-05-20 18:03:13

by Diederik de Haas

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

On Monday, 20 May 2024 19:20:28 CEST Dragan Simic wrote:
> 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)

Can confirm those messages are no longer there in ``dmesg``, I see no other
error/warning messages appear and my Q64-B still works. So

Tested-by: Diederik de Haas <[email protected]>

Thanks!

Cheers,
Diederik


Attachments:
signature.asc (235.00 B)
This is a digitally signed message part.

2024-05-27 22:43:25

by Heiko Stuebner

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

On Mon, 20 May 2024 19:20:28 +0200, Dragan Simic 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.
>
> [...]

Applied, thanks!

[1/1] arm64: dts: rockchip: Fix the DCDC_REG2 minimum voltage on Quartz64 Model B
commit: d201c92bff90f3d3d0b079fc955378c15c0483cc

Best regards,
--
Heiko Stuebner <[email protected]>

2024-05-29 16:27:45

by Chen-Yu Tsai

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

On Tue, May 21, 2024 at 1:20 AM Dragan Simic <[email protected]> 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: [email protected]
> Reported-By: Diederik de Haas <[email protected]>
> Signed-off-by: Dragan Simic <[email protected]>
> ---
> 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?


ChenYu

> regulator-max-microvolt = <1350000>;
> regulator-ramp-delay = <6001>;
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2024-05-30 22:49:06

by Dragan Simic

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

Hello Chen-Yu,

On 2024-05-29 18:27, Chen-Yu Tsai wrote:
> On Tue, May 21, 2024 at 1:20 AM Dragan Simic <[email protected]>
> 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: [email protected]
>> Reported-By: Diederik de Haas <[email protected]>
>> Signed-off-by: Dragan Simic <[email protected]>
>> ---
>> 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.

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.

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

2024-05-31 18:40:22

by Heiko Stuebner

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

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 <[email protected]>
> > 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: [email protected]
> >> Reported-By: Diederik de Haas <[email protected]>
> >> Signed-off-by: Dragan Simic <[email protected]>
> >> ---
> >> 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>;
>





2024-05-31 22:42:05

by Dragan Simic

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

Hello Heiko,

On 2024-05-31 20:40, Heiko Stübner wrote:
> Am Freitag, 31. Mai 2024, 00:48:45 CEST schrieb Dragan Simic:
>> On 2024-05-29 18:27, Chen-Yu Tsai wrote:
>> > On Tue, May 21, 2024 at 1:20 AM Dragan Simic <[email protected]>
>> > 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: [email protected]
>> >> Reported-By: Diederik de Haas <[email protected]>
>> >> Signed-off-by: Dragan Simic <[email protected]>
>> >> ---
>> >> 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.

The way I see it, the regulator-min-microvolt and
regulator-max-microvolt
parameters should be configured in a way that protects the consumer(s)
of the particular voltage regulator against undervoltage and overvoltage
conditions, which may be useful in some corner cases.

If there are multiple consumers, which in this case may actually happen
(IIRC, some boards use the same regulator for the GPU and NPU portions
of the SoC), the situation becomes far from ideal, because the consumers
might have different voltage requirements, but that's pretty much an
unavoidable compromise.

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

Agreed, but the value was selected according to what the other
RK3566-based
boards use, to establish some kind of consistency. Now, there's a good
chance for the second pass, so to speak, which should establish another
different state, but also consistent. :)

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

Yes, the same thoughts have already crossed my mind, but I thought we'd
like those patches to also include Fixes tags, so they also get
propagated
into the long-term kernel versions? In that case, we'd need one patch
per
board, to have a clear relation to the commits referenced in the Fixes
tags.

OTOH, if we don't want the patches to be propagated into the long-term
kernel
versions, then having one patch per SoC would be perfectly fine.

2024-06-03 03:49:30

by Chen-Yu Tsai

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

On Sat, Jun 1, 2024 at 6:41 AM Dragan Simic <[email protected]> wrote:
>
> Hello Heiko,
>
> On 2024-05-31 20:40, Heiko Stübner wrote:
> > Am Freitag, 31. Mai 2024, 00:48:45 CEST schrieb Dragan Simic:
> >> On 2024-05-29 18:27, Chen-Yu Tsai wrote:
> >> > On Tue, May 21, 2024 at 1:20 AM Dragan Simic <[email protected]>
> >> > 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: [email protected]
> >> >> Reported-By: Diederik de Haas <[email protected]>
> >> >> Signed-off-by: Dragan Simic <[email protected]>
> >> >> ---
> >> >> 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.
>
> The way I see it, the regulator-min-microvolt and
> regulator-max-microvolt
> parameters should be configured in a way that protects the consumer(s)
> of the particular voltage regulator against undervoltage and overvoltage
> conditions, which may be useful in some corner cases.
>
> If there are multiple consumers, which in this case may actually happen
> (IIRC, some boards use the same regulator for the GPU and NPU portions
> of the SoC), the situation becomes far from ideal, because the consumers
> might have different voltage requirements, but that's pretty much an
> unavoidable compromise.

As Dragan mentioned, the min/max voltage constraints are there to prevent
the implementation from setting a voltage that would make the hardware
inoperable, either temporarily or permanently. So the range set here
should be the intersection of the permitted ranges of all consumers on
that power rail.

Now if that intersection happens to be an empty set, then it would up
to the implementation to do proper lock-outs. Hopefully no one designs
such hardware as it's too easy to fry some part of the hardware.

> > 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.
>
> Agreed, but the value was selected according to what the other
> RK3566-based
> boards use, to establish some kind of consistency. Now, there's a good
> chance for the second pass, so to speak, which should establish another
> different state, but also consistent. :)
>
> >> 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.
>
> Yes, the same thoughts have already crossed my mind, but I thought we'd
> like those patches to also include Fixes tags, so they also get
> propagated
> into the long-term kernel versions? In that case, we'd need one patch
> per
> board, to have a clear relation to the commits referenced in the Fixes
> tags.
>
> OTOH, if we don't want the patches to be propagated into the long-term
> kernel
> versions, then having one patch per SoC would be perfectly fine.

It's really up to Heiko, but personally I don't think it's that important
to have them backported. These would be correctness patches, but don't
really affect functionality.

Regards
ChenYu

2024-06-03 04:41:50

by Dragan Simic

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

Hello Chen-Yu,

On 2024-06-03 05:49, Chen-Yu Tsai wrote:
> On Sat, Jun 1, 2024 at 6:41 AM Dragan Simic <[email protected]> wrote:
>> On 2024-05-31 20:40, Heiko Stübner wrote:
>> > Am Freitag, 31. Mai 2024, 00:48:45 CEST schrieb Dragan Simic:
>> >> On 2024-05-29 18:27, Chen-Yu Tsai wrote:
>> >> > On Tue, May 21, 2024 at 1:20 AM Dragan Simic <[email protected]>
>> >> > 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: [email protected]
>> >> >> Reported-By: Diederik de Haas <[email protected]>
>> >> >> Signed-off-by: Dragan Simic <[email protected]>
>> >> >> ---
>> >> >> 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.
>>
>> The way I see it, the regulator-min-microvolt and
>> regulator-max-microvolt
>> parameters should be configured in a way that protects the consumer(s)
>> of the particular voltage regulator against undervoltage and
>> overvoltage
>> conditions, which may be useful in some corner cases.
>>
>> If there are multiple consumers, which in this case may actually
>> happen
>> (IIRC, some boards use the same regulator for the GPU and NPU portions
>> of the SoC), the situation becomes far from ideal, because the
>> consumers
>> might have different voltage requirements, but that's pretty much an
>> unavoidable compromise.
>
> As Dragan mentioned, the min/max voltage constraints are there to
> prevent
> the implementation from setting a voltage that would make the hardware
> inoperable, either temporarily or permanently. So the range set here
> should be the intersection of the permitted ranges of all consumers on
> that power rail.
>
> Now if that intersection happens to be an empty set, then it would up
> to the implementation to do proper lock-outs. Hopefully no one designs
> such hardware as it's too easy to fry some part of the hardware.

Yes, such a hardware design would need fixing first on the schematic
level. When it comes to the RK3566's GPU and NPU sharing the same
regulator, we should be fine because the RK3566 datasheet states that
both the GPU and the NPU can go as low as 0.81 V, and their upper
absolute ratings are the same at 1.2 V, so 1.0 V, which is as far as
the GPU OPPs go, should be fine for both.

As a note, neither the RK3566 datasheet nor the RK3566 hardware design
guide specify the recommended upper voltage limit for the GPU or the
NPU. Though, their upper absolute ratings are the same, as already
described above.

>> > 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.
>>
>> Agreed, but the value was selected according to what the other
>> RK3566-based
>> boards use, to establish some kind of consistency. Now, there's a
>> good
>> chance for the second pass, so to speak, which should establish
>> another
>> different state, but also consistent. :)
>>
>> >> 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.
>>
>> Yes, the same thoughts have already crossed my mind, but I thought
>> we'd
>> like those patches to also include Fixes tags, so they also get
>> propagated
>> into the long-term kernel versions? In that case, we'd need one patch
>> per
>> board, to have a clear relation to the commits referenced in the Fixes
>> tags.
>>
>> OTOH, if we don't want the patches to be propagated into the long-term
>> kernel
>> versions, then having one patch per SoC would be perfectly fine.
>
> It's really up to Heiko, but personally I don't think it's that
> important
> to have them backported. These would be correctness patches, but don't
> really affect functionality.

On second thought, I also think that it might be better not to have
these changes propagated into the long-term kernel versions. That
would keep the amount of backported changes to the bare minimum, i.e.
containing just the really important fixes, while these changes are
more on the correctness side. Maybe together with providing a bit
of additional safety.

2024-06-03 04:53:11

by Dragan Simic

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

On 2024-06-03 06:41, Dragan Simic wrote:
> On 2024-06-03 05:49, Chen-Yu Tsai wrote:
>> On Sat, Jun 1, 2024 at 6:41 AM Dragan Simic <[email protected]>
>> wrote:
>>> On 2024-05-31 20:40, Heiko Stübner wrote:
>>> > Am Freitag, 31. Mai 2024, 00:48:45 CEST schrieb Dragan Simic:
>>> >> On 2024-05-29 18:27, Chen-Yu Tsai wrote:
>>> >> > On Tue, May 21, 2024 at 1:20 AM Dragan Simic <[email protected]>
>>> >> > 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: [email protected]
>>> >> >> Reported-By: Diederik de Haas <[email protected]>
>>> >> >> Signed-off-by: Dragan Simic <[email protected]>
>>> >> >> ---
>>> >> >> 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.
>>>
>>> The way I see it, the regulator-min-microvolt and
>>> regulator-max-microvolt
>>> parameters should be configured in a way that protects the
>>> consumer(s)
>>> of the particular voltage regulator against undervoltage and
>>> overvoltage
>>> conditions, which may be useful in some corner cases.
>>>
>>> If there are multiple consumers, which in this case may actually
>>> happen
>>> (IIRC, some boards use the same regulator for the GPU and NPU
>>> portions
>>> of the SoC), the situation becomes far from ideal, because the
>>> consumers
>>> might have different voltage requirements, but that's pretty much an
>>> unavoidable compromise.
>>
>> As Dragan mentioned, the min/max voltage constraints are there to
>> prevent
>> the implementation from setting a voltage that would make the hardware
>> inoperable, either temporarily or permanently. So the range set here
>> should be the intersection of the permitted ranges of all consumers on
>> that power rail.
>>
>> Now if that intersection happens to be an empty set, then it would up
>> to the implementation to do proper lock-outs. Hopefully no one designs
>> such hardware as it's too easy to fry some part of the hardware.
>
> Yes, such a hardware design would need fixing first on the schematic
> level. When it comes to the RK3566's GPU and NPU sharing the same
> regulator, we should be fine because the RK3566 datasheet states that
> both the GPU and the NPU can go as low as 0.81 V, and their upper
> absolute ratings are the same at 1.2 V, so 1.0 V, which is as far as
> the GPU OPPs go, should be fine for both.
>
> As a note, neither the RK3566 datasheet nor the RK3566 hardware design
> guide specify the recommended upper voltage limit for the GPU or the
> NPU. Though, their upper absolute ratings are the same, as already
> described above.

Uh-oh, this rabbit hole goes much deeper than expected. After a quick
check, I see there are also RK3399-based boards/devices that specify
the minimum and maximum values for their GPU regulators far outside
the recommended operating conditions of the RK3399's GPU.

Perhaps the scope of the upcoming patches should be expanded to cover
other boards as well, not just those based on the RK356x.

>>> > 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.
>>>
>>> Agreed, but the value was selected according to what the other
>>> RK3566-based
>>> boards use, to establish some kind of consistency. Now, there's a
>>> good
>>> chance for the second pass, so to speak, which should establish
>>> another
>>> different state, but also consistent. :)
>>>
>>> >> 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.
>>>
>>> Yes, the same thoughts have already crossed my mind, but I thought
>>> we'd
>>> like those patches to also include Fixes tags, so they also get
>>> propagated
>>> into the long-term kernel versions? In that case, we'd need one
>>> patch
>>> per
>>> board, to have a clear relation to the commits referenced in the
>>> Fixes
>>> tags.
>>>
>>> OTOH, if we don't want the patches to be propagated into the
>>> long-term
>>> kernel
>>> versions, then having one patch per SoC would be perfectly fine.
>>
>> It's really up to Heiko, but personally I don't think it's that
>> important
>> to have them backported. These would be correctness patches, but don't
>> really affect functionality.
>
> On second thought, I also think that it might be better not to have
> these changes propagated into the long-term kernel versions. That
> would keep the amount of backported changes to the bare minimum, i.e.
> containing just the really important fixes, while these changes are
> more on the correctness side. Maybe together with providing a bit
> of additional safety.
>
> _______________________________________________
> Linux-rockchip mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

2024-06-03 06:33:45

by Heiko Stuebner

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

Am Montag, 3. Juni 2024, 06:51:58 CEST schrieb Dragan Simic:
> On 2024-06-03 06:41, Dragan Simic wrote:
> > On 2024-06-03 05:49, Chen-Yu Tsai wrote:
> >> On Sat, Jun 1, 2024 at 6:41 AM Dragan Simic <[email protected]>
> >> wrote:
> >>> On 2024-05-31 20:40, Heiko Stübner wrote:
> >>> > Am Freitag, 31. Mai 2024, 00:48:45 CEST schrieb Dragan Simic:
> >>> >> On 2024-05-29 18:27, Chen-Yu Tsai wrote:
> >>> >> > On Tue, May 21, 2024 at 1:20 AM Dragan Simic <[email protected]>
> >>> >> > 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: [email protected]
> >>> >> >> Reported-By: Diederik de Haas <[email protected]>
> >>> >> >> Signed-off-by: Dragan Simic <[email protected]>
> >>> >> >> ---
> >>> >> >> 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.
> >>>
> >>> The way I see it, the regulator-min-microvolt and
> >>> regulator-max-microvolt
> >>> parameters should be configured in a way that protects the
> >>> consumer(s)
> >>> of the particular voltage regulator against undervoltage and
> >>> overvoltage
> >>> conditions, which may be useful in some corner cases.
> >>>
> >>> If there are multiple consumers, which in this case may actually
> >>> happen
> >>> (IIRC, some boards use the same regulator for the GPU and NPU
> >>> portions
> >>> of the SoC), the situation becomes far from ideal, because the
> >>> consumers
> >>> might have different voltage requirements, but that's pretty much an
> >>> unavoidable compromise.
> >>
> >> As Dragan mentioned, the min/max voltage constraints are there to
> >> prevent
> >> the implementation from setting a voltage that would make the hardware
> >> inoperable, either temporarily or permanently. So the range set here
> >> should be the intersection of the permitted ranges of all consumers on
> >> that power rail.
> >>
> >> Now if that intersection happens to be an empty set, then it would up
> >> to the implementation to do proper lock-outs. Hopefully no one designs
> >> such hardware as it's too easy to fry some part of the hardware.
> >
> > Yes, such a hardware design would need fixing first on the schematic
> > level. When it comes to the RK3566's GPU and NPU sharing the same
> > regulator, we should be fine because the RK3566 datasheet states that
> > both the GPU and the NPU can go as low as 0.81 V, and their upper
> > absolute ratings are the same at 1.2 V, so 1.0 V, which is as far as
> > the GPU OPPs go, should be fine for both.
> >
> > As a note, neither the RK3566 datasheet nor the RK3566 hardware design
> > guide specify the recommended upper voltage limit for the GPU or the
> > NPU. Though, their upper absolute ratings are the same, as already
> > described above.
>
> Uh-oh, this rabbit hole goes much deeper than expected. After a quick
> check, I see there are also RK3399-based boards/devices that specify
> the minimum and maximum values for their GPU regulators far outside
> the recommended operating conditions of the RK3399's GPU.
>
> Perhaps the scope of the upcoming patches should be expanded to cover
> other boards as well, not just those based on the RK356x.
>
> >>> > 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.
> >>>
> >>> Agreed, but the value was selected according to what the other
> >>> RK3566-based
> >>> boards use, to establish some kind of consistency. Now, there's a
> >>> good
> >>> chance for the second pass, so to speak, which should establish
> >>> another
> >>> different state, but also consistent. :)
> >>>
> >>> >> 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.
> >>>
> >>> Yes, the same thoughts have already crossed my mind, but I thought
> >>> we'd
> >>> like those patches to also include Fixes tags, so they also get
> >>> propagated
> >>> into the long-term kernel versions? In that case, we'd need one
> >>> patch
> >>> per
> >>> board, to have a clear relation to the commits referenced in the
> >>> Fixes
> >>> tags.
> >>>
> >>> OTOH, if we don't want the patches to be propagated into the
> >>> long-term
> >>> kernel
> >>> versions, then having one patch per SoC would be perfectly fine.
> >>
> >> It's really up to Heiko, but personally I don't think it's that
> >> important
> >> to have them backported. These would be correctness patches, but don't
> >> really affect functionality.
> >
> > On second thought, I also think that it might be better not to have
> > these changes propagated into the long-term kernel versions. That
> > would keep the amount of backported changes to the bare minimum, i.e.
> > containing just the really important fixes, while these changes are
> > more on the correctness side. Maybe together with providing a bit
> > of additional safety.

hehe, up to you I guess :-) .

At least we tied down the how (one patch per soc or so) and not meant
to be backported because more of the correctnes side. So yes I agree with
the arguments for changing the constraints.

Heiko



2024-06-03 06:54:49

by Dragan Simic

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

On 2024-06-03 08:33, Heiko Stübner wrote:
> Am Montag, 3. Juni 2024, 06:51:58 CEST schrieb Dragan Simic:
>> On 2024-06-03 06:41, Dragan Simic wrote:
>> > On 2024-06-03 05:49, Chen-Yu Tsai wrote:
>> >> On Sat, Jun 1, 2024 at 6:41 AM Dragan Simic <[email protected]>
>> >> wrote:
>> >>> On 2024-05-31 20:40, Heiko Stübner wrote:
>> >>> > Am Freitag, 31. Mai 2024, 00:48:45 CEST schrieb Dragan Simic:
>> >>> >> On 2024-05-29 18:27, Chen-Yu Tsai wrote:
>> >>> >> > On Tue, May 21, 2024 at 1:20 AM Dragan Simic <[email protected]>
>> >>> >> > 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: [email protected]
>> >>> >> >> Reported-By: Diederik de Haas <[email protected]>
>> >>> >> >> Signed-off-by: Dragan Simic <[email protected]>
>> >>> >> >> ---
>> >>> >> >> 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.
>> >>>
>> >>> The way I see it, the regulator-min-microvolt and
>> >>> regulator-max-microvolt
>> >>> parameters should be configured in a way that protects the
>> >>> consumer(s)
>> >>> of the particular voltage regulator against undervoltage and
>> >>> overvoltage
>> >>> conditions, which may be useful in some corner cases.
>> >>>
>> >>> If there are multiple consumers, which in this case may actually
>> >>> happen
>> >>> (IIRC, some boards use the same regulator for the GPU and NPU
>> >>> portions
>> >>> of the SoC), the situation becomes far from ideal, because the
>> >>> consumers
>> >>> might have different voltage requirements, but that's pretty much an
>> >>> unavoidable compromise.
>> >>
>> >> As Dragan mentioned, the min/max voltage constraints are there to
>> >> prevent
>> >> the implementation from setting a voltage that would make the hardware
>> >> inoperable, either temporarily or permanently. So the range set here
>> >> should be the intersection of the permitted ranges of all consumers on
>> >> that power rail.
>> >>
>> >> Now if that intersection happens to be an empty set, then it would up
>> >> to the implementation to do proper lock-outs. Hopefully no one designs
>> >> such hardware as it's too easy to fry some part of the hardware.
>> >
>> > Yes, such a hardware design would need fixing first on the schematic
>> > level. When it comes to the RK3566's GPU and NPU sharing the same
>> > regulator, we should be fine because the RK3566 datasheet states that
>> > both the GPU and the NPU can go as low as 0.81 V, and their upper
>> > absolute ratings are the same at 1.2 V, so 1.0 V, which is as far as
>> > the GPU OPPs go, should be fine for both.
>> >
>> > As a note, neither the RK3566 datasheet nor the RK3566 hardware design
>> > guide specify the recommended upper voltage limit for the GPU or the
>> > NPU. Though, their upper absolute ratings are the same, as already
>> > described above.
>>
>> Uh-oh, this rabbit hole goes much deeper than expected. After a quick
>> check, I see there are also RK3399-based boards/devices that specify
>> the minimum and maximum values for their GPU regulators far outside
>> the recommended operating conditions of the RK3399's GPU.
>>
>> Perhaps the scope of the upcoming patches should be expanded to cover
>> other boards as well, not just those based on the RK356x.
>>
>> >>> > 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.
>> >>>
>> >>> Agreed, but the value was selected according to what the other
>> >>> RK3566-based
>> >>> boards use, to establish some kind of consistency. Now, there's a
>> >>> good
>> >>> chance for the second pass, so to speak, which should establish
>> >>> another
>> >>> different state, but also consistent. :)
>> >>>
>> >>> >> 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.
>> >>>
>> >>> Yes, the same thoughts have already crossed my mind, but I thought
>> >>> we'd
>> >>> like those patches to also include Fixes tags, so they also get
>> >>> propagated
>> >>> into the long-term kernel versions? In that case, we'd need one
>> >>> patch
>> >>> per
>> >>> board, to have a clear relation to the commits referenced in the
>> >>> Fixes
>> >>> tags.
>> >>>
>> >>> OTOH, if we don't want the patches to be propagated into the
>> >>> long-term
>> >>> kernel
>> >>> versions, then having one patch per SoC would be perfectly fine.
>> >>
>> >> It's really up to Heiko, but personally I don't think it's that
>> >> important
>> >> to have them backported. These would be correctness patches, but don't
>> >> really affect functionality.
>> >
>> > On second thought, I also think that it might be better not to have
>> > these changes propagated into the long-term kernel versions. That
>> > would keep the amount of backported changes to the bare minimum, i.e.
>> > containing just the really important fixes, while these changes are
>> > more on the correctness side. Maybe together with providing a bit
>> > of additional safety.
>
> hehe, up to you I guess :-) .
>
> At least we tied down the how (one patch per soc or so) and not meant
> to be backported because more of the correctnes side. So yes I agree
> with
> the arguments for changing the constraints.

Thanks. :) I'll move forward with per-SoC patches, and I'll see
how far it will all go when it comes to correcting the GPU voltage
contraints for different SoCs. Maybe some other voltage contraints
in need of correction will pop up while checking all that.

When I think even more about it, perhaps making the descriptions
of regulator-min-microvolt and regulator-max-microvolt a bit more
descriptive in the bindings would make sense, which I'd be willing
to prepare patches for. Perhaps Krzysztof could provide an insight
into how much sense would that make.

2024-06-03 07:11:11

by Dragan Simic

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

+To: Krzysztof Kozlowski <[email protected]>

Please see the question at the very end of this message.

On 2024-06-03 08:54, Dragan Simic wrote:
> On 2024-06-03 08:33, Heiko Stübner wrote:
>> Am Montag, 3. Juni 2024, 06:51:58 CEST schrieb Dragan Simic:
>>> On 2024-06-03 06:41, Dragan Simic wrote:
>>> > On 2024-06-03 05:49, Chen-Yu Tsai wrote:
>>> >> On Sat, Jun 1, 2024 at 6:41 AM Dragan Simic <[email protected]>
>>> >> wrote:
>>> >>> On 2024-05-31 20:40, Heiko Stübner wrote:
>>> >>> > Am Freitag, 31. Mai 2024, 00:48:45 CEST schrieb Dragan Simic:
>>> >>> >> On 2024-05-29 18:27, Chen-Yu Tsai wrote:
>>> >>> >> > On Tue, May 21, 2024 at 1:20 AM Dragan Simic <[email protected]>
>>> >>> >> > 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: [email protected]
>>> >>> >> >> Reported-By: Diederik de Haas <[email protected]>
>>> >>> >> >> Signed-off-by: Dragan Simic <[email protected]>
>>> >>> >> >> ---
>>> >>> >> >> 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.
>>> >>>
>>> >>> The way I see it, the regulator-min-microvolt and
>>> >>> regulator-max-microvolt
>>> >>> parameters should be configured in a way that protects the
>>> >>> consumer(s)
>>> >>> of the particular voltage regulator against undervoltage and
>>> >>> overvoltage
>>> >>> conditions, which may be useful in some corner cases.
>>> >>>
>>> >>> If there are multiple consumers, which in this case may actually
>>> >>> happen
>>> >>> (IIRC, some boards use the same regulator for the GPU and NPU
>>> >>> portions
>>> >>> of the SoC), the situation becomes far from ideal, because the
>>> >>> consumers
>>> >>> might have different voltage requirements, but that's pretty much an
>>> >>> unavoidable compromise.
>>> >>
>>> >> As Dragan mentioned, the min/max voltage constraints are there to
>>> >> prevent
>>> >> the implementation from setting a voltage that would make the hardware
>>> >> inoperable, either temporarily or permanently. So the range set here
>>> >> should be the intersection of the permitted ranges of all consumers on
>>> >> that power rail.
>>> >>
>>> >> Now if that intersection happens to be an empty set, then it would up
>>> >> to the implementation to do proper lock-outs. Hopefully no one designs
>>> >> such hardware as it's too easy to fry some part of the hardware.
>>> >
>>> > Yes, such a hardware design would need fixing first on the schematic
>>> > level. When it comes to the RK3566's GPU and NPU sharing the same
>>> > regulator, we should be fine because the RK3566 datasheet states that
>>> > both the GPU and the NPU can go as low as 0.81 V, and their upper
>>> > absolute ratings are the same at 1.2 V, so 1.0 V, which is as far as
>>> > the GPU OPPs go, should be fine for both.
>>> >
>>> > As a note, neither the RK3566 datasheet nor the RK3566 hardware design
>>> > guide specify the recommended upper voltage limit for the GPU or the
>>> > NPU. Though, their upper absolute ratings are the same, as already
>>> > described above.
>>>
>>> Uh-oh, this rabbit hole goes much deeper than expected. After a
>>> quick
>>> check, I see there are also RK3399-based boards/devices that specify
>>> the minimum and maximum values for their GPU regulators far outside
>>> the recommended operating conditions of the RK3399's GPU.
>>>
>>> Perhaps the scope of the upcoming patches should be expanded to cover
>>> other boards as well, not just those based on the RK356x.
>>>
>>> >>> > 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.
>>> >>>
>>> >>> Agreed, but the value was selected according to what the other
>>> >>> RK3566-based
>>> >>> boards use, to establish some kind of consistency. Now, there's a
>>> >>> good
>>> >>> chance for the second pass, so to speak, which should establish
>>> >>> another
>>> >>> different state, but also consistent. :)
>>> >>>
>>> >>> >> 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.
>>> >>>
>>> >>> Yes, the same thoughts have already crossed my mind, but I thought
>>> >>> we'd
>>> >>> like those patches to also include Fixes tags, so they also get
>>> >>> propagated
>>> >>> into the long-term kernel versions? In that case, we'd need one
>>> >>> patch
>>> >>> per
>>> >>> board, to have a clear relation to the commits referenced in the
>>> >>> Fixes
>>> >>> tags.
>>> >>>
>>> >>> OTOH, if we don't want the patches to be propagated into the
>>> >>> long-term
>>> >>> kernel
>>> >>> versions, then having one patch per SoC would be perfectly fine.
>>> >>
>>> >> It's really up to Heiko, but personally I don't think it's that
>>> >> important
>>> >> to have them backported. These would be correctness patches, but don't
>>> >> really affect functionality.
>>> >
>>> > On second thought, I also think that it might be better not to have
>>> > these changes propagated into the long-term kernel versions. That
>>> > would keep the amount of backported changes to the bare minimum, i.e.
>>> > containing just the really important fixes, while these changes are
>>> > more on the correctness side. Maybe together with providing a bit
>>> > of additional safety.
>>
>> hehe, up to you I guess :-) .
>>
>> At least we tied down the how (one patch per soc or so) and not meant
>> to be backported because more of the correctnes side. So yes I agree
>> with
>> the arguments for changing the constraints.
>
> Thanks. :) I'll move forward with per-SoC patches, and I'll see
> how far it will all go when it comes to correcting the GPU voltage
> contraints for different SoCs. Maybe some other voltage contraints
> in need of correction will pop up while checking all that.
>
> When I think even more about it, perhaps making the descriptions
> of regulator-min-microvolt and regulator-max-microvolt a bit more
> descriptive in the bindings would make sense, which I'd be willing
> to prepare patches for. Perhaps Krzysztof could provide an insight
> into how much sense would that make.