Hi Geert & Maxime, geert@xxxxxxxxxxxxxx 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@xxxxxxxxxx> 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@xxxxxxxxxx> > > LGTM, so > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> 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@xxxxxxxxxxx> > 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