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

[Alex Elder]

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

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