Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Vignesh R]

+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

Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Roger Quadros]

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

Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Vignesh R]

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

Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Roger Quadros]

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

Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Vignesh R]

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

RE: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Chris Welch]

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==

Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Vignesh R]

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

Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Vignesh R]

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

RE: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Chris Welch]

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

Re: Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Niklas Cassel]

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;
>  }
> 
> 
> 
> 

Re: xhci_hcd HC died; cleaning up with TUSB7340 and µPD720201

[Vignesh R]

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