Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 1 Jul 2022 10:33:50 -0600
From:   Rob Herring <robh@kernel.org>
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc:     linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
        p.zabel@pengutronix.de, linux-kernel@vger.kernel.org,
        linux-mips@vger.kernel.org, vkoul@kernel.org, kishon@ti.com,
        rtanwar@maxlinear.com
Subject: Re: [PATCH v1 3/9] dt-bindings: reset: intel,rcu-gw: Update bindings
 for "legacy" SoCs
Message-ID: <20220701163350.GA1078312-robh@kernel.org>
References: <20220628124441.2385023-1-martin.blumenstingl@googlemail.com>
 <20220628124441.2385023-4-martin.blumenstingl@googlemail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220628124441.2385023-4-martin.blumenstingl@googlemail.com>
Precedence: bulk

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 ?

> +    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.

> +      - const: intel,rcu-lgm
>  
>    reg:
>      description: Reset controller registers.
> @@ -33,8 +40,6 @@ properties:
>          maximum: 31
>  
>    "#reset-cells":
> -    minimum: 2
> -    maximum: 3
>      description: |
>        First cell is reset request register offset.
>        Second cell is bit offset in reset request register.
> @@ -43,6 +48,43 @@ properties:
>        reset request and reset status registers is same. Whereas
>        3 for legacy SoCs as bit offset differs.
>  
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  big-endian: true
> +
> +patternProperties:
> +  "^usb2-phy@[0-9a-f]+$":
> +    type: object
> +    $ref: "../phy/lantiq,xway-rcu-usb2-phy.yaml"
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: intel,rcu-lgm
> +    then:
> +      properties:
> +        "#reset-cells":
> +          const: 2

else:
  properties:
    "#reset-cells":
      const: 3

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - lantiq,ase-rcu
> +              - lantiq,danube-rcu
> +              - lantiq,xrx100-rcu
> +              - lantiq,xrx200-rcu
> +    then:
> +      properties:
> +        "#reset-cells":
> +          const: 3
> +
>  required:
>    - compatible
>    - reg
> @@ -67,3 +109,35 @@ examples:
>          #pwm-cells = <2>;
>          resets = <&rcu0 0x30 21>;
>      };
> +  - |
> +    rcu_xrx200: rcu@203000 {
> +        compatible = "lantiq,xrx200-rcu", "simple-mfd", "syscon";
> +        reg = <0x203000 0x100>;
> +        big-endian;
> +
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        #reset-cells = <3>;
> +        intel,global-reset = <0x10 30 29>;
> +
> +        usb_phy0: usb2-phy@18 {
> +            compatible = "lantiq,xrx200-usb2-phy";
> +            reg = <0x18 4>, <0x38 4>;
> +            status = "disabled";

Why is your example disabled? Don't use 'status' in examples.

> +
> +            resets = <&rcu_xrx200 0x48 4 4>, <&rcu_xrx200 0x10 4 4>;

Humm, a dependency on the parent. Not a 'simple-mfd'.

> +            reset-names = "phy", "ctrl";
> +            #phy-cells = <0>;
> +        };
> +
> +        usb_phy1: usb2-phy@34 {
> +            compatible = "lantiq,xrx200-usb2-phy";
> +            reg = <0x34 4>, <0x3c 4>;
> +            status = "disabled";
> +
> +            resets = <&rcu_xrx200 0x48 5 5>, <&rcu_xrx200 0x10 4 4>;
> +            reset-names = "phy", "ctrl";
> +            #phy-cells = <0>;
> +        };
> +    };
> -- 
> 2.36.1
> 
>