[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aUUGeGnYR+joVR8c@duge-virtual-machine>
Date: Fri, 19 Dec 2025 16:02:00 +0800
From: Jiayu Du <jiayu.riscv@...c.iscas.ac.cn>
To: Xukai Wang <kingxukai@...omail.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 v9 2/3] clk: canaan: Add clock driver for Canaan K230
On Thu, Nov 27, 2025 at 08:45:13PM +0800, Xukai Wang wrote:
> This patch provides basic support for the K230 clock, which covers
> all clocks in K230 SoC.
>
> The clock tree of the K230 SoC consists of a 24MHZ external crystal
> oscillator, PLLs and an external pulse input for timerX, and their
> derived clocks.
>
> 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 | 2443 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 2450 insertions(+)
...
> diff --git a/drivers/clk/clk-k230.c b/drivers/clk/clk-k230.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..8750e9cbac04f30e31d8f2eb395c9b49027ca278
> --- /dev/null
> +++ b/drivers/clk/clk-k230.c
> @@ -0,0 +1,2443 @@
...
> +
> +K230_CLK_GATE_FORMAT(cpu0_src_gate,
> + K230_CPU0_SRC_GATE,
> + 0, 0, 0, 0,
> + &pll0_div2.hw);
Core-related clocks of cpu0/cpu1 (src/plic/apb/noc_ddrcp4, etc.)
lack protection flags, which risks accidental disabling.
Recommend to replace the flag bits for all CPU0/CPU1 core clock
nodes with `CLK_IS_CRITICAL`,like this:
`0, 0, 0, 0,` -> `0, 0, CLK_IS_CRITICAL, 0,`
> +
> +K230_CLK_RATE_FORMAT(cpu0_src_rate,
> + K230_CPU0_SRC_RATE,
> + 1, 16, 1, 0xF,
> + 16, 16, 0, 0x0,
> + 0x0, 31, mul, 0x0,
> + false, 0,
> + &cpu0_src_gate.clk.hw);
> +
same as above,`false, 0,` ->`false, CLK_IS_CRITICAL,`
> +K230_CLK_RATE_FORMAT(cpu0_axi_rate,
> + K230_CPU0_AXI_RATE,
> + 1, 1, 0, 0,
> + 1, 8, 6, 0x7,
> + 0x0, 31, div, 0x0,
> + 0, 0,
> + &cpu0_src_rate.clk.hw);
> +
same as above,`0, 0,` ->`0, CLK_IS_CRITICAL,`
> +K230_CLK_GATE_FORMAT(cpu0_plic_gate,
> + K230_CPU0_PLIC_GATE,
> + 0x0, 9, 0, 0,
> + &cpu0_src_rate.clk.hw);
> +
same as above,`0x0, 9, 0, 0,` -> `0x0, 9, CLK_IS_CRITICAL, 0,`
> +K230_CLK_RATE_FORMAT(cpu0_plic_rate,
> + K230_CPU0_PLIC_RATE,
> + 1, 1, 0, 0,
> + 1, 8, 10, 0x7,
> + 0x0, 31, div, 0x0,
> + false, 0,
> + &cpu0_plic_gate.clk.hw);
> +
same as above,`false, 0,` -> `false, CLK_IS_CRITICAL,`
> +K230_CLK_GATE_FORMAT(cpu0_noc_ddrcp4_gate,
> + K230_CPU0_NOC_DDRCP4_GATE,
> + 0x60, 7, 0, 0,
> + &cpu0_src_rate.clk.hw);
> +
same as above,`0x60, 7, 0, 0,` -> `0x60, 7, CLK_IS_CRITICAL, 0,`
> +K230_CLK_GATE_FORMAT(cpu0_apb_gate,
> + K230_CPU0_APB_GATE,
> + 0x0, 13, 0, 0,
> + &pll0_div4.hw);
> +
same as above,`0x0, 13, 0, 0,` -> `0x0, 13, CLK_IS_CRITICAL, 0,`
> +K230_CLK_RATE_FORMAT(cpu0_apb_rate,
> + K230_CPU0_APB_RATE,
> + 1, 1, 0, 0,
> + 1, 8, 15, 0x7,
> + 0x0, 31, div, 0x0,
> + false, 0,
> + &cpu0_apb_gate.clk.hw);
> +
same as above,`false, 0,` -> `false, CLK_IS_CRITICAL,`
> +static const struct clk_parent_data k230_cpu1_src_mux_pdata[] = {
> + { .hw = &pll0_div2.hw, },
> + { .hw = &pll3.hw, },
> + { .hw = &pll0.hw, },
> +};
> +
> +K230_CLK_MUX_FORMAT(cpu1_src_mux,
> + K230_CPU1_SRC_MUX,
> + 0x4, 1, 0x3,
> + 0, 0,
> + k230_cpu1_src_mux_pdata);
> +
same as above,`0, 0,` -> `CLK_IS_CRITICAL, 0,`
> +K230_CLK_GATE_FORMAT(cpu1_src_gate,
> + K230_CPU1_SRC_GATE,
> + 0x4, 0, CLK_IGNORE_UNUSED, 0,
> + &cpu1_src_mux.clk.hw);
> +
same as above,`0x4, 0, CLK_IGNORE_UNUSED, 0,` -> `0x4, 0, CLK_IS_CRITICAL, 0,`
> +K230_CLK_RATE_FORMAT(cpu1_src_rate,
> + K230_CPU1_SRC_GATE,
> + 1, 1, 0, 0,
> + 1, 8, 3, 0x7,
> + 0x4, 31, div, 0x0,
> + false, 0,
> + &cpu1_src_gate.clk.hw);
> +
same as above,`false, 0,` -> `false, CLK_IS_CRITICAL,`
> +K230_CLK_RATE_FORMAT(cpu1_axi_rate,
> + K230_CPU1_AXI_RATE,
> + 1, 1, 0, 0,
> + 1, 8, 12, 0x7,
> + 0x4, 31, div, 0x0,
> + false, 0,
> + &cpu1_src_rate.clk.hw);
> +
same as above,`false, 0,` -> `false, CLK_IS_CRITICAL,`
> +K230_CLK_GATE_FORMAT(cpu1_plic_gate,
> + K230_CPU1_PLIC_GATE,
> + 0x4, 15, CLK_IGNORE_UNUSED, 0,
> + &cpu1_src_rate.clk.hw);
> +
same as above,`0x4, 15, CLK_IGNORE_UNUSED, 0,` -> `0x4, 15, CLK_IS_CRITICAL, 0,`
> +K230_CLK_RATE_FORMAT(cpu1_plic_rate,
> + K230_CPU1_PLIC_RATE,
> + 1, 1, 0, 0,
> + 1, 8, 16, 0x7,
> + 0x4, 31, div, 0x0,
> + false, 0,
> + &cpu1_plic_gate.clk.hw);
> +
same as above,`false, 0,` -> `false, CLK_IS_CRITICAL,`
> +K230_CLK_GATE_FORMAT(cpu1_apb_gate,
> + K230_CPU1_APB_GATE,
> + 0x4, 19, 0, 0,
> + &pll0_div4.hw);
> +
same as above,`0x4, 19, 0, 0,` -> `0x4, 19, CLK_IS_CRITICAL, 0,`
> +K230_CLK_RATE_FORMAT(cpu1_apb_rate,
> + K230_CPU1_APB_RATE,
> + 1, 1, 0, 0,
> + 1, 8, 15, 0x7,
> + 0x0, 31, div, 0x0,
> + false, 0,
> + &cpu1_apb_gate.clk.hw);
> +
same as above,`false, 0,` -> `false, CLK_IS_CRITICAL,`
> +K230_CLK_GATE_FORMAT_PNAME(pmu_apb_gate,
> + K230_PMU_APB_GATE,
...
> +K230_CLK_GATE_FORMAT(hs_hclk_src_gate,
> + K230_HS_HCLK_SRC_GATE,
> + 0x18, 1, 0, 0,
> + &hs_hclk_high_src_rate.clk.hw);
> +
Incorrect register bit setting (bit1) and wrong parent
clock reference (hs_hclk_high_src_rate) for hs_hclk_src_gate,
which does not comply with K230 hardware specifications.
Here is correcting advice:
Register bit correction: `0x18, 1, 0, 0,` -> `0x18, 0, 0, 0,`
Parent clock correction: `&hs_hclk_high_src_rate.clk.hw` ->
`&hs_hclk_high_gate.clk.hw`
> +K230_CLK_RATE_FORMAT(hs_hclk_src_rate,
> + K230_HS_HCLK_SRC_RATE,
...
> +K230_CLK_RATE_FORMAT(hs_sd_card_src_rate,
> + K230_HS_SD_CARD_SRC_RATE,
> + 1, 1, 0, 0,
> + 2, 8, 12, 0x7,
> + 0x1C, 31, div, 0x0,
> + false, 0,
> + &pll0_div4.hw);
> +
The parent clock of hs_sd_card_src_rate is incorrectly pointed
to pll0_div4.
Here is correcting advice:
`&pll0_div4.hw` → `&hs_sd_card_src_gate.clk.hw`
> +K230_CLK_GATE_FORMAT(hs_sd0_card_gate,
> + K230_HS_SD0_CARD_GATE,
> + 0x18, 15, 0, 0,
...
> + },
> + .probe = k230_clk_probe,
> +};
> +builtin_platform_driver(k230_clk_driver);
>
> --
> 2.34.1
>
Powered by blists - more mailing lists