Messages in this thread | | | Subject | Re: [PATCH v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER | From | Kunihiko Hayashi <> | Date | Fri, 5 Jun 2020 11:36:08 +0900 |
| |
Hi Marc,
On 2020/06/04 19:11, Marc Zyngier wrote: > 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 > } > > ?
No problem. I'll rewrite it in the same way as yours in handling interrupts.
> If you have two handlers for the same interrupt, this is a shared > interrupt and each handler will be called in turn. Yes, MSI-0 is shared with PME and AER, and it will be like that.
Thank you,
--- Best Regards Kunihiko Hayashi
| |