[PATCH v3 0/3] Kirkwoode cpufreq driver

[PATCH v3 0/3] Kirkwoode cpufreq driver

[Andrew Lunn]

This patchset adds a cpufreq driver for Marvell Kirkwood SoCs.

The changes to kirkwood_defconfig enable it and set the default
governor to ondemand.

Changes since v2:

Remove unneeded #include file left over from debugging.
WARN_ON(1) instead of dev_err() for unexpected state
Various blank lines removed.
Reformat comment.
writel -> writel_relaxed

Changes since v1:

tabify Kconfig.arm entry
Sort order of include files
Remove some unnecassary include files
Reformat multiline comment to be coding style conform.

Andrew Lunn (3):
  cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs
  arm: kirkwood: Instantiate cpufreq driver
  arm: kirkwood: Enable cpufreq and ondemand on kirkwood_defconfig

 arch/arm/Kconfig                                  |    1 +
 arch/arm/configs/kirkwood_defconfig               |    3 +
 arch/arm/mach-kirkwood/board-dt.c                 |    3 +-
 arch/arm/mach-kirkwood/common.c                   |   23 ++
 arch/arm/mach-kirkwood/common.h                   |    2 +
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h |    2 +
 drivers/clk/mvebu/clk-gating-ctrl.c               |    1 +
 drivers/cpufreq/Kconfig.arm                       |    6 +
 drivers/cpufreq/Makefile                          |    1 +
 drivers/cpufreq/kirkwood-cpufreq.c                |  264 +++++++++++++++++++++
 10 files changed, 305 insertions(+), 1 deletion(-)
 create mode 100644 drivers/cpufreq/kirkwood-cpufreq.c

-- 
1.7.10.4

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Andrew Lunn]

The Marvell Kirkwood SoCs have simple cpufreq support in hardware. The
CPU can either use the a high speed cpu clock, or the slower DDR
clock. Add a driver to swap between these two clock sources.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/clk/mvebu/clk-gating-ctrl.c |    1 +
 drivers/cpufreq/Kconfig.arm         |    6 +
 drivers/cpufreq/Makefile            |    1 +
 drivers/cpufreq/kirkwood-cpufreq.c  |  264 +++++++++++++++++++++++++++++++++++
 4 files changed, 272 insertions(+)
 create mode 100644 drivers/cpufreq/kirkwood-cpufreq.c

diff --git a/drivers/clk/mvebu/clk-gating-ctrl.c b/drivers/clk/mvebu/clk-gating-ctrl.c
index 8fa5408..ebf141d 100644
--- a/drivers/clk/mvebu/clk-gating-ctrl.c
+++ b/drivers/clk/mvebu/clk-gating-ctrl.c
@@ -193,6 +193,7 @@ static const struct mvebu_soc_descr __initconst kirkwood_gating_descr[] = {
 	{ "runit", NULL, 7 },
 	{ "xor0", NULL, 8 },
 	{ "audio", NULL, 9 },
+	{ "powersave", "cpuclk", 11 },
 	{ "sata0", NULL, 14 },
 	{ "sata1", NULL, 15 },
 	{ "xor1", NULL, 16 },
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index a0b3661..80c8229 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -77,6 +77,12 @@ config ARM_EXYNOS5250_CPUFREQ
 	  This adds the CPUFreq driver for Samsung EXYNOS5250
 	  SoC.
 
+config ARM_KIRKWOOD_CPUFREQ
+	def_bool ARCH_KIRKWOOD && OF
+	help
+	  This adds the CPUFreq driver for Marvell Kirkwood
+	  SoCs.
+
 config ARM_SPEAR_CPUFREQ
 	bool "SPEAr CPUFreq support"
 	depends on PLAT_SPEAR
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index fadc4d4..39a0ffe 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_ARM_EXYNOS_CPUFREQ)	+= exynos-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ)	+= exynos4210-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ)	+= exynos4x12-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ)	+= exynos5250-cpufreq.o
+obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ)	+= kirkwood-cpufreq.o
 obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ)     += omap-cpufreq.o
 obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
 
diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c
new file mode 100644
index 0000000..d08e679
--- /dev/null
+++ b/drivers/cpufreq/kirkwood-cpufreq.c
@@ -0,0 +1,264 @@
+/*
+ *	kirkwood_freq.c: cpufreq driver for the Marvell kirkwood
+ *
+ *	Copyright (C) 2013 Andrew Lunn <andrew@lunn.ch>
+ *
+ *	This program is free software; you can redistribute it and/or
+ *	modify it under the terms of the GNU General Public License
+ *	as published by the Free Software Foundation; either version
+ *	2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/cpufreq.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <asm/proc-fns.h>
+
+#define CPU_SW_INT_BLK BIT(28)
+
+static struct priv
+{
+	struct clk *cpu_clk;
+	struct clk *ddr_clk;
+	struct clk *powersave_clk;
+	struct device *dev;
+	void __iomem *base;
+} priv;
+
+#define STATE_CPU_FREQ 0x01
+#define STATE_DDR_FREQ 0x02
+
+/*
+ * Kirkwood can swap the clock to the CPU between two clocks:
+ *
+ * - cpu clk
+ * - ddr clk
+ *
+ * The frequencies are set at runtime before registering this *
+ * table.
+ */
+static struct cpufreq_frequency_table kirkwood_freq_table[] = {
+	{STATE_CPU_FREQ,	0}, /* CPU uses cpuclk */
+	{STATE_DDR_FREQ,	0}, /* CPU uses ddrclk */
+	{0,			CPUFREQ_TABLE_END},
+};
+
+static unsigned int kirkwood_cpufreq_get_cpu_frequency(unsigned int cpu)
+{
+	if (__clk_is_enabled(priv.powersave_clk))
+		return kirkwood_freq_table[1].frequency;
+	return kirkwood_freq_table[0].frequency;
+}
+
+static void kirkwood_cpufreq_set_cpu_state(unsigned int index)
+{
+	struct cpufreq_freqs freqs;
+	unsigned int state = kirkwood_freq_table[index].index;
+	unsigned long reg;
+
+	freqs.old = kirkwood_cpufreq_get_cpu_frequency(0);
+	freqs.new = kirkwood_freq_table[index].frequency;
+	freqs.cpu = 0; /* Kirkwood is UP */
+
+	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+
+	dev_dbg(priv.dev, "Attempting to set frequency to %i KHz\n",
+		kirkwood_freq_table[index].frequency);
+	dev_dbg(priv.dev, "old frequency was %i KHz\n",
+		kirkwood_cpufreq_get_cpu_frequency(0));
+
+	if (freqs.old != freqs.new) {
+		local_irq_disable();
+
+		/* Disable interrupts to the CPU */
+		reg = readl_relaxed(priv.base);
+		reg |= CPU_SW_INT_BLK;
+		writel_relaxed(reg, priv.base);
+
+		switch (state) {
+		case STATE_CPU_FREQ:
+			clk_disable(priv.powersave_clk);
+			break;
+		case STATE_DDR_FREQ:
+			clk_enable(priv.powersave_clk);
+			break;
+		default:
+			WARN_ON(1);
+		}
+
+		/* Wait-for-Interrupt, while the hardware changes frequency */
+		cpu_do_idle();
+
+		/* Enable interrupts to the CPU */
+		reg = readl_relaxed(priv.base);
+		reg &= ~CPU_SW_INT_BLK;
+		writel_relaxed(reg, priv.base);
+
+		local_irq_enable();
+	}
+	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
+};
+
+static int kirkwood_cpufreq_verify(struct cpufreq_policy *policy)
+{
+	return cpufreq_frequency_table_verify(policy, kirkwood_freq_table);
+}
+
+static int kirkwood_cpufreq_target(struct cpufreq_policy *policy,
+			    unsigned int target_freq,
+			    unsigned int relation)
+{
+	unsigned int index = 0;
+
+	if (cpufreq_frequency_table_target(policy, kirkwood_freq_table,
+				target_freq, relation, &index))
+		return -EINVAL;
+
+	kirkwood_cpufreq_set_cpu_state(index);
+
+	return 0;
+}
+
+/* Module init and exit code */
+static int kirkwood_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+	int result;
+
+	/* cpuinfo and default policy values */
+	policy->cpuinfo.transition_latency = 5000; /* 5uS */
+	policy->cur = kirkwood_cpufreq_get_cpu_frequency(0);
+
+	result = cpufreq_frequency_table_cpuinfo(policy, kirkwood_freq_table);
+	if (result)
+		return result;
+
+	cpufreq_frequency_table_get_attr(kirkwood_freq_table, policy->cpu);
+
+	return 0;
+}
+
+static int kirkwood_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+	cpufreq_frequency_table_put_attr(policy->cpu);
+	return 0;
+}
+
+static struct freq_attr *kirkwood_cpufreq_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	NULL,
+};
+
+static struct cpufreq_driver kirkwood_cpufreq_driver = {
+	.get	= kirkwood_cpufreq_get_cpu_frequency,
+	.verify	= kirkwood_cpufreq_verify,
+	.target	= kirkwood_cpufreq_target,
+	.init	= kirkwood_cpufreq_cpu_init,
+	.exit	= kirkwood_cpufreq_cpu_exit,
+	.name	= "kirkwood_freq",
+	.owner	= THIS_MODULE,
+	.attr	= kirkwood_cpufreq_attr,
+};
+
+static int kirkwood_cpufreq_probe(struct platform_device *pdev)
+{
+	struct device_node *np = of_find_compatible_node(
+		NULL, NULL, "marvell,kirkwood-core-clock");
+
+	struct of_phandle_args clkspec;
+	struct resource *res;
+	int err;
+
+	priv.dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Cannot get memory resource\n");
+		return -ENODEV;
+	}
+	priv.base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!priv.base) {
+		dev_err(&pdev->dev, "Cannot ioremap\n");
+		return -ENOMEM;
+	}
+
+	clkspec.np = np;
+	clkspec.args_count = 1;
+	clkspec.args[0] = 1;
+
+	priv.cpu_clk = of_clk_get_from_provider(&clkspec);
+	if (IS_ERR(priv.cpu_clk)) {
+		dev_err(priv.dev, "Unable to get cpuclk");
+		return PTR_ERR(priv.cpu_clk);
+	}
+
+	clk_prepare_enable(priv.cpu_clk);
+	kirkwood_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
+
+	clkspec.args[0] = 3;
+	priv.ddr_clk = of_clk_get_from_provider(&clkspec);
+	if (IS_ERR(priv.ddr_clk)) {
+		dev_err(priv.dev, "Unable to get ddrclk");
+		err = PTR_ERR(priv.ddr_clk);
+		goto out_cpu;
+	}
+
+	clk_prepare_enable(priv.ddr_clk);
+	kirkwood_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
+
+	np = of_find_compatible_node(NULL, NULL,
+				     "marvell,kirkwood-gating-clock");
+	clkspec.np = np;
+	clkspec.args[0] = 11;
+	priv.powersave_clk = of_clk_get_from_provider(&clkspec);
+	if (IS_ERR(priv.powersave_clk)) {
+		dev_err(priv.dev, "Unable to get powersave");
+		err = PTR_ERR(priv.powersave_clk);
+		goto out_ddr;
+	}
+	clk_prepare(priv.powersave_clk);
+
+	err = cpufreq_register_driver(&kirkwood_cpufreq_driver);
+	if (!err)
+		return 0;
+	dev_err(priv.dev, "Failed to register cpufreq driver");
+
+	clk_disable_unprepare(priv.powersave_clk);
+out_ddr:
+	clk_disable_unprepare(priv.ddr_clk);
+out_cpu:
+	clk_disable_unprepare(priv.cpu_clk);
+
+	return err;
+}
+
+static int kirkwood_cpufreq_remove(struct platform_device *pdev)
+{
+	cpufreq_unregister_driver(&kirkwood_cpufreq_driver);
+
+	clk_disable_unprepare(priv.powersave_clk);
+	clk_disable_unprepare(priv.ddr_clk);
+	clk_disable_unprepare(priv.cpu_clk);
+
+	return 0;
+}
+
+static struct platform_driver kirkwood_cpufreq_platform_driver = {
+	.probe = kirkwood_cpufreq_probe,
+	.remove = kirkwood_cpufreq_remove,
+	.driver = {
+		.name = "kirkwood-cpufreq",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_platform_driver(kirkwood_cpufreq_platform_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch");
+MODULE_DESCRIPTION("cpufreq driver for Marvell's kirkwood CPU");
+MODULE_ALIAS("platform:kirkwood-cpufreq");
-- 
1.7.10.4

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Viresh Kumar]

On 27 January 2013 15:37, Andrew Lunn <andrew@lunn.ch> wrote:
> diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c
> +static void kirkwood_cpufreq_set_cpu_state(unsigned int index)
> +{

> +       if (freqs.old != freqs.new) {

> +               switch (state) {
> +               case STATE_CPU_FREQ:
> +                       clk_disable(priv.powersave_clk);
> +                       break;
> +               case STATE_DDR_FREQ:
> +                       clk_enable(priv.powersave_clk);
> +                       break;
> +               default:
> +                       WARN_ON(1);

I still don't feel this case is required :)

> +               }

> +static struct cpufreq_driver kirkwood_cpufreq_driver = {
> +       .get    = kirkwood_cpufreq_get_cpu_frequency,
> +       .verify = kirkwood_cpufreq_verify,
> +       .target = kirkwood_cpufreq_target,
> +       .init   = kirkwood_cpufreq_cpu_init,
> +       .exit   = kirkwood_cpufreq_cpu_exit,
> +       .name   = "kirkwood_freq",
> +       .owner  = THIS_MODULE,
> +       .attr   = kirkwood_cpufreq_attr,
> +};
> +
> +static int kirkwood_cpufreq_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = of_find_compatible_node(
> +               NULL, NULL, "marvell,kirkwood-core-clock");

np can be NULL here, want to check?

> +       np = of_find_compatible_node(NULL, NULL,
> +                                    "marvell,kirkwood-gating-clock");

here too.

> +}

> +static struct platform_driver kirkwood_cpufreq_platform_driver = {
> +       .probe = kirkwood_cpufreq_probe,
> +       .remove = kirkwood_cpufreq_remove,
> +       .driver = {
> +               .name = "kirkwood-cpufreq",
> +               .owner = THIS_MODULE,
> +       },
> +};

Two things. Any reason why you created an extra layer of platform_driver?
You could have called cpufreq_probe/remove (with names modified) from
module init/exit.

Also, in case you want to keep it (which i don't feel is required),
then would make
sense to keep same names in .name field for both structures.

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Andrew Lunn]

On Sun, Jan 27, 2013 at 08:19:56PM +0530, Viresh Kumar wrote:
> On 27 January 2013 15:37, Andrew Lunn <andrew@lunn.ch> wrote:
> > diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c
> > +static void kirkwood_cpufreq_set_cpu_state(unsigned int index)
> > +{
> 
> > +       if (freqs.old != freqs.new) {
> 
> > +               switch (state) {
> > +               case STATE_CPU_FREQ:
> > +                       clk_disable(priv.powersave_clk);
> > +                       break;
> > +               case STATE_DDR_FREQ:
> > +                       clk_enable(priv.powersave_clk);
> > +                       break;
> > +               default:
> > +                       WARN_ON(1);
> 
> I still don't feel this case is required :)

O.K, i will take it out.

> > +static struct platform_driver kirkwood_cpufreq_platform_driver = {
> > +       .probe = kirkwood_cpufreq_probe,
> > +       .remove = kirkwood_cpufreq_remove,
> > +       .driver = {
> > +               .name = "kirkwood-cpufreq",
> > +               .owner = THIS_MODULE,
> > +       },
> > +};
> 
> Two things. Any reason why you created an extra layer of platform_driver?
> You could have called cpufreq_probe/remove (with names modified) from
> module init/exit.

And how would the module get loaded? There is no hardware anchor to
make the module load. No enumeration of some bus causing it to be
loaded. In the ARM world, platform drivers are the norm.

	Andrew

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Viresh Kumar]

On 27 January 2013 21:12, Andrew Lunn <andrew@lunn.ch> wrote:
> And how would the module get loaded? There is no hardware anchor to
> make the module load. No enumeration of some bus causing it to be
> loaded. In the ARM world, platform drivers are the norm.

The way you do it now is by creating a platform device for it in your
arch/arm/mach-* directory. And you are passing an IOMEM resource too.

I believe, normally we don't require any DT node or platform device from
arch/arm/mach-* for cpufreq drivers. This is something which should
always be initialized once it is selected in .config.

Because we will call the _probe() (or renamed as kirkwood_cpufreq_init)
from module_init() now, it will get initialized directly. No need to any
enumeration at all. Now, in case there are multiple values of IOMEM resource
depending on machine type, you can create properties in cpu node. Otherwise
just embed the address directly into driver as nobody else is going to use it.

Makes sense?

--
viresh

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Andrew Lunn]

On Sun, Jan 27, 2013 at 09:33:04PM +0530, Viresh Kumar wrote:
> On 27 January 2013 21:12, Andrew Lunn <andrew@lunn.ch> wrote:
> > And how would the module get loaded? There is no hardware anchor to
> > make the module load. No enumeration of some bus causing it to be
> > loaded. In the ARM world, platform drivers are the norm.
> 
> The way you do it now is by creating a platform device for it in your
> arch/arm/mach-* directory. And you are passing an IOMEM resource too.
> 
> I believe, normally we don't require any DT node or platform device from
> arch/arm/mach-* for cpufreq drivers. This is something which should
> always be initialized once it is selected in .config.

What current happens is that most of the drivers use a late_initcall():

linux/drivers/cpufreq$ grep late_initcall *
acpi-cpufreq.c:late_initcall(acpi_cpufreq_init);
cpufreq-cpu0.c:late_initcall(cpu0_cpufreq_driver_init);
exynos-cpufreq.c:late_initcall(exynos_cpufreq_init);
longhaul.c:late_initcall(longhaul_init);
p4-clockmod.c:late_initcall(cpufreq_p4_init);
pcc-cpufreq.c:late_initcall(pcc_cpufreq_init);
powernow-k7.c:late_initcall(powernow_init);
powernow-k8.c:late_initcall(powernowk8_init);
s5pv210-cpufreq.c:late_initcall(s5pv210_cpufreq_init);
spear-cpufreq.c:late_initcall(spear_cpufreq_driver_init);
speedstep-centrino.c:late_initcall(centrino_init);

So when we have a multiplatform kernel with many of these drivers
built in, all but one are going to notice they are not on the hardware
they support, and return -ENODEV.

By making it a platform driver, the kirkwood cpufreq driver will only
get loaded on kirkwood systems, and won't slow down the boot for
everybody else.

	  Andrew

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Viresh Kumar]

On 27 January 2013 21:55, Andrew Lunn <andrew@lunn.ch> wrote:
> So when we have a multiplatform kernel with many of these drivers
> built in, all but one are going to notice they are not on the hardware
> they support, and return -ENODEV.
>
> By making it a platform driver, the kirkwood cpufreq driver will only
> get loaded on kirkwood systems, and won't slow down the boot for
> everybody else.

I tried to grep platform_driver in drivers/cpufreq/ and got only:
drivers/cpufreq/dbx500-cpufreq.c

Now, the counter argument to your explanation is, multiplatform kernel would be
built only for testing purpose and not for real product. So, even this kind
of delay is really not a big issue. On the other hand with platform driver
too, we will hit lot of other code and would consume some extra memory
for keeping its structures :)

--
viresh

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Andrew Lunn]

On Sun, Jan 27, 2013 at 10:15:18PM +0530, Viresh Kumar wrote:
> On 27 January 2013 21:55, Andrew Lunn <andrew@lunn.ch> wrote:
> > So when we have a multiplatform kernel with many of these drivers
> > built in, all but one are going to notice they are not on the hardware
> > they support, and return -ENODEV.
> >
> > By making it a platform driver, the kirkwood cpufreq driver will only
> > get loaded on kirkwood systems, and won't slow down the boot for
> > everybody else.
> 
> I tried to grep platform_driver in drivers/cpufreq/ and got only:
> drivers/cpufreq/dbx500-cpufreq.c
> 
> Now, the counter argument to your explanation is, multiplatform kernel would be
> built only for testing purpose and not for real product.

I expect Debian, Fedora etc will strongly disagree with you
there. They want one kernel that will run on as many products they
support as possible. Kirkwood is mostly used in NAS boxes and is
supported by all these distributions.

Now for a SoC used in a deeply embedded system, using a custom
distribution, buildroot, etc, multiplatform is probably not
wanted. But the products i've seen using Kirkwood don't fit this use
case.

> So, even this kind of delay is really not a big issue. On the other
> hand with platform driver too, we will hit lot of other code and
> would consume some extra memory for keeping its structures

Kirkwood has many platform drivers, all this code is already pulled
in, lots of structures already exist. The incremental costs of another
platform device is minimal.

   Andrew

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Viresh Kumar]

On 27 January 2013 22:47, Andrew Lunn <andrew@lunn.ch> wrote:
> I expect Debian, Fedora etc will strongly disagree with you
> there. They want one kernel that will run on as many products they
> support as possible. Kirkwood is mostly used in NAS boxes and is
> supported by all these distributions.
>
> Now for a SoC used in a deeply embedded system, using a custom
> distribution, buildroot, etc, multiplatform is probably not
> wanted. But the products i've seen using Kirkwood don't fit this use
> case.

Accepted. I was wrong with my comment here :)

>> So, even this kind of delay is really not a big issue. On the other
>> hand with platform driver too, we will hit lot of other code and
>> would consume some extra memory for keeping its structures
>
> Kirkwood has many platform drivers, all this code is already pulled
> in, lots of structures already exist. The incremental costs of another
> platform device is minimal.

Its about delay executing other code too for platform device.

Other than that, there is one more issue with this style. It against DT
philosophy. :(

We really shouldn't add any devices from arch/arm/mach-* for any new
drivers. And when we have DT support in driver, then it doesn't make
any sense. So, you really need to pass this from cpu node in DT.

--
viresh

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Russell King - ARM Linux]

On Sun, Jan 27, 2013 at 11:07:22AM +0100, Andrew Lunn wrote:
> +static struct priv
> +{
> +	struct clk *cpu_clk;
> +	struct clk *ddr_clk;
> +	struct clk *powersave_clk;
> +	struct device *dev;
> +	void __iomem *base;
> +} priv;

I guess you probably think that the compiler will do something special
with this, like reference these all through a base address and offset,
but you'd be wrong there.  This is no different from declaring each
as an individual static variable.

> +static unsigned int kirkwood_cpufreq_get_cpu_frequency(unsigned int cpu)
> +{
> +	if (__clk_is_enabled(priv.powersave_clk))

This looks to me to be a layering violation.

> +static int kirkwood_cpufreq_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = of_find_compatible_node(
> +		NULL, NULL, "marvell,kirkwood-core-clock");

What if np is NULL?

> +
> +	struct of_phandle_args clkspec;
> +	struct resource *res;
> +	int err;
> +
> +	priv.dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Cannot get memory resource\n");
> +		return -ENODEV;
> +	}
> +	priv.base = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!priv.base) {
> +		dev_err(&pdev->dev, "Cannot ioremap\n");
> +		return -ENOMEM;

A number of people have been pointing out that the right return value for
this is -EADDRNOTAVAIL as documented against devm_request_and_ioremap().

> +	}
> +
> +	clkspec.np = np;
> +	clkspec.args_count = 1;
> +	clkspec.args[0] = 1;
> +
> +	priv.cpu_clk = of_clk_get_from_provider(&clkspec);

Oh, yet another way to get clocks...

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Andrew Lunn]

> > +	clkspec.np = np;
> > +	clkspec.args_count = 1;
> > +	clkspec.args[0] = 1;
> > +
> > +	priv.cpu_clk = of_clk_get_from_provider(&clkspec);
> 
> Oh, yet another way to get clocks...

Yep. I didn't like it, but could not find a better way.  It has been
argued that cpufreq drivers should not have nodes in DT. So the normal
of_clk_get() does not work. Since the clocks themselves are
instantiated from DT, there are no clkdev alias, so plain clk_get()
also does not work.

Do you know of a better way to do this?

   Thanks
	Andrew

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Shawn Guo]

On Sun, Jan 27, 2013 at 06:23:58PM +0100, Andrew Lunn wrote:
> > > +	clkspec.np = np;
> > > +	clkspec.args_count = 1;
> > > +	clkspec.args[0] = 1;
> > > +
> > > +	priv.cpu_clk = of_clk_get_from_provider(&clkspec);
> > 
> > Oh, yet another way to get clocks...
> 
> Yep. I didn't like it, but could not find a better way.  It has been
> argued that cpufreq drivers should not have nodes in DT. So the normal
> of_clk_get() does not work. Since the clocks themselves are
> instantiated from DT, there are no clkdev alias, so plain clk_get()
> also does not work.
> 
But you should have a cpu node in DT, and it will be perfectly fine to
have cpu node consume that core_clk.

Shawn

[PATCH v3 1/3] cpufreq: kirkwood: Add a cpufreq driver for Marvell Kirkwood SoCs

[Andrew Lunn]

On Sun, Jan 27, 2013 at 05:11:11PM +0000, Russell King - ARM Linux wrote:
> On Sun, Jan 27, 2013 at 11:07:22AM +0100, Andrew Lunn wrote:
> > +static struct priv
> > +{
> > +	struct clk *cpu_clk;
> > +	struct clk *ddr_clk;
> > +	struct clk *powersave_clk;
> > +	struct device *dev;
> > +	void __iomem *base;
> > +} priv;
> 
> I guess you probably think that the compiler will do something special
> with this

Nope, i expect nothing at all from the compiler. I just know i need
only one of these structures so i statically allocated it. I could
allocate it dynamically, probably get the cleanup wrong in the error
path and get shouted at by Russel King. Oh well...

> > +static unsigned int kirkwood_cpufreq_get_cpu_frequency(unsigned int cpu)
> > +{
> > +	if (__clk_is_enabled(priv.powersave_clk))
> 
> This looks to me to be a layering violation.

Possibly is. Not sure. This is a gated clock, so clk_get_rate()
returns the rate of the parent clock independent of if the gated clock
is enabled or not. So that does not help me. The cpufreq driver needs
to cause a transition on this clock, either disabled->enabled, or
enabled->disabled, then do a WFI, and once the system is stable it
wakes up. If there is no transition, it was already enabled and all
that clk_enable() does is increment the count, the WFI never exits and
the system sleeps forever. I'm assuming here that no other driver is
using this clock, which i think is a sensible assumption. But i've no
idea of the initial state of the clock. Did uboot enable it or not?

Thanks
	Andrew

[PATCH v3 2/3] arm: kirkwood: Instantiate cpufreq driver

[Andrew Lunn]

Register a platform driver structure for the cpufreq driver.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/Kconfig                                  |    1 +
 arch/arm/mach-kirkwood/board-dt.c                 |    3 ++-
 arch/arm/mach-kirkwood/common.c                   |   23 +++++++++++++++++++++
 arch/arm/mach-kirkwood/common.h                   |    2 ++
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h |    2 ++
 5 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 67874b8..830975b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -543,6 +543,7 @@ config ARCH_DOVE
 
 config ARCH_KIRKWOOD
 	bool "Marvell Kirkwood"
+	select ARCH_HAS_CPUFREQ
 	select ARCH_REQUIRE_GPIOLIB
 	select CPU_FEROCEON
 	select GENERIC_CLOCKEVENTS
diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index de4fd2b..fab541d 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -70,7 +70,6 @@ static void __init kirkwood_legacy_clk_init(void)
 	clkspec.args[0] = CGC_BIT_SDIO;
 	orion_clkdev_add(NULL, "mvsdio",
 			 of_clk_get_from_provider(&clkspec));
-
 }
 
 static void __init kirkwood_of_clk_init(void)
@@ -95,6 +94,8 @@ static void __init kirkwood_dt_init(void)
 
 	kirkwood_l2_init();
 
+	kirkwood_cpufreq_init();
+
 	/* Setup root of clk tree */
 	kirkwood_of_clk_init();
 
diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index bac21a5..a3dc21c 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -584,6 +584,29 @@ void __init kirkwood_audio_init(void)
 }
 
 /*****************************************************************************
+ * CPU Frequency
+ ****************************************************************************/
+static struct resource kirkwood_cpufreq_resources[] = {
+	[0] = {
+		.start  = CPU_CONTROL_PHYS,
+		.end    = CPU_CONTROL_PHYS + 3,
+		.flags  = IORESOURCE_MEM,
+	},
+};
+
+static struct platform_device kirkwood_cpufreq_device = {
+	.name		= "kirkwood-cpufreq",
+	.id		= -1,
+	.num_resources	= ARRAY_SIZE(kirkwood_cpufreq_resources),
+	.resource	= kirkwood_cpufreq_resources,
+};
+
+void __init kirkwood_cpufreq_init(void)
+{
+	platform_device_register(&kirkwood_cpufreq_device);
+}
+
+/*****************************************************************************
  * General
  ****************************************************************************/
 /*
diff --git a/arch/arm/mach-kirkwood/common.h b/arch/arm/mach-kirkwood/common.h
index 5ffa57f..9ede04b 100644
--- a/arch/arm/mach-kirkwood/common.h
+++ b/arch/arm/mach-kirkwood/common.h
@@ -50,6 +50,8 @@ void kirkwood_nand_init(struct mtd_partition *parts, int nr_parts, int delay);
 void kirkwood_nand_init_rnb(struct mtd_partition *parts, int nr_parts,
 			    int (*dev_ready)(struct mtd_info *));
 void kirkwood_audio_init(void);
+void kirkwood_cpufreq_init(void);
+
 void kirkwood_restart(char, const char *);
 void kirkwood_clk_init(void);
 
diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
index 5c82b7d..d4cbe5e 100644
--- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
+++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
@@ -17,6 +17,7 @@
 #define CPU_CONFIG_ERROR_PROP	0x00000004
 
 #define CPU_CONTROL		(BRIDGE_VIRT_BASE + 0x0104)
+#define CPU_CONTROL_PHYS	(BRIDGE_PHYS_BASE + 0x0104)
 #define CPU_RESET		0x00000002
 
 #define RSTOUTn_MASK		(BRIDGE_VIRT_BASE + 0x0108)
@@ -69,6 +70,7 @@
 #define CGC_RUNIT		(1 << 7)
 #define CGC_XOR0		(1 << 8)
 #define CGC_AUDIO		(1 << 9)
+#define CGC_POWERSAVE           (1 << 11)
 #define CGC_SATA0		(1 << 14)
 #define CGC_SATA1		(1 << 15)
 #define CGC_XOR1		(1 << 16)
-- 
1.7.10.4

[PATCH v3 2/3] arm: kirkwood: Instantiate cpufreq driver

[Jason Cooper]

On Sun, Jan 27, 2013 at 11:07:23AM +0100, Andrew Lunn wrote:
> Register a platform driver structure for the cpufreq driver.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  arch/arm/Kconfig                                  |    1 +
>  arch/arm/mach-kirkwood/board-dt.c                 |    3 ++-
>  arch/arm/mach-kirkwood/common.c                   |   23 +++++++++++++++++++++
>  arch/arm/mach-kirkwood/common.h                   |    2 ++
>  arch/arm/mach-kirkwood/include/mach/bridge-regs.h |    2 ++
>  5 files changed, 30 insertions(+), 1 deletion(-)

Thanks again to Andrew for digging this one up.  Applied to mvebu/soc

thx,

Jason.

[PATCH v3 3/3] arm: kirkwood: Enable cpufreq and ondemand on kirkwood_defconfig

[Andrew Lunn]

Now that we have a cpufreq driver for kirkwood, enable it in
kirkwood_defconfig and set the default governer to ondemand.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/configs/kirkwood_defconfig |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/kirkwood_defconfig b/arch/arm/configs/kirkwood_defconfig
index 93f3794..6ecb7de 100644
--- a/arch/arm/configs/kirkwood_defconfig
+++ b/arch/arm/configs/kirkwood_defconfig
@@ -55,6 +55,9 @@ CONFIG_AEABI=y
 # CONFIG_OABI_COMPAT is not set
 CONFIG_ZBOOT_ROM_TEXT=0x0
 CONFIG_ZBOOT_ROM_BSS=0x0
+CONFIG_CPU_FREQ=y
+CONFIG_CPU_FREQ_STAT_DETAILS=y
+CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y
 CONFIG_CPU_IDLE=y
 CONFIG_NET=y
 CONFIG_PACKET=y
-- 
1.7.10.4

[PATCH v3 3/3] arm: kirkwood: Enable cpufreq and ondemand on kirkwood_defconfig

[Jason Cooper]

On Sun, Jan 27, 2013 at 11:07:24AM +0100, Andrew Lunn wrote:
> Now that we have a cpufreq driver for kirkwood, enable it in
> kirkwood_defconfig and set the default governer to ondemand.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  arch/arm/configs/kirkwood_defconfig |    3 +++
>  1 file changed, 3 insertions(+)

Applied to mvebu/defconfig

thx,

Jason.