Re: [PATCH v4 3/4] clk: spacemit: Add clock support for Spacemit K1 SoC

Haylen Chu
2025-03-03T09:44:43Z

On Thu, Feb 13, 2025 at 10:04:10PM -0600, Alex Elder wrote:
> On 1/3/25 3:56 PM, Haylen Chu wrote:
> > The clock tree of K1 SoC contains three main types of clock hardware
> > (PLL/DDN/MIX) and is managed by several independent controllers in
> > different SoC parts (APBC, APBS and etc.), thus different compatible
> > strings are added to distinguish them.
> >
> > Some controllers may share IO region with reset controller and other low
> > speed peripherals like watchdog, so all register operations are done
> > through regmap to avoid competition.
> >
> > Signed-off-by: Haylen Chu <heylenay@4d2.org>
>
> This is a really big patch (over 3000 lines), and a fairly large
> amount of code to review. But I've given it a really thorough
> read and I have a *lot* of review comments for you to consider.
>
> First, a few top-level comments.
> - This driver is very comprehensive. It represents essentially
> *all* of the clocks in the tree diagram shown here:
> https://developer.spacemit.com/resource/file/images?fileName=DkWGb4ed7oAziVxE6PIcbjTLnpd.png
> (I can tell you what's missing but I don't think it matters.)
> - In almost all cases, the names of the clocks match the names
> shown in that diagram, which is very helpful.
> - All of the clocks are implemented using "custom" clock
> implementations. I'm fairly certain that almost all of
> them can use standard clock framework types instead
> (fixed-rate, fixed-factor, fractional-divider, mux, and
> composite). But for now I think there are other things
> more important to improve.
> - A great deal of my commentary below is simply saying that the
> code is more complex than necessary. Some simple (though
> widespread) refactoring would improve things a lot. And
> some of the definitions can be done without having to
> specify nearly so many values.
> - Much of what might be considered generality in the
> implementation actually isn't needed, because it isn't used.
> This is especially true given that there are essentially no
> clocks left unspecified for the K1 SoC.
> - Once the refactoring I suggest has been done, I expect
> that more opportunities for simplification and cleanup will
> become obvious; we'll see.
> - I suggest these changes because the resulting simplicity
> will make the code much more understandable and maintainable
> in the long term. And if it's simpler to understand, it
> should be easier for a maintainer to accept.
>
> I'm not going to comment on the things related to Device Tree
> that have already been mentioned, nor on the Makefile or Kconfig,
> etc. I'm focusing just on the code.
>
> > ---
> > drivers/clk/Kconfig | 1 +
> > drivers/clk/Makefile | 1 +
> > drivers/clk/spacemit/Kconfig | 20 +
> > drivers/clk/spacemit/Makefile | 5 +
> > drivers/clk/spacemit/ccu-k1.c | 1747 +++++++++++++++++++++++++++++
> > drivers/clk/spacemit/ccu_common.h | 51 +
> > drivers/clk/spacemit/ccu_ddn.c | 140 +++
> > drivers/clk/spacemit/ccu_ddn.h | 84 ++
> > drivers/clk/spacemit/ccu_mix.c | 304 +++++
> > drivers/clk/spacemit/ccu_mix.h | 309 +++++
> > drivers/clk/spacemit/ccu_pll.c | 189 ++++
> > drivers/clk/spacemit/ccu_pll.h | 80 ++
> > 12 files changed, 2931 insertions(+)
> > create mode 100644 drivers/clk/spacemit/Kconfig
> > create mode 100644 drivers/clk/spacemit/Makefile
> > create mode 100644 drivers/clk/spacemit/ccu-k1.c
> > create mode 100644 drivers/clk/spacemit/ccu_common.h
> > create mode 100644 drivers/clk/spacemit/ccu_ddn.c
> > create mode 100644 drivers/clk/spacemit/ccu_ddn.h
> > create mode 100644 drivers/clk/spacemit/ccu_mix.c
> > create mode 100644 drivers/clk/spacemit/ccu_mix.h
> > create mode 100644 drivers/clk/spacemit/ccu_pll.c
> > create mode 100644 drivers/clk/spacemit/ccu_pll.h
> >

...

> > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> > new file mode 100644
> > index 000000000000..6fb0a12ec261
> > --- /dev/null
> > +++ b/drivers/clk/spacemit/ccu-k1.c

...

> The next set of clocks differ from essentially all others, in that
> they don't encode their frequency in the name. I.e., I would expect
> the first one to be named pll1_d2_1228p8.

I found this change may not be possible: with the frequency appended,
their names conflict with another set of MPMU gates.

>
> > +static CCU_GATE_FACTOR_DEFINE(pll1_d2, "pll1_d2", CCU_PARENT_HW(pll1),
> > + APB_SPARE2_REG,
> > + BIT(1), BIT(1), 0, 2, 1, 0);
> > +static CCU_GATE_FACTOR_DEFINE(pll1_d3, "pll1_d3", CCU_PARENT_HW(pll1),
> > + APB_SPARE2_REG,
> > + BIT(2), BIT(2), 0, 3, 1, 0);
> > +static CCU_GATE_FACTOR_DEFINE(pll1_d4, "pll1_d4", CCU_PARENT_HW(pll1),
> > + APB_SPARE2_REG,
> > + BIT(3), BIT(3), 0, 4, 1, 0);
> > +static CCU_GATE_FACTOR_DEFINE(pll1_d5, "pll1_d5", CCU_PARENT_HW(pll1),
> > + APB_SPARE2_REG,
> > + BIT(4), BIT(4), 0, 5, 1, 0);
> > +static CCU_GATE_FACTOR_DEFINE(pll1_d6, "pll1_d6", CCU_PARENT_HW(pll1),
> > + APB_SPARE2_REG,
> > + BIT(5), BIT(5), 0, 6, 1, 0);
> > +static CCU_GATE_FACTOR_DEFINE(pll1_d7, "pll1_d7", CCU_PARENT_HW(pll1),
> > + APB_SPARE2_REG,
> > + BIT(6), BIT(6), 0, 7, 1, 0);
> > +static CCU_GATE_FACTOR_DEFINE(pll1_d8, "pll1_d8", CCU_PARENT_HW(pll1),
> > + APB_SPARE2_REG,
> > + BIT(7), BIT(7), 0, 8, 1, 0);
> > +

...

> > +/* MPMU clocks start */

...

> > +static CCU_GATE_DEFINE(pll1_d3_819p2, "pll1_d3_819p2", CCU_PARENT_HW(pll1_d3),
> > + MPMU_ACGR,
> > + BIT(14), BIT(14), 0, 0);
> > +
> > +static CCU_GATE_DEFINE(pll1_d2_1228p8, "pll1_d2_1228p8", CCU_PARENT_HW(pll1_d2),
> > + MPMU_ACGR,
> > + BIT(16), BIT(16), 0, 0);

Here're the conflicts.

Although they don't happen on all the clocks, I prefer to keep the clock
names as is for now to keep the consistency.

Thanks,
Haylen Chu