Re: [PATCH v1 12/30] dt-bindings: reset: Add starfive,jh7110-reset bindings

On Tue, 11 Oct 2022 12:36:08 -0400, Krzysztof Kozlowski wrote:

On 11/10/2022 11:30, Hal Feng wrote:

quoted

On Thu, 29 Sep 2022 13:43:49 -0500, Rob Herring wrote:

quoted

On Fri, Sep 30, 2022 at 01:51:47AM +0800, Hal Feng wrote:

quoted

Add bindings for the reset controller on the JH7110 RISC-V
SoC by StarFive Technology Ltd.

Signed-off-by: Hal Feng <redacted>
---
 .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml

diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml
new file mode 100644
index 000000000000..bb0010c200f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml

@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 SoC Reset Controller Device Tree Bindings
+
+maintainers:
+  - Emil Renner Berthing <kernel@esmil.dk>
+  - Hal Feng <hal.feng@linux.starfivetech.com>
+
+properties:
+  compatible:
+    enum:
+      - starfive,jh7110-reset

'reg' needed? Is this a sub-block of something else?

Yes, the reset node is a child node of the syscon node, see patch 27 for detail.
You might not see the complete patches at that time due to technical issue of
our smtp email server. Again, I feel so sorry about that.

	syscrg: syscrg@13020000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x13020000 0x0 0x10000>;

		syscrg_clk: clock-controller@13020000 {
			compatible = "starfive,jh7110-clkgen-sys";
			clocks = <&osc>, <&gmac1_rmii_refin>,
				 <&gmac1_rgmii_rxin>,
				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
				 <&tdm_ext>, <&mclk_ext>;
			clock-names = "osc", "gmac1_rmii_refin",
				"gmac1_rgmii_rxin",
				"i2stx_bclk_ext", "i2stx_lrck_ext",
				"i2srx_bclk_ext", "i2srx_lrck_ext",
				"tdm_ext", "mclk_ext";
			#clock-cells = <1>;
		};

		syscrg_rst: reset-controller@13020000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;

So the answer to the "reg needed?" is what? You have unit address but no
reg, so this is not correct.

Not needed in the reset-controller node, but needed in its parent node. I am sorry
for missing description to point it out in the bindings. I will rewrite all bindings
for the next version. Unit address here should be deleted.

quoted

			starfive,assert-offset = <0x2F8>;
			starfive,status-offset= <0x308>;
			starfive,nr-resets = <JH7110_SYSRST_END>;
		};
	};

In this case, we get the memory mapped space through the parent node with syscon
APIs. You can see patch 13 for detail.

static int reset_starfive_register(struct platform_device *pdev, const u32 *asserted)
{

(...)

quoted

quoted

quoted

+
+  "#reset-cells":
+    const: 1
+
+  starfive,assert-offset:
+    description: Offset of the first ASSERT register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,status-offset:
+    description: Offset of the first STATUS register
+    $ref: /schemas/types.yaml#/definitions/uint32

These can't be implied from the compatible string?

Definitely can. We do this is for simplifying the reset driver.
Otherwise, we may need to define more compatibles because there
are multiple reset blocks in JH7110. Another case can be found at
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/reset/altr,rst-mgr.yaml

quoted

These two properties are the key differences among different reset controllers.

Different as in different compatibles? Please answer the questions..> 

quoted

There are five memory regions for clock and reset in StarFive JH7110 SoC. They
are "syscrg", "aoncrg", "stgcrg", "ispcrg" and "voutcrg". Each memory region
has different reset ASSERT/STATUS register offset and different number of reset
signals. 

Then these are not exactly the same devices, so using one compatible for
them does not look correct.

One compatible can just be matched by one device? I think this is what
confuses me.

Best regards,
Hal

quoted

After storing them in dt, the reset driver can register all reset
controllers with the same compatible string. 

Which is not how the compatible should be used...

quoted

All we expect is that all reset
controllers in a single SoC use the same compatible string for matching and the
reset driver can be applied to all StarFive SoCs using different compatible strings.

Keep driver out of the talks.

quoted

Just like

Existing bad pattern is not an argument to keep it going. Fix bad
patterns instead.

quoted

arch/riscv/boot/dts/starfive/jh7100.dtsi:

	rstgen: reset-controller@11840000 {
		compatible = "starfive,jh7100-reset";
		reg = <0x0 0x11840000 0x0 0x10000>;
		#reset-cells = <1>;
		starfive,assert-offset = <0x0>;
		starfive,status-offset= <0x10>;
		starfive,nr-resets = <JH7100_RSTN_END>;
	};

arch/riscv/boot/dts/starfive/jh7110.dtsi:

	syscrg: syscrg@13020000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x13020000 0x0 0x10000>;

		syscrg_clk: clock-controller@13020000 {
			compatible = "starfive,jh7110-clkgen-sys";
			...
		};

		syscrg_rst: reset-controller@13020000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;
			starfive,assert-offset = <0x2F8>;
			starfive,status-offset= <0x308>;
			starfive,nr-resets = <JH7110_SYSRST_END>;
		};
	};

	aoncrg: aoncrg@17000000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x17000000 0x0 0x10000>;

		aoncrg_clk: clock-controller@17000000 {
			compatible = "starfive,jh7110-clkgen-aon";
			...
		};

		aoncrg_rst: reset-controller@17000000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;
			starfive,assert-offset = <0x38>;
			starfive,status-offset= <0x3C>;
			starfive,nr-resets = <JH7110_AONRST_END>;
		};
	};

	stgcrg: stgcrg@10230000 {	//Not submmited yet
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x10230000 0x0 0x10000>;

		stgcrg_clk: clock-controller@10230000 {
			compatible = "starfive,jh7110-clkgen-stg";
			...
		};

		stgcrg_rst: reset-controller@10230000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;
			starfive,assert-offset = <0x74>;
			starfive,status-offset= <0x78>;
			starfive,nr-resets = <JH7110_STGRST_END>;
		};
	};
	...

quoted

quoted

+
+  starfive,nr-resets:
+    description: Number of reset signals
+    $ref: /schemas/types.yaml#/definitions/uint32

Why do you need this? Most bindings don't. If just to validate 'resets' 
args, then don't.

Can be removed. Instead, the reset driver should includes some related
binding headers or defines some macros for pointing out the number of
reset signals of each reset controller.

Best regards,
Hal

quoted

quoted

+
+required:
+  - compatible
+  - "#reset-cells"
+  - starfive,assert-offset
+  - starfive,status-offset
+  - starfive,nr-resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/starfive-jh7110.h>
+
+    syscrg_rst: reset-controller@13020000 {

Please test your patches.


Best regards,
Krzysztof