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

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

[Oza Pawandeep]

Arm® 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@arm.com>
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Oza Pawandeep <quic_poza@quicinc.com>
---

Notes:
    Will/Catalin: Rafael has acked and he prefers to take it via arm64 tree

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 4d537d56eb84..269d21209723 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -9,6 +9,7 @@
 #ifndef _ASM_ACPI_H
 #define _ASM_ACPI_H
 
+#include <linux/cpuidle.h>
 #include <linux/efi.h>
 #include <linux/memblock.h>
 #include <linux/psci.h>
@@ -44,6 +45,23 @@
 
 #define ACPI_MADT_GICC_TRBE  (offsetof(struct acpi_madt_generic_interrupt, \
 	trbe_interrupt) + sizeof(u16))
+/*
+ * Arm® Functional Fixed Hardware Specification Version 1.2.
+ * Table 2: Arm Architecture context loss flags
+ */
+#define CPUIDLE_CORE_CTXT		BIT(0) /* Core context Lost */
+
+static __always_inline void _arch_update_idle_state_flags(u32 arch_flags,
+							unsigned int *sflags)
+{
+	if (arch_flags & CPUIDLE_CORE_CTXT)
+		*sflags |= CPUIDLE_FLAG_TIMER_STOP;
+}
+#define arch_update_idle_state_flags _arch_update_idle_state_flags
+
+#define CPUIDLE_TRACE_CTXT		BIT(1) /* Trace context loss */
+#define CPUIDLE_GICR_CTXT		BIT(2) /* GICR */
+#define CPUIDLE_GICD_CTXT		BIT(3) /* GICD */
 
 /* Basic configuration for ACPI */
 #ifdef	CONFIG_ACPI
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);
 		if (i != 0 && lpi->entry_method == ACPI_CSTATE_FFH)
 			state->flags |= CPUIDLE_FLAG_RCU_IDLE;
 		state->enter = acpi_idle_lpi_enter;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a73246c3c35e..07a825c76bab 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1480,6 +1480,12 @@ static inline int lpit_read_residency_count_address(u64 *address)
 }
 #endif
 
+#ifdef CONFIG_ACPI_PROCESSOR_IDLE
+#ifndef arch_update_idle_state_flags
+#define arch_update_idle_state_flags(af, sf)	do {} while (0)
+#endif
+#endif /* CONFIG_ACPI_PROCESSOR_IDLE */
+
 #ifdef CONFIG_ACPI_PPTT
 int acpi_pptt_cpu_is_thread(unsigned int cpu);
 int find_acpi_cpu_topology(unsigned int cpu, int level);
-- 
2.25.1

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

[Will Deacon]

On Mon, Sep 18, 2023 at 10:21:40AM -0700, Oza Pawandeep wrote:
> Arm® 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@arm.com>
> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Oza Pawandeep <quic_poza@quicinc.com>
> ---
> 
> Notes:
>     Will/Catalin: Rafael has acked and he prefers to take it via arm64 tree
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 4d537d56eb84..269d21209723 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -9,6 +9,7 @@
>  #ifndef _ASM_ACPI_H
>  #define _ASM_ACPI_H
>  
> +#include <linux/cpuidle.h>
>  #include <linux/efi.h>
>  #include <linux/memblock.h>
>  #include <linux/psci.h>
> @@ -44,6 +45,23 @@
>  
>  #define ACPI_MADT_GICC_TRBE  (offsetof(struct acpi_madt_generic_interrupt, \
>  	trbe_interrupt) + sizeof(u16))
> +/*
> + * Arm® Functional Fixed Hardware Specification Version 1.2.
> + * Table 2: Arm Architecture context loss flags
> + */
> +#define CPUIDLE_CORE_CTXT		BIT(0) /* Core context Lost */
> +
> +static __always_inline void _arch_update_idle_state_flags(u32 arch_flags,
> +							unsigned int *sflags)

Why can't this just be 'static inline'?

> +{
> +	if (arch_flags & CPUIDLE_CORE_CTXT)
> +		*sflags |= CPUIDLE_FLAG_TIMER_STOP;
> +}
> +#define arch_update_idle_state_flags _arch_update_idle_state_flags

Usually, the function and the macro have the same name for this pattern,
so I think it would be more consistent to drop the leading underscore
from the C function name.

> +
> +#define CPUIDLE_TRACE_CTXT		BIT(1) /* Trace context loss */
> +#define CPUIDLE_GICR_CTXT		BIT(2) /* GICR */
> +#define CPUIDLE_GICD_CTXT		BIT(3) /* GICD */
>  
>  /* Basic configuration for ACPI */
>  #ifdef	CONFIG_ACPI
> 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);

?

>  		if (i != 0 && lpi->entry_method == ACPI_CSTATE_FFH)
>  			state->flags |= CPUIDLE_FLAG_RCU_IDLE;
>  		state->enter = acpi_idle_lpi_enter;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a73246c3c35e..07a825c76bab 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1480,6 +1480,12 @@ static inline int lpit_read_residency_count_address(u64 *address)
>  }
>  #endif
>  
> +#ifdef CONFIG_ACPI_PROCESSOR_IDLE
> +#ifndef arch_update_idle_state_flags
> +#define arch_update_idle_state_flags(af, sf)	do {} while (0)

I'd prefer defining this to point at an empty static inline function so
that we get evaluation and type-checking of the arguments.

> +#endif
> +#endif /* CONFIG_ACPI_PROCESSOR_IDLE */

Why do you need the outer CONFIG_ guards here?

Will

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

[Sudeep Holla]

On Fri, Sep 29, 2023 at 04:04:59PM +0100, Will Deacon wrote:
> On Mon, Sep 18, 2023 at 10:21:40AM -0700, Oza Pawandeep wrote:
> > Arm® 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@arm.com>
> > Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> > Signed-off-by: Oza Pawandeep <quic_poza@quicinc.com>
> > ---
> > 
> > Notes:
> >     Will/Catalin: Rafael has acked and he prefers to take it via arm64 tree
> > 
> > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> > index 4d537d56eb84..269d21209723 100644
> > --- a/arch/arm64/include/asm/acpi.h
> > +++ b/arch/arm64/include/asm/acpi.h
> > @@ -9,6 +9,7 @@
> >  #ifndef _ASM_ACPI_H
> >  #define _ASM_ACPI_H
> >  
> > +#include <linux/cpuidle.h>
> >  #include <linux/efi.h>
> >  #include <linux/memblock.h>
> >  #include <linux/psci.h>
> > @@ -44,6 +45,23 @@
> >  
> >  #define ACPI_MADT_GICC_TRBE  (offsetof(struct acpi_madt_generic_interrupt, \
> >  	trbe_interrupt) + sizeof(u16))
> > +/*
> > + * Arm® Functional Fixed Hardware Specification Version 1.2.
> > + * Table 2: Arm Architecture context loss flags
> > + */
> > +#define CPUIDLE_CORE_CTXT		BIT(0) /* Core context Lost */
> > +
> > +static __always_inline void _arch_update_idle_state_flags(u32 arch_flags,
> > +							unsigned int *sflags)
> 
> Why can't this just be 'static inline'?
> 
> > +{
> > +	if (arch_flags & CPUIDLE_CORE_CTXT)
> > +		*sflags |= CPUIDLE_FLAG_TIMER_STOP;
> > +}
> > +#define arch_update_idle_state_flags _arch_update_idle_state_flags
> 
> Usually, the function and the macro have the same name for this pattern,
> so I think it would be more consistent to drop the leading underscore
> from the C function name.
>

Sorry that's me telling him looking at some other example I think. I don't
have a strong opinion, just referred examples doing this way I guess.

Oza, please check it to as it was before I requested this.

--
Regards,
Sudeep

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

[Pawandeep Oza (QUIC)]

-----Original Message-----
From: Will Deacon <will@kernel.org> 
Sent: Friday, September 29, 2023 8:05 AM
To: Pawandeep Oza (QUIC) <quic_poza@quicinc.com>
Cc: sudeep.holla@arm.com; catalin.marinas@arm.com; rafael@kernel.org; lenb@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
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@arm.com>
> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Oza Pawandeep <quic_poza@quicinc.com>
> ---
> 
> Notes:
>     Will/Catalin: Rafael has acked and he prefers to take it via arm64 
> tree
> 
> diff --git a/arch/arm64/include/asm/acpi.h 
> b/arch/arm64/include/asm/acpi.h index 4d537d56eb84..269d21209723 
> 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -9,6 +9,7 @@
>  #ifndef _ASM_ACPI_H
>  #define _ASM_ACPI_H
>  
> +#include <linux/cpuidle.h>
>  #include <linux/efi.h>
>  #include <linux/memblock.h>
>  #include <linux/psci.h>
> @@ -44,6 +45,23 @@
>  
>  #define ACPI_MADT_GICC_TRBE  (offsetof(struct acpi_madt_generic_interrupt, \
>  	trbe_interrupt) + sizeof(u16))
> +/*
> + * Arm(r) Functional Fixed Hardware Specification Version 1.2.
> + * Table 2: Arm Architecture context loss flags  */
> +#define CPUIDLE_CORE_CTXT		BIT(0) /* Core context Lost */
> +
> +static __always_inline void _arch_update_idle_state_flags(u32 arch_flags,
> +							unsigned int *sflags)

Why can't this just be 'static inline'?

Oza: sure, will let compiler decide.

> +{
> +	if (arch_flags & CPUIDLE_CORE_CTXT)
> +		*sflags |= CPUIDLE_FLAG_TIMER_STOP; } #define 
> +arch_update_idle_state_flags _arch_update_idle_state_flags

Usually, the function and the macro have the same name for this pattern, so I think it would be more consistent to drop the leading underscore from the C function name.

Oza: sure

> +
> +#define CPUIDLE_TRACE_CTXT		BIT(1) /* Trace context loss */
> +#define CPUIDLE_GICR_CTXT		BIT(2) /* GICR */
> +#define CPUIDLE_GICD_CTXT		BIT(3) /* GICD */
>  
>  /* Basic configuration for ACPI */
>  #ifdef	CONFIG_ACPI
> 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: 

?

>  		if (i != 0 && lpi->entry_method == ACPI_CSTATE_FFH)
>  			state->flags |= CPUIDLE_FLAG_RCU_IDLE;
>  		state->enter = acpi_idle_lpi_enter; diff --git 
> a/include/linux/acpi.h b/include/linux/acpi.h index 
> a73246c3c35e..07a825c76bab 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1480,6 +1480,12 @@ static inline int 
> lpit_read_residency_count_address(u64 *address)  }  #endif
>  
> +#ifdef CONFIG_ACPI_PROCESSOR_IDLE
> +#ifndef arch_update_idle_state_flags
> +#define arch_update_idle_state_flags(af, sf)	do {} while (0)

I'd prefer defining this to point at an empty static inline function so that we get evaluation and type-checking of the arguments.

Oza: sure

> +#endif
> +#endif /* CONFIG_ACPI_PROCESSOR_IDLE */

Why do you need the outer CONFIG_ guards here?

Oza: this is because of non-ACPI kernel build issue for this config: https://download.01.org/0day-ci/archive/20230915/202309151138.69mFCPtW-lkp@intel.com/config
Throwing following

" 
All warnings (new ones prefixed by >>):

   In file included from arch/arm64/kernel/setup.c:36:
>> arch/arm64/include/asm/acpi.h:60: warning: 
>> "arch_update_idle_state_flags" redefined
      60 | #define arch_update_idle_state_flags _arch_update_idle_state_flags
         | 
   In file included from arch/arm64/kernel/setup.c:9:
   include/linux/acpi.h:1484: note: this is the location of the previous definition
    1484 | #define arch_update_idle_state_flags(af, sf)    do {} while (0)
         |
"

Will

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@kernel.org>
> Sent: Friday, September 29, 2023 8:05 AM
> To: Pawandeep Oza (QUIC) <quic_poza@quicinc.com>
> Cc: sudeep.holla@arm.com; catalin.marinas@arm.com; rafael@kernel.org; lenb@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
> 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@arm.com>
> > Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> > Signed-off-by: Oza Pawandeep <quic_poza@quicinc.com>
> > ---
> > 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

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

[Pawandeep Oza (QUIC)]

-----Original Message-----
From: Sudeep Holla <sudeep.holla@arm.com> 
Sent: Tuesday, October 3, 2023 2:46 AM
To: Pawandeep Oza (QUIC) <quic_poza@quicinc.com>
Cc: 'Will Deacon' <will@kernel.org>; Sudeep Holla <sudeep.holla@arm.com>; catalin.marinas@arm.com; rafael@kernel.org; lenb@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
Subject: Re: [PATCH v8] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer

On Mon, Oct 02, 2023 at 07:22:57PM +0000, Pawandeep Oza (QUIC) wrote:
>
>
> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: Friday, September 29, 2023 8:05 AM
> To: Pawandeep Oza (QUIC) <quic_poza@quicinc.com>
> Cc: sudeep.holla@arm.com; catalin.marinas@arm.com; rafael@kernel.org; 
> lenb@kernel.org; linux-arm-kernel@lists.infradead.org; 
> linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
> 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@arm.com>
> > Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> > Signed-off-by: Oza Pawandeep <quic_poza@quicinc.com>
> > ---
> > 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.

Oza: oh ! sorry about that, it was some mix-up with the reply to another comment from will where I wanted to point out kernel test robot results. So not sure what happened there.
This comment is already taken care in the patch now. Changed to 'arch_get_idle_state_flags'

--
Regards,
Sudeep