Re: [PATCH 4/6] ARM: psci: cpuidle: Introduce PSCI CPUidle driver

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Tue, Aug 06, 2019 at 05:10:33PM +0100, Sudeep Holla wrote:
> On Mon, Jul 22, 2019 at 04:37:43PM +0100, Lorenzo Pieralisi wrote:
> > PSCI firmware is the standard power management control for
> > all ARM64 based platforms and it is also deployed on some
> > ARM 32 bit platforms to date.
> >
> > Idle state entry in PSCI is currently achieved by calling
> > arm_cpuidle_init() and arm_cpuidle_suspend() in a generic
> > idle driver, which in turn relies on ARM/ARM64 CPUidle back-end
> > to relay the call into PSCI firmware if PSCI is the boot method.
> >
> > Given that PSCI is the standard idle entry method on ARM64 systems
> > (which means that no other CPUidle driver are expected on ARM64
> > platforms - so PSCI is already a generic idle driver), in order to
> > simplify idle entry and code maintenance, it makes sense to have a PSCI
> > specific idle driver so that idle code that it is currently living in
> > drivers/firmware directory can be hoisted out of it and moved
> > where it belongs, into a full-fledged PSCI driver, leaving PSCI code
> > in drivers/firmware as a pure firmware interface, as it should be.
> >
> > Implement a PSCI CPUidle driver. By default it is a silent Kconfig entry
> > which is left unselected, since it selection would clash with the
> > generic ARM CPUidle driver that provides a PSCI based idle driver
> > through the arm/arm64 arches back-ends CPU operations.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> 
> Once the error path issues pointed by Ulf are resolved,
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > ---
> >  MAINTAINERS                    |   8 ++
> >  drivers/cpuidle/Kconfig.arm    |   3 +
> >  drivers/cpuidle/Makefile       |   1 +
> >  drivers/cpuidle/cpuidle-psci.c | 150 +++++++++++++++++++++++++++++++++
> >  4 files changed, 162 insertions(+)
> >  create mode 100644 drivers/cpuidle/cpuidle-psci.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 783569e3c4b4..c2bf8ce65e83 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4286,6 +4286,14 @@ S:	Supported
> >  F:	drivers/cpuidle/cpuidle-exynos.c
> >  F:	arch/arm/mach-exynos/pm.c
> >
> > +CPUIDLE DRIVER - ARM PSCI
> > +M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > +M:	Sudeep Holla <sudeep.holla@arm.com>
> > +L:	linux-pm@vger.kernel.org
> > +L:	linux-arm-kernel@lists.infradead.org
> > +S:	Supported
> > +F:	drivers/cpuidle/cpuidle-psci.c
> > +
> >  CPU IDLE TIME MANAGEMENT FRAMEWORK
> >  M:	"Rafael J. Wysocki" <rjw@rjwysocki.net>
> >  M:	Daniel Lezcano <daniel.lezcano@linaro.org>
> > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> > index 48cb3d4bb7d1..929b57424ea4 100644
> > --- a/drivers/cpuidle/Kconfig.arm
> > +++ b/drivers/cpuidle/Kconfig.arm
> > @@ -13,6 +13,9 @@ config ARM_CPUIDLE
> >            initialized by calling the CPU operations init idle hook
> >            provided by architecture code.
> >
> > +config ARM_PSCI_CPUIDLE
> > +	bool
> > +
> 
> [nit] I understand the intention to keep it hidden, but can't we have
> the dependency and selection of other config as part of this patch to
> make it more complete ?

Yes we can, it makes sense.

Thanks,
Lorenzo