Message-ID: <ac0f6165-490a-d850-0ad4-a3c014f9d2e9@linaro.org>
Date:   Thu, 2 Feb 2023 21:31:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v9 1/4] dt-bindings: imx6q-pcie: Restruct i.MX PCIe schema
Content-Language: en-US
To:     Richard Zhu <hongxing.zhu@nxp.com>, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, l.stach@pengutronix.de,
        shawnguo@kernel.org, lorenzo.pieralisi@arm.com, peng.fan@nxp.com,
        marex@denx.de, marcel.ziswiler@toradex.com, tharvey@gateworks.com,
        frank.li@nxp.com
Cc:     devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, kernel@pengutronix.de,
        linux-imx@nxp.com
References: <1675323337-19358-1-git-send-email-hongxing.zhu@nxp.com>
 <1675323337-19358-2-git-send-email-hongxing.zhu@nxp.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <1675323337-19358-2-git-send-email-hongxing.zhu@nxp.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 02/02/2023 08:35, Richard Zhu wrote:
> Restruct i.MX PCIe schema, derive the common properties, thus they can
> be shared by both the RC and Endpoint schema.
> 
> Update the description of fsl,imx6q-pcie.yaml, and move the EP mode
> compatible to fsl,imx6q-pcie-ep.yaml.
> 
> Add support for i.MX8M PCIe Endpoint modes, and update the MAINTAINER
> accordingly.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  .../bindings/pci/fsl,imx6q-pcie-common.yaml   | 285 ++++++++++++++++++
>  .../bindings/pci/fsl,imx6q-pcie-ep.yaml       |  83 +++++
>  .../bindings/pci/fsl,imx6q-pcie.yaml          | 247 +--------------
>  MAINTAINERS                                   |   2 +
>  4 files changed, 376 insertions(+), 241 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
>  create mode 100644 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> new file mode 100644
> index 000000000000..f291f7529622
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
> @@ -0,0 +1,285 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/fsl,imx6q-pcie-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX6 PCIe RC/EP controller
> +
> +maintainers:
> +  - Lucas Stach <l.stach@pengutronix.de>
> +  - Richard Zhu <hongxing.zhu@nxp.com>
> +
> +description:
> +  Generic Freescale i.MX PCIe Root Port and Endpoint controller
> +  properties.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,imx6q-pcie
> +      - fsl,imx6sx-pcie
> +      - fsl,imx6qp-pcie
> +      - fsl,imx7d-pcie
> +      - fsl,imx8mq-pcie
> +      - fsl,imx8mm-pcie
> +      - fsl,imx8mp-pcie
> +      - fsl,imx8mm-pcie-ep
> +      - fsl,imx8mq-pcie-ep
> +      - fsl,imx8mp-pcie-ep

Compatibles are not part of common schema. Are you saying that EP
compatible is valid for PCIE not working as endpoint? This does not make
sense. The common part is only the part which is common. Compatible is
not common, not shared.


Also missing required: block.

(...)

> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
> new file mode 100644
> index 000000000000..f651bc3fcaf7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/fsl,imx6q-pcie-ep.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale i.MX6 PCIe Endpoint controller
> +
> +maintainers:
> +  - Lucas Stach <l.stach@pengutronix.de>
> +  - Richard Zhu <hongxing.zhu@nxp.com>
> +
> +description: |+
> +  This PCIe controller is based on the Synopsys DesignWare PCIe IP and
> +  thus inherits all the common properties defined in snps,dw-pcie-ep.yaml.
> +  The controller instances are dual mode where in they can work either in
> +  Root Port mode or Endpoint mode but one at a time.
> +
> +properties:
> +  reg:
> +    minItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: addr_space
> +
> +  interrupts:
> +    items:
> +      - description: builtin eDMA interrupter.
> +
> +  interrupt-names:
> +    items:
> +      - const: dma
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - num-lanes
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names

Several of these should be required by common schema/

> +
> +allOf:
> +  - $ref: /schemas/pci/snps,dw-pcie-ep.yaml#
> +  - $ref: /schemas/pci/fsl,imx6q-pcie-common.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx8mp-clock.h>
> +    #include <dt-bindings/power/imx8mp-power.h>
> +    #include <dt-bindings/reset/imx8mp-reset.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    pcie_ep: pcie-ep@33800000 {
> +      compatible = "fsl,imx8mp-pcie-ep";
> +      reg = <0x33800000 0x000400000>, <0x18000000 0x08000000>;
> +      reg-names = "dbi", "addr_space";
> +      clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
> +               <&clk IMX8MP_CLK_HSIO_AXI>,
> +               <&clk IMX8MP_CLK_PCIE_ROOT>;
> +      clock-names = "pcie", "pcie_bus", "pcie_aux";
> +      assigned-clocks = <&clk IMX8MP_CLK_PCIE_AUX>;
> +      assigned-clock-rates = <10000000>;
> +      assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_50M>;
> +      num-lanes = <1>;
> +      interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>; /* eDMA */
> +      interrupt-names = "dma";
> +      fsl,max-link-speed = <3>;
> +      power-domains = <&hsio_blk_ctrl IMX8MP_HSIOBLK_PD_PCIE>;
> +      resets = <&src IMX8MP_RESET_PCIE_CTRL_APPS_EN>,
> +               <&src IMX8MP_RESET_PCIE_CTRL_APPS_TURNOFF>;
> +      reset-names = "apps", "turnoff";
> +      phys = <&pcie_phy>;
> +      phy-names = "pcie-phy";
> +      num-ib-windows = <4>;
> +      num-ob-windows = <4>;
> +      status = "disabled";

Drop status

> +    };
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> index f13f87fddb3d..db1d0a04bde4 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> @@ -13,21 +13,13 @@ maintainers:
>  description: |+
>    This PCIe host controller is based on the Synopsys DesignWare PCIe IP
>    and thus inherits all the common properties defined in snps,dw-pcie.yaml.
> +  The controller instances are dual mode where in they can work either in
> +  Root Port mode or Endpoint mode but one at a time.
>  
> -properties:
> -  compatible:
> -    enum:
> -      - fsl,imx6q-pcie
> -      - fsl,imx6sx-pcie
> -      - fsl,imx6qp-pcie
> -      - fsl,imx7d-pcie
> -      - fsl,imx8mq-pcie
> -      - fsl,imx8mm-pcie
> -      - fsl,imx8mp-pcie

Compatibles should stay because these are not valid for EP schema.

> -      - fsl,imx8mm-pcie-ep
> -      - fsl,imx8mq-pcie-ep
> -      - fsl,imx8mp-pcie-ep
> +  See fsl,imx6q-pcie-ep.yaml for details on the Endpoint mode device tree
> +  bindings.
>  

	
Best regards,
Krzysztof