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

[Chuanhong Guo]

On Fri, May 22, 2026 at 11:45 PM Yao Zi <me@xxxxxxxx> wrote:
>
> 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@xxxxxxxx> 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.

I don't feel like arbitrarily splitting a single clock into two and exposing
an internal clock unused by anything outside the PLL block is a good idea.
I'll try reusing the ops and omit the enable/disable call for cmnpll_postdiv.

>
> > 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.

The wrapper needs to be spinlock + frequency reloading trigger,
because the built-in spinlock routine only covers the divider/gate.
writing. I see no point wrapping all the callbacks and writing a way
more complicated probe routine just to reuse a few lines of
calculations + register ops.

>
> > 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.

I just learned that clk_hw_register_composite won't result in
one more level of clock. but the second part still is true.
In order to reuse the mentioned kernel function I have to give
up on my current single for-loop probe function, add a separated
array of clocks with mux/div/gate, mux-only, div-only, gate-only
clocks, and write separated probe routines for them because
the clock composition is different. The current static pointers
in struct sf21_hw_clks will also need to be replaced because
clk_hw_register_composite allocates its own clk_hw instead.

I'd argue that the point of reusing code is to reduce code
duplication and simplify the driver.
In this case I could only see the driver getting way more
complex in order to reuse the routine you mentioned.

-- 
Regards,
Chuanhong Guo