Xilinx ZynqMP PS PCIe does not report AER interrupts using Advanced
Error Interrupt Message Number. The controller has dedicated interrupt line
for reporting PCIe errors along with AER.
Since pcie-xilinx-nwl.c is platform driver and AER uses irq from
pci_dev, using struct device_node private data to save xilinx controller
error irq line. Using PCI quirks this data is passed to sysdata of
root port pci_dev which is retrieved and used for AER handler registration.
Bharat Kumar Gogada (3):
PCI: xilinx-nwl: Save error IRQ number in device_node private data
PCI: Use dedicated Xilinx controller irq number for AER
PCI/portdrv: Add support for sharing xilinx controller irq with AER
drivers/pci/controller/pcie-xilinx-nwl.c | 6 ++++++
drivers/pci/pcie/portdrv_core.c | 4 ++++
drivers/pci/quirks.c | 29 +++++++++++++++++++++++++++++
3 files changed, 39 insertions(+), 0 deletions(-)
Xilinx ZynqMP PS PCIe does not report AER interrupts using Advanced
Error Interrupt Message Number. The controller has dedicated interrupt line
for reporting PCIe errors along with AER.
Using pci_dev->sysdata of root port to save controller irq number, which
will be used for registering AER irq handler.
Signed-off-by: Bharat Kumar Gogada <[email protected]>
---
drivers/pci/quirks.c | 29 +++++++++++++++++++++++++++++
1 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f439de8..e666373 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4753,3 +4753,32 @@ static void quirk_gpu_hda(struct pci_dev *hda)
PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
+
+#if defined(CONFIG_ARCH_ZYNQMP) && defined(CONFIG_PCIE_XILINX_NWL) && \
+ defined(CONFIG_PCIEAER)
+/*
+ * Xilinx ZynqMP PS PCIe does not report AER interrupts using Advanced
+ * Error Interrupt Message Number. The controller has dedicated interrupt line
+ * for reporting PCIe errors along with AER.
+ */
+#include <linux/of.h>
+#include <linux/of_irq.h>
+
+static void quirk_xilinx_aer_irq(struct pci_dev *dev)
+{
+ struct device_node *dev_node;
+
+ if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
+ pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR)) {
+ dev_node = of_find_compatible_node(NULL, NULL,
+ "xlnx,nwl-pcie-2.11");
+ if (!dev_node) {
+ dev_err(&dev->dev, "Error could not find ZynqMP PS PCIe node\n");
+ return;
+ }
+
+ dev->sysdata = dev_node->data;
+ }
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_XILINX, PCI_ANY_ID, quirk_xilinx_aer_irq);
+#endif
--
1.7.1
Xilinx ZynqMP PS PCIe has dedicated interrupt line for
reporting PCIe errors along with AER.
Save this error irq number in struct device_node private data,
this will be used via PCI qiurks for AER kernel service.
Signed-off-by: Bharat Kumar Gogada <[email protected]>
---
drivers/pci/controller/pcie-xilinx-nwl.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index fb32840..d505fe5 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -663,6 +663,9 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
struct platform_device *pdev = to_platform_device(dev);
u32 breg_val, ecam_val, first_busno = 0;
int err;
+#ifdef CONFIG_PCIEAER
+ struct device_node *node = dev->of_node;
+#endif
breg_val = nwl_bridge_readl(pcie, E_BREG_CAPABILITIES) & BREG_PRESENT;
if (!breg_val) {
@@ -744,6 +747,9 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
pcie->irq_misc);
return err;
}
+#ifdef CONFIG_PCIEAER
+ node->data = &pcie->irq_misc;
+#endif
/* Disable all misc interrupts */
nwl_bridge_writel(pcie, (u32)~MSGF_MISC_SR_MASKALL, MSGF_MISC_MASK);
--
1.7.1
Xilinx ZynqMP PS PCIe does not report AER interrupts using Advanced
Error Interrupt Message Number. The controller has dedicated interrupt line
for reporting PCIe errors along with AER.
Using dedicated controller irq number for AER which is shared with misc
interrupt handler in pcie-xilinx-nwl. This irq number is set
using PCI quirk.
Signed-off-by: Bharat Kumar Gogada <[email protected]>
---
drivers/pci/pcie/portdrv_core.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index e0261ad..fa9150e 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -264,6 +264,10 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
int retval;
struct pcie_device *pcie;
struct device *device;
+#if defined(CONFIG_ARCH_ZYNQMP) && defined(CONFIG_PCIE_XILINX_NWL)
+ if (service == PCIE_PORT_SERVICE_AER && pdev->sysdata)
+ irq = *(int *)pdev->sysdata;
+#endif
pcie = kzalloc(sizeof(*pcie), GFP_KERNEL);
if (!pcie)
--
1.7.1
On 8/1/2018 9:44 AM, Bharat Kumar Gogada wrote:
> Xilinx ZynqMP PS PCIe does not report AER interrupts using Advanced
> Error Interrupt Message Number. The controller has dedicated interrupt line
> for reporting PCIe errors along with AER.
>
> Using dedicated controller irq number for AER which is shared with misc
> interrupt handler in pcie-xilinx-nwl. This irq number is set
> using PCI quirk.
>
> Signed-off-by: Bharat Kumar Gogada<[email protected]>
> ---
> drivers/pci/pcie/portdrv_core.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index e0261ad..fa9150e 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -264,6 +264,10 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
> int retval;
> struct pcie_device *pcie;
> struct device *device;
> +#if defined(CONFIG_ARCH_ZYNQMP) && defined(CONFIG_PCIE_XILINX_NWL)
> + if (service == PCIE_PORT_SERVICE_AER && pdev->sysdata)
> + irq = *(int *)pdev->sysdata;
> +#endif
>
I remember seeing a similar patch before. The patch above looks ugly to
be honest. Need to find a way to generalize this. Can you search the
mailing list archive to find out what the history is?
> +#ifdef CONFIG_PCIEAER
> + struct device_node *node = dev->of_node;
> +#endif
>
> breg_val = nwl_bridge_readl(pcie, E_BREG_CAPABILITIES) & BREG_PRESENT;
> if (!breg_val) {
> @@ -744,6 +747,9 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
> pcie->irq_misc);
> return err;
> }
> +#ifdef CONFIG_PCIEAER
> + node->data = &pcie->irq_misc;
> +#endif
Is there any good reason for these ifdefs? Always assigning the node
data would seem harmless and much cleaner to me.
On Wed, Aug 01, 2018 at 11:05:09AM -0700, Sinan Kaya wrote:
> On 8/1/2018 9:44 AM, Bharat Kumar Gogada wrote:
> > Xilinx ZynqMP PS PCIe does not report AER interrupts using Advanced
> > Error Interrupt Message Number. The controller has dedicated interrupt line
> > for reporting PCIe errors along with AER.
> >
> > Using dedicated controller irq number for AER which is shared with misc
> > interrupt handler in pcie-xilinx-nwl. This irq number is set
> > using PCI quirk.
> >
> > Signed-off-by: Bharat Kumar Gogada<[email protected]>
> > ---
> > drivers/pci/pcie/portdrv_core.c | 4 ++++
> > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> > index e0261ad..fa9150e 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -264,6 +264,10 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
> > int retval;
> > struct pcie_device *pcie;
> > struct device *device;
> > +#if defined(CONFIG_ARCH_ZYNQMP) && defined(CONFIG_PCIE_XILINX_NWL)
> > + if (service == PCIE_PORT_SERVICE_AER && pdev->sysdata)
> > + irq = *(int *)pdev->sysdata;
> > +#endif
>
> I remember seeing a similar patch before.
Are you thinking of [1]?
> The patch above looks ugly to be honest. Need to find a way to
> generalize this. Can you search the mailing list archive to find out
> what the history is?
I agree, the patch above is too ugly as-is because (a) it puts very
arch-specific code in a generic code path and (b) it uses a
compile-time test, which means it probably breaks generic arm64
kernels that contain support for several platforms.
We also need to know whether this is a hardware defect or just
something that is allowed by the spec but Linux doesn't support yet.
The diagram in PCIe r4.0, sec 6.2.6, does leave room for MSI, MSI-X,
INTx, and platform-specific interrupt schemes.
[1] https://lkml.kernel.org/r/[email protected]
On Wed, Aug 01, 2018 at 10:14:48PM +0530, Bharat Kumar Gogada wrote:
> Xilinx ZynqMP PS PCIe does not report AER interrupts using Advanced
> Error Interrupt Message Number. The controller has dedicated interrupt line
> for reporting PCIe errors along with AER.
>
> Using pci_dev->sysdata of root port to save controller irq number, which
> will be used for registering AER irq handler.
>
> Signed-off-by: Bharat Kumar Gogada <[email protected]>
> ---
> drivers/pci/quirks.c | 29 +++++++++++++++++++++++++++++
> 1 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f439de8..e666373 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4753,3 +4753,32 @@ static void quirk_gpu_hda(struct pci_dev *hda)
> PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
> DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
> +
> +#if defined(CONFIG_ARCH_ZYNQMP) && defined(CONFIG_PCIE_XILINX_NWL) && \
> + defined(CONFIG_PCIEAER)
> +/*
> + * Xilinx ZynqMP PS PCIe does not report AER interrupts using Advanced
> + * Error Interrupt Message Number. The controller has dedicated interrupt line
> + * for reporting PCIe errors along with AER.
Your comment says "PCIe errors along with AER", but your ifdef tests
CONFIG_PCIEAER. Per PCIe r4.0, sec 6.2.6, interrupts can be generated
even if AER isn't supported. Don't we want to allow that?
> + */
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +
> +static void quirk_xilinx_aer_irq(struct pci_dev *dev)
> +{
> + struct device_node *dev_node;
> +
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> + pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR)) {
> + dev_node = of_find_compatible_node(NULL, NULL,
> + "xlnx,nwl-pcie-2.11");
> + if (!dev_node) {
> + dev_err(&dev->dev, "Error could not find ZynqMP PS PCIe node\n");
Use pci_err().
Doesn't arm64 support generic kernels that may contain support for
several platforms? We don't want a warning if a generic kernel
happens to be running on a non-ZynqMP system.
> + return;
> + }
> +
> + dev->sysdata = dev_node->data;
> + }
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_XILINX, PCI_ANY_ID, quirk_xilinx_aer_irq);
> +#endif
> --
> 1.7.1
>
On 8/6/2018 4:56 PM, Bjorn Helgaas wrote:
>> I remember seeing a similar patch before.
> Are you thinking of [1]?
>
yup.
> Subject: Re: [PATCH 3/3] PCI/portdrv: Add support for sharing xilinx
> controller irq with AER
>
> On Wed, Aug 01, 2018 at 11:05:09AM -0700, Sinan Kaya wrote:
> > On 8/1/2018 9:44 AM, Bharat Kumar Gogada wrote:
> > > Xilinx ZynqMP PS PCIe does not report AER interrupts using Advanced
> > > Error Interrupt Message Number. The controller has dedicated
> > > interrupt line for reporting PCIe errors along with AER.
> > >
> > > Using dedicated controller irq number for AER which is shared with
> > > misc interrupt handler in pcie-xilinx-nwl. This irq number is set
> > > using PCI quirk.
> > >
> > > Signed-off-by: Bharat Kumar Gogada<[email protected]>
> > > ---
> > > drivers/pci/pcie/portdrv_core.c | 4 ++++
> > > 1 files changed, 4 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/pci/pcie/portdrv_core.c
> > > b/drivers/pci/pcie/portdrv_core.c index e0261ad..fa9150e 100644
> > > --- a/drivers/pci/pcie/portdrv_core.c
> > > +++ b/drivers/pci/pcie/portdrv_core.c
> > > @@ -264,6 +264,10 @@ static int pcie_device_init(struct pci_dev *pdev,
> int service, int irq)
> > > int retval;
> > > struct pcie_device *pcie;
> > > struct device *device;
> > > +#if defined(CONFIG_ARCH_ZYNQMP) &&
> defined(CONFIG_PCIE_XILINX_NWL)
> > > + if (service == PCIE_PORT_SERVICE_AER && pdev->sysdata)
> > > + irq = *(int *)pdev->sysdata;
> > > +#endif
> >
> > I remember seeing a similar patch before.
>
> Are you thinking of [1]?
Thanks for reviewing. Yes this case is also similar to [1].
I'm not aware of this [1]. I will withdraw this current patches.
Will work on a different model based on your suggestions.
Regards,
Bharat