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

[Ryder Lee]

Hi Arnd,

I want to further explain what I have discussed in previous mail.


On Thu, 2017-05-11 at 17:08 +0800, Ryder Lee wrote:
> On Thu, 2017-05-11 at 09:17 +0200, Arnd Bergmann wrote:
> > On Thu, May 11, 2017 at 4:44 AM, Ryder Lee <ryder.lee@xxxxxxxxxxxx> wrote:
> > > 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:
> > 
> > >> >> > +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.
> > 
> > I think it's just a bug in this specific chip where the HW designers
> > wired the IRQs in a nonstandard way.
> > 
> > However, you really should not need the interrupt-map properties
> > in the child nodes, just change the address part in the parent
> > interrupt-map. Specifically, the 'bus' portion of the device address
> > in the interrupt-map would have to be nonzero to refer to
> > child devices.
> 
> This is what I modify for the parent node and remove interrupt-map
> properties from child..
> 
> interrupt-map-mask = <0xff800 0 0 0>;
> interrupt-map = <0x0000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
> 		 <0x0800 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
> 		 <0x1000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>,
> 
> 		 /* workaround here*/
> 		 <0x10000 0 0 0 &gic GIC_SPI 193 IRQ_TYPE_NONE>,
> 		 <0x20000 0 0 0 &gic GIC_SPI 194 IRQ_TYPE_NONE>,
> 	     <0x30000 0 0 0 &gic GIC_SPI 195 IRQ_TYPE_NONE>;
> 
> It works well. But how could we handle the situation if root port0
> status = "disabled" ? I think we cannot assign child bus number
> dynamically from binding.

That is to say, we route it statically if port0 (or port1) is
unavailable. The PCI child bus enumeration should look something like
this:

 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
 
 Go wrong here! IRQ 223/224 should be assigned to the devices behind
port0 and port1.
 pci 0000:01:00.0: fixup irq: got 223
 pci 0000:01:00.0: assigning IRQ 223
 pci 0000:02:00.0: fixup irq: got 224
 pci 0000:02:00.0: assigning IRQ 224

> > >> 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.
> > 
> > Something is going very wrong if adding the properties helps. I can't
> > think of what that is, but we have to find out before the binding can
> > be merged.
> 
> Not really understand PME service. But I will find the reason here.

I have do some test here. PME needs port IRQs, which interrupt type was
set correctly(IRQ_TYPE_LEVEL_LOW). But we cannot set it from
interrupt-map, according to gic_set_type() /* SPIs have restrictions on
the supported types */ .

So we need to add additional interrupt properties.