Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/2] ARM: dts: exynos: Adjust bus related OPPs to the
 values correct for Odroids
To:     Marek Szyprowski <m.szyprowski@samsung.com>,
        linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org
Cc:     Krzysztof Kozlowski <krzk@kernel.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
        Kamil Konieczny <k.konieczny@samsung.com>
From:   Chanwoo Choi <cw00.choi@samsung.com>
Organization: Samsung Electronics
Message-ID: <ebdb5558-2e0c-8167-8ad7-3413fa35ed8a@samsung.com>
Date:   Thu, 19 Dec 2019 18:23:34 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101
        Thunderbird/59.0
MIME-Version: 1.0
In-Reply-To: <2113afca-4f8b-ec06-ee1d-b84776b5b70d@samsung.com>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20191219082927.11898-1-m.szyprowski@samsung.com>
        <CGME20191219082939eucas1p2afc32535df1512dc21ca983daa012568@eucas1p2.samsung.com>
        <20191219082927.11898-3-m.szyprowski@samsung.com>
        <9ff5cf7a-1dea-4f35-c4d3-066c87fbe896@samsung.com>
        <2113afca-4f8b-ec06-ee1d-b84776b5b70d@samsung.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 12/19/19 6:09 PM, Marek Szyprowski wrote:
> Hi Chanwoo,
> 
> On 19.12.2019 10:07, Chanwoo Choi wrote:
>> 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@samsung.com>
>>> ---
>>>   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.
> 
> I've checked the dt bindings and I think that opp-shared property has to 
> be removed at all. Clocks between fsys and fsys2 buses are not related 
> and regulator is currently already handled by in a different way by the 
> passive governor.

You're right. I realized 'opp-shared' is not necessary
by reading Documentation/devicetree/bindings/opp/opp.txt. Thanks.

> 
>>>   	devfreq = <&bus_wcore>;
>>>   	status = "okay";
>>>   };
>>>
>> If you fix the things related to 'opp-shared', Looks good to me.
>> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
> Best regards
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics