| Message ID | 1376144839-1985-3-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 10, 2013 at 11:27:19AM -0300, Ezequiel Garcia wrote: > Implements suspend-to-RAM support in a standby-like fashion: > when the SoC enters suspend state the memory PM units are disabled, > the DDR is set in self-refresh mode, and the CPU is set in WFI. > > Signed-off-by: Simon Guinot <sguinot@lacie.com> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > Changes from v1: > > * Removed clock gating. Each peripheral is in charge of implementing > a proper suspend/resume path, including gateable clock handling. > > arch/arm/mach-kirkwood/Makefile | 1 + > arch/arm/mach-kirkwood/include/mach/bridge-regs.h | 2 + > arch/arm/mach-kirkwood/pm.c | 69 +++++++++++++++++++++++ > 3 files changed, 72 insertions(+) > create mode 100644 arch/arm/mach-kirkwood/pm.c > > diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile > index 937d4e1..788db9c 100644 > --- a/arch/arm/mach-kirkwood/Makefile > +++ b/arch/arm/mach-kirkwood/Makefile > @@ -1,4 +1,5 @@ > obj-y += common.o pcie.o > +obj-$(CONFIG_PM) += pm.o > obj-$(CONFIG_KIRKWOOD_LEGACY) += irq.o mpp.o > obj-$(CONFIG_MACH_D2NET_V2) += d2net_v2-setup.o lacie_v2-common.o > obj-$(CONFIG_MACH_NET2BIG_V2) += netxbig_v2-setup.o lacie_v2-common.o > diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h > index 91242c9..8b9d1c9 100644 > --- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h > +++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h > @@ -78,4 +78,6 @@ > #define CGC_TDM (1 << 20) > #define CGC_RESERVED (0x6 << 21) > > +#define MEMORY_PM_CTRL (BRIDGE_VIRT_BASE + 0x118) > + > #endif > diff --git a/arch/arm/mach-kirkwood/pm.c b/arch/arm/mach-kirkwood/pm.c > new file mode 100644 > index 0000000..364734c > --- /dev/null > +++ b/arch/arm/mach-kirkwood/pm.c > @@ -0,0 +1,69 @@ > +/* > + * Power Management driver for Marvell Kirkwood SoCs > + * > + * Copyright (C) 2013 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > + * Copyright (C) 2010 Simon Guinot <sguinot@lacie.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License, > + * version 2 of the License. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/suspend.h> > +#include <linux/io.h> > +#include <mach/bridge-regs.h> > + > +static void __iomem *ddr_operation_base; > + > +static void kirkwood_suspend_mem(void) > +{ > + u32 mem_pm_ctrl; > + > + mem_pm_ctrl = readl(MEMORY_PM_CTRL); > + > + /* Set peripherals to low-power mode */ > + writel_relaxed(~0, MEMORY_PM_CTRL); > + > + /* Set DDR in self-refresh */ > + writel_relaxed(0x7, ddr_operation_base); > + > + /* > + * Set CPU in wait-for-interrupt state. > + * This disables the CPU core clocks, > + * the array clocks, and also the L2 controller. > + */ > + cpu_do_idle(); > + > + writel_relaxed(mem_pm_ctrl, MEMORY_PM_CTRL); > +} > + > +static int kirkwood_suspend_enter(suspend_state_t state) > +{ > + switch (state) { > + case PM_SUSPEND_MEM: > + kirkwood_suspend_mem(); > + break; > + default: > + return -EINVAL; > + } > + return 0; > +} > + > +static const struct platform_suspend_ops kirkwood_suspend_ops = { > + .enter = kirkwood_suspend_enter, > + .valid = suspend_valid_only_mem, > +}; > + > +static int __init kirkwood_pm_init(void) > +{ > + ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4); > + suspend_set_ops(&kirkwood_suspend_ops); > + return 0; > +} > +arch_initcall(kirkwood_pm_init); Hi Ezequiel Does this work on a multi arch kernel? Should kirkwood_pm_init() be checking it is actually running on a kirkwood? Or call kirkwood_pm_init() from kirkwood_dt_init()? Another issue is that the ddr_operation_base should probably be accessed using your atomic_io_set_clear() function, since it is used by the cpuidle drivers as well. Andrew
On Sat, Aug 10, 2013 at 05:01:15PM +0200, Andrew Lunn wrote: > On Sat, Aug 10, 2013 at 11:27:19AM -0300, Ezequiel Garcia wrote: > > Implements suspend-to-RAM support in a standby-like fashion: > > when the SoC enters suspend state the memory PM units are disabled, > > the DDR is set in self-refresh mode, and the CPU is set in WFI. > > > > Signed-off-by: Simon Guinot <sguinot@lacie.com> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > --- > > Changes from v1: > > > > * Removed clock gating. Each peripheral is in charge of implementing > > a proper suspend/resume path, including gateable clock handling. > > > > arch/arm/mach-kirkwood/Makefile | 1 + > > arch/arm/mach-kirkwood/include/mach/bridge-regs.h | 2 + > > arch/arm/mach-kirkwood/pm.c | 69 +++++++++++++++++++++++ > > 3 files changed, 72 insertions(+) > > create mode 100644 arch/arm/mach-kirkwood/pm.c > > > > diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile > > index 937d4e1..788db9c 100644 > > --- a/arch/arm/mach-kirkwood/Makefile > > +++ b/arch/arm/mach-kirkwood/Makefile > > @@ -1,4 +1,5 @@ > > obj-y += common.o pcie.o > > +obj-$(CONFIG_PM) += pm.o > > obj-$(CONFIG_KIRKWOOD_LEGACY) += irq.o mpp.o > > obj-$(CONFIG_MACH_D2NET_V2) += d2net_v2-setup.o lacie_v2-common.o > > obj-$(CONFIG_MACH_NET2BIG_V2) += netxbig_v2-setup.o lacie_v2-common.o > > diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h > > index 91242c9..8b9d1c9 100644 > > --- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h > > +++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h > > @@ -78,4 +78,6 @@ > > #define CGC_TDM (1 << 20) > > #define CGC_RESERVED (0x6 << 21) > > > > +#define MEMORY_PM_CTRL (BRIDGE_VIRT_BASE + 0x118) > > + > > #endif > > diff --git a/arch/arm/mach-kirkwood/pm.c b/arch/arm/mach-kirkwood/pm.c > > new file mode 100644 > > index 0000000..364734c > > --- /dev/null > > +++ b/arch/arm/mach-kirkwood/pm.c > > @@ -0,0 +1,69 @@ > > +/* > > + * Power Management driver for Marvell Kirkwood SoCs > > + * > > + * Copyright (C) 2013 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > + * Copyright (C) 2010 Simon Guinot <sguinot@lacie.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License, > > + * version 2 of the License. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/suspend.h> > > +#include <linux/io.h> > > +#include <mach/bridge-regs.h> > > + > > +static void __iomem *ddr_operation_base; > > + > > +static void kirkwood_suspend_mem(void) > > +{ > > + u32 mem_pm_ctrl; > > + > > + mem_pm_ctrl = readl(MEMORY_PM_CTRL); > > + > > + /* Set peripherals to low-power mode */ > > + writel_relaxed(~0, MEMORY_PM_CTRL); > > + > > + /* Set DDR in self-refresh */ > > + writel_relaxed(0x7, ddr_operation_base); > > + > > + /* > > + * Set CPU in wait-for-interrupt state. > > + * This disables the CPU core clocks, > > + * the array clocks, and also the L2 controller. > > + */ > > + cpu_do_idle(); > > + > > + writel_relaxed(mem_pm_ctrl, MEMORY_PM_CTRL); > > +} > > + > > +static int kirkwood_suspend_enter(suspend_state_t state) > > +{ > > + switch (state) { > > + case PM_SUSPEND_MEM: > > + kirkwood_suspend_mem(); > > + break; > > + default: > > + return -EINVAL; > > + } > > + return 0; > > +} > > + > > +static const struct platform_suspend_ops kirkwood_suspend_ops = { > > + .enter = kirkwood_suspend_enter, > > + .valid = suspend_valid_only_mem, > > +}; > > + > > +static int __init kirkwood_pm_init(void) > > +{ > > + ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4); > > + suspend_set_ops(&kirkwood_suspend_ops); > > + return 0; > > +} > > +arch_initcall(kirkwood_pm_init); > > Hi Ezequiel > > Does this work on a multi arch kernel? Should kirkwood_pm_init() be > checking it is actually running on a kirkwood? Or call > kirkwood_pm_init() from kirkwood_dt_init()? > Oh, right... I think you already mentioned this. Sorry, forgot about this completely. On the other side, kirkwood is not multiplatform-enabled yet, right? So there's no way this can produce any build error. > Another issue is that the ddr_operation_base should probably be > accessed using your atomic_io_set_clear() function, since it is used > by the cpuidle drivers as well. > I think this is not needed. Both cpuidle suspend core code disables IRQ before entering any of the suspend or cpuidle states. I'm not sure this is sufficient in SMP context? And in any case, do we care about SMP, given kirkwood is UP? BTW, do you think the approach of *not* messing with clocks is ok? Thanks for the feedback!
> > > +static int __init kirkwood_pm_init(void) > > > +{ > > > + ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4); > > > + suspend_set_ops(&kirkwood_suspend_ops); > > > + return 0; > > > +} > > > +arch_initcall(kirkwood_pm_init); > > > > Hi Ezequiel > > > > Does this work on a multi arch kernel? Should kirkwood_pm_init() be > > checking it is actually running on a kirkwood? Or call > > kirkwood_pm_init() from kirkwood_dt_init()? > > > > Oh, right... I think you already mentioned this. Sorry, forgot about > this completely. > > On the other side, kirkwood is not multiplatform-enabled yet, right? > So there's no way this can produce any build error. Kirkwood is not currently multiplatform, but there is nothing i know of which will break when we do become multi-platform. I've been keeping an eye out for such issues, and made sure that cpuidle, cpufreq, etc are multiplatform safe. I think Thomas removed the last blocker for DT based boards going multiplatform when PCI moved to DT. So i don't want to add something now, which i know will break with multiplatform. Lets do it the right way now. > > Another issue is that the ddr_operation_base should probably be > > accessed using your atomic_io_set_clear() function, since it is used > > by the cpuidle drivers as well. > > > > I think this is not needed. Both cpuidle suspend core code disables > IRQ before entering any of the suspend or cpuidle states. I've no idea how PM works, so i cannot say if its safe or not... > > BTW, do you think the approach of *not* messing with clocks is ok? That is correct. The drivers should be disabling clocks, were they can be disabled. We know kirkwood ethernet is broken and we cannot "easily" disable its clock. Andrew
On Sat, Aug 10, 2013 at 06:00:19PM +0200, Andrew Lunn wrote: > > > > +static int __init kirkwood_pm_init(void) > > > > +{ > > > > + ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4); > > > > + suspend_set_ops(&kirkwood_suspend_ops); > > > > + return 0; > > > > +} > > > > +arch_initcall(kirkwood_pm_init); > > > > > > Hi Ezequiel > > > > > > Does this work on a multi arch kernel? Should kirkwood_pm_init() be > > > checking it is actually running on a kirkwood? Or call > > > kirkwood_pm_init() from kirkwood_dt_init()? > > > > > > > Oh, right... I think you already mentioned this. Sorry, forgot about > > this completely. > > > > On the other side, kirkwood is not multiplatform-enabled yet, right? > > So there's no way this can produce any build error. > > Kirkwood is not currently multiplatform, but there is nothing i know > of which will break when we do become multi-platform. I've been > keeping an eye out for such issues, and made sure that cpuidle, > cpufreq, etc are multiplatform safe. I think Thomas removed the last > blocker for DT based boards going multiplatform when PCI moved to DT. > So i don't want to add something now, which i know will break with > multiplatform. Lets do it the right way now. > Of course. And I'd say it's better call kirkwood_pm_init() from some place kirkwood-specific, rather than leaving the generic arch_initcall checking for kirkwood machine. It looks less bloaty, uh? > > > Another issue is that the ddr_operation_base should probably be > > > accessed using your atomic_io_set_clear() function, since it is used > > > by the cpuidle drivers as well. > > > > > > > I think this is not needed. Both cpuidle suspend core code disables > > IRQ before entering any of the suspend or cpuidle states. > > I've no idea how PM works, so i cannot say if its safe or not... Me neither. But I'm checking the code and it's screaming "local irqs disabled" before entering any state. Also, the ticker seems to be stopped. I'd like to know we *really* need such register protection, given it's not an easy task (see the atomic_io_clear_set discussion). > > > > BTW, do you think the approach of *not* messing with clocks is ok? > > That is correct. The drivers should be disabling clocks, were they can > be disabled. We know kirkwood ethernet is broken and we cannot > "easily" disable its clock. > Ok, good. One more thing I came across while reviewing the PM code. There's a "standby" state that seems much more appropriate than the current "mem" state. I looks like there's not any distinction in the PM code handling between the two "mem" and "standby" states, but it's a bit misleading for users to declare "suspend to RAM" when it's not. So I think we should use that instead. I'll fix all of these and prepare a v3. Thanks for the feedback!
diff --git a/arch/arm/mach-kirkwood/Makefile b/arch/arm/mach-kirkwood/Makefile index 937d4e1..788db9c 100644 --- a/arch/arm/mach-kirkwood/Makefile +++ b/arch/arm/mach-kirkwood/Makefile @@ -1,4 +1,5 @@ obj-y += common.o pcie.o +obj-$(CONFIG_PM) += pm.o obj-$(CONFIG_KIRKWOOD_LEGACY) += irq.o mpp.o obj-$(CONFIG_MACH_D2NET_V2) += d2net_v2-setup.o lacie_v2-common.o obj-$(CONFIG_MACH_NET2BIG_V2) += netxbig_v2-setup.o lacie_v2-common.o diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h index 91242c9..8b9d1c9 100644 --- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h +++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h @@ -78,4 +78,6 @@ #define CGC_TDM (1 << 20) #define CGC_RESERVED (0x6 << 21) +#define MEMORY_PM_CTRL (BRIDGE_VIRT_BASE + 0x118) + #endif diff --git a/arch/arm/mach-kirkwood/pm.c b/arch/arm/mach-kirkwood/pm.c new file mode 100644 index 0000000..364734c --- /dev/null +++ b/arch/arm/mach-kirkwood/pm.c @@ -0,0 +1,69 @@ +/* + * Power Management driver for Marvell Kirkwood SoCs + * + * Copyright (C) 2013 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> + * Copyright (C) 2010 Simon Guinot <sguinot@lacie.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, + * version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/suspend.h> +#include <linux/io.h> +#include <mach/bridge-regs.h> + +static void __iomem *ddr_operation_base; + +static void kirkwood_suspend_mem(void) +{ + u32 mem_pm_ctrl; + + mem_pm_ctrl = readl(MEMORY_PM_CTRL); + + /* Set peripherals to low-power mode */ + writel_relaxed(~0, MEMORY_PM_CTRL); + + /* Set DDR in self-refresh */ + writel_relaxed(0x7, ddr_operation_base); + + /* + * Set CPU in wait-for-interrupt state. + * This disables the CPU core clocks, + * the array clocks, and also the L2 controller. + */ + cpu_do_idle(); + + writel_relaxed(mem_pm_ctrl, MEMORY_PM_CTRL); +} + +static int kirkwood_suspend_enter(suspend_state_t state) +{ + switch (state) { + case PM_SUSPEND_MEM: + kirkwood_suspend_mem(); + break; + default: + return -EINVAL; + } + return 0; +} + +static const struct platform_suspend_ops kirkwood_suspend_ops = { + .enter = kirkwood_suspend_enter, + .valid = suspend_valid_only_mem, +}; + +static int __init kirkwood_pm_init(void) +{ + ddr_operation_base = ioremap(DDR_OPERATION_BASE, 4); + suspend_set_ops(&kirkwood_suspend_ops); + return 0; +} +arch_initcall(kirkwood_pm_init);