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;
