Re: [PATCH v2] xhci: Add quirk to workaround the errata seen on Cavium Thunder-X2 Soc

[George Cherian]

Hi Mathias,

Thanks for the review.

On 10/30/2018 03:20 PM, Mathias Nyman wrote:
> 
> On 29.10.2018 18:54, Cherian, George wrote:
>> Implement workaround for ThunderX2 Errata-129 (documented in
>> CN99XX Known Issues" available at Cavium support site).
>> As per ThunderX2errata-129, USB 2 device may come up as USB 1
>> if a connection to a USB 1 device is followed by another connection to
>> a USB 2 device, the link will come up as USB 1 for the USB 2 device.
>>
>> Resolution: Reset the PHY after the USB 1 device is disconnected.
>> The PHY reset sequence is done using private registers in XHCI register
>> space. After the PHY is reset we check for the PLL lock status and retry
>> the operation if it fails. From our tests, retrying 4 times is 
>> sufficient.
>>
>> Add a new quirk flag XHCI_RESET_PLL_ON_DISCONNECT to invoke the 
>> workaround
>> in handle_xhci_port_status().
>>
>> Signed-off-by: George Cherian <george.cherian@xxxxxxxxxx>
>> ---
>>  drivers/usb/host/xhci-pci.c | 5 +++++
>> Â drivers/usb/host/xhci-ring.c | 35 ++++++++++++++++++++++++++++++++++-
>> Â drivers/usb/host/xhci.hÂÂÂÂÂ |Â 1 +
>> Â 3 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 51dd8e0..334c009 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -231,6 +231,11 @@ static void xhci_pci_quirks(struct device *dev, 
>> struct xhci_hcd *xhci)
>> ÂÂÂÂÂ if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
>>
>> +ÂÂÂÂ if ((pdev->vendor == PCI_VENDOR_ID_BROADCOM ||
>> +ÂÂÂÂÂÂÂÂÂ pdev->vendor == PCI_VENDOR_ID_CAVIUM) &&
>> +ÂÂÂÂÂÂÂÂÂ pdev->device == 0x9026)
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ xhci->quirks |= XHCI_RESET_PLL_ON_DISCONNECT;
>> +
>> ÂÂÂÂÂ if (xhci->quirks & XHCI_RESET_ON_RESUME)
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "QUIRK: Resetting on resume");
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index f0a99aa..342dd5ac 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1517,6 +1517,35 @@ static void handle_device_notification(struct 
>> xhci_hcd *xhci,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ usb_wakeup_notification(udev->parent, udev->portnum);
>> Â }
>>
>> +/*
>> + * Quirk hanlder for errata seen on Cavium ThunderX2 processor XHCI
>> + * Controller.
>> + * As per ThunderX2errata-129 USB 2 device may come up as USB 1
>> + * If a connection to a USB 1 device is followed by another connection
>> + * to a USB 2 device.
>> + *
>> + * Reset the PHY after the USB device is disconnected if device speed
>> + * is less than HCD_USB3.
>> + * Retry the reset sequence max of 4 times checking the PLL lock status.
>> + *
>> + */
>> +static void xhci_handle_tx2_wrapper_reset(struct xhci_hcd *xhci)
> 
> Maybe rename this to make it more obvious it's about a quirk.
> xhci_cavium_reset_phy_quirk() maybe?
Yes will do
> 
>> +{
>> +ÂÂÂÂ struct usb_hcd *hcd = xhci_to_hcd(xhci);
>> +ÂÂÂÂ u32 pll_lock_check;
>> +ÂÂÂÂ u32 retry_count = 4;
>> +
>> +ÂÂÂÂ do {
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ /* Assert PHY reset */
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ writel(0x6F, hcd->regs + 0x1048);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ udelay(10);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ /* De-assert the PHY reset */
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ writel(0x7F, hcd->regs + 0x1048);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ udelay(200);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ pll_lock_check = readl(hcd->regs + 0x1070);
>> +ÂÂÂÂ } while (!(pll_lock_check & 0x1) && --retry_count);
>> +}
>> +
>> Â static void handle_port_status(struct xhci_hcd *xhci,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ union xhci_trb *event)
>> Â {
>> @@ -1642,8 +1671,12 @@ static void handle_port_status(struct xhci_hcd 
>> *xhci,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ goto cleanup;
>> ÂÂÂÂÂ }
>>
>> -ÂÂÂÂ if (hcd->speed < HCD_USB3)
>> +ÂÂÂÂ if (hcd->speed < HCD_USB3) {
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ xhci_test_and_clear_bit(xhci, port, PORT_PLC);
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ if ((portsc & PORT_CSC) && !(portsc & 0x1) &&
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ (xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT))
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ xhci_handle_tx2_wrapper_reset(xhci);
>> +ÂÂÂÂ }
> 
> Check for the quirk first, this way I can ignore the rest of the code when
> debugging other issues, and use PORT_CONNECT instead of 0x1
> 
> something like:
> 
> if ((xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT) &&
>  ÂÂÂ (portsc & PORT_CSC) && !(portsc & PORT_CONNECT))

Yes I will do the changes and post new version.
> 
> Thanks
> -Mathias

Regards
-George