On Wed, 12 Oct 2022 09:33:42 -0400, Krzysztof Kozlowski wrote:

On 12/10/2022 09:16, Hal Feng wrote:

quoted

quoted

quoted

quoted

quoted

+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. 

We do not talk about parent node. Rob's question was in this bindings.
Is this document a binding for the parent node or for this node?

This node. So not needed.

quoted

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

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.

The role of the bindings is not to simplify some specific driver in some
specific OS...

quoted

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

And why is this a problem? You have different hardware, so should have
different compatibles. Otherwise we would have a compatible
"all,everything" and use it in all possible devices.

Okay, I get it. Thanks a lot.

Best regards,
Hal