On 07/06/2017 05:05 AM, Joao Pinto wrote: > > Hi to all, > > Às 9:02 AM de 7/6/2017, Marc Zyngier escreveu: >> 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? > > I agree with Mark. The core driver is full of callbacks for custom SoC > operations, and most of them are not massively used. > Keystone with no doubt has its own way of dealing with interrupts, so I fully > support the a keystone_msi driver to be created as altera does. > > At this time my bandwidth is almost full, so I won't be able to do it. Muralli > what are your thoughts? Would you help on this task? I agree as well. If I recall, that was how the initial version of the driver looked like. But then based on review, it was changed to use core API and I had to add the callback functions into keystone dwc to handle msi. Unfortunately I am running into the same situation as Joao and afraid I can get to this anytime soon. Is there a possibility of excluding Keystone from this patch set without breaking the driver? If so we need an interim solution to have keystone not affected by this change and I can add the new driver later when I get some bandwidth. Murali > > Thanks, > Joao > > >> >> M. >> > -- Murali Karicheri Linux Kernel, Keystone
