Re: [PATCH v13 4/5] clk: sophgo: Add SG2042 clock driver

From: Chen Wang
Date: Fri Apr 12 2024 - 06:23:32 EST

My feedback embedded.

On 2024/4/11 12:11, Stephen Boyd wrote:
Quoting Chen Wang (2024-03-28 23:21:40)
From: Chen Wang <unicorn_wang@xxxxxxxxxxx>

Add a driver for the SOPHGO SG2042 clocks.

Signed-off-by: Chen Wang <unicorn_wang@xxxxxxxxxxx>
---
drivers/clk/Kconfig | 1 +
drivers/clk/Makefile | 1 +
drivers/clk/sophgo/Kconfig | 7 +
drivers/clk/sophgo/Makefile | 2 +
drivers/clk/sophgo/clk-sophgo-sg2042.c | 1410 ++++++++++++++++++++++++
drivers/clk/sophgo/clk-sophgo-sg2042.h | 216 ++++
Inline the contents of this file into the driver C file.
ok, now only one c source is needed.
diff --git a/drivers/clk/sophgo/clk-sophgo-sg2042.c b/drivers/clk/sophgo/clk-sophgo-sg2042.c
new file mode 100644
index 000000000000..7b468e7299ae
--- /dev/null
+++ b/drivers/clk/sophgo/clk-sophgo-sg2042.c
@@ -0,0 +1,1410 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Sophgo SG2042 Clock Generator Driver
+ *
+ * Copyright (C) 2024 Sophgo Technology Inc. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/iopoll.h>
+#include <linux/platform_device.h>
Need include for BIT macro, io.h for readl/writel, do_div() header,
ARRAY_SIZE, etc. please check.

ok, will check and add them, but I did not see warnings/errors when I build with W=1

[...]

+static inline int sg2042_pll_enable(struct sg2042_pll_clock *pll, bool en)
+{
+ unsigned int value = 0;
Don't need to initialize value? Also, type should be u32.

Sure, no initializatation is required here, and u32 should be better, I see some other places have the same problem, will fix them too.

[...]
+static unsigned long sg2042_pll_recalc_rate(unsigned int reg_value,
+ unsigned long parent_rate)
+{
+ struct sg2042_pll_ctrl ctrl_table;
+ u64 rate, numerator, denominator;
+
+ sg2042_pll_ctrl_decode(reg_value, &ctrl_table);
+
+ numerator = parent_rate * ctrl_table.fbdiv;
+ denominator = ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
+ do_div(numerator, denominator);
+ rate = numerator;
+
+ return rate;
return numerator

yes, my stupid, will fix it, thanks.

[...]

+
+ static int postdiv1_2[][3] = {
const

Good catch, will fix.

[...]
+static unsigned long sg2042_clk_divider_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct sg2042_divider_clock *divider = to_sg2042_clk_divider(hw);
+ unsigned int val;
+ unsigned long ret_rate;
+
+ if (!(readl(divider->reg) & BIT(3))) {
What is bit 3? Please make a define.

ok, will fix it and I see some other places have the similar problem, will fix also.

[...]

+ }
+ val |= value << divider->shift;
+ val |= 1 << 3;
Make a define for bit 3 and bit 0 please.

Same as the issue upon, will fix it. thanks.

[...]

+
+static const char *const clk_mux_ddr01_p[] = {
+ "clk_div_ddr01_0", "clk_div_ddr01_1"};
+static const char *const clk_mux_ddr23_p[] = {
+ "clk_div_ddr23_0", "clk_div_ddr23_1"};
+static const char *const clk_mux_rp_cpu_normal_p[] = {
+ "clk_div_rp_cpu_normal_0", "clk_div_rp_cpu_normal_1"};
+static const char *const clk_mux_axi_ddr_p[] = {
+ "clk_div_axi_ddr_0", "clk_div_axi_ddr_1"};
+
+static struct sg2042_mux_clock sg2042_mux_clks[] = {
+ SG2042_MUX(MUX_CLK_DDR01, "clk_mux_ddr01", clk_mux_ddr01_p,
Please use struct clk_parent_data or struct clk_hw directly instead of
string names.

Learned and will use clk_parent_data, thanks.

[......]

+ hw = &pll->hw;
+ ret = clk_hw_register(NULL, hw);
Use devm_clk_hw_register() and pass a device.

yes, will use devm_xxx.

[......]
+
+ hw = clk_hw_register_mux_table(NULL,
Pass a device and use devm.

Yes, will use devm_xxx.

[......]

+
+ clk_data->iobase = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
Why can't we use devm_platform_ioremap_resource()?

yes, will change todevm_platform_ioremap_resource

[......]

+
+ num_clks = ARRAY_SIZE(sg2042_pll_clks);
+ if (num_clks == 0) {
+ ret = -EINVAL;
+ goto error_out;
+ }
This is dead code, please remove.

Thanks, you are right, I will remove it together with some other dead code similiar.

+
+/*
+ * Divider clock
+ * @hw: clk_hw for initialization
+ * @id: used to map clk_onecell_data
+ * @reg: used for readl/writel.
+ * **NOTE**: DIV registers are ALL in CLOCK!
+ * @lock: spinlock to protect register access, modification of
+ * frequency can only be served one at the time
+ * @offset_ctrl: offset of divider control registers
+ * @shift: shift of "Clock Divider Factor" in divider control register
+ * @width: width of "Clock Divider Factor" in divider control register
+ * @div_flags: private flags for this clock, not for framework-specific
+ * @initval: In the divider control register, we can configure whether
+ * to use the value of "Clock Divider Factor" or just use
+ * the initial value pre-configured by IC. BIT[3] controls
+ * this and by default (value is 0), means initial value
+ * is used.
+ * **NOTE** that we cannot read the initial value (default
+ * value when poweron) and default value of "Clock Divider
+ * Factor" is zero, which I think is a hardware design flaw
+ * and should be sync-ed with the initial value. So in
+ * software we have to add a configuration item (initval)
+ * to manually configure this value and use it when BIT[3]
+ * is zero.
Do you use the init clk_op for this?

I think this does not apply to init ops, because I want to configure some parameters here, rather than communicate with the physical clocks and do any special initialization work. As I explained in the comments, due to hardware design issues, when the clock use the IC default initval, we cannot read the current initval from the register, so we can only use the initval we configure in code as current factor value.

[......]

+
+/*
+ * Gate clock
+ * @hw: clk_hw for initialization
+ * @id: used to map clk_onecell_data
+ * @offset_enable: offset of gate enable registers
+ * @bit_idx: which bit in the register controls gating of this clock
+ */
+struct sg2042_gate_clock {
+ struct clk_hw hw;
+
+ unsigned int id;
+
+ unsigned long offset_enable;
Usually we use a u32 or a shorter size so that the member width is
unchanged on different CPU architecture.

OK, I find some else similar problem and will fix them altogether.

[......]

+ * Mux clock
Please use kernel doc. See https://docs.kernel.org/doc-guide/kernel-doc.html

Sure，I will learn and try this.

+ * @hw: clk_hw for initialization
+ * @id: used to map clk_onecell_data
+ * @offset_select: offset of mux selection registers
+ * **NOTE**: MUX registers are ALL in CLOCK!
+ * @shift: shift of "Clock Select" in mux selection register
Is "Clock select" actually @offset_select?

No, the "offset_select" is the offset of register, while "Clock Select" is the bit shift for each mux. For SG2042, every mux has two parents and so just one bit is enough for it to select the parent, and one register contains multiple bits, one for each mux.

[......]