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

[Marc Zyngier <marc.zyngier@arm.com>]

On 05/07/17 22:03, Murali Karicheri wrote:
> 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? 

The MSI part indeed. Looking at this series, KS looks like a sore spot
on the face of rest of the MSI code, and it'd be better of opting out of
this MSI management code and provide its own.

It seems to me that this would simplify the DW MSI code (no more
indirections all over the place), and make it obvious that KW MSIs are
managed in a very different way.

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...