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

[Rob Herring <robh@kernel.org>]

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