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

[Chanwoo Choi <cw00.choi@samsung.com>]

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