[PATCH v2 0/2] Tegra194 cpufreq driver misc changes

[PATCH v2 0/2] Tegra194 cpufreq driver misc changes

[Sumit Gupta]

This patch set has below two changes:
1) get consistent cpuinfo_cur_freq value from freq_table.
2) Fix unlisted boot freq warning by setting closest higher
   freq from freq_table if the boot frequency gets filtered while
   creating freq_table in kernel.

v1[1] -> v2:
- Minor changes to improve comments and reduce debug prints.
- Get freq table from cluster specific data instead of policy.
- Set a freq from freq_table if boot freq is not present in table.
- Add online hook to fix unlisted boot freq warning in hotplug-on.

Sumit Gupta (2):
  cpufreq: tegra194: get consistent cpuinfo_cur_freq
  cpufreq: tegra194: Fix unlisted boot freq warning

 drivers/cpufreq/tegra194-cpufreq.c | 153 +++++++++++++++++++++++++++++++++----
 1 file changed, 139 insertions(+), 14 deletions(-)

[1] https://marc.info/?l=linux-arm-kernel&m=160028821117535&w=2

-- 
2.7.4

[PATCH v2 1/2] cpufreq: tegra194: get consistent cpuinfo_cur_freq

[Sumit Gupta]

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)
+{
+	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)
 {
-- 
2.7.4

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

[Sumit Gupta]

Warning coming during boot because the boot freq set by bootloader
gets filtered out due to big freq steps while creating freq_table.
Fix this by setting closest higher frequency from freq_table.
Warning:
  cpufreq: cpufreq_online: CPU0: Running at unlisted freq
  cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed

These warning messages also come during hotplug online of non-boot
CPU's while exiting from 'Suspend-to-RAM'. This happens because
during exit from 'Suspend-to-RAM', some time is taken to restore
last software requested CPU frequency written in register before
entering suspend. To fix this, adding online hook to wait till the
current frequency becomes equal or close to the last requested
frequency.

Fixes: df320f89359c ("cpufreq: Add Tegra194 cpufreq driver")
Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/cpufreq/tegra194-cpufreq.c | 86 ++++++++++++++++++++++++++++++++++----
 1 file changed, 79 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c
index d250e49..cc28b1e3 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
 
@@ -249,17 +249,22 @@ static unsigned int tegra194_get_speed(u32 cpu)
 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 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);
@@ -267,7 +272,23 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
 	policy->freq_table = data->tables[cl];
 	policy->cpuinfo.transition_latency = TEGRA_CPUFREQ_TRANSITION_LATENCY;
 
-	return 0;
+	policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY);
+
+	ret = cpufreq_table_validate_and_sort(policy);
+	if (ret)
+		return ret;
+
+	/* Are we running at unknown frequency ? */
+	ret = cpufreq_frequency_table_get_index(policy, policy->cur);
+	if (ret == -EINVAL) {
+		ret = __cpufreq_driver_target(policy, policy->cur - 1,
+					      CPUFREQ_RELATION_L);
+		if (ret)
+			return ret;
+		policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY);
+	}
+
+	return ret;
 }
 
 static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
@@ -285,6 +306,55 @@ static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
 	return 0;
 }
 
+static int tegra194_cpufreq_online(struct cpufreq_policy *policy)
+{
+	unsigned int interm_freq, last_set_freq;
+	struct cpufreq_frequency_table *pos;
+	u64 ndiv;
+	int ret;
+
+	if (!cpu_online(policy->cpu))
+		return -EINVAL;
+
+	/* get ndiv for the last frequency request from software  */
+	ret = smp_call_function_single(policy->cpu, get_cpu_ndiv, &ndiv, true);
+	if (ret) {
+		pr_err("cpufreq: Failed to get ndiv for CPU%d\n", policy->cpu);
+		return ret;
+	}
+
+	cpufreq_for_each_valid_entry(pos, policy->freq_table) {
+		if (pos->driver_data == ndiv) {
+			last_set_freq = pos->frequency;
+			break;
+		}
+	}
+
+	policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY);
+	interm_freq =  policy->cur;
+
+	/*
+	 * 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 - last_set_freq) <= 115200, 0,
+				100 * USEC_PER_MSEC, false, policy->cpu,
+				US_DELAY);
+	if (ret)
+		pr_warn("cpufreq: cpu%d, cur:%u, last set:%u, intermed:%u\n",
+			policy->cpu, policy->cur, last_set_freq, interm_freq);
+
+	return ret;
+}
+
+static int tegra194_cpufreq_offline(struct cpufreq_policy *policy)
+{
+	return 0;
+}
+
 static struct cpufreq_driver tegra194_cpufreq_driver = {
 	.name = "tegra194",
 	.flags = CPUFREQ_STICKY | CPUFREQ_CONST_LOOPS |
@@ -294,6 +364,8 @@ static struct cpufreq_driver tegra194_cpufreq_driver = {
 	.get = tegra194_get_speed,
 	.init = tegra194_cpufreq_init,
 	.attr = cpufreq_generic_attr,
+	.online = tegra194_cpufreq_online,
+	.offline = tegra194_cpufreq_offline,
 };
 
 static void tegra194_cpufreq_free_resources(void)
-- 
2.7.4

Re: [PATCH v2 1/2] cpufreq: tegra194: get consistent cpuinfo_cur_freq

[Viresh Kumar]

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;
>  }

-- 
viresh

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

[Viresh Kumar]

On 08-10-20, 18:31, 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.
> Fix this by setting closest higher frequency from freq_table.
> Warning:
>   cpufreq: cpufreq_online: CPU0: Running at unlisted freq
>   cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed
> 
> These warning messages also come during hotplug online of non-boot
> CPU's while exiting from 'Suspend-to-RAM'. This happens because
> during exit from 'Suspend-to-RAM', some time is taken to restore
> last software requested CPU frequency written in register before
> entering suspend.

And who does this restoration ?

> To fix this, adding online hook to wait till the
> current frequency becomes equal or close to the last requested
> frequency.
> 
> Fixes: df320f89359c ("cpufreq: Add Tegra194 cpufreq driver")
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> ---
>  drivers/cpufreq/tegra194-cpufreq.c | 86 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 79 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c
> index d250e49..cc28b1e3 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
>  
> @@ -249,17 +249,22 @@ static unsigned int tegra194_get_speed(u32 cpu)
>  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 ret;
>  	u32 cl;
>  
> -	smp_call_function_single(policy->cpu, get_cpu_cluster, &cl, true);
> +	if (!cpu_online(cpu))

Not required to check this.

> +		return -EINVAL;
> +
> +	ret = smp_call_function_single(cpu, get_cpu_cluster, &cl, true);
> +	if (ret) {

Same as in the other patch.

> +		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);
> @@ -267,7 +272,23 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
>  	policy->freq_table = data->tables[cl];
>  	policy->cpuinfo.transition_latency = TEGRA_CPUFREQ_TRANSITION_LATENCY;
>  
> -	return 0;
> +	policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY);
> +
> +	ret = cpufreq_table_validate_and_sort(policy);
> +	if (ret)
> +		return ret;
> +
> +	/* Are we running at unknown frequency ? */
> +	ret = cpufreq_frequency_table_get_index(policy, policy->cur);
> +	if (ret == -EINVAL) {
> +		ret = __cpufreq_driver_target(policy, policy->cur - 1,
> +					      CPUFREQ_RELATION_L);
> +		if (ret)
> +			return ret;

> +		policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY);

cpufreq-core will do this anyway, you don't need to do it.

> +	}
> +
> +	return ret;
>  }

I wonder if I should change the pr_warn() in cpufreq-core to pr_info()
instead, will that help you guys ? Will that still be a problem ? This
is exactly same as what we do there.

>  static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
> @@ -285,6 +306,55 @@ static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
>  	return 0;
>  }
>  
> +static int tegra194_cpufreq_online(struct cpufreq_policy *policy)
> +{
> +	unsigned int interm_freq, last_set_freq;
> +	struct cpufreq_frequency_table *pos;
> +	u64 ndiv;
> +	int ret;
> +
> +	if (!cpu_online(policy->cpu))
> +		return -EINVAL;
> +
> +	/* get ndiv for the last frequency request from software  */
> +	ret = smp_call_function_single(policy->cpu, get_cpu_ndiv, &ndiv, true);
> +	if (ret) {
> +		pr_err("cpufreq: Failed to get ndiv for CPU%d\n", policy->cpu);
> +		return ret;
> +	}
> +
> +	cpufreq_for_each_valid_entry(pos, policy->freq_table) {
> +		if (pos->driver_data == ndiv) {
> +			last_set_freq = pos->frequency;
> +			break;
> +		}
> +	}
> +
> +	policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY);
> +	interm_freq =  policy->cur;
> +
> +	/*
> +	 * 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 - last_set_freq) <= 115200, 0,
> +				100 * USEC_PER_MSEC, false, policy->cpu,
> +				US_DELAY);

The firmware does this update ? Why do we need to wait for this ? I
was actually suggesting an empty tegra194_cpufreq_online() routine
here.

-- 
viresh

Re: [PATCH v2 1/2] cpufreq: tegra194: get consistent cpuinfo_cur_freq

[Sumit Gupta]

>> 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
> 

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

[Sumit Gupta]

>> Warning coming during boot because the boot freq set by bootloader
>> gets filtered out due to big freq steps while creating freq_table.
>> Fix this by setting closest higher frequency from freq_table.
>> Warning:
>>    cpufreq: cpufreq_online: CPU0: Running at unlisted freq
>>    cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed
>>
>> These warning messages also come during hotplug online of non-boot
>> CPU's while exiting from 'Suspend-to-RAM'. This happens because
>> during exit from 'Suspend-to-RAM', some time is taken to restore
>> last software requested CPU frequency written in register before
>> entering suspend.
> 
> And who does this restoration ?
> 
>> To fix this, adding online hook to wait till the
>> current frequency becomes equal or close to the last requested
>> frequency.
>>
>> Fixes: df320f89359c ("cpufreq: Add Tegra194 cpufreq driver")
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   drivers/cpufreq/tegra194-cpufreq.c | 86 ++++++++++++++++++++++++++++++++++----
>>   1 file changed, 79 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/cpufreq/tegra194-cpufreq.c b/drivers/cpufreq/tegra194-cpufreq.c
>> index d250e49..cc28b1e3 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
>>
>> @@ -249,17 +249,22 @@ static unsigned int tegra194_get_speed(u32 cpu)
>>   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 ret;
>>        u32 cl;
>>
>> -     smp_call_function_single(policy->cpu, get_cpu_cluster, &cl, true);
>> +     if (!cpu_online(cpu))
> 
> Not required to check this.
> 
OK.

>> +             return -EINVAL;
>> +
>> +     ret = smp_call_function_single(cpu, get_cpu_cluster, &cl, true);
>> +     if (ret) {
> 
> Same as in the other patch.
> 
Got.

>> +             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);
>> @@ -267,7 +272,23 @@ static int tegra194_cpufreq_init(struct cpufreq_policy *policy)
>>        policy->freq_table = data->tables[cl];
>>        policy->cpuinfo.transition_latency = TEGRA_CPUFREQ_TRANSITION_LATENCY;
>>
>> -     return 0;
>> +     policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY);
>> +
>> +     ret = cpufreq_table_validate_and_sort(policy);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Are we running at unknown frequency ? */
>> +     ret = cpufreq_frequency_table_get_index(policy, policy->cur);
>> +     if (ret == -EINVAL) {
>> +             ret = __cpufreq_driver_target(policy, policy->cur - 1,
>> +                                           CPUFREQ_RELATION_L);
>> +             if (ret)
>> +                     return ret;
> 
>> +             policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY);
> 
> cpufreq-core will do this anyway, you don't need to do it.
> 
Got.

>> +     }
>> +
>> +     return ret;
>>   }
> 
> I wonder if I should change the pr_warn() in cpufreq-core to pr_info()
> instead, will that help you guys ? Will that still be a problem ? This
> is exactly same as what we do there.
> 
Yes, this will also work. Then we don't need the current patch.
You want me to send a patch with change from pr_warn to pr_info?

>>   static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
>> @@ -285,6 +306,55 @@ static int tegra194_cpufreq_set_target(struct cpufreq_policy *policy,
>>        return 0;
>>   }
>>
>> +static int tegra194_cpufreq_online(struct cpufreq_policy *policy)
>> +{
>> +     unsigned int interm_freq, last_set_freq;
>> +     struct cpufreq_frequency_table *pos;
>> +     u64 ndiv;
>> +     int ret;
>> +
>> +     if (!cpu_online(policy->cpu))
>> +             return -EINVAL;
>> +
>> +     /* get ndiv for the last frequency request from software  */
>> +     ret = smp_call_function_single(policy->cpu, get_cpu_ndiv, &ndiv, true);
>> +     if (ret) {
>> +             pr_err("cpufreq: Failed to get ndiv for CPU%d\n", policy->cpu);
>> +             return ret;
>> +     }
>> +
>> +     cpufreq_for_each_valid_entry(pos, policy->freq_table) {
>> +             if (pos->driver_data == ndiv) {
>> +                     last_set_freq = pos->frequency;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     policy->cur = tegra194_get_speed_common(policy->cpu, US_DELAY);
>> +     interm_freq =  policy->cur;
>> +
>> +     /*
>> +      * 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 - last_set_freq) <= 115200, 0,
>> +                             100 * USEC_PER_MSEC, false, policy->cpu,
>> +                             US_DELAY);
> 
> The firmware does this update ? Why do we need to wait for this ? I
> was actually suggesting an empty tegra194_cpufreq_online() routine
> here.
> > --
> viresh
> 

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

[Viresh Kumar]

On 12-10-20, 22:36, Sumit Gupta wrote:
> Yes, this will also work. Then we don't need the current patch.
> You want me to send a patch with change from pr_warn to pr_info?

I have sent one.

-- 
viresh