Re: [PATCH v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2020-06-04 10:43, Kunihiko Hayashi wrote:

[...]

-static void uniphier_pcie_irq_handler(struct irq_desc *desc)
+static void uniphier_pcie_misc_isr(struct pcie_port *pp)
 {
-    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 (pci_msi_enabled()) {

This checks whether the kernel supports MSIs. Not that they are
enabled in your controller. Is that really what you want to do?

The below two status bits are valid when the interrupt for MSI is asserted.
That is, pci_msi_enabled() is wrong.

I'll modify the function to check the two bits only if this function is
called from MSI handler.


+        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS) {
+            dev_dbg(pci->dev, "Root Error Status\n");
+            virq = irq_linear_revmap(pp->irq_domain, 0);
+            generic_handle_irq(virq);
+        }
+
+        if (val & PCL_CFG_PME_MSI_STATUS) {
+            dev_dbg(pci->dev, "PME Interrupt\n");
+            virq = irq_linear_revmap(pp->irq_domain, 0);
+            generic_handle_irq(virq);
+        }

These two cases do the exact same thing, calling the same interrupt.
What is the point of dealing with them independently?

Both PME and AER are asserted from MSI-0, and each handler checks its own status bit in the PCIe register (aer_irq() in pcie/aer.c and pcie_pme_irq()
in pcie/pme.c).
So I think this handler calls generic_handle_irq() for the same MSI-0.

So what is wrong with

        if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
                   PCL_CFG_PME_MSI_STATUS)) {
                // handle interrupt
        }

?

If you have two handlers for the same interrupt, this is a shared
interrupt and each handler will be called in turn.

        M.
--
Jazz is not dead. It just smells funny...



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux