Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

[Ryder Lee]

On Wed, 2017-05-10 at 12:01 +0200, Arnd Bergmann wrote:
> On Wed, May 10, 2017 at 11:31 AM, Ryder Lee <ryder.lee@xxxxxxxxxxxx> wrote:
> > On Wed, 2017-05-10 at 10:08 +0200, Arnd Bergmann wrote:
> >> On Wed, May 10, 2017 at 4:07 AM, Ryder Lee <ryder.lee@xxxxxxxxxxxx> wrote:
> >>
> >> > +- ranges:
> >> > +  - The first three entries are expected to translate the addresses for the root
> >> > +    port registers, which are referenced by the assigned-addresses property of
> >> > +    the root port nodes (see below).
> >>
> >> I don't understand this part. Why do you need a static translation for these?
> >> Shouldn't they just be listed in the 'reg' property of the parent node now that
> >> you have the clk/reset/phy properties in the parent as well?
> >
> > At first, I did like that. But I noticed that someone suggest it's
> > better to use 'assigned-addresses' to handle per-port registers, the
> > same path as tegra and marvell did, in other platform discussion thread.
> > So I just put shared register in root node. It could be rolled back if
> > you feel this is inappropriate.
> 
> The marvell case is not a good example for your case: their top-level
> device is made up by the OS to help with the shared resource allocation,
> while in your case the bus bridge actually exists in hardware.
> 
> I'm not too familiar with the Tegra case, and haven't looked at that here,
> but it could be an artifact of how for a while we used to list the config
> space access in the top-level "ranges" instead of the "reg" property.
> 
> I'd vote for moving it back, for consistency with the other port specific
> properties that are now in the root node. Once you do that, the port
> nodes can be removed completely, which is what I was aiming for with
> the comments on the previous version.
 
I'll move it back.

> >> > +Required properties:
> >> > +- device_type: Must be "pci"
> >> > +- assigned-addresses: Address and size of the port configuration registers
> >> > +- reg: Only the first four bytes are used to refer to the correct bus number
> >> > +  and device number.
> >> > +- #address-cells: Must be 3
> >> > +- #size-cells: Must be 2
> >> > +- #interrupt-cells: Must be 1
> >> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
> >> > +  Please refer to the standard PCI bus binding document for a more detailed
> >> > +  explanation.
> >>
> >> Child nodes do not normally have interrupt-map properties. Isn't this
> >> already covered by the interrupt-map in the parent?
> >>
> >
> > I have one Intel 4 port ethernet card(0000:00:01) and MTK WLAN card
> > (0000:00:02), probe message looks good to me.
> >
> > pci 0000:00:01.0: fixup irq: got 224
> > pci 0000:00:01.0: assigning IRQ 224
> > pci 0000:00:02.0: fixup irq: got 225
> > pci 0000:00:02.0: assigning IRQ 225
> >
> > pci 0000:01:00.0: fixup irq: got 224
> > pci 0000:01:00.0: assigning IRQ 224
> > pci 0000:01:00.1: fixup irq: got 224
> > pci 0000:01:00.1: assigning IRQ 224
> > pci 0000:01:00.2: fixup irq: got 224
> > pci 0000:01:00.2: assigning IRQ 224
> > pci 0000:01:00.3: fixup irq: got 224
> > pci 0000:01:00.3: assigning IRQ 224
> >
> > pci 0000:02:00.0: fixup irq: got 225
> > pci 0000:02:00.0: assigning IRQ 225
> >
> >
> > But child nodes without interrupt-map properties:
> > It seems incorrect.
> >
> > pci 0000:00:01.0: fixup irq: got 224
> > pci 0000:00:01.0: assigning IRQ 224
> > pci 0000:00:02.0: fixup irq: got 225
> > pci 0000:00:02.0: assigning IRQ 225
> >
> > pci 0000:01:00.0: fixup irq: got 223
> > pci 0000:01:00.0: assigning IRQ 223
> 
> Not entirely sure what happens here, but I guess the problem
> is that the 'reg' portion of the parent interrupt-map refers to
> the port devices, not the devices attached the devices behind
> them.

I agree with you. That's why I need additional interrupt-map properties
to resolve IRQ correctly for the devices behind root ports.

Not sure whether other platforms have similar case like me here.

> On a related note, I see that you still list
> 
> > +- interrupts: Three interrupt outputs of the controller. Must contain an
> > +  entry for each entry in the interrupt-names property.
> > +- interrupt-names: Must include the following names
> > +  - "pcie-int0"
> > +  - "pcie-int1"
> > +  - "pcie-int2"
> 
> This seems to be an artifact from the older version and should be
> removed as the driver correctly ignores the properties now.

Actually, everything works fine without these properties however when it
loads we see a few weird error message:

pcieport 0000:00:01.0: Signaling PME with IRQ 232
pcieport 0000:00:02.0: enabling device (0140 -> 0142)
pcieport 0000:00:02.0: enabling bus mastering
irq 232: nobody cared (try booting with the "irqpoll" option)
...
[<c03f6be4>] (pcie_pme_probe) from [<c03f47b8>] (pcie_port_probe_service
+0x44/0x6c)
(pcie_port_probe_service) from [<c0454cf8>] (driver_probe_device
+0x280/0x470)
...
(pcie_port_device_register) from [<c03f51a0>] (pcie_portdrv_probe
+0x3c/0xb4)
(pcie_portdrv_probe) from [<c03e7acc>] (pci_device_probe+0x98/0xfc)
(pci_device_probe) from [<c0454cf8>] (driver_probe_device+0x280/0x470)
handlers:
[<c03f68b0>] pcie_pme_irq
Disabling IRQ #233

I haven't dig it out yet, but just keep them here to solve that.