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

From: Alex Elder
Date: Mon Mar 03 2025 - 09:11:11 EST

On 3/3/25 3:41 AM, Haylen Chu wrote:
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@xxxxxxx>

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.

OK, that's fine, and perhaps is why it was done this way. Thanks
for checking. I look forward to the next version of the series.

-Alex



+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