diff mbox

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

Message ID 1464242406-20203-1-git-send-email-po.liu@nxp.com
State Changes Requested
Headers show

Commit Message

Po Liu May 26, 2016, 6 a.m. UTC
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.

Signed-off-by: Po Liu <po.liu@nxp.com>
---
 drivers/pci/pcie/portdrv_core.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

Comments

Bjorn Helgaas June 2, 2016, 3:48 a.m. UTC | #1
[+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?

My guess is that your device *does* support INTx, and we should use
that.  ACPI has the generic _PRT that describes INTx routing.  There
must be something similar for DT, but I don't know what it is.  It
seems like this should be described using that DT mechanism (whatever
it is), and we shouldn't need special-case code in the portdrv code.

> Signed-off-by: Po Liu <po.liu@nxp.com>
> ---
>  drivers/pci/pcie/portdrv_core.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 32d4d0a..64833e5 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -15,6 +15,7 @@
>  #include <linux/slab.h>
>  #include <linux/pcieport_if.h>
>  #include <linux/aer.h>
> +#include <linux/of_irq.h>
>  
>  #include "../pci.h"
>  #include "portdrv.h"
> @@ -199,6 +200,28 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *vectors, int mask)
>  static int init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  {
>  	int i, irq = -1;
> +	int ret;
> +	struct device_node *np = NULL;
> +
> +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
> +		irqs[i] = 0;
> +
> +	if (dev->bus->dev.of_node)
> +		np = dev->bus->dev.of_node;
> +
> +	/* If root port doesn't support MSI/MSI-X/INTx in RC mode,
> +	 * request irq for aer
> +	 */
> +	if (IS_ENABLED(CONFIG_OF_IRQ) && np &&
> +			(mask & PCIE_PORT_SERVICE_PME)) {
> +		ret = of_irq_get_byname(np, "aer");
> +		if (ret > 0) {
> +			irqs[PCIE_PORT_SERVICE_AER_SHIFT] = ret;
> +			if (dev->irq)
> +				irq = dev->irq;
> +			goto no_msi;
> +		}
> +	}
>  
>  	/*
>  	 * If MSI cannot be used for PCIe PME or hotplug, we have to use
> @@ -224,11 +247,13 @@ static int init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  		irq = dev->irq;
>  
>   no_msi:
> -	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
> -		irqs[i] = irq;
> +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
> +		if (!irqs[i])
> +			irqs[i] = irq;
> +	}
>  	irqs[PCIE_PORT_SERVICE_VC_SHIFT] = -1;
>  
> -	if (irq < 0)
> +	if (irq < 0 && irqs[PCIE_PORT_SERVICE_AER_SHIFT] < 0)
>  		return -ENODEV;
>  	return 0;
>  }
> -- 
> 2.1.0.27.g96db324
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Po Liu June 2, 2016, 5:01 a.m. UTC | #2
Hi Bjorn,


>  -----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.
And at same time, AER capability list in PCIe register descriptors. And also,
The Root Error Command/status register was proper to enable/disable the AER interrupt.

This hardware implementation make sense in some platforms, and this was described
in the function init_service_irqs() in the drivers/pci/pcie/portdrv_core.c in current
kernel as:

241      * We're not going to use MSI-X, so try MSI and fall back to INTx.     
242      * If neither MSI/MSI-X nor INTx available, try other interrupt.  On  
243      * some platforms, root port doesn't support MSI/MSI-X/INTx in RC mode
244      */                                                                             

So I think this was the proper place to update the irq number before aer service
driver was loaded.

>  

>  My guess is that your device *does* support INTx, and we should use that.

>  ACPI has the generic _PRT that describes INTx routing.  There must be

>  something similar for DT, but I don't know what it is.  It seems like

>  this should be described using that DT mechanism (whatever it is), and

>  we shouldn't need special-case code in the portdrv code.

>  

>  > Signed-off-by: Po Liu <po.liu@nxp.com>

>  > ---

>  >  drivers/pci/pcie/portdrv_core.c | 31 ++++++++++++++++++++++++++++---

>  >  1 file changed, 28 insertions(+), 3 deletions(-)

>  >

>  > diff --git a/drivers/pci/pcie/portdrv_core.c

>  > b/drivers/pci/pcie/portdrv_core.c index 32d4d0a..64833e5 100644

>  > --- a/drivers/pci/pcie/portdrv_core.c

>  > +++ b/drivers/pci/pcie/portdrv_core.c

>  > @@ -15,6 +15,7 @@

>  >  #include <linux/slab.h>

>  >  #include <linux/pcieport_if.h>

>  >  #include <linux/aer.h>

>  > +#include <linux/of_irq.h>

>  >

>  >  #include "../pci.h"

>  >  #include "portdrv.h"

>  > @@ -199,6 +200,28 @@ static int pcie_port_enable_msix(struct pci_dev

>  > *dev, int *vectors, int mask)  static int init_service_irqs(struct

>  > pci_dev *dev, int *irqs, int mask)  {

>  >  	int i, irq = -1;

>  > +	int ret;

>  > +	struct device_node *np = NULL;

>  > +

>  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)

>  > +		irqs[i] = 0;

>  > +

>  > +	if (dev->bus->dev.of_node)

>  > +		np = dev->bus->dev.of_node;

>  > +

>  > +	/* If root port doesn't support MSI/MSI-X/INTx in RC mode,

>  > +	 * request irq for aer

>  > +	 */

>  > +	if (IS_ENABLED(CONFIG_OF_IRQ) && np &&

>  > +			(mask & PCIE_PORT_SERVICE_PME)) {

>  > +		ret = of_irq_get_byname(np, "aer");

>  > +		if (ret > 0) {

>  > +			irqs[PCIE_PORT_SERVICE_AER_SHIFT] = ret;

>  > +			if (dev->irq)

>  > +				irq = dev->irq;

>  > +			goto no_msi;

>  > +		}

>  > +	}

>  >

>  >  	/*

>  >  	 * If MSI cannot be used for PCIe PME or hotplug, we have to use

>  @@

>  > -224,11 +247,13 @@ static int init_service_irqs(struct pci_dev *dev,

>  int *irqs, int mask)

>  >  		irq = dev->irq;

>  >

>  >   no_msi:

>  > -	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)

>  > -		irqs[i] = irq;

>  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {

>  > +		if (!irqs[i])

>  > +			irqs[i] = irq;

>  > +	}

>  >  	irqs[PCIE_PORT_SERVICE_VC_SHIFT] = -1;

>  >

>  > -	if (irq < 0)

>  > +	if (irq < 0 && irqs[PCIE_PORT_SERVICE_AER_SHIFT] < 0)

>  >  		return -ENODEV;

>  >  	return 0;

>  >  }

>  > --

>  > 2.1.0.27.g96db324

>  >

>  >

>  > _______________________________________________

>  > linux-arm-kernel mailing list

>  > linux-arm-kernel@lists.infradead.org

>  > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Bjorn Helgaas June 2, 2016, 1:55 p.m. UTC | #3
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.

What's on the other end of those interrupts is outside the scope of
PCI.  An INTx interrupt (either a conventional PCI wire or a PCIe
virtual INTx wire) might be connected to an IOAPIC, an ARM SPI, or
something else.  Drivers should not care about how it is connected,
and that's why I don't think this code really fits in portdrv.

Maybe your Root Port is non-compliant in some way and maybe we need a
quirk or something to work around that hardware defect.  But I'm not
convinced yet that we have that sort of problem.  It seems like we
just don't have the correct DT description.

> And at same time, AER capability list
> in PCIe register descriptors. And also, The Root Error
> Command/status register was proper to enable/disable the AER
> interrupt.

I'm not sure what you're saying here.  Here's what I think you said;
please correct me if I'm wrong:

  - Your Root Port has an AER capability.

  - Your Root Port does not support MSI or MSI-X.  (This would
    actually be a spec violation because the PCIe spec r3.0, sec 7.7
    says "All PCI Express device Functions that are capable of
    generating interrupts must implement MSI or MSI-X or both.")
  
  - The three enable bits in the Root Error Command Register enable or
    disable generation of interrupts.  These enable bits control all
    interrupts, whether MSI, MSI-X, or INTx.

  - The Root Error Status Register contains an "Advanced Error
    Interrupt Message Number" field, but that's only applicable if MSI
    or MSI-X is enabled, and if your device doesn't support MSI or
    MSI-X, this field doesn't apply.

> This hardware implementation make sense in some platforms, and this
> was described in the function init_service_irqs() in the
> drivers/pci/pcie/portdrv_core.c in current kernel as:

> 
> 241      * We're not going to use MSI-X, so try MSI and fall back to INTx.     
> 242      * If neither MSI/MSI-X nor INTx available, try other interrupt.  On  
> 243      * some platforms, root port doesn't support MSI/MSI-X/INTx in RC mode
> 244      */                                                                             
> 
> So I think this was the proper place to update the irq number before aer service
> driver was loaded.
> 
> >  
> >  My guess is that your device *does* support INTx, and we should use that.
> >  ACPI has the generic _PRT that describes INTx routing.  There must be
> >  something similar for DT, but I don't know what it is.  It seems like
> >  this should be described using that DT mechanism (whatever it is), and
> >  we shouldn't need special-case code in the portdrv code.
> >  
> >  > Signed-off-by: Po Liu <po.liu@nxp.com>
> >  > ---
> >  >  drivers/pci/pcie/portdrv_core.c | 31 ++++++++++++++++++++++++++++---
> >  >  1 file changed, 28 insertions(+), 3 deletions(-)
> >  >
> >  > diff --git a/drivers/pci/pcie/portdrv_core.c
> >  > b/drivers/pci/pcie/portdrv_core.c index 32d4d0a..64833e5 100644
> >  > --- a/drivers/pci/pcie/portdrv_core.c
> >  > +++ b/drivers/pci/pcie/portdrv_core.c
> >  > @@ -15,6 +15,7 @@
> >  >  #include <linux/slab.h>
> >  >  #include <linux/pcieport_if.h>
> >  >  #include <linux/aer.h>
> >  > +#include <linux/of_irq.h>
> >  >
> >  >  #include "../pci.h"
> >  >  #include "portdrv.h"
> >  > @@ -199,6 +200,28 @@ static int pcie_port_enable_msix(struct pci_dev
> >  > *dev, int *vectors, int mask)  static int init_service_irqs(struct
> >  > pci_dev *dev, int *irqs, int mask)  {
> >  >  	int i, irq = -1;
> >  > +	int ret;
> >  > +	struct device_node *np = NULL;
> >  > +
> >  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
> >  > +		irqs[i] = 0;
> >  > +
> >  > +	if (dev->bus->dev.of_node)
> >  > +		np = dev->bus->dev.of_node;
> >  > +
> >  > +	/* If root port doesn't support MSI/MSI-X/INTx in RC mode,
> >  > +	 * request irq for aer
> >  > +	 */
> >  > +	if (IS_ENABLED(CONFIG_OF_IRQ) && np &&
> >  > +			(mask & PCIE_PORT_SERVICE_PME)) {
> >  > +		ret = of_irq_get_byname(np, "aer");
> >  > +		if (ret > 0) {
> >  > +			irqs[PCIE_PORT_SERVICE_AER_SHIFT] = ret;
> >  > +			if (dev->irq)
> >  > +				irq = dev->irq;
> >  > +			goto no_msi;
> >  > +		}
> >  > +	}
> >  >
> >  >  	/*
> >  >  	 * If MSI cannot be used for PCIe PME or hotplug, we have to use
> >  @@
> >  > -224,11 +247,13 @@ static int init_service_irqs(struct pci_dev *dev,
> >  int *irqs, int mask)
> >  >  		irq = dev->irq;
> >  >
> >  >   no_msi:
> >  > -	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
> >  > -		irqs[i] = irq;
> >  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
> >  > +		if (!irqs[i])
> >  > +			irqs[i] = irq;
> >  > +	}
> >  >  	irqs[PCIE_PORT_SERVICE_VC_SHIFT] = -1;
> >  >
> >  > -	if (irq < 0)
> >  > +	if (irq < 0 && irqs[PCIE_PORT_SERVICE_AER_SHIFT] < 0)
> >  >  		return -ENODEV;
> >  >  	return 0;
> >  >  }
> >  > --
> >  > 2.1.0.27.g96db324
> >  >
> >  >
> >  > _______________________________________________
> >  > linux-arm-kernel mailing list
> >  > linux-arm-kernel@lists.infradead.org
> >  > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Murali Karicheri June 2, 2016, 3:37 p.m. UTC | #4
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.
> 
Bjorn.

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.

> What's on the other end of those interrupts is outside the scope of
> PCI.  An INTx interrupt (either a conventional PCI wire or a PCIe
> virtual INTx wire) might be connected to an IOAPIC, an ARM SPI, or
> something else.  Drivers should not care about how it is connected,
> and that's why I don't think this code really fits in portdrv.
> 
> Maybe your Root Port is non-compliant in some way and maybe we need a
> quirk or something to work around that hardware defect.  But I'm not
> convinced yet that we have that sort of problem.  It seems like we
> just don't have the correct DT description.

Is quirk is what is suggested here to support this?

Murali
> 
>> And at same time, AER capability list
>> in PCIe register descriptors. And also, The Root Error
>> Command/status register was proper to enable/disable the AER
>> interrupt.
> 
> I'm not sure what you're saying here.  Here's what I think you said;
> please correct me if I'm wrong:
> 
>   - Your Root Port has an AER capability.
> 
>   - Your Root Port does not support MSI or MSI-X.  (This would
>     actually be a spec violation because the PCIe spec r3.0, sec 7.7
>     says "All PCI Express device Functions that are capable of
>     generating interrupts must implement MSI or MSI-X or both.")
>   
>   - The three enable bits in the Root Error Command Register enable or
>     disable generation of interrupts.  These enable bits control all
>     interrupts, whether MSI, MSI-X, or INTx.
> 
>   - The Root Error Status Register contains an "Advanced Error
>     Interrupt Message Number" field, but that's only applicable if MSI
>     or MSI-X is enabled, and if your device doesn't support MSI or
>     MSI-X, this field doesn't apply.
> 
>> This hardware implementation make sense in some platforms, and this
>> was described in the function init_service_irqs() in the
>> drivers/pci/pcie/portdrv_core.c in current kernel as:
> 
>>
>> 241      * We're not going to use MSI-X, so try MSI and fall back to INTx.     
>> 242      * If neither MSI/MSI-X nor INTx available, try other interrupt.  On  
>> 243      * some platforms, root port doesn't support MSI/MSI-X/INTx in RC mode
>> 244      */                                                                             
>>
>> So I think this was the proper place to update the irq number before aer service
>> driver was loaded.
>>
>>>  
>>>  My guess is that your device *does* support INTx, and we should use that.
>>>  ACPI has the generic _PRT that describes INTx routing.  There must be
>>>  something similar for DT, but I don't know what it is.  It seems like
>>>  this should be described using that DT mechanism (whatever it is), and
>>>  we shouldn't need special-case code in the portdrv code.
>>>  
>>>  > Signed-off-by: Po Liu <po.liu@nxp.com>
>>>  > ---
>>>  >  drivers/pci/pcie/portdrv_core.c | 31 ++++++++++++++++++++++++++++---
>>>  >  1 file changed, 28 insertions(+), 3 deletions(-)
>>>  >
>>>  > diff --git a/drivers/pci/pcie/portdrv_core.c
>>>  > b/drivers/pci/pcie/portdrv_core.c index 32d4d0a..64833e5 100644
>>>  > --- a/drivers/pci/pcie/portdrv_core.c
>>>  > +++ b/drivers/pci/pcie/portdrv_core.c
>>>  > @@ -15,6 +15,7 @@
>>>  >  #include <linux/slab.h>
>>>  >  #include <linux/pcieport_if.h>
>>>  >  #include <linux/aer.h>
>>>  > +#include <linux/of_irq.h>
>>>  >
>>>  >  #include "../pci.h"
>>>  >  #include "portdrv.h"
>>>  > @@ -199,6 +200,28 @@ static int pcie_port_enable_msix(struct pci_dev
>>>  > *dev, int *vectors, int mask)  static int init_service_irqs(struct
>>>  > pci_dev *dev, int *irqs, int mask)  {
>>>  >  	int i, irq = -1;
>>>  > +	int ret;
>>>  > +	struct device_node *np = NULL;
>>>  > +
>>>  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
>>>  > +		irqs[i] = 0;
>>>  > +
>>>  > +	if (dev->bus->dev.of_node)
>>>  > +		np = dev->bus->dev.of_node;
>>>  > +
>>>  > +	/* If root port doesn't support MSI/MSI-X/INTx in RC mode,
>>>  > +	 * request irq for aer
>>>  > +	 */
>>>  > +	if (IS_ENABLED(CONFIG_OF_IRQ) && np &&
>>>  > +			(mask & PCIE_PORT_SERVICE_PME)) {
>>>  > +		ret = of_irq_get_byname(np, "aer");
>>>  > +		if (ret > 0) {
>>>  > +			irqs[PCIE_PORT_SERVICE_AER_SHIFT] = ret;
>>>  > +			if (dev->irq)
>>>  > +				irq = dev->irq;
>>>  > +			goto no_msi;
>>>  > +		}
>>>  > +	}
>>>  >
>>>  >  	/*
>>>  >  	 * If MSI cannot be used for PCIe PME or hotplug, we have to use
>>>  @@
>>>  > -224,11 +247,13 @@ static int init_service_irqs(struct pci_dev *dev,
>>>  int *irqs, int mask)
>>>  >  		irq = dev->irq;
>>>  >
>>>  >   no_msi:
>>>  > -	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
>>>  > -		irqs[i] = irq;
>>>  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
>>>  > +		if (!irqs[i])
>>>  > +			irqs[i] = irq;
>>>  > +	}
>>>  >  	irqs[PCIE_PORT_SERVICE_VC_SHIFT] = -1;
>>>  >
>>>  > -	if (irq < 0)
>>>  > +	if (irq < 0 && irqs[PCIE_PORT_SERVICE_AER_SHIFT] < 0)
>>>  >  		return -ENODEV;
>>>  >  	return 0;
>>>  >  }
>>>  > --
>>>  > 2.1.0.27.g96db324
>>>  >
>>>  >
>>>  > _______________________________________________
>>>  > linux-arm-kernel mailing list
>>>  > linux-arm-kernel@lists.infradead.org
>>>  > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Bjorn Helgaas June 3, 2016, 4:09 a.m. UTC | #5
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.

I assume you have a Root Port with an AER capability, no MSI
capability, and no MSI-X capability, right?  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.

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?

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?

> > What's on the other end of those interrupts is outside the scope of
> > PCI.  An INTx interrupt (either a conventional PCI wire or a PCIe
> > virtual INTx wire) might be connected to an IOAPIC, an ARM SPI, or
> > something else.  Drivers should not care about how it is connected,
> > and that's why I don't think this code really fits in portdrv.
> > 
> > Maybe your Root Port is non-compliant in some way and maybe we need a
> > quirk or something to work around that hardware defect.  But I'm not
> > convinced yet that we have that sort of problem.  It seems like we
> > just don't have the correct DT description.
> 
> Is quirk is what is suggested here to support this?

No, I don't understand the problem yet, so I'm not ready to suggest a
solution.

> >> And at same time, AER capability list
> >> in PCIe register descriptors. And also, The Root Error
> >> Command/status register was proper to enable/disable the AER
> >> interrupt.
> > 
> > I'm not sure what you're saying here.  Here's what I think you said;
> > please correct me if I'm wrong:
> > 
> >   - Your Root Port has an AER capability.
> > 
> >   - Your Root Port does not support MSI or MSI-X.  (This would
> >     actually be a spec violation because the PCIe spec r3.0, sec 7.7
> >     says "All PCI Express device Functions that are capable of
> >     generating interrupts must implement MSI or MSI-X or both.")
> >   
> >   - The three enable bits in the Root Error Command Register enable or
> >     disable generation of interrupts.  These enable bits control all
> >     interrupts, whether MSI, MSI-X, or INTx.
> > 
> >   - The Root Error Status Register contains an "Advanced Error
> >     Interrupt Message Number" field, but that's only applicable if MSI
> >     or MSI-X is enabled, and if your device doesn't support MSI or
> >     MSI-X, this field doesn't apply.
> > 
> >> This hardware implementation make sense in some platforms, and this
> >> was described in the function init_service_irqs() in the
> >> drivers/pci/pcie/portdrv_core.c in current kernel as:
> > 
> >>
> >> 241      * We're not going to use MSI-X, so try MSI and fall back to INTx.     
> >> 242      * If neither MSI/MSI-X nor INTx available, try other interrupt.  On  
> >> 243      * some platforms, root port doesn't support MSI/MSI-X/INTx in RC mode
> >> 244      */                                                                             
> >>
> >> So I think this was the proper place to update the irq number before aer service
> >> driver was loaded.
> >>
> >>>  
> >>>  My guess is that your device *does* support INTx, and we should use that.
> >>>  ACPI has the generic _PRT that describes INTx routing.  There must be
> >>>  something similar for DT, but I don't know what it is.  It seems like
> >>>  this should be described using that DT mechanism (whatever it is), and
> >>>  we shouldn't need special-case code in the portdrv code.
> >>>  
> >>>  > Signed-off-by: Po Liu <po.liu@nxp.com>
> >>>  > ---
> >>>  >  drivers/pci/pcie/portdrv_core.c | 31 ++++++++++++++++++++++++++++---
> >>>  >  1 file changed, 28 insertions(+), 3 deletions(-)
> >>>  >
> >>>  > diff --git a/drivers/pci/pcie/portdrv_core.c
> >>>  > b/drivers/pci/pcie/portdrv_core.c index 32d4d0a..64833e5 100644
> >>>  > --- a/drivers/pci/pcie/portdrv_core.c
> >>>  > +++ b/drivers/pci/pcie/portdrv_core.c
> >>>  > @@ -15,6 +15,7 @@
> >>>  >  #include <linux/slab.h>
> >>>  >  #include <linux/pcieport_if.h>
> >>>  >  #include <linux/aer.h>
> >>>  > +#include <linux/of_irq.h>
> >>>  >
> >>>  >  #include "../pci.h"
> >>>  >  #include "portdrv.h"
> >>>  > @@ -199,6 +200,28 @@ static int pcie_port_enable_msix(struct pci_dev
> >>>  > *dev, int *vectors, int mask)  static int init_service_irqs(struct
> >>>  > pci_dev *dev, int *irqs, int mask)  {
> >>>  >  	int i, irq = -1;
> >>>  > +	int ret;
> >>>  > +	struct device_node *np = NULL;
> >>>  > +
> >>>  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
> >>>  > +		irqs[i] = 0;
> >>>  > +
> >>>  > +	if (dev->bus->dev.of_node)
> >>>  > +		np = dev->bus->dev.of_node;
> >>>  > +
> >>>  > +	/* If root port doesn't support MSI/MSI-X/INTx in RC mode,
> >>>  > +	 * request irq for aer
> >>>  > +	 */
> >>>  > +	if (IS_ENABLED(CONFIG_OF_IRQ) && np &&
> >>>  > +			(mask & PCIE_PORT_SERVICE_PME)) {
> >>>  > +		ret = of_irq_get_byname(np, "aer");
> >>>  > +		if (ret > 0) {
> >>>  > +			irqs[PCIE_PORT_SERVICE_AER_SHIFT] = ret;
> >>>  > +			if (dev->irq)
> >>>  > +				irq = dev->irq;
> >>>  > +			goto no_msi;
> >>>  > +		}
> >>>  > +	}
> >>>  >
> >>>  >  	/*
> >>>  >  	 * If MSI cannot be used for PCIe PME or hotplug, we have to use
> >>>  @@
> >>>  > -224,11 +247,13 @@ static int init_service_irqs(struct pci_dev *dev,
> >>>  int *irqs, int mask)
> >>>  >  		irq = dev->irq;
> >>>  >
> >>>  >   no_msi:
> >>>  > -	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
> >>>  > -		irqs[i] = irq;
> >>>  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
> >>>  > +		if (!irqs[i])
> >>>  > +			irqs[i] = irq;
> >>>  > +	}
> >>>  >  	irqs[PCIE_PORT_SERVICE_VC_SHIFT] = -1;
> >>>  >
> >>>  > -	if (irq < 0)
> >>>  > +	if (irq < 0 && irqs[PCIE_PORT_SERVICE_AER_SHIFT] < 0)
> >>>  >  		return -ENODEV;
> >>>  >  	return 0;
> >>>  >  }
> >>>  > --
> >>>  > 2.1.0.27.g96db324
> >>>  >
> >>>  >
> >>>  > _______________________________________________
> >>>  > linux-arm-kernel mailing list
> >>>  > linux-arm-kernel@lists.infradead.org
> >>>  > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 
> -- 
> Murali Karicheri
> Linux Kernel, Keystone
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Murali Karicheri June 3, 2016, 5:31 p.m. UTC | #6
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.

> 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.

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 */


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?

Murali
> 
>>> What's on the other end of those interrupts is outside the scope of
>>> PCI.  An INTx interrupt (either a conventional PCI wire or a PCIe
>>> virtual INTx wire) might be connected to an IOAPIC, an ARM SPI, or
>>> something else.  Drivers should not care about how it is connected,
>>> and that's why I don't think this code really fits in portdrv.
>>>
>>> Maybe your Root Port is non-compliant in some way and maybe we need a
>>> quirk or something to work around that hardware defect.  But I'm not
>>> convinced yet that we have that sort of problem.  It seems like we
>>> just don't have the correct DT description.
>>
>> Is quirk is what is suggested here to support this?
> 
> No, I don't understand the problem yet, so I'm not ready to suggest a
> solution.
> 
>>>> And at same time, AER capability list
>>>> in PCIe register descriptors. And also, The Root Error
>>>> Command/status register was proper to enable/disable the AER
>>>> interrupt.
>>>
>>> I'm not sure what you're saying here.  Here's what I think you said;
>>> please correct me if I'm wrong:
>>>
>>>   - Your Root Port has an AER capability.
>>>
>>>   - Your Root Port does not support MSI or MSI-X.  (This would
>>>     actually be a spec violation because the PCIe spec r3.0, sec 7.7
>>>     says "All PCI Express device Functions that are capable of
>>>     generating interrupts must implement MSI or MSI-X or both.")
>>>   
>>>   - The three enable bits in the Root Error Command Register enable or
>>>     disable generation of interrupts.  These enable bits control all
>>>     interrupts, whether MSI, MSI-X, or INTx.
>>>
>>>   - The Root Error Status Register contains an "Advanced Error
>>>     Interrupt Message Number" field, but that's only applicable if MSI
>>>     or MSI-X is enabled, and if your device doesn't support MSI or
>>>     MSI-X, this field doesn't apply.
>>>
>>>> This hardware implementation make sense in some platforms, and this
>>>> was described in the function init_service_irqs() in the
>>>> drivers/pci/pcie/portdrv_core.c in current kernel as:
>>>
>>>>
>>>> 241      * We're not going to use MSI-X, so try MSI and fall back to INTx.     
>>>> 242      * If neither MSI/MSI-X nor INTx available, try other interrupt.  On  
>>>> 243      * some platforms, root port doesn't support MSI/MSI-X/INTx in RC mode
>>>> 244      */                                                                             
>>>>
>>>> So I think this was the proper place to update the irq number before aer service
>>>> driver was loaded.
>>>>
>>>>>  
>>>>>  My guess is that your device *does* support INTx, and we should use that.
>>>>>  ACPI has the generic _PRT that describes INTx routing.  There must be
>>>>>  something similar for DT, but I don't know what it is.  It seems like
>>>>>  this should be described using that DT mechanism (whatever it is), and
>>>>>  we shouldn't need special-case code in the portdrv code.
>>>>>  
>>>>>  > Signed-off-by: Po Liu <po.liu@nxp.com>
>>>>>  > ---
>>>>>  >  drivers/pci/pcie/portdrv_core.c | 31 ++++++++++++++++++++++++++++---
>>>>>  >  1 file changed, 28 insertions(+), 3 deletions(-)
>>>>>  >
>>>>>  > diff --git a/drivers/pci/pcie/portdrv_core.c
>>>>>  > b/drivers/pci/pcie/portdrv_core.c index 32d4d0a..64833e5 100644
>>>>>  > --- a/drivers/pci/pcie/portdrv_core.c
>>>>>  > +++ b/drivers/pci/pcie/portdrv_core.c
>>>>>  > @@ -15,6 +15,7 @@
>>>>>  >  #include <linux/slab.h>
>>>>>  >  #include <linux/pcieport_if.h>
>>>>>  >  #include <linux/aer.h>
>>>>>  > +#include <linux/of_irq.h>
>>>>>  >
>>>>>  >  #include "../pci.h"
>>>>>  >  #include "portdrv.h"
>>>>>  > @@ -199,6 +200,28 @@ static int pcie_port_enable_msix(struct pci_dev
>>>>>  > *dev, int *vectors, int mask)  static int init_service_irqs(struct
>>>>>  > pci_dev *dev, int *irqs, int mask)  {
>>>>>  >  	int i, irq = -1;
>>>>>  > +	int ret;
>>>>>  > +	struct device_node *np = NULL;
>>>>>  > +
>>>>>  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
>>>>>  > +		irqs[i] = 0;
>>>>>  > +
>>>>>  > +	if (dev->bus->dev.of_node)
>>>>>  > +		np = dev->bus->dev.of_node;
>>>>>  > +
>>>>>  > +	/* If root port doesn't support MSI/MSI-X/INTx in RC mode,
>>>>>  > +	 * request irq for aer
>>>>>  > +	 */
>>>>>  > +	if (IS_ENABLED(CONFIG_OF_IRQ) && np &&
>>>>>  > +			(mask & PCIE_PORT_SERVICE_PME)) {
>>>>>  > +		ret = of_irq_get_byname(np, "aer");
>>>>>  > +		if (ret > 0) {
>>>>>  > +			irqs[PCIE_PORT_SERVICE_AER_SHIFT] = ret;
>>>>>  > +			if (dev->irq)
>>>>>  > +				irq = dev->irq;
>>>>>  > +			goto no_msi;
>>>>>  > +		}
>>>>>  > +	}
>>>>>  >
>>>>>  >  	/*
>>>>>  >  	 * If MSI cannot be used for PCIe PME or hotplug, we have to use
>>>>>  @@
>>>>>  > -224,11 +247,13 @@ static int init_service_irqs(struct pci_dev *dev,
>>>>>  int *irqs, int mask)
>>>>>  >  		irq = dev->irq;
>>>>>  >
>>>>>  >   no_msi:
>>>>>  > -	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
>>>>>  > -		irqs[i] = irq;
>>>>>  > +	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
>>>>>  > +		if (!irqs[i])
>>>>>  > +			irqs[i] = irq;
>>>>>  > +	}
>>>>>  >  	irqs[PCIE_PORT_SERVICE_VC_SHIFT] = -1;
>>>>>  >
>>>>>  > -	if (irq < 0)
>>>>>  > +	if (irq < 0 && irqs[PCIE_PORT_SERVICE_AER_SHIFT] < 0)
>>>>>  >  		return -ENODEV;
>>>>>  >  	return 0;
>>>>>  >  }
>>>>>  > --
>>>>>  > 2.1.0.27.g96db324
>>>>>  >
>>>>>  >
>>>>>  > _______________________________________________
>>>>>  > linux-arm-kernel mailing list
>>>>>  > linux-arm-kernel@lists.infradead.org
>>>>>  > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>> -- 
>> Murali Karicheri
>> Linux Kernel, Keystone
Bjorn Helgaas June 4, 2016, 3:48 a.m. UTC | #7
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.

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.

> > 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?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Po Liu June 6, 2016, 7:32 a.m. UTC | #8
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.

>  

>  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.

>  

>  > > 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 June 6, 2016, 2:01 p.m. UTC | #9
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
Bjorn Helgaas June 6, 2016, 6:10 p.m. UTC | #10
On Mon, Jun 06, 2016 at 10:01:44AM -0400, Murali Karicheri wrote:
> 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.

I'm looking at the "Error Message Controls" diagram in the PCIe spec
r3.0, sec 6.2.6.  Does this hardware fit into the platform-specific
"System Error" case there?  Do the Root Control enable bits (in the
PCIe Capability) control this interrupt?  If so, maybe this makes more
sense than I thought.

We have to assume both notification paths work (the normal
INTx/MSI/MSI-X AER interrupts as well as the platform-specific System
Errors), so if we add support for System Errors, it should be
structured so we prefer INTx/MSI/MSI-X, and only fall back to System
Errors if they don't work.  AER would have to know which path it's
using so aer_enable_rootport() can disable the other one (currently it
always disables System Errors).

Then you'd have to add quirks to mark MSI/MSI-X/INTx as being broken
on your devices to force the fallback to System Errors.

How much would this screw up other PCIe services (PME, hotplug, VC,
etc)?  Does MSI work for them?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Po Liu June 7, 2016, 10:07 a.m. UTC | #11
SGkgQmpvcm4sDQoNCj4gIC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+ICANCj4gIE9uIE1v
biwgSnVuIDA2LCAyMDE2IGF0IDEwOjAxOjQ0QU0gLTA0MDAsIE11cmFsaSBLYXJpY2hlcmkgd3Jv
dGU6DQo+ICA+IE9uIDA2LzA2LzIwMTYgMDM6MzIgQU0sIFBvIExpdSB3cm90ZToNCj4gID4gPiBI
aSBCam9ybiwNCj4gID4gPiBJIGNvbmZpcm0gd2UgbWV0IHNhbWUgcHJvYmxlbSB3aXRoIEtleVN0
b25lIGJhc2Ugb24gRGVzaWduV2FyZQ0KPiAgZGVzaWduLg0KPiAgPiA+DQo+ICA+ID4NCj4gID4g
PiBCZXN0IHJlZ2FyZHMsDQo+ICA+ID4gTGl1IFBvDQo+ICA+ID4NCj4gID4gPj4gIC0tLS0tT3Jp
Z2luYWwgTWVzc2FnZS0tLS0tDQo+ICA+ID4+ICBGcm9tOiBCam9ybiBIZWxnYWFzIFttYWlsdG86
aGVsZ2Fhc0BrZXJuZWwub3JnXQ0KPiAgPiA+PiAgU2VudDogU2F0dXJkYXksIEp1bmUgMDQsIDIw
MTYgMTE6NDkgQU0NCj4gID4gPj4gIFRvOiBNdXJhbGkgS2FyaWNoZXJpDQo+ICA+ID4+ICBDYzog
UG8gTGl1OyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBsaW51eC1hcm0tDQo+ICA+ID4+IGtl
cm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOw0K
PiAgPiA+PiBkZXZpY2V0cmVlQHZnZXIua2VybmVsLm9yZzsgQXJuZCBCZXJnbWFubjsgUm95IFph
bmc7IE1hcmMgWnluZ2llcjsNCj4gID4gPj4gU3R1YXJ0IFlvZGVyOyBZYW5nLUxlbyBMaTsgTWlu
Z2h1YW4gTGlhbjsgQmpvcm4gSGVsZ2FhczsgU2hhd24gR3VvOw0KPiAgPiA+PiBNaW5na2FpIEh1
OyBSb2IgSGVycmluZw0KPiAgPiA+PiAgU3ViamVjdDogUmU6IFtQQVRDSCAyLzJdIGFlcjogYWRk
IHN1cHBvcnQgYWVyIGludGVycnVwdCB3aXRoIG5vbmUNCj4gID4gPj4gTVNJL01TSS1YL0lOVHgg
bW9kZQ0KPiAgPiA+Pg0KPiAgPiA+PiAgT24gRnJpLCBKdW4gMDMsIDIwMTYgYXQgMDE6MzE6MTFQ
TSAtMDQwMCwgTXVyYWxpIEthcmljaGVyaSB3cm90ZToNCj4gID4gPj4gID4gUG8sDQo+ICA+ID4+
ICA+DQo+ICA+ID4+ICA+IFNvcnJ5IHRvIGhpamFjayB5b3VyIGRpc2N1c3Npb24sIGJ1dCB0aGUg
cHJvYmxlbSBzZWVtcyB0byBiZQ0KPiAgPiA+PiBzYW1lIGZvciAgPiBLZXlzdG9uZSBQQ0kgY29u
dHJvbGxlciB3aGljaCBpcyBhbHNvIGRlc2lnbndhcmUgKG9sZA0KPiAgdmVyc2lvbikgYmFzZWQu
DQo+ICA+ID4+ICA+DQo+ICA+ID4+ICA+IE9uIDA2LzAzLzIwMTYgMTI6MDkgQU0sIEJqb3JuIEhl
bGdhYXMgd3JvdGU6DQo+ICA+ID4+ICA+ID4gT24gVGh1LCBKdW4gMDIsIDIwMTYgYXQgMTE6Mzc6
MjhBTSAtMDQwMCwgTXVyYWxpIEthcmljaGVyaQ0KPiAgd3JvdGU6DQo+ICA+ID4+ICA+ID4+IE9u
IDA2LzAyLzIwMTYgMDk6NTUgQU0sIEJqb3JuIEhlbGdhYXMgd3JvdGU6DQo+ICA+ID4+ICA+ID4+
PiBPbiBUaHUsIEp1biAwMiwgMjAxNiBhdCAwNTowMToxOUFNICswMDAwLCBQbyBMaXUgd3JvdGU6
DQo+ICA+ID4+ICA+ID4+Pj4+ICAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLSAgPiA+Pj4+PiAg
RnJvbTogQmpvcm4gSGVsZ2Fhcw0KPiAgPiA+PiBbbWFpbHRvOmhlbGdhYXNAa2VybmVsLm9yZ10g
ID4gPj4+Pj4gIFNlbnQ6IFRodXJzZGF5LCBKdW5lIDAyLCAyMDE2DQo+ICA+ID4+IDExOjQ4IEFN
ICA+ID4+Pj4+ICBUbzogUG8gTGl1ICA+ID4+Pj4+ICBDYzoNCj4gID4gPj4gbGludXgtcGNpQHZn
ZXIua2VybmVsLm9yZzsgID4gPj4+Pj4NCj4gID4gPj4gbGludXgtYXJtLWtlcm5lbEBsaXN0cy5p
bmZyYWRlYWQub3JnOw0KPiAgPiA+PiAgPiA+Pj4+PiAgbGludXgta2VybmVsQHZnZXIua2VybmVs
Lm9yZzsgZGV2aWNldHJlZUB2Z2VyLmtlcm5lbC5vcmc7DQo+ICA+ID4+IEFybmQgID4gPj4+Pj4g
QmVyZ21hbm47ICBSb3kgWmFuZzsgTWFyYyBaeW5naWVyOyBTdHVhcnQgWW9kZXI7DQo+ICA+ID4+
IFlhbmctTGVvIExpOyAgPiA+Pj4+PiBNaW5naHVhbiBMaWFuOyBCam9ybiAgSGVsZ2FhczsgU2hh
d24gR3VvOw0KPiAgPiA+PiBNaW5na2FpIEh1OyBSb2IgID4gPj4+Pj4gSGVycmluZyAgPiA+Pj4+
PiAgU3ViamVjdDogUmU6IFtQQVRDSCAyLzJdDQo+ICA+ID4+IGFlcjogYWRkIHN1cHBvcnQgYWVy
IGludGVycnVwdCB3aXRoICA+ID4+Pj4+IG5vbmUgIE1TSS9NU0ktWC9JTlR4DQo+ICA+ID4+IG1v
ZGUgID4gPj4+Pj4gID4gPj4+Pj4gIFsrY2MgUm9iXSAgPiA+Pj4+PiAgPiA+Pj4+PiAgSGkgUG8s
ICA+DQo+ICA+ID4+ID4+Pj4+ICA+ID4+Pj4+ICBPbiBUaHUsIE1heSAyNiwgMjAxNiBhdCAwMjow
MDowNlBNICswODAwLCBQbyBMaXUNCj4gID4gPj4gd3JvdGU6DQo+ICA+ID4+ICA+ID4+Pj4+ICA+
IE9uIHNvbWUgcGxhdGZvcm1zLCByb290IHBvcnQgZG9lc24ndCBzdXBwb3J0DQo+ICA+ID4+IE1T
SS9NU0ktWC9JTlR4ICBpbiBSQyBtb2RlLg0KPiAgPiA+PiAgPiA+Pj4+PiAgPiBXaGVuIGNoaXAg
c3VwcG9ydCB0aGUgYWVyIGludGVycnVwdCB3aXRoIG5vbmUNCj4gID4gPj4gTVNJL01TSS1YL0lO
VHggID4gPj4+Pj4gbW9kZSwgID4gbWF5YmUgdGhlcmUgaXMgaW50ZXJydXB0IGxpbmUgZm9yDQo+
ICA+ID4+IGFlciBwbWUgZXRjLiBTZWFyY2ggID4gPj4+Pj4gdGhlIGludGVycnVwdCAgPiBudW1i
ZXIgaW4gdGhlIGZkdA0KPiAgZmlsZS4NCj4gID4gPj4gID4gPj4+Pj4NCj4gID4gPj4gID4gPj4+
Pj4gIE15IHVuZGVyc3RhbmRpbmcgaXMgdGhhdCBBRVIgaW50ZXJydXB0IHNpZ25hbGluZyBjYW4g
YmUNCj4gID4gPj4gZG9uZSAgPiA+Pj4+PiB2aWEgSU5UeCwgIE1TSSwgb3IgTVNJLVggKFBDSWUg
c3BlYyByMy4wLCBzZWMNCj4gIDYuMi40LjEuMikuDQo+ICA+ID4+ICA+ID4+Pj4+IEFwcGFyZW50
bHkgeW91ciBkZXZpY2UgIGRvZXNuJ3Qgc3VwcG9ydCBNU0kgb3IgTVNJLVguICBBcmUNCj4gID4g
Pj4geW91ICA+ID4+Pj4+IHNheWluZyBpdCBkb2Vzbid0IHN1cHBvcnQgSU5UeCAgZWl0aGVyPyAg
SG93IGlzIHRoZQ0KPiAgPiA+PiBpbnRlcnJ1cHQgIHlvdSdyZSByZXF1ZXN0aW5nIGhlcmUgZGlm
ZmVyZW50IGZyb20gSU5UeD8NCj4gID4gPj4gID4gPj4+Pg0KPiAgPiA+PiAgPiA+Pj4+IExheWVy
c2NhcGUgdXNlIG5vbmUgb2YgTVNJIG9yIE1TSS1YIG9yIElOVHggdG8gaW5kaWNhdGUgdGhlDQo+
ICA+ID4+ID4gPj4+PiBkZXZpY2VzIG9yIHJvb3QgZXJyb3IgaW4gUkMgbW9kZS4gQnV0IHVzZSBh
biBpbmRlcGVuZGVudCBTUEkNCj4gID4gPj4gPiA+Pj4+IGludGVycnVwdChhcm0gaW50ZXJydXB0
IGNvbnRyb2xsZXIpIGxpbmUuDQo+ICA+ID4+ICA+ID4+Pg0KPiAgPiA+PiAgPiA+Pj4gVGhlIFJv
b3QgUG9ydCBpcyBhIFBDSSBkZXZpY2UgYW5kIHNob3VsZCBmb2xsb3cgdGhlIG5vcm1hbA0KPiAg
PiA+PiBQQ0kgID4gPj4+IHJ1bGVzIGZvciBpbnRlcnJ1cHRzLiAgQXMgZmFyIGFzIEkgdW5kZXJz
dGFuZCwgdGhhdA0KPiAgPiA+PiBtZWFucyBpdCAgPiA+Pj4gc2hvdWxkIHVzZSBNU0ksIE1TSS1Y
LCBvciBJTlR4LiAgSWYgeW91ciBSb290IFBvcnQNCj4gID4gPj4gZG9lc24ndCB1c2UgTVNJICA+
ID4+PiBvciBNU0ktWCwgaXQgc2hvdWxkIHVzZSBJTlR4LCB0aGUNCj4gID4gPj4gUENJX0lOVEVS
UlVQVF9QSU4gcmVnaXN0ZXIgID4gPj4+IHNob3VsZCB0ZWxsIHVzIHdoaWNoIChJTlRBLw0KPiAg
PiA+PiBJTlRCL2V0Yy4pLCBhbmQgIFBDSV9DT01NQU5EX0lOVFhfRElTQUJMRSBzaG91bGQgd29y
ayB0byBkaXNhYmxlIGl0Lg0KPiAgPiA+PiAgPiA+Pj4gVGhhdCdzIGFsbCBmcm9tIHRoZSBQQ0kg
cG9pbnQgb2Ygdmlldywgb2YgY291cnNlLg0KPiAgPiA+PiAgPiA+Pg0KPiAgPiA+PiAgPiA+PiBJ
IGFtIGZhY2VkIHdpdGggdGhlIHNhbWUgaXNzdWUgb24gS2V5c3RvbmUgUENJIGhhcmR3YXJlIGFu
ZA0KPiAgPiA+PiBpdCBoYXMgID4gPj4gYmVlbiBvbiBteSBUT0RPIGxpc3QgIGZvciBxdWl0ZSBz
b21lIHRpbWUuIEtleXN0b25lDQo+ICA+ID4+IFBDSSBoYXJkd2FyZSAgPiA+PiBhbHNvIGRvZXNu
J3QgdXNlIE1TSSBvciBNU0ktWCBvciBJTlR4IGZvcg0KPiAgPiA+PiByZXBvcnRpbmcgZXJyb3Jz
IHJlY2VpdmVkICA+ID4+IGF0IHRoZSByb290IHBvcnQsIGJ1dCB1c2UgYQ0KPiAgPiA+PiBwbGF0
Zm9ybSBpbnRlcnJ1cHQgaW5zdGVhZCAobm90ICA+ID4+IGNvbXBsYWludCB0byBQQ0kgc3RhbmRh
cmQgYXMNCj4gID4gPj4gcGVyIFBDSSBiYXNlIHNwZWMpLiBTbyBJIHdvdWxkIG5lZWQgID4gPj4g
c2ltaWxhciBjaGFuZ2UgdG8gaGF2ZQ0KPiAgPiA+PiB0aGUgZXJyb3IgaW50ZXJydXB0IHBhc3Nl
ZCB0byB0aGUgYWVyICA+ID4+IGRyaXZlci4gU28gdGhlcmUgYXJlDQo+ICA+ID4+IGhhcmR3YXJl
IG91dCB0aGVyZSBsaWtlIEtleXN0b25lIHdoaWNoICByZXF1aXJlcyB0byBzdXBwb3J0IHRoaXMN
Cj4gIHRocm91Z2ggcGxhdGZvcm0gSVJRLg0KPiAgPiA+PiAgPiA+DQo+ICA+ID4+ICA+ID4gVGhp
cyBpcyBub3QgYSBuZXcgYXJlYSBvZiB0aGUgc3BlYywgYW5kIGl0J3MgaGFyZCBmb3IgbWUgdG8N
Cj4gID4gPj4gYmVsaWV2ZSAgPiA+IHRoYXQgdGhlc2UgdHdvIG5ldyBQQ0llIGNvbnRyb2xsZXJz
IGFyZSBib3RoIGJyb2tlbg0KPiAgPiA+PiB0aGUgc2FtZSB3YXkgID4gPiAoYWx0aG91Z2ggSSBn
dWVzcyBib3RoIGFyZSBEZXNpZ25XYXJlLWJhc2VkLCBzbw0KPiAgPiA+PiBtYXliZSB0aGlzIGlz
IHRoZSAgPiA+IHNhbWUgdW5kZXJseWluZyBwcm9ibGVtIGluIGJvdGggY2FzZXM/KS4gIEkNCj4g
ID4gPj4gdGhpbmsgaXQncyBtb3JlIGxpa2VseSAgPiA+IHRoYXQgd2UganVzdCBoYXZlbid0IGZp
Z3VyZWQgb3V0IHRoZQ0KPiAgPiA+PiByaWdodCB3YXkgdG8gZGVzY3JpYmUgdGhpcyBpbiAgdGhl
IERULg0KPiAgPiA+PiAgPg0KPiAgPiA+PiAgPiBLZXlzdG9uZSBpcyB1c2luZyBhbiBvbGRlciB2
ZXJzaW9uIG9mIHRoZSBkZXNpZ253YXJlIElQIGFuZCBpdA0KPiAgPiA+PiA+IGltcGxlbWVudHMg
YWxsIG9mIHRoZSBpbnRlcnJ1cHRzIGluIHRoZSBhcHBsaWNhdGlvbiByZWdpc3Rlcg0KPiAgPiA+
PiBzcGFjZSAgPiB1bmxpa2Ugb3RoZXIgbmV3ZXIgdmVyc2lvbiBvZiB0aGUgaGFyZHdhcmUuIFNv
IEkgYXNzdW1lLA0KPiAgPiA+PiB0aGUgdmVyc2lvbiAgPiB1c2VkIG9uIExheWVyc2NhcGUgaXMg
YWxzbyBhbiBvbGRlciB2ZXJzaW9uIGFuZCB0aGUNCj4gID4gPj4gYm90aCBoYXZlIHNhbWUgID4g
aXNzdWUgaW4gdGVybXMgb2Ygbm9uIHN0YW5kYXJkIHBsYXRmb3JtIGludGVycnVwdA0KPiAgPiA+
PiB1c2VkIGZvciBlcnJvciAgcmVwb3J0aW5nLg0KPiAgPiA+PiAgPg0KPiAgPiA+PiAgPiA+IEkg
YXNzdW1lIHlvdSBoYXZlIGEgUm9vdCBQb3J0IHdpdGggYW4gQUVSIGNhcGFiaWxpdHksIG5vIE1T
SQ0KPiAgPiA+PiA+ID4gY2FwYWJpbGl0eSwgYW5kIG5vIE1TSS1YIGNhcGFiaWxpdHksIHJpZ2h0
Pw0KPiAgPiA+PiAgPg0KPiAgPiA+PiAgPiBIYXMgQUVSIGNhcGFiaWxpdHkgYW5kIGJvdGggTVNJ
IGFuZCBJTlR4IChsZWdhY3kpIGNhcGFiaWxpdHkgID4NCj4gID4gPj4gPiA+IFdoYXQgZG9lcyBp
dHMgSW50ZXJydXB0ICA+ID4gUGluIHJlZ2lzdGVyIGNvbnRhaW4/ICBJZiBpdCdzDQo+ICA+ID4+
IHplcm8sIGl0IGRvZXNuJ3QgdXNlIElOVHggZWl0aGVyLCBzbyAgPiA+IGFjY29yZGluZyB0byB0
aGUgc3BlYyBpdA0KPiAgPiA+PiBzaG91bGQgZ2VuZXJhdGUgbm8gaW50ZXJydXB0cy4NCj4gID4g
Pj4gID4gPg0KPiAgPiA+PiAgPiBBdCBhZGRyZXNzIG9mZnNldCAweDNDIGJ5IGRlZmF1bHQgaGFz
IGEgdmFsdWUgb2YgMSwgYnV0IGl0IGlzDQo+ICA+ID4+IHdyaXRhYmxlICA+IGJ5IHNvZnR3YXJl
LiBTbyBkZWZhdWx0IGlzIElOVHggQS4NCj4gID4gPj4NCj4gID4gPj4gIDB4M2MgaXMgdGhlIElu
dGVycnVwdCAqTGluZSosIHdoaWNoIGlzIHJlYWQvd3JpdGUuICBUaGUgSW50ZXJydXB0DQo+ICA+
ID4+ICAqUGluKiBpcyBhdCAweDNkIGFuZCBzaG91bGQgYmUgcmVhZC1vbmx5Lg0KPiAgPiA+Pg0K
PiAgPg0KPiAgPiBZb3UgYXJlIHJpZ2h0LiBCdXQgZGVmYXVsdCBpcyAxIGF0IHRoaXMgYWRkcmVz
cy4NCj4gID4NCj4gID4gPj4gIERvZXMgeW91ciBLZXlzdG9uZSBkcml2ZXIgc3VwcG9ydCBNU0k/
ICBJZiBzbywgc2luY2UgeW91ciBSb290DQo+ICA+ID4+IFBvcnQgIHN1cHBvcnRzIE1TSSwgSSB3
b3VsZCB0aGluayB3ZSB3b3VsZCB1c2UgdGhhdCBieSBkZWZhdWx0LCBhbmQNCj4gID4gPj4gdGhl
IElOVHggIHN0dWZmIHdvdWxkbid0IGV2ZW4gbWF0dGVyLg0KPiAgPiA+DQo+ICA+ID4gTGF5ZXJz
Y2FwZSBpcyBhbHNvIHNob3dzICJCb3RoIG1lc3NhZ2Ugc2lnbmFsZWQgaW50ZXJydXB0cyAoTVNJ
KSBhbmQNCj4gIGxlZ2FjeSBJTlR4IGFyZSBzdXBwb3J0ZWQuIg0KPiAgPiA+IEJ1dCBib3RoIG9m
IHRoZW0gbm90IHdvcmsgZm9yIEFFUiBpbnRlcnJ1cHQgd2hlbiBkZXZpY2VzIG9yIHJvb3QNCj4g
IHBvcnQgcmVwb3J0IGFlciBlcnJvci4NCj4gID4gPiBCdXQgYW5vdGhlciBHSUMgaW50ZXJydXB0
IGxpbmUgZG8uDQo+ICA+DQo+ICA+IFNhbWUgd2l0aCBLZXlzdG9uZS4gRXZlbiB0aG91Z2ggYm90
aCBNU0kgYW5kIElOVHggYXJlIHN1cHBvcnRlZCBlcnJvcg0KPiAgPiBpbnRlcnJ1cHQgYXQgcm9v
dCBwb3J0IGlzIHJlcG9ydGVkIG9uIGEgZGlmZmVyZW50IGludGVycnVwdCBsaW5lIHRoYW4NCj4g
ID4gTVNJL0lOVHguIFNvIGZvciBQb3dlciBNYW5hZ2VtZW50IGV2ZW50IGludGVycnVwdCBpcyBh
bHNvIGRpZmZlcmVudA0KPiAgPiBsaW5lLg0KPiAgDQo+ICBJJ20gbG9va2luZyBhdCB0aGUgIkVy
cm9yIE1lc3NhZ2UgQ29udHJvbHMiIGRpYWdyYW0gaW4gdGhlIFBDSWUgc3BlYw0KPiAgcjMuMCwg
c2VjIDYuMi42LiAgRG9lcyB0aGlzIGhhcmR3YXJlIGZpdCBpbnRvIHRoZSBwbGF0Zm9ybS1zcGVj
aWZpYw0KPiAgIlN5c3RlbSBFcnJvciIgY2FzZSB0aGVyZT8gIERvIHRoZSBSb290IENvbnRyb2wg
ZW5hYmxlIGJpdHMgKGluIHRoZSBQQ0llDQo+ICBDYXBhYmlsaXR5KSBjb250cm9sIHRoaXMgaW50
ZXJydXB0PyAgSWYgc28sIG1heWJlIHRoaXMgbWFrZXMgbW9yZSBzZW5zZQ0KPiAgdGhhbiBJIHRo
b3VnaHQuDQoNCkl0IHN1cHBvc2VkbHkgbm90IHRoZSAiU3lzdGVtIEVycm9yIiBjYXNlLiBCdXQg
InRoZSBFcnJvciBJbnRlcnJ1cHQiIGNhc2UuDQpXaGljaCBtZWFucyAiIFJvb3QgRXJyb3IgQ29t
bWFuZCByZWdpc3RlciAiIGNvdWxkIGNvbnRyb2wgdGhlIGludGVycnVwdA0KIGxpbmUgd2UgaGF2
ZSBub3cuIChyZWZlciBQQ0llIHNwZWMgcjMuMCwgc2VjIDYuMi42KQ0KDQpNYXkgdGhpcyBraW5k
IG9mIGhhcmR3YXJlIGRlc2lnbiByb3V0ZSBicm9rZW4gdGhlIHNwZWM/IA0KDQo+ICANCj4gIFdl
IGhhdmUgdG8gYXNzdW1lIGJvdGggbm90aWZpY2F0aW9uIHBhdGhzIHdvcmsgKHRoZSBub3JtYWwg
SU5UeC9NU0kvTVNJLQ0KPiAgWCBBRVIgaW50ZXJydXB0cyBhcyB3ZWxsIGFzIHRoZSBwbGF0Zm9y
bS1zcGVjaWZpYyBTeXN0ZW0gRXJyb3JzKSwgc28gaWYNCj4gIHdlIGFkZCBzdXBwb3J0IGZvciBT
eXN0ZW0gRXJyb3JzLCBpdCBzaG91bGQgYmUgc3RydWN0dXJlZCBzbyB3ZSBwcmVmZXINCj4gIElO
VHgvTVNJL01TSS1YLCBhbmQgb25seSBmYWxsIGJhY2sgdG8gU3lzdGVtIEVycm9ycyBpZiB0aGV5
IGRvbid0IHdvcmsuDQo+ICBBRVIgd291bGQgaGF2ZSB0byBrbm93IHdoaWNoIHBhdGggaXQncyB1
c2luZyBzbyBhZXJfZW5hYmxlX3Jvb3Rwb3J0KCkNCj4gIGNhbiBkaXNhYmxlIHRoZSBvdGhlciBv
bmUgKGN1cnJlbnRseSBpdCBhbHdheXMgZGlzYWJsZXMgU3lzdGVtIEVycm9ycykuDQo+ICANCj4g
IFRoZW4geW91J2QgaGF2ZSB0byBhZGQgcXVpcmtzIHRvIG1hcmsgTVNJL01TSS1YL0lOVHggYXMg
YmVpbmcgYnJva2VuIG9uDQo+ICB5b3VyIGRldmljZXMgdG8gZm9yY2UgdGhlIGZhbGxiYWNrIHRv
IFN5c3RlbSBFcnJvcnMuDQo+ICANCj4gIEhvdyBtdWNoIHdvdWxkIHRoaXMgc2NyZXcgdXAgb3Ro
ZXIgUENJZSBzZXJ2aWNlcyAoUE1FLCBob3RwbHVnLCBWQywgZXRjKT8NCj4gIERvZXMgTVNJIHdv
cmsgZm9yIHRoZW0/DQoNClBNRSBhbHNvIGxpa2UgdGhlIEFFUi4gSG90cGx1ZyBpcyBub3Qgc3Vw
cG9ydGVkLiBPdGhlcnMgbm90IGtub3duLg0KUG8gTGl1DQoNCj4gIA0KPiAgQmpvcm4NCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas June 7, 2016, 10:46 p.m. UTC | #12
On Tue, Jun 07, 2016 at 10:07:40AM +0000, Po Liu wrote:
> Hi Bjorn,
> 
> >  -----Original Message-----
> >  
> >  On Mon, Jun 06, 2016 at 10:01:44AM -0400, Murali Karicheri wrote:
> >  > 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.
> >  
> >  I'm looking at the "Error Message Controls" diagram in the PCIe spec
> >  r3.0, sec 6.2.6.  Does this hardware fit into the platform-specific
> >  "System Error" case there?  Do the Root Control enable bits (in the PCIe
> >  Capability) control this interrupt?  If so, maybe this makes more sense
> >  than I thought.
> 
> It supposedly not the "System Error" case. But "the Error Interrupt" case.
> Which means " Root Error Command register " could control the interrupt
>  line we have now. (refer PCIe spec r3.0, sec 6.2.6)

Did you actually try this out and verify that the PCIe Root Control
enable bits have no effect and the AER Root Error Command bits do
control it?  The names are very similar, so there's lots of room for
misunderstanding here :)

If the AER Root Error Command does control this interrupt, I think the
PCI_COMMAND_INTX_DISABLE bit in the PCI Command register should also
control it (per sec 6.2.4.1.2).

> May this kind of hardware design route broken the spec? 

If the Reporting Enable bits in the Root Port's AER Root Error Command
register control the interrupt, but the interrupt is not delivered via
the Root Port's INTx or MSI/MSI-X, I think the design is not following
the spec.

All the information needed by the AER driver should be communicated
via the config space mechanisms described in the spec (AER capability,
MSI/MSI-X capabilities, Interrupt Pin, etc.)  That way the driver
works without change on future spec-compliant hardware.

> PME also like the AER. Hotplug is not supported. Others not known.
> Po Liu

Per sec 6.1.6, I think PME *should* be signaled by the Root Port's
INTx or MSI/MSI-X.

In particular, it says "Note that all other interrupt sources within
the same Function will assert the same virtual INTx wire when
requesting service."  To me, that means that if we're using INTx, it
will be the same INTx for AER, PME, hotplug, etc., and it should be
the one indicated by the Interrupt Pin register.

But I think on your Root Port:

  - There is an MSI capability, but MSI doesn't actually work at all
  - Interrupt Pin contains 1, indicating INTA, which is routed to IRQ X
  - AER interrupts are routed to some different IRQ Y
  - PME interrupts are routed to a third IRQ Z

So how should we work around this?  I think you should be able to get
partway there with a quirk that sets:

  dev->no_msi = 1;
  dev->irq = Y;

for this device.  That should make AER work, but of course PME would
not work.

Is there a way to set up your interrupt controller so these three
interrupts (X, Y, Z above) all map to the same Linux IRQ?  If you can
do that, you could set up INTA, the AER interrupt, and the PME
interrupt to all be on the same IRQ and everything should work.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Po Liu June 8, 2016, 4:56 a.m. UTC | #13
SGkgQmpvcm4sDQoNClRoYW5rcyBmb3IgdGhlIGtpbmRseSByZXBseS4gQWxsIHRoZXNlIGFyZSBo
ZWxwZnVsLg0KDQo+ICBGcm9tOiBCam9ybiBIZWxnYWFzIFttYWlsdG86aGVsZ2Fhc0BrZXJuZWwu
b3JnXQ0KPiAgT24gV2VkLCBKdW5lIDA4LCAyMDE2IDY6NDcgQU0NCj4gIA0KPiAgT24gVHVlLCBK
dW4gMDcsIDIwMTYgYXQgMTA6MDc6NDBBTSArMDAwMCwgUG8gTGl1IHdyb3RlOg0KPiAgPiBIaSBC
am9ybiwNCj4gID4NCj4gID4gPiAgLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gID4gPg0K
PiAgPiA+ICBPbiBNb24sIEp1biAwNiwgMjAxNiBhdCAxMDowMTo0NEFNIC0wNDAwLCBNdXJhbGkg
S2FyaWNoZXJpIHdyb3RlOg0KPiAgPiA+ICA+IE9uIDA2LzA2LzIwMTYgMDM6MzIgQU0sIFBvIExp
dSB3cm90ZToNCj4gID4gPiAgPiA+IEhpIEJqb3JuLA0KPiAgPiA+ICA+ID4gSSBjb25maXJtIHdl
IG1ldCBzYW1lIHByb2JsZW0gd2l0aCBLZXlTdG9uZSBiYXNlIG9uIERlc2lnbldhcmUNCj4gID4g
PiBkZXNpZ24uDQo+ICA+ID4gID4gPg0KPiAgPiA+ICA+ID4NCj4gID4gPiAgPiA+IEJlc3QgcmVn
YXJkcywNCj4gID4gPiAgPiA+IExpdSBQbw0KPiAgPiA+ICA+ID4NCj4gID4gPiAgPiA+PiAgLS0t
LS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gID4gPiAgPiA+PiAgRnJvbTogQmpvcm4gSGVsZ2Fh
cyBbbWFpbHRvOmhlbGdhYXNAa2VybmVsLm9yZ10gID4gPj4gIFNlbnQ6DQo+ICA+ID4gU2F0dXJk
YXksIEp1bmUgMDQsIDIwMTYgMTE6NDkgQU0gID4gPj4gIFRvOiBNdXJhbGkgS2FyaWNoZXJpICA+
ID4+DQo+ICA+ID4gQ2M6IFBvIExpdTsgbGludXgtcGNpQHZnZXIua2VybmVsLm9yZzsgbGludXgt
YXJtLSAgPiA+Pg0KPiAgPiA+IGtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOyBsaW51eC1rZXJu
ZWxAdmdlci5rZXJuZWwub3JnOyAgPiA+Pg0KPiAgPiA+IGRldmljZXRyZWVAdmdlci5rZXJuZWwu
b3JnOyBBcm5kIEJlcmdtYW5uOyBSb3kgWmFuZzsgTWFyYyBaeW5naWVyOw0KPiAgPiA+ID4gPj4g
U3R1YXJ0IFlvZGVyOyBZYW5nLUxlbyBMaTsgTWluZ2h1YW4gTGlhbjsgQmpvcm4gSGVsZ2Fhczsg
U2hhd24NCj4gID4gPiBHdW87ICA+ID4+IE1pbmdrYWkgSHU7IFJvYiBIZXJyaW5nICA+ID4+ICBT
dWJqZWN0OiBSZTogW1BBVENIIDIvMl0NCj4gID4gPiBhZXI6IGFkZCBzdXBwb3J0IGFlciBpbnRl
cnJ1cHQgd2l0aCBub25lICA+ID4+IE1TSS9NU0ktWC9JTlR4IG1vZGUNCj4gID4gPiA+ID4+ICA+
ID4+ICBPbiBGcmksIEp1biAwMywgMjAxNiBhdCAwMTozMToxMVBNIC0wNDAwLCBNdXJhbGkNCj4g
ID4gPiBLYXJpY2hlcmkgd3JvdGU6DQo+ICA+ID4gID4gPj4gID4gUG8sDQo+ICA+ID4gID4gPj4g
ID4NCj4gID4gPiAgPiA+PiAgPiBTb3JyeSB0byBoaWphY2sgeW91ciBkaXNjdXNzaW9uLCBidXQg
dGhlIHByb2JsZW0gc2VlbXMgdG8NCj4gID4gPiBiZSAgPiA+PiBzYW1lIGZvciAgPiBLZXlzdG9u
ZSBQQ0kgY29udHJvbGxlciB3aGljaCBpcyBhbHNvDQo+ICA+ID4gZGVzaWdud2FyZSAob2xkDQo+
ICA+ID4gIHZlcnNpb24pIGJhc2VkLg0KPiAgPiA+ICA+ID4+ICA+DQo+ICA+ID4gID4gPj4gID4g
T24gMDYvMDMvMjAxNiAxMjowOSBBTSwgQmpvcm4gSGVsZ2FhcyB3cm90ZToNCj4gID4gPiAgPiA+
PiAgPiA+IE9uIFRodSwgSnVuIDAyLCAyMDE2IGF0IDExOjM3OjI4QU0gLTA0MDAsIE11cmFsaQ0K
PiAgPiA+IEthcmljaGVyaQ0KPiAgPiA+ICB3cm90ZToNCj4gID4gPiAgPiA+PiAgPiA+PiBPbiAw
Ni8wMi8yMDE2IDA5OjU1IEFNLCBCam9ybiBIZWxnYWFzIHdyb3RlOg0KPiAgPiA+ICA+ID4+ICA+
ID4+PiBPbiBUaHUsIEp1biAwMiwgMjAxNiBhdCAwNTowMToxOUFNICswMDAwLCBQbyBMaXUgd3Jv
dGU6DQo+ICA+ID4gID4gPj4gID4gPj4+Pj4gIC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tICA+
ID4+Pj4+ICBGcm9tOiBCam9ybg0KPiAgPiA+IEhlbGdhYXMgID4gPj4gW21haWx0bzpoZWxnYWFz
QGtlcm5lbC5vcmddICA+ID4+Pj4+ICBTZW50OiBUaHVyc2RheSwNCj4gID4gPiBKdW5lIDAyLCAy
MDE2ICA+ID4+IDExOjQ4IEFNICA+ID4+Pj4+ICBUbzogUG8gTGl1ICA+ID4+Pj4+ICBDYzoNCj4g
ID4gPiAgPiA+PiBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyAgPiA+Pj4+PiAgPiA+Pg0KPiAg
PiA+IGxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZzsNCj4gID4gPiAgPiA+PiAg
PiA+Pj4+PiAgbGludXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsNCj4gID4gPiBkZXZpY2V0cmVl
QHZnZXIua2VybmVsLm9yZzsgID4gPj4gQXJuZCAgPiA+Pj4+PiBCZXJnbWFubjsgIFJveSBaYW5n
Ow0KPiAgPiA+IE1hcmMgWnluZ2llcjsgU3R1YXJ0IFlvZGVyOyAgPiA+PiBZYW5nLUxlbyBMaTsg
ID4gPj4+Pj4gTWluZ2h1YW4NCj4gID4gPiBMaWFuOyBCam9ybiAgSGVsZ2FhczsgU2hhd24gR3Vv
OyAgPiA+PiBNaW5na2FpIEh1OyBSb2IgID4gPj4+Pj4NCj4gID4gPiBIZXJyaW5nICA+ID4+Pj4+
ICBTdWJqZWN0OiBSZTogW1BBVENIIDIvMl0gID4gPj4gYWVyOiBhZGQgc3VwcG9ydA0KPiAgPiA+
IGFlciBpbnRlcnJ1cHQgd2l0aCAgPiA+Pj4+PiBub25lICBNU0kvTVNJLVgvSU5UeCAgPiA+PiBt
b2RlICA+ID4+Pj4+DQo+ICA+ID4gPiA+Pj4+PiAgWytjYyBSb2JdICA+ID4+Pj4+ICA+ID4+Pj4+
ICBIaSBQbywgID4gID4gPj4gPj4+Pj4gID4gPj4+Pj4NCj4gID4gPiBPbiBUaHUsIE1heSAyNiwg
MjAxNiBhdCAwMjowMDowNlBNICswODAwLCBQbyBMaXUgID4gPj4gd3JvdGU6DQo+ICA+ID4gID4g
Pj4gID4gPj4+Pj4gID4gT24gc29tZSBwbGF0Zm9ybXMsIHJvb3QgcG9ydCBkb2Vzbid0IHN1cHBv
cnQgID4gPj4NCj4gID4gPiBNU0kvTVNJLVgvSU5UeCAgaW4gUkMgbW9kZS4NCj4gID4gPiAgPiA+
PiAgPiA+Pj4+PiAgPiBXaGVuIGNoaXAgc3VwcG9ydCB0aGUgYWVyIGludGVycnVwdCB3aXRoIG5v
bmUgID4NCj4gID4gPiA+PiBNU0kvTVNJLVgvSU5UeCAgPiA+Pj4+PiBtb2RlLCAgPiBtYXliZSB0
aGVyZSBpcyBpbnRlcnJ1cHQgbGluZQ0KPiAgPiA+IGZvciAgPiA+PiBhZXIgcG1lIGV0Yy4gU2Vh
cmNoICA+ID4+Pj4+IHRoZSBpbnRlcnJ1cHQgID4gbnVtYmVyIGluDQo+ICA+ID4gdGhlIGZkdCAg
ZmlsZS4NCj4gID4gPiAgPiA+PiAgPiA+Pj4+Pg0KPiAgPiA+ICA+ID4+ICA+ID4+Pj4+ICBNeSB1
bmRlcnN0YW5kaW5nIGlzIHRoYXQgQUVSIGludGVycnVwdCBzaWduYWxpbmcgY2FuDQo+ICA+ID4g
YmUgID4gPj4gZG9uZSAgPiA+Pj4+PiB2aWEgSU5UeCwgIE1TSSwgb3IgTVNJLVggKFBDSWUgc3Bl
YyByMy4wLCBzZWMNCj4gID4gPiA2LjIuNC4xLjIpLg0KPiAgPiA+ICA+ID4+ICA+ID4+Pj4+IEFw
cGFyZW50bHkgeW91ciBkZXZpY2UgIGRvZXNuJ3Qgc3VwcG9ydCBNU0kgb3IgTVNJLVguDQo+ICA+
ID4gQXJlICA+ID4+IHlvdSAgPiA+Pj4+PiBzYXlpbmcgaXQgZG9lc24ndCBzdXBwb3J0IElOVHgg
IGVpdGhlcj8gIEhvdw0KPiAgPiA+IGlzIHRoZSAgPiA+PiBpbnRlcnJ1cHQgIHlvdSdyZSByZXF1
ZXN0aW5nIGhlcmUgZGlmZmVyZW50IGZyb20gSU5UeD8NCj4gID4gPiAgPiA+PiAgPiA+Pj4+DQo+
ICA+ID4gID4gPj4gID4gPj4+PiBMYXllcnNjYXBlIHVzZSBub25lIG9mIE1TSSBvciBNU0ktWCBv
ciBJTlR4IHRvDQo+ICA+ID4gaW5kaWNhdGUgdGhlICA+ID4+ID4gPj4+PiBkZXZpY2VzIG9yIHJv
b3QgZXJyb3IgaW4gUkMgbW9kZS4gQnV0IHVzZQ0KPiAgPiA+IGFuIGluZGVwZW5kZW50IFNQSSAg
PiA+PiA+ID4+Pj4gaW50ZXJydXB0KGFybSBpbnRlcnJ1cHQgY29udHJvbGxlcikNCj4gIGxpbmUu
DQo+ICA+ID4gID4gPj4gID4gPj4+DQo+ICA+ID4gID4gPj4gID4gPj4+IFRoZSBSb290IFBvcnQg
aXMgYSBQQ0kgZGV2aWNlIGFuZCBzaG91bGQgZm9sbG93IHRoZQ0KPiAgPiA+IG5vcm1hbCAgPiA+
PiBQQ0kgID4gPj4+IHJ1bGVzIGZvciBpbnRlcnJ1cHRzLiAgQXMgZmFyIGFzIEkNCj4gID4gPiB1
bmRlcnN0YW5kLCB0aGF0ICA+ID4+IG1lYW5zIGl0ICA+ID4+PiBzaG91bGQgdXNlIE1TSSwgTVNJ
LVgsIG9yDQo+ICA+ID4gSU5UeC4gIElmIHlvdXIgUm9vdCBQb3J0ICA+ID4+IGRvZXNuJ3QgdXNl
IE1TSSAgPiA+Pj4gb3IgTVNJLVgsIGl0DQo+ICA+ID4gc2hvdWxkIHVzZSBJTlR4LCB0aGUgID4g
Pj4gUENJX0lOVEVSUlVQVF9QSU4gcmVnaXN0ZXIgID4gPj4+IHNob3VsZA0KPiAgPiA+IHRlbGwg
dXMgd2hpY2ggKElOVEEvICA+ID4+IElOVEIvZXRjLiksIGFuZCAgUENJX0NPTU1BTkRfSU5UWF9E
SVNBQkxFDQo+ICBzaG91bGQgd29yayB0byBkaXNhYmxlIGl0Lg0KPiAgPiA+ICA+ID4+ICA+ID4+
PiBUaGF0J3MgYWxsIGZyb20gdGhlIFBDSSBwb2ludCBvZiB2aWV3LCBvZiBjb3Vyc2UuDQo+ICA+
ID4gID4gPj4gID4gPj4NCj4gID4gPiAgPiA+PiAgPiA+PiBJIGFtIGZhY2VkIHdpdGggdGhlIHNh
bWUgaXNzdWUgb24gS2V5c3RvbmUgUENJIGhhcmR3YXJlDQo+ICA+ID4gYW5kICA+ID4+IGl0IGhh
cyAgPiA+PiBiZWVuIG9uIG15IFRPRE8gbGlzdCAgZm9yIHF1aXRlIHNvbWUgdGltZS4NCj4gID4g
PiBLZXlzdG9uZSAgPiA+PiBQQ0kgaGFyZHdhcmUgID4gPj4gYWxzbyBkb2Vzbid0IHVzZSBNU0kg
b3IgTVNJLVggb3INCj4gID4gPiBJTlR4IGZvciAgPiA+PiByZXBvcnRpbmcgZXJyb3JzIHJlY2Vp
dmVkICA+ID4+IGF0IHRoZSByb290IHBvcnQsIGJ1dA0KPiAgPiA+IHVzZSBhICA+ID4+IHBsYXRm
b3JtIGludGVycnVwdCBpbnN0ZWFkIChub3QgID4gPj4gY29tcGxhaW50IHRvIFBDSQ0KPiAgPiA+
IHN0YW5kYXJkIGFzICA+ID4+IHBlciBQQ0kgYmFzZSBzcGVjKS4gU28gSSB3b3VsZCBuZWVkICA+
ID4+IHNpbWlsYXINCj4gID4gPiBjaGFuZ2UgdG8gaGF2ZSAgPiA+PiB0aGUgZXJyb3IgaW50ZXJy
dXB0IHBhc3NlZCB0byB0aGUgYWVyICA+ID4+DQo+ICA+ID4gZHJpdmVyLiBTbyB0aGVyZSBhcmUg
ID4gPj4gaGFyZHdhcmUgb3V0IHRoZXJlIGxpa2UgS2V5c3RvbmUgd2hpY2gNCj4gID4gPiByZXF1
aXJlcyB0byBzdXBwb3J0IHRoaXMgIHRocm91Z2ggcGxhdGZvcm0gSVJRLg0KPiAgPiA+ICA+ID4+
ICA+ID4NCj4gID4gPiAgPiA+PiAgPiA+IFRoaXMgaXMgbm90IGEgbmV3IGFyZWEgb2YgdGhlIHNw
ZWMsIGFuZCBpdCdzIGhhcmQgZm9yIG1lDQo+ICA+ID4gdG8gID4gPj4gYmVsaWV2ZSAgPiA+IHRo
YXQgdGhlc2UgdHdvIG5ldyBQQ0llIGNvbnRyb2xsZXJzIGFyZSBib3RoDQo+ICA+ID4gYnJva2Vu
ICA+ID4+IHRoZSBzYW1lIHdheSAgPiA+IChhbHRob3VnaCBJIGd1ZXNzIGJvdGggYXJlDQo+ICA+
ID4gRGVzaWduV2FyZS1iYXNlZCwgc28gID4gPj4gbWF5YmUgdGhpcyBpcyB0aGUgID4gPiBzYW1l
IHVuZGVybHlpbmcNCj4gID4gPiBwcm9ibGVtIGluIGJvdGggY2FzZXM/KS4gIEkgID4gPj4gdGhp
bmsgaXQncyBtb3JlIGxpa2VseSAgPiA+IHRoYXQNCj4gID4gPiB3ZSBqdXN0IGhhdmVuJ3QgZmln
dXJlZCBvdXQgdGhlICA+ID4+IHJpZ2h0IHdheSB0byBkZXNjcmliZSB0aGlzIGluDQo+ICB0aGUg
RFQuDQo+ICA+ID4gID4gPj4gID4NCj4gID4gPiAgPiA+PiAgPiBLZXlzdG9uZSBpcyB1c2luZyBh
biBvbGRlciB2ZXJzaW9uIG9mIHRoZSBkZXNpZ253YXJlIElQIGFuZA0KPiAgPiA+IGl0ICA+ID4+
ID4gaW1wbGVtZW50cyBhbGwgb2YgdGhlIGludGVycnVwdHMgaW4gdGhlIGFwcGxpY2F0aW9uDQo+
ICA+ID4gcmVnaXN0ZXIgID4gPj4gc3BhY2UgID4gdW5saWtlIG90aGVyIG5ld2VyIHZlcnNpb24g
b2YgdGhlIGhhcmR3YXJlLg0KPiAgPiA+IFNvIEkgYXNzdW1lLCAgPiA+PiB0aGUgdmVyc2lvbiAg
PiB1c2VkIG9uIExheWVyc2NhcGUgaXMgYWxzbyBhbg0KPiAgPiA+IG9sZGVyIHZlcnNpb24gYW5k
IHRoZSAgPiA+PiBib3RoIGhhdmUgc2FtZSAgPiBpc3N1ZSBpbiB0ZXJtcyBvZiBub24NCj4gID4g
PiBzdGFuZGFyZCBwbGF0Zm9ybSBpbnRlcnJ1cHQgID4gPj4gdXNlZCBmb3IgZXJyb3IgIHJlcG9y
dGluZy4NCj4gID4gPiAgPiA+PiAgPg0KPiAgPiA+ICA+ID4+ICA+ID4gSSBhc3N1bWUgeW91IGhh
dmUgYSBSb290IFBvcnQgd2l0aCBhbiBBRVIgY2FwYWJpbGl0eSwgbm8NCj4gID4gPiBNU0kgID4g
Pj4gPiA+IGNhcGFiaWxpdHksIGFuZCBubyBNU0ktWCBjYXBhYmlsaXR5LCByaWdodD8NCj4gID4g
PiAgPiA+PiAgPg0KPiAgPiA+ICA+ID4+ICA+IEhhcyBBRVIgY2FwYWJpbGl0eSBhbmQgYm90aCBN
U0kgYW5kIElOVHggKGxlZ2FjeSkNCj4gID4gPiBjYXBhYmlsaXR5ICA+ICA+ID4+ID4gPiBXaGF0
IGRvZXMgaXRzIEludGVycnVwdCAgPiA+IFBpbiByZWdpc3Rlcg0KPiAgPiA+IGNvbnRhaW4/ICBJ
ZiBpdCdzICA+ID4+IHplcm8sIGl0IGRvZXNuJ3QgdXNlIElOVHggZWl0aGVyLCBzbyAgPiA+DQo+
ICA+ID4gYWNjb3JkaW5nIHRvIHRoZSBzcGVjIGl0ICA+ID4+IHNob3VsZCBnZW5lcmF0ZSBubyBp
bnRlcnJ1cHRzLg0KPiAgPiA+ICA+ID4+ICA+ID4NCj4gID4gPiAgPiA+PiAgPiBBdCBhZGRyZXNz
IG9mZnNldCAweDNDIGJ5IGRlZmF1bHQgaGFzIGEgdmFsdWUgb2YgMSwgYnV0IGl0DQo+ICA+ID4g
aXMgID4gPj4gd3JpdGFibGUgID4gYnkgc29mdHdhcmUuIFNvIGRlZmF1bHQgaXMgSU5UeCBBLg0K
PiAgPiA+ICA+ID4+DQo+ICA+ID4gID4gPj4gIDB4M2MgaXMgdGhlIEludGVycnVwdCAqTGluZSos
IHdoaWNoIGlzIHJlYWQvd3JpdGUuICBUaGUNCj4gID4gPiBJbnRlcnJ1cHQgID4gPj4gICpQaW4q
IGlzIGF0IDB4M2QgYW5kIHNob3VsZCBiZSByZWFkLW9ubHkuDQo+ICA+ID4gID4gPj4NCj4gID4g
PiAgPg0KPiAgPiA+ICA+IFlvdSBhcmUgcmlnaHQuIEJ1dCBkZWZhdWx0IGlzIDEgYXQgdGhpcyBh
ZGRyZXNzLg0KPiAgPiA+ICA+DQo+ICA+ID4gID4gPj4gIERvZXMgeW91ciBLZXlzdG9uZSBkcml2
ZXIgc3VwcG9ydCBNU0k/ICBJZiBzbywgc2luY2UgeW91cg0KPiAgPiA+IFJvb3QgID4gPj4gUG9y
dCAgc3VwcG9ydHMgTVNJLCBJIHdvdWxkIHRoaW5rIHdlIHdvdWxkIHVzZSB0aGF0IGJ5DQo+ICA+
ID4gZGVmYXVsdCwgYW5kICA+ID4+IHRoZSBJTlR4ICBzdHVmZiB3b3VsZG4ndCBldmVuIG1hdHRl
ci4NCj4gID4gPiAgPiA+DQo+ICA+ID4gID4gPiBMYXllcnNjYXBlIGlzIGFsc28gc2hvd3MgIkJv
dGggbWVzc2FnZSBzaWduYWxlZCBpbnRlcnJ1cHRzDQo+ICA+ID4gKE1TSSkgYW5kICBsZWdhY3kg
SU5UeCBhcmUgc3VwcG9ydGVkLiINCj4gID4gPiAgPiA+IEJ1dCBib3RoIG9mIHRoZW0gbm90IHdv
cmsgZm9yIEFFUiBpbnRlcnJ1cHQgd2hlbiBkZXZpY2VzIG9yDQo+ICA+ID4gcm9vdCAgcG9ydCBy
ZXBvcnQgYWVyIGVycm9yLg0KPiAgPiA+ICA+ID4gQnV0IGFub3RoZXIgR0lDIGludGVycnVwdCBs
aW5lIGRvLg0KPiAgPiA+ICA+DQo+ICA+ID4gID4gU2FtZSB3aXRoIEtleXN0b25lLiBFdmVuIHRo
b3VnaCBib3RoIE1TSSBhbmQgSU5UeCBhcmUgc3VwcG9ydGVkDQo+ICA+ID4gZXJyb3IgID4gaW50
ZXJydXB0IGF0IHJvb3QgcG9ydCBpcyByZXBvcnRlZCBvbiBhIGRpZmZlcmVudCBpbnRlcnJ1cHQN
Cj4gID4gPiBsaW5lIHRoYW4gID4gTVNJL0lOVHguIFNvIGZvciBQb3dlciBNYW5hZ2VtZW50IGV2
ZW50IGludGVycnVwdCBpcw0KPiAgPiA+IGFsc28gZGlmZmVyZW50ICA+IGxpbmUuDQo+ICA+ID4N
Cj4gID4gPiAgSSdtIGxvb2tpbmcgYXQgdGhlICJFcnJvciBNZXNzYWdlIENvbnRyb2xzIiBkaWFn
cmFtIGluIHRoZSBQQ0llDQo+ICA+ID4gc3BlYyAgcjMuMCwgc2VjIDYuMi42LiAgRG9lcyB0aGlz
IGhhcmR3YXJlIGZpdCBpbnRvIHRoZQ0KPiAgPiA+IHBsYXRmb3JtLXNwZWNpZmljICAiU3lzdGVt
IEVycm9yIiBjYXNlIHRoZXJlPyAgRG8gdGhlIFJvb3QgQ29udHJvbA0KPiAgPiA+IGVuYWJsZSBi
aXRzIChpbiB0aGUgUENJZQ0KPiAgPiA+ICBDYXBhYmlsaXR5KSBjb250cm9sIHRoaXMgaW50ZXJy
dXB0PyAgSWYgc28sIG1heWJlIHRoaXMgbWFrZXMgbW9yZQ0KPiAgPiA+IHNlbnNlICB0aGFuIEkg
dGhvdWdodC4NCj4gID4NCj4gID4gSXQgc3VwcG9zZWRseSBub3QgdGhlICJTeXN0ZW0gRXJyb3Ii
IGNhc2UuIEJ1dCAidGhlIEVycm9yIEludGVycnVwdCINCj4gIGNhc2UuDQo+ICA+IFdoaWNoIG1l
YW5zICIgUm9vdCBFcnJvciBDb21tYW5kIHJlZ2lzdGVyICIgY291bGQgY29udHJvbCB0aGUNCj4g
ID4gaW50ZXJydXB0ICBsaW5lIHdlIGhhdmUgbm93LiAocmVmZXIgUENJZSBzcGVjIHIzLjAsIHNl
YyA2LjIuNikNCj4gIA0KPiAgRGlkIHlvdSBhY3R1YWxseSB0cnkgdGhpcyBvdXQgYW5kIHZlcmlm
eSB0aGF0IHRoZSBQQ0llIFJvb3QgQ29udHJvbA0KPiAgZW5hYmxlIGJpdHMgaGF2ZSBubyBlZmZl
Y3QgYW5kIHRoZSBBRVIgUm9vdCBFcnJvciBDb21tYW5kIGJpdHMgZG8NCj4gIGNvbnRyb2wgaXQ/
ICBUaGUgbmFtZXMgYXJlIHZlcnkgc2ltaWxhciwgc28gdGhlcmUncyBsb3RzIG9mIHJvb20gZm9y
DQo+ICBtaXN1bmRlcnN0YW5kaW5nIGhlcmUgOikNCg0KWWVzLCBhbGwgdGhlc2UgcmVzdWx0IHdl
cmUgdGVzdGVkIGJlZm9yZSByZXBseS4NCg0KPiAgDQo+ICBJZiB0aGUgQUVSIFJvb3QgRXJyb3Ig
Q29tbWFuZCBkb2VzIGNvbnRyb2wgdGhpcyBpbnRlcnJ1cHQsIEkgdGhpbmsgdGhlDQo+ICBQQ0lf
Q09NTUFORF9JTlRYX0RJU0FCTEUgYml0IGluIHRoZSBQQ0kgQ29tbWFuZCByZWdpc3RlciBzaG91
bGQgYWxzbw0KPiAgY29udHJvbCBpdCAocGVyIHNlYyA2LjIuNC4xLjIpLg0KDQpZZXMsIEkgYW0g
c3VyZSB0aGUgUENJX0NPTU1BTkRfSU5UWF9ESVNBQkxFIGJpdCBjYW4gYWxzbyBjb250cm9sIHRo
aXMgaW50ZXJydXB0Lg0KDQo+ICANCj4gID4gTWF5IHRoaXMga2luZCBvZiBoYXJkd2FyZSBkZXNp
Z24gcm91dGUgYnJva2VuIHRoZSBzcGVjPw0KPiAgDQo+ICBJZiB0aGUgUmVwb3J0aW5nIEVuYWJs
ZSBiaXRzIGluIHRoZSBSb290IFBvcnQncyBBRVIgUm9vdCBFcnJvciBDb21tYW5kDQo+ICByZWdp
c3RlciBjb250cm9sIHRoZSBpbnRlcnJ1cHQsIGJ1dCB0aGUgaW50ZXJydXB0IGlzIG5vdCBkZWxp
dmVyZWQgdmlhDQo+ICB0aGUgUm9vdCBQb3J0J3MgSU5UeCBvciBNU0kvTVNJLVgsIEkgdGhpbmsg
dGhlIGRlc2lnbiBpcyBub3QgZm9sbG93aW5nDQo+ICB0aGUgc3BlYy4NCj4gIA0KPiAgQWxsIHRo
ZSBpbmZvcm1hdGlvbiBuZWVkZWQgYnkgdGhlIEFFUiBkcml2ZXIgc2hvdWxkIGJlIGNvbW11bmlj
YXRlZCB2aWENCj4gIHRoZSBjb25maWcgc3BhY2UgbWVjaGFuaXNtcyBkZXNjcmliZWQgaW4gdGhl
IHNwZWMgKEFFUiBjYXBhYmlsaXR5LA0KPiAgTVNJL01TSS1YIGNhcGFiaWxpdGllcywgSW50ZXJy
dXB0IFBpbiwgZXRjLikgIFRoYXQgd2F5IHRoZSBkcml2ZXIgd29ya3MNCj4gIHdpdGhvdXQgY2hh
bmdlIG9uIGZ1dHVyZSBzcGVjLWNvbXBsaWFudCBoYXJkd2FyZS4NCj4gIA0KPiAgPiBQTUUgYWxz
byBsaWtlIHRoZSBBRVIuIEhvdHBsdWcgaXMgbm90IHN1cHBvcnRlZC4gT3RoZXJzIG5vdCBrbm93
bi4NCj4gID4gUG8gTGl1DQo+ICANCj4gIFBlciBzZWMgNi4xLjYsIEkgdGhpbmsgUE1FICpzaG91
bGQqIGJlIHNpZ25hbGVkIGJ5IHRoZSBSb290IFBvcnQncyBJTlR4DQo+ICBvciBNU0kvTVNJLVgu
DQo+ICANCj4gIEluIHBhcnRpY3VsYXIsIGl0IHNheXMgIk5vdGUgdGhhdCBhbGwgb3RoZXIgaW50
ZXJydXB0IHNvdXJjZXMgd2l0aGluIHRoZQ0KPiAgc2FtZSBGdW5jdGlvbiB3aWxsIGFzc2VydCB0
aGUgc2FtZSB2aXJ0dWFsIElOVHggd2lyZSB3aGVuIHJlcXVlc3RpbmcNCj4gIHNlcnZpY2UuIiAg
VG8gbWUsIHRoYXQgbWVhbnMgdGhhdCBpZiB3ZSdyZSB1c2luZyBJTlR4LCBpdCB3aWxsIGJlIHRo
ZQ0KPiAgc2FtZSBJTlR4IGZvciBBRVIsIFBNRSwgaG90cGx1ZywgZXRjLiwgYW5kIGl0IHNob3Vs
ZCBiZSB0aGUgb25lDQo+ICBpbmRpY2F0ZWQgYnkgdGhlIEludGVycnVwdCBQaW4gcmVnaXN0ZXIu
DQo+ICANCj4gIEJ1dCBJIHRoaW5rIG9uIHlvdXIgUm9vdCBQb3J0Og0KPiAgDQo+ICAgIC0gVGhl
cmUgaXMgYW4gTVNJIGNhcGFiaWxpdHksIGJ1dCBNU0kgZG9lc24ndCBhY3R1YWxseSB3b3JrIGF0
IGFsbA0KPiAgICAtIEludGVycnVwdCBQaW4gY29udGFpbnMgMSwgaW5kaWNhdGluZyBJTlRBLCB3
aGljaCBpcyByb3V0ZWQgdG8gSVJRIFgNCj4gICAgLSBBRVIgaW50ZXJydXB0cyBhcmUgcm91dGVk
IHRvIHNvbWUgZGlmZmVyZW50IElSUSBZDQo+ICAgIC0gUE1FIGludGVycnVwdHMgYXJlIHJvdXRl
ZCB0byBhIHRoaXJkIElSUSBaDQo+ICANCg0KVGhlIGRlc2NyaXB0aW9ucyBhcmUgYWxsIHJpZ2h0
Lg0KDQo+ICBTbyBob3cgc2hvdWxkIHdlIHdvcmsgYXJvdW5kIHRoaXM/ICBJIHRoaW5rIHlvdSBz
aG91bGQgYmUgYWJsZSB0byBnZXQNCj4gIHBhcnR3YXkgdGhlcmUgd2l0aCBhIHF1aXJrIHRoYXQg
c2V0czoNCj4gIA0KPiAgICBkZXYtPm5vX21zaSA9IDE7DQo+ICAgIGRldi0+aXJxID0gWTsNCj4g
IA0KPiAgZm9yIHRoaXMgZGV2aWNlLiAgVGhhdCBzaG91bGQgbWFrZSBBRVIgd29yaywgYnV0IG9m
IGNvdXJzZSBQTUUgd291bGQgbm90DQo+ICB3b3JrLg0KPiAgDQo+ICBJcyB0aGVyZSBhIHdheSB0
byBzZXQgdXAgeW91ciBpbnRlcnJ1cHQgY29udHJvbGxlciBzbyB0aGVzZSB0aHJlZQ0KPiAgaW50
ZXJydXB0cyAoWCwgWSwgWiBhYm92ZSkgYWxsIG1hcCB0byB0aGUgc2FtZSBMaW51eCBJUlE/ICBJ
ZiB5b3UgY2FuIGRvDQo+ICB0aGF0LCB5b3UgY291bGQgc2V0IHVwIElOVEEsIHRoZSBBRVIgaW50
ZXJydXB0LCBhbmQgdGhlIFBNRSBpbnRlcnJ1cHQgdG8NCj4gIGFsbCBiZSBvbiB0aGUgc2FtZSBJ
UlEgYW5kIGV2ZXJ5dGhpbmcgc2hvdWxkIHdvcmsuDQo+ICANCj4gIEJqb3JuDQoNCldlJ2xsIHRo
aW5rIGFib3V0IGFsbCB0aGUgd2F5cy4gSXQgaXMgcmVhbGx5IGhlbHBmdWwsIHRoYW5rcyEgIA0K
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 32d4d0a..64833e5 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -15,6 +15,7 @@ 
 #include <linux/slab.h>
 #include <linux/pcieport_if.h>
 #include <linux/aer.h>
+#include <linux/of_irq.h>
 
 #include "../pci.h"
 #include "portdrv.h"
@@ -199,6 +200,28 @@  static int pcie_port_enable_msix(struct pci_dev *dev, int *vectors, int mask)
 static int init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 {
 	int i, irq = -1;
+	int ret;
+	struct device_node *np = NULL;
+
+	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
+		irqs[i] = 0;
+
+	if (dev->bus->dev.of_node)
+		np = dev->bus->dev.of_node;
+
+	/* If root port doesn't support MSI/MSI-X/INTx in RC mode,
+	 * request irq for aer
+	 */
+	if (IS_ENABLED(CONFIG_OF_IRQ) && np &&
+			(mask & PCIE_PORT_SERVICE_PME)) {
+		ret = of_irq_get_byname(np, "aer");
+		if (ret > 0) {
+			irqs[PCIE_PORT_SERVICE_AER_SHIFT] = ret;
+			if (dev->irq)
+				irq = dev->irq;
+			goto no_msi;
+		}
+	}
 
 	/*
 	 * If MSI cannot be used for PCIe PME or hotplug, we have to use
@@ -224,11 +247,13 @@  static int init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 		irq = dev->irq;
 
  no_msi:
-	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
-		irqs[i] = irq;
+	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
+		if (!irqs[i])
+			irqs[i] = irq;
+	}
 	irqs[PCIE_PORT_SERVICE_VC_SHIFT] = -1;
 
-	if (irq < 0)
+	if (irq < 0 && irqs[PCIE_PORT_SERVICE_AER_SHIFT] < 0)
 		return -ENODEV;
 	return 0;
 }