| Message ID | fe9549470bc06ea0d0dfc80f46a579baa49b911a.1567585181.git.eswara.kota@linux.intel.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | PCI: Add Intel PCIe Driver and respective dt-binding yaml file | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
Hi Dilip, On 9/4/2019 6:10 PM, Dilip Kota wrote: > The Intel PCIe RC controller is Synopsys Designware > based PCIe core. Add YAML schemas for PCIe in RC mode > present in Intel Universal Gateway soc. > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > changes on v3: > Add the appropriate License-Identifier > Rename intel,rst-interval to 'reset-assert-us' rst->interval to reset-assert-ms(should be typo error) > Add additionalProperties: false > Rename phy-names to 'pciephy' > Remove the dtsi node split of SoC and board in the example > Add #interrupt-cells = <1>; or else interrupt parsing will fail > Name yaml file with compatible name > > .../devicetree/bindings/pci/intel,lgm-pcie.yaml | 137 +++++++++++++++++++++ > 1 file changed, 137 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml > > diff --git a/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml > new file mode 100644 > index 000000000000..5e5cc7fd66cd > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml > @@ -0,0 +1,137 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/intel-pcie.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Intel AXI bus based PCI express root complex > + > +maintainers: > + - Dilip Kota <eswara.kota@linux.intel.com> > + > +properties: > + compatible: > + const: intel,lgm-pcie > + > + device_type: > + const: pci > + > + "#address-cells": > + const: 3 > + > + "#size-cells": > + const: 2 > + > + reg: > + items: > + - description: Controller control and status registers. > + - description: PCIe configuration registers. > + - description: Controller application registers. > + > + reg-names: > + items: > + - const: dbi > + - const: config > + - const: app > + > + ranges: > + description: Ranges for the PCI memory and I/O regions. > + > + resets: > + maxItems: 1 > + > + clocks: > + description: PCIe registers interface clock. > + > + phys: > + maxItems: 1 > + > + phy-names: > + const: pciephy > + > + reset-gpios: > + maxItems: 1 > + > + num-lanes: > + description: Number of lanes to use for this port. > + > + linux,pci-domain: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: PCI domain ID. > + > + interrupts: > + description: PCIe core integrated miscellaneous interrupt. > + > + '#interrupt-cells': > + const: 1 > + > + interrupt-map-mask: > + description: Standard PCI IRQ mapping properties. > + > + interrupt-map: > + description: Standard PCI IRQ mapping properties. > + > + max-link-speed: > + description: Specify PCI Gen for link capability. > + > + bus-range: > + description: Range of bus numbers associated with this controller. > + > + reset-assert-ms: > + description: | > + Device reset interval in ms. > + Some devices need an interval upto 500ms. By default it is 100ms. > + > +required: > + - compatible > + - device_type > + - reg > + - reg-names > + - ranges > + - resets > + - clocks > + - phys > + - phy-names > + - reset-gpios > + - num-lanes > + - linux,pci-domain > + - interrupts > + - interrupt-map > + - interrupt-map-mask > + > +additionalProperties: false > + > +examples: > + - | > + pcie10:pcie@d0e00000 { > + compatible = "intel,lgm-pcie"; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + reg = < > + 0xd0e00000 0x1000 > + 0xd2000000 0x800000 > + 0xd0a41000 0x1000 > + >; > + reg-names = "dbi", "config", "app"; > + linux,pci-domain = <0>; > + max-link-speed = <4>; > + bus-range = <0x00 0x08>; > + interrupt-parent = <&ioapic1>; > + interrupts = <67 1>; > + #interrupt-cells = <1>; > + interrupt-map-mask = <0 0 0 0x7>; > + interrupt-map = <0 0 0 1 &ioapic1 27 1>, > + <0 0 0 2 &ioapic1 28 1>, > + <0 0 0 3 &ioapic1 29 1>, > + <0 0 0 4 &ioapic1 30 1>; > + ranges = <0x02000000 0 0xd4000000 0xd4000000 0 0x04000000>; > + resets = <&rcu0 0x50 0>; > + clocks = <&cgu0 LGM_GCLK_PCIE10>; > + phys = <&cb0phy0>; > + phy-names = "pciephy"; > + status = "okay"; > + reset-assert-ms = <500>; > + reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>; > + num-lanes = <2>; > + };
Hi Dilip, On Wed, Sep 4, 2019 at 12:11 PM Dilip Kota <eswara.kota@linux.intel.com> wrote: [...] > +properties: > + compatible: > + const: intel,lgm-pcie should we add the "snps,dw-pcie" here (and in the example below) as well? (this is what for example Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt does) [...] > + phy-names: > + const: pciephy the most popular choice in Documentation/devicetree/bindings/pci/ is "pcie-phy" if Rob is happy with "pciephy" (which is already part of two other bindings) then I'm happy with "pciephy" as well > + num-lanes: > + description: Number of lanes to use for this port. are there SoCs with more than 2 lanes? you can list the allowed values in an enum so "num-lanes = <16>" causes an error when someone accidentally has this in their .dts (and runs the dt-bindings validation) [...] > + reset-assert-ms: maybe add: $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + Device reset interval in ms. > + Some devices need an interval upto 500ms. By default it is 100ms. > + > +required: > + - compatible > + - device_type > + - reg > + - reg-names > + - ranges > + - resets > + - clocks > + - phys > + - phy-names > + - reset-gpios > + - num-lanes > + - linux,pci-domain > + - interrupts > + - interrupt-map > + - interrupt-map-mask > + > +additionalProperties: false > + > +examples: > + - | > + pcie10:pcie@d0e00000 { > + compatible = "intel,lgm-pcie"; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + reg = < > + 0xd0e00000 0x1000 > + 0xd2000000 0x800000 > + 0xd0a41000 0x1000 > + >; > + reg-names = "dbi", "config", "app"; > + linux,pci-domain = <0>; > + max-link-speed = <4>; > + bus-range = <0x00 0x08>; > + interrupt-parent = <&ioapic1>; > + interrupts = <67 1>; > + #interrupt-cells = <1>; > + interrupt-map-mask = <0 0 0 0x7>; > + interrupt-map = <0 0 0 1 &ioapic1 27 1>, > + <0 0 0 2 &ioapic1 28 1>, > + <0 0 0 3 &ioapic1 29 1>, > + <0 0 0 4 &ioapic1 30 1>; is the "1" in the interrupts and interrupt-map properties IRQ_TYPE_EDGE_RISING? you can use these macros in this example as well, see Documentation/devicetree/bindings/iio/accel/adi,adxl372.yaml for example Martin
On 9/6/2019 4:31 AM, Martin Blumenstingl wrote: > Hi Dilip, > > On Wed, Sep 4, 2019 at 12:11 PM Dilip Kota <eswara.kota@linux.intel.com> wrote: > [...] >> +properties: >> + compatible: >> + const: intel,lgm-pcie > should we add the "snps,dw-pcie" here (and in the example below) as well? > (this is what for example > Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt does) Thanks for pointing out this. We should add this. > > [...] >> + phy-names: >> + const: pciephy > the most popular choice in Documentation/devicetree/bindings/pci/ is "pcie-phy" > if Rob is happy with "pciephy" (which is already part of two other > bindings) then I'm happy with "pciephy" as well Agree. > >> + num-lanes: >> + description: Number of lanes to use for this port. > are there SoCs with more than 2 lanes? > you can list the allowed values in an enum so "num-lanes = <16>" > causes an error when someone accidentally has this in their .dts (and > runs the dt-bindings validation) Our SoC(LGM) supports single lane or dual lane. Again this also depends on the board. I wonder if we should put this into board specific dts. To make multiple lanes work properly, it also depends on the phy mode. In my internal version, I put it into board dts. > > [...] >> + reset-assert-ms: > maybe add: > $ref: /schemas/types.yaml#/definitions/uint32 Agree >> + description: | >> + Device reset interval in ms. >> + Some devices need an interval upto 500ms. By default it is 100ms. >> + >> +required: >> + - compatible >> + - device_type >> + - reg >> + - reg-names >> + - ranges >> + - resets >> + - clocks >> + - phys >> + - phy-names >> + - reset-gpios >> + - num-lanes >> + - linux,pci-domain >> + - interrupts >> + - interrupt-map >> + - interrupt-map-mask >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + pcie10:pcie@d0e00000 { >> + compatible = "intel,lgm-pcie"; >> + device_type = "pci"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + reg = < >> + 0xd0e00000 0x1000 >> + 0xd2000000 0x800000 >> + 0xd0a41000 0x1000 >> + >; >> + reg-names = "dbi", "config", "app"; >> + linux,pci-domain = <0>; >> + max-link-speed = <4>; >> + bus-range = <0x00 0x08>; >> + interrupt-parent = <&ioapic1>; >> + interrupts = <67 1>; >> + #interrupt-cells = <1>; >> + interrupt-map-mask = <0 0 0 0x7>; >> + interrupt-map = <0 0 0 1 &ioapic1 27 1>, >> + <0 0 0 2 &ioapic1 28 1>, >> + <0 0 0 3 &ioapic1 29 1>, >> + <0 0 0 4 &ioapic1 30 1>; > is the "1" in the interrupts and interrupt-map properties IRQ_TYPE_EDGE_RISING? > you can use these macros in this example as well, see > Documentation/devicetree/bindings/iio/accel/adi,adxl372.yaml for > example No. 1 here means index from arch/x86/devicetree.c static struct of_ioapic_type of_ioapic_type[] = { { .out_type = IRQ_TYPE_EDGE_RISING, .trigger = IOAPIC_EDGE, .polarity = 1, }, { .out_type = IRQ_TYPE_LEVEL_LOW, .trigger = IOAPIC_LEVEL, .polarity = 0, }, { .out_type = IRQ_TYPE_LEVEL_HIGH, .trigger = IOAPIC_LEVEL, .polarity = 1, }, { .out_type = IRQ_TYPE_EDGE_FALLING, .trigger = IOAPIC_EDGE, .polarity = 0, }, }; static int dt_irqdomain_alloc(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs, void *arg) { struct irq_fwspec *fwspec = (struct irq_fwspec *)arg; struct of_ioapic_type *it; struct irq_alloc_info tmp; int type_index; if (WARN_ON(fwspec->param_count < 2)) return -EINVAL; type_index = fwspec->param[1]; // index. if (type_index >= ARRAY_SIZE(of_ioapic_type)) return -EINVAL; I would not see this definition is user-friendly. But it is how x86 handles at the moment. > > Martin
On Thu, Sep 05, 2019 at 10:31:29PM +0200, Martin Blumenstingl wrote: > On Wed, Sep 4, 2019 at 12:11 PM Dilip Kota <eswara.kota@linux.intel.com> wrote: > > + phy-names: > > + const: pciephy > the most popular choice in Documentation/devicetree/bindings/pci/ is "pcie-phy" > if Rob is happy with "pciephy" (which is already part of two other > bindings) then I'm happy with "pciephy" as well I'm not Rob, though I consider more practical to have a hyphenated variant.
Hi Chuan Hua, On 9/5/2019 10:23 AM, Chuan Hua, Lei wrote: > Hi Dilip, > > On 9/4/2019 6:10 PM, Dilip Kota wrote: >> The Intel PCIe RC controller is Synopsys Designware >> based PCIe core. Add YAML schemas for PCIe in RC mode >> present in Intel Universal Gateway soc. >> >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> changes on v3: >> Add the appropriate License-Identifier >> Rename intel,rst-interval to 'reset-assert-us' > rst->interval to reset-assert-ms(should be typo error) Sure, i will fix it. That's a typo error. Thanks for pointing it. Regards Dilip
On Fri, Sep 6, 2019 at 5:22 AM Chuan Hua, Lei <chuanhua.lei@linux.intel.com> wrote: [...] > >> +examples: > >> + - | > >> + pcie10:pcie@d0e00000 { > >> + compatible = "intel,lgm-pcie"; > >> + device_type = "pci"; > >> + #address-cells = <3>; > >> + #size-cells = <2>; > >> + reg = < > >> + 0xd0e00000 0x1000 > >> + 0xd2000000 0x800000 > >> + 0xd0a41000 0x1000 > >> + >; > >> + reg-names = "dbi", "config", "app"; > >> + linux,pci-domain = <0>; > >> + max-link-speed = <4>; > >> + bus-range = <0x00 0x08>; > >> + interrupt-parent = <&ioapic1>; > >> + interrupts = <67 1>; > >> + #interrupt-cells = <1>; > >> + interrupt-map-mask = <0 0 0 0x7>; > >> + interrupt-map = <0 0 0 1 &ioapic1 27 1>, > >> + <0 0 0 2 &ioapic1 28 1>, > >> + <0 0 0 3 &ioapic1 29 1>, > >> + <0 0 0 4 &ioapic1 30 1>; > > is the "1" in the interrupts and interrupt-map properties IRQ_TYPE_EDGE_RISING? > > you can use these macros in this example as well, see > > Documentation/devicetree/bindings/iio/accel/adi,adxl372.yaml for > > example > > No. 1 here means index from arch/x86/devicetree.c > > static struct of_ioapic_type of_ioapic_type[] = > { > { > .out_type = IRQ_TYPE_EDGE_RISING, > .trigger = IOAPIC_EDGE, > .polarity = 1, > }, > { > .out_type = IRQ_TYPE_LEVEL_LOW, > .trigger = IOAPIC_LEVEL, > .polarity = 0, > }, > { > .out_type = IRQ_TYPE_LEVEL_HIGH, > .trigger = IOAPIC_LEVEL, > .polarity = 1, > }, > { > .out_type = IRQ_TYPE_EDGE_FALLING, > .trigger = IOAPIC_EDGE, > .polarity = 0, > }, > }; > > static int dt_irqdomain_alloc(struct irq_domain *domain, unsigned int virq, > unsigned int nr_irqs, void *arg) > { > struct irq_fwspec *fwspec = (struct irq_fwspec *)arg; > struct of_ioapic_type *it; > struct irq_alloc_info tmp; > int type_index; > > if (WARN_ON(fwspec->param_count < 2)) > return -EINVAL; > > type_index = fwspec->param[1]; // index. > if (type_index >= ARRAY_SIZE(of_ioapic_type)) > return -EINVAL; > > I would not see this definition is user-friendly. But it is how x86 > handles at the moment. thank you for explaining this - I had no idea x86 is different from all other platforms I know the only upstream x86 .dts I could find (arch/x86/platform/ce4100/falconfalls.dts) also uses the magic x86 numbers so I'm fine with this until someone else knows a better solution Martin
On Fri, Sep 06, 2019 at 07:17:11PM +0200, Martin Blumenstingl wrote: > On Fri, Sep 6, 2019 at 5:22 AM Chuan Hua, Lei > <chuanhua.lei@linux.intel.com> wrote: > > type_index = fwspec->param[1]; // index. > > if (type_index >= ARRAY_SIZE(of_ioapic_type)) > > return -EINVAL; > > > > I would not see this definition is user-friendly. But it is how x86 > > handles at the moment. > thank you for explaining this - I had no idea x86 is different from > all other platforms I know > the only upstream x86 .dts I could find > (arch/x86/platform/ce4100/falconfalls.dts) also uses the magic x86 > numbers > so I'm fine with this until someone else knows a better solution Ivan, Cc'ed, had done few amendments to x86 DT support. Perhaps he may add something to the discussion.
On Fri, Sep 06, 2019 at 08:48:15PM +0300, Andy Shevchenko wrote: > On Fri, Sep 06, 2019 at 07:17:11PM +0200, Martin Blumenstingl wrote: > > On Fri, Sep 6, 2019 at 5:22 AM Chuan Hua, Lei > > <chuanhua.lei@linux.intel.com> wrote: > > > > type_index = fwspec->param[1]; // index. > > > if (type_index >= ARRAY_SIZE(of_ioapic_type)) > > > return -EINVAL; > > > > > > I would not see this definition is user-friendly. But it is how x86 > > > handles at the moment. > > thank you for explaining this - I had no idea x86 is different from > > all other platforms I know > > the only upstream x86 .dts I could find > > (arch/x86/platform/ce4100/falconfalls.dts) also uses the magic x86 > > numbers > > so I'm fine with this until someone else knows a better solution > > Ivan, Cc'ed, had done few amendments to x86 DT support. Perhaps he may add > something to the discussion. I just fixed broken interrupt support in x86-specific DT implementation. In CE4100, PCI devices are directly connected to I/O APIC input lines. Conventional PCI devices other than bridges don't need to be described in Device Tree or if they use standard PCI routing. Mapping INTA .. INTD pins to inputs of the bridge's interrupt parent depends on device number on the bus. In Device Tree, this mapping is described by "interrupt-map-mask" and "interrupt-map" properties of the bridge device node. Possible interrupt types described by Open Firmware Recomended Practice: 0 - Rising Edge 1 - Level triggered, active low
On 9/6/2019 5:19 PM, Andy Shevchenko wrote: > On Thu, Sep 05, 2019 at 10:31:29PM +0200, Martin Blumenstingl wrote: >> On Wed, Sep 4, 2019 at 12:11 PM Dilip Kota <eswara.kota@linux.intel.com> wrote: >>> + phy-names: >>> + const: pciephy >> the most popular choice in Documentation/devicetree/bindings/pci/ is "pcie-phy" >> if Rob is happy with "pciephy" (which is already part of two other >> bindings) then I'm happy with "pciephy" as well > I'm not Rob, though I consider more practical to have a hyphenated variant. I am okay to "pcie-phy". I will update it in next patch version. Regards, Dilip >
On Fri, Sep 06, 2019 at 12:19:50PM +0300, Andy Shevchenko wrote: > On Thu, Sep 05, 2019 at 10:31:29PM +0200, Martin Blumenstingl wrote: > > On Wed, Sep 4, 2019 at 12:11 PM Dilip Kota <eswara.kota@linux.intel.com> wrote: > > > > + phy-names: > > > + const: pciephy > > the most popular choice in Documentation/devicetree/bindings/pci/ is "pcie-phy" > > if Rob is happy with "pciephy" (which is already part of two other > > bindings) then I'm happy with "pciephy" as well > > I'm not Rob, though I consider more practical to have a hyphenated variant. *-names is kind of pointless when there is only one entry and 'foo' in the values of 'foo-names' is redundant. Rob
On Wed, Sep 04, 2019 at 06:10:30PM +0800, Dilip Kota wrote: > The Intel PCIe RC controller is Synopsys Designware > based PCIe core. Add YAML schemas for PCIe in RC mode > present in Intel Universal Gateway soc. > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > changes on v3: > Add the appropriate License-Identifier > Rename intel,rst-interval to 'reset-assert-us' > Add additionalProperties: false > Rename phy-names to 'pciephy' > Remove the dtsi node split of SoC and board in the example > Add #interrupt-cells = <1>; or else interrupt parsing will fail > Name yaml file with compatible name > > .../devicetree/bindings/pci/intel,lgm-pcie.yaml | 137 +++++++++++++++++++++ > 1 file changed, 137 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml > > diff --git a/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml > new file mode 100644 > index 000000000000..5e5cc7fd66cd > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml > @@ -0,0 +1,137 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/intel-pcie.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Intel AXI bus based PCI express root complex > + > +maintainers: > + - Dilip Kota <eswara.kota@linux.intel.com> > + > +properties: > + compatible: > + const: intel,lgm-pcie > + > + device_type: > + const: pci > + > + "#address-cells": > + const: 3 > + > + "#size-cells": > + const: 2 These all belong in a common schema. > + > + reg: > + items: > + - description: Controller control and status registers. > + - description: PCIe configuration registers. > + - description: Controller application registers. > + > + reg-names: > + items: > + - const: dbi > + - const: config > + - const: app > + > + ranges: > + description: Ranges for the PCI memory and I/O regions. And this. > + > + resets: > + maxItems: 1 > + > + clocks: > + description: PCIe registers interface clock. > + > + phys: > + maxItems: 1 > + > + phy-names: > + const: pciephy > + > + reset-gpios: > + maxItems: 1 > + > + num-lanes: > + description: Number of lanes to use for this port. > + > + linux,pci-domain: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: PCI domain ID. These 2 also should be common. > + > + interrupts: > + description: PCIe core integrated miscellaneous interrupt. How many? No need for description if there's only 1. > + > + '#interrupt-cells': > + const: 1 > + > + interrupt-map-mask: > + description: Standard PCI IRQ mapping properties. > + > + interrupt-map: > + description: Standard PCI IRQ mapping properties. > + > + max-link-speed: > + description: Specify PCI Gen for link capability. > + > + bus-range: > + description: Range of bus numbers associated with this controller. All common. > + > + reset-assert-ms: > + description: | > + Device reset interval in ms. > + Some devices need an interval upto 500ms. By default it is 100ms. This is a property of a device, so it belongs in a device node. How would you deal with this without DT? > + > +required: > + - compatible > + - device_type > + - reg > + - reg-names > + - ranges > + - resets > + - clocks > + - phys > + - phy-names > + - reset-gpios > + - num-lanes > + - linux,pci-domain > + - interrupts > + - interrupt-map > + - interrupt-map-mask > + > +additionalProperties: false > + > +examples: > + - | > + pcie10:pcie@d0e00000 { > + compatible = "intel,lgm-pcie"; > + device_type = "pci"; > + #address-cells = <3>; > + #size-cells = <2>; > + reg = < > + 0xd0e00000 0x1000 > + 0xd2000000 0x800000 > + 0xd0a41000 0x1000 > + >; > + reg-names = "dbi", "config", "app"; > + linux,pci-domain = <0>; > + max-link-speed = <4>; > + bus-range = <0x00 0x08>; > + interrupt-parent = <&ioapic1>; > + interrupts = <67 1>; > + #interrupt-cells = <1>; > + interrupt-map-mask = <0 0 0 0x7>; > + interrupt-map = <0 0 0 1 &ioapic1 27 1>, > + <0 0 0 2 &ioapic1 28 1>, > + <0 0 0 3 &ioapic1 29 1>, > + <0 0 0 4 &ioapic1 30 1>; > + ranges = <0x02000000 0 0xd4000000 0xd4000000 0 0x04000000>; > + resets = <&rcu0 0x50 0>; > + clocks = <&cgu0 LGM_GCLK_PCIE10>; > + phys = <&cb0phy0>; > + phy-names = "pciephy"; > + status = "okay"; > + reset-assert-ms = <500>; > + reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>; > + num-lanes = <2>; > + }; > -- > 2.11.0 >
Hi Rob, On 9/18/2019 2:33 AM, Rob Herring wrote: > On Fri, Sep 06, 2019 at 12:19:50PM +0300, Andy Shevchenko wrote: >> On Thu, Sep 05, 2019 at 10:31:29PM +0200, Martin Blumenstingl wrote: >>> On Wed, Sep 4, 2019 at 12:11 PM Dilip Kota <eswara.kota@linux.intel.com> wrote: >>>> + phy-names: >>>> + const: pciephy >>> the most popular choice in Documentation/devicetree/bindings/pci/ is "pcie-phy" >>> if Rob is happy with "pciephy" (which is already part of two other >>> bindings) then I'm happy with "pciephy" as well >> I'm not Rob, though I consider more practical to have a hyphenated variant. > *-names is kind of pointless when there is only one entry and 'foo' in > the values of 'foo-names' is redundant. Ok, i will change it to: phy-name: const: pcie Regards, Dilip > > Rob
On 9/18/2019 2:40 AM, Rob Herring wrote: > On Wed, Sep 04, 2019 at 06:10:30PM +0800, Dilip Kota wrote: >> The Intel PCIe RC controller is Synopsys Designware >> based PCIe core. Add YAML schemas for PCIe in RC mode >> present in Intel Universal Gateway soc. >> >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> changes on v3: >> Add the appropriate License-Identifier >> Rename intel,rst-interval to 'reset-assert-us' >> Add additionalProperties: false >> Rename phy-names to 'pciephy' >> Remove the dtsi node split of SoC and board in the example >> Add #interrupt-cells = <1>; or else interrupt parsing will fail >> Name yaml file with compatible name >> >> .../devicetree/bindings/pci/intel,lgm-pcie.yaml | 137 +++++++++++++++++++++ >> 1 file changed, 137 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml >> >> diff --git a/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml >> new file mode 100644 >> index 000000000000..5e5cc7fd66cd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml >> @@ -0,0 +1,137 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pci/intel-pcie.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Intel AXI bus based PCI express root complex >> + >> +maintainers: >> + - Dilip Kota <eswara.kota@linux.intel.com> >> + >> +properties: >> + compatible: >> + const: intel,lgm-pcie >> + >> + device_type: >> + const: pci >> + >> + "#address-cells": >> + const: 3 >> + >> + "#size-cells": >> + const: 2 > These all belong in a common schema. > >> + >> + reg: >> + items: >> + - description: Controller control and status registers. >> + - description: PCIe configuration registers. >> + - description: Controller application registers. >> + >> + reg-names: >> + items: >> + - const: dbi >> + - const: config >> + - const: app >> + >> + ranges: >> + description: Ranges for the PCI memory and I/O regions. > And this. > >> + >> + resets: >> + maxItems: 1 >> + >> + clocks: >> + description: PCIe registers interface clock. >> + >> + phys: >> + maxItems: 1 >> + >> + phy-names: >> + const: pciephy >> + >> + reset-gpios: >> + maxItems: 1 >> + >> + num-lanes: >> + description: Number of lanes to use for this port. >> + >> + linux,pci-domain: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: PCI domain ID. > These 2 also should be common. > >> + >> + interrupts: >> + description: PCIe core integrated miscellaneous interrupt. > How many? No need for description if there's only 1. > >> + >> + '#interrupt-cells': >> + const: 1 >> + >> + interrupt-map-mask: >> + description: Standard PCI IRQ mapping properties. >> + >> + interrupt-map: >> + description: Standard PCI IRQ mapping properties. >> + >> + max-link-speed: >> + description: Specify PCI Gen for link capability. >> + >> + bus-range: >> + description: Range of bus numbers associated with this controller. > All common. You mean to remove all the common schema entries description!. In most of the Documention/devicetree/binding/pci documents all the common entries are described so I followed the same. > >> + >> + reset-assert-ms: >> + description: | >> + Device reset interval in ms. >> + Some devices need an interval upto 500ms. By default it is 100ms. > This is a property of a device, so it belongs in a device node. How > would you deal with this without DT? This property is for the PCIe RC to keep a delay before notifying the reset to the device. If this entry is not present, PCIe driver will set a default value of 100ms. > >> + >> +required: >> + - compatible >> + - device_type >> + - reg >> + - reg-names >> + - ranges >> + - resets >> + - clocks >> + - phys >> + - phy-names >> + - reset-gpios >> + - num-lanes >> + - linux,pci-domain >> + - interrupts >> + - interrupt-map >> + - interrupt-map-mask >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + pcie10:pcie@d0e00000 { >> + compatible = "intel,lgm-pcie"; >> + device_type = "pci"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + reg = < >> + 0xd0e00000 0x1000 >> + 0xd2000000 0x800000 >> + 0xd0a41000 0x1000 >> + >; >> + reg-names = "dbi", "config", "app"; >> + linux,pci-domain = <0>; >> + max-link-speed = <4>; >> + bus-range = <0x00 0x08>; >> + interrupt-parent = <&ioapic1>; >> + interrupts = <67 1>; >> + #interrupt-cells = <1>; >> + interrupt-map-mask = <0 0 0 0x7>; >> + interrupt-map = <0 0 0 1 &ioapic1 27 1>, >> + <0 0 0 2 &ioapic1 28 1>, >> + <0 0 0 3 &ioapic1 29 1>, >> + <0 0 0 4 &ioapic1 30 1>; >> + ranges = <0x02000000 0 0xd4000000 0xd4000000 0 0x04000000>; >> + resets = <&rcu0 0x50 0>; >> + clocks = <&cgu0 LGM_GCLK_PCIE10>; >> + phys = <&cb0phy0>; >> + phy-names = "pciephy"; >> + status = "okay"; >> + reset-assert-ms = <500>; >> + reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>; >> + num-lanes = <2>; >> + }; >> -- >> 2.11.0 >>
Hi Rob, On 18/9/2019 2:56 PM, Dilip Kota wrote: > On 9/18/2019 2:40 AM, Rob Herring wrote: >> On Wed, Sep 04, 2019 at 06:10:30PM +0800, Dilip Kota wrote: >>> The Intel PCIe RC controller is Synopsys Designware >>> based PCIe core. Add YAML schemas for PCIe in RC mode >>> present in Intel Universal Gateway soc. >>> >>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >>> --- >>> changes on v3: >>> Add the appropriate License-Identifier >>> Rename intel,rst-interval to 'reset-assert-us' >>> Add additionalProperties: false >>> Rename phy-names to 'pciephy' >>> Remove the dtsi node split of SoC and board in the example >>> Add #interrupt-cells = <1>; or else interrupt parsing will fail >>> Name yaml file with compatible name >>> >>> .../devicetree/bindings/pci/intel,lgm-pcie.yaml | 137 >>> +++++++++++++++++++++ >>> 1 file changed, 137 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml >>> >>> diff --git >>> a/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml >>> b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml >>> new file mode 100644 >>> index 000000000000..5e5cc7fd66cd >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml >>> @@ -0,0 +1,137 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/pci/intel-pcie.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Intel AXI bus based PCI express root complex >>> + >>> +maintainers: >>> + - Dilip Kota <eswara.kota@linux.intel.com> >>> + >>> +properties: >>> + compatible: >>> + const: intel,lgm-pcie >>> + >>> + device_type: >>> + const: pci >>> + >>> + "#address-cells": >>> + const: 3 >>> + >>> + "#size-cells": >>> + const: 2 >> These all belong in a common schema. >> >>> + >>> + reg: >>> + items: >>> + - description: Controller control and status registers. >>> + - description: PCIe configuration registers. >>> + - description: Controller application registers. >>> + >>> + reg-names: >>> + items: >>> + - const: dbi >>> + - const: config >>> + - const: app >>> + >>> + ranges: >>> + description: Ranges for the PCI memory and I/O regions. >> And this. >> >>> + >>> + resets: >>> + maxItems: 1 >>> + >>> + clocks: >>> + description: PCIe registers interface clock. >>> + >>> + phys: >>> + maxItems: 1 >>> + >>> + phy-names: >>> + const: pciephy >>> + >>> + reset-gpios: >>> + maxItems: 1 >>> + >>> + num-lanes: >>> + description: Number of lanes to use for this port. >>> + >>> + linux,pci-domain: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: PCI domain ID. >> These 2 also should be common. >> >>> + >>> + interrupts: >>> + description: PCIe core integrated miscellaneous interrupt. >> How many? No need for description if there's only 1. >> >>> + >>> + '#interrupt-cells': >>> + const: 1 >>> + >>> + interrupt-map-mask: >>> + description: Standard PCI IRQ mapping properties. >>> + >>> + interrupt-map: >>> + description: Standard PCI IRQ mapping properties. >>> + >>> + max-link-speed: >>> + description: Specify PCI Gen for link capability. >>> + >>> + bus-range: >>> + description: Range of bus numbers associated with this controller. >> All common. > You mean to remove all the common schema entries description!. > In most of the Documention/devicetree/binding/pci documents all the > common entries are described so I followed the same. If common schemas are removed, getting below warning during the dt_binding_check. Documentation/devicetree/bindings/pci/intel,lgm-pcie.example.dt.yaml: pcie@d0e00000: '#address-cells', '#interrupt-cells', '#size-cells', 'bus-range', 'device_type', 'interrupt-map', 'interrupt-map-mask', 'interrupt-parent', 'linux,pci-domain', 'ranges', 'reset-gpios' do not match any of the regexes: 'pinctrl-[0-9]+' Regards, Dilip >> >>> + >>> + reset-assert-ms: >>> + description: | >>> + Device reset interval in ms. >>> + Some devices need an interval upto 500ms. By default it is 100ms. >> This is a property of a device, so it belongs in a device node. How >> would you deal with this without DT? > This property is for the PCIe RC to keep a delay before notifying the > reset to the device. > If this entry is not present, PCIe driver will set a default value of > 100ms. >> >>> + >>> +required: >>> + - compatible >>> + - device_type >>> + - reg >>> + - reg-names >>> + - ranges >>> + - resets >>> + - clocks >>> + - phys >>> + - phy-names >>> + - reset-gpios >>> + - num-lanes >>> + - linux,pci-domain >>> + - interrupts >>> + - interrupt-map >>> + - interrupt-map-mask >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + pcie10:pcie@d0e00000 { >>> + compatible = "intel,lgm-pcie"; >>> + device_type = "pci"; >>> + #address-cells = <3>; >>> + #size-cells = <2>; >>> + reg = < >>> + 0xd0e00000 0x1000 >>> + 0xd2000000 0x800000 >>> + 0xd0a41000 0x1000 >>> + >; >>> + reg-names = "dbi", "config", "app"; >>> + linux,pci-domain = <0>; >>> + max-link-speed = <4>; >>> + bus-range = <0x00 0x08>; >>> + interrupt-parent = <&ioapic1>; >>> + interrupts = <67 1>; >>> + #interrupt-cells = <1>; >>> + interrupt-map-mask = <0 0 0 0x7>; >>> + interrupt-map = <0 0 0 1 &ioapic1 27 1>, >>> + <0 0 0 2 &ioapic1 28 1>, >>> + <0 0 0 3 &ioapic1 29 1>, >>> + <0 0 0 4 &ioapic1 30 1>; >>> + ranges = <0x02000000 0 0xd4000000 0xd4000000 0 0x04000000>; >>> + resets = <&rcu0 0x50 0>; >>> + clocks = <&cgu0 LGM_GCLK_PCIE10>; >>> + phys = <&cb0phy0>; >>> + phy-names = "pciephy"; >>> + status = "okay"; >>> + reset-assert-ms = <500>; >>> + reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>; >>> + num-lanes = <2>; >>> + }; >>> -- 2.11.0 >>>
diff --git a/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml new file mode 100644 index 000000000000..5e5cc7fd66cd --- /dev/null +++ b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml @@ -0,0 +1,137 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/intel-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel AXI bus based PCI express root complex + +maintainers: + - Dilip Kota <eswara.kota@linux.intel.com> + +properties: + compatible: + const: intel,lgm-pcie + + device_type: + const: pci + + "#address-cells": + const: 3 + + "#size-cells": + const: 2 + + reg: + items: + - description: Controller control and status registers. + - description: PCIe configuration registers. + - description: Controller application registers. + + reg-names: + items: + - const: dbi + - const: config + - const: app + + ranges: + description: Ranges for the PCI memory and I/O regions. + + resets: + maxItems: 1 + + clocks: + description: PCIe registers interface clock. + + phys: + maxItems: 1 + + phy-names: + const: pciephy + + reset-gpios: + maxItems: 1 + + num-lanes: + description: Number of lanes to use for this port. + + linux,pci-domain: + $ref: /schemas/types.yaml#/definitions/uint32 + description: PCI domain ID. + + interrupts: + description: PCIe core integrated miscellaneous interrupt. + + '#interrupt-cells': + const: 1 + + interrupt-map-mask: + description: Standard PCI IRQ mapping properties. + + interrupt-map: + description: Standard PCI IRQ mapping properties. + + max-link-speed: + description: Specify PCI Gen for link capability. + + bus-range: + description: Range of bus numbers associated with this controller. + + reset-assert-ms: + description: | + Device reset interval in ms. + Some devices need an interval upto 500ms. By default it is 100ms. + +required: + - compatible + - device_type + - reg + - reg-names + - ranges + - resets + - clocks + - phys + - phy-names + - reset-gpios + - num-lanes + - linux,pci-domain + - interrupts + - interrupt-map + - interrupt-map-mask + +additionalProperties: false + +examples: + - | + pcie10:pcie@d0e00000 { + compatible = "intel,lgm-pcie"; + device_type = "pci"; + #address-cells = <3>; + #size-cells = <2>; + reg = < + 0xd0e00000 0x1000 + 0xd2000000 0x800000 + 0xd0a41000 0x1000 + >; + reg-names = "dbi", "config", "app"; + linux,pci-domain = <0>; + max-link-speed = <4>; + bus-range = <0x00 0x08>; + interrupt-parent = <&ioapic1>; + interrupts = <67 1>; + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0x7>; + interrupt-map = <0 0 0 1 &ioapic1 27 1>, + <0 0 0 2 &ioapic1 28 1>, + <0 0 0 3 &ioapic1 29 1>, + <0 0 0 4 &ioapic1 30 1>; + ranges = <0x02000000 0 0xd4000000 0xd4000000 0 0x04000000>; + resets = <&rcu0 0x50 0>; + clocks = <&cgu0 LGM_GCLK_PCIE10>; + phys = <&cb0phy0>; + phy-names = "pciephy"; + status = "okay"; + reset-assert-ms = <500>; + reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>; + num-lanes = <2>; + };
The Intel PCIe RC controller is Synopsys Designware based PCIe core. Add YAML schemas for PCIe in RC mode present in Intel Universal Gateway soc. Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> --- changes on v3: Add the appropriate License-Identifier Rename intel,rst-interval to 'reset-assert-us' Add additionalProperties: false Rename phy-names to 'pciephy' Remove the dtsi node split of SoC and board in the example Add #interrupt-cells = <1>; or else interrupt parsing will fail Name yaml file with compatible name .../devicetree/bindings/pci/intel,lgm-pcie.yaml | 137 +++++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml