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

[Kunihiko Hayashi]

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