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
