[PATCH 0/5] clk: read-only dividers and rate propagation fixup

[PATCH 0/5] clk: read-only dividers and rate propagation fixup

[Jerome Brunet]

A read-only divider may also have CLK_SET_RATE_PARENT flag set, in which
case it should propagate the requested rate to its parent, taking the
read-only divider value into account.

While this is done correctly in qcom's clk-regmap-divider, it is not in
the generic divider and lpc32xx.

Other drivers using divider_round_rate are not impacted because they are
using hard-coded flags without CLK_DIVIDER_READ_ONLY, so read-only
dividers does seems to be concern for them and rate propagation should
work as expected

Jerome Brunet (5):
  clk: divider: read-only divider can propagate rate change
  clk: lpc32xx: read-only divider can propagate rate change
  clk: divider: add divider_ro_round_rate helper
  clk: lpc32xx: use divider_ro_round_rate helper
  clk: qcom: use divider_ro_round_rate helper

 drivers/clk/clk-divider.c             | 35 +++++++++++++++++++++++++++++------
 drivers/clk/nxp/clk-lpc32xx.c         | 15 ++++++++-------
 drivers/clk/qcom/clk-regmap-divider.c | 19 ++++++-------------
 include/linux/clk-provider.h          | 15 +++++++++++++++
 4 files changed, 58 insertions(+), 26 deletions(-)

-- 
2.14.3

[PATCH 1/5] clk: divider: read-only divider can propagate rate change

[Jerome Brunet]

When a divider clock has CLK_DIVIDER_READ_ONLY set, it means that the
register shall be left un-touched, but it does not mean the clock
should stop rate propagation if CLK_SET_RATE_PARENT is set

This is properly handled in qcom clk-regmap-divider but it was not in
the generic divider

Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk-divider.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index b49942b9fe50..a851d3e04c7f 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -348,6 +348,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long *prate)
 {
 	struct clk_divider *divider = to_clk_divider(hw);
+	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
 	int bestdiv;
 
 	/* if read only, just return current value */
@@ -356,6 +357,15 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 		bestdiv &= div_mask(divider->width);
 		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
 			divider->width);
+
+		/* Even a read-only clock can propagate a rate change */
+		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
+			if (!hw_parent)
+				return -EINVAL;
+
+			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
+		}
+
 		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
 	}
 
-- 
2.14.3

[1/5] clk: divider: read-only divider can propagate rate change

[David Lechner]

On 01/05/2018 11:09 AM, Jerome Brunet wrote:
> When a divider clock has CLK_DIVIDER_READ_ONLY set, it means that the
> register shall be left un-touched, but it does not mean the clock
> should stop rate propagation if CLK_SET_RATE_PARENT is set
> 
> This is properly handled in qcom clk-regmap-divider but it was not in
> the generic divider
> 
> Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/clk/clk-divider.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index b49942b9fe50..a851d3e04c7f 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -348,6 +348,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
>   				unsigned long *prate)
>   {
>   	struct clk_divider *divider = to_clk_divider(hw);
> +	struct clk_hw *hw_parent = clk_hw_get_parent(hw);

Very minor suggestion: This could be moved inside the if block since it is only used there.

>   	int bestdiv;
>   
>   	/* if read only, just return current value */
> @@ -356,6 +357,15 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
>   		bestdiv &= div_mask(divider->width);
>   		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
>   			divider->width);
> +
> +		/* Even a read-only clock can propagate a rate change */
> +		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> +			if (!hw_parent)
> +				return -EINVAL;
> +
> +			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
> +		}
> +
>   		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
>   	}
>   
> 

Tested-by: David Lechner <david@lechnology.com>

[1/5] clk: divider: read-only divider can propagate rate change

[Jerome Brunet]

On Thu, 2018-01-11 at 16:55 -0600, David Lechner wrote:
> On 01/05/2018 11:09 AM, Jerome Brunet wrote:
> > When a divider clock has CLK_DIVIDER_READ_ONLY set, it means that the
> > register shall be left un-touched, but it does not mean the clock
> > should stop rate propagation if CLK_SET_RATE_PARENT is set
> > 
> > This is properly handled in qcom clk-regmap-divider but it was not in
> > the generic divider
> > 
> > Fixes: e6d5e7d90be9 ("clk-divider: Fix READ_ONLY when divider > 1")
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> >   drivers/clk/clk-divider.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> > index b49942b9fe50..a851d3e04c7f 100644
> > --- a/drivers/clk/clk-divider.c
> > +++ b/drivers/clk/clk-divider.c
> > @@ -348,6 +348,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
> >   				unsigned long *prate)
> >   {
> >   	struct clk_divider *divider = to_clk_divider(hw);
> > +	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
> 
> Very minor suggestion: This could be moved inside the if block since it is only used there.

Even if correct, this is something I have seen done in CCF. I tend to just
follow the way to be honest.

Stephen, do you have any preference ?

> 
> >   	int bestdiv;
> >   
> >   	/* if read only, just return current value */
> > @@ -356,6 +357,15 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
> >   		bestdiv &= div_mask(divider->width);
> >   		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
> >   			divider->width);
> > +
> > +		/* Even a read-only clock can propagate a rate change */
> > +		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> > +			if (!hw_parent)
> > +				return -EINVAL;
> > +
> > +			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
> > +		}
> > +
> >   		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
> >   	}
> >   
> > 
> 
> Tested-by: David Lechner <david@lechnology.com>

[PATCH 2/5] clk: lpc32xx: read-only divider can propagate rate change

[Jerome Brunet]

When a divider clock has CLK_DIVIDER_READ_ONLY set, it means that the
register shall be left un-touched, but it does not mean the clock
should stop rate propagation if CLK_SET_RATE_PARENT is set

This properly handled in qcom clk-regmap-divider but it was not in the
lpc32xx divider

Fixes: f7c82a60ba26 ("clk: lpc32xx: add common clock framework driver")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/nxp/clk-lpc32xx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/nxp/clk-lpc32xx.c b/drivers/clk/nxp/clk-lpc32xx.c
index f5d815f577e0..729333766f97 100644
--- a/drivers/clk/nxp/clk-lpc32xx.c
+++ b/drivers/clk/nxp/clk-lpc32xx.c
@@ -963,6 +963,7 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long *prate)
 {
 	struct lpc32xx_clk_div *divider = to_lpc32xx_div(hw);
+	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
 	unsigned int bestdiv;
 
 	/* if read only, just return current value */
@@ -972,6 +973,15 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 		bestdiv &= div_mask(divider->width);
 		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
 			divider->width);
+
+		/* Even a read-only clock can propagate a rate change */
+		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
+			if (!hw_parent)
+				return -EINVAL;
+
+			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
+		}
+
 		return DIV_ROUND_UP(*prate, bestdiv);
 	}
 
-- 
2.14.3

[PATCH 2/5] clk: lpc32xx: read-only divider can propagate rate change

[Vladimir Zapolskiy]

Hi Jerome,

On 01/05/2018 07:09 PM, Jerome Brunet wrote:
> When a divider clock has CLK_DIVIDER_READ_ONLY set, it means that the
> register shall be left un-touched, but it does not mean the clock
> should stop rate propagation if CLK_SET_RATE_PARENT is set
> 

okay, the statement sounds correct, but there is no such clocks on LPC32xx,
thus I hardly can confirm that adding dead/inapplicable code is a fix.

> This properly handled in qcom clk-regmap-divider but it was not in the
> lpc32xx divider
> 
> Fixes: f7c82a60ba26 ("clk: lpc32xx: add common clock framework driver")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

I would suggest to drop two LPC32xx clock driver changes from the series.

--
With best wishes,
Vladimir

[PATCH 2/5] clk: lpc32xx: read-only divider can propagate rate change

[Jerome Brunet]

On Fri, 2018-01-05 at 20:12 +0200, Vladimir Zapolskiy wrote:
> Hi Jerome,
> 
> On 01/05/2018 07:09 PM, Jerome Brunet wrote:
> > When a divider clock has CLK_DIVIDER_READ_ONLY set, it means that the
> > register shall be left un-touched, but it does not mean the clock
> > should stop rate propagation if CLK_SET_RATE_PARENT is set
> > 
> 
> okay, the statement sounds correct, but there is no such clocks on LPC32xx,
> thus I hardly can confirm that adding dead/inapplicable code is a fix.
> 
> > This properly handled in qcom clk-regmap-divider but it was not in the
> > lpc32xx divider
> > 
> > Fixes: f7c82a60ba26 ("clk: lpc32xx: add common clock framework driver")
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> 
> I would suggest to drop two LPC32xx clock driver changes from the series.

Hi Vladimir,

This is fine by me. Whether LPC32xx supports CLK_DIVIDER_READ_ONLY is up to you,
but you should be consistent about it.

I added the fix to LPC32xx because it looks like the generic divider (a lot) and
appears to support CLK_DIVIDER_READ_ONLY. If it does not, could you please kill
the related code ?

Regards
Jerome

> 
> --
> With best wishes,
> Vladimir

[PATCH 2/5] clk: lpc32xx: read-only divider can propagate rate change

[Vladimir Zapolskiy]

Hi Jerome,

On 01/05/2018 09:40 PM, Jerome Brunet wrote:
> On Fri, 2018-01-05 at 20:12 +0200, Vladimir Zapolskiy wrote:
>> Hi Jerome,
>>
>> On 01/05/2018 07:09 PM, Jerome Brunet wrote:
>>> When a divider clock has CLK_DIVIDER_READ_ONLY set, it means that the
>>> register shall be left un-touched, but it does not mean the clock
>>> should stop rate propagation if CLK_SET_RATE_PARENT is set
>>>
>>
>> okay, the statement sounds correct, but there is no such clocks on LPC32xx,
>> thus I hardly can confirm that adding dead/inapplicable code is a fix.
>>
>>> This properly handled in qcom clk-regmap-divider but it was not in the
>>> lpc32xx divider
>>>
>>> Fixes: f7c82a60ba26 ("clk: lpc32xx: add common clock framework driver")
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>
>> I would suggest to drop two LPC32xx clock driver changes from the series.
> 
> Hi Vladimir,
> 
> This is fine by me. Whether LPC32xx supports CLK_DIVIDER_READ_ONLY is up to you,
> but you should be consistent about it.
> 
> I added the fix to LPC32xx because it looks like the generic divider (a lot) and

right, the relevant divider operations were copied, however the difference
is important, unfortunately there is no simple option to get rid of regmap,
because System Control Block registers are shared with a number of other
device drivers.

> appears to support CLK_DIVIDER_READ_ONLY. If it does not, could you please kill
> the related code ?

The driver supports CLK_DIVIDER_READ_ONLY clocks, and it should not be
changed, but all such clocks don't have children with CLK_SET_RATE_PARENT
property, which invalidates your fix for LPC32xx. Please let me know,
if I missed something.

--
With best wishes,
Vladimir

[PATCH 2/5] clk: lpc32xx: read-only divider can propagate rate change

[Jerome Brunet]

On Sat, 2018-01-06 at 16:04 +0200, Vladimir Zapolskiy wrote:
> > I added the fix to LPC32xx because it looks like the generic divider (a lot) and
> 
> right, the relevant divider operations were copied, however the difference
> is important, unfortunately there is no simple option to get rid of regmap,
> because System Control Block registers are shared with a number of other
> device drivers.

I have the same issue ;)

> 
> > appears to support CLK_DIVIDER_READ_ONLY. If it does not, could you please kill
> > the related code ?
> 
> The driver supports CLK_DIVIDER_READ_ONLY clocks, and it should not be
> changed, but all such clocks don't have children with CLK_SET_RATE_PARENT
> property, which invalidates your fix for LPC32xx. Please let me know,
> if I missed something.

You did not miss anything. I understand your choice.
I just have different approach and usually prefer to avoid these particularity
which may catch you later on. 

At least, the fact that propagation would stop with CLK_DIVIDER_READ_ONLY on
LPC32xx, even with CLK_SET_RATE_PARENT, is now known.

Adding a comment in the code to make this explicit would be nice though.

Regards
Jerome

[PATCH 3/5] clk: divider: add divider_ro_round_rate helper

[Jerome Brunet]

Like divider_round_rate, a couple a of driver are doing more or less
the same operation to round the rate of the divider when it is read-only.

We can factor this code so let's provide an helper function for this

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk-divider.c    | 43 ++++++++++++++++++++++++++++---------------
 include/linux/clk-provider.h | 15 +++++++++++++++
 2 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index a851d3e04c7f..3eb2b27f3513 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -344,29 +344,42 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 }
 EXPORT_SYMBOL_GPL(divider_round_rate_parent);
 
+long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
+				  unsigned long rate, unsigned long *prate,
+				  const struct clk_div_table *table, u8 width,
+				  unsigned long flags, unsigned int val)
+{
+	int div;
+
+	div = _get_div(table, val, flags, width);
+
+	/* Even a read-only clock can propagate a rate change */
+	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
+		if (!parent)
+			return -EINVAL;
+
+		*prate = clk_hw_round_rate(parent, rate * div);
+	}
+
+	return DIV_ROUND_UP_ULL((u64)*prate, div);
+}
+EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);
+
+
 static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long *prate)
 {
 	struct clk_divider *divider = to_clk_divider(hw);
-	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
-	int bestdiv;
+	u32 val;
 
 	/* if read only, just return current value */
 	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
-		bestdiv = clk_readl(divider->reg) >> divider->shift;
-		bestdiv &= div_mask(divider->width);
-		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
-			divider->width);
-
-		/* Even a read-only clock can propagate a rate change */
-		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-			if (!hw_parent)
-				return -EINVAL;
-
-			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
-		}
+		val = clk_readl(divider->reg) >> divider->shift;
+		val &= div_mask(divider->width);
 
-		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
+		return divider_ro_round_rate(hw, rate, prate, divider->table,
+					     divider->width, divider->flags,
+					     val);
 	}
 
 	return divider_round_rate(hw, rate, prate, divider->table,
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 175a62a15619..eb2c3a035e98 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -417,6 +417,10 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 			       unsigned long rate, unsigned long *prate,
 			       const struct clk_div_table *table,
 			       u8 width, unsigned long flags);
+long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
+				  unsigned long rate, unsigned long *prate,
+				  const struct clk_div_table *table, u8 width,
+				  unsigned long flags, unsigned int val);
 int divider_get_val(unsigned long rate, unsigned long parent_rate,
 		const struct clk_div_table *table, u8 width,
 		unsigned long flags);
@@ -772,6 +776,17 @@ static inline long divider_round_rate(struct clk_hw *hw, unsigned long rate,
 					 rate, prate, table, width, flags);
 }
 
+static inline long divider_ro_round_rate(struct clk_hw *hw, unsigned long rate,
+					 unsigned long *prate,
+					 const struct clk_div_table *table,
+					 u8 width, unsigned long flags,
+					 unsigned int val)
+{
+	return divider_ro_round_rate_parent(hw, clk_hw_get_parent(hw),
+					    rate, prate, table, width, flags,
+					    val);
+}
+
 /*
  * FIXME clock api without lock protection
  */
-- 
2.14.3

[3/5] clk: divider: add divider_ro_round_rate helper

[David Lechner]

On 01/05/2018 11:09 AM, Jerome Brunet wrote:
> Like divider_round_rate, a couple a of driver are doing more or less
> the same operation to round the rate of the divider when it is read-only.
> 
> We can factor this code so let's provide an helper function for this

Perhaps you could also make use of this new helper here (clk-divider.c):

const struct clk_ops clk_divider_ro_ops = {
	.recalc_rate = clk_divider_recalc_rate,
	.round_rate = clk_divider_round_rate,
};
EXPORT_SYMBOL_GPL(clk_divider_ro_ops);

> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>   drivers/clk/clk-divider.c    | 43 ++++++++++++++++++++++++++++---------------
>   include/linux/clk-provider.h | 15 +++++++++++++++
>   2 files changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index a851d3e04c7f..3eb2b27f3513 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -344,29 +344,42 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
>   }
>   EXPORT_SYMBOL_GPL(divider_round_rate_parent);
>   

It is nice to have documentation comments on public functions. Especially
in this case where @prate is an in/out parameter and @table is optional.

> +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> +				  unsigned long rate, unsigned long *prate,
> +				  const struct clk_div_table *table, u8 width,
> +				  unsigned long flags, unsigned int val)
> +{
> +	int div;
> +
> +	div = _get_div(table, val, flags, width);
> +
> +	/* Even a read-only clock can propagate a rate change */
> +	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> +		if (!parent)
> +			return -EINVAL;
> +
> +		*prate = clk_hw_round_rate(parent, rate * div);
> +	}
> +
> +	return DIV_ROUND_UP_ULL((u64)*prate, div);
> +}
> +EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);
> +
> +
>   static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
>   				unsigned long *prate)
>   {
>   	struct clk_divider *divider = to_clk_divider(hw);
> -	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
> -	int bestdiv;
> +	u32 val;
>   
>   	/* if read only, just return current value */
>   	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> -		bestdiv = clk_readl(divider->reg) >> divider->shift;
> -		bestdiv &= div_mask(divider->width);
> -		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
> -			divider->width);
> -
> -		/* Even a read-only clock can propagate a rate change */
> -		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> -			if (!hw_parent)
> -				return -EINVAL;
> -
> -			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
> -		}
> +		val = clk_readl(divider->reg) >> divider->shift;
> +		val &= div_mask(divider->width);
>   
> -		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
> +		return divider_ro_round_rate(hw, rate, prate, divider->table,
> +					     divider->width, divider->flags,
> +					     val);
>   	}
>   
>   	return divider_round_rate(hw, rate, prate, divider->table,
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 175a62a15619..eb2c3a035e98 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -417,6 +417,10 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
>   			       unsigned long rate, unsigned long *prate,
>   			       const struct clk_div_table *table,
>   			       u8 width, unsigned long flags);
> +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> +				  unsigned long rate, unsigned long *prate,
> +				  const struct clk_div_table *table, u8 width,
> +				  unsigned long flags, unsigned int val);
>   int divider_get_val(unsigned long rate, unsigned long parent_rate,
>   		const struct clk_div_table *table, u8 width,
>   		unsigned long flags);
> @@ -772,6 +776,17 @@ static inline long divider_round_rate(struct clk_hw *hw, unsigned long rate,
>   					 rate, prate, table, width, flags);
>   }
>   

Same here (about documentation comment).

> +static inline long divider_ro_round_rate(struct clk_hw *hw, unsigned long rate,
> +					 unsigned long *prate,
> +					 const struct clk_div_table *table,
> +					 u8 width, unsigned long flags,
> +					 unsigned int val)
> +{
> +	return divider_ro_round_rate_parent(hw, clk_hw_get_parent(hw),
> +					    rate, prate, table, width, flags,
> +					    val);
> +}
> +
>   /*
>    * FIXME clock api without lock protection
>    */
> 

[3/5] clk: divider: add divider_ro_round_rate helper

[Jerome Brunet]

On Thu, 2018-01-11 at 17:08 -0600, David Lechner wrote:
> On 01/05/2018 11:09 AM, Jerome Brunet wrote:
> > Like divider_round_rate, a couple a of driver are doing more or less
> > the same operation to round the rate of the divider when it is read-only.
> > 
> > We can factor this code so let's provide an helper function for this
> 
> Perhaps you could also make use of this new helper here (clk-divider.c):
> 
> const struct clk_ops clk_divider_ro_ops = {
> 	.recalc_rate = clk_divider_recalc_rate,
> 	.round_rate = clk_divider_round_rate,
> };
> EXPORT_SYMBOL_GPL(clk_divider_ro_ops);

The helper is used in this driver and ro_ops will use it.
I don't get this comment, could you be more specific

> 
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> >   drivers/clk/clk-divider.c    | 43 ++++++++++++++++++++++++++++---------------
> >   include/linux/clk-provider.h | 15 +++++++++++++++
> >   2 files changed, 43 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> > index a851d3e04c7f..3eb2b27f3513 100644
> > --- a/drivers/clk/clk-divider.c
> > +++ b/drivers/clk/clk-divider.c
> > @@ -344,29 +344,42 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> >   }
> >   EXPORT_SYMBOL_GPL(divider_round_rate_parent);
> >   
> 
> It is nice to have documentation comments on public functions. Especially
> in this case where @prate is an in/out parameter and @table is optional.

Sure, but divider_round_rate_parent was already and undocumented. There is no
reason to change this is this particular patch (it is not the topic)

The RO helper being the counter part of this one, it did not do it differently

I suppose a documentation could be added in another patch.

However, I don't know if there is policy regarding the documentation of
functions internal to CCF. Maybe Stephen can tell us ?

Cheers
Jerome


> 
> > +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> > +				  unsigned long rate, unsigned long *prate,
> > +				  const struct clk_div_table *table, u8 width,
> > +				  unsigned long flags, unsigned int val)
> > +{
> > +	int div;
> > +
> > +	div = _get_div(table, val, flags, width);
> > +
> > +	/* Even a read-only clock can propagate a rate change */
> > +	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> > +		if (!parent)
> > +			return -EINVAL;
> > +
> > +		*prate = clk_hw_round_rate(parent, rate * div);
> > +	}
> > +
> > +	return DIV_ROUND_UP_ULL((u64)*prate, div);
> > +}
> > +EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);
> > +
> > +
> >   static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
> >   				unsigned long *prate)
> >   {
> >   	struct clk_divider *divider = to_clk_divider(hw);
> > -	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
> > -	int bestdiv;
> > +	u32 val;
> >   
> >   	/* if read only, just return current value */
> >   	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
> > -		bestdiv = clk_readl(divider->reg) >> divider->shift;
> > -		bestdiv &= div_mask(divider->width);
> > -		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
> > -			divider->width);
> > -
> > -		/* Even a read-only clock can propagate a rate change */
> > -		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> > -			if (!hw_parent)
> > -				return -EINVAL;
> > -
> > -			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
> > -		}
> > +		val = clk_readl(divider->reg) >> divider->shift;
> > +		val &= div_mask(divider->width);
> >   
> > -		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
> > +		return divider_ro_round_rate(hw, rate, prate, divider->table,
> > +					     divider->width, divider->flags,
> > +					     val);
> >   	}
> >   
> >   	return divider_round_rate(hw, rate, prate, divider->table,
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 175a62a15619..eb2c3a035e98 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -417,6 +417,10 @@ long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> >   			       unsigned long rate, unsigned long *prate,
> >   			       const struct clk_div_table *table,
> >   			       u8 width, unsigned long flags);
> > +long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
> > +				  unsigned long rate, unsigned long *prate,
> > +				  const struct clk_div_table *table, u8 width,
> > +				  unsigned long flags, unsigned int val);
> >   int divider_get_val(unsigned long rate, unsigned long parent_rate,
> >   		const struct clk_div_table *table, u8 width,
> >   		unsigned long flags);
> > @@ -772,6 +776,17 @@ static inline long divider_round_rate(struct clk_hw *hw, unsigned long rate,
> >   					 rate, prate, table, width, flags);
> >   }
> >   
> 
> Same here (about documentation comment).
> 
> > +static inline long divider_ro_round_rate(struct clk_hw *hw, unsigned long rate,
> > +					 unsigned long *prate,
> > +					 const struct clk_div_table *table,
> > +					 u8 width, unsigned long flags,
> > +					 unsigned int val)
> > +{
> > +	return divider_ro_round_rate_parent(hw, clk_hw_get_parent(hw),
> > +					    rate, prate, table, width, flags,
> > +					    val);
> > +}
> > +
> >   /*
> >    * FIXME clock api without lock protection
> >    */
> > 
> 
> 

[3/5] clk: divider: add divider_ro_round_rate helper

[David Lechner]

On 01/17/2018 11:47 AM, Jerome Brunet wrote:
> On Thu, 2018-01-11 at 17:08 -0600, David Lechner wrote:
>> On 01/05/2018 11:09 AM, Jerome Brunet wrote:
>>> Like divider_round_rate, a couple a of driver are doing more or less
>>> the same operation to round the rate of the divider when it is read-only.
>>>
>>> We can factor this code so let's provide an helper function for this
>>
>> Perhaps you could also make use of this new helper here (clk-divider.c):
>>
>> const struct clk_ops clk_divider_ro_ops = {
>> 	.recalc_rate = clk_divider_recalc_rate,
>> 	.round_rate = clk_divider_round_rate,
>> };
>> EXPORT_SYMBOL_GPL(clk_divider_ro_ops);
> 
> The helper is used in this driver and ro_ops will use it.
> I don't get this comment, could you be more specific

I suppose I am trying to make things too complicated. Let's just forget
about this.

[PATCH 4/5] clk: lpc32xx: use divider_ro_round_rate helper

[Jerome Brunet]

There is now an helper function to round the rate when the
divider is read-only. Let's use it

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/nxp/clk-lpc32xx.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/nxp/clk-lpc32xx.c b/drivers/clk/nxp/clk-lpc32xx.c
index 729333766f97..c20285e74b8e 100644
--- a/drivers/clk/nxp/clk-lpc32xx.c
+++ b/drivers/clk/nxp/clk-lpc32xx.c
@@ -963,26 +963,17 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long *prate)
 {
 	struct lpc32xx_clk_div *divider = to_lpc32xx_div(hw);
-	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
-	unsigned int bestdiv;
+	unsigned int val;
 
 	/* if read only, just return current value */
 	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
-		regmap_read(clk_regmap, divider->reg, &bestdiv);
-		bestdiv >>= divider->shift;
-		bestdiv &= div_mask(divider->width);
-		bestdiv = _get_div(divider->table, bestdiv, divider->flags,
-			divider->width);
-
-		/* Even a read-only clock can propagate a rate change */
-		if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-			if (!hw_parent)
-				return -EINVAL;
-
-			*prate = clk_hw_round_rate(hw_parent, rate * bestdiv);
-		}
+		regmap_read(clk_regmap, divider->reg, &val);
+		val >>= divider->shift;
+		val &= div_mask(divider->width);
 
-		return DIV_ROUND_UP(*prate, bestdiv);
+		return divider_ro_round_rate(hw, rate, prate, divider->table,
+					     divider->width, divider->flags,
+					     bestdiv);
 	}
 
 	return divider_round_rate(hw, rate, prate, divider->table,
-- 
2.14.3

[PATCH 5/5] clk: qcom: use divider_ro_round_rate helper

[Jerome Brunet]

There is now an helper function to round the rate when the
divider is read-only. Let's use it

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/qcom/clk-regmap-divider.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/qcom/clk-regmap-divider.c b/drivers/clk/qcom/clk-regmap-divider.c
index 4e9b8c2c8980..114e36b97255 100644
--- a/drivers/clk/qcom/clk-regmap-divider.c
+++ b/drivers/clk/qcom/clk-regmap-divider.c
@@ -28,22 +28,15 @@ static long div_round_ro_rate(struct clk_hw *hw, unsigned long rate,
 {
 	struct clk_regmap_div *divider = to_clk_regmap_div(hw);
 	struct clk_regmap *clkr = &divider->clkr;
-	u32 div;
+	u32 val;
 	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
 
-	regmap_read(clkr->regmap, divider->reg, &div);
-	div >>= divider->shift;
-	div &= BIT(divider->width) - 1;
-	div += 1;
-
-	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
-		if (!hw_parent)
-			return -EINVAL;
-
-		*prate = clk_hw_round_rate(hw_parent, rate * div);
-	}
+	regmap_read(clkr->regmap, divider->reg, &val);
+	val >>= divider->shift;
+	val &= BIT(divider->width) - 1;
 
-	return DIV_ROUND_UP_ULL((u64)*prate, div);
+	return divider_ro_round_rate(hw, rate, prate, NULL, divider->width,
+				     CLK_DIVIDER_ROUND_CLOSEST, val);
 }
 
 static long div_round_rate(struct clk_hw *hw, unsigned long rate,
-- 
2.14.3

[PATCH 5/5] clk: qcom: use divider_ro_round_rate helper

[Jerome Brunet]

On Fri, 2018-01-05 at 18:09 +0100, Jerome Brunet wrote:
> There is now an helper function to round the rate when the
> divider is read-only. Let's use it
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/clk/qcom/clk-regmap-divider.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-regmap-divider.c b/drivers/clk/qcom/clk-regmap-divider.c
> index 4e9b8c2c8980..114e36b97255 100644
> --- a/drivers/clk/qcom/clk-regmap-divider.c
> +++ b/drivers/clk/qcom/clk-regmap-divider.c
> @@ -28,22 +28,15 @@ static long div_round_ro_rate(struct clk_hw *hw, unsigned long rate,
>  {
>  	struct clk_regmap_div *divider = to_clk_regmap_div(hw);
>  	struct clk_regmap *clkr = &divider->clkr;
> -	u32 div;
> +	u32 val;
>  	struct clk_hw *hw_parent = clk_hw_get_parent(hw);

forgot to remove this line.

>  
> -	regmap_read(clkr->regmap, divider->reg, &div);
> -	div >>= divider->shift;
> -	div &= BIT(divider->width) - 1;
> -	div += 1;
> -
> -	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> -		if (!hw_parent)
> -			return -EINVAL;
> -
> -		*prate = clk_hw_round_rate(hw_parent, rate * div);
> -	}
> +	regmap_read(clkr->regmap, divider->reg, &val);
> +	val >>= divider->shift;
> +	val &= BIT(divider->width) - 1;
>  
> -	return DIV_ROUND_UP_ULL((u64)*prate, div);
> +	return divider_ro_round_rate(hw, rate, prate, NULL, divider->width,
> +				     CLK_DIVIDER_ROUND_CLOSEST, val);
>  }
>  
>  static long div_round_rate(struct clk_hw *hw, unsigned long rate,