| Message ID | 1600276277-7290-2-git-send-email-sumitg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | Tegra194 cpufreq driver misc changes | expand |
On 16/09/2020 18:11, 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. We should include the Fixes tag here ... Fixes: df320f89359c ("cpufreq: Add Tegra194 cpufreq driver") > > Signed-off-by: Sumit Gupta <sumitg@nvidia.com> Otherwise ... Reviewed-by: Jon Hunter <jonathanh@nvidia.com> Tested-by: Jon Hunter <jonathanh@nvidia.com> Viresh, ideally we need to include this fix for v5.9. Do you need Sumit to resend with the Fixes tag or are you happy to add? Thanks Jon
Adding Sudeep ... On 17/09/2020 09:36, Jon Hunter wrote: > > > On 16/09/2020 18:11, 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. > > We should include the Fixes tag here ... > > Fixes: df320f89359c ("cpufreq: Add Tegra194 cpufreq driver") > >> >> Signed-off-by: Sumit Gupta <sumitg@nvidia.com> > > Otherwise ... > > Reviewed-by: Jon Hunter <jonathanh@nvidia.com> > Tested-by: Jon Hunter <jonathanh@nvidia.com> > > Viresh, ideally we need to include this fix for v5.9. Do you need Sumit > to resend with the Fixes tag or are you happy to add? Sudeep, Rafael, looks like Viresh is out of office until next month. Please let me know if we can pick up both this patch and following patch for v5.9. Thanks! Jon
Hi Rafael, Sudeep, On 17/09/2020 09:44, Jon Hunter wrote: > Adding Sudeep ... > > On 17/09/2020 09:36, Jon Hunter wrote: >> >> >> On 16/09/2020 18:11, 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. >> >> We should include the Fixes tag here ... >> >> Fixes: df320f89359c ("cpufreq: Add Tegra194 cpufreq driver") >> >>> >>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com> >> >> Otherwise ... >> >> Reviewed-by: Jon Hunter <jonathanh@nvidia.com> >> Tested-by: Jon Hunter <jonathanh@nvidia.com> >> >> Viresh, ideally we need to include this fix for v5.9. Do you need Sumit >> to resend with the Fixes tag or are you happy to add? > > > Sudeep, Rafael, looks like Viresh is out of office until next month. > Please let me know if we can pick up both this patch and following patch > for v5.9. Any chance we can get these patches into v5.9? Thanks! Jon
On Wed, Sep 16, 2020 at 10:41:16PM +0530, 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 | 66 ++++++++++++++++++++++++++++++++------ > 1 file changed, 57 insertions(+), 9 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com>
On 17-09-20, 09:36, Jon Hunter wrote: > Viresh, ideally we need to include this fix for v5.9. Do you need Sumit > to resend with the Fixes tag or are you happy to add? I understand that this fixes a patch which got merged recently, but I am not sure if anything is broken badly right now, i.e. will make the hardware work incorrectly. Do we really need to get these in 5.9 ? As these are significant changes and may cause more bugs. Won't getting them in 5.9-stable and 5.10-rc1 be enough ?
On 16-09-20, 22:41, 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 | 66 ++++++++++++++++++++++++++++++++------ > 1 file changed, 57 insertions(+), 9 deletions(-) > > diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c > index e1d931c..d5b608d 100644 > --- a/drivers/cpufreq/tegra194-cpufreq.c > +++ b/drivers/cpufreq/tegra194-cpufreq.c > @@ -180,9 +180,65 @@ 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 cpufreq_frequency_table *table, *pos; > + struct cpufreq_policy policy; > + unsigned int rate; > + u64 ndiv; > + int err; > + > + cpufreq_get_policy(&policy, cpu); > + table = policy.freq_table; Maybe getting freq-table from cluster specific data would be better/faster. > + > + /* reconstruct actual cpu freq using counters*/ > + rate = tegra194_get_speed_common(cpu, US_DELAY); > + > + /* get last written ndiv value*/ > + err = smp_call_function_single(cpu, get_cpu_ndiv, &ndiv, true); > + if (err) { > + pr_err("cpufreq: Failed to get ndiv for CPU%d, ret:%d\n", > + cpu, err); > + return rate; > + } > + > + /* if the reconstructed frequency has acceptable delta from Both have got the multi-line comments wrong. Format is wrong and every sentence needs to start with a capital letter. > + * 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, table) { > + if (pos->driver_data != ndiv) > + continue; > + > + if (abs(pos->frequency - rate) > 115200) { > + pr_warn("cpufreq: high delta (%d) on CPU%d\n", > + abs(pos->frequency - rate), cpu); > + pr_warn("cpufreq: cur:%u, set:%u, set ndiv:%llu\n", > + rate, pos->frequency, ndiv); Both of these can be merged in a single line I think. There is no need to print delta as you already print both the frequencies. > + } else { > + rate = pos->frequency; > + } > + break; > + } > + return rate; > } > > static int tegra194_cpufreq_init(struct cpufreq_policy *policy) > @@ -209,14 +265,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) > { > -- > 2.7.4
On 05/10/2020 05:34, Viresh Kumar wrote: > On 17-09-20, 09:36, Jon Hunter wrote: >> Viresh, ideally we need to include this fix for v5.9. Do you need Sumit >> to resend with the Fixes tag or are you happy to add? > > I understand that this fixes a patch which got merged recently, but I am not > sure if anything is broken badly right now, i.e. will make the hardware work > incorrectly. > > Do we really need to get these in 5.9 ? As these are significant changes and may > cause more bugs. Won't getting them in 5.9-stable and 5.10-rc1 be enough ? Yes we want these in v5.9 ideally but yes we could merge to 5.9-stable once accepted into the mainline. Jon
>> 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 | 66 ++++++++++++++++++++++++++++++++------ >> 1 file changed, 57 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c >> index e1d931c..d5b608d 100644 >> --- a/drivers/cpufreq/tegra194-cpufreq.c >> +++ b/drivers/cpufreq/tegra194-cpufreq.c >> @@ -180,9 +180,65 @@ 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 cpufreq_frequency_table *table, *pos; >> + struct cpufreq_policy policy; >> + unsigned int rate; >> + u64 ndiv; >> + int err; >> + >> + cpufreq_get_policy(&policy, cpu); >> + table = policy.freq_table; > > Maybe getting freq-table from cluster specific data would be better/faster. > will do the change in next patch version. >> + >> + /* reconstruct actual cpu freq using counters*/ >> + rate = tegra194_get_speed_common(cpu, US_DELAY); >> + >> + /* get last written ndiv value*/ >> + err = smp_call_function_single(cpu, get_cpu_ndiv, &ndiv, true); >> + if (err) { >> + pr_err("cpufreq: Failed to get ndiv for CPU%d, ret:%d\n", >> + cpu, err); >> + return rate; >> + } >> + >> + /* if the reconstructed frequency has acceptable delta from > > Both have got the multi-line comments wrong. Format is wrong and every sentence > needs to start with a capital letter. > will correct. >> + * 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, table) { >> + if (pos->driver_data != ndiv) >> + continue; >> + >> + if (abs(pos->frequency - rate) > 115200) { >> + pr_warn("cpufreq: high delta (%d) on CPU%d\n", >> + abs(pos->frequency - rate), cpu); >> + pr_warn("cpufreq: cur:%u, set:%u, set ndiv:%llu\n", >> + rate, pos->frequency, ndiv); > > Both of these can be merged in a single line I think. There is no need to print > delta as you already print both the frequencies. > Will do this also in next patch version. >> + } else { >> + rate = pos->frequency; >> + } >> + break; >> + } >> + return rate; >> } >> >> static int tegra194_cpufreq_init(struct cpufreq_policy *policy) >> @@ -209,14 +265,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) >> { >> -- >> 2.7.4 > > -- > viresh >
diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c index e1d931c..d5b608d 100644 --- a/drivers/cpufreq/tegra194-cpufreq.c +++ b/drivers/cpufreq/tegra194-cpufreq.c @@ -180,9 +180,65 @@ 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 cpufreq_frequency_table *table, *pos; + struct cpufreq_policy policy; + unsigned int rate; + u64 ndiv; + int err; + + cpufreq_get_policy(&policy, cpu); + table = policy.freq_table; + + /* reconstruct actual cpu freq using counters*/ + rate = tegra194_get_speed_common(cpu, US_DELAY); + + /* get last written ndiv value*/ + err = smp_call_function_single(cpu, get_cpu_ndiv, &ndiv, true); + if (err) { + pr_err("cpufreq: Failed to get ndiv for CPU%d, ret:%d\n", + cpu, err); + 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, table) { + if (pos->driver_data != ndiv) + continue; + + if (abs(pos->frequency - rate) > 115200) { + pr_warn("cpufreq: high delta (%d) on CPU%d\n", + abs(pos->frequency - rate), cpu); + pr_warn("cpufreq: cur:%u, set:%u, set ndiv:%llu\n", + rate, pos->frequency, ndiv); + } else { + rate = pos->frequency; + } + break; + } + return rate; } static int tegra194_cpufreq_init(struct cpufreq_policy *policy) @@ -209,14 +265,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 | 66 ++++++++++++++++++++++++++++++++------ 1 file changed, 57 insertions(+), 9 deletions(-)