Re: [RFC PATCH v2 03/19] dt-bindings: power: thead,th1520: Add support for power domains

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/12/2024 10:31, Michal Wilczynski wrote:
> 
> 
> On 12/23/24 17:09, Krzysztof Kozlowski wrote:
>> On 23/12/2024 13:55, Michal Wilczynski wrote:
>>> +  compatible:
>>> +    const: thead,th1520-pd
>>> +
>>> +  "#power-domain-cells":
>>> +    const: 1
>>> +
>>> +additionalProperties: false
>>> +
>>> +required:
>>> +  - compatible
>>> +  - "#power-domain-cells"
>>> +
>>> +examples:
>>> +  - |
>>> +    firmware {
>>
>> Drop
>>
>>> +        aon: aon {
>>> +            compatible = "thead,th1520-aon";
>>> +            mboxes = <&mbox_910t 1>;
>>> +            mbox-names = "aon";
>>
>> Drop aon node... but the main problem is you do not have any resources
>> in your power-domain device node, assuming your binding is complete.
>> This suggests that this is part of aon, not separate device. Fold the
>> device node into its parent (so everything goes to AON).
> 
> Merging everything to AON node would definitely work. I was looking at
> the other implementations of firmware protocols for example, and that's
> how I figured the current implementation:
> 
> arch/arm/boot/dts/broadcom/bcm2835-rpi.dtsi
> soc {
> 	firmware: firmware {
> 		compatible = "raspberrypi,bcm2835-firmware", "simple-mfd";
> 		mboxes = <&mailbox>;

That's a 10 year old binding. Not even reviewed initially by DT maintainers.

> 
> 		firmware_clocks: clocks {
> 			compatible = "raspberrypi,firmware-clocks";
> 			#clock-cells = <1>;
> 		};
> 	};
> 
> 	power: power {
> 		compatible = "raspberrypi,bcm2835-power";
> 		firmware = <&firmware>;
> 		#power-domain-cells = <1>;
> 	};
> };
> 
> This is fairly similar, as the firmware is passed as property, instead
> as in a parent-child relationship. Would you consider it more canonical
> ?

I consider early Raspberry bindings as antipattern. Same as many other
early approaches and that's not an accusation to their authors. Just
rules changed and capacity of DT maintainers increased.

> 
> I would be happy to merge everything to AON node, and merge the
> power-domain driver and AON driver together, but it seemed to me like
> those could use some separation, and since power-domain and the AON
> represent actual HW it's fine to represent them in the device tree.
> 

I said nothing about drivers and I did not suggest merging drivers.
Device nodes should not be split because they have different functions.
This is one single device with multiple functions. Just like hundreds of
others, e.g. clock, power domain and reset controller - all in one. Lack
of separate address space is the main aspect here indicating this is not
a separate device.

So merge the device nodes.



Best regards,
Krzysztof