Re: [PATCH 4/5] dt-bindings: clock: airoha: Document support for AN7583 clock

[Christian Marangi <ansuelsmth@gmail.com>]

On Wed, May 28, 2025 at 01:56:54PM +0200, Krzysztof Kozlowski wrote:
> On 28/05/2025 10:54, Christian Marangi wrote:
> > On Wed, May 28, 2025 at 09:30:37AM +0200, Krzysztof Kozlowski wrote:
> >> On 28/05/2025 02:49, Christian Marangi wrote:
> >>>    - if:
> >>>        properties:
> >>>          compatible:
> >>> @@ -75,6 +78,17 @@ allOf:
> >>>          reg:
> >>>            maxItems: 1
> >>>  
> >>> +      required:
> >>> +        - reg
> >>> +
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          const: airoha,an7583-clock
> >>> +    then:
> >>> +      properties:
> >>> +        reg: false
> >>
> >>
> >> No resources here, so this should be part of parent node.
> >>
> > 
> > Ok hope you can help here. This is another case of "MFD" thing.
> > 
> > I was with the idea that it was O.K. to use this with very different
> > devices. (current scenario Clock controller and MDIO controller)
> > 
> > The node structure I had in mind was
> > 
> > 		system-controller@1fa20000 {
> > 			compatible = "airoha,an7583-scu", "syscon", "simple-mfd";
> > 			reg = <0x0 0x1fb00000 0x0 0x970>;
> > 
> > 			scuclk: scuclk {
> > 				compatible = "airoha,an7583-clock";
> > 				#clock-cells = <1>;
> > 				#reset-cells = <1>;
> > 			};
> > 
> > 			mdio {
> > 				compatible = "airoha,an7583-mdio";
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 
> > 				mdio_0: bus@0 {
> > 					reg = <0>;
> > 					resets = <&scuclk AN7583_MDIO0>;
> > 				};
> > 
> > 				mdio_1: bus@1 {
> > 					reg = <1>;
> > 					resets = <&scuclk AN7583_MDIO1>;
> > 				};
> > 			};
> > 		};
> > 
> > But you want
> > 
> > system-controller@1fa20000 {
> >         compatible = "airoha,an7583-scu", "syscon";
> >         reg = <0x0 0x1fb00000 0x0 0x970>;
> > 
> >         #clock-cells = <1>;
> >         #reset-cells = <1>;
> > 
> 
> mdio could be here just to group the bus (it's pretty common I think),
> although not sure if compatible is useful then.
> 
> >         mdio_0: bus@0 {
> >                 reg = <0>;
> >                 resets = <&scuclk AN7583_MDIO0>;
> >         };
> > 
> >         mdio_1: bus@1 {
> >                 reg = <1>;
> >                 resets = <&scuclk AN7583_MDIO1>;
> >         };
> > };
> > 
> > Again sorry if this question keeps coming around and I can totally
> > understand if you are getting annoyed by this. The reason I always ask
> > this is because it's a total PAIN to implement this with the driver
> > structure due to the old "simple-mfd" model.
> 
> ... and Rob was saying multiple times: be careful when adding
> simple-mfd. If it bites back, then I am sorry, but everyone were warned,
> weren't they?
> 
> What is exactly the pain anyway? You cannot instantiate children from
> SCU driver?
>

Answering below since they are related.

> > 
> > (as again putting everything in a single node conflicts with the OF
> > principle of autoprobing stuff with compatible property)
> 
> I am not sure if I follow. What principle? Where is this principle
> expressed?
> 
> And you do not have in your second example additional compatibles, so
> even if such principle exists it is not broken: everything autoprobes, I
> think.
> 
> > 
> 
>

The principle I'm talking about is one driver for one compatible.
(to be more precise excluding syscon compatible that is actually
ignored, if a driver for the compatible is found, any other compatible
is ignored.)

This means that declaring multiple compatible as:

compatible = "airoha,clock", "airoha,mdio"

doesn't result in the clock driver and the mdio driver probed but only
one of the 2 (probably only clock since it does have priority)

The "simple-mfd" compatible is just a simple compatible that indicate to
the OF system that every child (with a compatible) should be also probed.
And then automagically the driver gets probed.

Now the ""PAIN"" explaination. Not using the "simple-mfd" way with the
child with compatible and putting everything in the node means having to
create a dedicated MFD driver that just instruct to manually probe the
clock and mdio driver. (cause the compatible system can't be used)

So it's 3 driver instead of 2 with the extra effort of MFD driver
maintainer saying "Why simple-mfd is not used?"


There is a solution for this but I always feel it's more of a workaround
since it doesn't really describe the HW with the DT node.

The workaround is:

		system-controller@1fa20000 {
                        /* The parent SCU node implement the clock driver */
                        compatible = "airoha,an7583-scu", "syscon";
                        reg = <0x0 0x1fb00000 0x0 0x970>;

                        #clock-cells = <1>;
                        #reset-cells = <1>;

                        /* Clock driver is instructed to probe child */
                        mdio {
                                compatible = "airoha,an7583-mdio";
                                #address-cells = <1>;
                                #size-cells = <0>;

                                mdio_0: bus@0 {
                                        reg = <0>;
                                        resets = <&scuclk AN7583_MDIO0>;
                                };

                                mdio_1: bus@1 {
                                        reg = <1>;
                                        resets = <&scuclk AN7583_MDIO1>;
                                };
                        };
                };


But this really moves the probe from the simple-mfd to the clock driver.

So it's either 3 solution
1. 2 driver + "simple-mfd"
2. 3 driver + PAIN (due to MFD required driver)
3. 2 driver + not very correct DT node structure

Maybe option 3. is more acceptable?

The SCU node is mainly clock + reset controller and the MDIO bus is an
expansion of it?

Hope the long explaination makes sense to you (especially about the
OF principle thing)

--
Ansuel