On Tue, Sep 15, 2015 at 05:07:10PM +0800, Dongsheng Wang wrote: > From: Wang Dongsheng <dongsheng.w...@freescale.com> > > Add system STANDBY implement for ls1021 platform. > > Signed-off-by: Chenhui Zhao <chenhui.z...@freescale.com> > Signed-off-by: Wang Dongsheng <dongsheng.w...@freescale.com> > --- > *v2*: > - Remove PSCI code. Just implement STANDBY in platform code.
Why stepping back? We are encouraged to move to PSCI, aren't we? > diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile > index fb689d8..d7a2d1d 100644 > --- a/arch/arm/mach-imx/Makefile > +++ b/arch/arm/mach-imx/Makefile > @@ -90,6 +90,7 @@ ifeq ($(CONFIG_SUSPEND),y) > AFLAGS_suspend-imx6.o :=-Wa,-march=armv7-a > obj-$(CONFIG_SOC_IMX6) += suspend-imx6.o > obj-$(CONFIG_SOC_IMX53) += suspend-imx53.o > +obj-$(CONFIG_SOC_LS1021A) += pm-ls1.o > endif > obj-$(CONFIG_SOC_IMX6) += pm-imx6.o > > diff --git a/arch/arm/mach-imx/pm-ls1.c b/arch/arm/mach-imx/pm-ls1.c > new file mode 100644 > index 0000000..90775cf > --- /dev/null > +++ b/arch/arm/mach-imx/pm-ls1.c > @@ -0,0 +1,225 @@ > +/* > + * Support Power Management Control for LS1 > + * > + * Copyright 2015 Freescale Semiconductor Inc. > + * > + * This program is free software; you can redistribute it and/or > modify it > + * under the terms of the GNU General Public License as published by > the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + */ > + > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/of_platform.h> > +#include <linux/of_address.h> > +#include <linux/suspend.h> > + > +#include <asm/cacheflush.h> > +#include <asm/suspend.h> > + > +#define FSL_SLEEP 0x1 > +#define FSL_DEEP_SLEEP 0x2 Unused defines. > + > +#define CCSR_SCFG_CLUSTERPMCR 0x904 > +#define CCSR_SCFG_CLUSTERPMCR_WFIL2EN 0x80000000 > +#define CCSR_SCFG_CLUSTERPM_ENABLE 1 > +#define CCSR_SCFG_CLUSTERPM_DISABLE 0 > + > +#define CCSR_RCPM_POWMGTCSR 0x130 > +#define CCSR_RCPM_POWMGTCSR_LPM20_REQ 0x00100000 > +#define CCSR_RCPM_IPPDEXPCR0 0x140 > +#define CCSR_RCPM_IPPDEXPCR1 0x144 > +#define CCSR_RCPM_IPPDEXPCR1_OCRAM1 0x10000000 > + > +#define SLEEP_ARRAY_SIZE 3 The name of the macro doesn't appealing. > + > +static u32 ippdexpcr0, ippdexpcr1; It makes more sense to have a structure holds all these variables and the following base addresses. > + > +struct ls1_pm_baseaddr { > + void __iomem *rcpm; > + void __iomem *scfg; > +}; > + > +static struct ls1_pm_baseaddr ls1_pm_base; > + > +static inline void ls1_clrsetbits_be32(void __iomem *addr, u32 clear, u32 > set) > +{ > + u32 val; > + > + val = ioread32be(addr); > + val = (val & ~clear) | set; > + iowrite32be(val, addr); > +} > + > +static int ls1_pm_iomap(suspend_state_t state) How is argument 'state' used? > +{ > + struct device_node *np; > + void *base; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,ls1021a-scfg"); > + if (!np) { > + pr_err("Missing SCFG node in the device tree\n"); > + return -EINVAL; > + } > + > + base = of_iomap(np, 0); > + of_node_put(np); > + if (!base) { > + pr_err("Couldn't map SCFG registers\n"); > + return -EINVAL; > + } > + > + ls1_pm_base.scfg = base; > + > + return 0; > +} > + > +static void ls1_pm_uniomap(suspend_state_t state) > +{ > + iounmap(ls1_pm_base.scfg); > +} > + > +/* set IP powerdown exception, make them work during sleep/deep sleep */ > +static void ls1_set_powerdown(void) > +{ > + iowrite32be(ippdexpcr0, ls1_pm_base.rcpm + CCSR_RCPM_IPPDEXPCR0); > + iowrite32be(ippdexpcr1, ls1_pm_base.rcpm + CCSR_RCPM_IPPDEXPCR1); > +} > + > +static void ls1_set_power_except(struct device *dev, int on) How argument 'on' is used? > +{ > + int ret; > + u32 value[SLEEP_ARRAY_SIZE]; > + > + /* > + * Get the values in the "rcpm-wakeup" property. There are three values. > + * The first points to the RCPM node, the second is the value of > + * the ippdexpcr0 register, and the third is the value of > + * the ippdexpcr1 register. > + */ > + ret = of_property_read_u32_array(dev->of_node, "rcpm-wakeup", > + value, SLEEP_ARRAY_SIZE); The property should be documented in device tree bindings. And you need a good reason for why these register values should be defined by device tree. > + if (ret) { > + dev_err(dev, "%s: Can not find the \"sleep\" property.\n", > + __func__); > + return; > + } > + > + ippdexpcr0 |= value[1]; > + ippdexpcr1 |= value[2]; > + > + pr_debug("%s: set %s as a wakeup source", __func__, When you have device pointer, you should use dev_dbg() instead of pr_debug(). > + dev->of_node->full_name); > +} > + > +static void ls1_set_wakeup_device(struct device *dev, void *enable) > +{ > + /* set each device which can act as wakeup source */ > + if (device_may_wakeup(dev)) > + ls1_set_power_except(dev, *((int *)enable)); > +} > + > +/* enable cluster to enter the PCL10 state */ > +static void ls1_set_clusterpm(int enable) > +{ > + u32 val = 0; > + > + if (enable) > + val = CCSR_SCFG_CLUSTERPMCR_WFIL2EN; > + > + iowrite32be(val, ls1_pm_base.scfg + CCSR_SCFG_CLUSTERPMCR); > +} > + > +static int ls1_suspend_enter(suspend_state_t state) > +{ > + int ret = 0; > + > + ls1_set_powerdown(); > + ls1_set_clusterpm(CCSR_SCFG_CLUSTERPM_ENABLE); > + > + switch (state) { > + case PM_SUSPEND_STANDBY: > + flush_cache_louis(); > + ls1_clrsetbits_be32(ls1_pm_base.rcpm + CCSR_RCPM_POWMGTCSR, > + CCSR_RCPM_POWMGTCSR_LPM20_REQ, > + CCSR_RCPM_POWMGTCSR_LPM20_REQ); > + cpu_do_idle(); > + break; > + case PM_SUSPEND_MEM: > + default: > + ret = -EINVAL; > + break; > + } > + > + ls1_set_clusterpm(CCSR_SCFG_CLUSTERPM_DISABLE); > + > + return ret; > +} > + > +static int ls1_suspend_valid(suspend_state_t state) > +{ > + if (state == PM_SUSPEND_STANDBY) > + return 1; > + > + return 0; > +} > + > +static int ls1_suspend_begin(suspend_state_t state) > +{ > + int res = -EINVAL; The convention of return variable is 'ret' than 'res'. > + > + ippdexpcr0 = 0; > + ippdexpcr1 = 0; > + > + if (state == PM_SUSPEND_STANDBY) > + res = ls1_pm_iomap(state); > + > + if (!res) > + dpm_for_each_dev(NULL, ls1_set_wakeup_device); > + > + return res; > +} > + > +static void ls1_suspend_end(void) > +{ > + ls1_pm_uniomap(PM_SUSPEND_STANDBY); > +} The .begin() and .end() hooks will be called each time that system enters/exits suspend, right? Are you sure the setup you're doing in ls1_suspend_begin() and ls1_suspend_end() is needed each time? Or they only need to be done in ls1_pm_init() for once? > + > +static const struct platform_suspend_ops ls1_suspend_ops = { > + .valid = ls1_suspend_valid, > + .enter = ls1_suspend_enter, > + .begin = ls1_suspend_begin, > + .end = ls1_suspend_end, > +}; > + > +static const struct of_device_id rcpm_matches[] = { > + { > + .compatible = "fsl,ls1021a-rcpm", Undocumented compatible. > + }, > + {} > +}; > + > +static int __init ls1_pm_init(void) > +{ > + const struct of_device_id *match; > + struct device_node *np; > + > + np = of_find_matching_node_and_match(NULL, rcpm_matches, &match); You are simply trying to find "fsl,ls1021a-rcpm" node, so why not just use of_find_compatible_node() like you try to find "fsl,ls1021a-scfg" in ls1_pm_iomap(). > + if (!np) { > + pr_err("%s: can't find the rcpm node.\n", __func__); > + return -EINVAL; > + } > + > + ls1_pm_base.rcpm = of_iomap(np, 0); > + of_node_put(np); > + if (!ls1_pm_base.rcpm) > + return -ENOMEM; Right. Why cannot iomap of scfg be done here just like rcpm? > + > + suspend_set_ops(&ls1_suspend_ops); > + > + pr_info("Freescale Power Management Control Registered\n"); > + > + return 0; > +} > +arch_initcall(ls1_pm_init); Bear it in mind that we're now in multi-platform world, where a single kernel image will run multiple targets, LS1021A, IMX, Vybrid, and even non-FSL SoCs. You cannot use such initcall here, which will get the function called on non-LS1021A platform. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/