| Message ID | 1602162066-26442-2-git-send-email-sumitg@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] cpufreq: tegra194: get consistent cpuinfo_cur_freq | expand |
On 08-10-20, 18:31, Sumit Gupta wrote: > Frequency returned by 'cpuinfo_cur_freq' using counters is not fixed > and keeps changing slightly. This change returns a consistent value > from freq_table. If the reconstructed frequency has acceptable delta > from the last written value, then return the frequency corresponding > to the last written ndiv value from freq_table. Otherwise, print a > warning and return the reconstructed freq. > > Signed-off-by: Sumit Gupta <sumitg@nvidia.com> > --- > drivers/cpufreq/tegra194-cpufreq.c | 71 +++++++++++++++++++++++++++++++++----- > 1 file changed, 62 insertions(+), 9 deletions(-) > > diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c > index e1d931c..d250e49 100644 > --- a/drivers/cpufreq/tegra194-cpufreq.c > +++ b/drivers/cpufreq/tegra194-cpufreq.c > @@ -180,9 +180,70 @@ static unsigned int tegra194_get_speed_common(u32 cpu, u32 delay) > return (rate_mhz * KHZ); /* in KHz */ > } > > +static void get_cpu_ndiv(void *ndiv) > +{ > + u64 ndiv_val; > + > + asm volatile("mrs %0, s3_0_c15_c0_4" : "=r" (ndiv_val) : ); > + > + *(u64 *)ndiv = ndiv_val; > +} > + > +static void set_cpu_ndiv(void *data) You weren't required to do this unnecessary change. > +{ > + struct cpufreq_frequency_table *tbl = data; > + u64 ndiv_val = (u64)tbl->driver_data; > + > + asm volatile("msr s3_0_c15_c0_4, %0" : : "r" (ndiv_val)); > +} > + > static unsigned int tegra194_get_speed(u32 cpu) > { > - return tegra194_get_speed_common(cpu, US_DELAY); > + struct tegra194_cpufreq_data *data = cpufreq_get_driver_data(); > + struct cpufreq_frequency_table *pos; > + unsigned int rate; > + u64 ndiv; > + int ret; > + u32 cl; > + > + if (!cpu_online(cpu)) This isn't required. The CPU is guaranteed to be online here. > + return -EINVAL; > + > + smp_call_function_single(cpu, get_cpu_cluster, &cl, true); > + > + if (cl >= data->num_clusters) Is it really possible here ? I meant you must have already checked this at cpufreq-init level already. Else mark it unlikely at least. > + return -EINVAL; > + > + /* reconstruct actual cpu freq using counters */ > + rate = tegra194_get_speed_common(cpu, US_DELAY); > + > + /* get last written ndiv value */ > + ret = smp_call_function_single(cpu, get_cpu_ndiv, &ndiv, true); > + if (ret) { What exactly can fail here ? get_cpu_ndiv() can't fail. Do we really need this check ? What about WARN_ON_ONCE() ? > + pr_err("cpufreq: Failed to get ndiv for CPU%d, ret:%d\n", > + cpu, ret); > + return rate; > + } > + > + /* > + * If the reconstructed frequency has acceptable delta from > + * the last written value, then return freq corresponding > + * to the last written ndiv value from freq_table. This is > + * done to return consistent value. > + */ > + cpufreq_for_each_valid_entry(pos, data->tables[cl]) { > + if (pos->driver_data != ndiv) > + continue; > + > + if (abs(pos->frequency - rate) > 115200) { where does this 115200 comes from ? Strange that it matches tty's baud rate :) This is 115 MHz, right ? Isn't that too big of a delta ? > + pr_warn("cpufreq: cpu%d,cur:%u,set:%u,set ndiv:%llu\n", > + cpu, rate, pos->frequency, ndiv); > + } else { > + rate = pos->frequency; > + } > + break; > + } > + return rate; > }
>> Frequency returned by 'cpuinfo_cur_freq' using counters is not fixed >> and keeps changing slightly. This change returns a consistent value >> from freq_table. If the reconstructed frequency has acceptable delta >> from the last written value, then return the frequency corresponding >> to the last written ndiv value from freq_table. Otherwise, print a >> warning and return the reconstructed freq. >> >> Signed-off-by: Sumit Gupta <sumitg@nvidia.com> >> --- >> drivers/cpufreq/tegra194-cpufreq.c | 71 +++++++++++++++++++++++++++++++++----- >> 1 file changed, 62 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c >> index e1d931c..d250e49 100644 >> --- a/drivers/cpufreq/tegra194-cpufreq.c >> +++ b/drivers/cpufreq/tegra194-cpufreq.c >> @@ -180,9 +180,70 @@ static unsigned int tegra194_get_speed_common(u32 cpu, u32 delay) >> return (rate_mhz * KHZ); /* in KHz */ >> } >> >> +static void get_cpu_ndiv(void *ndiv) >> +{ >> + u64 ndiv_val; >> + >> + asm volatile("mrs %0, s3_0_c15_c0_4" : "=r" (ndiv_val) : ); >> + >> + *(u64 *)ndiv = ndiv_val; >> +} >> + >> +static void set_cpu_ndiv(void *data) > > You weren't required to do this unnecessary change. > ya, moved the function up to keep both {get_|set_} calls together. >> +{ >> + struct cpufreq_frequency_table *tbl = data; >> + u64 ndiv_val = (u64)tbl->driver_data; >> + >> + asm volatile("msr s3_0_c15_c0_4, %0" : : "r" (ndiv_val)); >> +} >> + >> static unsigned int tegra194_get_speed(u32 cpu) >> { >> - return tegra194_get_speed_common(cpu, US_DELAY); >> + struct tegra194_cpufreq_data *data = cpufreq_get_driver_data(); >> + struct cpufreq_frequency_table *pos; >> + unsigned int rate; >> + u64 ndiv; >> + int ret; >> + u32 cl; >> + >> + if (!cpu_online(cpu)) > > This isn't required. The CPU is guaranteed to be online here. > OK, will remove this in next version. >> + return -EINVAL; >> + >> + smp_call_function_single(cpu, get_cpu_cluster, &cl, true); >> + >> + if (cl >= data->num_clusters) > > Is it really possible here ? I meant you must have already checked > this at cpufreq-init level already. Else mark it unlikely at least. > Ya, will remove the check here. >> + return -EINVAL; >> + >> + /* reconstruct actual cpu freq using counters */ >> + rate = tegra194_get_speed_common(cpu, US_DELAY); >> + >> + /* get last written ndiv value */ >> + ret = smp_call_function_single(cpu, get_cpu_ndiv, &ndiv, true); >> + if (ret) { > > What exactly can fail here ? get_cpu_ndiv() can't fail. Do we really > need this check ? What about WARN_ON_ONCE() ? > OK. >> + pr_err("cpufreq: Failed to get ndiv for CPU%d, ret:%d\n", >> + cpu, ret); >> + return rate; >> + } >> + >> + /* >> + * If the reconstructed frequency has acceptable delta from >> + * the last written value, then return freq corresponding >> + * to the last written ndiv value from freq_table. This is >> + * done to return consistent value. >> + */ >> + cpufreq_for_each_valid_entry(pos, data->tables[cl]) { >> + if (pos->driver_data != ndiv) >> + continue; >> + >> + if (abs(pos->frequency - rate) > 115200) { > > where does this 115200 comes from ? Strange that it matches tty's baud > rate :) >The value is equal to one freq step size. > This is 115 MHz, right ? Isn't that too big of a delta ? > The is the acceptable delta used during our testing keeping some margin. >> + pr_warn("cpufreq: cpu%d,cur:%u,set:%u,set ndiv:%llu\n", >> + cpu, rate, pos->frequency, ndiv); >> + } else { >> + rate = pos->frequency; >> + } >> + break; >> + } >> + return rate; >> } > > -- > viresh >
diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c index e1d931c..d250e49 100644 --- a/drivers/cpufreq/tegra194-cpufreq.c +++ b/drivers/cpufreq/tegra194-cpufreq.c @@ -180,9 +180,70 @@ static unsigned int tegra194_get_speed_common(u32 cpu, u32 delay) return (rate_mhz * KHZ); /* in KHz */ } +static void get_cpu_ndiv(void *ndiv) +{ + u64 ndiv_val; + + asm volatile("mrs %0, s3_0_c15_c0_4" : "=r" (ndiv_val) : ); + + *(u64 *)ndiv = ndiv_val; +} + +static void set_cpu_ndiv(void *data) +{ + struct cpufreq_frequency_table *tbl = data; + u64 ndiv_val = (u64)tbl->driver_data; + + asm volatile("msr s3_0_c15_c0_4, %0" : : "r" (ndiv_val)); +} + static unsigned int tegra194_get_speed(u32 cpu) { - return tegra194_get_speed_common(cpu, US_DELAY); + struct tegra194_cpufreq_data *data = cpufreq_get_driver_data(); + struct cpufreq_frequency_table *pos; + unsigned int rate; + u64 ndiv; + int ret; + u32 cl; + + if (!cpu_online(cpu)) + return -EINVAL; + + smp_call_function_single(cpu, get_cpu_cluster, &cl, true); + + if (cl >= data->num_clusters) + return -EINVAL; + + /* reconstruct actual cpu freq using counters */ + rate = tegra194_get_speed_common(cpu, US_DELAY); + + /* get last written ndiv value */ + ret = smp_call_function_single(cpu, get_cpu_ndiv, &ndiv, true); + if (ret) { + pr_err("cpufreq: Failed to get ndiv for CPU%d, ret:%d\n", + cpu, ret); + return rate; + } + + /* + * If the reconstructed frequency has acceptable delta from + * the last written value, then return freq corresponding + * to the last written ndiv value from freq_table. This is + * done to return consistent value. + */ + cpufreq_for_each_valid_entry(pos, data->tables[cl]) { + if (pos->driver_data != ndiv) + continue; + + if (abs(pos->frequency - rate) > 115200) { + pr_warn("cpufreq: cpu%d,cur:%u,set:%u,set ndiv:%llu\n", + cpu, rate, pos->frequency, ndiv); + } else { + rate = pos->frequency; + } + break; + } + return rate; } static int tegra194_cpufreq_init(struct cpufreq_policy *policy) @@ -209,14 +270,6 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy) return 0; } -static void set_cpu_ndiv(void *data) -{ - struct cpufreq_frequency_table *tbl = data; - u64 ndiv_val = (u64)tbl->driver_data; - - asm volatile("msr s3_0_c15_c0_4, %0" : : "r" (ndiv_val)); -} - static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) {
Frequency returned by 'cpuinfo_cur_freq' using counters is not fixed and keeps changing slightly. This change returns a consistent value from freq_table. If the reconstructed frequency has acceptable delta from the last written value, then return the frequency corresponding to the last written ndiv value from freq_table. Otherwise, print a warning and return the reconstructed freq. Signed-off-by: Sumit Gupta <sumitg@nvidia.com> --- drivers/cpufreq/tegra194-cpufreq.c | 71 +++++++++++++++++++++++++++++++++----- 1 file changed, 62 insertions(+), 9 deletions(-)