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
