Messages in this thread | | | Date | Tue, 30 Jun 2020 14:23:24 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH v5 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER |
| |
On 2020-06-29 10:49, Kunihiko Hayashi wrote: > Hi Marc, > > On 2020/06/27 18:48, Marc Zyngier wrote: >> On Thu, 18 Jun 2020 09:38:09 +0100, >> Kunihiko Hayashi <hayashi.kunihiko@socionext.com> 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 >>> interrupts, which is called from both INTx and MSI handlers. >>> This function detects PME and AER interrupts with the status >>> register, >>> and invoke PME and AER drivers related to 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@kernel.org> >>> Cc: Jingoo Han <jingoohan1@gmail.com> >>> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com> >>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>> --- >>> drivers/pci/controller/dwc/pcie-uniphier.c | 57 >>> ++++++++++++++++++++++++------ >>> 1 file changed, 46 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c >>> b/drivers/pci/controller/dwc/pcie-uniphier.c >>> index a5401a0..5ce2479 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; >> >> Does this affect endpoints? Or just the RC itself? > > These interrupts are asserted by RC itself, so this part affects only > RC. > >>> + >>> + writel(val, priv->base + PCL_RCV_INT); >>> writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX); >>> } >>> @@ -231,32 +241,56 @@ 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, 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 is for Lorenzo to make up his mind about this anyway.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |