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 <[email protected]>
Signed-off-by: Oza Pawandeep <[email protected]>
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 4d537d56eb84..a30b6e16628d 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,25 @@
#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 */
+
+#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
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..f8c561a4215e 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1480,6 +1480,10 @@ static inline int lpit_read_residency_count_address(u64 *address)
}
#endif
+#ifndef arch_update_idle_state_flags
+#define arch_update_idle_state_flags(af, sf) 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
On Tue, Sep 12, 2023 at 10:29:33AM -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 <[email protected]>
IIRC, Rafael had acked this, perhaps missing the tag ?
Also just add a note to Will/Catalin that Rafael has acked and prefer to
take it via arm64 tree.
--
Regards,
Sudeep
On Wed, Sep 13, 2023 at 09:43:01AM +0100, Sudeep Holla wrote:
> On Tue, Sep 12, 2023 at 10:29:33AM -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 <[email protected]>
>
> IIRC, Rafael had acked this, perhaps missing the tag ?
> Also just add a note to Will/Catalin that Rafael has acked and prefer to
> take it via arm64 tree.
Is this a fix? If so, please can I have a "Fixes:" tag (and does it need to
go into stable?)
Will
On Wed, Sep 13, 2023 at 11:27:21AM +0100, Will Deacon wrote:
> On Wed, Sep 13, 2023 at 09:43:01AM +0100, Sudeep Holla wrote:
> > On Tue, Sep 12, 2023 at 10:29:33AM -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 <[email protected]>
> >
> > IIRC, Rafael had acked this, perhaps missing the tag ?
> > Also just add a note to Will/Catalin that Rafael has acked and prefer to
> > take it via arm64 tree.
>
> Is this a fix? If so, please can I have a "Fixes:" tag (and does it need to
> go into stable?)
>
Well, most platform today have CPUIDLE_CORE_CTXT set so the existing code
works as expected. It is this Qcom platform that doesn't set it and need
different behaviour. So based on their requirement for running stable
tree, the fixes tag can be added. In short yes it can be seen as a fix
if this new requirement is considered.
Sorry the main reason for trying to avoid is there are multiple patches
adding the initial support and there has been some code restructuring
around this. So it may need proper backporting based on the version.
I just want to avoid if there is no real requirement for that.
--
Regards,
Sudeep
On Tue, Sep 12, 2023 at 10:29:33AM -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 <[email protected]>
> Signed-off-by: Oza Pawandeep <[email protected]>
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 4d537d56eb84..a30b6e16628d 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,25 @@
>
> #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 */
> +
> +#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
Why do you need the #ifndef/endif guards here? I'd have thought you'd
_always_ want this definition to be the one used, with the compiler
complaining otherwise.
Will
-----Original Message-----
From: Will Deacon <[email protected]>
Sent: Thursday, September 14, 2023 6:22 AM
To: Pawandeep Oza (QUIC) <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer
On Tue, Sep 12, 2023 at 10:29:33AM -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.
>
> Reviewed-by: Sudeep Holla <[email protected]>
> Signed-off-by: Oza Pawandeep <[email protected]>
>
> diff --git a/arch/arm64/include/asm/acpi.h
> b/arch/arm64/include/asm/acpi.h index 4d537d56eb84..a30b6e16628d
> 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,25 @@
>
> #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 */
> +
> +#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
Why do you need the #ifndef/endif guards here? I'd have thought you'd _always_ want this definition to be the one used, with the compiler complaining otherwise.
Oza: thanks, Will!, will remove it.
Will