Hi Rob. Thanks for the review. > -----Original Message----- > From: Rob Herring <robh@xxxxxxxxxx> > Sent: Wednesday, October 28, 2020 10:42 PM > To: Wan Mohamad, Wan Ahmad Zainie > <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx> > Cc: bhelgaas@xxxxxxxxxx; lorenzo.pieralisi@xxxxxxx; linux- > pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > andriy.shevchenko@xxxxxxxxxxxxxxx; mgross@xxxxxxxxxxxxxxx; Raja > Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@xxxxxxxxx> > 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@xxxxxxxxx> > > --- > > .../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@xxxxxxxxx> > > + > > +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@xxxxxxxxx> > > + > > +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
