| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250429131225.eza27be7wm4lmehy@troy-wujie14-pro>
Date: Tue, 29 Apr 2025 21:12:25 +0800
From: Troy Mitchell <troymitchell988@...il.com>
To: Xukai Wang <kingxukai@...omail.com>,
Chen Wang <unicorn_wang@...look.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Conor Dooley <conor@...nel.org>
Cc: linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
Samuel Holland <samuel.holland@...ive.com>,
Troy Mitchell <TroyMitchell988@...il.com>
Subject: Re: [PATCH v6 2/3] clk: canaan: Add clock driver for Canaan K230
On Tue, Apr 22, 2025 at 04:01:35PM +0800, Xukai Wang wrote:
>
> 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@...il.com>
> >> Signed-off-by: Troy Mitchell <TroyMitchell988@...il.com>
> >> Signed-off-by: Xukai Wang <kingxukai@...omail.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.
>
Hi Xukai, feel free to skip replying one by one.
just point out anything you disagree with or find confusing.
- Troy
> >
> >> + 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.
> >
> > [......]
> >
> >
Powered by blists - more mailing lists