diff mbox series

[2/2] cpufreq: tegra194: Fix unlisted boot freq warning

Message ID 1600276277-7290-3-git-send-email-sumitg@nvidia.com
State New, archived
Headers show
Series Tegra194 cpufreq driver misc changes | expand

Commit Message

Sumit Gupta Sept. 16, 2020, 5:11 p.m. UTC
Warning coming during boot because the boot freq set by bootloader
gets filtered out due to big freq steps while creating freq_table.
Fixing this by setting closest ndiv value from freq_table.
Warning:
  cpufreq: cpufreq_online: CPU0: Running at unlisted freq
  cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed

Also, added change in init to wait till current frequency becomes
equal or near to the previously requested frequency. This is done
because it takes some time to restore the previous frequency while
turning-on non-boot cores during exit from SC7(Suspend-to-RAM).

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/cpufreq/tegra194-cpufreq.c | 118 ++++++++++++++++++++++++++++++++++---
 1 file changed, 111 insertions(+), 7 deletions(-)

Comments

Jon Hunter Sept. 17, 2020, 8:38 a.m. UTC | #1
On 16/09/2020 18:11, Sumit Gupta wrote:
> Warning coming during boot because the boot freq set by bootloader
> gets filtered out due to big freq steps while creating freq_table.
> Fixing this by setting closest ndiv value from freq_table.
> Warning:
>   cpufreq: cpufreq_online: CPU0: Running at unlisted freq
>   cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed
> 
> Also, added change in init to wait till current frequency becomes
> equal or near to the previously requested frequency. This is done
> because it takes some time to restore the previous frequency while
> turning-on non-boot cores during exit from SC7(Suspend-to-RAM).

Same here ...

Fixes: df320f89359c ("cpufreq: Add Tegra194 cpufreq driver")

> 
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Viresh, this is also needed for v5.9.

Thanks
Jon
Jon Hunter Sept. 17, 2020, 8:45 a.m. UTC | #2
On 17/09/2020 09:38, Jon Hunter wrote:
> 
> On 16/09/2020 18:11, Sumit Gupta wrote:
>> Warning coming during boot because the boot freq set by bootloader
>> gets filtered out due to big freq steps while creating freq_table.
>> Fixing this by setting closest ndiv value from freq_table.
>> Warning:
>>   cpufreq: cpufreq_online: CPU0: Running at unlisted freq
>>   cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed
>>
>> Also, added change in init to wait till current frequency becomes
>> equal or near to the previously requested frequency. This is done
>> because it takes some time to restore the previous frequency while
>> turning-on non-boot cores during exit from SC7(Suspend-to-RAM).
> 
> Same here ...
> 
> Fixes: df320f89359c ("cpufreq: Add Tegra194 cpufreq driver")
> 
>>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
> Tested-by: Jon Hunter <jonathanh@nvidia.com>
> 
> Viresh, this is also needed for v5.9.

Adding Sudeep.

Jon
Thierry Reding Sept. 24, 2020, 8:56 a.m. UTC | #3
On Wed, Sep 16, 2020 at 10:41:17PM +0530, Sumit Gupta wrote:
> Warning coming during boot because the boot freq set by bootloader
> gets filtered out due to big freq steps while creating freq_table.
> Fixing this by setting closest ndiv value from freq_table.
> Warning:
>   cpufreq: cpufreq_online: CPU0: Running at unlisted freq
>   cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed
> 
> Also, added change in init to wait till current frequency becomes
> equal or near to the previously requested frequency. This is done
> because it takes some time to restore the previous frequency while
> turning-on non-boot cores during exit from SC7(Suspend-to-RAM).
> 
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> ---
>  drivers/cpufreq/tegra194-cpufreq.c | 118 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 111 insertions(+), 7 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>
Viresh Kumar Oct. 5, 2020, 4:54 a.m. UTC | #4
On 16-09-20, 22:41, Sumit Gupta wrote:
> Warning coming during boot because the boot freq set by bootloader
> gets filtered out due to big freq steps while creating freq_table.
> Fixing this by setting closest ndiv value from freq_table.
> Warning:
>   cpufreq: cpufreq_online: CPU0: Running at unlisted freq
>   cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed
> 
> Also, added change in init to wait till current frequency becomes
> equal or near to the previously requested frequency. This is done
> because it takes some time to restore the previous frequency while
> turning-on non-boot cores during exit from SC7(Suspend-to-RAM).

So you are trying to figure if the frequency is listed in freq-table or not,
otherwise setting the frequency to a value you think is appropriate. Right ?

This is what the cpufreq core already does when it printed these boot time
messages. Do we really need to add this much code in here ?

If you really don't want to see the warning, how about fixing it the way cpufreq
core does ? i.e. with this call:

ret = __cpufreq_driver_target(policy, policy->cur - 1, CPUFREQ_RELATION_L);
Sumit Gupta Oct. 5, 2020, 6:54 p.m. UTC | #5
>> Warning coming during boot because the boot freq set by bootloader
>> gets filtered out due to big freq steps while creating freq_table.
>> Fixing this by setting closest ndiv value from freq_table.
>> Warning:
>>    cpufreq: cpufreq_online: CPU0: Running at unlisted freq
>>    cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed
>>
>> Also, added change in init to wait till current frequency becomes
>> equal or near to the previously requested frequency. This is done
>> because it takes some time to restore the previous frequency while
>> turning-on non-boot cores during exit from SC7(Suspend-to-RAM).
> 
> So you are trying to figure if the frequency is listed in freq-table or not,
> otherwise setting the frequency to a value you think is appropriate. Right ?
During boot, want to set the frequency from freq_table which is closest 
to the one set by bootloader.
During resume from suspend-to-idle, want to restore the frequency which 
was set on non-boot cores before they were hotplug powered off.

> 
> This is what the cpufreq core already does when it printed these boot time
> messages. Do we really need to add this much code in here ?
We want to avoid the warning messages.

> 
> If you really don't want to see the warning, how about fixing it the way cpufreq
> core does ? i.e. with this call:
> 
> ret = __cpufreq_driver_target(policy, policy->cur - 1, CPUFREQ_RELATION_L);
> 
The cpufreq core change will help in bootup case but not during the case 
of resume.
In this change, reading the previously stored value and restoring it 
will also fix the warning message during resume.


> --
> viresh
>
Viresh Kumar Oct. 6, 2020, 5:38 a.m. UTC | #6
On 06-10-20, 00:24, Sumit Gupta wrote:
> 
> > > Warning coming during boot because the boot freq set by bootloader
> > > gets filtered out due to big freq steps while creating freq_table.
> > > Fixing this by setting closest ndiv value from freq_table.
> > > Warning:
> > >    cpufreq: cpufreq_online: CPU0: Running at unlisted freq
> > >    cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed
> > > 
> > > Also, added change in init to wait till current frequency becomes
> > > equal or near to the previously requested frequency. This is done
> > > because it takes some time to restore the previous frequency while
> > > turning-on non-boot cores during exit from SC7(Suspend-to-RAM).
> > 
> > So you are trying to figure if the frequency is listed in freq-table or not,
> > otherwise setting the frequency to a value you think is appropriate. Right ?
> During boot, want to set the frequency from freq_table which is closest to
> the one set by bootloader.

Right.

> During resume from suspend-to-idle, want to restore the frequency which was
> set on non-boot cores before they were hotplug powered off.

Why exactly do you want to do that ? Rather you should provide
online/offline hooks for the cpufreq driver and do light-weight
suspend/resume as is done by cpufreq-dt.c as well.

> > 
> > This is what the cpufreq core already does when it printed these boot time
> > messages. Do we really need to add this much code in here ?
> We want to avoid the warning messages.

Hmm, okay.

> > 
> > If you really don't want to see the warning, how about fixing it the way cpufreq
> > core does ? i.e. with this call:
> > 
> > ret = __cpufreq_driver_target(policy, policy->cur - 1, CPUFREQ_RELATION_L);
> > 
> The cpufreq core change will help in bootup case but not during the case of
> resume.
> In this change, reading the previously stored value and restoring it will
> also fix the warning message during resume.

You were getting the message during resume as well ? Why ? The
firmware is updating the frequency to a previous value ? If that is
so, you should just set the frequency again to some other value during
resume to fix it.
Sumit Gupta Oct. 8, 2020, 11:11 a.m. UTC | #7
>>
>>>> Warning coming during boot because the boot freq set by bootloader
>>>> gets filtered out due to big freq steps while creating freq_table.
>>>> Fixing this by setting closest ndiv value from freq_table.
>>>> Warning:
>>>>     cpufreq: cpufreq_online: CPU0: Running at unlisted freq
>>>>     cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed
>>>>
>>>> Also, added change in init to wait till current frequency becomes
>>>> equal or near to the previously requested frequency. This is done
>>>> because it takes some time to restore the previous frequency while
>>>> turning-on non-boot cores during exit from SC7(Suspend-to-RAM).
>>>
>>> So you are trying to figure if the frequency is listed in freq-table or not,
>>> otherwise setting the frequency to a value you think is appropriate. Right ?
>> During boot, want to set the frequency from freq_table which is closest to
>> the one set by bootloader.
> 
> Right.
> 
>> During resume from suspend-to-idle, want to restore the frequency which was
>> set on non-boot cores before they were hotplug powered off.
> 
> Why exactly do you want to do that ? Rather you should provide
> online/offline hooks for the cpufreq driver and do light-weight
> suspend/resume as is done by cpufreq-dt.c as well.
> 
Thank you for pointer. Added online hook to avoid warning during 
hot-plug-on for the non-boot CPU's while exiting from Suspend-to-RAM. Will
send new version with the changes.

>>>
>>> This is what the cpufreq core already does when it printed these boot time
>>> messages. Do we really need to add this much code in here ?
>> We want to avoid the warning messages.
> 
> Hmm, okay.
> 
>>>
>>> If you really don't want to see the warning, how about fixing it the way cpufreq
>>> core does ? i.e. with this call:
>>>
>>> ret = __cpufreq_driver_target(policy, policy->cur - 1, CPUFREQ_RELATION_L);
>>>
>> The cpufreq core change will help in bootup case but not during the case of
>> resume.
>> In this change, reading the previously stored value and restoring it will
>> also fix the warning message during resume.
> 
> You were getting the message during resume as well ? Why ? The
> firmware is updating the frequency to a previous value ? If that is
> so, you should just set the frequency again to some other value during
> resume to fix it.
Yes, it boots at a predefined frequency and then some time is taken to 
restore the last frequency which software requested before entering 
Suspend-to-RAM. We don't need to re-write the register again.

> 
> --
> viresh
>
diff mbox series

Patch

diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c
index d5b608d..c3c058a3 100644
--- a/drivers/cpufreq/tegra194-cpufreq.c
+++ b/drivers/cpufreq/tegra194-cpufreq.c
@@ -7,6 +7,7 @@ 
 #include <linux/cpufreq.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
@@ -21,7 +22,6 @@ 
 #define KHZ                     1000
 #define REF_CLK_MHZ             408 /* 408 MHz */
 #define US_DELAY                500
-#define US_DELAY_MIN            2
 #define CPUFREQ_TBL_STEP_HZ     (50 * KHZ * KHZ)
 #define MAX_CNT                 ~0U
 
@@ -241,26 +241,130 @@  static unsigned int tegra194_get_speed(u32 cpu)
 	return rate;
 }
 
+static int
+freqtable_find_index_closest_ndiv(struct cpufreq_frequency_table *table,
+				  unsigned int target_ndiv)
+{
+	struct cpufreq_frequency_table *pos;
+	unsigned int ndiv;
+	int idx, best = -1;
+
+	cpufreq_for_each_valid_entry_idx(pos, table, idx) {
+		ndiv = pos->driver_data;
+
+		if (ndiv == target_ndiv)
+			return idx;
+
+		if (ndiv < target_ndiv) {
+			best = idx;
+			continue;
+		}
+
+		/* No ndiv found below target_ndiv */
+		if (best == -1)
+			return idx;
+
+		/* Choose the closest ndiv */
+		if (target_ndiv - table[best].driver_data > ndiv - target_ndiv)
+			return idx;
+
+		return best;
+	}
+
+	return best;
+}
+
+static int
+freqtable_set_closest_ndiv(struct cpufreq_frequency_table *freq_table,
+			   int cpu)
+{
+	u64 ndiv;
+	int idx, ret;
+
+	if (!cpu_online(cpu))
+		return -EINVAL;
+
+	/* get ndiv for the last frequency request from software  */
+	ret = smp_call_function_single(cpu, get_cpu_ndiv, &ndiv, true);
+	if (ret) {
+		pr_err("cpufreq: Failed to get ndiv for CPU%d\n", cpu);
+		return ret;
+	}
+
+	/* while creating freq_table during boot, if the ndiv value got
+	 * filtered out due to large freq steps, then find closest ndiv
+	 * from freq_table and set that.
+	 */
+	idx = freqtable_find_index_closest_ndiv(freq_table, ndiv);
+
+	if (ndiv != freq_table[idx].driver_data) {
+		pr_debug("cpufreq: new freq:%d ndiv:%d, old ndiv:%llu\n",
+			 freq_table[idx].frequency,
+			 freq_table[idx].driver_data, ndiv);
+
+		ret = smp_call_function_single(cpu, set_cpu_ndiv,
+					       freq_table + idx, true);
+		if (ret) {
+			pr_err("cpufreq: setting ndiv for CPU%d failed\n",
+			       cpu);
+			return ret;
+		}
+	}
+
+	return freq_table[idx].frequency;
+}
+
 static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
 {
 	struct tegra194_cpufreq_data *data = cpufreq_get_driver_data();
-	u32 cpu;
+	u32 cpu = policy->cpu;
+	int new_freq, ret;
 	u32 cl;
 
-	smp_call_function_single(policy->cpu, get_cpu_cluster, &cl, true);
+	if (!cpu_online(cpu))
+		return -EINVAL;
+
+	ret = smp_call_function_single(cpu, get_cpu_cluster, &cl, true);
+	if (ret) {
+		pr_err("cpufreq: Failed to get cluster for CPU%d\n", cpu);
+		return ret;
+	}
 
 	if (cl >= data->num_clusters)
 		return -EINVAL;
 
-	/* boot freq */
-	policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY_MIN);
-
 	/* set same policy for all cpus in a cluster */
 	for (cpu = (cl * 2); cpu < ((cl + 1) * 2); cpu++)
 		cpumask_set_cpu(cpu, policy->cpus);
 
 	policy->freq_table = data->tables[cl];
-	policy->cpuinfo.transition_latency = TEGRA_CPUFREQ_TRANSITION_LATENCY;
+	policy->cpuinfo.transition_latency =
+		TEGRA_CPUFREQ_TRANSITION_LATENCY;
+
+	/* Find and set the closest ndiv from freq_table if the boot freq
+	 * already set is filtered out from freq_table or not present.
+	 */
+	new_freq = freqtable_set_closest_ndiv(policy->freq_table, policy->cpu);
+	if (new_freq < 0) {
+		pr_err("cpufreq: set closest ndiv for CPU%d failed(%d)\n",
+		       policy->cpu, new_freq);
+		return new_freq;
+	}
+
+	/* It takes some time to restore the previous frequency while
+	 * turning-on non-boot cores during exit from SC7(Suspend-to-RAM).
+	 * So, wait till it reaches the previous value and timeout if the
+	 * time taken to reach requested freq is >100ms
+	 */
+	ret = read_poll_timeout(tegra194_get_speed_common, policy->cur,
+				abs(policy->cur - new_freq) <= 115200, 0,
+				100 * USEC_PER_MSEC, false, policy->cpu,
+				US_DELAY);
+	if (ret)
+		pr_warn("cpufreq: time taken to restore freq >100ms\n");
+
+	pr_debug("cpufreq: cpu%d, curfreq:%d, setfreq:%d\n", policy->cpu,
+		 policy->cur, new_freq);
 
 	return 0;
 }