| Message ID | 20221018-clk-range-checks-fixes-v4-39-971d5077e7d2@cerno.tech (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: Make determine_rate mandatory for muxes | expand |
On Fri, May 5, 2023 at 1:27 PM Maxime Ripard <maxime@cerno.tech> wrote: > The Versatile sp810 "timerclken" clock implements a mux with a > set_parent hook, but doesn't provide a determine_rate implementation. > > This is a bit odd, since set_parent() is there to, as its name implies, > change the parent of a clock. However, the most likely candidates to > trigger that parent change are either the assigned-clock-parents device > tree property or a call to clk_set_rate(), with determine_rate() > figuring out which parent is the best suited for a given rate. > > This mismatch is probably due to the fact that the driver introduction > predates the determine_rate introduction, and it was never revised since > then. > > The default, implicit, behaviour that has been in use so far has thus > been to simply keep using the current parent in all cases. This is also > the behaviour of the new clk_hw_determine_rate_no_reparent() helper, so > we can simply use it to make our expectation explicit. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Acked-by: Linus Walleij <linus.walleij@linaro.org> I think Pawel's reply reads as an ACK as well? Yours, Linus Walleij
On 05/05/2023 12:30, Linus Walleij wrote: > On Fri, May 5, 2023 at 1:27 PM Maxime Ripard <maxime@cerno.tech> wrote: > >> The Versatile sp810 "timerclken" clock implements a mux with a >> set_parent hook, but doesn't provide a determine_rate implementation. >> >> This is a bit odd, since set_parent() is there to, as its name implies, >> change the parent of a clock. However, the most likely candidates to >> trigger that parent change are either the assigned-clock-parents device >> tree property or a call to clk_set_rate(), with determine_rate() >> figuring out which parent is the best suited for a given rate. >> >> This mismatch is probably due to the fact that the driver introduction >> predates the determine_rate introduction, and it was never revised since >> then. >> >> The default, implicit, behaviour that has been in use so far has thus >> been to simply keep using the current parent in all cases. This is also >> the behaviour of the new clk_hw_determine_rate_no_reparent() helper, so >> we can simply use it to make our expectation explicit. >> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: Pawel Moll <pawel.moll@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > I think Pawel's reply reads as an ACK as well? Indeed, for what it's worth (not much ;-) Acked-by: Pawel Moll <pawel.moll@arm.com> Cheers! Paweł
diff --git a/drivers/clk/versatile/clk-sp810.c b/drivers/clk/versatile/clk-sp810.c index caf0cd2fb5b6..45adac1b4630 100644 --- a/drivers/clk/versatile/clk-sp810.c +++ b/drivers/clk/versatile/clk-sp810.c @@ -63,6 +63,7 @@ static int clk_sp810_timerclken_set_parent(struct clk_hw *hw, u8 index) } static const struct clk_ops clk_sp810_timerclken_ops = { + .determine_rate = clk_hw_determine_rate_no_reparent, .get_parent = clk_sp810_timerclken_get_parent, .set_parent = clk_sp810_timerclken_set_parent, };
The Versatile sp810 "timerclken" clock implements a mux with a set_parent hook, but doesn't provide a determine_rate implementation. This is a bit odd, since set_parent() is there to, as its name implies, change the parent of a clock. However, the most likely candidates to trigger that parent change are either the assigned-clock-parents device tree property or a call to clk_set_rate(), with determine_rate() figuring out which parent is the best suited for a given rate. This mismatch is probably due to the fact that the driver introduction predates the determine_rate introduction, and it was never revised since then. The default, implicit, behaviour that has been in use so far has thus been to simply keep using the current parent in all cases. This is also the behaviour of the new clk_hw_determine_rate_no_reparent() helper, so we can simply use it to make our expectation explicit. Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/versatile/clk-sp810.c | 1 + 1 file changed, 1 insertion(+)