| Message ID | 92ebc9cba6eb669df73efd478e4f5745056a4ce5.1723614345.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: allwinner: a64: Move CPU OPPs to the SoC dtsi file | expand |
Hi, On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@manjaro.org> wrote: > > Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and, consequently, > adjust the contents of the affected board dts(i) files appropriately, to > "encapsulate" the CPU OPPs into the SoC dtsi file. > > Moving the CPU OPPs to the SoC dtsi file, instead of requiring the board > dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi file, > reduces the possibility for incomplete SoC data inclusion and improves the > overall hierarchical representation of data. Moreover, the CPU OPPs are > not used anywhere but together with the SoC dtsi file, which additionally > justifies the folding of the CPU OPPs into the SoC dtsi file. > > No functional changes are introduced, which was validated by decompiling and > comparing all affected board dtb files before and after these changes. When > compared with the decompiled original dtb files, the updated dtb files have > some of their blocks shuffled around a bit and some of their phandles have > different values, as a result of the changes to the order in which the > building blocks from the parent dtsi files are included into them, but they > still effectively remain the same as the originals. IIRC, this was a conscious decision requiring board dts files to set their CPU supply before OPPs are given. The bootloader does not boot the SoC at the highest possible OPP / regulator voltage, so if the OPPs are given but the supply is not, the kernel will attempt to raise the frequency beyond what the current voltage can supply, causing it to hang. Now that all existing boards have it properly enabled, there should be no need for this. However I would appreciate a second opinion. ChenYu > Signed-off-by: Dragan Simic <dsimic@manjaro.org> > --- > > Notes: > These changes follow the general approach used for the Rockchip RK3588 > and RK3399 SoCs, which was introduced and described further in commits > def88eb4d836 ("arm64: dts: rockchip: Prepare RK3588 SoC dtsi files for > per-variant OPPs") [1] and 296602b8e5f7 ("arm64: dts: rockchip: Move > RK3399 OPPs to dtsi files for SoC variants"), [2] respectively. Please > see those commits for a more detailed explanation. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=def88eb4d8365a4aa064d28405d03550a9d0a3be > [2] https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=296602b8e5f715d6a0ccdcd37d57170c2c81d5e4 > > .../allwinner/sun50i-a64-amarula-relic.dts | 2 - > .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 2 - > .../dts/allwinner/sun50i-a64-cpu-opp.dtsi | 75 ------------------- > .../dts/allwinner/sun50i-a64-nanopi-a64.dts | 2 - > .../dts/allwinner/sun50i-a64-olinuxino.dts | 2 - > .../dts/allwinner/sun50i-a64-orangepi-win.dts | 2 - > .../boot/dts/allwinner/sun50i-a64-pine64.dts | 2 - > .../dts/allwinner/sun50i-a64-pinebook.dts | 2 - > .../dts/allwinner/sun50i-a64-pinephone.dtsi | 2 - > .../boot/dts/allwinner/sun50i-a64-pinetab.dts | 2 - > .../boot/dts/allwinner/sun50i-a64-sopine.dtsi | 2 - > .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 - > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 66 +++++++++++++++- > .../allwinner/sun50i-h64-remix-mini-pc.dts | 2 - > 14 files changed, 63 insertions(+), 102 deletions(-) > delete mode 100644 arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts > index 8233582f6288..1590a455683f 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts > @@ -5,8 +5,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > > / { > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts > index d1f415acd7b5..869fd4a50582 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts > @@ -4,8 +4,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > > / { > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi > deleted file mode 100644 > index e39db51eb448..000000000000 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi > +++ /dev/null > @@ -1,75 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> > - */ > - > -/ { > - cpu0_opp_table: opp-table-cpu { > - compatible = "operating-points-v2"; > - opp-shared; > - > - opp-648000000 { > - opp-hz = /bits/ 64 <648000000>; > - opp-microvolt = <1040000>; > - clock-latency-ns = <244144>; /* 8 32k periods */ > - }; > - > - opp-816000000 { > - opp-hz = /bits/ 64 <816000000>; > - opp-microvolt = <1100000>; > - clock-latency-ns = <244144>; /* 8 32k periods */ > - }; > - > - opp-912000000 { > - opp-hz = /bits/ 64 <912000000>; > - opp-microvolt = <1120000>; > - clock-latency-ns = <244144>; /* 8 32k periods */ > - }; > - > - opp-960000000 { > - opp-hz = /bits/ 64 <960000000>; > - opp-microvolt = <1160000>; > - clock-latency-ns = <244144>; /* 8 32k periods */ > - }; > - > - opp-1008000000 { > - opp-hz = /bits/ 64 <1008000000>; > - opp-microvolt = <1200000>; > - clock-latency-ns = <244144>; /* 8 32k periods */ > - }; > - > - opp-1056000000 { > - opp-hz = /bits/ 64 <1056000000>; > - opp-microvolt = <1240000>; > - clock-latency-ns = <244144>; /* 8 32k periods */ > - }; > - > - opp-1104000000 { > - opp-hz = /bits/ 64 <1104000000>; > - opp-microvolt = <1260000>; > - clock-latency-ns = <244144>; /* 8 32k periods */ > - }; > - > - opp-1152000000 { > - opp-hz = /bits/ 64 <1152000000>; > - opp-microvolt = <1300000>; > - clock-latency-ns = <244144>; /* 8 32k periods */ > - }; > - }; > -}; > - > -&cpu0 { > - operating-points-v2 = <&cpu0_opp_table>; > -}; > - > -&cpu1 { > - operating-points-v2 = <&cpu0_opp_table>; > -}; > - > -&cpu2 { > - operating-points-v2 = <&cpu0_opp_table>; > -}; > - > -&cpu3 { > - operating-points-v2 = <&cpu0_opp_table>; > -}; > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts > index dec9960a7440..e3b1943f7f63 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts > @@ -4,8 +4,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > > / { > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts > index fd3794678c33..f1a4a9ab295b 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts > @@ -4,8 +4,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > > / { > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts > index c8303a66438d..f3c9a3534612 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts > @@ -5,8 +5,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > > / { > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts > index 09e71fd60785..4f65eec043d0 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts > @@ -4,8 +4,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > > / { > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > index 379c2c8466f5..a06a0b34bd3f 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > @@ -5,8 +5,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/gpio-keys.h> > #include <dt-bindings/input/input.h> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > index 6eab61a12cd8..0e38cd02a90a 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > @@ -4,8 +4,6 @@ > // Copyright (C) 2020 Ondrej Jirman <megous@megous.com> > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > #include <dt-bindings/leds/common.h> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts > index f5fb1ee32dad..4a49f137885b 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts > @@ -7,8 +7,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > #include <dt-bindings/pwm/pwm.h> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > index 6d78a1c98f10..4ba5c11e6870 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > @@ -4,8 +4,6 @@ > // Copyright (c) 2016 ARM Ltd. > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > > &codec_analog { > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > index b407e1dd08a7..61ccd90bae01 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > @@ -5,8 +5,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > #include <dt-bindings/pwm/pwm.h> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > index e868ca5ae753..f842e64562b7 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > @@ -1,7 +1,10 @@ > // SPDX-License-Identifier: (GPL-2.0+ OR MIT) > -// Copyright (C) 2016 ARM Ltd. > -// based on the Allwinner H3 dtsi: > -// Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> > +/* > + * Copyright (C) 2016 ARM Ltd. > + * based on the Allwinner H3 dtsi: > + * Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> > + * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> > + */ > > #include <dt-bindings/clock/sun50i-a64-ccu.h> > #include <dt-bindings/clock/sun6i-rtc.h> > @@ -54,6 +57,7 @@ cpu0: cpu@0 { > clocks = <&ccu CLK_CPUX>; > clock-names = "cpu"; > #cooling-cells = <2>; > + operating-points-v2 = <&cpu_opp_table>; > i-cache-size = <0x8000>; > i-cache-line-size = <64>; > i-cache-sets = <256>; > @@ -71,6 +75,7 @@ cpu1: cpu@1 { > clocks = <&ccu CLK_CPUX>; > clock-names = "cpu"; > #cooling-cells = <2>; > + operating-points-v2 = <&cpu_opp_table>; > i-cache-size = <0x8000>; > i-cache-line-size = <64>; > i-cache-sets = <256>; > @@ -88,6 +93,7 @@ cpu2: cpu@2 { > clocks = <&ccu CLK_CPUX>; > clock-names = "cpu"; > #cooling-cells = <2>; > + operating-points-v2 = <&cpu_opp_table>; > i-cache-size = <0x8000>; > i-cache-line-size = <64>; > i-cache-sets = <256>; > @@ -105,6 +111,7 @@ cpu3: cpu@3 { > clocks = <&ccu CLK_CPUX>; > clock-names = "cpu"; > #cooling-cells = <2>; > + operating-points-v2 = <&cpu_opp_table>; > i-cache-size = <0x8000>; > i-cache-line-size = <64>; > i-cache-sets = <256>; > @@ -124,6 +131,59 @@ l2_cache: l2-cache { > }; > }; > > + cpu_opp_table: opp-table-cpu { > + compatible = "operating-points-v2"; > + opp-shared; > + > + opp-648000000 { > + opp-hz = /bits/ 64 <648000000>; > + opp-microvolt = <1040000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + > + opp-816000000 { > + opp-hz = /bits/ 64 <816000000>; > + opp-microvolt = <1100000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + > + opp-912000000 { > + opp-hz = /bits/ 64 <912000000>; > + opp-microvolt = <1120000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + > + opp-960000000 { > + opp-hz = /bits/ 64 <960000000>; > + opp-microvolt = <1160000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + > + opp-1008000000 { > + opp-hz = /bits/ 64 <1008000000>; > + opp-microvolt = <1200000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + > + opp-1056000000 { > + opp-hz = /bits/ 64 <1056000000>; > + opp-microvolt = <1240000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + > + opp-1104000000 { > + opp-hz = /bits/ 64 <1104000000>; > + opp-microvolt = <1260000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + > + opp-1152000000 { > + opp-hz = /bits/ 64 <1152000000>; > + opp-microvolt = <1300000>; > + clock-latency-ns = <244144>; /* 8 32k periods */ > + }; > + }; > + > de: display-engine { > compatible = "allwinner,sun50i-a64-display-engine"; > allwinner,pipelines = <&mixer0>, > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts > index ce90327e1b2e..19cb74cf1f57 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts > @@ -4,8 +4,6 @@ > /dts-v1/; > > #include "sun50i-a64.dtsi" > -#include "sun50i-a64-cpu-opp.dtsi" > - > #include <dt-bindings/gpio/gpio.h> > > / {
Hello Chen-Yu, On 2024-08-14 18:11, Chen-Yu Tsai wrote: > On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@manjaro.org> > wrote: >> >> Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and, >> consequently, >> adjust the contents of the affected board dts(i) files appropriately, >> to >> "encapsulate" the CPU OPPs into the SoC dtsi file. >> >> Moving the CPU OPPs to the SoC dtsi file, instead of requiring the >> board >> dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi >> file, >> reduces the possibility for incomplete SoC data inclusion and improves >> the >> overall hierarchical representation of data. Moreover, the CPU OPPs >> are >> not used anywhere but together with the SoC dtsi file, which >> additionally >> justifies the folding of the CPU OPPs into the SoC dtsi file. >> >> No functional changes are introduced, which was validated by >> decompiling and >> comparing all affected board dtb files before and after these changes. >> When >> compared with the decompiled original dtb files, the updated dtb files >> have >> some of their blocks shuffled around a bit and some of their phandles >> have >> different values, as a result of the changes to the order in which the >> building blocks from the parent dtsi files are included into them, but >> they >> still effectively remain the same as the originals. > > IIRC, this was a conscious decision requiring board dts files to set > their > CPU supply before OPPs are given. The bootloader does not boot the SoC > at the highest possible OPP / regulator voltage, so if the OPPs are > given > but the supply is not, the kernel will attempt to raise the frequency > beyond what the current voltage can supply, causing it to hang. > > Now that all existing boards have it properly enabled, there should be > no > need for this. However I would appreciate a second opinion. Good point, thanks for the clarification. This is quite similar to how board dts(i) files for Rockchip SoCs need to enable the SoC's built-in TSADC for temperature sensing, before the CPU thermal throttling can actually work and prevent the SoC from overheating, etc. The consensus for Rockchip boards is that it's up to the authors and reviewers of the board dts(i) files to make sure that the built-in TSADC is enabled, etc. With that approach in mind, and knowing that all Allwinner A64 board dts(i) files are in good shape when it comes to the associated voltage regulators, I think it's fine to follow the same approach of "encapsulating" the CPU OPPs into the A64 SoC dtsi file. >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >> --- >> >> Notes: >> These changes follow the general approach used for the Rockchip >> RK3588 >> and RK3399 SoCs, which was introduced and described further in >> commits >> def88eb4d836 ("arm64: dts: rockchip: Prepare RK3588 SoC dtsi files >> for >> per-variant OPPs") [1] and 296602b8e5f7 ("arm64: dts: rockchip: >> Move >> RK3399 OPPs to dtsi files for SoC variants"), [2] respectively. >> Please >> see those commits for a more detailed explanation. >> >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=def88eb4d8365a4aa064d28405d03550a9d0a3be >> [2] >> https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=296602b8e5f715d6a0ccdcd37d57170c2c81d5e4 >> >> .../allwinner/sun50i-a64-amarula-relic.dts | 2 - >> .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 2 - >> .../dts/allwinner/sun50i-a64-cpu-opp.dtsi | 75 >> ------------------- >> .../dts/allwinner/sun50i-a64-nanopi-a64.dts | 2 - >> .../dts/allwinner/sun50i-a64-olinuxino.dts | 2 - >> .../dts/allwinner/sun50i-a64-orangepi-win.dts | 2 - >> .../boot/dts/allwinner/sun50i-a64-pine64.dts | 2 - >> .../dts/allwinner/sun50i-a64-pinebook.dts | 2 - >> .../dts/allwinner/sun50i-a64-pinephone.dtsi | 2 - >> .../boot/dts/allwinner/sun50i-a64-pinetab.dts | 2 - >> .../boot/dts/allwinner/sun50i-a64-sopine.dtsi | 2 - >> .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 - >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 66 +++++++++++++++- >> .../allwinner/sun50i-h64-remix-mini-pc.dts | 2 - >> 14 files changed, 63 insertions(+), 102 deletions(-) >> delete mode 100644 >> arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >> >> diff --git >> a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >> index 8233582f6288..1590a455683f 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >> @@ -5,8 +5,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> >> / { >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >> index d1f415acd7b5..869fd4a50582 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >> @@ -4,8 +4,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> >> / { >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >> deleted file mode 100644 >> index e39db51eb448..000000000000 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >> +++ /dev/null >> @@ -1,75 +0,0 @@ >> -// SPDX-License-Identifier: GPL-2.0 >> -/* >> - * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> >> - */ >> - >> -/ { >> - cpu0_opp_table: opp-table-cpu { >> - compatible = "operating-points-v2"; >> - opp-shared; >> - >> - opp-648000000 { >> - opp-hz = /bits/ 64 <648000000>; >> - opp-microvolt = <1040000>; >> - clock-latency-ns = <244144>; /* 8 32k periods >> */ >> - }; >> - >> - opp-816000000 { >> - opp-hz = /bits/ 64 <816000000>; >> - opp-microvolt = <1100000>; >> - clock-latency-ns = <244144>; /* 8 32k periods >> */ >> - }; >> - >> - opp-912000000 { >> - opp-hz = /bits/ 64 <912000000>; >> - opp-microvolt = <1120000>; >> - clock-latency-ns = <244144>; /* 8 32k periods >> */ >> - }; >> - >> - opp-960000000 { >> - opp-hz = /bits/ 64 <960000000>; >> - opp-microvolt = <1160000>; >> - clock-latency-ns = <244144>; /* 8 32k periods >> */ >> - }; >> - >> - opp-1008000000 { >> - opp-hz = /bits/ 64 <1008000000>; >> - opp-microvolt = <1200000>; >> - clock-latency-ns = <244144>; /* 8 32k periods >> */ >> - }; >> - >> - opp-1056000000 { >> - opp-hz = /bits/ 64 <1056000000>; >> - opp-microvolt = <1240000>; >> - clock-latency-ns = <244144>; /* 8 32k periods >> */ >> - }; >> - >> - opp-1104000000 { >> - opp-hz = /bits/ 64 <1104000000>; >> - opp-microvolt = <1260000>; >> - clock-latency-ns = <244144>; /* 8 32k periods >> */ >> - }; >> - >> - opp-1152000000 { >> - opp-hz = /bits/ 64 <1152000000>; >> - opp-microvolt = <1300000>; >> - clock-latency-ns = <244144>; /* 8 32k periods >> */ >> - }; >> - }; >> -}; >> - >> -&cpu0 { >> - operating-points-v2 = <&cpu0_opp_table>; >> -}; >> - >> -&cpu1 { >> - operating-points-v2 = <&cpu0_opp_table>; >> -}; >> - >> -&cpu2 { >> - operating-points-v2 = <&cpu0_opp_table>; >> -}; >> - >> -&cpu3 { >> - operating-points-v2 = <&cpu0_opp_table>; >> -}; >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >> index dec9960a7440..e3b1943f7f63 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >> @@ -4,8 +4,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> >> / { >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >> index fd3794678c33..f1a4a9ab295b 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >> @@ -4,8 +4,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> >> / { >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >> index c8303a66438d..f3c9a3534612 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >> @@ -5,8 +5,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> >> / { >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >> index 09e71fd60785..4f65eec043d0 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >> @@ -4,8 +4,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> >> / { >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> index 379c2c8466f5..a06a0b34bd3f 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> @@ -5,8 +5,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> #include <dt-bindings/input/gpio-keys.h> >> #include <dt-bindings/input/input.h> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >> index 6eab61a12cd8..0e38cd02a90a 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >> @@ -4,8 +4,6 @@ >> // Copyright (C) 2020 Ondrej Jirman <megous@megous.com> >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> #include <dt-bindings/input/input.h> >> #include <dt-bindings/leds/common.h> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >> index f5fb1ee32dad..4a49f137885b 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >> @@ -7,8 +7,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> #include <dt-bindings/input/input.h> >> #include <dt-bindings/pwm/pwm.h> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >> index 6d78a1c98f10..4ba5c11e6870 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >> @@ -4,8 +4,6 @@ >> // Copyright (c) 2016 ARM Ltd. >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> >> &codec_analog { >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >> index b407e1dd08a7..61ccd90bae01 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >> @@ -5,8 +5,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> #include <dt-bindings/input/input.h> >> #include <dt-bindings/pwm/pwm.h> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> index e868ca5ae753..f842e64562b7 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> @@ -1,7 +1,10 @@ >> // SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> -// Copyright (C) 2016 ARM Ltd. >> -// based on the Allwinner H3 dtsi: >> -// Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> >> +/* >> + * Copyright (C) 2016 ARM Ltd. >> + * based on the Allwinner H3 dtsi: >> + * Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> >> + * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> >> + */ >> >> #include <dt-bindings/clock/sun50i-a64-ccu.h> >> #include <dt-bindings/clock/sun6i-rtc.h> >> @@ -54,6 +57,7 @@ cpu0: cpu@0 { >> clocks = <&ccu CLK_CPUX>; >> clock-names = "cpu"; >> #cooling-cells = <2>; >> + operating-points-v2 = <&cpu_opp_table>; >> i-cache-size = <0x8000>; >> i-cache-line-size = <64>; >> i-cache-sets = <256>; >> @@ -71,6 +75,7 @@ cpu1: cpu@1 { >> clocks = <&ccu CLK_CPUX>; >> clock-names = "cpu"; >> #cooling-cells = <2>; >> + operating-points-v2 = <&cpu_opp_table>; >> i-cache-size = <0x8000>; >> i-cache-line-size = <64>; >> i-cache-sets = <256>; >> @@ -88,6 +93,7 @@ cpu2: cpu@2 { >> clocks = <&ccu CLK_CPUX>; >> clock-names = "cpu"; >> #cooling-cells = <2>; >> + operating-points-v2 = <&cpu_opp_table>; >> i-cache-size = <0x8000>; >> i-cache-line-size = <64>; >> i-cache-sets = <256>; >> @@ -105,6 +111,7 @@ cpu3: cpu@3 { >> clocks = <&ccu CLK_CPUX>; >> clock-names = "cpu"; >> #cooling-cells = <2>; >> + operating-points-v2 = <&cpu_opp_table>; >> i-cache-size = <0x8000>; >> i-cache-line-size = <64>; >> i-cache-sets = <256>; >> @@ -124,6 +131,59 @@ l2_cache: l2-cache { >> }; >> }; >> >> + cpu_opp_table: opp-table-cpu { >> + compatible = "operating-points-v2"; >> + opp-shared; >> + >> + opp-648000000 { >> + opp-hz = /bits/ 64 <648000000>; >> + opp-microvolt = <1040000>; >> + clock-latency-ns = <244144>; /* 8 32k periods >> */ >> + }; >> + >> + opp-816000000 { >> + opp-hz = /bits/ 64 <816000000>; >> + opp-microvolt = <1100000>; >> + clock-latency-ns = <244144>; /* 8 32k periods >> */ >> + }; >> + >> + opp-912000000 { >> + opp-hz = /bits/ 64 <912000000>; >> + opp-microvolt = <1120000>; >> + clock-latency-ns = <244144>; /* 8 32k periods >> */ >> + }; >> + >> + opp-960000000 { >> + opp-hz = /bits/ 64 <960000000>; >> + opp-microvolt = <1160000>; >> + clock-latency-ns = <244144>; /* 8 32k periods >> */ >> + }; >> + >> + opp-1008000000 { >> + opp-hz = /bits/ 64 <1008000000>; >> + opp-microvolt = <1200000>; >> + clock-latency-ns = <244144>; /* 8 32k periods >> */ >> + }; >> + >> + opp-1056000000 { >> + opp-hz = /bits/ 64 <1056000000>; >> + opp-microvolt = <1240000>; >> + clock-latency-ns = <244144>; /* 8 32k periods >> */ >> + }; >> + >> + opp-1104000000 { >> + opp-hz = /bits/ 64 <1104000000>; >> + opp-microvolt = <1260000>; >> + clock-latency-ns = <244144>; /* 8 32k periods >> */ >> + }; >> + >> + opp-1152000000 { >> + opp-hz = /bits/ 64 <1152000000>; >> + opp-microvolt = <1300000>; >> + clock-latency-ns = <244144>; /* 8 32k periods >> */ >> + }; >> + }; >> + >> de: display-engine { >> compatible = "allwinner,sun50i-a64-display-engine"; >> allwinner,pipelines = <&mixer0>, >> diff --git >> a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >> index ce90327e1b2e..19cb74cf1f57 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >> @@ -4,8 +4,6 @@ >> /dts-v1/; >> >> #include "sun50i-a64.dtsi" >> -#include "sun50i-a64-cpu-opp.dtsi" >> - >> #include <dt-bindings/gpio/gpio.h> >> >> / {
On Thu, 15 Aug 2024 18:34:58 +0200 Dragan Simic <dsimic@manjaro.org> wrote: Hi, > On 2024-08-14 18:11, Chen-Yu Tsai wrote: > > On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@manjaro.org> > > wrote: > >> > >> Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and, > >> consequently, > >> adjust the contents of the affected board dts(i) files appropriately, > >> to > >> "encapsulate" the CPU OPPs into the SoC dtsi file. > >> > >> Moving the CPU OPPs to the SoC dtsi file, instead of requiring the > >> board > >> dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi > >> file, > >> reduces the possibility for incomplete SoC data inclusion and improves > >> the > >> overall hierarchical representation of data. Moreover, the CPU OPPs > >> are > >> not used anywhere but together with the SoC dtsi file, which > >> additionally > >> justifies the folding of the CPU OPPs into the SoC dtsi file. > >> > >> No functional changes are introduced, which was validated by > >> decompiling and > >> comparing all affected board dtb files before and after these changes. > >> When > >> compared with the decompiled original dtb files, the updated dtb files > >> have > >> some of their blocks shuffled around a bit and some of their phandles > >> have > >> different values, as a result of the changes to the order in which the > >> building blocks from the parent dtsi files are included into them, but > >> they > >> still effectively remain the same as the originals. > > > > IIRC, this was a conscious decision requiring board dts files to set > > their > > CPU supply before OPPs are given. The bootloader does not boot the SoC > > at the highest possible OPP / regulator voltage, so if the OPPs are > > given > > but the supply is not, the kernel will attempt to raise the frequency > > beyond what the current voltage can supply, causing it to hang. Yes, this is what I remember as well: this forces boards to opt in to DVFS, otherwise they get a fixed 816 MHz. Since there is only one OPP table for all boards with that SoC, I think it's reasonable to ask for this, since the cooling could not be adequate for higher frequencies in the first place, or the power supply is not up to par. > > Now that all existing boards have it properly enabled, there should be > > no > > need for this. However I would appreciate a second opinion. Well, since there is no way to opt *out* now, I am somewhat reluctant to just have this. What is the actual problem we are solving here? After all there is just one OPP table for all A64 boards, so there is less confusion about what to include in each board file. Which IIUC is a more complicated situation on the Rockchip side. I still have to try "operating-points-v2", but at least on the H616 side putting a 'status = "disabled";' into the OPP node didn't prevent it from probing. Otherwise this would have been a nice compromise, I think. > Good point, thanks for the clarification. This is quite similar to how > board dts(i) files for Rockchip SoCs need to enable the SoC's built-in > TSADC for temperature sensing, before the CPU thermal throttling can > actually work and prevent the SoC from overheating, etc. > > The consensus for Rockchip boards is that it's up to the authors and > reviewers of the board dts(i) files to make sure that the built-in TSADC > is enabled, etc. With that approach in mind, and knowing that all > Allwinner > A64 board dts(i) files are in good shape when it comes to the associated > voltage regulators, I think it's fine to follow the same approach of > "encapsulating" the CPU OPPs into the A64 SoC dtsi file. As mentioned above, I am not so sure about this. With this patch here, *every* board gets DVFS. And while this seems to be fine when looking at the current DTs in the tree (which have it anyway), it creates a potentially dangerous situation for new boards. So pragmatically speaking, this patch would be fine, but it leaves me a bit uneasy about future or downstream boards. Cheers, Andre > >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> > >> --- > >> > >> Notes: > >> These changes follow the general approach used for the Rockchip > >> RK3588 > >> and RK3399 SoCs, which was introduced and described further in > >> commits > >> def88eb4d836 ("arm64: dts: rockchip: Prepare RK3588 SoC dtsi files > >> for > >> per-variant OPPs") [1] and 296602b8e5f7 ("arm64: dts: rockchip: > >> Move > >> RK3399 OPPs to dtsi files for SoC variants"), [2] respectively. > >> Please > >> see those commits for a more detailed explanation. > >> > >> [1] > >> https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=def88eb4d8365a4aa064d28405d03550a9d0a3be > >> [2] > >> https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=296602b8e5f715d6a0ccdcd37d57170c2c81d5e4 > >> > >> .../allwinner/sun50i-a64-amarula-relic.dts | 2 - > >> .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 2 - > >> .../dts/allwinner/sun50i-a64-cpu-opp.dtsi | 75 > >> ------------------- > >> .../dts/allwinner/sun50i-a64-nanopi-a64.dts | 2 - > >> .../dts/allwinner/sun50i-a64-olinuxino.dts | 2 - > >> .../dts/allwinner/sun50i-a64-orangepi-win.dts | 2 - > >> .../boot/dts/allwinner/sun50i-a64-pine64.dts | 2 - > >> .../dts/allwinner/sun50i-a64-pinebook.dts | 2 - > >> .../dts/allwinner/sun50i-a64-pinephone.dtsi | 2 - > >> .../boot/dts/allwinner/sun50i-a64-pinetab.dts | 2 - > >> .../boot/dts/allwinner/sun50i-a64-sopine.dtsi | 2 - > >> .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 - > >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 66 +++++++++++++++- > >> .../allwinner/sun50i-h64-remix-mini-pc.dts | 2 - > >> 14 files changed, 63 insertions(+), 102 deletions(-) > >> delete mode 100644 > >> arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi > >> > >> diff --git > >> a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts > >> index 8233582f6288..1590a455683f 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts > >> @@ -5,8 +5,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> > >> / { > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts > >> index d1f415acd7b5..869fd4a50582 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts > >> @@ -4,8 +4,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> > >> / { > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi > >> deleted file mode 100644 > >> index e39db51eb448..000000000000 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi > >> +++ /dev/null > >> @@ -1,75 +0,0 @@ > >> -// SPDX-License-Identifier: GPL-2.0 > >> -/* > >> - * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> > >> - */ > >> - > >> -/ { > >> - cpu0_opp_table: opp-table-cpu { > >> - compatible = "operating-points-v2"; > >> - opp-shared; > >> - > >> - opp-648000000 { > >> - opp-hz = /bits/ 64 <648000000>; > >> - opp-microvolt = <1040000>; > >> - clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> - }; > >> - > >> - opp-816000000 { > >> - opp-hz = /bits/ 64 <816000000>; > >> - opp-microvolt = <1100000>; > >> - clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> - }; > >> - > >> - opp-912000000 { > >> - opp-hz = /bits/ 64 <912000000>; > >> - opp-microvolt = <1120000>; > >> - clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> - }; > >> - > >> - opp-960000000 { > >> - opp-hz = /bits/ 64 <960000000>; > >> - opp-microvolt = <1160000>; > >> - clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> - }; > >> - > >> - opp-1008000000 { > >> - opp-hz = /bits/ 64 <1008000000>; > >> - opp-microvolt = <1200000>; > >> - clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> - }; > >> - > >> - opp-1056000000 { > >> - opp-hz = /bits/ 64 <1056000000>; > >> - opp-microvolt = <1240000>; > >> - clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> - }; > >> - > >> - opp-1104000000 { > >> - opp-hz = /bits/ 64 <1104000000>; > >> - opp-microvolt = <1260000>; > >> - clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> - }; > >> - > >> - opp-1152000000 { > >> - opp-hz = /bits/ 64 <1152000000>; > >> - opp-microvolt = <1300000>; > >> - clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> - }; > >> - }; > >> -}; > >> - > >> -&cpu0 { > >> - operating-points-v2 = <&cpu0_opp_table>; > >> -}; > >> - > >> -&cpu1 { > >> - operating-points-v2 = <&cpu0_opp_table>; > >> -}; > >> - > >> -&cpu2 { > >> - operating-points-v2 = <&cpu0_opp_table>; > >> -}; > >> - > >> -&cpu3 { > >> - operating-points-v2 = <&cpu0_opp_table>; > >> -}; > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts > >> index dec9960a7440..e3b1943f7f63 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts > >> @@ -4,8 +4,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> > >> / { > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts > >> index fd3794678c33..f1a4a9ab295b 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts > >> @@ -4,8 +4,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> > >> / { > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts > >> index c8303a66438d..f3c9a3534612 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts > >> @@ -5,8 +5,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> > >> / { > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts > >> index 09e71fd60785..4f65eec043d0 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts > >> @@ -4,8 +4,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> > >> / { > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > >> index 379c2c8466f5..a06a0b34bd3f 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > >> @@ -5,8 +5,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> #include <dt-bindings/input/gpio-keys.h> > >> #include <dt-bindings/input/input.h> > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > >> index 6eab61a12cd8..0e38cd02a90a 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > >> @@ -4,8 +4,6 @@ > >> // Copyright (C) 2020 Ondrej Jirman <megous@megous.com> > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> #include <dt-bindings/input/input.h> > >> #include <dt-bindings/leds/common.h> > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts > >> index f5fb1ee32dad..4a49f137885b 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts > >> @@ -7,8 +7,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> #include <dt-bindings/input/input.h> > >> #include <dt-bindings/pwm/pwm.h> > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > >> index 6d78a1c98f10..4ba5c11e6870 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > >> @@ -4,8 +4,6 @@ > >> // Copyright (c) 2016 ARM Ltd. > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> > >> &codec_analog { > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > >> index b407e1dd08a7..61ccd90bae01 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > >> @@ -5,8 +5,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> #include <dt-bindings/input/input.h> > >> #include <dt-bindings/pwm/pwm.h> > >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > >> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > >> index e868ca5ae753..f842e64562b7 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi > >> @@ -1,7 +1,10 @@ > >> // SPDX-License-Identifier: (GPL-2.0+ OR MIT) > >> -// Copyright (C) 2016 ARM Ltd. > >> -// based on the Allwinner H3 dtsi: > >> -// Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> > >> +/* > >> + * Copyright (C) 2016 ARM Ltd. > >> + * based on the Allwinner H3 dtsi: > >> + * Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> > >> + * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> > >> + */ > >> > >> #include <dt-bindings/clock/sun50i-a64-ccu.h> > >> #include <dt-bindings/clock/sun6i-rtc.h> > >> @@ -54,6 +57,7 @@ cpu0: cpu@0 { > >> clocks = <&ccu CLK_CPUX>; > >> clock-names = "cpu"; > >> #cooling-cells = <2>; > >> + operating-points-v2 = <&cpu_opp_table>; > >> i-cache-size = <0x8000>; > >> i-cache-line-size = <64>; > >> i-cache-sets = <256>; > >> @@ -71,6 +75,7 @@ cpu1: cpu@1 { > >> clocks = <&ccu CLK_CPUX>; > >> clock-names = "cpu"; > >> #cooling-cells = <2>; > >> + operating-points-v2 = <&cpu_opp_table>; > >> i-cache-size = <0x8000>; > >> i-cache-line-size = <64>; > >> i-cache-sets = <256>; > >> @@ -88,6 +93,7 @@ cpu2: cpu@2 { > >> clocks = <&ccu CLK_CPUX>; > >> clock-names = "cpu"; > >> #cooling-cells = <2>; > >> + operating-points-v2 = <&cpu_opp_table>; > >> i-cache-size = <0x8000>; > >> i-cache-line-size = <64>; > >> i-cache-sets = <256>; > >> @@ -105,6 +111,7 @@ cpu3: cpu@3 { > >> clocks = <&ccu CLK_CPUX>; > >> clock-names = "cpu"; > >> #cooling-cells = <2>; > >> + operating-points-v2 = <&cpu_opp_table>; > >> i-cache-size = <0x8000>; > >> i-cache-line-size = <64>; > >> i-cache-sets = <256>; > >> @@ -124,6 +131,59 @@ l2_cache: l2-cache { > >> }; > >> }; > >> > >> + cpu_opp_table: opp-table-cpu { > >> + compatible = "operating-points-v2"; > >> + opp-shared; > >> + > >> + opp-648000000 { > >> + opp-hz = /bits/ 64 <648000000>; > >> + opp-microvolt = <1040000>; > >> + clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> + }; > >> + > >> + opp-816000000 { > >> + opp-hz = /bits/ 64 <816000000>; > >> + opp-microvolt = <1100000>; > >> + clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> + }; > >> + > >> + opp-912000000 { > >> + opp-hz = /bits/ 64 <912000000>; > >> + opp-microvolt = <1120000>; > >> + clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> + }; > >> + > >> + opp-960000000 { > >> + opp-hz = /bits/ 64 <960000000>; > >> + opp-microvolt = <1160000>; > >> + clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> + }; > >> + > >> + opp-1008000000 { > >> + opp-hz = /bits/ 64 <1008000000>; > >> + opp-microvolt = <1200000>; > >> + clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> + }; > >> + > >> + opp-1056000000 { > >> + opp-hz = /bits/ 64 <1056000000>; > >> + opp-microvolt = <1240000>; > >> + clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> + }; > >> + > >> + opp-1104000000 { > >> + opp-hz = /bits/ 64 <1104000000>; > >> + opp-microvolt = <1260000>; > >> + clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> + }; > >> + > >> + opp-1152000000 { > >> + opp-hz = /bits/ 64 <1152000000>; > >> + opp-microvolt = <1300000>; > >> + clock-latency-ns = <244144>; /* 8 32k periods > >> */ > >> + }; > >> + }; > >> + > >> de: display-engine { > >> compatible = "allwinner,sun50i-a64-display-engine"; > >> allwinner,pipelines = <&mixer0>, > >> diff --git > >> a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts > >> b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts > >> index ce90327e1b2e..19cb74cf1f57 100644 > >> --- a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts > >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts > >> @@ -4,8 +4,6 @@ > >> /dts-v1/; > >> > >> #include "sun50i-a64.dtsi" > >> -#include "sun50i-a64-cpu-opp.dtsi" > >> - > >> #include <dt-bindings/gpio/gpio.h> > >> > >> / { >
Hello Andre, On 2024-08-15 19:15, Andre Przywara wrote: > On Thu, 15 Aug 2024 18:34:58 +0200 > Dragan Simic <dsimic@manjaro.org> wrote: >> On 2024-08-14 18:11, Chen-Yu Tsai wrote: >> > On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@manjaro.org> >> > wrote: >> >> >> >> Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and, >> >> consequently, >> >> adjust the contents of the affected board dts(i) files appropriately, >> >> to >> >> "encapsulate" the CPU OPPs into the SoC dtsi file. >> >> >> >> Moving the CPU OPPs to the SoC dtsi file, instead of requiring the >> >> board >> >> dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi >> >> file, >> >> reduces the possibility for incomplete SoC data inclusion and improves >> >> the >> >> overall hierarchical representation of data. Moreover, the CPU OPPs >> >> are >> >> not used anywhere but together with the SoC dtsi file, which >> >> additionally >> >> justifies the folding of the CPU OPPs into the SoC dtsi file. >> >> >> >> No functional changes are introduced, which was validated by >> >> decompiling and >> >> comparing all affected board dtb files before and after these changes. >> >> When >> >> compared with the decompiled original dtb files, the updated dtb files >> >> have >> >> some of their blocks shuffled around a bit and some of their phandles >> >> have >> >> different values, as a result of the changes to the order in which the >> >> building blocks from the parent dtsi files are included into them, but >> >> they >> >> still effectively remain the same as the originals. >> > >> > IIRC, this was a conscious decision requiring board dts files to set >> > their >> > CPU supply before OPPs are given. The bootloader does not boot the SoC >> > at the highest possible OPP / regulator voltage, so if the OPPs are >> > given >> > but the supply is not, the kernel will attempt to raise the frequency >> > beyond what the current voltage can supply, causing it to hang. > > Yes, this is what I remember as well: this forces boards to opt in to > DVFS, otherwise they get a fixed 816 MHz. Since there is only one OPP > table for all boards with that SoC, I think it's reasonable to ask for > this, since the cooling could not be adequate for higher frequencies in > the first place, or the power supply is not up to par. If the cooling isn't capable enough to dissipate the additional heat generated at higher frequencies, the thermal governor is there to handle that by lowering the operating frequency. If the PSU isn't capable to provide an additional watt or two, I think a better PSU is needed. :) No reasonably sized PSU should work at ~100% of its power output. On top of that, all currently supported A64-based boards have the CPU OPPs defined and CPU DVFS enabled, so no such issues are possible there. Though, there could be some issues with new A64-based boards, which is discussed further below. >> > Now that all existing boards have it properly enabled, there should be >> > no >> > need for this. However I would appreciate a second opinion. > > Well, since there is no way to opt *out* now, I am somewhat reluctant > to > just have this. What is the actual problem we are solving here? After > all > there is just one OPP table for all A64 boards, so there is less > confusion > about what to include in each board file. Which IIUC is a more > complicated > situation on the Rockchip side. Well, this patch doesn't solve some real problem, but it makes the things neater and a bit more clean. The things are more complicated with Rockchip SoCs, but following the concept of "encapsulating" the CPU OPPs into the A64 SoC dtsi makes things neater. Moreover, the A64 GPU OPPs are already in the A64 SoC dtsi, so we could also say that folding the A64 CPU OPPs into the SoC dtsi follows the A64 GPU OPPs. > I still have to try "operating-points-v2", but at least on the H616 > side > putting a 'status = "disabled";' into the OPP node didn't prevent it > from > probing. Otherwise this would have been a nice compromise, I think. > >> Good point, thanks for the clarification. This is quite similar to >> how >> board dts(i) files for Rockchip SoCs need to enable the SoC's built-in >> TSADC for temperature sensing, before the CPU thermal throttling can >> actually work and prevent the SoC from overheating, etc. >> >> The consensus for Rockchip boards is that it's up to the authors and >> reviewers of the board dts(i) files to make sure that the built-in >> TSADC >> is enabled, etc. With that approach in mind, and knowing that all >> Allwinner >> A64 board dts(i) files are in good shape when it comes to the >> associated >> voltage regulators, I think it's fine to follow the same approach of >> "encapsulating" the CPU OPPs into the A64 SoC dtsi file. > > As mentioned above, I am not so sure about this. With this patch here, > *every* board gets DVFS. And while this seems to be fine when looking > at > the current DTs in the tree (which have it anyway), it creates a > potentially dangerous situation for new boards. > > So pragmatically speaking, this patch would be fine, but it leaves me a > bit uneasy about future or downstream boards. Frankly, I wouldn't be worried about that. When a new A64-based board is added, it should be verified that CPU DVFS works as expected, etc., before the new board dts file is accepted upstream. Maybe we could take into account some possible issues when someone starts putting together a new A64-based board dts file, but there are already many dangerous things that someone can do in the process, such as messing up various regulators and voltages unrelated to the CPU DVFS, so everyone putting a new board dts file together simply have to know what are they doing. I see no way for escaping from that need. >> >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >> >> --- >> >> >> >> Notes: >> >> These changes follow the general approach used for the Rockchip >> >> RK3588 >> >> and RK3399 SoCs, which was introduced and described further in >> >> commits >> >> def88eb4d836 ("arm64: dts: rockchip: Prepare RK3588 SoC dtsi files >> >> for >> >> per-variant OPPs") [1] and 296602b8e5f7 ("arm64: dts: rockchip: >> >> Move >> >> RK3399 OPPs to dtsi files for SoC variants"), [2] respectively. >> >> Please >> >> see those commits for a more detailed explanation. >> >> >> >> [1] >> >> https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=def88eb4d8365a4aa064d28405d03550a9d0a3be >> >> [2] >> >> https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=296602b8e5f715d6a0ccdcd37d57170c2c81d5e4 >> >> >> >> .../allwinner/sun50i-a64-amarula-relic.dts | 2 - >> >> .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 2 - >> >> .../dts/allwinner/sun50i-a64-cpu-opp.dtsi | 75 >> >> ------------------- >> >> .../dts/allwinner/sun50i-a64-nanopi-a64.dts | 2 - >> >> .../dts/allwinner/sun50i-a64-olinuxino.dts | 2 - >> >> .../dts/allwinner/sun50i-a64-orangepi-win.dts | 2 - >> >> .../boot/dts/allwinner/sun50i-a64-pine64.dts | 2 - >> >> .../dts/allwinner/sun50i-a64-pinebook.dts | 2 - >> >> .../dts/allwinner/sun50i-a64-pinephone.dtsi | 2 - >> >> .../boot/dts/allwinner/sun50i-a64-pinetab.dts | 2 - >> >> .../boot/dts/allwinner/sun50i-a64-sopine.dtsi | 2 - >> >> .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 - >> >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 66 +++++++++++++++- >> >> .../allwinner/sun50i-h64-remix-mini-pc.dts | 2 - >> >> 14 files changed, 63 insertions(+), 102 deletions(-) >> >> delete mode 100644 >> >> arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >> >> >> >> diff --git >> >> a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >> >> index 8233582f6288..1590a455683f 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >> >> @@ -5,8 +5,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> >> >> / { >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >> >> index d1f415acd7b5..869fd4a50582 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >> >> @@ -4,8 +4,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> >> >> / { >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >> >> deleted file mode 100644 >> >> index e39db51eb448..000000000000 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >> >> +++ /dev/null >> >> @@ -1,75 +0,0 @@ >> >> -// SPDX-License-Identifier: GPL-2.0 >> >> -/* >> >> - * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> >> >> - */ >> >> - >> >> -/ { >> >> - cpu0_opp_table: opp-table-cpu { >> >> - compatible = "operating-points-v2"; >> >> - opp-shared; >> >> - >> >> - opp-648000000 { >> >> - opp-hz = /bits/ 64 <648000000>; >> >> - opp-microvolt = <1040000>; >> >> - clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> - }; >> >> - >> >> - opp-816000000 { >> >> - opp-hz = /bits/ 64 <816000000>; >> >> - opp-microvolt = <1100000>; >> >> - clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> - }; >> >> - >> >> - opp-912000000 { >> >> - opp-hz = /bits/ 64 <912000000>; >> >> - opp-microvolt = <1120000>; >> >> - clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> - }; >> >> - >> >> - opp-960000000 { >> >> - opp-hz = /bits/ 64 <960000000>; >> >> - opp-microvolt = <1160000>; >> >> - clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> - }; >> >> - >> >> - opp-1008000000 { >> >> - opp-hz = /bits/ 64 <1008000000>; >> >> - opp-microvolt = <1200000>; >> >> - clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> - }; >> >> - >> >> - opp-1056000000 { >> >> - opp-hz = /bits/ 64 <1056000000>; >> >> - opp-microvolt = <1240000>; >> >> - clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> - }; >> >> - >> >> - opp-1104000000 { >> >> - opp-hz = /bits/ 64 <1104000000>; >> >> - opp-microvolt = <1260000>; >> >> - clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> - }; >> >> - >> >> - opp-1152000000 { >> >> - opp-hz = /bits/ 64 <1152000000>; >> >> - opp-microvolt = <1300000>; >> >> - clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> - }; >> >> - }; >> >> -}; >> >> - >> >> -&cpu0 { >> >> - operating-points-v2 = <&cpu0_opp_table>; >> >> -}; >> >> - >> >> -&cpu1 { >> >> - operating-points-v2 = <&cpu0_opp_table>; >> >> -}; >> >> - >> >> -&cpu2 { >> >> - operating-points-v2 = <&cpu0_opp_table>; >> >> -}; >> >> - >> >> -&cpu3 { >> >> - operating-points-v2 = <&cpu0_opp_table>; >> >> -}; >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >> >> index dec9960a7440..e3b1943f7f63 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >> >> @@ -4,8 +4,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> >> >> / { >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >> >> index fd3794678c33..f1a4a9ab295b 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >> >> @@ -4,8 +4,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> >> >> / { >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >> >> index c8303a66438d..f3c9a3534612 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >> >> @@ -5,8 +5,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> >> >> / { >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >> >> index 09e71fd60785..4f65eec043d0 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >> >> @@ -4,8 +4,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> >> >> / { >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> >> index 379c2c8466f5..a06a0b34bd3f 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> >> @@ -5,8 +5,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> #include <dt-bindings/input/gpio-keys.h> >> >> #include <dt-bindings/input/input.h> >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >> >> index 6eab61a12cd8..0e38cd02a90a 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >> >> @@ -4,8 +4,6 @@ >> >> // Copyright (C) 2020 Ondrej Jirman <megous@megous.com> >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> #include <dt-bindings/input/input.h> >> >> #include <dt-bindings/leds/common.h> >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >> >> index f5fb1ee32dad..4a49f137885b 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >> >> @@ -7,8 +7,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> #include <dt-bindings/input/input.h> >> >> #include <dt-bindings/pwm/pwm.h> >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >> >> index 6d78a1c98f10..4ba5c11e6870 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >> >> @@ -4,8 +4,6 @@ >> >> // Copyright (c) 2016 ARM Ltd. >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> >> >> &codec_analog { >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >> >> index b407e1dd08a7..61ccd90bae01 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >> >> @@ -5,8 +5,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> #include <dt-bindings/input/input.h> >> >> #include <dt-bindings/pwm/pwm.h> >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> >> index e868ca5ae753..f842e64562b7 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> >> @@ -1,7 +1,10 @@ >> >> // SPDX-License-Identifier: (GPL-2.0+ OR MIT) >> >> -// Copyright (C) 2016 ARM Ltd. >> >> -// based on the Allwinner H3 dtsi: >> >> -// Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> >> >> +/* >> >> + * Copyright (C) 2016 ARM Ltd. >> >> + * based on the Allwinner H3 dtsi: >> >> + * Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> >> >> + * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> >> >> + */ >> >> >> >> #include <dt-bindings/clock/sun50i-a64-ccu.h> >> >> #include <dt-bindings/clock/sun6i-rtc.h> >> >> @@ -54,6 +57,7 @@ cpu0: cpu@0 { >> >> clocks = <&ccu CLK_CPUX>; >> >> clock-names = "cpu"; >> >> #cooling-cells = <2>; >> >> + operating-points-v2 = <&cpu_opp_table>; >> >> i-cache-size = <0x8000>; >> >> i-cache-line-size = <64>; >> >> i-cache-sets = <256>; >> >> @@ -71,6 +75,7 @@ cpu1: cpu@1 { >> >> clocks = <&ccu CLK_CPUX>; >> >> clock-names = "cpu"; >> >> #cooling-cells = <2>; >> >> + operating-points-v2 = <&cpu_opp_table>; >> >> i-cache-size = <0x8000>; >> >> i-cache-line-size = <64>; >> >> i-cache-sets = <256>; >> >> @@ -88,6 +93,7 @@ cpu2: cpu@2 { >> >> clocks = <&ccu CLK_CPUX>; >> >> clock-names = "cpu"; >> >> #cooling-cells = <2>; >> >> + operating-points-v2 = <&cpu_opp_table>; >> >> i-cache-size = <0x8000>; >> >> i-cache-line-size = <64>; >> >> i-cache-sets = <256>; >> >> @@ -105,6 +111,7 @@ cpu3: cpu@3 { >> >> clocks = <&ccu CLK_CPUX>; >> >> clock-names = "cpu"; >> >> #cooling-cells = <2>; >> >> + operating-points-v2 = <&cpu_opp_table>; >> >> i-cache-size = <0x8000>; >> >> i-cache-line-size = <64>; >> >> i-cache-sets = <256>; >> >> @@ -124,6 +131,59 @@ l2_cache: l2-cache { >> >> }; >> >> }; >> >> >> >> + cpu_opp_table: opp-table-cpu { >> >> + compatible = "operating-points-v2"; >> >> + opp-shared; >> >> + >> >> + opp-648000000 { >> >> + opp-hz = /bits/ 64 <648000000>; >> >> + opp-microvolt = <1040000>; >> >> + clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> + }; >> >> + >> >> + opp-816000000 { >> >> + opp-hz = /bits/ 64 <816000000>; >> >> + opp-microvolt = <1100000>; >> >> + clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> + }; >> >> + >> >> + opp-912000000 { >> >> + opp-hz = /bits/ 64 <912000000>; >> >> + opp-microvolt = <1120000>; >> >> + clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> + }; >> >> + >> >> + opp-960000000 { >> >> + opp-hz = /bits/ 64 <960000000>; >> >> + opp-microvolt = <1160000>; >> >> + clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> + }; >> >> + >> >> + opp-1008000000 { >> >> + opp-hz = /bits/ 64 <1008000000>; >> >> + opp-microvolt = <1200000>; >> >> + clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> + }; >> >> + >> >> + opp-1056000000 { >> >> + opp-hz = /bits/ 64 <1056000000>; >> >> + opp-microvolt = <1240000>; >> >> + clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> + }; >> >> + >> >> + opp-1104000000 { >> >> + opp-hz = /bits/ 64 <1104000000>; >> >> + opp-microvolt = <1260000>; >> >> + clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> + }; >> >> + >> >> + opp-1152000000 { >> >> + opp-hz = /bits/ 64 <1152000000>; >> >> + opp-microvolt = <1300000>; >> >> + clock-latency-ns = <244144>; /* 8 32k periods >> >> */ >> >> + }; >> >> + }; >> >> + >> >> de: display-engine { >> >> compatible = "allwinner,sun50i-a64-display-engine"; >> >> allwinner,pipelines = <&mixer0>, >> >> diff --git >> >> a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >> >> b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >> >> index ce90327e1b2e..19cb74cf1f57 100644 >> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >> >> @@ -4,8 +4,6 @@ >> >> /dts-v1/; >> >> >> >> #include "sun50i-a64.dtsi" >> >> -#include "sun50i-a64-cpu-opp.dtsi" >> >> - >> >> #include <dt-bindings/gpio/gpio.h> >> >> >> >> / { >>
Hello, Just checking, any further thoughts about this patch? On 2024-08-17 06:25, Dragan Simic wrote: > Hello Andre, > > On 2024-08-15 19:15, Andre Przywara wrote: >> On Thu, 15 Aug 2024 18:34:58 +0200 >> Dragan Simic <dsimic@manjaro.org> wrote: >>> On 2024-08-14 18:11, Chen-Yu Tsai wrote: >>> > On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@manjaro.org> >>> > wrote: >>> >> >>> >> Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and, >>> >> consequently, >>> >> adjust the contents of the affected board dts(i) files appropriately, >>> >> to >>> >> "encapsulate" the CPU OPPs into the SoC dtsi file. >>> >> >>> >> Moving the CPU OPPs to the SoC dtsi file, instead of requiring the >>> >> board >>> >> dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi >>> >> file, >>> >> reduces the possibility for incomplete SoC data inclusion and improves >>> >> the >>> >> overall hierarchical representation of data. Moreover, the CPU OPPs >>> >> are >>> >> not used anywhere but together with the SoC dtsi file, which >>> >> additionally >>> >> justifies the folding of the CPU OPPs into the SoC dtsi file. >>> >> >>> >> No functional changes are introduced, which was validated by >>> >> decompiling and >>> >> comparing all affected board dtb files before and after these changes. >>> >> When >>> >> compared with the decompiled original dtb files, the updated dtb files >>> >> have >>> >> some of their blocks shuffled around a bit and some of their phandles >>> >> have >>> >> different values, as a result of the changes to the order in which the >>> >> building blocks from the parent dtsi files are included into them, but >>> >> they >>> >> still effectively remain the same as the originals. >>> > >>> > IIRC, this was a conscious decision requiring board dts files to set >>> > their >>> > CPU supply before OPPs are given. The bootloader does not boot the SoC >>> > at the highest possible OPP / regulator voltage, so if the OPPs are >>> > given >>> > but the supply is not, the kernel will attempt to raise the frequency >>> > beyond what the current voltage can supply, causing it to hang. >> >> Yes, this is what I remember as well: this forces boards to opt in to >> DVFS, otherwise they get a fixed 816 MHz. Since there is only one OPP >> table for all boards with that SoC, I think it's reasonable to ask for >> this, since the cooling could not be adequate for higher frequencies >> in >> the first place, or the power supply is not up to par. > > If the cooling isn't capable enough to dissipate the additional heat > generated at higher frequencies, the thermal governor is there to > handle > that by lowering the operating frequency. If the PSU isn't capable to > provide an additional watt or two, I think a better PSU is needed. :) > No reasonably sized PSU should work at ~100% of its power output. > > On top of that, all currently supported A64-based boards have the CPU > OPPs defined and CPU DVFS enabled, so no such issues are possible > there. > Though, there could be some issues with new A64-based boards, which is > discussed further below. > >>> > Now that all existing boards have it properly enabled, there should be >>> > no >>> > need for this. However I would appreciate a second opinion. >> >> Well, since there is no way to opt *out* now, I am somewhat reluctant >> to >> just have this. What is the actual problem we are solving here? After >> all >> there is just one OPP table for all A64 boards, so there is less >> confusion >> about what to include in each board file. Which IIUC is a more >> complicated >> situation on the Rockchip side. > > Well, this patch doesn't solve some real problem, but it makes the > things > neater and a bit more clean. The things are more complicated with > Rockchip > SoCs, but following the concept of "encapsulating" the CPU OPPs into > the > A64 SoC dtsi makes things neater. Moreover, the A64 GPU OPPs are > already > in the A64 SoC dtsi, so we could also say that folding the A64 CPU OPPs > into the SoC dtsi follows the A64 GPU OPPs. > >> I still have to try "operating-points-v2", but at least on the H616 >> side >> putting a 'status = "disabled";' into the OPP node didn't prevent it >> from >> probing. Otherwise this would have been a nice compromise, I think. >> >>> Good point, thanks for the clarification. This is quite similar to >>> how >>> board dts(i) files for Rockchip SoCs need to enable the SoC's >>> built-in >>> TSADC for temperature sensing, before the CPU thermal throttling can >>> actually work and prevent the SoC from overheating, etc. >>> >>> The consensus for Rockchip boards is that it's up to the authors and >>> reviewers of the board dts(i) files to make sure that the built-in >>> TSADC >>> is enabled, etc. With that approach in mind, and knowing that all >>> Allwinner >>> A64 board dts(i) files are in good shape when it comes to the >>> associated >>> voltage regulators, I think it's fine to follow the same approach of >>> "encapsulating" the CPU OPPs into the A64 SoC dtsi file. >> >> As mentioned above, I am not so sure about this. With this patch here, >> *every* board gets DVFS. And while this seems to be fine when looking >> at >> the current DTs in the tree (which have it anyway), it creates a >> potentially dangerous situation for new boards. >> >> So pragmatically speaking, this patch would be fine, but it leaves me >> a >> bit uneasy about future or downstream boards. > > Frankly, I wouldn't be worried about that. When a new A64-based board > is added, it should be verified that CPU DVFS works as expected, etc., > before the new board dts file is accepted upstream. > > Maybe we could take into account some possible issues when someone > starts > putting together a new A64-based board dts file, but there are already > many dangerous things that someone can do in the process, such as > messing > up various regulators and voltages unrelated to the CPU DVFS, so > everyone > putting a new board dts file together simply have to know what are they > doing. I see no way for escaping from that need. > >>> >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >>> >> --- >>> >> >>> >> Notes: >>> >> These changes follow the general approach used for the Rockchip >>> >> RK3588 >>> >> and RK3399 SoCs, which was introduced and described further in >>> >> commits >>> >> def88eb4d836 ("arm64: dts: rockchip: Prepare RK3588 SoC dtsi files >>> >> for >>> >> per-variant OPPs") [1] and 296602b8e5f7 ("arm64: dts: rockchip: >>> >> Move >>> >> RK3399 OPPs to dtsi files for SoC variants"), [2] respectively. >>> >> Please >>> >> see those commits for a more detailed explanation. >>> >> >>> >> [1] >>> >> https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=def88eb4d8365a4aa064d28405d03550a9d0a3be >>> >> [2] >>> >> https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=296602b8e5f715d6a0ccdcd37d57170c2c81d5e4 >>> >> >>> >> .../allwinner/sun50i-a64-amarula-relic.dts | 2 - >>> >> .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 2 - >>> >> .../dts/allwinner/sun50i-a64-cpu-opp.dtsi | 75 >>> >> ------------------- >>> >> .../dts/allwinner/sun50i-a64-nanopi-a64.dts | 2 - >>> >> .../dts/allwinner/sun50i-a64-olinuxino.dts | 2 - >>> >> .../dts/allwinner/sun50i-a64-orangepi-win.dts | 2 - >>> >> .../boot/dts/allwinner/sun50i-a64-pine64.dts | 2 - >>> >> .../dts/allwinner/sun50i-a64-pinebook.dts | 2 - >>> >> .../dts/allwinner/sun50i-a64-pinephone.dtsi | 2 - >>> >> .../boot/dts/allwinner/sun50i-a64-pinetab.dts | 2 - >>> >> .../boot/dts/allwinner/sun50i-a64-sopine.dtsi | 2 - >>> >> .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 - >>> >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 66 +++++++++++++++- >>> >> .../allwinner/sun50i-h64-remix-mini-pc.dts | 2 - >>> >> 14 files changed, 63 insertions(+), 102 deletions(-) >>> >> delete mode 100644 >>> >> arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >>> >> >>> >> diff --git >>> >> a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >>> >> index 8233582f6288..1590a455683f 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts >>> >> @@ -5,8 +5,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> >>> >> / { >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >>> >> index d1f415acd7b5..869fd4a50582 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts >>> >> @@ -4,8 +4,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> >>> >> / { >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >>> >> deleted file mode 100644 >>> >> index e39db51eb448..000000000000 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi >>> >> +++ /dev/null >>> >> @@ -1,75 +0,0 @@ >>> >> -// SPDX-License-Identifier: GPL-2.0 >>> >> -/* >>> >> - * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> >>> >> - */ >>> >> - >>> >> -/ { >>> >> - cpu0_opp_table: opp-table-cpu { >>> >> - compatible = "operating-points-v2"; >>> >> - opp-shared; >>> >> - >>> >> - opp-648000000 { >>> >> - opp-hz = /bits/ 64 <648000000>; >>> >> - opp-microvolt = <1040000>; >>> >> - clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> - }; >>> >> - >>> >> - opp-816000000 { >>> >> - opp-hz = /bits/ 64 <816000000>; >>> >> - opp-microvolt = <1100000>; >>> >> - clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> - }; >>> >> - >>> >> - opp-912000000 { >>> >> - opp-hz = /bits/ 64 <912000000>; >>> >> - opp-microvolt = <1120000>; >>> >> - clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> - }; >>> >> - >>> >> - opp-960000000 { >>> >> - opp-hz = /bits/ 64 <960000000>; >>> >> - opp-microvolt = <1160000>; >>> >> - clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> - }; >>> >> - >>> >> - opp-1008000000 { >>> >> - opp-hz = /bits/ 64 <1008000000>; >>> >> - opp-microvolt = <1200000>; >>> >> - clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> - }; >>> >> - >>> >> - opp-1056000000 { >>> >> - opp-hz = /bits/ 64 <1056000000>; >>> >> - opp-microvolt = <1240000>; >>> >> - clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> - }; >>> >> - >>> >> - opp-1104000000 { >>> >> - opp-hz = /bits/ 64 <1104000000>; >>> >> - opp-microvolt = <1260000>; >>> >> - clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> - }; >>> >> - >>> >> - opp-1152000000 { >>> >> - opp-hz = /bits/ 64 <1152000000>; >>> >> - opp-microvolt = <1300000>; >>> >> - clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> - }; >>> >> - }; >>> >> -}; >>> >> - >>> >> -&cpu0 { >>> >> - operating-points-v2 = <&cpu0_opp_table>; >>> >> -}; >>> >> - >>> >> -&cpu1 { >>> >> - operating-points-v2 = <&cpu0_opp_table>; >>> >> -}; >>> >> - >>> >> -&cpu2 { >>> >> - operating-points-v2 = <&cpu0_opp_table>; >>> >> -}; >>> >> - >>> >> -&cpu3 { >>> >> - operating-points-v2 = <&cpu0_opp_table>; >>> >> -}; >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >>> >> index dec9960a7440..e3b1943f7f63 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts >>> >> @@ -4,8 +4,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> >>> >> / { >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >>> >> index fd3794678c33..f1a4a9ab295b 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts >>> >> @@ -4,8 +4,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> >>> >> / { >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >>> >> index c8303a66438d..f3c9a3534612 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts >>> >> @@ -5,8 +5,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> >>> >> / { >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >>> >> index 09e71fd60785..4f65eec043d0 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts >>> >> @@ -4,8 +4,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> >>> >> / { >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >>> >> index 379c2c8466f5..a06a0b34bd3f 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >>> >> @@ -5,8 +5,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> #include <dt-bindings/input/gpio-keys.h> >>> >> #include <dt-bindings/input/input.h> >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >>> >> index 6eab61a12cd8..0e38cd02a90a 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi >>> >> @@ -4,8 +4,6 @@ >>> >> // Copyright (C) 2020 Ondrej Jirman <megous@megous.com> >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> #include <dt-bindings/input/input.h> >>> >> #include <dt-bindings/leds/common.h> >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >>> >> index f5fb1ee32dad..4a49f137885b 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts >>> >> @@ -7,8 +7,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> #include <dt-bindings/input/input.h> >>> >> #include <dt-bindings/pwm/pwm.h> >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >>> >> index 6d78a1c98f10..4ba5c11e6870 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi >>> >> @@ -4,8 +4,6 @@ >>> >> // Copyright (c) 2016 ARM Ltd. >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> >>> >> &codec_analog { >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >>> >> index b407e1dd08a7..61ccd90bae01 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts >>> >> @@ -5,8 +5,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> #include <dt-bindings/input/input.h> >>> >> #include <dt-bindings/pwm/pwm.h> >>> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >>> >> index e868ca5ae753..f842e64562b7 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >>> >> @@ -1,7 +1,10 @@ >>> >> // SPDX-License-Identifier: (GPL-2.0+ OR MIT) >>> >> -// Copyright (C) 2016 ARM Ltd. >>> >> -// based on the Allwinner H3 dtsi: >>> >> -// Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> >>> >> +/* >>> >> + * Copyright (C) 2016 ARM Ltd. >>> >> + * based on the Allwinner H3 dtsi: >>> >> + * Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> >>> >> + * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> >>> >> + */ >>> >> >>> >> #include <dt-bindings/clock/sun50i-a64-ccu.h> >>> >> #include <dt-bindings/clock/sun6i-rtc.h> >>> >> @@ -54,6 +57,7 @@ cpu0: cpu@0 { >>> >> clocks = <&ccu CLK_CPUX>; >>> >> clock-names = "cpu"; >>> >> #cooling-cells = <2>; >>> >> + operating-points-v2 = <&cpu_opp_table>; >>> >> i-cache-size = <0x8000>; >>> >> i-cache-line-size = <64>; >>> >> i-cache-sets = <256>; >>> >> @@ -71,6 +75,7 @@ cpu1: cpu@1 { >>> >> clocks = <&ccu CLK_CPUX>; >>> >> clock-names = "cpu"; >>> >> #cooling-cells = <2>; >>> >> + operating-points-v2 = <&cpu_opp_table>; >>> >> i-cache-size = <0x8000>; >>> >> i-cache-line-size = <64>; >>> >> i-cache-sets = <256>; >>> >> @@ -88,6 +93,7 @@ cpu2: cpu@2 { >>> >> clocks = <&ccu CLK_CPUX>; >>> >> clock-names = "cpu"; >>> >> #cooling-cells = <2>; >>> >> + operating-points-v2 = <&cpu_opp_table>; >>> >> i-cache-size = <0x8000>; >>> >> i-cache-line-size = <64>; >>> >> i-cache-sets = <256>; >>> >> @@ -105,6 +111,7 @@ cpu3: cpu@3 { >>> >> clocks = <&ccu CLK_CPUX>; >>> >> clock-names = "cpu"; >>> >> #cooling-cells = <2>; >>> >> + operating-points-v2 = <&cpu_opp_table>; >>> >> i-cache-size = <0x8000>; >>> >> i-cache-line-size = <64>; >>> >> i-cache-sets = <256>; >>> >> @@ -124,6 +131,59 @@ l2_cache: l2-cache { >>> >> }; >>> >> }; >>> >> >>> >> + cpu_opp_table: opp-table-cpu { >>> >> + compatible = "operating-points-v2"; >>> >> + opp-shared; >>> >> + >>> >> + opp-648000000 { >>> >> + opp-hz = /bits/ 64 <648000000>; >>> >> + opp-microvolt = <1040000>; >>> >> + clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> + }; >>> >> + >>> >> + opp-816000000 { >>> >> + opp-hz = /bits/ 64 <816000000>; >>> >> + opp-microvolt = <1100000>; >>> >> + clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> + }; >>> >> + >>> >> + opp-912000000 { >>> >> + opp-hz = /bits/ 64 <912000000>; >>> >> + opp-microvolt = <1120000>; >>> >> + clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> + }; >>> >> + >>> >> + opp-960000000 { >>> >> + opp-hz = /bits/ 64 <960000000>; >>> >> + opp-microvolt = <1160000>; >>> >> + clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> + }; >>> >> + >>> >> + opp-1008000000 { >>> >> + opp-hz = /bits/ 64 <1008000000>; >>> >> + opp-microvolt = <1200000>; >>> >> + clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> + }; >>> >> + >>> >> + opp-1056000000 { >>> >> + opp-hz = /bits/ 64 <1056000000>; >>> >> + opp-microvolt = <1240000>; >>> >> + clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> + }; >>> >> + >>> >> + opp-1104000000 { >>> >> + opp-hz = /bits/ 64 <1104000000>; >>> >> + opp-microvolt = <1260000>; >>> >> + clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> + }; >>> >> + >>> >> + opp-1152000000 { >>> >> + opp-hz = /bits/ 64 <1152000000>; >>> >> + opp-microvolt = <1300000>; >>> >> + clock-latency-ns = <244144>; /* 8 32k periods >>> >> */ >>> >> + }; >>> >> + }; >>> >> + >>> >> de: display-engine { >>> >> compatible = "allwinner,sun50i-a64-display-engine"; >>> >> allwinner,pipelines = <&mixer0>, >>> >> diff --git >>> >> a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >>> >> b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >>> >> index ce90327e1b2e..19cb74cf1f57 100644 >>> >> --- a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >>> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts >>> >> @@ -4,8 +4,6 @@ >>> >> /dts-v1/; >>> >> >>> >> #include "sun50i-a64.dtsi" >>> >> -#include "sun50i-a64-cpu-opp.dtsi" >>> >> - >>> >> #include <dt-bindings/gpio/gpio.h> >>> >> >>> >> / { >>>
Hi, On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@manjaro.org> wrote: > > Hello, > > Just checking, any further thoughts about this patch? Sorry, but I feel like it's not really worth the churn. There's not really a problem to be solved here. What you are arguing for is more about aesthetics, and we could argue that having them separate makes it easier to read and turn on/off. And even though the GPU OPPs are in the dtsi, it's just one OPP acting as a default clock rate. ChenYu > On 2024-08-17 06:25, Dragan Simic wrote: > > Hello Andre, > > > > On 2024-08-15 19:15, Andre Przywara wrote: > >> On Thu, 15 Aug 2024 18:34:58 +0200 > >> Dragan Simic <dsimic@manjaro.org> wrote: > >>> On 2024-08-14 18:11, Chen-Yu Tsai wrote: > >>> > On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@manjaro.org> > >>> > wrote: > >>> >> > >>> >> Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and, > >>> >> consequently, > >>> >> adjust the contents of the affected board dts(i) files appropriately, > >>> >> to > >>> >> "encapsulate" the CPU OPPs into the SoC dtsi file. > >>> >> > >>> >> Moving the CPU OPPs to the SoC dtsi file, instead of requiring the > >>> >> board > >>> >> dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi > >>> >> file, > >>> >> reduces the possibility for incomplete SoC data inclusion and improves > >>> >> the > >>> >> overall hierarchical representation of data. Moreover, the CPU OPPs > >>> >> are > >>> >> not used anywhere but together with the SoC dtsi file, which > >>> >> additionally > >>> >> justifies the folding of the CPU OPPs into the SoC dtsi file. > >>> >> > >>> >> No functional changes are introduced, which was validated by > >>> >> decompiling and > >>> >> comparing all affected board dtb files before and after these changes. > >>> >> When > >>> >> compared with the decompiled original dtb files, the updated dtb files > >>> >> have > >>> >> some of their blocks shuffled around a bit and some of their phandles > >>> >> have > >>> >> different values, as a result of the changes to the order in which the > >>> >> building blocks from the parent dtsi files are included into them, but > >>> >> they > >>> >> still effectively remain the same as the originals. > >>> > > >>> > IIRC, this was a conscious decision requiring board dts files to set > >>> > their > >>> > CPU supply before OPPs are given. The bootloader does not boot the SoC > >>> > at the highest possible OPP / regulator voltage, so if the OPPs are > >>> > given > >>> > but the supply is not, the kernel will attempt to raise the frequency > >>> > beyond what the current voltage can supply, causing it to hang. > >> > >> Yes, this is what I remember as well: this forces boards to opt in to > >> DVFS, otherwise they get a fixed 816 MHz. Since there is only one OPP > >> table for all boards with that SoC, I think it's reasonable to ask for > >> this, since the cooling could not be adequate for higher frequencies > >> in > >> the first place, or the power supply is not up to par. > > > > If the cooling isn't capable enough to dissipate the additional heat > > generated at higher frequencies, the thermal governor is there to > > handle > > that by lowering the operating frequency. If the PSU isn't capable to > > provide an additional watt or two, I think a better PSU is needed. :) > > No reasonably sized PSU should work at ~100% of its power output. > > > > On top of that, all currently supported A64-based boards have the CPU > > OPPs defined and CPU DVFS enabled, so no such issues are possible > > there. > > Though, there could be some issues with new A64-based boards, which is > > discussed further below. > > > >>> > Now that all existing boards have it properly enabled, there should be > >>> > no > >>> > need for this. However I would appreciate a second opinion. > >> > >> Well, since there is no way to opt *out* now, I am somewhat reluctant > >> to > >> just have this. What is the actual problem we are solving here? After > >> all > >> there is just one OPP table for all A64 boards, so there is less > >> confusion > >> about what to include in each board file. Which IIUC is a more > >> complicated > >> situation on the Rockchip side. > > > > Well, this patch doesn't solve some real problem, but it makes the > > things > > neater and a bit more clean. The things are more complicated with > > Rockchip > > SoCs, but following the concept of "encapsulating" the CPU OPPs into > > the > > A64 SoC dtsi makes things neater. Moreover, the A64 GPU OPPs are > > already > > in the A64 SoC dtsi, so we could also say that folding the A64 CPU OPPs > > into the SoC dtsi follows the A64 GPU OPPs. > > > >> I still have to try "operating-points-v2", but at least on the H616 > >> side > >> putting a 'status = "disabled";' into the OPP node didn't prevent it > >> from > >> probing. Otherwise this would have been a nice compromise, I think. > >> > >>> Good point, thanks for the clarification. This is quite similar to > >>> how > >>> board dts(i) files for Rockchip SoCs need to enable the SoC's > >>> built-in > >>> TSADC for temperature sensing, before the CPU thermal throttling can > >>> actually work and prevent the SoC from overheating, etc. > >>> > >>> The consensus for Rockchip boards is that it's up to the authors and > >>> reviewers of the board dts(i) files to make sure that the built-in > >>> TSADC > >>> is enabled, etc. With that approach in mind, and knowing that all > >>> Allwinner > >>> A64 board dts(i) files are in good shape when it comes to the > >>> associated > >>> voltage regulators, I think it's fine to follow the same approach of > >>> "encapsulating" the CPU OPPs into the A64 SoC dtsi file. > >> > >> As mentioned above, I am not so sure about this. With this patch here, > >> *every* board gets DVFS. And while this seems to be fine when looking > >> at > >> the current DTs in the tree (which have it anyway), it creates a > >> potentially dangerous situation for new boards. > >> > >> So pragmatically speaking, this patch would be fine, but it leaves me > >> a > >> bit uneasy about future or downstream boards. > > > > Frankly, I wouldn't be worried about that. When a new A64-based board > > is added, it should be verified that CPU DVFS works as expected, etc., > > before the new board dts file is accepted upstream. > > > > Maybe we could take into account some possible issues when someone > > starts > > putting together a new A64-based board dts file, but there are already > > many dangerous things that someone can do in the process, such as > > messing > > up various regulators and voltages unrelated to the CPU DVFS, so > > everyone > > putting a new board dts file together simply have to know what are they > > doing. I see no way for escaping from that need. > > [...]
Hello Chen-Yu, On 2024-09-05 14:26, Chen-Yu Tsai wrote: > Hi, > > On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@manjaro.org> wrote: >> >> Hello, >> >> Just checking, any further thoughts about this patch? > > Sorry, but I feel like it's not really worth the churn. There's not > really a problem to be solved here. What you are arguing for is more > about aesthetics, and we could argue that having them separate makes > it easier to read and turn on/off. > > And even though the GPU OPPs are in the dtsi, it's just one OPP acting > as a default clock rate. Thanks for your response, I understand why it isn't acceptable. >> On 2024-08-17 06:25, Dragan Simic wrote: >> > Hello Andre, >> > >> > On 2024-08-15 19:15, Andre Przywara wrote: >> >> On Thu, 15 Aug 2024 18:34:58 +0200 >> >> Dragan Simic <dsimic@manjaro.org> wrote: >> >>> On 2024-08-14 18:11, Chen-Yu Tsai wrote: >> >>> > On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@manjaro.org> >> >>> > wrote: >> >>> >> >> >>> >> Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and, >> >>> >> consequently, >> >>> >> adjust the contents of the affected board dts(i) files appropriately, >> >>> >> to >> >>> >> "encapsulate" the CPU OPPs into the SoC dtsi file. >> >>> >> >> >>> >> Moving the CPU OPPs to the SoC dtsi file, instead of requiring the >> >>> >> board >> >>> >> dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi >> >>> >> file, >> >>> >> reduces the possibility for incomplete SoC data inclusion and improves >> >>> >> the >> >>> >> overall hierarchical representation of data. Moreover, the CPU OPPs >> >>> >> are >> >>> >> not used anywhere but together with the SoC dtsi file, which >> >>> >> additionally >> >>> >> justifies the folding of the CPU OPPs into the SoC dtsi file. >> >>> >> >> >>> >> No functional changes are introduced, which was validated by >> >>> >> decompiling and >> >>> >> comparing all affected board dtb files before and after these changes. >> >>> >> When >> >>> >> compared with the decompiled original dtb files, the updated dtb files >> >>> >> have >> >>> >> some of their blocks shuffled around a bit and some of their phandles >> >>> >> have >> >>> >> different values, as a result of the changes to the order in which the >> >>> >> building blocks from the parent dtsi files are included into them, but >> >>> >> they >> >>> >> still effectively remain the same as the originals. >> >>> > >> >>> > IIRC, this was a conscious decision requiring board dts files to set >> >>> > their >> >>> > CPU supply before OPPs are given. The bootloader does not boot the SoC >> >>> > at the highest possible OPP / regulator voltage, so if the OPPs are >> >>> > given >> >>> > but the supply is not, the kernel will attempt to raise the frequency >> >>> > beyond what the current voltage can supply, causing it to hang. >> >> >> >> Yes, this is what I remember as well: this forces boards to opt in to >> >> DVFS, otherwise they get a fixed 816 MHz. Since there is only one OPP >> >> table for all boards with that SoC, I think it's reasonable to ask for >> >> this, since the cooling could not be adequate for higher frequencies >> >> in >> >> the first place, or the power supply is not up to par. >> > >> > If the cooling isn't capable enough to dissipate the additional heat >> > generated at higher frequencies, the thermal governor is there to >> > handle >> > that by lowering the operating frequency. If the PSU isn't capable to >> > provide an additional watt or two, I think a better PSU is needed. :) >> > No reasonably sized PSU should work at ~100% of its power output. >> > >> > On top of that, all currently supported A64-based boards have the CPU >> > OPPs defined and CPU DVFS enabled, so no such issues are possible >> > there. >> > Though, there could be some issues with new A64-based boards, which is >> > discussed further below. >> > >> >>> > Now that all existing boards have it properly enabled, there should be >> >>> > no >> >>> > need for this. However I would appreciate a second opinion. >> >> >> >> Well, since there is no way to opt *out* now, I am somewhat reluctant >> >> to >> >> just have this. What is the actual problem we are solving here? After >> >> all >> >> there is just one OPP table for all A64 boards, so there is less >> >> confusion >> >> about what to include in each board file. Which IIUC is a more >> >> complicated >> >> situation on the Rockchip side. >> > >> > Well, this patch doesn't solve some real problem, but it makes the >> > things >> > neater and a bit more clean. The things are more complicated with >> > Rockchip >> > SoCs, but following the concept of "encapsulating" the CPU OPPs into >> > the >> > A64 SoC dtsi makes things neater. Moreover, the A64 GPU OPPs are >> > already >> > in the A64 SoC dtsi, so we could also say that folding the A64 CPU OPPs >> > into the SoC dtsi follows the A64 GPU OPPs. >> > >> >> I still have to try "operating-points-v2", but at least on the H616 >> >> side >> >> putting a 'status = "disabled";' into the OPP node didn't prevent it >> >> from >> >> probing. Otherwise this would have been a nice compromise, I think. >> >> >> >>> Good point, thanks for the clarification. This is quite similar to >> >>> how >> >>> board dts(i) files for Rockchip SoCs need to enable the SoC's >> >>> built-in >> >>> TSADC for temperature sensing, before the CPU thermal throttling can >> >>> actually work and prevent the SoC from overheating, etc. >> >>> >> >>> The consensus for Rockchip boards is that it's up to the authors and >> >>> reviewers of the board dts(i) files to make sure that the built-in >> >>> TSADC >> >>> is enabled, etc. With that approach in mind, and knowing that all >> >>> Allwinner >> >>> A64 board dts(i) files are in good shape when it comes to the >> >>> associated >> >>> voltage regulators, I think it's fine to follow the same approach of >> >>> "encapsulating" the CPU OPPs into the A64 SoC dtsi file. >> >> >> >> As mentioned above, I am not so sure about this. With this patch here, >> >> *every* board gets DVFS. And while this seems to be fine when looking >> >> at >> >> the current DTs in the tree (which have it anyway), it creates a >> >> potentially dangerous situation for new boards. >> >> >> >> So pragmatically speaking, this patch would be fine, but it leaves me >> >> a >> >> bit uneasy about future or downstream boards. >> > >> > Frankly, I wouldn't be worried about that. When a new A64-based board >> > is added, it should be verified that CPU DVFS works as expected, etc., >> > before the new board dts file is accepted upstream. >> > >> > Maybe we could take into account some possible issues when someone >> > starts >> > putting together a new A64-based board dts file, but there are already >> > many dangerous things that someone can do in the process, such as >> > messing >> > up various regulators and voltages unrelated to the CPU DVFS, so >> > everyone >> > putting a new board dts file together simply have to know what are they >> > doing. I see no way for escaping from that need. >> > > > [...]
On Thu, 5 Sep 2024 20:26:15 +0800 Chen-Yu Tsai <wens@csie.org> wrote: > Hi, > > On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@manjaro.org> wrote: > > > > Hello, > > > > Just checking, any further thoughts about this patch? > > Sorry, but I feel like it's not really worth the churn. There's not > really a problem to be solved here. What you are arguing for is more > about aesthetics, and we could argue that having them separate makes > it easier to read and turn on/off. Yeah, I agree. If a board wants to support OPPs, they just have to include a single file and define the CPU regulator, and that's a nice opt-in, IMHO. But having this patch would make it quite hard to opt out, I believe. For Linux there are probably ways to disable DVFS nevertheless, but I am not sure this is true in an OS agnostic pure-DT-only way. This could probably be solved, but same as Chen-Yu I don't see any good enough reason for this patch in the first place. Cheers, Andre > And even though the GPU OPPs are in the dtsi, it's just one OPP acting > as a default clock rate. > > > ChenYu > > > On 2024-08-17 06:25, Dragan Simic wrote: > > > Hello Andre, > > > > > > On 2024-08-15 19:15, Andre Przywara wrote: > > >> On Thu, 15 Aug 2024 18:34:58 +0200 > > >> Dragan Simic <dsimic@manjaro.org> wrote: > > >>> On 2024-08-14 18:11, Chen-Yu Tsai wrote: > > >>> > On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@manjaro.org> > > >>> > wrote: > > >>> >> > > >>> >> Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and, > > >>> >> consequently, > > >>> >> adjust the contents of the affected board dts(i) files appropriately, > > >>> >> to > > >>> >> "encapsulate" the CPU OPPs into the SoC dtsi file. > > >>> >> > > >>> >> Moving the CPU OPPs to the SoC dtsi file, instead of requiring the > > >>> >> board > > >>> >> dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi > > >>> >> file, > > >>> >> reduces the possibility for incomplete SoC data inclusion and improves > > >>> >> the > > >>> >> overall hierarchical representation of data. Moreover, the CPU OPPs > > >>> >> are > > >>> >> not used anywhere but together with the SoC dtsi file, which > > >>> >> additionally > > >>> >> justifies the folding of the CPU OPPs into the SoC dtsi file. > > >>> >> > > >>> >> No functional changes are introduced, which was validated by > > >>> >> decompiling and > > >>> >> comparing all affected board dtb files before and after these changes. > > >>> >> When > > >>> >> compared with the decompiled original dtb files, the updated dtb files > > >>> >> have > > >>> >> some of their blocks shuffled around a bit and some of their phandles > > >>> >> have > > >>> >> different values, as a result of the changes to the order in which the > > >>> >> building blocks from the parent dtsi files are included into them, but > > >>> >> they > > >>> >> still effectively remain the same as the originals. > > >>> > > > >>> > IIRC, this was a conscious decision requiring board dts files to set > > >>> > their > > >>> > CPU supply before OPPs are given. The bootloader does not boot the SoC > > >>> > at the highest possible OPP / regulator voltage, so if the OPPs are > > >>> > given > > >>> > but the supply is not, the kernel will attempt to raise the frequency > > >>> > beyond what the current voltage can supply, causing it to hang. > > >> > > >> Yes, this is what I remember as well: this forces boards to opt in to > > >> DVFS, otherwise they get a fixed 816 MHz. Since there is only one OPP > > >> table for all boards with that SoC, I think it's reasonable to ask for > > >> this, since the cooling could not be adequate for higher frequencies > > >> in > > >> the first place, or the power supply is not up to par. > > > > > > If the cooling isn't capable enough to dissipate the additional heat > > > generated at higher frequencies, the thermal governor is there to > > > handle > > > that by lowering the operating frequency. If the PSU isn't capable to > > > provide an additional watt or two, I think a better PSU is needed. :) > > > No reasonably sized PSU should work at ~100% of its power output. > > > > > > On top of that, all currently supported A64-based boards have the CPU > > > OPPs defined and CPU DVFS enabled, so no such issues are possible > > > there. > > > Though, there could be some issues with new A64-based boards, which is > > > discussed further below. > > > > > >>> > Now that all existing boards have it properly enabled, there should be > > >>> > no > > >>> > need for this. However I would appreciate a second opinion. > > >> > > >> Well, since there is no way to opt *out* now, I am somewhat reluctant > > >> to > > >> just have this. What is the actual problem we are solving here? After > > >> all > > >> there is just one OPP table for all A64 boards, so there is less > > >> confusion > > >> about what to include in each board file. Which IIUC is a more > > >> complicated > > >> situation on the Rockchip side. > > > > > > Well, this patch doesn't solve some real problem, but it makes the > > > things > > > neater and a bit more clean. The things are more complicated with > > > Rockchip > > > SoCs, but following the concept of "encapsulating" the CPU OPPs into > > > the > > > A64 SoC dtsi makes things neater. Moreover, the A64 GPU OPPs are > > > already > > > in the A64 SoC dtsi, so we could also say that folding the A64 CPU OPPs > > > into the SoC dtsi follows the A64 GPU OPPs. > > > > > >> I still have to try "operating-points-v2", but at least on the H616 > > >> side > > >> putting a 'status = "disabled";' into the OPP node didn't prevent it > > >> from > > >> probing. Otherwise this would have been a nice compromise, I think. > > >> > > >>> Good point, thanks for the clarification. This is quite similar to > > >>> how > > >>> board dts(i) files for Rockchip SoCs need to enable the SoC's > > >>> built-in > > >>> TSADC for temperature sensing, before the CPU thermal throttling can > > >>> actually work and prevent the SoC from overheating, etc. > > >>> > > >>> The consensus for Rockchip boards is that it's up to the authors and > > >>> reviewers of the board dts(i) files to make sure that the built-in > > >>> TSADC > > >>> is enabled, etc. With that approach in mind, and knowing that all > > >>> Allwinner > > >>> A64 board dts(i) files are in good shape when it comes to the > > >>> associated > > >>> voltage regulators, I think it's fine to follow the same approach of > > >>> "encapsulating" the CPU OPPs into the A64 SoC dtsi file. > > >> > > >> As mentioned above, I am not so sure about this. With this patch here, > > >> *every* board gets DVFS. And while this seems to be fine when looking > > >> at > > >> the current DTs in the tree (which have it anyway), it creates a > > >> potentially dangerous situation for new boards. > > >> > > >> So pragmatically speaking, this patch would be fine, but it leaves me > > >> a > > >> bit uneasy about future or downstream boards. > > > > > > Frankly, I wouldn't be worried about that. When a new A64-based board > > > is added, it should be verified that CPU DVFS works as expected, etc., > > > before the new board dts file is accepted upstream. > > > > > > Maybe we could take into account some possible issues when someone > > > starts > > > putting together a new A64-based board dts file, but there are already > > > many dangerous things that someone can do in the process, such as > > > messing > > > up various regulators and voltages unrelated to the CPU DVFS, so > > > everyone > > > putting a new board dts file together simply have to know what are they > > > doing. I see no way for escaping from that need. > > > > > [...]
Hello Andre, On 2024-09-05 14:34, Andre Przywara wrote: > On Thu, 5 Sep 2024 20:26:15 +0800 > Chen-Yu Tsai <wens@csie.org> wrote: > >> Hi, >> >> On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@manjaro.org> >> wrote: >> > >> > Hello, >> > >> > Just checking, any further thoughts about this patch? >> >> Sorry, but I feel like it's not really worth the churn. There's not >> really a problem to be solved here. What you are arguing for is more >> about aesthetics, and we could argue that having them separate makes >> it easier to read and turn on/off. > > Yeah, I agree. If a board wants to support OPPs, they just have to > include > a single file and define the CPU regulator, and that's a nice opt-in, > IMHO. > But having this patch would make it quite hard to opt out, I believe. > For > Linux there are probably ways to disable DVFS nevertheless, but I am > not > sure this is true in an OS agnostic pure-DT-only way. Thanks for your response. The only thing that still makes me wonder is why would a board want to opt out of DVFS? Frankly, I'd consider the design of the boards that must keep DVFS disabled broken. > This could probably be solved, but same as Chen-Yu I don't see any good > enough reason for this patch in the first place. > >> And even though the GPU OPPs are in the dtsi, it's just one OPP acting >> as a default clock rate.
On Thu, 05 Sep 2024 14:38:53 +0200 Dragan Simic <dsimic@manjaro.org> wrote: > Hello Andre, > > On 2024-09-05 14:34, Andre Przywara wrote: > > On Thu, 5 Sep 2024 20:26:15 +0800 > > Chen-Yu Tsai <wens@csie.org> wrote: > > > >> Hi, > >> > >> On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@manjaro.org> > >> wrote: > >> > > >> > Hello, > >> > > >> > Just checking, any further thoughts about this patch? > >> > >> Sorry, but I feel like it's not really worth the churn. There's not > >> really a problem to be solved here. What you are arguing for is more > >> about aesthetics, and we could argue that having them separate makes > >> it easier to read and turn on/off. > > > > Yeah, I agree. If a board wants to support OPPs, they just have to > > include > > a single file and define the CPU regulator, and that's a nice opt-in, > > IMHO. > > But having this patch would make it quite hard to opt out, I believe. > > For > > Linux there are probably ways to disable DVFS nevertheless, but I am > > not > > sure this is true in an OS agnostic pure-DT-only way. > > Thanks for your response. The only thing that still makes me wonder > is why would a board want to opt out of DVFS? Frankly, I'd consider > the design of the boards that must keep DVFS disabled broken. Yes! Among the boards using Allwinner SoCs there are some, say less-optimal designs ;-) Cheers, Andre > > This could probably be solved, but same as Chen-Yu I don't see any good > > enough reason for this patch in the first place. > > > >> And even though the GPU OPPs are in the dtsi, it's just one OPP acting > >> as a default clock rate.
On 2024-09-05 14:42, Andre Przywara wrote: > On Thu, 05 Sep 2024 14:38:53 +0200 > Dragan Simic <dsimic@manjaro.org> wrote: > >> Hello Andre, >> >> On 2024-09-05 14:34, Andre Przywara wrote: >> > On Thu, 5 Sep 2024 20:26:15 +0800 >> > Chen-Yu Tsai <wens@csie.org> wrote: >> > >> >> Hi, >> >> >> >> On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@manjaro.org> >> >> wrote: >> >> > >> >> > Hello, >> >> > >> >> > Just checking, any further thoughts about this patch? >> >> >> >> Sorry, but I feel like it's not really worth the churn. There's not >> >> really a problem to be solved here. What you are arguing for is more >> >> about aesthetics, and we could argue that having them separate makes >> >> it easier to read and turn on/off. >> > >> > Yeah, I agree. If a board wants to support OPPs, they just have to >> > include >> > a single file and define the CPU regulator, and that's a nice opt-in, >> > IMHO. >> > But having this patch would make it quite hard to opt out, I believe. >> > For >> > Linux there are probably ways to disable DVFS nevertheless, but I am >> > not >> > sure this is true in an OS agnostic pure-DT-only way. >> >> Thanks for your response. The only thing that still makes me wonder >> is why would a board want to opt out of DVFS? Frankly, I'd consider >> the design of the boards that must keep DVFS disabled broken. > > Yes! Among the boards using Allwinner SoCs there are some, say > less-optimal > designs ;-) I see, but such boards could simply disable the "cpu0_opp_table" node in their dts(i) files, for the encapsulated CPU OPPs scenario, and everything would still work and be defined in a clean(er) way. I mean, if there are some suboptimal designs, perhaps the defaults should be tailored towards the good designs, and the suboptimal designs should be some kind of exceptions. >> > This could probably be solved, but same as Chen-Yu I don't see any good >> > enough reason for this patch in the first place. >> > >> >> And even though the GPU OPPs are in the dtsi, it's just one OPP acting >> >> as a default clock rate.
On Thu, 05 Sep 2024 14:54:03 +0200 Dragan Simic <dsimic@manjaro.org> wrote: > On 2024-09-05 14:42, Andre Przywara wrote: > > On Thu, 05 Sep 2024 14:38:53 +0200 > > Dragan Simic <dsimic@manjaro.org> wrote: > > > >> Hello Andre, > >> > >> On 2024-09-05 14:34, Andre Przywara wrote: > >> > On Thu, 5 Sep 2024 20:26:15 +0800 > >> > Chen-Yu Tsai <wens@csie.org> wrote: > >> > > >> >> Hi, > >> >> > >> >> On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@manjaro.org> > >> >> wrote: > >> >> > > >> >> > Hello, > >> >> > > >> >> > Just checking, any further thoughts about this patch? > >> >> > >> >> Sorry, but I feel like it's not really worth the churn. There's not > >> >> really a problem to be solved here. What you are arguing for is more > >> >> about aesthetics, and we could argue that having them separate makes > >> >> it easier to read and turn on/off. > >> > > >> > Yeah, I agree. If a board wants to support OPPs, they just have to > >> > include > >> > a single file and define the CPU regulator, and that's a nice opt-in, > >> > IMHO. > >> > But having this patch would make it quite hard to opt out, I believe. > >> > For > >> > Linux there are probably ways to disable DVFS nevertheless, but I am > >> > not > >> > sure this is true in an OS agnostic pure-DT-only way. > >> > >> Thanks for your response. The only thing that still makes me wonder > >> is why would a board want to opt out of DVFS? Frankly, I'd consider > >> the design of the boards that must keep DVFS disabled broken. > > > > Yes! Among the boards using Allwinner SoCs there are some, say > > less-optimal > > designs ;-) > > I see, but such boards could simply disable the "cpu0_opp_table" node in > their dts(i) files, for the encapsulated CPU OPPs scenario, and > everything > would still work and be defined in a clean(er) way. I agree, and I was already about to suggest this as a reply to your initial post, but I think I tried that, and IIRC this doesn't work: the "status" property is not honoured for this node. But please double check that. Cheers, Andre > I mean, if there are some suboptimal designs, perhaps the defaults > should > be tailored towards the good designs, and the suboptimal designs should > be > some kind of exceptions. > > >> > This could probably be solved, but same as Chen-Yu I don't see any good > >> > enough reason for this patch in the first place. > >> > > >> >> And even though the GPU OPPs are in the dtsi, it's just one OPP acting > >> >> as a default clock rate.
Hello Andre, On 2024-09-05 15:20, Andre Przywara wrote: > On Thu, 05 Sep 2024 14:54:03 +0200 Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-09-05 14:42, Andre Przywara wrote: >> > On Thu, 05 Sep 2024 14:38:53 +0200 >> > Dragan Simic <dsimic@manjaro.org> wrote: >> >> On 2024-09-05 14:34, Andre Przywara wrote: >> >> > On Thu, 5 Sep 2024 20:26:15 +0800 >> >> > Chen-Yu Tsai <wens@csie.org> wrote: >> >> >> On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@manjaro.org> >> >> >> wrote: >> >> >> > Just checking, any further thoughts about this patch? >> >> >> >> >> >> Sorry, but I feel like it's not really worth the churn. There's not >> >> >> really a problem to be solved here. What you are arguing for is more >> >> >> about aesthetics, and we could argue that having them separate makes >> >> >> it easier to read and turn on/off. >> >> > >> >> > Yeah, I agree. If a board wants to support OPPs, they just have to >> >> > include >> >> > a single file and define the CPU regulator, and that's a nice opt-in, >> >> > IMHO. >> >> > But having this patch would make it quite hard to opt out, I believe. >> >> > For >> >> > Linux there are probably ways to disable DVFS nevertheless, but I am >> >> > not >> >> > sure this is true in an OS agnostic pure-DT-only way. >> >> >> >> Thanks for your response. The only thing that still makes me wonder >> >> is why would a board want to opt out of DVFS? Frankly, I'd consider >> >> the design of the boards that must keep DVFS disabled broken. >> > >> > Yes! Among the boards using Allwinner SoCs there are some, say >> > less-optimal designs ;-) >> >> I see, but such boards could simply disable the "cpu0_opp_table" >> node in their dts(i) files, for the encapsulated CPU OPPs scenario, >> and everything would still work and be defined in a clean(er) way. > > I agree, and I was already about to suggest this as a reply to your > initial > post, but I think I tried that, and IIRC this doesn't work: the > "status" > property is not honoured for this node. > But please double check that. I apologize for my delayed response. Perhaps a safer approach could be to introduce a new dtsi file, named sun50i-a64-cpu-opps-disabled.dtsi, with the following contents: /delete-node/ &cpu0_opp_table; &cpu0 { /delete-property/ operating-points-v2; }; &cpu1 { /delete-property/ operating-points-v2; }; &cpu2 { /delete-property/ operating-points-v2; }; &cpu3 { /delete-property/ operating-points-v2; }; The purpose of this new file would be to delete the CPU OPPs for the suboptimally designed boards, when included into their dts(i) files, while the CPU OPPs would be "encapsulated" into the SoC dtsi. Though, I'm not sure how much cleaner this approach would be, but I think it would fit rather well with the suggested approach of having such suboptimal board designs treated as an exception that would be handled in some special way. As a bonus, it would also make locating dts(i) files for such suboptimal designs a bit simpler, by grepping for "cpu-opps-disabled". >> I mean, if there are some suboptimal designs, perhaps the defaults >> should be tailored towards the good designs, and the suboptimal >> designs should be some kind of exceptions. >> >> >> > This could probably be solved, but same as Chen-Yu I don't see any good >> >> > enough reason for this patch in the first place. >> >> > >> >> >> And even though the GPU OPPs are in the dtsi, it's just one OPP acting >> >> >> as a default clock rate.
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts index 8233582f6288..1590a455683f 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts @@ -5,8 +5,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> / { diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts index d1f415acd7b5..869fd4a50582 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts @@ -4,8 +4,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> / { diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi deleted file mode 100644 index e39db51eb448..000000000000 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi +++ /dev/null @@ -1,75 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 -/* - * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> - */ - -/ { - cpu0_opp_table: opp-table-cpu { - compatible = "operating-points-v2"; - opp-shared; - - opp-648000000 { - opp-hz = /bits/ 64 <648000000>; - opp-microvolt = <1040000>; - clock-latency-ns = <244144>; /* 8 32k periods */ - }; - - opp-816000000 { - opp-hz = /bits/ 64 <816000000>; - opp-microvolt = <1100000>; - clock-latency-ns = <244144>; /* 8 32k periods */ - }; - - opp-912000000 { - opp-hz = /bits/ 64 <912000000>; - opp-microvolt = <1120000>; - clock-latency-ns = <244144>; /* 8 32k periods */ - }; - - opp-960000000 { - opp-hz = /bits/ 64 <960000000>; - opp-microvolt = <1160000>; - clock-latency-ns = <244144>; /* 8 32k periods */ - }; - - opp-1008000000 { - opp-hz = /bits/ 64 <1008000000>; - opp-microvolt = <1200000>; - clock-latency-ns = <244144>; /* 8 32k periods */ - }; - - opp-1056000000 { - opp-hz = /bits/ 64 <1056000000>; - opp-microvolt = <1240000>; - clock-latency-ns = <244144>; /* 8 32k periods */ - }; - - opp-1104000000 { - opp-hz = /bits/ 64 <1104000000>; - opp-microvolt = <1260000>; - clock-latency-ns = <244144>; /* 8 32k periods */ - }; - - opp-1152000000 { - opp-hz = /bits/ 64 <1152000000>; - opp-microvolt = <1300000>; - clock-latency-ns = <244144>; /* 8 32k periods */ - }; - }; -}; - -&cpu0 { - operating-points-v2 = <&cpu0_opp_table>; -}; - -&cpu1 { - operating-points-v2 = <&cpu0_opp_table>; -}; - -&cpu2 { - operating-points-v2 = <&cpu0_opp_table>; -}; - -&cpu3 { - operating-points-v2 = <&cpu0_opp_table>; -}; diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts index dec9960a7440..e3b1943f7f63 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts @@ -4,8 +4,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> / { diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts index fd3794678c33..f1a4a9ab295b 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts @@ -4,8 +4,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> / { diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts index c8303a66438d..f3c9a3534612 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts @@ -5,8 +5,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> / { diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts index 09e71fd60785..4f65eec043d0 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts @@ -4,8 +4,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> / { diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts index 379c2c8466f5..a06a0b34bd3f 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts @@ -5,8 +5,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/input/gpio-keys.h> #include <dt-bindings/input/input.h> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi index 6eab61a12cd8..0e38cd02a90a 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi @@ -4,8 +4,6 @@ // Copyright (C) 2020 Ondrej Jirman <megous@megous.com> #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/input/input.h> #include <dt-bindings/leds/common.h> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts index f5fb1ee32dad..4a49f137885b 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts @@ -7,8 +7,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/input/input.h> #include <dt-bindings/pwm/pwm.h> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi index 6d78a1c98f10..4ba5c11e6870 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi @@ -4,8 +4,6 @@ // Copyright (c) 2016 ARM Ltd. #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> &codec_analog { diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts index b407e1dd08a7..61ccd90bae01 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts @@ -5,8 +5,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/input/input.h> #include <dt-bindings/pwm/pwm.h> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index e868ca5ae753..f842e64562b7 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -1,7 +1,10 @@ // SPDX-License-Identifier: (GPL-2.0+ OR MIT) -// Copyright (C) 2016 ARM Ltd. -// based on the Allwinner H3 dtsi: -// Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> +/* + * Copyright (C) 2016 ARM Ltd. + * based on the Allwinner H3 dtsi: + * Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> + * Copyright (C) 2020 Vasily khoruzhick <anarsoul@gmail.com> + */ #include <dt-bindings/clock/sun50i-a64-ccu.h> #include <dt-bindings/clock/sun6i-rtc.h> @@ -54,6 +57,7 @@ cpu0: cpu@0 { clocks = <&ccu CLK_CPUX>; clock-names = "cpu"; #cooling-cells = <2>; + operating-points-v2 = <&cpu_opp_table>; i-cache-size = <0x8000>; i-cache-line-size = <64>; i-cache-sets = <256>; @@ -71,6 +75,7 @@ cpu1: cpu@1 { clocks = <&ccu CLK_CPUX>; clock-names = "cpu"; #cooling-cells = <2>; + operating-points-v2 = <&cpu_opp_table>; i-cache-size = <0x8000>; i-cache-line-size = <64>; i-cache-sets = <256>; @@ -88,6 +93,7 @@ cpu2: cpu@2 { clocks = <&ccu CLK_CPUX>; clock-names = "cpu"; #cooling-cells = <2>; + operating-points-v2 = <&cpu_opp_table>; i-cache-size = <0x8000>; i-cache-line-size = <64>; i-cache-sets = <256>; @@ -105,6 +111,7 @@ cpu3: cpu@3 { clocks = <&ccu CLK_CPUX>; clock-names = "cpu"; #cooling-cells = <2>; + operating-points-v2 = <&cpu_opp_table>; i-cache-size = <0x8000>; i-cache-line-size = <64>; i-cache-sets = <256>; @@ -124,6 +131,59 @@ l2_cache: l2-cache { }; }; + cpu_opp_table: opp-table-cpu { + compatible = "operating-points-v2"; + opp-shared; + + opp-648000000 { + opp-hz = /bits/ 64 <648000000>; + opp-microvolt = <1040000>; + clock-latency-ns = <244144>; /* 8 32k periods */ + }; + + opp-816000000 { + opp-hz = /bits/ 64 <816000000>; + opp-microvolt = <1100000>; + clock-latency-ns = <244144>; /* 8 32k periods */ + }; + + opp-912000000 { + opp-hz = /bits/ 64 <912000000>; + opp-microvolt = <1120000>; + clock-latency-ns = <244144>; /* 8 32k periods */ + }; + + opp-960000000 { + opp-hz = /bits/ 64 <960000000>; + opp-microvolt = <1160000>; + clock-latency-ns = <244144>; /* 8 32k periods */ + }; + + opp-1008000000 { + opp-hz = /bits/ 64 <1008000000>; + opp-microvolt = <1200000>; + clock-latency-ns = <244144>; /* 8 32k periods */ + }; + + opp-1056000000 { + opp-hz = /bits/ 64 <1056000000>; + opp-microvolt = <1240000>; + clock-latency-ns = <244144>; /* 8 32k periods */ + }; + + opp-1104000000 { + opp-hz = /bits/ 64 <1104000000>; + opp-microvolt = <1260000>; + clock-latency-ns = <244144>; /* 8 32k periods */ + }; + + opp-1152000000 { + opp-hz = /bits/ 64 <1152000000>; + opp-microvolt = <1300000>; + clock-latency-ns = <244144>; /* 8 32k periods */ + }; + }; + de: display-engine { compatible = "allwinner,sun50i-a64-display-engine"; allwinner,pipelines = <&mixer0>, diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts index ce90327e1b2e..19cb74cf1f57 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-h64-remix-mini-pc.dts @@ -4,8 +4,6 @@ /dts-v1/; #include "sun50i-a64.dtsi" -#include "sun50i-a64-cpu-opp.dtsi" - #include <dt-bindings/gpio/gpio.h> / {
Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and, consequently, adjust the contents of the affected board dts(i) files appropriately, to "encapsulate" the CPU OPPs into the SoC dtsi file. Moving the CPU OPPs to the SoC dtsi file, instead of requiring the board dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi file, reduces the possibility for incomplete SoC data inclusion and improves the overall hierarchical representation of data. Moreover, the CPU OPPs are not used anywhere but together with the SoC dtsi file, which additionally justifies the folding of the CPU OPPs into the SoC dtsi file. No functional changes are introduced, which was validated by decompiling and comparing all affected board dtb files before and after these changes. When compared with the decompiled original dtb files, the updated dtb files have some of their blocks shuffled around a bit and some of their phandles have different values, as a result of the changes to the order in which the building blocks from the parent dtsi files are included into them, but they still effectively remain the same as the originals. Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- Notes: These changes follow the general approach used for the Rockchip RK3588 and RK3399 SoCs, which was introduced and described further in commits def88eb4d836 ("arm64: dts: rockchip: Prepare RK3588 SoC dtsi files for per-variant OPPs") [1] and 296602b8e5f7 ("arm64: dts: rockchip: Move RK3399 OPPs to dtsi files for SoC variants"), [2] respectively. Please see those commits for a more detailed explanation. [1] https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=def88eb4d8365a4aa064d28405d03550a9d0a3be [2] https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=for-next&id=296602b8e5f715d6a0ccdcd37d57170c2c81d5e4 .../allwinner/sun50i-a64-amarula-relic.dts | 2 - .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 2 - .../dts/allwinner/sun50i-a64-cpu-opp.dtsi | 75 ------------------- .../dts/allwinner/sun50i-a64-nanopi-a64.dts | 2 - .../dts/allwinner/sun50i-a64-olinuxino.dts | 2 - .../dts/allwinner/sun50i-a64-orangepi-win.dts | 2 - .../boot/dts/allwinner/sun50i-a64-pine64.dts | 2 - .../dts/allwinner/sun50i-a64-pinebook.dts | 2 - .../dts/allwinner/sun50i-a64-pinephone.dtsi | 2 - .../boot/dts/allwinner/sun50i-a64-pinetab.dts | 2 - .../boot/dts/allwinner/sun50i-a64-sopine.dtsi | 2 - .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 2 - arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 66 +++++++++++++++- .../allwinner/sun50i-h64-remix-mini-pc.dts | 2 - 14 files changed, 63 insertions(+), 102 deletions(-) delete mode 100644 arch/arm64/boot/dts/allwinner/sun50i-a64-cpu-opp.dtsi