Re: [PATCH 4/4] clk: add support for siflower sf21-topcrm

[Yao Zi <me@ziyao.cc>]

On Mon, May 18, 2026 at 09:34:17PM +0800, Chuanhong Guo wrote:
> Hi!
> 
> On Mon, May 18, 2026 at 8:22 PM Yao Zi <me@ziyao.cc> wrote:
> >
> > On Sun, May 17, 2026 at 10:12:58PM +0800, Chuanhong Guo wrote:

...

> >
> > [...]
> >
> > Besides field/register offsets, the only difference I could tell between
> > cmnpll_postdiv and pciepll_fout is that pciepll_fout clocks could be
> > gated.
> >
> > Would it be a good idea to describe the gating function separately as a
> > clock, and merge the common part of pciepll_fout and cmnpll_postdiv? In
> > which way you could save a lot of mostly duplicated code.
> 
> I don't think so. Since the majority of the existing code are register
> operations, and the register bit layout are completely different,
> there isn't much to share between these two PLLs.
> Writing a struct to describe both register layouts together seems
> unnecessarily complicated and harder to read.

I'm not suggesting merging the implementation of the two PLLs
themselves, but only the two types of clocks derived from them,
i.e. cmnpll_postdiv and pciepll_fout.

> BTW the CMNPLL and PCIEPLL are actually different hardware.
> The former is an integer PLL while the latter actually supports
> fractional operations. I didn't add support for the fractional part
> due to the lack of use cases and documentation. PCIE and GMAC
> clocks are fed by this PLL and require exact clock frequencies
> which can be achieved using only the integer mode.
> 
> >
> > > +     .recalc_rate = sf21_pciepll_fout_recalc_rate,
> > > +     .determine_rate = sf21_pciepll_fout_determine_rate,
> > > +     .set_rate = sf21_pciepll_fout_set_rate,
> > > +};
> > [...]
> > > +
> > > +     spin_lock_irqsave(cmn_priv->lock, flags);
> > > +     if (index)
> > > +             sf_rmw(cmn_priv, mux_reg, 0, BIT(mux_offs));
> > > +     else
> > > +             sf_rmw(cmn_priv, mux_reg, BIT(mux_offs), 0);
> > > +
> > > +     spin_unlock_irqrestore(cmn_priv->lock, flags);
> > > +     return 0;
> > > +}
> >
> > I believe besides the divider reloading part, clk_mux_ops,
> > clk_divider_ops, and clk_gate_ops have already provided the logic
> > you implemented here. So it might be a better option to composite them
> > together to implement your clocks instead of building from scratch.
> 
> The divider reloading is the exact reason I chose to not compose them
> with the function you mentioned. The reloading bit need to be set on
> both clock divider change and clock enabling, because the clock divider
> loading only happens when the clock is running. Since I already have
> to write two of the three parts you mentioned, trying to reuse
> clk_mux_ops doesn't seem to reduce the code complexity here.

You don't have to write divider/gate operations from scratch, but only
wrappers that first call clk_{divider,gate}_ops.* then trigger frequency
reloading.

> It's just trading get_parent and set_parent call with one more level
> in the clock tree for every clock and more code to wire them together
> in the probe function.
> 
> >
> > > +static const struct clk_ops sf21_clk_muxdiv_ops = {
> > > +     .enable = sf21_muxdiv_enable,
> > > +     .disable = sf21_muxdiv_disable,
> > > +     .is_enabled = sf21_muxdiv_is_enabled,
> > > +     .recalc_rate = sf21_muxdiv_recalc_rate,
> > > +     .determine_rate = sf21_muxdiv_determine_rate,
> > > +     .set_rate = sf21_muxdiv_set_rate,
> > > +     .get_parent = sf21_muxdiv_get_parent,
> > > +     .set_parent = sf21_muxdiv_set_parent,
> > > +};

Regards,
Yao Zi