On Fri, Jun 10, 2016 at 01:50:28PM +0100, Lorenzo Pieralisi wrote:
> [+ Daniel, Kevin]
>
> On Wed, May 11, 2016 at 04:37:41PM +0100, Sudeep Holla wrote:
> > This patch adds appropriate callbacks to support ACPI Low Power Idle
> > (LPI) on ARM64.
> >
> > Cc: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
> > Cc: Mark Rutland <mark.rutl...@arm.com>
> > Cc: linux-arm-ker...@lists.infradead.org
> > Signed-off-by: Sudeep Holla <sudeep.ho...@arm.com>
> > ---
[ ... ]
> > +#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT BIT(0)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT BIT(1)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT BIT(2)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT BIT(3)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT \
> > + (ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT | \
> > + ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT | \
> > + ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT | \
> > + ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
> > +
> > +struct acpi_lpi_state *lpi;
> > +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx)
> > +{
> > + int ret = 0;
> > + bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;
>
> I am not really that keen on this, as you know. Those flags are
> there to say "save these components registers". I see the CPU PM
> notifiers as a way to save/restore CPU peripheral state, but
> they should *not* carry out any action that affects the power
> state itself, that's down to the suspend finisher (eg PSCI),
> because that's where the specific idle states are managed.
>
> I agree we have no clue whatsoever on what we *really* need
> to save/restore, but that's orthogonal to what you are solving
> here.
>
> See eg gic_cpu_if_down(). Do we call it from the GIC CPU PM notifier ?
> No. We should not handle the same problem differently.
>
> On top of that, we have no way to solve this problem for DT,
> all I am saying is that it is ill-defined and given that LPI
> is new I'd rather we got it right from the beginning.
>
> I am open to suggestions here.
There is a part of the idle state flags integer which is reserved for the
arch specific flag and can be masked with:
CPUIDLE_DRIVER_FLAGS_MASK()
May be these context flags can be added in the generic cpuidle driver and
reused.
Concerning the DT, why not use the power domains to tell which context to
save ? yeah, probably mentionned n-th times :)
> > +
> > + if (!idx) {
> > + cpu_do_idle();
> > + return idx;
> > + }
> > +
> > + /* TODO cpu_pm_{enter,exit} can be done in generic code ? */
> > + if (save_ctx)
> > + ret = cpu_pm_enter();
> > + if (!ret) {
> > + /*
> > + * Pass idle state index to cpu_suspend which in turn will
> > + * call the CPU ops suspend protocol with idle index as a
> > + * parameter.
> > + */
> > + ret = arm_cpuidle_suspend(idx);
> > +
> > + if (save_ctx)
> > + cpu_pm_exit();
> > + }
> > +
> > + return ret ? -1 : idx;
>
> The body of this function (if we remove save_ctx) is identical
> to arm_enter_idle_state(), it would be nice if we found a way
> where to put this code and share it with the ARM CPUidle driver,
+1
We don't want to redo another unmaintable acpi idle driver.
-- Daniel
