Re: [PATCH v3 2/6] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

[Kunihiko Hayashi <hayashi.kunihiko@socionext.com>]

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