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@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxx>
>>>> Signed-off-by: Kamil Konieczny <k.konieczny@xxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>>   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