| Message ID | 1473741659-17618-3-git-send-email-po.liu@nxp.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Tue, Sep 13, 2016 at 12:40:59PM +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. Then fixup the dev->irq with it. > > Signed-off-by: Po Liu <po.liu@nxp.com> Will the new kernel work with existing/old DTB? I'm trying to understand the dependency between driver and DTS changes. Shawn > --- > changes for v5: > - Add clear 'aer' interrup-names description > > .../devicetree/bindings/pci/layerscape-pci.txt | 11 +++++--- > drivers/pci/pcie/portdrv_core.c | 31 +++++++++++++++++++--- > 2 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/layerscape-pci.txt b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > index 41e9f55..101d0a7 100644 > --- a/Documentation/devicetree/bindings/pci/layerscape-pci.txt > +++ b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > @@ -18,8 +18,10 @@ Required properties: > - reg: base addresses and lengths of the PCIe controller > - interrupts: A list of interrupt outputs of the controller. Must contain an > entry for each entry in the interrupt-names property. > -- interrupt-names: Must include the following entries: > - "intr": The interrupt that is asserted for controller interrupts > +- interrupt-names: It may be include the following entries: > + "aer": The interrupt that is asserted for aer interrupt > + "pme": The interrupt that is asserted for pme interrupt > + ...... > - fsl,pcie-scfg: Must include two entries. > The first entry must be a link to the SCFG device node > The second entry must be '0' or '1' based on physical PCIe controller index. > @@ -35,8 +37,9 @@ Example: > reg = <0x00 0x03400000 0x0 0x00010000 /* controller registers */ > 0x40 0x00000000 0x0 0x00002000>; /* configuration space */ > reg-names = "regs", "config"; > - interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */ > - interrupt-names = "intr"; > + interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>, /* aer interrupt */ > + <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* pme interrupt */ > + interrupt-names = "aer", "pme"; > fsl,pcie-scfg = <&scfg 0>; > #address-cells = <3>; > #size-cells = <2>; > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index e9270b4..7c4943d 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -16,6 +16,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" > @@ -200,6 +201,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 > @@ -225,11 +248,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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Shawn, > -----Original Message----- > From: Shawn Guo [mailto:shawnguo@kernel.org] > Sent: Sunday, September 18, 2016 8:52 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; Roy Zang; Arnd > Bergmann; Marc Zyngier; Stuart Yoder; Leo Li; M.H. Lian; Murali > Karicheri; Bjorn Helgaas; Mingkai Hu > Subject: Re: [PATCH v5 3/3] pci:aer: add support aer interrupt with none > MSI/MSI-X/INTx mode > > On Tue, Sep 13, 2016 at 12:40:59PM +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. Then fixup the dev->irq with it. > > > > Signed-off-by: Po Liu <po.liu@nxp.com> > > Will the new kernel work with existing/old DTB? I'm trying to > understand the dependency between driver and DTS changes. Yes, We've never use name 'intr' before. So we remove it is ok. 'aer' is a dts name for researching it's true interrupt number by kernel. This patch is first time to use name 'aer'. So it must be compatible with existing/old DTB. > > Shawn > > > --- > > changes for v5: > > - Add clear 'aer' interrup-names description > > > > .../devicetree/bindings/pci/layerscape-pci.txt | 11 +++++--- > > drivers/pci/pcie/portdrv_core.c | 31 > +++++++++++++++++++--- > > 2 files changed, 35 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > index 41e9f55..101d0a7 100644 > > --- a/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > +++ b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > @@ -18,8 +18,10 @@ Required properties: > > - reg: base addresses and lengths of the PCIe controller > > - interrupts: A list of interrupt outputs of the controller. Must > contain an > > entry for each entry in the interrupt-names property. > > -- interrupt-names: Must include the following entries: > > - "intr": The interrupt that is asserted for controller interrupts > > +- interrupt-names: It may be include the following entries: > > + "aer": The interrupt that is asserted for aer interrupt > > + "pme": The interrupt that is asserted for pme interrupt > > + ...... > > - fsl,pcie-scfg: Must include two entries. > > The first entry must be a link to the SCFG device node > > The second entry must be '0' or '1' based on physical PCIe > controller index. > > @@ -35,8 +37,9 @@ Example: > > reg = <0x00 0x03400000 0x0 0x00010000 /* controller > registers */ > > 0x40 0x00000000 0x0 0x00002000>; /* configuration > space */ > > reg-names = "regs", "config"; > > - interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* > controller interrupt */ > > - interrupt-names = "intr"; > > + interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>, /* aer > interrupt */ > > + <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* pme interrupt */ > > + interrupt-names = "aer", "pme"; > > fsl,pcie-scfg = <&scfg 0>; > > #address-cells = <3>; > > #size-cells = <2>; > > diff --git a/drivers/pci/pcie/portdrv_core.c > > b/drivers/pci/pcie/portdrv_core.c index e9270b4..7c4943d 100644 > > --- a/drivers/pci/pcie/portdrv_core.c > > +++ b/drivers/pci/pcie/portdrv_core.c > > @@ -16,6 +16,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" > > @@ -200,6 +201,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 > @@ > > -225,11 +248,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
On Sun, Sep 18, 2016 at 03:37:27AM +0000, Po Liu wrote: > Hi Shawn, > > > > -----Original Message----- > > From: Shawn Guo [mailto:shawnguo@kernel.org] > > Sent: Sunday, September 18, 2016 8:52 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; Roy Zang; Arnd > > Bergmann; Marc Zyngier; Stuart Yoder; Leo Li; M.H. Lian; Murali > > Karicheri; Bjorn Helgaas; Mingkai Hu > > Subject: Re: [PATCH v5 3/3] pci:aer: add support aer interrupt with none > > MSI/MSI-X/INTx mode > > > > On Tue, Sep 13, 2016 at 12:40:59PM +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. Then fixup the dev->irq with it. > > > > > > Signed-off-by: Po Liu <po.liu@nxp.com> > > > > Will the new kernel work with existing/old DTB? I'm trying to > > understand the dependency between driver and DTS changes. > > Yes, We've never use name 'intr' before. So we remove it is ok. > 'aer' is a dts name for researching it's true interrupt number by kernel. This patch is first time to use name 'aer'. So it must be compatible with existing/old DTB. Does that mean driver and DTS changes can go through separate trees, i.e. PCI and arm-soc, without introducing regressions on either tree? Or does the patch series needs to go in as a whole? Shawn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 13, 2016 at 12:40:59PM +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. Then fixup the dev->irq with it. > > Signed-off-by: Po Liu <po.liu@nxp.com> > --- > changes for v5: > - Add clear 'aer' interrup-names description > > .../devicetree/bindings/pci/layerscape-pci.txt | 11 +++++--- > drivers/pci/pcie/portdrv_core.c | 31 +++++++++++++++++++--- > 2 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/layerscape-pci.txt b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > index 41e9f55..101d0a7 100644 > --- a/Documentation/devicetree/bindings/pci/layerscape-pci.txt > +++ b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > @@ -18,8 +18,10 @@ Required properties: > - reg: base addresses and lengths of the PCIe controller > - interrupts: A list of interrupt outputs of the controller. Must contain an > entry for each entry in the interrupt-names property. > -- interrupt-names: Must include the following entries: > - "intr": The interrupt that is asserted for controller interrupts > +- interrupt-names: It may be include the following entries: > + "aer": The interrupt that is asserted for aer interrupt > + "pme": The interrupt that is asserted for pme interrupt > + ...... > - fsl,pcie-scfg: Must include two entries. > The first entry must be a link to the SCFG device node > The second entry must be '0' or '1' based on physical PCIe controller index. > @@ -35,8 +37,9 @@ Example: > reg = <0x00 0x03400000 0x0 0x00010000 /* controller registers */ > 0x40 0x00000000 0x0 0x00002000>; /* configuration space */ > reg-names = "regs", "config"; > - interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */ > - interrupt-names = "intr"; > + interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>, /* aer interrupt */ > + <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* pme interrupt */ > + interrupt-names = "aer", "pme"; > fsl,pcie-scfg = <&scfg 0>; > #address-cells = <3>; > #size-cells = <2>; > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index e9270b4..7c4943d 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -16,6 +16,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" > @@ -200,6 +201,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; > + } > + } We definitely need to solve this somehow. But this approach doesn't feel quite right because it's hard to map this code back to anything in the spec, and it uses a completely platform-dependent name ("interrupt-names aer") in code that is supposedly generic. What if we added some sort of hook that would return the IRQ? Maybe a pcibios_*() hook right now, with the idea of making it a pci_host_bridge function pointer someday? I know the body of the hook would look a lot like what you have here, but at least it would be more obvious that it's platform-specific code. I think your platform supports PME interrupts as well as AER, and they're different IRQs. So you'd probably have to do something similar to the pcie_port_enable_msix() interface, so you can fill in both IRQs. > /* > * If MSI cannot be used for PCIe PME or hotplug, we have to use > @@ -225,11 +248,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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: Thursday, September 22, 2016 6:38 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; Roy Zang; Arnd > Bergmann; Marc Zyngier; Stuart Yoder; Leo Li; M.H. Lian; Murali > Karicheri; Bjorn Helgaas; Shawn Guo; Mingkai Hu > Subject: Re: [PATCH v5 3/3] pci:aer: add support aer interrupt with none > MSI/MSI-X/INTx mode > > On Tue, Sep 13, 2016 at 12:40:59PM +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. Then fixup the dev->irq with it. > > > > Signed-off-by: Po Liu <po.liu@nxp.com> > > --- > > changes for v5: > > - Add clear 'aer' interrup-names description > > > > .../devicetree/bindings/pci/layerscape-pci.txt | 11 +++++--- > > drivers/pci/pcie/portdrv_core.c | 31 > +++++++++++++++++++--- > > 2 files changed, 35 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > index 41e9f55..101d0a7 100644 > > --- a/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > +++ b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > @@ -18,8 +18,10 @@ Required properties: > > - reg: base addresses and lengths of the PCIe controller > > - interrupts: A list of interrupt outputs of the controller. Must > contain an > > entry for each entry in the interrupt-names property. > > -- interrupt-names: Must include the following entries: > > - "intr": The interrupt that is asserted for controller interrupts > > +- interrupt-names: It may be include the following entries: > > + "aer": The interrupt that is asserted for aer interrupt > > + "pme": The interrupt that is asserted for pme interrupt > > + ...... > > - fsl,pcie-scfg: Must include two entries. > > The first entry must be a link to the SCFG device node > > The second entry must be '0' or '1' based on physical PCIe > controller index. > > @@ -35,8 +37,9 @@ Example: > > reg = <0x00 0x03400000 0x0 0x00010000 /* controller > registers */ > > 0x40 0x00000000 0x0 0x00002000>; /* configuration > space */ > > reg-names = "regs", "config"; > > - interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* > controller interrupt */ > > - interrupt-names = "intr"; > > + interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>, /* aer > interrupt */ > > + <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* pme interrupt */ > > + interrupt-names = "aer", "pme"; > > fsl,pcie-scfg = <&scfg 0>; > > #address-cells = <3>; > > #size-cells = <2>; > > diff --git a/drivers/pci/pcie/portdrv_core.c > > b/drivers/pci/pcie/portdrv_core.c index e9270b4..7c4943d 100644 > > --- a/drivers/pci/pcie/portdrv_core.c > > +++ b/drivers/pci/pcie/portdrv_core.c > > @@ -16,6 +16,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" > > @@ -200,6 +201,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; > > + } > > + } > > We definitely need to solve this somehow. But this approach doesn't > feel quite right because it's hard to map this code back to anything in > the spec, and it uses a completely platform-dependent name ("interrupt- > names aer") in code that is supposedly generic. Very agree. > > What if we added some sort of hook that would return the IRQ? Maybe a > pcibios_*() hook right now, with the idea of making it a pci_host_bridge > function pointer someday? Good idea. I'll try one version patch soon. > > I know the body of the hook would look a lot like what you have here, > but at least it would be more obvious that it's platform-specific code. > Not agree more. > I think your platform supports PME interrupts as well as AER, and > they're different IRQs. So you'd probably have to do something similar > to the pcie_port_enable_msix() interface, so you can fill in both IRQs. > Ok, understand. Po > > /* > > * If MSI cannot be used for PCIe PME or hotplug, we have to use > @@ > > -225,11 +248,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
On Sun, Sep 18, 2016 at 03:37:27AM +0000, Po Liu wrote: > Hi Shawn, > > > > -----Original Message----- > > From: Shawn Guo [mailto:shawnguo@kernel.org] > > Sent: Sunday, September 18, 2016 8:52 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; Roy Zang; Arnd > > Bergmann; Marc Zyngier; Stuart Yoder; Leo Li; M.H. Lian; Murali > > Karicheri; Bjorn Helgaas; Mingkai Hu > > Subject: Re: [PATCH v5 3/3] pci:aer: add support aer interrupt with none > > MSI/MSI-X/INTx mode > > > > On Tue, Sep 13, 2016 at 12:40:59PM +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. Then fixup the dev->irq with it. > > > > > > Signed-off-by: Po Liu <po.liu@nxp.com> > > > > Will the new kernel work with existing/old DTB? I'm trying to > > understand the dependency between driver and DTS changes. > > Yes, We've never use name 'intr' before. So we remove it is ok. > 'aer' is a dts name for researching it's true interrupt number by > kernel. This patch is first time to use name 'aer'. So it must be > compatible with existing/old DTB. Please explain why you are not breaking compatibility in the commit message. I asked for this on v2. > > > --- > > > changes for v5: > > > - Add clear 'aer' interrup-names description > > > > > > .../devicetree/bindings/pci/layerscape-pci.txt | 11 +++++--- > > > drivers/pci/pcie/portdrv_core.c | 31 > > +++++++++++++++++++--- > > > 2 files changed, 35 insertions(+), 7 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > > b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > > index 41e9f55..101d0a7 100644 > > > --- a/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > > +++ b/Documentation/devicetree/bindings/pci/layerscape-pci.txt > > > @@ -18,8 +18,10 @@ Required properties: > > > - reg: base addresses and lengths of the PCIe controller > > > - interrupts: A list of interrupt outputs of the controller. Must > > contain an > > > entry for each entry in the interrupt-names property. > > > -- interrupt-names: Must include the following entries: > > > - "intr": The interrupt that is asserted for controller interrupts > > > +- interrupt-names: It may be include the following entries: "may be" is not okay. It should be "must" or explain when an interrupt would not be present. Really, differences in interrupts means you need different compatible strings. Rob > > > + "aer": The interrupt that is asserted for aer interrupt > > > + "pme": The interrupt that is asserted for pme interrupt > > > + ...... -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 21, 2016 at 06:54:59AM +0000, Po Liu wrote: > > > > Will the new kernel work with existing/old DTB? I'm trying to > > > > understand the dependency between driver and DTS changes. > > > > > > Yes, We've never use name 'intr' before. So we remove it is ok. > > > 'aer' is a dts name for researching it's true interrupt number by > > kernel. This patch is first time to use name 'aer'. So it must be > > compatible with existing/old DTB. > > > > Does that mean driver and DTS changes can go through separate trees, i.e. > > PCI and arm-soc, without introducing regressions on either tree? > > Or does the patch series needs to go in as a whole? > > Should be as a whole. The driver base on the dts. Or else, the driver would not found the 'aer' point. Let me try to understand this one more time. So if the new kernel boots with an existing DTB, the driver will fail to find 'aer' interrupt, right? My question is that in this case, the driver will just stop working or keep working in the same way as old kernel. The former case is a regression which breaks old DTB support, and the latter is pretty much like that we are adding a new support, and the DTS change can go independently with driver part through different subsystem tree. Hope I make my question clear this time. Shawn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/pci/layerscape-pci.txt b/Documentation/devicetree/bindings/pci/layerscape-pci.txt index 41e9f55..101d0a7 100644 --- a/Documentation/devicetree/bindings/pci/layerscape-pci.txt +++ b/Documentation/devicetree/bindings/pci/layerscape-pci.txt @@ -18,8 +18,10 @@ Required properties: - reg: base addresses and lengths of the PCIe controller - interrupts: A list of interrupt outputs of the controller. Must contain an entry for each entry in the interrupt-names property. -- interrupt-names: Must include the following entries: - "intr": The interrupt that is asserted for controller interrupts +- interrupt-names: It may be include the following entries: + "aer": The interrupt that is asserted for aer interrupt + "pme": The interrupt that is asserted for pme interrupt + ...... - fsl,pcie-scfg: Must include two entries. The first entry must be a link to the SCFG device node The second entry must be '0' or '1' based on physical PCIe controller index. @@ -35,8 +37,9 @@ Example: reg = <0x00 0x03400000 0x0 0x00010000 /* controller registers */ 0x40 0x00000000 0x0 0x00002000>; /* configuration space */ reg-names = "regs", "config"; - interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */ - interrupt-names = "intr"; + interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>, /* aer interrupt */ + <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* pme interrupt */ + interrupt-names = "aer", "pme"; fsl,pcie-scfg = <&scfg 0>; #address-cells = <3>; #size-cells = <2>; diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index e9270b4..7c4943d 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -16,6 +16,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" @@ -200,6 +201,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 @@ -225,11 +248,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; }
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. Then fixup the dev->irq with it. Signed-off-by: Po Liu <po.liu@nxp.com> --- changes for v5: - Add clear 'aer' interrup-names description .../devicetree/bindings/pci/layerscape-pci.txt | 11 +++++--- drivers/pci/pcie/portdrv_core.c | 31 +++++++++++++++++++--- 2 files changed, 35 insertions(+), 7 deletions(-)