[PATCH v4 0/4] Dove cpufreq driver

[PATCH v4 0/4] Dove cpufreq driver

[Andrew Lunn]

This patchset adds a cpufreq driver for Dove.

The patchset is based on Rafael's linux-next branch from yesterday.
Quite likely Jason Cooper will take patches 2-3 via the mvebu tree,
but i will let you decide on that.

For version 3 there was a comment that the device tree binding should
be reviewed, and Viresh offered to do that. The binding is identical
to the kirkwood binding, with the addition that Dove requires an
interrupt handler, so the binding also lists this interrupt, using the
standard DT properties for interrupts.

Andrew Lunn (4):
  cpufreq: Add a cpufreq driver for Marvell Dove
  mvebu: Dove: Instantiate cpufreq driver.
  mvebu: Dove: Enable cpufreq driver in defconfig
  mvebu: Dove: Add clocks and DFS interrupt to cpu node in DT.

 arch/arm/Kconfig                       |   1 +
 arch/arm/boot/dts/dove.dtsi            |   4 +
 arch/arm/configs/dove_defconfig        |   2 +
 arch/arm/mach-dove/board-dt.c          |   3 +
 arch/arm/mach-dove/common.c            |  36 +++++
 arch/arm/mach-dove/common.h            |   1 +
 arch/arm/mach-dove/include/mach/dove.h |   1 +
 drivers/cpufreq/Kconfig.arm            |   5 +
 drivers/cpufreq/Makefile               |   1 +
 drivers/cpufreq/dove-cpufreq.c         | 259 +++++++++++++++++++++++++++++++++
 10 files changed, 313 insertions(+)
 create mode 100644 drivers/cpufreq/dove-cpufreq.c

-- 
1.8.5

[PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Andrew Lunn]

The Marvell Dove SoC can run the CPU at two frequencies. The high
frequencey is from a PLL, while the lower is the same as the DDR
clock. Add a cpufreq driver to swap between these frequences.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Sort header files
Remove unneeded header files
Notify only on change
Use get_cpu_device(0), devm_clk_get()
Use platform_get_resource_byname()
Error check clk_prepare_enable()
Comment the interrupt handler

rebase onto 3.13-rc2 linux-next
use target_index
---
 drivers/cpufreq/Kconfig.arm    |   5 +
 drivers/cpufreq/Makefile       |   1 +
 drivers/cpufreq/dove-cpufreq.c | 259 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 265 insertions(+)
 create mode 100644 drivers/cpufreq/dove-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index ce52ed949249..af7c4947f218 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -8,6 +8,11 @@ config ARM_BIG_LITTLE_CPUFREQ
 	help
 	  This enables the Generic CPUfreq driver for ARM big.LITTLE platforms.
 
+config ARM_DOVE_CPUFREQ
+	def_bool ARCH_DOVE && OF
+	help
+	  This adds the CPUFreq driver for Marvell Dove SoCs.
+
 config ARM_DT_BL_CPUFREQ
 	tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
 	depends on ARM_BIG_LITTLE_CPUFREQ && OF
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 74945652dd7a..cd72c2bbfd44 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_ARM_DT_BL_CPUFREQ)		+= arm_big_little_dt.o
 
 obj-$(CONFIG_ARCH_DAVINCI_DA850)	+= davinci-cpufreq.o
 obj-$(CONFIG_UX500_SOC_DB8500)		+= dbx500-cpufreq.o
+obj-$(CONFIG_ARM_DOVE_CPUFREQ)		+= dove-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS_CPUFREQ)	+= exynos-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ)	+= exynos4210-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ)	+= exynos4x12-cpufreq.o
diff --git a/drivers/cpufreq/dove-cpufreq.c b/drivers/cpufreq/dove-cpufreq.c
new file mode 100644
index 000000000000..7f7711ccccbe
--- /dev/null
+++ b/drivers/cpufreq/dove-cpufreq.c
@@ -0,0 +1,259 @@
+/*
+ *	dove_freq.c: cpufreq driver for the Marvell dove
+ *
+ *	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/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <asm/proc-fns.h>
+
+#define DFS_CR			0x00
+#define  DFS_EN			BIT(0)
+#define  CPU_SLOW_EN		BIT(1)
+#define  L2_RATIO_OFFS		9
+#define  L2_RATIO_MASK		(0x3F << L2_RATIO_OFFS)
+#define DFS_SR			0x04
+#define  CPU_SLOW_MODE_STTS	BIT(1)
+
+/* PMU_CR */
+#define  MASK_FIQ		BIT(28)
+#define  MASK_IRQ		BIT(24)	/* PMU_CR */
+
+/* CPU Clock Divider Control 0 Register */
+#define DPRATIO_OFFS		24
+#define DPRATIO_MASK		(0x3F << DPRATIO_OFFS)
+#define XPRATIO_OFFS		16
+#define XPRATIO_MASK		(0x3F << XPRATIO_OFFS)
+
+static struct priv
+{
+	struct clk *cpu_clk;
+	struct clk *ddr_clk;
+	struct device *dev;
+	unsigned long dpratio;
+	unsigned long xpratio;
+	void __iomem *dfs;
+	void __iomem *pmu_cr;
+	void __iomem *pmu_clk_div;
+} priv;
+
+#define STATE_CPU_FREQ 0x01
+#define STATE_DDR_FREQ 0x02
+
+/*
+ * Dove 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 dove_freq_table[] = {
+	{STATE_CPU_FREQ,	0}, /* CPU uses cpuclk */
+	{STATE_DDR_FREQ,	0}, /* CPU uses ddrclk */
+	{0,			CPUFREQ_TABLE_END},
+};
+
+static unsigned int dove_cpufreq_get_cpu_frequency(unsigned int cpu)
+{
+	unsigned long reg = readl_relaxed(priv.dfs + DFS_SR);
+
+	if (reg & CPU_SLOW_MODE_STTS)
+		return dove_freq_table[1].frequency;
+	return dove_freq_table[0].frequency;
+}
+
+static int dove_cpufreq_target(struct cpufreq_policy *policy,
+			       unsigned int index)
+{
+	unsigned int state = dove_freq_table[index].driver_data;
+	unsigned long reg, cr;
+
+	local_irq_disable();
+
+	/* Mask IRQ and FIQ to CPU */
+	cr = readl(priv.pmu_cr);
+	cr |= MASK_IRQ | MASK_FIQ;
+	writel(cr, priv.pmu_cr);
+
+	/* Set/Clear the CPU_SLOW_EN bit */
+	reg = readl_relaxed(priv.dfs + DFS_CR);
+	reg &= ~L2_RATIO_MASK;
+
+	switch (state) {
+	case STATE_CPU_FREQ:
+		reg |= priv.xpratio;
+		reg &= ~CPU_SLOW_EN;
+		break;
+	case STATE_DDR_FREQ:
+		reg |= (priv.dpratio | CPU_SLOW_EN);
+		break;
+	}
+
+	/* Start the DFS process */
+	reg |= DFS_EN;
+
+	writel(reg, priv.dfs + DFS_CR);
+
+	/* Wait-for-Interrupt, while the hardware changes frequency */
+	cpu_do_idle();
+
+	local_irq_enable();
+
+	return 0;
+}
+
+static int dove_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+	return cpufreq_generic_init(policy, dove_freq_table, 5000);
+}
+
+/*
+ * Handle the interrupt raised when the frequency change is
+ * complete. Without having an interrupt handler, the WFI will
+ * exit on the next timer tick, reducing performance.
+ */
+static irqreturn_t dove_cpufreq_irq(int irq, void *dev)
+{
+	return IRQ_HANDLED;
+}
+
+static struct cpufreq_driver dove_cpufreq_driver = {
+	.get	= dove_cpufreq_get_cpu_frequency,
+	.verify	= cpufreq_generic_frequency_table_verify,
+	.target_index = dove_cpufreq_target,
+	.init	= dove_cpufreq_cpu_init,
+	.exit	= cpufreq_generic_exit,
+	.name	= "dove-cpufreq",
+	.attr	= cpufreq_generic_attr,
+};
+
+static int dove_cpufreq_probe(struct platform_device *pdev)
+{
+	struct device *cpu_dev;
+	struct resource *res;
+	int err, irq;
+
+	memset(&priv, 0, sizeof(priv));
+	priv.dev = &pdev->dev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "cpufreq: DFS");
+	priv.dfs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv.dfs))
+		return PTR_ERR(priv.dfs);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "cpufreq: PMU CR");
+	priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv.pmu_cr))
+		return PTR_ERR(priv.pmu_cr);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "cpufreq: PMU Clk Div");
+	priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv.pmu_clk_div))
+		return PTR_ERR(priv.pmu_clk_div);
+
+	cpu_dev = get_cpu_device(0);
+
+	priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk");
+	if (IS_ERR(priv.cpu_clk)) {
+		err = PTR_ERR(priv.cpu_clk);
+		goto out;
+	}
+
+	err = clk_prepare_enable(priv.cpu_clk);
+	if (err)
+		goto out;
+
+	dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
+
+	priv.ddr_clk = devm_clk_get(cpu_dev, "ddrclk");
+	if (IS_ERR(priv.ddr_clk)) {
+		err = PTR_ERR(priv.ddr_clk);
+		goto out;
+	}
+
+	err = clk_prepare_enable(priv.ddr_clk);
+	if (err)
+		goto out;
+
+	dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
+
+	irq = irq_of_parse_and_map(cpu_dev->of_node, 0);
+	if (!irq) {
+		err = -ENXIO;
+		goto out;
+	}
+
+	err = devm_request_irq(&pdev->dev, irq, dove_cpufreq_irq,
+			       0, "dove-cpufreq", NULL);
+	if (err) {
+		dev_err(&pdev->dev, "cannot assign irq %d, %d\n", irq, err);
+		goto out;
+	}
+
+	/* Read the target ratio which should be the DDR ratio */
+	priv.dpratio = readl_relaxed(priv.pmu_clk_div);
+	priv.dpratio = (priv.dpratio & DPRATIO_MASK) >> DPRATIO_OFFS;
+	priv.dpratio = priv.dpratio << L2_RATIO_OFFS;
+
+	/* Save L2 ratio at reset */
+	priv.xpratio = readl(priv.pmu_clk_div);
+	priv.xpratio = (priv.xpratio & XPRATIO_MASK) >> XPRATIO_OFFS;
+	priv.xpratio = priv.xpratio << L2_RATIO_OFFS;
+
+	err = cpufreq_register_driver(&dove_cpufreq_driver);
+	if (!err)
+		return 0;
+
+	dev_err(priv.dev, "Failed to register cpufreq driver");
+
+out:
+	clk_disable_unprepare(priv.ddr_clk);
+	clk_disable_unprepare(priv.cpu_clk);
+
+	return err;
+}
+
+static int dove_cpufreq_remove(struct platform_device *pdev)
+{
+	cpufreq_unregister_driver(&dove_cpufreq_driver);
+
+	clk_disable_unprepare(priv.ddr_clk);
+	clk_disable_unprepare(priv.cpu_clk);
+
+	return 0;
+}
+
+static struct platform_driver dove_cpufreq_platform_driver = {
+	.probe = dove_cpufreq_probe,
+	.remove = dove_cpufreq_remove,
+	.driver = {
+		.name = "dove-cpufreq",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_platform_driver(dove_cpufreq_platform_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch");
+MODULE_DESCRIPTION("cpufreq driver for Marvell's dove CPU");
+MODULE_ALIAS("platform:dove-cpufreq");
-- 
1.8.5

Re: [PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Jason Cooper]

On Wed, Dec 04, 2013 at 03:17:18PM +0100, Andrew Lunn wrote:
> The Marvell Dove SoC can run the CPU at two frequencies. The high
> frequencey is from a PLL, while the lower is the same as the DDR
> clock. Add a cpufreq driver to swap between these frequences.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Sort header files
> Remove unneeded header files
> Notify only on change
> Use get_cpu_device(0), devm_clk_get()
> Use platform_get_resource_byname()
> Error check clk_prepare_enable()
> Comment the interrupt handler
> 
> rebase onto 3.13-rc2 linux-next

Is there something you need that prevents you from basing on v3.13-rc1?

> use target_index
> ---
>  drivers/cpufreq/Kconfig.arm    |   5 +
>  drivers/cpufreq/Makefile       |   1 +
>  drivers/cpufreq/dove-cpufreq.c | 259 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 265 insertions(+)
>  create mode 100644 drivers/cpufreq/dove-cpufreq.c

Other than that,

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

Re: [PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Andrew Lunn]

On Wed, Dec 04, 2013 at 09:33:17AM -0500, Jason Cooper wrote:
> On Wed, Dec 04, 2013 at 03:17:18PM +0100, Andrew Lunn wrote:
> > The Marvell Dove SoC can run the CPU at two frequencies. The high
> > frequencey is from a PLL, while the lower is the same as the DDR
> > clock. Add a cpufreq driver to swap between these frequences.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > Sort header files
> > Remove unneeded header files
> > Notify only on change
> > Use get_cpu_device(0), devm_clk_get()
> > Use platform_get_resource_byname()
> > Error check clk_prepare_enable()
> > Comment the interrupt handler
> > 
> > rebase onto 3.13-rc2 linux-next
> 
> Is there something you need that prevents you from basing on v3.13-rc1?

It seems to be Rafael's policy that patches for pm need to be based on
his linux-next branch. Viresh has also been making some major changes
to all the cpufreq drivers to move code into the core and out of
drivers. These changes are in rafaels linux-next, and i've made
similar changes to the driver so it fits in.

	Andrew

Re: [PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Jason Cooper]

On Wed, Dec 04, 2013 at 03:56:35PM +0100, Andrew Lunn wrote:
> On Wed, Dec 04, 2013 at 09:33:17AM -0500, Jason Cooper wrote:
> > On Wed, Dec 04, 2013 at 03:17:18PM +0100, Andrew Lunn wrote:
> > > The Marvell Dove SoC can run the CPU at two frequencies. The high
> > > frequencey is from a PLL, while the lower is the same as the DDR
> > > clock. Add a cpufreq driver to swap between these frequences.
> > > 
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > > Sort header files
> > > Remove unneeded header files
> > > Notify only on change
> > > Use get_cpu_device(0), devm_clk_get()
> > > Use platform_get_resource_byname()
> > > Error check clk_prepare_enable()
> > > Comment the interrupt handler
> > > 
> > > rebase onto 3.13-rc2 linux-next
> > 
> > Is there something you need that prevents you from basing on v3.13-rc1?
> 
> It seems to be Rafael's policy that patches for pm need to be based on
> his linux-next branch. Viresh has also been making some major changes
> to all the cpufreq drivers to move code into the core and out of
> drivers. These changes are in rafaels linux-next, and i've made
> similar changes to the driver so it fits in.

Ah, ok.

thx,

Jason.

Re: [PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Mark Rutland]

On Wed, Dec 04, 2013 at 02:17:18PM +0000, Andrew Lunn wrote:
> The Marvell Dove SoC can run the CPU at two frequencies. The high
> frequencey is from a PLL, while the lower is the same as the DDR
> clock. Add a cpufreq driver to swap between these frequences.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Sort header files
> Remove unneeded header files
> Notify only on change
> Use get_cpu_device(0), devm_clk_get()
> Use platform_get_resource_byname()
> Error check clk_prepare_enable()
> Comment the interrupt handler
> 
> rebase onto 3.13-rc2 linux-next
> use target_index
> ---
>  drivers/cpufreq/Kconfig.arm    |   5 +
>  drivers/cpufreq/Makefile       |   1 +
>  drivers/cpufreq/dove-cpufreq.c | 259 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 265 insertions(+)
>  create mode 100644 drivers/cpufreq/dove-cpufreq.c

[...]

> +static int dove_cpufreq_target(struct cpufreq_policy *policy,
> +			       unsigned int index)
> +{
> +	unsigned int state = dove_freq_table[index].driver_data;
> +	unsigned long reg, cr;
> +
> +	local_irq_disable();
> +
> +	/* Mask IRQ and FIQ to CPU */
> +	cr = readl(priv.pmu_cr);
> +	cr |= MASK_IRQ | MASK_FIQ;
> +	writel(cr, priv.pmu_cr);
> +
> +	/* Set/Clear the CPU_SLOW_EN bit */
> +	reg = readl_relaxed(priv.dfs + DFS_CR);
> +	reg &= ~L2_RATIO_MASK;
> +
> +	switch (state) {
> +	case STATE_CPU_FREQ:
> +		reg |= priv.xpratio;
> +		reg &= ~CPU_SLOW_EN;
> +		break;
> +	case STATE_DDR_FREQ:
> +		reg |= (priv.dpratio | CPU_SLOW_EN);
> +		break;
> +	}
> +
> +	/* Start the DFS process */
> +	reg |= DFS_EN;
> +
> +	writel(reg, priv.dfs + DFS_CR);
> +
> +	/* Wait-for-Interrupt, while the hardware changes frequency */
> +	cpu_do_idle();
> +
> +	local_irq_enable();

Just to check -- does the PMU automatically unmask IRQ and FIQ? 

[...]

> +static int dove_cpufreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cpu_dev;
> +	struct resource *res;
> +	int err, irq;
> +
> +	memset(&priv, 0, sizeof(priv));
> +	priv.dev = &pdev->dev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +					   "cpufreq: DFS");
> +	priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv.dfs))
> +		return PTR_ERR(priv.dfs);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +					   "cpufreq: PMU CR");
> +	priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv.pmu_cr))
> +		return PTR_ERR(priv.pmu_cr);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +					   "cpufreq: PMU Clk Div");
> +	priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv.pmu_clk_div))
> +		return PTR_ERR(priv.pmu_clk_div);

I'd very much prefer that the PMU were described in the DT, and the
driver probed based on that.

> +
> +	cpu_dev = get_cpu_device(0);
> +
> +	priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk");
> +	if (IS_ERR(priv.cpu_clk)) {
> +		err = PTR_ERR(priv.cpu_clk);
> +		goto out;
> +	}

I think it would be better to describe the clocks as being fed to the
PMU than directly to the CPU -- the PMU selects the appropriate clock
and feeds it to the CPU.

Also, the clocks could be named better with respect to their logical
function. One is the high-frequency default, and the other is a
low-frequency option. They're both able to feed the CPU, and the fact
that one also feeds the DDR isn't relevant to the CPU. Does the PMu also
control the input to the DDR?

> +
> +	err = clk_prepare_enable(priv.cpu_clk);
> +	if (err)
> +		goto out;
> +
> +	dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
> +
> +	priv.ddr_clk = devm_clk_get(cpu_dev, "ddrclk");
> +	if (IS_ERR(priv.ddr_clk)) {
> +		err = PTR_ERR(priv.ddr_clk);
> +		goto out;
> +	}
> +
> +	err = clk_prepare_enable(priv.ddr_clk);
> +	if (err)
> +		goto out;
> +
> +	dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
> +
> +	irq = irq_of_parse_and_map(cpu_dev->of_node, 0);
> +	if (!irq) {
> +		err = -ENXIO;
> +		goto out;
> +	}

Similarly, the interrupt is a property of the PMU.

Thanks,
Mark.

Re: [PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Andrew Lunn]

> > +static int dove_cpufreq_target(struct cpufreq_policy *policy,
> > +			       unsigned int index)
> > +{
> > +	unsigned int state = dove_freq_table[index].driver_data;
> > +	unsigned long reg, cr;
> > +
> > +	local_irq_disable();
> > +
> > +	/* Mask IRQ and FIQ to CPU */
> > +	cr = readl(priv.pmu_cr);
> > +	cr |= MASK_IRQ | MASK_FIQ;
> > +	writel(cr, priv.pmu_cr);
> > +
> > +	/* Set/Clear the CPU_SLOW_EN bit */
> > +	reg = readl_relaxed(priv.dfs + DFS_CR);
> > +	reg &= ~L2_RATIO_MASK;
> > +
> > +	switch (state) {
> > +	case STATE_CPU_FREQ:
> > +		reg |= priv.xpratio;
> > +		reg &= ~CPU_SLOW_EN;
> > +		break;
> > +	case STATE_DDR_FREQ:
> > +		reg |= (priv.dpratio | CPU_SLOW_EN);
> > +		break;
> > +	}
> > +
> > +	/* Start the DFS process */
> > +	reg |= DFS_EN;
> > +
> > +	writel(reg, priv.dfs + DFS_CR);
> > +
> > +	/* Wait-for-Interrupt, while the hardware changes frequency */
> > +	cpu_do_idle();
> > +
> > +	local_irq_enable();
> 
> Just to check -- does the PMU automatically unmask IRQ and FIQ? 

The data sheet says so, yes.

> > +static int dove_cpufreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device *cpu_dev;
> > +	struct resource *res;
> > +	int err, irq;
> > +
> > +	memset(&priv, 0, sizeof(priv));
> > +	priv.dev = &pdev->dev;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +					   "cpufreq: DFS");
> > +	priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(priv.dfs))
> > +		return PTR_ERR(priv.dfs);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +					   "cpufreq: PMU CR");
> > +	priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(priv.pmu_cr))
> > +		return PTR_ERR(priv.pmu_cr);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +					   "cpufreq: PMU Clk Div");
> > +	priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(priv.pmu_clk_div))
> > +		return PTR_ERR(priv.pmu_clk_div);
> 
> I'd very much prefer that the PMU were described in the DT, and the
> driver probed based on that.

Please see my other response. We can discuss that there.
 
> > +
> > +	cpu_dev = get_cpu_device(0);
> > +
> > +	priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk");
> > +	if (IS_ERR(priv.cpu_clk)) {
> > +		err = PTR_ERR(priv.cpu_clk);
> > +		goto out;
> > +	}
> 
> I think it would be better to describe the clocks as being fed to the
> PMU than directly to the CPU -- the PMU selects the appropriate clock
> and feeds it to the CPU.

The wording in the data sheet is not very clear:

    The PMU initiates new clock ratio values for the CPU subsystem
    clocks generation unit.

So to me, the PMU is not the clock consumer, it just controls the
clock generation unit. There is also a block diagram which shows the
"CPU Subsystem ClockGen" as being external to the PMU.
 
> Also, the clocks could be named better with respect to their logical
> function. One is the high-frequency default, and the other is a
> low-frequency option. They're both able to feed the CPU, and the fact
> that one also feeds the DDR isn't relevant to the CPU.

The data sheet uses the terms Turbo Speed and Slow Speed modes. So i
could call them turbo and slow? However, the current names follow the
kirkwood cpufreq driver. It also can swap between a fast clock and the
DDR clock. So i thought keeping the drivers similar would help with
the maintenance burden. 

    Andrew

Re: [PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Mark Rutland]

On Wed, Dec 04, 2013 at 03:27:26PM +0000, Andrew Lunn wrote:

[...]
  
> > > +
> > > +	cpu_dev = get_cpu_device(0);
> > > +
> > > +	priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk");
> > > +	if (IS_ERR(priv.cpu_clk)) {
> > > +		err = PTR_ERR(priv.cpu_clk);
> > > +		goto out;
> > > +	}
> > 
> > I think it would be better to describe the clocks as being fed to the
> > PMU than directly to the CPU -- the PMU selects the appropriate clock
> > and feeds it to the CPU.
> 
> The wording in the data sheet is not very clear:
> 
>     The PMU initiates new clock ratio values for the CPU subsystem
>     clocks generation unit.
> 
> So to me, the PMU is not the clock consumer, it just controls the
> clock generation unit. There is also a block diagram which shows the
> "CPU Subsystem ClockGen" as being external to the PMU.

Hmm. That's somewhat arguable.

>From a block diagram I've found, the situation seems more like the
ClockGen takes two clocks as input, and feeds the appropriate clock to
the CPU. Logically, the CPU itself is a consumer of one clock.

The ClockGen unit is controlled by a clock manager. The clock manager
appears to be a component of the PMU itself, but I may have
misunderstood. If that is the case, and the CPU Subsytem ClockGen is not
controlled by anything else, then I don't see the problem with grouping
that together logically with the clock manager (and therefore the PMU).
That would imply describing the clock inputs on the PMU is fine.

Have I missed something?

One thing I'm worried about is adding lots of SoC-specific variations on
data to generic nodes rather than doing that in a uniform fashion. I
believe describing the clocks in the way you propose clashes with the
cpufreq-cpu0 way of doing things (whereby the CPU has one clock input).

If the ClockGen were modelled as a mux that allows switching between two
rates, then that would make the description more uniform.

>  
> > Also, the clocks could be named better with respect to their logical
> > function. One is the high-frequency default, and the other is a
> > low-frequency option. They're both able to feed the CPU, and the fact
> > that one also feeds the DDR isn't relevant to the CPU.
> 
> The data sheet uses the terms Turbo Speed and Slow Speed modes. So i
> could call them turbo and slow? However, the current names follow the
> kirkwood cpufreq driver. It also can swap between a fast clock and the
> DDR clock. So i thought keeping the drivers similar would help with
> the maintenance burden. 

I think given the above, the changes I'm asking for would already
involve sufficient changes to make this concern irrelevant. However, I
don't want to force an unmaintainable change.

Mark.

[RFC PATCH v5 0/4] Dove Cpufreq driver

[Andrew Lunn]

This is an RFC where i attempt to address Mark Rutland's request for a
PMU node in DT, and all clocks and interrupts required by the cpufreq
driver are within this node. I would like to receive comments about
this binding.

This version does not address any other comments made to the code. I
will fix those issues once the binding is mostly decided.

Thanks
	Andrew

Andrew Lunn (4):
  cpufreq: Add a cpufreq driver for Marvell Dove
  mvebu: Dove: Indicate the architecture supports cpufreq.
  mvebu: Dove: Enable cpufreq driver in defconfig
  mvebu: Dove: Add PMU node for cpufreq driver

 .../devicetree/bindings/cpufreq/cpufreq-dove.txt   |  23 ++
 arch/arm/Kconfig                                   |   1 +
 arch/arm/boot/dts/dove.dtsi                        |  10 +
 arch/arm/configs/dove_defconfig                    |   2 +
 drivers/cpufreq/Kconfig.arm                        |   5 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/dove-cpufreq.c                     | 265 +++++++++++++++++++++
 7 files changed, 307 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt
 create mode 100644 drivers/cpufreq/dove-cpufreq.c

-- 
1.8.5.1

[RFC PATCH v5 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Andrew Lunn]

The Marvell Dove SoC can run the CPU at two frequencies. The high
frequencey is from a PLL, while the lower is the same as the DDR
clock. Add a cpufreq driver to swap between these frequences.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Sort header files
Remove unneeded header files
Notify only on change
Use get_cpu_device(0), devm_clk_get()
Use platform_get_resource_byname()
Error check clk_prepare_enable()
Comment the interrupt handler

rebase onto 3.13-rc2 linux-next
use target_index
---
 .../devicetree/bindings/cpufreq/cpufreq-dove.txt   |  23 ++
 drivers/cpufreq/Kconfig.arm                        |   5 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/dove-cpufreq.c                     | 265 +++++++++++++++++++++
 4 files changed, 294 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt
 create mode 100644 drivers/cpufreq/dove-cpufreq.c

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt
new file mode 100644
index 000000000000..f95cc2234fc1
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt
@@ -0,0 +1,23 @@
+Dove cpufreq driver
+-------------------
+
+The Dove cpufreq driver makes use of the pmu node in the device tree.
+
+Required properties:
+- compatibility: Must be "marvell,dove-pmu"
+- interrupts:	 Interrupt to know the completion of cpu frequency change.
+- clocks:	 phandles and clock specifiers pairs for CPU and DDR clock
+- clock-names: 	 "cpu" and "ddr"
+- reg: 		 Address ranges of the PMU. Two ranges are expected.
+
+Example:
+
+	pmu: pmu@d0000 {
+		compatible = "marvell,dove-pmu";
+		reg = <0xd0000 0x0700>,
+		      <0xd8000 0x0140>;
+		clocks = <&core_clk 1>, <&core_clk 3>;
+		clock-names = "cpu", "ddr";
+		interrupt-parent = <&pmu_intc>;
+		interrupts = <0>;
+	};
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index ce52ed949249..af7c4947f218 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -8,6 +8,11 @@ config ARM_BIG_LITTLE_CPUFREQ
 	help
 	  This enables the Generic CPUfreq driver for ARM big.LITTLE platforms.
 
+config ARM_DOVE_CPUFREQ
+	def_bool ARCH_DOVE && OF
+	help
+	  This adds the CPUFreq driver for Marvell Dove SoCs.
+
 config ARM_DT_BL_CPUFREQ
 	tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
 	depends on ARM_BIG_LITTLE_CPUFREQ && OF
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 74945652dd7a..cd72c2bbfd44 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_ARM_DT_BL_CPUFREQ)		+= arm_big_little_dt.o
 
 obj-$(CONFIG_ARCH_DAVINCI_DA850)	+= davinci-cpufreq.o
 obj-$(CONFIG_UX500_SOC_DB8500)		+= dbx500-cpufreq.o
+obj-$(CONFIG_ARM_DOVE_CPUFREQ)		+= dove-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS_CPUFREQ)	+= exynos-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ)	+= exynos4210-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ)	+= exynos4x12-cpufreq.o
diff --git a/drivers/cpufreq/dove-cpufreq.c b/drivers/cpufreq/dove-cpufreq.c
new file mode 100644
index 000000000000..158dcf27f79f
--- /dev/null
+++ b/drivers/cpufreq/dove-cpufreq.c
@@ -0,0 +1,265 @@
+/*
+ *	dove_freq.c: cpufreq driver for the Marvell dove
+ *
+ *	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/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <asm/proc-fns.h>
+
+#define DFS_CR			0x0000 /* Relative to core */
+#define  DFS_EN			BIT(0)
+#define  CPU_SLOW_EN		BIT(1)
+#define  L2_RATIO_OFFS		9
+#define  L2_RATIO_MASK		(0x3F << L2_RATIO_OFFS)
+
+#define DFS_SR			0x0004 /* Relative to core */
+#define  CPU_SLOW_MODE_STTS	BIT(1)
+
+#define PMU_CLK_DIV		0x0044 /* Relative to core */
+#define  DPRATIO_OFFS		24
+#define  DPRATIO_MASK		(0x3F << DPRATIO_OFFS)
+#define  XPRATIO_OFFS		16
+#define  XPRATIO_MASK		(0x3F << XPRATIO_OFFS)
+
+#define PMU_CR			0x8000
+#define  MASK_FIQ		BIT(28)
+#define  MASK_IRQ		BIT(24)	/* PMU_CR */
+
+static struct priv
+{
+	struct clk *cpu_clk;
+	struct clk *ddr_clk;
+	struct device *dev;
+	unsigned long dpratio;
+	unsigned long xpratio;
+	void __iomem *base;
+	void __iomem *pmu_cr;
+	void __iomem *pmu_clk_div;
+} priv;
+
+#define STATE_CPU_FREQ 0x01
+#define STATE_DDR_FREQ 0x02
+
+/*
+ * Dove 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 dove_freq_table[] = {
+	{STATE_CPU_FREQ,	0}, /* CPU uses cpuclk */
+	{STATE_DDR_FREQ,	0}, /* CPU uses ddrclk */
+	{0,			CPUFREQ_TABLE_END},
+};
+
+static unsigned int dove_cpufreq_get_cpu_frequency(unsigned int cpu)
+{
+	unsigned long reg = readl_relaxed(priv.base + DFS_SR);
+
+	if (reg & CPU_SLOW_MODE_STTS)
+		return dove_freq_table[1].frequency;
+	return dove_freq_table[0].frequency;
+}
+
+static int dove_cpufreq_target(struct cpufreq_policy *policy,
+			       unsigned int index)
+{
+	unsigned int state = dove_freq_table[index].driver_data;
+	unsigned long reg, cr;
+
+	local_irq_disable();
+
+	/* Mask IRQ and FIQ to CPU */
+	cr = readl(priv.base + PMU_CR);
+	cr |= MASK_IRQ | MASK_FIQ;
+	writel(cr, priv.base + PMU_CR);
+
+	/* Set/Clear the CPU_SLOW_EN bit */
+	reg = readl_relaxed(priv.base + DFS_CR);
+	reg &= ~L2_RATIO_MASK;
+
+	switch (state) {
+	case STATE_CPU_FREQ:
+		reg |= priv.xpratio;
+		reg &= ~CPU_SLOW_EN;
+		break;
+	case STATE_DDR_FREQ:
+		reg |= (priv.dpratio | CPU_SLOW_EN);
+		break;
+	}
+
+	/* Start the DFS process */
+	reg |= DFS_EN;
+
+	writel(reg, priv.base + DFS_CR);
+
+	/*
+	 * Wait-for-Interrupt, while the hardware changes frequency.
+	 * IRQ and FIQ will be automagically unmasked on
+	 * completion
+	 */
+	cpu_do_idle();
+
+	local_irq_enable();
+
+	return 0;
+}
+
+static int dove_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+	return cpufreq_generic_init(policy, dove_freq_table, 5000);
+}
+
+/*
+ * Handle the interrupt raised when the frequency change is
+ * complete. Without having an interrupt handler, the WFI will
+ * exit on the next timer tick, reducing performance.
+ */
+static irqreturn_t dove_cpufreq_irq(int irq, void *dev)
+{
+	return IRQ_HANDLED;
+}
+
+static struct cpufreq_driver dove_cpufreq_driver = {
+	.get	= dove_cpufreq_get_cpu_frequency,
+	.verify	= cpufreq_generic_frequency_table_verify,
+	.target_index = dove_cpufreq_target,
+	.init	= dove_cpufreq_cpu_init,
+	.exit	= cpufreq_generic_exit,
+	.name	= "dove-cpufreq",
+	.attr	= cpufreq_generic_attr,
+};
+
+static const struct of_device_id dove_cpufreq_match[] = {
+	{
+		.compatible = "marvell,dove-pmu",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, dove_cpufreq_match);
+
+
+static int dove_cpufreq_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int err, irq;
+
+	if (!np)
+		return -ENODEV;
+
+	memset(&priv, 0, sizeof(priv));
+	priv.dev = &pdev->dev;
+
+	priv.base = of_iomap(np, 0);
+	if (IS_ERR(priv.base)) {
+		err = PTR_ERR(priv.base);
+		goto out;
+	}
+
+	priv.cpu_clk = devm_clk_get(&pdev->dev, "cpu");
+	if (IS_ERR(priv.cpu_clk)) {
+		err = PTR_ERR(priv.cpu_clk);
+		goto out;
+	}
+
+	err = clk_prepare_enable(priv.cpu_clk);
+	if (err)
+		goto out;
+
+	dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
+
+	priv.ddr_clk = devm_clk_get(&pdev->dev, "ddr");
+	if (IS_ERR(priv.ddr_clk)) {
+		err = PTR_ERR(priv.ddr_clk);
+		goto out;
+	}
+
+	err = clk_prepare_enable(priv.ddr_clk);
+	if (err)
+		goto out;
+
+	dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (!irq) {
+		err = -ENXIO;
+		goto out;
+	}
+
+	err = devm_request_irq(&pdev->dev, irq, dove_cpufreq_irq,
+			       0, "dove-cpufreq", NULL);
+	if (err) {
+		dev_err(&pdev->dev, "cannot assign irq %d, %d\n", irq, err);
+		goto out;
+	}
+
+	/* Read the target ratio which should be the DDR ratio */
+	priv.dpratio = readl_relaxed(priv.base + PMU_CLK_DIV);
+	priv.dpratio = (priv.dpratio & DPRATIO_MASK) >> DPRATIO_OFFS;
+	priv.dpratio = priv.dpratio << L2_RATIO_OFFS;
+
+	/* Save L2 ratio at reset */
+	priv.xpratio = readl(priv.base + PMU_CLK_DIV);
+	priv.xpratio = (priv.xpratio & XPRATIO_MASK) >> XPRATIO_OFFS;
+	priv.xpratio = priv.xpratio << L2_RATIO_OFFS;
+
+	err = cpufreq_register_driver(&dove_cpufreq_driver);
+	if (!err)
+		return 0;
+
+	dev_err(priv.dev, "Failed to register cpufreq driver");
+
+out:
+	if (!IS_ERR(priv.ddr_clk))
+		clk_disable_unprepare(priv.ddr_clk);
+	if (!IS_ERR(priv.cpu_clk))
+		clk_disable_unprepare(priv.cpu_clk);
+
+	return err;
+}
+
+static int dove_cpufreq_remove(struct platform_device *pdev)
+{
+	cpufreq_unregister_driver(&dove_cpufreq_driver);
+
+	clk_disable_unprepare(priv.ddr_clk);
+	clk_disable_unprepare(priv.cpu_clk);
+
+	return 0;
+}
+
+static struct platform_driver dove_cpufreq_platform_driver = {
+	.probe = dove_cpufreq_probe,
+	.remove = dove_cpufreq_remove,
+	.driver = {
+		.name = "dove-cpufreq",
+		.owner = THIS_MODULE,
+		.of_match_table = dove_cpufreq_match,
+	},
+};
+
+module_platform_driver(dove_cpufreq_platform_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Andrew Lunn <andrew@lunn.ch");
+MODULE_DESCRIPTION("cpufreq driver for Marvell's dove CPU");
+MODULE_ALIAS("platform:dove-cpufreq");
-- 
1.8.5.1

Re: [RFC PATCH v5 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Jason Cooper]

On Tue, Dec 17, 2013 at 10:41:10PM +0100, Andrew Lunn wrote:
> The Marvell Dove SoC can run the CPU at two frequencies. The high
> frequencey is from a PLL, while the lower is the same as the DDR
> clock. Add a cpufreq driver to swap between these frequences.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Sort header files
> Remove unneeded header files
> Notify only on change
> Use get_cpu_device(0), devm_clk_get()
> Use platform_get_resource_byname()
> Error check clk_prepare_enable()
> Comment the interrupt handler
> 
> rebase onto 3.13-rc2 linux-next
> use target_index
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-dove.txt   |  23 ++
>  drivers/cpufreq/Kconfig.arm                        |   5 +
>  drivers/cpufreq/Makefile                           |   1 +
>  drivers/cpufreq/dove-cpufreq.c                     | 265 +++++++++++++++++++++
>  4 files changed, 294 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt
>  create mode 100644 drivers/cpufreq/dove-cpufreq.c
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt
> new file mode 100644
> index 000000000000..f95cc2234fc1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-dove.txt
> @@ -0,0 +1,23 @@
> +Dove cpufreq driver
> +-------------------
> +
> +The Dove cpufreq driver makes use of the pmu node in the device tree.

I would prefer to reword this to make it OS-agnostic.  The cpufreq
driver is Linux-specific.

> +
> +Required properties:
> +- compatibility: Must be "marvell,dove-pmu"
> +- interrupts:	 Interrupt to know the completion of cpu frequency change.
> +- clocks:	 phandles and clock specifiers pairs for CPU and DDR clock
> +- clock-names: 	 "cpu" and "ddr"
> +- reg: 		 Address ranges of the PMU. Two ranges are expected.

Shall we define the two ranges?

> +
> +Example:
> +
> +	pmu: pmu@d0000 {
> +		compatible = "marvell,dove-pmu";
> +		reg = <0xd0000 0x0700>,
> +		      <0xd8000 0x0140>;
> +		clocks = <&core_clk 1>, <&core_clk 3>;
> +		clock-names = "cpu", "ddr";
> +		interrupt-parent = <&pmu_intc>;
> +		interrupts = <0>;
> +	};

thx,

Jason.

Re: [RFC PATCH v5 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Andrew Lunn]

> > +Dove cpufreq driver
> > +-------------------
> > +
> > +The Dove cpufreq driver makes use of the pmu node in the device tree.
> 
> I would prefer to reword this to make it OS-agnostic.  The cpufreq
> driver is Linux-specific.

Hi Jason

Why is it Linux specific? Why for example would the FreeBSD cpu freq
driver not be able to use this?

> > +
> > +Required properties:
> > +- compatibility: Must be "marvell,dove-pmu"
> > +- interrupts:	 Interrupt to know the completion of cpu frequency change.
> > +- clocks:	 phandles and clock specifiers pairs for CPU and DDR clock
> > +- clock-names: 	 "cpu" and "ddr"
> > +- reg: 		 Address ranges of the PMU. Two ranges are expected.
> 
> Shall we define the two ranges?

Yes, i will extend this, once Mark agrees to the basic idea.

Thanks
	Andrew

Re: [RFC PATCH v5 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Jason Cooper]

Andrew,

On Sat, Dec 21, 2013 at 05:26:45PM +0100, Andrew Lunn wrote:
> > > +Dove cpufreq driver
> > > +-------------------
> > > +
> > > +The Dove cpufreq driver makes use of the pmu node in the device tree.
> > 
> > I would prefer to reword this to make it OS-agnostic.  The cpufreq
> > driver is Linux-specific.
> 
> Why is it Linux specific? 

I'll admit I may be bike-shedding here, and I'm more seeking
opinion/clarification than mandating anything.  The above sentence,
imho, is describing how Linux/FreeBSD uses the node, not describing the
hardware.

thx,

Jason.

Re: [RFC PATCH v5 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Andrew Lunn]

On Sat, Dec 21, 2013 at 02:25:01PM -0500, Jason Cooper wrote:
> Andrew,
> 
> On Sat, Dec 21, 2013 at 05:26:45PM +0100, Andrew Lunn wrote:
> > > > +Dove cpufreq driver
> > > > +-------------------
> > > > +
> > > > +The Dove cpufreq driver makes use of the pmu node in the device tree.
> > > 
> > > I would prefer to reword this to make it OS-agnostic.  The cpufreq
> > > driver is Linux-specific.
> > 
> > Why is it Linux specific? 
> 
> I'll admit I may be bike-shedding here, and I'm more seeking
> opinion/clarification than mandating anything.  The above sentence,
> imho, is describing how Linux/FreeBSD uses the node, not describing the
> hardware.

How about:

Dove has a PMU, which contains DFS, DVS, RTC, clock gates, and thermal
management,

	Andrew

Re: [RFC PATCH v5 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Jason Cooper]

On Sat, Dec 21, 2013 at 10:54:24PM +0100, Andrew Lunn wrote:
> On Sat, Dec 21, 2013 at 02:25:01PM -0500, Jason Cooper wrote:
> > Andrew,
> > 
> > On Sat, Dec 21, 2013 at 05:26:45PM +0100, Andrew Lunn wrote:
> > > > > +Dove cpufreq driver
> > > > > +-------------------
> > > > > +
> > > > > +The Dove cpufreq driver makes use of the pmu node in the device tree.
> > > > 
> > > > I would prefer to reword this to make it OS-agnostic.  The cpufreq
> > > > driver is Linux-specific.
> > > 
> > > Why is it Linux specific? 
> > 
> > I'll admit I may be bike-shedding here, and I'm more seeking
> > opinion/clarification than mandating anything.  The above sentence,
> > imho, is describing how Linux/FreeBSD uses the node, not describing the
> > hardware.
> 
> How about:
> 
> Dove has a PMU, which contains DFS, DVS, RTC, clock gates, and thermal
> management,


Yep, much better.

thx,

Jason.

[RFC PATCH v5 2/4] mvebu: Dove: Indicate the architecture supports cpufreq.

[Andrew Lunn]

Enable ARCH_HAS_CPUFREQ so allowing the cpufreq framework to be
enabled.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
 arch/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index c1f1a7eee953..908b02f8d901 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -500,6 +500,7 @@ config ARCH_IXP4XX
 
 config ARCH_DOVE
 	bool "Marvell Dove"
+	select ARCH_HAS_CPUFREQ
 	select ARCH_REQUIRE_GPIOLIB
 	select CPU_PJ4
 	select GENERIC_CLOCKEVENTS
-- 
1.8.5.1

[RFC PATCH v5 3/4] mvebu: Dove: Enable cpufreq driver in defconfig

[Andrew Lunn]

Add cpufreq to dove_defconfig, so it is built by default.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
 arch/arm/configs/dove_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/dove_defconfig b/arch/arm/configs/dove_defconfig
index 110105476848..3d9235017e89 100644
--- a/arch/arm/configs/dove_defconfig
+++ b/arch/arm/configs/dove_defconfig
@@ -22,6 +22,8 @@ CONFIG_ZBOOT_ROM_TEXT=0x0
 CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_ARM_APPENDED_DTB=y
 CONFIG_ARM_ATAG_DTB_COMPAT=y
+CONFIG_CPU_FREQ=y
+CONFIG_CPU_FREQ_GOV_ONDEMAND=y
 CONFIG_VFP=y
 CONFIG_NET=y
 CONFIG_PACKET=y
-- 
1.8.5.1

[RFC PATCH v5 4/4] mvebu: Dove: Add PMU node for cpufreq driver

[Andrew Lunn]

The cpufreq driver uses the PMU node in the device tree, since it is
part of the PMU device. The dove-cpufreq driver needs access to the
DDR and CPU clock, which are the two clocks the CPU can use. There is
also an interrupt generated when the DFS hardware completes a change
of frequencey. Add these to the DT.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
 arch/arm/boot/dts/dove.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 4c8028513133..42b9810e96c1 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -136,6 +136,16 @@
 				marvell,#interrupts = <5>;
 			};
 
+			pmu: pmu@d0000 {
+				compatible = "marvell,dove-pmu";
+				reg = <0xd0000 0x0700>,
+				      <0xd8000 0x0140>;
+				clocks = <&core_clk 1>, <&core_clk 3>;
+				clock-names = "cpu", "ddr";
+				interrupt-parent = <&pmu_intc>;
+				interrupts = <0>;
+			};
+
 			pmu_intc: pmu-interrupt-ctrl@d0050 {
 				compatible = "marvell,dove-pmu-intc";
 				interrupt-controller;
-- 
1.8.5.1

Re: [PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Viresh Kumar]

On 4 December 2013 19:47, Andrew Lunn <andrew@lunn.ch> wrote:
> The Marvell Dove SoC can run the CPU at two frequencies. The high
> frequencey is from a PLL, while the lower is the same as the DDR
> clock. Add a cpufreq driver to swap between these frequences.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

I was sure about it and checked it again. I haven't Acked this patch
until now. :)

> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index ce52ed949249..af7c4947f218 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -8,6 +8,11 @@ config ARM_BIG_LITTLE_CPUFREQ
>         help
>           This enables the Generic CPUfreq driver for ARM big.LITTLE platforms.
>
> +config ARM_DOVE_CPUFREQ
> +       def_bool ARCH_DOVE && OF
> +       help
> +         This adds the CPUFreq driver for Marvell Dove SoCs.

Please add this one after DT_BL one.. I know you wanted to keep
them in alphabetical order (probably we need to rename DT_BL),
but keeping both BIG LITTLE options makes more sense.

I will do the renaming later.

>  config ARM_DT_BL_CPUFREQ
>         tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
>         depends on ARM_BIG_LITTLE_CPUFREQ && OF

> diff --git a/drivers/cpufreq/dove-cpufreq.c b/drivers/cpufreq/dove-cpufreq.c
> +static int dove_cpufreq_probe(struct platform_device *pdev)
> +{
> +       struct device *cpu_dev;
> +       struct resource *res;
> +       int err, irq;
> +
> +       memset(&priv, 0, sizeof(priv));
> +       priv.dev = &pdev->dev;
> +
> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +                                          "cpufreq: DFS");
> +       priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(priv.dfs))
> +               return PTR_ERR(priv.dfs);
> +
> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +                                          "cpufreq: PMU CR");
> +       priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(priv.pmu_cr))
> +               return PTR_ERR(priv.pmu_cr);
> +
> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +                                          "cpufreq: PMU Clk Div");
> +       priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(priv.pmu_clk_div))
> +               return PTR_ERR(priv.pmu_clk_div);
> +
> +       cpu_dev = get_cpu_device(0);
> +
> +       priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk");
> +       if (IS_ERR(priv.cpu_clk)) {
> +               err = PTR_ERR(priv.cpu_clk);
> +               goto out;
> +       }

You are doing clk_disable_unprepare in out, and you haven't enabled
them yet. So, just return error from here.

> +       err = clk_prepare_enable(priv.cpu_clk);
> +       if (err)
> +               goto out;

same here

> +       dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
> +
> +       priv.ddr_clk = devm_clk_get(cpu_dev, "ddrclk");
> +       if (IS_ERR(priv.ddr_clk)) {
> +               err = PTR_ERR(priv.ddr_clk);
> +               goto out;

You need to do unprepare only for cpu clk here.

> +       }
> +
> +       err = clk_prepare_enable(priv.ddr_clk);
> +       if (err)
> +               goto out;

same here

> +
> +       dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;

Because you are using 0 and 1 in index for CPU and DDR freq at multiple
places, probably makes sense to make Macros for them ?

> +
> +       irq = irq_of_parse_and_map(cpu_dev->of_node, 0);
> +       if (!irq) {
> +               err = -ENXIO;
> +               goto out;
> +       }
> +
> +       err = devm_request_irq(&pdev->dev, irq, dove_cpufreq_irq,
> +                              0, "dove-cpufreq", NULL);
> +       if (err) {
> +               dev_err(&pdev->dev, "cannot assign irq %d, %d\n", irq, err);
> +               goto out;
> +       }
> +
> +       /* Read the target ratio which should be the DDR ratio */
> +       priv.dpratio = readl_relaxed(priv.pmu_clk_div);
> +       priv.dpratio = (priv.dpratio & DPRATIO_MASK) >> DPRATIO_OFFS;
> +       priv.dpratio = priv.dpratio << L2_RATIO_OFFS;
> +
> +       /* Save L2 ratio at reset */
> +       priv.xpratio = readl(priv.pmu_clk_div);
> +       priv.xpratio = (priv.xpratio & XPRATIO_MASK) >> XPRATIO_OFFS;
> +       priv.xpratio = priv.xpratio << L2_RATIO_OFFS;
> +
> +       err = cpufreq_register_driver(&dove_cpufreq_driver);
> +       if (!err)
> +               return 0;
> +
> +       dev_err(priv.dev, "Failed to register cpufreq driver");
> +
> +out:
> +       clk_disable_unprepare(priv.ddr_clk);
> +       clk_disable_unprepare(priv.cpu_clk);
> +
> +       return err;
> +}
> +
> +static int dove_cpufreq_remove(struct platform_device *pdev)
> +{
> +       cpufreq_unregister_driver(&dove_cpufreq_driver);
> +
> +       clk_disable_unprepare(priv.ddr_clk);
> +       clk_disable_unprepare(priv.cpu_clk);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver dove_cpufreq_platform_driver = {
> +       .probe = dove_cpufreq_probe,
> +       .remove = dove_cpufreq_remove,
> +       .driver = {
> +               .name = "dove-cpufreq",
> +               .owner = THIS_MODULE,
> +       },
> +};
> +
> +module_platform_driver(dove_cpufreq_platform_driver);

Otherwise looks fine :)

Re: [PATCH v4 1/4] cpufreq: Add a cpufreq driver for Marvell Dove

[Andrew Lunn]

> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index ce52ed949249..af7c4947f218 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -8,6 +8,11 @@ config ARM_BIG_LITTLE_CPUFREQ
> >         help
> >           This enables the Generic CPUfreq driver for ARM big.LITTLE platforms.
> >
> > +config ARM_DOVE_CPUFREQ
> > +       def_bool ARCH_DOVE && OF
> > +       help
> > +         This adds the CPUFreq driver for Marvell Dove SoCs.
> 
> Please add this one after DT_BL one.. I know you wanted to keep
> them in alphabetical order (probably we need to rename DT_BL),
> but keeping both BIG LITTLE options makes more sense.
> 
> I will do the renaming later.

O.K, once the binding is agreed, i will make this change.
 
> >  config ARM_DT_BL_CPUFREQ
> >         tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
> >         depends on ARM_BIG_LITTLE_CPUFREQ && OF
> 
> > diff --git a/drivers/cpufreq/dove-cpufreq.c b/drivers/cpufreq/dove-cpufreq.c
> > +static int dove_cpufreq_probe(struct platform_device *pdev)
> > +{
> > +       struct device *cpu_dev;
> > +       struct resource *res;
> > +       int err, irq;
> > +
> > +       memset(&priv, 0, sizeof(priv));
> > +       priv.dev = &pdev->dev;
> > +
> > +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +                                          "cpufreq: DFS");
> > +       priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(priv.dfs))
> > +               return PTR_ERR(priv.dfs);
> > +
> > +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +                                          "cpufreq: PMU CR");
> > +       priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(priv.pmu_cr))
> > +               return PTR_ERR(priv.pmu_cr);
> > +
> > +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +                                          "cpufreq: PMU Clk Div");
> > +       priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(priv.pmu_clk_div))
> > +               return PTR_ERR(priv.pmu_clk_div);
> > +
> > +       cpu_dev = get_cpu_device(0);
> > +
> > +       priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk");
> > +       if (IS_ERR(priv.cpu_clk)) {
> > +               err = PTR_ERR(priv.cpu_clk);
> > +               goto out;
> > +       }
> 
> You are doing clk_disable_unprepare in out, and you haven't enabled
> them yet. So, just return error from here.

You are missing part of the clk API, which makes this work, and keeps
the code simple. NULL is a valid clock, and enable/disable and
prepare/unprepare are NOP with a NULL clock. So the code after out:
should mostly do the right thing. However what i do have wrong is not
checking for IS_ERR() before clk_disable_unprepare(). I will fix that.

> > +
> > +       dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
> 
> Because you are using 0 and 1 in index for CPU and DDR freq at multiple
> places, probably makes sense to make Macros for them ?

O.K.
 
Thanks for the review,

       Andrew

[PATCH v4 2/4] mvebu: Dove: Instantiate cpufreq driver.

[Andrew Lunn]

Add a platform driver definition to instantiate the dove cpufreq
driver. Also indicate the ARCH has cpufreq support, so allowing the
cpufreq framework to be enabled.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Make resource names consistent.
Fix length of ranges
---
 arch/arm/Kconfig                       |  1 +
 arch/arm/mach-dove/board-dt.c          |  3 +++
 arch/arm/mach-dove/common.c            | 36 ++++++++++++++++++++++++++++++++++
 arch/arm/mach-dove/common.h            |  1 +
 arch/arm/mach-dove/include/mach/dove.h |  1 +
 5 files changed, 42 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index c1f1a7eee953..908b02f8d901 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -500,6 +500,7 @@ config ARCH_IXP4XX
 
 config ARCH_DOVE
 	bool "Marvell Dove"
+	select ARCH_HAS_CPUFREQ
 	select ARCH_REQUIRE_GPIOLIB
 	select CPU_PJ4
 	select GENERIC_CLOCKEVENTS
diff --git a/arch/arm/mach-dove/board-dt.c b/arch/arm/mach-dove/board-dt.c
index 49fa9abd09da..845e8b328432 100644
--- a/arch/arm/mach-dove/board-dt.c
+++ b/arch/arm/mach-dove/board-dt.c
@@ -27,6 +27,9 @@ static void __init dove_dt_init(void)
 	tauros2_init(0);
 #endif
 	BUG_ON(mvebu_mbus_dt_init());
+
+	dove_cpufreq_init();
+
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index c122bcff9f7c..323fcd4b27b6 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -344,6 +344,42 @@ void __init dove_sdio1_init(void)
 	platform_device_register(&dove_sdio1);
 }
 
+/*****************************************************************************
+ * CPU Frequency
+ ****************************************************************************/
+static struct resource dove_cpufreq_resources[] = {
+	[0] = {
+		.start  = DOVE_PMU_PHYS_BASE,
+		.end    = DOVE_PMU_PHYS_BASE + 0x7,
+		.flags  = IORESOURCE_MEM,
+		.name   = "cpufreq: DFS"
+	},
+	[1] = {
+		.start  = DOVE_PMU_PHYS_BASE + 0x8000,
+		.end    = DOVE_PMU_PHYS_BASE + 0x8003,
+		.flags  = IORESOURCE_MEM,
+		.name   = "cpufreq: PMU CR"
+	},
+	[2] = {
+		.start  = DOVE_PMU_PHYS_BASE + 0x0044,
+		.end    = DOVE_PMU_PHYS_BASE + 0x0047,
+		.flags  = IORESOURCE_MEM,
+		.name   = "cpufreq: PMU Clk Div"
+	},
+};
+
+static struct platform_device dove_cpufreq_device = {
+	.name		= "dove-cpufreq",
+	.id		= -1,
+	.num_resources	= ARRAY_SIZE(dove_cpufreq_resources),
+	.resource	= dove_cpufreq_resources,
+};
+
+void __init dove_cpufreq_init(void)
+{
+	platform_device_register(&dove_cpufreq_device);
+}
+
 void __init dove_setup_cpu_wins(void)
 {
 	/*
diff --git a/arch/arm/mach-dove/common.h b/arch/arm/mach-dove/common.h
index 1d725224d146..5c9a77bdd442 100644
--- a/arch/arm/mach-dove/common.h
+++ b/arch/arm/mach-dove/common.h
@@ -44,6 +44,7 @@ void dove_spi1_init(void);
 void dove_i2c_init(void);
 void dove_sdio0_init(void);
 void dove_sdio1_init(void);
+void dove_cpufreq_init(void);
 void dove_restart(enum reboot_mode, const char *);
 
 #endif
diff --git a/arch/arm/mach-dove/include/mach/dove.h b/arch/arm/mach-dove/include/mach/dove.h
index 0c4b35f4ee5b..48db186ae6bc 100644
--- a/arch/arm/mach-dove/include/mach/dove.h
+++ b/arch/arm/mach-dove/include/mach/dove.h
@@ -144,6 +144,7 @@
 #define  DOVE_SD0_GPIO_SEL		(1 << 0)
 
 /* Power Management */
+#define DOVE_PMU_PHYS_BASE	(DOVE_SB_REGS_PHYS_BASE + 0xd0000)
 #define DOVE_PMU_VIRT_BASE	(DOVE_SB_REGS_VIRT_BASE + 0xd0000)
 #define DOVE_PMU_SIG_CTRL	(DOVE_PMU_VIRT_BASE + 0x802c)
 
-- 
1.8.5

Re: [PATCH v4 2/4] mvebu: Dove: Instantiate cpufreq driver.

[Mark Rutland]

On Wed, Dec 04, 2013 at 02:17:19PM +0000, Andrew Lunn wrote:
> Add a platform driver definition to instantiate the dove cpufreq
> driver. Also indicate the ARCH has cpufreq support, so allowing the
> cpufreq framework to be enabled.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Make resource names consistent.
> Fix length of ranges
> ---
>  arch/arm/Kconfig                       |  1 +
>  arch/arm/mach-dove/board-dt.c          |  3 +++
>  arch/arm/mach-dove/common.c            | 36 ++++++++++++++++++++++++++++++++++
>  arch/arm/mach-dove/common.h            |  1 +
>  arch/arm/mach-dove/include/mach/dove.h |  1 +
>  5 files changed, 42 insertions(+)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index c1f1a7eee953..908b02f8d901 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -500,6 +500,7 @@ config ARCH_IXP4XX
>  
>  config ARCH_DOVE
>  	bool "Marvell Dove"
> +	select ARCH_HAS_CPUFREQ
>  	select ARCH_REQUIRE_GPIOLIB
>  	select CPU_PJ4
>  	select GENERIC_CLOCKEVENTS
> diff --git a/arch/arm/mach-dove/board-dt.c b/arch/arm/mach-dove/board-dt.c
> index 49fa9abd09da..845e8b328432 100644
> --- a/arch/arm/mach-dove/board-dt.c
> +++ b/arch/arm/mach-dove/board-dt.c
> @@ -27,6 +27,9 @@ static void __init dove_dt_init(void)
>  	tauros2_init(0);
>  #endif
>  	BUG_ON(mvebu_mbus_dt_init());
> +
> +	dove_cpufreq_init();
> +
>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>  }
>  
> diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
> index c122bcff9f7c..323fcd4b27b6 100644
> --- a/arch/arm/mach-dove/common.c
> +++ b/arch/arm/mach-dove/common.c
> @@ -344,6 +344,42 @@ void __init dove_sdio1_init(void)
>  	platform_device_register(&dove_sdio1);
>  }
>  
> +/*****************************************************************************
> + * CPU Frequency
> + ****************************************************************************/
> +static struct resource dove_cpufreq_resources[] = {
> +	[0] = {
> +		.start  = DOVE_PMU_PHYS_BASE,
> +		.end    = DOVE_PMU_PHYS_BASE + 0x7,
> +		.flags  = IORESOURCE_MEM,
> +		.name   = "cpufreq: DFS"
> +	},
> +	[1] = {
> +		.start  = DOVE_PMU_PHYS_BASE + 0x8000,
> +		.end    = DOVE_PMU_PHYS_BASE + 0x8003,
> +		.flags  = IORESOURCE_MEM,
> +		.name   = "cpufreq: PMU CR"
> +	},
> +	[2] = {
> +		.start  = DOVE_PMU_PHYS_BASE + 0x0044,
> +		.end    = DOVE_PMU_PHYS_BASE + 0x0047,
> +		.flags  = IORESOURCE_MEM,
> +		.name   = "cpufreq: PMU Clk Div"
> +	},
> +};
> +
> +static struct platform_device dove_cpufreq_device = {
> +	.name		= "dove-cpufreq",
> +	.id		= -1,
> +	.num_resources	= ARRAY_SIZE(dove_cpufreq_resources),
> +	.resource	= dove_cpufreq_resources,
> +};
> +
> +void __init dove_cpufreq_init(void)
> +{
> +	platform_device_register(&dove_cpufreq_device);
> +}

Please describe your power management unit in the DT. Your hardcoding
physical addresses that can easily be described in the DT, and you don't
appear to have an early probing requirement.

> +
>  void __init dove_setup_cpu_wins(void)
>  {
>  	/*
> diff --git a/arch/arm/mach-dove/common.h b/arch/arm/mach-dove/common.h
> index 1d725224d146..5c9a77bdd442 100644
> --- a/arch/arm/mach-dove/common.h
> +++ b/arch/arm/mach-dove/common.h
> @@ -44,6 +44,7 @@ void dove_spi1_init(void);
>  void dove_i2c_init(void);
>  void dove_sdio0_init(void);
>  void dove_sdio1_init(void);
> +void dove_cpufreq_init(void);
>  void dove_restart(enum reboot_mode, const char *);
>  
>  #endif
> diff --git a/arch/arm/mach-dove/include/mach/dove.h b/arch/arm/mach-dove/include/mach/dove.h
> index 0c4b35f4ee5b..48db186ae6bc 100644
> --- a/arch/arm/mach-dove/include/mach/dove.h
> +++ b/arch/arm/mach-dove/include/mach/dove.h
> @@ -144,6 +144,7 @@
>  #define  DOVE_SD0_GPIO_SEL		(1 << 0)
>  
>  /* Power Management */
> +#define DOVE_PMU_PHYS_BASE	(DOVE_SB_REGS_PHYS_BASE + 0xd0000)
>  #define DOVE_PMU_VIRT_BASE	(DOVE_SB_REGS_VIRT_BASE + 0xd0000)
>  #define DOVE_PMU_SIG_CTRL	(DOVE_PMU_VIRT_BASE + 0x802c)

If you need to describe the large block this appears to be in, do that
too.

Thanks,
Mark.

Re: [PATCH v4 2/4] mvebu: Dove: Instantiate cpufreq driver.

[Andrew Lunn]

On Wed, Dec 04, 2013 at 02:36:28PM +0000, Mark Rutland wrote:
> On Wed, Dec 04, 2013 at 02:17:19PM +0000, Andrew Lunn wrote:
> > Add a platform driver definition to instantiate the dove cpufreq
> > driver. Also indicate the ARCH has cpufreq support, so allowing the
> > cpufreq framework to be enabled.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > ---
> > Make resource names consistent.
> > Fix length of ranges
> > ---
> >  arch/arm/Kconfig                       |  1 +
> >  arch/arm/mach-dove/board-dt.c          |  3 +++
> >  arch/arm/mach-dove/common.c            | 36 ++++++++++++++++++++++++++++++++++
> >  arch/arm/mach-dove/common.h            |  1 +
> >  arch/arm/mach-dove/include/mach/dove.h |  1 +
> >  5 files changed, 42 insertions(+)
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index c1f1a7eee953..908b02f8d901 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -500,6 +500,7 @@ config ARCH_IXP4XX
> >  
> >  config ARCH_DOVE
> >  	bool "Marvell Dove"
> > +	select ARCH_HAS_CPUFREQ
> >  	select ARCH_REQUIRE_GPIOLIB
> >  	select CPU_PJ4
> >  	select GENERIC_CLOCKEVENTS
> > diff --git a/arch/arm/mach-dove/board-dt.c b/arch/arm/mach-dove/board-dt.c
> > index 49fa9abd09da..845e8b328432 100644
> > --- a/arch/arm/mach-dove/board-dt.c
> > +++ b/arch/arm/mach-dove/board-dt.c
> > @@ -27,6 +27,9 @@ static void __init dove_dt_init(void)
> >  	tauros2_init(0);
> >  #endif
> >  	BUG_ON(mvebu_mbus_dt_init());
> > +
> > +	dove_cpufreq_init();
> > +
> >  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> >  }
> >  
> > diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
> > index c122bcff9f7c..323fcd4b27b6 100644
> > --- a/arch/arm/mach-dove/common.c
> > +++ b/arch/arm/mach-dove/common.c
> > @@ -344,6 +344,42 @@ void __init dove_sdio1_init(void)
> >  	platform_device_register(&dove_sdio1);
> >  }
> >  
> > +/*****************************************************************************
> > + * CPU Frequency
> > + ****************************************************************************/
> > +static struct resource dove_cpufreq_resources[] = {
> > +	[0] = {
> > +		.start  = DOVE_PMU_PHYS_BASE,
> > +		.end    = DOVE_PMU_PHYS_BASE + 0x7,
> > +		.flags  = IORESOURCE_MEM,
> > +		.name   = "cpufreq: DFS"
> > +	},
> > +	[1] = {
> > +		.start  = DOVE_PMU_PHYS_BASE + 0x8000,
> > +		.end    = DOVE_PMU_PHYS_BASE + 0x8003,
> > +		.flags  = IORESOURCE_MEM,
> > +		.name   = "cpufreq: PMU CR"
> > +	},
> > +	[2] = {
> > +		.start  = DOVE_PMU_PHYS_BASE + 0x0044,
> > +		.end    = DOVE_PMU_PHYS_BASE + 0x0047,
> > +		.flags  = IORESOURCE_MEM,
> > +		.name   = "cpufreq: PMU Clk Div"
> > +	},
> > +};
> > +
> > +static struct platform_device dove_cpufreq_device = {
> > +	.name		= "dove-cpufreq",
> > +	.id		= -1,
> > +	.num_resources	= ARRAY_SIZE(dove_cpufreq_resources),
> > +	.resource	= dove_cpufreq_resources,
> > +};
> > +
> > +void __init dove_cpufreq_init(void)
> > +{
> > +	platform_device_register(&dove_cpufreq_device);
> > +}
> 
> Please describe your power management unit in the DT. Your hardcoding
> physical addresses that can easily be described in the DT, and you don't
> appear to have an early probing requirement.

Hi Mark

We have been here before, with the kirkwood cpufreq driver. My v1 of
that driver had the cpufreq driver fully described in DT. My binding
got shot down and i was told that pseudo devices, like cpufreq,
cpuidle, should not be in DT, and a platform driver was the way to go.
So that is how kirkwood cpufreq works and dove follows that.

Do you want to reverse that decision now?

Thanks
	Andrew

Re: [PATCH v4 2/4] mvebu: Dove: Instantiate cpufreq driver.

[Mark Rutland]

On Wed, Dec 04, 2013 at 03:02:28PM +0000, Andrew Lunn wrote:
> On Wed, Dec 04, 2013 at 02:36:28PM +0000, Mark Rutland wrote:
> > On Wed, Dec 04, 2013 at 02:17:19PM +0000, Andrew Lunn wrote:
> > > Add a platform driver definition to instantiate the dove cpufreq
> > > driver. Also indicate the ARCH has cpufreq support, so allowing the
> > > cpufreq framework to be enabled.
> > > 
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > > ---
> > > Make resource names consistent.
> > > Fix length of ranges
> > > ---
> > >  arch/arm/Kconfig                       |  1 +
> > >  arch/arm/mach-dove/board-dt.c          |  3 +++
> > >  arch/arm/mach-dove/common.c            | 36 ++++++++++++++++++++++++++++++++++
> > >  arch/arm/mach-dove/common.h            |  1 +
> > >  arch/arm/mach-dove/include/mach/dove.h |  1 +
> > >  5 files changed, 42 insertions(+)
> > > 
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > index c1f1a7eee953..908b02f8d901 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -500,6 +500,7 @@ config ARCH_IXP4XX
> > >  
> > >  config ARCH_DOVE
> > >  	bool "Marvell Dove"
> > > +	select ARCH_HAS_CPUFREQ
> > >  	select ARCH_REQUIRE_GPIOLIB
> > >  	select CPU_PJ4
> > >  	select GENERIC_CLOCKEVENTS
> > > diff --git a/arch/arm/mach-dove/board-dt.c b/arch/arm/mach-dove/board-dt.c
> > > index 49fa9abd09da..845e8b328432 100644
> > > --- a/arch/arm/mach-dove/board-dt.c
> > > +++ b/arch/arm/mach-dove/board-dt.c
> > > @@ -27,6 +27,9 @@ static void __init dove_dt_init(void)
> > >  	tauros2_init(0);
> > >  #endif
> > >  	BUG_ON(mvebu_mbus_dt_init());
> > > +
> > > +	dove_cpufreq_init();
> > > +
> > >  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> > >  }
> > >  
> > > diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
> > > index c122bcff9f7c..323fcd4b27b6 100644
> > > --- a/arch/arm/mach-dove/common.c
> > > +++ b/arch/arm/mach-dove/common.c
> > > @@ -344,6 +344,42 @@ void __init dove_sdio1_init(void)
> > >  	platform_device_register(&dove_sdio1);
> > >  }
> > >  
> > > +/*****************************************************************************
> > > + * CPU Frequency
> > > + ****************************************************************************/
> > > +static struct resource dove_cpufreq_resources[] = {
> > > +	[0] = {
> > > +		.start  = DOVE_PMU_PHYS_BASE,
> > > +		.end    = DOVE_PMU_PHYS_BASE + 0x7,
> > > +		.flags  = IORESOURCE_MEM,
> > > +		.name   = "cpufreq: DFS"
> > > +	},
> > > +	[1] = {
> > > +		.start  = DOVE_PMU_PHYS_BASE + 0x8000,
> > > +		.end    = DOVE_PMU_PHYS_BASE + 0x8003,
> > > +		.flags  = IORESOURCE_MEM,
> > > +		.name   = "cpufreq: PMU CR"
> > > +	},
> > > +	[2] = {
> > > +		.start  = DOVE_PMU_PHYS_BASE + 0x0044,
> > > +		.end    = DOVE_PMU_PHYS_BASE + 0x0047,
> > > +		.flags  = IORESOURCE_MEM,
> > > +		.name   = "cpufreq: PMU Clk Div"
> > > +	},
> > > +};
> > > +
> > > +static struct platform_device dove_cpufreq_device = {
> > > +	.name		= "dove-cpufreq",
> > > +	.id		= -1,
> > > +	.num_resources	= ARRAY_SIZE(dove_cpufreq_resources),
> > > +	.resource	= dove_cpufreq_resources,
> > > +};
> > > +
> > > +void __init dove_cpufreq_init(void)
> > > +{
> > > +	platform_device_register(&dove_cpufreq_device);
> > > +}
> > 
> > Please describe your power management unit in the DT. Your hardcoding
> > physical addresses that can easily be described in the DT, and you don't
> > appear to have an early probing requirement.
> 
> Hi Mark
> 
> We have been here before, with the kirkwood cpufreq driver. My v1 of
> that driver had the cpufreq driver fully described in DT. My binding
> got shot down and i was told that pseudo devices, like cpufreq,
> cpuidle, should not be in DT, and a platform driver was the way to go.
> So that is how kirkwood cpufreq works and dove follows that.
> 
> Do you want to reverse that decision now?

I'm not arguing for describing a pseudo-device. The power management
unit is a real device. If we used that to register a cpufreq driver, it
doesn't mean that we've registered a pseudo-device, but rather that we
made a decision to register the cpufreq portions based on the necessary
real devices being present.

This code registers a platform device that has PMU register fields. We
can easily describe the PMU in dt. The PMU _could_ be used by frameworks
other than cpufreq, so describing it as a cpufreq device is wrong.
Perhaps this was the problem with the proposed kirkwood binding?

Thanks,
Mark.

Re: [PATCH v4 2/4] mvebu: Dove: Instantiate cpufreq driver.

[Andrew Lunn]

> I'm not arguing for describing a pseudo-device. The power management
> unit is a real device. If we used that to register a cpufreq driver, it
> doesn't mean that we've registered a pseudo-device, but rather that we
> made a decision to register the cpufreq portions based on the necessary
> real devices being present.

Hi Mark

I need a more concrete explanation to understand what you mean.

Are you suggesting that i add a drivers/pmu/dove.c, which gets
instantiated by DT? This driver would then instantiate the cpufreq
driver? If so, it should also instantiate the gated clocks in
drivers/clk/mvebu/dove.c, since they are also part of the PMU,
according to the data sheet.  The RTC and the thermal management unit
are also part of the PMU. Should these drivers also be instantiated by
the PMU somehow?

> Perhaps this was the problem with the proposed kirkwood binding?

Kirkwood does not have a PMU, at least not according to the data
sheet. There are registers to control cpuidle, cpufreq, gating some
clocks, the exact same RTC and a somewhat similar thermal
sensor. However the data sheet makes no reference as to calling the
collection the "PMU".

   Thanks
      Andrew	   

Re: [PATCH v4 2/4] mvebu: Dove: Instantiate cpufreq driver.

[Jason Cooper]

On Thu, Dec 05, 2013 at 07:29:39PM +0100, Andrew Lunn wrote:
> > I'm not arguing for describing a pseudo-device. The power management
> > unit is a real device. If we used that to register a cpufreq driver, it
> > doesn't mean that we've registered a pseudo-device, but rather that we
> > made a decision to register the cpufreq portions based on the necessary
> > real devices being present.
> 
> Hi Mark
> 
> I need a more concrete explanation to understand what you mean.
> 
> Are you suggesting that i add a drivers/pmu/dove.c, which gets
> instantiated by DT? This driver would then instantiate the cpufreq
> driver? If so, it should also instantiate the gated clocks in
> drivers/clk/mvebu/dove.c, since they are also part of the PMU,
> according to the data sheet.  The RTC and the thermal management unit
> are also part of the PMU. Should these drivers also be instantiated by
> the PMU somehow?

No, I don't think that is where Mark is heading.  Separate the DT from
Linux.  Describe the PMU in DT.  It's then up to Linux to figure out how
to use (or not) that device.

I don't know if we've encountered this scenario yet, but is it possible
for Linux to have two drivers (cpufreq and cpuidle) answer to the same
compatible string?  Not that I'm advocating for that solution here...

The only problem I see with that is locking on any resources both
drivers use.  I think we had a similar issue with watchdog/rtc.

> > Perhaps this was the problem with the proposed kirkwood binding?
> 
> Kirkwood does not have a PMU, at least not according to the data
> sheet. There are registers to control cpuidle, cpufreq, gating some
> clocks, the exact same RTC and a somewhat similar thermal
> sensor. However the data sheet makes no reference as to calling the
> collection the "PMU".

If it quacks like a duck... :)

thx,

Jason.

[PATCH v4 3/4] mvebu: Dove: Enable cpufreq driver in defconfig

[Andrew Lunn]

Add cpufreq to dove_defconfig, so it is built by default.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
 arch/arm/configs/dove_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/dove_defconfig b/arch/arm/configs/dove_defconfig
index 110105476848..3d9235017e89 100644
--- a/arch/arm/configs/dove_defconfig
+++ b/arch/arm/configs/dove_defconfig
@@ -22,6 +22,8 @@ CONFIG_ZBOOT_ROM_TEXT=0x0
 CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_ARM_APPENDED_DTB=y
 CONFIG_ARM_ATAG_DTB_COMPAT=y
+CONFIG_CPU_FREQ=y
+CONFIG_CPU_FREQ_GOV_ONDEMAND=y
 CONFIG_VFP=y
 CONFIG_NET=y
 CONFIG_PACKET=y
-- 
1.8.5

[PATCH v4 4/4] mvebu: Dove: Add clocks and DFS interrupt to cpu node in DT.

[Andrew Lunn]

The dove-cpufreq driver needs access to the DDR and CPU clock.  There
is also an interrupt generated when the DFS hardware completes a
change of frequencey. Add these to the cpu node in DT.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
 arch/arm/boot/dts/dove.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
index 4c8028513133..3842ba02dddf 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -22,6 +22,10 @@
 			device_type = "cpu";
 			next-level-cache = <&l2>;
 			reg = <0>;
+			clocks = <&core_clk 1>, <&core_clk 3>;
+			clock-names = "cpu_clk", "ddrclk";
+			interrupt-parent = <&pmu_intc>;
+			interrupts = <0>;
 		};
 	};
 
-- 
1.8.5

Re: [PATCH v4 4/4] mvebu: Dove: Add clocks and DFS interrupt to cpu node in DT.

[Mark Rutland]

On Wed, Dec 04, 2013 at 02:17:21PM +0000, Andrew Lunn wrote:
> The dove-cpufreq driver needs access to the DDR and CPU clock.  There
> is also an interrupt generated when the DFS hardware completes a
> change of frequencey. Add these to the cpu node in DT.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
>  arch/arm/boot/dts/dove.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> index 4c8028513133..3842ba02dddf 100644
> --- a/arch/arm/boot/dts/dove.dtsi
> +++ b/arch/arm/boot/dts/dove.dtsi
> @@ -22,6 +22,10 @@
>  			device_type = "cpu";
>  			next-level-cache = <&l2>;
>  			reg = <0>;
> +			clocks = <&core_clk 1>, <&core_clk 3>;
> +			clock-names = "cpu_clk", "ddrclk";
> +			interrupt-parent = <&pmu_intc>;
> +			interrupts = <0>;

Is this interrupt actually generated by the CPU, or by the power
management unit?

Is ddrclk actually fed into the CPU? Given that you seem to have an
external L2 I don't see why the CPU would need the DDR clock.

Thanks,
Mark.

Re: [PATCH v4 4/4] mvebu: Dove: Add clocks and DFS interrupt to cpu node in DT.

[Mark Rutland]

On Wed, Dec 04, 2013 at 02:39:21PM +0000, Mark Rutland wrote:
> Is ddrclk actually fed into the CPU? Given that you seem to have an
> external L2 I don't see why the CPU would need the DDR clock.

I see from the other patches that it's simply a misleading name. Sorry
for the noise.

Mark.

Re: [PATCH v4 4/4] mvebu: Dove: Add clocks and DFS interrupt to cpu node in DT.

[Jason Cooper]

On Wed, Dec 04, 2013 at 03:17:21PM +0100, Andrew Lunn wrote:
> The dove-cpufreq driver needs access to the DDR and CPU clock.  There
> is also an interrupt generated when the DFS hardware completes a
> change of frequencey. Add these to the cpu node in DT.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
>  arch/arm/boot/dts/dove.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi
> index 4c8028513133..3842ba02dddf 100644
> --- a/arch/arm/boot/dts/dove.dtsi
> +++ b/arch/arm/boot/dts/dove.dtsi
> @@ -22,6 +22,10 @@
>  			device_type = "cpu";
>  			next-level-cache = <&l2>;
>  			reg = <0>;
> +			clocks = <&core_clk 1>, <&core_clk 3>;

This isn't a blocker by any means, but does anyone have on their TODO
list a DT include file of macros for the clocks? So this would become:

			clocks = <&core_clk DOVE_CPU_CLK>, <&core_clk DOVE_DDRCLK>;

or similar?

If not, would it be considered useful?

thx,

Jason.

> +			clock-names = "cpu_clk", "ddrclk";
> +			interrupt-parent = <&pmu_intc>;
> +			interrupts = <0>;
>  		};
>  	};
>  
> -- 
> 1.8.5
>