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

[Chen Wang]

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 at gmail.com>
> Signed-off-by: Troy Mitchell <TroyMitchell988 at gmail.com>
> Signed-off-by: Xukai Wang <kingxukai at 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.


> +	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?

> +
> +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?

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?
> +
> +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.

[......]


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?

[......]

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

[......]