cpufreq: tegra186: Fix initial frequency

PATCH Applied Jon Hunter 2 participants 10 messages Jul 12, 2020 – Aug 4, 2020

Subsystems (2)

Tegra architecture support CPU frequency scaling framework

lore

Share & tools

lore

Jon Hunter

author maintainer

2020-07-12 10:06 UTC #0 lore raw

PATCH 1/2 cpufreq: tegra186: Fix initial frequency

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 <[email protected]>

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

drivers/cpufreq/tegra186-cpufreq.c +8 -0

index 3d2f143748ef..c44190ce3f03 100644
@@ -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;
 	}

Signature · 1 line

2.17.1

Jon Hunter

author maintainer

2020-07-12 10:06 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/cpufreq/tegra186-cpufreq.c | 6 ------
 1 file changed, 6 deletions(-)

drivers/cpufreq/tegra186-cpufreq.c +0 -6

index c44190ce3f03..bf8cab357277 100644
@@ -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);

Signature · 1 line

2.17.1

Viresh Kumar

maintainer Applied

2020-07-13 03:28 UTC #2 lore raw

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

Quoted 21 lines · 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 <[email protected]>
---
 drivers/cpufreq/tegra186-cpufreq.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/cpufreq/tegra186-cpufreq.c +0 -2
index c44190ce3f03..bf8cab357277 100644
@@ -231,15 +231,9 @@ static int tegra186_cpufreq_probe(struct platform_device *pdev)
 		}
 	}
 
-	tegra_bpmp_put(bpmp);
-

drivers/cpufreq/tegra186-cpufreq.c static int tegra186_cpufreq_probe(struct platform_device *pdev) · line 239

Quoted 10 lines

drivers/cpufreq/tegra186-cpufreq.c +0 -4

 	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.

Signature · 1 line

viresh

Viresh Kumar

maintainer

2020-07-13 03:25 UTC #3 lore raw

In reply to Jon Hunter

Quoted 18 lines · 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

Quoted 4 lines

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.

Quoted 38 lines

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 <[email protected]>

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

drivers/cpufreq/tegra186-cpufreq.c +8 -0

index 3d2f143748ef..c44190ce3f03 100644
@@ -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;
 	}

Signature · 1 line

2.17.1

Signature · 1 line

viresh

Jon Hunter

author maintainer

2020-07-13 16:37 UTC #4 lore raw

Quoted 19 lines · Viresh Kumar

Quoted 18 lines · 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

Quoted 6 lines

Quoted 4 lines
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.

Quoted 2 lines

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

Signature · 1 line

nvpublic

Viresh Kumar

maintainer

2020-07-14 03:46 UTC #5 lore raw

Quoted 33 lines · Jon Hunter

Quoted 19 lines · Viresh Kumar
Quoted 18 lines · 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
Quoted 6 lines
Quoted 4 lines
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.
Quoted 2 lines
It is better to provide the get() callback as it is also used to show the current frequency in userspace.

Quoted 6 lines

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.

Signature · 1 line

viresh

Jon Hunter

author maintainer

2020-07-14 07:26 UTC #6 lore raw

Quoted 41 lines · Viresh Kumar

Quoted 33 lines · Jon Hunter
Quoted 19 lines · Viresh Kumar
Quoted 18 lines · 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
Quoted 6 lines
Quoted 4 lines
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.
Quoted 2 lines
It is better to provide the get() callback as it is also used to show the current frequency in userspace.
Quoted 6 lines
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.

Quoted 3 lines

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

Signature · 1 line

nvpublic

Viresh Kumar

maintainer

2020-07-14 07:31 UTC #7 lore raw

Quoted 6 lines · Jon Hunter

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.

Quoted 1 line

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.

Signature · 1 line

viresh

Jon Hunter

author maintainer

2020-07-31 12:14 UTC #8 lore raw

In reply to Viresh Kumar

Hi Viresh,

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

...

Quoted 3 lines

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 <[email protected]>
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

Signature · 1 line

nvpublic

Viresh Kumar

maintainer

2020-08-04 05:25 UTC #9 lore raw

Quoted 26 lines · Jon Hunter

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 ...

Quoted 7 lines

commit c034b02e213d271b98c45c4a7b54af8f69aaac1e
Author: Dirk Brandewie <[email protected]>
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.

Signature · 1 line

viresh