[PATCH v2 0/2] clk: fractional-divider: do a clean up

[PATCH v2 0/2] clk: fractional-divider: do a clean up

[Andy Shevchenko]

Patches are self-explanatory I think. So, just changelog is provided here.

It would be really nice to queue them to v4.1.

Changelog v2:
- move to rational_best_approximation() and mult_frac()
- add patch 2/2

Andy Shevchenko (2):
  clk: fractional-divider: switch to rational best approximation
  clk: fractional-divider: fix sparse warnings

 drivers/clk/Kconfig                  |  3 +-
 drivers/clk/clk-fractional-divider.c | 72 ++++++++++++++++++------------------
 2 files changed, 36 insertions(+), 39 deletions(-)

-- 
2.1.4

[PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation

[Andy Shevchenko]

This patch converts the code to use rational best approximation algorithm which
is more precise.

Suggested-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/clk/Kconfig                  |  3 +--
 drivers/clk/clk-fractional-divider.c | 39 ++++++++++++++----------------------
 2 files changed, 16 insertions(+), 26 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 0b474a0..46d90a9 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -14,6 +14,7 @@ config COMMON_CLK
 	select HAVE_CLK_PREPARE
 	select CLKDEV_LOOKUP
 	select SRCU
+	select RATIONAL
 	---help---
 	  The common clock framework is a single definition of struct
 	  clk, useful across many platforms, as well as an
@@ -63,7 +64,6 @@ config COMMON_CLK_SI5351
 	tristate "Clock driver for SiLabs 5351A/B/C"
 	depends on I2C
 	select REGMAP_I2C
-	select RATIONAL
 	---help---
 	  This driver supports Silicon Labs 5351A/B/C programmable clock
 	  generators.
@@ -139,7 +139,6 @@ config COMMON_CLK_CDCE706
 	tristate "Clock driver for TI CDCE706 clock synthesizer"
 	depends on I2C
 	select REGMAP_I2C
-	select RATIONAL
 	---help---
 	  This driver supports TI CDCE706 programmable 3-PLL clock synthesizer.
 
diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 6aa72d9..9c53704 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -7,13 +7,14 @@
  *
  * Adjustable fractional divider clock implementation.
  * Output rate = (m / n) * parent_rate.
+ * Uses rational best approximation algorithm.
  */
 
 #include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/slab.h>
-#include <linux/gcd.h>
+#include <linux/rational.h>
 
 #define to_clk_fd(_hw) container_of(_hw, struct clk_fractional_divider, hw)
 
@@ -21,9 +22,9 @@ static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
 					unsigned long parent_rate)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
+	unsigned long m, n;
 	unsigned long flags = 0;
-	u32 val, m, n;
-	u64 ret;
+	u32 val;
 
 	if (fd->lock)
 		spin_lock_irqsave(fd->lock, flags);
@@ -39,30 +40,22 @@ static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
 	if (!n || !m)
 		return parent_rate;
 
-	ret = (u64)parent_rate * m;
-	do_div(ret, n);
-
-	return ret;
+	return mult_frac(parent_rate, m, n);
 }
 
 static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
-			      unsigned long *prate)
+			      unsigned long *parent_rate)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
-	unsigned maxn = (fd->nmask >> fd->nshift) + 1;
-	unsigned div;
+	unsigned long m, n;
 
-	if (!rate || rate >= *prate)
-		return *prate;
+	if (!rate || rate >= *parent_rate)
+		return *parent_rate;
 
-	div = gcd(*prate, rate);
-
-	while ((*prate / div) > maxn) {
-		div <<= 1;
-		rate <<= 1;
-	}
+	rational_best_approximation(rate, *parent_rate,
+			fd->mmask >> fd->mshift, fd->nmask >> fd->nshift, &m, &n);
 
-	return rate;
+	return mult_frac(rate, m, n);
 }
 
 static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -70,13 +63,11 @@ static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
 	unsigned long flags = 0;
-	unsigned long div;
-	unsigned n, m;
+	unsigned long n, m;
 	u32 val;
 
-	div = gcd(parent_rate, rate);
-	m = rate / div;
-	n = parent_rate / div;
+	rational_best_approximation(rate, parent_rate,
+			fd->mmask >> fd->mshift, fd->nmask >> fd->nshift, &m, &n);
 
 	if (fd->lock)
 		spin_lock_irqsave(fd->lock, flags);
-- 
2.1.4

Re: [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation

[Stephen Boyd]

On 03/30/15 11:57, Andy Shevchenko wrote:
> This patch converts the code to use rational best approximation algorithm which
> is more precise.
>
> Suggested-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/clk/Kconfig                  |  3 +--
>  drivers/clk/clk-fractional-divider.c | 39 ++++++++++++++----------------------
>  2 files changed, 16 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 0b474a0..46d90a9 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -14,6 +14,7 @@ config COMMON_CLK
>  	select HAVE_CLK_PREPARE
>  	select CLKDEV_LOOKUP
>  	select SRCU
> +	select RATIONAL
>  	---help---
>  	  The common clock framework is a single definition of struct
>  	  clk, useful across many platforms, as well as an
> @@ -63,7 +64,6 @@ config COMMON_CLK_SI5351
>  	tristate "Clock driver for SiLabs 5351A/B/C"
>  	depends on I2C
>  	select REGMAP_I2C
> -	select RATIONAL
>  	---help---
>  	  This driver supports Silicon Labs 5351A/B/C programmable clock
>  	  generators.
> @@ -139,7 +139,6 @@ config COMMON_CLK_CDCE706
>  	tristate "Clock driver for TI CDCE706 clock synthesizer"
>  	depends on I2C
>  	select REGMAP_I2C
> -	select RATIONAL
>  	---help---
>  	  This driver supports TI CDCE706 programmable 3-PLL clock synthesizer.

Will some kernel janitor find us if we leave the selects here? I know
you removed it because COMMON_CLK must be Y here and we'd always select
RATIONAL, but it feels better to leave it alone and actually split off
the basic clock types into individual configs so that the tiny kernel
users don't have unused code lying around. That could be future work
someday and then we might forget to select RATIONAL again.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation

[Andy Shevchenko]

On Tue, Mar 31, 2015 at 3:32 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 03/30/15 11:57, Andy Shevchenko wrote:
>> This patch converts the code to use rational best approximation algorithm which
>> is more precise.

>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -14,6 +14,7 @@ config COMMON_CLK
>>       select HAVE_CLK_PREPARE
>>       select CLKDEV_LOOKUP
>>       select SRCU
>> +     select RATIONAL
>>       ---help---
>>         The common clock framework is a single definition of struct
>>         clk, useful across many platforms, as well as an
>> @@ -63,7 +64,6 @@ config COMMON_CLK_SI5351
>>       tristate "Clock driver for SiLabs 5351A/B/C"
>>       depends on I2C
>>       select REGMAP_I2C
>> -     select RATIONAL
>>       ---help---
>>         This driver supports Silicon Labs 5351A/B/C programmable clock
>>         generators.
>> @@ -139,7 +139,6 @@ config COMMON_CLK_CDCE706
>>       tristate "Clock driver for TI CDCE706 clock synthesizer"
>>       depends on I2C
>>       select REGMAP_I2C
>> -     select RATIONAL
>>       ---help---
>>         This driver supports TI CDCE706 programmable 3-PLL clock synthesizer.
>
> Will some kernel janitor find us if we leave the selects here? I know
> you removed it because COMMON_CLK must be Y here and we'd always select
> RATIONAL, but it feels better to leave it alone and actually split off
> the basic clock types into individual configs so that the tiny kernel
> users don't have unused code lying around. That could be future work
> someday and then we might forget to select RATIONAL again.

I'm fine with that as well. So, do I need to resend or you apply
modified version?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation

[Andy Shevchenko]

On Tue, 2015-03-31 at 10:42 +0300, Andy Shevchenko wrote:
> On Tue, Mar 31, 2015 at 3:32 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 03/30/15 11:57, Andy Shevchenko wrote:
> >> This patch converts the code to use rational best approximation algorithm which
> >> is more precise.
> 
> >> --- a/drivers/clk/Kconfig
> >> +++ b/drivers/clk/Kconfig
> >> @@ -14,6 +14,7 @@ config COMMON_CLK
> >>       select HAVE_CLK_PREPARE
> >>       select CLKDEV_LOOKUP
> >>       select SRCU
> >> +     select RATIONAL
> >>       ---help---
> >>         The common clock framework is a single definition of struct
> >>         clk, useful across many platforms, as well as an
> >> @@ -63,7 +64,6 @@ config COMMON_CLK_SI5351
> >>       tristate "Clock driver for SiLabs 5351A/B/C"
> >>       depends on I2C
> >>       select REGMAP_I2C
> >> -     select RATIONAL
> >>       ---help---
> >>         This driver supports Silicon Labs 5351A/B/C programmable clock
> >>         generators.
> >> @@ -139,7 +139,6 @@ config COMMON_CLK_CDCE706
> >>       tristate "Clock driver for TI CDCE706 clock synthesizer"
> >>       depends on I2C
> >>       select REGMAP_I2C
> >> -     select RATIONAL
> >>       ---help---
> >>         This driver supports TI CDCE706 programmable 3-PLL clock synthesizer.
> >
> > Will some kernel janitor find us if we leave the selects here? I know
> > you removed it because COMMON_CLK must be Y here and we'd always select
> > RATIONAL, but it feels better to leave it alone and actually split off
> > the basic clock types into individual configs so that the tiny kernel
> > users don't have unused code lying around. That could be future work
> > someday and then we might forget to select RATIONAL again.
> 
> I'm fine with that as well. So, do I need to resend or you apply
> modified version?

There are couple of issues with arithmetics, such as we can't use
mult_frac() because of potential overflow on 32 bit kernels, so I will
resend new version.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

[PATCH v2 2/2] clk: fractional-divider: fix sparse warnings

[Andy Shevchenko]

Sparse complains about possible imbalance in locking.

drivers/clk/clk-fractional-divider.c:37:9: warning: context imbalance in 'clk_fd_recalc_rate' - different lock contexts for basic block
drivers/clk/clk-fractional-divider.c:61:12: warning: context imbalance in 'clk_fd_set_rate' - different lock contexts for basic block

Let's rewrite code to fix this and make it more straight.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/clk/clk-fractional-divider.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 9c53704..3766da8 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -26,13 +26,13 @@ static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
 	unsigned long flags = 0;
 	u32 val;
 
-	if (fd->lock)
+	if (fd->lock) {
 		spin_lock_irqsave(fd->lock, flags);
-
-	val = clk_readl(fd->reg);
-
-	if (fd->lock)
+		val = clk_readl(fd->reg);
 		spin_unlock_irqrestore(fd->lock, flags);
+	} else {
+		val = clk_readl(fd->reg);
+	}
 
 	m = (val & fd->mmask) >> fd->mshift;
 	n = (val & fd->nmask) >> fd->nshift;
@@ -58,27 +58,34 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
 	return mult_frac(rate, m, n);
 }
 
+static void clk_fd_update(struct clk_fractional_divider *fd,
+			  unsigned long m, unsigned long n)
+{
+	u32 val;
+
+	val = clk_readl(fd->reg);
+	val &= ~(fd->mmask | fd->nmask);
+	val |= (m << fd->mshift) | (n << fd->nshift);
+	clk_writel(val, fd->reg);
+}
+
 static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
 			   unsigned long parent_rate)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
 	unsigned long flags = 0;
 	unsigned long n, m;
-	u32 val;
 
 	rational_best_approximation(rate, parent_rate,
 			fd->mmask >> fd->mshift, fd->nmask >> fd->nshift, &m, &n);
 
-	if (fd->lock)
+	if (fd->lock) {
 		spin_lock_irqsave(fd->lock, flags);
-
-	val = clk_readl(fd->reg);
-	val &= ~(fd->mmask | fd->nmask);
-	val |= (m << fd->mshift) | (n << fd->nshift);
-	clk_writel(val, fd->reg);
-
-	if (fd->lock)
+		clk_fd_update(fd, m, n);
 		spin_unlock_irqrestore(fd->lock, flags);
+	} else {
+		clk_fd_update(fd, m, n);
+	}
 
 	return 0;
 }
-- 
2.1.4