diff mbox series

[v3,1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller

Message ID fe9549470bc06ea0d0dfc80f46a579baa49b911a.1567585181.git.eswara.kota@linux.intel.com (mailing list archive)
State Superseded, archived
Headers show
Series PCI: Add Intel PCIe Driver and respective dt-binding yaml file | expand

Commit Message

Dilip Kota Sept. 4, 2019, 10:10 a.m. UTC
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

Comments

Chuan Hua, Lei Sept. 5, 2019, 2:23 a.m. UTC | #1
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>;
> +    };
Martin Blumenstingl Sept. 5, 2019, 8:31 p.m. UTC | #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
Chuan Hua, Lei Sept. 6, 2019, 3:22 a.m. UTC | #3
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
Andy Shevchenko Sept. 6, 2019, 9:19 a.m. UTC | #4
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.
Dilip Kota Sept. 6, 2019, 10:39 a.m. UTC | #5
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
Martin Blumenstingl Sept. 6, 2019, 5:17 p.m. UTC | #6
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
Andy Shevchenko Sept. 6, 2019, 5:48 p.m. UTC | #7
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.
Ivan Gorinov Sept. 7, 2019, 1:48 a.m. UTC | #8
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
Dilip Kota Sept. 9, 2019, 6:52 a.m. UTC | #9
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

>
Rob Herring Sept. 17, 2019, 6:33 p.m. UTC | #10
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
Rob Herring Sept. 17, 2019, 6:40 p.m. UTC | #11
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
>
Dilip Kota Sept. 18, 2019, 6:48 a.m. UTC | #12
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
Dilip Kota Sept. 18, 2019, 6:56 a.m. UTC | #13
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
>>
Dilip Kota Oct. 3, 2019, 6:35 a.m. UTC | #14
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 mbox series

Patch

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>;
+    };