Re: [PATCH v2 2/4] dt-bindings: PCI: mediatek,mt7621: add missing child node reg

[Bjorn Helgaas]

On Thu, Apr 11, 2024 at 09:21:07AM -0500, Rob Herring wrote:
> On Thu, Apr 11, 2024 at 07:39:17AM -0500, Bjorn Helgaas wrote:
> > On Thu, Apr 11, 2024 at 08:13:18AM +0200, Sergio Paracuellos wrote:
> > > On Thu, Apr 11, 2024 at 8:01 AM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > > > On 10/04/2024 23:26, Bjorn Helgaas wrote:
> > > > > On Wed, Apr 10, 2024 at 08:15:19PM +0200, Krzysztof Kozlowski wrote:
> > > > >> MT7621 PCI host bridge has children which apparently are also PCI host
> > > > >> bridges, at least that's what the binding suggest.
> > > > >
> > > > > What does it even mean for a PCI host bridge to have a child that is
> > > > > also a PCI host bridge?
> 
> It should say 'root port' instead as the binding description correctly 
> says.

OK, that makes a lot more sense, and we should fix the commit log.

> > > > I think the question should be towards Mediatek folks. I don't know what
> > > > this hardware is exactly, just looks like pci-pci-bridge. The driver
> > > > calls the children host bridges as "ports".
> > > 
> > > You can see the topology here in my first driver submit cover letter
> > > message [0].
> > > 
> > >  [0]: https://lore.kernel.org/all/CAMhs-H-BA+KzEwuDPzcmrDPdgJBFA2XdYTBvT4R4MEOUB=WQ1g@xxxxxxxxxxxxxx/t/
> > 
> > Nothing unusual here, this looks like the standard PCIe topology.
> > 
> > What *might* be unusual is describing the Root Ports in DT.  Since
> > they are standard PCI devices, they shouldn't need DT description
> > unless there's some unusual power/clock/reset control or something
> > that is not discoverable via PCI enumeration.
> 
> It's only unusual because typically there's only 1 RP per host bridge 
> and properties which really apply to the RP get stuck in the host bridge 
> node because we don't have a RP node. An example is perst-gpios. That's 
> not a property of the RP either, but the RP is the upstream side of a 
> slot and we often don't have a node for the device either.

Makes sense.

I'm still confused about one thing, maybe just because I don't really
know how to read these bindings.  The binding now looks like this:

  properties:
    compatible:
      const: mediatek,mt7621-pci

    reg:
      items:
        - description: host-pci bridge registers
        - description: pcie port 0 RC control registers       # A
        - description: pcie port 1 RC control registers       # A
        - description: pcie port 2 RC control registers       # A

  patternProperties:
    '^pcie@[0-2],0$':
      type: object
      $ref: /schemas/pci/pci-pci-bridge.yaml#

      properties:
        reg:                                                  # B
          maxItems: 1

It looks like the "A" items are separate things from the "B" items?

But I think the relevant code is here:

  mt7621_pcie_probe
    mt7621_pcie_parse_dt
      pcie->base = devm_platform_ioremap_resource(pdev, 0)             # 1
      for_each_available_child_of_node(node, child)
        mt7621_pcie_parse_port
          port->base = devm_platform_ioremap_resource(pdev, slot + 1)  # 2

where it looks like both "1" and "2" use the items in the "A" list,
i.e., resources 0, 1, 2, 3, all from the same platform device.  Is
there code that uses the "B" item that this patch adds?

Bjorn