Messages in this thread | | | Date | Thu, 04 Jun 2020 11:11:34 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER |
| |
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...
| |