> -----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.
- 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.
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
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.
CP11X_LABEL(syscon1): system-controller@400000 {
compatible = "syscon", "simple-mfd";
reg = <0x400000 0x1000>;
#address-cells = <1>;
#size-cells = <1>;
CP11X_LABEL(thermal): thermal-sensor@70 {
compatible = "marvell,armada-cp110-thermal";
reg = <0x70 0x10>;
interrupts-extended =
<&CP11X_LABEL(icu_sei) 116 IRQ_TYPE_LEVEL_HIGH>;
#thermal-sensor-cells = <1>;
};
};
>
>
>
> Best regards,
> Krzysztof
