diff mbox series

[1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller

Message ID 20201027060011.25893-2-wan.ahmad.zainie.wan.mohamad@intel.com
State Changes Requested
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: keembay: Add support for Intel Keem Bay | expand

Commit Message

Wan Mohamad, Wan Ahmad Zainie Oct. 27, 2020, 6 a.m. UTC
Document DT bindings for PCIe controller found on Intel Keem Bay SoC.

Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
---
 .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
 .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
 2 files changed, 206 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml

Comments

Rob Herring Oct. 28, 2020, 1:57 p.m. UTC | #1
On Tue, 27 Oct 2020 14:00:10 +0800, Wan Ahmad Zainie wrote:
> Document DT bindings for PCIe controller found on Intel Keem Bay SoC.
> 
> Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> ---
>  .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
>  .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
>  2 files changed, 206 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
>  create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml:14:7: [warning] wrong indentation: expected 4 but found 6 (indentation)
./Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml:17:7: [warning] wrong indentation: expected 4 but found 6 (indentation)

dtschema/dtc warnings/errors:


See https://patchwork.ozlabs.org/patch/1388284

The base for the patch is generally the last rc1. Any dependencies
should be noted.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring Oct. 28, 2020, 2:42 p.m. UTC | #2
On Tue, Oct 27, 2020 at 02:00:10PM +0800, Wan Ahmad Zainie wrote:
> Document DT bindings for PCIe controller found on Intel Keem Bay SoC.
> 
> Signed-off-by: Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> ---
>  .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
>  .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
>  2 files changed, 206 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
>  create mode 100644 Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> new file mode 100644
> index 000000000000..11962c205744
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> @@ -0,0 +1,86 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie-ep.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Intel Keem Bay PCIe EP controller
> +
> +maintainers:
> +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> +
> +properties:
> +  compatible:
> +      const: intel,keembay-pcie-ep
> +
> +  reg:
> +    items:
> +      - description: DesignWare PCIe registers
> +      - description: PCIe configuration space
> +      - description: Keem Bay specific registers
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: addr_space
> +      - const: apb
> +
> +  interrupts:
> +    items:
> +      - description: PCIe interrupt
> +      - description: PCIe event interrupt
> +      - description: PCIe error interrupt
> +      - description: PCIe memory access interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: intr
> +      - const: ev_intr
> +      - const: err_intr
> +      - const: mem_access_intr

'_intr' is redundant. Drop it. You'll need a better name for the first 
one though.

> +
> +  num-ib-windows:
> +    description: Number of inbound address translation windows
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  num-ob-windows:
> +    description: Number of outbound address translation windows
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  num-lanes:
> +    description: Number of lanes to use.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 1, 2, 4, 8 ]
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - num-ib-windows
> +  - num-ob-windows
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +  - |
> +    pcie-ep@37000000 {
> +          compatible = "intel,keembay-pcie-ep";
> +          reg = <0x37000000 0x00800000>,
> +                <0x36000000 0x01000000>,
> +                <0x37800000 0x00000200>;
> +          reg-names = "dbi", "addr_space", "apb";
> +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 108 IRQ_TYPE_EDGE_RISING>,
> +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> +          interrupt-names = "intr", "ev_intr", "err_intr",
> +                       "mem_access_intr";
> +          num-ib-windows = <4>;
> +          num-ob-windows = <4>;
> +          num-lanes = <2>;
> +    };
> diff --git a/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> new file mode 100644
> index 000000000000..49e5d3d35bd4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> @@ -0,0 +1,120 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Intel Keem Bay PCIe RC controller
> +
> +maintainers:
> +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +properties:
> +  compatible:
> +      const: intel,keembay-pcie

> +
> +  device_type:
> +    const: pci
> +
> +  "#address-cells":
> +    const: 3
> +
> +  "#size-cells":
> +    const: 2

Can drop these 3 as pci-bus.yaml defines them.

> +
> +  ranges:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  reg:
> +    items:
> +      - description: DesignWare PCIe registers
> +      - description: PCIe configuration space
> +      - description: Keem Bay specific registers
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: config
> +      - const: apb
> +
> +  clocks:
> +    items:
> +      - description: bus clock
> +      - description: auxiliary clock

The EP doesn't have clocks? You should have roughly the same resources 
for RC and EP modes.

> +
> +  clock-names:
> +    items:
> +      - const: master
> +      - const: aux
> +
> +  interrupts:
> +    items:
> +      - description: PCIe interrupt
> +      - description: PCIe event interrupt
> +      - description: PCIe error interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: intr
> +      - const: ev_intr
> +      - const: err_intr
> +
> +  num-lanes:
> +    description: Number of lanes to use.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 1, 2, 4, 8 ]
> +
> +  num-viewport:
> +    description: Number of view ports configured in hardware.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 2

Pretty sure it's not 2 if num-ib-windows and num-ob-windows are 4.

> +
> +required:
> +  - compatible

> +  - device_type
> +  - "#address-cells"
> +  - "#size-cells"

Can drop these too.

> +  - reg
> +  - reg-names
> +  - ranges
> +  - clocks
> +  - interrupts
> +  - interrupt-names
> +  - reset-gpios
> +
> +additionalProperties: false

Use 'unevaluatedProperties: false' instead.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    #define KEEM_BAY_A53_PCIE
> +    #define KEEM_BAY_A53_AUX_PCIE
> +    pcie@37000000 {
> +          compatible = "intel,keembay-pcie";
> +          reg = <0x37000000 0x00800000>,
> +                <0x36e00000 0x00200000>,
> +                <0x37800000 0x00000200>;
> +          reg-names = "dbi", "config", "apb";
> +          #address-cells = <3>;
> +          #size-cells = <2>;
> +          device_type = "pci";
> +          ranges = <0x02000000 0 0x36000000 0x36000000 0 0x00e00000>;
> +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
> +          interrupt-names = "intr", "ev_intr", "err_intr";
> +          clocks = <&scmi_clk KEEM_BAY_A53_PCIE>,
> +                   <&scmi_clk KEEM_BAY_A53_AUX_PCIE>;
> +          clock-names = "master", "aux";
> +          reset-gpios = <&pca2 9 GPIO_ACTIVE_LOW>;
> +          num-viewport = <4>;
> +          num-lanes = <2>;
> +    };
> -- 
> 2.17.1
>
Wan Mohamad, Wan Ahmad Zainie Oct. 30, 2020, 1:04 p.m. UTC | #3
Hi Rob.

Thanks for the review.

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, October 28, 2020 10:42 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> pci@vger.kernel.org; devicetree@vger.kernel.org;
> andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller
> 
> On Tue, Oct 27, 2020 at 02:00:10PM +0800, Wan Ahmad Zainie wrote:
> > Document DT bindings for PCIe controller found on Intel Keem Bay SoC.
> >
> > Signed-off-by: Wan Ahmad Zainie
> > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > ---
> >  .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
> >  .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
> >  2 files changed, 206 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> >  create mode 100644
> > Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > new file mode 100644
> > index 000000000000..11962c205744
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-
> ep.yaml
> > @@ -0,0 +1,86 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie-ep.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Intel Keem Bay PCIe EP controller
> > +
> > +maintainers:
> > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > +
> > +properties:
> > +  compatible:
> > +      const: intel,keembay-pcie-ep

Fixed in v2, wrong indentation as per report.

> > +
> > +  reg:
> > +    items:
> > +      - description: DesignWare PCIe registers
> > +      - description: PCIe configuration space
> > +      - description: Keem Bay specific registers
> > +
> > +  reg-names:
> > +    items:
> > +      - const: dbi
> > +      - const: addr_space
> > +      - const: apb
> > +
> > +  interrupts:
> > +    items:
> > +      - description: PCIe interrupt
> > +      - description: PCIe event interrupt
> > +      - description: PCIe error interrupt
> > +      - description: PCIe memory access interrupt
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: intr
> > +      - const: ev_intr
> > +      - const: err_intr
> > +      - const: mem_access_intr
> 
> '_intr' is redundant. Drop it. You'll need a better name for the first one
> though.

I will drop _intr in v2.
I will send out once I get suitable name from Keem Bay data book.

> 
> > +
> > +  num-ib-windows:
> > +    description: Number of inbound address translation windows
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  num-ob-windows:
> > +    description: Number of outbound address translation windows
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  num-lanes:
> > +    description: Number of lanes to use.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [ 1, 2, 4, 8 ]
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - num-ib-windows
> > +  - num-ob-windows
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +  - |
> > +    pcie-ep@37000000 {
> > +          compatible = "intel,keembay-pcie-ep";
> > +          reg = <0x37000000 0x00800000>,
> > +                <0x36000000 0x01000000>,
> > +                <0x37800000 0x00000200>;
> > +          reg-names = "dbi", "addr_space", "apb";
> > +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 108 IRQ_TYPE_EDGE_RISING>,
> > +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> > +          interrupt-names = "intr", "ev_intr", "err_intr",
> > +                       "mem_access_intr";
> > +          num-ib-windows = <4>;
> > +          num-ob-windows = <4>;
> > +          num-lanes = <2>;
> > +    };
> > diff --git
> > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > new file mode 100644
> > index 000000000000..49e5d3d35bd4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > @@ -0,0 +1,120 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Intel Keem Bay PCIe RC controller
> > +
> > +maintainers:
> > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +
> > +properties:
> > +  compatible:
> > +      const: intel,keembay-pcie

Wrong indentation as per report.
I will fix in v2.

> 
> > +
> > +  device_type:
> > +    const: pci
> > +
> > +  "#address-cells":
> > +    const: 3
> > +
> > +  "#size-cells":
> > +    const: 2
> 
> Can drop these 3 as pci-bus.yaml defines them.

I will drop these 3 in v2.

> 
> > +
> > +  ranges:
> > +    maxItems: 1
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +
> > +  reg:
> > +    items:
> > +      - description: DesignWare PCIe registers
> > +      - description: PCIe configuration space
> > +      - description: Keem Bay specific registers
> > +
> > +  reg-names:
> > +    items:
> > +      - const: dbi
> > +      - const: config
> > +      - const: apb
> > +
> > +  clocks:
> > +    items:
> > +      - description: bus clock
> > +      - description: auxiliary clock
> 
> The EP doesn't have clocks? You should have roughly the same resources for
> RC and EP modes.

For Keem Bay, EP mode link initialization is done in boot firmware.
This include setup the clocks.
That's why I do not include clocks for EP.

> 
> > +
> > +  clock-names:
> > +    items:
> > +      - const: master
> > +      - const: aux
> > +
> > +  interrupts:
> > +    items:
> > +      - description: PCIe interrupt
> > +      - description: PCIe event interrupt
> > +      - description: PCIe error interrupt
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: intr
> > +      - const: ev_intr
> > +      - const: err_intr
> > +
> > +  num-lanes:
> > +    description: Number of lanes to use.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [ 1, 2, 4, 8 ]
> > +
> > +  num-viewport:
> > +    description: Number of view ports configured in hardware.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 2
> 
> Pretty sure it's not 2 if num-ib-windows and num-ob-windows are 4.

As per pcie-designware-host.c, default value is 2, if it is not set.
My example and the DT in my system is 4.
I will fix in v2, by using const: 4.
Should I drop default?

> 
> > +
> > +required:
> > +  - compatible
> 
> > +  - device_type
> > +  - "#address-cells"
> > +  - "#size-cells"
> 
> Can drop these too.

I will drop them in v2.

> 
> > +  - reg
> > +  - reg-names
> > +  - ranges
> > +  - clocks
> > +  - interrupts
> > +  - interrupt-names
> > +  - reset-gpios
> > +
> > +additionalProperties: false
> 
> Use 'unevaluatedProperties: false' instead.

I will fix in v2.

> 
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #define KEEM_BAY_A53_PCIE
> > +    #define KEEM_BAY_A53_AUX_PCIE
> > +    pcie@37000000 {
> > +          compatible = "intel,keembay-pcie";
> > +          reg = <0x37000000 0x00800000>,
> > +                <0x36e00000 0x00200000>,
> > +                <0x37800000 0x00000200>;
> > +          reg-names = "dbi", "config", "apb";
> > +          #address-cells = <3>;
> > +          #size-cells = <2>;
> > +          device_type = "pci";
> > +          ranges = <0x02000000 0 0x36000000 0x36000000 0 0x00e00000>;
> > +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
> > +          interrupt-names = "intr", "ev_intr", "err_intr";
> > +          clocks = <&scmi_clk KEEM_BAY_A53_PCIE>,
> > +                   <&scmi_clk KEEM_BAY_A53_AUX_PCIE>;
> > +          clock-names = "master", "aux";
> > +          reset-gpios = <&pca2 9 GPIO_ACTIVE_LOW>;
> > +          num-viewport = <4>;
> > +          num-lanes = <2>;
> > +    };
> > --
> > 2.17.1
> >

Best regards,
Zainie
Rob Herring Oct. 30, 2020, 2:55 p.m. UTC | #4
On Fri, Oct 30, 2020 at 8:05 AM Wan Mohamad, Wan Ahmad Zainie
<wan.ahmad.zainie.wan.mohamad@intel.com> wrote:
>
> Hi Rob.
>
> Thanks for the review.
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Wednesday, October 28, 2020 10:42 PM
> > To: Wan Mohamad, Wan Ahmad Zainie
> > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> > pci@vger.kernel.org; devicetree@vger.kernel.org;
> > andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> > Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> > Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller
> >
> > On Tue, Oct 27, 2020 at 02:00:10PM +0800, Wan Ahmad Zainie wrote:
> > > Document DT bindings for PCIe controller found on Intel Keem Bay SoC.
> > >
> > > Signed-off-by: Wan Ahmad Zainie
> > > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > ---
> > >  .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
> > >  .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
> > >  2 files changed, 206 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > >  create mode 100644
> > > Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > > new file mode 100644
> > > index 000000000000..11962c205744
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-
> > ep.yaml
> > > @@ -0,0 +1,86 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie-ep.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Intel Keem Bay PCIe EP controller
> > > +
> > > +maintainers:
> > > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +      const: intel,keembay-pcie-ep
>
> Fixed in v2, wrong indentation as per report.
>
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: DesignWare PCIe registers
> > > +      - description: PCIe configuration space
> > > +      - description: Keem Bay specific registers
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: dbi
> > > +      - const: addr_space
> > > +      - const: apb
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: PCIe interrupt
> > > +      - description: PCIe event interrupt
> > > +      - description: PCIe error interrupt
> > > +      - description: PCIe memory access interrupt
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: intr
> > > +      - const: ev_intr
> > > +      - const: err_intr
> > > +      - const: mem_access_intr
> >
> > '_intr' is redundant. Drop it. You'll need a better name for the first one
> > though.
>
> I will drop _intr in v2.
> I will send out once I get suitable name from Keem Bay data book.
>
> >
> > > +
> > > +  num-ib-windows:
> > > +    description: Number of inbound address translation windows
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  num-ob-windows:
> > > +    description: Number of outbound address translation windows
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  num-lanes:
> > > +    description: Number of lanes to use.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [ 1, 2, 4, 8 ]
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - reg-names
> > > +  - interrupts
> > > +  - interrupt-names
> > > +  - num-ib-windows
> > > +  - num-ob-windows
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +  - |
> > > +    pcie-ep@37000000 {
> > > +          compatible = "intel,keembay-pcie-ep";
> > > +          reg = <0x37000000 0x00800000>,
> > > +                <0x36000000 0x01000000>,
> > > +                <0x37800000 0x00000200>;
> > > +          reg-names = "dbi", "addr_space", "apb";
> > > +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> > > +                       <GIC_SPI 108 IRQ_TYPE_EDGE_RISING>,
> > > +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
> > > +                       <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> > > +          interrupt-names = "intr", "ev_intr", "err_intr",
> > > +                       "mem_access_intr";
> > > +          num-ib-windows = <4>;
> > > +          num-ob-windows = <4>;
> > > +          num-lanes = <2>;
> > > +    };
> > > diff --git
> > > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > > new file mode 100644
> > > index 000000000000..49e5d3d35bd4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > > @@ -0,0 +1,120 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Intel Keem Bay PCIe RC controller
> > > +
> > > +maintainers:
> > > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/pci/pci-bus.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +      const: intel,keembay-pcie
>
> Wrong indentation as per report.
> I will fix in v2.
>
> >
> > > +
> > > +  device_type:
> > > +    const: pci
> > > +
> > > +  "#address-cells":
> > > +    const: 3
> > > +
> > > +  "#size-cells":
> > > +    const: 2
> >
> > Can drop these 3 as pci-bus.yaml defines them.
>
> I will drop these 3 in v2.
>
> >
> > > +
> > > +  ranges:
> > > +    maxItems: 1
> > > +
> > > +  reset-gpios:
> > > +    maxItems: 1
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: DesignWare PCIe registers
> > > +      - description: PCIe configuration space
> > > +      - description: Keem Bay specific registers
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: dbi
> > > +      - const: config
> > > +      - const: apb
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: bus clock
> > > +      - description: auxiliary clock
> >
> > The EP doesn't have clocks? You should have roughly the same resources for
> > RC and EP modes.
>
> For Keem Bay, EP mode link initialization is done in boot firmware.
> This include setup the clocks.
> That's why I do not include clocks for EP.
>
> >
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: master
> > > +      - const: aux
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: PCIe interrupt
> > > +      - description: PCIe event interrupt
> > > +      - description: PCIe error interrupt
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: intr
> > > +      - const: ev_intr
> > > +      - const: err_intr
> > > +
> > > +  num-lanes:
> > > +    description: Number of lanes to use.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [ 1, 2, 4, 8 ]
> > > +
> > > +  num-viewport:
> > > +    description: Number of view ports configured in hardware.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    default: 2
> >
> > Pretty sure it's not 2 if num-ib-windows and num-ob-windows are 4.
>
> As per pcie-designware-host.c, default value is 2, if it is not set.

Yes, that's true.

> My example and the DT in my system is 4.
> I will fix in v2, by using const: 4.
> Should I drop default?

Yes.

BTW, I'm going to make all 3 properties obsolete. I'm working on a
patch to detect all this. It's pretty straight-forward, just see how
many registers are writable. The WIP patch is on my for-kernelci
branch.

The problem with these properties is they are defined as RC and EP
specific, but they are really fixed h/w config independent of the
mode. And num-viewport is incomplete because the inbound and outbound
sizes are independent. The driver just currently doesn't use inbound
windows for RC mode. Also, the driver claims there can be up to 256
windows, but I'm not really sure that's right. There's 2 platforms
upstream (ls1088a and ls208xa) claiming 256 windows in DT, but testing
with the detection code indicates they only have 16 IB and 16 OB
windows. Perhaps if you have the DWC manual you could confirm what's
possible.

Rob
Wan Mohamad, Wan Ahmad Zainie Nov. 3, 2020, 6:01 a.m. UTC | #5
Hi Rob.

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, October 30, 2020 10:56 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com>
> Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> pci@vger.kernel.org; devicetree@vger.kernel.org;
> andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller
> 
> On Fri, Oct 30, 2020 at 8:05 AM Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@intel.com> wrote:
> >
> > Hi Rob.
> >
> > Thanks for the review.
> >
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Wednesday, October 28, 2020 10:42 PM
> > > To: Wan Mohamad, Wan Ahmad Zainie
> > > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> > > pci@vger.kernel.org; devicetree@vger.kernel.org;
> > > andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> > > Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> > > Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe
> > > controller
> > >
> > > On Tue, Oct 27, 2020 at 02:00:10PM +0800, Wan Ahmad Zainie wrote:

...

> > > > +  num-viewport:
> > > > +    description: Number of view ports configured in hardware.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    default: 2
> > >
> > > Pretty sure it's not 2 if num-ib-windows and num-ob-windows are 4.
> >
> > As per pcie-designware-host.c, default value is 2, if it is not set.
> 
> Yes, that's true.
> 
> > My example and the DT in my system is 4.
> > I will fix in v2, by using const: 4.
> > Should I drop default?
> 
> Yes.
> 
> BTW, I'm going to make all 3 properties obsolete. I'm working on a patch to
> detect all this. It's pretty straight-forward, just see how many registers are
> writable. The WIP patch is on my for-kernelci branch.

I will take note on this.

I also take a look into for-kernelci branch. I will spend some time to try it
out with my patch.

> 
> The problem with these properties is they are defined as RC and EP specific,
> but they are really fixed h/w config independent of the mode. And num-
> viewport is incomplete because the inbound and outbound sizes are
> independent. The driver just currently doesn't use inbound windows for RC
> mode. Also, the driver claims there can be up to 256 windows, but I'm not
> really sure that's right. There's 2 platforms upstream (ls1088a and ls208xa)
> claiming 256 windows in DT, but testing with the detection code indicates
> they only have 16 IB and 16 OB windows. Perhaps if you have the DWC
> manual you could confirm what's possible.

Unfortunately, I don't have details on DWC manual. As for Keem Bay,
from the information in its databook, it is synthesized with 8 IB and 8 OB
windows. The values that I used for DT is based on recommendation from
our boot firmware team.

> 
> Rob

Best regards,
Zainie
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
new file mode 100644
index 000000000000..11962c205744
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
@@ -0,0 +1,86 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie-ep.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Intel Keem Bay PCIe EP controller
+
+maintainers:
+  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
+
+properties:
+  compatible:
+      const: intel,keembay-pcie-ep
+
+  reg:
+    items:
+      - description: DesignWare PCIe registers
+      - description: PCIe configuration space
+      - description: Keem Bay specific registers
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: addr_space
+      - const: apb
+
+  interrupts:
+    items:
+      - description: PCIe interrupt
+      - description: PCIe event interrupt
+      - description: PCIe error interrupt
+      - description: PCIe memory access interrupt
+
+  interrupt-names:
+    items:
+      - const: intr
+      - const: ev_intr
+      - const: err_intr
+      - const: mem_access_intr
+
+  num-ib-windows:
+    description: Number of inbound address translation windows
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  num-ob-windows:
+    description: Number of outbound address translation windows
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  num-lanes:
+    description: Number of lanes to use.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 1, 2, 4, 8 ]
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - num-ib-windows
+  - num-ob-windows
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+  - |
+    pcie-ep@37000000 {
+          compatible = "intel,keembay-pcie-ep";
+          reg = <0x37000000 0x00800000>,
+                <0x36000000 0x01000000>,
+                <0x37800000 0x00000200>;
+          reg-names = "dbi", "addr_space", "apb";
+          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 108 IRQ_TYPE_EDGE_RISING>,
+                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+          interrupt-names = "intr", "ev_intr", "err_intr",
+                       "mem_access_intr";
+          num-ib-windows = <4>;
+          num-ob-windows = <4>;
+          num-lanes = <2>;
+    };
diff --git a/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
new file mode 100644
index 000000000000..49e5d3d35bd4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
@@ -0,0 +1,120 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Intel Keem Bay PCIe RC controller
+
+maintainers:
+  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+      const: intel,keembay-pcie
+
+  device_type:
+    const: pci
+
+  "#address-cells":
+    const: 3
+
+  "#size-cells":
+    const: 2
+
+  ranges:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  reg:
+    items:
+      - description: DesignWare PCIe registers
+      - description: PCIe configuration space
+      - description: Keem Bay specific registers
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: config
+      - const: apb
+
+  clocks:
+    items:
+      - description: bus clock
+      - description: auxiliary clock
+
+  clock-names:
+    items:
+      - const: master
+      - const: aux
+
+  interrupts:
+    items:
+      - description: PCIe interrupt
+      - description: PCIe event interrupt
+      - description: PCIe error interrupt
+
+  interrupt-names:
+    items:
+      - const: intr
+      - const: ev_intr
+      - const: err_intr
+
+  num-lanes:
+    description: Number of lanes to use.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 1, 2, 4, 8 ]
+
+  num-viewport:
+    description: Number of view ports configured in hardware.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 2
+
+required:
+  - compatible
+  - device_type
+  - "#address-cells"
+  - "#size-cells"
+  - reg
+  - reg-names
+  - ranges
+  - clocks
+  - interrupts
+  - interrupt-names
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #define KEEM_BAY_A53_PCIE
+    #define KEEM_BAY_A53_AUX_PCIE
+    pcie@37000000 {
+          compatible = "intel,keembay-pcie";
+          reg = <0x37000000 0x00800000>,
+                <0x36e00000 0x00200000>,
+                <0x37800000 0x00000200>;
+          reg-names = "dbi", "config", "apb";
+          #address-cells = <3>;
+          #size-cells = <2>;
+          device_type = "pci";
+          ranges = <0x02000000 0 0x36000000 0x36000000 0 0x00e00000>;
+          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
+          interrupt-names = "intr", "ev_intr", "err_intr";
+          clocks = <&scmi_clk KEEM_BAY_A53_PCIE>,
+                   <&scmi_clk KEEM_BAY_A53_AUX_PCIE>;
+          clock-names = "master", "aux";
+          reset-gpios = <&pca2 9 GPIO_ACTIVE_LOW>;
+          num-viewport = <4>;
+          num-lanes = <2>;
+    };