-----Original Message-----
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: Thursday, February 27, 2025 10:57 PM
To: Wilson Ding <redacted>; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
Cc: andrew@lunn.ch; gregory.clement@bootlin.com;
sebastian.hesselbarth@gmail.com; robh@kernel.org; krzk+dt@kernel.org;
conor+dt@kernel.org; p.zabel@pengutronix.de; Sanghoon Lee
[off-list ref]; Geethasowjanya Akula [off-list ref]
Subject: [EXTERNAL] Re: [PATCH v3 3/3] arm64: dts: marvell: cp11x: Add reset
controller node

On 27/02/2025 20: 25, Wilson Ding wrote: > Add the reset controller node as
a sub-node to the system controller > node. > > Signed-off-by: Wilson Ding
<dingwei@ marvell. com> > --- > arch/arm64/boot/dts/marvell/armada-
cp11x. dtsi

On 27/02/2025 20:25, Wilson Ding wrote:

quoted

Add the reset controller node as a sub-node to the system controller
node.

Signed-off-by: Wilson Ding <redacted>
---
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi

b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi

quoted

index 161beec0b6b0..c27058d1534e 100644

--- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi

@@ -226,6 +226,8 @@ CP11X_LABEL(rtc): rtc@284000 {
 		CP11X_LABEL(syscon0): system-controller@440000 {
 			compatible = "syscon", "simple-mfd";
 			reg = <0x440000 0x2000>;
+			#address-cells = <1>;
+			#size-cells = <1>;

 			CP11X_LABEL(clk): clock {

Wait, no unit address here.

This subnode came from the existing code. I didn't touch this subnode
in my patch. As you can see, the system-controller has a wide address
range, which includes clock, GPIO registers as well as the unit-softreset
register.

quoted

 				compatible = "marvell,cp110-clock";

@@ -273,6 +275,12 @@ CP11X_LABEL(gpio2): gpio@140 {
 					 <&CP11X_LABEL(clk) 1 17>;
 				status = "disabled";
 			};
+
+			CP11X_LABEL(swrst): reset-controller@268 {

So why here it appeared? This is wrong and not even necessary. Entire
child should be folded into parent, so finally you will fix the
incomplete parent compatible.

We do need the reset-controller as a subnode under system-controller node
for the following reasons:

- We need to have 'reg' property in this subnode so that we can get the offset
  to system-controller register base defined in parent node. This is suggested
  by Rob in V2 comments. 
  And we need to know the register size to calculate the number of reset lines.
  This is suggested by Philipp in V1 comments.

- We also need to define the 'reset-cells' in this subnode. And the consumer of
  the reset controller uses the label of this subnode for the phandle and reset
  specifier pair. 

As I mentioned in my reply to the first comment, the reset-controller is not the
only device within the system-controller register spaces. Do you still think I
should fold it into the parent node. And what I proposed is exactly same as
that the armada_thermal driver did (See below). I wonder why what was accepted
in the past become not accepted now. 

CP11X_LABEL(syscon1): system-controller@400000 {
	compatible = "syscon", "simple-mfd";
	reg = <0x400000 0x1000>;
	#address-cells = <1>;
	#size-cells = <1>;

	CP11X_LABEL(thermal): thermal-sensor@70 {
		compatible = "marvell,armada-cp110-thermal";
		reg = <0x70 0x10>;
		interrupts-extended =
			<&CP11X_LABEL(icu_sei) 116 IRQ_TYPE_LEVEL_HIGH>;
		#thermal-sensor-cells = <1>;
	};
};

Best regards,
Krzysztof