Re: [PATCH 3/5] clk: sunxi-ng: mp: support clocks with just a shift register

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 3 Sep 2025 12:20:55 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

> On Wed, Sep 3, 2025 at 8:09 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > The "mp" clock models a mod clock with divider and a shift field. At
> > least one clock in the Allwinner A523 features just a power-of-2 divider
> > field, so support an initialisation of the clock without providing an
> > actual divider field.
> >
> > Add a check whether the "width" field is 0, and skip the divider
> > handling in this case, as the GENMASK macro will not work with a zero
> > length.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---  
> 
> In my series I have a patch that adds this to the divider clocks,
> thus adding a P-clock type to the M-clock bits.

Yeah, I saw that, but wasn't convinced this would be better. Hence wanted
to post my version as an alternative.

> Maybe use that instead? I prefer we use actual matching types instead
> of disabling one part of a complex clock type.

Good that you bring up that topic: when looking for matching clocks I saw
we have a lot of them, though often one is just a subset of some others,
with some code duplication. And we use the pattern of "use type A, but
without feature X" already, for instance for "NKMP without the K".

So I was wondering if we should revisit this and clean this up. IIUC those
clocks were all modelled after the H3 and earlier generation, and the
clocks have changed since then. For instance I don't see PLLs with two
multipliers (NK) after the A64 anymore.

So what about we consolidate the various types into just a few distinct
ones, like NKMP for all PLLs, for instance, and provides macros that
disable fields as needed? This could ideally be done under the hood,
leaving the per-SoC drivers mostly alone, hopefully.

What do people think about that?

Cheers,
Andre

> >  drivers/clk/sunxi-ng/ccu_mp.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/sunxi-ng/ccu_mp.c b/drivers/clk/sunxi-ng/ccu_mp.c
> > index 354c981943b6f..a03dac294d048 100644
> > --- a/drivers/clk/sunxi-ng/ccu_mp.c
> > +++ b/drivers/clk/sunxi-ng/ccu_mp.c
> > @@ -236,9 +236,11 @@ static int ccu_mp_set_rate(struct clk_hw *hw, unsigned long rate,
> >         spin_lock_irqsave(cmp->common.lock, flags);
> >
> >         reg = readl(cmp->common.base + cmp->common.reg);
> > -       reg &= ~GENMASK(cmp->m.width + cmp->m.shift - 1, cmp->m.shift);
> > +       if (cmp->m.width)
> > +               reg &= ~GENMASK(cmp->m.width + cmp->m.shift - 1, cmp->m.shift);
> >         reg &= ~GENMASK(cmp->p.width + cmp->p.shift - 1, cmp->p.shift);
> > -       reg |= (m - cmp->m.offset) << cmp->m.shift;
> > +       if (cmp->m.width)
> > +               reg |= (m - cmp->m.offset) << cmp->m.shift;
> >         if (shift)
> >                 reg |= ilog2(p) << cmp->p.shift;
> >         else
> > --
> > 2.46.3
> >