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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 04/03/2025 03:17, Wilson Ding wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Saturday, March 1, 2025 5:46 AM
>> 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: 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@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?
>>
> 
> I didn't intend to extend discussion to the driver in this thread. The following
> Is the review thread of the dt-binding for the thermal device (in 2018).
> Indeed, there is no comments challenging why not fold the thermal sub-node
> Into the parent 'syscon' node.
> 
> https://lore.kernel.org/linux-arm-kernel/20180703211335.GA8858@rob-hp-laptop/

Indeed, this one got review. I was checking armada-thermal and it never
got any, when it was merged back in the 2013.

> 
> Digging further, I found some interesting history about the parent 'syscon' node
> of the reset-controller. I'd appreciate if you can take a look into the following
> patches/thread -
> 
> The syscon0 node was initially added along with Armada clock driver support.
> It was the very beginning of the upstream for Armada SoCs support (2016).
> And the clock driver is one of the earliest drivers to be mainlined. At that time,
> the clock controller is the only supported device within sycon register range.
> As you can see, the clock dt-binding was exactly aligned with what your suggested
> (no sub-node, compatible and clock-cells just in syscon). 
> 
> https://lore.kernel.org/all/1460648013-31320-5-git-send-email-thomas.petazzoni@free-electrons.com/
> 
> Besides the clock controller, the system controller also includes the GPIO controller,
> pinctl controller, reset controller and other miscellaneous configurations. Before
> adding the pinctl dt-binding, it's decided to use the sub-nodes to present the multiple
> function blocks of various devices.
> 
> https://lore.kernel.org/all/b27495e10fb4f4d8a7fd1a760d49402bbae83b58.1496328934.git-series.gregory.clement@free-electrons.com/

So this is the source here. I see.

Commit comes with a rationale that it will grow significantly.

> 
> In the following patch, it was clearly addressed why sub-nodes was chosen
> over one flat node.
> 
> https://lore.kernel.org/all/bb21ee9acc55efac884450ff710049b99b27f8bf.1496328934.git-series.gregory.clement@free-electrons.com/
> 
> "The initial intent when the binding of the cp110 system controller was to
> have one flat node. The idea being that what is currently a clock-only
> driver in drivers would become a MFD driver, exposing the clock, GPIO and
> pinctrl functionality. However, after taking a step back, this would lead
> to a messy binding. Indeed, a single node would be a GPIO controller,
> clock controller, pinmux controller, and more.
> 
> This patch adopts a more classical solution of a top-level syscon node
> with sub-nodes for the individual devices. The main benefit will be to
> have each functional block associated to its own sub-node where we can
> put its own properties."
> 
> Since then, the dt-binding of Armada's system controller became an
> exception. But I think it's sensible. If we do put all these controllers into
> one node, you can image the properties of different devices will be
> messed up, e.g., not just #reset-cells, #clock-cells and #gpio-cells will
> be gathered. There will be a long compatible list of all devices.
> 
> Going back to my current patch - if we fold the reset controller into the
> parent node, the syscon node will become a hybrid, which GPIO and 
> clock controller are still sub-nodes while reset controller is folded into
> the syscon node. Isn't it very confusing?

Yes, it will be. But more confusing is existing pattern of mixing MMIO
nodes with non-MMIO which you grow. So okay, keep them as separate
child, but drop offset in your patch or unify everything into 'reg'.


Best regards,
Krzysztof