[1/1] dt-bindings: pci: layerscape-pci: Convert to yaml file

  Convert layerscape pcie bind document to yaml file.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../bindings/pci/fsl,layerscape-pcie-ep.yaml  |  84 +++++++++
 .../bindings/pci/fsl,layerscape-pcie.yaml     | 163 ++++++++++++++++++
 .../bindings/pci/layerscape-pci.txt           |  79 ---------
 3 files changed, 247 insertions(+), 79 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml
 delete mode 100644 Documentation/devicetree/bindings/pci/layerscape-pci.txt
  

Hey Frank,

On Wed, Feb 07, 2024 at 01:24:02AM -0500, Frank Li wrote:
> Convert layerscape pcie bind document to yaml file.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  .../bindings/pci/fsl,layerscape-pcie-ep.yaml  |  84 +++++++++
>  .../bindings/pci/fsl,layerscape-pcie.yaml     | 163 ++++++++++++++++++
>  .../bindings/pci/layerscape-pci.txt           |  79 ---------
>  3 files changed, 247 insertions(+), 79 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
>  create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml
>  delete mode 100644 Documentation/devicetree/bindings/pci/layerscape-pci.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> new file mode 100644
> index 0000000000000..3b592c820eb4c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/fsl,layerscape-pcie-ep.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale Layerscape PCIe controller
> +
> +maintainers:
> +  - Frank Li <Frank.Li@nxp.com>
> +
> +description: |+

Are you sure that you need this chomping operator?

> +  This PCIe endpoint controller is based on the Synopsys DesignWare PCIe IP

> +  and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml.

You shouldn't need this statement given you have the ref: below.

> +
> +  This controller derives its clocks from the Reset Configuration Word (RCW)
> +  which is used to describe the PLL settings at the time of chip-reset.
> +
> +  Also as per the available Reference Manuals, there is no specific 'version'
> +  register available in the Freescale PCIe controller register set,
> +  which can allow determining the underlying DesignWare PCIe controller version
> +  information.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,ls2088a-pcie-ep
> +      - fsl,ls1088a-pcie-ep
> +      - fsl,ls1046a-pcie-ep
> +      - fsl,ls1028a-pcie-ep
> +      - fsl,lx2160ar2-pcie-ep

Where did the fallback compatible go?

> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: regs
> +      - const: addr_space

The example uses "regs" and "config". Where did addr_space come from?

> +  fsl,pcie-scfg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: A phandle to the SCFG device node. The second entry is the
> +      physical PCIe controller index starting from '0'. This is used to get
> +      SCFG PEXN registers.
> +
> +  dma-coherent:

dma-coherent: true

> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: Indicates that the hardware IP block can ensure the coherency
> +      of the data transferred from/to the IP block. This can avoid the software
> +      cache flush/invalid actions, and improve the performance significantly.
> +
> +  big-endian:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: If the PEX_LUT and PF register block is in big-endian, specify
> +      this property.
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names

This was not previously required, why is it required now?

> +  - "#address-cells"
> +  - "#size-cells"
> +  - device_type
> +  - bus-range
> +  - ranges
> +
> +allOf:
> +  - $ref: /schemas/pci/snps,dw-pcie-ep.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - fsl,ls1028a-pcie-ep
> +            - fsl,ls1046a-pcie-ep
> +            - fsl,ls1088a-pcie-ep
> +    then:
> +      properties:
> +        interrupt-names:
> +          items:
> +            - const: pme

Please define the interrupt properties at the top-level and constrain
them on a per-device basis.

> +
> +unevaluatedProperties: false
> diff --git a/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml
> new file mode 100644
> index 0000000000000..e3719da306f25
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml
> @@ -0,0 +1,163 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/fsl,layerscape-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#

Only brief comments here, as it most is the same comments as for the 
> +
> +title: Freescale Layerscape PCIe controller
> +
> +maintainers:
> +  - Frank Li <Frank.Li@nxp.com>
> +
> +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.

Same two comments here as above.

> +
> +  This controller derives its clocks from the Reset Configuration Word (RCW)
> +  which is used to describe the PLL settings at the time of chip-reset.
> +
> +  Also as per the available Reference Manuals, there is no specific 'version'
> +  register available in the Freescale PCIe controller register set,
> +  which can allow determining the underlying DesignWare PCIe controller version
> +  information.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - fsl,ls1021a-pcie
> +      - fsl,ls2080a-pcie
> +      - fsl,ls2085a-pcie
> +      - fsl,ls2088a-pcie
> +      - fsl,ls1088a-pcie
> +      - fsl,ls1046a-pcie
> +      - fsl,ls1043a-pcie
> +      - fsl,ls1012a-pcie
> +      - fsl,ls1028a-pcie
> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: regs
> +      - const: config
> +
> +  fsl,pcie-scfg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: A phandle to the SCFG device node. The second entry is the
> +      physical PCIe controller index starting from '0'. This is used to get
> +      SCFG PEXN registers.
> +
> +  dma-coherent:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: Indicates that the hardware IP block can ensure the coherency
> +      of the data transferred from/to the IP block. This can avoid the software
> +      cache flush/invalid actions, and improve the performance significantly.

Same here.

> +
> +  big-endian:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: If the PEX_LUT and PF register block is in big-endian, specify
> +      this property.
> +
> +  msi-parent: true
> +
> +  iommu-map: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names

Same here.

> +  - "#address-cells"
> +  - "#size-cells"
> +  - device_type
> +  - bus-range
> +  - ranges
> +  - interrupts
> +  - interrupt-names
> +  - "#interrupt-cells"
> +  - interrupt-map-mask
> +  - interrupt-map
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +

> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - fsl,ls1028a-pcie
> +            - fsl,ls1046a-pcie
> +            - fsl,ls1043a-pcie
> +            - fsl,ls1012a-pcie
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 2
> +        interrupt-names:
> +          items:
> +            - const: pme
> +            - const: aer
> +
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - fsl,ls2080a-pcie
> +            - fsl,ls2085a-pcie
> +            - fsl,ls2088a-pcie
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +        interrupt-names:
> +          items:
> +            - const: intr
> +
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - fsl,ls1088a-pcie
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +        interrupt-names:
> +          items:
> +            - const: aer

And same here.

Thanks,
Conor.
  

On Wed, Feb 07, 2024 at 05:17:55PM +0000, Conor Dooley wrote:
> Hey Frank,
> 
> On Wed, Feb 07, 2024 at 01:24:02AM -0500, Frank Li wrote:
> > Convert layerscape pcie bind document to yaml file.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  .../bindings/pci/fsl,layerscape-pcie-ep.yaml  |  84 +++++++++
> >  .../bindings/pci/fsl,layerscape-pcie.yaml     | 163 ++++++++++++++++++
> >  .../bindings/pci/layerscape-pci.txt           |  79 ---------
> >  3 files changed, 247 insertions(+), 79 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> >  create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/pci/layerscape-pci.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> > new file mode 100644
> > index 0000000000000..3b592c820eb4c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> > @@ -0,0 +1,84 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/fsl,layerscape-pcie-ep.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Freescale Layerscape PCIe controller
> > +
> > +maintainers:
> > +  - Frank Li <Frank.Li@nxp.com>
> > +
> > +description: |+
> 
> Are you sure that you need this chomping operator?
> 
> > +  This PCIe endpoint controller is based on the Synopsys DesignWare PCIe IP
> 
> > +  and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml.
> 
> You shouldn't need this statement given you have the ref: below.
> 
> > +
> > +  This controller derives its clocks from the Reset Configuration Word (RCW)
> > +  which is used to describe the PLL settings at the time of chip-reset.
> > +
> > +  Also as per the available Reference Manuals, there is no specific 'version'
> > +  register available in the Freescale PCIe controller register set,
> > +  which can allow determining the underlying DesignWare PCIe controller version
> > +  information.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,ls2088a-pcie-ep
> > +      - fsl,ls1088a-pcie-ep
> > +      - fsl,ls1046a-pcie-ep
> > +      - fsl,ls1028a-pcie-ep
> > +      - fsl,lx2160ar2-pcie-ep
> 
> Where did the fallback compatible go?

So far, no fallback compatible needed now. each devices already have its
compatible string.

> 
> > +
> > +  reg:
> > +    maxItems: 2
> > +
> > +  reg-names:
> > +    items:
> > +      - const: regs
> > +      - const: addr_space
> 
> The example uses "regs" and "config". Where did addr_space come from?

Example just show pcie-host part. Not show pcie-ep part.
pcie-ep part need 'addr_space'.

> 
> > +  fsl,pcie-scfg:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: A phandle to the SCFG device node. The second entry is the
> > +      physical PCIe controller index starting from '0'. This is used to get
> > +      SCFG PEXN registers.
> > +
> > +  dma-coherent:
> 
> dma-coherent: true
> 
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description: Indicates that the hardware IP block can ensure the coherency
> > +      of the data transferred from/to the IP block. This can avoid the software
> > +      cache flush/invalid actions, and improve the performance significantly.
> > +
> > +  big-endian:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description: If the PEX_LUT and PF register block is in big-endian, specify
> > +      this property.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> 
> This was not previously required, why is it required now?

Actually its needed. 

> 
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - device_type
> > +  - bus-range
> > +  - ranges
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/snps,dw-pcie-ep.yaml#
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          enum:
> > +            - fsl,ls1028a-pcie-ep
> > +            - fsl,ls1046a-pcie-ep
> > +            - fsl,ls1088a-pcie-ep
> > +    then:
> > +      properties:
> > +        interrupt-names:
> > +          items:
> > +            - const: pme
> 
> Please define the interrupt properties at the top-level and constrain
> them on a per-device basis.
> 
> > +
> > +unevaluatedProperties: false
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml
> > new file mode 100644
> > index 0000000000000..e3719da306f25
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml
> > @@ -0,0 +1,163 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/fsl,layerscape-pcie.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> 
> Only brief comments here, as it most is the same comments as for the 
> > +
> > +title: Freescale Layerscape PCIe controller
> > +
> > +maintainers:
> > +  - Frank Li <Frank.Li@nxp.com>
> > +
> > +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.
> 
> Same two comments here as above.
> 
> > +
> > +  This controller derives its clocks from the Reset Configuration Word (RCW)
> > +  which is used to describe the PLL settings at the time of chip-reset.
> > +
> > +  Also as per the available Reference Manuals, there is no specific 'version'
> > +  register available in the Freescale PCIe controller register set,
> > +  which can allow determining the underlying DesignWare PCIe controller version
> > +  information.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - fsl,ls1021a-pcie
> > +      - fsl,ls2080a-pcie
> > +      - fsl,ls2085a-pcie
> > +      - fsl,ls2088a-pcie
> > +      - fsl,ls1088a-pcie
> > +      - fsl,ls1046a-pcie
> > +      - fsl,ls1043a-pcie
> > +      - fsl,ls1012a-pcie
> > +      - fsl,ls1028a-pcie
> > +
> > +  reg:
> > +    maxItems: 2
> > +
> > +  reg-names:
> > +    items:
> > +      - const: regs
> > +      - const: config
> > +
> > +  fsl,pcie-scfg:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: A phandle to the SCFG device node. The second entry is the
> > +      physical PCIe controller index starting from '0'. This is used to get
> > +      SCFG PEXN registers.
> > +
> > +  dma-coherent:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description: Indicates that the hardware IP block can ensure the coherency
> > +      of the data transferred from/to the IP block. This can avoid the software
> > +      cache flush/invalid actions, and improve the performance significantly.
> 
> Same here.
> 
> > +
> > +  big-endian:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description: If the PEX_LUT and PF register block is in big-endian, specify
> > +      this property.
> > +
> > +  msi-parent: true
> > +
> > +  iommu-map: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> 
> Same here.
> 
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - device_type
> > +  - bus-range
> > +  - ranges
> > +  - interrupts
> > +  - interrupt-names
> > +  - "#interrupt-cells"
> > +  - interrupt-map-mask
> > +  - interrupt-map
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +
> 
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          enum:
> > +            - fsl,ls1028a-pcie
> > +            - fsl,ls1046a-pcie
> > +            - fsl,ls1043a-pcie
> > +            - fsl,ls1012a-pcie
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 2
> > +        interrupt-names:
> > +          items:
> > +            - const: pme
> > +            - const: aer
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          enum:
> > +            - fsl,ls2080a-pcie
> > +            - fsl,ls2085a-pcie
> > +            - fsl,ls2088a-pcie
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 1
> > +        interrupt-names:
> > +          items:
> > +            - const: intr
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          enum:
> > +            - fsl,ls1088a-pcie
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          maxItems: 1
> > +        interrupt-names:
> > +          items:
> > +            - const: aer
> 
> And same here.
> 
> Thanks,
> Conor.
  

On Wed, Feb 07, 2024 at 12:49:19PM -0500, Frank Li wrote:
> On Wed, Feb 07, 2024 at 05:17:55PM +0000, Conor Dooley wrote:
> > Hey Frank,
> > 
> > On Wed, Feb 07, 2024 at 01:24:02AM -0500, Frank Li wrote:
> > > Convert layerscape pcie bind document to yaml file.
> > > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  .../bindings/pci/fsl,layerscape-pcie-ep.yaml  |  84 +++++++++
> > >  .../bindings/pci/fsl,layerscape-pcie.yaml     | 163 ++++++++++++++++++
> > >  .../bindings/pci/layerscape-pci.txt           |  79 ---------
> > >  3 files changed, 247 insertions(+), 79 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> > >  create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml
> > >  delete mode 100644 Documentation/devicetree/bindings/pci/layerscape-pci.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> > > new file mode 100644
> > > index 0000000000000..3b592c820eb4c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> > > @@ -0,0 +1,84 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/pci/fsl,layerscape-pcie-ep.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Freescale Layerscape PCIe controller
> > > +
> > > +maintainers:
> > > +  - Frank Li <Frank.Li@nxp.com>
> > > +
> > > +description: |+
> > 
> > Are you sure that you need this chomping operator?
> > 
> > > +  This PCIe endpoint controller is based on the Synopsys DesignWare PCIe IP
> > 
> > > +  and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml.
> > 
> > You shouldn't need this statement given you have the ref: below.
> > 
> > > +
> > > +  This controller derives its clocks from the Reset Configuration Word (RCW)
> > > +  which is used to describe the PLL settings at the time of chip-reset.
> > > +
> > > +  Also as per the available Reference Manuals, there is no specific 'version'
> > > +  register available in the Freescale PCIe controller register set,
> > > +  which can allow determining the underlying DesignWare PCIe controller version
> > > +  information.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - fsl,ls2088a-pcie-ep
> > > +      - fsl,ls1088a-pcie-ep
> > > +      - fsl,ls1046a-pcie-ep
> > > +      - fsl,ls1028a-pcie-ep
> > > +      - fsl,lx2160ar2-pcie-ep
> > 
> > Where did the fallback compatible go?
> 
> So far, no fallback compatible needed now. each devices already have its
> compatible string.

It used to exist though, have you checked that u-boot or *bsd etc do not
use the fallback compatible? You also need to mention your justification
for removing it in the commit message.

> > > +
> > > +  reg:
> > > +    maxItems: 2
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: regs
> > > +      - const: addr_space
> > 
> > The example uses "regs" and "config". Where did addr_space come from?
> 
> Example just show pcie-host part. Not show pcie-ep part.
> pcie-ep part need 'addr_space'.

Okay. Again, please mention where this is coming from.

> 
> > 
> > > +  fsl,pcie-scfg:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description: A phandle to the SCFG device node. The second entry is the
> > > +      physical PCIe controller index starting from '0'. This is used to get
> > > +      SCFG PEXN registers.
> > > +
> > > +  dma-coherent:
> > 
> > dma-coherent: true
> > 
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +    description: Indicates that the hardware IP block can ensure the coherency
> > > +      of the data transferred from/to the IP block. This can avoid the software
> > > +      cache flush/invalid actions, and improve the performance significantly.
> > > +
> > > +  big-endian:
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +    description: If the PEX_LUT and PF register block is in big-endian, specify
> > > +      this property.
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - reg-names
> > 
> > This was not previously required, why is it required now?
> 
> Actually its needed.

Well, if it wasn't, I'd hope that you wouldn't be making it required.
But I asked /why/ and you've not given a reason. Please mention the why
in your commit message for v2.

Cheers,
Conor.
  

On Thu, Feb 08, 2024 at 07:12:47PM +0000, Conor Dooley wrote:
> On Wed, Feb 07, 2024 at 12:49:19PM -0500, Frank Li wrote:
> > On Wed, Feb 07, 2024 at 05:17:55PM +0000, Conor Dooley wrote:
> > > Hey Frank,
> > > 
> > > On Wed, Feb 07, 2024 at 01:24:02AM -0500, Frank Li wrote:
> > > > Convert layerscape pcie bind document to yaml file.
> > > > 
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > >  .../bindings/pci/fsl,layerscape-pcie-ep.yaml  |  84 +++++++++
> > > >  .../bindings/pci/fsl,layerscape-pcie.yaml     | 163 ++++++++++++++++++
> > > >  .../bindings/pci/layerscape-pci.txt           |  79 ---------
> > > >  3 files changed, 247 insertions(+), 79 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> > > >  create mode 100644 Documentation/devicetree/bindings/pci/fsl,layerscape-pcie.yaml
> > > >  delete mode 100644 Documentation/devicetree/bindings/pci/layerscape-pci.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> > > > new file mode 100644
> > > > index 0000000000000..3b592c820eb4c
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/pci/fsl,layerscape-pcie-ep.yaml
> > > > @@ -0,0 +1,84 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/pci/fsl,layerscape-pcie-ep.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Freescale Layerscape PCIe controller
> > > > +
> > > > +maintainers:
> > > > +  - Frank Li <Frank.Li@nxp.com>
> > > > +
> > > > +description: |+
> > > 
> > > Are you sure that you need this chomping operator?
> > > 
> > > > +  This PCIe endpoint controller is based on the Synopsys DesignWare PCIe IP
> > > 
> > > > +  and thus inherits all the common properties defined in snps,dw-pcie-ep.yaml.
> > > 
> > > You shouldn't need this statement given you have the ref: below.
> > > 
> > > > +
> > > > +  This controller derives its clocks from the Reset Configuration Word (RCW)
> > > > +  which is used to describe the PLL settings at the time of chip-reset.
> > > > +
> > > > +  Also as per the available Reference Manuals, there is no specific 'version'
> > > > +  register available in the Freescale PCIe controller register set,
> > > > +  which can allow determining the underlying DesignWare PCIe controller version
> > > > +  information.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - fsl,ls2088a-pcie-ep
> > > > +      - fsl,ls1088a-pcie-ep
> > > > +      - fsl,ls1046a-pcie-ep
> > > > +      - fsl,ls1028a-pcie-ep
> > > > +      - fsl,lx2160ar2-pcie-ep
> > > 
> > > Where did the fallback compatible go?
> > 
> > So far, no fallback compatible needed now. each devices already have its
> > compatible string.
> 
> It used to exist though, have you checked that u-boot or *bsd etc do not
> use the fallback compatible? You also need to mention your justification
> for removing it in the commit message.

This commit just convert binding doc from txt to yaml. I just make sure
which equal to what descript in txt. If there are someting wrong in "uboot"
or "bsd", we can fixed it later. I checked driver code. exited dts tree
under kernel, which use unexited fallback compatible string
"fsl, lx-pcie-ep", which should be removed at dts file.

> 
> > > > +
> > > > +  reg:
> > > > +    maxItems: 2
> > > > +
> > > > +  reg-names:
> > > > +    items:
> > > > +      - const: regs
> > > > +      - const: addr_space
> > > 
> > > The example uses "regs" and "config". Where did addr_space come from?
> > 
> > Example just show pcie-host part. Not show pcie-ep part.
> > pcie-ep part need 'addr_space'.
> 
> Okay. Again, please mention where this is coming from.

Ideally it comes from snsp,dwc-pcie-ep.yaml. but it is use 'dbi' instead
of 'regs'. It needs extra effort to make driver code algin common
snps,dwc-pcie-ep.yaml, and update exist all dts files.

I think it will be deleted soon. 

> 
> > 
> > > 
> > > > +  fsl,pcie-scfg:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description: A phandle to the SCFG device node. The second entry is the
> > > > +      physical PCIe controller index starting from '0'. This is used to get
> > > > +      SCFG PEXN registers.
> > > > +
> > > > +  dma-coherent:
> > > 
> > > dma-coherent: true
> > > 
> > > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > > +    description: Indicates that the hardware IP block can ensure the coherency
> > > > +      of the data transferred from/to the IP block. This can avoid the software
> > > > +      cache flush/invalid actions, and improve the performance significantly.
> > > > +
> > > > +  big-endian:
> > > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > > +    description: If the PEX_LUT and PF register block is in big-endian, specify
> > > > +      this property.
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - reg-names
> > > 
> > > This was not previously required, why is it required now?
> > 
> > Actually its needed.
> 
> Well, if it wasn't, I'd hope that you wouldn't be making it required.
> But I asked /why/ and you've not given a reason. Please mention the why
> in your commit message for v2.

Sorry, I just sent v2 before see this. According to driver code, it is
needed. Please check v2, I will update at v3. 


> 
> Cheers,
> Conor.
>
  

On Thu, Feb 08, 2024 at 02:34:47PM -0500, Frank Li wrote:
> On Thu, Feb 08, 2024 at 07:12:47PM +0000, Conor Dooley wrote:
> > On Wed, Feb 07, 2024 at 12:49:19PM -0500, Frank Li wrote:
> > > On Wed, Feb 07, 2024 at 05:17:55PM +0000, Conor Dooley wrote:
> > > > On Wed, Feb 07, 2024 at 01:24:02AM -0500, Frank Li wrote:

> > > > > +
> > > > > +  This controller derives its clocks from the Reset Configuration Word (RCW)
> > > > > +  which is used to describe the PLL settings at the time of chip-reset.
> > > > > +
> > > > > +  Also as per the available Reference Manuals, there is no specific 'version'
> > > > > +  register available in the Freescale PCIe controller register set,
> > > > > +  which can allow determining the underlying DesignWare PCIe controller version
> > > > > +  information.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - fsl,ls2088a-pcie-ep
> > > > > +      - fsl,ls1088a-pcie-ep
> > > > > +      - fsl,ls1046a-pcie-ep
> > > > > +      - fsl,ls1028a-pcie-ep
> > > > > +      - fsl,lx2160ar2-pcie-ep
> > > > 
> > > > Where did the fallback compatible go?
> > > 
> > > So far, no fallback compatible needed now. each devices already have its
> > > compatible string.
> > 
> > It used to exist though, have you checked that u-boot or *bsd etc do not
> > use the fallback compatible? You also need to mention your justification
> > for removing it in the commit message.
> 
> This commit just convert binding doc from txt to yaml. I just make sure
> which equal to what descript in txt.

The text binding does have a fallback compatible though:
  EP mode:
	"fsl,ls1028a-pcie-ep", "fsl,ls-pcie-ep"
So this is a change compared to the text binding, without any
justification for it being okay to do.

> If there are someting wrong in "uboot"
> or "bsd", we can fixed it later.

If other bits of software are using the fallback, you cannot remove it.

> I checked driver code. exited dts tree
> under kernel, which use unexited fallback compatible string
> "fsl, lx-pcie-ep", which should be removed at dts file.

What do you mean by "unexisted"? It was in the text binding, so it is
perfectly fine to have it in the dts. Given it has users, I don't think
you should be removing the fallback without a very good justification.

> > > > > +  reg:
> > > > > +    maxItems: 2
> > > > > +
> > > > > +  reg-names:
> > > > > +    items:
> > > > > +      - const: regs
> > > > > +      - const: addr_space
> > > > 
> > > > The example uses "regs" and "config". Where did addr_space come from?
> > > 
> > > Example just show pcie-host part. Not show pcie-ep part.
> > > pcie-ep part need 'addr_space'.
> > 
> > Okay. Again, please mention where this is coming from.
> 
> Ideally it comes from snsp,dwc-pcie-ep.yaml. but it is use 'dbi' instead
> of 'regs'. It needs extra effort to make driver code algin common
> snps,dwc-pcie-ep.yaml, and update exist all dts files.
> 
> I think it will be deleted soon. 

What I am looking for here is you to explain in the commit message that
the endpoint driver in linux and the dts have always used "addr_space".
Checking that there's not a u-boot or *bsd that uses "config" would also
be very helpful.

Thanks,
Conor.
  

On Thu, Feb 08, 2024 at 07:44:59PM +0000, Conor Dooley wrote:
> On Thu, Feb 08, 2024 at 02:34:47PM -0500, Frank Li wrote:
> > On Thu, Feb 08, 2024 at 07:12:47PM +0000, Conor Dooley wrote:
> > > On Wed, Feb 07, 2024 at 12:49:19PM -0500, Frank Li wrote:
> > > > On Wed, Feb 07, 2024 at 05:17:55PM +0000, Conor Dooley wrote:
> > > > > On Wed, Feb 07, 2024 at 01:24:02AM -0500, Frank Li wrote:
> 
> > > > > > +
> > > > > > +  This controller derives its clocks from the Reset Configuration Word (RCW)
> > > > > > +  which is used to describe the PLL settings at the time of chip-reset.
> > > > > > +
> > > > > > +  Also as per the available Reference Manuals, there is no specific 'version'
> > > > > > +  register available in the Freescale PCIe controller register set,
> > > > > > +  which can allow determining the underlying DesignWare PCIe controller version
> > > > > > +  information.
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    enum:
> > > > > > +      - fsl,ls2088a-pcie-ep
> > > > > > +      - fsl,ls1088a-pcie-ep
> > > > > > +      - fsl,ls1046a-pcie-ep
> > > > > > +      - fsl,ls1028a-pcie-ep
> > > > > > +      - fsl,lx2160ar2-pcie-ep
> > > > > 
> > > > > Where did the fallback compatible go?
> > > > 
> > > > So far, no fallback compatible needed now. each devices already have its
> > > > compatible string.
> > > 
> > > It used to exist though, have you checked that u-boot or *bsd etc do not
> > > use the fallback compatible? You also need to mention your justification
> > > for removing it in the commit message.
> > 
> > This commit just convert binding doc from txt to yaml. I just make sure
> > which equal to what descript in txt.
> 
> The text binding does have a fallback compatible though:
>   EP mode:
> 	"fsl,ls1028a-pcie-ep", "fsl,ls-pcie-ep"
> So this is a change compared to the text binding, without any
> justification for it being okay to do.

Okay, I see what your concern. I think old txt is wrong. Or try to work
as it, but actually not. 

> 
> > If there are someting wrong in "uboot"
> > or "bsd", we can fixed it later.
> 
> If other bits of software are using the fallback, you cannot remove it.
> 
> > I checked driver code. exited dts tree
> > under kernel, which use unexited fallback compatible string
> > "fsl, lx-pcie-ep", which should be removed at dts file.
> 
> What do you mean by "unexisted"? It was in the text binding, so it is
> perfectly fine to have it in the dts. Given it has users, I don't think
> you should be removing the fallback without a very good justification.
> 

No driver parse "fsl,lx-pcie-ep". I can keep it as equal as old txt file
and remove later if need.

> > > > > > +  reg:
> > > > > > +    maxItems: 2
> > > > > > +
> > > > > > +  reg-names:
> > > > > > +    items:
> > > > > > +      - const: regs
> > > > > > +      - const: addr_space
> > > > > 
> > > > > The example uses "regs" and "config". Where did addr_space come from?
> > > > 
> > > > Example just show pcie-host part. Not show pcie-ep part.
> > > > pcie-ep part need 'addr_space'.
> > > 
> > > Okay. Again, please mention where this is coming from.
> > 
> > Ideally it comes from snsp,dwc-pcie-ep.yaml. but it is use 'dbi' instead
> > of 'regs'. It needs extra effort to make driver code algin common
> > snps,dwc-pcie-ep.yaml, and update exist all dts files.
> > 
> > I think it will be deleted soon. 
> 
> What I am looking for here is you to explain in the commit message that
> the endpoint driver in linux and the dts have always used "addr_space".
> Checking that there's not a u-boot or *bsd that uses "config" would also
> be very helpful.

I confused. Actually this two part PCIE-RC and PCIE-EP.
PCIE-RC using 'config'
PCIE-EP using 'addr_spcae'

I check old txt file, which have not mention it. I can remove it.

Frank
> 
> Thanks,
> Conor.
>
  

On Thu, Feb 08, 2024 at 03:20:50PM -0500, Frank Li wrote:

> > > > > > > +  reg:
> > > > > > > +    maxItems: 2
> > > > > > > +
> > > > > > > +  reg-names:
> > > > > > > +    items:
> > > > > > > +      - const: regs
> > > > > > > +      - const: addr_space
> > > > > > 
> > > > > > The example uses "regs" and "config". Where did addr_space come from?
> > > > > 
> > > > > Example just show pcie-host part. Not show pcie-ep part.
> > > > > pcie-ep part need 'addr_space'.
> > > > 
> > > > Okay. Again, please mention where this is coming from.
> > > 
> > > Ideally it comes from snsp,dwc-pcie-ep.yaml. but it is use 'dbi' instead
> > > of 'regs'. It needs extra effort to make driver code algin common
> > > snps,dwc-pcie-ep.yaml, and update exist all dts files.
> > > 
> > > I think it will be deleted soon. 
> > 
> > What I am looking for here is you to explain in the commit message that
> > the endpoint driver in linux and the dts have always used "addr_space".
> > Checking that there's not a u-boot or *bsd that uses "config" would also
> > be very helpful.
> 
> I confused. Actually this two part PCIE-RC and PCIE-EP.
> PCIE-RC using 'config'
> PCIE-EP using 'addr_spcae'

Yeah, I get this. The text binding makes it seem like "config" should be
used for both RC and EP, so I am just asking you to check that there are
no drivers in other kernels or bootloaders that use "config" for EP
mode.

> I check old txt file, which have not mention it. I can remove it.

if you drop "addr_space", you'll need to update the endpoint driver so
that it supports both "addr_space" and "config". If there are no
endpoint drivers using "config" in other operating systems, and all the
dts files use "addr_space", documenting "reg" and "addr_space" for
endpoint mode seems fair to me.

Thanks,
Conor.
  

On Thu, Feb 08, 2024 at 09:20:08PM +0000, Conor Dooley wrote:
> On Thu, Feb 08, 2024 at 03:20:50PM -0500, Frank Li wrote:
> 
> > > > > > > > +  reg:
> > > > > > > > +    maxItems: 2
> > > > > > > > +
> > > > > > > > +  reg-names:
> > > > > > > > +    items:
> > > > > > > > +      - const: regs
> > > > > > > > +      - const: addr_space
> > > > > > > 
> > > > > > > The example uses "regs" and "config". Where did addr_space come from?
> > > > > > 
> > > > > > Example just show pcie-host part. Not show pcie-ep part.
> > > > > > pcie-ep part need 'addr_space'.
> > > > > 
> > > > > Okay. Again, please mention where this is coming from.
> > > > 
> > > > Ideally it comes from snsp,dwc-pcie-ep.yaml. but it is use 'dbi' instead
> > > > of 'regs'. It needs extra effort to make driver code algin common
> > > > snps,dwc-pcie-ep.yaml, and update exist all dts files.
> > > > 
> > > > I think it will be deleted soon. 
> > > 
> > > What I am looking for here is you to explain in the commit message that
> > > the endpoint driver in linux and the dts have always used "addr_space".
> > > Checking that there's not a u-boot or *bsd that uses "config" would also
> > > be very helpful.
> > 
> > I confused. Actually this two part PCIE-RC and PCIE-EP.
> > PCIE-RC using 'config'
> > PCIE-EP using 'addr_spcae'
> 
> Yeah, I get this. The text binding makes it seem like "config" should be
> used for both RC and EP, so I am just asking you to check that there are
> no drivers in other kernels or bootloaders that use "config" for EP
> mode.

There are not 'config' concept for EP mode. Only RC mode have 'config'
space concept to get PCIe device's config space. EP mode only have
"add_space" for outbound windows. If other place using "config" for EP, it
is totally wrong, they should fix it.

> 
> > I check old txt file, which have not mention it. I can remove it.
> 
> if you drop "addr_space", you'll need to update the endpoint driver so
> that it supports both "addr_space" and "config". If there are no
> endpoint drivers using "config" in other operating systems, and all the
> dts files use "addr_space", documenting "reg" and "addr_space" for
> endpoint mode seems fair to me.

It is up to how to create patches. "addr_space" needs. If you want me to
create one version, which 100% match original txt. I can do that. Then
create increment patch to fix the problem.

If want to create a basic work version like this, which included some minus
fixes.  

The both method is fine for me. Second method just need more efforts.

Frank
> 
> Thanks,
> Conor.
  

On Thu, Feb 08, 2024 at 04:34:19PM -0500, Frank Li wrote:
> On Thu, Feb 08, 2024 at 09:20:08PM +0000, Conor Dooley wrote:
> > On Thu, Feb 08, 2024 at 03:20:50PM -0500, Frank Li wrote:
> > 
> > > > > > > > > +  reg:
> > > > > > > > > +    maxItems: 2
> > > > > > > > > +
> > > > > > > > > +  reg-names:
> > > > > > > > > +    items:
> > > > > > > > > +      - const: regs
> > > > > > > > > +      - const: addr_space
> > > > > > > > 
> > > > > > > > The example uses "regs" and "config". Where did addr_space come from?
> > > > > > > 
> > > > > > > Example just show pcie-host part. Not show pcie-ep part.
> > > > > > > pcie-ep part need 'addr_space'.
> > > > > > 
> > > > > > Okay. Again, please mention where this is coming from.
> > > > > 
> > > > > Ideally it comes from snsp,dwc-pcie-ep.yaml. but it is use 'dbi' instead
> > > > > of 'regs'. It needs extra effort to make driver code algin common
> > > > > snps,dwc-pcie-ep.yaml, and update exist all dts files.
> > > > > 
> > > > > I think it will be deleted soon. 
> > > > 
> > > > What I am looking for here is you to explain in the commit message that
> > > > the endpoint driver in linux and the dts have always used "addr_space".
> > > > Checking that there's not a u-boot or *bsd that uses "config" would also
> > > > be very helpful.
> > > 
> > > I confused. Actually this two part PCIE-RC and PCIE-EP.
> > > PCIE-RC using 'config'
> > > PCIE-EP using 'addr_spcae'
> > 
> > Yeah, I get this. The text binding makes it seem like "config" should be
> > used for both RC and EP, so I am just asking you to check that there are
> > no drivers in other kernels or bootloaders that use "config" for EP
> > mode.
> 
> There are not 'config' concept for EP mode. Only RC mode have 'config'
> space concept to get PCIe device's config space. EP mode only have
> "add_space" for outbound windows. If other place using "config" for EP, it
> is totally wrong, they should fix it.

It might be a totally wrong concept, but it is what the binding said, so
they are within their rights to use that name.

> 
> > 
> > > I check old txt file, which have not mention it. I can remove it.
> > 
> > if you drop "addr_space", you'll need to update the endpoint driver so
> > that it supports both "addr_space" and "config". If there are no
> > endpoint drivers using "config" in other operating systems, and all the
> > dts files use "addr_space", documenting "reg" and "addr_space" for
> > endpoint mode seems fair to me.
> 
> It is up to how to create patches. "addr_space" needs. If you want me to
> create one version, which 100% match original txt. I can do that. Then
> create increment patch to fix the problem.
> 
> If want to create a basic work version like this, which included some minus
> fixes.  
> 
> The both method is fine for me. Second method just need more efforts.

I don't have a problem with you making it "addr_space" as long as you
check to make sure that there are no users of "config" outside the
kernel and you mention in the commit message that this is a difference
from the text binding (and why).

Thanks,
Conor.