> -----Original Message----- > From: Vignesh R [mailto:vigneshr@xxxxxx] > Sent: Tuesday, November 21, 2017 12:48 AM > To: Roger Quadros <rogerq@xxxxxx> > Cc: Chris Welch <Chris.Welch@xxxxxxxxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; > linux-pci@xxxxxxxxxxxxxxx; Joao Pinto <jpinto@xxxxxxxxxxxx>; KISHON VIJAY > ABRAHAM <kishon@xxxxxx> > Subject: Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 > > > > On Monday 20 November 2017 07:01 PM, Roger Quadros wrote: > > On 20/11/17 15:19, Vignesh R wrote: > >> > >> > >> On Monday 20 November 2017 01:31 PM, Roger Quadros wrote: > >> [...] > >>>> > >>>> So, could you try reverting commit 8c934095fa2f3 and also apply > >>>> below patch and let me know if that fixes the issue? > >>>> > >>>> ----------- > >>>> > >>>> diff --git a/drivers/pci/dwc/pci-dra7xx.c > >>>> b/drivers/pci/dwc/pci-dra7xx.c index e77a4ceed74c..8280abc56f30 > >>>> 100644 > >>>> --- a/drivers/pci/dwc/pci-dra7xx.c > >>>> +++ b/drivers/pci/dwc/pci-dra7xx.c > >>>> @@ -259,10 +259,17 @@ static irqreturn_t > dra7xx_pcie_msi_irq_handler(int irq, void *arg) > >>>> u32 reg; > >>>> > >>>> reg = dra7xx_pcie_readl(dra7xx, > >>>> PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI); > >>>> + dra7xx_pcie_writel(dra7xx, > >>>> + PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI, reg); > >>>> > >>>> switch (reg) { > >>>> case MSI: > >>>> - dw_handle_msi_irq(pp); > >>>> + /* > >>>> + * Need to make sure no MSI IRQs are pending before > >>>> + * exiting handler, else the wrapper will not catch new > >>>> + * IRQs. So loop around till dw_handle_msi_irq() returns > >>>> + * IRQ_NONE > >>>> + */ > >>>> + while (dw_handle_msi_irq(pp) != IRQ_NONE); The patch looks good, I haven't had a failure in a few days of testing. You should also look at incorporating the following that I needed to change to get our product working. The first change fixes a miss by one error with the interrupt lines. The second change extends a patch you developed for errata i870 but we found is applicable to RC operation as well as EPs. Thanks very much for your help! diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c old mode 100644 new mode 100755 index defa272..6245d89 --- a/drivers/pci/dwc/pci-dra7xx.c +++ b/drivers/pci/dwc/pci-dra7xx.c @@ -238,8 +238,8 @@ static int dra7xx_pcie_init_irq_domain(struct pcie_port *pp) dev_err(dev, "No PCIe Intc node found\n"); return -ENODEV; } - - dra7xx->irq_domain = irq_domain_add_linear(pcie_intc_node, 4, + // PCI interrupt lines start at 1 not zero so need to add 1 + dra7xx->irq_domain = irq_domain_add_linear(pcie_intc_node, 4 + 1, &intx_domain_ops, pp); if (!dra7xx->irq_domain) { dev_err(dev, "Failed to get a INTx IRQ domain\n"); @@ -706,10 +706,16 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, DEVICE_TYPE_RC); + // Errata i870 applies to RC as well as EP + ret = dra7xx_pcie_ep_legacy_mode(dev); + if (ret) + goto err_gpio; + ret = dra7xx_add_pcie_port(dra7xx, pdev); if (ret < 0) goto err_gpio; break; > >>> > >>> To avoid this kind of looping, shouldn't we be disabling all IRQ > >>> events while the interrupt handler is running and enable them just > >>> before we return from the hardirq handler? > >> > >> IIUC, you are saying to disable all MSIs at PCIe designware core > >> level, then call dw_handle_msi_irq() and then enable MSIs after > >> hardirq returns. But, the problem is if PCIe EP raises another MSI > >> after the call to EP's handler but before re-enabling MSIs, then it > >> will be ignored as IRQs are not yet enabled. > >> Ideally, EP's support Per Vector Masking(PVM) which allow RC to > >> prevent EP from sending MSI messages for sometime. But, > >> unfortunately, the cards mentioned here don't support this feature. > > > > I'm not aware of MSIs. > > > > But for any typical hardware, there should be an interrupt event > > enable register and an interrupt mask register. > > > > In the IRQ handler, we mask the interrupt but still keep the interrupt > > events enabled so that they can be latched during the time the interrupt was > masked. > > > > When the interrupt is unmasked at end of the IRQ handler, it should > > re-trigger the interrupt if any events were latched and pending. > > > > This way you don't need to keep checking for any pending events in the IRQ > handler. > > > > Thanks for the suggestion! I tried using interrupt masking at designware level. > But, unfortunately that does not help and my test cases still fail. > Seems like designware MSI status register is a non-masked status and dra7xx > specific wrapper seems to be relying on this non-masked status to raise > IRQ(instead of actual IRQ signal of designware) to CPU. There is very little > documentation in the TRM wrt how wrapper forwards designware IRQ status to > CPU. > > So, at best, I will add a check in the above while() loop and break and exit IRQ > handler, lets say after 1K loops. > > > -- > Regards > Vignesh
