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

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

On 07/05/2017 11:26 AM, Marc Zyngier wrote:
> On 05/07/17 11:57, Joao Pinto wrote:
>> Às 6:05 PM de 6/20/2017, Murali Karicheri escreveu:
>>> 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?
>>
>> Well, that can be challenging, since it is a very particular operation.
>> Maybe we should implement a _pci_setup_msi_msg in the keystone driver and
>> overide this one if necessary. What do you think?
> 
> At that rate, and given that keystone seems to override almost all
> operations in this driver, wouldn't it make sense for this to be
> implemented as a *separate* driver?
Marc,

What you mean by separate driver? Just the MSI handling part? 

Murali
> 
> Thanks,
> 
> 	M.
> 


-- 
Murali Karicheri
Linux Kernel, Keystone