cpufreq: frequency scaling spec in DT node

cpufreq: frequency scaling spec in DT node

[Mason]

Hello,

I have two similar, but slightly different SoCs.

Firmware/bootloader sets the "nominal" CPU frequency to
- 1215 MHz on SoC A
- 1206 MHz on SoC B

On both systems, software can reduce the CPU frequency by
writing an 8-bit integer divider to an MMIO register.

Originally, I wanted to define a small number of operating points,
defined only by the divider value, and compute the actual OPP freq
at init.

For example, use { 1, 2, 3, 5, 9 } for dividers =>
1215, 607.5, 405, 243, 135 on SoC A
1206, 603, 402, 241.2, 134 on Soc B

I'm using the generic cpufreq driver.

Binding for the generic cpufreq driver:
https://www.kernel.org/doc/Documentation/devicetree/bindings/cpufreq/cpufreq-dt.txt

I don't think there's a way to do what I want with the
existing driver, right?

It's not a big deal, I can write the actual target frequencies
in the DT. (BTW, the OPPs are more SW than HW desc, right?)

But my problem is: what happens if firmware/bootloader is
changed without me knowing, and they change the nominal
frequency? Because of the rounding, if the nominal freq
is slightly increased, the SoC will start working at
*slower* speeds.

For example, if nominal is 1215, and I request 603, I will
actually get 405.

This effect can be seen if I define SoC B OPPs on SoC A:

$ cat scaling_available_frequencies
134000 241200 402000 603000 1206000 
/sys/devices/system/cpu/cpu0/cpufreq$ echo 603000 > scaling_max_freq
[   60.401883] set_target: index=3
[   60.405118] clk_divider_set_rate: rate=405000000 parent_rate=1215000000 div=3


What can I do against that?

Should I check the nominal frequency in my clk driver?
(I'm not sure reading properties of unrelated nodes is acceptable practice.)

Regards.

cpufreq: frequency scaling spec in DT node

[Mason]

Hello,

I have two similar, but slightly different SoCs.

Firmware/bootloader sets the "nominal" CPU frequency to
- 1215 MHz on SoC A
- 1206 MHz on SoC B

On both systems, software can reduce the CPU frequency by
writing an 8-bit integer divider to an MMIO register.

Originally, I wanted to define a small number of operating points,
defined only by the divider value, and compute the actual OPP freq
at init.

For example, use { 1, 2, 3, 5, 9 } for dividers =>
1215, 607.5, 405, 243, 135 on SoC A
1206, 603, 402, 241.2, 134 on Soc B

I'm using the generic cpufreq driver.

Binding for the generic cpufreq driver:
https://www.kernel.org/doc/Documentation/devicetree/bindings/cpufreq/cpufreq-dt.txt

I don't think there's a way to do what I want with the
existing driver, right?

It's not a big deal, I can write the actual target frequencies
in the DT. (BTW, the OPPs are more SW than HW desc, right?)

But my problem is: what happens if firmware/bootloader is
changed without me knowing, and they change the nominal
frequency? Because of the rounding, if the nominal freq
is slightly increased, the SoC will start working at
*slower* speeds.

For example, if nominal is 1215, and I request 603, I will
actually get 405.

This effect can be seen if I define SoC B OPPs on SoC A:

$ cat scaling_available_frequencies
134000 241200 402000 603000 1206000 
/sys/devices/system/cpu/cpu0/cpufreq$ echo 603000 > scaling_max_freq
[   60.401883] set_target: index=3
[   60.405118] clk_divider_set_rate: rate=405000000 parent_rate=1215000000 div=3


What can I do against that?

Should I check the nominal frequency in my clk driver?
(I'm not sure reading properties of unrelated nodes is acceptable practice.)

Regards.

Re: cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

On 29-06-17, 11:48, Mason wrote:
> Hello,
> 
> I have two similar, but slightly different SoCs.
> 
> Firmware/bootloader sets the "nominal" CPU frequency to

So nominal here is MAX cpu frequency.

> - 1215 MHz on SoC A
> - 1206 MHz on SoC B
> 
> On both systems, software can reduce the CPU frequency by
> writing an 8-bit integer divider to an MMIO register.
> 
> Originally, I wanted to define a small number of operating points,
> defined only by the divider value, and compute the actual OPP freq
> at init.
> 
> For example, use { 1, 2, 3, 5, 9 } for dividers =>
> 1215, 607.5, 405, 243, 135 on SoC A
> 1206, 603, 402, 241.2, 134 on Soc B
> 
> I'm using the generic cpufreq driver.
> 
> Binding for the generic cpufreq driver:
> https://www.kernel.org/doc/Documentation/devicetree/bindings/cpufreq/cpufreq-dt.txt
> 
> I don't think there's a way to do what I want with the
> existing driver, right?

No, you should rather use actual target frequency values.

> It's not a big deal, I can write the actual target frequencies
> in the DT.

Right.

> (BTW, the OPPs are more SW than HW desc, right?)

Hmm, I wouldn't say that exactly :)

What OPP contains is mostly defined by hardware, apart from the
frequency values we are talking about. And those are decided by the
boot loaders and they are like hardware to the kernel really. They
define hardware capabilities IOW.

If you want, you can actually try implementing a ->target() type
cpufreq driver instead of ->target_index() and you will be able to
select any frequency you want. But with the above example, what you
can select is Max divided by integer value and so you can have 9
different OPPs and reuse cpufreq-dt.

> But my problem is: what happens if firmware/bootloader is
> changed without me knowing, and they change the nominal
> frequency?

The kernel doesn't have any authority over what frequencies we are
allowed to use and we depend on the boot loader for that. If someone
changes that, screw him :)

> Because of the rounding, if the nominal freq
> is slightly increased, the SoC will start working at

              decreased ?

> *slower* speeds.
> 
> For example, if nominal is 1215, and I request 603, I will
> actually get 405.

No, you will normally get a frequency >= requested frequency with the
cpufreq governors we have.

> This effect can be seen if I define SoC B OPPs on SoC A:
> 
> $ cat scaling_available_frequencies
> 134000 241200 402000 603000 1206000 
> /sys/devices/system/cpu/cpu0/cpufreq$ echo 603000 > scaling_max_freq

Wow. This is not how you request a frequency. What you said here is
that the MAX frequency allowed now is 603000 instead of 1206000. And
because 603000 isn't a valid frequency, we go down to 405000.

So, you should try using the userspace governor and play with
scaling_setspeed sysfs file.

> [   60.401883] set_target: index=3
> [   60.405118] clk_divider_set_rate: rate=405000000 parent_rate=1215000000 div=3
> 
> 
> What can I do against that?
> 
> Should I check the nominal frequency in my clk driver?
> (I'm not sure reading properties of unrelated nodes is acceptable practice.)

We rely on the boot loader to get these details.

There is one thing you can do to avoid adding OPP entries in the DT.
You can rather add them dynamically with help of: dev_pm_opp_add() and
cpufreq-dt will continue to work with that too.

But you should understand how to use the sysfs interface first and
make sure you are doing the right thing.

-- 
viresh

cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

On 29-06-17, 11:48, Mason wrote:
> Hello,
> 
> I have two similar, but slightly different SoCs.
> 
> Firmware/bootloader sets the "nominal" CPU frequency to

So nominal here is MAX cpu frequency.

> - 1215 MHz on SoC A
> - 1206 MHz on SoC B
> 
> On both systems, software can reduce the CPU frequency by
> writing an 8-bit integer divider to an MMIO register.
> 
> Originally, I wanted to define a small number of operating points,
> defined only by the divider value, and compute the actual OPP freq
> at init.
> 
> For example, use { 1, 2, 3, 5, 9 } for dividers =>
> 1215, 607.5, 405, 243, 135 on SoC A
> 1206, 603, 402, 241.2, 134 on Soc B
> 
> I'm using the generic cpufreq driver.
> 
> Binding for the generic cpufreq driver:
> https://www.kernel.org/doc/Documentation/devicetree/bindings/cpufreq/cpufreq-dt.txt
> 
> I don't think there's a way to do what I want with the
> existing driver, right?

No, you should rather use actual target frequency values.

> It's not a big deal, I can write the actual target frequencies
> in the DT.

Right.

> (BTW, the OPPs are more SW than HW desc, right?)

Hmm, I wouldn't say that exactly :)

What OPP contains is mostly defined by hardware, apart from the
frequency values we are talking about. And those are decided by the
boot loaders and they are like hardware to the kernel really. They
define hardware capabilities IOW.

If you want, you can actually try implementing a ->target() type
cpufreq driver instead of ->target_index() and you will be able to
select any frequency you want. But with the above example, what you
can select is Max divided by integer value and so you can have 9
different OPPs and reuse cpufreq-dt.

> But my problem is: what happens if firmware/bootloader is
> changed without me knowing, and they change the nominal
> frequency?

The kernel doesn't have any authority over what frequencies we are
allowed to use and we depend on the boot loader for that. If someone
changes that, screw him :)

> Because of the rounding, if the nominal freq
> is slightly increased, the SoC will start working at

              decreased ?

> *slower* speeds.
> 
> For example, if nominal is 1215, and I request 603, I will
> actually get 405.

No, you will normally get a frequency >= requested frequency with the
cpufreq governors we have.

> This effect can be seen if I define SoC B OPPs on SoC A:
> 
> $ cat scaling_available_frequencies
> 134000 241200 402000 603000 1206000 
> /sys/devices/system/cpu/cpu0/cpufreq$ echo 603000 > scaling_max_freq

Wow. This is not how you request a frequency. What you said here is
that the MAX frequency allowed now is 603000 instead of 1206000. And
because 603000 isn't a valid frequency, we go down to 405000.

So, you should try using the userspace governor and play with
scaling_setspeed sysfs file.

> [   60.401883] set_target: index=3
> [   60.405118] clk_divider_set_rate: rate=405000000 parent_rate=1215000000 div=3
> 
> 
> What can I do against that?
> 
> Should I check the nominal frequency in my clk driver?
> (I'm not sure reading properties of unrelated nodes is acceptable practice.)

We rely on the boot loader to get these details.

There is one thing you can do to avoid adding OPP entries in the DT.
You can rather add them dynamically with help of: dev_pm_opp_add() and
cpufreq-dt will continue to work with that too.

But you should understand how to use the sysfs interface first and
make sure you are doing the right thing.

-- 
viresh

Re: cpufreq: frequency scaling spec in DT node

[Mason]

On 29/06/2017 12:04, Viresh Kumar wrote:

> On 29-06-17, 11:48, Mason wrote:
>
>> I have two similar, but slightly different SoCs.
>>
>> Firmware/bootloader sets the "nominal" CPU frequency to
> 
> So nominal here is MAX cpu frequency.
> 
>> - 1215 MHz on SoC A
>> - 1206 MHz on SoC B
>>
>> On both systems, software can reduce the CPU frequency by
>> writing an 8-bit integer divider to an MMIO register.
>>
>> Originally, I wanted to define a small number of operating points,
>> defined only by the divider value, and compute the actual OPP freq
>> at init.
>>
>> For example, use { 1, 2, 3, 5, 9 } for dividers =>
>> 1215, 607.5, 405, 243, 135 on SoC A
>> 1206, 603, 402, 241.2, 134 on Soc B
>>
>> I'm using the generic cpufreq driver.
>>
>> Binding for the generic cpufreq driver:
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/cpufreq/cpufreq-dt.txt
>>
>> I don't think there's a way to do what I want with the
>> existing driver, right?
> 
> No, you should rather use actual target frequency values.
> 
>> It's not a big deal, I can write the actual target frequencies
>> in the DT.
> 
> Right.
> 
>> (BTW, the OPPs are more SW than HW desc, right?)
> 
> Hmm, I wouldn't say that exactly :)
> 
> What OPP contains is mostly defined by hardware, apart from the
> frequency values we are talking about. And those are decided by the
> boot loaders and they are like hardware to the kernel really. They
> define hardware capabilities IOW.
> 
> If you want, you can actually try implementing a ->target() type
> cpufreq driver instead of ->target_index() and you will be able to
> select any frequency you want. But with the above example, what you
> can select is Max divided by integer value and so you can have 9
> different OPPs and reuse cpufreq-dt.
> 
>> But my problem is: what happens if firmware/bootloader is
>> changed without me knowing, and they change the nominal
>> frequency?
> 
> The kernel doesn't have any authority over what frequencies we are
> allowed to use and we depend on the boot loader for that. If someone
> changes that, screw him :)
> 
>> Because of the rounding, if the nominal freq
>> is slightly increased, the SoC will start working at
> 
>               decreased ?
> 
>> *slower* speeds.
>>
>> For example, if nominal is 1215, and I request 603, I will
>> actually get 405.
> 
> No, you will normally get a frequency >= requested frequency with the
> cpufreq governors we have.
> 
>> This effect can be seen if I define SoC B OPPs on SoC A:
>>
>> $ cat scaling_available_frequencies
>> 134000 241200 402000 603000 1206000 
>> /sys/devices/system/cpu/cpu0/cpufreq$ echo 603000 > scaling_max_freq
> 
> Wow. This is not how you request a frequency. What you said here is
> that the MAX frequency allowed now is 603000 instead of 1206000. And
> because 603000 isn't a valid frequency, we go down to 405000.
> 
> So, you should try using the userspace governor and play with
> scaling_setspeed sysfs file.

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.

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.

My concern is that if I don't check somewhere that the
nominal frequency is as expected in the DT, the CPU might
run slower than expected (max freq cut in half).

>> [   60.401883] set_target: index=3
>> [   60.405118] clk_divider_set_rate: rate=405000000 parent_rate=1215000000 div=3
>>
>>
>> What can I do against that?
>>
>> Should I check the nominal frequency in my clk driver?
>> (I'm not sure reading properties of unrelated nodes is acceptable practice.)
> 
> We rely on the boot loader to get these details.
> 
> There is one thing you can do to avoid adding OPP entries in the DT.
> You can rather add them dynamically with help of: dev_pm_opp_add() and
> cpufreq-dt will continue to work with that too.

In what driver should I call these... the clk driver?
(drivers/clk/tegra/cvb.c seems to be doind that)

A problem might arise when I need to do voltage scaling,
though, since I also need to specify voltages, right?

> But you should understand how to use the sysfs interface first and
> make sure you are doing the right thing.

You're talking about this document, right?
https://www.kernel.org/doc/Documentation/cpu-freq/user-guide.txt

Regards.

cpufreq: frequency scaling spec in DT node

[Mason]

On 29/06/2017 12:04, Viresh Kumar wrote:

> On 29-06-17, 11:48, Mason wrote:
>
>> I have two similar, but slightly different SoCs.
>>
>> Firmware/bootloader sets the "nominal" CPU frequency to
> 
> So nominal here is MAX cpu frequency.
> 
>> - 1215 MHz on SoC A
>> - 1206 MHz on SoC B
>>
>> On both systems, software can reduce the CPU frequency by
>> writing an 8-bit integer divider to an MMIO register.
>>
>> Originally, I wanted to define a small number of operating points,
>> defined only by the divider value, and compute the actual OPP freq
>> at init.
>>
>> For example, use { 1, 2, 3, 5, 9 } for dividers =>
>> 1215, 607.5, 405, 243, 135 on SoC A
>> 1206, 603, 402, 241.2, 134 on Soc B
>>
>> I'm using the generic cpufreq driver.
>>
>> Binding for the generic cpufreq driver:
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/cpufreq/cpufreq-dt.txt
>>
>> I don't think there's a way to do what I want with the
>> existing driver, right?
> 
> No, you should rather use actual target frequency values.
> 
>> It's not a big deal, I can write the actual target frequencies
>> in the DT.
> 
> Right.
> 
>> (BTW, the OPPs are more SW than HW desc, right?)
> 
> Hmm, I wouldn't say that exactly :)
> 
> What OPP contains is mostly defined by hardware, apart from the
> frequency values we are talking about. And those are decided by the
> boot loaders and they are like hardware to the kernel really. They
> define hardware capabilities IOW.
> 
> If you want, you can actually try implementing a ->target() type
> cpufreq driver instead of ->target_index() and you will be able to
> select any frequency you want. But with the above example, what you
> can select is Max divided by integer value and so you can have 9
> different OPPs and reuse cpufreq-dt.
> 
>> But my problem is: what happens if firmware/bootloader is
>> changed without me knowing, and they change the nominal
>> frequency?
> 
> The kernel doesn't have any authority over what frequencies we are
> allowed to use and we depend on the boot loader for that. If someone
> changes that, screw him :)
> 
>> Because of the rounding, if the nominal freq
>> is slightly increased, the SoC will start working at
> 
>               decreased ?
> 
>> *slower* speeds.
>>
>> For example, if nominal is 1215, and I request 603, I will
>> actually get 405.
> 
> No, you will normally get a frequency >= requested frequency with the
> cpufreq governors we have.
> 
>> This effect can be seen if I define SoC B OPPs on SoC A:
>>
>> $ cat scaling_available_frequencies
>> 134000 241200 402000 603000 1206000 
>> /sys/devices/system/cpu/cpu0/cpufreq$ echo 603000 > scaling_max_freq
> 
> Wow. This is not how you request a frequency. What you said here is
> that the MAX frequency allowed now is 603000 instead of 1206000. And
> because 603000 isn't a valid frequency, we go down to 405000.
> 
> So, you should try using the userspace governor and play with
> scaling_setspeed sysfs file.

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.

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.

My concern is that if I don't check somewhere that the
nominal frequency is as expected in the DT, the CPU might
run slower than expected (max freq cut in half).

>> [   60.401883] set_target: index=3
>> [   60.405118] clk_divider_set_rate: rate=405000000 parent_rate=1215000000 div=3
>>
>>
>> What can I do against that?
>>
>> Should I check the nominal frequency in my clk driver?
>> (I'm not sure reading properties of unrelated nodes is acceptable practice.)
> 
> We rely on the boot loader to get these details.
> 
> There is one thing you can do to avoid adding OPP entries in the DT.
> You can rather add them dynamically with help of: dev_pm_opp_add() and
> cpufreq-dt will continue to work with that too.

In what driver should I call these... the clk driver?
(drivers/clk/tegra/cvb.c seems to be doind that)

A problem might arise when I need to do voltage scaling,
though, since I also need to specify voltages, right?

> But you should understand how to use the sysfs interface first and
> make sure you are doing the right thing.

You're talking about this document, right?
https://www.kernel.org/doc/Documentation/cpu-freq/user-guide.txt

Regards.

Re: cpufreq: frequency scaling spec in DT node

[Mason]

On 29/06/2017 13:41, Mason wrote:

> On 29/06/2017 12:04, Viresh Kumar wrote:
> 
>> There is one thing you can do to avoid adding OPP entries in the DT.
>> You can rather add them dynamically with help of: dev_pm_opp_add() and
>> cpufreq-dt will continue to work with that too.
> 
> In what driver should I call these... the clk driver?
> (drivers/clk/tegra/cvb.c seems to be doind that)

The problem I run into is that calling get_cpu_device(0) from the
clk driver returns NULL, because topology_init() has not run yet
(to initialize the cpu_sys_devices).

So the OPP table needs to be built *after* topology_init() but
*before* dt_cpufreq_probe().

subsys_initcall(topology_init);
device_initcall(cpufreq_dt_platdev_init);

I'm not sure how to proceed.

Regards.

cpufreq: frequency scaling spec in DT node

[Mason]

On 29/06/2017 13:41, Mason wrote:

> On 29/06/2017 12:04, Viresh Kumar wrote:
> 
>> There is one thing you can do to avoid adding OPP entries in the DT.
>> You can rather add them dynamically with help of: dev_pm_opp_add() and
>> cpufreq-dt will continue to work with that too.
> 
> In what driver should I call these... the clk driver?
> (drivers/clk/tegra/cvb.c seems to be doind that)

The problem I run into is that calling get_cpu_device(0) from the
clk driver returns NULL, because topology_init() has not run yet
(to initialize the cpu_sys_devices).

So the OPP table needs to be built *after* topology_init() but
*before* dt_cpufreq_probe().

subsys_initcall(topology_init);
device_initcall(cpufreq_dt_platdev_init);

I'm not sure how to proceed.

Regards.

Re: cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

On 29-06-17, 15:01, Mason wrote:
> On 29/06/2017 13:41, Mason wrote:
> 
> > On 29/06/2017 12:04, Viresh Kumar wrote:
> > 
> >> There is one thing you can do to avoid adding OPP entries in the DT.
> >> You can rather add them dynamically with help of: dev_pm_opp_add() and
> >> cpufreq-dt will continue to work with that too.
> > 
> > In what driver should I call these... the clk driver?
> > (drivers/clk/tegra/cvb.c seems to be doind that)
> 
> The problem I run into is that calling get_cpu_device(0) from the
> clk driver returns NULL, because topology_init() has not run yet
> (to initialize the cpu_sys_devices).
> 
> So the OPP table needs to be built *after* topology_init() but
> *before* dt_cpufreq_probe().
> 
> subsys_initcall(topology_init);
> device_initcall(cpufreq_dt_platdev_init);
> 
> I'm not sure how to proceed.

As we discussed over IRC, you can control the creation of cpufreq-dt
platform device and hence when the cpufreq-dt driver gets probed. Your
own cpufreq driver is the way to go here.

-- 
viresh

cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

On 29-06-17, 15:01, Mason wrote:
> On 29/06/2017 13:41, Mason wrote:
> 
> > On 29/06/2017 12:04, Viresh Kumar wrote:
> > 
> >> There is one thing you can do to avoid adding OPP entries in the DT.
> >> You can rather add them dynamically with help of: dev_pm_opp_add() and
> >> cpufreq-dt will continue to work with that too.
> > 
> > In what driver should I call these... the clk driver?
> > (drivers/clk/tegra/cvb.c seems to be doind that)
> 
> The problem I run into is that calling get_cpu_device(0) from the
> clk driver returns NULL, because topology_init() has not run yet
> (to initialize the cpu_sys_devices).
> 
> So the OPP table needs to be built *after* topology_init() but
> *before* dt_cpufreq_probe().
> 
> subsys_initcall(topology_init);
> device_initcall(cpufreq_dt_platdev_init);
> 
> I'm not sure how to proceed.

As we discussed over IRC, you can control the creation of cpufreq-dt
platform device and hence when the cpufreq-dt driver gets probed. Your
own cpufreq driver is the way to go here.

-- 
viresh

Re: cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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.
> 
> 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.

> My concern is that if I don't check somewhere that the
> nominal frequency is as expected in the DT, the CPU might
> run slower than expected (max freq cut in half).

Its for the bootloader to decide, not a kernel developer. So whatever
they choose is what we want to use. This can happen with everyone else
as well, we are all using DT.

But surely, increasing the max OPP value shouldn't have any bad
effects.

> In what driver should I call these... the clk driver?
> (drivers/clk/tegra/cvb.c seems to be doind that)
> 
> A problem might arise when I need to do voltage scaling,
> though, since I also need to specify voltages, right?

Your cpufreq driver, which uses cpufreq-dt.c.

> You're talking about this document, right?
> https://www.kernel.org/doc/Documentation/cpu-freq/user-guide.txt

Documentation/admin-guide/pm/cpufreq.rst in mainline kernel.

-- 
viresh

cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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.
> 
> 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.

> My concern is that if I don't check somewhere that the
> nominal frequency is as expected in the DT, the CPU might
> run slower than expected (max freq cut in half).

Its for the bootloader to decide, not a kernel developer. So whatever
they choose is what we want to use. This can happen with everyone else
as well, we are all using DT.

But surely, increasing the max OPP value shouldn't have any bad
effects.

> In what driver should I call these... the clk driver?
> (drivers/clk/tegra/cvb.c seems to be doind that)
> 
> A problem might arise when I need to do voltage scaling,
> though, since I also need to specify voltages, right?

Your cpufreq driver, which uses cpufreq-dt.c.

> You're talking about this document, right?
> https://www.kernel.org/doc/Documentation/cpu-freq/user-guide.txt

Documentation/admin-guide/pm/cpufreq.rst in mainline kernel.

-- 
viresh

Re: cpufreq: frequency scaling spec in DT node

[Mason]

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.
>>
>> 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.
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
[    9.213647] cpu cpu0: dev_pm_opp_set_rate: failed to find OPP for freq 1215000000 (-34)
[    9.222029] cpufreq: __target_index: Failed to change cpu frequency: -34



I'll experiment with the other solution of creating the OPP table
at init.

Regards.

cpufreq: frequency scaling spec in DT node

[Mason]

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.
>>
>> 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.
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
[    9.213647] cpu cpu0: dev_pm_opp_set_rate: failed to find OPP for freq 1215000000 (-34)
[    9.222029] cpufreq: __target_index: Failed to change cpu frequency: -34



I'll experiment with the other solution of creating the OPP table
at init.

Regards.

Re: cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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?

> >>
> >> 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.

> 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),
but clk framework rounded it up to 1215 (this should happen without the
CLK_DIVIDER_ROUND_CLOSEST flag as well).

> [    9.213647] cpu cpu0: dev_pm_opp_set_rate: failed to find OPP for freq 1215000000 (-34)

Now we failed because DT didn't had the max set to 1215, why ?

> I'll experiment with the other solution of creating the OPP table
> at init.

Sorry, I haven't got a convincing answer to why you see the problem you are
seeing.

-- 
viresh

cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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?

> >>
> >> 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.

> 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),
but clk framework rounded it up to 1215 (this should happen without the
CLK_DIVIDER_ROUND_CLOSEST flag as well).

> [    9.213647] cpu cpu0: dev_pm_opp_set_rate: failed to find OPP for freq 1215000000 (-34)

Now we failed because DT didn't had the max set to 1215, why ?

> I'll experiment with the other solution of creating the OPP table
> at init.

Sorry, I haven't got a convincing answer to why you see the problem you are
seeing.

-- 
viresh

Re: cpufreq: frequency scaling spec in DT node

[Mason]

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.

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",


> 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.

cpufreq: frequency scaling spec in DT node

[Mason]

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.

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",


> 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.

Re: cpufreq: frequency scaling spec in DT node

[Mason]

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.

cpufreq: frequency scaling spec in DT node

[Mason]

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.

Re: cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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 :)

-- 
viresh

cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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 :)

-- 
viresh

Re: cpufreq: frequency scaling spec in DT node

[Mason]

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.

cpufreq: frequency scaling spec in DT node

[Mason]

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.

Re: cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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 :)

-- 
viresh

cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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 :)

-- 
viresh

Re: cpufreq: frequency scaling spec in DT node

[Mason]

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.

cpufreq: frequency scaling spec in DT node

[Mason]

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.

Re: cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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.

-- 
viresh

cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

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.

-- 
viresh

Re: cpufreq: frequency scaling spec in DT node

[Mason]

On 11/07/2017 11:27, Mason wrote:

> I'll experiment with the other solution of creating the OPP table
> at init.

For the sake of discussion, based on your suggestion to use
dev_pm_opp_add(), I have tested the patch below.

There is one remaining error:
[    2.071139] of: dev_pm_opp_of_cpumask_add_table: couldn't find opp table for cpu:0, -19

Reached through:
[    1.878461] [<c0359a0c>] (dev_pm_opp_of_add_table) from [<c0359ff0>] (dev_pm_opp_of_cpumask_add_table+0x40/0xc8)
[    1.888695] [<c0359ff0>] (dev_pm_opp_of_cpumask_add_table) from [<c03d66d8>] (cpufreq_init+0x23c/0x2dc)
[    1.898139] [<c03d66d8>] (cpufreq_init) from [<c03d3d94>] (cpufreq_online+0xb8/0x660)
[    1.906009] [<c03d3d94>] (cpufreq_online) from [<c03d43f8>] (cpufreq_add_dev+0xbc/0xcc)
[    1.914058] [<c03d43f8>] (cpufreq_add_dev) from [<c0350560>] (subsys_interface_register+0x90/0xcc)
[    1.923065] [<c0350560>] (subsys_interface_register) from [<c03d32f4>] (cpufreq_register_driver+0x15c/0x1e0)
[    1.932945] [<c03d32f4>] (cpufreq_register_driver) from [<c03d67f0>] (dt_cpufreq_probe+0x78/0xec)
[    1.941866] [<c03d67f0>] (dt_cpufreq_probe) from [<c035327c>] (platform_drv_probe+0x34/0x6c)

The cpufreq-dt driver seems to recover despite the error:

/sys/devices/system/cpu/cpu0/cpufreq$ cat scaling_available_frequencies
135000 243000 405000 607500 1215000 

Regards.



 arch/arm/boot/dts/tango4-smp8758.dtsi |  1 -
 drivers/cpufreq/Makefile              |  1 +
 drivers/cpufreq/cpufreq-dt-platdev.c  |  2 --
 drivers/cpufreq/tango-cpufreq.c       | 45 +++++++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
index d2e65c46bcc7..eca33d568690 100644
--- a/arch/arm/boot/dts/tango4-smp8758.dtsi
+++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
@@ -13,7 +13,6 @@
 			reg = <0>;
 			clocks = <&clkgen CPU_CLK>;
 			clock-latency = <1>;
-			operating-points = <1215000 0 607500 0 405000 0 243000 0 135000 0>;
 		};
 
 		cpu1: cpu@1 {
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 0a9b6a093646..5ae156dc0d35 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ)	+= sa1110-cpufreq.o
 obj-$(CONFIG_ARM_SCPI_CPUFREQ)		+= scpi-cpufreq.o
 obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
 obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
+obj-$(CONFIG_ARCH_TANGO)		+= tango-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 71267626456b..1490e4ce3fbc 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -70,8 +70,6 @@
 	{ .compatible = "rockchip,rk3368", },
 	{ .compatible = "rockchip,rk3399", },
 
-	{ .compatible = "sigma,tango4" },
-
 	{ .compatible = "ti,am33xx", },
 	{ .compatible = "ti,dra7", },
 	{ .compatible = "ti,omap2", },
diff --git a/drivers/cpufreq/tango-cpufreq.c b/drivers/cpufreq/tango-cpufreq.c
new file mode 100644
index 000000000000..270c8261ebfa
--- /dev/null
+++ b/drivers/cpufreq/tango-cpufreq.c
@@ -0,0 +1,45 @@
+#include <linux/of.h>
+#include <linux/cpu.h>
+#include <linux/clk.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+
+static const struct of_device_id machines[] __initconst = {
+	{ .compatible = "sigma,tango4" },
+	{ .compatible = "sigma,tango5" },
+	{ /* sentinel */ }
+};
+
+static void __init build_opp_table(struct device *cpu_dev, struct clk *cpu_clk)
+{
+	unsigned long max_freq = clk_get_rate(cpu_clk);
+
+	dev_pm_opp_add(cpu_dev, max_freq / 1, 0);
+	dev_pm_opp_add(cpu_dev, max_freq / 2, 0);
+	dev_pm_opp_add(cpu_dev, max_freq / 3, 0);
+	dev_pm_opp_add(cpu_dev, max_freq / 5, 0);
+	dev_pm_opp_add(cpu_dev, max_freq / 9, 0);
+}
+
+static int __init cpufreq_dt_platdev_init(void)
+{
+	struct device *cpu_dev = get_cpu_device(0);
+	const struct of_device_id *match;
+	struct clk *cpu_clk;
+	void *res;
+
+	match = of_match_node(machines, of_root);
+	if (!match)
+		return -ENODEV;
+
+	cpu_clk = clk_get(cpu_dev, NULL);
+	if (IS_ERR(cpu_clk))
+		return -ENODEV;
+
+	build_opp_table(cpu_dev, cpu_clk);
+
+	res = platform_device_register_data(NULL, "cpufreq-dt", -1, NULL, 0);
+
+	return PTR_ERR_OR_ZERO(res);
+}
+device_initcall(cpufreq_dt_platdev_init);

cpufreq: frequency scaling spec in DT node

[Mason]

On 11/07/2017 11:27, Mason wrote:

> I'll experiment with the other solution of creating the OPP table
> at init.

For the sake of discussion, based on your suggestion to use
dev_pm_opp_add(), I have tested the patch below.

There is one remaining error:
[    2.071139] of: dev_pm_opp_of_cpumask_add_table: couldn't find opp table for cpu:0, -19

Reached through:
[    1.878461] [<c0359a0c>] (dev_pm_opp_of_add_table) from [<c0359ff0>] (dev_pm_opp_of_cpumask_add_table+0x40/0xc8)
[    1.888695] [<c0359ff0>] (dev_pm_opp_of_cpumask_add_table) from [<c03d66d8>] (cpufreq_init+0x23c/0x2dc)
[    1.898139] [<c03d66d8>] (cpufreq_init) from [<c03d3d94>] (cpufreq_online+0xb8/0x660)
[    1.906009] [<c03d3d94>] (cpufreq_online) from [<c03d43f8>] (cpufreq_add_dev+0xbc/0xcc)
[    1.914058] [<c03d43f8>] (cpufreq_add_dev) from [<c0350560>] (subsys_interface_register+0x90/0xcc)
[    1.923065] [<c0350560>] (subsys_interface_register) from [<c03d32f4>] (cpufreq_register_driver+0x15c/0x1e0)
[    1.932945] [<c03d32f4>] (cpufreq_register_driver) from [<c03d67f0>] (dt_cpufreq_probe+0x78/0xec)
[    1.941866] [<c03d67f0>] (dt_cpufreq_probe) from [<c035327c>] (platform_drv_probe+0x34/0x6c)

The cpufreq-dt driver seems to recover despite the error:

/sys/devices/system/cpu/cpu0/cpufreq$ cat scaling_available_frequencies
135000 243000 405000 607500 1215000 

Regards.



 arch/arm/boot/dts/tango4-smp8758.dtsi |  1 -
 drivers/cpufreq/Makefile              |  1 +
 drivers/cpufreq/cpufreq-dt-platdev.c  |  2 --
 drivers/cpufreq/tango-cpufreq.c       | 45 +++++++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
index d2e65c46bcc7..eca33d568690 100644
--- a/arch/arm/boot/dts/tango4-smp8758.dtsi
+++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
@@ -13,7 +13,6 @@
 			reg = <0>;
 			clocks = <&clkgen CPU_CLK>;
 			clock-latency = <1>;
-			operating-points = <1215000 0 607500 0 405000 0 243000 0 135000 0>;
 		};
 
 		cpu1: cpu at 1 {
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 0a9b6a093646..5ae156dc0d35 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ)	+= sa1110-cpufreq.o
 obj-$(CONFIG_ARM_SCPI_CPUFREQ)		+= scpi-cpufreq.o
 obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
 obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
+obj-$(CONFIG_ARCH_TANGO)		+= tango-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 71267626456b..1490e4ce3fbc 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -70,8 +70,6 @@
 	{ .compatible = "rockchip,rk3368", },
 	{ .compatible = "rockchip,rk3399", },
 
-	{ .compatible = "sigma,tango4" },
-
 	{ .compatible = "ti,am33xx", },
 	{ .compatible = "ti,dra7", },
 	{ .compatible = "ti,omap2", },
diff --git a/drivers/cpufreq/tango-cpufreq.c b/drivers/cpufreq/tango-cpufreq.c
new file mode 100644
index 000000000000..270c8261ebfa
--- /dev/null
+++ b/drivers/cpufreq/tango-cpufreq.c
@@ -0,0 +1,45 @@
+#include <linux/of.h>
+#include <linux/cpu.h>
+#include <linux/clk.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+
+static const struct of_device_id machines[] __initconst = {
+	{ .compatible = "sigma,tango4" },
+	{ .compatible = "sigma,tango5" },
+	{ /* sentinel */ }
+};
+
+static void __init build_opp_table(struct device *cpu_dev, struct clk *cpu_clk)
+{
+	unsigned long max_freq = clk_get_rate(cpu_clk);
+
+	dev_pm_opp_add(cpu_dev, max_freq / 1, 0);
+	dev_pm_opp_add(cpu_dev, max_freq / 2, 0);
+	dev_pm_opp_add(cpu_dev, max_freq / 3, 0);
+	dev_pm_opp_add(cpu_dev, max_freq / 5, 0);
+	dev_pm_opp_add(cpu_dev, max_freq / 9, 0);
+}
+
+static int __init cpufreq_dt_platdev_init(void)
+{
+	struct device *cpu_dev = get_cpu_device(0);
+	const struct of_device_id *match;
+	struct clk *cpu_clk;
+	void *res;
+
+	match = of_match_node(machines, of_root);
+	if (!match)
+		return -ENODEV;
+
+	cpu_clk = clk_get(cpu_dev, NULL);
+	if (IS_ERR(cpu_clk))
+		return -ENODEV;
+
+	build_opp_table(cpu_dev, cpu_clk);
+
+	res = platform_device_register_data(NULL, "cpufreq-dt", -1, NULL, 0);
+
+	return PTR_ERR_OR_ZERO(res);
+}
+device_initcall(cpufreq_dt_platdev_init);

Re: cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

On 11-07-17, 15:36, Mason wrote:
> On 11/07/2017 11:27, Mason wrote:
> 
> > I'll experiment with the other solution of creating the OPP table
> > at init.
> 
> For the sake of discussion, based on your suggestion to use
> dev_pm_opp_add(), I have tested the patch below.
> 
> There is one remaining error:
> [    2.071139] of: dev_pm_opp_of_cpumask_add_table: couldn't find opp table for cpu:0, -19
> 
> Reached through:
> [    1.878461] [<c0359a0c>] (dev_pm_opp_of_add_table) from [<c0359ff0>] (dev_pm_opp_of_cpumask_add_table+0x40/0xc8)
> [    1.888695] [<c0359ff0>] (dev_pm_opp_of_cpumask_add_table) from [<c03d66d8>] (cpufreq_init+0x23c/0x2dc)
> [    1.898139] [<c03d66d8>] (cpufreq_init) from [<c03d3d94>] (cpufreq_online+0xb8/0x660)
> [    1.906009] [<c03d3d94>] (cpufreq_online) from [<c03d43f8>] (cpufreq_add_dev+0xbc/0xcc)
> [    1.914058] [<c03d43f8>] (cpufreq_add_dev) from [<c0350560>] (subsys_interface_register+0x90/0xcc)
> [    1.923065] [<c0350560>] (subsys_interface_register) from [<c03d32f4>] (cpufreq_register_driver+0x15c/0x1e0)
> [    1.932945] [<c03d32f4>] (cpufreq_register_driver) from [<c03d67f0>] (dt_cpufreq_probe+0x78/0xec)
> [    1.941866] [<c03d67f0>] (dt_cpufreq_probe) from [<c035327c>] (platform_drv_probe+0x34/0x6c)

I have sent a patch to fix that.

> The cpufreq-dt driver seems to recover despite the error:
> 
> /sys/devices/system/cpu/cpu0/cpufreq$ cat scaling_available_frequencies
> 135000 243000 405000 607500 1215000 

Yeah, it worked as expected.

>  arch/arm/boot/dts/tango4-smp8758.dtsi |  1 -
>  drivers/cpufreq/Makefile              |  1 +
>  drivers/cpufreq/cpufreq-dt-platdev.c  |  2 --
>  drivers/cpufreq/tango-cpufreq.c       | 45 +++++++++++++++++++++++++++++++++++
>  4 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
> index d2e65c46bcc7..eca33d568690 100644
> --- a/arch/arm/boot/dts/tango4-smp8758.dtsi
> +++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
> @@ -13,7 +13,6 @@
>  			reg = <0>;
>  			clocks = <&clkgen CPU_CLK>;
>  			clock-latency = <1>;
> -			operating-points = <1215000 0 607500 0 405000 0 243000 0 135000 0>;
>  		};
>  
>  		cpu1: cpu@1 {
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 0a9b6a093646..5ae156dc0d35 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ)	+= sa1110-cpufreq.o
>  obj-$(CONFIG_ARM_SCPI_CPUFREQ)		+= scpi-cpufreq.o
>  obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
>  obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
> +obj-$(CONFIG_ARCH_TANGO)		+= tango-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index 71267626456b..1490e4ce3fbc 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -70,8 +70,6 @@
>  	{ .compatible = "rockchip,rk3368", },
>  	{ .compatible = "rockchip,rk3399", },
>  
> -	{ .compatible = "sigma,tango4" },
> -
>  	{ .compatible = "ti,am33xx", },
>  	{ .compatible = "ti,dra7", },
>  	{ .compatible = "ti,omap2", },
> diff --git a/drivers/cpufreq/tango-cpufreq.c b/drivers/cpufreq/tango-cpufreq.c
> new file mode 100644
> index 000000000000..270c8261ebfa
> --- /dev/null
> +++ b/drivers/cpufreq/tango-cpufreq.c
> @@ -0,0 +1,45 @@
> +#include <linux/of.h>
> +#include <linux/cpu.h>
> +#include <linux/clk.h>
> +#include <linux/pm_opp.h>
> +#include <linux/platform_device.h>
> +
> +static const struct of_device_id machines[] __initconst = {
> +	{ .compatible = "sigma,tango4" },
> +	{ .compatible = "sigma,tango5" },
> +	{ /* sentinel */ }
> +};
> +
> +static void __init build_opp_table(struct device *cpu_dev, struct clk *cpu_clk)
> +{
> +	unsigned long max_freq = clk_get_rate(cpu_clk);

I kind of get the idea on how is the bootloader fixing your max. So you take the
frequency at which the CPU booted to be Max, right ?

I think that is part of the problem here. IMHO, the kernel really isn't required
to do that. It can have a MAX value of its own. Not sure if we can get it via DT
somehow today, otherwise statically should be fine as well.

And if kernel can take care of max and not depend on the value set by
bootloader, you wouldn't be required to have this patch and use DT based OPP.

> +	dev_pm_opp_add(cpu_dev, max_freq / 1, 0);
> +	dev_pm_opp_add(cpu_dev, max_freq / 2, 0);
> +	dev_pm_opp_add(cpu_dev, max_freq / 3, 0);
> +	dev_pm_opp_add(cpu_dev, max_freq / 5, 0);
> +	dev_pm_opp_add(cpu_dev, max_freq / 9, 0);
> +}

-- 
viresh

cpufreq: frequency scaling spec in DT node

[Viresh Kumar]

On 11-07-17, 15:36, Mason wrote:
> On 11/07/2017 11:27, Mason wrote:
> 
> > I'll experiment with the other solution of creating the OPP table
> > at init.
> 
> For the sake of discussion, based on your suggestion to use
> dev_pm_opp_add(), I have tested the patch below.
> 
> There is one remaining error:
> [    2.071139] of: dev_pm_opp_of_cpumask_add_table: couldn't find opp table for cpu:0, -19
> 
> Reached through:
> [    1.878461] [<c0359a0c>] (dev_pm_opp_of_add_table) from [<c0359ff0>] (dev_pm_opp_of_cpumask_add_table+0x40/0xc8)
> [    1.888695] [<c0359ff0>] (dev_pm_opp_of_cpumask_add_table) from [<c03d66d8>] (cpufreq_init+0x23c/0x2dc)
> [    1.898139] [<c03d66d8>] (cpufreq_init) from [<c03d3d94>] (cpufreq_online+0xb8/0x660)
> [    1.906009] [<c03d3d94>] (cpufreq_online) from [<c03d43f8>] (cpufreq_add_dev+0xbc/0xcc)
> [    1.914058] [<c03d43f8>] (cpufreq_add_dev) from [<c0350560>] (subsys_interface_register+0x90/0xcc)
> [    1.923065] [<c0350560>] (subsys_interface_register) from [<c03d32f4>] (cpufreq_register_driver+0x15c/0x1e0)
> [    1.932945] [<c03d32f4>] (cpufreq_register_driver) from [<c03d67f0>] (dt_cpufreq_probe+0x78/0xec)
> [    1.941866] [<c03d67f0>] (dt_cpufreq_probe) from [<c035327c>] (platform_drv_probe+0x34/0x6c)

I have sent a patch to fix that.

> The cpufreq-dt driver seems to recover despite the error:
> 
> /sys/devices/system/cpu/cpu0/cpufreq$ cat scaling_available_frequencies
> 135000 243000 405000 607500 1215000 

Yeah, it worked as expected.

>  arch/arm/boot/dts/tango4-smp8758.dtsi |  1 -
>  drivers/cpufreq/Makefile              |  1 +
>  drivers/cpufreq/cpufreq-dt-platdev.c  |  2 --
>  drivers/cpufreq/tango-cpufreq.c       | 45 +++++++++++++++++++++++++++++++++++
>  4 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tango4-smp8758.dtsi b/arch/arm/boot/dts/tango4-smp8758.dtsi
> index d2e65c46bcc7..eca33d568690 100644
> --- a/arch/arm/boot/dts/tango4-smp8758.dtsi
> +++ b/arch/arm/boot/dts/tango4-smp8758.dtsi
> @@ -13,7 +13,6 @@
>  			reg = <0>;
>  			clocks = <&clkgen CPU_CLK>;
>  			clock-latency = <1>;
> -			operating-points = <1215000 0 607500 0 405000 0 243000 0 135000 0>;
>  		};
>  
>  		cpu1: cpu at 1 {
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 0a9b6a093646..5ae156dc0d35 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ)	+= sa1110-cpufreq.o
>  obj-$(CONFIG_ARM_SCPI_CPUFREQ)		+= scpi-cpufreq.o
>  obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
>  obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
> +obj-$(CONFIG_ARCH_TANGO)		+= tango-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index 71267626456b..1490e4ce3fbc 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -70,8 +70,6 @@
>  	{ .compatible = "rockchip,rk3368", },
>  	{ .compatible = "rockchip,rk3399", },
>  
> -	{ .compatible = "sigma,tango4" },
> -
>  	{ .compatible = "ti,am33xx", },
>  	{ .compatible = "ti,dra7", },
>  	{ .compatible = "ti,omap2", },
> diff --git a/drivers/cpufreq/tango-cpufreq.c b/drivers/cpufreq/tango-cpufreq.c
> new file mode 100644
> index 000000000000..270c8261ebfa
> --- /dev/null
> +++ b/drivers/cpufreq/tango-cpufreq.c
> @@ -0,0 +1,45 @@
> +#include <linux/of.h>
> +#include <linux/cpu.h>
> +#include <linux/clk.h>
> +#include <linux/pm_opp.h>
> +#include <linux/platform_device.h>
> +
> +static const struct of_device_id machines[] __initconst = {
> +	{ .compatible = "sigma,tango4" },
> +	{ .compatible = "sigma,tango5" },
> +	{ /* sentinel */ }
> +};
> +
> +static void __init build_opp_table(struct device *cpu_dev, struct clk *cpu_clk)
> +{
> +	unsigned long max_freq = clk_get_rate(cpu_clk);

I kind of get the idea on how is the bootloader fixing your max. So you take the
frequency at which the CPU booted to be Max, right ?

I think that is part of the problem here. IMHO, the kernel really isn't required
to do that. It can have a MAX value of its own. Not sure if we can get it via DT
somehow today, otherwise statically should be fine as well.

And if kernel can take care of max and not depend on the value set by
bootloader, you wouldn't be required to have this patch and use DT based OPP.

> +	dev_pm_opp_add(cpu_dev, max_freq / 1, 0);
> +	dev_pm_opp_add(cpu_dev, max_freq / 2, 0);
> +	dev_pm_opp_add(cpu_dev, max_freq / 3, 0);
> +	dev_pm_opp_add(cpu_dev, max_freq / 5, 0);
> +	dev_pm_opp_add(cpu_dev, max_freq / 9, 0);
> +}

-- 
viresh