Enabling PSCI based idle on ARM 32-bit platforms

The suspend() hook in the cpuidle_ops struct is always called on the cpu entering idle, which means that the cpu parameter passed to the suspend hook always corresponds to the local cpu, making it somewhat redundant.

This patch removes the logical cpu parameter from the ARM cpuidle_ops.suspend hook and updates all the existing kernel implementations to reflect this change.

Signed-off-by Lorenzo Pieralisi <[email protected]>

Reviewed-by Lina Iyer <[email protected]>

Tested-by Lina Iyer <[email protected]>

Tested-by Jisheng Zhang <[email protected]> [psci]

Cc Lina Iyer <[email protected]>

Cc Russell King <[email protected]>

Cc Daniel Lezcano <[email protected]>

---

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 <[email protected]>

Acked-by Catalin Marinas <[email protected]> [arch/arm64]

Acked-by Mark Rutland <[email protected]>

Tested-by Jisheng Zhang <[email protected]>

Cc Will Deacon <[email protected]>

Cc Sudeep Holla <[email protected]>

Cc Russell King <[email protected]>

Cc Daniel Lezcano <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Jisheng Zhang <[email protected]>

---

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.

Quoted 17 lines · Lorenzo Pieralisi

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.

Quoted 1 line

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.

Quoted 34 lines · Russell King - ARM Linux

Quoted 17 lines · Lorenzo Pieralisi

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.

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 16 lines

Quoted 16 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 17 lines · Russell King - ARM Linux

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)

Quoted 5 lines

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.

Quoted 27 lines · Lorenzo Pieralisi

Quoted 17 lines · Russell King - ARM Linux

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)

Quoted 5 lines

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:

Quoted 6 lines

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.

Quoted 27 lines · Lorenzo Pieralisi

Quoted 17 lines · Russell King - ARM Linux

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)

Quoted 5 lines

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:

Quoted 6 lines

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)

Quoted 6 lines · Russell King - ARM Linux

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.

Quoted 11 lines · Lorenzo Pieralisi

Quoted 6 lines · Russell King - ARM Linux

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.