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