lkml.org 
[lkml]   [2020]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER
    From
    Date
    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

    \
     
     \ /
      Last update: 2020-06-04 11:44    [W:4.956 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site