Re: [PATCH v1 3/9] dt-bindings: reset: intel,rcu-gw: Update bindings for "legacy" SoCs

[Rob Herring <robh@kernel.org>]

On Sun, Jul 03, 2022 at 01:04:20AM +0200, Martin Blumenstingl wrote:
>  Hi Rob,
> 
> On Fri, Jul 1, 2022 at 6:33 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Jun 28, 2022 at 02:44:35PM +0200, Martin Blumenstingl wrote:
> > > The Lantiq Amazon-SE, Danube, xRX100 and xRX200 SoCs have up to two USB2
> > > PHYs which are part of the RCU register space. The RCU registers on
> > > these SoCs are using big endian. Update the binding for these SoCs to
> > > properly describe this IP:
> > > - Add compatible strings for Amazon-SE, Danube and xRX100
> > > - Rename the xRX200 compatible string (which is not used anywhere) and
> > >   switch to the one previously documented in mips/lantiq/rcu.txt
> > > - Allow usage of "simple-mfd" and "syscon" in the compatible string so the
> > >   child devices (USB2 PHYs) can be described
> > > - Allow #address-cells and #size-cells to be set to 1 for describing the
> > >   child devices (USB2 PHYs)
> > > - #reset-cells must always be 3 (offset, reset bit and status bit) on the
> > >   legacy SoCs while LGM uses a fixed value of 2 (offset and reset bit -
> > >   status bit is always identical to the reset bit).
> > >
> > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > ---
> > >  .../bindings/reset/intel,rcu-gw.yaml          | 84 +++++++++++++++++--
> > >  1 file changed, 79 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml
> > > index be64f8597710..b90913c7b7d3 100644
> > > --- a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml
> > > +++ b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml
> > > @@ -11,9 +11,16 @@ maintainers:
> > >
> > >  properties:
> > >    compatible:
> > > -    enum:
> > > -      - intel,rcu-lgm
> > > -      - intel,rcu-xrx200
> >
> > It is okay to remove/change this because ?
> I'll update the description in v2. The "intel,rcu-xrx200" compatible
> string isn't used anywhere (upstream or downstream in OpenWrt).
> u-boot on Lantiq xRX200 SoCs is too old to pass a dtb to the kernel,
> so we're appending the DTB to the kernel image.
> 
> > > +    oneOf:
> > > +      - items:
> > > +          - enum:
> > > +              - lantiq,ase-rcu
> > > +              - lantiq,danube-rcu
> > > +              - lantiq,xrx100-rcu
> > > +              - lantiq,xrx200-rcu
> > > +          - const: simple-mfd
> >
> > This says child nodes have 0 dependence on anything in the parent node.
> > Such as a clock in the parent needing to be enabled.
> >
> > > +          - const: syscon
> >
> > Given the child nodes depend on this, I find the combination to be a
> > contradiction. But it's widely used, so oh well.
> I can think of two ways to solve this:
> 1) remove the simple-mfd compatible string and make the driver also
> discover child nodes
> 2) remove the simple-mfd compatible string and remove the USB PHY
> child nodes - then add add #phy-cells = <1> to the RCU node itself
> (and somehow update the RCU and USB PHY drivers accordingly)
> 3) introduce a separate child node for the reset-controller, then the
> child nodes depend on each other (but there's no strict dependency on
> the parent anymore other than the fact that the parent needs a
> "syscon" compatible string).
> 
> My understanding of this IP block is that it was initially designed as
> a reset controller, hence its name "reset controller unit" (RCU). Then
> additional logic was added after the fact.
> So I think 1) (dropping the simple-mfd compatible string) or 2)
> (dropping the simple-mfd compatible string and the child nodes
> altogether) is the right way to go here. Which route would you go and
> why?

2 would be my choice. That's the simplest binding. Unless the phy 
registers show up in different places on multiple devices, then maybe 
it's worth keeping the child node.

Rob