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

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

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..5493b044864f 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,27 @@
 #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..60f17c99465b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1471,6 +1471,15 @@ static inline int lpit_read_residency_count_address(u64 *address)
 }
 #endif
 
+#ifndef arch_update_idle_state_flags
+static __always_inline void arch_update_idle_state_flags(u32 arch_flags,
+							unsigned int *sflags)
+{
+
+}
+#define arch_update_idle_state_flags arch_update_idle_state_flags
+#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

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

[Rafael J. Wysocki]

On Mon, Aug 7, 2023 at 5:48 PM Oza Pawandeep <quic_poza@quicinc.com> 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.
>
> 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..5493b044864f 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,27 @@
>  #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..60f17c99465b 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1471,6 +1471,15 @@ static inline int lpit_read_residency_count_address(u64 *address)
>  }
>  #endif
>
> +#ifndef arch_update_idle_state_flags
> +static __always_inline void arch_update_idle_state_flags(u32 arch_flags,
> +                                                       unsigned int *sflags)
> +{
> +
> +}
> +#define arch_update_idle_state_flags arch_update_idle_state_flags
> +#endif
> +
>  #ifdef CONFIG_ACPI_PPTT
>  int acpi_pptt_cpu_is_thread(unsigned int cpu);
>  int find_acpi_cpu_topology(unsigned int cpu, int level);
> --

If I'm to apply this, an ACK from the ARM people is requisite.

Thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Mon, Aug 07, 2023 at 08:48:34AM -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.
> 
> 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..5493b044864f 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,27 @@
>  #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;
> +  }

I think there are whitespaces issue here for sure. Also you don't need
braces for single statement under if condition.

> +}
> +#define arch_update_idle_state_flags arch_update_idle_state_flags

I prefer to rename the function prepending underscore or something to
distinguish the macro name and function.

> +#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..60f17c99465b 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1471,6 +1471,15 @@ static inline int lpit_read_residency_count_address(u64 *address)
>  }
>  #endif
>  
> +#ifndef arch_update_idle_state_flags
> +static __always_inline void arch_update_idle_state_flags(u32 arch_flags,
> +							unsigned int *sflags)
> +{
> +
> +}
> +#define arch_update_idle_state_flags arch_update_idle_state_flags

Can this be simply
#define arch_update_idle_state_flags(..)	do {} while(0)

I don't see the need for both empty function and a macro.

With those fixed, you can add
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: ACPI binding with SCMI

[Sudeep Holla]

On Fri, Aug 25, 2023 at 09:34:59PM +0000, Pawandeep Oza (QUIC) wrote:

[...]

>
> But I still fail to see how fastchannel doorbell and shared memory space
> could get bind to PCC space (type 3) ? I am on 6.3 kernel.

They simply don't bind to PCC, PCC can't be fast channel. What exactly are you
looking for here ? If you provide more details on that, I can see if I can
help or suggest or may be explore along with you if the solution doesn't
exists yet.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: ACPI binding with SCMI

[Pawandeep Oza]

I am looking for scmi binding to ACPI. Basically, SCMI based perf control. 
We have ACPI based system, so I am looking for where I can describe (some way to describe doorbell and share memory in ACPI), 
and then scmi perf can bind to it via some sort of transport (perhaps fastchannels ? )

Regards,
Oza.
-----Original Message-----
From: Sudeep Holla <sudeep.holla@arm.com> 
Sent: Tuesday, August 29, 2023 2:19 AM
To: Pawandeep Oza (QUIC) <quic_poza@quicinc.com>
Cc: rafael@kernel.org; Sudeep Holla <sudeep.holla@arm.com>; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org
Subject: Re: ACPI binding with SCMI

WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.

On Fri, Aug 25, 2023 at 09:34:59PM +0000, Pawandeep Oza (QUIC) wrote:

[...]

>
> But I still fail to see how fastchannel doorbell and shared memory 
> space could get bind to PCC space (type 3) ? I am on 6.3 kernel.

They simply don't bind to PCC, PCC can't be fast channel. What exactly are you looking for here ? If you provide more details on that, I can see if I can help or suggest or may be explore along with you if the solution doesn't exists yet.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: ACPI binding with SCMI

[Sudeep Holla]

On Tue, Aug 29, 2023 at 03:10:34PM +0000, Pawandeep Oza wrote:
> I am looking for scmi binding to ACPI. Basically, SCMI based perf control.
> We have ACPI based system, so I am looking for where I can describe (some
> way to describe doorbell and share memory in ACPI), and then scmi perf can
> bind to it via some sort of transport (perhaps fastchannels ? )

OK, the Section 5.2 ACPI-based Transport in the SCMI spec can be more clearer
IMO. It does state:
" SCMI FastChannels can be represented as ACPI System Memory and used
  in the Continuous Performance Control (CPC) object when using ACPI 
  Collaborative Processor Performance Control (CPPC)."

It doesn't cover the fact that CPPC is compatible only with SCMI fastchannels
without doorbells. Just a plain MMIO register to set the perf.

The normal PCC mailbox works fine if the CPPC Desired Perf Register is
a PCC address space based GAS. However the command is not 1:1 compatible
with SCMI perf. This is one of the reason why the protocols 0x0-0xF was
reserved in SCMI to be compatible with the ACPI CPPC way of using PCC
for perf.

Hope this helps and I didn't make it more complicated for you.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel