[PATCH v3 0/7] clk: replace div_mask() by clk_div_mask()

[PATCH v3 0/7] clk: replace div_mask() by clk_div_mask()

[Andy Shevchenko]

This series introduces clk_div_mask() helper in one place instead of div_mask() macros in many clock providers.

Changelog v3:
- fix clk_div_mask() prototype: seems tired like on Friday evening!

Changelog v2:
- fix patches 5,6,7 where parameter of the macro was d instead of d->width

Andy Shevchenko (7):
  clk: introduce clk_div_mask() helper
  clk: mmp: switch to clk_div_mask()
  clk: divider: switch to clk_div_mask()
  clk: socfpga: switch to clk_div_mask()
  clk: ti: divider: switch to clk_div_mask()
  clk: tegra: switch to clk_div_mask()
  ARM: imx: switch to clk_div_mask()

 arch/arm/mach-imx/clk-fixup-div.c |  7 +++----
 drivers/clk/clk-divider.c         | 18 ++++++++----------
 drivers/clk/mmp/clk-mix.c         |  2 +-
 drivers/clk/socfpga/clk-gate.c    |  2 +-
 drivers/clk/socfpga/clk-periph.c  |  2 +-
 drivers/clk/socfpga/clk.h         |  1 -
 drivers/clk/tegra/clk-divider.c   |  7 +++----
 drivers/clk/ti/divider.c          | 18 ++++++++----------
 include/linux/clk-provider.h      |  5 +++++
 9 files changed, 30 insertions(+), 32 deletions(-)

-- 
2.1.4

[PATCH v3 1/7] clk: introduce clk_div_mask() helper

[Andy Shevchenko]

The patch introduces a helper that gives a divider mask based on register field
width.

The sequential patches will replace custom div_mask() macro in the
corresponding clock provider modules.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/clk-provider.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5591ea7..20b0b67 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -353,6 +353,11 @@ struct clk_divider {
 	spinlock_t	*lock;
 };
 
+static inline unsigned long clk_div_mask(u8 width)
+{
+	return (1 << width) - 1;
+}
+
 #define CLK_DIVIDER_ONE_BASED		BIT(0)
 #define CLK_DIVIDER_POWER_OF_TWO	BIT(1)
 #define CLK_DIVIDER_ALLOW_ZERO		BIT(2)
-- 
2.1.4

Re: [PATCH v3 1/7] clk: introduce clk_div_mask() helper

[Stephen Boyd]

On 03/31, Andy Shevchenko wrote:
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 5591ea7..20b0b67 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -353,6 +353,11 @@ struct clk_divider {
>  	spinlock_t	*lock;
>  };
>  
> +static inline unsigned long clk_div_mask(u8 width)
> +{
> +	return (1 << width) - 1;
> +}
> +

Why not just change drivers to use GENMASK? It's a proven and
tested way to generate a bitmask.

So I'd rather see drivers converted to use that macro directly
especially because the mask may need to start at some bit that
isn't 0.

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

Re: [PATCH v3 1/7] clk: introduce clk_div_mask() helper

[Andy Shevchenko]

On Thu, 2015-06-18 at 12:48 -0700, Stephen Boyd wrote:
> On 03/31, Andy Shevchenko wrote:
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk
> > -provider.h
> > index 5591ea7..20b0b67 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -353,6 +353,11 @@ struct clk_divider {
> >  	spinlock_t	*lock;
> >  };
> >  
> > +static inline unsigned long clk_div_mask(u8 width)
> > +{
> > +	return (1 << width) - 1;
> > +}
> > +
> 
> Why not just change drivers to use GENMASK? It's a proven and
> tested way to generate a bitmask.

Too many unneeded calculations I suppose.

Compare:
	mask = clk_div_mask(mm) << ms;
	which is simple ((1 << mm) - 1) << ms
and
	mask = GENMASK(mm + ms - 1, ms);
	which is (~0 << ms) & (~0 >> (BITS_PER_LONG - 1 - (mm + ms -
1)))

> 
> So I'd rather see drivers converted to use that macro directly
> especially because the mask may need to start at some bit that
> isn't 0.

If you think the above is not a burden, I can do the conversion to
GENMASK.

Though it might make sense when ms = 0 explicitly.

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

Re: [PATCH v3 1/7] clk: introduce clk_div_mask() helper

[Stephen Boyd]

On 07/07, Andy Shevchenko wrote:
> On Thu, 2015-06-18 at 12:48 -0700, Stephen Boyd wrote:
> > On 03/31, Andy Shevchenko wrote:
> > > diff --git a/include/linux/clk-provider.h b/include/linux/clk
> > > -provider.h
> > > index 5591ea7..20b0b67 100644
> > > --- a/include/linux/clk-provider.h
> > > +++ b/include/linux/clk-provider.h
> > > @@ -353,6 +353,11 @@ struct clk_divider {
> > >  	spinlock_t	*lock;
> > >  };
> > >  
> > > +static inline unsigned long clk_div_mask(u8 width)
> > > +{
> > > +	return (1 << width) - 1;
> > > +}
> > > +
> > 
> > Why not just change drivers to use GENMASK? It's a proven and
> > tested way to generate a bitmask.
> 
> Too many unneeded calculations I suppose.

That's what compiler optimizations are for.

> 
> Compare:
> 	mask = clk_div_mask(mm) << ms;
> 	which is simple ((1 << mm) - 1) << ms
> and
> 	mask = GENMASK(mm + ms - 1, ms);
> 	which is (~0 << ms) & (~0 >> (BITS_PER_LONG - 1 - (mm + ms -
> 1)))

And if mm is 32 then we hit undefined behavior.

> 
> > 
> > So I'd rather see drivers converted to use that macro directly
> > especially because the mask may need to start at some bit that
> > isn't 0.
> 
> If you think the above is not a burden, I can do the conversion to
> GENMASK.
> 
> Though it might make sense when ms = 0 explicitly.
> 

Yes let's use GENMASK. I imagine the extra few instructions are
negligible.

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

[PATCH v3 2/7] clk: mmp: switch to clk_div_mask()

[Andy Shevchenko]

Convert the code to use clk_div_mask() helper instead of custom approach.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/clk/mmp/clk-mix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/mmp/clk-mix.c b/drivers/clk/mmp/clk-mix.c
index de6a873..0810e27 100644
--- a/drivers/clk/mmp/clk-mix.c
+++ b/drivers/clk/mmp/clk-mix.c
@@ -26,7 +26,7 @@
 
 static unsigned int _get_maxdiv(struct mmp_clk_mix *mix)
 {
-	unsigned int div_mask = (1 << mix->reg_info.width_div) - 1;
+	unsigned int div_mask = clk_div_mask(mix->reg_info.width_div);
 	unsigned int maxdiv = 0;
 	struct clk_div_table *clkt;
 
-- 
2.1.4

[PATCH v3 3/7] clk: divider: switch to clk_div_mask()

[Andy Shevchenko]

Convert the code to use clk_div_mask() helper instead of div_mask() macro.

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

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 25006a8..9cd3d8f 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -30,8 +30,6 @@
 
 #define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
 
-#define div_mask(width)	((1 << (width)) - 1)
-
 static unsigned int _get_table_maxdiv(const struct clk_div_table *table)
 {
 	unsigned int maxdiv = 0;
@@ -58,12 +56,12 @@ static unsigned int _get_maxdiv(const struct clk_div_table *table, u8 width,
 				unsigned long flags)
 {
 	if (flags & CLK_DIVIDER_ONE_BASED)
-		return div_mask(width);
+		return clk_div_mask(width);
 	if (flags & CLK_DIVIDER_POWER_OF_TWO)
-		return 1 << div_mask(width);
+		return 1 << clk_div_mask(width);
 	if (table)
 		return _get_table_maxdiv(table);
-	return div_mask(width) + 1;
+	return clk_div_mask(width) + 1;
 }
 
 static unsigned int _get_table_div(const struct clk_div_table *table,
@@ -138,7 +136,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
 	unsigned int val;
 
 	val = clk_readl(divider->reg) >> divider->shift;
-	val &= div_mask(divider->width);
+	val &= clk_div_mask(divider->width);
 
 	return divider_recalc_rate(hw, parent_rate, val, divider->table,
 				   divider->flags);
@@ -350,7 +348,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 	/* if read only, just return current value */
 	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
 		bestdiv = readl(divider->reg) >> divider->shift;
-		bestdiv &= div_mask(divider->width);
+		bestdiv &= clk_div_mask(divider->width);
 		bestdiv = _get_div(divider->table, bestdiv, divider->flags);
 		return DIV_ROUND_UP(*prate, bestdiv);
 	}
@@ -372,7 +370,7 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate,
 
 	value = _get_val(table, div, flags);
 
-	return min_t(unsigned int, value, div_mask(width));
+	return min_t(unsigned int, value, clk_div_mask(width));
 }
 EXPORT_SYMBOL_GPL(divider_get_val);
 
@@ -391,10 +389,10 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 		spin_lock_irqsave(divider->lock, flags);
 
 	if (divider->flags & CLK_DIVIDER_HIWORD_MASK) {
-		val = div_mask(divider->width) << (divider->shift + 16);
+		val = clk_div_mask(divider->width) << (divider->shift + 16);
 	} else {
 		val = clk_readl(divider->reg);
-		val &= ~(div_mask(divider->width) << divider->shift);
+		val &= ~(clk_div_mask(divider->width) << divider->shift);
 	}
 	val |= value << divider->shift;
 	clk_writel(val, divider->reg);
-- 
2.1.4

[PATCH v3 4/7] clk: socfpga: switch to clk_div_mask()

[Andy Shevchenko]

Convert the code to use clk_div_mask() helper instead of div_mask() macro.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/clk/socfpga/clk-gate.c   | 2 +-
 drivers/clk/socfpga/clk-periph.c | 2 +-
 drivers/clk/socfpga/clk.h        | 1 -
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c
index dd3a78c..684d3bd 100644
--- a/drivers/clk/socfpga/clk-gate.c
+++ b/drivers/clk/socfpga/clk-gate.c
@@ -110,7 +110,7 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk,
 		div = socfpgaclk->fixed_div;
 	else if (socfpgaclk->div_reg) {
 		val = readl(socfpgaclk->div_reg) >> socfpgaclk->shift;
-		val &= div_mask(socfpgaclk->width);
+		val &= clk_div_mask(socfpgaclk->width);
 		/* Check for GPIO_DB_CLK by its offset */
 		if ((int) socfpgaclk->div_reg & SOCFPGA_GPIO_DB_CLK_OFFSET)
 			div = val + 1;
diff --git a/drivers/clk/socfpga/clk-periph.c b/drivers/clk/socfpga/clk-periph.c
index 46531c3..241cbc4 100644
--- a/drivers/clk/socfpga/clk-periph.c
+++ b/drivers/clk/socfpga/clk-periph.c
@@ -36,7 +36,7 @@ static unsigned long clk_periclk_recalc_rate(struct clk_hw *hwclk,
 	} else {
 		if (socfpgaclk->div_reg) {
 			val = readl(socfpgaclk->div_reg) >> socfpgaclk->shift;
-			val &= div_mask(socfpgaclk->width);
+			val &= clk_div_mask(socfpgaclk->width);
 			parent_rate /= (val + 1);
 		}
 		div = ((readl(socfpgaclk->hw.reg) & 0x1ff) + 1);
diff --git a/drivers/clk/socfpga/clk.h b/drivers/clk/socfpga/clk.h
index d291f60..5278156 100644
--- a/drivers/clk/socfpga/clk.h
+++ b/drivers/clk/socfpga/clk.h
@@ -27,7 +27,6 @@
 #define CLKMGR_PERPLL_SRC	0xAC
 
 #define SOCFPGA_MAX_PARENTS		3
-#define div_mask(width) ((1 << (width)) - 1)
 
 extern void __iomem *clk_mgr_base_addr;
 
-- 
2.1.4

[PATCH v3 5/7] clk: ti: divider: switch to clk_div_mask()

[Andy Shevchenko]

Convert the code to use clk_div_mask() helper instead of div_mask() macro.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/clk/ti/divider.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/ti/divider.c b/drivers/clk/ti/divider.c
index 6211893..d73f7ae 100644
--- a/drivers/clk/ti/divider.c
+++ b/drivers/clk/ti/divider.c
@@ -28,8 +28,6 @@
 
 #define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
 
-#define div_mask(d)	((1 << ((d)->width)) - 1)
-
 static unsigned int _get_table_maxdiv(const struct clk_div_table *table)
 {
 	unsigned int maxdiv = 0;
@@ -44,12 +42,12 @@ static unsigned int _get_table_maxdiv(const struct clk_div_table *table)
 static unsigned int _get_maxdiv(struct clk_divider *divider)
 {
 	if (divider->flags & CLK_DIVIDER_ONE_BASED)
-		return div_mask(divider);
+		return clk_div_mask(divider->width);
 	if (divider->flags & CLK_DIVIDER_POWER_OF_TWO)
-		return 1 << div_mask(divider);
+		return 1 << clk_div_mask(divider->width);
 	if (divider->table)
 		return _get_table_maxdiv(divider->table);
-	return div_mask(divider) + 1;
+	return clk_div_mask(divider->width) + 1;
 }
 
 static unsigned int _get_table_div(const struct clk_div_table *table,
@@ -103,7 +101,7 @@ static unsigned long ti_clk_divider_recalc_rate(struct clk_hw *hw,
 	unsigned int div, val;
 
 	val = ti_clk_ll_ops->clk_readl(divider->reg) >> divider->shift;
-	val &= div_mask(divider);
+	val &= clk_div_mask(divider->width);
 
 	div = _get_div(divider, val);
 	if (!div) {
@@ -225,17 +223,17 @@ static int ti_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
 	div = DIV_ROUND_UP(parent_rate, rate);
 	value = _get_val(divider, div);
 
-	if (value > div_mask(divider))
-		value = div_mask(divider);
+	if (value > clk_div_mask(divider->width))
+		value = clk_div_mask(divider->width);
 
 	if (divider->lock)
 		spin_lock_irqsave(divider->lock, flags);
 
 	if (divider->flags & CLK_DIVIDER_HIWORD_MASK) {
-		val = div_mask(divider) << (divider->shift + 16);
+		val = clk_div_mask(divider->width) << (divider->shift + 16);
 	} else {
 		val = ti_clk_ll_ops->clk_readl(divider->reg);
-		val &= ~(div_mask(divider) << divider->shift);
+		val &= ~(clk_div_mask(divider->width) << divider->shift);
 	}
 	val |= value << divider->shift;
 	ti_clk_ll_ops->clk_writel(val, divider->reg);
-- 
2.1.4

[PATCH v3 6/7] clk: tegra: switch to clk_div_mask()

[Andy Shevchenko]

Convert the code to use clk_div_mask() helper instead of div_mask() macro.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/clk/tegra/clk-divider.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
index 59a5714..d23dbc1 100644
--- a/drivers/clk/tegra/clk-divider.c
+++ b/drivers/clk/tegra/clk-divider.c
@@ -24,9 +24,8 @@
 #include "clk.h"
 
 #define pll_out_override(p) (BIT((p->shift - 6)))
-#define div_mask(d) ((1 << (d->width)) - 1)
 #define get_mul(d) (1 << d->frac_width)
-#define get_max_div(d) div_mask(d)
+#define get_max_div(d) clk_div_mask(d->width)
 
 #define PERIPH_CLK_UART_DIV_ENB BIT(24)
 
@@ -73,7 +72,7 @@ static unsigned long clk_frac_div_recalc_rate(struct clk_hw *hw,
 	u64 rate = parent_rate;
 
 	reg = readl_relaxed(divider->reg) >> divider->shift;
-	div = reg & div_mask(divider);
+	div = reg & clk_div_mask(divider->width);
 
 	mul = get_mul(divider);
 	div += mul;
@@ -120,7 +119,7 @@ static int clk_frac_div_set_rate(struct clk_hw *hw, unsigned long rate,
 		spin_lock_irqsave(divider->lock, flags);
 
 	val = readl_relaxed(divider->reg);
-	val &= ~(div_mask(divider) << divider->shift);
+	val &= ~(clk_div_mask(divider->width) << divider->shift);
 	val |= div << divider->shift;
 
 	if (divider->flags & TEGRA_DIVIDER_UART) {
-- 
2.1.4

[PATCH v3 7/7] ARM: imx: switch to clk_div_mask()

[Andy Shevchenko]

Convert the code to use clk_div_mask() helper instead of div_mask() macro.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/arm/mach-imx/clk-fixup-div.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-imx/clk-fixup-div.c b/arch/arm/mach-imx/clk-fixup-div.c
index 21db020..e5a83bc 100644
--- a/arch/arm/mach-imx/clk-fixup-div.c
+++ b/arch/arm/mach-imx/clk-fixup-div.c
@@ -16,7 +16,6 @@
 #include "clk.h"
 
 #define to_clk_div(_hw) container_of(_hw, struct clk_divider, hw)
-#define div_mask(d)	((1 << (d->width)) - 1)
 
 /**
  * struct clk_fixup_div - imx integer fixup divider clock
@@ -70,13 +69,13 @@ static int clk_fixup_div_set_rate(struct clk_hw *hw, unsigned long rate,
 	/* Zero based divider */
 	value = divider - 1;
 
-	if (value > div_mask(div))
-		value = div_mask(div);
+	if (value > clk_div_mask(div->width))
+		value = clk_div_mask(div->width);
 
 	spin_lock_irqsave(div->lock, flags);
 
 	val = readl(div->reg);
-	val &= ~(div_mask(div) << div->shift);
+	val &= ~(clk_div_mask(div->width) << div->shift);
 	val |= value << div->shift;
 	fixup_div->fixup(&val);
 	writel(val, div->reg);
-- 
2.1.4

Re: [PATCH v3 0/7] clk: replace div_mask() by clk_div_mask()

[Russell King - ARM Linux]

On Tue, Mar 31, 2015 at 08:22:20PM +0300, Andy Shevchenko wrote:
> This series introduces clk_div_mask() helper in one place instead of div_mask() macros in many clock providers.
> 
> Changelog v3:
> - fix clk_div_mask() prototype: seems tired like on Friday evening!

Yea, that's three versions in the space of half an hour.  Some of
us, who are buried in email, really don't need such stuff filling
our mailboxes with junk like this.

At least build your changes before posting them - or if you haven't
say that you haven't, and then don't worry about it not building.
Don't flood people's mailboxes with three versions of the patches
because they weren't actually ready to be posted.

What this shows is that you should never rush patches out at the
last minute, that's always a recipe for mistakes to be made. :)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 0/7] clk: replace div_mask() by clk_div_mask()

[Andy Shevchenko]

On Tue, 2015-03-31 at 20:22 +0300, Andy Shevchenko wrote:
> This series introduces clk_div_mask() helper in one place instead of div_mask() macros in many clock providers.

This one is tested on x86 with patchset [1] applied.

Any comments, suggestions?

[1] https://lkml.org/lkml/2015/4/1/189

> 
> Changelog v3:
> - fix clk_div_mask() prototype: seems tired like on Friday evening!
> 
> Changelog v2:
> - fix patches 5,6,7 where parameter of the macro was d instead of d->width
> 
> Andy Shevchenko (7):
>   clk: introduce clk_div_mask() helper
>   clk: mmp: switch to clk_div_mask()
>   clk: divider: switch to clk_div_mask()
>   clk: socfpga: switch to clk_div_mask()
>   clk: ti: divider: switch to clk_div_mask()
>   clk: tegra: switch to clk_div_mask()
>   ARM: imx: switch to clk_div_mask()
> 
>  arch/arm/mach-imx/clk-fixup-div.c |  7 +++----
>  drivers/clk/clk-divider.c         | 18 ++++++++----------
>  drivers/clk/mmp/clk-mix.c         |  2 +-
>  drivers/clk/socfpga/clk-gate.c    |  2 +-
>  drivers/clk/socfpga/clk-periph.c  |  2 +-
>  drivers/clk/socfpga/clk.h         |  1 -
>  drivers/clk/tegra/clk-divider.c   |  7 +++----
>  drivers/clk/ti/divider.c          | 18 ++++++++----------
>  include/linux/clk-provider.h      |  5 +++++
>  9 files changed, 30 insertions(+), 32 deletions(-)
> 


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

Re: [PATCH v3 0/7] clk: replace div_mask() by clk_div_mask()

[Stephen Boyd]

On 05/06, Andy Shevchenko wrote:
> On Tue, 2015-03-31 at 20:22 +0300, Andy Shevchenko wrote:
> > This series introduces clk_div_mask() helper in one place instead of div_mask() macros in many clock providers.
> 
> This one is tested on x86 with patchset [1] applied.
> 
> Any comments, suggestions?
> 

Given that it went through multiple revisions so fast I've put it
near the bottom of my queue. I'll get to it sometime soon, or
maybe Mike can look in the meantime.

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