[02/16] dt-bindings: PCI: dwc: Add Baikal-T1 PCIe Root Port bindings

Baikal-T1 SoC is equipped with DWC PCIe v4.60a Root Port controller, which
link can be trained to work on up to Gen.3 speed over up to x4 lanes. The
controller is supposed to be fed up with four clock sources: DBI
peripheral clock, AXI application Tx/Rx clocks and external PHY/core
reference clock generating the 100MHz signal. In addition to that the
platform provide a way to reset each part of the controller:
sticky/non-sticky bits, host controller core, PIPE interface, PCS/PHY and
Hot/Power reset signal. The Root Port controller is equipped with multiple
IRQ lines like MSI, system AER, PME, HP, Bandwidth change, Link
equalization request and eDMA ones. The registers space is accessed over
the DBI interface. There can be no more than four inbound or outbound iATU
windows configured.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Rob, this DT-bindings most likely will fail the dt_bindings_check
evaluations with the "'*' is not of type 'array'" errors for the same
reason described in the previous patch. Let's discuss the problem there.
---
 .../bindings/pci/baikal,bt1-pcie.yaml         | 148 ++++++++++++++++++
 1 file changed, 148 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml

On Thu, Mar 24, 2022 at 04:37:20AM +0300, Serge Semin wrote:
> Baikal-T1 SoC is equipped with DWC PCIe v4.60a Root Port controller, which
> link can be trained to work on up to Gen.3 speed over up to x4 lanes. The
> controller is supposed to be fed up with four clock sources: DBI
> peripheral clock, AXI application Tx/Rx clocks and external PHY/core
> reference clock generating the 100MHz signal. In addition to that the
> platform provide a way to reset each part of the controller:
> sticky/non-sticky bits, host controller core, PIPE interface, PCS/PHY and
> Hot/Power reset signal. The Root Port controller is equipped with multiple
> IRQ lines like MSI, system AER, PME, HP, Bandwidth change, Link
> equalization request and eDMA ones. The registers space is accessed over
> the DBI interface. There can be no more than four inbound or outbound iATU
> windows configured.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ---
> 
> Rob, this DT-bindings most likely will fail the dt_bindings_check
> evaluations with the "'*' is not of type 'array'" errors for the same
> reason described in the previous patch. Let's discuss the problem there.
> ---
>  .../bindings/pci/baikal,bt1-pcie.yaml         | 148 ++++++++++++++++++
>  1 file changed, 148 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> new file mode 100644
> index 000000000000..f34438330a86
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> @@ -0,0 +1,148 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/baikal,bt1-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Baikal-T1 PCIe Root Port Controller
> +
> +maintainers:
> +  - Serge Semin <fancer.lancer@gmail.com>
> +
> +description: |
> +  Embedded into Baikal-T1 SoC Root Complex controller. It's based on the
> +  DWC RC PCIe v4.60a IP-core, which is configured to have just a single Root
> +  Port function and is capable of establishing the link up to Gen.3 speed
> +  on x4 lanes. It doesn't have embedded clock and reset control module, so
> +  the proper interface initialization is supposed to be performed by software.
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: snps,dw-pcie.yaml#
> +
> +properties:
> +  compatible:
> +    contains:
> +      const: baikal,bt1-pcie
> +
> +  reg:
> +    description:
> +      DBI, DBI2 and at least 4KB outbound iATU-capable region are available.

outbound iATU-capable region? While configured with iATU, it's just 
config space from a binding standpoint.

It's a bit more precise to do this:

items:
  - description: DBI...
  - description: DBI2...
  - ...


> +    maxItems: 3

And then drop this.

> +
> +  reg-names:
> +    minItems: 3
> +    maxItems: 3
> +    items:
> +      enum: [ dbi, dbi2, config ]

To enforce the order:

items:
  - const: dbi
  - const: dbi2
  - const: config

> +
> +  interrupts:
> +    description:
> +      MSI, AER, PME, Hot-plug, Link Bandwidth Management, Link Equalization
> +      request and eight Read/Write eDMA IRQ lines are available.
> +    maxItems: 14
> +
> +  interrupt-names:
> +    minItems: 14
> +    maxItems: 14
> +    items:
> +      oneOf:
> +        - pattern: '^dma[0-7]$'
> +        - enum: [ msi, aer, pme, hp, bw_mg, l_eq ]

Can't you define the order?

> +
> +  clocks:
> +    description:
> +      DBI (attached to the APB bus), AXI-bus master and slave interfaces
> +      are fed up by the dedicated application clocks. A common reference
> +      clock signal is supposed to be attached to the corresponding Ref-pad
> +      of the SoC. It will be redistributed amongst the controller core
> +      sub-modules (pipe, core, aux, etc).
> +    minItems: 4
> +    maxItems: 4
> +
> +  clock-names:
> +    minItems: 4
> +    maxItems: 4
> +    items:
> +      enum: [ dbi, mstr, slv, ref ]
> +
> +  resets:
> +    description:
> +      A comprehensive controller reset logic is supposed to be implemented
> +      by software, so almost all the possible application and core reset
> +      signals are exposed via the system CCU module.
> +    minItems: 9
> +    maxItems: 9
> +
> +  reset-names:
> +    minItems: 9
> +    maxItems: 9
> +    items:
> +      enum: [ mstr, slv, pwr, hot, phy, core, pipe, sticky, non-sticky ]

Again, define the order.

> +
> +  syscon:

Needs a vendor prefix.

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the Baikal-T1 System Controller DT node. It's required to
> +      access some additional PM, Reset-related and LTSSM signals.
> +
> +  num-lanes:
> +    maximum: 4
> +
> +  max-link-speed:
> +    maximum: 3
> +

> +  num-ob-windows:
> +    const: 4
> +
> +  num-ib-windows:
> +    const: 4

These are deprecated. The driver probes for how many now.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    pcie@1f052000 {
> +      compatible = "baikal,bt1-pcie", "snps,dw-pcie-4.60a", "snps,dw-pcie";
> +      device_type = "pci";
> +      reg = <0x1f052000 0x1000>, <0x1f053000 0x1000>, <0x1bdbf000 0x1000>;
> +      reg-names = "dbi", "dbi2", "config";
> +      #address-cells = <3>;
> +      #size-cells = <2>;
> +      ranges = <0x81000000 0 0x00000000 0x1bdb0000 0 0x00008000>,
> +               <0x82000000 0 0x20000000 0x08000000 0 0x13db0000>;
> +      bus-range = <0x0 0xff>;
> +
> +      interrupts = <0 80 4>, <0 81 4>, <0 82 4>, <0 83 4>,
> +                   <0 84 4>, <0 85 4>, <0 86 4>, <0 87 4>,
> +                   <0 88 4>, <0 89 4>, <0 90 4>, <0 91 4>,
> +                   <0 92 4>, <0 93 4>;
> +      interrupt-names = "dma0", "dma1", "dma2", "dma3", "dma4", "dma5", "dma6",
> +                        "dma7", "msi", "aer", "pme", "hp", "bw_mg", "l_eq";
> +
> +      clocks = <&ccu_sys 1>, <&ccu_axi 6>, <&ccu_axi 7>, <&clk_pcie>;
> +      clock-names = "dbi", "mstr", "slv", "ref";
> +
> +      resets = <&ccu_axi 6>, <&ccu_axi 7>, <&ccu_sys 7>, <&ccu_sys 10>,
> +               <&ccu_sys 4>, <&ccu_sys 6>, <&ccu_sys 5>, <&ccu_sys 8>,
> +               <&ccu_sys 9>;
> +      reset-names = "mstr", "slv", "pwr", "hot", "phy", "core", "pipe",
> +                    "sticky", "non-sticky";
> +
> +      reset-gpios = <&port0 0 1>;
> +
> +      num-lanes = <4>;
> +      max-link-speed = <3>;
> +    };
> +...
> -- 
> 2.35.1
> 
>

On Mon, Mar 28, 2022 at 06:20:03PM -0500, Rob Herring wrote:
> On Thu, Mar 24, 2022 at 04:37:20AM +0300, Serge Semin wrote:
> > Baikal-T1 SoC is equipped with DWC PCIe v4.60a Root Port controller, which
> > link can be trained to work on up to Gen.3 speed over up to x4 lanes. The
> > controller is supposed to be fed up with four clock sources: DBI
> > peripheral clock, AXI application Tx/Rx clocks and external PHY/core
> > reference clock generating the 100MHz signal. In addition to that the
> > platform provide a way to reset each part of the controller:
> > sticky/non-sticky bits, host controller core, PIPE interface, PCS/PHY and
> > Hot/Power reset signal. The Root Port controller is equipped with multiple
> > IRQ lines like MSI, system AER, PME, HP, Bandwidth change, Link
> > equalization request and eDMA ones. The registers space is accessed over
> > the DBI interface. There can be no more than four inbound or outbound iATU
> > windows configured.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > ---
> > 
> > Rob, this DT-bindings most likely will fail the dt_bindings_check
> > evaluations with the "'*' is not of type 'array'" errors for the same
> > reason described in the previous patch. Let's discuss the problem there.
> > ---
> >  .../bindings/pci/baikal,bt1-pcie.yaml         | 148 ++++++++++++++++++
> >  1 file changed, 148 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> > new file mode 100644
> > index 000000000000..f34438330a86
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> > @@ -0,0 +1,148 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/baikal,bt1-pcie.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Baikal-T1 PCIe Root Port Controller
> > +
> > +maintainers:
> > +  - Serge Semin <fancer.lancer@gmail.com>
> > +
> > +description: |
> > +  Embedded into Baikal-T1 SoC Root Complex controller. It's based on the
> > +  DWC RC PCIe v4.60a IP-core, which is configured to have just a single Root
> > +  Port function and is capable of establishing the link up to Gen.3 speed
> > +  on x4 lanes. It doesn't have embedded clock and reset control module, so
> > +  the proper interface initialization is supposed to be performed by software.
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +  - $ref: snps,dw-pcie.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    contains:
> > +      const: baikal,bt1-pcie
> > +
> > +  reg:
> > +    description:
> > +      DBI, DBI2 and at least 4KB outbound iATU-capable region are available.
> 

> outbound iATU-capable region? While configured with iATU, it's just 
> config space from a binding standpoint.

In this particular device the config-space is supposed to be mapped
over the outbound iATU region. It's a MMIO region mapped to the Source
side of the outbound iATU interface.

> 
> It's a bit more precise to do this:
> 
> items:
>   - description: DBI...
>   - description: DBI2...
>   - ...

I know, but I prefer using the reg-names property for that, while
having the 'reg' property with minItems/maxItems constraints specified.
In this case the reg property would be left unordered.

> 
> 
> > +    maxItems: 3
> 
> And then drop this.
> 
> > +
> > +  reg-names:
> > +    minItems: 3
> > +    maxItems: 3
> > +    items:
> > +      enum: [ dbi, dbi2, config ]
> 

> To enforce the order:
> 
> items:
>   - const: dbi
>   - const: dbi2
>   - const: config

Could you please explain why do you always suggest to define the ordered
X/X-names properties group? I just don't see much benefits in such
approach except that it's a bit more readable and one line shorter.

> 
> > +
> > +  interrupts:
> > +    description:
> > +      MSI, AER, PME, Hot-plug, Link Bandwidth Management, Link Equalization
> > +      request and eight Read/Write eDMA IRQ lines are available.
> > +    maxItems: 14
> > +
> > +  interrupt-names:
> > +    minItems: 14
> > +    maxItems: 14
> > +    items:
> > +      oneOf:
> > +        - pattern: '^dma[0-7]$'
> > +        - enum: [ msi, aer, pme, hp, bw_mg, l_eq ]
> 

> Can't you define the order?

Please, see my question in the previous comment.

> 
> > +
> > +  clocks:
> > +    description:
> > +      DBI (attached to the APB bus), AXI-bus master and slave interfaces
> > +      are fed up by the dedicated application clocks. A common reference
> > +      clock signal is supposed to be attached to the corresponding Ref-pad
> > +      of the SoC. It will be redistributed amongst the controller core
> > +      sub-modules (pipe, core, aux, etc).
> > +    minItems: 4
> > +    maxItems: 4
> > +
> > +  clock-names:
> > +    minItems: 4
> > +    maxItems: 4
> > +    items:
> > +      enum: [ dbi, mstr, slv, ref ]
> > +
> > +  resets:
> > +    description:
> > +      A comprehensive controller reset logic is supposed to be implemented
> > +      by software, so almost all the possible application and core reset
> > +      signals are exposed via the system CCU module.
> > +    minItems: 9
> > +    maxItems: 9
> > +
> > +  reset-names:
> > +    minItems: 9
> > +    maxItems: 9
> > +    items:
> > +      enum: [ mstr, slv, pwr, hot, phy, core, pipe, sticky, non-sticky ]
> 

> Again, define the order.

Could you tell me why it's better than unordered property? Really I
don't see good reason enough to define more constraints than required.

> 
> > +
> > +  syscon:
> 

> Needs a vendor prefix.

Ok.

> 
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the Baikal-T1 System Controller DT node. It's required to
> > +      access some additional PM, Reset-related and LTSSM signals.
> > +
> > +  num-lanes:
> > +    maximum: 4
> > +
> > +  max-link-speed:
> > +    maximum: 3
> > +
> 
> > +  num-ob-windows:
> > +    const: 4
> > +
> > +  num-ib-windows:
> > +    const: 4
> 

> These are deprecated. The driver probes for how many now.

I know that they are deprecated. But they are deprecated on the DT
node level, not in the DT schema. Here by explicitly defining these
properties I signify that there are four in- and outbound iATU
windows. Doesn't that better describe our hardware?

-Sergey

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - clocks
> > +  - clock-names
> > +  - resets
> > +  - reset-names
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    pcie@1f052000 {
> > +      compatible = "baikal,bt1-pcie", "snps,dw-pcie-4.60a", "snps,dw-pcie";
> > +      device_type = "pci";
> > +      reg = <0x1f052000 0x1000>, <0x1f053000 0x1000>, <0x1bdbf000 0x1000>;
> > +      reg-names = "dbi", "dbi2", "config";
> > +      #address-cells = <3>;
> > +      #size-cells = <2>;
> > +      ranges = <0x81000000 0 0x00000000 0x1bdb0000 0 0x00008000>,
> > +               <0x82000000 0 0x20000000 0x08000000 0 0x13db0000>;
> > +      bus-range = <0x0 0xff>;
> > +
> > +      interrupts = <0 80 4>, <0 81 4>, <0 82 4>, <0 83 4>,
> > +                   <0 84 4>, <0 85 4>, <0 86 4>, <0 87 4>,
> > +                   <0 88 4>, <0 89 4>, <0 90 4>, <0 91 4>,
> > +                   <0 92 4>, <0 93 4>;
> > +      interrupt-names = "dma0", "dma1", "dma2", "dma3", "dma4", "dma5", "dma6",
> > +                        "dma7", "msi", "aer", "pme", "hp", "bw_mg", "l_eq";
> > +
> > +      clocks = <&ccu_sys 1>, <&ccu_axi 6>, <&ccu_axi 7>, <&clk_pcie>;
> > +      clock-names = "dbi", "mstr", "slv", "ref";
> > +
> > +      resets = <&ccu_axi 6>, <&ccu_axi 7>, <&ccu_sys 7>, <&ccu_sys 10>,
> > +               <&ccu_sys 4>, <&ccu_sys 6>, <&ccu_sys 5>, <&ccu_sys 8>,
> > +               <&ccu_sys 9>;
> > +      reset-names = "mstr", "slv", "pwr", "hot", "phy", "core", "pipe",
> > +                    "sticky", "non-sticky";
> > +
> > +      reset-gpios = <&port0 0 1>;
> > +
> > +      num-lanes = <4>;
> > +      max-link-speed = <3>;
> > +    };
> > +...
> > -- 
> > 2.35.1
> > 
> >