[PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[k.konieczny]

Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
dev_pm_opp_set_rate()") introduced errors:
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)

They are caused by incorrect PLL assigned to clock source, which results
in clock rate outside of OPP range. Add workaround for this in
exynos_bus_parse_of() by adjusting clock rate to those present in OPP.

Fixes: 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()")
Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 drivers/devfreq/exynos-bus.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index c832673273a2..37bd34d5625b 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -243,7 +243,7 @@ static int exynos_bus_parse_of(struct device_node *np,
 {
 	struct device *dev = bus->dev;
 	struct dev_pm_opp *opp;
-	unsigned long rate;
+	unsigned long rate, opp_rate;
 	int ret;
 
 	/* Get the clock to provide each bus with source clock */
@@ -267,13 +267,21 @@ static int exynos_bus_parse_of(struct device_node *np,
 	}
 
 	rate = clk_get_rate(bus->clk);
-
-	opp = devfreq_recommended_opp(dev, &rate, 0);
+	opp_rate = rate;
+	opp = devfreq_recommended_opp(dev, &opp_rate, 0);
 	if (IS_ERR(opp)) {
 		dev_err(dev, "failed to find dev_pm_opp\n");
 		ret = PTR_ERR(opp);
 		goto err_opp;
 	}
+	/*
+	 * FIXME: U-boot leaves clock source at incorrect PLL, this results
+	 * in clock rate outside defined OPP rate. Work around this bug by
+	 * setting clock rate to recommended one.
+	 */
+	if (rate > opp_rate)
+		clk_set_rate(bus->clk, opp_rate);
+
 	bus->curr_freq = dev_pm_opp_get_freq(opp);
 	dev_pm_opp_put(opp);
 
-- 
2.23.0

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Chanwoo Choi]

On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
> dev_pm_opp_set_rate()") introduced errors:
> 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)
> 
> They are caused by incorrect PLL assigned to clock source, which results
> in clock rate outside of OPP range. Add workaround for this in
> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.

If the clock caused this issue, you can set the initial clock on DeviceTree
with assigned-clock-* properties. Because the probe time of clock driver
is early than the any device drivers.

It is not proper to fix the clock issue on other device driver.
I think you can fix it by using the supported clock properties.


> 
> Fixes: 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()")
> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index c832673273a2..37bd34d5625b 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -243,7 +243,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>  {
>  	struct device *dev = bus->dev;
>  	struct dev_pm_opp *opp;
> -	unsigned long rate;
> +	unsigned long rate, opp_rate;
>  	int ret;
>  
>  	/* Get the clock to provide each bus with source clock */
> @@ -267,13 +267,21 @@ static int exynos_bus_parse_of(struct device_node *np,
>  	}
>  
>  	rate = clk_get_rate(bus->clk);
> -
> -	opp = devfreq_recommended_opp(dev, &rate, 0);
> +	opp_rate = rate;
> +	opp = devfreq_recommended_opp(dev, &opp_rate, 0);
>  	if (IS_ERR(opp)) {
>  		dev_err(dev, "failed to find dev_pm_opp\n");
>  		ret = PTR_ERR(opp);
>  		goto err_opp;
>  	}
> +	/*
> +	 * FIXME: U-boot leaves clock source at incorrect PLL, this results
> +	 * in clock rate outside defined OPP rate. Work around this bug by
> +	 * setting clock rate to recommended one.
> +	 */
> +	if (rate > opp_rate)
> +		clk_set_rate(bus->clk, opp_rate);
> +
>  	bus->curr_freq = dev_pm_opp_get_freq(opp);
>  	dev_pm_opp_put(opp);
>  
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Marek Szyprowski]

Hi Chanwoo,

On 10.10.2019 04:50, Chanwoo Choi wrote:
> On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
>> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
>> dev_pm_opp_set_rate()") introduced errors:
>> 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)
>>
>> They are caused by incorrect PLL assigned to clock source, which results
>> in clock rate outside of OPP range. Add workaround for this in
>> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.
> If the clock caused this issue, you can set the initial clock on DeviceTree
> with assigned-clock-* properties. Because the probe time of clock driver
> is early than the any device drivers.
>
> It is not proper to fix the clock issue on other device driver.
> I think you can fix it by using the supported clock properties.

This issue is about something completely different. The OPPs defined in 
DT cannot be applied, because it is not possible to derive the needed 
clock rate from the bootloader-configured clock topology (mainly due to 
lack of common divisor values for some of the parent clocks). Some time 
ago Lukasz tried initially to redefine this clock topology using 
assigned-clock-rates/parents properties (see 
https://lkml.org/lkml/2019/7/15/276), but it has limitations and some 
such changes has to be done in bootloader. Until this is resolved, 
devfreq simply cannot set some of the defined OPPs.

This issue was there from the beginning, recent Kamil's patch only 
revealed it. In fact it was even worse - devfreq and common clock 
framework silently set lower clock than the given OPP defined.

>> Fixes: 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()")
>> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>> ---
>>   drivers/devfreq/exynos-bus.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index c832673273a2..37bd34d5625b 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -243,7 +243,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>   {
>>   	struct device *dev = bus->dev;
>>   	struct dev_pm_opp *opp;
>> -	unsigned long rate;
>> +	unsigned long rate, opp_rate;
>>   	int ret;
>>   
>>   	/* Get the clock to provide each bus with source clock */
>> @@ -267,13 +267,21 @@ static int exynos_bus_parse_of(struct device_node *np,
>>   	}
>>   
>>   	rate = clk_get_rate(bus->clk);
>> -
>> -	opp = devfreq_recommended_opp(dev, &rate, 0);
>> +	opp_rate = rate;
>> +	opp = devfreq_recommended_opp(dev, &opp_rate, 0);
>>   	if (IS_ERR(opp)) {
>>   		dev_err(dev, "failed to find dev_pm_opp\n");
>>   		ret = PTR_ERR(opp);
>>   		goto err_opp;
>>   	}
>> +	/*
>> +	 * FIXME: U-boot leaves clock source at incorrect PLL, this results
>> +	 * in clock rate outside defined OPP rate. Work around this bug by
>> +	 * setting clock rate to recommended one.
>> +	 */
>> +	if (rate > opp_rate)
>> +		clk_set_rate(bus->clk, opp_rate);
>> +
>>   	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>   	dev_pm_opp_put(opp);
>>   
>>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Chanwoo Choi]

Hi Marek,

On 19. 10. 11. 오후 8:33, Marek Szyprowski wrote:
> Hi Chanwoo,
> 
> On 10.10.2019 04:50, Chanwoo Choi wrote:
>> On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
>>> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
>>> dev_pm_opp_set_rate()") introduced errors:
>>> 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)
>>>
>>> They are caused by incorrect PLL assigned to clock source, which results
>>> in clock rate outside of OPP range. Add workaround for this in
>>> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.
>> If the clock caused this issue, you can set the initial clock on DeviceTree
>> with assigned-clock-* properties. Because the probe time of clock driver
>> is early than the any device drivers.
>>
>> It is not proper to fix the clock issue on other device driver.
>> I think you can fix it by using the supported clock properties.
> 
> This issue is about something completely different. The OPPs defined in 
> DT cannot be applied, because it is not possible to derive the needed 
> clock rate from the bootloader-configured clock topology (mainly due to 
> lack of common divisor values for some of the parent clocks). Some time 
> ago Lukasz tried initially to redefine this clock topology using 
> assigned-clock-rates/parents properties (see 
> https://protect2.fireeye.com/url?k=4b80c0304459bc8e.4b814b7f-f87f1e1aee1a85c0&u=https://lkml.org/lkml/2019/7/15/276), but it has limitations and some 
> such changes has to be done in bootloader. Until this is resolved, 
> devfreq simply cannot set some of the defined OPPs.

As you mentioned, the wrong setting in bootloader cause the this issue.
So, this patch change the rate on exynos-bus.c in order to fix
the issue with workaround style. 

But, also, it can be fixed by initializing the clock rate on DT
although it is not fundamental solution as you mentioned.

If above two method are workaround way, I think that set the clock
rate in DT is proper. The role of 'assigned-clock-*' properties
is for this case in order to set the initial frequency on probe time.

I think that the previous patch[1] of Kamil Konieczny is missing
the patches which initialize the clock rate on DT file.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4294a779bd8dff6c65e7e85ffe7a1ea236e92a68

> 
> This issue was there from the beginning, recent Kamil's patch only 
> revealed it. In fact it was even worse - devfreq and common clock 
> framework silently set lower clock than the given OPP defined.
> 
>>> Fixes: 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()")
>>> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>> ---
>>>   drivers/devfreq/exynos-bus.c | 14 +++++++++++---
>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index c832673273a2..37bd34d5625b 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -243,7 +243,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>   {
>>>   	struct device *dev = bus->dev;
>>>   	struct dev_pm_opp *opp;
>>> -	unsigned long rate;
>>> +	unsigned long rate, opp_rate;
>>>   	int ret;
>>>   
>>>   	/* Get the clock to provide each bus with source clock */
>>> @@ -267,13 +267,21 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>   	}
>>>   
>>>   	rate = clk_get_rate(bus->clk);
>>> -
>>> -	opp = devfreq_recommended_opp(dev, &rate, 0);
>>> +	opp_rate = rate;
>>> +	opp = devfreq_recommended_opp(dev, &opp_rate, 0);
>>>   	if (IS_ERR(opp)) {
>>>   		dev_err(dev, "failed to find dev_pm_opp\n");
>>>   		ret = PTR_ERR(opp);
>>>   		goto err_opp;
>>>   	}
>>> +	/*
>>> +	 * FIXME: U-boot leaves clock source at incorrect PLL, this results
>>> +	 * in clock rate outside defined OPP rate. Work around this bug by
>>> +	 * setting clock rate to recommended one.
>>> +	 */
>>> +	if (rate > opp_rate)
>>> +		clk_set_rate(bus->clk, opp_rate);
>>> +
>>>   	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>   	dev_pm_opp_put(opp);
>>>   
>>>
>>
> Best regards
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Kamil Konieczny]

Hi Chanwoo,

On 14.10.2019 08:46, Chanwoo Choi wrote:
> Hi Marek,
> 
> On 19. 10. 11. 오후 8:33, Marek Szyprowski wrote:
>> Hi Chanwoo,
>>
>> On 10.10.2019 04:50, Chanwoo Choi wrote:
>>> On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
>>>> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
>>>> dev_pm_opp_set_rate()") introduced errors:
>>>> 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)
>>>>
>>>> They are caused by incorrect PLL assigned to clock source, which results
>>>> in clock rate outside of OPP range. Add workaround for this in
>>>> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.
>>> If the clock caused this issue, you can set the initial clock on DeviceTree
>>> with assigned-clock-* properties. Because the probe time of clock driver
>>> is early than the any device drivers.
>>>
>>> It is not proper to fix the clock issue on other device driver.
>>> I think you can fix it by using the supported clock properties.
>>
>> This issue is about something completely different. The OPPs defined in 
>> DT cannot be applied, because it is not possible to derive the needed 
>> clock rate from the bootloader-configured clock topology (mainly due to 
>> lack of common divisor values for some of the parent clocks). Some time 
>> ago Lukasz tried initially to redefine this clock topology using 
>> assigned-clock-rates/parents properties (see 
>> https://protect2.fireeye.com/url?k=4b80c0304459bc8e.4b814b7f-f87f1e1aee1a85c0&u=https://lkml.org/lkml/2019/7/15/276), but it has limitations and some 
>> such changes has to be done in bootloader. Until this is resolved, 
>> devfreq simply cannot set some of the defined OPPs.
> 
> As you mentioned, the wrong setting in bootloader cause the this issue.
> So, this patch change the rate on exynos-bus.c in order to fix
> the issue with workaround style. 
> 
> But, also, it can be fixed by initializing the clock rate on DT
> although it is not fundamental solution as you mentioned.
> 
> If above two method are workaround way, I think that set the clock
> rate in DT is proper. The role of 'assigned-clock-*' properties
> is for this case in order to set the initial frequency on probe time.

I can add 'assigned-clock-*' to DT, but the issue is caused in opp points,
so the warning from exynos-bus will still be there.

Before this fix, devfreq will issue warning and then change clock to max
frequency within opp range. This fix mask warning, and as Marek and
Lukasz Luba wrotes, the proper fix will be to make changes in u-boot
(and connect proper PLLs to IPs).

Second solution would be to write down new OPP points with currently used
frequencies, and with max one for 532 MHz.

> I think that the previous patch[1] of Kamil Konieczny is missing
> the patches which initialize the clock rate on DT file.
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4294a779bd8dff6c65e7e85ffe7a1ea236e92a68
> 
>>
>> This issue was there from the beginning, recent Kamil's patch only 
>> revealed it. In fact it was even worse - devfreq and common clock 
>> framework silently set lower clock than the given OPP defined.
>>
>>>> Fixes: 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()")
>>>> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>> ---
>>>>   drivers/devfreq/exynos-bus.c | 14 +++++++++++---
>>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>> index c832673273a2..37bd34d5625b 100644
>>>> --- a/drivers/devfreq/exynos-bus.c
>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>> @@ -243,7 +243,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>   {
>>>>   	struct device *dev = bus->dev;
>>>>   	struct dev_pm_opp *opp;
>>>> -	unsigned long rate;
>>>> +	unsigned long rate, opp_rate;
>>>>   	int ret;
>>>>   
>>>>   	/* Get the clock to provide each bus with source clock */
>>>> @@ -267,13 +267,21 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>   	}
>>>>   
>>>>   	rate = clk_get_rate(bus->clk);
>>>> -
>>>> -	opp = devfreq_recommended_opp(dev, &rate, 0);
>>>> +	opp_rate = rate;
>>>> +	opp = devfreq_recommended_opp(dev, &opp_rate, 0);
>>>>   	if (IS_ERR(opp)) {
>>>>   		dev_err(dev, "failed to find dev_pm_opp\n");
>>>>   		ret = PTR_ERR(opp);
>>>>   		goto err_opp;
>>>>   	}
>>>> +	/*
>>>> +	 * FIXME: U-boot leaves clock source at incorrect PLL, this results
>>>> +	 * in clock rate outside defined OPP rate. Work around this bug by
>>>> +	 * setting clock rate to recommended one.
>>>> +	 */
>>>> +	if (rate > opp_rate)
>>>> +		clk_set_rate(bus->clk, opp_rate);
>>>> +
>>>>   	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>   	dev_pm_opp_put(opp);
>>>>   
>>>>
>>>
>> Best regards
>>
> 
> 

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Chanwoo Choi]

Hi Kamil,

On 11/14/19 12:12 AM, Kamil Konieczny wrote:
> Hi Chanwoo,
> 
> On 14.10.2019 08:46, Chanwoo Choi wrote:
>> Hi Marek,
>>
>> On 19. 10. 11. 오후 8:33, Marek Szyprowski wrote:
>>> Hi Chanwoo,
>>>
>>> On 10.10.2019 04:50, Chanwoo Choi wrote:
>>>> On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
>>>>> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
>>>>> dev_pm_opp_set_rate()") introduced errors:
>>>>> 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)
>>>>>
>>>>> They are caused by incorrect PLL assigned to clock source, which results
>>>>> in clock rate outside of OPP range. Add workaround for this in
>>>>> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.
>>>> If the clock caused this issue, you can set the initial clock on DeviceTree
>>>> with assigned-clock-* properties. Because the probe time of clock driver
>>>> is early than the any device drivers.
>>>>
>>>> It is not proper to fix the clock issue on other device driver.
>>>> I think you can fix it by using the supported clock properties.
>>>
>>> This issue is about something completely different. The OPPs defined in 
>>> DT cannot be applied, because it is not possible to derive the needed 
>>> clock rate from the bootloader-configured clock topology (mainly due to 
>>> lack of common divisor values for some of the parent clocks). Some time 
>>> ago Lukasz tried initially to redefine this clock topology using 
>>> assigned-clock-rates/parents properties (see 
>>> https://protect2.fireeye.com/url?k=4b80c0304459bc8e.4b814b7f-f87f1e1aee1a85c0&u=https://lkml.org/lkml/2019/7/15/276), but it has limitations and some 
>>> such changes has to be done in bootloader. Until this is resolved, 
>>> devfreq simply cannot set some of the defined OPPs.
>>
>> As you mentioned, the wrong setting in bootloader cause the this issue.
>> So, this patch change the rate on exynos-bus.c in order to fix
>> the issue with workaround style. 
>>
>> But, also, it can be fixed by initializing the clock rate on DT
>> although it is not fundamental solution as you mentioned.
>>
>> If above two method are workaround way, I think that set the clock
>> rate in DT is proper. The role of 'assigned-clock-*' properties
>> is for this case in order to set the initial frequency on probe time.
> 
> I can add 'assigned-clock-*' to DT, but the issue is caused in opp points,
> so the warning from exynos-bus will still be there.
> 
> Before this fix, devfreq will issue warning and then change clock to max
> frequency within opp range. This fix mask warning, and as Marek and
> Lukasz Luba wrotes, the proper fix will be to make changes in u-boot
> (and connect proper PLLs to IPs).

PLL could be changed by clock device driver in the linux kernel.
If you don't add the supported frequency into PLL frequency table 
of clock device driver, will fail to change the wanted frequency
on the linux kernel. It means that it is not fixed by only touching
the bootloader. 

As you commented, the wrong opp points which are specified on dt
cause this issue. Usually, have to initialize the clock rate on dt
by using 'assigned-clocks-*' property and then use the clock
with the preferable clock rate. I think that we have to fix
the fundamental problem. 

Without bootloader problem, you can fix it by initializing
the clock on dt with 'assigned-clocks-*' property.

As I knew that it is correct way and I always tried to do this method
for resolving the similar clock issue.

Lastly, I think that my opinion is more simple and correct.
It could give the more correct information to linux kernel user
which refer to the device tree file.

1. Your suggestion 
	a. Add opp-table with unsupported frequency on dt
	b. Try to change the clock rate on exynos-bus.c by using unsupported frequency from opp-table
	c. If failed, retry to change the clock rate on exynos-bus.c

2. My opinion
	a. Initialize the PLL or any clock by using assigned-clock-* property on dt
	   and add opp-table with supported frequency on dt
	b. Try to change the clock rate on exynos-bus.c by using supported frequency from opp-table

> 
> Second solution would be to write down new OPP points with currently used
> frequencies, and with max one for 532 MHz.
> 
>> I think that the previous patch[1] of Kamil Konieczny is missing
>> the patches which initialize the clock rate on DT file.
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4294a779bd8dff6c65e7e85ffe7a1ea236e92a68
>>
>>>
>>> This issue was there from the beginning, recent Kamil's patch only 
>>> revealed it. In fact it was even worse - devfreq and common clock 
>>> framework silently set lower clock than the given OPP defined.
>>>
>>>>> Fixes: 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()")
>>>>> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>>> ---
>>>>>   drivers/devfreq/exynos-bus.c | 14 +++++++++++---
>>>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>> index c832673273a2..37bd34d5625b 100644
>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>> @@ -243,7 +243,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>   {
>>>>>   	struct device *dev = bus->dev;
>>>>>   	struct dev_pm_opp *opp;
>>>>> -	unsigned long rate;
>>>>> +	unsigned long rate, opp_rate;
>>>>>   	int ret;
>>>>>   
>>>>>   	/* Get the clock to provide each bus with source clock */
>>>>> @@ -267,13 +267,21 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>   	}
>>>>>   
>>>>>   	rate = clk_get_rate(bus->clk);
>>>>> -
>>>>> -	opp = devfreq_recommended_opp(dev, &rate, 0);
>>>>> +	opp_rate = rate;
>>>>> +	opp = devfreq_recommended_opp(dev, &opp_rate, 0);
>>>>>   	if (IS_ERR(opp)) {
>>>>>   		dev_err(dev, "failed to find dev_pm_opp\n");
>>>>>   		ret = PTR_ERR(opp);
>>>>>   		goto err_opp;
>>>>>   	}
>>>>> +	/*
>>>>> +	 * FIXME: U-boot leaves clock source at incorrect PLL, this results
>>>>> +	 * in clock rate outside defined OPP rate. Work around this bug by
>>>>> +	 * setting clock rate to recommended one.
>>>>> +	 */
>>>>> +	if (rate > opp_rate)
>>>>> +		clk_set_rate(bus->clk, opp_rate);
>>>>> +
>>>>>   	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>>   	dev_pm_opp_put(opp);
>>>>>   
>>>>>
>>>>
>>> Best regards
>>>
>>
>>
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Chanwoo Choi]

Hi Kamil,

On 11/14/19 3:07 PM, Chanwoo Choi wrote:
> Hi Kamil,
> 
> On 11/14/19 12:12 AM, Kamil Konieczny wrote:
>> Hi Chanwoo,
>>
>> On 14.10.2019 08:46, Chanwoo Choi wrote:
>>> Hi Marek,
>>>
>>> On 19. 10. 11. 오후 8:33, Marek Szyprowski wrote:
>>>> Hi Chanwoo,
>>>>
>>>> On 10.10.2019 04:50, Chanwoo Choi wrote:
>>>>> On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
>>>>>> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
>>>>>> dev_pm_opp_set_rate()") introduced errors:
>>>>>> 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)
>>>>>>
>>>>>> They are caused by incorrect PLL assigned to clock source, which results
>>>>>> in clock rate outside of OPP range. Add workaround for this in
>>>>>> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.
>>>>> If the clock caused this issue, you can set the initial clock on DeviceTree
>>>>> with assigned-clock-* properties. Because the probe time of clock driver
>>>>> is early than the any device drivers.
>>>>>
>>>>> It is not proper to fix the clock issue on other device driver.
>>>>> I think you can fix it by using the supported clock properties.
>>>>
>>>> This issue is about something completely different. The OPPs defined in 
>>>> DT cannot be applied, because it is not possible to derive the needed 
>>>> clock rate from the bootloader-configured clock topology (mainly due to 
>>>> lack of common divisor values for some of the parent clocks). Some time 
>>>> ago Lukasz tried initially to redefine this clock topology using 
>>>> assigned-clock-rates/parents properties (see 
>>>> https://protect2.fireeye.com/url?k=4b80c0304459bc8e.4b814b7f-f87f1e1aee1a85c0&u=https://lkml.org/lkml/2019/7/15/276), but it has limitations and some 
>>>> such changes has to be done in bootloader. Until this is resolved, 
>>>> devfreq simply cannot set some of the defined OPPs.
>>>
>>> As you mentioned, the wrong setting in bootloader cause the this issue.
>>> So, this patch change the rate on exynos-bus.c in order to fix
>>> the issue with workaround style. 
>>>
>>> But, also, it can be fixed by initializing the clock rate on DT
>>> although it is not fundamental solution as you mentioned.
>>>
>>> If above two method are workaround way, I think that set the clock
>>> rate in DT is proper. The role of 'assigned-clock-*' properties
>>> is for this case in order to set the initial frequency on probe time.
>>
>> I can add 'assigned-clock-*' to DT, but the issue is caused in opp points,
>> so the warning from exynos-bus will still be there.
>>
>> Before this fix, devfreq will issue warning and then change clock to max
>> frequency within opp range. This fix mask warning, and as Marek and
>> Lukasz Luba wrotes, the proper fix will be to make changes in u-boot
>> (and connect proper PLLs to IPs).
> 
> PLL could be changed by clock device driver in the linux kernel.
> If you don't add the supported frequency into PLL frequency table 
> of clock device driver, will fail to change the wanted frequency
> on the linux kernel. It means that it is not fixed by only touching
> the bootloader. 
> 
> As you commented, the wrong opp points which are specified on dt
> cause this issue. Usually, have to initialize the clock rate on dt
> by using 'assigned-clocks-*' property and then use the clock
> with the preferable clock rate. I think that we have to fix
> the fundamental problem. 
> 
> Without bootloader problem, you can fix it by initializing
> the clock on dt with 'assigned-clocks-*' property.
> 
> As I knew that it is correct way and I always tried to do this method
> for resolving the similar clock issue.
> 
> Lastly, I think that my opinion is more simple and correct.
> It could give the more correct information to linux kernel user
> which refer to the device tree file.
> 
> 1. Your suggestion 
> 	a. Add opp-table with unsupported frequency on dt
> 	b. Try to change the clock rate on exynos-bus.c by using unsupported frequency from opp-table
> 	c. If failed, retry to change the clock rate on exynos-bus.c
> 
> 2. My opinion
> 	a. Initialize the PLL or any clock by using assigned-clock-* property on dt
> 	   and add opp-table with supported frequency on dt
> 	b. Try to change the clock rate on exynos-bus.c by using supported frequency from opp-table
> 

Just I tried to add 'assigned-clock-rates' property to initialize
the clock rate of some bus node as following on odroid-xu3 board:

diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
index 829147e320e0..9a237af5436a 100644
--- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
+++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
@@ -42,6 +42,8 @@
 };
 
 &bus_wcore {
+       assigned-clocks = <&clock CLK_DOUT_ACLK400_WCORE>;
+       assigned-clock-rates = <400000000>;
        devfreq-events = <&nocp_mem0_0>, <&nocp_mem0_1>,
                        <&nocp_mem1_0>, <&nocp_mem1_1>;
        vdd-supply = <&buck3_reg>;
@@ -50,11 +52,15 @@
 };
 
 &bus_noc {
+       assigned-clocks = <&clock CLK_DOUT_ACLK100_NOC>;
+       assigned-clock-rates = <100000000>;
        devfreq = <&bus_wcore>;
        status = "okay";
 };
 
 &bus_fsys_apb {
+       assigned-clocks = <&clock CLK_DOUT_PCLK200_FSYS>;
+       assigned-clock-rates = <200000000>;
        devfreq = <&bus_wcore>;
        status = "okay";
 };
@@ -120,6 +126,8 @@
 };
 
 &bus_mscl {
+       assigned-clocks = <&clock CLK_DOUT_ACLK400_MSCL>;
+       assigned-clock-rates = <400000000>;
        devfreq = <&bus_wcore>;
        status = "okay";
 };


In result,
[Before on v5.4-rc6, failed to set the rate by dev_pm_opp_set_rate()]
[    4.855811] exynos-bus: new bus device registered: soc:bus_wcore ( 84000 KHz ~ 400000 KHz)
[    4.863374] exynos-bus: new bus device registered: soc:bus_noc ( 67000 KHz ~ 100000 KHz)
[    4.871240] exynos-bus: new bus device registered: soc:bus_fsys_apb (100000 KHz ~ 200000 KHz)
[    4.879509] exynos-bus: new bus device registered: soc:bus_fsys (100000 KHz ~ 200000 KHz)
[    4.887957] exynos-bus: new bus device registered: soc:bus_fsys2 ( 75000 KHz ~ 150000 KHz)
[    4.896361] exynos-bus: new bus device registered: soc:bus_mfc ( 96000 KHz ~ 333000 KHz)
[    4.904330] exynos-bus: new bus device registered: soc:bus_gen ( 89000 KHz ~ 267000 KHz)
[    4.911802] exynos-bus soc:bus_wcore: dev_pm_opp_set_rate: failed to find current OPP for freq 532000000 (-34)
[    4.912710] exynos-bus: new bus device registered: soc:bus_peri ( 67000 KHz ~  67000 KHz)
[    4.924655] exynos-bus soc:bus_noc: dev_pm_opp_set_rate: failed to find current OPP for freq 111000000 (-34)
[    4.932125] exynos-bus: new bus device registered: soc:bus_g2d ( 84000 KHz ~ 333000 KHz)
[    4.939607] exynos-bus soc:bus_fsys_apb: dev_pm_opp_set_rate: failed to find current OPP for freq 222000000 (-34)
[    4.949758] exynos-bus: new bus device registered: soc:bus_g2d_acp ( 67000 KHz ~ 267000 KHz)
[    4.966991] exynos-bus: new bus device registered: soc:bus_jpeg ( 75000 KHz ~ 300000 KHz)
[    4.975136] exynos-bus: new bus device registered: soc:bus_jpeg_apb ( 84000 KHz ~ 167000 KHz)
[    4.983452] exynos-bus: new bus device registered: soc:bus_disp1_fimd (120000 KHz ~ 200000 KHz)
[    4.992218] exynos-bus: new bus device registered: soc:bus_disp1 (120000 KHz ~ 300000 KHz)
[    5.000483] exynos-bus: new bus device registered: soc:bus_gscl_scaler (150000 KHz ~ 300000 KHz)
[    5.009331] exynos-bus: new bus device registered: soc:bus_mscl ( 84000 KHz ~ 400000 KHz)
[    5.020207] exynos-bus soc:bus_mscl: dev_pm_opp_set_rate: failed to find current OPP for freq 666000000 (-34)

[After applied the 'assigned-clock-*' patch on v5.4-rc6]
[    4.840571] exynos-bus: new bus device registered: soc:bus_wcore ( 84000 KHz ~ 400000 KHz)
[    4.848099] exynos-bus: new bus device registered: soc:bus_noc ( 67000 KHz ~ 100000 KHz)
[    4.856016] exynos-bus: new bus device registered: soc:bus_fsys_apb (100000 KHz ~ 200000 KHz)
[    4.864307] exynos-bus: new bus device registered: soc:bus_fsys (100000 KHz ~ 200000 KHz)
[    4.872723] exynos-bus: new bus device registered: soc:bus_fsys2 ( 75000 KHz ~ 150000 KHz)
[    4.881124] exynos-bus: new bus device registered: soc:bus_mfc ( 96000 KHz ~ 333000 KHz)
[    4.889147] exynos-bus: new bus device registered: soc:bus_gen ( 89000 KHz ~ 267000 KHz)
[    4.896867] exynos-bus: new bus device registered: soc:bus_peri ( 67000 KHz ~  67000 KHz)
[    4.907430] exynos-bus: new bus device registered: soc:bus_g2d ( 84000 KHz ~ 333000 KHz)
[    4.914797] exynos-bus: new bus device registered: soc:bus_g2d_acp ( 67000 KHz ~ 267000 KHz)
[    4.923205] exynos-bus: new bus device registered: soc:bus_jpeg ( 75000 KHz ~ 300000 KHz)
[    4.931352] exynos-bus: new bus device registered: soc:bus_jpeg_apb ( 84000 KHz ~ 167000 KHz)
[    4.939658] exynos-bus: new bus device registered: soc:bus_disp1_fimd (120000 KHz ~ 200000 KHz)
[    4.948401] exynos-bus: new bus device registered: soc:bus_disp1 (120000 KHz ~ 300000 KHz)
[    4.956650] exynos-bus: new bus device registered: soc:bus_gscl_scaler (150000 KHz ~ 300000 KHz)
[    4.965573] exynos-bus: new bus device registered: soc:bus_mscl ( 84000 KHz ~ 400000 KHz)


>>
>> Second solution would be to write down new OPP points with currently used
>> frequencies, and with max one for 532 MHz.
>>
>>> I think that the previous patch[1] of Kamil Konieczny is missing
>>> the patches which initialize the clock rate on DT file.
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4294a779bd8dff6c65e7e85ffe7a1ea236e92a68
>>>
>>>>
>>>> This issue was there from the beginning, recent Kamil's patch only 
>>>> revealed it. In fact it was even worse - devfreq and common clock 
>>>> framework silently set lower clock than the given OPP defined.
>>>>
>>>>>> Fixes: 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()")
>>>>>> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
>>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>>>> ---
>>>>>>   drivers/devfreq/exynos-bus.c | 14 +++++++++++---
>>>>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>>> index c832673273a2..37bd34d5625b 100644
>>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>>> @@ -243,7 +243,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>   {
>>>>>>   	struct device *dev = bus->dev;
>>>>>>   	struct dev_pm_opp *opp;
>>>>>> -	unsigned long rate;
>>>>>> +	unsigned long rate, opp_rate;
>>>>>>   	int ret;
>>>>>>   
>>>>>>   	/* Get the clock to provide each bus with source clock */
>>>>>> @@ -267,13 +267,21 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>   	}
>>>>>>   
>>>>>>   	rate = clk_get_rate(bus->clk);
>>>>>> -
>>>>>> -	opp = devfreq_recommended_opp(dev, &rate, 0);
>>>>>> +	opp_rate = rate;
>>>>>> +	opp = devfreq_recommended_opp(dev, &opp_rate, 0);
>>>>>>   	if (IS_ERR(opp)) {
>>>>>>   		dev_err(dev, "failed to find dev_pm_opp\n");
>>>>>>   		ret = PTR_ERR(opp);
>>>>>>   		goto err_opp;
>>>>>>   	}
>>>>>> +	/*
>>>>>> +	 * FIXME: U-boot leaves clock source at incorrect PLL, this results
>>>>>> +	 * in clock rate outside defined OPP rate. Work around this bug by
>>>>>> +	 * setting clock rate to recommended one.
>>>>>> +	 */
>>>>>> +	if (rate > opp_rate)
>>>>>> +		clk_set_rate(bus->clk, opp_rate);
>>>>>> +
>>>>>>   	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>>>   	dev_pm_opp_put(opp);
>>>>>>   
>>>>>>
>>>>>
>>>> Best regards
>>>>
>>>
>>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Chanwoo Choi]

Hi Kamil,

On 11/14/19 4:38 PM, Chanwoo Choi wrote:
> Hi Kamil,
> 
> On 11/14/19 3:07 PM, Chanwoo Choi wrote:
>> Hi Kamil,
>>
>> On 11/14/19 12:12 AM, Kamil Konieczny wrote:
>>> Hi Chanwoo,
>>>
>>> On 14.10.2019 08:46, Chanwoo Choi wrote:
>>>> Hi Marek,
>>>>
>>>> On 19. 10. 11. 오후 8:33, Marek Szyprowski wrote:
>>>>> Hi Chanwoo,
>>>>>
>>>>> On 10.10.2019 04:50, Chanwoo Choi wrote:
>>>>>> On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
>>>>>>> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
>>>>>>> dev_pm_opp_set_rate()") introduced errors:
>>>>>>> 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)
>>>>>>>
>>>>>>> They are caused by incorrect PLL assigned to clock source, which results
>>>>>>> in clock rate outside of OPP range. Add workaround for this in
>>>>>>> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.
>>>>>> If the clock caused this issue, you can set the initial clock on DeviceTree
>>>>>> with assigned-clock-* properties. Because the probe time of clock driver
>>>>>> is early than the any device drivers.
>>>>>>
>>>>>> It is not proper to fix the clock issue on other device driver.
>>>>>> I think you can fix it by using the supported clock properties.
>>>>>
>>>>> This issue is about something completely different. The OPPs defined in 
>>>>> DT cannot be applied, because it is not possible to derive the needed 
>>>>> clock rate from the bootloader-configured clock topology (mainly due to 
>>>>> lack of common divisor values for some of the parent clocks). Some time 
>>>>> ago Lukasz tried initially to redefine this clock topology using 
>>>>> assigned-clock-rates/parents properties (see 
>>>>> https://protect2.fireeye.com/url?k=4b80c0304459bc8e.4b814b7f-f87f1e1aee1a85c0&u=https://lkml.org/lkml/2019/7/15/276), but it has limitations and some 
>>>>> such changes has to be done in bootloader. Until this is resolved, 
>>>>> devfreq simply cannot set some of the defined OPPs.
>>>>
>>>> As you mentioned, the wrong setting in bootloader cause the this issue.
>>>> So, this patch change the rate on exynos-bus.c in order to fix
>>>> the issue with workaround style. 
>>>>
>>>> But, also, it can be fixed by initializing the clock rate on DT
>>>> although it is not fundamental solution as you mentioned.
>>>>
>>>> If above two method are workaround way, I think that set the clock
>>>> rate in DT is proper. The role of 'assigned-clock-*' properties
>>>> is for this case in order to set the initial frequency on probe time.
>>>
>>> I can add 'assigned-clock-*' to DT, but the issue is caused in opp points,
>>> so the warning from exynos-bus will still be there.
>>>
>>> Before this fix, devfreq will issue warning and then change clock to max
>>> frequency within opp range. This fix mask warning, and as Marek and
>>> Lukasz Luba wrotes, the proper fix will be to make changes in u-boot
>>> (and connect proper PLLs to IPs).
>>
>> PLL could be changed by clock device driver in the linux kernel.
>> If you don't add the supported frequency into PLL frequency table 
>> of clock device driver, will fail to change the wanted frequency
>> on the linux kernel. It means that it is not fixed by only touching
>> the bootloader. 
>>
>> As you commented, the wrong opp points which are specified on dt
>> cause this issue. Usually, have to initialize the clock rate on dt
>> by using 'assigned-clocks-*' property and then use the clock
>> with the preferable clock rate. I think that we have to fix
>> the fundamental problem. 
>>
>> Without bootloader problem, you can fix it by initializing
>> the clock on dt with 'assigned-clocks-*' property.
>>
>> As I knew that it is correct way and I always tried to do this method
>> for resolving the similar clock issue.
>>
>> Lastly, I think that my opinion is more simple and correct.
>> It could give the more correct information to linux kernel user
>> which refer to the device tree file.
>>
>> 1. Your suggestion 
>> 	a. Add opp-table with unsupported frequency on dt
>> 	b. Try to change the clock rate on exynos-bus.c by using unsupported frequency from opp-table
>> 	c. If failed, retry to change the clock rate on exynos-bus.c
>>
>> 2. My opinion
>> 	a. Initialize the PLL or any clock by using assigned-clock-* property on dt
>> 	   and add opp-table with supported frequency on dt
>> 	b. Try to change the clock rate on exynos-bus.c by using supported frequency from opp-table
>>
> 
> Just I tried to add 'assigned-clock-rates' property to initialize
> the clock rate of some bus node as following on odroid-xu3 board:
> 
> diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> index 829147e320e0..9a237af5436a 100644
> --- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> +++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> @@ -42,6 +42,8 @@
>  };
>  
>  &bus_wcore {
> +       assigned-clocks = <&clock CLK_DOUT_ACLK400_WCORE>;
> +       assigned-clock-rates = <400000000>;
>         devfreq-events = <&nocp_mem0_0>, <&nocp_mem0_1>,
>                         <&nocp_mem1_0>, <&nocp_mem1_1>;
>         vdd-supply = <&buck3_reg>;
> @@ -50,11 +52,15 @@
>  };
>  
>  &bus_noc {
> +       assigned-clocks = <&clock CLK_DOUT_ACLK100_NOC>;
> +       assigned-clock-rates = <100000000>;
>         devfreq = <&bus_wcore>;
>         status = "okay";
>  };
>  
>  &bus_fsys_apb {
> +       assigned-clocks = <&clock CLK_DOUT_PCLK200_FSYS>;
> +       assigned-clock-rates = <200000000>;
>         devfreq = <&bus_wcore>;
>         status = "okay";
>  };
> @@ -120,6 +126,8 @@
>  };
>  
>  &bus_mscl {
> +       assigned-clocks = <&clock CLK_DOUT_ACLK400_MSCL>;
> +       assigned-clock-rates = <400000000>;
>         devfreq = <&bus_wcore>;
>         status = "okay";
>  };
> 
> 
> In result,
> [Before on v5.4-rc6, failed to set the rate by dev_pm_opp_set_rate()]
> [    4.855811] exynos-bus: new bus device registered: soc:bus_wcore ( 84000 KHz ~ 400000 KHz)
> [    4.863374] exynos-bus: new bus device registered: soc:bus_noc ( 67000 KHz ~ 100000 KHz)
> [    4.871240] exynos-bus: new bus device registered: soc:bus_fsys_apb (100000 KHz ~ 200000 KHz)
> [    4.879509] exynos-bus: new bus device registered: soc:bus_fsys (100000 KHz ~ 200000 KHz)
> [    4.887957] exynos-bus: new bus device registered: soc:bus_fsys2 ( 75000 KHz ~ 150000 KHz)
> [    4.896361] exynos-bus: new bus device registered: soc:bus_mfc ( 96000 KHz ~ 333000 KHz)
> [    4.904330] exynos-bus: new bus device registered: soc:bus_gen ( 89000 KHz ~ 267000 KHz)
> [    4.911802] exynos-bus soc:bus_wcore: dev_pm_opp_set_rate: failed to find current OPP for freq 532000000 (-34)
> [    4.912710] exynos-bus: new bus device registered: soc:bus_peri ( 67000 KHz ~  67000 KHz)
> [    4.924655] exynos-bus soc:bus_noc: dev_pm_opp_set_rate: failed to find current OPP for freq 111000000 (-34)
> [    4.932125] exynos-bus: new bus device registered: soc:bus_g2d ( 84000 KHz ~ 333000 KHz)
> [    4.939607] exynos-bus soc:bus_fsys_apb: dev_pm_opp_set_rate: failed to find current OPP for freq 222000000 (-34)
> [    4.949758] exynos-bus: new bus device registered: soc:bus_g2d_acp ( 67000 KHz ~ 267000 KHz)
> [    4.966991] exynos-bus: new bus device registered: soc:bus_jpeg ( 75000 KHz ~ 300000 KHz)
> [    4.975136] exynos-bus: new bus device registered: soc:bus_jpeg_apb ( 84000 KHz ~ 167000 KHz)
> [    4.983452] exynos-bus: new bus device registered: soc:bus_disp1_fimd (120000 KHz ~ 200000 KHz)
> [    4.992218] exynos-bus: new bus device registered: soc:bus_disp1 (120000 KHz ~ 300000 KHz)
> [    5.000483] exynos-bus: new bus device registered: soc:bus_gscl_scaler (150000 KHz ~ 300000 KHz)
> [    5.009331] exynos-bus: new bus device registered: soc:bus_mscl ( 84000 KHz ~ 400000 KHz)
> [    5.020207] exynos-bus soc:bus_mscl: dev_pm_opp_set_rate: failed to find current OPP for freq 666000000 (-34)
> 
> [After applied the 'assigned-clock-*' patch on v5.4-rc6]
> [    4.840571] exynos-bus: new bus device registered: soc:bus_wcore ( 84000 KHz ~ 400000 KHz)
> [    4.848099] exynos-bus: new bus device registered: soc:bus_noc ( 67000 KHz ~ 100000 KHz)
> [    4.856016] exynos-bus: new bus device registered: soc:bus_fsys_apb (100000 KHz ~ 200000 KHz)
> [    4.864307] exynos-bus: new bus device registered: soc:bus_fsys (100000 KHz ~ 200000 KHz)
> [    4.872723] exynos-bus: new bus device registered: soc:bus_fsys2 ( 75000 KHz ~ 150000 KHz)
> [    4.881124] exynos-bus: new bus device registered: soc:bus_mfc ( 96000 KHz ~ 333000 KHz)
> [    4.889147] exynos-bus: new bus device registered: soc:bus_gen ( 89000 KHz ~ 267000 KHz)
> [    4.896867] exynos-bus: new bus device registered: soc:bus_peri ( 67000 KHz ~  67000 KHz)
> [    4.907430] exynos-bus: new bus device registered: soc:bus_g2d ( 84000 KHz ~ 333000 KHz)
> [    4.914797] exynos-bus: new bus device registered: soc:bus_g2d_acp ( 67000 KHz ~ 267000 KHz)
> [    4.923205] exynos-bus: new bus device registered: soc:bus_jpeg ( 75000 KHz ~ 300000 KHz)
> [    4.931352] exynos-bus: new bus device registered: soc:bus_jpeg_apb ( 84000 KHz ~ 167000 KHz)
> [    4.939658] exynos-bus: new bus device registered: soc:bus_disp1_fimd (120000 KHz ~ 200000 KHz)
> [    4.948401] exynos-bus: new bus device registered: soc:bus_disp1 (120000 KHz ~ 300000 KHz)
> [    4.956650] exynos-bus: new bus device registered: soc:bus_gscl_scaler (150000 KHz ~ 300000 KHz)
> [    4.965573] exynos-bus: new bus device registered: soc:bus_mscl ( 84000 KHz ~ 400000 KHz)
> 

Actually, I don't want to leave this problem on mainline.
We have to need to fix the fail of exynos-bus registration for kernel booting.
Please reply your opinion or send the fix-up patch as I commented above.

> 
>>>
>>> Second solution would be to write down new OPP points with currently used
>>> frequencies, and with max one for 532 MHz.
>>>
>>>> I think that the previous patch[1] of Kamil Konieczny is missing
>>>> the patches which initialize the clock rate on DT file.
>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4294a779bd8dff6c65e7e85ffe7a1ea236e92a68
>>>>
>>>>>
>>>>> This issue was there from the beginning, recent Kamil's patch only 
>>>>> revealed it. In fact it was even worse - devfreq and common clock 
>>>>> framework silently set lower clock than the given OPP defined.
>>>>>
>>>>>>> Fixes: 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()")
>>>>>>> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
>>>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>>>>> ---
>>>>>>>   drivers/devfreq/exynos-bus.c | 14 +++++++++++---
>>>>>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>>>> index c832673273a2..37bd34d5625b 100644
>>>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>>>> @@ -243,7 +243,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>>   {
>>>>>>>   	struct device *dev = bus->dev;
>>>>>>>   	struct dev_pm_opp *opp;
>>>>>>> -	unsigned long rate;
>>>>>>> +	unsigned long rate, opp_rate;
>>>>>>>   	int ret;
>>>>>>>   
>>>>>>>   	/* Get the clock to provide each bus with source clock */
>>>>>>> @@ -267,13 +267,21 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>>   	}
>>>>>>>   
>>>>>>>   	rate = clk_get_rate(bus->clk);
>>>>>>> -
>>>>>>> -	opp = devfreq_recommended_opp(dev, &rate, 0);
>>>>>>> +	opp_rate = rate;
>>>>>>> +	opp = devfreq_recommended_opp(dev, &opp_rate, 0);
>>>>>>>   	if (IS_ERR(opp)) {
>>>>>>>   		dev_err(dev, "failed to find dev_pm_opp\n");
>>>>>>>   		ret = PTR_ERR(opp);
>>>>>>>   		goto err_opp;
>>>>>>>   	}
>>>>>>> +	/*
>>>>>>> +	 * FIXME: U-boot leaves clock source at incorrect PLL, this results
>>>>>>> +	 * in clock rate outside defined OPP rate. Work around this bug by
>>>>>>> +	 * setting clock rate to recommended one.
>>>>>>> +	 */
>>>>>>> +	if (rate > opp_rate)
>>>>>>> +		clk_set_rate(bus->clk, opp_rate);
>>>>>>> +
>>>>>>>   	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>>>>   	dev_pm_opp_put(opp);
>>>>>>>   
>>>>>>>
>>>>>>
>>>>> Best regards
>>>>>
>>>>
>>>>
>>>
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Marek Szyprowski]

Hi

On 14.11.2019 08:38, Chanwoo Choi wrote:
> On 11/14/19 3:07 PM, Chanwoo Choi wrote:
>> On 11/14/19 12:12 AM, Kamil Konieczny wrote:
>>> On 14.10.2019 08:46, Chanwoo Choi wrote:
>>>> On 19. 10. 11. 오후 8:33, Marek Szyprowski wrote:
>>>>> On 10.10.2019 04:50, Chanwoo Choi wrote:
>>>>>> On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
>>>>>>> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
>>>>>>> dev_pm_opp_set_rate()") introduced errors:
>>>>>>> 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)
>>>>>>>
>>>>>>> They are caused by incorrect PLL assigned to clock source, which results
>>>>>>> in clock rate outside of OPP range. Add workaround for this in
>>>>>>> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.
>>>>>> If the clock caused this issue, you can set the initial clock on DeviceTree
>>>>>> with assigned-clock-* properties. Because the probe time of clock driver
>>>>>> is early than the any device drivers.
>>>>>>
>>>>>> It is not proper to fix the clock issue on other device driver.
>>>>>> I think you can fix it by using the supported clock properties.
>>>>> This issue is about something completely different. The OPPs defined in
>>>>> DT cannot be applied, because it is not possible to derive the needed
>>>>> clock rate from the bootloader-configured clock topology (mainly due to
>>>>> lack of common divisor values for some of the parent clocks). Some time
>>>>> ago Lukasz tried initially to redefine this clock topology using
>>>>> assigned-clock-rates/parents properties (see
>>>>> https://protect2.fireeye.com/url?k=4b80c0304459bc8e.4b814b7f-f87f1e1aee1a85c0&u=https://lkml.org/lkml/2019/7/15/276), but it has limitations and some
>>>>> such changes has to be done in bootloader. Until this is resolved,
>>>>> devfreq simply cannot set some of the defined OPPs.
>>>> As you mentioned, the wrong setting in bootloader cause the this issue.
>>>> So, this patch change the rate on exynos-bus.c in order to fix
>>>> the issue with workaround style.
>>>>
>>>> But, also, it can be fixed by initializing the clock rate on DT
>>>> although it is not fundamental solution as you mentioned.
>>>>
>>>> If above two method are workaround way, I think that set the clock
>>>> rate in DT is proper. The role of 'assigned-clock-*' properties
>>>> is for this case in order to set the initial frequency on probe time.
>>> I can add 'assigned-clock-*' to DT, but the issue is caused in opp points,
>>> so the warning from exynos-bus will still be there.
>>>
>>> Before this fix, devfreq will issue warning and then change clock to max
>>> frequency within opp range. This fix mask warning, and as Marek and
>>> Lukasz Luba wrotes, the proper fix will be to make changes in u-boot
>>> (and connect proper PLLs to IPs).
>> PLL could be changed by clock device driver in the linux kernel.
>> If you don't add the supported frequency into PLL frequency table
>> of clock device driver, will fail to change the wanted frequency
>> on the linux kernel. It means that it is not fixed by only touching
>> the bootloader.
>>
>> As you commented, the wrong opp points which are specified on dt
>> cause this issue. Usually, have to initialize the clock rate on dt
>> by using 'assigned-clocks-*' property and then use the clock
>> with the preferable clock rate. I think that we have to fix
>> the fundamental problem.
>>
>> Without bootloader problem, you can fix it by initializing
>> the clock on dt with 'assigned-clocks-*' property.
>>
>> As I knew that it is correct way and I always tried to do this method
>> for resolving the similar clock issue.
>>
>> Lastly, I think that my opinion is more simple and correct.
>> It could give the more correct information to linux kernel user
>> which refer to the device tree file.
>>
>> 1. Your suggestion
>> 	a. Add opp-table with unsupported frequency on dt
>> 	b. Try to change the clock rate on exynos-bus.c by using unsupported frequency from opp-table
>> 	c. If failed, retry to change the clock rate on exynos-bus.c
>>
>> 2. My opinion
>> 	a. Initialize the PLL or any clock by using assigned-clock-* property on dt
>> 	   and add opp-table with supported frequency on dt
>> 	b. Try to change the clock rate on exynos-bus.c by using supported frequency from opp-table
>>
> Just I tried to add 'assigned-clock-rates' property to initialize
> the clock rate of some bus node as following on odroid-xu3 board:
>
> diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> index 829147e320e0..9a237af5436a 100644
> --- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> +++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> @@ -42,6 +42,8 @@
>   };
>   
>   &bus_wcore {
> +       assigned-clocks = <&clock CLK_DOUT_ACLK400_WCORE>;
> +       assigned-clock-rates = <400000000>;
>          devfreq-events = <&nocp_mem0_0>, <&nocp_mem0_1>,
>                          <&nocp_mem1_0>, <&nocp_mem1_1>;
>          vdd-supply = <&buck3_reg>;
> @@ -50,11 +52,15 @@
>   };
>   
>   &bus_noc {
> +       assigned-clocks = <&clock CLK_DOUT_ACLK100_NOC>;
> +       assigned-clock-rates = <100000000>;
>          devfreq = <&bus_wcore>;
>          status = "okay";
>   };
>   
>   &bus_fsys_apb {
> +       assigned-clocks = <&clock CLK_DOUT_PCLK200_FSYS>;
> +       assigned-clock-rates = <200000000>;
>          devfreq = <&bus_wcore>;
>          status = "okay";
>   };
> @@ -120,6 +126,8 @@
>   };
>   
>   &bus_mscl {
> +       assigned-clocks = <&clock CLK_DOUT_ACLK400_MSCL>;
> +       assigned-clock-rates = <400000000>;
>          devfreq = <&bus_wcore>;
>          status = "okay";
>   };


Well, this is a poor workaround. There is indeed no warning, but the 
clock rates are far from the specified in the device tree. For WCORE 
assigned-clock-rates = <400000000> on Odroid XU3/XU4 kernel will set 
dout_aclk400_wcore clock to 266MHz, because it is not possible to derive 
400MHz from 532MHz MPLL...

I plan to measure the impact of different rates on the performance of 
the various components and overall power consumption. Only then IMHO it 
makes sense to decide if we really should adjust OPPs to the current 
PLLs configuration (-> basically define following OPPs for WCORE: 
532MHz, 266MHz, 133MHZ and 77MHz) or change PLL configuration and 
re-parent WCORE to 1200MHz to properly drive: 400MHz, 300MHz, 200MHz and 
100MHz.

Other devfreq buses should IMHO use the values similar to the selected 
for WCORE.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Chanwoo Choi]

Hi Marek,

On 12/5/19 8:23 PM, Marek Szyprowski wrote:
> Hi
> 
> On 14.11.2019 08:38, Chanwoo Choi wrote:
>> On 11/14/19 3:07 PM, Chanwoo Choi wrote:
>>> On 11/14/19 12:12 AM, Kamil Konieczny wrote:
>>>> On 14.10.2019 08:46, Chanwoo Choi wrote:
>>>>> On 19. 10. 11. 오후 8:33, Marek Szyprowski wrote:
>>>>>> On 10.10.2019 04:50, Chanwoo Choi wrote:
>>>>>>> On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
>>>>>>>> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
>>>>>>>> dev_pm_opp_set_rate()") introduced errors:
>>>>>>>> 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)
>>>>>>>>
>>>>>>>> They are caused by incorrect PLL assigned to clock source, which results
>>>>>>>> in clock rate outside of OPP range. Add workaround for this in
>>>>>>>> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.
>>>>>>> If the clock caused this issue, you can set the initial clock on DeviceTree
>>>>>>> with assigned-clock-* properties. Because the probe time of clock driver
>>>>>>> is early than the any device drivers.
>>>>>>>
>>>>>>> It is not proper to fix the clock issue on other device driver.
>>>>>>> I think you can fix it by using the supported clock properties.
>>>>>> This issue is about something completely different. The OPPs defined in
>>>>>> DT cannot be applied, because it is not possible to derive the needed
>>>>>> clock rate from the bootloader-configured clock topology (mainly due to
>>>>>> lack of common divisor values for some of the parent clocks). Some time
>>>>>> ago Lukasz tried initially to redefine this clock topology using
>>>>>> assigned-clock-rates/parents properties (see
>>>>>> https://protect2.fireeye.com/url?k=4b80c0304459bc8e.4b814b7f-f87f1e1aee1a85c0&u=https://lkml.org/lkml/2019/7/15/276), but it has limitations and some
>>>>>> such changes has to be done in bootloader. Until this is resolved,
>>>>>> devfreq simply cannot set some of the defined OPPs.
>>>>> As you mentioned, the wrong setting in bootloader cause the this issue.
>>>>> So, this patch change the rate on exynos-bus.c in order to fix
>>>>> the issue with workaround style.
>>>>>
>>>>> But, also, it can be fixed by initializing the clock rate on DT
>>>>> although it is not fundamental solution as you mentioned.
>>>>>
>>>>> If above two method are workaround way, I think that set the clock
>>>>> rate in DT is proper. The role of 'assigned-clock-*' properties
>>>>> is for this case in order to set the initial frequency on probe time.
>>>> I can add 'assigned-clock-*' to DT, but the issue is caused in opp points,
>>>> so the warning from exynos-bus will still be there.
>>>>
>>>> Before this fix, devfreq will issue warning and then change clock to max
>>>> frequency within opp range. This fix mask warning, and as Marek and
>>>> Lukasz Luba wrotes, the proper fix will be to make changes in u-boot
>>>> (and connect proper PLLs to IPs).
>>> PLL could be changed by clock device driver in the linux kernel.
>>> If you don't add the supported frequency into PLL frequency table
>>> of clock device driver, will fail to change the wanted frequency
>>> on the linux kernel. It means that it is not fixed by only touching
>>> the bootloader.
>>>
>>> As you commented, the wrong opp points which are specified on dt
>>> cause this issue. Usually, have to initialize the clock rate on dt
>>> by using 'assigned-clocks-*' property and then use the clock
>>> with the preferable clock rate. I think that we have to fix
>>> the fundamental problem.
>>>
>>> Without bootloader problem, you can fix it by initializing
>>> the clock on dt with 'assigned-clocks-*' property.
>>>
>>> As I knew that it is correct way and I always tried to do this method
>>> for resolving the similar clock issue.
>>>
>>> Lastly, I think that my opinion is more simple and correct.
>>> It could give the more correct information to linux kernel user
>>> which refer to the device tree file.
>>>
>>> 1. Your suggestion
>>> 	a. Add opp-table with unsupported frequency on dt
>>> 	b. Try to change the clock rate on exynos-bus.c by using unsupported frequency from opp-table
>>> 	c. If failed, retry to change the clock rate on exynos-bus.c
>>>
>>> 2. My opinion
>>> 	a. Initialize the PLL or any clock by using assigned-clock-* property on dt
>>> 	   and add opp-table with supported frequency on dt
>>> 	b. Try to change the clock rate on exynos-bus.c by using supported frequency from opp-table
>>>
>> Just I tried to add 'assigned-clock-rates' property to initialize
>> the clock rate of some bus node as following on odroid-xu3 board:
>>
>> diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
>> index 829147e320e0..9a237af5436a 100644
>> --- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
>> +++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
>> @@ -42,6 +42,8 @@
>>   };
>>   
>>   &bus_wcore {
>> +       assigned-clocks = <&clock CLK_DOUT_ACLK400_WCORE>;
>> +       assigned-clock-rates = <400000000>;
>>          devfreq-events = <&nocp_mem0_0>, <&nocp_mem0_1>,
>>                          <&nocp_mem1_0>, <&nocp_mem1_1>;
>>          vdd-supply = <&buck3_reg>;
>> @@ -50,11 +52,15 @@
>>   };
>>   
>>   &bus_noc {
>> +       assigned-clocks = <&clock CLK_DOUT_ACLK100_NOC>;
>> +       assigned-clock-rates = <100000000>;
>>          devfreq = <&bus_wcore>;
>>          status = "okay";
>>   };
>>   
>>   &bus_fsys_apb {
>> +       assigned-clocks = <&clock CLK_DOUT_PCLK200_FSYS>;
>> +       assigned-clock-rates = <200000000>;
>>          devfreq = <&bus_wcore>;
>>          status = "okay";
>>   };
>> @@ -120,6 +126,8 @@
>>   };
>>   
>>   &bus_mscl {
>> +       assigned-clocks = <&clock CLK_DOUT_ACLK400_MSCL>;
>> +       assigned-clock-rates = <400000000>;
>>          devfreq = <&bus_wcore>;
>>          status = "okay";
>>   };
> 
> 
> Well, this is a poor workaround. There is indeed no warning, but the 
> clock rates are far from the specified in the device tree. For WCORE 
> assigned-clock-rates = <400000000> on Odroid XU3/XU4 kernel will set 
> dout_aclk400_wcore clock to 266MHz, because it is not possible to derive 
> 400MHz from 532MHz MPLL...

Ah. You're right. It seems that my mistake of original patch of bus_wcore_opp_table.
I think that OPP table has the wrong OPP entries.

> 
> I plan to measure the impact of different rates on the performance of 
> the various components and overall power consumption. Only then IMHO it 
> makes sense to decide if we really should adjust OPPs to the current 
> PLLs configuration (-> basically define following OPPs for WCORE: 
> 532MHz, 266MHz, 133MHZ and 77MHz) or change PLL configuration and 
> re-parent WCORE to 1200MHz to properly drive: 400MHz, 300MHz, 200MHz and 
> 100MHz.
I agree your both suggestions. 

> 
> Other devfreq buses should IMHO use the values similar to the selected 
> for WCORE.
> 
> Best regards
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] devfreq: exynos-bus: workaround dev_pm_opp_set_rate() errors on Exynos5422/5800 SoCs

[Chanwoo Choi]

Hi Kamil,

Ping.

On 10/10/19 11:50 AM, Chanwoo Choi wrote:
> On 2019년 10월 08일 22:49, k.konieczny@partner.samsung.com wrote:
>> Commit 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use
>> dev_pm_opp_set_rate()") introduced errors:
>> 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)
>>
>> They are caused by incorrect PLL assigned to clock source, which results
>> in clock rate outside of OPP range. Add workaround for this in
>> exynos_bus_parse_of() by adjusting clock rate to those present in OPP.
> 
> If the clock caused this issue, you can set the initial clock on DeviceTree
> with assigned-clock-* properties. Because the probe time of clock driver
> is early than the any device drivers.
> 
> It is not proper to fix the clock issue on other device driver.
> I think you can fix it by using the supported clock properties.

Is there any reply? or other patch to fix it?

> 
> 
>>
>> Fixes: 4294a779bd8d ("PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()")
>> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>> ---
>>  drivers/devfreq/exynos-bus.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index c832673273a2..37bd34d5625b 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -243,7 +243,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>  {
>>  	struct device *dev = bus->dev;
>>  	struct dev_pm_opp *opp;
>> -	unsigned long rate;
>> +	unsigned long rate, opp_rate;
>>  	int ret;
>>  
>>  	/* Get the clock to provide each bus with source clock */
>> @@ -267,13 +267,21 @@ static int exynos_bus_parse_of(struct device_node *np,
>>  	}
>>  
>>  	rate = clk_get_rate(bus->clk);
>> -
>> -	opp = devfreq_recommended_opp(dev, &rate, 0);
>> +	opp_rate = rate;
>> +	opp = devfreq_recommended_opp(dev, &opp_rate, 0);
>>  	if (IS_ERR(opp)) {
>>  		dev_err(dev, "failed to find dev_pm_opp\n");
>>  		ret = PTR_ERR(opp);
>>  		goto err_opp;
>>  	}
>> +	/*
>> +	 * FIXME: U-boot leaves clock source at incorrect PLL, this results
>> +	 * in clock rate outside defined OPP rate. Work around this bug by
>> +	 * setting clock rate to recommended one.
>> +	 */
>> +	if (rate > opp_rate)
>> +		clk_set_rate(bus->clk, opp_rate);
>> +
>>  	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>  	dev_pm_opp_put(opp);
>>  
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics