[PATCH 1/2] cpufreq: tegra186: Fix initial frequency

[PATCH 1/2] cpufreq: tegra186: Fix initial frequency

[Jon Hunter]

Commit 6cc3d0e9a097 ("cpufreq: tegra186: add
CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for
Tegra186 but as a consequence the following warnings are now seen on
boot ...

 cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz
 cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz
 cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz
 cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz
 cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz
 cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz
 cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz
 cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz
 cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz
 cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz
 cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz
 cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz

Although we could fix this by adding a 'get' operator for the Tegra186
CPUFREQ driver, there is really little point because the CPUFREQ on
Tegra186 is set by writing a value stored in the frequency table to a
register and we just need to set the initial frequency. So for Tegra186
the simplest way to fix this is read the register that sets the
frequency for each CPU and set the initial frequency when initialising
the CPUFREQ driver.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/cpufreq/tegra186-cpufreq.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
index 3d2f143748ef..c44190ce3f03 100644
--- a/drivers/cpufreq/tegra186-cpufreq.c
+++ b/drivers/cpufreq/tegra186-cpufreq.c
@@ -59,6 +59,7 @@ static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
 		struct tegra186_cpufreq_cluster *cluster = &data->clusters[i];
 		const struct tegra186_cpufreq_cluster_info *info =
 			cluster->info;
+		u32 edvd_val;
 		int core;
 
 		for (core = 0; core < ARRAY_SIZE(info->cpus); core++) {
@@ -71,6 +72,13 @@ static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
 		policy->driver_data =
 			data->regs + info->offset + EDVD_CORE_VOLT_FREQ(core);
 		policy->freq_table = cluster->table;
+
+		edvd_val = readl(policy->driver_data);
+
+		for (i = 0; cluster->table[i].frequency != CPUFREQ_TABLE_END; i++) {
+			if (cluster->table[i].driver_data == edvd_val)
+				policy->cur = cluster->table[i].frequency;
+		}
 		break;
 	}
 
-- 
2.17.1

[PATCH 2/2] cpufreq: tegra186: Simplify probe return path

[Jon Hunter]

We always put the reference to BPMP device on exit of the Tegra186
CPUFREQ driver and so there is no need to have separate exit paths
for success and failure. Therefore, simplify the probe return path
in the Tegra186 CPUFREQ driver by combining the success and failure
paths.

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/cpufreq/tegra186-cpufreq.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
index c44190ce3f03..bf8cab357277 100644
--- a/drivers/cpufreq/tegra186-cpufreq.c
+++ b/drivers/cpufreq/tegra186-cpufreq.c
@@ -231,15 +231,9 @@ static int tegra186_cpufreq_probe(struct platform_device *pdev)
 		}
 	}
 
-	tegra_bpmp_put(bpmp);
-
 	tegra186_cpufreq_driver.driver_data = data;
 
 	err = cpufreq_register_driver(&tegra186_cpufreq_driver);
-	if (err)
-		return err;
-
-	return 0;
 
 put_bpmp:
 	tegra_bpmp_put(bpmp);
-- 
2.17.1

Re: [PATCH 1/2] cpufreq: tegra186: Fix initial frequency

[Viresh Kumar]

On 12-07-20, 11:06, Jon Hunter wrote:
> Commit 6cc3d0e9a097 ("cpufreq: tegra186: add
> CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for
> Tegra186 but as a consequence the following warnings are now seen on
> boot ...
> 
>  cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz
>  cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz
>  cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz
>  cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz
>  cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz
>  cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz
>  cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz
>  cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz
>  cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz
>  cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz
>  cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz
>  cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz
> 
> Although we could fix this by adding a 'get' operator for the Tegra186
> CPUFREQ driver, there is really little point because the CPUFREQ on
> Tegra186 is set by writing a value stored in the frequency table to a
> register and we just need to set the initial frequency.

The hardware still runs at the frequency requested by cpufreq core here, right ?
It is better to provide the get() callback as it is also used to show the
current frequency in userspace.

> So for Tegra186
> the simplest way to fix this is read the register that sets the
> frequency for each CPU and set the initial frequency when initialising
> the CPUFREQ driver.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/cpufreq/tegra186-cpufreq.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
> index 3d2f143748ef..c44190ce3f03 100644
> --- a/drivers/cpufreq/tegra186-cpufreq.c
> +++ b/drivers/cpufreq/tegra186-cpufreq.c
> @@ -59,6 +59,7 @@ static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
>  		struct tegra186_cpufreq_cluster *cluster = &data->clusters[i];
>  		const struct tegra186_cpufreq_cluster_info *info =
>  			cluster->info;
> +		u32 edvd_val;
>  		int core;
>  
>  		for (core = 0; core < ARRAY_SIZE(info->cpus); core++) {
> @@ -71,6 +72,13 @@ static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
>  		policy->driver_data =
>  			data->regs + info->offset + EDVD_CORE_VOLT_FREQ(core);
>  		policy->freq_table = cluster->table;
> +
> +		edvd_val = readl(policy->driver_data);
> +
> +		for (i = 0; cluster->table[i].frequency != CPUFREQ_TABLE_END; i++) {
> +			if (cluster->table[i].driver_data == edvd_val)
> +				policy->cur = cluster->table[i].frequency;
> +		}
>  		break;
>  	}
>  
> -- 
> 2.17.1

-- 
viresh

Re: [PATCH 2/2] cpufreq: tegra186: Simplify probe return path

[Viresh Kumar]

On 12-07-20, 11:06, Jon Hunter wrote:
> We always put the reference to BPMP device on exit of the Tegra186
> CPUFREQ driver and so there is no need to have separate exit paths
> for success and failure. Therefore, simplify the probe return path
> in the Tegra186 CPUFREQ driver by combining the success and failure
> paths.
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/cpufreq/tegra186-cpufreq.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
> index c44190ce3f03..bf8cab357277 100644
> --- a/drivers/cpufreq/tegra186-cpufreq.c
> +++ b/drivers/cpufreq/tegra186-cpufreq.c
> @@ -231,15 +231,9 @@ static int tegra186_cpufreq_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	tegra_bpmp_put(bpmp);
> -
>  	tegra186_cpufreq_driver.driver_data = data;
>  
>  	err = cpufreq_register_driver(&tegra186_cpufreq_driver);
> -	if (err)
> -		return err;
> -
> -	return 0;
>  
>  put_bpmp:
>  	tegra_bpmp_put(bpmp);

Applied. Thanks.

-- 
viresh

Re: [PATCH 1/2] cpufreq: tegra186: Fix initial frequency

[Jon Hunter]

On 13/07/2020 04:25, Viresh Kumar wrote:
> On 12-07-20, 11:06, Jon Hunter wrote:
>> Commit 6cc3d0e9a097 ("cpufreq: tegra186: add
>> CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for
>> Tegra186 but as a consequence the following warnings are now seen on
>> boot ...
>>
>>  cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz
>>  cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz
>>  cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz
>>  cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz
>>  cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz
>>  cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz
>>  cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz
>>  cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz
>>  cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz
>>  cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz
>>  cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz
>>  cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz
>>
>> Although we could fix this by adding a 'get' operator for the Tegra186
>> CPUFREQ driver, there is really little point because the CPUFREQ on
>> Tegra186 is set by writing a value stored in the frequency table to a
>> register and we just need to set the initial frequency.
> 
> The hardware still runs at the frequency requested by cpufreq core here, right ?

Yes.

> It is better to provide the get() callback as it is also used to show the
> current frequency in userspace.

I looked at that and I saw that if the get() callback is not provided,
the current frequency showed by userspace is policy->cur. For this
device, policy->cur is accurate and so if we added the get() callback we
essentially just going to return policy->cur. Therefore, given that we
already know policy->cur, I did not see the point in adding a device
specific handler to do the same thing.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 1/2] cpufreq: tegra186: Fix initial frequency

[Viresh Kumar]

On 13-07-20, 17:37, Jon Hunter wrote:
> 
> On 13/07/2020 04:25, Viresh Kumar wrote:
> > On 12-07-20, 11:06, Jon Hunter wrote:
> >> Commit 6cc3d0e9a097 ("cpufreq: tegra186: add
> >> CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for
> >> Tegra186 but as a consequence the following warnings are now seen on
> >> boot ...
> >>
> >>  cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz
> >>  cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz
> >>  cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz
> >>  cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz
> >>  cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz
> >>  cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz
> >>  cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz
> >>  cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz
> >>  cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz
> >>  cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz
> >>  cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz
> >>  cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz
> >>
> >> Although we could fix this by adding a 'get' operator for the Tegra186
> >> CPUFREQ driver, there is really little point because the CPUFREQ on
> >> Tegra186 is set by writing a value stored in the frequency table to a
> >> register and we just need to set the initial frequency.
> > 
> > The hardware still runs at the frequency requested by cpufreq core here, right ?
> 
> Yes.
> 
> > It is better to provide the get() callback as it is also used to show the
> > current frequency in userspace.
> 
> I looked at that and I saw that if the get() callback is not provided,
> the current frequency showed by userspace is policy->cur. For this
> device, policy->cur is accurate and so if we added the get() callback we
> essentially just going to return policy->cur. Therefore, given that we
> already know policy->cur, I did not see the point in adding a device
> specific handler to do the same thing.

The get() callback is supposed to read the frequency from hardware and
return it, no cached value here. policy->cur may end up being wrong in
case there is a bug.

-- 
viresh

Re: [PATCH 1/2] cpufreq: tegra186: Fix initial frequency

[Jon Hunter]

On 14/07/2020 04:46, Viresh Kumar wrote:
> On 13-07-20, 17:37, Jon Hunter wrote:
>>
>> On 13/07/2020 04:25, Viresh Kumar wrote:
>>> On 12-07-20, 11:06, Jon Hunter wrote:
>>>> Commit 6cc3d0e9a097 ("cpufreq: tegra186: add
>>>> CPUFREQ_NEED_INITIAL_FREQ_CHECK flag") fixed CPUFREQ support for
>>>> Tegra186 but as a consequence the following warnings are now seen on
>>>> boot ...
>>>>
>>>>  cpufreq: cpufreq_online: CPU0: Running at unlisted freq: 0 KHz
>>>>  cpufreq: cpufreq_online: CPU0: Unlisted initial frequency changed to: 2035200 KHz
>>>>  cpufreq: cpufreq_online: CPU1: Running at unlisted freq: 0 KHz
>>>>  cpufreq: cpufreq_online: CPU1: Unlisted initial frequency changed to: 2035200 KHz
>>>>  cpufreq: cpufreq_online: CPU2: Running at unlisted freq: 0 KHz
>>>>  cpufreq: cpufreq_online: CPU2: Unlisted initial frequency changed to: 2035200 KHz
>>>>  cpufreq: cpufreq_online: CPU3: Running at unlisted freq: 0 KHz
>>>>  cpufreq: cpufreq_online: CPU3: Unlisted initial frequency changed to: 2035200 KHz
>>>>  cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 0 KHz
>>>>  cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 2035200 KHz
>>>>  cpufreq: cpufreq_online: CPU5: Running at unlisted freq: 0 KHz
>>>>  cpufreq: cpufreq_online: CPU5: Unlisted initial frequency changed to: 2035200 KHz
>>>>
>>>> Although we could fix this by adding a 'get' operator for the Tegra186
>>>> CPUFREQ driver, there is really little point because the CPUFREQ on
>>>> Tegra186 is set by writing a value stored in the frequency table to a
>>>> register and we just need to set the initial frequency.
>>>
>>> The hardware still runs at the frequency requested by cpufreq core here, right ?
>>
>> Yes.
>>
>>> It is better to provide the get() callback as it is also used to show the
>>> current frequency in userspace.
>>
>> I looked at that and I saw that if the get() callback is not provided,
>> the current frequency showed by userspace is policy->cur. For this
>> device, policy->cur is accurate and so if we added the get() callback we
>> essentially just going to return policy->cur. Therefore, given that we
>> already know policy->cur, I did not see the point in adding a device
>> specific handler to do the same thing.
> 
> The get() callback is supposed to read the frequency from hardware and
> return it, no cached value here. policy->cur may end up being wrong in
> case there is a bug.

OK, I can add a get callback. However, there are a few other drivers
that set the current frequency in the init() and don't implement a get()
callback ...

drivers/cpufreq/pasemi-cpufreq.c
drivers/cpufreq/ppc_cbe_cpufreq.c
drivers/cpufreq/intel_pstate.c

Jon

-- 
nvpublic

Re: [PATCH 1/2] cpufreq: tegra186: Fix initial frequency

[Viresh Kumar]

On 14-07-20, 08:26, Jon Hunter wrote:
> OK, I can add a get callback. However, there are a few other drivers
> that set the current frequency in the init() and don't implement a get()
> callback ...
> 
> drivers/cpufreq/pasemi-cpufreq.c
> drivers/cpufreq/ppc_cbe_cpufreq.c

These are quite old and I am not sure why they didn't do it.

> drivers/cpufreq/intel_pstate.c

With intel-pstate driver, the firmware sets the frequency of the CPUs
and it can be different from what cpufreq core has asked it to. And so
the kernel normally has no idea of what the frequency a CPU is running
at.

-- 
viresh

Re: [PATCH 1/2] cpufreq: tegra186: Fix initial frequency

[Jon Hunter]

Hi Viresh,

On 14/07/2020 04:46, Viresh Kumar wrote:

...

> The get() callback is supposed to read the frequency from hardware and
> return it, no cached value here. policy->cur may end up being wrong in
> case there is a bug.

I have been doing some more testing on Tegra, I noticed that when
reading the current CPU frequency via the sysfs scaling_cur_freq entry,
this always returns the cached value (at least for Tegra). Looking at
the implementation of scaling_cur_freq I see ...

static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
{
        ssize_t ret; 
        unsigned int freq;

        freq = arch_freq_get_on_cpu(policy->cpu);
        if (freq)
                ret = sprintf(buf, "%u\n", freq);
        else if (cpufreq_driver && cpufreq_driver->setpolicy &&
                        cpufreq_driver->get)
                ret = sprintf(buf, "%u\n", cpufreq_driver->get(policy->cpu));
        else
                ret = sprintf(buf, "%u\n", policy->cur);
        return ret; 
}

The various Tegra CPU frequency drivers do not implement the
set_policy callback and hence why we always get the cached value. I
see the following commit added this and before it simply return the
cached value ...

commit c034b02e213d271b98c45c4a7b54af8f69aaac1e
Author: Dirk Brandewie <dirk.j.brandewie@intel.com>
Date:   Mon Oct 13 08:37:40 2014 -0700

    cpufreq: expose scaling_cur_freq sysfs file for set_policy() drivers

Is this intentional? 

Cheers
Jon

-- 
nvpublic

Re: [PATCH 1/2] cpufreq: tegra186: Fix initial frequency

[Viresh Kumar]

On 31-07-20, 13:14, Jon Hunter wrote:
> I have been doing some more testing on Tegra, I noticed that when
> reading the current CPU frequency via the sysfs scaling_cur_freq entry,
> this always returns the cached value (at least for Tegra). Looking at
> the implementation of scaling_cur_freq I see ...
> 
> static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
> {
>         ssize_t ret; 
>         unsigned int freq;
> 
>         freq = arch_freq_get_on_cpu(policy->cpu);
>         if (freq)
>                 ret = sprintf(buf, "%u\n", freq);
>         else if (cpufreq_driver && cpufreq_driver->setpolicy &&
>                         cpufreq_driver->get)
>                 ret = sprintf(buf, "%u\n", cpufreq_driver->get(policy->cpu));
>         else
>                 ret = sprintf(buf, "%u\n", policy->cur);
>         return ret; 
> }
> 
> The various Tegra CPU frequency drivers do not implement the
> set_policy callback and hence why we always get the cached value. I
> see the following commit added this and before it simply return the
> cached value ...
> 
> commit c034b02e213d271b98c45c4a7b54af8f69aaac1e
> Author: Dirk Brandewie <dirk.j.brandewie@intel.com>
> Date:   Mon Oct 13 08:37:40 2014 -0700
> 
>     cpufreq: expose scaling_cur_freq sysfs file for set_policy() drivers
> 
> Is this intentional? 

Yes, it is.

There are two sysfs files to read the current frequency.

- scaling_cur_freq: as you noticed it returns cached value unless it is for
  setpolicy drivers in whose case cpufreq core doesn't control the frequency and
  so doesn't cache any values.

- cpuinfo_cur_freq: This will return the value as read from hardware using
  ->get() callback.

-- 
viresh