cpuidle: kirkwood: Move out of mach directory, add DT.

Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and into drivers/cpuidle. Convert the driver into a platform driver and add a device tree binding. Add a DT node to instantiate the driver for boards converted to DT, and a platform data for old style boards.

Signed-off-by Andrew Lunn <[email protected]>

---
 .../devicetree/bindings/power/qnap-poweroff.txt    |   14 +++
 arch/arm/boot/dts/kirkwood.dtsi                    |    5 +
 arch/arm/configs/kirkwood_defconfig                |    1 +
 arch/arm/mach-kirkwood/Makefile                    |    1 -
 arch/arm/mach-kirkwood/common.c                    |   23 ++++
 arch/arm/mach-kirkwood/cpuidle.c                   |   73 -------------
 arch/arm/mach-kirkwood/include/mach/kirkwood.h     |    3 +-
 drivers/cpuidle/Kconfig                            |    6 ++
 drivers/cpuidle/Makefile                           |    1 +
 drivers/cpuidle/cpuidle-kirkwood.c                 |  114 ++++++++++++++++++++
 10 files changed, 166 insertions(+), 75 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/qnap-poweroff.txt
 delete mode 100644 arch/arm/mach-kirkwood/cpuidle.c
 create mode 100644 drivers/cpuidle/cpuidle-kirkwood.c

Documentation/devicetree/bindings/power/qnap-poweroff.txt new file +0 -0

new file mode 100644
index 0000000..e15a334

Documentation/devicetree/bindings/power/qnap-poweroff.txt new file +13 -0

+
+QNAP NAS devices have a microcontroller controlling the main power
+supply. This microcontroller is connected to UART1 of the Kirkwood and
+Orion5x SoCs. Sending the charactor 'A', at 19200 baud, tells the
+microcontroller to turn the power off. This driver adds a handler to
+pm_power_off which is called to turn the power off.
+
+Required Properties:
+- compatibile: Should be "qnap,power-off"
+
+- reg: Address and length of the register set for UART1
+- clocks: tclk clock
+

arch/arm/boot/dts/kirkwood.dtsi +0 -0

index 7735cee..ca99aa2 100644

arch/arm/boot/dts/kirkwood.dtsi +0 -0

 		core_clk: core-clocks at 10030 {
 			compatible = "marvell,kirkwood-core-clock";
 			reg = <0x10030 0x4>;

arch/arm/configs/kirkwood_defconfig +1 -0

index 93f3794..13482ea 100644
@@ -56,6 +56,7 @@ CONFIG_AEABI=y
 CONFIG_ZBOOT_ROM_TEXT=0x0
 CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_CPU_IDLE=y
+CONFIG_CPU_IDLE_KIRKWOOD=y
 CONFIG_NET=y
 CONFIG_PACKET=y
 CONFIG_UNIX=y

arch/arm/mach-kirkwood/Makefile +0 -1

index 8d2e5a9..d665309 100644
@@ -19,7 +19,6 @@ obj-$(CONFIG_MACH_NET2BIG_V2)		+= netxbig_v2-setup.o lacie_v2-common.o
 obj-$(CONFIG_MACH_NET5BIG_V2)		+= netxbig_v2-setup.o lacie_v2-common.o
 obj-$(CONFIG_MACH_T5325)		+= t5325-setup.o
 
-obj-$(CONFIG_CPU_IDLE)			+= cpuidle.o
 obj-$(CONFIG_ARCH_KIRKWOOD_DT)		+= board-dt.o
 obj-$(CONFIG_MACH_DREAMPLUG_DT)		+= board-dreamplug.o
 obj-$(CONFIG_MACH_ICONNECT_DT)		+= board-iconnect.o

arch/arm/mach-kirkwood/common.c +23 -0

index bac21a5..e8a2978 100644
@@ -499,6 +499,28 @@ void __init kirkwood_wdt_init(void)
 	orion_wdt_init();
 }
 
+/*****************************************************************************
+ * CPU idle
+ ****************************************************************************/
+static struct resource kirkwood_cpuidle_resource[] = {
+	{
+		.flags	= IORESOURCE_MEM,
+		.start	= DDR_OPERATION_BASE,
+		.end	= 3,
+	},
+};
+
+static struct platform_device kirkwood_cpuidle = {
+	.name		= "kirkwood_cpuidle",
+	.id		= -1,
+	.resource	= kirkwood_cpuidle_resource,
+	.num_resources	= 1,
+};
+
+static void __init kirkwood_cpuidle_init(void)
+{
+	platform_device_register(&kirkwood_cpuidle);
+}
 
 /*****************************************************************************
  * Time handling

@@ -671,6 +693,7 @@ void __init kirkwood_init(void)
 	kirkwood_xor1_init();
 	kirkwood_crypto_init();
 
+	kirkwood_cpuidle_init();
 #ifdef CONFIG_KEXEC
 	kexec_reinit = kirkwood_enable_pcie;
 #endif

arch/arm/mach-kirkwood/cpuidle.c deleted +0 -73

deleted file mode 100644
index f730467..0000000
@@ -1,73 +0,0 @@
-/*
- * arch/arm/mach-kirkwood/cpuidle.c
- *
- * CPU idle Marvell Kirkwood SoCs
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2.  This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
- *
- * The cpu idle uses wait-for-interrupt and DDR self refresh in order
- * to implement two idle states -
- * #1 wait-for-interrupt
- * #2 wait-for-interrupt and DDR self refresh
- */
-
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/platform_device.h>
-#include <linux/cpuidle.h>
-#include <linux/io.h>
-#include <linux/export.h>
-#include <asm/proc-fns.h>
-#include <asm/cpuidle.h>
-#include <mach/kirkwood.h>
-
-#define KIRKWOOD_MAX_STATES	2
-
-/* Actual code that puts the SoC in different idle states */
-static int kirkwood_enter_idle(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-			       int index)
-{
-	writel(0x7, DDR_OPERATION_BASE);
-	cpu_do_idle();
-
-	return index;
-}
-
-static struct cpuidle_driver kirkwood_idle_driver = {
-	.name			= "kirkwood_idle",
-	.owner			= THIS_MODULE,
-	.en_core_tk_irqen	= 1,
-	.states[0]		= ARM_CPUIDLE_WFI_STATE,
-	.states[1]		= {
-		.enter			= kirkwood_enter_idle,
-		.exit_latency		= 10,
-		.target_residency	= 100000,
-		.flags			= CPUIDLE_FLAG_TIME_VALID,
-		.name			= "DDR SR",
-		.desc			= "WFI and DDR Self Refresh",
-	},
-	.state_count = KIRKWOOD_MAX_STATES,
-};
-
-static DEFINE_PER_CPU(struct cpuidle_device, kirkwood_cpuidle_device);
-
-/* Initialize CPU idle by registering the idle states */
-static int kirkwood_init_cpuidle(void)
-{
-	struct cpuidle_device *device;
-
-	device = &per_cpu(kirkwood_cpuidle_device, smp_processor_id());
-	device->state_count = KIRKWOOD_MAX_STATES;
-
-	cpuidle_register_driver(&kirkwood_idle_driver);
-	if (cpuidle_register_device(device)) {
-		pr_err("kirkwood_init_cpuidle: Failed registering\n");
-		return -EIO;
-	}
-	return 0;
-}
-
-device_initcall(kirkwood_init_cpuidle);

arch/arm/mach-kirkwood/include/mach/kirkwood.h +2 -1

index 041653a..a05563a 100644
@@ -60,8 +60,9 @@
  * Register Map
  */
 #define DDR_VIRT_BASE		(KIRKWOOD_REGS_VIRT_BASE + 0x00000)
+#define DDR_PHYS_BASE		(KIRKWOOD_REGS_PHYS_BASE + 0x00000)
 #define  DDR_WINDOW_CPU_BASE	(DDR_VIRT_BASE + 0x1500)
-#define DDR_OPERATION_BASE	(DDR_VIRT_BASE + 0x1418)
+#define DDR_OPERATION_BASE	(DDR_PHYS_BASE + 0x1418)
 
 #define DEV_BUS_PHYS_BASE	(KIRKWOOD_REGS_PHYS_BASE + 0x10000)
 #define DEV_BUS_VIRT_BASE	(KIRKWOOD_REGS_VIRT_BASE + 0x10000)

drivers/cpuidle/Kconfig +6 -0

index c4cc27e..071e2c3 100644
@@ -39,4 +39,10 @@ config CPU_IDLE_CALXEDA
 	help
 	  Select this to enable cpuidle on Calxeda processors.
 
+config CPU_IDLE_KIRKWOOD
+	bool "CPU Idle Driver for Kirkwood processors"
+	depends on ARCH_KIRKWOOD
+	help
+	  Select this to enable cpuidle on Kirkwood processors.
+
 endif

drivers/cpuidle/Makefile +1 -0

index 03ee874..24c6e7d 100644
@@ -6,3 +6,4 @@ obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
 obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
 
 obj-$(CONFIG_CPU_IDLE_CALXEDA) += cpuidle-calxeda.o
+obj-$(CONFIG_CPU_IDLE_KIRKWOOD) += cpuidle-kirkwood.o

drivers/cpuidle/cpuidle-kirkwood.c new file +114 -0

new file mode 100644
index 0000000..2d9d5b3
@@ -0,0 +1,114 @@
+/*
+ * arch/arm/mach-kirkwood/cpuidle.c
+ *
+ * CPU idle Marvell Kirkwood SoCs
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * The cpu idle uses wait-for-interrupt and DDR self refresh in order
+ * to implement two idle states -
+ * #1 wait-for-interrupt
+ * #2 wait-for-interrupt and DDR self refresh
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/cpuidle.h>
+#include <linux/io.h>
+#include <linux/export.h>
+#include <linux/of.h>
+#include <asm/proc-fns.h>
+#include <asm/cpuidle.h>
+
+#define KIRKWOOD_MAX_STATES	2
+
+static void __iomem *ddr_operation_base;
+
+/* Actual code that puts the SoC in different idle states */
+static int kirkwood_enter_idle(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv,
+			       int index)
+{
+	writel(0x7, ddr_operation_base);
+	cpu_do_idle();
+
+	return index;
+}
+
+static struct cpuidle_driver kirkwood_idle_driver = {
+	.name			= "kirkwood_idle",
+	.owner			= THIS_MODULE,
+	.en_core_tk_irqen	= 1,
+	.states[0]		= ARM_CPUIDLE_WFI_STATE,
+	.states[1]		= {
+		.enter			= kirkwood_enter_idle,
+		.exit_latency		= 10,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "DDR SR",
+		.desc			= "WFI and DDR Self Refresh",
+	},
+	.state_count = KIRKWOOD_MAX_STATES,
+};
+static struct cpuidle_device *device;
+
+static DEFINE_PER_CPU(struct cpuidle_device, kirkwood_cpuidle_device);
+
+/* Initialize CPU idle by registering the idle states */
+static int kirkwood_cpuidle_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL)
+		return -EINVAL;
+
+	ddr_operation_base = devm_ioremap(&pdev->dev, res->start,
+					  resource_size(res));
+	if (ddr_operation_base == NULL)
+		return -EINVAL;
+
+	device = &per_cpu(kirkwood_cpuidle_device, smp_processor_id());
+	device->state_count = KIRKWOOD_MAX_STATES;
+
+	cpuidle_register_driver(&kirkwood_idle_driver);
+	if (cpuidle_register_device(device)) {
+		pr_err("kirkwood_init_cpuidle: Failed registering\n");
+		return -EIO;
+	}
+	return 0;
+}
+
+int kirkwood_cpuidle_remove(struct platform_device *pdev)
+{
+	cpuidle_unregister_device(device);
+	cpuidle_unregister_driver(&kirkwood_idle_driver);
+
+	return 0;
+}
+
+static const struct of_device_id of_kirkwood_cpuidle_match[] = {
+	{ .compatible = "marvell,kirkwood-cpuidle", },
+	{},
+};
+
+static struct platform_driver kirkwood_cpuidle_driver = {
+	.probe = kirkwood_cpuidle_probe,
+	.remove = __devexit_p(kirkwood_cpuidle_remove),
+	.driver = {
+		   .name = "kirkwood_cpuidle",
+		   .owner = THIS_MODULE,
+		   .of_match_table = of_kirkwood_cpuidle_match,
+		   },
+};
+
+module_platform_driver(kirkwood_cpuidle_driver);
+
+MODULE_AUTHOR("Andrew Lunn <[email protected]>");
+MODULE_DESCRIPTION("Kirkwood cpu idle driver");
+MODULE_LICENSE("GPLv2");
+MODULE_ALIAS("platform:kirkwood-cpuidle");

Quoted 4 lines · Andrew Lunn

Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and into drivers/cpuidle. Convert the driver into a platform driver and add a device tree binding. Add a DT node to instantiate the driver for boards converted to DT, and a platform data for old style boards.

Is this an old comment? I don't see any platform data.

Quoted 19 lines

Signed-off-by Andrew Lunn <[email protected]>

---
 .../devicetree/bindings/power/qnap-poweroff.txt    |   14 +++
 arch/arm/boot/dts/kirkwood.dtsi                    |    5 +
 arch/arm/configs/kirkwood_defconfig                |    1 +
 arch/arm/mach-kirkwood/Makefile                    |    1 -
 arch/arm/mach-kirkwood/common.c                    |   23 ++++
 arch/arm/mach-kirkwood/cpuidle.c                   |   73 -------------
 arch/arm/mach-kirkwood/include/mach/kirkwood.h     |    3 +-
 drivers/cpuidle/Kconfig                            |    6 ++
 drivers/cpuidle/Makefile                           |    1 +
 drivers/cpuidle/cpuidle-kirkwood.c                 |  114 ++++++++++++++++++++
 10 files changed, 166 insertions(+), 75 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/qnap-poweroff.txt
 delete mode 100644 arch/arm/mach-kirkwood/cpuidle.c
 create mode 100644 drivers/cpuidle/cpuidle-kirkwood.c

Documentation/devicetree/bindings/power/qnap-poweroff.txt line 1

Quoted 7 lines

Documentation/devicetree/bindings/power/qnap-poweroff.txt new file +1 -0

new file mode 100644
index 0000000..e15a334
@@ -0,0 +1,14 @@
+* QNAP Power Off

This doesn't match the rest of the patch.

Quoted 6 lines

arch/arm/boot/dts/kirkwood.dtsi +0 -0

@@ -23,6 +23,11 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 

arch/arm/boot/dts/kirkwood.dtsi line 30

Quoted 5 lines

arch/arm/boot/dts/kirkwood.dtsi +5 -0

+		cpuidle at 1418 {
+			compatible = "marvell,kirkwood-cpuidle";
+			reg = <0x1418 0x4>;
+		};
+

This is describing what linux wants, not the hardware. This is a common problem with cpuidle drivers in that they use shared registers. I don't have a good solution, but this doesn't belong in DTS.

Rob

Quoted 307 lines

arch/arm/boot/dts/kirkwood.dtsi +0 -0

 		core_clk: core-clocks at 10030 {
 			compatible = "marvell,kirkwood-core-clock";
 			reg = <0x10030 0x4>;

arch/arm/configs/kirkwood_defconfig +1 -0

index 93f3794..13482ea 100644
@@ -56,6 +56,7 @@ CONFIG_AEABI=y
 CONFIG_ZBOOT_ROM_TEXT=0x0
 CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_CPU_IDLE=y
+CONFIG_CPU_IDLE_KIRKWOOD=y
 CONFIG_NET=y
 CONFIG_PACKET=y
 CONFIG_UNIX=y

arch/arm/mach-kirkwood/Makefile +0 -1

index 8d2e5a9..d665309 100644
@@ -19,7 +19,6 @@ obj-$(CONFIG_MACH_NET2BIG_V2)		+= netxbig_v2-setup.o lacie_v2-common.o
 obj-$(CONFIG_MACH_NET5BIG_V2)		+= netxbig_v2-setup.o lacie_v2-common.o
 obj-$(CONFIG_MACH_T5325)		+= t5325-setup.o
 
-obj-$(CONFIG_CPU_IDLE)			+= cpuidle.o
 obj-$(CONFIG_ARCH_KIRKWOOD_DT)		+= board-dt.o
 obj-$(CONFIG_MACH_DREAMPLUG_DT)		+= board-dreamplug.o
 obj-$(CONFIG_MACH_ICONNECT_DT)		+= board-iconnect.o

arch/arm/mach-kirkwood/common.c +23 -0

index bac21a5..e8a2978 100644
@@ -499,6 +499,28 @@ void __init kirkwood_wdt_init(void)
 	orion_wdt_init();
 }
 
+/*****************************************************************************
+ * CPU idle
+ ****************************************************************************/
+static struct resource kirkwood_cpuidle_resource[] = {
+	{
+		.flags	= IORESOURCE_MEM,
+		.start	= DDR_OPERATION_BASE,
+		.end	= 3,
+	},
+};
+
+static struct platform_device kirkwood_cpuidle = {
+	.name		= "kirkwood_cpuidle",
+	.id		= -1,
+	.resource	= kirkwood_cpuidle_resource,
+	.num_resources	= 1,
+};
+
+static void __init kirkwood_cpuidle_init(void)
+{
+	platform_device_register(&kirkwood_cpuidle);
+}
 
 /*****************************************************************************
  * Time handling

@@ -671,6 +693,7 @@ void __init kirkwood_init(void)
 	kirkwood_xor1_init();
 	kirkwood_crypto_init();
 
+	kirkwood_cpuidle_init();
 #ifdef CONFIG_KEXEC
 	kexec_reinit = kirkwood_enable_pcie;
 #endif

arch/arm/mach-kirkwood/cpuidle.c deleted +0 -73

deleted file mode 100644
index f730467..0000000
@@ -1,73 +0,0 @@
-/*
- * arch/arm/mach-kirkwood/cpuidle.c
- *
- * CPU idle Marvell Kirkwood SoCs
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2.  This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
- *
- * The cpu idle uses wait-for-interrupt and DDR self refresh in order
- * to implement two idle states -
- * #1 wait-for-interrupt
- * #2 wait-for-interrupt and DDR self refresh
- */
-
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/platform_device.h>
-#include <linux/cpuidle.h>
-#include <linux/io.h>
-#include <linux/export.h>
-#include <asm/proc-fns.h>
-#include <asm/cpuidle.h>
-#include <mach/kirkwood.h>
-
-#define KIRKWOOD_MAX_STATES	2
-
-/* Actual code that puts the SoC in different idle states */
-static int kirkwood_enter_idle(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-			       int index)
-{
-	writel(0x7, DDR_OPERATION_BASE);
-	cpu_do_idle();
-
-	return index;
-}
-
-static struct cpuidle_driver kirkwood_idle_driver = {
-	.name			= "kirkwood_idle",
-	.owner			= THIS_MODULE,
-	.en_core_tk_irqen	= 1,
-	.states[0]		= ARM_CPUIDLE_WFI_STATE,
-	.states[1]		= {
-		.enter			= kirkwood_enter_idle,
-		.exit_latency		= 10,
-		.target_residency	= 100000,
-		.flags			= CPUIDLE_FLAG_TIME_VALID,
-		.name			= "DDR SR",
-		.desc			= "WFI and DDR Self Refresh",
-	},
-	.state_count = KIRKWOOD_MAX_STATES,
-};
-
-static DEFINE_PER_CPU(struct cpuidle_device, kirkwood_cpuidle_device);
-
-/* Initialize CPU idle by registering the idle states */
-static int kirkwood_init_cpuidle(void)
-{
-	struct cpuidle_device *device;
-
-	device = &per_cpu(kirkwood_cpuidle_device, smp_processor_id());
-	device->state_count = KIRKWOOD_MAX_STATES;
-
-	cpuidle_register_driver(&kirkwood_idle_driver);
-	if (cpuidle_register_device(device)) {
-		pr_err("kirkwood_init_cpuidle: Failed registering\n");
-		return -EIO;
-	}
-	return 0;
-}
-
-device_initcall(kirkwood_init_cpuidle);

arch/arm/mach-kirkwood/include/mach/kirkwood.h +2 -1

index 041653a..a05563a 100644
@@ -60,8 +60,9 @@
  * Register Map
  */
 #define DDR_VIRT_BASE		(KIRKWOOD_REGS_VIRT_BASE + 0x00000)
+#define DDR_PHYS_BASE		(KIRKWOOD_REGS_PHYS_BASE + 0x00000)
 #define  DDR_WINDOW_CPU_BASE	(DDR_VIRT_BASE + 0x1500)
-#define DDR_OPERATION_BASE	(DDR_VIRT_BASE + 0x1418)
+#define DDR_OPERATION_BASE	(DDR_PHYS_BASE + 0x1418)
 
 #define DEV_BUS_PHYS_BASE	(KIRKWOOD_REGS_PHYS_BASE + 0x10000)
 #define DEV_BUS_VIRT_BASE	(KIRKWOOD_REGS_VIRT_BASE + 0x10000)

drivers/cpuidle/Kconfig +6 -0

index c4cc27e..071e2c3 100644
@@ -39,4 +39,10 @@ config CPU_IDLE_CALXEDA
 	help
 	  Select this to enable cpuidle on Calxeda processors.
 
+config CPU_IDLE_KIRKWOOD
+	bool "CPU Idle Driver for Kirkwood processors"
+	depends on ARCH_KIRKWOOD
+	help
+	  Select this to enable cpuidle on Kirkwood processors.
+
 endif

drivers/cpuidle/Makefile +1 -0

index 03ee874..24c6e7d 100644
@@ -6,3 +6,4 @@ obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
 obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
 
 obj-$(CONFIG_CPU_IDLE_CALXEDA) += cpuidle-calxeda.o
+obj-$(CONFIG_CPU_IDLE_KIRKWOOD) += cpuidle-kirkwood.o

drivers/cpuidle/cpuidle-kirkwood.c new file +114 -0

new file mode 100644
index 0000000..2d9d5b3
@@ -0,0 +1,114 @@
+/*
+ * arch/arm/mach-kirkwood/cpuidle.c
+ *
+ * CPU idle Marvell Kirkwood SoCs
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * The cpu idle uses wait-for-interrupt and DDR self refresh in order
+ * to implement two idle states -
+ * #1 wait-for-interrupt
+ * #2 wait-for-interrupt and DDR self refresh
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/cpuidle.h>
+#include <linux/io.h>
+#include <linux/export.h>
+#include <linux/of.h>
+#include <asm/proc-fns.h>
+#include <asm/cpuidle.h>
+
+#define KIRKWOOD_MAX_STATES	2
+
+static void __iomem *ddr_operation_base;
+
+/* Actual code that puts the SoC in different idle states */
+static int kirkwood_enter_idle(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv,
+			       int index)
+{
+	writel(0x7, ddr_operation_base);
+	cpu_do_idle();
+
+	return index;
+}
+
+static struct cpuidle_driver kirkwood_idle_driver = {
+	.name			= "kirkwood_idle",
+	.owner			= THIS_MODULE,
+	.en_core_tk_irqen	= 1,
+	.states[0]		= ARM_CPUIDLE_WFI_STATE,
+	.states[1]		= {
+		.enter			= kirkwood_enter_idle,
+		.exit_latency		= 10,
+		.target_residency	= 100000,
+		.flags			= CPUIDLE_FLAG_TIME_VALID,
+		.name			= "DDR SR",
+		.desc			= "WFI and DDR Self Refresh",
+	},
+	.state_count = KIRKWOOD_MAX_STATES,
+};
+static struct cpuidle_device *device;
+
+static DEFINE_PER_CPU(struct cpuidle_device, kirkwood_cpuidle_device);
+
+/* Initialize CPU idle by registering the idle states */
+static int kirkwood_cpuidle_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL)
+		return -EINVAL;
+
+	ddr_operation_base = devm_ioremap(&pdev->dev, res->start,
+					  resource_size(res));
+	if (ddr_operation_base == NULL)
+		return -EINVAL;
+
+	device = &per_cpu(kirkwood_cpuidle_device, smp_processor_id());
+	device->state_count = KIRKWOOD_MAX_STATES;
+
+	cpuidle_register_driver(&kirkwood_idle_driver);
+	if (cpuidle_register_device(device)) {
+		pr_err("kirkwood_init_cpuidle: Failed registering\n");
+		return -EIO;
+	}
+	return 0;
+}
+
+int kirkwood_cpuidle_remove(struct platform_device *pdev)
+{
+	cpuidle_unregister_device(device);
+	cpuidle_unregister_driver(&kirkwood_idle_driver);
+
+	return 0;
+}
+
+static const struct of_device_id of_kirkwood_cpuidle_match[] = {
+	{ .compatible = "marvell,kirkwood-cpuidle", },
+	{},
+};
+
+static struct platform_driver kirkwood_cpuidle_driver = {
+	.probe = kirkwood_cpuidle_probe,
+	.remove = __devexit_p(kirkwood_cpuidle_remove),
+	.driver = {
+		   .name = "kirkwood_cpuidle",
+		   .owner = THIS_MODULE,
+		   .of_match_table = of_kirkwood_cpuidle_match,
+		   },
+};
+
+module_platform_driver(kirkwood_cpuidle_driver);
+
+MODULE_AUTHOR("Andrew Lunn <[email protected]>");
+MODULE_DESCRIPTION("Kirkwood cpu idle driver");
+MODULE_LICENSE("GPLv2");
+MODULE_ALIAS("platform:kirkwood-cpuidle");

Quoted 6 lines · Rob Herring

Quoted 4 lines · Andrew Lunn

Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and into drivers/cpuidle. Convert the driver into a platform driver and add a device tree binding. Add a DT node to instantiate the driver for boards converted to DT, and a platform data for old style boards.

Quoted 1 line

Is this an old comment? I don't see any platform data.

There is no platform data, since all the driver needs is an address of the DDR control register. The code to create a platform device entry is in common.c hunk.

Quoted 9 lines

arch/arm/boot/dts/kirkwood.dtsi line 30

Quoted 5 lines

arch/arm/boot/dts/kirkwood.dtsi +5 -0

+		cpuidle at 1418 {
+			compatible = "marvell,kirkwood-cpuidle";
+			reg = <0x1418 0x4>;
+		};
+

This is describing what linux wants, not the hardware. This is a common problem with cpuidle drivers in that they use shared registers. I don't have a good solution, but this doesn't belong in DTS.

Do you have a bad solution?

I could just hard code the address, since its the same for all kirkwood SoCs. Also, the register is not being used by any other code on kirkwood, so its not shared.

Quoted 7 lines · Andrew Lunn

Quoted 6 lines · Rob Herring

Quoted 4 lines · Andrew Lunn

Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and into drivers/cpuidle. Convert the driver into a platform driver and add a device tree binding. Add a DT node to instantiate the driver for boards converted to DT, and a platform data for old style boards.

Quoted 5 lines

Quoted 1 line

Is this an old comment? I don't see any platform data.

There is no platform data, since all the driver needs is an address of the DDR control register. The code to create a platform device entry is in common.c hunk.

So you should say "a platform device for old style boards".

Quoted 10 lines

Quoted 9 lines

arch/arm/boot/dts/kirkwood.dtsi line 30

Quoted 5 lines

arch/arm/boot/dts/kirkwood.dtsi +5 -0

+		cpuidle at 1418 {
+			compatible = "marvell,kirkwood-cpuidle";
+			reg = <0x1418 0x4>;
+		};
+

This is describing what linux wants, not the hardware. This is a common problem with cpuidle drivers in that they use shared registers. I don't have a good solution, but this doesn't belong in DTS.

Quoted 1 line

Do you have a bad solution?

Ha! :) I should say I don't have a clear, obvious solution.

Don't do a platform driver and just check the machine compatible property which is what I did for highbank.

Have the machine code create
the platform device. Not *all* platform devices have to be created based on the DTB. Create an MFD driver for the whole block of registers.

Quoted 4 lines

I could just hard code the address, since its the same for all kirkwood SoCs. Also, the register is not being used by any other code on kirkwood, so its not shared.

Then describe it based on the reference manual, but you need to do so assuming you are using all the other registers. I assume there are other registers at say 0x1414 or 0x141c. You have to be careful if you create separate nodes for each register or sub-group of registers. It needs to work no matter what the OS expectation is.

Quoted 8 lines · Rob Herring

Quoted 7 lines · Andrew Lunn

Quoted 6 lines · Rob Herring

Quoted 4 lines · Andrew Lunn

Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and into drivers/cpuidle. Convert the driver into a platform driver and add a device tree binding. Add a DT node to instantiate the driver for boards converted to DT, and a platform data for old style boards.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Is this an old comment? I don't see any platform data.

There is no platform data, since all the driver needs is an address of the DDR control register. The code to create a platform device entry is in common.c hunk.

So you should say "a platform device for old style boards".

Hi Rob

O.K. I will change it.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

arch/arm/boot/dts/kirkwood.dtsi line 30

Quoted 5 lines

arch/arm/boot/dts/kirkwood.dtsi +5 -0

+		cpuidle at 1418 {
+			compatible = "marvell,kirkwood-cpuidle";
+			reg = <0x1418 0x4>;
+		};
+

This is describing what linux wants, not the hardware. This is a common problem with cpuidle drivers in that they use shared registers. I don't have a good solution, but this doesn't belong in DTS.

Quoted 6 lines

Quoted 1 line

Do you have a bad solution?

Ha! :) I should say I don't have a clear, obvious solution.

Don't do a platform driver and just check the machine compatible property which is what I did for highbank.

Yes, i've seen your cpuidle-calxeda.c. I can follow that model, but i still somehow need the address of the SDRAM controller "Operation Register".

Quoted 3 lines

Have the machine code create
the platform device. Not *all* platform devices have to be created based on the DTB. Create an MFD driver for the whole block of registers.

The block of registers is for controlling the SDRAM. Its not really a MFD. The cpuidle code is putting the SDRAM controller into self refresh mode and then doing a WFI.

Quoted 5 lines

Quoted 4 lines

I could just hard code the address, since its the same for all kirkwood SoCs. Also, the register is not being used by any other code on kirkwood, so its not shared.

Quoted 5 lines

Then describe it based on the reference manual, but you need to do so assuming you are using all the other registers. I assume there are other registers at say 0x1414 or 0x141c. You have to be careful if you create separate nodes for each register or sub-group of registers. It needs to work no matter what the OS expectation is.

I don't understand what you are try to explain here. It makes little sense for the cpuidle driver to take all the SDRAM control registers.

Thanks

Andrew

Quoted 3 lines

The block of registers is for controlling the SDRAM. Its not really a MFD. The cpuidle code is putting the SDRAM controller into self refresh mode and then doing a WFI.

It is multi-function in the sense that multiple subsystems needing to access shared registers. If you have ECC, then you would need to give EDAC subsystem access to ECC related registers.

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

I could just hard code the address, since its the same for all kirkwood SoCs. Also, the register is not being used by any other code on kirkwood, so its not shared.

Then describe it based on the reference manual, but you need to do so assuming you are using all the other registers. I assume there are other registers at say 0x1414 or 0x141c. You have to be careful if you create separate nodes for each register or sub-group of registers. It needs to work no matter what the OS expectation is.

Quoted 2 lines

I don't understand what you are try to explain here. It makes little sense for the cpuidle driver to take all the SDRAM control registers.

Exactly. The same is true of the dts. It makes little sense to describe only 1 register of a h/w block. Perhaps the memory controller subsystem (drivers/memory) can be expanded to include self-refresh functions. Entering self-refresh can be tricky, so it might not really be possible to define a common api here.

Quoted 7 lines

Quoted 3 lines

The block of registers is for controlling the SDRAM. Its not really a MFD. The cpuidle code is putting the SDRAM controller into self refresh mode and then doing a WFI.

It is multi-function in the sense that multiple subsystems needing to access shared registers. If you have ECC, then you would need to give EDAC subsystem access to ECC related registers.

There is no ECC. Data sheet specifically says its not supported.

Currently, there are no other users of the SDRAM controller than cpuidle. Linux is not touching the configuration, so i assume early boot code in u-boot is setting up the controller.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

I could just hard code the address, since its the same for all kirkwood SoCs. Also, the register is not being used by any other code on kirkwood, so its not shared.

Then describe it based on the reference manual, but you need to do so assuming you are using all the other registers. I assume there are other registers at say 0x1414 or 0x141c. You have to be careful if you create separate nodes for each register or sub-group of registers. It needs to work no matter what the OS expectation is.

Quoted 8 lines

Quoted 2 lines

I don't understand what you are try to explain here. It makes little sense for the cpuidle driver to take all the SDRAM control registers.

Exactly. The same is true of the dts. It makes little sense to describe only 1 register of a h/w block. Perhaps the memory controller subsystem (drivers/memory) can be expanded to include self-refresh functions. Entering self-refresh can be tricky, so it might not really be possible to define a common api here.

For kirkwood it is easy. Poke 0x7 into the register and then WFI.

However, since there are no other users of the SDRAM controller, it seems overkill to implement a drivers/memory driver, or a MFD driver, for just one register.

So maybe i just end up hard coding the register address in the driver?

Thanks

Andrew

Quoted 42 lines · Andrew Lunn

Quoted 8 lines · Rob Herring

Quoted 7 lines · Andrew Lunn

Quoted 6 lines · Rob Herring

Quoted 4 lines · Andrew Lunn

Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and into drivers/cpuidle. Convert the driver into a platform driver and add a device tree binding. Add a DT node to instantiate the driver for boards converted to DT, and a platform data for old style boards.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

Is this an old comment? I don't see any platform data.

There is no platform data, since all the driver needs is an address of the DDR control register. The code to create a platform device entry is in common.c hunk.

So you should say "a platform device for old style boards".

Hi Rob

O.K. I will change it.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

arch/arm/boot/dts/kirkwood.dtsi line 30

Quoted 5 lines

arch/arm/boot/dts/kirkwood.dtsi +5 -0

+		cpuidle at 1418 {
+			compatible = "marvell,kirkwood-cpuidle";
+			reg = <0x1418 0x4>;
+		};
+

This is describing what linux wants, not the hardware. This is a common problem with cpuidle drivers in that they use shared registers. I don't have a good solution, but this doesn't belong in DTS.

Quoted 6 lines

Quoted 1 line

Do you have a bad solution?

Ha! :) I should say I don't have a clear, obvious solution.

Don't do a platform driver and just check the machine compatible property which is what I did for highbank.

Yes, i've seen your cpuidle-calxeda.c. I can follow that model, but i still somehow need the address of the SDRAM controller "Operation Register".

Quoted 7 lines

Quoted 3 lines

Have the machine code create
the platform device. Not *all* platform devices have to be created based on the DTB. Create an MFD driver for the whole block of registers.

The block of registers is for controlling the SDRAM. Its not really a MFD. The cpuidle code is putting the SDRAM controller into self refresh mode and then doing a WFI.

It is multi-function in the sense that multiple subsystems needing to access shared registers. If you have ECC, then you would need to give EDAC subsystem access to ECC related registers.

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

I could just hard code the address, since its the same for all kirkwood SoCs. Also, the register is not being used by any other code on kirkwood, so its not shared.

Then describe it based on the reference manual, but you need to do so assuming you are using all the other registers. I assume there are other registers at say 0x1414 or 0x141c. You have to be careful if you create separate nodes for each register or sub-group of registers. It needs to work no matter what the OS expectation is.

Quoted 2 lines

I don't understand what you are try to explain here. It makes little sense for the cpuidle driver to take all the SDRAM control registers.

Exactly. The same is true of the dts. It makes little sense to describe only 1 register of a h/w block. Perhaps the memory controller subsystem (drivers/memory) can be expanded to include self-refresh functions. Entering self-refresh can be tricky, so it might not really be possible to define a common api here.

Putting DDR/SDRAM into self refresh means, you no longer have RAM available to execute and any code CPU needs to execute after that has to be executed either from lock down cache with no cache evictions or from some internal on chip memory with caches disabled to avoid accesses to DDR. Not sure how below code works if the first writel puts DDR into self refresh. Is cpu_do_idle() code copied to some internal memory ?

(quoted diff) +2 -0

+	writel(0x7, ddr_operation_base);
+	cpu_do_idle();

May be I am missing something but i have worked on such a code for OMAP3 kind of devices and seen major issues with DDR self refresh and CPU entering into idle state.

Quoted 10 lines

Putting DDR/SDRAM into self refresh means, you no longer have RAM available to execute and any code CPU needs to execute after that has to be executed either from lock down cache with no cache evictions or from some internal on chip memory with caches disabled to avoid accesses to DDR. Not sure how below code works if the first writel puts DDR into self refresh. Is cpu_do_idle() code copied to some internal memory ?

(quoted diff) +2 -0

+	writel(0x7, ddr_operation_base);
+	cpu_do_idle();

Quoted 3 lines

May be I am missing something but i have worked on such a code for OMAP3 kind of devices and seen major issues with DDR self refresh and CPU entering into idle state.

Quoting the datasheet:

To place the SDRAM into self refresh set the <Cmd> field in the SDRAM Operation Register (Table 174 p. 400) to 0x7. The SDRAM controller waits for 256 cycles and then generates a self refresh command to SDRAM, and clears the SDRAM Operation register.

I assume we always manage to execute the WFI within these 256 cycles.

There is also some text about handling new pending transactions, so even if it does not WFI, e.g. because of an interrupts, it seems to do the right thing.

Is this single CPU or multi-cpu machine ?

Even though the cpu_do_idle()
has just couple of instructions, there can be lot more happening in background especially with multi masters system. It might be safe if the single CPU is the only master accessing DDR. In multi-master, multi-CPU scenario though it can't work reliably.

Quoted 1 line

Is this single CPU or multi-cpu machine ?

Its a uniprocessor.

Quoted 5 lines

Even though the cpu_do_idle()
has just couple of instructions, there can be lot more happening in background especially with multi masters system. It might be safe if the single CPU is the only master accessing DDR. In multi-master, multi-CPU scenario though it can't work reliably.

There are DMA engines which could be active, moving stuff into/out of memory.

Having said that, this code is not new, it is just getting a new home. There has not been problems before. Having this 256 cycle delay etc, suggests the hardware design is robust.

Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and into drivers/cpuidle. Convert the driver into a platform driver and add a device tree binding. Add a DT node to instantiate the driver for boards converted to DT, and a platform data for old style boards.

Signed-off-by Andrew Lunn <[email protected]>

---

Hello Andrew,

Le 12/28/12 13:47, Andrew Lunn a ??crit :

Quoted 7 lines

Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and into drivers/cpuidle. Convert the driver into a platform driver and add a device tree binding. Add a DT node to instantiate the driver for boards converted to DT, and a platform data for old style boards.

Signed-off-by Andrew Lunn <[email protected]>

---

[snip]

Quoted 1 line

arch/arm/mach-kirkwood/common.c void __init kirkwood_wdt_init(void) · line 509

Quoted 8 lines

arch/arm/mach-kirkwood/common.c +8 -0

+/*****************************************************************************
+ * CPU idle
+ ****************************************************************************/
+static struct resource kirkwood_cpuidle_resource[] = {
+	{
+		.flags	= IORESOURCE_MEM,
+		.start	= DDR_OPERATION_BASE,
+		.end	= 3,

Should not this be DDR_OPERATION_BASE + 3?

Move the Kirkwood cpuidle driver out of arch/arm/mach-kirkwood and into drivers/cpuidle. Convert the driver into a platform driver and add a device tree binding. Add a DT node to instantiate the driver for boards converted to DT, and a platform data for old style boards.

Signed-off-by Andrew Lunn <[email protected]>

---
 .../devicetree/bindings/power/qnap-poweroff.txt    |   14 +++
 arch/arm/boot/dts/kirkwood.dtsi                    |    5 +
 arch/arm/configs/kirkwood_defconfig                |    1 +
 arch/arm/mach-kirkwood/Makefile                    |    1 -
 arch/arm/mach-kirkwood/common.c                    |   23 ++++
 arch/arm/mach-kirkwood/cpuidle.c                   |   73 -------------
 arch/arm/mach-kirkwood/include/mach/kirkwood.h     |    3 +-
 drivers/cpuidle/Kconfig                            |    6 ++
 drivers/cpuidle/Makefile                           |    1 +
 drivers/cpuidle/cpuidle-kirkwood.c                 |  114 ++++++++++++++++++++
 10 files changed, 166 insertions(+), 75 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/qnap-poweroff.txt
 delete mode 100644 arch/arm/mach-kirkwood/cpuidle.c
 create mode 100644 drivers/cpuidle/cpuidle-kirkwood.c

Documentation/devicetree/bindings/power/qnap-poweroff.txt new file +0 -0

new file mode 100644
index 0000000..e15a334

Quoted 6 lines

Documentation/devicetree/bindings/power/qnap-poweroff.txt new file +6 -0

+* QNAP Power Off
+
+QNAP NAS devices have a microcontroller controlling the main power
+supply. This microcontroller is connected to UART1 of the Kirkwood and
+Orion5x SoCs. Sending the charactor 'A', at 19200 baud, tells the
+microcontroller to turn the power off. This driver adds a handler to

Documentation/devicetree/bindings/power/qnap-poweroff.txt line 10

Quoted 4 lines

Documentation/devicetree/bindings/power/qnap-poweroff.txt new file +4 -0

+pm_power_off which is called to turn the power off.
+
+Required Properties:
+- compatibile: Should be "qnap,power-off"

It would do well to have the model number embedded into this compatible string since it is unlikely that every single QNAP device will use the same controller interface.

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/qnap-poweroff.txt new file +6 -0

+* QNAP Power Off
+
+QNAP NAS devices have a microcontroller controlling the main power
+supply. This microcontroller is connected to UART1 of the Kirkwood and
+Orion5x SoCs. Sending the charactor 'A', at 19200 baud, tells the
+microcontroller to turn the power off. This driver adds a handler to

Quoted 8 lines

Documentation/devicetree/bindings/power/qnap-poweroff.txt line 10

Quoted 4 lines

Documentation/devicetree/bindings/power/qnap-poweroff.txt new file +4 -0

+pm_power_off which is called to turn the power off.
+
+Required Properties:
+- compatibile: Should be "qnap,power-off"

It would do well to have the model number embedded into this compatible string since it is unlikely that every single QNAP device will use the same controller interface.

Hi Grant

All QNAP devices known to mainline use this same controller interface.

So that would be ts109, ts209, ts409, ts119, ts219, ts419, plus a few other models which we don't differentiate, like ts219P and ts219P+.

Any suggestions what to use in the compatible string?