[EXTERNAL] Re: [PATCH v3 3/3] arm64: dts: marvell: cp11x: Add reset controller node

[Wilson Ding]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk at kernel.org>
> Sent: Thursday, February 27, 2025 10:57 PM
> To: Wilson Ding <dingwei at marvell.com>; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Cc: andrew at lunn.ch; gregory.clement at bootlin.com;
> sebastian.hesselbarth at gmail.com; robh at kernel.org; krzk+dt at kernel.org;
> conor+dt at kernel.org; p.zabel at pengutronix.de; Sanghoon Lee
> <salee at marvell.com>; Geethasowjanya Akula <gakula at marvell.com>
> 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:
> > Add the reset controller node as a sub-node to the system controller
> > node.
> >
> > Signed-off-by: Wilson Ding <dingwei at marvell.com>
> > ---
> >  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
> > 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 at 284000 {
> >  		CP11X_LABEL(syscon0): system-controller at 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.

> 
> >  				compatible = "marvell,cp110-clock";
> > @@ -273,6 +275,12 @@ CP11X_LABEL(gpio2): gpio at 140 {
> >  					 <&CP11X_LABEL(clk) 1 17>;
> >  				status = "disabled";
> >  			};
> > +
> > +			CP11X_LABEL(swrst): reset-controller at 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 at 400000 {
	compatible = "syscon", "simple-mfd";
	reg = <0x400000 0x1000>;
	#address-cells = <1>;
	#size-cells = <1>;

	CP11X_LABEL(thermal): thermal-sensor at 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