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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <BY3PR18MB46730C150D4CB9619B3B05FBA7CC2@BY3PR18MB4673.namprd18.prod.outlook.com>
Date: Fri, 28 Feb 2025 20:18:17 +0000
From: Wilson Ding <dingwei@...vell.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>
CC: "andrew@...n.ch" <andrew@...n.ch>,
        "gregory.clement@...tlin.com"
	<gregory.clement@...tlin.com>,
        "sebastian.hesselbarth@...il.com"
	<sebastian.hesselbarth@...il.com>,
        "robh@...nel.org" <robh@...nel.org>,
        "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
        "conor+dt@...nel.org"
	<conor+dt@...nel.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        Sanghoon Lee <salee@...vell.com>,
        Geethasowjanya Akula <gakula@...vell.com>
Subject: RE: [EXTERNAL] Re: [PATCH v3 3/3] arm64: dts: marvell: cp11x: Add
 reset controller node



> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@...nel.org>
> Sent: Thursday, February 27, 2025 10:57 PM
> To: Wilson Ding <dingwei@...vell.com>; linux-kernel@...r.kernel.org;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org
> Cc: andrew@...n.ch; gregory.clement@...tlin.com;
> sebastian.hesselbarth@...il.com; robh@...nel.org; krzk+dt@...nel.org;
> conor+dt@...nel.org; p.zabel@...gutronix.de; Sanghoon Lee
> <salee@...vell.com>; Geethasowjanya Akula <gakula@...vell.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@...vell.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@...000 {
> >  		CP11X_LABEL(syscon0): system-controller@...000 {
> >  			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@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@...000 {
	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