Re: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
From: Kunihiko Hayashi
Date: Tue Jun 30 2020 - 22:18:34 EST
Hi Marc,
On 2020/06/30 22:23, Marc Zyngier wrote:
On 2020-06-29 10:49, Kunihiko Hayashi wrote:
Hi Marc,
On 2020/06/27 18:48, Marc Zyngier wrote:
On Thu, 18 Jun 2020 09:38:09 +0100,
Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx> wrote:
The misc interrupts consisting of PME, AER, and Link event, is handled
by INTx handler, however, these interrupts should be also handled by
MSI handler.
This adds the function uniphier_pcie_misc_isr() that handles misc
interrupts, which is called from both INTx and MSI handlers.
This function detects PME and AER interrupts with the status register,
and invoke PME and AER drivers related to MSI.
And this sets the mask for misc interrupts from INTx if MSI is enabled
and sets the mask for misc interrupts from MSI if MSI is disabled.
Cc: Marc Zyngier <maz@xxxxxxxxxx>
Cc: Jingoo Han <jingoohan1@xxxxxxxxx>
Cc: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 57 ++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index a5401a0..5ce2479 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -44,7 +44,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)
@@ -167,7 +169,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;
Does this affect endpoints? Or just the RC itself?
These interrupts are asserted by RC itself, so this part affects only RC.
+
+ÂÂÂ writel(val, priv->base + PCL_RCV_INT);
ÂÂÂÂÂ writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
 }
 @@ -231,32 +241,56 @@ 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, virq;
 - /* 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 (val & PCL_CFG_PME_MSI_STATUS)
+ÂÂÂÂÂÂÂÂÂÂÂ dev_dbg(pci->dev, "PME Interrupt\n");
+
+ÂÂÂÂÂÂÂ if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ PCL_CFG_PME_MSI_STATUS)) {
+ÂÂÂÂÂÂÂÂÂÂÂ virq = irq_linear_revmap(pp->irq_domain, 0);
+ÂÂÂÂÂÂÂÂÂÂÂ generic_handle_irq(virq);
+ÂÂÂÂÂÂÂ }
+ÂÂÂ }
Please have two handlers: one for interrupts that are from the RC,
another for interrupts coming from the endpoints.
I assume that this handler treats interrupts from the RC only and
this is set on the member ".msi_host_isr" added in the patch 1/6.
I think that the handler for interrupts coming from endpoints should be
treated as a normal case (after calling .msi_host_isr in
dw_handle_msi_irq()).
It looks pretty odd that you end-up dealing with both from the
same "parent" interrupt. I guess this is in keeping with the
rest of the DW PCIe hacks... :-/
It might be odd, however, in case of UniPhier SoC,
both MSI interrupts from endpoints and PME/AER interrupts from RC are
asserted by same "parent" interrupt. In other words, PME/AER interrupts
are notified using the parent interrupt for MSI.
MSI interrupts are treated as child interrupts with reference to
the status register in DW core. This is handled in a for-loop in
dw_handle_msi_irq().
PME/AER interrupts are treated with reference to the status register
in UniPhier glue layer, however, this couldn't be handled in the same way
directly.
So I'm trying to add .msi_host_isr function to handle this
with reference to the SoC-specific registers.
This exported function asserts MSI-0 as a shared child interrupt.
As a result, PME/AER are registered like the followings in dmesg:
pcieport 0000:00:00.0: PME: Signaling with IRQ 25
pcieport 0000:00:00.0: AER: enabled with IRQ 25
And these interrupts are shared as MSI-0:
# cat /proc/interrupts | grep 25:
25: 0 0 0 0 PCI-MSI 0 Edge PCIe PME, aerdrv
This might be a special case, though, I think that this is needed to handle
interrupts from RC sharing MSI parent.
It is for Lorenzo to make up his mind about this anyway.
I'd like to Lorenzo's opinion, too.
Thank you,
---
Best Regards
Kunihiko Hayashi