[PATCH 1/2] ARM: tegra: fix overflow in tegra20_pll_clk_round_rate()

[PATCH 1/2] ARM: tegra: fix overflow in tegra20_pll_clk_round_rate()

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

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 <pgaikwad@nvidia.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
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(-)

diff --git a/arch/arm/mach-tegra/tegra20_clocks.c b/arch/arm/mach-tegra/tegra20_clocks.c
index ee6922b..e2a43e4 100644
--- a/arch/arm/mach-tegra/tegra20_clocks.c
+++ b/arch/arm/mach-tegra/tegra20_clocks.c
@@ -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;
 
-- 
1.7.0.4

[PATCH 2/2] ARM: tegra: cpu-tegra: explicitly manage re-parenting

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

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 <pgaikwad@nvidia.com>
Cc: Peter De Schrijver <pdeschrijver@nvidia.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
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(-)

diff --git a/arch/arm/mach-tegra/cpu-tegra.c b/arch/arm/mach-tegra/cpu-tegra.c
index ceb52db..627bf0f 100644
--- a/arch/arm/mach-tegra/cpu-tegra.c
+++ b/arch/arm/mach-tegra/cpu-tegra.c
@@ -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);
-- 
1.7.0.4

[PATCH 2/2] ARM: tegra: cpu-tegra: explicitly manage re-parenting

[Mike Turquette]

Quoting Stephen Warren (2012-09-10 16:12:38)
> From: Stephen Warren <swarren@nvidia.com>
> 
> 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.

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

Regards,
Mike

> Cc: Prashant Gaikwad <pgaikwad@nvidia.com>
> Cc: Peter De Schrijver <pdeschrijver@nvidia.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> 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(-)
> 
> diff --git a/arch/arm/mach-tegra/cpu-tegra.c b/arch/arm/mach-tegra/cpu-tegra.c
> index ceb52db..627bf0f 100644
> --- a/arch/arm/mach-tegra/cpu-tegra.c
> +++ b/arch/arm/mach-tegra/cpu-tegra.c
> @@ -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);
> -- 
> 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

[PATCH 2/2] ARM: tegra: cpu-tegra: explicitly manage re-parenting

[Stephen Warren]

On 09/10/2012 10:45 PM, Mike Turquette wrote:
> Quoting Stephen Warren (2012-09-10 16:12:38)
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> 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:-)

[PATCH 1/2] ARM: tegra: fix overflow in tegra20_pll_clk_round_rate()

[Prashant Gaikwad]

On Tuesday 11 September 2012 04:42 AM, Stephen Warren wrote:
> From: Stephen Warren<swarren@nvidia.com>
>
> 32-bit math isn't enough when e.g. *prate=12000000, and sel->n=1000.
> Use 64-bit math to prevent this.

Thanks Stephen!!

> Cc: Prashant Gaikwad<pgaikwad@nvidia.com>
> Signed-off-by: Stephen Warren<swarren@nvidia.com>
> ---
> Prashant, can you please audit all of the Tegra clock driver to see if
> there are any other instances of the same issue? Thanks.

Sure.

> ---
>   arch/arm/mach-tegra/tegra20_clocks.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/tegra20_clocks.c b/arch/arm/mach-tegra/tegra20_clocks.c
> index ee6922b..e2a43e4 100644
> --- a/arch/arm/mach-tegra/tegra20_clocks.c
> +++ b/arch/arm/mach-tegra/tegra20_clocks.c
> @@ -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;
>

[PATCH 1/2] ARM: tegra: fix overflow in tegra20_pll_clk_round_rate()

[Stephen Warren]

On 09/10/2012 05:12 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> 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 <pgaikwad@nvidia.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

I've applied the series to Tegra's for-3.7/common-clk branch, since both
patches fix issues triggered by the common clock conversion.

[PATCH 2/2] ARM: tegra: cpu-tegra: explicitly manage re-parenting

[Mike Turquette]

Quoting Prashant Gaikwad (2012-09-11 04:39:18)
> On Tuesday 11 September 2012 11:13 AM, Stephen Warren wrote:
> 
>     On 09/10/2012 10:45 PM, Mike Turquette wrote:
> 
>         Quoting Stephen Warren (2012-09-10 16:12:38)
> 
>             From: Stephen Warren <swarren@nvidia.com>
> 
>             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.

OMAP 4460/4470 have a similar constraint to have reparenting operation
depending on the frequency that the ARM runs at.  I'll try to implement
this soon and I will Cc you guys on it.  Hopefully the same method makes
sense for you.

Regards,
Mike

[PATCH 2/2] ARM: tegra: cpu-tegra: explicitly manage re-parenting

[Turquette, Mike]

On Tue, Sep 11, 2012 at 9:10 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Mon, Sep 10, 2012 at 09:45:30PM -0700, Mike Turquette wrote:
>> > 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.
>>
> 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.
>

Hmm, the cpufreq-cpu0 case is a good point.  Well like I said in my
previous email this can be done in the framework.  I'll try to get an
example of this on the list ASAP but of course my todo list is a fifo
not a stack.

Regards,
Mike

> Regards,
> Shawn
>

[PATCH 2/2] ARM: tegra: cpu-tegra: explicitly manage re-parenting

[Shawn Guo]

On Mon, Sep 10, 2012 at 09:45:30PM -0700, Mike Turquette wrote:
> > 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.
> 
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