| Message ID | 20200712100645.13927-1-jonathanh@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [1/2] cpufreq: tegra186: Fix initial frequency | expand |
On 12-07-20, 11:06, Jon Hunter wrote: > Commit 6cc3d0e9a097 ("cpufreq: tegra186: add > CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for > Tegra186 but as a consequence the following warnings are now seen on > boot ... > > cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz > cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz > cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz > cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz > cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz > cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz > cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz > cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz > cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz > cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz > cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz > cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz > > Although we could fix this by adding a 'get' operator for the Tegra186 > CPUFREQ driver, there is really little point because the CPUFREQ on > Tegra186 is set by writing a value stored in the frequency table to a > register and we just need to set the initial frequency. The hardware still runs at the frequency requested by cpufreq core here, right ? It is better to provide the get() callback as it is also used to show the current frequency in userspace. > So for Tegra186 > the simplest way to fix this is read the register that sets the > frequency for each CPU and set the initial frequency when initialising > the CPUFREQ driver. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/cpufreq/tegra186-cpufreq.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c > index 3d2f143748ef..c44190ce3f03 100644 > --- a/drivers/cpufreq/tegra186-cpufreq.c > +++ b/drivers/cpufreq/tegra186-cpufreq.c > @@ -59,6 +59,7 @@ static int tegra186_cpufreq_init(struct cpufreq_policy *policy) > struct tegra186_cpufreq_cluster *cluster = &data->clusters[i]; > const struct tegra186_cpufreq_cluster_info *info = > cluster->info; > + u32 edvd_val; > int core; > > for (core = 0; core < ARRAY_SIZE(info->cpus); core++) { > @@ -71,6 +72,13 @@ static int tegra186_cpufreq_init(struct cpufreq_policy *policy) > policy->driver_data = > data->regs + info->offset + EDVD_CORE_VOLT_FREQ(core); > policy->freq_table = cluster->table; > + > + edvd_val = readl(policy->driver_data); > + > + for (i = 0; cluster->table[i].frequency != CPUFREQ_TABLE_END; i++) { > + if (cluster->table[i].driver_data == edvd_val) > + policy->cur = cluster->table[i].frequency; > + } > break; > } > > -- > 2.17.1
On 13/07/2020 04:25, Viresh Kumar wrote: > On 12-07-20, 11:06, Jon Hunter wrote: >> Commit 6cc3d0e9a097 ("cpufreq: tegra186: add >> CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for >> Tegra186 but as a consequence the following warnings are now seen on >> boot ... >> >> cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz >> cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz >> cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz >> cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz >> cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz >> cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz >> cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz >> cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz >> cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz >> cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz >> cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz >> cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz >> >> Although we could fix this by adding a 'get' operator for the Tegra186 >> CPUFREQ driver, there is really little point because the CPUFREQ on >> Tegra186 is set by writing a value stored in the frequency table to a >> register and we just need to set the initial frequency. > > The hardware still runs at the frequency requested by cpufreq core here, right ? Yes. > It is better to provide the get() callback as it is also used to show the > current frequency in userspace. I looked at that and I saw that if the get() callback is not provided, the current frequency showed by userspace is policy->cur. For this device, policy->cur is accurate and so if we added the get() callback we essentially just going to return policy->cur. Therefore, given that we already know policy->cur, I did not see the point in adding a device specific handler to do the same thing. Cheers Jon
On 13-07-20, 17:37, Jon Hunter wrote: > > On 13/07/2020 04:25, Viresh Kumar wrote: > > On 12-07-20, 11:06, Jon Hunter wrote: > >> Commit 6cc3d0e9a097 ("cpufreq: tegra186: add > >> CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for > >> Tegra186 but as a consequence the following warnings are now seen on > >> boot ... > >> > >> cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz > >> cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz > >> cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz > >> cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz > >> cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz > >> cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz > >> cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz > >> cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz > >> cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz > >> cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz > >> cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz > >> cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz > >> > >> Although we could fix this by adding a 'get' operator for the Tegra186 > >> CPUFREQ driver, there is really little point because the CPUFREQ on > >> Tegra186 is set by writing a value stored in the frequency table to a > >> register and we just need to set the initial frequency. > > > > The hardware still runs at the frequency requested by cpufreq core here, right ? > > Yes. > > > It is better to provide the get() callback as it is also used to show the > > current frequency in userspace. > > I looked at that and I saw that if the get() callback is not provided, > the current frequency showed by userspace is policy->cur. For this > device, policy->cur is accurate and so if we added the get() callback we > essentially just going to return policy->cur. Therefore, given that we > already know policy->cur, I did not see the point in adding a device > specific handler to do the same thing. The get() callback is supposed to read the frequency from hardware and return it, no cached value here. policy->cur may end up being wrong in case there is a bug.
On 14/07/2020 04:46, Viresh Kumar wrote: > On 13-07-20, 17:37, Jon Hunter wrote: >> >> On 13/07/2020 04:25, Viresh Kumar wrote: >>> On 12-07-20, 11:06, Jon Hunter wrote: >>>> Commit 6cc3d0e9a097 ("cpufreq: tegra186: add >>>> CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for >>>> Tegra186 but as a consequence the following warnings are now seen on >>>> boot ... >>>> >>>> cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz >>>> cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz >>>> cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz >>>> cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz >>>> cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz >>>> cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz >>>> cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz >>>> cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz >>>> cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz >>>> cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz >>>> cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz >>>> cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz >>>> >>>> Although we could fix this by adding a 'get' operator for the Tegra186 >>>> CPUFREQ driver, there is really little point because the CPUFREQ on >>>> Tegra186 is set by writing a value stored in the frequency table to a >>>> register and we just need to set the initial frequency. >>> >>> The hardware still runs at the frequency requested by cpufreq core here, right ? >> >> Yes. >> >>> It is better to provide the get() callback as it is also used to show the >>> current frequency in userspace. >> >> I looked at that and I saw that if the get() callback is not provided, >> the current frequency showed by userspace is policy->cur. For this >> device, policy->cur is accurate and so if we added the get() callback we >> essentially just going to return policy->cur. Therefore, given that we >> already know policy->cur, I did not see the point in adding a device >> specific handler to do the same thing. > > The get() callback is supposed to read the frequency from hardware and > return it, no cached value here. policy->cur may end up being wrong in > case there is a bug. OK, I can add a get callback. However, there are a few other drivers that set the current frequency in the init() and don't implement a get() callback ... drivers/cpufreq/pasemi-cpufreq.c drivers/cpufreq/ppc_cbe_cpufreq.c drivers/cpufreq/intel_pstate.c Jon
On 14-07-20, 08:26, Jon Hunter wrote: > OK, I can add a get callback. However, there are a few other drivers > that set the current frequency in the init() and don't implement a get() > callback ... > > drivers/cpufreq/pasemi-cpufreq.c > drivers/cpufreq/ppc_cbe_cpufreq.c These are quite old and I am not sure why they didn't do it. > drivers/cpufreq/intel_pstate.c With intel-pstate driver, the firmware sets the frequency of the CPUs and it can be different from what cpufreq core has asked it to. And so the kernel normally has no idea of what the frequency a CPU is running at.
Hi Viresh, On 14/07/2020 04:46, Viresh Kumar wrote: ... > The get() callback is supposed to read the frequency from hardware and > return it, no cached value here. policy->cur may end up being wrong in > case there is a bug. I have been doing some more testing on Tegra, I noticed that when reading the current CPU frequency via the sysfs scaling_cur_freq entry, this always returns the cached value (at least for Tegra). Looking at the implementation of scaling_cur_freq I see ... static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf) { ssize_t ret; unsigned int freq; freq = arch_freq_get_on_cpu(policy->cpu); if (freq) ret = sprintf(buf, "%u\n", freq); else if (cpufreq_driver && cpufreq_driver->setpolicy && cpufreq_driver->get) ret = sprintf(buf, "%u\n", cpufreq_driver->get(policy->cpu)); else ret = sprintf(buf, "%u\n", policy->cur); return ret; } The various Tegra CPU frequency drivers do not implement the set_policy callback and hence why we always get the cached value. I see the following commit added this and before it simply return the cached value ... commit c034b02e213d271b98c45c4a7b54af8f69aaac1e Author: Dirk Brandewie <dirk.j.brandewie@intel.com> Date: Mon Oct 13 08:37:40 2014 -0700 cpufreq: expose scaling_cur_freq sysfs file for set_policy() drivers Is this intentional? Cheers Jon
On 31-07-20, 13:14, Jon Hunter wrote: > I have been doing some more testing on Tegra, I noticed that when > reading the current CPU frequency via the sysfs scaling_cur_freq entry, > this always returns the cached value (at least for Tegra). Looking at > the implementation of scaling_cur_freq I see ... > > static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf) > { > ssize_t ret; > unsigned int freq; > > freq = arch_freq_get_on_cpu(policy->cpu); > if (freq) > ret = sprintf(buf, "%u\n", freq); > else if (cpufreq_driver && cpufreq_driver->setpolicy && > cpufreq_driver->get) > ret = sprintf(buf, "%u\n", cpufreq_driver->get(policy->cpu)); > else > ret = sprintf(buf, "%u\n", policy->cur); > return ret; > } > > The various Tegra CPU frequency drivers do not implement the > set_policy callback and hence why we always get the cached value. I > see the following commit added this and before it simply return the > cached value ... > > commit c034b02e213d271b98c45c4a7b54af8f69aaac1e > Author: Dirk Brandewie <dirk.j.brandewie@intel.com> > Date: Mon Oct 13 08:37:40 2014 -0700 > > cpufreq: expose scaling_cur_freq sysfs file for set_policy() drivers > > Is this intentional? Yes, it is. There are two sysfs files to read the current frequency. - scaling_cur_freq: as you noticed it returns cached value unless it is for setpolicy drivers in whose case cpufreq core doesn't control the frequency and so doesn't cache any values. - cpuinfo_cur_freq: This will return the value as read from hardware using ->get() callback.
diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c index 3d2f143748ef..c44190ce3f03 100644 --- a/drivers/cpufreq/tegra186-cpufreq.c +++ b/drivers/cpufreq/tegra186-cpufreq.c @@ -59,6 +59,7 @@ static int tegra186_cpufreq_init(struct cpufreq_policy *policy) struct tegra186_cpufreq_cluster *cluster = &data->clusters[i]; const struct tegra186_cpufreq_cluster_info *info = cluster->info; + u32 edvd_val; int core; for (core = 0; core < ARRAY_SIZE(info->cpus); core++) { @@ -71,6 +72,13 @@ static int tegra186_cpufreq_init(struct cpufreq_policy *policy) policy->driver_data = data->regs + info->offset + EDVD_CORE_VOLT_FREQ(core); policy->freq_table = cluster->table; + + edvd_val = readl(policy->driver_data); + + for (i = 0; cluster->table[i].frequency != CPUFREQ_TABLE_END; i++) { + if (cluster->table[i].driver_data == edvd_val) + policy->cur = cluster->table[i].frequency; + } break; }
Commit 6cc3d0e9a097 ("cpufreq: tegra186: add CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for Tegra186 but as a consequence the following warnings are now seen on boot ... cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz Although we could fix this by adding a 'get' operator for the Tegra186 CPUFREQ driver, there is really little point because the CPUFREQ on Tegra186 is set by writing a value stored in the frequency table to a register and we just need to set the initial frequency. So for Tegra186 the simplest way to fix this is read the register that sets the frequency for each CPU and set the initial frequency when initialising the CPUFREQ driver. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/cpufreq/tegra186-cpufreq.c | 8 ++++++++ 1 file changed, 8 insertions(+)