Re: [PATCH v5 2/4] clk: sunxi-ng: div: add read-only operation support

On Sun, Jul 19, 2026 at 9:29 PM Jerome Brunet <[email protected]> wrote:
>
> On dim. 19 juil. 2026 at 18:59, Chen-Yu Tsai <[email protected]> wrote:
>
> >
> >> > +
> >> > +     } else {
> >>
> >> [ ... ]
> >>
> >> > @@ -143,3 +157,15 @@ const struct clk_ops ccu_div_ops = {
> >> >       .set_rate       = ccu_div_set_rate,
> >> >  };
> >> >  EXPORT_SYMBOL_NS_GPL(ccu_div_ops, "SUNXI_CCU");
> >> > +
> >> > +const struct clk_ops ccu_rodiv_ops = {
> >> > +     .disable        = ccu_div_disable,
> >> > +     .enable         = ccu_div_enable,
> >> > +     .is_enabled     = ccu_div_is_enabled,
> >> > +
> >> > +     .get_parent     = ccu_div_get_parent,
> >> > +
> >> > +     .determine_rate = ccu_div_determine_rate,
> >> > +     .recalc_rate    = ccu_div_recalc_rate,
> >> > +};
> >>
> >> [Severity: High]
> >> Could the missing .set_parent callback in ccu_rodiv_ops lead to a silent
> >> desynchronization between the software clock tree and the hardware?
> >>
> >> The struct provides reparenting capabilities via .get_parent and
> >> .determine_rate, but omits .set_parent. If a rate request causes
> >> ccu_mux_helper_determine_rate() to evaluate and select a new parent, the
> >> Common Clock Framework will apply this new parent in software but skip
> >> updating the hardware since .set_parent is NULL.
> >
> > This is an issue. I'm not sure if what Sashiko says actually happens.
>
> I think it could. I've defenitely made a mistake here.
>
> > But 1. this is "read-only divider", not "read-only mux & divider",
>
> The correct way to choose between the 2 is CLK_SET_RATE_NO_REPARENT I think.

For standard muxes, just dropping the .set_parent and .determine_rate
callbacks works. The core will just pass the determine_rate request
to the current parent. In fact that is what the basic clk-mux does.
However we have to deal with the pre-dividers, so we need the
.determine_rate callback.

On the other hand CLK_SET_RATE_NO_REPARENT just says that the rate change
cannot reparent the clock. And it requires the ops to actually check for
it. (In the past we didn't always check ...). So we should still drop the
.set_parent op for clarity.

But I think the main thing is that the naming and your intention in
this patch is that the divider is read-only, not the mux.

> > so the .set_parent callback should be provided. And 2. this is using
> > the ccu_mux_determine_rate_helper, so it's possible a clk_set_rate()
> > call is going to cause a reparent.
> >
> > I can add it while applying if there are no other issues.
>
> As you prefer, I don't mind sending another version in a few days
> (giving some review time to patch #1)

That also works if you want to do it. It does help with preserving
history.


Thanks
ChenYu