diff mbox

[v3,2/2] ARM64: kernel: PSCI: move PSCI idle management code to drivers/firmware

Message ID 1445011379-8787-3-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lorenzo Pieralisi Oct. 16, 2015, 4:02 p.m. UTC
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>
---
 arch/arm64/kernel/psci.c |  99 +--------------------------------------
 drivers/firmware/psci.c  | 119 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/psci.h     |   3 ++
 3 files changed, 124 insertions(+), 97 deletions(-)

Comments

Daniel Lezcano Dec. 16, 2015, 8:57 p.m. UTC | #1
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>
Russell King - ARM Linux Jan. 5, 2016, 10:59 a.m. UTC | #2
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.
Lorenzo Pieralisi Jan. 5, 2016, 12:31 p.m. UTC | #3
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
Russell King - ARM Linux Jan. 5, 2016, 12:51 p.m. UTC | #4
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.
Lorenzo Pieralisi Jan. 5, 2016, 1:27 p.m. UTC | #5
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
Russell King - ARM Linux Jan. 5, 2016, 1:34 p.m. UTC | #6
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.
Lorenzo Pieralisi Jan. 5, 2016, 3:28 p.m. UTC | #7
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
Lorenzo Pieralisi Jan. 6, 2016, 4:55 p.m. UTC | #8
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
Russell King - ARM Linux Jan. 6, 2016, 9:44 p.m. UTC | #9
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.
Lorenzo Pieralisi Jan. 7, 2016, 9:46 a.m. UTC | #10
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 mbox

Patch

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);