Hi Viresh,
On 4/26/2018 9:29 AM, Prakash, Prashanth wrote:
>
> 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.
On second thoughts, If we have a scenario where we have a platform on which the
"implementor" matches a case statement, but the "part_num" does not have a
corresponding case statement. In this case we would need multiple levels of default,
so I think the current implementation is simpler.
Thoughts?
>>> + }
>>> +
>>> + 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