> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: Monday, March 3, 2025 11:14 PM
> To: Wilson Ding <dingwei@xxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> robh@xxxxxxxxxx
> Cc: andrew@xxxxxxx; gregory.clement@xxxxxxxxxxx;
> sebastian.hesselbarth@xxxxxxxxx; 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 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_l
> > inux-2Darm-2Dkernel_20180703211335.GA8858-40rob-2Dhp-
> 2Dlaptop_&d=DwIDa
> >
> Q&c=nKjWec2b6R0mOyPaz7xtfQ&r=sXDQZu4GyqNVDlFUXakSGJl0Dh81ZIPl
> U26YS4KHG
> >
> IA&m=38vmzSKDh1f33oyqXyKxLMelJiyurSZl38hR43cMNb1JmGrdorIuj2bCEn
> HRs37W&
> > s=MsDwvAhgZcRztTZgawJ_imU4Ld5aZebUxhtCaXUgY5A&e=
>
> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_a
> > ll_1460648013-2D31320-2D5-2Dgit-2Dsend-2Demail-
> 2Dthomas.petazzoni-40fr
> > ee-
> 2Delectrons.com_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=sXDQZu4Gy
> qNVDl
> >
> FUXakSGJl0Dh81ZIPlU26YS4KHGIA&m=38vmzSKDh1f33oyqXyKxLMelJiyurSZl
> 38hR43
> > cMNb1JmGrdorIuj2bCEnHRs37W&s=pvsI6D-
> tAm32RQzrFFUl7pAclLGStG4bBgQ_iHs2R
> > _Q&e=
> >
> > 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_a
> > ll_b27495e10fb4f4d8a7fd1a760d49402bbae83b58.1496328934.git-
> 2Dseries.gr
> > egory.clement-40free-
> 2Delectrons.com_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtf
> >
> Q&r=sXDQZu4GyqNVDlFUXakSGJl0Dh81ZIPlU26YS4KHGIA&m=38vmzSKDh1
> f33oyqXyKx
> >
> LMelJiyurSZl38hR43cMNb1JmGrdorIuj2bCEnHRs37W&s=R58U_4PGdQrcLB
> Wh1gfNnZe
> > voNQfbABbW5QlvgOrAGY&e=
>
> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_a
> > ll_bb21ee9acc55efac884450ff710049b99b27f8bf.1496328934.git-
> 2Dseries.gr
> > egory.clement-40free-
> 2Delectrons.com_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtf
> >
> Q&r=sXDQZu4GyqNVDlFUXakSGJl0Dh81ZIPlU26YS4KHGIA&m=38vmzSKDh1
> f33oyqXyKx
> >
> LMelJiyurSZl38hR43cMNb1JmGrdorIuj2bCEnHRs37W&s=8U7S14wRqQiL6eO
> C6G2GPpY
> > tlvgFIbmNmEjljAKDdCA&e=
> >
> > "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
I did consider shrinking the syscon's register address range to
make the reset-controller node to be independent from the
syscon node. However, I found the syscon node is also referred
by some devices for miscellaneous configurations . The reset
configuration register happens to be located in between these
registers and clock/GPIO registers.
> drop offset in your patch or unify everything into 'reg'.
>
This is exactly what I proposed in v3 patch. Do I misunderstand
you?
CP11X_LABEL(swrst): reset-controller@268 {
compatible = "marvell,armada8k-reset";
reg = <0x268 0x4>;
#reset-cells = <1>;
};
>
> Best regards,
> Krzysztof
