Re: [PATCH 0/6] ARM: psci: cpuidle: PSCI CPUidle rework

From: Ulf Hansson
Date: Tue Jul 23 2019 - 07:49:55 EST


On Mon, 22 Jul 2019 at 17:37, Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> wrote:
>
> Current PSCI CPUidle driver is built on top of the generic ARM
> CPUidle infrastructure that relies on the architectural back-end
> idle operations to initialize and enter idle states.
>
> On ARM64 systems, PSCI is the only interface the kernel ever uses
> to enter idle states, so, having to rely on a generic ARM CPUidle
> driver when there is and there will always be only one method
> for entering idle states proved to be overkill, more so given
> that on ARM 32-bit systems (that can also enable the generic
> ARM CPUidle driver) only one additional idle back-end was
> ever added:
>
> drivers/soc/qcom/spm.c
>
> and it can be easily converted to a full-fledged CPUidle driver
> without requiring the generic ARM CPUidle framework.
>
> Furthermore, the generic ARM CPUidle infrastructure forces the
> PSCI firmware layer to keep CPUidle specific information in it,
> which does not really fit its purpose that should be kernel
> control/data structure agnostic.
>
> Lastly, the interface between the generic ARM CPUidle driver and
> the arch back-end requires an idle state index to be passed to
> suspend operations, with idle states back-end internals (such
> as idle state parameters) hidden in architectural back-ends and
> not available to the generic ARM CPUidle driver.
>
> To improve the above mentioned shortcomings, implement a stand
> alone PSCI CPUidle driver; this improves the current kernel
> code from several perspective:
>
> - Move CPUidle internal knowledge into CPUidle driver out of
> the PSCI firmware interface
> - Give the PSCI CPUidle driver control over power state parameters,
> in particular in preparation for PSCI OSI support
> - Remove generic CPUidle operations infrastructure from the kernel
>
> This patchset does not go as far as removing the generic ARM CPUidle
> infrastructure in order to collect feedback on the new approach
> before completing the removal from the kernel, the generic and PSCI
> CPUidle driver are left to co-exist.

I like the approach and I think this series definitely moves things in
the right direction.

Of course, some additional cleanups/re-works on top are needed to show
its full benefit, but step by step we reach that point.

>
> Tested on Juno platform with both DT and ACPI boot firmwares.
>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
>
> Lorenzo Pieralisi (6):
> ARM: cpuidle: Remove useless header include
> ARM: cpuidle: Remove overzealous error logging
> drivers: firmware: psci: Decouple checker from generic ARM CPUidle
> ARM: psci: cpuidle: Introduce PSCI CPUidle driver
> ARM: psci: cpuidle: Enable PSCI CPUidle driver
> PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
>
> MAINTAINERS | 8 +
> arch/arm/configs/imx_v6_v7_defconfig | 1 +
> arch/arm64/configs/defconfig | 1 +
> arch/arm64/kernel/cpuidle.c | 50 +++++-
> arch/arm64/kernel/psci.c | 4 -
> drivers/cpuidle/Kconfig.arm | 7 +
> drivers/cpuidle/Makefile | 1 +
> drivers/cpuidle/cpuidle-arm.c | 13 +-
> drivers/cpuidle/cpuidle-psci.c | 235 +++++++++++++++++++++++++++
> drivers/firmware/psci/psci.c | 167 +------------------
> drivers/firmware/psci/psci_checker.c | 16 +-
> include/linux/cpuidle.h | 17 +-
> include/linux/psci.h | 4 +-
> 13 files changed, 338 insertions(+), 186 deletions(-)
> create mode 100644 drivers/cpuidle/cpuidle-psci.c
>
> --
> 2.21.0
>

For the series, besides the minor comments I had on patch 4, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>

Kind regards
Uffe