* [PATCH] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer @ 2023-07-12 17:24 Oza Pawandeep 2023-07-18 13:32 ` Sudeep Holla 0 siblings, 1 reply; 3+ messages in thread From: Oza Pawandeep @ 2023-07-12 17:24 UTC (permalink / raw) To: Catalin Marinas, Will Deacon, Rafael J . Wysocki, Len Brown, linux-arm-kernel, linux-kernel, linux-acpi, jiles Cc: Oza Pawandeep Arm® Functional Fixed Hardware Specification defines LPI states, which provides 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. 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..9c335968316c 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -42,6 +42,24 @@ #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_acpi_lpi_timer_stopped +static __always_inline bool arch_acpi_lpi_timer_stopped(u32 arch_flags) +{ + return arch_flags & CPUIDLE_CORE_CTXT; +} +#define arch_acpi_lpi_timer_stopped arch_acpi_lpi_timer_stopped +#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..8ea1f2b3bf96 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -1221,7 +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) + if (arch_acpi_lpi_timer_stopped(lpi->arch_flags)) state->flags |= CPUIDLE_FLAG_TIMER_STOP; if (i != 0 && lpi->entry_method == ACPI_CSTATE_FFH) state->flags |= CPUIDLE_FLAG_RCU_IDLE; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index d584f94409e1..b24f1cd1cebb 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -1471,6 +1471,14 @@ static inline int lpit_read_residency_count_address(u64 *address) } #endif +#ifndef arch_acpi_lpi_timer_stopped +static __always_inline bool arch_acpi_lpi_timer_stopped(u32 arch_flags) +{ + return (arch_flags != 0); +} +#define arch_acpi_lpi_timer_stopped arch_acpi_lpi_timer_stopped +#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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer 2023-07-12 17:24 [PATCH] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer Oza Pawandeep @ 2023-07-18 13:32 ` Sudeep Holla 2023-07-27 19:03 ` Pawandeep Oza (QUIC) 0 siblings, 1 reply; 3+ messages in thread From: Sudeep Holla @ 2023-07-18 13:32 UTC (permalink / raw) To: Oza Pawandeep Cc: Catalin Marinas, Will Deacon, Sudeep Holla, Rafael J . Wysocki, Len Brown, linux-arm-kernel, linux-kernel, linux-acpi, jiles On Wed, Jul 12, 2023 at 10:24:58AM -0700, Oza Pawandeep wrote: > Arm® Functional Fixed Hardware Specification defines LPI states, > which provides 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. > > 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..9c335968316c 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -42,6 +42,24 @@ > #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_acpi_lpi_timer_stopped > +static __always_inline bool arch_acpi_lpi_timer_stopped(u32 arch_flags) As mentioned by you above, the core context is not just timer context, so calling this function so is misleading. > +{ > + return arch_flags & CPUIDLE_CORE_CTXT; > +} > +#define arch_acpi_lpi_timer_stopped arch_acpi_lpi_timer_stopped > +#endif > + > +#define CPUIDLE_TRACE_CTXT BIT(1) /* Trace context loss */ > +#define CPUIDLE_GICR_CTXT BIT(2) /* GICR */ > +#define CPUIDLE_GICD_CTXT BIT(3) /* GICD */ > + Do we really need to define these unused bitfields ? DO you have plans to use them ? > /* 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..8ea1f2b3bf96 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -1221,7 +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) > + if (arch_acpi_lpi_timer_stopped(lpi->arch_flags)) While setting CPUIDLE_FLAG_TIMER_STOP if any flags set is already questionable, checking for arch specific flag in the generic code is even more questionable now. I wonder if it makes more sense to have a arch specific helper to update the state->flags based on how arch specific interpretation of lpi->arch_flags ? > state->flags |= CPUIDLE_FLAG_TIMER_STOP; > if (i != 0 && lpi->entry_method == ACPI_CSTATE_FFH) > state->flags |= CPUIDLE_FLAG_RCU_IDLE; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index d584f94409e1..b24f1cd1cebb 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -1471,6 +1471,14 @@ static inline int lpit_read_residency_count_address(u64 *address) > } > #endif > > +#ifndef arch_acpi_lpi_timer_stopped > +static __always_inline bool arch_acpi_lpi_timer_stopped(u32 arch_flags) > +{ > + return (arch_flags != 0); > +} > +#define arch_acpi_lpi_timer_stopped arch_acpi_lpi_timer_stopped > +#endif > + This looks ugly and main reason for my above comment. I am thinking of arch_update_idle_state_flags(lpi->arch_flags, &state->flags) and make it do nothing on non arm platforms. I don't think we will be breaking anything(i.e. no need to check arch_flags != 0. It is incorrect strictly speaking but there are no non-arm users ATM, but that doesn't mean we can trickle the arch specific LPI FFH details into the generic code. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer 2023-07-18 13:32 ` Sudeep Holla @ 2023-07-27 19:03 ` Pawandeep Oza (QUIC) 0 siblings, 0 replies; 3+ messages in thread From: Pawandeep Oza (QUIC) @ 2023-07-27 19:03 UTC (permalink / raw) To: 'Sudeep Holla', Pawandeep Oza (QUIC) Cc: Catalin Marinas, Will Deacon, Rafael J . Wysocki, Len Brown, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, Jamie Iles Hi Sudeep, Please find my comments inline. Regards, Oza. -----Original Message----- From: Sudeep Holla <sudeep.holla@arm.com> Sent: Tuesday, July 18, 2023 6:32 AM To: Pawandeep Oza (QUIC) <quic_poza@quicinc.com> Cc: Catalin Marinas <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Sudeep Holla <sudeep.holla@arm.com>; Rafael J . Wysocki <rafael@kernel.org>; Len Brown <lenb@kernel.org>; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; Jamie Iles <jiles@qti.qualcomm.com> Subject: Re: [PATCH] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer On Wed, Jul 12, 2023 at 10:24:58AM -0700, Oza Pawandeep wrote: > Arm(r) Functional Fixed Hardware Specification defines LPI states, which > provides 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. > > 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..9c335968316c > 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -42,6 +42,24 @@ > #define ACPI_MADT_GICC_SPE (offsetof(struct acpi_madt_generic_interrupt, \ > spe_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_acpi_lpi_timer_stopped > +static __always_inline bool arch_acpi_lpi_timer_stopped(u32 > +arch_flags) As mentioned by you above, the core context is not just timer context, so calling this function so is misleading. Oza: Will take care of you last 2 comments, probably this should be taken care as well with that. > +{ > + return arch_flags & CPUIDLE_CORE_CTXT; } #define > +arch_acpi_lpi_timer_stopped arch_acpi_lpi_timer_stopped #endif > + > +#define CPUIDLE_TRACE_CTXT BIT(1) /* Trace context loss */ > +#define CPUIDLE_GICR_CTXT BIT(2) /* GICR */ > +#define CPUIDLE_GICD_CTXT BIT(3) /* GICD */ > + Do we really need to define these unused bitfields ? DO you have plans to use them ? Oza: this is basically defined in ARM FFH - I think no harm is having them here for sake of completeness. > /* 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..8ea1f2b3bf96 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -1221,7 +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) > + if (arch_acpi_lpi_timer_stopped(lpi->arch_flags)) While setting CPUIDLE_FLAG_TIMER_STOP if any flags set is already questionable, checking for arch specific flag in the generic code is even more questionable now. I wonder if it makes more sense to have a arch specific helper to update the state->flags based on how arch specific interpretation of lpi->arch_flags ? > state->flags |= CPUIDLE_FLAG_TIMER_STOP; > if (i != 0 && lpi->entry_method == ACPI_CSTATE_FFH) > state->flags |= CPUIDLE_FLAG_RCU_IDLE; diff --git > a/include/linux/acpi.h b/include/linux/acpi.h index > d584f94409e1..b24f1cd1cebb 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -1471,6 +1471,14 @@ static inline int > lpit_read_residency_count_address(u64 *address) } #endif > > +#ifndef arch_acpi_lpi_timer_stopped > +static __always_inline bool arch_acpi_lpi_timer_stopped(u32 > +arch_flags) { > + return (arch_flags != 0); > +} > +#define arch_acpi_lpi_timer_stopped arch_acpi_lpi_timer_stopped > +#endif > + This looks ugly and main reason for my above comment. I am thinking of arch_update_idle_state_flags(lpi->arch_flags, &state->flags) and make it do nothing on non arm platforms. I don't think we will be breaking anything(i.e. no need to check arch_flags != 0. It is incorrect strictly speaking but there are no non-arm users ATM, but that doesn't mean we can trickle the arch specific LPI FFH details into the generic code. Oza: Let me work on above 2 comments and post the new patch. -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-27 19:04 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-12 17:24 [PATCH] cpuidle, ACPI: Evaluate LPI arch_flags for broadcast timer Oza Pawandeep 2023-07-18 13:32 ` Sudeep Holla 2023-07-27 19:03 ` Pawandeep Oza (QUIC)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox