| Message ID | 1603848703-21099-4-git-send-email-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: uniphier: Add PME/AER support for UniPhier PCIe host controller | expand |
On Wed, Oct 28, 2020 at 10:31:43AM +0900, Kunihiko Hayashi wrote: > This patch adds misc interrupt handler to detect and invoke PME/AER event. > > In UniPhier PCIe controller, PME/AER signals are assigned to the same > signal as MSI by the internal logic. These signals should be detected by > the internal register, however, DWC MSI handler can't handle these signals. I don't know what "PME/AER signals are assigned to the same signal as MSI" means. I'm trying to figure out if this is talking about PME/AER MSI vector numbers (probably not) or some internal wire that's not architecturally visible or what. Probably also not related to the fact that PME, hotplug, and bandwidth notifications share the same MSI/MSI-X vector. Is this something that's going to be applicable to all the DWC-based drivers? > DWC MSI handler calls .msi_host_isr() callback function, that detects > PME/AER signals with the internal register and invokes the interrupt > with PME/AER vIRQ numbers. > > These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port() > function. > > Cc: Marc Zyngier <maz@kernel.org> > Cc: Jingoo Han <jingoohan1@gmail.com> > Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > Reviewed-by: Rob Herring <robh@kernel.org> > --- > drivers/pci/controller/dwc/pcie-uniphier.c | 77 +++++++++++++++++++++++++----- > 1 file changed, 66 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c > index 4817626..237537a 100644 > --- a/drivers/pci/controller/dwc/pcie-uniphier.c > +++ b/drivers/pci/controller/dwc/pcie-uniphier.c > @@ -21,6 +21,7 @@ > #include <linux/reset.h> > > #include "pcie-designware.h" > +#include "../../pcie/portdrv.h" > > #define PCL_PINCTRL0 0x002c > #define PCL_PERST_PLDN_REGEN BIT(12) > @@ -44,7 +45,9 @@ > #define PCL_SYS_AUX_PWR_DET BIT(8) > > #define PCL_RCV_INT 0x8108 > +#define PCL_RCV_INT_ALL_INT_MASK GENMASK(28, 25) > #define PCL_RCV_INT_ALL_ENABLE GENMASK(20, 17) > +#define PCL_RCV_INT_ALL_MSI_MASK GENMASK(12, 9) > #define PCL_CFG_BW_MGT_STATUS BIT(4) > #define PCL_CFG_LINK_AUTO_BW_STATUS BIT(3) > #define PCL_CFG_AER_RC_ERR_MSI_STATUS BIT(2) > @@ -68,6 +71,8 @@ struct uniphier_pcie_priv { > struct reset_control *rst; > struct phy *phy; > struct irq_domain *legacy_irq_domain; > + int aer_irq; > + int pme_irq; > }; > > #define to_uniphier_pcie(x) dev_get_drvdata((x)->dev) > @@ -167,7 +172,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci) > > static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv) > { > - writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT); > + u32 val; > + > + val = PCL_RCV_INT_ALL_ENABLE; > + if (pci_msi_enabled()) > + val |= PCL_RCV_INT_ALL_INT_MASK; > + else > + val |= PCL_RCV_INT_ALL_MSI_MASK; I'm confused about how this works. Root Ports can signal AER errors with either INTx or MSI. This is controlled by the architected Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at PCIe r5.0, sec 6.2.4.1.2). The code here doesn't look related to those bits. Does this code mean that if pci_msi_enabled(), the Root Port will always signal with MSI (if MSI Enable is set) and will *never* signal with INTx? > + writel(val, priv->base + PCL_RCV_INT); > writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX); > } > > @@ -231,28 +244,52 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = { > .map = uniphier_pcie_intx_map, > }; > > -static void uniphier_pcie_irq_handler(struct irq_desc *desc) > +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi) > { > - struct pcie_port *pp = irq_desc_get_handler_data(desc); > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > - struct irq_chip *chip = irq_desc_get_chip(desc); > - unsigned long reg; > - u32 val, bit, virq; > + u32 val; > > - /* INT for debug */ > val = readl(priv->base + PCL_RCV_INT); > > if (val & PCL_CFG_BW_MGT_STATUS) > dev_dbg(pci->dev, "Link Bandwidth Management Event\n"); > + Looks like a spurious whitespace change? > if (val & PCL_CFG_LINK_AUTO_BW_STATUS) > dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n"); > - if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) > - dev_dbg(pci->dev, "Root Error\n"); > - if (val & PCL_CFG_PME_MSI_STATUS) > - dev_dbg(pci->dev, "PME Interrupt\n"); > + > + if (is_msi) { > + if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) { > + dev_dbg(pci->dev, "Root Error Status\n"); > + if (priv->aer_irq) > + generic_handle_irq(priv->aer_irq); > + } > + > + if (val & PCL_CFG_PME_MSI_STATUS) { > + dev_dbg(pci->dev, "PME Interrupt\n"); > + if (priv->pme_irq) > + generic_handle_irq(priv->pme_irq); > + } > + } > > writel(val, priv->base + PCL_RCV_INT); > +} > + > +static void uniphier_pcie_msi_host_isr(struct pcie_port *pp) > +{ > + uniphier_pcie_misc_isr(pp, true); > +} > + > +static void uniphier_pcie_irq_handler(struct irq_desc *desc) > +{ > + struct pcie_port *pp = irq_desc_get_handler_data(desc); > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + unsigned long reg; > + u32 val, bit, virq; > + > + uniphier_pcie_misc_isr(pp, false); > > /* INTx */ > chained_irq_enter(chip, desc); > @@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp) > > static const struct dw_pcie_host_ops uniphier_pcie_host_ops = { > .host_init = uniphier_pcie_host_init, > + .msi_host_isr = uniphier_pcie_msi_host_isr, > }; > > static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > @@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > struct dw_pcie *pci = &priv->pci; > struct pcie_port *pp = &pci->pp; > struct device *dev = &pdev->dev; > + struct pci_dev *pcidev; > int ret; > > pp->ops = &uniphier_pcie_host_ops; > @@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > return ret; > } > > + /* irq for PME */ > + list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) { > + priv->pme_irq = > + pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME); > + if (priv->pme_irq) > + break; Does this mean that all Root Ports must use the same MSI vector? I don't think that's a PCIe spec requirement, though of course DWC may have its own restrictions. I don't think this depends on CONFIG_PCIEPORTBUS, so it looks like it's possible to have # CONFIG_PCIEPORTBUS is not set PCIE_UNIPHIER=y in which case I think you'll have a link error. > + } > + > + /* irq for AER */ > + list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) { > + priv->aer_irq = > + pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_AER); > + if (priv->aer_irq) > + break; > + } > + > return 0; > } > > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Nov 24, 2020 at 05:20:37PM -0600, Bjorn Helgaas wrote: > On Wed, Oct 28, 2020 at 10:31:43AM +0900, Kunihiko Hayashi wrote: > > This patch adds misc interrupt handler to detect and invoke PME/AER event. > > > > In UniPhier PCIe controller, PME/AER signals are assigned to the same > > signal as MSI by the internal logic. These signals should be detected by > > the internal register, however, DWC MSI handler can't handle these signals. > > I don't know what "PME/AER signals are assigned to the same signal as > MSI" means. The host controller embeds an interrupt-controller whose IRQ wire output is cascaded into the main interrupt controller. The host-bridge embedded controller receives MSI writes from devices and it turns them into an edge IRQ into the main interrupt controller. To ack/mask the MSIs at host contoller interrupt controller level, there is a control register in the host controller that needs handling upon IRQ reception. The *RP* (and AFAIU the RP *only*) signals the PME/AER MSI using the same wire to the main interrupt controller but its ack/mask is handled by a different bit in the host bridge control register above, therefore the cascaded IRQ isr needs to know which virq it is actually handling to ack/mask accordingly. IMO this should be modelled with a separate IRQ domain and chip for the root port (yes this implies describing the root port in the dts file with a separate msi-parent). This series as it stands is a kludge. Lorenzo > I'm trying to figure out if this is talking about PME/AER MSI vector > numbers (probably not) or some internal wire that's not > architecturally visible or what. > > Probably also not related to the fact that PME, hotplug, and bandwidth > notifications share the same MSI/MSI-X vector. > > Is this something that's going to be applicable to all the DWC-based > drivers? > > > DWC MSI handler calls .msi_host_isr() callback function, that detects > > PME/AER signals with the internal register and invokes the interrupt > > with PME/AER vIRQ numbers. > > > > These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port() > > function. > > > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Jingoo Han <jingoohan1@gmail.com> > > Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > Reviewed-by: Rob Herring <robh@kernel.org> > > --- > > drivers/pci/controller/dwc/pcie-uniphier.c | 77 +++++++++++++++++++++++++----- > > 1 file changed, 66 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c > > index 4817626..237537a 100644 > > --- a/drivers/pci/controller/dwc/pcie-uniphier.c > > +++ b/drivers/pci/controller/dwc/pcie-uniphier.c > > @@ -21,6 +21,7 @@ > > #include <linux/reset.h> > > > > #include "pcie-designware.h" > > +#include "../../pcie/portdrv.h" > > > > #define PCL_PINCTRL0 0x002c > > #define PCL_PERST_PLDN_REGEN BIT(12) > > @@ -44,7 +45,9 @@ > > #define PCL_SYS_AUX_PWR_DET BIT(8) > > > > #define PCL_RCV_INT 0x8108 > > +#define PCL_RCV_INT_ALL_INT_MASK GENMASK(28, 25) > > #define PCL_RCV_INT_ALL_ENABLE GENMASK(20, 17) > > +#define PCL_RCV_INT_ALL_MSI_MASK GENMASK(12, 9) > > #define PCL_CFG_BW_MGT_STATUS BIT(4) > > #define PCL_CFG_LINK_AUTO_BW_STATUS BIT(3) > > #define PCL_CFG_AER_RC_ERR_MSI_STATUS BIT(2) > > @@ -68,6 +71,8 @@ struct uniphier_pcie_priv { > > struct reset_control *rst; > > struct phy *phy; > > struct irq_domain *legacy_irq_domain; > > + int aer_irq; > > + int pme_irq; > > }; > > > > #define to_uniphier_pcie(x) dev_get_drvdata((x)->dev) > > @@ -167,7 +172,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci) > > > > static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv) > > { > > - writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT); > > + u32 val; > > + > > + val = PCL_RCV_INT_ALL_ENABLE; > > + if (pci_msi_enabled()) > > + val |= PCL_RCV_INT_ALL_INT_MASK; > > + else > > + val |= PCL_RCV_INT_ALL_MSI_MASK; > > I'm confused about how this works. Root Ports can signal AER errors > with either INTx or MSI. This is controlled by the architected > Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at > PCIe r5.0, sec 6.2.4.1.2). > > The code here doesn't look related to those bits. Does this code mean > that if pci_msi_enabled(), the Root Port will always signal with MSI > (if MSI Enable is set) and will *never* signal with INTx? > > > + writel(val, priv->base + PCL_RCV_INT); > > writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX); > > } > > > > @@ -231,28 +244,52 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = { > > .map = uniphier_pcie_intx_map, > > }; > > > > -static void uniphier_pcie_irq_handler(struct irq_desc *desc) > > +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi) > > { > > - struct pcie_port *pp = irq_desc_get_handler_data(desc); > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > > - struct irq_chip *chip = irq_desc_get_chip(desc); > > - unsigned long reg; > > - u32 val, bit, virq; > > + u32 val; > > > > - /* INT for debug */ > > val = readl(priv->base + PCL_RCV_INT); > > > > if (val & PCL_CFG_BW_MGT_STATUS) > > dev_dbg(pci->dev, "Link Bandwidth Management Event\n"); > > + > > Looks like a spurious whitespace change? > > > if (val & PCL_CFG_LINK_AUTO_BW_STATUS) > > dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n"); > > - if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) > > - dev_dbg(pci->dev, "Root Error\n"); > > - if (val & PCL_CFG_PME_MSI_STATUS) > > - dev_dbg(pci->dev, "PME Interrupt\n"); > > + > > + if (is_msi) { > > + if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) { > > + dev_dbg(pci->dev, "Root Error Status\n"); > > + if (priv->aer_irq) > > + generic_handle_irq(priv->aer_irq); > > + } > > + > > + if (val & PCL_CFG_PME_MSI_STATUS) { > > + dev_dbg(pci->dev, "PME Interrupt\n"); > > + if (priv->pme_irq) > > + generic_handle_irq(priv->pme_irq); > > + } > > + } > > > > writel(val, priv->base + PCL_RCV_INT); > > +} > > + > > +static void uniphier_pcie_msi_host_isr(struct pcie_port *pp) > > +{ > > + uniphier_pcie_misc_isr(pp, true); > > +} > > + > > +static void uniphier_pcie_irq_handler(struct irq_desc *desc) > > +{ > > + struct pcie_port *pp = irq_desc_get_handler_data(desc); > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + unsigned long reg; > > + u32 val, bit, virq; > > + > > + uniphier_pcie_misc_isr(pp, false); > > > > /* INTx */ > > chained_irq_enter(chip, desc); > > @@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp) > > > > static const struct dw_pcie_host_ops uniphier_pcie_host_ops = { > > .host_init = uniphier_pcie_host_init, > > + .msi_host_isr = uniphier_pcie_msi_host_isr, > > }; > > > > static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > > @@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > > struct dw_pcie *pci = &priv->pci; > > struct pcie_port *pp = &pci->pp; > > struct device *dev = &pdev->dev; > > + struct pci_dev *pcidev; > > int ret; > > > > pp->ops = &uniphier_pcie_host_ops; > > @@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > > return ret; > > } > > > > + /* irq for PME */ > > + list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) { > > + priv->pme_irq = > > + pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME); > > + if (priv->pme_irq) > > + break; > > Does this mean that all Root Ports must use the same MSI vector? I > don't think that's a PCIe spec requirement, though of course DWC may > have its own restrictions. > > I don't think this depends on CONFIG_PCIEPORTBUS, so it looks like > it's possible to have > > # CONFIG_PCIEPORTBUS is not set > PCIE_UNIPHIER=y > > in which case I think you'll have a link error. > > > + } > > + > > + /* irq for AER */ > > + list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) { > > + priv->aer_irq = > > + pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_AER); > > + if (priv->aer_irq) > > + break; > > + } > > + > > return 0; > > } > > > > -- > > 2.7.4 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Bjorn Lorenzo, On 2020/11/25 19:23, Lorenzo Pieralisi wrote: > On Tue, Nov 24, 2020 at 05:20:37PM -0600, Bjorn Helgaas wrote: >> On Wed, Oct 28, 2020 at 10:31:43AM +0900, Kunihiko Hayashi wrote: >>> This patch adds misc interrupt handler to detect and invoke PME/AER event. >>> >>> In UniPhier PCIe controller, PME/AER signals are assigned to the same >>> signal as MSI by the internal logic. These signals should be detected by >>> the internal register, however, DWC MSI handler can't handle these signals. >> >> I don't know what "PME/AER signals are assigned to the same signal as >> MSI" means. > > The host controller embeds an interrupt-controller whose IRQ wire output > is cascaded into the main interrupt controller. > > The host-bridge embedded controller receives MSI writes from devices > and it turns them into an edge IRQ into the main interrupt controller. > > To ack/mask the MSIs at host contoller interrupt controller level, there > is a control register in the host controller that needs handling upon > IRQ reception. Thanks for explaining that. In my understanding, PME/AER signals are cascaded to MSI by embedded interrupt controller (not "assigned"). > The *RP* (and AFAIU the RP *only*) signals the PME/AER MSI using the > same wire to the main interrupt controller but its ack/mask is handled > by a different bit in the host bridge control register above, therefore > the cascaded IRQ isr needs to know which virq it is actually handling > to ack/mask accordingly. Sorry what is RP? Root complex or something? > IMO this should be modelled with a separate IRQ domain and chip for > the root port (yes this implies describing the root port in the dts > file with a separate msi-parent). > > This series as it stands is a kludge. I see. However I need some time to consider the way to separate IRQ domain. Is there any idea or example to handle PME/AER with IRQ domain? >> I'm trying to figure out if this is talking about PME/AER MSI vector >> numbers (probably not) or some internal wire that's not >> architecturally visible or what. >> >> Probably also not related to the fact that PME, hotplug, and bandwidth >> notifications share the same MSI/MSI-X vector. >> >> Is this something that's going to be applicable to all the DWC-based >> drivers? I think that this feature depends on the vendor specification. At least, the registers to control or check these signals are implemented in the vendor's logic. >>> DWC MSI handler calls .msi_host_isr() callback function, that detects >>> PME/AER signals with the internal register and invokes the interrupt >>> with PME/AER vIRQ numbers. >>> >>> These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port() >>> function. >>> >>> Cc: Marc Zyngier <maz@kernel.org> >>> Cc: Jingoo Han <jingoohan1@gmail.com> >>> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com> >>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>> Reviewed-by: Rob Herring <robh@kernel.org> >>> --- >>> drivers/pci/controller/dwc/pcie-uniphier.c | 77 +++++++++++++++++++++++++----- >>> 1 file changed, 66 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c >>> index 4817626..237537a 100644 >>> --- a/drivers/pci/controller/dwc/pcie-uniphier.c >>> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c >>> @@ -21,6 +21,7 @@ >>> #include <linux/reset.h> >>> >>> #include "pcie-designware.h" >>> +#include "../../pcie/portdrv.h" >>> >>> #define PCL_PINCTRL0 0x002c >>> #define PCL_PERST_PLDN_REGEN BIT(12) >>> @@ -44,7 +45,9 @@ >>> #define PCL_SYS_AUX_PWR_DET BIT(8) >>> >>> #define PCL_RCV_INT 0x8108 >>> +#define PCL_RCV_INT_ALL_INT_MASK GENMASK(28, 25) >>> #define PCL_RCV_INT_ALL_ENABLE GENMASK(20, 17) >>> +#define PCL_RCV_INT_ALL_MSI_MASK GENMASK(12, 9) >>> #define PCL_CFG_BW_MGT_STATUS BIT(4) >>> #define PCL_CFG_LINK_AUTO_BW_STATUS BIT(3) >>> #define PCL_CFG_AER_RC_ERR_MSI_STATUS BIT(2) >>> @@ -68,6 +71,8 @@ struct uniphier_pcie_priv { >>> struct reset_control *rst; >>> struct phy *phy; >>> struct irq_domain *legacy_irq_domain; >>> + int aer_irq; >>> + int pme_irq; >>> }; >>> >>> #define to_uniphier_pcie(x) dev_get_drvdata((x)->dev) >>> @@ -167,7 +172,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci) >>> >>> static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv) >>> { >>> - writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT); >>> + u32 val; >>> + >>> + val = PCL_RCV_INT_ALL_ENABLE; >>> + if (pci_msi_enabled()) >>> + val |= PCL_RCV_INT_ALL_INT_MASK; >>> + else >>> + val |= PCL_RCV_INT_ALL_MSI_MASK; >> >> I'm confused about how this works. Root Ports can signal AER errors >> with either INTx or MSI. This is controlled by the architected >> Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at >> PCIe r5.0, sec 6.2.4.1.2). >> >> The code here doesn't look related to those bits. Does this code mean >> that if pci_msi_enabled(), the Root Port will always signal with MSI >> (if MSI Enable is set) and will *never* signal with INTx? According to the spec sheet, we need to set interrupt enable bit for either INTx or MSI, the other bit should be reset. These bits are in config space and handled by the framework. The controller signals AER errors with the interrupt that is either INTx or MSI enabled. I think that the only way to know if MSI is enabled (and INTX is disabled) is to use pci_msi_enabled(). >>> + writel(val, priv->base + PCL_RCV_INT); >>> writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX); >>> } >>> >>> @@ -231,28 +244,52 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = { >>> .map = uniphier_pcie_intx_map, >>> }; >>> >>> -static void uniphier_pcie_irq_handler(struct irq_desc *desc) >>> +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi) >>> { >>> - struct pcie_port *pp = irq_desc_get_handler_data(desc); >>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >>> struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); >>> - struct irq_chip *chip = irq_desc_get_chip(desc); >>> - unsigned long reg; >>> - u32 val, bit, virq; >>> + u32 val; >>> >>> - /* INT for debug */ >>> val = readl(priv->base + PCL_RCV_INT); >>> >>> if (val & PCL_CFG_BW_MGT_STATUS) >>> dev_dbg(pci->dev, "Link Bandwidth Management Event\n"); >>> + >> >> Looks like a spurious whitespace change? Oops, I'll remove it. >>> if (val & PCL_CFG_LINK_AUTO_BW_STATUS) >>> dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n"); >>> - if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) >>> - dev_dbg(pci->dev, "Root Error\n"); >>> - if (val & PCL_CFG_PME_MSI_STATUS) >>> - dev_dbg(pci->dev, "PME Interrupt\n"); >>> + >>> + if (is_msi) { >>> + if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) { >>> + dev_dbg(pci->dev, "Root Error Status\n"); >>> + if (priv->aer_irq) >>> + generic_handle_irq(priv->aer_irq); >>> + } >>> + >>> + if (val & PCL_CFG_PME_MSI_STATUS) { >>> + dev_dbg(pci->dev, "PME Interrupt\n"); >>> + if (priv->pme_irq) >>> + generic_handle_irq(priv->pme_irq); >>> + } >>> + } >>> >>> writel(val, priv->base + PCL_RCV_INT); >>> +} >>> + >>> +static void uniphier_pcie_msi_host_isr(struct pcie_port *pp) >>> +{ >>> + uniphier_pcie_misc_isr(pp, true); >>> +} >>> + >>> +static void uniphier_pcie_irq_handler(struct irq_desc *desc) >>> +{ >>> + struct pcie_port *pp = irq_desc_get_handler_data(desc); >>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >>> + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); >>> + struct irq_chip *chip = irq_desc_get_chip(desc); >>> + unsigned long reg; >>> + u32 val, bit, virq; >>> + >>> + uniphier_pcie_misc_isr(pp, false); >>> >>> /* INTx */ >>> chained_irq_enter(chip, desc); >>> @@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp) >>> >>> static const struct dw_pcie_host_ops uniphier_pcie_host_ops = { >>> .host_init = uniphier_pcie_host_init, >>> + .msi_host_isr = uniphier_pcie_msi_host_isr, >>> }; >>> >>> static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, >>> @@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, >>> struct dw_pcie *pci = &priv->pci; >>> struct pcie_port *pp = &pci->pp; >>> struct device *dev = &pdev->dev; >>> + struct pci_dev *pcidev; >>> int ret; >>> >>> pp->ops = &uniphier_pcie_host_ops; >>> @@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, >>> return ret; >>> } >>> >>> + /* irq for PME */ >>> + list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) { >>> + priv->pme_irq = >>> + pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME); >>> + if (priv->pme_irq) >>> + break; >> >> Does this mean that all Root Ports must use the same MSI vector? I >> don't think that's a PCIe spec requirement, though of course DWC may >> have its own restrictions. >> This controller has one port implementation only, so this assumes that there is one root port. >> I don't think this depends on CONFIG_PCIEPORTBUS, so it looks like >> it's possible to have >> >> # CONFIG_PCIEPORTBUS is not set >> PCIE_UNIPHIER=y >> >> in which case I think you'll have a link error. Indeed. To use port functions needs to define PCIEPORTBUS. I'll update PCIE_UNIPHIER in Kconfig. Thank you, --- Best Regards Kunihiko Hayashi
Hello Kunihiko! Now I found also this older email... On Friday 27 November 2020 21:02:05 Kunihiko Hayashi wrote: > Hi Bjorn Lorenzo, > > On 2020/11/25 19:23, Lorenzo Pieralisi wrote: > > On Tue, Nov 24, 2020 at 05:20:37PM -0600, Bjorn Helgaas wrote: > > > On Wed, Oct 28, 2020 at 10:31:43AM +0900, Kunihiko Hayashi wrote: > > > > This patch adds misc interrupt handler to detect and invoke PME/AER event. > > > > > > > > In UniPhier PCIe controller, PME/AER signals are assigned to the same > > > > signal as MSI by the internal logic. These signals should be detected by > > > > the internal register, however, DWC MSI handler can't handle these signals. > > > > > > I don't know what "PME/AER signals are assigned to the same signal as > > > MSI" means. > > > > The host controller embeds an interrupt-controller whose IRQ wire output > > is cascaded into the main interrupt controller. > > > > The host-bridge embedded controller receives MSI writes from devices > > and it turns them into an edge IRQ into the main interrupt controller. > > > > To ack/mask the MSIs at host contoller interrupt controller level, there > > is a control register in the host controller that needs handling upon > > IRQ reception. > > Thanks for explaining that. > In my understanding, PME/AER signals are cascaded to MSI by embedded > interrupt controller (not "assigned"). > > > > The *RP* (and AFAIU the RP *only*) signals the PME/AER MSI using the > > same wire to the main interrupt controller but its ack/mask is handled > > by a different bit in the host bridge control register above, therefore > > the cascaded IRQ isr needs to know which virq it is actually handling > > to ack/mask accordingly. > > Sorry what is RP? Root complex or something? RP = Root Port In lspci output you can find it as "root" of the tree topology and should have "PCI bridge" class/name. > > IMO this should be modelled with a separate IRQ domain and chip for > > the root port (yes this implies describing the root port in the dts > > file with a separate msi-parent). > > > > This series as it stands is a kludge. > > I see. However I need some time to consider the way to separate IRQ domain. > Is there any idea or example to handle PME/AER with IRQ domain? Seems that you are dealing with very similar issues as me with aardvark driver. As an inspiration look at my aardvark patch which setup separate IRQ domain for PME, AER and HP interrupts: https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/ Thanks to custom driver map_irq function, it is not needed to describe root port with separate msi-parent in DTS. > > > I'm trying to figure out if this is talking about PME/AER MSI vector > > > numbers (probably not) Bjorn, see my email, based on my experience with aardvark controller I think they are MSI vector numbers, but controller instead uses own proprietary way how to signal PME and AER interrupts. https://lore.kernel.org/linux-pci/20210718002614.3l74hlondwgthuby@pali/ > > > or some internal wire that's not > > > architecturally visible or what. > > > > > > Probably also not related to the fact that PME, hotplug, and bandwidth > > > notifications share the same MSI/MSI-X vector. > > > > > > Is this something that's going to be applicable to all the DWC-based > > > drivers? > > I think that this feature depends on the vendor specification. > At least, the registers to control or check these signals are implemented > in the vendor's logic. > > > > > > DWC MSI handler calls .msi_host_isr() callback function, that detects > > > > PME/AER signals with the internal register and invokes the interrupt > > > > with PME/AER vIRQ numbers. > > > > > > > > These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port() > > > > function. > > > > > > > > Cc: Marc Zyngier <maz@kernel.org> > > > > Cc: Jingoo Han <jingoohan1@gmail.com> > > > > Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > > --- > > > > drivers/pci/controller/dwc/pcie-uniphier.c | 77 +++++++++++++++++++++++++----- > > > > 1 file changed, 66 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c > > > > index 4817626..237537a 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-uniphier.c > > > > +++ b/drivers/pci/controller/dwc/pcie-uniphier.c > > > > @@ -21,6 +21,7 @@ > > > > #include <linux/reset.h> > > > > #include "pcie-designware.h" > > > > +#include "../../pcie/portdrv.h" > > > > #define PCL_PINCTRL0 0x002c > > > > #define PCL_PERST_PLDN_REGEN BIT(12) > > > > @@ -44,7 +45,9 @@ > > > > #define PCL_SYS_AUX_PWR_DET BIT(8) > > > > #define PCL_RCV_INT 0x8108 > > > > +#define PCL_RCV_INT_ALL_INT_MASK GENMASK(28, 25) > > > > #define PCL_RCV_INT_ALL_ENABLE GENMASK(20, 17) > > > > +#define PCL_RCV_INT_ALL_MSI_MASK GENMASK(12, 9) > > > > #define PCL_CFG_BW_MGT_STATUS BIT(4) > > > > #define PCL_CFG_LINK_AUTO_BW_STATUS BIT(3) > > > > #define PCL_CFG_AER_RC_ERR_MSI_STATUS BIT(2) > > > > @@ -68,6 +71,8 @@ struct uniphier_pcie_priv { > > > > struct reset_control *rst; > > > > struct phy *phy; > > > > struct irq_domain *legacy_irq_domain; > > > > + int aer_irq; > > > > + int pme_irq; > > > > }; > > > > #define to_uniphier_pcie(x) dev_get_drvdata((x)->dev) > > > > @@ -167,7 +172,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci) > > > > static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv) > > > > { > > > > - writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT); > > > > + u32 val; > > > > + > > > > + val = PCL_RCV_INT_ALL_ENABLE; > > > > + if (pci_msi_enabled()) > > > > + val |= PCL_RCV_INT_ALL_INT_MASK; > > > > + else > > > > + val |= PCL_RCV_INT_ALL_MSI_MASK; > > > > > > I'm confused about how this works. Root Ports can signal AER errors > > > with either INTx or MSI. This is controlled by the architected > > > Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at > > > PCIe r5.0, sec 6.2.4.1.2). > > > > > > The code here doesn't look related to those bits. Does this code mean > > > that if pci_msi_enabled(), the Root Port will always signal with MSI > > > (if MSI Enable is set) and will *never* signal with INTx? > > According to the spec sheet, we need to set interrupt enable bit for either > INTx or MSI, the other bit should be reset. These bits are in config space > and handled by the framework. Is spec sheet available publicly? > The controller signals AER errors with the interrupt that is either INTx > or MSI enabled. I think that the only way to know if MSI is enabled > (and INTX is disabled) is to use pci_msi_enabled(). > > > > > > + writel(val, priv->base + PCL_RCV_INT); > > > > writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX); > > > > } > > > > @@ -231,28 +244,52 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = { > > > > .map = uniphier_pcie_intx_map, > > > > }; > > > > -static void uniphier_pcie_irq_handler(struct irq_desc *desc) > > > > +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi) > > > > { > > > > - struct pcie_port *pp = irq_desc_get_handler_data(desc); > > > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > > > struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > > > > - struct irq_chip *chip = irq_desc_get_chip(desc); > > > > - unsigned long reg; > > > > - u32 val, bit, virq; > > > > + u32 val; > > > > - /* INT for debug */ > > > > val = readl(priv->base + PCL_RCV_INT); > > > > if (val & PCL_CFG_BW_MGT_STATUS) > > > > dev_dbg(pci->dev, "Link Bandwidth Management Event\n"); > > > > + > > > > > > Looks like a spurious whitespace change? > > Oops, I'll remove it. > > > > > > if (val & PCL_CFG_LINK_AUTO_BW_STATUS) > > > > dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n"); > > > > - if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) > > > > - dev_dbg(pci->dev, "Root Error\n"); > > > > - if (val & PCL_CFG_PME_MSI_STATUS) > > > > - dev_dbg(pci->dev, "PME Interrupt\n"); > > > > + > > > > + if (is_msi) { > > > > + if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) { > > > > + dev_dbg(pci->dev, "Root Error Status\n"); > > > > + if (priv->aer_irq) > > > > + generic_handle_irq(priv->aer_irq); > > > > + } > > > > + > > > > + if (val & PCL_CFG_PME_MSI_STATUS) { > > > > + dev_dbg(pci->dev, "PME Interrupt\n"); > > > > + if (priv->pme_irq) > > > > + generic_handle_irq(priv->pme_irq); > > > > + } > > > > + } > > > > writel(val, priv->base + PCL_RCV_INT); > > > > +} > > > > + > > > > +static void uniphier_pcie_msi_host_isr(struct pcie_port *pp) > > > > +{ > > > > + uniphier_pcie_misc_isr(pp, true); > > > > +} > > > > + > > > > +static void uniphier_pcie_irq_handler(struct irq_desc *desc) > > > > +{ > > > > + struct pcie_port *pp = irq_desc_get_handler_data(desc); > > > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > > > + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); > > > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > > > + unsigned long reg; > > > > + u32 val, bit, virq; > > > > + > > > > + uniphier_pcie_misc_isr(pp, false); > > > > /* INTx */ > > > > chained_irq_enter(chip, desc); > > > > @@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp) > > > > static const struct dw_pcie_host_ops uniphier_pcie_host_ops = { > > > > .host_init = uniphier_pcie_host_init, > > > > + .msi_host_isr = uniphier_pcie_msi_host_isr, > > > > }; > > > > static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > > > > @@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > > > > struct dw_pcie *pci = &priv->pci; > > > > struct pcie_port *pp = &pci->pp; > > > > struct device *dev = &pdev->dev; > > > > + struct pci_dev *pcidev; > > > > int ret; > > > > pp->ops = &uniphier_pcie_host_ops; > > > > @@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > > > > return ret; > > > > } > > > > + /* irq for PME */ > > > > + list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) { > > > > + priv->pme_irq = > > > > + pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME); > > > > + if (priv->pme_irq) > > > > + break; > > > > > > Does this mean that all Root Ports must use the same MSI vector? I > > > don't think that's a PCIe spec requirement, though of course DWC may > > > have its own restrictions. > > > > > This controller has one port implementation only, > so this assumes that there is one root port. > > > > > I don't think this depends on CONFIG_PCIEPORTBUS, so it looks like > > > it's possible to have > > > > > > # CONFIG_PCIEPORTBUS is not set > > > PCIE_UNIPHIER=y > > > > > > in which case I think you'll have a link error. > > Indeed. To use port functions needs to define PCIEPORTBUS. > I'll update PCIE_UNIPHIER in Kconfig. > > > Thank you, > > --- > Best Regards > Kunihiko Hayashi
Hi Pali. On 2021/07/18 9:51, Pali Rohar wrote: > Hello Kunihiko! Now I found also this older email... > > On Friday 27 November 2020 21:02:05 Kunihiko Hayashi wrote: > > Hi Bjorn Lorenzo, > > > > On 2020/11/25 19:23, Lorenzo Pieralisi wrote: > > > On Tue, Nov 24, 2020 at 05:20:37PM -0600, Bjorn Helgaas wrote: > > > > On Wed, Oct 28, 2020 at 10:31:43AM +0900, Kunihiko Hayashi wrote: > > > > > This patch adds misc interrupt handler to detect and invoke PME/AER event. > > > > > > > > > > In UniPhier PCIe controller, PME/AER signals are assigned to the same > > > > > signal as MSI by the internal logic. These signals should be detected by > > > > > the internal register, however, DWC MSI handler can't handle these signals. > > > > > > > > I don't know what "PME/AER signals are assigned to the same signal as > > > > MSI" means. > > > > > > The host controller embeds an interrupt-controller whose IRQ wire output > > > is cascaded into the main interrupt controller. > > > > > > The host-bridge embedded controller receives MSI writes from devices > > > and it turns them into an edge IRQ into the main interrupt controller. > > > > > > To ack/mask the MSIs at host contoller interrupt controller level, there > > > is a control register in the host controller that needs handling upon > > > IRQ reception. > > > > Thanks for explaining that. > > In my understanding, PME/AER signals are cascaded to MSI by embedded > > interrupt controller (not "assigned"). > > > > > > > The *RP* (and AFAIU the RP *only*) signals the PME/AER MSI using the > > > same wire to the main interrupt controller but its ack/mask is handled > > > by a different bit in the host bridge control register above, therefore > > > the cascaded IRQ isr needs to know which virq it is actually handling > > > to ack/mask accordingly. > > > > Sorry what is RP? Root complex or something? > > RP = Root Port > > In lspci output you can find it as "root" of the tree topology and > should have "PCI bridge" class/name. Ok, I understand. > > > > IMO this should be modelled with a separate IRQ domain and chip for > > > the root port (yes this implies describing the root port in the dts > > > file with a separate msi-parent). > > > > > > This series as it stands is a kludge. > > > > I see. However I need some time to consider the way to separate IRQ domain. > > Is there any idea or example to handle PME/AER with IRQ domain? > > Seems that you are dealing with very similar issues as me with aardvark > driver. > > As an inspiration look at my aardvark patch which setup separate IRQ > domain for PME, AER and HP interrupts: > https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/ > > Thanks to custom driver map_irq function, it is not needed to describe > root port with separate msi-parent in DTS. I need to understand your solution, though, this might be the same situation as my driver. > > > > I'm trying to figure out if this is talking about PME/AER MSI vector > > > > numbers (probably not) > > Bjorn, see my email, based on my experience with aardvark controller I > think they are MSI vector numbers, but controller instead uses own > proprietary way how to signal PME and AER interrupts. > https://lore.kernel.org/linux-pci/20210718002614.3l74hlondwgthuby@pali/ > > > > > or some internal wire that's not > > > > architecturally visible or what. > > > > > > > > Probably also not related to the fact that PME, hotplug, and bandwidth > > > > notifications share the same MSI/MSI-X vector. > > > > > > > > Is this something that's going to be applicable to all the DWC-based > > > > drivers? > > > > I think that this feature depends on the vendor specification. > > At least, the registers to control or check these signals are implemented > > in the vendor's logic. > > > > > > > > > DWC MSI handler calls .msi_host_isr() callback function, that detects > > > > > PME/AER signals with the internal register and invokes the interrupt > > > > > with PME/AER vIRQ numbers. > > > > > > > > > > These vIRQ numbers is obtained from portdrv in uniphier_add_pcie_port() > > > > > function. > > > > > > > > > > Cc: Marc Zyngier <maz@kernel.org> > > > > > Cc: Jingoo Han <jingoohan1@gmail.com> > > > > > Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > > > --- > > > > > drivers/pci/controller/dwc/pcie-uniphier.c | 77 +++++++++++++++++++++++++----- > > > > > 1 file changed, 66 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c > > > > > index 4817626..237537a 100644 > > > > > --- a/drivers/pci/controller/dwc/pcie-uniphier.c > > > > > +++ b/drivers/pci/controller/dwc/pcie-uniphier.c > > > > > @@ -21,6 +21,7 @@ > > > > > #include <linux/reset.h> > > > > > #include "pcie-designware.h" > > > > > +#include "../../pcie/portdrv.h" > > > > > #define PCL_PINCTRL0 0x002c > > > > > #define PCL_PERST_PLDN_REGEN BIT(12) > > > > > @@ -44,7 +45,9 @@ > > > > > #define PCL_SYS_AUX_PWR_DET BIT(8) > > > > > #define PCL_RCV_INT 0x8108 > > > > > +#define PCL_RCV_INT_ALL_INT_MASK GENMASK(28, 25) > > > > > #define PCL_RCV_INT_ALL_ENABLE GENMASK(20, 17) > > > > > +#define PCL_RCV_INT_ALL_MSI_MASK GENMASK(12, 9) > > > > > #define PCL_CFG_BW_MGT_STATUS BIT(4) > > > > > #define PCL_CFG_LINK_AUTO_BW_STATUS BIT(3) > > > > > #define PCL_CFG_AER_RC_ERR_MSI_STATUS BIT(2) > > > > > @@ -68,6 +71,8 @@ struct uniphier_pcie_priv { > > > > > struct reset_control *rst; > > > > > struct phy *phy; > > > > > struct irq_domain *legacy_irq_domain; > > > > > + int aer_irq; > > > > > + int pme_irq; > > > > > }; > > > > > #define to_uniphier_pcie(x) dev_get_drvdata((x)->dev) > > > > > @@ -167,7 +172,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci) > > > > > static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv) > > > > > { > > > > > - writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT); > > > > > + u32 val; > > > > > + > > > > > + val = PCL_RCV_INT_ALL_ENABLE; > > > > > + if (pci_msi_enabled()) > > > > > + val |= PCL_RCV_INT_ALL_INT_MASK; > > > > > + else > > > > > + val |= PCL_RCV_INT_ALL_MSI_MASK; > > > > > > > > I'm confused about how this works. Root Ports can signal AER errors > > > > with either INTx or MSI. This is controlled by the architected > > > > Interrupt Disable bit and the MSI/MSI-X enable bits (I'm looking at > > > > PCIe r5.0, sec 6.2.4.1.2). > > > > > > > > The code here doesn't look related to those bits. Does this code mean > > > > that if pci_msi_enabled(), the Root Port will always signal with MSI > > > > (if MSI Enable is set) and will *never* signal with INTx? > > > > According to the spec sheet, we need to set interrupt enable bit for either > > INTx or MSI, the other bit should be reset. These bits are in config space > > and handled by the framework. > > Is spec sheet available publicly? Sorry the spec sheet isn't open to the public. Currently initial configuration is to use MSI, and I assume MSI interrupt is used as default. Thank you, --- Best Regards Kunihiko Hayashi
On Friday 23 July 2021 01:54:10 Kunihiko Hayashi wrote: > On 2021/07/18 9:51, Pali Rohar wrote: > > > > IMO this should be modelled with a separate IRQ domain and chip for > > > > the root port (yes this implies describing the root port in the dts > > > > file with a separate msi-parent). > > > > > > > > This series as it stands is a kludge. > > > > > > I see. However I need some time to consider the way to separate IRQ domain. > > > Is there any idea or example to handle PME/AER with IRQ domain? > > > > Seems that you are dealing with very similar issues as me with aardvark > > driver. > > > > As an inspiration look at my aardvark patch which setup separate IRQ > > domain for PME, AER and HP interrupts: > > https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/ > > > > Thanks to custom driver map_irq function, it is not needed to describe > > root port with separate msi-parent in DTS. > > I need to understand your solution, though, this might be the same situation as my driver. I think it is very very similar as aardvark also returns zero as hw irq number (and it is not possible to change it). So simple solution for you is also to register separate IRQ domain for Root Port Bridge and then re-trigger interrupt with number 0 (which you wrote that is default) as: virq = irq_find_mapping(priv->irq_domain, 0); generic_handle_irq(virq); in your uniphier_pcie_misc_isr() function. There is no need to modify DTS. And also no need to use complicated logic for finding registered virq number via pcie_port_service_get_irq() and uniphier_pcie_port_get_irq() functions.
Hi Pali, On 2021/07/23 2:26, Pali Rohár wrote: > On Friday 23 July 2021 01:54:10 Kunihiko Hayashi wrote: >> On 2021/07/18 9:51, Pali Rohar wrote: >>>>> IMO this should be modelled with a separate IRQ domain and chip for >>>>> the root port (yes this implies describing the root port in the dts >>>>> file with a separate msi-parent). >>>>> >>>>> This series as it stands is a kludge. >>>> >>>> I see. However I need some time to consider the way to separate IRQ domain. >>>> Is there any idea or example to handle PME/AER with IRQ domain? >>> >>> Seems that you are dealing with very similar issues as me with aardvark >>> driver. >>> >>> As an inspiration look at my aardvark patch which setup separate IRQ >>> domain for PME, AER and HP interrupts: >>> https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/ >>> >>> Thanks to custom driver map_irq function, it is not needed to describe >>> root port with separate msi-parent in DTS. >> >> I need to understand your solution, though, this might be the same situation as my driver. > > I think it is very very similar as aardvark also returns zero as hw irq > number (and it is not possible to change it). > > So simple solution for you is also to register separate IRQ domain for > Root Port Bridge and then re-trigger interrupt with number 0 (which you > wrote that is default) as: > > virq = irq_find_mapping(priv->irq_domain, 0); > generic_handle_irq(virq); > > in your uniphier_pcie_misc_isr() function. I'm not sure "register separate IRQ domain for Root Port Bridge". Do you mean that your suggestion is to create new IRQ domain, and add this domain to root port? Or could you show me something example? The re-trigger part is the same method as v5 patch I wrote. > There is no need to modify DTS. And also no need to use complicated > logic for finding registered virq number via pcie_port_service_get_irq() > and uniphier_pcie_port_get_irq() functions. I see. GIC interrupt for MSI is handled by the MSI domain by pcie-designware-host.c. My concern is how to trigger PME/AER event with another IRQ domain. Thank you, --- Best Regards Kunihiko Hayashi
On Friday 23 July 2021 15:59:12 Kunihiko Hayashi wrote: > Hi Pali, > > On 2021/07/23 2:26, Pali Rohár wrote: > > On Friday 23 July 2021 01:54:10 Kunihiko Hayashi wrote: > > > On 2021/07/18 9:51, Pali Rohar wrote: > > > > > > IMO this should be modelled with a separate IRQ domain and chip for > > > > > > the root port (yes this implies describing the root port in the dts > > > > > > file with a separate msi-parent). > > > > > > > > > > > > This series as it stands is a kludge. > > > > > > > > > > I see. However I need some time to consider the way to separate IRQ domain. > > > > > Is there any idea or example to handle PME/AER with IRQ domain? > > > > > > > > Seems that you are dealing with very similar issues as me with aardvark > > > > driver. > > > > > > > > As an inspiration look at my aardvark patch which setup separate IRQ > > > > domain for PME, AER and HP interrupts: > > > > https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/ > > > > > > > > Thanks to custom driver map_irq function, it is not needed to describe > > > > root port with separate msi-parent in DTS. > > > > > > I need to understand your solution, though, this might be the same situation as my driver. > > > > I think it is very very similar as aardvark also returns zero as hw irq > > number (and it is not possible to change it). > > > > So simple solution for you is also to register separate IRQ domain for > > Root Port Bridge and then re-trigger interrupt with number 0 (which you > > wrote that is default) as: > > > > virq = irq_find_mapping(priv->irq_domain, 0); > > generic_handle_irq(virq); > > > > in your uniphier_pcie_misc_isr() function. > > I'm not sure "register separate IRQ domain for Root Port Bridge". > Do you mean that your suggestion is to create new IRQ domain, and add this domain to root port? Yes. > Or could you show me something example? I have already sent link to patch above which it implements for pci-aardvark.c driver. https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/ In device prove callback register domain by irq_domain_add_linear(). In bridge map_irq() callback use irq_create_mapping() for Root Port device (and otherwise default of_irq_parse_and_map_pci()). And in uniphier_pcie_misc_isr() retrigger interrupt into new domain. > The re-trigger part is the same method as v5 patch I wrote. Just you need to specify that new/private IRQ domain into irq_find_mapping() call. > > There is no need to modify DTS. And also no need to use complicated > > logic for finding registered virq number via pcie_port_service_get_irq() > > and uniphier_pcie_port_get_irq() functions. > > I see. > GIC interrupt for MSI is handled by the MSI domain by pcie-designware-host.c. > My concern is how to trigger PME/AER event with another IRQ domain. > > Thank you, > > --- > Best Regards > Kunihiko Hayashi
Hi Pali, On 2021/07/23 17:37, Pali Rohár wrote: > On Friday 23 July 2021 15:59:12 Kunihiko Hayashi wrote: >> Hi Pali, >> >> On 2021/07/23 2:26, Pali Rohár wrote: >>> On Friday 23 July 2021 01:54:10 Kunihiko Hayashi wrote: >>>> On 2021/07/18 9:51, Pali Rohar wrote: >>>>>>> IMO this should be modelled with a separate IRQ domain and chip for >>>>>>> the root port (yes this implies describing the root port in the dts >>>>>>> file with a separate msi-parent). >>>>>>> >>>>>>> This series as it stands is a kludge. >>>>>> >>>>>> I see. However I need some time to consider the way to separate IRQ domain. >>>>>> Is there any idea or example to handle PME/AER with IRQ domain? >>>>> >>>>> Seems that you are dealing with very similar issues as me with aardvark >>>>> driver. >>>>> >>>>> As an inspiration look at my aardvark patch which setup separate IRQ >>>>> domain for PME, AER and HP interrupts: >>>>> https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/ >>>>> >>>>> Thanks to custom driver map_irq function, it is not needed to describe >>>>> root port with separate msi-parent in DTS. >>>> >>>> I need to understand your solution, though, this might be the same situation as my driver. >>> >>> I think it is very very similar as aardvark also returns zero as hw irq >>> number (and it is not possible to change it). >>> >>> So simple solution for you is also to register separate IRQ domain for >>> Root Port Bridge and then re-trigger interrupt with number 0 (which you >>> wrote that is default) as: >>> >>> virq = irq_find_mapping(priv->irq_domain, 0); >>> generic_handle_irq(virq); >>> >>> in your uniphier_pcie_misc_isr() function. >> >> I'm not sure "register separate IRQ domain for Root Port Bridge". >> Do you mean that your suggestion is to create new IRQ domain, and add this domain to root port? > > Yes. > >> Or could you show me something example? > > I have already sent link to patch above which it implements for > pci-aardvark.c driver. > > https://lore.kernel.org/linux-pci/20210506153153.30454-32-pali@kernel.org/ Thank you for the example. > In device prove callback register domain by irq_domain_add_linear(). > In bridge map_irq() callback use irq_create_mapping() for Root Port > device (and otherwise default of_irq_parse_and_map_pci()). And in > uniphier_pcie_misc_isr() retrigger interrupt into new domain. I understand it late. The main point is to replace bridge->map_irq() with private own map_irq(). > >> The re-trigger part is the same method as v5 patch I wrote. > > Just you need to specify that new/private IRQ domain into > irq_find_mapping() call. I'll try to replace the events with new IRQ domain. Thank you, --- Best Regards Kunihiko Hayashi
Hi Lorenzo, Pali, On 2021/07/23 18:36, Kunihiko Hayashi wrote: > Hi Pali, [snip] >> Just you need to specify that new/private IRQ domain into >> irq_find_mapping() call. > > I'll try to replace the events with new IRQ domain. According to Pali's suggestion, the bridge handles INTX and it isn't difficult to change IRQ's map for Root Port like the example. It seems that it can't be applied to MSI. On the other hand, according to Lorenzo's suggestion, >>>>>>> IMO this should be modelled with a separate IRQ domain and chip for >>>>>>> the root port (yes this implies describing the root port in the dts >>>>>>> file with a separate msi-parent). Interrupts for PME/AER event is assigned to number 0 of MSI IRQ domain. (pcie_port_enable_irq_vec() in portdrv_core.c) This expects MSI status bit 0 to be set when the event occurs. However, in the uniphier PCIe controller, MSI status bit 0 is not set, but the PME/AER status bit in the glue logic is set. I think that it's hard to associate the new domain and "MSI-IRQ 0" event if the new IRQ domain and chip is modelled. So, I have no idea to handle both new IRQ domain and cascaded MSI event. Is there any example for that? Thank you, --- Best Regards Kunihiko Hayashi
On Wednesday 28 July 2021 14:29:15 Kunihiko Hayashi wrote: > Hi Lorenzo, Pali, > > On 2021/07/23 18:36, Kunihiko Hayashi wrote: > > Hi Pali, > > [snip] > > > > Just you need to specify that new/private IRQ domain into > > > irq_find_mapping() call. > > > > I'll try to replace the events with new IRQ domain. > According to Pali's suggestion, the bridge handles INTX and it isn't difficult > to change IRQ's map for Root Port like the example. > It seems that it can't be applied to MSI. Hm... And it is hard to change mapping also for MSI via custom/new callback? > On the other hand, according to Lorenzo's suggestion, > > >>>>>>> IMO this should be modelled with a separate IRQ domain and chip for > >>>>>>> the root port (yes this implies describing the root port in the dts > >>>>>>> file with a separate msi-parent). > > Interrupts for PME/AER event is assigned to number 0 of MSI IRQ domain. Yes. This is because Root Port of your PCIe controller provides this information to OS. > (pcie_port_enable_irq_vec() in portdrv_core.c) > This expects MSI status bit 0 to be set when the event occurs. Obviously (according to PCIe spec). > However, in the uniphier PCIe controller, MSI status bit 0 is not set, but > the PME/AER status bit in the glue logic is set. And this "violates" PCIe spec and your controller needs custom handling in driver to work... > I think that it's hard to associate the new domain and "MSI-IRQ 0" event > if the new IRQ domain and chip is modelled. No. It was mean to assign all MSI IRQs (not only MSI number 0) which comes from Root Port to new domain. Assigning just one MSI number to separate domain is I guess impossible (and also does not make sense). This is required to difference between MSI number 0 which comes from real MSI path and "fake MSI number 0" which is just specific for Root Port and does not share anything with real MSI interrupts. As MSI interrupts are not shared it is required to prevent "mixing" interrupt sources. And kernel can do it via different MSI domains. So in the end you would have two different MSI numbers 0. > So, I have no idea to handle both new IRQ domain and cascaded MSI event. It was mean to define Root Port PCIe device in DTS. Then define a new IRQ chip / domain in DTS. And specify in DTS that this Root Port PCIe device should use this new IRQ chip / domain instead of default one. And then you need to implement "driver" for this "virtual" IRQ chip / domain to handle specific glue logic in this controller driver. I was hoping that it is possible to set this mapping directly in controller driver. But if you checked that only legacy IRQs can be done in this way, and not MSI then it is either needed to go via this DTS path OR try to figure out how to define this mapping in PCI subsystem (maybe needs some changes?) also for MSI. > Is there any example for that? I do not know. I think you are the first one who have such buggy PCIe controller which needs this specific kind of configuration and domains. In my case in pci-aardvark.c, emulated kernel Root Port does not support MSI interrupts (yet) so these "fake" interrupts are routed as legacy INTA. And because legacy INTx are shared interrupts they can be mixed together with real (as it is done prior my patch). My patch (link sent in previous email) just separates "fake" INTA from "real" INTA via separate domains for performance reasons. But you use MSI interrupts, which means that it is required to have logic to separate them into different domains to prevent mixing. > Thank you, > > --- > Best Regards > Kunihiko Hayashi
diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c index 4817626..237537a 100644 --- a/drivers/pci/controller/dwc/pcie-uniphier.c +++ b/drivers/pci/controller/dwc/pcie-uniphier.c @@ -21,6 +21,7 @@ #include <linux/reset.h> #include "pcie-designware.h" +#include "../../pcie/portdrv.h" #define PCL_PINCTRL0 0x002c #define PCL_PERST_PLDN_REGEN BIT(12) @@ -44,7 +45,9 @@ #define PCL_SYS_AUX_PWR_DET BIT(8) #define PCL_RCV_INT 0x8108 +#define PCL_RCV_INT_ALL_INT_MASK GENMASK(28, 25) #define PCL_RCV_INT_ALL_ENABLE GENMASK(20, 17) +#define PCL_RCV_INT_ALL_MSI_MASK GENMASK(12, 9) #define PCL_CFG_BW_MGT_STATUS BIT(4) #define PCL_CFG_LINK_AUTO_BW_STATUS BIT(3) #define PCL_CFG_AER_RC_ERR_MSI_STATUS BIT(2) @@ -68,6 +71,8 @@ struct uniphier_pcie_priv { struct reset_control *rst; struct phy *phy; struct irq_domain *legacy_irq_domain; + int aer_irq; + int pme_irq; }; #define to_uniphier_pcie(x) dev_get_drvdata((x)->dev) @@ -167,7 +172,15 @@ static void uniphier_pcie_stop_link(struct dw_pcie *pci) static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv) { - writel(PCL_RCV_INT_ALL_ENABLE, priv->base + PCL_RCV_INT); + u32 val; + + val = PCL_RCV_INT_ALL_ENABLE; + if (pci_msi_enabled()) + val |= PCL_RCV_INT_ALL_INT_MASK; + else + val |= PCL_RCV_INT_ALL_MSI_MASK; + + writel(val, priv->base + PCL_RCV_INT); writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX); } @@ -231,28 +244,52 @@ static const struct irq_domain_ops uniphier_intx_domain_ops = { .map = uniphier_pcie_intx_map, }; -static void uniphier_pcie_irq_handler(struct irq_desc *desc) +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi) { - struct pcie_port *pp = irq_desc_get_handler_data(desc); struct dw_pcie *pci = to_dw_pcie_from_pp(pp); struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); - struct irq_chip *chip = irq_desc_get_chip(desc); - unsigned long reg; - u32 val, bit, virq; + u32 val; - /* INT for debug */ val = readl(priv->base + PCL_RCV_INT); if (val & PCL_CFG_BW_MGT_STATUS) dev_dbg(pci->dev, "Link Bandwidth Management Event\n"); + if (val & PCL_CFG_LINK_AUTO_BW_STATUS) dev_dbg(pci->dev, "Link Autonomous Bandwidth Event\n"); - if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) - dev_dbg(pci->dev, "Root Error\n"); - if (val & PCL_CFG_PME_MSI_STATUS) - dev_dbg(pci->dev, "PME Interrupt\n"); + + if (is_msi) { + if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) { + dev_dbg(pci->dev, "Root Error Status\n"); + if (priv->aer_irq) + generic_handle_irq(priv->aer_irq); + } + + if (val & PCL_CFG_PME_MSI_STATUS) { + dev_dbg(pci->dev, "PME Interrupt\n"); + if (priv->pme_irq) + generic_handle_irq(priv->pme_irq); + } + } writel(val, priv->base + PCL_RCV_INT); +} + +static void uniphier_pcie_msi_host_isr(struct pcie_port *pp) +{ + uniphier_pcie_misc_isr(pp, true); +} + +static void uniphier_pcie_irq_handler(struct irq_desc *desc) +{ + struct pcie_port *pp = irq_desc_get_handler_data(desc); + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); + struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci); + struct irq_chip *chip = irq_desc_get_chip(desc); + unsigned long reg; + u32 val, bit, virq; + + uniphier_pcie_misc_isr(pp, false); /* INTx */ chained_irq_enter(chip, desc); @@ -329,6 +366,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp) static const struct dw_pcie_host_ops uniphier_pcie_host_ops = { .host_init = uniphier_pcie_host_init, + .msi_host_isr = uniphier_pcie_msi_host_isr, }; static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, @@ -337,6 +375,7 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, struct dw_pcie *pci = &priv->pci; struct pcie_port *pp = &pci->pp; struct device *dev = &pdev->dev; + struct pci_dev *pcidev; int ret; pp->ops = &uniphier_pcie_host_ops; @@ -353,6 +392,22 @@ static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, return ret; } + /* irq for PME */ + list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) { + priv->pme_irq = + pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_PME); + if (priv->pme_irq) + break; + } + + /* irq for AER */ + list_for_each_entry(pcidev, &pp->bridge->bus->devices, bus_list) { + priv->aer_irq = + pcie_port_service_get_irq(pcidev, PCIE_PORT_SERVICE_AER); + if (priv->aer_irq) + break; + } + return 0; }