Kirkwood CPU Freq driver

Kirkwood CPU Freq driver

[Adam Baker]

Hi,

I've been trying to test Andrew Lunn's work creating a CPU Idle driver 
for Kirkwood. The first problem I encountered was that somehow in 
merging the patch the ARCH_HAS_CPUFREQ line appears to have got lost in 
arch/arm/Kconfig so I put that back (this may not be an issue if you 
build a multiarch kernel but I was targetting just Kirkwood)

--- a/arch/arm/Kconfig	2013-05-24 19:45:59.000000000 +0100
+++ b/arch/arm/Kconfig	2013-05-17 19:58:21.000000000 +0100
@@ -567,6 +567,7 @@ config ARCH_DOVE

  config ARCH_KIRKWOOD
  	bool "Marvell Kirkwood"
+	select ARCH_HAS_CPUFREQ
  	select ARCH_REQUIRE_GPIOLIB
  	select CPU_FEROCEON
  	select GENERIC_CLOCKEVENTS

Then I saw that the driver needs a cpu definition and none of the 
kirkwood dts files provide one. I believe all kirkwood SoCs are 
uniprocessor so I created an entry in kirkwood.dtsi

--- a/arch/arm/boot/dts/kirkwood.dtsi	2013-05-19 22:57:07.000000000 +0100
+++ b/arch/arm/boot/dts/kirkwood.dtsi	2013-05-19 23:10:32.000000000 +0100
@@ -4,6 +4,18 @@
  	compatible = "marvell,kirkwood";
  	interrupt-parent = <&intc>;

+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu at 0 {
+		      device_type = "cpu";
+		      compatible = "marvell,feroceon";
+		      clocks = <&core_clk 1>, <&core_clk 3>, <&gate_clk 11>;
+		      clock-names = "cpu_clk", "ddrclk", "powersave";
+		};
+	};
+
  	aliases {
  	       gpio0 = &gpio0;
  	       gpio1 = &gpio1;

(Yes I know Thunderbird has mangled the wordwrap in the patch but as I'm 
only providing it as a description of what I did not to be applied I 
won't beat it into submission this time)

As nothing seems to depend upon it I went with the CPU compatible type 
as defined in Documentation/devicetree/bindings/arm/cpus.txt rather than 
the marvell,sheeva-88SV131 in 
Documentation/devicetree/bindings/arm/kirkwood.txt

I tried putting some debug code in kirkwood_cpufreq_probe() and it 
doesn't appear to be getting called.

Have I

a) misunderstood what kirkwood variants this driver is supposed to work with
b) manged my CPU definition so it doesn't work
or c) missed some other critical configuration step?

Many thanks

Adam

Kirkwood CPU Freq driver

[Andrew Lunn]

On Sat, May 25, 2013 at 10:32:04PM +0100, Adam Baker wrote:
> Hi,
> 
> I've been trying to test Andrew Lunn's work creating a CPU Idle
> driver for Kirkwood.

Hi Adam

Thanks for testing this. I must admit, i never tested it once the
first rc came out. I clearly should of, because its broken. It was a
three part patch series, and only part 1/3 made it in. Hence the
issues you are seeing.

Jason, please can you pick up:

https://patchwork.kernel.org/patch/2051401/
https://patchwork.kernel.org/patch/2080931/

	Andrew

Kirkwood CPU Freq driver

[Jason Cooper]

On Sun, May 26, 2013 at 10:36:14AM +0200, Andrew Lunn wrote:
> On Sat, May 25, 2013 at 10:32:04PM +0100, Adam Baker wrote:
> > I've been trying to test Andrew Lunn's work creating a CPU Idle
> > driver for Kirkwood.
> 
> Thanks for testing this. I must admit, i never tested it once the
> first rc came out. I clearly should of, because its broken. It was a
> three part patch series, and only part 1/3 made it in. Hence the
> issues you are seeing.
> 
> Jason, please can you pick up:
> 
> https://patchwork.kernel.org/patch/2051401/
> https://patchwork.kernel.org/patch/2080931/

Yep, got them in now.  Sorry for the mix up.

thx,

Jason.

Kirkwood CPU Freq driver

[Adam Baker]

On 26/05/13 19:05, Jason Cooper wrote:
> On Sun, May 26, 2013 at 10:36:14AM +0200, Andrew Lunn wrote:
>> On Sat, May 25, 2013 at 10:32:04PM +0100, Adam Baker wrote:
>>> I've been trying to test Andrew Lunn's work creating a CPU Idle
>>> driver for Kirkwood.
>>
>> Thanks for testing this. I must admit, i never tested it once the
>> first rc came out. I clearly should of, because its broken. It was a
>> three part patch series, and only part 1/3 made it in. Hence the
>> issues you are seeing.
>>
>> Jason, please can you pick up:
>>
>> https://patchwork.kernel.org/patch/2051401/
>> https://patchwork.kernel.org/patch/2080931/
>
> Yep, got them in now.  Sorry for the mix up.
>

I've now tested with the first of these patches applied and the changes 
from the second added to my .config

To get it to work I had to follow the instructions in the Documentation 
to add the cpus entry to kirkwood.dtsi so I'll post that as a patch in a 
day or two but after doing that I have a working cpufreq driver.

Measuring the power consumption of my NAS server I observed a slight 
reduction in power consumption when the clock rate was reduced. I 
haven't yet measured for long enough to get an accurate measurement but 
it looks like maybe 1/4 W power saving.

Feel free to add

Tested-by: Adam Baker <linux@baker-net.org.uk>

Regards

Adam

Kirkwood CPU Freq driver

[Andrew Lunn]

On Tue, May 28, 2013 at 10:36:13PM +0100, Adam Baker wrote:
> On 26/05/13 19:05, Jason Cooper wrote:
> >On Sun, May 26, 2013 at 10:36:14AM +0200, Andrew Lunn wrote:
> >>On Sat, May 25, 2013 at 10:32:04PM +0100, Adam Baker wrote:
> >>>I've been trying to test Andrew Lunn's work creating a CPU Idle
> >>>driver for Kirkwood.
> >>
> >>Thanks for testing this. I must admit, i never tested it once the
> >>first rc came out. I clearly should of, because its broken. It was a
> >>three part patch series, and only part 1/3 made it in. Hence the
> >>issues you are seeing.
> >>
> >>Jason, please can you pick up:
> >>
> >>https://patchwork.kernel.org/patch/2051401/
> >>https://patchwork.kernel.org/patch/2080931/
> >
> >Yep, got them in now.  Sorry for the mix up.
> >
> 
> I've now tested with the first of these patches applied and the
> changes from the second added to my .config
> 
> To get it to work I had to follow the instructions in the
> Documentation to add the cpus entry to kirkwood.dtsi so I'll post
> that as a patch in a day or two but after doing that I have a
> working cpufreq driver.

Great that you got it working. Something must of changed in the
generic code, since originally a cpu node was not required.

The clocks are not needed in node, so please don't list them.

> Measuring the power consumption of my NAS server I observed a slight
> reduction in power consumption when the clock rate was reduced. I
> haven't yet measured for long enough to get an accurate measurement
> but it looks like maybe 1/4 W power saving.

I don't have a killawatt meter etc, so i had no idea how much power it
actually saves.  Thanks for the figure.

	 Andrew

Kirkwood CPU Freq driver

[Jason Cooper]

On Tue, May 28, 2013 at 10:36:13PM +0100, Adam Baker wrote:
> On 26/05/13 19:05, Jason Cooper wrote:
> >On Sun, May 26, 2013 at 10:36:14AM +0200, Andrew Lunn wrote:
> >>On Sat, May 25, 2013 at 10:32:04PM +0100, Adam Baker wrote:
> >>>I've been trying to test Andrew Lunn's work creating a CPU Idle
> >>>driver for Kirkwood.
> >>
> >>Thanks for testing this. I must admit, i never tested it once the
> >>first rc came out. I clearly should of, because its broken. It was a
> >>three part patch series, and only part 1/3 made it in. Hence the
> >>issues you are seeing.
> >>
> >>Jason, please can you pick up:
> >>
> >>https://patchwork.kernel.org/patch/2051401/
> >>https://patchwork.kernel.org/patch/2080931/
> >
> >Yep, got them in now.  Sorry for the mix up.
> >
> 
> I've now tested with the first of these patches applied and the
> changes from the second added to my .config
> 
> To get it to work I had to follow the instructions in the
> Documentation to add the cpus entry to kirkwood.dtsi so I'll post
> that as a patch in a day or two but after doing that I have a
> working cpufreq driver.
> 
> Measuring the power consumption of my NAS server I observed a slight
> reduction in power consumption when the clock rate was reduced. I
> haven't yet measured for long enough to get an accurate measurement
> but it looks like maybe 1/4 W power saving.
> 
> Feel free to add
> 
> Tested-by: Adam Baker <linux@baker-net.org.uk>

I apologize, but I won't be able to add this.  I'm using a new work
flow, and took the patches in too quickly (well, they had been on the
list a while... :-P)  I should've let them sit in my tree for a few more
days before sending the pull request to arm-soc.  If I had done that, I
could've gone in and added the Tested-by:.  I'll be more careful in the
future.

When you submit v2 of your DT change, please add the Tested-by to that.

Thanks again for testing, we do appreciate the effort.

thx,

Jason.

Kirkwood CPU Freq driver

[Jason Cooper]

On Tue, May 28, 2013 at 08:58:25PM -0400, Jason Cooper wrote:
> On Tue, May 28, 2013 at 10:36:13PM +0100, Adam Baker wrote:
> > On 26/05/13 19:05, Jason Cooper wrote:
> > >On Sun, May 26, 2013 at 10:36:14AM +0200, Andrew Lunn wrote:
> > >>On Sat, May 25, 2013 at 10:32:04PM +0100, Adam Baker wrote:
> > >>>I've been trying to test Andrew Lunn's work creating a CPU Idle
> > >>>driver for Kirkwood.
> > >>
> > >>Thanks for testing this. I must admit, i never tested it once the
> > >>first rc came out. I clearly should of, because its broken. It was a
> > >>three part patch series, and only part 1/3 made it in. Hence the
> > >>issues you are seeing.
> > >>
> > >>Jason, please can you pick up:
> > >>
> > >>https://patchwork.kernel.org/patch/2051401/
> > >>https://patchwork.kernel.org/patch/2080931/
> > >
> > >Yep, got them in now.  Sorry for the mix up.
> > >
> > 
> > I've now tested with the first of these patches applied and the
> > changes from the second added to my .config
> > 
> > To get it to work I had to follow the instructions in the
> > Documentation to add the cpus entry to kirkwood.dtsi so I'll post
> > that as a patch in a day or two but after doing that I have a
> > working cpufreq driver.
> > 
> > Measuring the power consumption of my NAS server I observed a slight
> > reduction in power consumption when the clock rate was reduced. I
> > haven't yet measured for long enough to get an accurate measurement
> > but it looks like maybe 1/4 W power saving.
> > 
> > Feel free to add
> > 
> > Tested-by: Adam Baker <linux@baker-net.org.uk>
> 
> I apologize, but I won't be able to add this.  I'm using a new work
> flow, and took the patches in too quickly (well, they had been on the
> list a while... :-P)  I should've let them sit in my tree for a few more
> days before sending the pull request to arm-soc.  If I had done that, I
> could've gone in and added the Tested-by:.  I'll be more careful in the
> future.

I take this back.  Olof didn't pull last night like I thought he would,
and he said I have a few hours until he pulls tonight.  So I've added it
now.  Sorry for the confusion.

thx,

Jason.

Kirkwood CPU Freq driver

[Adam Baker]

On 28/05/13 22:51, Andrew Lunn wrote:
>> I've now tested with the first of these patches applied and the
>> >changes from the second added to my .config
>> >
>> >To get it to work I had to follow the instructions in the
>> >Documentation to add the cpus entry to kirkwood.dtsi so I'll post
>> >that as a patch in a day or two but after doing that I have a
>> >working cpufreq driver.
> Great that you got it working. Something must of changed in the
> generic code, since originally a cpu node was not required.
>
> The clocks are not needed in node, so please don't list them.
>
Andrew, I just tried testing without the clocks line in the .dtsi and I 
get the messages below in dmesg

[   18.554990] ERROR: could not get clock /cpus/cpu at 0:cpu_clk(0)
[   18.560840] kirkwood-cpufreq kirkwood-cpufreq: Unable to get cpuclk
[   18.567154] kirkwood-cpufreq: probe of kirkwood-cpufreq failed with 
error -2

looking at the code in kirkwood_cpufreq_probe() this is what I would 
expect. Unless I've missed something the driver is simply advertising 
that it needs those clocks and as they presumably are not gateable it 
may be safe to not bother reserving them but I don't know the code well 
enough to make that decision.

Having now looked at the range of kirkwood SoCs I see oretthat there is 
a 88F632X family that include 2 processor cores. The cpus definitions 
therefore theoretically ought to live in kirkwood-6281.dtsi and 
kirkwood-6282.dtsi. The only 2 boards I can see that don't include one 
of those two at present are kirkwood-km for which the SoC data isn't 
published and kirkwood-nsa310 which seems to simply be missing pinmux 
information for things like the sata ports.

Would everyone prefer to see the cpus node in the 628x.dtsi files or 
should it go in kirkwood.dtsi until support for the 88F632X family is added?

Regards

Adam

Kirkwood CPU Freq driver

[Andrew Lunn]

On Fri, May 31, 2013 at 12:19:01AM +0100, Adam Baker wrote:
> On 28/05/13 22:51, Andrew Lunn wrote:
> >>I've now tested with the first of these patches applied and the
> >>>changes from the second added to my .config
> >>>
> >>>To get it to work I had to follow the instructions in the
> >>>Documentation to add the cpus entry to kirkwood.dtsi so I'll post
> >>>that as a patch in a day or two but after doing that I have a
> >>>working cpufreq driver.
> >Great that you got it working. Something must of changed in the
> >generic code, since originally a cpu node was not required.
> >
> >The clocks are not needed in node, so please don't list them.
> >
> Andrew, I just tried testing without the clocks line in the .dtsi
> and I get the messages below in dmesg
> 
> [   18.554990] ERROR: could not get clock /cpus/cpu at 0:cpu_clk(0)
> [   18.560840] kirkwood-cpufreq kirkwood-cpufreq: Unable to get cpuclk
> [   18.567154] kirkwood-cpufreq: probe of kirkwood-cpufreq failed
> with error -2

Duh! I have forgotten how my driver works! Yes, it needs the node,
with clocks. I even documented it!

Strange thing is, i cannot find a patch adding such a node.

I suggest you add the node to the kirkwood.dtsi. I've never seen any
products using the 88F632X, we have no support for it, and very likely
its much more work needed than just moving a few DT nodes around.

Please submit your patch to Jason.

       Thanks
	Andrew