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

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

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