| Message ID | 20221018-clk-range-checks-fixes-v3-28-9a1358472d52@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: Make determine_rate mandatory for muxes | expand |
CC Gareth, Hervé, Miquel, Ralph On Tue, Apr 4, 2023 at 2:44 PM Maxime Ripard <maxime@cerno.tech> wrote: > The Renesas r9a06g032 bitselect 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 candidate to > trigger that parent change is a call to clk_set_rate(), with > determine_rate() figuring out which parent is the best suited for a > given rate. > > The other trigger would be a call to clk_set_parent(), but it's far less > used, and it doesn't look like there's any obvious user for that clock. > > So, the set_parent hook is effectively unused, possibly because of an > oversight. However, it could also be an explicit decision by the > original author to avoid any reparenting but through an explicit call to > clk_set_parent(). > > The latter case would be equivalent to setting the flag > CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook > to __clk_mux_determine_rate(). Indeed, if no determine_rate > implementation is provided, clk_round_rate() (through > clk_core_round_rate_nolock()) will call itself on the parent if > CLK_SET_RATE_PARENT is set, and will not change the clock rate > otherwise. __clk_mux_determine_rate() has the exact same behavior when > CLK_SET_RATE_NO_REPARENT is set. > > And if it was an oversight, then we are at least explicit about our > behavior now and it can be further refined down the line. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> LGTM, so Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> But I do not have the hardware. > --- a/drivers/clk/renesas/r9a06g032-clocks.c > +++ b/drivers/clk/renesas/r9a06g032-clocks.c > @@ -1121,6 +1121,7 @@ static int r9a06g032_clk_mux_set_parent(struct clk_hw *hw, u8 index) > } > > static const struct clk_ops clk_bitselect_ops = { > + .determine_rate = __clk_mux_determine_rate, > .get_parent = r9a06g032_clk_mux_get_parent, > .set_parent = r9a06g032_clk_mux_set_parent, > }; > @@ -1145,7 +1146,7 @@ r9a06g032_register_bitsel(struct r9a06g032_priv *clocks, > > init.name = desc->name; > init.ops = &clk_bitselect_ops; > - init.flags = CLK_SET_RATE_PARENT; > + init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT; > init.parent_names = names; > init.num_parents = 2; > Gr{oetje,eeting}s, Geert
Hi Geert & Maxime, geert@linux-m68k.org wrote on Tue, 11 Apr 2023 12:27:38 +0200: > CC Gareth, Hervé, Miquel, Ralph > > On Tue, Apr 4, 2023 at 2:44 PM Maxime Ripard <maxime@cerno.tech> wrote: > > The Renesas r9a06g032 bitselect 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 candidate to > > trigger that parent change is a call to clk_set_rate(), with > > determine_rate() figuring out which parent is the best suited for a > > given rate. > > > > The other trigger would be a call to clk_set_parent(), but it's far less > > used, and it doesn't look like there's any obvious user for that clock. > > > > So, the set_parent hook is effectively unused, possibly because of an > > oversight. However, it could also be an explicit decision by the > > original author to avoid any reparenting but through an explicit call to > > clk_set_parent(). > > > > The latter case would be equivalent to setting the flag > > CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook > > to __clk_mux_determine_rate(). Indeed, if no determine_rate > > implementation is provided, clk_round_rate() (through > > clk_core_round_rate_nolock()) will call itself on the parent if > > CLK_SET_RATE_PARENT is set, and will not change the clock rate > > otherwise. __clk_mux_determine_rate() has the exact same behavior when > > CLK_SET_RATE_NO_REPARENT is set. > > > > And if it was an oversight, then we are at least explicit about our > > behavior now and it can be further refined down the line. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > LGTM, so > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> I searched for 'possible callers', I didn't find any places where this would be used on the consumer side. However, downstream, there is a rzn1-clock-bitselect.c clock driver which states: + * This clock provider handles the case of the RZN1 where you have peripherals + * that have two potential clock source and two gates, one for each of the + * clock source - the used clock source (for all sub clocks) is selected by a + * single bit. + * That single bit affects all sub-clocks, and therefore needs to change the + * active gate (and turn the others off) and force a recalculation of the rates. I don't know how much of this file has been upstreamed (under a different form) but this might very well be related to the fact that reparenting in some cases would be a major issue and thus needs to be avoided unless done on purpose (guessing?). Maybe Ralph can comment, but for what I understand, Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> > But I do not have the hardware. > > > --- a/drivers/clk/renesas/r9a06g032-clocks.c > > +++ b/drivers/clk/renesas/r9a06g032-clocks.c > > @@ -1121,6 +1121,7 @@ static int r9a06g032_clk_mux_set_parent(struct clk_hw *hw, u8 index) > > } > > > > static const struct clk_ops clk_bitselect_ops = { > > + .determine_rate = __clk_mux_determine_rate, > > .get_parent = r9a06g032_clk_mux_get_parent, > > .set_parent = r9a06g032_clk_mux_set_parent, > > }; > > @@ -1145,7 +1146,7 @@ r9a06g032_register_bitsel(struct r9a06g032_priv *clocks, > > > > init.name = desc->name; > > init.ops = &clk_bitselect_ops; > > - init.flags = CLK_SET_RATE_PARENT; > > + init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT; > > init.parent_names = names; > > init.num_parents = 2; > > > > Gr{oetje,eeting}s, > > Geert > Thanks, Miquèl
diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c index 40828616f723..56108b37f94e 100644 --- a/drivers/clk/renesas/r9a06g032-clocks.c +++ b/drivers/clk/renesas/r9a06g032-clocks.c @@ -1121,6 +1121,7 @@ static int r9a06g032_clk_mux_set_parent(struct clk_hw *hw, u8 index) } static const struct clk_ops clk_bitselect_ops = { + .determine_rate = __clk_mux_determine_rate, .get_parent = r9a06g032_clk_mux_get_parent, .set_parent = r9a06g032_clk_mux_set_parent, }; @@ -1145,7 +1146,7 @@ r9a06g032_register_bitsel(struct r9a06g032_priv *clocks, init.name = desc->name; init.ops = &clk_bitselect_ops; - init.flags = CLK_SET_RATE_PARENT; + init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT; init.parent_names = names; init.num_parents = 2;
The Renesas r9a06g032 bitselect 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 candidate to trigger that parent change is a call to clk_set_rate(), with determine_rate() figuring out which parent is the best suited for a given rate. The other trigger would be a call to clk_set_parent(), but it's far less used, and it doesn't look like there's any obvious user for that clock. So, the set_parent hook is effectively unused, possibly because of an oversight. However, it could also be an explicit decision by the original author to avoid any reparenting but through an explicit call to clk_set_parent(). The latter case would be equivalent to setting the flag CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook to __clk_mux_determine_rate(). Indeed, if no determine_rate implementation is provided, clk_round_rate() (through clk_core_round_rate_nolock()) will call itself on the parent if CLK_SET_RATE_PARENT is set, and will not change the clock rate otherwise. __clk_mux_determine_rate() has the exact same behavior when CLK_SET_RATE_NO_REPARENT is set. And if it was an oversight, then we are at least explicit about our behavior now and it can be further refined down the line. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/renesas/r9a06g032-clocks.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)