Re: [PATCH v2 0/9] add new irq api to pcie-designware

[Murali Karicheri <m-karicheri2@ti.com>]

On 06/20/2017 12:28 PM, Joao Pinto wrote:
> Às 4:50 PM de 6/20/2017, Murali Karicheri escreveu:
>> On 06/19/2017 12:31 PM, Joao Pinto wrote:
>>>
>>> Hi Murali,
>>>
>>> Às 10:18 PM de 6/12/2017, Murali Karicheri escreveu:
>>>> Joao,
>>>>
>>>> On 06/05/2017 12:19 PM, Joao Pinto wrote:
>>>>> This patch series adds the new interrupt api to pcie-designware
>>>>> make it possible to use features like MSIX.
>>>>>
>>>>> The work consisted of adapting the pcie-designware-host and each SoC
>>>>> specific driver.
>>>>>
>>>>> The patch set was made against the Bjorn' next branch.
>>>>>
>>>>> Joao Pinto (9):
>>>>>   pci: adding new irq api to pci-designware
>>>>>   pci: exynos SoC driver adapted to new irq API
>>>>>   pci: imx6 SoC driver adapted to new irq API
>>>>>   pci: artpec6 SoC driver adapted to new irq API
>>>>>   pci: generic PCIe DW driver adapted to new irq API
>>>>>   pci: qcom SoC driver adapted to new irq API
>>>>>   pci: keystone SoC driver adapted to new irq API
>>>>>   pci: removing old irq api from pcie-designware
>>>>>   pci: remove limitation of the number of the available IRQs
>>>>>
>>>>>  drivers/pci/dwc/pci-exynos.c           |  18 --
>>>>>  drivers/pci/dwc/pci-imx6.c             |  18 --
>>>>>  drivers/pci/dwc/pci-keystone-dw.c      |  96 +-------
>>>>>  drivers/pci/dwc/pci-keystone.c         |   1 +
>>>>>  drivers/pci/dwc/pci-keystone.h         |   4 +-
>>>>>  drivers/pci/dwc/pci-layerscape.c       |   4 +-
>>>>>  drivers/pci/dwc/pcie-artpec6.c         |  18 --
>>>>>  drivers/pci/dwc/pcie-designware-host.c | 395 +++++++++++++++++++--------------
>>>>>  drivers/pci/dwc/pcie-designware-plat.c |  15 --
>>>>>  drivers/pci/dwc/pcie-designware.h      |  30 ++-
>>>>>  drivers/pci/dwc/pcie-qcom.c            |  15 --
>>>>>  11 files changed, 255 insertions(+), 359 deletions(-)
>>>>>
>>>>
>>>> I gave this a try today and it failed. Logs at
>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__pastebin.ubuntu.com_24843963_&d=DwIC-g&c=DPL6_X_6JkXFx7AXWqB0tg&r=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0&m=FR_WRctJN74GGjjHco1VUqNfINiGA7lGruObX4bplAY&s=8QYJU-SCW5JEmMwLUqbOEBJA5hio9vIyeNyotVRos4Q&e= 
>>>>
>>>> The first part of the log is with your patch series.
>>>> The second part is before applying the patch. You will see
>>>> that there is qc timeout log that shows the issue
>>>>
>>>> [   12.791852] ata2.00: qc timeout (cmd 0xec)
>>>> [   12.795947] ata2.00: failed to IDENTIFY (I/O error, err_mask=0x4)
>>>> [   12.802061] ata2: limiting SATA link speed to 3.0 Gbps
>>>> [   13.301853] ata2: SATA link up 3.0 Gbps (SStatus 123 SControl 320)
>>>> [   23.511852] ata2.00: qc timeout (cmd 0xec)
>>>> [   23.515947] ata2.00: failed to IDENTIFY (I/O error, err_mask=0x4)
>>>> [   24.021850] ata2: SATA link up 3.0 Gbps (SStatus 123 SControl 320)
>>>>
>>>> Is there any DT change I need to make for this? I didn't have a
>>>> chance to review your patch w.r.t Keystone. But I see you have change
>>>> the MSI IRq handling.
>>>>
>>>
>>> There is no need to change your DT. I think I will need to debug this issue
>>> further. Could you please advice a board containing this SoC?
>>>
>> I think I may be able to work with you on this. Keystone has different 
>> register set for MSI IRQ. It is in application space. Also there are
>> 8 interrupts to ARM GIC that multiplexes the 32 MSI interrupts. So
>> please review the original code vs the changed one w.r.t to this. I haven't
>> had a chance to review the code as I am in the middle of a release.
>> But will be able to spend some time after two weeks. Meanwhile please
>> spend some time reviewing the code and exchange email to identify
>> the issue. I can add specific debug prints for you as well and provide
>> you the log for analysis.
> 
> Ok, I agree! Thanks!
> 
>>
Look at this logic in the patch where a potential issue exists for Keystone.

+static void dw_pci_setup_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+       struct dw_pcie *pci = irq_data_get_irq_chip_data(data);
+       struct pcie_port *pp = &pci->pp;
+       u64 msi_target;
+
+       if (pp->ops->get_msi_addr)
+               msi_target = pp->ops->get_msi_addr(pp);
+       else
+               msi_target = virt_to_phys((void *)pp->msi_data);
+
+       msg->address_lo = lower_32_bits(msi_target);
+       msg->address_hi = upper_32_bits(msi_target);
+
+       if (pp->ops->get_msi_data)
+               msg->data = pp->ops->get_msi_data(pp, data->hwirq);
+       else
+               msg->data = data->hwirq;
+
+       dev_dbg(pci->dev, "msi#%d address_hi %#x address_lo %#x\n",
+               (int)data->hwirq, msg->address_hi, msg->address_lo);
+}

Keystone has 8 hw irqs, 4 multiplexed over each since the register
set is different than other DWC cores. So will not work for Keystone,
right?

Murali

>> Let me know.
>>
>> Murali
>>> Thanks.
>>>
>>>
>>>
>>>
>>
>>
> 


-- 
Murali Karicheri
Linux Kernel, Keystone