LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2 0/2] clk: fractional-divider: do a clean up
@ 2015-03-30 18:57 Andy Shevchenko
  2015-03-30 18:57 ` [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation Andy Shevchenko
  2015-03-30 18:57 ` [PATCH v2 2/2] clk: fractional-divider: fix sparse warnings Andy Shevchenko
  0 siblings, 2 replies; 6+ messages in thread
From: Andy Shevchenko @ 2015-03-30 18:57 UTC (permalink / raw)
  To: Stephen Boyd, linux-kernel, heikki.krogerus; +Cc: 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation
  2015-03-30 18:57 [PATCH v2 0/2] clk: fractional-divider: do a clean up Andy Shevchenko
@ 2015-03-30 18:57 ` Andy Shevchenko
  2015-03-31  0:32   ` Stephen Boyd
  2015-03-30 18:57 ` [PATCH v2 2/2] clk: fractional-divider: fix sparse warnings Andy Shevchenko
  1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2015-03-30 18:57 UTC (permalink / raw)
  To: Stephen Boyd, linux-kernel, heikki.krogerus; +Cc: 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] clk: fractional-divider: fix sparse warnings
  2015-03-30 18:57 [PATCH v2 0/2] clk: fractional-divider: do a clean up Andy Shevchenko
  2015-03-30 18:57 ` [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation Andy Shevchenko
@ 2015-03-30 18:57 ` Andy Shevchenko
  1 sibling, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2015-03-30 18:57 UTC (permalink / raw)
  To: Stephen Boyd, linux-kernel, heikki.krogerus; +Cc: 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation
  2015-03-30 18:57 ` [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation Andy Shevchenko
@ 2015-03-31  0:32   ` Stephen Boyd
  2015-03-31  7:42     ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2015-03-31  0:32 UTC (permalink / raw)
  To: Andy Shevchenko, linux-kernel, heikki.krogerus

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation
  2015-03-31  0:32   ` Stephen Boyd
@ 2015-03-31  7:42     ` Andy Shevchenko
  2015-03-31  9:33       ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2015-03-31  7:42 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: Andy Shevchenko, linux-kernel, Krogerus, Heikki

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation
  2015-03-31  7:42     ` Andy Shevchenko
@ 2015-03-31  9:33       ` Andy Shevchenko
  0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2015-03-31  9:33 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Stephen Boyd, linux-kernel, Krogerus, Heikki

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-03-31  9:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-30 18:57 [PATCH v2 0/2] clk: fractional-divider: do a clean up Andy Shevchenko
2015-03-30 18:57 ` [PATCH v2 1/2] clk: fractional-divider: switch to rational best approximation Andy Shevchenko
2015-03-31  0:32   ` Stephen Boyd
2015-03-31  7:42     ` Andy Shevchenko
2015-03-31  9:33       ` Andy Shevchenko
2015-03-30 18:57 ` [PATCH v2 2/2] clk: fractional-divider: fix sparse warnings Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).