diff mbox series

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

Message ID 1600276277-7290-2-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
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(-)

Comments

Jon Hunter Sept. 17, 2020, 8:36 a.m. UTC | #1
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
Jon Hunter Sept. 17, 2020, 8:44 a.m. UTC | #2
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
Jon Hunter Sept. 23, 2020, 8:34 a.m. UTC | #3
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
Thierry Reding Sept. 24, 2020, 8:56 a.m. UTC | #4
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>
Viresh Kumar Oct. 5, 2020, 4:34 a.m. UTC | #5
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 ?
Viresh Kumar Oct. 5, 2020, 4:46 a.m. UTC | #6
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
Jon Hunter Oct. 5, 2020, 9:12 a.m. UTC | #7
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
Sumit Gupta Oct. 5, 2020, 6:40 p.m. UTC | #8
>> 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 mbox series

Patch

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