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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/02/2025 21:18, Wilson Ding wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Thursday, February 27, 2025 10:57 PM
>> To: Wilson Ding <dingwei@marvell.com>; 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
>> <salee@marvell.com>; Geethasowjanya Akula <gakula@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@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@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.
> 
>>
>>>  				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