Re: [PATCH v8] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer

[Sudeep Holla]

On Mon, Oct 02, 2023 at 07:22:57PM +0000, Pawandeep Oza (QUIC) wrote:
>
>
> -----Original Message-----
> From: Will Deacon <will@xxxxxxxxxx>
> Sent: Friday, September 29, 2023 8:05 AM
> To: Pawandeep Oza (QUIC) <quic_poza@xxxxxxxxxxx>
> Cc: sudeep.holla@xxxxxxx; catalin.marinas@xxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v8] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer
>
> On Mon, Sep 18, 2023 at 10:21:40AM -0700, Oza Pawandeep wrote:
> > Arm(r) Functional Fixed Hardware Specification defines LPI states, which
> > provide an architectural context loss flags field that can be used to
> > describe the context that might be lost when an LPI state is entered.
> >
> > - Core context Lost
> >         - General purpose registers.
> >         - Floating point and SIMD registers.
> >         - System registers, include the System register based
> >         - generic timer for the core.
> >         - Debug register in the core power domain.
> >         - PMU registers in the core power domain.
> >         - Trace register in the core power domain.
> > - Trace context loss
> > - GICR
> > - GICD
> >
> > Qualcomm's custom CPUs preserves the architectural state, including
> > keeping the power domain for local timers active.
> > when core is power gated, the local timers are sufficient to wake the
> > core up without needing broadcast timer.
> >
> > The patch fixes the evaluation of cpuidle arch_flags, and moves only
> > to broadcast timer if core context lost is defined in ACPI LPI.
> >
> > Fixes: a36a7fecfe607 ("Add support for Low Power Idle(LPI) states")
> > Reviewed-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> > Acked-by: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> > Signed-off-by: Oza Pawandeep <quic_poza@xxxxxxxxxxx>
> > ---
> > diff --git a/drivers/acpi/processor_idle.c
> > b/drivers/acpi/processor_idle.c index dc615ef6550a..5c1d13eecdd1
> > 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -1217,8 +1217,7 @@ static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
> >  		strscpy(state->desc, lpi->desc, CPUIDLE_DESC_LEN);
> >  		state->exit_latency = lpi->wake_latency;
> >  		state->target_residency = lpi->min_residency;
> > -		if (lpi->arch_flags)
> > -			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> > +		arch_update_idle_state_flags(lpi->arch_flags, &state->flags);
>
> Hmm, I know Rafael has Acked this, but I think this is pretending to be more
> generic than it really is. While passing in a pointer to the flags field
> allows the arch code to set and clear arbitrary flags, we're calling this
> before we've set CPUIDLE_FLAG_RCU_IDLE, so that cannot be changed.
>
> Why not just name it like it is and return the arch flags directly:
>
> 	state->flags |= arch_get_idle_state_flags(lpi->arch_flags);
>
> Oza:
>
> ?

Not sure if this "?" is by mistake or you didn't follow Will's comment.

The point made was that it is cleaner for arch code to just provide the
flags that needs to be set via some helper like 'arch_get_idle_state_flags()'
rather than set/update itself via 'arch_update_idle_state_flags()' like
you have in this patch.

I completely agree with Will as this is much cleaner and arch code just
returns the requested flags and the core code is still in charge to update
the flags.

--
Regards,
Sudeep