| Message ID | 1591174481-13975-3-git-send-email-hayashi.kunihiko@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: uniphier: Add features for UniPhier PCIe host controller | expand |
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 > 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@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 | 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? > + 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? > + } > > 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. > > /* INTx */ > chained_irq_enter(chip, desc); > @@ -330,6 +360,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_misc_isr, > }; > > static int uniphier_add_pcie_port(struct uniphier_pcie_priv *priv, Thanks, M.
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@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 | 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
On 2020-06-04 10:43, Kunihiko Hayashi wrote: [...] >>> -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. So what is wrong with if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS | PCL_CFG_PME_MSI_STATUS)) { // handle interrupt } ? If you have two handlers for the same interrupt, this is a shared interrupt and each handler will be called in turn. M.
Hi Marc, On 2020/06/04 19:11, Marc Zyngier wrote: > On 2020-06-04 10:43, Kunihiko Hayashi wrote: > > [...] > >>>> -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. > > So what is wrong with > > if (val & (PCL_CFG_AER_RC_ERR_MSI_STATUS | > PCL_CFG_PME_MSI_STATUS)) { > // handle interrupt > } > > ? No problem. I'll rewrite it in the same way as yours in handling interrupts. > If you have two handlers for the same interrupt, this is a shared > interrupt and each handler will be called in turn. Yes, MSI-0 is shared with PME and AER, and it will be like that. 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..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()) { + 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); + } + } 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); /* INTx */ chained_irq_enter(chip, desc); @@ -330,6 +360,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_misc_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 intterupts, 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 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@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 | 53 +++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 11 deletions(-)