From: Marek Szyprowski <m.szyprowski@samsung.com>
To: Chanwoo Choi <cw00.choi@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>
Subject: Re: [PATCH 2/2] ARM: dts: exynos: Adjust bus related OPPs to the values correct for Odroids
Date: Thu, 19 Dec 2019 10:09:37 +0100 [thread overview]
Message-ID: <2113afca-4f8b-ec06-ee1d-b84776b5b70d@samsung.com> (raw)
In-Reply-To: <9ff5cf7a-1dea-4f35-c4d3-066c87fbe896@samsung.com>
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.
>> 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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
next prev parent reply other threads:[~2019-12-19 9:09 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20191219082938eucas1p254fe738574f287a44630d5a7eef7385e@eucas1p2.samsung.com>
2019-12-19 8:29 ` [PATCH 0/2] Exynos5422: fix bus related OPPs for Odroid XU3/XU4/HC1 Marek Szyprowski
2019-12-19 8:29 ` [PATCH 1/2] ARM: dts: exynos: Move bus related OPPs to the boards DTS Marek Szyprowski
2019-12-19 9:02 ` Chanwoo Choi
2019-12-19 8:29 ` [PATCH 2/2] ARM: dts: exynos: Adjust bus related OPPs to the values correct for Odroids Marek Szyprowski
2019-12-19 9:07 ` Chanwoo Choi
2019-12-19 9:09 ` Marek Szyprowski [this message]
2019-12-19 9:23 ` Chanwoo Choi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2113afca-4f8b-ec06-ee1d-b84776b5b70d@samsung.com \
--to=m.szyprowski@samsung.com \
--cc=b.zolnierkie@samsung.com \
--cc=cw00.choi@samsung.com \
--cc=k.konieczny@samsung.com \
--cc=krzk@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox