On 4/25/2018 11:25 PM, Viresh Kumar wrote:
> On 25-04-18, 10:26, Prashanth Prakash wrote:
>> Add support to specify platform specific transition_delay_us instead
>> of using the transition delay derived from PCC.
>>
>> With commit "3d41386d556d: cpufreq: CPPC: Use transition_delay_us
>> depending transition_latency" we are setting transition_delay_us
>> directly and not applying the LATENCY_MULTIPLIER. With this on Qualcomm
>> Centriq we can end up with a very high rate of frequency change requests
>> when using schedutil governor (default rate_limit_us=10 compared to an
>> earlier value of 10000).
>>
>> The PCC subspace describes the rate at which the platform can accept
>> commands on the CPPC's PCC channel. This includes read and write
>> command on the PCC channel that can be used for reasons other than
>> frequency transitions. Moreover the same PCC subspace can be used by
>> multiple freq domains and deriving transition_delay_us from it as we do
>> now can be sub-optimal.
>>
>> Moreover if a platform does not use PCC for desired_perf register then
>> there is no way to compute the transition latency or the delay_us.
>>
>> CPPC does not have a standard defined mechanism to get the transition
>> rate or the latency at the moment.
>>
>> Given the above limitations, it is simpler to have a platform specific
>> transition_delay_us and rely on PCC derived value only if a platform
>> specific value is not available.
>>
>> Signed-off-by: Prashanth Prakash <pprakash@xxxxxxxxxxxxxx>
>> Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>> Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
>> Cc: 4.14+ <stable@xxxxxxxxxxxxxxx>
>> Fixes: 3d41386d556d ("cpufreq: CPPC: Use transition_delay_us depending
>> transition_latency)
>> ---
>> v2:
>> * Return final delay_us from cppc_cpufreq_get_transition_delay_us (Viresh)
>> ---
>> drivers/cpufreq/cppc_cpufreq.c | 43 ++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index bc5fc16..b1e32ad 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -126,6 +126,46 @@ static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
>> cpu->perf_caps.lowest_perf, cpu_num, ret);
>> }
>>
>> +/*
>> + * The PCC subspace describes the rate at which platform can accept commands
>> + * on the shared PCC channel (including READs which do not count towards freq
>> + * trasition requests), so ideally we need to use the PCC values as a fallback
>> + * if we don't have a platform specific transition_delay_us
>> + */
>> +#if defined(CONFIG_ARM64)
> You can use #ifdef here instead.
I will update.
>
>> +#include <asm/cputype.h>
>> +
>> +static unsigned int cppc_cpufreq_get_transition_delay_us(int cpu)
>> +{
>> + unsigned long implementor = read_cpuid_implementor();
>> + unsigned long part_num = read_cpuid_part_number();
>> + unsigned int delay_us = 0;
>> +
>> + switch (implementor) {
>> + case ARM_CPU_IMP_QCOM:
>> + switch (part_num) {
>> + case QCOM_CPU_PART_FALKOR_V1:
>> + case QCOM_CPU_PART_FALKOR:
>> + delay_us = 10000;
>> + break;
>> + }
>> + break;
> What about adding a default: case here and moving the below code into that ?
Agree, it will much cleaner.. I will update.
>
>> + }
>> +
>> + if (!delay_us)
>> + delay_us = cppc_get_transition_latency(cpu) / NSEC_PER_USEC;
>> +
>> + return delay_us;
>> +}
>> +
>> +#else
>> +
>> +static unsigned int cppc_cpufreq_get_transition_delay_us(int cpu)
>> +{
>> + return cppc_get_transition_latency(cpu) / NSEC_PER_USEC;
>> +}
>> +#endif
>> +
>> static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> {
>> struct cppc_cpudata *cpu;
>> @@ -162,8 +202,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> cpu->perf_caps.highest_perf;
>> policy->cpuinfo.max_freq = cppc_dmi_max_khz;
>>
>> - policy->transition_delay_us = cppc_get_transition_latency(cpu_num) /
>> - NSEC_PER_USEC;
>> + policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu_num);
>> policy->shared_type = cpu->shared_type;
>>
>> if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
>> --
>> Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the
>> Code Aurora Forum, a Linux Foundation Collaborative Project.
--
Thanks,
Prashanth