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

Message ID 20180105170959.17266-3-jbrunet@baylibre.com
State New
Headers show
Series
  • clk: read-only dividers and rate propagation fixup
Related show

Commit Message

Jerome Brunet Jan. 5, 2018, 5:09 p.m.
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(+)

-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jerome Brunet Jan. 5, 2018, 7:40 p.m. | #1
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


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vladimir Zapolskiy Jan. 6, 2018, 2:04 p.m. | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jerome Brunet Jan. 8, 2018, 9:10 a.m. | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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);
 	}