Re: [PATCH 2/3] dt-bindings: imx6q-pcie: Handle various PD configurations

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Marek,

Am Donnerstag, 3. November 2022, 17:25:46 CET schrieb Marek Vasut:
> On 11/3/22 13:32, Rob Herring wrote:
> > On Thu, Nov 3, 2022 at 3:29 AM Alexander Stein
> > 
> > <alexander.stein@ew.tq-group.com> wrote:
> >> Hi Marek,
> >> 
> >> Am Mittwoch, 2. November 2022, 22:57:28 CET schrieb Marek Vasut:
> >>> The i.MX SoCs have various power domain configurations routed into
> >>> the PCIe IP. MX6SX is the only one which contains 2 domains and also
> >>> uses power-domain-names. MX6QDL do not use any domains. All the rest
> >>> uses one domain and does not use power-domain-names anymore.
> >>> 
> >>> Document all those configurations in the DT binding document.
> >>> 
> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> ---
> >>> Cc: Fabio Estevam <festevam@gmail.com>
> >>> Cc: Lucas Stach <l.stach@pengutronix.de>
> >>> Cc: Richard Zhu <hongxing.zhu@nxp.com>
> >>> Cc: Rob Herring <robh+dt@kernel.org>
> >>> Cc: Shawn Guo <shawnguo@kernel.org>
> >>> Cc: linux-arm-kernel@lists.infradead.org
> >>> Cc: NXP Linux Team <linux-imx@nxp.com>
> >>> To: devicetree@vger.kernel.org
> >>> ---
> >>> 
> >>>   .../bindings/pci/fsl,imx6q-pcie.yaml          | 47 ++++++++++++++-----
> >>>   1 file changed, 34 insertions(+), 13 deletions(-)
> >>> 
> >>> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> >>> b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml index
> >>> 1cfea8ca72576..fc8d4d7b80b38 100644
> >>> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> >>> 
> >>> @@ -68,19 +68,6 @@ properties:
> >>>       description: A phandle to an fsl,imx7d-pcie-phy node. Additional
> >>>       
> >>>         required properties for imx7d-pcie and imx8mq-pcie.
> >>> 
> >>> -  power-domains:
> >>> -    items:
> >>> -      - description: The phandle pointing to the DISPLAY domain for
> >>> -          imx6sx-pcie, to PCIE_PHY power domain for imx7d-pcie and
> >>> -          imx8mq-pcie.
> >>> -      - description: The phandle pointing to the PCIE_PHY power domains
> >>> -          for imx6sx-pcie.
> >>> -
> >>> -  power-domain-names:
> >>> -    items:
> >>> -      - const: pcie
> >>> -      - const: pcie_phy
> >>> -
> >>> 
> >>>     resets:
> >>>       maxItems: 3
> >>>       description: Phandles to PCIe-related reset lines exposed by SRC
> >>> 
> >>> @@ -241,6 +228,40 @@ allOf:
> >>>                   - const: pcie_bus
> >>>                   - const: pcie_phy
> >>> 
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: fsl,imx6sx-pcie
> >>> +    then:
> >>> +      properties:
> >>> +        power-domains:
> >>> +          items:
> >>> +            - description: The phandle pointing to the DISPLAY domain
> >>> for
> >>> +                imx6sx-pcie, to PCIE_PHY power domain for imx7d-pcie
> >>> and
> >>> +                imx8mq-pcie.
> >>> +            - description: The phandle pointing to the PCIE_PHY power
> >>> domains +                for imx6sx-pcie.
> >>> +        power-domain-names:
> >>> +          items:
> >>> +            - const: pcie
> >>> +            - const: pcie_phy
> >>> +    else:
> >>> +      if:
> >>> +        not:
> >>> +          properties:
> >>> +            compatible:
> >>> +              contains:
> >>> +                enum:
> >>> +                  - fsl,imx6q-pcie
> >>> +                  - fsl,imx6qp-pcie
> >>> +      then:
> >>> +        properties:
> >>> +          power-domains:
> >>> +            description: |
> >>> +               The phandle pointing to the DISPLAY domain for
> >>> imx6sx-pcie,
> >>> to +               PCIE_PHY power domain for imx7d-pcie and imx8mq-pcie.
> >>> +
> >> 
> >> Doesn't it makes more sense to keep the power-domains descriptions in the
> >> common part on top, as before, but adjust minItems/maxItems for each
> >> compatible?
> > 
> > Yes. Keep properties defined at the top level.
> 
> The problem I keep running into here is that if I apply patch like below
> (basically what you and Alex are suggesting), I get this warning:
> 
> arch/arm64/boot/dts/freescale/imx8mm-board.dtb: pcie@33800000:
> power-domains: [[86]] is too short

I guess you need this:
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
@@ -65,6 +65,7 @@ properties:
       required properties for imx7d-pcie and imx8mq-pcie.
 
   power-domains:
+    minItems: 1
     items:
       - description: The phandle pointing to the DISPLAY domain for
           imx6sx-pcie, to PCIE_PHY power domain for imx7d-pcie and

I have a similar WIP change on my tree which add 'minItems: 1' to power-
domains and also sets 'maxItems: 1' to power-domains for everything being not 
fsl,imx6sx-pcie.

Best regards,
Alexander

> I think that's because power-domains: contains items: and to validate
> that imx8mm.dtsi with pcie@33800000 { power-domains = <&pgc_pcie>; };, I
> would need to get rid of those items: ? Which is what I did in the
> aforementioned patch for imx8m, that's why I removed it from the common
> part.
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> index 12c7baba489aa..ec5e8dfe541ea 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> @@ -68,6 +68,18 @@ properties:
>       description: A phandle to an fsl,imx7d-pcie-phy node. Additional
>         required properties for imx7d-pcie and imx8mq-pcie.
> 
> +  power-domains:
> +    items:
> +      - description: The phandle pointing to the DISPLAY domain for
> +          imx6sx-pcie, to PCIE_PHY power domain for imx7d-pcie and
> +          imx8mq-pcie.
> +      - description: The phandle pointing to the PCIE_PHY power domains
> +          for imx6sx-pcie.
> +  power-domain-names:
> +    items:
> +      - const: pcie
> +      - const: pcie_phy
> +
>     resets:
>       maxItems: 2
>       description: Phandles to PCIe-related reset lines exposed by SRC
> @@ -235,16 +247,11 @@ allOf:
>       then:
>         properties:
>           power-domains:
> -          items:
> -            - description: The phandle pointing to the DISPLAY domain for
> -                imx6sx-pcie, to PCIE_PHY power domain for imx7d-pcie and
> -                imx8mq-pcie.
> -            - description: The phandle pointing to the PCIE_PHY power
> domains
> -                for imx6sx-pcie.
> +          minItems: 2
> +          maxItems: 2
>           power-domain-names:
> -          items:
> -            - const: pcie
> -            - const: pcie_phy
> +          minItems: 2
> +          maxItems: 2
>       else:
>         if:
>           not:
> @@ -257,9 +264,8 @@ allOf:
>         then:
>           properties:
>             power-domains:
> -            description: |
> -               The phandle pointing to the DISPLAY domain for
> imx6sx-pcie, to
> -               PCIE_PHY power domain for imx7d-pcie and imx8mq-pcie.
> +            minItems: 1
> +            maxItems: 1
> 
>     - if:
>         properties: