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
