| Message ID | 51A12DD4.9010907@baker-net.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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.
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.
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@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
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@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
--- 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@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;