On 04/03/2025 03:17, Wilson Ding wrote:
>
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>> Sent: Saturday, March 1, 2025 5:46 AM
>> To: Wilson Ding <dingwei@xxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
>> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: andrew@xxxxxxx; gregory.clement@xxxxxxxxxxx;
>> sebastian.hesselbarth@xxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
>> conor+dt@xxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx; Sanghoon Lee
>> <salee@xxxxxxxxxxx>; Geethasowjanya Akula <gakula@xxxxxxxxxxx>
>> 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@xxxxxxxxxx>
>>>> Sent: Thursday, February 27, 2025 10:57 PM
>>>> To: Wilson Ding <dingwei@xxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
>>>> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>>> Cc: andrew@xxxxxxx; gregory.clement@xxxxxxxxxxx;
>>>> sebastian.hesselbarth@xxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
>>>> conor+dt@xxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx; Sanghoon Lee
>>>> <salee@xxxxxxxxxxx>; Geethasowjanya Akula <gakula@xxxxxxxxxxx>
>>>> 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@xxxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxxxxxxxxxx/
>
> 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@xxxxxxxxxxxxxxxxxx/
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@xxxxxxxxxxxxxxxxxx/
>
> "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
