ARM: tegra: fix overflow in tegra20_pll_clk_round_rate()

From: Stephen Warren <[email protected]>

32-bit math isn't enough when e.g. *prate=12000000, and sel->n=1000. Use 64-bit math to prevent this.

Cc Prashant Gaikwad <[email protected]>

Signed-off-by Stephen Warren <[email protected]>

From: Stephen Warren<[email protected]>

32-bit math isn't enough when e.g. *prate=12000000, and sel->n=1000. Use 64-bit math to prevent this.

Cc Prashant Gaikwad<[email protected]>

Signed-off-by Stephen Warren<[email protected]>

---
Prashant, can you please audit all of the Tegra clock driver to see if there are any other instances of the same issue? Thanks.

---
  arch/arm/mach-tegra/tegra20_clocks.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mach-tegra/tegra20_clocks.c +1 -1

index ee6922b..e2a43e4 100644
@@ -798,7 +798,7 @@ static long tegra20_pll_clk_round_rate(struct clk_hw *hw, unsigned long rate,
  	struct clk_tegra *c = to_clk_tegra(hw);
  	const struct clk_pll_freq_table *sel;
  	unsigned long input_rate = *prate;
-	unsigned long output_rate = *prate;
+	u64 output_rate = *prate;
  	int mul;
  	int div;
 

From: Stephen Warren <[email protected]>

When changing a PLL's rate, it must have no active children. The CPU clock cannot be stopped, and CPU clock's divider is not used. The old clock driver used to handle this by internally reparenting the CPU clock onto a different PLL when changing the CPU clock rate. However, the new common-clock based clock driver does not do this, and probably cannot do this due to the locking issues it would cause.

To solve this, have the Tegra cpufreq driver explicitly perform the reparenting operations itself. This is probably reasonable anyway, since such reparenting is somewhat a matter of policy (e.g. which alternate clock source to use, whether to leave the CPU clock a child of the alternate clock source if it's running at the desired rate), and hence is something more appropriate for the cpufreq driver than the core clock driver anyway.

Cc Prashant Gaikwad <[email protected]>

Cc Peter De Schrijver <[email protected]>

Signed-off-by Stephen Warren <[email protected]>

---
These two fixes are required to resolve the regression exposed by commit ec971ea "ARM: add cpufreq transiton notifier to adjust loops_per_jiffy for smp"

---
 arch/arm/mach-tegra/cpu-tegra.c |   48 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 47 insertions(+), 1 deletions(-)

arch/arm/mach-tegra/cpu-tegra.c +47 -1

index ceb52db..627bf0f 100644
@@ -49,6 +49,8 @@ static struct cpufreq_frequency_table freq_table[] = {
 #define NUM_CPUS       2
 
 static struct clk *cpu_clk;
+static struct clk *pll_x_clk;
+static struct clk *pll_p_clk;
 static struct clk *emc_clk;
 
 static unsigned long target_cpu_speed[NUM_CPUS];

@@ -71,6 +73,42 @@ static unsigned int tegra_getspeed(unsigned int cpu)
        return rate;
 }
 
+static int tegra_cpu_clk_set_rate(unsigned long rate)
+{
+       int ret;
+
+       /*
+        * Take an extra reference to the main pll so it doesn't turn
+        * off when we move the cpu off of it
+        */
+       clk_prepare_enable(pll_x_clk);
+
+       ret = clk_set_parent(cpu_clk, pll_p_clk);
+       if (ret) {
+               pr_err("Failed to switch cpu to clock pll_p\n");
+               goto out;
+       }
+
+       if (rate == clk_get_rate(pll_p_clk))
+               goto out;
+
+       ret = clk_set_rate(pll_x_clk, rate);
+       if (ret) {
+               pr_err("Failed to change pll_x to %lu\n", rate);
+               goto out;
+       }
+
+       ret = clk_set_parent(cpu_clk, pll_x_clk);
+       if (ret) {
+               pr_err("Failed to switch cpu to clock pll_x\n");
+               goto out;
+       }
+
+out:
+       clk_disable_unprepare(pll_x_clk);
+       return ret;
+}
+
 static int tegra_update_cpu_speed(unsigned long rate)
 {
        int ret = 0;

@@ -101,7 +139,7 @@ static int tegra_update_cpu_speed(unsigned long rate)
               freqs.old, freqs.new);
 #endif
 
-       ret = clk_set_rate(cpu_clk, freqs.new * 1000);
+       ret = tegra_cpu_clk_set_rate(freqs.new * 1000);
        if (ret) {
                pr_err("cpu-tegra: Failed to set cpu frequency to %d kHz\n",
                        freqs.new);

@@ -183,6 +221,14 @@ static int tegra_cpu_init(struct cpufreq_policy *policy)
        if (IS_ERR(cpu_clk))
                return PTR_ERR(cpu_clk);
 
+       pll_x_clk = clk_get_sys(NULL, "pll_x");
+       if (IS_ERR(pll_x_clk))
+               return PTR_ERR(pll_x_clk);
+
+       pll_p_clk = clk_get_sys(NULL, "pll_p");
+       if (IS_ERR(pll_p_clk))
+               return PTR_ERR(pll_p_clk);
+
        emc_clk = clk_get_sys("cpu", "emc");
        if (IS_ERR(emc_clk)) {
                clk_put(cpu_clk);

Signature · 7 lines

1.7.0.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoting Stephen Warren (2012-09-10 16:12:38)

Quoted 9 lines

From: Stephen Warren <[email protected]>

When changing a PLL's rate, it must have no active children. The CPU clock cannot be stopped, and CPU clock's divider is not used. The old clock driver used to handle this by internally reparenting the CPU clock onto a different PLL when changing the CPU clock rate. However, the new common-clock based clock driver does not do this, and probably cannot do this due to the locking issues it would cause.

This is possible today. Clock drivers can call __clk_reparent to update the common clk bookkeeping to reflect changes in parent muxing. There are some examples of this out in the wild, and the unmerged OMAP port certainly uses this during the PLL relock sequence.

Quoted 7 lines

To solve this, have the Tegra cpufreq driver explicitly perform the reparenting operations itself. This is probably reasonable anyway, since such reparenting is somewhat a matter of policy (e.g. which alternate clock source to use, whether to leave the CPU clock a child of the alternate clock source if it's running at the desired rate), and hence is something more appropriate for the cpufreq driver than the core clock driver anyway.

I definitely agree about the policy. Just FYI I'm hacking on an RFC to make reparenting clocks from a call to clk_set_rate even easier, but perhaps in your case it is better the cpufreq driver knows the clock tree topology details.

On 09/10/2012 10:45 PM, Mike Turquette wrote:

Quoting Stephen Warren (2012-09-10 16:12:38)

From: Stephen Warren <[email protected]>

When changing a PLL's rate, it must have no active children. The CPU clock cannot be stopped, and CPU clock's divider is not used. The old clock driver used to handle this by internally reparenting the CPU clock onto a different PLL when changing the CPU clock rate. However, the new common-clock based clock driver does not do this, and probably cannot do this due to the locking issues it would cause.

This is possible today. Clock drivers can call __clk_reparent to update the common clk bookkeeping to reflect changes in parent muxing. There are some examples of this out in the wild, and the unmerged OMAP port certainly uses this during the PLL relock sequence.

The CPU clock's set_rate needs to both __clk_reparent() /and/ set the rate of the parent PLL. I think a (non-static) __clk_set_rate() is missing? (although perhaps that could be easily solved if desired).

To solve this, have the Tegra cpufreq driver explicitly perform the reparenting operations itself. This is probably reasonable anyway, since such reparenting is somewhat a matter of policy (e.g. which alternate clock source to use, whether to leave the CPU clock a child of the alternate clock source if it's running at the desired rate), and hence is something more appropriate for the cpufreq driver than the core clock driver anyway.

I definitely agree about the policy. Just FYI I'm hacking on an RFC to make reparenting clocks from a call to clk_set_rate even easier, but perhaps in your case it is better the cpufreq driver knows the clock tree topology details.

OK, sounds fine to me:-)

Policies can change for Tegra20 and Tegra30. IMO we should implement it in clock driver. But it can not be done unless there is a way to call clk_set_rate from clk ops.

Quoted 8 lines

To solve this, have the Tegra cpufreq driver explicitly perform the reparenting operations itself. This is probably reasonable anyway, since such reparenting is somewhat a matter of policy (e.g. which alternate clock source to use, whether to leave the CPU clock a child of the alternate clock source if it's running at the desired rate), and hence is something more appropriate for the cpufreq driver than the core clock driver anyway.

I definitely agree about the policy. Just FYI I'm hacking on an RFC to make reparenting clocks from a call to clk_set_rate even easier, but perhaps in your case it is better the cpufreq driver knows the clock tree topology details.

From what I see, if tegra clock driver can handle the dependency between

Quoted 9 lines · Mike Turquette

Quoted 8 lines

To solve this, have the Tegra cpufreq driver explicitly perform the reparenting operations itself. This is probably reasonable anyway, since such reparenting is somewhat a matter of policy (e.g. which alternate clock source to use, whether to leave the CPU clock a child of the alternate clock source if it's running at the desired rate), and hence is something more appropriate for the cpufreq driver than the core clock driver anyway.

Quoted 5 lines

I definitely agree about the policy. Just FYI I'm hacking on an RFC to make reparenting clocks from a call to clk_set_rate even easier, but perhaps in your case it is better the cpufreq driver knows the clock tree topology details.

I disagree. The whole point of clock framework is to hide clock details like tree topology behind clock API, so that we do not need every single clock client driver to handle those details on their own. It's definitely desired to have clk_set_rate call handle reparenting nicely.

From what I see, if tegra clock driver can handle the dependency between cpu_clk and emc_clk that is currently handled in cpufreq driver. It's likely that the generic cpufreq driver (cpufreq-cpu0) would work for tegra. But with this patch, the driver moves to the opposite direction.

Regards,
Shawn