| Message ID | 1356698844-4220-1-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/28/2012 06:47 AM, Andrew Lunn wrote: > 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. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > .../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 > > diff --git a/Documentation/devicetree/bindings/power/qnap-poweroff.txt b/Documentation/devicetree/bindings/power/qnap-poweroff.txt > new file mode 100644 > index 0000000..e15a334 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/qnap-poweroff.txt > @@ -0,0 +1,14 @@ > +* QNAP Power Off This doesn't match the rest of the patch. > + > +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 > + > diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi > index 7735cee..ca99aa2 100644 > --- a/arch/arm/boot/dts/kirkwood.dtsi > +++ b/arch/arm/boot/dts/kirkwood.dtsi > @@ -23,6 +23,11 @@ > #address-cells = <1>; > #size-cells = <1>; > > + cpuidle@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 > core_clk: core-clocks@10030 { > compatible = "marvell,kirkwood-core-clock"; > reg = <0x10030 0x4>; > diff --git a/arch/arm/configs/kirkwood_defconfig b/arch/arm/configs/kirkwood_defconfig > index 93f3794..13482ea 100644 > --- a/arch/arm/configs/kirkwood_defconfig > +++ b/arch/arm/configs/kirkwood_defconfig > @@ -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 > diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile > index 8d2e5a9..d665309 100644 > --- a/arch/arm/mach-kirkwood/Makefile > +++ b/arch/arm/mach-kirkwood/Makefile > @@ -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 > diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c > index bac21a5..e8a2978 100644 > --- a/arch/arm/mach-kirkwood/common.c > +++ b/arch/arm/mach-kirkwood/common.c > @@ -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 > diff --git a/arch/arm/mach-kirkwood/cpuidle.c b/arch/arm/mach-kirkwood/cpuidle.c > deleted file mode 100644 > index f730467..0000000 > --- a/arch/arm/mach-kirkwood/cpuidle.c > +++ /dev/null > @@ -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); > diff --git a/arch/arm/mach-kirkwood/include/mach/kirkwood.h b/arch/arm/mach-kirkwood/include/mach/kirkwood.h > index 041653a..a05563a 100644 > --- a/arch/arm/mach-kirkwood/include/mach/kirkwood.h > +++ b/arch/arm/mach-kirkwood/include/mach/kirkwood.h > @@ -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) > diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig > index c4cc27e..071e2c3 100644 > --- a/drivers/cpuidle/Kconfig > +++ b/drivers/cpuidle/Kconfig > @@ -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 > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile > index 03ee874..24c6e7d 100644 > --- a/drivers/cpuidle/Makefile > +++ b/drivers/cpuidle/Makefile > @@ -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 > diff --git a/drivers/cpuidle/cpuidle-kirkwood.c b/drivers/cpuidle/cpuidle-kirkwood.c > new file mode 100644 > index 0000000..2d9d5b3 > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-kirkwood.c > @@ -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 <andrew@lunn.ch>"); > +MODULE_DESCRIPTION("Kirkwood cpu idle driver"); > +MODULE_LICENSE("GPLv2"); > +MODULE_ALIAS("platform:kirkwood-cpuidle"); >
Hello Andrew, Le 12/28/12 13:47, Andrew Lunn a écrit : > 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 <andrew@lunn.ch> > --- [snip] > > +/***************************************************************************** > + * 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? -- Florian
On Fri, Dec 28, 2012 at 08:18:42AM -0600, Rob Herring wrote: > On 12/28/2012 06:47 AM, Andrew Lunn wrote: > > 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. 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. > > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > --- > > .../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 > > > > diff --git a/Documentation/devicetree/bindings/power/qnap-poweroff.txt b/Documentation/devicetree/bindings/power/qnap-poweroff.txt > > new file mode 100644 > > index 0000000..e15a334 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/power/qnap-poweroff.txt > > @@ -0,0 +1,14 @@ > > +* QNAP Power Off > > This doesn't match the rest of the patch. Yep, does not belong here. It should be in one of the next patches. > > --- a/arch/arm/boot/dts/kirkwood.dtsi > > +++ b/arch/arm/boot/dts/kirkwood.dtsi > > @@ -23,6 +23,11 @@ > > #address-cells = <1>; > > #size-cells = <1>; > > > > + cpuidle@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. > > Rob > > > core_clk: core-clocks@10030 { > > compatible = "marvell,kirkwood-core-clock"; > > reg = <0x10030 0x4>; > > diff --git a/arch/arm/configs/kirkwood_defconfig b/arch/arm/configs/kirkwood_defconfig > > index 93f3794..13482ea 100644 > > --- a/arch/arm/configs/kirkwood_defconfig > > +++ b/arch/arm/configs/kirkwood_defconfig > > @@ -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 > > diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile > > index 8d2e5a9..d665309 100644 > > --- a/arch/arm/mach-kirkwood/Makefile > > +++ b/arch/arm/mach-kirkwood/Makefile > > @@ -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 > > diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c > > index bac21a5..e8a2978 100644 > > --- a/arch/arm/mach-kirkwood/common.c > > +++ b/arch/arm/mach-kirkwood/common.c > > @@ -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 > > diff --git a/arch/arm/mach-kirkwood/cpuidle.c b/arch/arm/mach-kirkwood/cpuidle.c > > deleted file mode 100644 > > index f730467..0000000 > > --- a/arch/arm/mach-kirkwood/cpuidle.c > > +++ /dev/null > > @@ -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); > > diff --git a/arch/arm/mach-kirkwood/include/mach/kirkwood.h b/arch/arm/mach-kirkwood/include/mach/kirkwood.h > > index 041653a..a05563a 100644 > > --- a/arch/arm/mach-kirkwood/include/mach/kirkwood.h > > +++ b/arch/arm/mach-kirkwood/include/mach/kirkwood.h > > @@ -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) > > diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig > > index c4cc27e..071e2c3 100644 > > --- a/drivers/cpuidle/Kconfig > > +++ b/drivers/cpuidle/Kconfig > > @@ -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 > > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile > > index 03ee874..24c6e7d 100644 > > --- a/drivers/cpuidle/Makefile > > +++ b/drivers/cpuidle/Makefile > > @@ -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 > > diff --git a/drivers/cpuidle/cpuidle-kirkwood.c b/drivers/cpuidle/cpuidle-kirkwood.c > > new file mode 100644 > > index 0000000..2d9d5b3 > > --- /dev/null > > +++ b/drivers/cpuidle/cpuidle-kirkwood.c > > @@ -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 <andrew@lunn.ch>"); > > +MODULE_DESCRIPTION("Kirkwood cpu idle driver"); > > +MODULE_LICENSE("GPLv2"); > > +MODULE_ALIAS("platform:kirkwood-cpuidle"); > > >
On Fri, Dec 28, 2012 at 03:32:08PM +0100, Florian Fainelli wrote: > Hello Andrew, > > Le 12/28/12 13:47, Andrew Lunn a ??crit : > >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 <andrew@lunn.ch> > >--- > > [snip] > > > > >+/***************************************************************************** > >+ * 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? Yes it should. Thanks Andrew
On 12/28/2012 08:35 AM, Andrew Lunn wrote: > On Fri, Dec 28, 2012 at 08:18:42AM -0600, Rob Herring wrote: >> On 12/28/2012 06:47 AM, Andrew Lunn wrote: >>> 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. > > 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". >>> + cpuidle@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? 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. > 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. Rob
On Fri, Dec 28, 2012 at 08:55:31AM -0600, Rob Herring wrote: > On 12/28/2012 08:35 AM, Andrew Lunn wrote: > > On Fri, Dec 28, 2012 at 08:18:42AM -0600, Rob Herring wrote: > >> On 12/28/2012 06:47 AM, Andrew Lunn wrote: > >>> 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. > > > > 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. > >>> + cpuidle@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? > > 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". > 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. > > > 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. 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
On 12/28/2012 09:49 AM, Andrew Lunn wrote: > On Fri, Dec 28, 2012 at 08:55:31AM -0600, Rob Herring wrote: >> On 12/28/2012 08:35 AM, Andrew Lunn wrote: >>> On Fri, Dec 28, 2012 at 08:18:42AM -0600, Rob Herring wrote: >>>> On 12/28/2012 06:47 AM, Andrew Lunn wrote: >>>>> 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. >>> >>> 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. > >>>>> + cpuidle@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? >> >> 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". > >> 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. >> >>> 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. > > 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. Rob > Thanks > > Andrew >
> > 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. > >>> 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. > > > > 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
On Friday 28 December 2012 09:44 PM, Rob Herring wrote: > On 12/28/2012 09:49 AM, Andrew Lunn wrote: >> On Fri, Dec 28, 2012 at 08:55:31AM -0600, Rob Herring wrote: >>> On 12/28/2012 08:35 AM, Andrew Lunn wrote: >>>> On Fri, Dec 28, 2012 at 08:18:42AM -0600, Rob Herring wrote: >>>>> On 12/28/2012 06:47 AM, Andrew Lunn wrote: >>>>>> 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. >>>> >>>> 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. >> >>>>>> + cpuidle@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? >>> >>> 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". >> >>> 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. > >>> >>>> 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. >> >> 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 ? + 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. Regards Santosh
On 12/28/2012 10:38 AM, Andrew Lunn wrote: >>> 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. That was only an example. > 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. > >>>>> 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. >>> >>> 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? You could just define the memory controller node and use it directly by cpuidle for now. If or when you need to support multiple users, then the kernel can be updated to have some intermediate MFD or memory controller driver. You just don't want to be changing the DTS when that happens. Hardcoding it is what we have now, so it's not any worse and would be acceptable. Rob > > Thanks > > Andrew >
> 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 ? > > + 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. 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. Andrew
On Friday 28 December 2012 10:58 PM, Andrew Lunn wrote: >> 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 ? >> >> + 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. > > 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. > Thanks for additional info. > 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. Regards Santosh
> Is this single CPU or multi-cpu machine ? Its a uniprocessor. > 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. Andrew
On Friday 28 December 2012 11:26 PM, Andrew Lunn wrote: >> Is this single CPU or multi-cpu machine ? > > Its a uniprocessor. > That should work then. >> 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. > Sure but you must be stopping DMA before entering idle where DDR can enter into self-refresh otherwise DMA transfer will be aborted. DDR controller also can take care of such a scenario by not entering into self refresh when DMA is active and self refresh command is issued. > 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. > Thanks for information that it is robust and working well. I was just curious having faced some issues on this topic in past. Regards Santosh
On Fri, 28 Dec 2012 13:47:24 +0100, Andrew Lunn <andrew@lunn.ch> wrote: > 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 <andrew@lunn.ch> > --- > .../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 > > diff --git a/Documentation/devicetree/bindings/power/qnap-poweroff.txt b/Documentation/devicetree/bindings/power/qnap-poweroff.txt > new file mode 100644 > index 0000000..e15a334 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/qnap-poweroff.txt > @@ -0,0 +1,14 @@ > +* 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 > +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. g.
> > +* 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 > > +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? Thanks Andrew
On Sun, 10 Feb 2013 19:58:12 +0100, Andrew Lunn <andrew@lunn.ch> wrote: > > > +* 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 > > > +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? for the ts109: compatible = "qnap,ts109-poweroff"; for the others: compatible = "qnap,ts[model]-poweroff", "qnap,ts109-poweroff"; That is the usual way things are done. The newer model claims compatibility with the older. Some have also tried to use a 'generic' value for claiming compatibility (ie. "qnap,tsxxx-poweroff") but I don't recommend that because generic values aren't 'anchored' to any real piece of hardware. Instead 'qnap,ts109-poweroff' is used as the 'generic' version with the advantage that when new hardware appears that doesn't conform you simply drop the string for the older version. It is also recommended to also include the specific model number simply as a mechanism to test for quirks if ever needed... not critical though for system level devices since the top level system model property can also be used for that. g.
diff --git a/Documentation/devicetree/bindings/power/qnap-poweroff.txt b/Documentation/devicetree/bindings/power/qnap-poweroff.txt new file mode 100644 index 0000000..e15a334 --- /dev/null +++ b/Documentation/devicetree/bindings/power/qnap-poweroff.txt @@ -0,0 +1,14 @@ +* 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 +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 + diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi index 7735cee..ca99aa2 100644 --- a/arch/arm/boot/dts/kirkwood.dtsi +++ b/arch/arm/boot/dts/kirkwood.dtsi @@ -23,6 +23,11 @@ #address-cells = <1>; #size-cells = <1>; + cpuidle@1418 { + compatible = "marvell,kirkwood-cpuidle"; + reg = <0x1418 0x4>; + }; + core_clk: core-clocks@10030 { compatible = "marvell,kirkwood-core-clock"; reg = <0x10030 0x4>; diff --git a/arch/arm/configs/kirkwood_defconfig b/arch/arm/configs/kirkwood_defconfig index 93f3794..13482ea 100644 --- a/arch/arm/configs/kirkwood_defconfig +++ b/arch/arm/configs/kirkwood_defconfig @@ -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 diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile index 8d2e5a9..d665309 100644 --- a/arch/arm/mach-kirkwood/Makefile +++ b/arch/arm/mach-kirkwood/Makefile @@ -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 diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c index bac21a5..e8a2978 100644 --- a/arch/arm/mach-kirkwood/common.c +++ b/arch/arm/mach-kirkwood/common.c @@ -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 diff --git a/arch/arm/mach-kirkwood/cpuidle.c b/arch/arm/mach-kirkwood/cpuidle.c deleted file mode 100644 index f730467..0000000 --- a/arch/arm/mach-kirkwood/cpuidle.c +++ /dev/null @@ -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); diff --git a/arch/arm/mach-kirkwood/include/mach/kirkwood.h b/arch/arm/mach-kirkwood/include/mach/kirkwood.h index 041653a..a05563a 100644 --- a/arch/arm/mach-kirkwood/include/mach/kirkwood.h +++ b/arch/arm/mach-kirkwood/include/mach/kirkwood.h @@ -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) diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig index c4cc27e..071e2c3 100644 --- a/drivers/cpuidle/Kconfig +++ b/drivers/cpuidle/Kconfig @@ -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 diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index 03ee874..24c6e7d 100644 --- a/drivers/cpuidle/Makefile +++ b/drivers/cpuidle/Makefile @@ -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 diff --git a/drivers/cpuidle/cpuidle-kirkwood.c b/drivers/cpuidle/cpuidle-kirkwood.c new file mode 100644 index 0000000..2d9d5b3 --- /dev/null +++ b/drivers/cpuidle/cpuidle-kirkwood.c @@ -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 <andrew@lunn.ch>"); +MODULE_DESCRIPTION("Kirkwood cpu idle driver"); +MODULE_LICENSE("GPLv2"); +MODULE_ALIAS("platform:kirkwood-cpuidle");
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 <andrew@lunn.ch> --- .../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