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

[PATCH v3] 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.

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Oza Pawandeep <quic_poza@quicinc.com>

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index bd68e1b7f29f..0d455b02971e 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>
@@ -42,6 +43,26 @@
 #define ACPI_MADT_GICC_SPE  (offsetof(struct acpi_madt_generic_interrupt, \
 	spe_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 */
+
+#ifndef arch_update_idle_state_flags
+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
+#endif
+
+#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
 pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 9718d07cc2a2..420baec3465c 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1221,8 +1221,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 d584f94409e1..d49488fdbc49 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1471,6 +1471,10 @@ static inline int lpit_read_residency_count_address(u64 *address)
 }
 #endif
 
+#ifndef arch_update_idle_state_flags
+#define arch_update_idle_state_flags	do {} while (0);
+#endif
+
 #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 v3] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer

[kernel test robot]

Hi Oza,

kernel test robot noticed the following build warnings:

[auto build test WARNING on soc/for-next]
[also build test WARNING on v6.5]
[cannot apply to rafael-pm/linux-next arm64/for-next/core linus/master next-20230829]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Oza-Pawandeep/cpuidle-ACPI-Evaluate-LPI-arch_flags-for-broadcast-timer/20230830-041258
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link:    https://lore.kernel.org/r/20230829201101.3330337-1-quic_poza%40quicinc.com
patch subject: [PATCH v3] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20230830/202308300720.JEfxnfQ5-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230830/202308300720.JEfxnfQ5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308300720.JEfxnfQ5-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/acpi/processor_idle.c: In function 'acpi_processor_setup_lpi_states':
>> drivers/acpi/processor_idle.c:1220:61: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    1220 |                 arch_update_idle_state_flags(lpi->arch_flags, &state->flags);
         |                                                             ^
>> drivers/acpi/processor_idle.c:1220:61: warning: statement with no effect [-Wunused-value]
    1220 |                 arch_update_idle_state_flags(lpi->arch_flags, &state->flags);
         |                                             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~


vim +1220 drivers/acpi/processor_idle.c

  1201	
  1202	static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
  1203	{
  1204		int i;
  1205		struct acpi_lpi_state *lpi;
  1206		struct cpuidle_state *state;
  1207		struct cpuidle_driver *drv = &acpi_idle_driver;
  1208	
  1209		if (!pr->flags.has_lpi)
  1210			return -EOPNOTSUPP;
  1211	
  1212		for (i = 0; i < pr->power.count && i < CPUIDLE_STATE_MAX; i++) {
  1213			lpi = &pr->power.lpi_states[i];
  1214	
  1215			state = &drv->states[i];
  1216			snprintf(state->name, CPUIDLE_NAME_LEN, "LPI-%d", i);
  1217			strscpy(state->desc, lpi->desc, CPUIDLE_DESC_LEN);
  1218			state->exit_latency = lpi->wake_latency;
  1219			state->target_residency = lpi->min_residency;
> 1220			arch_update_idle_state_flags(lpi->arch_flags, &state->flags);
  1221			if (i != 0 && lpi->entry_method == ACPI_CSTATE_FFH)
  1222				state->flags |= CPUIDLE_FLAG_RCU_IDLE;
  1223			state->enter = acpi_idle_lpi_enter;
  1224			drv->safe_state_index = i;
  1225		}
  1226	
  1227		drv->state_count = i;
  1228	
  1229		return 0;
  1230	}
  1231	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Sudeep Holla]

On Tue, Aug 29, 2023 at 01:11:01PM -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.
> 
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Oza Pawandeep <quic_poza@quicinc.com>
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index bd68e1b7f29f..0d455b02971e 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>
> @@ -42,6 +43,26 @@
>  #define ACPI_MADT_GICC_SPE  (offsetof(struct acpi_madt_generic_interrupt, \
>  	spe_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 */
> +
> +#ifndef arch_update_idle_state_flags
> +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
> +#endif
> +
> +#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
>  pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 9718d07cc2a2..420baec3465c 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1221,8 +1221,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 d584f94409e1..d49488fdbc49 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1471,6 +1471,10 @@ static inline int lpit_read_residency_count_address(u64 *address)
>  }
>  #endif
>  
> +#ifndef arch_update_idle_state_flags
> +#define arch_update_idle_state_flags	do {} while (0);

I think I suggested it in v2 as without that you will get the warnings the
kernel build bot has posted now.

#define arch_update_idle_state_flags(af, sf)	do {} while (0)

Add the parameters and drop the leading ';'.

-- 
Regards,
Sudeep

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

[Marc Zyngier]

On 2023-08-29 21:11, 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.

Isn't that what should be exposed by GTDT when ACPI_GTDT_ALWAYS_ON
is set on the relevant interrupt and EL? The arch timer already
deals with that.

Why do we need anything else?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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

[Sudeep Holla]

On Wed, Aug 30, 2023 at 03:07:35PM +0100, Marc Zyngier wrote:
> On 2023-08-29 21:11, 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.
> 
> Isn't that what should be exposed by GTDT when ACPI_GTDT_ALWAYS_ON
> is set on the relevant interrupt and EL? The arch timer already
> deals with that.
> 
> Why do we need anything else?
> 

OK, let me try to write down my understanding here:

1. DT -> "always-on" property in the timer device node
   ACPI -> ACPI_GTDT_ALWAYS_ON flags in GDTD table

  The above 2 indicates if the timer is always on or will it stop in
  (deeper) lower idle states. This flag is used in the driver to set the
  clock source feature appropriate so that this clock source can be
  selected as broadcast timer or not.

2. DT-> "local-timer-stop" property in each idle state
   ACPI -> Core context Lost and other flags as shown above in each _LPI

   These above are used to indicate if the timer is stopped(in case of DT)
   and other contexts (only in ACPI though not used in the kernel) are lost

Not sure why the information was not used in both place(both with DT and
ACPI). One of the discussion I can remember for ACPI was to ensure the
flags can be set appropriately if the context was saved/restored by the
firmware and not related to the hardware power domain related property.

That said, I don't have a strong argument as why the other was not used
here in this case. Since I added LPI support, I used this LPI flag(probably
looking at the DT implementation) and this patch is just changing from
blanket whole flags check to just "Core context Lost" bit in the flag as
the other bits (GICR/GICD/Trace context loss) can be still set on this
platform where core context is not lost as the timers are always on.

Yes it is duplication of the data in the ACPI spec as well as DT. Not
sure if this needs to be fixed though(bit worried about backward
compatibility with broken firmware/DT)

--
Regards,
Sudeep