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: <050ae833-10b5-4d80-9856-8bc2f434a74f@kernel.org>
Date: Sat, 1 Mar 2025 14:46:06 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Wilson Ding <dingwei@...vell.com>,
 "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

On 28/02/2025 21:18, Wilson Ding wrote:
> 
> 
>> -----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.

You do not need and you received that comment as well. It is implied by
compatible.

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

reset-cells will be in the parent once you fold it.

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

You provided very little hardware description of the device. So based on
hardware description you provided: yes.

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

We did not discuss here drivers, but if you insist talking about
"marvell,armada-cp110-thermal" then point me to review or ack from DT
people. You claim it was accepted so how did we accept it?

It was 2013 so that's another answer: many things done 12 years ago were
done not according to best practices. Also best practices evolved.

Best regards,
Krzysztof