| Message ID | 446399362bd2dbeeaecd8351f68811165429749a.1719637113.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: Add optional GPU OPP voltage ranges to RK356x SoC dtsi | expand |
Hi Dragan, 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. 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. Heiko > 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 { >
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@manjaro.org/ > 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On 2024-06-29 17:25, Dragan Simic wrote: > 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@manjaro.org/ On second thought, after seeing that the RK3399 CPU and GPU OPPs already specify voltage ranges, I think it would be better to drop the distinction between the separate strict voltages and the voltage ranges in this patch, and to add some additional debugging messages to drivers/opp/of.c that would allow any misconfiguration issues to be rather easily detected. >> 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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
Am Samstag, 29. Juni 2024, 17:39:34 CEST schrieb Dragan Simic: > On 2024-06-29 17:25, Dragan Simic wrote: > > 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@manjaro.org/ > > On second thought, after seeing that the RK3399 CPU and GPU OPPs > already specify voltage ranges, I think it would be better to drop > the distinction between the separate strict voltages and the voltage > ranges in this patch, yes, that was what I was trying to say :-) Also it makes the OPPs less cluttered. Also dt is firmware, I do expect people to be reasonably knowledgeable if they mess around with their boards OPPs ;-) .
On 2024-06-29 18:18, Heiko Stübner wrote: > Am Samstag, 29. Juni 2024, 17:39:34 CEST schrieb Dragan Simic: >> On 2024-06-29 17:25, Dragan Simic wrote: >> > 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@manjaro.org/ >> >> On second thought, after seeing that the RK3399 CPU and GPU OPPs >> already specify voltage ranges, I think it would be better to drop >> the distinction between the separate strict voltages and the voltage >> ranges in this patch, > > yes, that was what I was trying to say :-) > > Also it makes the OPPs less cluttered. Also dt is firmware, I do expect > people to be reasonably knowledgeable if they mess around with their > boards OPPs ;-) . Yes, but we still need new regulator/OPP debugging facilities that should to be used while writing DTs for new boards and while verifying already existing board DTs. :) I'll prepare and send the v2 of this patch, and I'll also start working on the new patch for those debugging facilities.
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index d8543b5557ee..febda473dc38 100644 --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi @@ -1,5 +1,11 @@ // SPDX-License-Identifier: (GPL-2.0+ OR MIT) /* + * The defined GPU OPPs optionally support voltage ranges, which are useful + * for RK356x-based boards that are designed to use the same power supply for + * the GPU and NPU portions of the SoC. To use GPU OPPs with voltage ranges + * on such boards, define the RK356X_GPU_NPU_SHARED_REGULATOR macro in the + * descendant board dts(i) file, before including this file. + * * Copyright (c) 2021 Rockchip Electronics Co., Ltd. */ @@ -193,6 +199,7 @@ scmi_clk: protocol@14 { gpu_opp_table: opp-table-1 { compatible = "operating-points-v2"; +#ifndef RK356X_GPU_NPU_SHARED_REGULATOR 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 {
Add optional 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 from Rockchip: - Rockchip RK3566 Hardware Design Guide, version 1.1.0, page 37 - Rockchip RK3568 Hardware Design Guide, version 1.2, page 78 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] However, our values have served us well so far, so let's keep them for now, until we actually start supporting the CPU and GPU binning, together with the related voltage adjustments. No functional changes are introduced, which was validated by decompiling and comparing all affected dtb files before and after these changes. [1] https://github.com/rockchip-linux/kernel/commit/f8b9431ee38ed561650be7092ab93f564598daa9 [2] https://raw.githubusercontent.com/rockchip-linux/kernel/f8b9431ee38ed561650be7092ab93f564598daa9/arch/arm64/boot/dts/rockchip/rk3568.dtsi Suggested-by: Diederik de Haas <didi.debian@cknow.org> Helped-by: Jonas Karlman <jonas@kwiboo.se> Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- arch/arm64/boot/dts/rockchip/rk356x.dtsi | 38 ++++++++++++++++++++++++ 1 file changed, 38 insertions(+)