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
> +/* 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@xxxxxxx>
--
Regards,
Sudeep
