diff mbox

cpufreq: frequency scaling spec in DT node

Message ID c0838592-5613-660d-56fc-e7022a38a86d@free.fr (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Mason July 11, 2017, 11:09 a.m. UTC
On 11/07/2017 12:25, Viresh Kumar wrote:
> On 11-07-17, 11:27, Mason wrote:
>> On 29/06/2017 16:34, Viresh Kumar wrote:
>>
>>> On 29-06-17, 13:41, Mason wrote:
>>>
>>>> I was trying to "emulate" the behavior of the ondemand governor.
>>>> Based on your reaction, I got it wrong...
>>>> Here is the actual issue:
>>>>
>>>> I'm on SoC B, where nominal/max freq is expected to be 1206 MHz.
>>>> So the OPPs in the DT are:
>>>> operating-points = <1206000 0 603000 0 402000 0 241200 0 134000 0>;
>>>> *But* FW changed the max freq behind my back, to 1215 MHz.
> 
> What does this line mean really? Where is this frequency changed ? In the OPP
> table in DT?

I apologize for being unclear.

What I meant is that the bootloader originally set the max frequency
to 1206 MHz. The OPP table in DTS was written based on that value.

Later, someone changed the bootloader code to set a slightly higher
max frequency. When I flashed the new bootloader on my board, the
OPP table no longer matches the actual frequency.

But I am not notified when bootloader authors change max frequencies,
which is why I wrote "changed the max freq behind my back".

Again, sorry for the confusing statements.

(The bootloader is not DT-aware, so it leaves the DT untouched.)

>>>> Here is what happens when I execute:
>>>> echo ondemand >scaling_governor
>>>> sleep 2
>>>> cpuburn-a9 & cpuburn-a9 & cpuburn-a9 & cpuburn-a9
>>>> ### cpuburn-a9 spins in a tight infinite loop,
>>>> ### hitting all FUs to raise the CPU temperature
>>>>
>>>> # cpufreq_test.sh
>>>> [   69.933874] set_target: index=4
>>>> [   69.944799] set_target: index=2
>>>> [   69.947988] clk_divider_set_rate: rate=303750000 parent_rate=1215000000 div=4
>>>> [   69.955542] set_target: index=4
>>>> [   69.958801] clk_divider_set_rate: rate=607500000 parent_rate=1215000000 div=2
>>>> [   69.984789] set_target: index=0
>>>> [   69.987980] clk_divider_set_rate: rate=121500000 parent_rate=1215000000 div=10
>>>> [   71.947597] set_target: index=4
>>>> [   71.950996] clk_divider_set_rate: rate=607500000 parent_rate=1215000000 div=2
>>>>
>>>> As you can see, the divider remains stuck at 2, so the SoC
>>>> is actually running only at 607.5 MHz (instead of 1215 MHz).
>>>>
>>>> If I fix the OPPs in DT to:
>>>> operating-points = <1215000 0 607500 0 405000 0 243000 0 135000 0>;
>>>> Then I get the expected behavior:
>>>>
>>>> $ cpufreq_test.sh 
>>>> [   32.717930] set_target: index=1
>>>> [   32.721131] clk_divider_set_rate: rate=243000000 parent_rate=1215000000 div=5
>>>> [   32.731326] set_target: index=4
>>>> [   32.734521] clk_divider_set_rate: rate=1215000000 parent_rate=1215000000 div=1
>>>> [   32.754556] set_target: index=0
>>>> [   32.757738] clk_divider_set_rate: rate=135000000 parent_rate=1215000000 div=9
>>>> [   32.765864] set_target: index=4
>>>> [   32.769217] clk_divider_set_rate: rate=1215000000 parent_rate=1215000000 div=1
>>>> [   33.438811] set_target: index=0
>>>> [   33.442001] clk_divider_set_rate: rate=135000000 parent_rate=1215000000 div=9
>>>> [   33.450249] set_target: index=4
>>>> [   33.453470] clk_divider_set_rate: rate=1215000000 parent_rate=1215000000 div=1
>>>> [   33.477888] set_target: index=0
>>>> [   33.481067] clk_divider_set_rate: rate=135000000 parent_rate=1215000000 div=9
>>>> [   34.714786] set_target: index=4
>>>> [   34.718237] clk_divider_set_rate: rate=1215000000 parent_rate=1215000000 div=1
>>>>
>>>> Divider settles at 1 (full speed) to provide maximum
>>>> performance for the user-space processes.
>>>
>>> I am not sure how such behavior will happen just because we changed
>>> the max OPP (actually increased it). You need to dig in a bit to see
>>> why this happens, as I can't agree to your numbers for now.
>>
>> I had a closer look.
>>
>> static int _div_round(...)
>> {
>> 	if (flags & CLK_DIVIDER_ROUND_CLOSEST)
>> 		return _div_round_closest(table, parent_rate, rate, flags);
>>
>> 	return _div_round_up(table, parent_rate, rate, flags);
>> }
>>
>> This flag was /not/ set for the CPU divider clock.
> 
> i.e. _div_round_up() was getting called ? And you failed to explain why do you
> think this results in that awkward behavior.

Yes _div_round_up() was being called

[   10.631624] _div_round_up: parent_rate=1215000000 rate=1206000000 div=2
[   10.648229] clk_divider_set_rate: rate=607500000 parent_rate=1215000000 div=2

	int div = DIV_ROUND_UP_ULL((u64)parent_rate, rate);

Since the divider is rounded up, and parent_rate > rate,
we get a divider of 2, so 607.5 MHz

All dividers (d + epsilon) are rounded to d+1

>> But setting it breaks in dev_pm_opp_set_rate()
>>
>> [    9.201681] set_target: index=4
>> [    9.204870] dev_pm_opp_set_rate: target_freq=1206000000 freq=1215000000 old_freq=243000000
> 
> I have some assumptions on how you are printing this line and will present my
> analysis based on that.



> cpufreq core asked for a freq of 1206 (why?, DT should have had 1215 as max),

That's the source of the problem: DT has 1206 as max.
(Because max freq was changed in the bootloader, and the DT
was not updated.)

Maybe this is the real issue that needs to be addressed,
rather than the symptoms that turn up later because of
the root issue.

What's your take?

Regards.

Comments

Mason July 11, 2017, 11:56 a.m. UTC | #1
On 11/07/2017 13:09, Mason wrote:
> On 11/07/2017 12:25, Viresh Kumar wrote:
>> On 11-07-17, 11:27, Mason wrote:
>>> On 29/06/2017 16:34, Viresh Kumar wrote:
>>>
>>>> On 29-06-17, 13:41, Mason wrote:
>>>>
>>>>> I'm on SoC B, where nominal/max freq is expected to be 1206 MHz.
>>>>> So the OPPs in the DT are:
>>>>> operating-points = <1206000 0 603000 0 402000 0 241200 0 134000 0>;
>>>>> *But* FW changed the max freq behind my back, to 1215 MHz.
>>
>> What does this line mean really? Where is this frequency changed?
>> In the OPP table in DT?
> 
> I apologize for being unclear.
> 
> What I meant is that the bootloader originally set the max frequency
> to 1206 MHz. The OPP table in DTS was written based on that value.
> 
> Later, someone changed the bootloader code to set a slightly higher
> max frequency. When I flashed the new bootloader on my board, the
> OPP table no longer matches the actual frequency.
> 
> But I am not notified when bootloader authors change max frequencies,
> which is why I wrote "changed the max freq behind my back".
> 
> Again, sorry for the confusing statements.
> 
> (The bootloader is not DT-aware, so it leaves the DT untouched.)

I just realized that there's another unstated assumption.

Since the bootloader is not DT-aware, the DTB is, in fact, appended
to the kernel image. This is why it's possible to have "mismatching"
kernel and bootloader.

Regards.
Viresh Kumar July 12, 2017, 3:41 a.m. UTC | #2
On 11-07-17, 13:09, Mason wrote:
> I apologize for being unclear.
> 
> What I meant is that the bootloader originally set the max frequency
> to 1206 MHz. The OPP table in DTS was written based on that value.
> 
> Later, someone changed the bootloader code to set a slightly higher
> max frequency. When I flashed the new bootloader on my board, the
> OPP table no longer matches the actual frequency.
> 
> But I am not notified when bootloader authors change max frequencies,
> which is why I wrote "changed the max freq behind my back".
> 
> Again, sorry for the confusing statements.
> 
> (The bootloader is not DT-aware, so it leaves the DT untouched.)

Here we go. Finally I have understood what the problem you are facing is :)
And yes, it was really not clear to me until now. I though that someone just
changed the max in DT and that's making things go bad :)

Anyway, how does the bootloader control the max frequency? For the boards I
worked on, its just a PLL that the kernel needs to set and kernel can choose to
program it the way it wants to irrespective of the way bootloader has worked on
it.

> Maybe this is the real issue that needs to be addressed,
> rather than the symptoms that turn up later because of
> the root issue.

Sure, I just need a bit more of input from you :)
Mason July 12, 2017, 9:58 a.m. UTC | #3
On 12/07/2017 05:41, Viresh Kumar wrote:

> On 11-07-17, 13:09, Mason wrote:
>
>> What I meant is that the bootloader originally set the max frequency
>> to 1206 MHz. The OPP table in DTS was written based on that value.
>>
>> Later, someone changed the bootloader code to set a slightly higher
>> max frequency. When I flashed the new bootloader on my board, the
>> OPP table no longer matches the actual frequency.
>>
>> But I am not notified when bootloader authors change max frequencies,
>> which is why I wrote "changed the max freq behind my back".
>>
>> (The bootloader is not DT-aware, so it leaves the DT untouched.)
> 
> Here we go. Finally I have understood what the problem you are facing is :)
> And yes, it was really not clear to me until now. I though that someone just
> changed the max in DT and that's making things go bad :)
> 
> Anyway, how does the bootloader control the max frequency? For the boards I
> worked on, it's just a PLL that the kernel needs to set and kernel can choose to
> program it the way it wants to irrespective of the way bootloader has worked on it.

I would object to the characterization of "just a PLL" :-)

The PLL outputs "garbage" before actually "locking" a target
frequency. It is not possible for the CPU to blindly change
the PLL settings, because that crashes the system.

The bootloader implements the steps required to change said
settings, so the strategy has been: have Linux use whatever
PLL frequency the bootloader programs.

Behind the PLL, there is a glitch-free divider, which is able
to divide the PLL output without crashing the system. I've
been using that divider for DFS.

drivers/clk/clk-tango4.c

Regards.
Viresh Kumar July 12, 2017, 10:09 a.m. UTC | #4
On 12-07-17, 11:58, Mason wrote:
> I would object to the characterization of "just a PLL" :-)
> 
> The PLL outputs "garbage" before actually "locking" a target
> frequency. It is not possible for the CPU to blindly change
> the PLL settings, because that crashes the system.
> 
> The bootloader implements the steps required to change said
> settings, so the strategy has been: have Linux use whatever
> PLL frequency the bootloader programs.
> 
> Behind the PLL, there is a glitch-free divider, which is able
> to divide the PLL output without crashing the system. I've
> been using that divider for DFS.
> 
> drivers/clk/clk-tango4.c

Okay, got it now.

Yes, you *really* need to create these OPPs dynamically. I am
convinced now :)
Mason July 12, 2017, 11:25 a.m. UTC | #5
On 12/07/2017 12:09, Viresh Kumar wrote:
> On 12-07-17, 11:58, Mason wrote:
>> I would object to the characterization of "just a PLL" :-)
>>
>> The PLL outputs "garbage" before actually "locking" a target
>> frequency. It is not possible for the CPU to blindly change
>> the PLL settings, because that crashes the system.
>>
>> The bootloader implements the steps required to change said
>> settings, so the strategy has been: have Linux use whatever
>> PLL frequency the bootloader programs.
>>
>> Behind the PLL, there is a glitch-free divider, which is able
>> to divide the PLL output without crashing the system. I've
>> been using that divider for DFS.
>>
>> drivers/clk/clk-tango4.c
> 
> Okay, got it now.
> 
> Yes, you *really* need to create these OPPs dynamically.
> I am convinced now :)

I will test your patch on my 4.9 branch.

Does tango-cpufreq.c look acceptable to you?
(I am aware the patch needs work in the the Kconfig/Makefile part.
That was quick and dirty.)

Regards.
Viresh Kumar July 12, 2017, 2:08 p.m. UTC | #6
On 12-07-17, 13:25, Mason wrote:
> I will test your patch on my 4.9 branch.
> 
> Does tango-cpufreq.c look acceptable to you?
> (I am aware the patch needs work in the the Kconfig/Makefile part.
> That was quick and dirty.)

Mostly yes. Just make sure everything is well handled when u send it.
diff mbox

Patch

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 6441dfda489f..53ec09eaa01c 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -603,6 +603,9 @@  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 
        old_freq = clk_get_rate(clk);
 
+       printk("%s: target_freq=%lu freq=%lu old_freq=%lu\n",
+                       __func__, target_freq, freq, old_freq);
+
        /* Return early if nothing to do */
        if (old_freq == freq) {
                dev_dbg(dev, "%s: old/new frequencies (%lu Hz) are same, nothing to do\n",