Re: [PATCH 05/13] dt-bindings: mfd: add Documentation for Airoha EN7581 SCU

[Christian Marangi <ansuelsmth@gmail.com>]

On Mon, Mar 10, 2025 at 10:21:45AM +0100, Krzysztof Kozlowski wrote:
> On 09/03/2025 14:29, Christian Marangi wrote:
> > Add Documentation for Airoha EN7581 SCU.
> > 
> > Airoha EN7581 SoC expose registers to control miscellaneous pheriperals
> > via the SCU (System Controller Unit).
> > 
> > Example of these pheriperals are reset-controller, clock-controller,
> > PCIe line speed controller and bits to configure different Serdes ports
> > for USB or Ethernet usage.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../mfd/airoha,en7581-scu-sysctl.yaml         | 68 +++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml
> > new file mode 100644
> > index 000000000000..d7dc66f912c1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml
> > @@ -0,0 +1,68 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/airoha,en7581-scu-sysctl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Airoha EN7581 SCU (System Controller Unit)
> > +
> > +maintainers:
> > +  - Christian Marangi <ansuelsmth@gmail.com>
> > +
> > +description:
> > +  Airoha EN7581 SoC expose registers to control miscellaneous
> > +  pheriperals via the SCU (System Controller Unit).
> > +
> One more comment - there is no such thing as "sysctl" in your hardware.
> Look at the SCU binding which clearly says that it is the hardware you
> are duplicating here, so the "System Control Unit".
> 
> So you have existing "This node defines the System Control Unit of the
> EN7523 SoC" and you add one more node which defines the "System Control
> Unit", so you have two "System Control Unit" device nodes?
> 
> Look also what Stephen asked for:
> 
> https://lore.kernel.org/all/20220106013100.842FCC36AEB@smtp.kernel.org/
> 
> so how system-controller can now became clock-controller? Now, it was
> the system controller since the beginning.
>

The main problem here (and we had a similar problem with GPIO and PWM)
is that the Vendor (Airoha) wasn't so bright in placing the different
registers for the SoC so we have case where everything is mixed and not
one after another... 

Example we have 
- CLK register part 1
- Some bits that configure PCIe
- CLK register part 2
- GPIO
- CLK register part 3
- ...

The driver solution for this is syscon and the simple-mfd node
structure.

Now the main problem is how to modle this in DT. There are lots of case
where the simple-mfd model is used (like the one proposed) but probably
this is not accepted anymore. But again this should be clearly stated or
we have a chicken-egg problem when other devs implement similar thing and
have to implement simple MFD driver to handle this. (and driver
maintainers say "Use the simple-mfd model like it was already done)

For this specific case (and to give an answer to the clock patch after
this) the problem is that this register space was originally used only
to control the clock and I wasn't aware that it was also used to control
USB. Now that I'm implementing support for it, the disaster happened.

So In short SCU is lots of thing, both a system-controller, a
clock-controller and even a reset-controller.

To make it short, 2 different solution:
1. We can keep the current node structure of the node-controller and add a
child node for the SSR part (with a dedicated compatible).
2. Those property need to be be defined in the clock-controller node?

The ideal solution is 1. Does it work for you?

Sorry for the long post and hope you understand why this mess of
reworking the binding.

-- 
	Ansuel