| Message ID | 20180105170959.17266-3-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
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 -- 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
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
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
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
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); }
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(+)