Re: [PATCH 2/2] ARM: dts: exynos: Adjust bus related OPPs to the values correct for Odroids

From: Chanwoo Choi
Date: Thu Dec 19 2019 - 04:00:46 EST

Hi Marek,

On 12/19/19 5:29 PM, Marek Szyprowski wrote:
> Hardkernel's Odroid XU3/XU4/HC1 boards use bootloader, which configures top
> PLLs to the following values: MPLL: 532MHz, CPLL: 666MHz and DPLL: 600MHz.
>
> Adjust all bus related OPPs to the values that are possible to derive from
> the top PLL configured by the bootloader. Also add a comment for each bus
> describing which PLL is used for it.
>
> The most significant change is the highest rate for wcore bus. It has been
> increased to 532MHz as this is the value configured initially by the
> bootloader. Also the voltage for this OPP is changed to match the value
> set by the bootloader.
>
> This patch finally allows the buses to operate on the rates matching the
> values set for each OPP and fixes the following warnings observed on boot:
>
> exynos-bus: new bus device registered: soc:bus_wcore ( 84000 KHz ~ 400000 KHz)
> exynos-bus: new bus device registered: soc:bus_noc ( 67000 KHz ~ 100000 KHz)
> exynos-bus: new bus device registered: soc:bus_fsys_apb (100000 KHz ~ 200000 KHz)
> ...
> exynos-bus soc:bus_wcore: dev_pm_opp_set_rate: failed to find current OPP for freq 532000000 (-34)
> exynos-bus soc:bus_noc: dev_pm_opp_set_rate: failed to find current OPP for freq 111000000 (-34)
> exynos-bus soc:bus_fsys_apb: dev_pm_opp_set_rate: failed to find current OPP for freq 222000000 (-34)
>
> The problem with setting incorrect (in some cases much lower) clock rate
> for the defined OPP were there from the beginning, but went unnoticed
> because the only way to observe it was to manually check the rate of the
> respective clocks. The commit 4294a779bd8d ("PM / devfreq: exynos-bus:
> Convert to use dev_pm_opp_set_rate()") finally revealed it, because it
> enabled use of the generic code from the OPP framework, which issues the
> above mentioned warnings.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> ---
> arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 75 +++++++++++--------
> 1 file changed, 45 insertions(+), 30 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> index 663a38d53c9e..b6d6022e8735 100644
> --- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> +++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> @@ -38,42 +38,44 @@
> bus_wcore_opp_table: opp_table2 {
> compatible = "operating-points-v2";
>
> + /* derived from 532MHz MPLL */
> opp00 {
> - opp-hz = /bits/ 64 <84000000>;
> + opp-hz = /bits/ 64 <88700000>;
> opp-microvolt = <925000 925000 1400000>;
> };
> opp01 {
> - opp-hz = /bits/ 64 <111000000>;
> + opp-hz = /bits/ 64 <133000000>;
> opp-microvolt = <950000 950000 1400000>;
> };
> opp02 {
> - opp-hz = /bits/ 64 <222000000>;
> + opp-hz = /bits/ 64 <177400000>;
> opp-microvolt = <950000 950000 1400000>;
> };
> opp03 {
> - opp-hz = /bits/ 64 <333000000>;
> + opp-hz = /bits/ 64 <266000000>;
> opp-microvolt = <950000 950000 1400000>;
> };
> opp04 {
> - opp-hz = /bits/ 64 <400000000>;
> - opp-microvolt = <987500 987500 1400000>;
> + opp-hz = /bits/ 64 <532000000>;
> + opp-microvolt = <1000000 1000000 1400000>;
> };
> };
>
> bus_noc_opp_table: opp_table3 {
> compatible = "operating-points-v2";
>
> + /* derived from 666MHz CPLL */
> opp00 {
> - opp-hz = /bits/ 64 <67000000>;
> + opp-hz = /bits/ 64 <66600000>;
> };
> opp01 {
> - opp-hz = /bits/ 64 <75000000>;
> + opp-hz = /bits/ 64 <74000000>;
> };
> opp02 {
> - opp-hz = /bits/ 64 <86000000>;
> + opp-hz = /bits/ 64 <83250000>;
> };
> opp03 {
> - opp-hz = /bits/ 64 <100000000>;
> + opp-hz = /bits/ 64 <111000000>;
> };
> };
>
> @@ -81,39 +83,42 @@
> compatible = "operating-points-v2";
> opp-shared;
>
> + /* derived from 666MHz CPLL */
> opp00 {
> - opp-hz = /bits/ 64 <100000000>;
> + opp-hz = /bits/ 64 <111000000>;
> };
> opp01 {
> - opp-hz = /bits/ 64 <200000000>;
> + opp-hz = /bits/ 64 <222000000>;
> };
> };
>
> bus_fsys2_opp_table: opp_table5 {
> compatible = "operating-points-v2";
>
> + /* derived from 600MHz DPLL */
> opp00 {
> opp-hz = /bits/ 64 <75000000>;
> };
> opp01 {
> - opp-hz = /bits/ 64 <100000000>;
> + opp-hz = /bits/ 64 <120000000>;
> };
> opp02 {
> - opp-hz = /bits/ 64 <150000000>;
> + opp-hz = /bits/ 64 <200000000>;
> };
> };
>
> bus_mfc_opp_table: opp_table6 {
> compatible = "operating-points-v2";
>
> + /* derived from 666MHz CPLL */
> opp00 {
> - opp-hz = /bits/ 64 <96000000>;
> + opp-hz = /bits/ 64 <83250000>;
> };
> opp01 {
> opp-hz = /bits/ 64 <111000000>;
> };
> opp02 {
> - opp-hz = /bits/ 64 <167000000>;
> + opp-hz = /bits/ 64 <166500000>;
> };
> opp03 {
> opp-hz = /bits/ 64 <222000000>;
> @@ -126,8 +131,9 @@
> bus_gen_opp_table: opp_table7 {
> compatible = "operating-points-v2";
>
> + /* derived from 532MHz MPLL */
> opp00 {
> - opp-hz = /bits/ 64 <89000000>;
> + opp-hz = /bits/ 64 <88700000>;
> };
> opp01 {
> opp-hz = /bits/ 64 <133000000>;
> @@ -136,32 +142,34 @@
> opp-hz = /bits/ 64 <178000000>;
> };
> opp03 {
> - opp-hz = /bits/ 64 <267000000>;
> + opp-hz = /bits/ 64 <266000000>;
> };
> };
>
> bus_peri_opp_table: opp_table8 {
> compatible = "operating-points-v2";
>
> + /* derived from 666MHz CPLL */
> opp00 {
> - opp-hz = /bits/ 64 <67000000>;
> + opp-hz = /bits/ 64 <66600000>;
> };
> };
>
> bus_g2d_opp_table: opp_table9 {
> compatible = "operating-points-v2";
>
> + /* derived from 666MHz CPLL */
> opp00 {
> - opp-hz = /bits/ 64 <84000000>;
> + opp-hz = /bits/ 64 <83250000>;
> };
> opp01 {
> - opp-hz = /bits/ 64 <167000000>;
> + opp-hz = /bits/ 64 <111000000>;
> };
> opp02 {
> - opp-hz = /bits/ 64 <222000000>;
> + opp-hz = /bits/ 64 <166500000>;
> };
> opp03 {
> - opp-hz = /bits/ 64 <300000000>;
> + opp-hz = /bits/ 64 <222000000>;
> };
> opp04 {
> opp-hz = /bits/ 64 <333000000>;
> @@ -171,8 +179,9 @@
> bus_g2d_acp_opp_table: opp_table10 {
> compatible = "operating-points-v2";
>
> + /* derived from 532MHz MPLL */
> opp00 {
> - opp-hz = /bits/ 64 <67000000>;
> + opp-hz = /bits/ 64 <66500000>;
> };
> opp01 {
> opp-hz = /bits/ 64 <133000000>;
> @@ -181,13 +190,14 @@
> opp-hz = /bits/ 64 <178000000>;
> };
> opp03 {
> - opp-hz = /bits/ 64 <267000000>;
> + opp-hz = /bits/ 64 <266000000>;
> };
> };
>
> bus_jpeg_opp_table: opp_table11 {
> compatible = "operating-points-v2";
>
> + /* derived from 600MHz DPLL */
> opp00 {
> opp-hz = /bits/ 64 <75000000>;
> };
> @@ -205,23 +215,25 @@
> bus_jpeg_apb_opp_table: opp_table12 {
> compatible = "operating-points-v2";
>
> + /* derived from 666MHz CPLL */
> opp00 {
> - opp-hz = /bits/ 64 <84000000>;
> + opp-hz = /bits/ 64 <83250000>;
> };
> opp01 {
> opp-hz = /bits/ 64 <111000000>;
> };
> opp02 {
> - opp-hz = /bits/ 64 <134000000>;
> + opp-hz = /bits/ 64 <133000000>;
> };
> opp03 {
> - opp-hz = /bits/ 64 <167000000>;
> + opp-hz = /bits/ 64 <166500000>;
> };
> };
>
> bus_disp1_fimd_opp_table: opp_table13 {
> compatible = "operating-points-v2";
>
> + /* derived from 600MHz DPLL */
> opp00 {
> opp-hz = /bits/ 64 <120000000>;
> };
> @@ -233,6 +245,7 @@
> bus_disp1_opp_table: opp_table14 {
> compatible = "operating-points-v2";
>
> + /* derived from 600MHz DPLL */
> opp00 {
> opp-hz = /bits/ 64 <120000000>;
> };
> @@ -247,6 +260,7 @@
> bus_gscl_opp_table: opp_table15 {
> compatible = "operating-points-v2";
>
> + /* derived from 600MHz DPLL */
> opp00 {
> opp-hz = /bits/ 64 <150000000>;
> };
> @@ -261,6 +275,7 @@
> bus_mscl_opp_table: opp_table16 {
> compatible = "operating-points-v2";
>
> + /* derived from 666MHz CPLL */
> opp00 {
> opp-hz = /bits/ 64 <84000000>;
> };
> @@ -274,7 +289,7 @@
> opp-hz = /bits/ 64 <333000000>;
> };
> opp04 {
> - opp-hz = /bits/ 64 <400000000>;
> + opp-hz = /bits/ 64 <666000000>;
> };
> };
>
> @@ -398,7 +413,7 @@
> };
>
> &bus_fsys {
> - operating-points-v2 = <&bus_fsys_apb_opp_table>;
> + operating-points-v2 = <&bus_fsys2_opp_table>;


Need to remove 'opp-shared' property in bus_fsys_apb_opp_table.
And need to add 'opp-shared' property to bus_fsys2_opp_table.


> devfreq = <&bus_wcore>;
> status = "okay";
> };
>

If you fix the things related to 'opp-shared', Looks good to me.
Tested-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
Reviewed-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>

--
Best Regards,
Chanwoo Choi
Samsung Electronics