diff mbox series

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

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

Commit Message

Sumit Gupta Oct. 8, 2020, 1:01 p.m. UTC
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(-)

Comments

Viresh Kumar Oct. 12, 2020, 5:22 a.m. UTC | #1
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;
>  }
Sumit Gupta Oct. 12, 2020, 4:34 p.m. UTC | #2
>> 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 mbox series

Patch

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