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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 30/05/2025 17:26, Christian Marangi wrote:
> On Thu, May 29, 2025 at 11:00:39AM +0200, Krzysztof Kozlowski wrote:
>> On 28/05/2025 14:57, Christian Marangi wrote:
>>>>> 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.
>>
>> There is no such principle. One compatible can map to many drivers and
>> many compatibles can map to one driver.
>>
> 
> I might be wrong but there is currently a limitation on the OF system
> where if a driver gets probed for a node then it's ignored for any other
> driver. (sorry for the bad explaination, hope it's understandable)

Yes but this can be changed easily. See: depopulate. Whether you
populate or not-populate is not a reason to model hardware description
one way or another.

> 
>>> (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)
>>
>> I don't understand this example. It makes no sense - clock is not
>> compatible with mdio.
>>
> 
> This was an example to put every properties in the oparent node.

So it was a bad example because it makes no sense. You move the
properties, not merge compatibles!

> 
>>>
>>> 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)
>>
>> You already have that driver - SCU. No need for new MFD driver...
>>
> 
> The SCU driver is actually the clock driver (currently). This was done
> for simplicity and because in SCU there were only some bits.
> 
> But now with AN7583 they put 2 MDIO controller in it.
> 
>>
>>>
>>> So it's 3 driver instead of 2 with the extra effort of MFD driver
>>> maintainer saying "Why simple-mfd is not used?"
>>
>> Sorry, that's a wrong argument. You can use simple-mfd, iff it follows
>> standard practices. If it does not fit standard practices, you cannot
>> use an argument "now I need more complicated solution".
>>
> 
> Then I think we are getting confused because I am following the usual
> pattern.
> 
> This is what would be the ideal and easy solution. (ti what was done on
> EN7581 with pinctrl and pwm)
> 
> 		scu: system-controller@1fa20000 {
> 			compatible = "syscon", "simple-mfd";
> 			reg = <0x0 0x1fb00000 0x0 0x970>;
> 
> 			scuclk: scuclk {
> 				compatible = "airoha,an7583-scu";
> 				#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>;
> 				};
> 			};
> 		};
> 
> 

By repeating the same you will not get different answers but rather me
become annoyed.

> 
>>>
>>>
>>> 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.
>>
>> Really? All arguments you used here are driver arguments - that
>> something is a pain in drivers. Now you mention that hardware would not
>> match description.
>>
>> Then let's change entire talk towards hardware description and send
>> patches matching hardware, not matching your MFD driver structure.
>>
> 
> Ok to describe the HW for this register block
> 
> SCU register:

What is here?

> - clock

Here are clocks, but what is in "SCU register"?

> - mdio controller 1
> - mdio controller 2
> 
> So this is why I think a good matching node block is:
> 
> parent node (SCU register):

So what is here?

> 	- child 1 (clock)
> 	- child 2 (mdio controller)
> 		- child 1 (mdio bus 1)
> 		- child 2 (mdio bus 2)
> 
> Is it wrong to model the DT node this way?

Yes and I explained you already why.

> 
>>>
>>> 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";
>>
>> Again, drop compatible.
>>
> 
> To drop the compatible a dedicated MFD driver is needed (or adding code
> in the clock driver to register the MDIO controller and that is net
> clean code wise)

If you need to do some driver changes, do some driver changes...


Best regards,
Krzysztof