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