* Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 [not found] ` <3dd7a4fc-da86-03cc-9b01-a0d29dd73230@ti.com> @ 2017-11-16 12:26 ` Vignesh R 2017-11-20 8:01 ` Roger Quadros 2018-02-08 8:54 ` Niklas Cassel 0 siblings, 2 replies; 11+ messages in thread From: Vignesh R @ 2017-11-16 12:26 UTC (permalink / raw) To: Quadros, Roger Cc: Chris Welch, linux-usb@vger.kernel.org, linux-pci, Joao Pinto, KISHON VIJAY ABRAHAM +linux-pci Hi Chris, On Thursday 16 November 2017 05:20 PM, Quadros, Roger wrote: > +Vignesh > > On 13/09/17 17:26, Chris Welch wrote: >> We are developing a product based on the TI AM5728 EVM. The product utilizes a TUSB7340 PCIe USB host for additional ports. The TUSB7340 is detected and setup properly and works OK with low data rate devices. However, hot plugging a Realtek USB network adapter and doing Ethernet transfer bandwidth testing using iperf3 causes the TUSB7340 host to be locked out. The TUSB7340 host appears to no longer communicate and the logging indicates xhci_hcd 0000:01:00.0: HC died; cleaning up. Same issue occurs with another USB Ethernet adapter I tried (Asus). >> >> We looked at using another host and found a mini PCIe card that utilizes the µPD720201 and can be directly installed on the TI AM5728 EVM. The card is detected properly and we reran the transfer test. The uPD720201 gets locks out with the same problem. >> >> The AM5728 testing was performed using the TI SD card stock am57xx-evm-linux-04.00.00.04.img, kernel am57xx-evm 4.9.28-geed43d1050, and it reports that it is using the TI AM572x EVM Rev A3 device tree. >> >> It shows the following logging when it fails (this is with the TI EVM and uPD720201). >> >> [ 630.400899] xhci_hcd 0000:01:00.0: xHCI host not responding to stop endpoint command. >> [ 630.408769] xhci_hcd 0000:01:00.0: Assuming host is dying, halting host. >> [ 630.420849] r8152 2-4:1.0 enp1s0u4: Tx status -108 >> [ 630.425667] r8152 2-4:1.0 enp1s0u4: Tx status -108 >> [ 630.430483] r8152 2-4:1.0 enp1s0u4: Tx status -108 >> [ 630.435297] r8152 2-4:1.0 enp1s0u4: Tx status -108 >> [ 630.440122] xhci_hcd 0000:01:00.0: HC died; cleaning up >> [ 630.453961] usb 2-4: USB disconnect, device number 2 >> >> The problem appears to be a general driver issue given we get the same problem with both the TUSB7340 and the µPD720201. Seems like PCIe driver is missing MSI IRQs leading to stall. Reading xHCI registers via PCIe mem space confirms this. I see two problems wrt MSI handling: Since commit 8c934095fa2f3 ("PCI: dwc: Clear MSI interrupt status after it is handled, not before"), dwc clears MSI status after calling EP's IRQ handler. But, it happens that another MSI interrupt is raised just at the end of EP's IRQ handler and before clearing MSI status. This will result in loss of new MSI IRQ as we clear the MSI IRQ status without handling. Another problem appears to be wrt dra7xx PCIe wrapper: PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI does not seem to catch MSI IRQs unless, its ensured that PCIE_MSI_INTR0_STATUS register read returns 0. 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); break; case INTA: case INTB: @@ -273,8 +280,6 @@ static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg) break; } - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI, reg); - return IRQ_HANDLED; } -- Regards Vignesh ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2017-11-16 12:26 ` xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 Vignesh R @ 2017-11-20 8:01 ` Roger Quadros 2017-11-20 13:19 ` Vignesh R 2018-02-08 8:54 ` Niklas Cassel 1 sibling, 1 reply; 11+ messages in thread From: Roger Quadros @ 2017-11-20 8:01 UTC (permalink / raw) To: Vignesh R Cc: Chris Welch, linux-usb@vger.kernel.org, linux-pci, Joao Pinto, KISHON VIJAY ABRAHAM Hi Vignesh, On 16/11/17 14:26, Vignesh R wrote: > +linux-pci > > Hi Chris, > > On Thursday 16 November 2017 05:20 PM, Quadros, Roger wrote: >> +Vignesh >> >> On 13/09/17 17:26, Chris Welch wrote: >>> We are developing a product based on the TI AM5728 EVM. The product utilizes a TUSB7340 PCIe USB host for additional ports. The TUSB7340 is detected and setup properly and works OK with low data rate devices. However, hot plugging a Realtek USB network adapter and doing Ethernet transfer bandwidth testing using iperf3 causes the TUSB7340 host to be locked out. The TUSB7340 host appears to no longer communicate and the logging indicates xhci_hcd 0000:01:00.0: HC died; cleaning up. Same issue occurs with another USB Ethernet adapter I tried (Asus). >>> >>> We looked at using another host and found a mini PCIe card that utilizes the µPD720201 and can be directly installed on the TI AM5728 EVM. The card is detected properly and we reran the transfer test. The uPD720201 gets locks out with the same problem. >>> >>> The AM5728 testing was performed using the TI SD card stock am57xx-evm-linux-04.00.00.04.img, kernel am57xx-evm 4.9.28-geed43d1050, and it reports that it is using the TI AM572x EVM Rev A3 device tree. >>> >>> It shows the following logging when it fails (this is with the TI EVM and uPD720201). >>> >>> [ 630.400899] xhci_hcd 0000:01:00.0: xHCI host not responding to stop endpoint command. >>> [ 630.408769] xhci_hcd 0000:01:00.0: Assuming host is dying, halting host. >>> [ 630.420849] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>> [ 630.425667] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>> [ 630.430483] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>> [ 630.435297] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>> [ 630.440122] xhci_hcd 0000:01:00.0: HC died; cleaning up >>> [ 630.453961] usb 2-4: USB disconnect, device number 2 >>> >>> The problem appears to be a general driver issue given we get the same problem with both the TUSB7340 and the µPD720201. > > Seems like PCIe driver is missing MSI IRQs leading to stall. > Reading xHCI registers via PCIe mem space confirms this. > > I see two problems wrt MSI handling: > Since commit 8c934095fa2f3 ("PCI: dwc: Clear MSI interrupt status after it is handled, not before"), > dwc clears MSI status after calling EP's IRQ handler. But, it happens that another MSI interrupt is > raised just at the end of EP's IRQ handler and before clearing MSI status. > This will result in loss of new MSI IRQ as we clear the MSI IRQ status without handling. > > Another problem appears to be wrt dra7xx PCIe wrapper: > PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI does not seem to catch MSI IRQs unless, > its ensured that PCIE_MSI_INTR0_STATUS register read returns 0. > > 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); 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? > break; > case INTA: > case INTB: > @@ -273,8 +280,6 @@ static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg) > break; > } > > - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI, reg); > - > return IRQ_HANDLED; > } > > > > -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2017-11-20 8:01 ` Roger Quadros @ 2017-11-20 13:19 ` Vignesh R 2017-11-20 13:31 ` Roger Quadros 0 siblings, 1 reply; 11+ messages in thread From: Vignesh R @ 2017-11-20 13:19 UTC (permalink / raw) To: Roger Quadros Cc: Chris Welch, linux-usb@vger.kernel.org, linux-pci, Joao Pinto, KISHON VIJAY ABRAHAM 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); > > 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. > > >> break; >> case INTA: >> case INTB: >> @@ -273,8 +280,6 @@ static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg) >> break; >> } >> >> - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI, reg); >> - >> return IRQ_HANDLED; >> } >> >> >> >> > -- Regards Vignesh ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2017-11-20 13:19 ` Vignesh R @ 2017-11-20 13:31 ` Roger Quadros 2017-11-21 5:47 ` Vignesh R 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2017-11-20 13:31 UTC (permalink / raw) To: Vignesh R Cc: Chris Welch, linux-usb@vger.kernel.org, linux-pci, Joao Pinto, KISHON VIJAY ABRAHAM 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); >> >> 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. > >> >> >>> break; >>> case INTA: >>> case INTB: >>> @@ -273,8 +280,6 @@ static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg) >>> break; >>> } >>> >>> - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI, reg); >>> - >>> return IRQ_HANDLED; >>> } >>> >>> >>> >>> >> > -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2017-11-20 13:31 ` Roger Quadros @ 2017-11-21 5:47 ` Vignesh R 2017-11-21 14:50 ` Chris Welch 0 siblings, 1 reply; 11+ messages in thread From: Vignesh R @ 2017-11-21 5:47 UTC (permalink / raw) To: Roger Quadros Cc: Chris Welch, linux-usb@vger.kernel.org, linux-pci, Joao Pinto, KISHON VIJAY ABRAHAM 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); >>> >>> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2017-11-21 5:47 ` Vignesh R @ 2017-11-21 14:50 ` Chris Welch 2017-11-22 8:59 ` Vignesh R 2017-11-22 10:41 ` Vignesh R 0 siblings, 2 replies; 11+ messages in thread From: Chris Welch @ 2017-11-21 14:50 UTC (permalink / raw) To: Vignesh R, Roger Quadros Cc: linux-usb@vger.kernel.org, linux-pci@vger.kernel.org, Joao Pinto, KISHON VIJAY ABRAHAM DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogVmlnbmVzaCBSIFttYWls dG86dmlnbmVzaHJAdGkuY29tXQ0KPiBTZW50OiBUdWVzZGF5LCBOb3ZlbWJlciAyMSwgMjAxNyAx Mjo0OCBBTQ0KPiBUbzogUm9nZXIgUXVhZHJvcyA8cm9nZXJxQHRpLmNvbT4NCj4gQ2M6IENocmlz IFdlbGNoIDxDaHJpcy5XZWxjaEB2aWF2aXNvbHV0aW9ucy5jb20+OyBsaW51eC11c2JAdmdlci5r ZXJuZWwub3JnOw0KPiBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBKb2FvIFBpbnRvIDxqcGlu dG9Ac3lub3BzeXMuY29tPjsgS0lTSE9OIFZJSkFZDQo+IEFCUkFIQU0gPGtpc2hvbkB0aS5jb20+ DQo+IFN1YmplY3Q6IFJlOiB4aGNpX2hjZCBIQyBkaWVkOyBjbGVhbmluZyB1cCB3aXRoIFRVU0I3 MzQwIGFuZCDCtVBENzIwMjAxDQo+IA0KPiANCj4gDQo+IE9uIE1vbmRheSAyMCBOb3ZlbWJlciAy MDE3IDA3OjAxIFBNLCBSb2dlciBRdWFkcm9zIHdyb3RlOg0KPiA+IE9uIDIwLzExLzE3IDE1OjE5 LCBWaWduZXNoIFIgd3JvdGU6DQo+ID4+DQo+ID4+DQo+ID4+IE9uIE1vbmRheSAyMCBOb3ZlbWJl ciAyMDE3IDAxOjMxIFBNLCBSb2dlciBRdWFkcm9zIHdyb3RlOg0KPiA+PiBbLi4uXQ0KPiA+Pj4+ DQo+ID4+Pj4gU28sIGNvdWxkIHlvdSB0cnkgcmV2ZXJ0aW5nIGNvbW1pdCA4YzkzNDA5NWZhMmYz IGFuZCBhbHNvIGFwcGx5DQo+ID4+Pj4gYmVsb3cgcGF0Y2ggYW5kIGxldCBtZSBrbm93IGlmIHRo YXQgZml4ZXMgdGhlIGlzc3VlPw0KPiA+Pj4+DQo+ID4+Pj4gLS0tLS0tLS0tLS0NCj4gPj4+Pg0K PiA+Pj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3BjaS9kd2MvcGNpLWRyYTd4eC5jDQo+ID4+Pj4g Yi9kcml2ZXJzL3BjaS9kd2MvcGNpLWRyYTd4eC5jIGluZGV4IGU3N2E0Y2VlZDc0Yy4uODI4MGFi YzU2ZjMwDQo+ID4+Pj4gMTAwNjQ0DQo+ID4+Pj4gLS0tIGEvZHJpdmVycy9wY2kvZHdjL3BjaS1k cmE3eHguYw0KPiA+Pj4+ICsrKyBiL2RyaXZlcnMvcGNpL2R3Yy9wY2ktZHJhN3h4LmMNCj4gPj4+ PiBAQCAtMjU5LDEwICsyNTksMTcgQEAgc3RhdGljIGlycXJldHVybl90DQo+IGRyYTd4eF9wY2ll X21zaV9pcnFfaGFuZGxlcihpbnQgaXJxLCB2b2lkICphcmcpDQo+ID4+Pj4gICAgICAgICB1MzIg cmVnOw0KPiA+Pj4+DQo+ID4+Pj4gICAgICAgICByZWcgPSBkcmE3eHhfcGNpZV9yZWFkbChkcmE3 eHgsDQo+ID4+Pj4gUENJRUNUUkxfRFJBN1hYX0NPTkZfSVJRU1RBVFVTX01TSSk7DQo+ID4+Pj4g KyAgICAgICBkcmE3eHhfcGNpZV93cml0ZWwoZHJhN3h4LA0KPiA+Pj4+ICsgUENJRUNUUkxfRFJB N1hYX0NPTkZfSVJRU1RBVFVTX01TSSwgcmVnKTsNCj4gPj4+Pg0KPiA+Pj4+ICAgICAgICAgc3dp dGNoIChyZWcpIHsNCj4gPj4+PiAgICAgICAgIGNhc2UgTVNJOg0KPiA+Pj4+IC0gICAgICAgICAg ICAgICBkd19oYW5kbGVfbXNpX2lycShwcCk7DQo+ID4+Pj4gKyAgICAgICAgICAgICAgIC8qDQo+ ID4+Pj4gKyAgICAgICAgICAgICAgICAqIE5lZWQgdG8gbWFrZSBzdXJlIG5vIE1TSSBJUlFzIGFy ZSBwZW5kaW5nIGJlZm9yZQ0KPiA+Pj4+ICsgICAgICAgICAgICAgICAgKiBleGl0aW5nIGhhbmRs ZXIsIGVsc2UgdGhlIHdyYXBwZXIgd2lsbCBub3QgY2F0Y2ggbmV3DQo+ID4+Pj4gKyAgICAgICAg ICAgICAgICAqIElSUXMuIFNvIGxvb3AgYXJvdW5kIHRpbGwgZHdfaGFuZGxlX21zaV9pcnEoKSBy ZXR1cm5zDQo+ID4+Pj4gKyAgICAgICAgICAgICAgICAqIElSUV9OT05FDQo+ID4+Pj4gKyAgICAg ICAgICAgICAgICAqLw0KPiA+Pj4+ICsgICAgICAgICAgICAgICB3aGlsZSAoZHdfaGFuZGxlX21z aV9pcnEocHApICE9IElSUV9OT05FKTsNCg0KVGhlIHBhdGNoIGxvb2tzIGdvb2QsIEkgaGF2ZW4n dCBoYWQgYSBmYWlsdXJlIGluIGEgZmV3IGRheXMgb2YgdGVzdGluZy4gIA0KDQpZb3Ugc2hvdWxk IGFsc28gbG9vayBhdCBpbmNvcnBvcmF0aW5nIHRoZSBmb2xsb3dpbmcgdGhhdCBJIG5lZWRlZCB0 byBjaGFuZ2UgdG8gZ2V0IG91ciBwcm9kdWN0IHdvcmtpbmcuICBUaGUgZmlyc3QgY2hhbmdlIGZp eGVzIGEgbWlzcyBieSBvbmUgZXJyb3Igd2l0aCB0aGUgaW50ZXJydXB0IGxpbmVzLiAgDQoNClRo ZSBzZWNvbmQgY2hhbmdlIGV4dGVuZHMgYSBwYXRjaCB5b3UgZGV2ZWxvcGVkIGZvciBlcnJhdGEg aTg3MCBidXQgd2UgZm91bmQgaXMgYXBwbGljYWJsZSB0byBSQyBvcGVyYXRpb24gYXMgd2VsbCBh cyBFUHMuICBUaGFua3MgdmVyeSBtdWNoIGZvciB5b3VyIGhlbHAhDQoNCmRpZmYgLS1naXQgYS9k cml2ZXJzL3BjaS9kd2MvcGNpLWRyYTd4eC5jIGIvZHJpdmVycy9wY2kvZHdjL3BjaS1kcmE3eHgu Yw0Kb2xkIG1vZGUgMTAwNjQ0DQpuZXcgbW9kZSAxMDA3NTUNCmluZGV4IGRlZmEyNzIuLjYyNDVk ODkNCi0tLSBhL2RyaXZlcnMvcGNpL2R3Yy9wY2ktZHJhN3h4LmMNCisrKyBiL2RyaXZlcnMvcGNp L2R3Yy9wY2ktZHJhN3h4LmMNCkBAIC0yMzgsOCArMjM4LDggQEAgc3RhdGljIGludCBkcmE3eHhf cGNpZV9pbml0X2lycV9kb21haW4oc3RydWN0IHBjaWVfcG9ydCAqcHApDQogICAgICAgICAgICAg ICAgZGV2X2VycihkZXYsICJObyBQQ0llIEludGMgbm9kZSBmb3VuZFxuIik7DQogICAgICAgICAg ICAgICAgcmV0dXJuIC1FTk9ERVY7DQogICAgICAgIH0NCi0NCi0gICAgICAgZHJhN3h4LT5pcnFf ZG9tYWluID0gaXJxX2RvbWFpbl9hZGRfbGluZWFyKHBjaWVfaW50Y19ub2RlLCA0LA0KKyAgICAg ICAgLy8gUENJIGludGVycnVwdCBsaW5lcyBzdGFydCBhdCAxIG5vdCB6ZXJvIHNvIG5lZWQgdG8g YWRkIDENCisgICAgICAgZHJhN3h4LT5pcnFfZG9tYWluID0gaXJxX2RvbWFpbl9hZGRfbGluZWFy KHBjaWVfaW50Y19ub2RlLCA0ICsgMSwNCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICZpbnR4X2RvbWFpbl9vcHMsIHBwKTsNCiAgICAgICAgaWYgKCFk cmE3eHgtPmlycV9kb21haW4pIHsNCiAgICAgICAgICAgICAgICBkZXZfZXJyKGRldiwgIkZhaWxl ZCB0byBnZXQgYSBJTlR4IElSUSBkb21haW5cbiIpOw0KQEAgLTcwNiwxMCArNzA2LDE2IEBAIHN0 YXRpYyBpbnQgX19pbml0IGRyYTd4eF9wY2llX3Byb2JlKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2Ug KnBkZXYpDQogICAgICAgICAgICAgICAgZHJhN3h4X3BjaWVfd3JpdGVsKGRyYTd4eCwgUENJRUNU UkxfVElfQ09ORl9ERVZJQ0VfVFlQRSwNCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgREVWSUNFX1RZUEVfUkMpOw0KIA0KKyAgICAgICAgICAgICAgIC8vIEVycmF0YSBpODcwIGFw cGxpZXMgdG8gUkMgYXMgd2VsbCBhcyBFUA0KKyAgICAgICAgICAgICAgIHJldCA9IGRyYTd4eF9w Y2llX2VwX2xlZ2FjeV9tb2RlKGRldik7DQorICAgICAgICAgICAgICAgaWYgKHJldCkNCisgICAg ICAgICAgICAgICAgICAgICAgIGdvdG8gZXJyX2dwaW87DQorDQogICAgICAgICAgICAgICAgcmV0 ID0gZHJhN3h4X2FkZF9wY2llX3BvcnQoZHJhN3h4LCBwZGV2KTsNCiAgICAgICAgICAgICAgICBp ZiAocmV0IDwgMCkNCiAgICAgICAgICAgICAgICAgICAgICAgIGdvdG8gZXJyX2dwaW87DQogICAg ICAgICAgICAgICAgYnJlYWs7DQoNCg0KPiA+Pj4NCj4gPj4+IFRvIGF2b2lkIHRoaXMga2luZCBv ZiBsb29waW5nLCBzaG91bGRuJ3Qgd2UgYmUgZGlzYWJsaW5nIGFsbCBJUlENCj4gPj4+IGV2ZW50 cyB3aGlsZSB0aGUgaW50ZXJydXB0IGhhbmRsZXIgaXMgcnVubmluZyBhbmQgZW5hYmxlIHRoZW0g anVzdA0KPiA+Pj4gYmVmb3JlIHdlIHJldHVybiBmcm9tIHRoZSBoYXJkaXJxIGhhbmRsZXI/DQo+ ID4+DQo+ID4+IElJVUMsIHlvdSBhcmUgc2F5aW5nIHRvIGRpc2FibGUgYWxsIE1TSXMgYXQgUENJ ZSBkZXNpZ253YXJlIGNvcmUNCj4gPj4gbGV2ZWwsIHRoZW4gY2FsbCBkd19oYW5kbGVfbXNpX2ly cSgpIGFuZCB0aGVuIGVuYWJsZSBNU0lzIGFmdGVyDQo+ID4+IGhhcmRpcnEgcmV0dXJucy4gQnV0 LCB0aGUgcHJvYmxlbSBpcyBpZiBQQ0llIEVQIHJhaXNlcyBhbm90aGVyIE1TSQ0KPiA+PiBhZnRl ciB0aGUgY2FsbCB0byBFUCdzIGhhbmRsZXIgYnV0IGJlZm9yZSByZS1lbmFibGluZyBNU0lzLCB0 aGVuIGl0DQo+ID4+IHdpbGwgYmUgaWdub3JlZCBhcyBJUlFzIGFyZSBub3QgeWV0IGVuYWJsZWQu DQo+ID4+IElkZWFsbHksIEVQJ3Mgc3VwcG9ydCBQZXIgVmVjdG9yIE1hc2tpbmcoUFZNKSB3aGlj aCBhbGxvdyBSQyB0bw0KPiA+PiBwcmV2ZW50IEVQIGZyb20gc2VuZGluZyBNU0kgbWVzc2FnZXMg Zm9yIHNvbWV0aW1lLiBCdXQsDQo+ID4+IHVuZm9ydHVuYXRlbHksIHRoZSBjYXJkcyBtZW50aW9u ZWQgaGVyZSBkb24ndCBzdXBwb3J0IHRoaXMgZmVhdHVyZS4NCj4gPg0KPiA+IEknbSBub3QgYXdh cmUgb2YgTVNJcy4NCj4gPg0KPiA+IEJ1dCBmb3IgYW55IHR5cGljYWwgaGFyZHdhcmUsIHRoZXJl IHNob3VsZCBiZSBhbiBpbnRlcnJ1cHQgZXZlbnQNCj4gPiBlbmFibGUgcmVnaXN0ZXIgYW5kIGFu IGludGVycnVwdCBtYXNrIHJlZ2lzdGVyLg0KPiA+DQo+ID4gSW4gdGhlIElSUSBoYW5kbGVyLCB3 ZSBtYXNrIHRoZSBpbnRlcnJ1cHQgYnV0IHN0aWxsIGtlZXAgdGhlIGludGVycnVwdA0KPiA+IGV2 ZW50cyBlbmFibGVkIHNvIHRoYXQgdGhleSBjYW4gYmUgbGF0Y2hlZCBkdXJpbmcgdGhlIHRpbWUg dGhlIGludGVycnVwdCB3YXMNCj4gbWFza2VkLg0KPiA+DQo+ID4gV2hlbiB0aGUgaW50ZXJydXB0 IGlzIHVubWFza2VkIGF0IGVuZCBvZiB0aGUgSVJRIGhhbmRsZXIsIGl0IHNob3VsZA0KPiA+IHJl LXRyaWdnZXIgdGhlIGludGVycnVwdCBpZiBhbnkgZXZlbnRzIHdlcmUgbGF0Y2hlZCBhbmQgcGVu ZGluZy4NCj4gPg0KPiA+IFRoaXMgd2F5IHlvdSBkb24ndCBuZWVkIHRvIGtlZXAgY2hlY2tpbmcg Zm9yIGFueSBwZW5kaW5nIGV2ZW50cyBpbiB0aGUgSVJRDQo+IGhhbmRsZXIuDQo+ID4NCj4gDQo+ IFRoYW5rcyBmb3IgdGhlIHN1Z2dlc3Rpb24hIEkgdHJpZWQgdXNpbmcgaW50ZXJydXB0IG1hc2tp bmcgYXQgZGVzaWdud2FyZSBsZXZlbC4NCj4gQnV0LCB1bmZvcnR1bmF0ZWx5IHRoYXQgZG9lcyBu b3QgaGVscCBhbmQgbXkgdGVzdCBjYXNlcyBzdGlsbCBmYWlsLg0KPiBTZWVtcyBsaWtlIGRlc2ln bndhcmUgTVNJIHN0YXR1cyByZWdpc3RlciBpcyBhIG5vbi1tYXNrZWQgc3RhdHVzIGFuZCBkcmE3 eHgNCj4gc3BlY2lmaWMgd3JhcHBlciBzZWVtcyB0byBiZSByZWx5aW5nIG9uIHRoaXMgbm9uLW1h c2tlZCBzdGF0dXMgdG8gcmFpc2UNCj4gSVJRKGluc3RlYWQgb2YgYWN0dWFsIElSUSBzaWduYWwg b2YgZGVzaWdud2FyZSkgdG8gQ1BVLiBUaGVyZSBpcyB2ZXJ5IGxpdHRsZQ0KPiBkb2N1bWVudGF0 aW9uIGluIHRoZSBUUk0gd3J0IGhvdyB3cmFwcGVyIGZvcndhcmRzIGRlc2lnbndhcmUgSVJRIHN0 YXR1cyB0bw0KPiBDUFUuDQo+IA0KPiBTbywgYXQgYmVzdCwgSSB3aWxsIGFkZCBhIGNoZWNrIGlu IHRoZSBhYm92ZSB3aGlsZSgpIGxvb3AgYW5kIGJyZWFrIGFuZCBleGl0IElSUQ0KPiBoYW5kbGVy LCBsZXRzIHNheSBhZnRlciAxSyBsb29wcy4NCj4gDQo+IA0KPiAtLQ0KPiBSZWdhcmRzDQo+IFZp Z25lc2gNCg== ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2017-11-21 14:50 ` Chris Welch @ 2017-11-22 8:59 ` Vignesh R 2017-11-22 10:41 ` Vignesh R 1 sibling, 0 replies; 11+ messages in thread From: Vignesh R @ 2017-11-22 8:59 UTC (permalink / raw) To: Chris Welch, Quadros, Roger Cc: linux-usb@vger.kernel.org, linux-pci@vger.kernel.org, Joao Pinto, KISHON VIJAY ABRAHAM, Sekhar Nori On Tuesday 21 November 2017 08:20 PM, Chris Welch wrote: > > >> -----Original Message----- >> From: Vignesh R [mailto:vigneshr@ti.com] >> Sent: Tuesday, November 21, 2017 12:48 AM >> To: Roger Quadros <rogerq@ti.com> >> Cc: Chris Welch <Chris.Welch@viavisolutions.com>; linux-usb@vger.kernel.org; >> linux-pci@vger.kernel.org; Joao Pinto <jpinto@synopsys.com>; KISHON VIJAY >> ABRAHAM <kishon@ti.com> >> 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. Thanks for testing! I will submit patches formally with some minor tweaks. > > 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. Yeah, this is a known issue and has been discussed[1] before. We are exploring better solution > > 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! Thanks for pointing that out, will submit a patch for errata workaround. [1] https://lkml.org/lkml/2016/9/14/241 Regards Vignesh > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2017-11-21 14:50 ` Chris Welch 2017-11-22 8:59 ` Vignesh R @ 2017-11-22 10:41 ` Vignesh R 2017-11-22 14:32 ` Chris Welch 1 sibling, 1 reply; 11+ messages in thread From: Vignesh R @ 2017-11-22 10:41 UTC (permalink / raw) To: Chris Welch, Quadros, Roger Cc: linux-usb@vger.kernel.org, linux-pci@vger.kernel.org, Joao Pinto, KISHON VIJAY ABRAHAM On Tuesday 21 November 2017 08:20 PM, Chris Welch wrote: > > >> -----Original Message----- >> From: Vignesh R [mailto:vigneshr@ti.com] >> Sent: Tuesday, November 21, 2017 12:48 AM >> To: Roger Quadros <rogerq@ti.com> >> Cc: Chris Welch <Chris.Welch@viavisolutions.com>; linux-usb@vger.kernel.org; >> linux-pci@vger.kernel.org; Joao Pinto <jpinto@synopsys.com>; KISHON VIJAY >> ABRAHAM <kishon@ti.com> >> 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! BTW, do you have a test case which fails w/o errata i870 workaround? -- Regards Vignesh ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2017-11-22 10:41 ` Vignesh R @ 2017-11-22 14:32 ` Chris Welch 0 siblings, 0 replies; 11+ messages in thread From: Chris Welch @ 2017-11-22 14:32 UTC (permalink / raw) To: Vignesh R, Quadros, Roger Cc: linux-usb@vger.kernel.org, linux-pci@vger.kernel.org, Joao Pinto, KISHON VIJAY ABRAHAM PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBWaWduZXNoIFIgW21haWx0bzp2 aWduZXNockB0aS5jb21dDQo+IFNlbnQ6IFdlZG5lc2RheSwgTm92ZW1iZXIgMjIsIDIwMTcgNTo0 MSBBTQ0KPiBUbzogQ2hyaXMgV2VsY2ggPENocmlzLldlbGNoQHZpYXZpc29sdXRpb25zLmNvbT47 IFF1YWRyb3MsIFJvZ2VyDQo+IDxyb2dlcnFAdGkuY29tPg0KPiBDYzogbGludXgtdXNiQHZnZXIu a2VybmVsLm9yZzsgbGludXgtcGNpQHZnZXIua2VybmVsLm9yZzsgSm9hbyBQaW50bw0KPiA8anBp bnRvQHN5bm9wc3lzLmNvbT47IEtJU0hPTiBWSUpBWSBBQlJBSEFNIDxraXNob25AdGkuY29tPg0K PiBTdWJqZWN0OiBSZTogeGhjaV9oY2QgSEMgZGllZDsgY2xlYW5pbmcgdXAgd2l0aCBUVVNCNzM0 MCBhbmQgwrVQRDcyMDIwMQ0KPiANCj4gDQo+IA0KPiBPbiBUdWVzZGF5IDIxIE5vdmVtYmVyIDIw MTcgMDg6MjAgUE0sIENocmlzIFdlbGNoIHdyb3RlOg0KPiA+DQo+ID4NCj4gPj4gLS0tLS1Pcmln aW5hbCBNZXNzYWdlLS0tLS0NCj4gPj4gRnJvbTogVmlnbmVzaCBSIFttYWlsdG86dmlnbmVzaHJA dGkuY29tXQ0KPiA+PiBTZW50OiBUdWVzZGF5LCBOb3ZlbWJlciAyMSwgMjAxNyAxMjo0OCBBTQ0K PiA+PiBUbzogUm9nZXIgUXVhZHJvcyA8cm9nZXJxQHRpLmNvbT4NCj4gPj4gQ2M6IENocmlzIFdl bGNoIDxDaHJpcy5XZWxjaEB2aWF2aXNvbHV0aW9ucy5jb20+Ow0KPiA+PiBsaW51eC11c2JAdmdl ci5rZXJuZWwub3JnOyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBKb2FvIFBpbnRvDQo+ID4+ IDxqcGludG9Ac3lub3BzeXMuY29tPjsgS0lTSE9OIFZJSkFZIEFCUkFIQU0gPGtpc2hvbkB0aS5j b20+DQo+ID4+IFN1YmplY3Q6IFJlOiB4aGNpX2hjZCBIQyBkaWVkOyBjbGVhbmluZyB1cCB3aXRo IFRVU0I3MzQwIGFuZA0KPiA+PiDCtVBENzIwMjAxDQo+ID4+DQo+ID4+DQo+ID4+DQo+ID4+IE9u IE1vbmRheSAyMCBOb3ZlbWJlciAyMDE3IDA3OjAxIFBNLCBSb2dlciBRdWFkcm9zIHdyb3RlOg0K PiA+Pj4gT24gMjAvMTEvMTcgMTU6MTksIFZpZ25lc2ggUiB3cm90ZToNCj4gPj4+Pg0KPiA+Pj4+ DQo+ID4+Pj4gT24gTW9uZGF5IDIwIE5vdmVtYmVyIDIwMTcgMDE6MzEgUE0sIFJvZ2VyIFF1YWRy b3Mgd3JvdGU6DQo+ID4+Pj4gWy4uLl0NCj4gPj4+Pj4+DQo+ID4+Pj4+PiBTbywgY291bGQgeW91 IHRyeSByZXZlcnRpbmcgY29tbWl0IDhjOTM0MDk1ZmEyZjMgYW5kIGFsc28gYXBwbHkNCj4gPj4+ Pj4+IGJlbG93IHBhdGNoIGFuZCBsZXQgbWUga25vdyBpZiB0aGF0IGZpeGVzIHRoZSBpc3N1ZT8N Cj4gPj4+Pj4+DQo+ID4+Pj4+PiAtLS0tLS0tLS0tLQ0KPiA+Pj4+Pj4NCj4gPj4+Pj4+IGRpZmYg LS1naXQgYS9kcml2ZXJzL3BjaS9kd2MvcGNpLWRyYTd4eC5jDQo+ID4+Pj4+PiBiL2RyaXZlcnMv cGNpL2R3Yy9wY2ktZHJhN3h4LmMgaW5kZXggZTc3YTRjZWVkNzRjLi44MjgwYWJjNTZmMzANCj4g Pj4+Pj4+IDEwMDY0NA0KPiA+Pj4+Pj4gLS0tIGEvZHJpdmVycy9wY2kvZHdjL3BjaS1kcmE3eHgu Yw0KPiA+Pj4+Pj4gKysrIGIvZHJpdmVycy9wY2kvZHdjL3BjaS1kcmE3eHguYw0KPiA+Pj4+Pj4g QEAgLTI1OSwxMCArMjU5LDE3IEBAIHN0YXRpYyBpcnFyZXR1cm5fdA0KPiA+PiBkcmE3eHhfcGNp ZV9tc2lfaXJxX2hhbmRsZXIoaW50IGlycSwgdm9pZCAqYXJnKQ0KPiA+Pj4+Pj4gICAgICAgICB1 MzIgcmVnOw0KPiA+Pj4+Pj4NCj4gPj4+Pj4+ICAgICAgICAgcmVnID0gZHJhN3h4X3BjaWVfcmVh ZGwoZHJhN3h4LA0KPiA+Pj4+Pj4gUENJRUNUUkxfRFJBN1hYX0NPTkZfSVJRU1RBVFVTX01TSSk7 DQo+ID4+Pj4+PiArICAgICAgIGRyYTd4eF9wY2llX3dyaXRlbChkcmE3eHgsDQo+ID4+Pj4+PiAr IFBDSUVDVFJMX0RSQTdYWF9DT05GX0lSUVNUQVRVU19NU0ksIHJlZyk7DQo+ID4+Pj4+Pg0KPiA+ Pj4+Pj4gICAgICAgICBzd2l0Y2ggKHJlZykgew0KPiA+Pj4+Pj4gICAgICAgICBjYXNlIE1TSToN Cj4gPj4+Pj4+IC0gICAgICAgICAgICAgICBkd19oYW5kbGVfbXNpX2lycShwcCk7DQo+ID4+Pj4+ PiArICAgICAgICAgICAgICAgLyoNCj4gPj4+Pj4+ICsgICAgICAgICAgICAgICAgKiBOZWVkIHRv IG1ha2Ugc3VyZSBubyBNU0kgSVJRcyBhcmUgcGVuZGluZyBiZWZvcmUNCj4gPj4+Pj4+ICsgICAg ICAgICAgICAgICAgKiBleGl0aW5nIGhhbmRsZXIsIGVsc2UgdGhlIHdyYXBwZXIgd2lsbCBub3Qg Y2F0Y2ggbmV3DQo+ID4+Pj4+PiArICAgICAgICAgICAgICAgICogSVJRcy4gU28gbG9vcCBhcm91 bmQgdGlsbCBkd19oYW5kbGVfbXNpX2lycSgpIHJldHVybnMNCj4gPj4+Pj4+ICsgICAgICAgICAg ICAgICAgKiBJUlFfTk9ORQ0KPiA+Pj4+Pj4gKyAgICAgICAgICAgICAgICAqLw0KPiA+Pj4+Pj4g KyAgICAgICAgICAgICAgIHdoaWxlIChkd19oYW5kbGVfbXNpX2lycShwcCkgIT0gSVJRX05PTkUp Ow0KPiA+DQo+ID4gVGhlIHBhdGNoIGxvb2tzIGdvb2QsIEkgaGF2ZW4ndCBoYWQgYSBmYWlsdXJl IGluIGEgZmV3IGRheXMgb2YgdGVzdGluZy4NCj4gPg0KPiA+IFlvdSBzaG91bGQgYWxzbyBsb29r IGF0IGluY29ycG9yYXRpbmcgdGhlIGZvbGxvd2luZyB0aGF0IEkgbmVlZGVkIHRvIGNoYW5nZSB0 bw0KPiBnZXQgb3VyIHByb2R1Y3Qgd29ya2luZy4gIFRoZSBmaXJzdCBjaGFuZ2UgZml4ZXMgYSBt aXNzIGJ5IG9uZSBlcnJvciB3aXRoIHRoZQ0KPiBpbnRlcnJ1cHQgbGluZXMuDQo+ID4NCj4gPiBU aGUgc2Vjb25kIGNoYW5nZSBleHRlbmRzIGEgcGF0Y2ggeW91IGRldmVsb3BlZCBmb3IgZXJyYXRh IGk4NzAgYnV0IHdlDQo+IGZvdW5kIGlzIGFwcGxpY2FibGUgdG8gUkMgb3BlcmF0aW9uIGFzIHdl bGwgYXMgRVBzLiAgVGhhbmtzIHZlcnkgbXVjaCBmb3IgeW91cg0KPiBoZWxwIQ0KPiANCj4gQlRX LCBkbyB5b3UgaGF2ZSBhIHRlc3QgY2FzZSB3aGljaCBmYWlscyB3L28gZXJyYXRhIGk4NzAgd29y a2Fyb3VuZD8NCg0KWWVzLCB3ZSBoYXZlIGEgUEVYODYwNiBQQ0kgc3dpdGNoIGF0dGFjaGVkIHRv IHRoZSBSQyB3aGljaCBmYWlscyB0byBwcm9iZSBpZiB0aGUgaTg3MCB3b3JrYXJvdW5kIGlzIG5v dCB1c2VkLg0KDQpUaGUgY29kZSBwaWNrcyB1cCB0aGUgd3JvbmcgY2xhc3MgZm9yIHRoZSBzd2l0 Y2ggaWYgdGhlIHdvcmsgYXJvdW5kIGlzIG5vdCBpbiBwbGFjZS4NCg0KPiANCj4gDQo+IC0tDQo+ IFJlZ2FyZHMNCj4gVmlnbmVzaA0K ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2017-11-16 12:26 ` xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 Vignesh R 2017-11-20 8:01 ` Roger Quadros @ 2018-02-08 8:54 ` Niklas Cassel 2018-02-08 12:52 ` Vignesh R 1 sibling, 1 reply; 11+ messages in thread From: Niklas Cassel @ 2018-02-08 8:54 UTC (permalink / raw) To: Vignesh R, Quadros, Roger Cc: Chris Welch, linux-usb@vger.kernel.org, linux-pci, Joao Pinto, KISHON VIJAY ABRAHAM, Rabin Vincent, faiz_abbas On 16/11/17 13:26, Vignesh R wrote: > +linux-pci > > Hi Chris, > > On Thursday 16 November 2017 05:20 PM, Quadros, Roger wrote: >> +Vignesh >> >> On 13/09/17 17:26, Chris Welch wrote: >>> We are developing a product based on the TI AM5728 EVM. The product utilizes a TUSB7340 PCIe USB host for additional ports. The TUSB7340 is detected and setup properly and works OK with low data rate devices. However, hot plugging a Realtek USB network adapter and doing Ethernet transfer bandwidth testing using iperf3 causes the TUSB7340 host to be locked out. The TUSB7340 host appears to no longer communicate and the logging indicates xhci_hcd 0000:01:00.0: HC died; cleaning up. Same issue occurs with another USB Ethernet adapter I tried (Asus). >>> >>> We looked at using another host and found a mini PCIe card that utilizes the µPD720201 and can be directly installed on the TI AM5728 EVM. The card is detected properly and we reran the transfer test. The uPD720201 gets locks out with the same problem. >>> >>> The AM5728 testing was performed using the TI SD card stock am57xx-evm-linux-04.00.00.04.img, kernel am57xx-evm 4.9.28-geed43d1050, and it reports that it is using the TI AM572x EVM Rev A3 device tree. >>> >>> It shows the following logging when it fails (this is with the TI EVM and uPD720201). >>> >>> [ 630.400899] xhci_hcd 0000:01:00.0: xHCI host not responding to stop endpoint command. >>> [ 630.408769] xhci_hcd 0000:01:00.0: Assuming host is dying, halting host. >>> [ 630.420849] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>> [ 630.425667] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>> [ 630.430483] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>> [ 630.435297] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>> [ 630.440122] xhci_hcd 0000:01:00.0: HC died; cleaning up >>> [ 630.453961] usb 2-4: USB disconnect, device number 2 >>> >>> The problem appears to be a general driver issue given we get the same problem with both the TUSB7340 and the µPD720201. > > Seems like PCIe driver is missing MSI IRQs leading to stall. > Reading xHCI registers via PCIe mem space confirms this. > > I see two problems wrt MSI handling: > Since commit 8c934095fa2f3 ("PCI: dwc: Clear MSI interrupt status after it is handled, not before"), > dwc clears MSI status after calling EP's IRQ handler. But, it happens that another MSI interrupt is > raised just at the end of EP's IRQ handler and before clearing MSI status. > This will result in loss of new MSI IRQ as we clear the MSI IRQ status without handling. > > Another problem appears to be wrt dra7xx PCIe wrapper: > PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI does not seem to catch MSI IRQs unless, > its ensured that PCIE_MSI_INTR0_STATUS register read returns 0. > > So, could you try reverting commit 8c934095fa2f3 and > also apply below patch and let me know if that fixes the issue? Hello Vignesh, It is not only dra7xx that is affected by this bug, all other DWC based drivers as well. I suggest that we either: Revert 8c934095fa2f ("PCI: dwc: Clear MSI interrupt status after it is handled, not before") or You implement a generic solution (i.e. in drivers/pci/dwc/pcie-designware-host.c) rather than just a dra7xx specific solution. Regards, Niklas > > ----------- > > 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); > break; > case INTA: > case INTB: > @@ -273,8 +280,6 @@ static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg) > break; > } > > - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI, reg); > - > return IRQ_HANDLED; > } > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 2018-02-08 8:54 ` Niklas Cassel @ 2018-02-08 12:52 ` Vignesh R 0 siblings, 0 replies; 11+ messages in thread From: Vignesh R @ 2018-02-08 12:52 UTC (permalink / raw) To: Niklas Cassel, Quadros, Roger Cc: linux-usb@vger.kernel.org, linux-pci, Joao Pinto, KISHON VIJAY ABRAHAM, Rabin Vincent, faiz_abbas On Thursday 08 February 2018 02:24 PM, Niklas Cassel wrote: > On 16/11/17 13:26, Vignesh R wrote: >> +linux-pci >> >> Hi Chris, >> >> On Thursday 16 November 2017 05:20 PM, Quadros, Roger wrote: >>> +Vignesh >>> >>> On 13/09/17 17:26, Chris Welch wrote: >>>> We are developing a product based on the TI AM5728 EVM. The product utilizes a TUSB7340 PCIe USB host for additional ports. The TUSB7340 is detected and setup properly and works OK with low data rate devices. However, hot plugging a Realtek USB network adapter and doing Ethernet transfer bandwidth testing using iperf3 causes the TUSB7340 host to be locked out. The TUSB7340 host appears to no longer communicate and the logging indicates xhci_hcd 0000:01:00.0: HC died; cleaning up. Same issue occurs with another USB Ethernet adapter I tried (Asus). >>>> >>>> We looked at using another host and found a mini PCIe card that utilizes the µPD720201 and can be directly installed on the TI AM5728 EVM. The card is detected properly and we reran the transfer test. The uPD720201 gets locks out with the same problem. >>>> >>>> The AM5728 testing was performed using the TI SD card stock am57xx-evm-linux-04.00.00.04.img, kernel am57xx-evm 4.9.28-geed43d1050, and it reports that it is using the TI AM572x EVM Rev A3 device tree. >>>> >>>> It shows the following logging when it fails (this is with the TI EVM and uPD720201). >>>> >>>> [ 630.400899] xhci_hcd 0000:01:00.0: xHCI host not responding to stop endpoint command. >>>> [ 630.408769] xhci_hcd 0000:01:00.0: Assuming host is dying, halting host. >>>> [ 630.420849] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>>> [ 630.425667] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>>> [ 630.430483] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>>> [ 630.435297] r8152 2-4:1.0 enp1s0u4: Tx status -108 >>>> [ 630.440122] xhci_hcd 0000:01:00.0: HC died; cleaning up >>>> [ 630.453961] usb 2-4: USB disconnect, device number 2 >>>> >>>> The problem appears to be a general driver issue given we get the same problem with both the TUSB7340 and the µPD720201. >> >> Seems like PCIe driver is missing MSI IRQs leading to stall. >> Reading xHCI registers via PCIe mem space confirms this. >> >> I see two problems wrt MSI handling: >> Since commit 8c934095fa2f3 ("PCI: dwc: Clear MSI interrupt status after it is handled, not before"), >> dwc clears MSI status after calling EP's IRQ handler. But, it happens that another MSI interrupt is >> raised just at the end of EP's IRQ handler and before clearing MSI status. >> This will result in loss of new MSI IRQ as we clear the MSI IRQ status without handling. >> >> Another problem appears to be wrt dra7xx PCIe wrapper: >> PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI does not seem to catch MSI IRQs unless, >> its ensured that PCIE_MSI_INTR0_STATUS register read returns 0. >> >> So, could you try reverting commit 8c934095fa2f3 and >> also apply below patch and let me know if that fixes the issue? > > Hello Vignesh, > > It is not only dra7xx that is affected by this bug, > all other DWC based drivers as well. > > I suggest that we either: > Revert 8c934095fa2f ("PCI: dwc: Clear MSI interrupt status after it is handled, > not before") I will send a revert soon. > or > You implement a generic solution > (i.e. in drivers/pci/dwc/pcie-designware-host.c) > rather than just a dra7xx specific solution. Reverting 8c934095fa2f should unblock all other DWC drivers. For dra7xx, I will send separate patches to fix TI dra7xx specific wrapper level IRQ handler. >> ----------- >> >> 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); >> break; >> case INTA: >> case INTB: >> @@ -273,8 +280,6 @@ static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg) >> break; >> } >> >> - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI, reg); >> - >> return IRQ_HANDLED; >> } >> >> >> >> -- Regards Vignesh ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-02-08 12:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <BN6PR18MB124994D85EAC4B5B1AD5EC56866E0@BN6PR18MB1249.namprd18.prod.outlook.com>
[not found] ` <3dd7a4fc-da86-03cc-9b01-a0d29dd73230@ti.com>
2017-11-16 12:26 ` xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201 Vignesh R
2017-11-20 8:01 ` Roger Quadros
2017-11-20 13:19 ` Vignesh R
2017-11-20 13:31 ` Roger Quadros
2017-11-21 5:47 ` Vignesh R
2017-11-21 14:50 ` Chris Welch
2017-11-22 8:59 ` Vignesh R
2017-11-22 10:41 ` Vignesh R
2017-11-22 14:32 ` Chris Welch
2018-02-08 8:54 ` Niklas Cassel
2018-02-08 12:52 ` Vignesh R
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).