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

[Rafael J. Wysocki]

On Wed, May 11, 2022 at 4:30 PM Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
> On Wed, May 11, 2022 at 03:45:57PM +0200, Pierre Gondois wrote:
> > From: Pierre Gondois <Pierre.Gondois@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> >  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;