| Message ID | 1445011379-8787-3-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/16/2015 06:02 PM, Lorenzo Pieralisi wrote: > ARM64 PSCI kernel interfaces that initialize idle states and implement > the suspend API to enter them are generic and can be shared with the > ARM architecture. > > To achieve that goal, this patch moves ARM64 PSCI idle management > code to drivers/firmware, so that the interface to initialize and > enter idle states can actually be shared by ARM and ARM64 arches > back-ends. > > The ARM generic CPUidle implementation also requires the definition of > a cpuidle_ops section entry for the kernel to initialize the CPUidle > operations at boot based on the enable-method (ie ARM64 has the > statically initialized cpu_ops counterparts for that purpose); therefore > this patch also adds the required section entry on CONFIG_ARM for PSCI so > that the kernel can initialize the PSCI CPUidle back-end when PSCI is > the probed enable-method. > > On ARM64 this patch provides no functional change. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Acked-by: Catalin Marinas <catalin.marinas@arm.com> [arch/arm64] > Acked-by: Mark Rutland <mark.rutland@arm.com> > Tested-by: Jisheng Zhang <jszhang@marvell.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Jisheng Zhang <jszhang@marvell.com> > --- Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
On Fri, Oct 16, 2015 at 05:02:59PM +0100, Lorenzo Pieralisi wrote: > ARM64 PSCI kernel interfaces that initialize idle states and implement > the suspend API to enter them are generic and can be shared with the > ARM architecture. > > To achieve that goal, this patch moves ARM64 PSCI idle management > code to drivers/firmware, so that the interface to initialize and > enter idle states can actually be shared by ARM and ARM64 arches > back-ends. > > The ARM generic CPUidle implementation also requires the definition of > a cpuidle_ops section entry for the kernel to initialize the CPUidle > operations at boot based on the enable-method (ie ARM64 has the > statically initialized cpu_ops counterparts for that purpose); therefore > this patch also adds the required section entry on CONFIG_ARM for PSCI so > that the kernel can initialize the PSCI CPUidle back-end when PSCI is > the probed enable-method. > > On ARM64 this patch provides no functional change. On ARM64, it causes build breakage though: drivers/built-in.o: In function `psci_suspend_finisher': arm_pmu.c:(.text+0xc6494): undefined reference to `cpu_resume' arm_pmu.c:(.text+0xc6498): undefined reference to `cpu_resume' drivers/built-in.o: In function `psci_cpu_suspend_enter': arm_pmu.c:(.text+0xc66c0): undefined reference to `cpu_suspend' The code which has been moved looks similar. However, when it lived in arch/arm64/kernel/psci.c, it was protected by #ifdef CONFIG_HOTPLUG_CPU. In its new location, there are no ifdefs around it, and so if it gets built without CONFIG_ARM_CPU_SUSPEND on ARM, or CONFIG_CPU_PM for ARM64, it will error out like the above. As this is causing a regression, and I've now closed my tree, I will be doing what I said yesterday: I'll be dropping this patch for this merge window in order to stabilise my tree. Sorry.
On Tue, Jan 05, 2016 at 10:59:01AM +0000, Russell King - ARM Linux wrote: > On Fri, Oct 16, 2015 at 05:02:59PM +0100, Lorenzo Pieralisi wrote: > > ARM64 PSCI kernel interfaces that initialize idle states and implement > > the suspend API to enter them are generic and can be shared with the > > ARM architecture. > > > > To achieve that goal, this patch moves ARM64 PSCI idle management > > code to drivers/firmware, so that the interface to initialize and > > enter idle states can actually be shared by ARM and ARM64 arches > > back-ends. > > > > The ARM generic CPUidle implementation also requires the definition of > > a cpuidle_ops section entry for the kernel to initialize the CPUidle > > operations at boot based on the enable-method (ie ARM64 has the > > statically initialized cpu_ops counterparts for that purpose); therefore > > this patch also adds the required section entry on CONFIG_ARM for PSCI so > > that the kernel can initialize the PSCI CPUidle back-end when PSCI is > > the probed enable-method. > > > > On ARM64 this patch provides no functional change. > > On ARM64, it causes build breakage though: > > drivers/built-in.o: In function `psci_suspend_finisher': > arm_pmu.c:(.text+0xc6494): undefined reference to `cpu_resume' > arm_pmu.c:(.text+0xc6498): undefined reference to `cpu_resume' > drivers/built-in.o: In function `psci_cpu_suspend_enter': > arm_pmu.c:(.text+0xc66c0): undefined reference to `cpu_suspend' > > The code which has been moved looks similar. However, when it lived > in arch/arm64/kernel/psci.c, it was protected by > #ifdef CONFIG_HOTPLUG_CPU. In its new location, there are no ifdefs > around it, and so if it gets built without CONFIG_ARM_CPU_SUSPEND on > ARM, or CONFIG_CPU_PM for ARM64, it will error out like the above. > > As this is causing a regression, and I've now closed my tree, I will > be doing what I said yesterday: I'll be dropping this patch for this > merge window in order to stabilise my tree. Sorry. My bad, I apologise, I will likely have to add a config option to make sure cpu_{suspend/resume} code is compiled in (on both ARM/ARM64), thanks for spotting it. Lorenzo
On Tue, Jan 05, 2016 at 12:31:34PM +0000, Lorenzo Pieralisi wrote: > On Tue, Jan 05, 2016 at 10:59:01AM +0000, Russell King - ARM Linux wrote: > > On Fri, Oct 16, 2015 at 05:02:59PM +0100, Lorenzo Pieralisi wrote: > > > ARM64 PSCI kernel interfaces that initialize idle states and implement > > > the suspend API to enter them are generic and can be shared with the > > > ARM architecture. > > > > > > To achieve that goal, this patch moves ARM64 PSCI idle management > > > code to drivers/firmware, so that the interface to initialize and > > > enter idle states can actually be shared by ARM and ARM64 arches > > > back-ends. > > > > > > The ARM generic CPUidle implementation also requires the definition of > > > a cpuidle_ops section entry for the kernel to initialize the CPUidle > > > operations at boot based on the enable-method (ie ARM64 has the > > > statically initialized cpu_ops counterparts for that purpose); therefore > > > this patch also adds the required section entry on CONFIG_ARM for PSCI so > > > that the kernel can initialize the PSCI CPUidle back-end when PSCI is > > > the probed enable-method. > > > > > > On ARM64 this patch provides no functional change. > > > > On ARM64, it causes build breakage though: > > > > drivers/built-in.o: In function `psci_suspend_finisher': > > arm_pmu.c:(.text+0xc6494): undefined reference to `cpu_resume' > > arm_pmu.c:(.text+0xc6498): undefined reference to `cpu_resume' > > drivers/built-in.o: In function `psci_cpu_suspend_enter': > > arm_pmu.c:(.text+0xc66c0): undefined reference to `cpu_suspend' > > > > The code which has been moved looks similar. However, when it lived > > in arch/arm64/kernel/psci.c, it was protected by > > #ifdef CONFIG_HOTPLUG_CPU. In its new location, there are no ifdefs > > around it, and so if it gets built without CONFIG_ARM_CPU_SUSPEND on > > ARM, or CONFIG_CPU_PM for ARM64, it will error out like the above. > > > > As this is causing a regression, and I've now closed my tree, I will > > be doing what I said yesterday: I'll be dropping this patch for this > > merge window in order to stabilise my tree. Sorry. > > My bad, I apologise, I will likely have to add a config option to > make sure cpu_{suspend/resume} code is compiled in (on both ARM/ARM64), > thanks for spotting it. On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely controls whether cpu_suspend/resume are present. ARM64 just needs to adopt this, and use that to control the code which is built in drivers/firmware/psci.c. However, I don't think it's as simple as just adding that to ARM64, as you need to be careful of the Kconfig dependencies. For ARM, this is: Generic code: - SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for any cpu_suspend enabled CPU.) - PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS ARM sets: - CPU_PM if SUSPEND || CPU_IDLE. - ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM) What this means is that CPU_PM is entirely independent of ARM_CPU_SUSPEND. One does not imply the other, so I think you need to consider carefully what ifdef you need in drivers/firmware/psci.c. This is why I think fixing this is not simple as it first looks.
On Tue, Jan 05, 2016 at 12:51:42PM +0000, Russell King - ARM Linux wrote: [...] > > > > On ARM64 this patch provides no functional change. > > > > > > On ARM64, it causes build breakage though: > > > > > > drivers/built-in.o: In function `psci_suspend_finisher': > > > arm_pmu.c:(.text+0xc6494): undefined reference to `cpu_resume' > > > arm_pmu.c:(.text+0xc6498): undefined reference to `cpu_resume' > > > drivers/built-in.o: In function `psci_cpu_suspend_enter': > > > arm_pmu.c:(.text+0xc66c0): undefined reference to `cpu_suspend' > > > > > > The code which has been moved looks similar. However, when it lived > > > in arch/arm64/kernel/psci.c, it was protected by > > > #ifdef CONFIG_HOTPLUG_CPU. In its new location, there are no ifdefs > > > around it, and so if it gets built without CONFIG_ARM_CPU_SUSPEND on > > > ARM, or CONFIG_CPU_PM for ARM64, it will error out like the above. > > > > > > As this is causing a regression, and I've now closed my tree, I will > > > be doing what I said yesterday: I'll be dropping this patch for this > > > merge window in order to stabilise my tree. Sorry. > > > > My bad, I apologise, I will likely have to add a config option to > > make sure cpu_{suspend/resume} code is compiled in (on both ARM/ARM64), > > thanks for spotting it. > > On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely > controls whether cpu_suspend/resume are present. ARM64 just needs > to adopt this, and use that to control the code which is built in > drivers/firmware/psci.c. > > However, I don't think it's as simple as just adding that to ARM64, > as you need to be careful of the Kconfig dependencies. For ARM, > this is: > > Generic code: > - SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for > any cpu_suspend enabled CPU.) > - PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS > ARM sets: > - CPU_PM if SUSPEND || CPU_IDLE. > - ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM) > > What this means is that CPU_PM is entirely independent of > ARM_CPU_SUSPEND. One does not imply the other, so I think you need > to consider carefully what ifdef you need in drivers/firmware/psci.c. > > This is why I think fixing this is not simple as it first looks. Not saying it is nice, but unless I find a cleaner way I was keener on adding a silent config entry in drivers/firmware, say: config ARM_PSCI_CPU_IDLE def_bool ARM_PSCI_FW && CPU_IDLE select ARM_CPU_SUSPEND if ARM and use that to either guard the code or stub it out and compile it if that config option is enabled. I will post a v4 at -rc1. Thanks, Lorenzo
On Tue, Jan 05, 2016 at 01:27:01PM +0000, Lorenzo Pieralisi wrote: > On Tue, Jan 05, 2016 at 12:51:42PM +0000, Russell King - ARM Linux wrote: > > On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely > > controls whether cpu_suspend/resume are present. ARM64 just needs > > to adopt this, and use that to control the code which is built in > > drivers/firmware/psci.c. > > > > However, I don't think it's as simple as just adding that to ARM64, > > as you need to be careful of the Kconfig dependencies. For ARM, > > this is: > > > > Generic code: > > - SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for > > any cpu_suspend enabled CPU.) > > - PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS > > ARM sets: > > - CPU_PM if SUSPEND || CPU_IDLE. > > - ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM) > > > > What this means is that CPU_PM is entirely independent of > > ARM_CPU_SUSPEND. One does not imply the other, so I think you need > > to consider carefully what ifdef you need in drivers/firmware/psci.c. > > > > This is why I think fixing this is not simple as it first looks. > > Not saying it is nice, but unless I find a cleaner way I was keener on > adding a silent config entry in drivers/firmware, say: > > config ARM_PSCI_CPU_IDLE > def_bool ARM_PSCI_FW && CPU_IDLE > select ARM_CPU_SUSPEND if ARM > > and use that to either guard the code or stub it out and compile it > if that config option is enabled. That immediately worries me, because it bypasses the CPU dependencies for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE. I'd prefer instead: config ARM_PSCI_CPU_IDLE def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND) Really, the answer is to stop ARM64 diverging from ARM, so we stop having these architecture conditionals all over the place. If ARM64 builds its cpu_suspend code in the same way that ARM does (iow, keyed off ARM_CPU_SUSPEND, which it can select), then we end up with the above being: config ARM_PSCI_CPU_IDLE def_bool ARM_PSCI_FW && CPU_IDLE && ARM_CPU_SUSPEND which is a helll of a lot simpler. The dependency on ARM_PSCI_FW could be regarded as redundant if we're only using ARM_PSCI_CPU_IDLE to control code built within drivers/firmware/psci.c, as that won't be built unless ARM_PSCI_FW is set.
On Tue, Jan 05, 2016 at 01:34:47PM +0000, Russell King - ARM Linux wrote: > On Tue, Jan 05, 2016 at 01:27:01PM +0000, Lorenzo Pieralisi wrote: > > On Tue, Jan 05, 2016 at 12:51:42PM +0000, Russell King - ARM Linux wrote: > > > On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely > > > controls whether cpu_suspend/resume are present. ARM64 just needs > > > to adopt this, and use that to control the code which is built in > > > drivers/firmware/psci.c. > > > > > > However, I don't think it's as simple as just adding that to ARM64, > > > as you need to be careful of the Kconfig dependencies. For ARM, > > > this is: > > > > > > Generic code: > > > - SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for > > > any cpu_suspend enabled CPU.) > > > - PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS > > > ARM sets: > > > - CPU_PM if SUSPEND || CPU_IDLE. > > > - ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM) > > > > > > What this means is that CPU_PM is entirely independent of > > > ARM_CPU_SUSPEND. One does not imply the other, so I think you need > > > to consider carefully what ifdef you need in drivers/firmware/psci.c. > > > > > > This is why I think fixing this is not simple as it first looks. > > > > Not saying it is nice, but unless I find a cleaner way I was keener on > > adding a silent config entry in drivers/firmware, say: > > > > config ARM_PSCI_CPU_IDLE > > def_bool ARM_PSCI_FW && CPU_IDLE > > select ARM_CPU_SUSPEND if ARM > > > > and use that to either guard the code or stub it out and compile it > > if that config option is enabled. > > That immediately worries me, because it bypasses the CPU dependencies > for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE. I'd > prefer instead: > > config ARM_PSCI_CPU_IDLE > def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND) Ok, I think the above is reasonable, only question I have is if on ARM: CONFIG_SUSPEND=n CONFIG_CPU_IDLE=y this would imply: CONFIG_ARM_CPU_SUSPEND=n => ARM_PSCI_CPU_IDLE=n (which is a questionable setup but possible) we can't do PSCI based CPUidle (I agree with you that bypassing the dependencies is not ideal or correct in the first place though), that's a problem for all subsystems "selecting" ARM_CPU_SUSPEND. > Really, the answer is to stop ARM64 diverging from ARM, so we stop > having these architecture conditionals all over the place. If ARM64 > builds its cpu_suspend code in the same way that ARM does (iow, > keyed off ARM_CPU_SUSPEND, which it can select), then we end up > with the above being: > > config ARM_PSCI_CPU_IDLE > def_bool ARM_PSCI_FW && CPU_IDLE && ARM_CPU_SUSPEND Yes, we could do that, on ARM64 should be = SUSPEND || CPU_IDLE, ergo the value of CPU_PM on ARM64, that's why we do not have another config entry at present. > which is a helll of a lot simpler. The dependency on ARM_PSCI_FW > could be regarded as redundant if we're only using ARM_PSCI_CPU_IDLE > to control code built within drivers/firmware/psci.c, as that won't > be built unless ARM_PSCI_FW is set. Yep, that's a good point and I will remove ARM_PSCI_FW from the first option you provided above. Thanks, Lorenzo
On Tue, Jan 05, 2016 at 01:34:47PM +0000, Russell King - ARM Linux wrote: > On Tue, Jan 05, 2016 at 01:27:01PM +0000, Lorenzo Pieralisi wrote: > > On Tue, Jan 05, 2016 at 12:51:42PM +0000, Russell King - ARM Linux wrote: > > > On ARM, that option is CONFIG_ARM_CPU_SUSPEND - that option solely > > > controls whether cpu_suspend/resume are present. ARM64 just needs > > > to adopt this, and use that to control the code which is built in > > > drivers/firmware/psci.c. > > > > > > However, I don't think it's as simple as just adding that to ARM64, > > > as you need to be careful of the Kconfig dependencies. For ARM, > > > this is: > > > > > > Generic code: > > > - SUSPEND defaults to y, depends on ARCH_SUSPEND_POSSIBLE (which is set for > > > any cpu_suspend enabled CPU.) > > > - PM_SLEEP if SUSPEND || HIBERNATE_CALLBACKS > > > ARM sets: > > > - CPU_PM if SUSPEND || CPU_IDLE. > > > - ARM_CPU_SUSPEND if PM_SLEEP || BL_SWITCHER || (ARCH_PXA && PM) > > > > > > What this means is that CPU_PM is entirely independent of > > > ARM_CPU_SUSPEND. One does not imply the other, so I think you need > > > to consider carefully what ifdef you need in drivers/firmware/psci.c. > > > > > > This is why I think fixing this is not simple as it first looks. > > > > Not saying it is nice, but unless I find a cleaner way I was keener on > > adding a silent config entry in drivers/firmware, say: > > > > config ARM_PSCI_CPU_IDLE > > def_bool ARM_PSCI_FW && CPU_IDLE > > select ARM_CPU_SUSPEND if ARM > > > > and use that to either guard the code or stub it out and compile it > > if that config option is enabled. > > That immediately worries me, because it bypasses the CPU dependencies > for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE. I'd > prefer instead: > > config ARM_PSCI_CPU_IDLE > def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND) If you are not against it, I could make ARM_PSCI select ARM_CPU_SUSPEND, the CPU dependency would be taken into account (ie CPU_V7) and this would mirror what's done for the eg BL_SWITCHER. This would remove the need for ARM_PSCI_CPU_IDLE above altogether and I could guard the code in drivers/firmware/psci.c through CONFIG_CPU_IDLE. It is just an option, please let me know. Thanks, Lorenzo
On Wed, Jan 06, 2016 at 04:55:45PM +0000, Lorenzo Pieralisi wrote: > On Tue, Jan 05, 2016 at 01:34:47PM +0000, Russell King - ARM Linux wrote: > > That immediately worries me, because it bypasses the CPU dependencies > > for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE. I'd > > prefer instead: > > > > config ARM_PSCI_CPU_IDLE > > def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND) > > If you are not against it, I could make ARM_PSCI select ARM_CPU_SUSPEND, > the CPU dependency would be taken into account (ie CPU_V7) and this > would mirror what's done for the eg BL_SWITCHER. If you're proposing to always build the code in psci.c when ARM_PSCI_FW is enabled, I'd rather do this: config ARM_CPU_SUSPEND def_bool PM_SLEEP || BL_SWITCHER || ARM_PSCI_FW depends on ARCH_SUSPEND_POSSIBLE rather than have stuff select this option.
On Wed, Jan 06, 2016 at 09:44:39PM +0000, Russell King - ARM Linux wrote: > On Wed, Jan 06, 2016 at 04:55:45PM +0000, Lorenzo Pieralisi wrote: > > On Tue, Jan 05, 2016 at 01:34:47PM +0000, Russell King - ARM Linux wrote: > > > That immediately worries me, because it bypasses the CPU dependencies > > > for ARM_CPU_SUSPEND implicitly applied via ARCH_SUSPEND_POSSIBLE. I'd > > > prefer instead: > > > > > > config ARM_PSCI_CPU_IDLE > > > def_bool ARM_PSCI_FW && CPU_IDLE && (!ARM || ARM_CPU_SUSPEND) > > > > If you are not against it, I could make ARM_PSCI select ARM_CPU_SUSPEND, > > the CPU dependency would be taken into account (ie CPU_V7) and this > > would mirror what's done for the eg BL_SWITCHER. > > If you're proposing to always build the code in psci.c when ARM_PSCI_FW > is enabled, I'd rather do this: > > config ARM_CPU_SUSPEND > def_bool PM_SLEEP || BL_SWITCHER || ARM_PSCI_FW > depends on ARCH_SUSPEND_POSSIBLE > > rather than have stuff select this option. Agreed, I will do that with two patches (ie one to update the BL_SWITCHER config entry). It has the side effect of pulling in ARM_CPU_SUSPEND even if !SUSPEND && !CPU_IDLE when ARM_PSCI is selected but I do not think that's a real issue, to be confirmed. Still, some ARM CPUidle drivers will have to select ARM_CPU_SUSPEND on a case by case basis, I do not know how we can improve that, but that's not related to this patch series per-se. Thanks, Lorenzo
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index f67f35b..42816be 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -20,7 +20,6 @@ #include <linux/smp.h> #include <linux/delay.h> #include <linux/psci.h> -#include <linux/slab.h> #include <uapi/linux/psci.h> @@ -28,73 +27,6 @@ #include <asm/cpu_ops.h> #include <asm/errno.h> #include <asm/smp_plat.h> -#include <asm/suspend.h> - -static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state); - -static int __maybe_unused cpu_psci_cpu_init_idle(unsigned int cpu) -{ - int i, ret, count = 0; - u32 *psci_states; - struct device_node *state_node, *cpu_node; - - cpu_node = of_get_cpu_node(cpu, NULL); - if (!cpu_node) - return -ENODEV; - - /* - * If the PSCI cpu_suspend function hook has not been initialized - * idle states must not be enabled, so bail out - */ - if (!psci_ops.cpu_suspend) - return -EOPNOTSUPP; - - /* Count idle states */ - while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states", - count))) { - count++; - of_node_put(state_node); - } - - if (!count) - return -ENODEV; - - psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL); - if (!psci_states) - return -ENOMEM; - - for (i = 0; i < count; i++) { - u32 state; - - state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i); - - ret = of_property_read_u32(state_node, - "arm,psci-suspend-param", - &state); - if (ret) { - pr_warn(" * %s missing arm,psci-suspend-param property\n", - state_node->full_name); - of_node_put(state_node); - goto free_mem; - } - - of_node_put(state_node); - pr_debug("psci-power-state %#x index %d\n", state, i); - if (!psci_power_state_is_valid(state)) { - pr_warn("Invalid PSCI power state %#x\n", state); - ret = -EINVAL; - goto free_mem; - } - psci_states[i] = state; - } - /* Idle states parsed correctly, initialize per-cpu pointer */ - per_cpu(psci_power_state, cpu) = psci_states; - return 0; - -free_mem: - kfree(psci_states); - return ret; -} static int __init cpu_psci_cpu_init(unsigned int cpu) { @@ -178,38 +110,11 @@ static int cpu_psci_cpu_kill(unsigned int cpu) } #endif -static int psci_suspend_finisher(unsigned long index) -{ - u32 *state = __this_cpu_read(psci_power_state); - - return psci_ops.cpu_suspend(state[index - 1], - virt_to_phys(cpu_resume)); -} - -static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index) -{ - int ret; - u32 *state = __this_cpu_read(psci_power_state); - /* - * idle state index 0 corresponds to wfi, should never be called - * from the cpu_suspend operations - */ - if (WARN_ON_ONCE(!index)) - return -EINVAL; - - if (!psci_power_state_loses_context(state[index - 1])) - ret = psci_ops.cpu_suspend(state[index - 1], 0); - else - ret = cpu_suspend(index, psci_suspend_finisher); - - return ret; -} - const struct cpu_operations cpu_psci_ops = { .name = "psci", #ifdef CONFIG_CPU_IDLE - .cpu_init_idle = cpu_psci_cpu_init_idle, - .cpu_suspend = cpu_psci_cpu_suspend, + .cpu_init_idle = psci_cpu_init_idle, + .cpu_suspend = psci_cpu_suspend_enter, #endif .cpu_init = cpu_psci_cpu_init, .cpu_prepare = cpu_psci_cpu_prepare, diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index 492db42..73046de 100644 --- a/drivers/firmware/psci.c +++ b/drivers/firmware/psci.c @@ -13,17 +13,21 @@ #define pr_fmt(fmt) "psci: " fmt +#include <linux/cpuidle.h> #include <linux/errno.h> #include <linux/linkage.h> #include <linux/of.h> +#include <linux/percpu.h> #include <linux/pm.h> #include <linux/printk.h> #include <linux/psci.h> #include <linux/reboot.h> +#include <linux/slab.h> #include <linux/suspend.h> #include <uapi/linux/psci.h> +#include <asm/cpuidle.h> #include <asm/cputype.h> #include <asm/system_misc.h> #include <asm/smp_plat.h> @@ -225,6 +229,121 @@ static int __init psci_features(u32 psci_func_id) psci_func_id, 0, 0); } +static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state); + +static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu) +{ + int i, ret, count = 0; + u32 *psci_states; + struct device_node *state_node; + + /* + * If the PSCI cpu_suspend function hook has not been initialized + * idle states must not be enabled, so bail out + */ + if (!psci_ops.cpu_suspend) + return -EOPNOTSUPP; + + /* Count idle states */ + while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states", + count))) { + count++; + of_node_put(state_node); + } + + if (!count) + return -ENODEV; + + psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL); + if (!psci_states) + return -ENOMEM; + + for (i = 0; i < count; i++) { + u32 state; + + state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i); + + ret = of_property_read_u32(state_node, + "arm,psci-suspend-param", + &state); + if (ret) { + pr_warn(" * %s missing arm,psci-suspend-param property\n", + state_node->full_name); + of_node_put(state_node); + goto free_mem; + } + + of_node_put(state_node); + pr_debug("psci-power-state %#x index %d\n", state, i); + if (!psci_power_state_is_valid(state)) { + pr_warn("Invalid PSCI power state %#x\n", state); + ret = -EINVAL; + goto free_mem; + } + psci_states[i] = state; + } + /* Idle states parsed correctly, initialize per-cpu pointer */ + per_cpu(psci_power_state, cpu) = psci_states; + return 0; + +free_mem: + kfree(psci_states); + return ret; +} + +int psci_cpu_init_idle(unsigned int cpu) +{ + struct device_node *cpu_node; + int ret; + + cpu_node = of_get_cpu_node(cpu, NULL); + if (!cpu_node) + return -ENODEV; + + ret = psci_dt_cpu_init_idle(cpu_node, cpu); + + of_node_put(cpu_node); + + return ret; +} + +static int psci_suspend_finisher(unsigned long index) +{ + u32 *state = __this_cpu_read(psci_power_state); + + return psci_ops.cpu_suspend(state[index - 1], + virt_to_phys(cpu_resume)); +} + +int psci_cpu_suspend_enter(unsigned long index) +{ + int ret; + u32 *state = __this_cpu_read(psci_power_state); + /* + * idle state index 0 corresponds to wfi, should never be called + * from the cpu_suspend operations + */ + if (WARN_ON_ONCE(!index)) + return -EINVAL; + + if (!psci_power_state_loses_context(state[index - 1])) + ret = psci_ops.cpu_suspend(state[index - 1], 0); + else + ret = cpu_suspend(index, psci_suspend_finisher); + + return ret; +} + +/* ARM specific CPU idle operations */ +#ifdef CONFIG_ARM +static struct cpuidle_ops psci_cpuidle_ops __initdata = { + .suspend = psci_cpu_suspend_enter, + .init = psci_dt_cpu_init_idle, +}; + +CPUIDLE_METHOD_OF_DECLARE(psci, "arm,psci", &psci_cpuidle_ops); +#endif + static int psci_system_suspend(unsigned long unused) { return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND), diff --git a/include/linux/psci.h b/include/linux/psci.h index 12c4865..393efe2 100644 --- a/include/linux/psci.h +++ b/include/linux/psci.h @@ -24,6 +24,9 @@ bool psci_tos_resident_on(int cpu); bool psci_power_state_loses_context(u32 state); bool psci_power_state_is_valid(u32 state); +int psci_cpu_init_idle(unsigned int cpu); +int psci_cpu_suspend_enter(unsigned long index); + struct psci_operations { int (*cpu_suspend)(u32 state, unsigned long entry_point); int (*cpu_off)(u32 state);