Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Retry device slot init on failure

["Wu, Hao A" <hao.a.wu@intel.com>]

Hello Heng,

It looks to me that the patch is addressing a similar issue you met on the UpXtreme board mentioned in:
https://bugzilla.tianocore.org/show_bug.cgi?id=3007

Could you help to check if such retry during the slot initialization helps for the USB device on your board?
Thanks in advance.

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeff
> Brasen
> Sent: Wednesday, October 21, 2020 11:45 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Jon
> Hunter <jonathanh@nvidia.com>; Jeff Brasen <jbrasen@nvidia.com>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Retry device slot
> init on failure
> 
> From: Jon Hunter <jonathanh@nvidia.com>
> 
> With some super-speed USB mass storage devices it has been observed that
> a USB transaction error may occur when attempting the set the device
> address during enumeration.
> 
> According the the xHCI specification (section 4.6.5) ...
> 
> "A USB Transaction ErrorCompletion Code for an Address Device Command
> may be due to a Stall response from a device. Software should issue a
> Disable Slot Commandfor the Device Slot then an Enable Slot Command  to
> recover from this error."
> 
> To fix this, retry the device slot initialization if it fails due to a device error.
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 71 ++++++++++++++--------
> --
>  1 file changed, 42 insertions(+), 29 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 9cb115363c..1a16864205 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -1717,9 +1717,11 @@ XhcPollPortStatusChange (
>    EFI_STATUS        Status;
>    UINT8             Speed;
>    UINT8             SlotId;
> +  UINT8             Retries;
>    USB_DEV_ROUTE     RouteChart;
> 
>    Status = EFI_SUCCESS;
> +  Retries = 1;
> 
>    if ((PortState->PortChangeStatus & (USB_PORT_STAT_C_CONNECTION |
> USB_PORT_STAT_C_ENABLE | USB_PORT_STAT_C_OVERCURRENT |
> USB_PORT_STAT_C_RESET)) == 0) {
>      return EFI_SUCCESS;
> @@ -1739,40 +1741,51 @@ XhcPollPortStatusChange (
>      RouteChart.Route.TierNum       = ParentRouteChart.Route.TierNum + 1;
>    }
> 
> -  SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
> -  if (SlotId != 0) {
> -    if (Xhc->HcCParams.Data.Csz == 0) {
> -      Status = XhcDisableSlotCmd (Xhc, SlotId);
> -    } else {
> -      Status = XhcDisableSlotCmd64 (Xhc, SlotId);
> -    }
> -  }
> -
> -  if (((PortState->PortStatus & USB_PORT_STAT_ENABLE) != 0) &&
> -      ((PortState->PortStatus & USB_PORT_STAT_CONNECTION) != 0)) {
> -    //
> -    // Has a device attached, Identify device speed after port is enabled.
> -    //
> -    Speed = EFI_USB_SPEED_FULL;
> -    if ((PortState->PortStatus & USB_PORT_STAT_LOW_SPEED) != 0) {
> -      Speed = EFI_USB_SPEED_LOW;
> -    } else if ((PortState->PortStatus & USB_PORT_STAT_HIGH_SPEED) != 0) {
> -      Speed = EFI_USB_SPEED_HIGH;
> -    } else if ((PortState->PortStatus & USB_PORT_STAT_SUPER_SPEED) != 0) {
> -      Speed = EFI_USB_SPEED_SUPER;
> -    }
> -    //
> -    // Execute Enable_Slot cmd for attached device, initialize device context
> and assign device address.
> -    //
> +  do {
>      SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
> -    if ((SlotId == 0) && ((PortState->PortChangeStatus &
> USB_PORT_STAT_C_RESET) != 0)) {
> +    if (SlotId != 0) {
>        if (Xhc->HcCParams.Data.Csz == 0) {
> -        Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart, Port,
> RouteChart, Speed);
> +        Status = XhcDisableSlotCmd (Xhc, SlotId);
>        } else {
> -        Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart, Port,
> RouteChart, Speed);
> +        Status = XhcDisableSlotCmd64 (Xhc, SlotId);
>        }
>      }
> -  }
> +
> +    if (((PortState->PortStatus & USB_PORT_STAT_ENABLE) != 0) &&
> +        ((PortState->PortStatus & USB_PORT_STAT_CONNECTION) != 0)) {
> +      //
> +      // Has a device attached, Identify device speed after port is enabled.
> +      //
> +      Speed = EFI_USB_SPEED_FULL;
> +      if ((PortState->PortStatus & USB_PORT_STAT_LOW_SPEED) != 0) {
> +        Speed = EFI_USB_SPEED_LOW;
> +      } else if ((PortState->PortStatus & USB_PORT_STAT_HIGH_SPEED) != 0) {
> +        Speed = EFI_USB_SPEED_HIGH;
> +      } else if ((PortState->PortStatus & USB_PORT_STAT_SUPER_SPEED) != 0)
> {
> +        Speed = EFI_USB_SPEED_SUPER;
> +      }
> +      //
> +      // Execute Enable_Slot cmd for attached device, initialize device context
> and assign device address.
> +      //
> +      SlotId = XhcRouteStringToSlotId (Xhc, RouteChart);
> +      if ((SlotId == 0) && ((PortState->PortChangeStatus &
> USB_PORT_STAT_C_RESET) != 0)) {
> +        if (Xhc->HcCParams.Data.Csz == 0) {
> +          Status = XhcInitializeDeviceSlot (Xhc, ParentRouteChart, Port,
> RouteChart, Speed);
> +        } else {
> +          Status = XhcInitializeDeviceSlot64 (Xhc, ParentRouteChart, Port,
> RouteChart, Speed);
> +        }
> +      }
> +    }
> +
> +    //
> +    // According to the xHCI specification (section 4.6.5), "a USB Transaction
> +    // Error Completion Code for an Address Device Command may be due to
> a Stall
> +    // response from a device. Software should issue a Disable Slot Command
> for
> +    // the Device Slot then an Enable Slot Command to recover from this
> error."
> +    // Therefore, retry the device slot initialization if it fails due to a
> +    // device error.
> +    //
> +  } while ((Status == EFI_DEVICE_ERROR) && (Retries--));
> 
>    return Status;
>  }
> --
> 2.25.1
> 
> 
> 
> 
>