Re: [PATCH v6 2/3] clk: canaan: Add clock driver for Canaan K230

[Xukai Wang <kingxukai@zohomail.com>]

On 2025/4/21 18:43, Chen Wang wrote:
> Hi, Xukai, I have some comments below.
>
> In general, my suggestion is that the code can be further optimized,
> especially in terms of readability.
>
>
> On 2025/4/15 22:25, Xukai Wang wrote:
>> This patch provides basic support for the K230 clock, which does not
>> cover all clocks.
>>
>> The clock tree of the K230 SoC consists of OSC24M, PLLs and sysclk.
>>
>> Co-developed-by: Troy Mitchell <TroyMitchell988@gmail.com>
>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>> Signed-off-by: Xukai Wang <kingxukai@zohomail.com>
>> ---
>>   drivers/clk/Kconfig    |    6 +
>>   drivers/clk/Makefile   |    1 +
>>   drivers/clk/clk-k230.c | 1710
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 1717 insertions(+)
>>
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index
>> 299bc678ed1b9fcd9110bb8c5937a1bd1ea60e23..1817b8883af9a3d00ac7af2cb88496274b591001
>> 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -464,6 +464,12 @@ config COMMON_CLK_K210
>>       help
>>         Support for the Canaan Kendryte K210 RISC-V SoC clocks.
>>   +config COMMON_CLK_K230
>> +    bool "Clock driver for the Canaan Kendryte K230 SoC"
>> +    depends on ARCH_CANAAN || COMPILE_TEST
>> +        help
>> +          Support for the Canaan Kendryte K230 RISC-V SoC clocks.
>> +
>>   config COMMON_CLK_SP7021
>>       tristate "Clock driver for Sunplus SP7021 SoC"
>>       depends on SOC_SP7021 || COMPILE_TEST
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index
>> fb8878a5d7d93da6bec487460cdf63f1f764a431..5df50b1e14c701ed38397bfb257db26e8dd278b8
>> 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -51,6 +51,7 @@ obj-$(CONFIG_MACH_ASPEED_G6)        += clk-ast2600.o
>>   obj-$(CONFIG_ARCH_HIGHBANK)        += clk-highbank.o
>>   obj-$(CONFIG_CLK_HSDK)            += clk-hsdk-pll.o
>>   obj-$(CONFIG_COMMON_CLK_K210)        += clk-k210.o
>> +obj-$(CONFIG_COMMON_CLK_K230)        += clk-k230.o
>>   obj-$(CONFIG_LMK04832)            += clk-lmk04832.o
>>   obj-$(CONFIG_COMMON_CLK_LAN966X)    += clk-lan966x.o
>>   obj-$(CONFIG_COMMON_CLK_LOCHNAGAR)    += clk-lochnagar.o
>> diff --git a/drivers/clk/clk-k230.c b/drivers/clk/clk-k230.c
>> new file mode 100644
>> index
>> 0000000000000000000000000000000000000000..84a4a2a293e5f278d21510d73888aee4ff9351df
>> --- /dev/null
>> +++ b/drivers/clk/clk-k230.c
>> @@ -0,0 +1,1710 @@
> [......]
>> +
>> +struct k230_pll {
>> +    enum k230_pll_id id;
>> +    struct k230_sysclk *ksc;
>> +    void __iomem *div, *bypass, *gate, *lock;
>
> No need define these iomem address, just calculate them and use them
> when use them. The clock reading and writing efficiency requirements
> are not that high, so there is no need to waste memory for this.
>
I see, I'll drop these next version.
>
>> +    struct clk_hw hw;
>> +};
>> +
>> +#define to_k230_pll(_hw)    container_of(_hw, struct k230_pll, hw)
>> +
>> +struct k230_pll_cfg {
>> +    u32 reg;
>> +    const char *name;
>> +    struct k230_pll *pll;
>> +};
>
> Can we combine k230_pll and k230_pll_cfg into one to simplfy the code?
OK, and I think the role of k230_*_cfg is maybe some redundant, and I'm
considering remove them.
>
>> +
>> +struct k230_pll_div {
>> +    struct k230_sysclk *ksc;
>> +    struct clk_hw *hw;
>
> I see k230_clk use "struct clk_hw", but here we use "struct clk_hw*", 
> can we unify these?
The clk_hw in k230_pll_div is a pointer returned by
`clk_hw_register_fixed_factor()`, whereas the clk_hw in the PLL and in
regular clocks is the actual entity that gets populated by
`clk_hw_register()`.
>
> Just use "struct clk_hw" and init it as static global var should be
> enough, see drivers/clk/sophgo/clk-cv1800.c for example.
>
>> +};
>> +
>> +struct k230_pll_div_cfg {
>> +    const char *parent_name, *name;
>> +    int div;
>> +    struct k230_pll_div *pll_div;
>> +};
>> +
>> +enum k230_pll_div_id {
>> +    K230_PLL0_DIV2,
>> +    K230_PLL0_DIV3,
>> +    K230_PLL0_DIV4,
>> +    K230_PLL0_DIV16,
>> +    K230_PLL1_DIV2,
>> +    K230_PLL1_DIV3,
>> +    K230_PLL1_DIV4,
>> +    K230_PLL2_DIV2,
>> +    K230_PLL2_DIV3,
>> +    K230_PLL2_DIV4,
>> +    K230_PLL3_DIV2,
>> +    K230_PLL3_DIV3,
>> +    K230_PLL3_DIV4,
>> +    K230_PLL_DIV_NUM
>> +};
>> +
>> +enum k230_clk_div_type {
>> +    K230_MUL,
>> +    K230_DIV,
>> +    K230_MUL_DIV,
>> +};
> Please document what's meaning of MUL, DIV, and both? They are type
> for what?
OK, I'll add one to explain this.
>> +
>> +struct k230_clk {
>> +    int id;
>> +    struct k230_sysclk *ksc;
>> +    struct clk_hw hw;
>> +};
>> +
>> +#define to_k230_clk(_hw)    container_of(_hw, struct k230_clk, hw)
>> +
>> +struct k230_sysclk {
>> +    struct platform_device *pdev;
>> +    void __iomem           *pll_regs, *regs;
>> +    spinlock_t           pll_lock, clk_lock;
>> +    struct k230_pll           *plls;
>> +    struct k230_clk           *clks;
>> +    struct k230_pll_div    *dclks;
>> +};
>> +
>> +struct k230_clk_rate_cfg {
>> +    /* rate reg */
>> +    u32 rate_reg_off;
>> +    void __iomem *rate_reg;
>> +    /* rate info*/
>> +    u32 rate_write_enable_bit;
>> +    enum k230_clk_div_type method;
>> +    /* rate mul */
>> +    u32 rate_mul_min;
>> +    u32 rate_mul_max;
>> +    u32 rate_mul_shift;
>> +    u32 rate_mul_mask;
>> +    /* rate div */
>> +    u32 rate_div_min;
>> +    u32 rate_div_max;
>> +    u32 rate_div_shift;
>> +    u32 rate_div_mask;
>> +};
>> +
>> +struct k230_clk_rate_cfg_c {
>> +    /* rate_c reg */
>> +    u32 rate_reg_off_c;
>> +    void __iomem *rate_reg_c;
>> +
>> +    /* rate_c info */
>> +    u32 rate_write_enable_bit_c;
>> +
>> +    /* rate mul-changable */
>> +    u32 rate_mul_min_c;
>> +    u32 rate_mul_max_c;
>> +    u32 rate_mul_shift_c;
>> +    u32 rate_mul_mask_c;
>> +};
>> +
>
> What's "k230_clk_rate_cfg_c", and what's the difference against
> "k230_clk_gate_cfg". Please document it and clarify this.
>
> It is recommended to add documentation comments to important structure
> types and their members.
>
> Regarding how to document kernel code, see
> https://docs.kernel.org/doc-guide/kernel-doc.html.
OK, I'll try to clarify it next version.
>
> [......]
>
>
> This structure definition looks a bit complicated, with nested
> structure pointers. Can it be simplified, similar to struct
> k210_clk_cfg in drivers/clk/clk-k210.c?
>
> And can we use composite clk here?
I'm considering using clk_composite here.
>
> [......]
>
>> +static struct k230_clk_cfg k230_cpu0_src = {
>> +    .name = "cpu0_src",
>> +    .read_only = false,
>> +    .flags = 0,
>> +    .num_parent = 1,
>> +    .parent[0] = {
>> +        .type = K230_PLL_DIV,
>> +        .pll_div_cfg = &k230_pll_div_cfgs[K230_PLL0_DIV2],
>> +    },
>> +    .rate_cfg = &k230_cpu0_src_rate,
>> +    .rate_cfg_c = NULL,
>> +    .gate_cfg = &k230_cpu0_src_gate,
>> +    .mux_cfg = NULL,
>> +};
>> +
>> +static struct k230_clk_cfg k230_cpu0_aclk = {
>> +    .name = "cpu0_aclk",
>> +    .read_only = false,
>> +    .flags = 0,
>> +    .num_parent = 1,
>> +    .parent[0] = {
>> +        .type = K230_CLK_COMPOSITE,
>> +        .clk_cfg = &k230_cpu0_src,
>> +    },
>> +    .rate_cfg = &k230_cpu0_aclk_rate,
>> +    .rate_cfg_c = NULL,
>> +    .gate_cfg = NULL,
>> +    .mux_cfg = NULL,
>> +};
>> +
>
> Suggest use Macro to simplify the code here, see
> drivers/clk/sophgo/clk-cv1800.c for example.
I see, I' ll replace these init code with macro instead.
>
> [......]
>
>