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