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

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Wed, Jul 01, 2020 at 11:18:29AM +0900, Kunihiko Hayashi wrote:

[...]

> > > > >   -static void uniphier_pcie_irq_handler(struct irq_desc *desc)
> > > > > +static void uniphier_pcie_misc_isr(struct pcie_port *pp, bool is_msi)
> > > > >   {
> > > > > -    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 (is_msi) {
> > > > > +        if (val & PCL_CFG_AER_RC_ERR_MSI_STATUS)
> > > > > +            dev_dbg(pci->dev, "Root Error Status\n");
> > > > > +
> > > > > +        if (val & PCL_CFG_PME_MSI_STATUS)
> > > > > +            dev_dbg(pci->dev, "PME Interrupt\n");
> > > > > +
> > > > > +        if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS |
> > > > > +               PCL_CFG_PME_MSI_STATUS)) {
> > > > > +            virq = irq_linear_revmap(pp->irq_domain, 0);
> > > > > +            generic_handle_irq(virq);
> > > > > +        }
> > > > > +    }
> > > > 
> > > > Please have two handlers: one for interrupts that are from the RC,
> > > > another for interrupts coming from the endpoints.
> > > I assume that this handler treats interrupts from the RC only and
> > > this is set on the member ".msi_host_isr" added in the patch 1/6.
> > > I think that the handler for interrupts coming from endpoints should be
> > > treated as a normal case (after calling .msi_host_isr in
> > > dw_handle_msi_irq()).
> > 
> > It looks pretty odd that you end-up dealing with both from the
> > same "parent" interrupt. I guess this is in keeping with the
> > rest of the DW PCIe hacks... :-/
> 
> It might be odd, however, in case of UniPhier SoC,
> both MSI interrupts from endpoints and PME/AER interrupts from RC are
> asserted by same "parent" interrupt. In other words, PME/AER interrupts
> are notified using the parent interrupt for MSI.
> 
> MSI interrupts are treated as child interrupts with reference to
> the status register in DW core. This is handled in a for-loop in
> dw_handle_msi_irq().
> 
> PME/AER interrupts are treated with reference to the status register
> in UniPhier glue layer, however, this couldn't be handled in the same way
> directly.
> 
> So I'm trying to add .msi_host_isr function to handle this
> with reference to the SoC-specific registers.
> 
> This exported function asserts MSI-0 as a shared child interrupt.
> As a result, PME/AER are registered like the followings in dmesg:
> 
>    pcieport 0000:00:00.0: PME: Signaling with IRQ 25
>    pcieport 0000:00:00.0: AER: enabled with IRQ 25
> 
> And these interrupts are shared as MSI-0:
> 
>    # cat /proc/interrupts | grep 25:
>     25:          0          0          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv
> 
> This might be a special case, though, I think that this is needed to handle
> interrupts from RC sharing MSI parent.

Can you please send me (with this series *applied* of course and if
possible with an endpoint MSI/MSI-X capable enabled):

- full dmesg log
- lspci -vv output
- cat /proc/interrupts

I need to understand how this system HW works before commenting any
further.

> > It is for Lorenzo to make up his mind about this anyway.
> 
> I'd like to Lorenzo's opinion, too.

I am trying to understand how the HW is wired up (and that's what Marc
asked as well) so first things first, please send the logs.

Lorenzo