| Message ID | 20230829201101.3330337-1-quic_poza@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v3] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer | expand |
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
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 ';'.
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.
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
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);
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>