[PATCH V2] cpufreq: tegra186: Fix initial frequency

[PATCH V2] 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

Fix this by adding a 'get' callback for the Tegra186 CPUFREQ driver to
retrieve the current operating frequency for a given CPU. The 'get'
callback uses the current 'ndiv' value that is programmed to determine
that current operating frequency.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
Changes since V1:
- Moved code into a 'get' callback

 drivers/cpufreq/tegra186-cpufreq.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
index 01e1f58ba422..0d0fcff60765 100644
--- a/drivers/cpufreq/tegra186-cpufreq.c
+++ b/drivers/cpufreq/tegra186-cpufreq.c
@@ -14,6 +14,7 @@
 
 #define EDVD_CORE_VOLT_FREQ(core)		(0x20 + (core) * 0x4)
 #define EDVD_CORE_VOLT_FREQ_F_SHIFT		0
+#define EDVD_CORE_VOLT_FREQ_F_MASK		0xffff
 #define EDVD_CORE_VOLT_FREQ_V_SHIFT		16
 
 struct tegra186_cpufreq_cluster_info {
@@ -91,10 +92,39 @@ static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
 	return 0;
 }
 
+static unsigned int tegra186_cpufreq_get(unsigned int cpu)
+{
+	struct cpufreq_frequency_table *tbl;
+	struct cpufreq_policy *policy;
+	void __iomem *edvd_reg;
+	unsigned int i, freq = 0;
+	u32 ndiv;
+
+	policy = cpufreq_cpu_get(cpu);
+	if (!policy)
+		return -EINVAL;
+
+	tbl = policy->freq_table;
+	edvd_reg = policy->driver_data;
+	ndiv = readl(edvd_reg) & EDVD_CORE_VOLT_FREQ_F_MASK;
+
+	for (i = 0; tbl[i].frequency != CPUFREQ_TABLE_END; i++) {
+		if ((tbl[i].driver_data & EDVD_CORE_VOLT_FREQ_F_MASK) == ndiv) {
+			freq = tbl[i].frequency;
+			break;
+		}
+	}
+
+	cpufreq_cpu_put(policy);
+
+	return freq;
+}
+
 static struct cpufreq_driver tegra186_cpufreq_driver = {
 	.name = "tegra186",
 	.flags = CPUFREQ_STICKY | CPUFREQ_HAVE_GOVERNOR_PER_POLICY |
 			CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+	.get = tegra186_cpufreq_get,
 	.verify = cpufreq_generic_frequency_table_verify,
 	.target_index = tegra186_cpufreq_set_target,
 	.init = tegra186_cpufreq_init,
-- 
2.25.1

Re: [PATCH V2] cpufreq: tegra186: Fix initial frequency

[Viresh Kumar]

On 24-08-20, 15:59, 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
> 
> Fix this by adding a 'get' callback for the Tegra186 CPUFREQ driver to
> retrieve the current operating frequency for a given CPU. The 'get'
> callback uses the current 'ndiv' value that is programmed to determine
> that current operating frequency.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> Changes since V1:
> - Moved code into a 'get' callback
> 
>  drivers/cpufreq/tegra186-cpufreq.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
> index 01e1f58ba422..0d0fcff60765 100644
> --- a/drivers/cpufreq/tegra186-cpufreq.c
> +++ b/drivers/cpufreq/tegra186-cpufreq.c
> @@ -14,6 +14,7 @@
>  
>  #define EDVD_CORE_VOLT_FREQ(core)		(0x20 + (core) * 0x4)
>  #define EDVD_CORE_VOLT_FREQ_F_SHIFT		0
> +#define EDVD_CORE_VOLT_FREQ_F_MASK		0xffff
>  #define EDVD_CORE_VOLT_FREQ_V_SHIFT		16
>  
>  struct tegra186_cpufreq_cluster_info {
> @@ -91,10 +92,39 @@ static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
>  	return 0;
>  }
>  
> +static unsigned int tegra186_cpufreq_get(unsigned int cpu)
> +{
> +	struct cpufreq_frequency_table *tbl;
> +	struct cpufreq_policy *policy;
> +	void __iomem *edvd_reg;
> +	unsigned int i, freq = 0;
> +	u32 ndiv;
> +
> +	policy = cpufreq_cpu_get(cpu);
> +	if (!policy)
> +		return -EINVAL;

This should be return 0;

Applied with that change. Thanks.

-- 
viresh

Re: [PATCH V2] cpufreq: tegra186: Fix initial frequency

[Jon Hunter]

On 25/08/2020 06:50, Viresh Kumar wrote:
> On 24-08-20, 15:59, 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
>>
>> Fix this by adding a 'get' callback for the Tegra186 CPUFREQ driver to
>> retrieve the current operating frequency for a given CPU. The 'get'
>> callback uses the current 'ndiv' value that is programmed to determine
>> that current operating frequency.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>> Changes since V1:
>> - Moved code into a 'get' callback
>>
>>  drivers/cpufreq/tegra186-cpufreq.c | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
>> index 01e1f58ba422..0d0fcff60765 100644
>> --- a/drivers/cpufreq/tegra186-cpufreq.c
>> +++ b/drivers/cpufreq/tegra186-cpufreq.c
>> @@ -14,6 +14,7 @@
>>  
>>  #define EDVD_CORE_VOLT_FREQ(core)		(0x20 + (core) * 0x4)
>>  #define EDVD_CORE_VOLT_FREQ_F_SHIFT		0
>> +#define EDVD_CORE_VOLT_FREQ_F_MASK		0xffff
>>  #define EDVD_CORE_VOLT_FREQ_V_SHIFT		16
>>  
>>  struct tegra186_cpufreq_cluster_info {
>> @@ -91,10 +92,39 @@ static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
>>  	return 0;
>>  }
>>  
>> +static unsigned int tegra186_cpufreq_get(unsigned int cpu)
>> +{
>> +	struct cpufreq_frequency_table *tbl;
>> +	struct cpufreq_policy *policy;
>> +	void __iomem *edvd_reg;
>> +	unsigned int i, freq = 0;
>> +	u32 ndiv;
>> +
>> +	policy = cpufreq_cpu_get(cpu);
>> +	if (!policy)
>> +		return -EINVAL;
> 
> This should be return 0;
> 
> Applied with that change. Thanks.


OK, thanks!

Jon

-- 
nvpublic

Re: [PATCH V2] cpufreq: tegra186: Fix initial frequency

[Jon Hunter]

Hi Viresh,

On 25/08/2020 06:50, Viresh Kumar wrote:
> On 24-08-20, 15:59, 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
>>
>> Fix this by adding a 'get' callback for the Tegra186 CPUFREQ driver to
>> retrieve the current operating frequency for a given CPU. The 'get'
>> callback uses the current 'ndiv' value that is programmed to determine
>> that current operating frequency.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>> Changes since V1:
>> - Moved code into a 'get' callback
>>
>>  drivers/cpufreq/tegra186-cpufreq.c | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
>> index 01e1f58ba422..0d0fcff60765 100644
>> --- a/drivers/cpufreq/tegra186-cpufreq.c
>> +++ b/drivers/cpufreq/tegra186-cpufreq.c
>> @@ -14,6 +14,7 @@
>>  
>>  #define EDVD_CORE_VOLT_FREQ(core)		(0x20 + (core) * 0x4)
>>  #define EDVD_CORE_VOLT_FREQ_F_SHIFT		0
>> +#define EDVD_CORE_VOLT_FREQ_F_MASK		0xffff
>>  #define EDVD_CORE_VOLT_FREQ_V_SHIFT		16
>>  
>>  struct tegra186_cpufreq_cluster_info {
>> @@ -91,10 +92,39 @@ static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
>>  	return 0;
>>  }
>>  
>> +static unsigned int tegra186_cpufreq_get(unsigned int cpu)
>> +{
>> +	struct cpufreq_frequency_table *tbl;
>> +	struct cpufreq_policy *policy;
>> +	void __iomem *edvd_reg;
>> +	unsigned int i, freq = 0;
>> +	u32 ndiv;
>> +
>> +	policy = cpufreq_cpu_get(cpu);
>> +	if (!policy)
>> +		return -EINVAL;
> 
> This should be return 0;
> 
> Applied with that change. Thanks.


If not too late, would you mind dropping this patch for v5.10?

The patch was working for me locally, but when testing on more boards, I
actually found this broke CPUFREQ support on some Tegra186 boards. The
reason is that the boot frequency may not be in the frequency table on
all boards. The boot frequency is constant across all boards, but the
frequency table can vary slightly with device. Therefore, for some
boards, such as mine, the boot frequency is found the in frequency table
but this is not always the case.

Now that you have made the warning an info print, this change is not
really needed and the simplest thing to do is dropped this completely.
Sorry about that.

Jon

-- 
nvpublic

Re: [PATCH V2] cpufreq: tegra186: Fix initial frequency

[Viresh Kumar]

On 15-10-20, 15:03, Jon Hunter wrote:
> If not too late, would you mind dropping this patch for v5.10?

It is already part of Linus's master now.

-- 
viresh

Re: [PATCH V2] cpufreq: tegra186: Fix initial frequency

[Jon Hunter]

On 16/10/2020 05:07, Viresh Kumar wrote:
> On 15-10-20, 15:03, Jon Hunter wrote:
>> If not too late, would you mind dropping this patch for v5.10?
> 
> It is already part of Linus's master now.

OK, thanks. I will send a revert for this once rc1 is out.

Cheers
Jon

-- 
nvpublic

Re: [PATCH V2] cpufreq: tegra186: Fix initial frequency

[Jon Hunter]

On 19/10/2020 10:33, Jon Hunter wrote:
> 
> On 16/10/2020 05:07, Viresh Kumar wrote:
>> On 15-10-20, 15:03, Jon Hunter wrote:
>>> If not too late, would you mind dropping this patch for v5.10?
>>
>> It is already part of Linus's master now.
> 
> OK, thanks. I will send a revert for this once rc1 is out.


Thinking about this some more, what are your thoughts on making the
following change? 

Basically, if the driver sets the CPUFREQ_NEED_INITIAL_FREQ_CHECK,
then I wonder if we should not fail if the frequency return by
>get() is not known. This would fix the problem I see on Tegra186
where the initial boot frequency may not be in the frequency table.

Cheers
Jon

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f4b60663efe6..b7d3b61577b0 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1426,13 +1426,8 @@ static int cpufreq_online(unsigned int cpu)
                                CPUFREQ_CREATE_POLICY, policy);
        }
 
-       if (cpufreq_driver->get && has_target()) {
+       if (cpufreq_driver->get && has_target())
                policy->cur = cpufreq_driver->get(policy->cpu);
-               if (!policy->cur) {
-                       pr_err("%s: ->get() failed\n", __func__);
-                       goto out_destroy_policy;
-               }
-       }
 
        /*
         * Sometimes boot loaders set CPU frequency to a value outside of
@@ -1471,6 +1466,11 @@ static int cpufreq_online(unsigned int cpu)
                        pr_info("%s: CPU%d: Running at unlisted initial frequency: %u KHz, changing to: %u KHz\n",
                                __func__, policy->cpu, old_freq, policy->cur);
                }
+       } else {
+               if (!policy->cur) {
+                       pr_err("%s: ->get() failed\n", __func__);
+                       goto out_destroy_policy;
+               }
        }
 
        if (new_policy) {

-- 
nvpublic

Re: [PATCH V2] cpufreq: tegra186: Fix initial frequency

[Viresh Kumar]

On 26-10-20, 12:57, Jon Hunter wrote:
> Thinking about this some more, what are your thoughts on making the
> following change? 
> 
> Basically, if the driver sets the CPUFREQ_NEED_INITIAL_FREQ_CHECK,

This flag only means that the platform would like the core to check
the currently programmed frequency and get it in sync with the table.

> then I wonder if we should not fail if the frequency return by
> >get() is not known.

When do we fail if the frequency isn't known ? That's the case where
we try to set it to one from the table.

But (looking at your change), ->get() can't really return 0. We depend
on it to get us the exact frequency the hardware is programmed at
instead of reading a cached value in the software.

> >This would fix the problem I see on Tegra186
> where the initial boot frequency may not be in the frequency table.

With current mainline, what's the problem you see now ? Sorry I missed
track of it a bit :)

-- 
viresh

Re: [PATCH V2] cpufreq: tegra186: Fix initial frequency

[Jon Hunter]

On 28/10/2020 04:11, Viresh Kumar wrote:
> On 26-10-20, 12:57, Jon Hunter wrote:
>> Thinking about this some more, what are your thoughts on making the
>> following change? 
>>
>> Basically, if the driver sets the CPUFREQ_NEED_INITIAL_FREQ_CHECK,
> 
> This flag only means that the platform would like the core to check
> the currently programmed frequency and get it in sync with the table.

Yes exactly.

>> then I wonder if we should not fail if the frequency return by
>>> get() is not known.
> 
> When do we fail if the frequency isn't known ? That's the case where
> we try to set it to one from the table.

Currently, if the frequency is not known, we fail right before we do the
initial frequency check [0].

> But (looking at your change), ->get() can't really return 0. We depend
> on it to get us the exact frequency the hardware is programmed at
> instead of reading a cached value in the software.

Actually it can and it does currently. Note in tegra186_cpufreq_get()
the variable 'freq' is initialised to 0, and if no match is found, then
it returns 0. This is what happens currently on some Tegra186 boards.

>>> This would fix the problem I see on Tegra186
>> where the initial boot frequency may not be in the frequency table.
> 
> With current mainline, what's the problem you see now ? Sorry I missed
> track of it a bit :)

No problem, this has been an on-going saga now for sometime.

Cheers
Jon

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpufreq/cpufreq.c#n1429
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpufreq/tegra186-cpufreq.c#n95

-- 
nvpublic

Re: [PATCH V2] cpufreq: tegra186: Fix initial frequency

[Viresh Kumar]

On 28-10-20, 12:31, Jon Hunter wrote:
> On 28/10/2020 04:11, Viresh Kumar wrote:
> > When do we fail if the frequency isn't known ? That's the case where
> > we try to set it to one from the table.
> 
> Currently, if the frequency is not known, we fail right before we do the
> initial frequency check [0].

Right, so the frequency returned there is 0. By unknown I assumed that
you are talking about the case where the frequency isn't found in the
table.

> > But (looking at your change), ->get() can't really return 0. We depend
> > on it to get us the exact frequency the hardware is programmed at
> > instead of reading a cached value in the software.
> 
> Actually it can and it does currently. Note in tegra186_cpufreq_get()
> the variable 'freq' is initialised to 0, and if no match is found, then
> it returns 0. This is what happens currently on some Tegra186 boards.

Then there is a problem with the implementation of this helper in your
case. This shouldn't try to go compare the value read from the
register with the table, rather convert that directly to a meaningful
frequency. Its like reading registers and then doing some computations
with the values read with the parent PLLs frequency. Something like
what you will normally do in the implementation of clk_get_rate().

-- 
viresh