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