[PATCH v3 0/3] clk: fractional-divider: do a clean up

[PATCH v3 0/3] 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.

Patches 2/3 and 3/3 are dependent to the patch 1/7 from [1] "clk: replace
div_mask() by clk_div_mask()".

The series was tested with 8250_dw UART driver on Intel Braswell.

[1] http://www.spinics.net/lists/kernel/msg1958958.html

Changelog v3:
- add patch 2/3 to simplify further usage
- don't use mult_frac() due to potential overflow on 32 bit kernels
- guarantee in ->round_rate() that m and n will not overflow

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

Andy Shevchenko (3):
  clk: fractional-divider: fix sparse warnings
  clk: fractional-divider: keep mwidth and nwidth internally
  clk: fractional-divider: switch to rational best approximation

 drivers/clk/Kconfig                  |  1 +
 drivers/clk/clk-fractional-divider.c | 86 +++++++++++++++++++++---------------
 include/linux/clk-provider.h         |  3 +-
 3 files changed, 54 insertions(+), 36 deletions(-)

-- 
2.1.4

[PATCH v3 1/3] 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 | 44 +++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 6aa72d9..786aa482 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -21,17 +21,18 @@ 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 flags = 0;
-	u32 val, m, n;
+	unsigned long flags;
+	unsigned long m, n;
+	u32 val;
 	u64 ret;
 
-	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;
@@ -65,29 +66,36 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
 	return rate;
 }
 
+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 flags;
 	unsigned long div;
-	unsigned n, m;
-	u32 val;
+	unsigned long m, n;
 
 	div = gcd(parent_rate, rate);
 	m = rate / div;
 	n = parent_rate / div;
 
-	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

[PATCH v3 2/3] clk: fractional-divider: keep mwidth and nwidth internally

[Andy Shevchenko]

The patch adds mwidth and nwidth fields to the struct clk_fractional_divider
for further usage. While here, use clk_div_mask() instead of open coding this
functionality.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/clk/clk-fractional-divider.c | 8 +++++---
 include/linux/clk-provider.h         | 3 ++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 786aa482..23a56a0 100644
--- a/drivers/clk/clk-fractional-divider.c
+++ b/drivers/clk/clk-fractional-divider.c
@@ -50,7 +50,7 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
 			      unsigned long *prate)
 {
 	struct clk_fractional_divider *fd = to_clk_fd(hw);
-	unsigned maxn = (fd->nmask >> fd->nshift) + 1;
+	unsigned maxn = clk_div_mask(fd->nwidth) + 1;
 	unsigned div;
 
 	if (!rate || rate >= *prate)
@@ -130,9 +130,11 @@ struct clk *clk_register_fractional_divider(struct device *dev,
 
 	fd->reg = reg;
 	fd->mshift = mshift;
-	fd->mmask = (BIT(mwidth) - 1) << mshift;
+	fd->mwidth = mwidth;
+	fd->mmask = clk_div_mask(mwidth) << mshift;
 	fd->nshift = nshift;
-	fd->nmask = (BIT(nwidth) - 1) << nshift;
+	fd->nwidth = nwidth;
+	fd->nmask = clk_div_mask(nwidth) << nshift;
 	fd->flags = clk_divider_flags;
 	fd->lock = lock;
 	fd->hw.init = &init;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 20b0b67..eb3c96f 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -480,13 +480,14 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
  *
  * Clock with adjustable fractional divider affecting its output frequency.
  */
-
 struct clk_fractional_divider {
 	struct clk_hw	hw;
 	void __iomem	*reg;
 	u8		mshift;
+	u8		mwidth;
 	u32		mmask;
 	u8		nshift;
+	u8		nwidth;
 	u32		nmask;
 	u8		flags;
 	spinlock_t	*lock;
-- 
2.1.4

[PATCH v3 3/3] 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                  |  1 +
 drivers/clk/clk-fractional-divider.c | 38 +++++++++++++++++++++---------------
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 0b474a0..3a1b1dc 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
diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
index 23a56a0..f060be8 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)
 
@@ -47,23 +48,29 @@ static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
 }
 
 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 = clk_div_mask(fd->nwidth) + 1;
-	unsigned div;
+	unsigned long scale;
+	unsigned long m, n;
+	u64 ret;
 
-	if (!rate || rate >= *prate)
-		return *prate;
+	if (!rate || rate >= *parent_rate)
+		return *parent_rate;
 
-	div = gcd(*prate, rate);
+	/* Get rate closer to *parent_rate */
+	scale = fls_long(*parent_rate / rate - 1);
+	if (scale > fd->nwidth)
+		rate <<= scale - fd->nwidth;
 
-	while ((*prate / div) > maxn) {
-		div <<= 1;
-		rate <<= 1;
-	}
+	rational_best_approximation(rate, *parent_rate,
+			clk_div_mask(fd->mwidth), clk_div_mask(fd->nwidth),
+			&m, &n);
 
-	return rate;
+	ret = (u64)*parent_rate * m;
+	do_div(ret, n);
+
+	return ret;
 }
 
 static void clk_fd_update(struct clk_fractional_divider *fd,
@@ -82,12 +89,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;
-	unsigned long div;
 	unsigned long m, n;
 
-	div = gcd(parent_rate, rate);
-	m = rate / div;
-	n = parent_rate / div;
+	rational_best_approximation(rate, parent_rate,
+			clk_div_mask(fd->mwidth), clk_div_mask(fd->nwidth),
+			&m, &n);
 
 	if (fd->lock) {
 		spin_lock_irqsave(fd->lock, flags);
-- 
2.1.4

Re: [PATCH v3 2/3] clk: fractional-divider: keep mwidth and nwidth internally

[Ming Lei]

On Wed, Apr 1, 2015 at 6:09 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> The patch adds mwidth and nwidth fields to the struct clk_fractional_divider
> for further usage. While here, use clk_div_mask() instead of open coding this
> functionality.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/clk/clk-fractional-divider.c | 8 +++++---
>  include/linux/clk-provider.h         | 3 ++-
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> index 786aa482..23a56a0 100644
> --- a/drivers/clk/clk-fractional-divider.c
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -50,7 +50,7 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
>                               unsigned long *prate)
>  {
>         struct clk_fractional_divider *fd = to_clk_fd(hw);
> -       unsigned maxn = (fd->nmask >> fd->nshift) + 1;
> +       unsigned maxn = clk_div_mask(fd->nwidth) + 1;

Looks clk_div_mask() can't be found when I build the latest
next with these 3 patches:

  CC      drivers/clk/clk-fractional-divider.o
drivers/clk/clk-fractional-divider.c: In function ‘clk_fd_round_rate’:
drivers/clk/clk-fractional-divider.c:67:4: error: implicit declaration
of function ‘clk_div_mask’ [-Werror=implicit-function-declaration]
    clk_div_mask(fd->mwidth), clk_div_mask(fd->nwidth),
    ^
cc1: some warnings being treated as errors
make[2]: *** [drivers/clk/clk-fractional-divider.o] Error 1

Thanks,

>         unsigned div;
>
>         if (!rate || rate >= *prate)
> @@ -130,9 +130,11 @@ struct clk *clk_register_fractional_divider(struct device *dev,
>
>         fd->reg = reg;
>         fd->mshift = mshift;
> -       fd->mmask = (BIT(mwidth) - 1) << mshift;
> +       fd->mwidth = mwidth;
> +       fd->mmask = clk_div_mask(mwidth) << mshift;
>         fd->nshift = nshift;
> -       fd->nmask = (BIT(nwidth) - 1) << nshift;
> +       fd->nwidth = nwidth;
> +       fd->nmask = clk_div_mask(nwidth) << nshift;
>         fd->flags = clk_divider_flags;
>         fd->lock = lock;
>         fd->hw.init = &init;
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 20b0b67..eb3c96f 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -480,13 +480,14 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
>   *
>   * Clock with adjustable fractional divider affecting its output frequency.
>   */
> -
>  struct clk_fractional_divider {
>         struct clk_hw   hw;
>         void __iomem    *reg;
>         u8              mshift;
> +       u8              mwidth;
>         u32             mmask;
>         u8              nshift;
> +       u8              nwidth;
>         u32             nmask;
>         u8              flags;
>         spinlock_t      *lock;
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Ming Lei

Re: [PATCH v3 2/3] clk: fractional-divider: keep mwidth and nwidth internally

[Andy Shevchenko]

On Thu, 2015-04-09 at 16:22 +0800, Ming Lei wrote:
> On Wed, Apr 1, 2015 at 6:09 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > The patch adds mwidth and nwidth fields to the struct clk_fractional_divider
> > for further usage. While here, use clk_div_mask() instead of open coding this
> > functionality.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/clk/clk-fractional-divider.c | 8 +++++---
> >  include/linux/clk-provider.h         | 3 ++-
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> > index 786aa482..23a56a0 100644
> > --- a/drivers/clk/clk-fractional-divider.c
> > +++ b/drivers/clk/clk-fractional-divider.c
> > @@ -50,7 +50,7 @@ static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
> >                               unsigned long *prate)
> >  {
> >         struct clk_fractional_divider *fd = to_clk_fd(hw);
> > -       unsigned maxn = (fd->nmask >> fd->nshift) + 1;
> > +       unsigned maxn = clk_div_mask(fd->nwidth) + 1;
> 
> Looks clk_div_mask() can't be found when I build the latest
> next with these 3 patches:

And you have read cover letter carefully, right?
It mentions that you have to apply at least one patch from the other
series: http://www.spinics.net/lists/kernel/msg1958958.html

> 
>   CC      drivers/clk/clk-fractional-divider.o
> drivers/clk/clk-fractional-divider.c: In function ‘clk_fd_round_rate’:
> drivers/clk/clk-fractional-divider.c:67:4: error: implicit declaration
> of function ‘clk_div_mask’ [-Werror=implicit-function-declaration]
>     clk_div_mask(fd->mwidth), clk_div_mask(fd->nwidth),
>     ^
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/clk/clk-fractional-divider.o] Error 1
> 
> Thanks,
> 
> >         unsigned div;
> >
> >         if (!rate || rate >= *prate)
> > @@ -130,9 +130,11 @@ struct clk *clk_register_fractional_divider(struct device *dev,
> >
> >         fd->reg = reg;
> >         fd->mshift = mshift;
> > -       fd->mmask = (BIT(mwidth) - 1) << mshift;
> > +       fd->mwidth = mwidth;
> > +       fd->mmask = clk_div_mask(mwidth) << mshift;
> >         fd->nshift = nshift;
> > -       fd->nmask = (BIT(nwidth) - 1) << nshift;
> > +       fd->nwidth = nwidth;
> > +       fd->nmask = clk_div_mask(nwidth) << nshift;
> >         fd->flags = clk_divider_flags;
> >         fd->lock = lock;
> >         fd->hw.init = &init;
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 20b0b67..eb3c96f 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -480,13 +480,14 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
> >   *
> >   * Clock with adjustable fractional divider affecting its output frequency.
> >   */
> > -
> >  struct clk_fractional_divider {
> >         struct clk_hw   hw;
> >         void __iomem    *reg;
> >         u8              mshift;
> > +       u8              mwidth;
> >         u32             mmask;
> >         u8              nshift;
> > +       u8              nwidth;
> >         u32             nmask;
> >         u8              flags;
> >         spinlock_t      *lock;
> > --
> > 2.1.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
> 


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

Re: [PATCH v3 0/3] clk: fractional-divider: do a clean up

[Stephen Boyd]

On 04/01, Andy Shevchenko wrote:
> Patches are self-explanatory I think. So, just changelog is provided here.
> It would be really nice to queue them to v4.1.
> 
> Patches 2/3 and 3/3 are dependent to the patch 1/7 from [1] "clk: replace
> div_mask() by clk_div_mask()".
> 
> The series was tested with 8250_dw UART driver on Intel Braswell.
> 

Please undo the dependency of this series on clk_div_mask and
then it can be applied, targeting 4.3.

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