[v4,12/17] 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>

---

Changelog v2:
- Rename 'syscon' property to 'baikal,bt1-syscon'.
- Fix the 'compatible' property definition to being more specific about
  what strings are supposed to be used. Due to that we had to add the
  select property to evaluate the schema against the Baikal-T1 PCIe DT
  nodes only.
---
 .../bindings/pci/baikal,bt1-pcie.yaml         | 154 ++++++++++++++++++
 1 file changed, 154 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml

On Thu, Jul 28, 2022 at 05:34:22PM +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>
> 
> ---
> 
> Changelog v2:
> - Rename 'syscon' property to 'baikal,bt1-syscon'.
> - Fix the 'compatible' property definition to being more specific about
>   what strings are supposed to be used. Due to that we had to add the
>   select property to evaluate the schema against the Baikal-T1 PCIe DT
>   nodes only.
> ---
>  .../bindings/pci/baikal,bt1-pcie.yaml         | 154 ++++++++++++++++++
>  1 file changed, 154 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..23bd1d0aa5c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> @@ -0,0 +1,154 @@
> +# 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.
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: baikal,bt1-pcie
> +
> +  required:
> +    - compatible
> +
> +allOf:
> +  - $ref: /schemas/pci/snps,dw-pcie.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: baikal,bt1-pcie
> +      - const: snps,dw-pcie-4.60a
> +      - const: snps,dw-pcie

Again, these fallbacks simply aren't useful.

> +
> +  reg:
> +    description:
> +      DBI, DBI2 and at least 4KB outbound iATU-capable region.

'iATU-capable region' means config space? That's not very clear.

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

Define the order. Here, and the rest.

> +
> +  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 ]
> +
> +  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 ]
> +
> +  baikal,bt1-syscon:
> +    $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. Don't add them for a new binding.


> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-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, Aug 01, 2022 at 12:13:11PM -0600, Rob Herring wrote:
> On Thu, Jul 28, 2022 at 05:34:22PM +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>
> > 
> > ---
> > 
> > Changelog v2:
> > - Rename 'syscon' property to 'baikal,bt1-syscon'.
> > - Fix the 'compatible' property definition to being more specific about
> >   what strings are supposed to be used. Due to that we had to add the
> >   select property to evaluate the schema against the Baikal-T1 PCIe DT
> >   nodes only.
> > ---
> >  .../bindings/pci/baikal,bt1-pcie.yaml         | 154 ++++++++++++++++++
> >  1 file changed, 154 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..23bd1d0aa5c5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> > @@ -0,0 +1,154 @@
> > +# 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.
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        const: baikal,bt1-pcie
> > +
> > +  required:
> > +    - compatible
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: baikal,bt1-pcie
> > +      - const: snps,dw-pcie-4.60a
> > +      - const: snps,dw-pcie
> 

> Again, these fallbacks simply aren't useful.

Ok. I give up. You are the boss. I'll drop them =)

> 
> > +
> > +  reg:
> > +    description:
> > +      DBI, DBI2 and at least 4KB outbound iATU-capable region.
> 

> 'iATU-capable region' means config space? That's not very clear.

No 'iATU-capable region' means the region, which can be used as a
source address for the iATU engine. In general it can be used for any
accesses (IO, MEM, CFG). But the DW PCIe driver will indeed use it for
the config-space accesses.

IMO the 'config' reg space is kind of virtual. Due to the outbound
iATU capability the driver could use any free outbound iATU region it
found instead.

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

> Define the order. Here, and the rest.

Ok. I will, but please answer to my question, I asked you in the
previous email thread:

Serge Semin wrote:
> Rob Herring wrote:
> > ...
> > Tell me why you need random order.
>
> Because I don't see a need in constraining the order. If we get to set
> the order requirement, then why do we need to have the "*-names"
> property at all?
> IMO having "reg" with max/minItems restriction plus generic
> description and "reg-names" with possible values enumerated seems very
> suitable pattern in this case. Don't you think?

In addition to that what about optional names? How would you suggest
to handle such case without the non-ordered pattern?

> 
> > +
> > +  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 ]
> > +
> > +  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 ]
> > +
> > +  baikal,bt1-syscon:
> > +    $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. Don't add them for a new binding.

Ok.

-Sergey

> 
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - interrupt-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, Aug 8, 2022 at 10:01 AM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Mon, Aug 01, 2022 at 12:13:11PM -0600, Rob Herring wrote:
> > On Thu, Jul 28, 2022 at 05:34:22PM +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>
> > >
> > > ---
> > >
> > > Changelog v2:
> > > - Rename 'syscon' property to 'baikal,bt1-syscon'.
> > > - Fix the 'compatible' property definition to being more specific about
> > >   what strings are supposed to be used. Due to that we had to add the
> > >   select property to evaluate the schema against the Baikal-T1 PCIe DT
> > >   nodes only.
> > > ---
> > >  .../bindings/pci/baikal,bt1-pcie.yaml         | 154 ++++++++++++++++++
> > >  1 file changed, 154 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..23bd1d0aa5c5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> > > @@ -0,0 +1,154 @@
> > > +# 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.
> > > +
> > > +select:
> > > +  properties:
> > > +    compatible:
> > > +      contains:
> > > +        const: baikal,bt1-pcie
> > > +
> > > +  required:
> > > +    - compatible
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: baikal,bt1-pcie
> > > +      - const: snps,dw-pcie-4.60a
> > > +      - const: snps,dw-pcie
> >
>
> > Again, these fallbacks simply aren't useful.
>
> Ok. I give up. You are the boss. I'll drop them =)
>
> >
> > > +
> > > +  reg:
> > > +    description:
> > > +      DBI, DBI2 and at least 4KB outbound iATU-capable region.
> >
>
> > 'iATU-capable region' means config space? That's not very clear.
>
> No 'iATU-capable region' means the region, which can be used as a
> source address for the iATU engine. In general it can be used for any
> accesses (IO, MEM, CFG). But the DW PCIe driver will indeed use it for
> the config-space accesses.
>
> IMO the 'config' reg space is kind of virtual. Due to the outbound
> iATU capability the driver could use any free outbound iATU region it
> found instead.

It is and in hindsight, we maybe should have described the whole
address space and let the OS alloc the config space out of it. But
then again, original OpenFirmware PCI binding reflects what the
firmware discovered AND how it is configured. So specifying where
config space is makes sense.

Bottom line is the binding defines putting the config space region in
'reg', not an iATU region.

> > > +    maxItems: 3
> > > +
> > > +  reg-names:
> > > +    minItems: 3
> > > +    maxItems: 3
> > > +    items:
> > > +      enum: [ dbi, dbi2, config ]
> >
>
> > Define the order. Here, and the rest.
>
> Ok. I will, but please answer to my question, I asked you in the
> previous email thread:
>
> Serge Semin wrote:
> > Rob Herring wrote:
> > > ...
> > > Tell me why you need random order.
> >
> > Because I don't see a need in constraining the order. If we get to set
> > the order requirement, then why do we need to have the "*-names"
> > property at all?

Originally, it was for cases where you have a variable number of
entries and can't determine what each entry is. IOW, when you have
optional entries in the middle of required entries. But then everyone
*loves* -names even when not needed or useful such as 'phy-names =
"pcie"' (the phy subsys requiring names was part of the problem there,
but that's been fixed).

> > IMO having "reg" with max/minItems restriction plus generic
> > description and "reg-names" with possible values enumerated seems very
> > suitable pattern in this case. Don't you think?

No, I think this is just as concise and defines the order too:

reg-names:
  items:
    - const: dbi
    - const: dbi2
    - const: config

>
> In addition to that what about optional names? How would you suggest
> to handle such case without the non-ordered pattern?

Sorry, I don't follow.

Rob

On Tue, Aug 09, 2022 at 09:12:31AM -0600, Rob Herring wrote:
> On Mon, Aug 8, 2022 at 10:01 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Mon, Aug 01, 2022 at 12:13:11PM -0600, Rob Herring wrote:
> > > On Thu, Jul 28, 2022 at 05:34:22PM +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>
> > > >
> > > > ---
> > > >
> > > > Changelog v2:
> > > > - Rename 'syscon' property to 'baikal,bt1-syscon'.
> > > > - Fix the 'compatible' property definition to being more specific about
> > > >   what strings are supposed to be used. Due to that we had to add the
> > > >   select property to evaluate the schema against the Baikal-T1 PCIe DT
> > > >   nodes only.
> > > > ---
> > > >  .../bindings/pci/baikal,bt1-pcie.yaml         | 154 ++++++++++++++++++
> > > >  1 file changed, 154 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..23bd1d0aa5c5
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/pci/baikal,bt1-pcie.yaml
> > > > @@ -0,0 +1,154 @@
> > > > +# 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.
> > > > +
> > > > +select:
> > > > +  properties:
> > > > +    compatible:
> > > > +      contains:
> > > > +        const: baikal,bt1-pcie
> > > > +
> > > > +  required:
> > > > +    - compatible
> > > > +
> > > > +allOf:
> > > > +  - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - const: baikal,bt1-pcie
> > > > +      - const: snps,dw-pcie-4.60a
> > > > +      - const: snps,dw-pcie
> > >
> >
> > > Again, these fallbacks simply aren't useful.
> >
> > Ok. I give up. You are the boss. I'll drop them =)
> >
> > >
> > > > +
> > > > +  reg:
> > > > +    description:
> > > > +      DBI, DBI2 and at least 4KB outbound iATU-capable region.
> > >
> >
> > > 'iATU-capable region' means config space? That's not very clear.
> >
> > No 'iATU-capable region' means the region, which can be used as a
> > source address for the iATU engine. In general it can be used for any
> > accesses (IO, MEM, CFG). But the DW PCIe driver will indeed use it for
> > the config-space accesses.
> >
> > IMO the 'config' reg space is kind of virtual. Due to the outbound
> > iATU capability the driver could use any free outbound iATU region it
> > found instead.
> 
> It is and in hindsight, we maybe should have described the whole
> address space and let the OS alloc the config space out of it. But
> then again, original OpenFirmware PCI binding reflects what the
> firmware discovered AND how it is configured. So specifying where
> config space is makes sense.
> 
> Bottom line is the binding defines putting the config space region in
> 'reg', not an iATU region.
> 
> > > > +    maxItems: 3
> > > > +
> > > > +  reg-names:
> > > > +    minItems: 3
> > > > +    maxItems: 3
> > > > +    items:
> > > > +      enum: [ dbi, dbi2, config ]
> > >
> >
> > > Define the order. Here, and the rest.
> >
> > Ok. I will, but please answer to my question, I asked you in the
> > previous email thread:
> >
> > Serge Semin wrote:
> > > Rob Herring wrote:
> > > > ...
> > > > Tell me why you need random order.
> > >
> > > Because I don't see a need in constraining the order. If we get to set
> > > the order requirement, then why do we need to have the "*-names"
> > > property at all?
> 
> Originally, it was for cases where you have a variable number of
> entries and can't determine what each entry is. IOW, when you have
> optional entries in the middle of required entries. But then everyone
> *loves* -names even when not needed or useful such as 'phy-names =
> "pcie"' (the phy subsys requiring names was part of the problem there,
> but that's been fixed).
> 
> > > IMO having "reg" with max/minItems restriction plus generic
> > > description and "reg-names" with possible values enumerated seems very
> > > suitable pattern in this case. Don't you think?
> 
> No, I think this is just as concise and defines the order too:
> 
> reg-names:
>   items:
>     - const: dbi
>     - const: dbi2
>     - const: config
> 
> >
> > In addition to that what about optional names? How would you suggest
> > to handle such case without the non-ordered pattern?
> 

> Sorry, I don't follow.

I meant exactly the case you've described as the main goal of the
named properties. My worry was that by using the pattern:

reg-names:
  items:
    - const: name
    - const: another_name
    - const: one_more_name

you get to fix the names order, which they were invented to get rid
from. If you get to use that pattern the only optional names could be
the names at the tail of the array, which isn't always applicable. In
that case you'd have no choice but to use the pattern suggested by
me.

-Sergey

> 
> Rob

On Tue, Aug 9, 2022 at 1:28 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Tue, Aug 09, 2022 at 09:12:31AM -0600, Rob Herring wrote:
> > On Mon, Aug 8, 2022 at 10:01 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > >
> > > On Mon, Aug 01, 2022 at 12:13:11PM -0600, Rob Herring wrote:
> > > > On Thu, Jul 28, 2022 at 05:34:22PM +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>
> > > > >

[...]

> > > > > +  reg-names:
> > > > > +    minItems: 3
> > > > > +    maxItems: 3
> > > > > +    items:
> > > > > +      enum: [ dbi, dbi2, config ]
> > > >
> > >
> > > > Define the order. Here, and the rest.
> > >
> > > Ok. I will, but please answer to my question, I asked you in the
> > > previous email thread:
> > >
> > > Serge Semin wrote:
> > > > Rob Herring wrote:
> > > > > ...
> > > > > Tell me why you need random order.
> > > >
> > > > Because I don't see a need in constraining the order. If we get to set
> > > > the order requirement, then why do we need to have the "*-names"
> > > > property at all?
> >
> > Originally, it was for cases where you have a variable number of
> > entries and can't determine what each entry is. IOW, when you have
> > optional entries in the middle of required entries. But then everyone
> > *loves* -names even when not needed or useful such as 'phy-names =
> > "pcie"' (the phy subsys requiring names was part of the problem there,
> > but that's been fixed).
> >

> > > > IMO having "reg" with max/minItems restriction plus generic
> > > > description and "reg-names" with possible values enumerated seems very
> > > > suitable pattern in this case. Don't you think?
> >
> > No, I think this is just as concise and defines the order too:
> >
> > reg-names:
> >   items:
> >     - const: dbi
> >     - const: dbi2
> >     - const: config
> >
> > >
> > > In addition to that what about optional names? How would you suggest
> > > to handle such case without the non-ordered pattern?
> >
>
> > Sorry, I don't follow.
>
> I meant exactly the case you've described as the main goal of the
> named properties. My worry was that by using the pattern:
>
> reg-names:
>   items:
>     - const: name
>     - const: another_name
>     - const: one_more_name
>
> you get to fix the names order, which they were invented to get rid
> from. If you get to use that pattern the only optional names could be
> the names at the tail of the array, which isn't always applicable. In
> that case you'd have no choice but to use the pattern suggested by
> me.

For this binding, we use reg-names because the order and what's
present varies by platform. But for a given platform the order is
fixed.

Rob

On Tue, Aug 09, 2022 at 02:06:16PM -0600, Rob Herring wrote:
> On Tue, Aug 9, 2022 at 1:28 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Tue, Aug 09, 2022 at 09:12:31AM -0600, Rob Herring wrote:
> > > On Mon, Aug 8, 2022 at 10:01 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > >
> > > > On Mon, Aug 01, 2022 at 12:13:11PM -0600, Rob Herring wrote:
> > > > > On Thu, Jul 28, 2022 at 05:34:22PM +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>
> > > > > >
> 
> [...]
> 
> > > > > > +  reg-names:
> > > > > > +    minItems: 3
> > > > > > +    maxItems: 3
> > > > > > +    items:
> > > > > > +      enum: [ dbi, dbi2, config ]
> > > > >
> > > >
> > > > > Define the order. Here, and the rest.
> > > >
> > > > Ok. I will, but please answer to my question, I asked you in the
> > > > previous email thread:
> > > >
> > > > Serge Semin wrote:
> > > > > Rob Herring wrote:
> > > > > > ...
> > > > > > Tell me why you need random order.
> > > > >
> > > > > Because I don't see a need in constraining the order. If we get to set
> > > > > the order requirement, then why do we need to have the "*-names"
> > > > > property at all?
> > >
> > > Originally, it was for cases where you have a variable number of
> > > entries and can't determine what each entry is. IOW, when you have
> > > optional entries in the middle of required entries. But then everyone
> > > *loves* -names even when not needed or useful such as 'phy-names =
> > > "pcie"' (the phy subsys requiring names was part of the problem there,
> > > but that's been fixed).
> > >
> 
> > > > > IMO having "reg" with max/minItems restriction plus generic
> > > > > description and "reg-names" with possible values enumerated seems very
> > > > > suitable pattern in this case. Don't you think?
> > >
> > > No, I think this is just as concise and defines the order too:
> > >
> > > reg-names:
> > >   items:
> > >     - const: dbi
> > >     - const: dbi2
> > >     - const: config
> > >
> > > >
> > > > In addition to that what about optional names? How would you suggest
> > > > to handle such case without the non-ordered pattern?
> > >
> >
> > > Sorry, I don't follow.
> >
> > I meant exactly the case you've described as the main goal of the
> > named properties. My worry was that by using the pattern:
> >
> > reg-names:
> >   items:
> >     - const: name
> >     - const: another_name
> >     - const: one_more_name
> >
> > you get to fix the names order, which they were invented to get rid
> > from. If you get to use that pattern the only optional names could be
> > the names at the tail of the array, which isn't always applicable. In
> > that case you'd have no choice but to use the pattern suggested by
> > me.
> 
> For this binding, we use reg-names because the order and what's
> present varies by platform. But for a given platform the order is
> fixed.

Got it. Thanks for your time and for the detailed case clarification.

-Sergey

> 
> Rob