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

[Kunihiko Hayashi]

Hi Marc,

On 2020/06/03 20:22, Marc Zyngier wrote:
> On 2020-06-03 09:54, 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.
> >
> > This adds the function uniphier_pcie_misc_isr() that handles misc
> > intterupts, which is called from both INTx and MSI handlers.
>
> interrupts

Okay, I'll fix it.

> > This function detects PME and AER interrupts with the status register,
> > and invoke PME and AER drivers related to INTx or 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 | 53 +++++++++++++++++++++++-------
> > Â1 file changed, 42 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c
> > b/drivers/pci/controller/dwc/pcie-uniphier.c
> > index a5401a0..a8dda39 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,28 +241,48 @@ 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)
> > Â{
> > -ÂÂÂ 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.

> > +ÂÂÂ }
> >
> > ÂÂÂÂ writel(val, priv->base + PCL_RCV_INT);
> > +}
> > +
> > +static void uniphier_pcie_irq_handler(struct irq_desc *desc)
> > +{
> > +ÂÂÂ 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;
> > +
> > +ÂÂÂ /* misc interrupt */
> > +ÂÂÂ uniphier_pcie_misc_isr(pp);
>
> This is a chained handler called outside of a chained_irq_enter/exit
> block. It isn't acceptable.

I got it.
This call should be called in the block.

Thank you,

---
Best Regards
Kunihiko Hayashi