Re: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
From: Kunihiko Hayashi
Date: Wed Jul 15 2020 - 06:04:22 EST
Hi Lorenzo,
On 2020/07/14 22:27, Lorenzo Pieralisi wrote:
On Thu, Jun 18, 2020 at 05:38:09PM +0900, Kunihiko Hayashi 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.
Define what you mean please.
AER/PME signals are assigned to the same signal as MSI by internal logic,
that is, AER/PME and MSI are assigned to the same GIC interrupt number.
So it's necessary to modify the code to call the misc handler from MSI handler.
I'll rewrite it next.
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;
+
+ 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);
I think this is wrong. pp->irq_domain is the DWC MSI domain, how do
you know that hwirq 0 *is* the AER/PME interrupt ?
When AER/PME drivers are probed, AER/PME interrupts are registered
as MSI-0.
The pcie_message_numbers() function refers the following fields of
PCI registers,
- PCI_EXP_FLAGS_IRQ (for PME)
- PCI_ERR_ROOT_AER_IRQ (for AER)
and decides AER/PME interrupts numbers in MSI domain.
Initial values of both fields are 0, so these interrupts are set to MSI-0.
However, pcie_uniphier driver doesn't know that these interrupts are MSI-0.
Surely using 0 here is wrong.
I think that the method to get virq for AER/PME from pcieport is needed.
It just *works* in this case because the port driver probes and alloc
MSIs before any PCI device has a chance to do it and actually I think
this is just wrong also because hwirq 0 *is* usable by devices but
it can't be used because current code takes it for the PME/AER interrupt
(which AFAICS is an internal signal disconnected from the DWC MSI
interrupt controller).
AER/PME interrupts are with IRQF_SHARED, so hwirq 0 can share
any PCI device, however, the multiple handlers might be called
with other factor, so I don't think it is desiable.
I think this extra glue logic should be separate MSI domain
otherwise there is no way you can reliably look-up the virq
corresponding to AER/PME.
Ok, however, it seems that there is no way to get virq for AER/PME
from pcieport in pcie/portdrv_core.c.
When I try to separate AER/PME interrtups from MSI domain,
how should I get virq for AER/PME?
How does it work in HW ? Is the root port really sending a memory
write to raise an IRQ or it just signal the IRQ through internal
logic ? I think the root port MSI handling is different from the
DWC logic and should be treated separately.
The internal logic assigns the same signal as MSI interrupt
to AER/PME interrupts.
The MSI handler checks internal status register PCL_RCV_INT,
and know if the signal is AER or PME interrupt. MSI memory write
isn't used for the signal.
Since DWC MSI handler doesn't have a method to check the internal
status register, I added callback .msi_host_isr() to DWC MSI handler
in patch 1/6.
Thank you,
---
Best Regards
Kunihiko Hayashi