diff mbox series

[v1,3/5] ACPI: CPPC: Assume no transition latency if no PCCT

Message ID 20220511134559.1466925-3-pierre.gondois@arm.com
State Superseded
Headers show
Series [v1,1/5] ACPI: CPPC: Check _OSC for flexible address space | expand

Commit Message

Pierre Gondois May 11, 2022, 1:45 p.m. UTC 
From: Pierre Gondois <Pierre.Gondois@arm.com>

The transition_delay_us (struct cpufreq_policy) is currently defined
as:
  Preferred average time interval between consecutive invocations of
  the driver to set the frequency for this policy.  To be set by the
  scaling driver (0, which is the default, means no preference).
The transition_latency represents the amount of time necessary for a
CPU to change its frequency.

A PCCT table advertises mutliple values:
- pcc_nominal: Expected latency to process a command, in microseconds
- pcc_mpar: The maximum number of periodic requests that the subspace
  channel can support, reported in commands per minute. 0 indicates no
  limitation.
- pcc_mrtt: The minimum amount of time that OSPM must wait after the
  completion of a command before issuing the next command,
  in microseconds.
cppc_get_transition_latency() allows to get the max of them.

commit d4f3388afd48 ("cpufreq / CPPC: Set platform specific
transition_delay_us") allows to select transition_delay_us based on
the platform, and fallbacks to cppc_get_transition_latency()
otherwise.

If _CPC objects are not using PCC channels (no PPCT table), the
transition_delay_us is set to CPUFREQ_ETERNAL, leading to really long
periods between frequency updates (~4s).

If the desired_reg, where performance requests are written, is in
SystemMemory or SystemIo ACPI address space, there is no delay
in requests. So return 0 instead of CPUFREQ_ETERNAL, leading to
transition_delay_us being set to LATENCY_MULTIPLIER us (1000 us).

This patch also adds two macros to check the address spaces.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 drivers/acpi/cppc_acpi.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Sudeep Holla May 11, 2022, 2:30 p.m. UTC | #1 
On Wed, May 11, 2022 at 03:45:57PM +0200, Pierre Gondois wrote:
> From: Pierre Gondois <Pierre.Gondois@arm.com>
> 
> The transition_delay_us (struct cpufreq_policy) is currently defined
> as:
>   Preferred average time interval between consecutive invocations of
>   the driver to set the frequency for this policy.  To be set by the
>   scaling driver (0, which is the default, means no preference).
> The transition_latency represents the amount of time necessary for a
> CPU to change its frequency.
> 
> A PCCT table advertises mutliple values:
> - pcc_nominal: Expected latency to process a command, in microseconds
> - pcc_mpar: The maximum number of periodic requests that the subspace
>   channel can support, reported in commands per minute. 0 indicates no
>   limitation.
> - pcc_mrtt: The minimum amount of time that OSPM must wait after the
>   completion of a command before issuing the next command,
>   in microseconds.
> cppc_get_transition_latency() allows to get the max of them.
> 
> commit d4f3388afd48 ("cpufreq / CPPC: Set platform specific
> transition_delay_us") allows to select transition_delay_us based on
> the platform, and fallbacks to cppc_get_transition_latency()
> otherwise.
> 
> If _CPC objects are not using PCC channels (no PPCT table), the
> transition_delay_us is set to CPUFREQ_ETERNAL, leading to really long
> periods between frequency updates (~4s).
> 
> If the desired_reg, where performance requests are written, is in
> SystemMemory or SystemIo ACPI address space, there is no delay
> in requests. So return 0 instead of CPUFREQ_ETERNAL, leading to
> transition_delay_us being set to LATENCY_MULTIPLIER us (1000 us).
> 
> This patch also adds two macros to check the address spaces.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  drivers/acpi/cppc_acpi.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 6f09fe011544..cc932ec1b613 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -100,6 +100,16 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>  				(cpc)->cpc_entry.reg.space_id ==	\
>  				ACPI_ADR_SPACE_PLATFORM_COMM)
>  
> +/* Check if a CPC register is in SystemMemory */
> +#define CPC_IN_SM(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
> +				(cpc)->cpc_entry.reg.space_id ==	\
> +				ACPI_ADR_SPACE_SYSTEM_MEMORY)
> +

Again my taste or preference: s/SM/SYS_MEM or SYSTEM_MEM

> +/* Check if a CPC register is in SystemIo */
> +#define CPC_IN_SIO(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
> +				(cpc)->cpc_entry.reg.space_id ==	\
> +				ACPI_ADR_SPACE_SYSTEM_IO)
> +

Ditto, s/SM/SYS_IO or SYSTEM_IO

I need not refer back to the macro when reading the code. SM/SIO is too
short and makes it hard to infer from the name in general.

>  /* Evaluates to True if reg is a NULL register descriptor */
>  #define IS_NULL_REG(reg) ((reg)->space_id ==  ACPI_ADR_SPACE_SYSTEM_MEMORY && \
>  				(reg)->address == 0 &&			\
> @@ -1456,6 +1466,9 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);
>   * transition latency for performance change requests. The closest we have
>   * is the timing information from the PCCT tables which provides the info
>   * on the number and frequency of PCC commands the platform can handle.
> + *
> + * If desired_reg is in the SystemMemory or SystemIo ACPI address space,
> + * then assume there is no latency.
>   */
>  unsigned int cppc_get_transition_latency(int cpu_num)
>  {
> @@ -1481,7 +1494,9 @@ unsigned int cppc_get_transition_latency(int cpu_num)
>  		return CPUFREQ_ETERNAL;
>  
>  	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
> -	if (!CPC_IN_PCC(desired_reg))
> +	if (CPC_IN_SM(desired_reg) || CPC_IN_SIO(desired_reg))
> +		return 0;
> +	else if (!CPC_IN_PCC(desired_reg))
>  		return CPUFREQ_ETERNAL;

Apart from the above,

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
Rafael J. Wysocki May 12, 2022, 3:04 p.m. UTC | #2 
On Wed, May 11, 2022 at 4:30 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, May 11, 2022 at 03:45:57PM +0200, Pierre Gondois wrote:
> > From: Pierre Gondois <Pierre.Gondois@arm.com>
> >
> > The transition_delay_us (struct cpufreq_policy) is currently defined
> > as:
> >   Preferred average time interval between consecutive invocations of
> >   the driver to set the frequency for this policy.  To be set by the
> >   scaling driver (0, which is the default, means no preference).
> > The transition_latency represents the amount of time necessary for a
> > CPU to change its frequency.
> >
> > A PCCT table advertises mutliple values:
> > - pcc_nominal: Expected latency to process a command, in microseconds
> > - pcc_mpar: The maximum number of periodic requests that the subspace
> >   channel can support, reported in commands per minute. 0 indicates no
> >   limitation.
> > - pcc_mrtt: The minimum amount of time that OSPM must wait after the
> >   completion of a command before issuing the next command,
> >   in microseconds.
> > cppc_get_transition_latency() allows to get the max of them.
> >
> > commit d4f3388afd48 ("cpufreq / CPPC: Set platform specific
> > transition_delay_us") allows to select transition_delay_us based on
> > the platform, and fallbacks to cppc_get_transition_latency()
> > otherwise.
> >
> > If _CPC objects are not using PCC channels (no PPCT table), the
> > transition_delay_us is set to CPUFREQ_ETERNAL, leading to really long
> > periods between frequency updates (~4s).
> >
> > If the desired_reg, where performance requests are written, is in
> > SystemMemory or SystemIo ACPI address space, there is no delay
> > in requests. So return 0 instead of CPUFREQ_ETERNAL, leading to
> > transition_delay_us being set to LATENCY_MULTIPLIER us (1000 us).
> >
> > This patch also adds two macros to check the address spaces.
> >
> > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> > ---
> >  drivers/acpi/cppc_acpi.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > index 6f09fe011544..cc932ec1b613 100644
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -100,6 +100,16 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
> >                               (cpc)->cpc_entry.reg.space_id ==        \
> >                               ACPI_ADR_SPACE_PLATFORM_COMM)
> >
> > +/* Check if a CPC register is in SystemMemory */
> > +#define CPC_IN_SM(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&           \
> > +                             (cpc)->cpc_entry.reg.space_id ==        \
> > +                             ACPI_ADR_SPACE_SYSTEM_MEMORY)
> > +
>
> Again my taste or preference: s/SM/SYS_MEM or SYSTEM_MEM

SYSTEM_MEMORY even.

>
> > +/* Check if a CPC register is in SystemIo */
> > +#define CPC_IN_SIO(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&          \
> > +                             (cpc)->cpc_entry.reg.space_id ==        \
> > +                             ACPI_ADR_SPACE_SYSTEM_IO)
> > +
>
> Ditto, s/SM/SYS_IO or SYSTEM_IO
>
> I need not refer back to the macro when reading the code. SM/SIO is too
> short and makes it hard to infer from the name in general.

Right.

> >  /* Evaluates to True if reg is a NULL register descriptor */
> >  #define IS_NULL_REG(reg) ((reg)->space_id ==  ACPI_ADR_SPACE_SYSTEM_MEMORY && \
> >                               (reg)->address == 0 &&                  \
> > @@ -1456,6 +1466,9 @@ EXPORT_SYMBOL_GPL(cppc_set_perf);
> >   * transition latency for performance change requests. The closest we have
> >   * is the timing information from the PCCT tables which provides the info
> >   * on the number and frequency of PCC commands the platform can handle.
> > + *
> > + * If desired_reg is in the SystemMemory or SystemIo ACPI address space,
> > + * then assume there is no latency.
> >   */
> >  unsigned int cppc_get_transition_latency(int cpu_num)
> >  {
> > @@ -1481,7 +1494,9 @@ unsigned int cppc_get_transition_latency(int cpu_num)
> >               return CPUFREQ_ETERNAL;
> >
> >       desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
> > -     if (!CPC_IN_PCC(desired_reg))
> > +     if (CPC_IN_SM(desired_reg) || CPC_IN_SIO(desired_reg))
> > +             return 0;
> > +     else if (!CPC_IN_PCC(desired_reg))
> >               return CPUFREQ_ETERNAL;
diff mbox series

Patch

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 6f09fe011544..cc932ec1b613 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -100,6 +100,16 @@  static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
 				(cpc)->cpc_entry.reg.space_id ==	\
 				ACPI_ADR_SPACE_PLATFORM_COMM)
 
+/* Check if a CPC register is in SystemMemory */
+#define CPC_IN_SM(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
+				(cpc)->cpc_entry.reg.space_id ==	\
+				ACPI_ADR_SPACE_SYSTEM_MEMORY)
+
+/* Check if a CPC register is in SystemIo */
+#define CPC_IN_SIO(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
+				(cpc)->cpc_entry.reg.space_id ==	\
+				ACPI_ADR_SPACE_SYSTEM_IO)
+
 /* Evaluates to True if reg is a NULL register descriptor */
 #define IS_NULL_REG(reg) ((reg)->space_id ==  ACPI_ADR_SPACE_SYSTEM_MEMORY && \
 				(reg)->address == 0 &&			\
@@ -1456,6 +1466,9 @@  EXPORT_SYMBOL_GPL(cppc_set_perf);
  * transition latency for performance change requests. The closest we have
  * is the timing information from the PCCT tables which provides the info
  * on the number and frequency of PCC commands the platform can handle.
+ *
+ * If desired_reg is in the SystemMemory or SystemIo ACPI address space,
+ * then assume there is no latency.
  */
 unsigned int cppc_get_transition_latency(int cpu_num)
 {
@@ -1481,7 +1494,9 @@  unsigned int cppc_get_transition_latency(int cpu_num)
 		return CPUFREQ_ETERNAL;
 
 	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
-	if (!CPC_IN_PCC(desired_reg))
+	if (CPC_IN_SM(desired_reg) || CPC_IN_SIO(desired_reg))
+		return 0;
+	else if (!CPC_IN_PCC(desired_reg))
 		return CPUFREQ_ETERNAL;
 
 	if (pcc_ss_id < 0)