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

[Dragan Simic]

Hello Heiko,

On 2024-06-29 17:10, Heiko Stübner wrote:
> Am Samstag, 29. Juni 2024, 07:11:24 CEST schrieb Dragan Simic:
>
> > +#ifndef RK356X_GPU_NPU_SHARED_REGULATOR
>
> is there some reason for this duplicating of opps?
>
> The regulator framework should pick the lowest supported voltage
> anyway, so it seems you're just extending them upwards a bit.
>
> So I really don't so why we'd need to sets here.

The reason is improved strictness.  Having the exact GPU OPP voltages
required for the boards whose GPU regulators can provide those exact
voltages makes it possible to detect misconfigurations much easier,
just like it was the case with the board dts misconfiguration that
resulted in the recent DCDC_REG2 patch. [1]

If we had GPU OPP voltage ranges in place instead, the aforementioned
issue would probably remain undetected for some time.  It wouldn't be
the end of the world, :) of course, but the resulting increased power
consumption isn't one of the desired outcomes.

[1] 
https://lore.kernel.org/linux-rockchip/e70742ea2df432bf57b3f7de542d81ca22b0da2f.1716225483.git.dsimic@xxxxxxxxxxx/

> Also the voltage-range thing makes sense for non-gpu-npu-sharing
> boards, when the supplying regulator does not fully support the
> direct single-value voltage.
>
> (rk3399-puma was such a case if I remember correctly)
>
> So I really see no reason for this duplication.

Perhaps we could rename the RK356X_GPU_NPU_SHARED_REGULATOR macro
accordingly in the v2, to RK356X_GPU_OPP_VOLTAGE_RANGES, for example,
with some additional explanations in the patch description and the
RK356x SoC dtsi file itself.

> >  		opp-200000000 {
> >  			opp-hz = /bits/ 64 <200000000>;
> >  			opp-microvolt = <825000>;
> > @@ -222,6 +229,37 @@ opp-800000000 {
> >  			opp-hz = /bits/ 64 <800000000>;
> >  			opp-microvolt = <1000000>;
> >  		};
> > +#else
> > +		opp-200000000 {
> > +			opp-hz = /bits/ 64 <200000000>;
> > +			opp-microvolt = <825000 825000 1000000>;
> > +		};
> > +
> > +		opp-300000000 {
> > +			opp-hz = /bits/ 64 <300000000>;
> > +			opp-microvolt = <825000 825000 1000000>;
> > +		};
> > +
> > +		opp-400000000 {
> > +			opp-hz = /bits/ 64 <400000000>;
> > +			opp-microvolt = <825000 825000 1000000>;
> > +		};
> > +
> > +		opp-600000000 {
> > +			opp-hz = /bits/ 64 <600000000>;
> > +			opp-microvolt = <825000 825000 1000000>;
> > +		};
> > +
> > +		opp-700000000 {
> > +			opp-hz = /bits/ 64 <700000000>;
> > +			opp-microvolt = <900000 900000 1000000>;
> > +		};
> > +
> > +		opp-800000000 {
> > +			opp-hz = /bits/ 64 <800000000>;
> > +			opp-microvolt = <1000000 1000000 1000000>;
> > +		};
> > +#endif /* RK356X_GPU_NPU_SHARED_REGULATOR */
> >  	};
> >
> >  	hdmi_sound: hdmi-sound {
>
>
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-rockchip