Re: [PATCH 2/2] aer: add support aer interrupt with none MSI/MSI-X/INTx mode

[Murali Karicheri <m-karicheri2@ti.com>]

On 06/06/2016 03:32 AM, Po Liu wrote:
> Hi Bjorn,
> I confirm we met same problem with KeyStone base on DesignWare design.
> 
> 
> Best regards,
> Liu Po
> 
>>  -----Original Message-----
>>  From: Bjorn Helgaas [mailto:helgaas@kernel.org]
>>  Sent: Saturday, June 04, 2016 11:49 AM
>>  To: Murali Karicheri
>>  Cc: Po Liu; linux-pci@vger.kernel.org; linux-arm-
>>  kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>>  devicetree@vger.kernel.org; Arnd Bergmann; Roy Zang; Marc Zyngier;
>>  Stuart Yoder; Yang-Leo Li; Minghuan Lian; Bjorn Helgaas; Shawn Guo;
>>  Mingkai Hu; Rob Herring
>>  Subject: Re: [PATCH 2/2] aer: add support aer interrupt with none
>>  MSI/MSI-X/INTx mode
>>  
>>  On Fri, Jun 03, 2016 at 01:31:11PM -0400, Murali Karicheri wrote:
>>  > Po,
>>  >
>>  > Sorry to hijack your discussion, but the problem seems to be same for
>>  > Keystone PCI controller which is also designware (old version) based.
>>  >
>>  > On 06/03/2016 12:09 AM, Bjorn Helgaas wrote:
>>  > > On Thu, Jun 02, 2016 at 11:37:28AM -0400, Murali Karicheri wrote:
>>  > >> On 06/02/2016 09:55 AM, Bjorn Helgaas wrote:
>>  > >>> On Thu, Jun 02, 2016 at 05:01:19AM +0000, Po Liu wrote:
>>  > >>>>>  -----Original Message-----
>>  > >>>>>  From: Bjorn Helgaas [mailto:helgaas@kernel.org]
>>  > >>>>>  Sent: Thursday, June 02, 2016 11:48 AM
>>  > >>>>>  To: Po Liu
>>  > >>>>>  Cc: linux-pci@vger.kernel.org;
>>  > >>>>> linux-arm-kernel@lists.infradead.org;
>>  > >>>>>  linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Arnd
>>  > >>>>> Bergmann;  Roy Zang; Marc Zyngier; Stuart Yoder; Yang-Leo Li;
>>  > >>>>> Minghuan Lian; Bjorn  Helgaas; Shawn Guo; Mingkai Hu; Rob
>>  > >>>>> Herring
>>  > >>>>>  Subject: Re: [PATCH 2/2] aer: add support aer interrupt with
>>  > >>>>> none  MSI/MSI-X/INTx mode
>>  > >>>>>
>>  > >>>>>  [+cc Rob]
>>  > >>>>>
>>  > >>>>>  Hi Po,
>>  > >>>>>
>>  > >>>>>  On Thu, May 26, 2016 at 02:00:06PM +0800, Po Liu wrote:
>>  > >>>>>  > On some platforms, root port doesn't support MSI/MSI-X/INTx
>>  in RC mode.
>>  > >>>>>  > When chip support the aer interrupt with none MSI/MSI-X/INTx
>>  > >>>>> mode,  > maybe there is interrupt line for aer pme etc. Search
>>  > >>>>> the interrupt  > number in the fdt file.
>>  > >>>>>
>>  > >>>>>  My understanding is that AER interrupt signaling can be done
>>  > >>>>> via INTx,  MSI, or MSI-X (PCIe spec r3.0, sec 6.2.4.1.2).
>>  > >>>>> Apparently your device  doesn't support MSI or MSI-X.  Are you
>>  > >>>>> saying it doesn't support INTx  either?  How is the interrupt
>>  you're requesting here different from INTx?
>>  > >>>>
>>  > >>>> Layerscape use none of MSI or MSI-X or INTx to indicate the
>>  > >>>> devices or root error in RC mode. But use an independent SPI
>>  > >>>> interrupt(arm interrupt controller) line.
>>  > >>>
>>  > >>> The Root Port is a PCI device and should follow the normal PCI
>>  > >>> rules for interrupts.  As far as I understand, that means it
>>  > >>> should use MSI, MSI-X, or INTx.  If your Root Port doesn't use MSI
>>  > >>> or MSI-X, it should use INTx, the PCI_INTERRUPT_PIN register
>>  > >>> should tell us which (INTA/ INTB/etc.), and
>>  PCI_COMMAND_INTX_DISABLE should work to disable it.
>>  > >>> That's all from the PCI point of view, of course.
>>  > >>
>>  > >> I am faced with the same issue on Keystone PCI hardware and it has
>>  > >> been on my TODO list  for quite some time. Keystone PCI hardware
>>  > >> also doesn't use MSI or MSI-X or INTx for reporting errors received
>>  > >> at the root port, but use a platform interrupt instead (not
>>  > >> complaint to PCI standard as per PCI base spec). So I would need
>>  > >> similar change to have the error interrupt passed to the aer
>>  > >> driver. So there are hardware out there like Keystone which
>>  requires to support this through platform IRQ.
>>  > >
>>  > > This is not a new area of the spec, and it's hard for me to believe
>>  > > that these two new PCIe controllers are both broken the same way
>>  > > (although I guess both are DesignWare-based, so maybe this is the
>>  > > same underlying problem in both cases?).  I think it's more likely
>>  > > that we just haven't figured out the right way to describe this in
>>  the DT.
>>  >
>>  > Keystone is using an older version of the designware IP and it
>>  > implements all of the interrupts in the application register space
>>  > unlike other newer version of the hardware. So I assume, the version
>>  > used on Layerscape is also an older version and the both have same
>>  > issue in terms of non standard platform interrupt used for error
>>  reporting.
>>  >
>>  > > I assume you have a Root Port with an AER capability, no MSI
>>  > > capability, and no MSI-X capability, right?
>>  >
>>  > Has AER capability and both MSI and INTx (legacy) capability
>>  >
>>  > > What does its Interrupt
>>  > > Pin register contain?  If it's zero, it doesn't use INTx either, so
>>  > > according to the spec it should generate no interrupts.
>>  > >
>>  > At address offset 0x3C by default has a value of 1, but it is writable
>>  > by software. So default is INTx A.
>>  
>>  0x3c is the Interrupt *Line*, which is read/write.  The Interrupt
>>  *Pin* is at 0x3d and should be read-only.
>>  

You are right. But default is 1 at this address.

>>  Does your Keystone driver support MSI?  If so, since your Root Port
>>  supports MSI, I would think we would use that by default, and the INTx
>>  stuff wouldn't even matter.
> 
> Layerscape is also shows "Both message signaled interrupts (MSI) and legacy INTx are supported."
> But both of them not work for AER interrupt when devices or root port report aer error.
> But another GIC interrupt line do.

Same with Keystone. Even though both MSI and INTx are supported
error interrupt at root port is reported on a different interrupt
line than MSI/INTx. So for Power Management event interrupt is also
different line.

Murali
> 
>>  
>>  > > But if Interrupt Pin is non-zero, that means the Root Port should be
>>  > > able to generate virtual INTx interrupts.  Presumably the Root
>>  > > Complex connects those interrupts to something; maybe to your
>>  > > platform interrupt?
>>  >
>>  > Probably that is what is happening. Both Power management and Error
>>  > interrupts are raised on platform interrupt lines.
>>  >
>>  > 12 Error Interrupts
>>  >
>>  > [0] System error (OR of fatal, nonfatal, correctable errors) (RC mode
>>  > only) [1] PCIe fatal error (RC mode only) [2] PCIe non-fatal error (RC
>>  > mode only) [3] PCIe correctable error (RC mode only) [4] AXI Error due
>>  > to fatal condition in AXI bridge (EP/RC modes) [5] PCIe advanced error
>>  > (RC mode only)
>>  >
>>  > 13 Power management and reset event interrupts
>>  >
>>  > [0] Power management turn-off message interrupt (EP mode only) [1]
>>  > Power management ack message interrupt (RC mode only) [2] Power
>>  > management event interrupt (RC mode only) [3] Link request reset
>>  > interrupt (hot reset or link down) (RC mode only)
>>  >
>>  > > PCI doesn't say anything about an interrupt after it leaves the Root
>>  > > Complex, so the fact that it's connected to a "platform interrupt"
>>  > > or "SPI interrupt" or "IOAPIC interrupt" doesn't make it non-
>>  compliant.
>>  > > Shouldn't we be able to use the interrupt-map and related DT
>>  > > properties to express the fact that Root Port virtual INTx is routed
>>  > > to platform interrupt Y, even without any special-case code in
>>  > > portdrv?
>>  >
>>  > My understanding is if RC also raise interrupt on INTx, then below map
>>  > make sense, where either EP or RC can raise interrupt and the line
>>  > will be muxed for interrupt from EP or RC port.
>>  
>>  I'm sorry, I didn't quite catch your meaning here, so let me try to
>>  clarify some terminology.  Maybe we'll eventually blunder into a common
>>  understanding :)
>>  
>>  INTx is a PCI concept and only means something in the PCI hierarchy.
>>  The RC would *receive* virtual INTx interrupts from the PCI hierarchy
>>  and turn them into some platform-specific interrupt (not INTx) on the
>>  upstream side.
>>  
>>  So strictly speaking, the RC might raise platform-specific interrupts
>>  when it receives INTx interrupts, but it would not raise any INTx
>>  interrupts itself.
>>  
>>  > Here is the DT entry in PCIE keystone for Legacy interrupt mapping to
>>  > host interrupt.
>>  >                         #interrupt-cells = <1>;
>>  >                         interrupt-map-mask = <0 0 0 7>;
>>  >                         interrupt-map = <0 0 0 1 &pcie_intc0 0>, /*
>>  INT A */
>>  >                                         <0 0 0 2 &pcie_intc0 1>, /*
>>  INT B */
>>  >                                         <0 0 0 3 &pcie_intc0 2>, /*
>>  INT C */
>>  >                                         <0 0 0 4 &pcie_intc0 3>; /*
>>  > INT D */
>>  
>>  If I understand correctly, this is the mapping from the PCI world (INTA,
>>  INTB, etc.) to the platform-specific world (pcie_intc0 0, etc.)
>>  
>>  If a Root Port raises a virtual INTA, the RC should turn it into the
>>  corresponding platform interrupt, i.e., GIC_SPI 48 from the description
>>  below.
>>  
>>  > And  then
>>  >
>>  >                         pcie_intc0: legacy-interrupt-controller {
>>  >                                 interrupt-controller;
>>  >                                 #interrupt-cells = <1>;
>>  >                                 interrupt-parent = <&gic>;
>>  >                                 interrupts = <GIC_SPI 48
>>  IRQ_TYPE_EDGE_RISING>,
>>  >                                         <GIC_SPI 49
>>  IRQ_TYPE_EDGE_RISING>,
>>  >                                         <GIC_SPI 50
>>  IRQ_TYPE_EDGE_RISING>,
>>  >                                         <GIC_SPI 51
>>  IRQ_TYPE_EDGE_RISING>;
>>  >                         };
>>  >
>>  > So if RC interrupt for Power management and Error interrupt is a
>>  > separate line, how can software describe that using the above
>>  interrupt map?
>>  
>>  I don't know anything about interrupts the RC might generate on its own
>>  behalf.  A lot of the RC behavior is not specified by the PCIe spec
>>  because the RC is on the border between the upstream platform-specific
>>  stuff and the downstream PCIe stuff.  Is there something in the PCIe
>>  spec about the power management and error interrupts you're talking
>>  about?  Of maybe you can point me to a section of the Keystone spec that
>>  talks about interrupts generated by the RC?
> 
> Below is one of the PCIE controller interrupts list: 
> 
> 142 PEX1 INTA 
> 143 PEX1 INTB 
> 144 PEX1 INTC 
> 145 PEX1 INTD 
> 146-147 Reserved
> 148 PEX1 MSI 
> 149 PEX1 PME 
> 150 PEX1 CFG err interrupt
> 
> Only the "150 PEX1 CFG err interrupt" routing to the aer interrupt. 
> 
> 
>>  
>>  In any event, the AER interrupts we're looking for in portdrv are from
>>  the Root Port, not from the RC.  For INTx, my understanding is that the
>>  RC *transforms* virtual INTx messages from the Root Port in the PCIe
>>  domain into GIC transactions in the platform domain.
>>  
>>  Bjorn


-- 
Murali Karicheri
Linux Kernel, Keystone