Re: [PATCH v4 12/17] dt-bindings: PCI: dwc: Add Baikal-T1 PCIe Root Port bindings

[Rob Herring]

On Mon, Aug 8, 2022 at 10:01 AM Serge Semin <fancer.lancer@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
> > >
> > > ---
> > >
> > > 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@xxxxxxxxx>
> > > +
> > > +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