riscv: sophgo: add clock support for sg2042

From: Chen Wang <[email protected]>

Add bindings for the clock generator on the SG2042 RISC-V SoC.

Signed-off-by Chen Wang <[email protected]>

---
 .../clock/sophgo/sophgo,sg2042-clkgen.yaml    |  42 +++++
 include/dt-bindings/clock/sophgo-sg2042-clk.h | 169 ++++++++++++++++++
 2 files changed, 211 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/sophgo/sophgo,sg2042-clkgen.yaml
 create mode 100644 include/dt-bindings/clock/sophgo-sg2042-clk.h

Documentation/devicetree/bindings/clock/sophgo/sophgo,sg2042-clkgen.yaml new file +15 -0

new file mode 100644
index 000000000000..6c0d0461e489
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/sophgo/sophgo,sg2042-clkgen.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2042 Clock Generator
+
+maintainers:
+  - Chen Wang <[email protected]>
+
+properties:
+  compatible:
+    const: sophgo,sg2042-clkgen
+

From: Chen Wang <[email protected]>

Add documentation to describe Sophgo System Controller Registers for SG2042.

Signed-off-by Chen Wang <[email protected]>

---
 .../soc/sophgo/sophgo,sg2042-syscon.yaml      | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-syscon.yaml

Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-syscon.yaml new file +20 -0

new file mode 100644
index 000000000000..329d645091b4
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/sophgo/sophgo,sg2042-syscon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo SG2042 SoC system controller
+
+maintainers:
+  - Chen Wang <[email protected]>
+
+description:
+  The Sophgo SG2042 SoC system controller provides register information such
+  as offset, mask and shift to configure some modules, such as clocks, reset
+  signals and pinctrl.
+
+properties:
+  compatible:
+    items:
+      - enum:

From: Chen Wang <[email protected]>

Add a driver for the SOPHGO SG2042 clock generator.

Signed-off-by Chen Wang <[email protected]>

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +49 -0

+static void __init sg2042_clk_init(struct device_node *node)
+{
+	struct sg2042_clk_data *clk_data = NULL;
+	int i, ret = 0;
+	int num_clks = 0;
+
+	num_clks = ARRAY_SIZE(sg2042_pll_clks) +
+		   ARRAY_SIZE(sg2042_div_clks) +
+		   ARRAY_SIZE(sg2042_gate_clks) +
+		   ARRAY_SIZE(sg2042_mux_clks);
+	if (num_clks == 0) {
+		ret = -EINVAL;
+		goto error_out;
+	}
+
+	ret = sg2042_clk_init_clk_data(node, num_clks, &clk_data);
+	if (ret < 0)
+		goto error_out;
+
+	ret = sg2042_clk_register_plls(clk_data, sg2042_pll_clks,
+				ARRAY_SIZE(sg2042_pll_clks));
+	if (ret)
+		goto cleanup;
+
+	ret = sg2042_clk_register_divs(clk_data, sg2042_div_clks,
+				ARRAY_SIZE(sg2042_div_clks));
+	if (ret)
+		goto cleanup;
+
+	ret = sg2042_clk_register_gates(clk_data, sg2042_gate_clks,
+				ARRAY_SIZE(sg2042_gate_clks));
+	if (ret)
+		goto cleanup;
+
+	ret = sg2042_clk_register_muxs(clk_data, sg2042_mux_clks,
+				ARRAY_SIZE(sg2042_mux_clks));
+	if (ret)
+		goto cleanup;
+
+	for (i = 0; i < num_clks; i++)
+		dbg_info("provider [%d]: %s\n", i, clk_hw_get_name(clk_data->onecell_data.hws[i]));
+	ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, &clk_data->onecell_data);
+	if (ret)
+		goto cleanup;
+
+	return;
+
+cleanup:
+	for (i = 0; i < num_clks; i++) {

drivers/clk/sophgo/clk-sophgo-sg2042.h new file +0 -1

new file mode 100644
index 000000000000..ca1c8b9a937a
--- /dev/null

Quoted 5 lines · Chen Wang

From: Chen Wang <[email protected]>

Add a driver for the SOPHGO SG2042 clock generator.

Signed-off-by Chen Wang <[email protected]>

...

Quoted 53 lines

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +53 -0

+static void __init sg2042_clk_init(struct device_node *node)
+{
+	struct sg2042_clk_data *clk_data = NULL;
+	int i, ret = 0;
+	int num_clks = 0;
+
+	num_clks = ARRAY_SIZE(sg2042_pll_clks) +
+		   ARRAY_SIZE(sg2042_div_clks) +
+		   ARRAY_SIZE(sg2042_gate_clks) +
+		   ARRAY_SIZE(sg2042_mux_clks);
+	if (num_clks == 0) {
+		ret = -EINVAL;
+		goto error_out;
+	}
+
+	ret = sg2042_clk_init_clk_data(node, num_clks, &clk_data);
+	if (ret < 0)
+		goto error_out;
+
+	ret = sg2042_clk_register_plls(clk_data, sg2042_pll_clks,
+				ARRAY_SIZE(sg2042_pll_clks));
+	if (ret)
+		goto cleanup;
+
+	ret = sg2042_clk_register_divs(clk_data, sg2042_div_clks,
+				ARRAY_SIZE(sg2042_div_clks));
+	if (ret)
+		goto cleanup;
+
+	ret = sg2042_clk_register_gates(clk_data, sg2042_gate_clks,
+				ARRAY_SIZE(sg2042_gate_clks));
+	if (ret)
+		goto cleanup;
+
+	ret = sg2042_clk_register_muxs(clk_data, sg2042_mux_clks,
+				ARRAY_SIZE(sg2042_mux_clks));
+	if (ret)
+		goto cleanup;
+
+	for (i = 0; i < num_clks; i++)
+		dbg_info("provider [%d]: %s\n", i, clk_hw_get_name(clk_data->onecell_data.hws[i]));
+	ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, &clk_data->onecell_data);
+	if (ret)
+		goto cleanup;
+
+	return;
+
+cleanup:
+	for (i = 0; i < num_clks; i++) {
+		if (clk_data->onecell_data.hws[i] != NULL)
+			clk_hw_unregister(clk_data->onecell_data.hws[i]);
+	}
+	kfree(clk_data);

drivers/clk/sophgo/clk-sophgo-sg2042.c line 1357

Quoted 6 lines

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +6 -0

+
+error_out:
+	pr_err("%s failed error number %d\n", __func__, ret);
+}
+
+CLK_OF_DECLARE(sg2042_clk, "sophgo,sg2042-clkgen", sg2042_clk_init);

No, this should be platform device. It's a child of another device, so you cannot use other way of init ordering.

Quoted 61 lines · Krzysztof Kozlowski

Quoted 5 lines · Chen Wang

From: Chen Wang <[email protected]>

Add a driver for the SOPHGO SG2042 clock generator.

Signed-off-by Chen Wang <[email protected]>

...

Quoted 53 lines

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +53 -0

+static void __init sg2042_clk_init(struct device_node *node)
+{
+	struct sg2042_clk_data *clk_data = NULL;
+	int i, ret = 0;
+	int num_clks = 0;
+
+	num_clks = ARRAY_SIZE(sg2042_pll_clks) +
+		   ARRAY_SIZE(sg2042_div_clks) +
+		   ARRAY_SIZE(sg2042_gate_clks) +
+		   ARRAY_SIZE(sg2042_mux_clks);
+	if (num_clks == 0) {
+		ret = -EINVAL;
+		goto error_out;
+	}
+
+	ret = sg2042_clk_init_clk_data(node, num_clks, &clk_data);
+	if (ret < 0)
+		goto error_out;
+
+	ret = sg2042_clk_register_plls(clk_data, sg2042_pll_clks,
+				ARRAY_SIZE(sg2042_pll_clks));
+	if (ret)
+		goto cleanup;
+
+	ret = sg2042_clk_register_divs(clk_data, sg2042_div_clks,
+				ARRAY_SIZE(sg2042_div_clks));
+	if (ret)
+		goto cleanup;
+
+	ret = sg2042_clk_register_gates(clk_data, sg2042_gate_clks,
+				ARRAY_SIZE(sg2042_gate_clks));
+	if (ret)
+		goto cleanup;
+
+	ret = sg2042_clk_register_muxs(clk_data, sg2042_mux_clks,
+				ARRAY_SIZE(sg2042_mux_clks));
+	if (ret)
+		goto cleanup;
+
+	for (i = 0; i < num_clks; i++)
+		dbg_info("provider [%d]: %s\n", i, clk_hw_get_name(clk_data->onecell_data.hws[i]));
+	ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, &clk_data->onecell_data);
+	if (ret)
+		goto cleanup;
+
+	return;
+
+cleanup:
+	for (i = 0; i < num_clks; i++) {
+		if (clk_data->onecell_data.hws[i] != NULL)
+			clk_hw_unregister(clk_data->onecell_data.hws[i]);
+	}
+	kfree(clk_data);

Quoted 8 lines

drivers/clk/sophgo/clk-sophgo-sg2042.c line 1357

Quoted 6 lines

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +6 -0

+
+error_out:
+	pr_err("%s failed error number %d\n", __func__, ret);
+}
+
+CLK_OF_DECLARE(sg2042_clk, "sophgo,sg2042-clkgen", sg2042_clk_init);

No, this should be platform device. It's a child of another device, so you cannot use other way of init ordering.

hi, Krzysztof,

Thanks for your review.

I don't quite understand your opinion. Do you mean CLK_OF_DECLARE is only used for platform device so it can not be use here? But I think

this driver is still for platform device though I move the clock controller node as a child of the system contoller node. System controller node is just a block of registers which are used to control some other platform devices ,such as clock controller, reset controller and pin controller for this SoC.

And I also see other similar code in kernel, for example:
drivers/clk/clk-k210.c.

And I'm confused by your input "so you cannot use other way of init ordering." Do you mean "so you CAN use other way of init ordering"?

What's the other way of init ordering do you mean?

Quoted 16 lines · Chen Wang

Quoted 11 lines · Krzysztof Kozlowski

Quoted 5 lines · Chen Wang

From: Chen Wang <[email protected]>

Add a driver for the SOPHGO SG2042 clock generator.

Signed-off-by Chen Wang <[email protected]>

...

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +3 -0

+}
+
+CLK_OF_DECLARE(sg2042_clk, "sophgo,sg2042-clkgen", sg2042_clk_init);

Quoted 2 lines

No, this should be platform device. It's a child of another device, so you cannot use other way of init ordering.

hi, Krzysztof,

Quoted 4 lines

Thanks for your review.

I don't quite understand your opinion. Do you mean CLK_OF_DECLARE is only used for platform device so it can not be use here? But I think

No, I meant you mix init ordering: you depend now on syscon earlier initcall than CLK_OF_DECLARE. Do you remember which one is first? If anything changes here, your driver is broken. There is no dependency, no probe deferral.

Quoted 7 lines

this driver is still for platform device though I move the clock controller node as a child of the system contoller node. System controller node is just a block of registers which are used to control some other platform devices ,such as clock controller, reset controller and pin controller for this SoC.

Quoted 5 lines

And I also see other similar code in kernel, for example:
drivers/clk/clk-k210.c.

And I'm confused by your input "so you cannot use other way of init ordering." Do you mean "so you CAN use other way of init ordering"?

No, I meant you cannot. If you want to use syscon, then your driver should be a proper driver. Therefore add a driver.

Quoted 1 line

What's the other way of init ordering do you mean?

The one coming not from initcalls but driver model.

Best regards,
Krzysztof

Quoted 17 lines · Krzysztof Kozlowski

Quoted 16 lines · Chen Wang

Quoted 11 lines · Krzysztof Kozlowski

Quoted 5 lines · Chen Wang

From: Chen Wang <[email protected]>

Add a driver for the SOPHGO SG2042 clock generator.

Signed-off-by Chen Wang <[email protected]>

...

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +3 -0

+}
+
+CLK_OF_DECLARE(sg2042_clk, "sophgo,sg2042-clkgen", sg2042_clk_init);

Quoted 2 lines

No, this should be platform device. It's a child of another device, so you cannot use other way of init ordering.

hi, Krzysztof,

Quoted 8 lines

Quoted 4 lines

Thanks for your review.

I don't quite understand your opinion. Do you mean CLK_OF_DECLARE is only used for platform device so it can not be use here? But I think

No, I meant you mix init ordering: you depend now on syscon earlier initcall than CLK_OF_DECLARE. Do you remember which one is first? If anything changes here, your driver is broken. There is no dependency, no probe deferral.

hi, Krzysztof,

I found that the initcall method cannot be used for the clock controller of sg2042. We need to initialize the clock earlier because there are two dw-apb-timers in sg2042 (Sorry, I have not added them in the current DTS of sg2042, will be submitted later). The initialization of these timers (timer_probe()) depends on the initialization of the clock controller. If we use the initcall mechanism, it will be too late for the timer. So it seems better to use CLK_OF_DECLARE provided by CCF.

I have a question here that I would like to discuss. The design of sg2042 is like this, according to the design of memorymap in its TRM:

070:3001:0000 ~ 070:3001:0FFF SYS_CTRL 4K
070:3001:1000 ~ 070:3001:1FFF PINMUX 4K
070:3001:2000 ~ 070:3001:2FFF CLOCK 4K
070:3001:3000 ~ 070:3001:3FFF RESET 4K

But also as per hw design (I don't know why and I don't like it also :( ), some of the PLL/GATE CLOCK control registers are defined in the scope of SYS_CTRL, and others are defined in the scope of CLOCK. That's why in the current code, I define the syscon node corresponding to SYS_CTRL. The purpose is just to get the regmap of syscon for the clock controller through the device tree (through device_node_to_regmap()), so that the syscon defined in SYS_CTRL can be accessed through the regmap from clock. The clock controller driver itself does not rely on other operations of syscon.

So based on the above analysis, is it still necessary for us to define the clock controller as a child node of syscon? In the version v1 of this patch, I actually did not define the clock controller as a child node of syscon, but only accessed syscon through the phandle method. [1]

After more read of the TRM, I believe this situation only exists for clock. That is to say, there will be only one child node of clook under syscon. From a hardware design perspective, CLOCK and SYS_CTRL are two different blocks. So I think it is better to restore the original method, that is, restore clock and syscon to nodes of the same level, and let clock use phandle to access syscon.

What do you think or do you have any good suggestions?

Quoted 28 lines · Chen Wang

Quoted 17 lines · Krzysztof Kozlowski

Quoted 16 lines · Chen Wang

Quoted 11 lines · Krzysztof Kozlowski

Quoted 5 lines · Chen Wang

From: Chen Wang <[email protected]>

Add a driver for the SOPHGO SG2042 clock generator.

Signed-off-by Chen Wang <[email protected]>

...

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +3 -0

+}
+
+CLK_OF_DECLARE(sg2042_clk, "sophgo,sg2042-clkgen", sg2042_clk_init);

Quoted 2 lines

No, this should be platform device. It's a child of another device, so you cannot use other way of init ordering.

hi, Krzysztof,

Quoted 8 lines

Quoted 4 lines

Thanks for your review.

I don't quite understand your opinion. Do you mean CLK_OF_DECLARE is only used for platform device so it can not be use here? But I think

No, I meant you mix init ordering: you depend now on syscon earlier initcall than CLK_OF_DECLARE. Do you remember which one is first? If anything changes here, your driver is broken. There is no dependency, no probe deferral.

hi, Krzysztof,

Quoted 7 lines

I found that the initcall method cannot be used for the clock controller of sg2042. We need to initialize the clock earlier because there are two dw-apb-timers in sg2042 (Sorry, I have not added them in the current DTS of sg2042, will be submitted later). The initialization of these timers (timer_probe()) depends on the initialization of the clock controller. If we use the initcall mechanism, it will be too late for the timer. So it seems better to use CLK_OF_DECLARE provided by CCF.

Sure, that's fine, but don't use syscon in such case.

Quoted 19 lines

I have a question here that I would like to discuss. The design of sg2042 is like this, according to the design of memorymap in its TRM:

070:3001:0000 ~ 070:3001:0FFF SYS_CTRL 4K
070:3001:1000 ~ 070:3001:1FFF PINMUX 4K
070:3001:2000 ~ 070:3001:2FFF CLOCK 4K
070:3001:3000 ~ 070:3001:3FFF RESET 4K

But also as per hw design (I don't know why and I don't like it also :( ), some of the PLL/GATE CLOCK control registers are defined in the scope of SYS_CTRL, and others are defined in the scope of CLOCK. That's why in the current code, I define the syscon node corresponding to SYS_CTRL. The purpose is just to get the regmap of syscon for the clock controller through the device tree (through device_node_to_regmap()), so that the syscon defined in SYS_CTRL can be accessed through the regmap from clock. The clock controller driver itself does not rely on other operations of syscon.

Quoted 4 lines

So based on the above analysis, is it still necessary for us to define the clock controller as a child node of syscon? In the version v1 of this patch, I actually did not define the clock controller as a child node of syscon, but only accessed syscon through the phandle method. [1]

I have impression you ask me if your solution is ok, but I already pointed the problem. Address the problem - how do you enforce ordering of syscon and CLK_OF_DECLARE? What initcalls are both?

Quoted 8 lines

After more read of the TRM, I believe this situation only exists for clock. That is to say, there will be only one child node of clook under syscon. From a hardware design perspective, CLOCK and SYS_CTRL are two different blocks. So I think it is better to restore the original method, that is, restore clock and syscon to nodes of the same level, and let clock use phandle to access syscon.

What do you think or do you have any good suggestions?

Best regards,
Krzysztof

Quoted 17 lines · Krzysztof Kozlowski

Quoted 16 lines · Chen Wang

Quoted 11 lines · Krzysztof Kozlowski

Quoted 5 lines · Chen Wang

From: Chen Wang <[email protected]>

Add a driver for the SOPHGO SG2042 clock generator.

Signed-off-by Chen Wang <[email protected]>

...

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +3 -0

+}
+
+CLK_OF_DECLARE(sg2042_clk, "sophgo,sg2042-clkgen", sg2042_clk_init);

Quoted 2 lines

No, this should be platform device. It's a child of another device, so you cannot use other way of init ordering.

hi, Krzysztof,

Quoted 8 lines

Quoted 4 lines

Thanks for your review.

I don't quite understand your opinion. Do you mean CLK_OF_DECLARE is only used for platform device so it can not be use here? But I think

No, I meant you mix init ordering: you depend now on syscon earlier initcall than CLK_OF_DECLARE. Do you remember which one is first? If anything changes here, your driver is broken. There is no dependency, no probe deferral.

hi, Krzysztof,

I found that the initcall method cannot be used for the clock controller of sg2042. We need to initialize the clock earlier because there are two dw-apb-timers in sg2042 (Sorry, I have not added them in the current DTS of sg2042, will be submitted later). The initialization of these timers (timer_probe()) depends on the initialization of the clock controller. If we use the initcall mechanism, it will be too late for the timer. So it seems better to use CLK_OF_DECLARE provided by CCF.

I have a question here that I would like to discuss. The design of sg2042 is like this, according to the design of memorymap in its TRM:

070:3001:0000 ~ 070:3001:0FFF SYS_CTRL 4K
070:3001:1000 ~ 070:3001:1FFF PINMUX 4K
070:3001:2000 ~ 070:3001:2FFF CLOCK 4K
070:3001:3000 ~ 070:3001:3FFF RESET 4K

But also as per hw design (I don't know why and I don't like it also :( ), some of the PLL/GATE CLOCK control registers are defined in the scope of SYS_CTRL, and others are defined in the scope of CLOCK. That's why in the current code, I define the syscon node corresponding to SYS_CTRL. The purpose is just to get the regmap of syscon for the clock controller through the device tree (through device_node_to_regmap()), so that the syscon defined in SYS_CTRL can be accessed through the regmap from clock. The clock controller driver itself does not rely on other operations of syscon.

So based on the above analysis, is it still necessary for us to define the clock controller as a child node of syscon? In the version v1 of this patch, I actually did not define the clock controller as a child node of syscon, but only accessed syscon through the phandle method. [1]

After more read of the TRM, I believe this situation only exists for clock. That is to say, there will be only one child node of clook under syscon. From a hardware design perspective, CLOCK and SYS_CTRL are two different blocks. So I think it is better to restore the original method, that is, restore clock and syscon to nodes of the same level, and let clock use phandle to access syscon.

What do you think or do you have any good suggestions?

Link https://lore.kernel.org/linux-riscv/20231114-timid-habitat-a06e52e59c9c@squawk/#t open here

[1]

Thanks

Chen

Quoted 21 lines

Quoted 7 lines

this driver is still for platform device though I move the clock controller node as a child of the system contoller node. System controller node is just a block of registers which are used to control some other platform devices ,such as clock controller, reset controller and pin controller for this SoC.

Quoted 5 lines

And I also see other similar code in kernel, for example:
drivers/clk/clk-k210.c.

And I'm confused by your input "so you cannot use other way of init ordering." Do you mean "so you CAN use other way of init ordering"?

No, I meant you cannot. If you want to use syscon, then your driver should be a proper driver. Therefore add a driver.

Quoted 1 line

What's the other way of init ordering do you mean?

The one coming not from initcalls but driver model.

Best regards,
Krzysztof

Quoted 53 lines · Chen Wang

Quoted 17 lines · Krzysztof Kozlowski

Quoted 16 lines · Chen Wang

Quoted 11 lines · Krzysztof Kozlowski

Quoted 5 lines · Chen Wang

From: Chen Wang <[email protected]>

Add a driver for the SOPHGO SG2042 clock generator.

Signed-off-by Chen Wang <[email protected]>

...

drivers/clk/sophgo/clk-sophgo-sg2042.c new file +3 -0

+}
+
+CLK_OF_DECLARE(sg2042_clk, "sophgo,sg2042-clkgen", sg2042_clk_init);

Quoted 2 lines

No, this should be platform device. It's a child of another device, so you cannot use other way of init ordering.

hi, Krzysztof,

Quoted 8 lines

Quoted 4 lines

Thanks for your review.

I don't quite understand your opinion. Do you mean CLK_OF_DECLARE is only used for platform device so it can not be use here? But I think

No, I meant you mix init ordering: you depend now on syscon earlier initcall than CLK_OF_DECLARE. Do you remember which one is first? If anything changes here, your driver is broken. There is no dependency, no probe deferral.

hi, Krzysztof,

I found that the initcall method cannot be used for the clock controller of sg2042. We need to initialize the clock earlier because there are two dw-apb-timers in sg2042 (Sorry, I have not added them in the current DTS of sg2042, will be submitted later). The initialization of these timers (timer_probe()) depends on the initialization of the clock controller. If we use the initcall mechanism, it will be too late for the timer. So it seems better to use CLK_OF_DECLARE provided by CCF.

I have a question here that I would like to discuss. The design of sg2042 is like this, according to the design of memorymap in its TRM:

070:3001:0000 ~ 070:3001:0FFF SYS_CTRL 4K
070:3001:1000 ~ 070:3001:1FFF PINMUX 4K
070:3001:2000 ~ 070:3001:2FFF CLOCK 4K
070:3001:3000 ~ 070:3001:3FFF RESET 4K

But also as per hw design (I don't know why and I don't like it also :( ), some of the PLL/GATE CLOCK control registers are defined in the scope of SYS_CTRL, and others are defined in the scope of CLOCK. That's why in the current code, I define the syscon node corresponding to SYS_CTRL. The purpose is just to get the regmap of syscon for the clock controller through the device tree (through device_node_to_regmap()), so that the syscon defined in SYS_CTRL can be accessed through the regmap from clock. The clock controller driver itself does not rely on other operations of syscon.

Quoted 4 lines

So based on the above analysis, is it still necessary for us to define the clock controller as a child node of syscon? In the version v1 of this patch, I actually did not define the clock controller as a child node of syscon, but only accessed syscon through the phandle method. [1]

In that version of the code, clkgen, your DTS, looked like:

(quoted diff) +8 -0

+	clkgen: clock-controller {
+		compatible = "sophgo,sg2042-clkgen";
+		#clock-cells = <1>;
+		system-ctrl = <&sys_ctrl>;
+		clocks = <&cgi>;
+		assigned-clocks = \
 
+		assigned-clock-rates = \
 
+	};

It had no register regions of its own, just what it got from the sys ctrl block, which is why I said that. The syscon block looked like:

(quoted diff) +4 -0

+		sys_ctrl: syscon@7030010000 {
+			compatible = "sophgo,sg2042-syscon", "syscon";
+			reg = <0x70 0x30010000 0x0 0x8000>;
+		};

which given the register map does not seem like an accurate reflection of the size of this region. The "0x8000" should be "0x1000".

Quoted 6 lines

After more read of the TRM, I believe this situation only exists for clock. That is to say, there will be only one child node of clook under syscon.
From a hardware design perspective, CLOCK and SYS_CTRL are two different
blocks. So I think it is better to restore the original method, that is, restore clock and syscon to nodes of the same level, and let clock use phandle to access syscon.

This sounds two me like there are two different devices. One the "CLOCK" region at 070:3001:2000 that should be documented as being "sophgo,sg2042-clkgen" or similar and the second being the "SYS_CTRL" at 070:3001:0000 that is called something like "sophgo,sg2042-sysctrl". Having more than one clock controller is not a problem and sounds like a more accurate description of the hardware.

Quoted 30 lines

What do you think or do you have any good suggestions?

Link https://lore.kernel.org/linux-riscv/20231114-timid-habitat-a06e52e59c9c@squawk/#t open here

[1]

Thanks

Chen

Quoted 20 lines

Quoted 11 lines

this driver is still for platform device though I move the clock controller node as a child of the system contoller node. System controller node is just a block of registers which are used to control some other platform devices ,such as clock controller, reset controller and pin controller for this SoC.

And I also see other similar code in kernel, for example:
drivers/clk/clk-k210.c.

And I'm confused by your input "so you cannot use other way of init ordering." Do you mean "so you CAN use other way of init ordering"?

No, I meant you cannot. If you want to use syscon, then your driver should be a proper driver. Therefore add a driver.

Quoted 1 line

What's the other way of init ordering do you mean?

The one coming not from initcalls but driver model.

Best regards,
Krzysztof