| Message ID | 1592469493-1549-3-git-send-email-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: uniphier: Add features for UniPhier PCIe host controller | expand |
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? > + > + 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. M.
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()). Thank you, --- Best Regards Kunihiko Hayashi
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.
Hi Marc, On 2020/06/30 22:23, Marc Zyngier wrote: > 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 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. > It is for Lorenzo to make up his mind about this anyway. I'd like to Lorenzo's opinion, too. Thank you, --- Best Regards Kunihiko Hayashi
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
Hi Lorenzo, On 2020/07/11 1:14, Lorenzo Pieralisi wrote: > 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. Okay, I attach all the log to this mail. This controller has MSI support only, and I attached R8169 ethernet card to PCIe slot to enable the controller. > >>> 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. I hope this will help your understanding. Thank you, --- Best Regards Kunihiko Hayashi root@akebi96:~# dmesg [ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd082] [ 0.000000] Linux version 5.8.0-rc4-next-20200710-00007-g868c2f1a3dad (hayashi@ubuntu) (aarch64-linux-gnu-gcc (Linaro GCC 7.5-2019.12) 7.5.0, GNU ld (Linaro_Binutils-2019.12) 2.28.2.20170706) #21 SMP PREEMPT Mon Jul 13 09:48:17 JST 2020 [ 0.000000] Machine model: Akebi96 [ 0.000000] efi: UEFI not found. [ 0.000000] cma: Reserved 32 MiB at 0x00000000fdc00000 [ 0.000000] NUMA: No NUMA configuration found [ 0.000000] NUMA: Faking a node at [mem 0x0000000080000000-0x000000013fffffff] [ 0.000000] NUMA: NODE_DATA [mem 0x13f9f5340-0x13f9f723f] [ 0.000000] Zone ranges: [ 0.000000] DMA [mem 0x0000000080000000-0x00000000bfffffff] [ 0.000000] DMA32 [mem 0x00000000c0000000-0x00000000ffffffff] [ 0.000000] Normal [mem 0x0000000100000000-0x000000013fffffff] [ 0.000000] Movable zone start for each node [ 0.000000] Early memory node ranges [ 0.000000] node 0: [mem 0x0000000080000000-0x0000000080ffffff] [ 0.000000] node 0: [mem 0x0000000082000000-0x00000000bfffffff] [ 0.000000] node 0: [mem 0x00000000c2000000-0x000000013fffffff] [ 0.000000] Initmem setup node 0 [mem 0x0000000080000000-0x000000013fffffff] [ 0.000000] On node 0 totalpages: 774144 [ 0.000000] DMA zone: 4096 pages used for memmap [ 0.000000] DMA zone: 0 pages reserved [ 0.000000] DMA zone: 258048 pages, LIFO batch:63 [ 0.000000] DMA32 zone: 4096 pages used for memmap [ 0.000000] DMA32 zone: 253952 pages, LIFO batch:63 [ 0.000000] Normal zone: 4096 pages used for memmap [ 0.000000] Normal zone: 262144 pages, LIFO batch:63 [ 0.000000] psci: probing for conduit method from DT. [ 0.000000] psci: PSCIv1.1 detected in firmware. [ 0.000000] psci: Using standard PSCI v0.2 function IDs [ 0.000000] psci: Trusted OS migration not required [ 0.000000] psci: SMC Calling Convention v1.2 [ 0.000000] percpu: Embedded 23 pages/cpu s54040 r8192 d31976 u94208 [ 0.000000] pcpu-alloc: s54040 r8192 d31976 u94208 alloc=23*4096 [ 0.000000] pcpu-alloc: [0] 0 [0] 1 [0] 2 [0] 3 [ 0.000000] Detected PIPT I-cache on CPU0 [ 0.000000] CPU features: detected: GIC system register CPU interface [ 0.000000] CPU features: detected: EL2 vector hardening [ 0.000000] Speculative Store Bypass Disable mitigation not required [ 0.000000] CPU features: detected: ARM errata 1165522, 1319367, or 1530923 [ 0.000000] Built 1 zonelists, mobility grouping on. Total pages: 761856 [ 0.000000] Policy zone: Normal [ 0.000000] Kernel command line: [ 0.000000] Dentry cache hash table entries: 524288 (order: 10, 4194304 bytes, linear) [ 0.000000] Inode-cache hash table entries: 262144 (order: 9, 2097152 bytes, linear) [ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off [ 0.000000] software IO TLB: mapped [mem 0xbbfff000-0xbffff000] (64MB) [ 0.000000] Memory: 2881608K/3096576K available (14076K kernel code, 2240K rwdata, 7572K rodata, 5696K init, 486K bss, 182200K reserved, 32768K cma-reserved) [ 0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1 [ 0.000000] rcu: Preemptible hierarchical RCU implementation. [ 0.000000] rcu: RCU event tracing is enabled. [ 0.000000] rcu: RCU restricting CPUs from NR_CPUS=256 to nr_cpu_ids=4. [ 0.000000] Trampoline variant of Tasks RCU enabled. [ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay is 25 jiffies. [ 0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=4 [ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0 [ 0.000000] GICv3: GIC: Using split EOI/Deactivate mode [ 0.000000] GICv3: 512 SPIs implemented [ 0.000000] GICv3: 0 Extended SPIs implemented [ 0.000000] GICv3: Distributor has no Range Selector support [ 0.000000] GICv3: 16 PPIs implemented [ 0.000000] GICv3: CPU0: found redistributor 0 region 0:0x000000005fe80000 [ 0.000000] random: get_random_bytes called from start_kernel+0x314/0x4f0 with crng_init=0 [ 0.000000] arch_timer: cp15 timer(s) running at 50.00MHz (phys). [ 0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles: 0xb8812736b, max_idle_ns: 440795202655 ns [ 0.000003] sched_clock: 56 bits at 50MHz, resolution 20ns, wraps every 4398046511100ns [ 0.000358] Console: colour dummy device 80x25 [ 0.000628] printk: console [tty0] enabled [ 0.000690] Calibrating delay loop (skipped), value calculated using timer frequency.. 100.00 BogoMIPS (lpj=200000) [ 0.000705] pid_max: default: 32768 minimum: 301 [ 0.000758] LSM: Security Framework initializing [ 0.000846] Mount-cache hash table entries: 8192 (order: 4, 65536 bytes, linear) [ 0.000905] Mountpoint-cache hash table entries: 8192 (order: 4, 65536 bytes, linear) [ 0.002094] rcu: Hierarchical SRCU implementation. [ 0.003161] EFI services will not be available. [ 0.003357] smp: Bringing up secondary CPUs ... [ 0.028171] Detected PIPT I-cache on CPU1 [ 0.028201] GICv3: CPU1: found redistributor 1 region 0:0x000000005fea0000 [ 0.028228] CPU1: Booted secondary processor 0x0000000001 [0x410fd082] [ 0.040780] CPU features: detected: ARM erratum 845719 [ 0.040794] Detected VIPT I-cache on CPU2 [ 0.040821] GICv3: CPU2: found redistributor 100 region 0:0x000000005fec0000 [ 0.040852] CPU2: Booted secondary processor 0x0000000100 [0x410fd034] [ 0.053434] Detected VIPT I-cache on CPU3 [ 0.053453] GICv3: CPU3: found redistributor 101 region 0:0x000000005fee0000 [ 0.053471] CPU3: Booted secondary processor 0x0000000101 [0x410fd034] [ 0.053567] smp: Brought up 1 node, 4 CPUs [ 0.053628] SMP: Total of 4 processors activated. [ 0.053636] CPU features: detected: 32-bit EL0 Support [ 0.053644] CPU features: detected: CRC32 instructions [ 0.053652] CPU features: detected: 32-bit EL1 Support [ 0.069374] CPU: All CPU(s) started at EL2 [ 0.069409] alternatives: patching kernel code [ 0.070560] devtmpfs: initialized [ 0.074955] KASLR disabled due to lack of seed [ 0.075508] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 7645041785100000 ns [ 0.075541] futex hash table entries: 1024 (order: 4, 65536 bytes, linear) [ 0.076843] pinctrl core: initialized pinctrl subsystem [ 0.078133] thermal_sys: Registered thermal governor 'step_wise' [ 0.078136] thermal_sys: Registered thermal governor 'power_allocator' [ 0.078489] DMI not present or invalid. [ 0.079115] NET: Registered protocol family 16 [ 0.080223] DMA: preallocated 512 KiB GFP_KERNEL pool for atomic allocations [ 0.080365] DMA: preallocated 512 KiB GFP_KERNEL|GFP_DMA pool for atomic allocations [ 0.080608] DMA: preallocated 512 KiB GFP_KERNEL|GFP_DMA32 pool for atomic allocations [ 0.080719] audit: initializing netlink subsys (disabled) [ 0.080920] audit: type=2000 audit(0.080:1): state=initialized audit_enabled=0 res=1 [ 0.081732] cpuidle: using governor menu [ 0.081826] hw-breakpoint: found 6 breakpoint and 4 watchpoint registers. [ 0.081907] ASID allocator initialised with 65536 entries [ 0.083251] Serial: AMBA PL011 UART driver [ 0.106086] HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages [ 0.106112] HugeTLB registered 32.0 MiB page size, pre-allocated 0 pages [ 0.106126] HugeTLB registered 2.00 MiB page size, pre-allocated 0 pages [ 0.106140] HugeTLB registered 64.0 KiB page size, pre-allocated 0 pages [ 0.107261] cryptd: max_cpu_qlen set to 1000 [ 0.109289] ACPI: Interpreter disabled. [ 0.110385] iommu: Default domain type: Translated [ 0.110624] vgaarb: loaded [ 0.110992] SCSI subsystem initialized [ 0.111140] libata version 3.00 loaded. [ 0.111382] usbcore: registered new interface driver usbfs [ 0.111433] usbcore: registered new interface driver hub [ 0.111502] usbcore: registered new device driver usb [ 0.112240] pps_core: LinuxPPS API ver. 1 registered [ 0.112252] pps_core: Software ver. 5.3.6 - Copyright 2005-2007 Rodolfo Giometti <giometti@linux.it> [ 0.112279] PTP clock support registered [ 0.112413] EDAC MC: Ver: 3.0.0 [ 0.113709] FPGA manager framework [ 0.113811] Advanced Linux Sound Architecture Driver Initialized. [ 0.114454] clocksource: Switched to clocksource arch_sys_counter [ 0.114662] VFS: Disk quotas dquot_6.6.0 [ 0.114727] VFS: Dquot-cache hash table entries: 512 (order 0, 4096 bytes) [ 0.114943] pnp: PnP ACPI: disabled [ 0.120538] NET: Registered protocol family 2 [ 0.120950] tcp_listen_portaddr_hash hash table entries: 2048 (order: 3, 32768 bytes, linear) [ 0.121018] TCP established hash table entries: 32768 (order: 6, 262144 bytes, linear) [ 0.121260] TCP bind hash table entries: 32768 (order: 7, 524288 bytes, linear) [ 0.121802] TCP: Hash tables configured (established 32768 bind 32768) [ 0.121949] UDP hash table entries: 2048 (order: 4, 65536 bytes, linear) [ 0.122047] UDP-Lite hash table entries: 2048 (order: 4, 65536 bytes, linear) [ 0.122263] NET: Registered protocol family 1 [ 0.122796] RPC: Registered named UNIX socket transport module. [ 0.122810] RPC: Registered udp transport module. [ 0.122821] RPC: Registered tcp transport module. [ 0.122831] RPC: Registered tcp NFSv4.1 backchannel transport module. [ 0.122850] PCI: CLS 0 bytes, default 64 [ 0.123057] Unpacking initramfs... [ 1.191696] Freeing initrd memory: 24236K [ 1.192241] kvm [1]: IPA Size Limit: 40bits [ 1.193026] kvm [1]: GICv3: no GICV resource entry [ 1.193040] kvm [1]: disabling GICv2 emulation [ 1.193069] kvm [1]: GIC system register CPU interface enabled [ 1.193178] kvm [1]: vgic interrupt IRQ1 [ 1.193323] kvm [1]: Hyp mode initialized successfully [ 1.196991] Initialise system trusted keyrings [ 1.197142] workingset: timestamp_bits=44 max_order=20 bucket_order=0 [ 1.203283] squashfs: version 4.0 (2009/01/31) Phillip Lougher [ 1.203979] NFS: Registering the id_resolver key type [ 1.204017] Key type id_resolver registered [ 1.204027] Key type id_legacy registered [ 1.204046] nfs4filelayout_init: NFSv4 File Layout Driver Registering... [ 1.204229] 9p: Installing v9fs 9p2000 file system support [ 1.244795] Key type asymmetric registered [ 1.244810] Asymmetric key parser 'x509' registered [ 1.244850] Block layer SCSI generic (bsg) driver version 0.4 loaded (major 245) [ 1.244867] io scheduler mq-deadline registered [ 1.244879] io scheduler kyber registered [ 1.253988] gpio-427 (xirq0): hogged as input [ 1.254024] gpio-437 (xirq10): hogged as input [ 1.256150] uniphier-pcie 66000000.pcie: invalid resource [ 1.257038] EINJ: ACPI disabled. [ 1.273041] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled [ 1.275558] 54006800.serial: ttyS0 at MMIO 0x54006800 (irq = 7, base_baud = 3676470) is a 16550A [ 2.195244] printk: console [ttyS0] enabled [ 2.200114] 54006a00.serial: ttyS2 at MMIO 0x54006a00 (irq = 8, base_baud = 3676470) is a 16550A [ 2.209472] 54006b00.serial: ttyS3 at MMIO 0x54006b00 (irq = 9, base_baud = 3676470) is a 16550A [ 2.219222] SuperH (H)SCI(F) driver initialized [ 2.224360] msm_serial: driver initialized [ 2.229969] cacheinfo: Unable to detect cache hierarchy for CPU 0 [ 2.243361] loop: module loaded [ 2.247520] megasas: 07.714.04.00-rc1 [ 2.256366] libphy: Fixed MDIO Bus: probed [ 2.261405] tun: Universal TUN/TAP device driver, 1.6 [ 2.267383] thunder_xcv, ver 1.0 [ 2.270667] thunder_bgx, ver 1.0 [ 2.273932] nicpf, ver 1.0 [ 2.277750] hclge is initializing [ 2.281131] hns3: Hisilicon Ethernet Network Driver for Hip08 Family - version [ 2.288386] hns3: Copyright (c) 2017 Huawei Corporation. [ 2.293763] e1000: Intel(R) PRO/1000 Network Driver [ 2.298658] e1000: Copyright (c) 1999-2006 Intel Corporation. [ 2.304465] e1000e: Intel(R) PRO/1000 Network Driver [ 2.309451] e1000e: Copyright(c) 1999 - 2015 Intel Corporation. [ 2.315426] igb: Intel(R) Gigabit Ethernet Network Driver [ 2.320844] igb: Copyright (c) 2007-2014 Intel Corporation. [ 2.326467] igbvf: Intel(R) Gigabit Virtual Function Network Driver [ 2.332752] igbvf: Copyright (c) 2009 - 2012 Intel Corporation. [ 2.339037] sky2: driver version 1.30 [ 2.343674] ave 65000000.ethernet: Using random MAC address: 82:4d:4d:25:34:9d [ 2.474822] libphy: uniphier-mdio: probed [ 2.483088] RTL8211E Gigabit Ethernet 65000000.ethernet-ffffffff:00: attached PHY driver [RTL8211E Gigabit Ethernet] (mii_bus:phy_addr=65000000.ethernet-ffffffff:00, irq=POLL) [ 2.499197] ave 65000000.ethernet: Socionext AVE4 Ethernet IP v1.4 (irq=18, phy=rgmii) [ 2.507943] VFIO - User Level meta-driver version: 0.3 [ 2.516227] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver [ 2.522832] ehci-pci: EHCI PCI platform driver [ 2.527335] ehci-platform: EHCI generic platform driver [ 2.532726] ehci-orion: EHCI orion driver [ 2.536867] ehci-exynos: EHCI Exynos driver [ 2.541158] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver [ 2.547376] ohci-pci: OHCI PCI platform driver [ 2.551873] ohci-platform: OHCI generic platform driver [ 2.557223] ohci-exynos: OHCI Exynos driver [ 2.562108] usbcore: registered new interface driver usb-storage [ 2.570705] i2c /dev entries driver [ 2.582814] uniphier-wdt 61840000.sysctrl:watchdog: watchdog driver (timeout=64 sec, nowayout=0) [ 2.594503] sdhci: Secure Digital Host Controller Interface driver [ 2.600746] sdhci: Copyright(c) Pierre Ossman [ 2.605573] Synopsys Designware Multimedia Card Interface Driver [ 2.612587] sdhci-pltfm: SDHCI platform and OF driver helper [ 2.618719] sdhci-cdns 5a000000.mmc: allocated mmc-pwrseq [ 2.655708] mmc0: SDHCI controller on 5a000000.mmc [5a000000.mmc] using ADMA 64-bit [ 2.665301] ledtrig-cpu: registered to indicate activity on CPUs [ 2.672070] SMCCC: SOC_ID: ARCH_SOC_ID(0) returned error: ffffffffffffffff [ 2.679816] usbcore: registered new interface driver usbhid [ 2.685435] usbhid: USB HID core driver [ 2.692948] optee: probing for conduit method. [ 2.697452] optee: revision 3.8 (af141c61) [ 2.704614] optee: dynamic shared memory is enabled [ 2.755775] optee: initialized driver [ 2.761336] NET: Registered protocol family 17 [ 2.765987] 9pnet: Installing 9P2000 support [ 2.770332] Key type dns_resolver registered [ 2.774840] registered taskstats version 1 [ 2.778967] Loading compiled-in X.509 certificates [ 2.791384] mmc0: new HS200 MMC card at address 0001 [ 2.797156] mmcblk0: mmc0:0001 016G30 14.7 GiB [ 2.801188] uniphier-pcie 66000000.pcie: invalid resource [ 2.802087] mmcblk0boot0: mmc0:0001 016G30 partition 1 4.00 MiB [ 2.813353] mmcblk0boot1: mmc0:0001 016G30 partition 2 4.00 MiB [ 2.819480] mmcblk0rpmb: mmc0:0001 016G30 partition 3 4.00 MiB, chardev (234:0) [ 2.831986] mmcblk0: p1 p2 p3 p4 p5 [ 3.107392] uniphier-pcie 66000000.pcie: host bridge /soc@0/pcie@66000000 ranges: [ 3.114962] uniphier-pcie 66000000.pcie: IO 0x002ffe0000..0x002ffeffff -> 0x0000000000 [ 3.123457] uniphier-pcie 66000000.pcie: MEM 0x0020000000..0x002ffdffff -> 0x0020000000 [ 3.232144] uniphier-pcie 66000000.pcie: Link up [ 3.236912] uniphier-pcie 66000000.pcie: PCI host bridge to bus 0000:00 [ 3.243557] pci_bus 0000:00: root bus resource [bus 00-ff] [ 3.249068] pci_bus 0000:00: root bus resource [io 0x0000-0xffff] [ 3.255274] pci_bus 0000:00: root bus resource [mem 0x20000000-0x2ffdffff] [ 3.262206] pci 0000:00:00.0: [50c1:0001] type 01 class 0x060400 [ 3.268261] pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x03ffffff] [ 3.274564] pci 0000:00:00.0: reg 0x38: [mem 0x00000000-0x0000ffff pref] [ 3.281352] pci 0000:00:00.0: supports D1 [ 3.285380] pci 0000:00:00.0: PME# supported from D0 D1 D3hot D3cold [ 3.293567] pci 0000:01:00.0: [10ec:8168] type 00 class 0x020000 [ 3.299772] pci 0000:01:00.0: reg 0x10: [io 0x0000-0x00ff] [ 3.305503] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x00000fff 64bit] [ 3.312410] pci 0000:01:00.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit pref] [ 3.320290] pci 0000:01:00.0: supports D1 D2 [ 3.324582] pci 0000:01:00.0: PME# supported from D0 D1 D2 D3hot D3cold [ 3.343875] pci 0000:00:00.0: BAR 0: assigned [mem 0x20000000-0x23ffffff] [ 3.350700] pci 0000:00:00.0: BAR 14: assigned [mem 0x24000000-0x240fffff] [ 3.357603] pci 0000:00:00.0: BAR 15: assigned [mem 0x24100000-0x241fffff 64bit pref] [ 3.365462] pci 0000:00:00.0: BAR 6: assigned [mem 0x24200000-0x2420ffff pref] [ 3.372711] pci 0000:00:00.0: BAR 13: assigned [io 0x1000-0x1fff] [ 3.378919] pci 0000:01:00.0: BAR 4: assigned [mem 0x24100000-0x24103fff 64bit pref] [ 3.386762] pci 0000:01:00.0: BAR 2: assigned [mem 0x24000000-0x24000fff 64bit] [ 3.394164] pci 0000:01:00.0: BAR 0: assigned [io 0x1000-0x10ff] [ 3.400303] pci 0000:00:00.0: PCI bridge to [bus 01-ff] [ 3.405549] pci 0000:00:00.0: bridge window [io 0x1000-0x1fff] [ 3.411669] pci 0000:00:00.0: bridge window [mem 0x24000000-0x240fffff] [ 3.418484] pci 0000:00:00.0: bridge window [mem 0x24100000-0x241fffff 64bit pref] [ 3.426635] pcieport 0000:00:00.0: PME: Signaling with IRQ 24 [ 3.432638] pcieport 0000:00:00.0: AER: enabled with IRQ 24 [ 3.438585] r8169 0000:01:00.0: enabling device (0000 -> 0003) [ 3.448382] libphy: r8169: probed [ 3.452575] r8169 0000:01:00.0 eth1: RTL8168e/8111e, 7c:8b:ca:03:1f:33, XID 2c2, IRQ 25 [ 3.460628] r8169 0000:01:00.0 eth1: jumbo features [frames: 9194 bytes, tx checksumming: ko] [ 3.478193] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller [ 3.483775] xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 1 [ 3.491643] xhci-hcd xhci-hcd.0.auto: hcc params 0x0230fe6d hci version 0x110 quirks 0x0000000002010010 [ 3.501129] xhci-hcd xhci-hcd.0.auto: irq 19, io mem 0x65a00000 [ 3.507936] hub 1-0:1.0: USB hub found [ 3.511732] hub 1-0:1.0: 4 ports detected [ 3.516166] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller [ 3.521687] xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 2 [ 3.529386] xhci-hcd xhci-hcd.0.auto: Host supports USB 3.0 SuperSpeed [ 3.535993] usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. [ 3.544539] hub 2-0:1.0: USB hub found [ 3.548330] hub 2-0:1.0: 2 ports detected [ 3.553185] ALSA device list: [ 3.556179] No soundcards found. [ 3.563862] Freeing unused kernel memory: 5696K [ 3.568513] Run /init as init process [ 3.572184] with arguments: [ 3.572186] /init [ 3.572188] with environment: [ 3.572191] HOME=/ [ 3.572193] TERM=linux [ 3.702799] udevd[151]: starting version 3.2.1 [ 3.707631] random: udevd: uninitialized urandom read (16 bytes read) [ 3.714216] random: udevd: uninitialized urandom read (16 bytes read) [ 3.720702] random: udevd: uninitialized urandom read (16 bytes read) [ 3.731790] udevd[152]: starting eudev-3.2.1 [ 3.848597] random: fast init done [ 3.854529] usb 1-4: new high-speed USB device number 2 using xhci-hcd [ 4.125374] splice write not supported for file (pid: 215 comm: cat) [ 4.266321] splice write not supported for file (pid: 263 comm: cat) [ 4.277542] splice write not supported for file (pid: 266 comm: cat) [ 4.284170] splice write not supported for file (pid: 266 comm: cat) [ 4.295556] splice write not supported for file (pid: 270 comm: cat) [ 4.307134] splice write not supported for file (pid: 273 comm: cat) [ 4.324074] splice write not supported for file (pid: 278 comm: cat) [ 4.335313] splice write not supported for file (pid: 281 comm: cat) [ 4.342004] splice write not supported for file (pid: 281 comm: cat) [ 4.353363] splice write not supported for file (pid: 285 comm: cat) root@akebi96:~# root@akebi96:~# lspci -vv 00:00.0 Class 0604: Device 50c1:0001 (rev 01) Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin A routed to IRQ 24 Region 0: Memory at 20000000 (32-bit, non-prefetchable) [size=64M] Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0 I/O behind bridge: 00001000-00001fff Memory behind bridge: 24000000-240fffff Prefetchable memory behind bridge: 0000000024100000-00000000241fffff Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR- [virtual] Expansion ROM at 24200000 [disabled] [size=64K] BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B- PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn- Capabilities: [40] Power Management version 3 Flags: PMEClk- DSI- D1+ D2- AuxCurrent=375mA PME(D0+,D1+,D2-,D3hot+,D3cold+) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable+ Count=1/1 Maskable+ 64bit+ Address: 00000000bbfff000 Data: 0000 Masking: 00000000 Pending: 00000000 Capabilities: [70] Express (v2) Root Port (Slot-), MSI 00 DevCap: MaxPayload 256 bytes, PhantFunc 0 ExtTag- RBE+ DevCtl: Report errors: Correctable+ Non-Fatal+ Fatal+ Unsupported+ RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop- MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend- LnkCap: Port #0, Speed 5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <1us, L1 <64us ClockPM- Surprise- LLActRep+ BwNot+ ASPMOptComp+ LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt- RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible- RootCap: CRSVisible- RootSta: PME ReqID 0000, PMEStatus- PMEPending- DevCap2: Completion Timeout: Range ABCD, TimeoutDis+, LTR+, OBFF Via message/WAKE# ARIFwd- DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR+, OBFF Disabled ARIFwd- LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis- Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS- Compliance De-emphasis: -6dB LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete-, EqualizationPhase1- EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest- Capabilities: [100 v2] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+ AERCap: First Error Pointer: 00, GenCap+ CGenEn- ChkCap+ ChkEn- Kernel driver in use: pcieport 01:00.0 Class 0200: Device 10ec:8168 (rev 06) Subsystem: Device 7470:3468 Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+ Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0, Cache Line Size: 64 bytes Interrupt: pin A routed to IRQ 23 Region 0: I/O ports at 1000 [size=256] Region 2: Memory at 24000000 (64-bit, non-prefetchable) [size=4K] Region 4: Memory at 24100000 (64-bit, prefetchable) [size=16K] Capabilities: [40] Power Management version 3 Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+) Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ Address: 0000000000000000 Data: 0000 Capabilities: [70] Express (v2) Endpoint, MSI 01 DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset- SlotPowerLimit 0.000W DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop- MaxPayload 128 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr+ TransPend- LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <512ns, L1 <64us ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp- LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt- DevCap2: Completion Timeout: Not Supported, TimeoutDis+, LTR-, OBFF Not Supported DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-, OBFF Disabled LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis- Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS- Compliance De-emphasis: -6dB LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1- EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest- Capabilities: [b0] MSI-X: Enable+ Count=4 Masked- Vector table: BAR=4 offset=00000000 PBA: BAR=4 offset=00000800 Capabilities: [d0] Vital Product Data Not readable Capabilities: [100 v1] Advanced Error Reporting UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol- UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol- CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr- CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+ AERCap: First Error Pointer: 00, GenCap+ CGenEn- ChkCap+ ChkEn- Capabilities: [140 v1] Virtual Channel Caps: LPEVC=0 RefClk=100ns PATEntryBits=1 Arb: Fixed- WRR32- WRR64- WRR128- Ctrl: ArbSelect=Fixed Status: InProgress- VC0: Caps: PATOffset=00 MaxTimeSlots=1 RejSnoopTrans- Arb: Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256- Ctrl: Enable+ ID=0 ArbSelect=Fixed TC/VC=ff Status: NegoPending- InProgress- Capabilities: [160 v1] Device Serial Number f8-45-00-00-68-4c-e0-00 Kernel driver in use: r8169 root@akebi96:~# cat /proc/interrupts [ 189.905575] do_splice_from: 3 callbacks suppressed [ 189.905588] splice write not supported for file /ttyS0 (pid: 383 comm: cat) CPU0 CPU1 CPU2 CPU3 1: 0 0 0 0 GICv3 25 Level vgic 3: 539 846 1110 1171 GICv3 30 Level arch_timer 4: 0 0 0 0 GICv3 27 Level kvm guest vtimer 7: 2078 0 0 0 GICv3 65 Level ttyS0 11: 0 0 0 0 GICv3 73 Level 58780000.i2c 12: 0 0 0 0 GICv3 74 Level 58781000.i2c 13: 0 0 0 0 GICv3 75 Level 58782000.i2c 14: 0 0 0 0 GICv3 57 Level 58785000.i2c 15: 702 0 0 0 GICv3 110 Level mmc0 17: 0 0 0 0 GICv3 35 Level thermal 18: 355 0 0 0 GICv3 98 Level eth0 19: 46 0 0 0 GICv3 166 Level xhci-hcd:usb1 24: 0 0 0 0 PCI-MSI 0 Edge PCIe PME, aerdrv 25: 0 0 0 0 PCI-MSI 524288 Edge eth1 IPI0: 375 508 409 457 Rescheduling interrupts IPI1: 288 275 541 413 Function call interrupts IPI2: 0 0 0 0 CPU stop interrupts IPI3: 0 0 0 0 CPU stop (for crash dump) interrupts IPI4: 0 0 0 0 Timer broadcast interrupts IPI5: 0 0 0 0 IRQ work interrupts IPI6: 0 0 0 0 CPU wake-up interrupts Err: 0 root@akebi96:~#
On Thu, Jun 18, 2020 at 05:38:09PM +0900, 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. Define what you mean please. > 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; > + > + 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); I think this is wrong. pp->irq_domain is the DWC MSI domain, how do you know that hwirq 0 *is* the AER/PME interrupt ? It just *works* in this case because the port driver probes and alloc MSIs before any PCI device has a chance to do it and actually I think this is just wrong also because hwirq 0 *is* usable by devices but it can't be used because current code takes it for the PME/AER interrupt (which AFAICS is an internal signal disconnected from the DWC MSI interrupt controller). I think this extra glue logic should be separate MSI domain otherwise there is no way you can reliably look-up the virq corresponding to AER/PME. How does it work in HW ? Is the root port really sending a memory write to raise an IRQ or it just signal the IRQ through internal logic ? I think the root port MSI handling is different from the DWC logic and should be treated separately. Lorenzo > + generic_handle_irq(virq); > + } > + } > > writel(val, priv->base + PCL_RCV_INT); > +} > + > +static void uniphier_pcie_msi_host_isr(struct pcie_port *pp) > +{ > + uniphier_pcie_misc_isr(pp, true); > +} > + > +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; > > /* INTx */ > chained_irq_enter(chip, desc); > > + uniphier_pcie_misc_isr(pp, false); > + > val = readl(priv->base + PCL_RCV_INTX); > reg = FIELD_GET(PCL_RCV_INTX_ALL_STATUS, val); > > @@ -330,6 +364,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp) > > static const struct dw_pcie_host_ops uniphier_pcie_host_ops = { > .host_init = uniphier_pcie_host_init, > + .msi_host_isr = uniphier_pcie_msi_host_isr, > }; > > static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, > -- > 2.7.4 >
Hi Lorenzo, On 2020/07/14 22:27, Lorenzo Pieralisi wrote: > On Thu, Jun 18, 2020 at 05:38:09PM +0900, 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. > > Define what you mean please. AER/PME signals are assigned to the same signal as MSI by internal logic, that is, AER/PME and MSI are assigned to the same GIC interrupt number. So it's necessary to modify the code to call the misc handler from MSI handler. I'll rewrite it next. > >> 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; >> + >> + 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); > > I think this is wrong. pp->irq_domain is the DWC MSI domain, how do > you know that hwirq 0 *is* the AER/PME interrupt ? When AER/PME drivers are probed, AER/PME interrupts are registered as MSI-0. The pcie_message_numbers() function refers the following fields of PCI registers, - PCI_EXP_FLAGS_IRQ (for PME) - PCI_ERR_ROOT_AER_IRQ (for AER) and decides AER/PME interrupts numbers in MSI domain. Initial values of both fields are 0, so these interrupts are set to MSI-0. However, pcie_uniphier driver doesn't know that these interrupts are MSI-0. Surely using 0 here is wrong. I think that the method to get virq for AER/PME from pcieport is needed. > > It just *works* in this case because the port driver probes and alloc > MSIs before any PCI device has a chance to do it and actually I think > this is just wrong also because hwirq 0 *is* usable by devices but > it can't be used because current code takes it for the PME/AER interrupt > (which AFAICS is an internal signal disconnected from the DWC MSI > interrupt controller). AER/PME interrupts are with IRQF_SHARED, so hwirq 0 can share any PCI device, however, the multiple handlers might be called with other factor, so I don't think it is desiable. > > I think this extra glue logic should be separate MSI domain > otherwise there is no way you can reliably look-up the virq > corresponding to AER/PME. Ok, however, it seems that there is no way to get virq for AER/PME from pcieport in pcie/portdrv_core.c. When I try to separate AER/PME interrtups from MSI domain, how should I get virq for AER/PME? > > How does it work in HW ? Is the root port really sending a memory > write to raise an IRQ or it just signal the IRQ through internal > logic ? I think the root port MSI handling is different from the > DWC logic and should be treated separately. The internal logic assigns the same signal as MSI interrupt to AER/PME interrupts. The MSI handler checks internal status register PCL_RCV_INT, and know if the signal is AER or PME interrupt. MSI memory write isn't used for the signal. Since DWC MSI handler doesn't have a method to check the internal status register, I added callback .msi_host_isr() to DWC MSI handler in patch 1/6. Thank you, --- Best Regards Kunihiko Hayashi
On 2020/07/15 19:04, Kunihiko Hayashi wrote: > Hi Lorenzo, > > On 2020/07/14 22:27, Lorenzo Pieralisi wrote: >> On Thu, Jun 18, 2020 at 05:38:09PM +0900, 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. >> >> Define what you mean please. [snip] >> I think this is wrong. pp->irq_domain is the DWC MSI domain, how do >> you know that hwirq 0 *is* the AER/PME interrupt ? > > When AER/PME drivers are probed, AER/PME interrupts are registered > as MSI-0. > > The pcie_message_numbers() function refers the following fields of > PCI registers, > > - PCI_EXP_FLAGS_IRQ (for PME) > - PCI_ERR_ROOT_AER_IRQ (for AER) > > and decides AER/PME interrupts numbers in MSI domain. > Initial values of both fields are 0, so these interrupts are set to MSI-0. > > However, pcie_uniphier driver doesn't know that these interrupts are MSI-0. > Surely using 0 here is wrong. > I think that the method to get virq for AER/PME from pcieport is needed. To avoid using hard-coded MSI-0, I'll add new function to get a virq number corresponding to each service (AER/PME) to portdrv. I'll update the series in v6. Thank you, --- Best Regards Kunihiko Hayashi
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; + + 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); + } + } writel(val, priv->base + PCL_RCV_INT); +} + +static void uniphier_pcie_msi_host_isr(struct pcie_port *pp) +{ + uniphier_pcie_misc_isr(pp, true); +} + +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; /* INTx */ chained_irq_enter(chip, desc); + uniphier_pcie_misc_isr(pp, false); + val = readl(priv->base + PCL_RCV_INTX); reg = FIELD_GET(PCL_RCV_INTX_ALL_STATUS, val); @@ -330,6 +364,7 @@ static int uniphier_pcie_host_init(struct pcie_port *pp) static const struct dw_pcie_host_ops uniphier_pcie_host_ops = { .host_init = uniphier_pcie_host_init, + .msi_host_isr = uniphier_pcie_msi_host_isr, }; static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv,
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(-)