linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Dove cpufreq driver
@ 2013-10-19 12:37 Andrew Lunn
  2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
                   ` (3 more replies)
  0 siblings, 4 replies; 26+ messages in thread
From: Andrew Lunn @ 2013-10-19 12:37 UTC (permalink / raw)
  To: linux-arm-kernel

Marvell Dove can run its CPU at two different frequencies. Add a
cpufreq driver.

These patches are based on top of Rafael's for-next branch, since
there have been a lot of changes to the cpufreq core code.

However, this driver also has a run time dependency on the PMU
interrupt controller patches:

http://www.spinics.net/lists/devicetree/msg06547.html

If the PMU interrupt controller is not available, the cpufreq probe
function will fail, but the machine keeps running O.K.

Probably Jason will want to take the mvebu patches, and Rafeal or
Viresh for the cpufreq driver itself.

Once github comes back online, the following branch will have these patches
and the dependencies.

https://github.com/lunn/linux v3.12-rc5-rafael-next-dove-cpufreq

Andrew

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          |   2 +
 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            |   7 +
 drivers/cpufreq/Makefile               |   1 +
 drivers/cpufreq/dove-cpufreq.c         | 276 +++++++++++++++++++++++++++++++++
 10 files changed, 331 insertions(+)
 create mode 100644 drivers/cpufreq/dove-cpufreq.c

-- 
1.8.4.rc3

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-19 12:37 [PATCH 0/4] Dove cpufreq driver Andrew Lunn
@ 2013-10-19 12:37 ` Andrew Lunn
  2013-10-19 23:09   ` Luka Perkov
                     ` (3 more replies)
  2013-10-19 12:37 ` [PATCH 2/4] mvebu: Dove: Instantiate cpufreq driver Andrew Lunn
                   ` (2 subsequent siblings)
  3 siblings, 4 replies; 26+ messages in thread
From: Andrew Lunn @ 2013-10-19 12:37 UTC (permalink / raw)
  To: linux-arm-kernel

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>
---
 drivers/cpufreq/Kconfig.arm    |   7 ++
 drivers/cpufreq/Makefile       |   1 +
 drivers/cpufreq/dove-cpufreq.c | 276 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 284 insertions(+)
 create mode 100644 drivers/cpufreq/dove-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 701ec95..3d77633 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -8,6 +8,13 @@ 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
+	select CPU_FREQ_TABLE
+	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 b7948bb..5956661 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 0000000..4730b05
--- /dev/null
+++ b/drivers/cpufreq/dove-cpufreq.c
@@ -0,0 +1,276 @@
+/*
+ *	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.
+ */
+
+#define DEBUG
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/cpufreq.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/of_irq.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@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 void dove_cpufreq_set_cpu_state(struct cpufreq_policy *policy,
+				       unsigned int index)
+{
+	struct cpufreq_freqs freqs;
+	unsigned int state = dove_freq_table[index].driver_data;
+	unsigned long reg, cr;
+
+	freqs.old = dove_cpufreq_get_cpu_frequency(0);
+	freqs.new = dove_freq_table[index].frequency;
+
+	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
+
+	if (freqs.old != freqs.new) {
+		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();
+	}
+	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
+}
+
+static int dove_cpufreq_target(struct cpufreq_policy *policy,
+			    unsigned int target_freq,
+			    unsigned int relation)
+{
+	unsigned int index = 0;
+
+	if (cpufreq_frequency_table_target(policy, dove_freq_table,
+				target_freq, relation, &index))
+		return -EINVAL;
+
+	dove_cpufreq_set_cpu_state(policy, index);
+
+	return 0;
+}
+
+static int dove_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+	return cpufreq_generic_init(policy, dove_freq_table, 5000);
+}
+
+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 = 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_node *np;
+	struct resource *res;
+	int err;
+	int irq;
+
+	priv.dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv.dfs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv.dfs))
+		return PTR_ERR(priv.dfs);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	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(pdev, IORESOURCE_MEM, 2);
+	priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv.pmu_clk_div))
+		return PTR_ERR(priv.pmu_clk_div);
+
+	np = of_find_node_by_path("/cpus/cpu at 0");
+	if (!np)
+		return -ENODEV;
+
+	priv.cpu_clk = of_clk_get_by_name(np, "cpu_clk");
+	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);
+	dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
+
+	priv.ddr_clk = of_clk_get_by_name(np, "ddrclk");
+	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);
+	dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (!irq) {
+		dev_err(priv.dev, "irq_of_parse_and_map failed\n");
+		return 0;
+	}
+
+	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);
+		return err;
+	}
+
+	of_node_put(np);
+	np = NULL;
+
+	/* 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@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");
+
+	clk_disable_unprepare(priv.ddr_clk);
+out_cpu:
+	clk_disable_unprepare(priv.cpu_clk);
+	of_node_put(np);
+
+	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.4.rc3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/4] mvebu: Dove: Instantiate cpufreq driver.
  2013-10-19 12:37 [PATCH 0/4] Dove cpufreq driver Andrew Lunn
  2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
@ 2013-10-19 12:37 ` Andrew Lunn
  2013-10-21 10:47   ` Sebastian Hesselbarth
  2013-10-19 12:37 ` [PATCH 3/4] mvebu: Dove: Enable cpufreq driver in defconfig Andrew Lunn
  2013-10-19 12:37 ` [PATCH 4/4] mvebu: Dove: Add clocks and DFS interrupt to cpu node in DT Andrew Lunn
  3 siblings, 1 reply; 26+ messages in thread
From: Andrew Lunn @ 2013-10-19 12:37 UTC (permalink / raw)
  To: linux-arm-kernel

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>
---
 arch/arm/Kconfig                       |  1 +
 arch/arm/mach-dove/board-dt.c          |  2 ++
 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, 41 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 1ad6fb6..7744415 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -495,6 +495,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 49f72a8..98202de 100644
--- a/arch/arm/mach-dove/board-dt.c
+++ b/arch/arm/mach-dove/board-dt.c
@@ -70,6 +70,8 @@ static void __init dove_dt_init(void)
 	/* Setup clocks for legacy devices */
 	dove_legacy_clk_init();
 
+	dove_cpufreq_init();
+
 	/* Internal devices not ported to DT yet */
 	dove_pcie_init(1, 1);
 
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index c122bcf..9e648a8 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 + 0x8004,
+		.flags  = IORESOURCE_MEM,
+		.name   = "cpufreq PMU CR"
+	},
+	[2] = {
+		.start  = DOVE_PMU_PHYS_BASE + 0x0044,
+		.end    = DOVE_PMU_PHYS_BASE + 0x0048,
+		.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 1d72522..5c9a77b 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 0c4b35f..48db186 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.4.rc3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/4] mvebu: Dove: Enable cpufreq driver in defconfig
  2013-10-19 12:37 [PATCH 0/4] Dove cpufreq driver Andrew Lunn
  2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
  2013-10-19 12:37 ` [PATCH 2/4] mvebu: Dove: Instantiate cpufreq driver Andrew Lunn
@ 2013-10-19 12:37 ` Andrew Lunn
  2013-10-19 12:37 ` [PATCH 4/4] mvebu: Dove: Add clocks and DFS interrupt to cpu node in DT Andrew Lunn
  3 siblings, 0 replies; 26+ messages in thread
From: Andrew Lunn @ 2013-10-19 12:37 UTC (permalink / raw)
  To: linux-arm-kernel

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

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 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 1101054..3d92350 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.4.rc3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/4] mvebu: Dove: Add clocks and DFS interrupt to cpu node in DT.
  2013-10-19 12:37 [PATCH 0/4] Dove cpufreq driver Andrew Lunn
                   ` (2 preceding siblings ...)
  2013-10-19 12:37 ` [PATCH 3/4] mvebu: Dove: Enable cpufreq driver in defconfig Andrew Lunn
@ 2013-10-19 12:37 ` Andrew Lunn
  3 siblings, 0 replies; 26+ messages in thread
From: Andrew Lunn @ 2013-10-19 12:37 UTC (permalink / raw)
  To: linux-arm-kernel

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>
---
 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 1fd615d..e96d7a6 100644
--- a/arch/arm/boot/dts/dove.dtsi
+++ b/arch/arm/boot/dts/dove.dtsi
@@ -19,6 +19,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.4.rc3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
@ 2013-10-19 23:09   ` Luka Perkov
  2013-10-20  8:42     ` Andrew Lunn
  2013-10-21 10:42   ` Sebastian Hesselbarth
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 26+ messages in thread
From: Luka Perkov @ 2013-10-19 23:09 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Andrew,

On Sat, Oct 19, 2013 at 02:37:38PM +0200, 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>
> ---
>  drivers/cpufreq/Kconfig.arm    |   7 ++
>  drivers/cpufreq/Makefile       |   1 +
>  drivers/cpufreq/dove-cpufreq.c | 276 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 284 insertions(+)
>  create mode 100644 drivers/cpufreq/dove-cpufreq.c

...

> --- /dev/null
> +++ b/drivers/cpufreq/dove-cpufreq.c
> @@ -0,0 +1,276 @@
> +/*
> + *	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.
> + */
> +
> +#define DEBUG

It seems to me that this define is not used at all...

Luka

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-19 23:09   ` Luka Perkov
@ 2013-10-20  8:42     ` Andrew Lunn
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Lunn @ 2013-10-20  8:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Oct 20, 2013 at 01:09:08AM +0200, Luka Perkov wrote:
> Hi Andrew,
> 
> On Sat, Oct 19, 2013 at 02:37:38PM +0200, 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>
> > ---
> >  drivers/cpufreq/Kconfig.arm    |   7 ++
> >  drivers/cpufreq/Makefile       |   1 +
> >  drivers/cpufreq/dove-cpufreq.c | 276 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 284 insertions(+)
> >  create mode 100644 drivers/cpufreq/dove-cpufreq.c
> 
> ...
> 
> > --- /dev/null
> > +++ b/drivers/cpufreq/dove-cpufreq.c
> > @@ -0,0 +1,276 @@
> > +/*
> > + *	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.
> > + */
> > +
> > +#define DEBUG
> 
> It seems to me that this define is not used at all...

Hi Luke

Correct. Its left over from my debugging. I will collect more comments
and then remove it in a v2 patchset.

    Thanks
	Andrew

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
  2013-10-19 23:09   ` Luka Perkov
@ 2013-10-21 10:42   ` Sebastian Hesselbarth
  2013-10-21 15:42     ` Andrew Lunn
  2013-10-22  9:01   ` Viresh Kumar
  2013-10-22 10:19   ` Sudeep KarkadaNagesha
  3 siblings, 1 reply; 26+ messages in thread
From: Sebastian Hesselbarth @ 2013-10-21 10:42 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/19/2013 01:37 PM, 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>

Andrew,

thanks for adding Dove cpufreq! I have some remarks below.

> ---
>   drivers/cpufreq/Kconfig.arm    |   7 ++
>   drivers/cpufreq/Makefile       |   1 +
>   drivers/cpufreq/dove-cpufreq.c | 276 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 284 insertions(+)
>   create mode 100644 drivers/cpufreq/dove-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 701ec95..3d77633 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -8,6 +8,13 @@ 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
> +	select CPU_FREQ_TABLE
> +	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 b7948bb..5956661 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 0000000..4730b05
> --- /dev/null
> +++ b/drivers/cpufreq/dove-cpufreq.c
> @@ -0,0 +1,276 @@
> +/*
> + *	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.
> + */
> +
> +#define DEBUG
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/cpufreq.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/of_irq.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 void dove_cpufreq_set_cpu_state(struct cpufreq_policy *policy,
> +				       unsigned int index)
> +{
> +	struct cpufreq_freqs freqs;
> +	unsigned int state = dove_freq_table[index].driver_data;
> +	unsigned long reg, cr;
> +
> +	freqs.old = dove_cpufreq_get_cpu_frequency(0);
> +	freqs.new = dove_freq_table[index].frequency;
> +
> +	cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
> +
> +	if (freqs.old != freqs.new) {
> +		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);

Does slow mode really (only) depend on the value written into CPUL2CR?
Dove FS isn't that chatty about it and I cannot really test right now.
But reading it, I think there is two CPU PLL to {L2,DDR} ratios that you
can switch between by (de-)asserting CPU_SLOW_EN.

If so, I guess it would be better to set those ratios once in init and
take care of CPU_SLOW_EN here only.

> +			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();
> +	}
> +	cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
> +}
> +
> +static int dove_cpufreq_target(struct cpufreq_policy *policy,
> +			    unsigned int target_freq,
> +			    unsigned int relation)
> +{
> +	unsigned int index = 0;
> +
> +	if (cpufreq_frequency_table_target(policy, dove_freq_table,
> +				target_freq, relation, &index))
> +		return -EINVAL;
> +
> +	dove_cpufreq_set_cpu_state(policy, index);
> +
> +	return 0;
> +}
> +
> +static int dove_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> +	return cpufreq_generic_init(policy, dove_freq_table, 5000);
> +}
> +
> +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 = 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_node *np;
> +	struct resource *res;
> +	int err;
> +	int irq;
> +
> +	priv.dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv.dfs))
> +		return PTR_ERR(priv.dfs);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv.pmu_cr))

Cleanup path for most of the resources here and below is kind of
missing. Also, maybe give names to the different areas and not depend
on ordering?

Sebastian

> +		return PTR_ERR(priv.pmu_cr);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv.pmu_clk_div))
> +		return PTR_ERR(priv.pmu_clk_div);
> +
> +	np = of_find_node_by_path("/cpus/cpu at 0");
> +	if (!np)
> +		return -ENODEV;
> +
> +	priv.cpu_clk = of_clk_get_by_name(np, "cpu_clk");
> +	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);
> +	dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
> +
> +	priv.ddr_clk = of_clk_get_by_name(np, "ddrclk");
> +	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);
> +	dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
> +
> +	irq = irq_of_parse_and_map(np, 0);
> +	if (!irq) {
> +		dev_err(priv.dev, "irq_of_parse_and_map failed\n");
> +		return 0;
> +	}
> +
> +	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);
> +		return err;
> +	}
> +
> +	of_node_put(np);
> +	np = NULL;
> +
> +	/* 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");
> +
> +	clk_disable_unprepare(priv.ddr_clk);
> +out_cpu:
> +	clk_disable_unprepare(priv.cpu_clk);
> +	of_node_put(np);
> +
> +	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");
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 2/4] mvebu: Dove: Instantiate cpufreq driver.
  2013-10-19 12:37 ` [PATCH 2/4] mvebu: Dove: Instantiate cpufreq driver Andrew Lunn
@ 2013-10-21 10:47   ` Sebastian Hesselbarth
  2013-10-21 15:26     ` Andrew Lunn
  0 siblings, 1 reply; 26+ messages in thread
From: Sebastian Hesselbarth @ 2013-10-21 10:47 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/19/2013 01:37 PM, 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>
> ---
>   arch/arm/Kconfig                       |  1 +
>   arch/arm/mach-dove/board-dt.c          |  2 ++
>   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, 41 insertions(+)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 1ad6fb6..7744415 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -495,6 +495,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 49f72a8..98202de 100644
> --- a/arch/arm/mach-dove/board-dt.c
> +++ b/arch/arm/mach-dove/board-dt.c
> @@ -70,6 +70,8 @@ static void __init dove_dt_init(void)
>   	/* Setup clocks for legacy devices */
>   	dove_legacy_clk_init();
>
> +	dove_cpufreq_init();
> +

What ever the outcome of "DT: blessing or curse" discussion at ELCE will
be, are there any plans to probe the cpufreq/cpuidle drivers directly
from DT? Someday, we want to get rid of .machine_init.

>   	/* Internal devices not ported to DT yet */
>   	dove_pcie_init(1, 1);
>
> diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
> index c122bcf..9e648a8 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 + 0x8004,

BASE + 0x8004 - 1 ?

> +		.flags  = IORESOURCE_MEM,
> +		.name   = "cpufreq PMU CR"
> +	},
> +	[2] = {
> +		.start  = DOVE_PMU_PHYS_BASE + 0x0044,
> +		.end    = DOVE_PMU_PHYS_BASE + 0x0048,

ditto.

Sebastian

> +		.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 1d72522..5c9a77b 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 0c4b35f..48db186 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)
>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 2/4] mvebu: Dove: Instantiate cpufreq driver.
  2013-10-21 10:47   ` Sebastian Hesselbarth
@ 2013-10-21 15:26     ` Andrew Lunn
  0 siblings, 0 replies; 26+ messages in thread
From: Andrew Lunn @ 2013-10-21 15:26 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Oct 21, 2013 at 11:47:32AM +0100, Sebastian Hesselbarth wrote:
> On 10/19/2013 01:37 PM, 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>
> >---
> >  arch/arm/Kconfig                       |  1 +
> >  arch/arm/mach-dove/board-dt.c          |  2 ++
> >  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, 41 insertions(+)
> >
> >diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >index 1ad6fb6..7744415 100644
> >--- a/arch/arm/Kconfig
> >+++ b/arch/arm/Kconfig
> >@@ -495,6 +495,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 49f72a8..98202de 100644
> >--- a/arch/arm/mach-dove/board-dt.c
> >+++ b/arch/arm/mach-dove/board-dt.c
> >@@ -70,6 +70,8 @@ static void __init dove_dt_init(void)
> >  	/* Setup clocks for legacy devices */
> >  	dove_legacy_clk_init();
> >
> >+	dove_cpufreq_init();
> >+
> 
> What ever the outcome of "DT: blessing or curse" discussion at ELCE will
> be, are there any plans to probe the cpufreq/cpuidle drivers directly
> from DT? Someday, we want to get rid of .machine_init.

Hi Sebastian

This has already been discussed once for cpufreq drivers. I triggered
it with the kirkwood cpufreq driver. The outcome was that not
everything needs to the DT, platform drivers are still OK for things
which are not really devices. But as you say, the outcome from
Edinburgh might repeal that decision.

> >+	[1] = {
> >+		.start  = DOVE_PMU_PHYS_BASE + 0x8000,
> >+		.end    = DOVE_PMU_PHYS_BASE + 0x8004,
> 
> BASE + 0x8004 - 1 ?

Ah, yes, was thinking DT.

See you soon,

    Andrew

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-21 10:42   ` Sebastian Hesselbarth
@ 2013-10-21 15:42     ` Andrew Lunn
  2013-10-21 16:38       ` Sebastian Hesselbarth
  0 siblings, 1 reply; 26+ messages in thread
From: Andrew Lunn @ 2013-10-21 15:42 UTC (permalink / raw)
  To: linux-arm-kernel

> >+	if (freqs.old != freqs.new) {
> >+		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);
> 
> Does slow mode really (only) depend on the value written into CPUL2CR?
> Dove FS isn't that chatty about it and I cannot really test right now.
> But reading it, I think there is two CPU PLL to {L2,DDR} ratios that you
> can switch between by (de-)asserting CPU_SLOW_EN.

Hi Sebastian

The lack of details in the datasheet was very annoying. I spent many
evenings looking at a frozen up cubox. In the end i had to risk eye
cancer and look at the Marvell vendor code. It always sets these
values on each transition, and it even calculated one of the ratios
each time the frequency changed. I never actually tried setting them
once and then leaving them alone. Worth a try once i have the hardware
available.

> >+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+	priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> >+	if (IS_ERR(priv.dfs))
> >+		return PTR_ERR(priv.dfs);
> >+
> >+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >+	priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
> >+	if (IS_ERR(priv.pmu_cr))
> 
> Cleanup path for most of the resources here and below is kind of
> missing. 

Well, i'm using devm_ioremap_resources().  The cleanup for that should
happen automagically. And platform_get_resource() is just a lookup
function.

> Also, maybe give names to the different areas and not depend
> on ordering?

Actually, they already have names, but you probably have not go to
that patch yet. So swapping to platform_get_resource_byname() would be
simple.

Thanks
	Andrew

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-21 15:42     ` Andrew Lunn
@ 2013-10-21 16:38       ` Sebastian Hesselbarth
  0 siblings, 0 replies; 26+ messages in thread
From: Sebastian Hesselbarth @ 2013-10-21 16:38 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/21/2013 04:42 PM, Andrew Lunn wrote:
>>> +	if (freqs.old != freqs.new) {
>>> +		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);
>>
>> Does slow mode really (only) depend on the value written into CPUL2CR?
>> Dove FS isn't that chatty about it and I cannot really test right now.
>> But reading it, I think there is two CPU PLL to {L2,DDR} ratios that you
>> can switch between by (de-)asserting CPU_SLOW_EN.
>
> The lack of details in the datasheet was very annoying. I spent many
> evenings looking at a frozen up cubox. In the end i had to risk eye
> cancer and look at the Marvell vendor code. It always sets these
> values on each transition, and it even calculated one of the ratios
> each time the frequency changed. I never actually tried setting them
> once and then leaving them alone. Worth a try once i have the hardware
> available.

I actually just tried it and realized that both xpratio and dpratio are
equal (=4). And I wonder that not setting l2 ratio hangs it, while not
setting ddr ratio is fine. Anyway, without proper documentation IMHO
your approach is as fine as it can get. We can have a closer look at
it, when we both have time/tools/space available.

>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	priv.dfs = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(priv.dfs))
>>> +		return PTR_ERR(priv.dfs);
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> +	priv.pmu_cr = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(priv.pmu_cr))
>>
>> Cleanup path for most of the resources here and below is kind of
>> missing.
>
> Well, i'm using devm_ioremap_resources().  The cleanup for that should
> happen automagically. And platform_get_resource() is just a lookup
> function.

True. But e.g. on the clock errors, you do not drop node or clk. No
clk_disable_unprepare on irq failures or if cpufreq_register_driver
fails.

>> Also, maybe give names to the different areas and not depend
>> on ordering?
>
> Actually, they already have names, but you probably have not go to
> that patch yet. So swapping to platform_get_resource_byname() would be
> simple.

I did flip over the non-DT resources and now (finally) realized, that
there is no specific DT node (You told me at least twice before).

Currently, I don't see why cpufreq shouldn't get its own node as this
adds dependency to legacy code we are trying to get rid of. But as you
already said, let's wait for summit results on DT future.

BTW, have another look at the resource names and remove the ':' (or
add it to all).

See you soon,

Sebastian

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
  2013-10-19 23:09   ` Luka Perkov
  2013-10-21 10:42   ` Sebastian Hesselbarth
@ 2013-10-22  9:01   ` Viresh Kumar
  2013-10-22 15:57     ` Andrew Lunn
  2013-10-22 10:19   ` Sudeep KarkadaNagesha
  3 siblings, 1 reply; 26+ messages in thread
From: Viresh Kumar @ 2013-10-22  9:01 UTC (permalink / raw)
  To: linux-arm-kernel

On 19 October 2013 18:07, Andrew Lunn <andrew@lunn.ch> wrote:

> +config ARM_DOVE_CPUFREQ
> +       def_bool ARCH_DOVE && OF
> +       select CPU_FREQ_TABLE

Refer:
3bc28ab cpufreq: remove CONFIG_CPU_FREQ_TABLE

> +       help
> +         This adds the CPUFreq driver for Marvell Dove
> +         SoCs.

Above can come in one line?

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

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

why do you need this one?

> +#include <linux/cpufreq.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/of_irq.h>

Can you add them in ascending order please?

> +#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 */

Use space after #define instead of tabs. And then use tabs
consistently after Macro name to keep macro values aligned.

> +/* 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)

Here as well.. aligning with earlier macro's might look better.

> +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 void dove_cpufreq_set_cpu_state(struct cpufreq_policy *policy,
> +                                      unsigned int index)
> +{
> +       struct cpufreq_freqs freqs;
> +       unsigned int state = dove_freq_table[index].driver_data;
> +       unsigned long reg, cr;
> +
> +       freqs.old = dove_cpufreq_get_cpu_frequency(0);
> +       freqs.new = dove_freq_table[index].frequency;
> +
> +       cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
> +
> +       if (freqs.old != freqs.new) {

If this is false, why should we start sending notifications?

> +               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();
> +       }
> +       cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
> +}
> +
> +static int dove_cpufreq_target(struct cpufreq_policy *policy,
> +                           unsigned int target_freq,
> +                           unsigned int relation)

there are few patches floating in making target light weight.. This part will
change significantly ones those are in..

> +{
> +       unsigned int index = 0;
> +
> +       if (cpufreq_frequency_table_target(policy, dove_freq_table,
> +                               target_freq, relation, &index))
> +               return -EINVAL;
> +
> +       dove_cpufreq_set_cpu_state(policy, index);
> +
> +       return 0;
> +}
> +
> +static int dove_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> +       return cpufreq_generic_init(policy, dove_freq_table, 5000);
> +}
> +
> +static irqreturn_t dove_cpufreq_irq(int irq, void *dev)
> +{
> +       return IRQ_HANDLED;
> +}

what is this for?

> +static struct cpufreq_driver dove_cpufreq_driver = {
> +       .get    = dove_cpufreq_get_cpu_frequency,
> +       .verify = cpufreq_generic_frequency_table_verify,
> +       .target = 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_node *np;
> +       struct resource *res;
> +       int err;
> +       int irq;

Above two can be merged.

> +       priv.dev = &pdev->dev;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(priv.dfs))
> +               return PTR_ERR(priv.dfs);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       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(pdev, IORESOURCE_MEM, 2);
> +       priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(priv.pmu_clk_div))
> +               return PTR_ERR(priv.pmu_clk_div);
> +
> +       np = of_find_node_by_path("/cpus/cpu at 0");
> +       if (!np)
> +               return -ENODEV;
> +
> +       priv.cpu_clk = of_clk_get_by_name(np, "cpu_clk");
> +       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);

this can fail..

> +       dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
> +
> +       priv.ddr_clk = of_clk_get_by_name(np, "ddrclk");
> +       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);

and so can this..

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
                     ` (2 preceding siblings ...)
  2013-10-22  9:01   ` Viresh Kumar
@ 2013-10-22 10:19   ` Sudeep KarkadaNagesha
  3 siblings, 0 replies; 26+ messages in thread
From: Sudeep KarkadaNagesha @ 2013-10-22 10:19 UTC (permalink / raw)
  To: linux-arm-kernel

On 19/10/13 13:37, 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>
> ---
>  drivers/cpufreq/Kconfig.arm    |   7 ++
>  drivers/cpufreq/Makefile       |   1 +
>  drivers/cpufreq/dove-cpufreq.c | 276 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 284 insertions(+)
>  create mode 100644 drivers/cpufreq/dove-cpufreq.c
> 

[snip]

> +static int dove_cpufreq_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np;
> +	struct resource *res;
> +	int err;
> +	int irq;
> +
> +	priv.dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv.dfs))
> +		return PTR_ERR(priv.dfs);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	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(pdev, IORESOURCE_MEM, 2);
> +	priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(priv.pmu_clk_div))
> +		return PTR_ERR(priv.pmu_clk_div);
> +
> +	np = of_find_node_by_path("/cpus/cpu at 0");
> +	if (!np)
> +		return -ENODEV;
> +
You need not search for cpu nodes. When the cpu are registered, their of_node
gets initialised. So you can just use:
	np = of_cpu_device_node_get(0);

However I think its better to just get:
	cpu_dev = get_cpu_device(0);
as clk APIs can use cpu_dev

> +	priv.cpu_clk = of_clk_get_by_name(np, "cpu_clk");

Now this can be
	priv.cpu_clk = devm_clk_get(cpu_dev, "cpu_clk");

> +	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);
> +	dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;
> +
> +	priv.ddr_clk = of_clk_get_by_name(np, "ddrclk");

Similarly priv.ddr_clk = devm_clk_get(cpu_dev, "ddrclk");

> +	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);
> +	dove_freq_table[1].frequency = clk_get_rate(priv.ddr_clk) / 1000;
> +
> +	irq = irq_of_parse_and_map(np, 0);

Here you can use cpu_dev->of_node

Regards,
Sudeep

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-22  9:01   ` Viresh Kumar
@ 2013-10-22 15:57     ` Andrew Lunn
  2013-10-23  4:29       ` Viresh Kumar
  0 siblings, 1 reply; 26+ messages in thread
From: Andrew Lunn @ 2013-10-22 15:57 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Viresh

Thanks for your comments. I will include them in the next version.

> > +static irqreturn_t dove_cpufreq_irq(int irq, void *dev)
> > +{
> > +       return IRQ_HANDLED;
> > +}
> 
> what is this for?

The hardware raises an interrupt when the change is complete. This
interrupt is what brings the CPU out of the WFI. I don't know the
interrupt code too well, but i think it is necessary for the driver to
acknowledged it. The interrupt core code counts interrupts which
nobody acknowledges, and after 1000 are received, it disables the
interrupt. That would probably result in the machine locking solid,
never coming out of the WFI. So i'm playing it safe and handling the
interrupt.

> > +static int dove_cpufreq_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *np;
> > +       struct resource *res;
> > +       int err;
> > +       int irq;
> 
> Above two can be merged.

Well, i was told the exact opposite by the USB serial maintainer :-) 
I will use your coding style here, and his for USB code, and try to
keep everybody happy.

 
> > +       priv.dev = &pdev->dev;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       priv.dfs = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(priv.dfs))
> > +               return PTR_ERR(priv.dfs);
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +       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(pdev, IORESOURCE_MEM, 2);
> > +       priv.pmu_clk_div = devm_ioremap_resource(&pdev->dev, res);
> > +       if (IS_ERR(priv.pmu_clk_div))
> > +               return PTR_ERR(priv.pmu_clk_div);
> > +
> > +       np = of_find_node_by_path("/cpus/cpu at 0");
> > +       if (!np)
> > +               return -ENODEV;
> > +
> > +       priv.cpu_clk = of_clk_get_by_name(np, "cpu_clk");
> > +       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);
> 
> this can fail..

In theory yes. In practice no. It is a fixed rate clock. prepare and
enable are actually NOPs for this type of clock. However the API
documentation explicitly states you need to call prepare and enable
before you can call clk_get_rate(). But i can add error checking if
you want.

> 
> > +       dove_freq_table[0].frequency = clk_get_rate(priv.cpu_clk) / 1000;

  Andrew

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-22 15:57     ` Andrew Lunn
@ 2013-10-23  4:29       ` Viresh Kumar
  0 siblings, 0 replies; 26+ messages in thread
From: Viresh Kumar @ 2013-10-23  4:29 UTC (permalink / raw)
  To: linux-arm-kernel

On 22 October 2013 21:27, Andrew Lunn <andrew@lunn.ch> wrote:
> The hardware raises an interrupt when the change is complete. This
> interrupt is what brings the CPU out of the WFI. I don't know the
> interrupt code too well, but i think it is necessary for the driver to
> acknowledged it. The interrupt core code counts interrupts which
> nobody acknowledges, and after 1000 are received, it disables the
> interrupt. That would probably result in the machine locking solid,
> never coming out of the WFI. So i'm playing it safe and handling the
> interrupt.

Theory looks correct AFAIK.. But it would be better to give it a try
on real hardware, hopefully you have that :)

Also, in case this is required, it would be better to document it a bit
over the routine..

> In theory yes. In practice no. It is a fixed rate clock. prepare and
> enable are actually NOPs for this type of clock. However the API
> documentation explicitly states you need to call prepare and enable
> before you can call clk_get_rate(). But i can add error checking if
> you want.

Leave it then..

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
@ 2013-10-23 13:04 Andrew Lunn
  2013-10-23 13:40 ` Viresh Kumar
  0 siblings, 1 reply; 26+ messages in thread
From: Andrew Lunn @ 2013-10-23 13:04 UTC (permalink / raw)
  To: linux-arm-kernel

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>
---
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
---
 drivers/cpufreq/Kconfig.arm    |   5 +
 drivers/cpufreq/Makefile       |   1 +
 drivers/cpufreq/dove-cpufreq.c | 279 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 285 insertions(+)
 create mode 100644 drivers/cpufreq/dove-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 701ec95..9560384 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 b7948bb..5956661 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 0000000..10f013c
--- /dev/null
+++ b/drivers/cpufreq/dove-cpufreq.c
@@ -0,0 +1,279 @@
+/*
+ *	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@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 void dove_cpufreq_set_cpu_state(struct cpufreq_policy *policy,
+				       unsigned int index)
+{
+	struct cpufreq_freqs freqs;
+	unsigned int state = dove_freq_table[index].driver_data;
+	unsigned long reg, cr;
+
+	freqs.old = dove_cpufreq_get_cpu_frequency(0);
+	freqs.new = dove_freq_table[index].frequency;
+
+	if (freqs.old != freqs.new) {
+		cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE);
+		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();
+		cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE);
+	}
+}
+
+static int dove_cpufreq_target(struct cpufreq_policy *policy,
+			    unsigned int target_freq,
+			    unsigned int relation)
+{
+	unsigned int index = 0;
+
+	if (cpufreq_frequency_table_target(policy, dove_freq_table,
+				target_freq, relation, &index))
+		return -EINVAL;
+
+	dove_cpufreq_set_cpu_state(policy, index);
+
+	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.
+ */
+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 = 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@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.4.rc3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-23 13:04 [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
@ 2013-10-23 13:40 ` Viresh Kumar
  2013-10-23 13:51   ` Andrew Lunn
  0 siblings, 1 reply; 26+ messages in thread
From: Viresh Kumar @ 2013-10-23 13:40 UTC (permalink / raw)
  To: linux-arm-kernel

On 23 October 2013 18:34, 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>
> ---
> Sort header files
> Comment the interrupt handler

Really? I don't see these two comments being incorporated..
Also, you would be required to update your patchset ones my
patches are in (And that will happen for v3.13)..

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-23 13:40 ` Viresh Kumar
@ 2013-10-23 13:51   ` Andrew Lunn
  2013-10-23 14:25     ` Viresh Kumar
  0 siblings, 1 reply; 26+ messages in thread
From: Andrew Lunn @ 2013-10-23 13:51 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Oct 23, 2013 at 07:10:41PM +0530, Viresh Kumar wrote:
> On 23 October 2013 18:34, 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>
> > ---
> > Sort header files
> > Comment the interrupt handler
> 
> Really? I don't see these two comments being incorporated..
> Also, you would be required to update your patchset ones my
> patches are in (And that will happen for v3.13)..

+#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>

There does seem to be a convention that kernel.h and module.h come
first, and then i have the rest in order. 

+/*
+ * Handle the interrupt raised when the frequency change is
+ * complete.
+ */
+static irqreturn_t dove_cpufreq_irq(int irq, void *dev)
+{
+       return IRQ_HANDLED;
+}

What more would you like to see in the comment?

Thanks
     Andrew

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-23 13:51   ` Andrew Lunn
@ 2013-10-23 14:25     ` Viresh Kumar
  2013-10-23 14:36       ` Andrew Lunn
  0 siblings, 1 reply; 26+ messages in thread
From: Viresh Kumar @ 2013-10-23 14:25 UTC (permalink / raw)
  To: linux-arm-kernel

On 23 October 2013 19:21, Andrew Lunn <andrew@lunn.ch> wrote:
> There does seem to be a convention that kernel.h and module.h come
> first, and then i have the rest in order.

Atleast I am not aware of any such convention and so got
confused.

> +/*
> + * Handle the interrupt raised when the frequency change is
> + * complete.
> + */
> +static irqreturn_t dove_cpufreq_irq(int irq, void *dev)
> +{
> +       return IRQ_HANDLED;
> +}
>
> What more would you like to see in the comment?

Ahh.. I read it as "Commented interrupt handler and so it is no
more registered" :)

So, you have actually tested your code without interrupt handler?
What exactly happens in that case?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-23 14:25     ` Viresh Kumar
@ 2013-10-23 14:36       ` Andrew Lunn
  2013-10-23 15:00         ` Viresh Kumar
  0 siblings, 1 reply; 26+ messages in thread
From: Andrew Lunn @ 2013-10-23 14:36 UTC (permalink / raw)
  To: linux-arm-kernel

> So, you have actually tested your code without interrupt handler?

No.

> What exactly happens in that case?

Take a look at request_threaded_irq(). It contains:

1421         if (!handler) {
1422                 if (!thread_fn)
1423                         return -EINVAL;

So devm_request_irq() will fail, and so the probe function will fail.

   Andrew

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-23 15:00         ` Viresh Kumar
@ 2013-10-23 14:59           ` Andrew Lunn
  2013-10-24  2:17             ` Viresh Kumar
  2013-10-28 11:31           ` Andrew Lunn
  1 sibling, 1 reply; 26+ messages in thread
From: Andrew Lunn @ 2013-10-23 14:59 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Oct 23, 2013 at 08:30:02PM +0530, Viresh Kumar wrote:
> On 23 October 2013 20:06, Andrew Lunn <andrew@lunn.ch> wrote:
> >> So, you have actually tested your code without interrupt handler?
> >
> > No.
> 
> It would be better if you atleast try this and confirm that this dummy
> handler is required.
> 
> >> What exactly happens in that case?
> >
> > Take a look at request_threaded_irq(). It contains:
> >
> > 1421         if (!handler) {
> > 1422                 if (!thread_fn)
> > 1423                         return -EINVAL;
> >
> > So devm_request_irq() will fail, and so the probe function will fail.
> 
> Obviously I wanted you to remove all irq specific code and hence
> devm_request_irq() :)

So you want to know if WFI exits without an interrupt being delivered?
The Marvell documentation says the interrupt should be enabled, but we
can try it with it disabled.

    Andrew

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-23 14:36       ` Andrew Lunn
@ 2013-10-23 15:00         ` Viresh Kumar
  2013-10-23 14:59           ` Andrew Lunn
  2013-10-28 11:31           ` Andrew Lunn
  0 siblings, 2 replies; 26+ messages in thread
From: Viresh Kumar @ 2013-10-23 15:00 UTC (permalink / raw)
  To: linux-arm-kernel

On 23 October 2013 20:06, Andrew Lunn <andrew@lunn.ch> wrote:
>> So, you have actually tested your code without interrupt handler?
>
> No.

It would be better if you atleast try this and confirm that this dummy
handler is required.

>> What exactly happens in that case?
>
> Take a look at request_threaded_irq(). It contains:
>
> 1421         if (!handler) {
> 1422                 if (!thread_fn)
> 1423                         return -EINVAL;
>
> So devm_request_irq() will fail, and so the probe function will fail.

Obviously I wanted you to remove all irq specific code and hence
devm_request_irq() :)

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-23 14:59           ` Andrew Lunn
@ 2013-10-24  2:17             ` Viresh Kumar
  0 siblings, 0 replies; 26+ messages in thread
From: Viresh Kumar @ 2013-10-24  2:17 UTC (permalink / raw)
  To: linux-arm-kernel

On 23 October 2013 20:29, Andrew Lunn <andrew@lunn.ch> wrote:
> So you want to know if WFI exits without an interrupt being delivered?

Yeah..

> The Marvell documentation says the interrupt should be enabled, but we
> can try it with it disabled.

Yes, that will work.. So we will not explicitly disable interrupt but let kernel
do it for us (i.e. by not registering an interrupt handler)..

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-23 15:00         ` Viresh Kumar
  2013-10-23 14:59           ` Andrew Lunn
@ 2013-10-28 11:31           ` Andrew Lunn
  2013-10-29 11:33             ` Viresh Kumar
  1 sibling, 1 reply; 26+ messages in thread
From: Andrew Lunn @ 2013-10-28 11:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Oct 23, 2013 at 08:30:02PM +0530, Viresh Kumar wrote:
> On 23 October 2013 20:06, Andrew Lunn <andrew@lunn.ch> wrote:
> >> So, you have actually tested your code without interrupt handler?
> >
> > No.
> 
> It would be better if you atleast try this and confirm that this dummy
> handler is required.

Hi Viresh

I have tested this now. The dummy handler is required. Without it, it
seems like the WFI exits at the next timer tick, not immediately once
the cpufreq change has finished. I get poor numbers out of
cpufreq-bench without the handler.

	      Andrew

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove
  2013-10-28 11:31           ` Andrew Lunn
@ 2013-10-29 11:33             ` Viresh Kumar
  0 siblings, 0 replies; 26+ messages in thread
From: Viresh Kumar @ 2013-10-29 11:33 UTC (permalink / raw)
  To: linux-arm-kernel

On 28 October 2013 17:01, Andrew Lunn <andrew@lunn.ch> wrote:
> I have tested this now. The dummy handler is required. Without it, it
> seems like the WFI exits at the next timer tick, not immediately once
> the cpufreq change has finished. I get poor numbers out of
> cpufreq-bench without the handler.

Ahh.. that helps.. You can now put exactly this stuff in the comment over
the handler..

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2013-10-29 11:33 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-19 12:37 [PATCH 0/4] Dove cpufreq driver Andrew Lunn
2013-10-19 12:37 ` [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
2013-10-19 23:09   ` Luka Perkov
2013-10-20  8:42     ` Andrew Lunn
2013-10-21 10:42   ` Sebastian Hesselbarth
2013-10-21 15:42     ` Andrew Lunn
2013-10-21 16:38       ` Sebastian Hesselbarth
2013-10-22  9:01   ` Viresh Kumar
2013-10-22 15:57     ` Andrew Lunn
2013-10-23  4:29       ` Viresh Kumar
2013-10-22 10:19   ` Sudeep KarkadaNagesha
2013-10-19 12:37 ` [PATCH 2/4] mvebu: Dove: Instantiate cpufreq driver Andrew Lunn
2013-10-21 10:47   ` Sebastian Hesselbarth
2013-10-21 15:26     ` Andrew Lunn
2013-10-19 12:37 ` [PATCH 3/4] mvebu: Dove: Enable cpufreq driver in defconfig Andrew Lunn
2013-10-19 12:37 ` [PATCH 4/4] mvebu: Dove: Add clocks and DFS interrupt to cpu node in DT Andrew Lunn
  -- strict thread matches above, loose matches on Subject: below --
2013-10-23 13:04 [PATCH 1/4] cpufreq: Add a cpufreq driver for Marvell Dove Andrew Lunn
2013-10-23 13:40 ` Viresh Kumar
2013-10-23 13:51   ` Andrew Lunn
2013-10-23 14:25     ` Viresh Kumar
2013-10-23 14:36       ` Andrew Lunn
2013-10-23 15:00         ` Viresh Kumar
2013-10-23 14:59           ` Andrew Lunn
2013-10-24  2:17             ` Viresh Kumar
2013-10-28 11:31           ` Andrew Lunn
2013-10-29 11:33             ` Viresh Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).