Hi Krzysztof: Thanks for your review comments. > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > Sent: 2023年2月3日 4:31 > To: Hongxing Zhu <hongxing.zhu@xxxxxxx>; robh+dt@xxxxxxxxxx; > krzysztof.kozlowski+dt@xxxxxxxxxx; l.stach@xxxxxxxxxxxxxx; > shawnguo@xxxxxxxxxx; lorenzo.pieralisi@xxxxxxx; Peng Fan > <peng.fan@xxxxxxx>; marex@xxxxxxx; Marcel Ziswiler > <marcel.ziswiler@xxxxxxxxxxx>; tharvey@xxxxxxxxxxxxx; Frank Li > <frank.li@xxxxxxx> > Cc: devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; dl-linux-imx > <linux-imx@xxxxxxx> > Subject: Re: [PATCH v9 1/4] dt-bindings: imx6q-pcie: Restruct i.MX PCIe > schema > > 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@xxxxxxx> > > --- > > .../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: > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > +cetree.org%2Fschemas%2Fpci%2Ffsl%2Cimx6q-pcie-common.yaml%23&dat > a=05% > > > +7C01%7Chongxing.zhu%40nxp.com%7Cbc4ab9b963194b84cfb408db055c79 > 30%7C68 > > > +6ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638109666953361711% > 7CUnknown > > > +%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha > WwiLC > > > +JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=xbM1eZn2swqrsdlwpNA4eCe > KTdVWTL3RU9 > > +78v7d0DMU%3D&reserved=0 > > +$schema: > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7Chongxin > g.zhu% > > > +40nxp.com%7Cbc4ab9b963194b84cfb408db055c7930%7C686ea1d3bc2b4c > 6fa92cd9 > > > +9c5c301635%7C0%7C0%7C638109666953361711%7CUnknown%7CTWFpb > GZsb3d8eyJWI > > > +joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7 > C3000% > > > +7C%7C%7C&sdata=%2F%2FCEvu0g51SzeBXDToMlYrefPYbBWARm1FqQVai7x > Bc%3D&res > > +erved=0 > > + > > +title: Freescale i.MX6 PCIe RC/EP controller > > + > > +maintainers: > > + - Lucas Stach <l.stach@xxxxxxxxxxxxxx> > > + - Richard Zhu <hongxing.zhu@xxxxxxx> > > + > > +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. > Okay, would sperate the compatibles by RC ane EP. > > Also missing required: block. > Would add the common required: block too. Since the RC and EP schema has different description of the reg/reg-names/interrupts... Is it fine to let RC or EP schema has its own reg/reg-names/interrupts... properties? > (...) > > > 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: > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > +cetree.org%2Fschemas%2Fpci%2Ffsl%2Cimx6q-pcie-ep.yaml%23&data=05 > %7C01 > > > +%7Chongxing.zhu%40nxp.com%7Cbc4ab9b963194b84cfb408db055c7930% > 7C686ea1 > > > +d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638109666953361711%7CU > nknown%7CT > > > +WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL > CJXVC > > > +I6Mn0%3D%7C3000%7C%7C%7C&sdata=4UbTmbEFMOn9nDEO4WwVoheX > tl2zk%2BAo%2Ff > > +rbonvl0yo%3D&reserved=0 > > +$schema: > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7Chongxin > g.zhu% > > > +40nxp.com%7Cbc4ab9b963194b84cfb408db055c7930%7C686ea1d3bc2b4c > 6fa92cd9 > > > +9c5c301635%7C0%7C0%7C638109666953361711%7CUnknown%7CTWFpb > GZsb3d8eyJWI > > > +joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7 > C3000% > > > +7C%7C%7C&sdata=%2F%2FCEvu0g51SzeBXDToMlYrefPYbBWARm1FqQVai7x > Bc%3D&res > > +erved=0 > > + > > +title: Freescale i.MX6 PCIe Endpoint controller > > + > > +maintainers: > > + - Lucas Stach <l.stach@xxxxxxxxxxxxxx> > > + - Richard Zhu <hongxing.zhu@xxxxxxx> > > + > > +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/ How about this definitions of the required: block? <for common> required: - num-lanes - clocks - clock-names - power-domains - power-domain-names - resets - reset-names <for ep> required: - compatible - reg - reg-names - interrupts - interrupt-names <for rc> required: - compatible - reg - reg-names - "#address-cells" - "#size-cells" - device_type - bus-range - ranges - interrupts - interrupt-names - "#interrupt-cells" - interrupt-map-mask - interrupt-map > > > + > > +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 Okay. > > > + }; > > 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. Okay. Best Regards Richard Zhu > > > - - 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
