[PATCH v1 0/1] MdeModulePkg:Add Warm Reset for Xhc

[PATCH v1 0/1] MdeModulePkg:Add Warm Reset for Xhc

[zhoucheng]

Description according to Chapter 7.5.2 of USB 3.2 spec protocol.
When the Usb state machine is in Inactive, the software is required 
to perform a warm reset operation.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Wang Jian J <jian.j.wang@intel.com>
Reviewed-by: Wu Hao A <hao.a.wu@intel.com>
Signed-off-by: Cheng Zhou <zhoucheng@phytium.com.cn>

zhoucheng (1):
  MdeModulePkg:Add Warm Reset for Xhc

 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
2.17.1

[PATCH v1 1/1] MdeModulePkg:Add Warm Reset for Xhc

[zhoucheng]

Description according to Chapter 7.5.2 of USB 3.2 spec protocol.
When the Usb state machine is in Inactive, the software is required
to perform a warm reset operation.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Cheng Zhou <zhoucheng@phytium.com.cn>
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index c05431ff30ec..938c8e2e28f7 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -558,6 +558,20 @@ XhcSetRootHubPortFeature (
       State |= XHC_PORTSC_RESET;
       XhcWriteOpReg (Xhc, Offset, State);
       XhcWaitOpRegBit (Xhc, Offset, XHC_PORTSC_PRC, TRUE, XHC_GENERIC_TIMEOUT);
+
+      //
+      // Usb 3.2 spec 7.5.2
+      // When the USB state machine is Inactive state, the device is abnormal.
+      // eSS.Inactive is a state where a link has failed Enhanced SuperSpeed operation.Software
+      // is required for warm reset intervention.This flag only applies to USB3 protocol ports.
+      //
+      State = XhcReadOpReg (Xhc, Offset);
+      if ((((State & 0x1e0) >> 5) == 6) && ((State & 3) == 0)) {
+        State |= 0x80000000;
+        XhcWriteOpReg (Xhc, Offset, State);
+        XhcWaitOpRegBit (Xhc, Offset, XHC_PORTSC_PRC, TRUE, XHC_GENERIC_TIMEOUT);
+        DEBUG ((DEBUG_INFO, "Warm Reset Successful! \n"));
+      }
       break;
 
     case EfiUsbPortPower:
-- 
2.17.1

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg:Add Warm Reset for Xhc

[Pedro Falcato]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

On Thu, Nov 3, 2022 at 3:53 PM zhoucheng <zhoucheng@phytium.com.cn> wrote:

> Description according to Chapter 7.5.2 of USB 3.2 spec protocol.
> When the Usb state machine is in Inactive, the software is required
> to perform a warm reset operation.
>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Cheng Zhou <zhoucheng@phytium.com.cn>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index c05431ff30ec..938c8e2e28f7 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -558,6 +558,20 @@ XhcSetRootHubPortFeature (
>        State |= XHC_PORTSC_RESET;
>        XhcWriteOpReg (Xhc, Offset, State);
>        XhcWaitOpRegBit (Xhc, Offset, XHC_PORTSC_PRC, TRUE,
> XHC_GENERIC_TIMEOUT);
> +
> +      //
> +      // Usb 3.2 spec 7.5.2
> +      // When the USB state machine is Inactive state, the device is
> abnormal.
> +      // eSS.Inactive is a state where a link has failed Enhanced
> SuperSpeed operation.Software
> +      // is required for warm reset intervention.This flag only applies
> to USB3 protocol ports.
> +      //
> +      State = XhcReadOpReg (Xhc, Offset);
> +      if ((((State & 0x1e0) >> 5) == 6) && ((State & 3) == 0)) {
> +        State |= 0x80000000;
>
Please use defines for all of these magic values, it improves readability
so much.

> +        XhcWriteOpReg (Xhc, Offset, State);
> +        XhcWaitOpRegBit (Xhc, Offset, XHC_PORTSC_PRC, TRUE,
> XHC_GENERIC_TIMEOUT);
> +        DEBUG ((DEBUG_INFO, "Warm Reset Successful! \n"));
> +      }
>        break;
>
>      case EfiUsbPortPower:
> --
> 2.17.1
>
>
>
> 
>
>
>

-- 
Pedro Falcato

[-- Attachment #2: Type: text/html, Size: 2865 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg:Add Warm Reset for Xhc

[zhoucheng]

[-- Attachment #1.1: Type: text/plain, Size: 1337 bytes --]

Hi Pedro:

//  The description here is the judgment state，we can watch to XHC PORTSC register.
// according to Xhc Spec 5.4.8 chapter Table39.we can knowleadge more information.
// We need to pay attention to the PED CCS and PLS bits of the PORTSC register.

+    State = XhcReadOpReg (Xhc, Offset);
+      if ((( (State & 0x1e0) >> 5) == 6) && ( (State & 3) == 0)) {
↓                                                   ↓
PLS bit                                   PED and CCS bit

//  PLS: Port Link State PED: Port Enabled/Disabled CCS: Current Connect Status
//  If the CCS bit and PED are 0, the device is disconnected and the port is not enabled.
//  If PLS is 6, the controller state machine is abnormal and software intervention is required.

// According to Usb3.2 spec 7.5.2 chapter.Inactive is a state where a link has failed Enhanced SuperSpeed operation.
// The software is required to perform a warm reset operation.
// Therefore, according to the Xhc1.1 spec 5.8 chapter Table39, we can write 1 to its highest bit and perform a hot reset operation to restore the controller.

+        State |= 0x80000000;

For the value of this block, the above explanation has been made, and the discussion can be continued at the same time.

Thanks!

[-- Attachment #1.2: Type: text/html, Size: 2107 bytes --]

[-- Attachment #2: dummyfile.0.part --]
[-- Type: image/png, Size: 38497 bytes --]

[-- Attachment #3: dummyfile.1.part --]
[-- Type: image/png, Size: 70313 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg:Add Warm Reset for Xhc

[Wu, Hao A]

[-- Attachment #1.1: Type: text/plain, Size: 2538 bytes --]

Really sorry for the delayed response. I have some questions.

Could you help to provide more information on the issue met that leads to this proposed code change? Also, what tests have been performed for the patch?

The XHCI spec mentions that WPR (bit31 in PORTSC register) only applies to USB3 protocol ports.
Does the “PLS == 6” check ensure the port supports USB3 protocol or additional checks (Port Speed field in PORTSC register + xHCI Supported Protocol Capability) should be added before issuing a Warm Reset?

The proposed code change adds “a eSS.Inactive check and performs a Warm Reset if the check satisfies” after a Hot Reset attempt.
Does the Hot Reset attempt required to resolve the issue? How about the below flow:
if eSS.Inactive
    Warm Reset (WPR=1 in PORTSC)
else
    Hot Reset (PR=1 in PORTSC)

Best Regards,
Hao Wu

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of zhoucheng
Sent: Friday, November 4, 2022 11:16 AM
To: Pedro Falcato <pedro.falcato@gmail.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg:Add Warm Reset for Xhc

Hi Pedro:
  //  The description here is the judgment state，we can watch to XHC PORTSC register.
  // according to Xhc Spec 5.4.8 chapter Table39.we can knowleadge more information.
  // We need to pay attention to the PED CCS and PLS bits of the PORTSC register.
+    State = XhcReadOpReg (Xhc, Offset);
+      if ((((State & 0x1e0) >> 5) == 6) && ((State & 3) == 0)) {
                        ↓                                                   ↓
                  PLS bit                                   PED and CCS bit

//  PLS: Port Link State    PED: Port Enabled/Disabled    CCS: Current Connect Status
//  If the CCS bit and PED are 0, the device is disconnected and the port is not enabled.
//  If PLS is 6, the controller state machine is abnormal and software intervention is required.

[cid:image001.png@01D8F9C8.94D8C800]

// According to Usb3.2 spec 7.5.2 chapter.Inactive is a state where a link has failed Enhanced SuperSpeed operation.
// The software is required to perform a warm reset operation.
// Therefore, according to the Xhc1.1 spec 5.8 chapter Table39, we can write 1 to its highest bit and perform a hot reset operation to restore the controller.
[cid:image002.png@01D8F9C8.94D8C800]

+        State |= 0x80000000;

   For the value of this block, the above explanation has been made, and the discussion can be continued at the same time.

Thanks!


[-- Attachment #1.2: Type: text/html, Size: 6508 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 38497 bytes --]

[-- Attachment #3: image002.png --]
[-- Type: image/png, Size: 70313 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg:Add Warm Reset for Xhc

[zhoucheng]

[-- Attachment #1.1: Type: text/plain, Size: 740 bytes --]

HI:
The printing test is added here. When the USB disk device is inserted, the repeated mass restart test will be carried out.There
will be a probability of disk loss. When the disk is dropped, the PORTSC register is read. The USB state machine is abnormal.
Its PLS field is 6. Check the protocol document, so warm reset it. The test is normal and no disk loss occurs again.

The left is normal, and the right is abnormal.
After XhcUsbPortReset.The normal PORTSC register status is 0x1203, EXCEPTION PORTSC register status is 0x2202c0.
The USB device has been disconnected.

After hot reset, the device can be connected normally.

In the source code, it is confirmed that Usb3.0 is used. Do you need to judge again?

Thanks!

[-- Attachment #1.2: Type: text/html, Size: 997 bytes --]

[-- Attachment #2: dummyfile.0.part --]
[-- Type: image/png, Size: 532317 bytes --]

[-- Attachment #3: dummyfile.1.part --]
[-- Type: image/png, Size: 63035 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg:Add Warm Reset for Xhc

[Wu, Hao A]

[-- Attachment #1.1: Type: text/plain, Size: 3093 bytes --]

Thanks for the information.

For checking “port supports USB3 protocol”, my take is that having the XHC register interface does not mean it is USB3.
It is possible for xHC implementation to have ports that only support USB2 protocol.
I am referring to the “Port Routing and Control” and “xHCI Supported Protocol Capability” sections within the xHCI specification:
[cid:image003.png@01D8FA68.E99F1790]

Also, could you help to check if below code (issue the Warm Reset only instead of Hot Reset + Warm Reset when the port is in inactive state) can address your issue?
    case EfiUsbPortReset:
      DEBUG ((DEBUG_INFO, "XhcUsbPortReset!\n"));
      //
      // Make sure Host Controller not halt before reset it
      //
      if (XhcIsHalt (Xhc)) {
        Status = XhcRunHC (Xhc, XHC_GENERIC_TIMEOUT);

        if (EFI_ERROR (Status)) {
          DEBUG ((DEBUG_INFO, "XhcSetRootHubPortFeature :failed to start HC - %r\n", Status));
          break;
        }
      }

      if ((((State & 0x1e0) >> 5) == 6) && ((State & 3) == 0)) {
        //
        // Usb 3.2 spec 7.5.2
        // When the USB state machine is Inactive state, the device is abnormal.
        // eSS.Inactive is a state where a link has failed Enhanced SuperSpeed operation.Software
        // is required for warm reset intervention.This flag only applies to USB3 protocol ports.
        //
        State |= 0x80000000;
        XhcWriteOpReg (Xhc, Offset, State);
        XhcWaitOpRegBit (Xhc, Offset, XHC_PORTSC_PRC, TRUE, XHC_GENERIC_TIMEOUT);
        DEBUG ((DEBUG_INFO, "Warm Reset Successful! \n"));
      } else {
        //
        // 4.3.1 Resetting a Root Hub Port
        // 1) Write the PORTSC register with the Port Reset (PR) bit set to '1'.
        //
        State |= XHC_PORTSC_RESET;
        XhcWriteOpReg (Xhc, Offset, State);
        XhcWaitOpRegBit (Xhc, Offset, XHC_PORTSC_PRC, TRUE, XHC_GENERIC_TIMEOUT);
      }
      break;

Best Regards,
Hao Wu

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of zhoucheng
Sent: Wednesday, November 16, 2022 7:37 PM
To: Wu; Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg:Add Warm Reset for Xhc

HI:
   The printing test is added here. When the USB disk device is inserted, the repeated mass restart test will be carried out.There
will be a probability of disk loss. When the disk is dropped, the PORTSC register is read. The USB state machine is abnormal.
Its PLS field is 6. Check the protocol document, so warm reset it. The test is normal and no disk loss occurs again.

  The left is normal, and the right is abnormal.
  After XhcUsbPortReset.The normal PORTSC register status is 0x1203, EXCEPTION PORTSC register status is 0x2202c0.
  The USB device has been disconnected.
  [cid:image001.png@01D8FA68.339FC0C0]
  After hot reset, the device can be connected normally.

In the source code, it is confirmed that Usb3.0 is used. Do you need to judge again?
  [cid:image002.png@01D8FA68.339FC0C0]

Thanks!


[-- Attachment #1.2: Type: text/html, Size: 10678 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 532317 bytes --]

[-- Attachment #3: image002.png --]
[-- Type: image/png, Size: 63035 bytes --]

[-- Attachment #4: image003.png --]
[-- Type: image/png, Size: 100776 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg:Add Warm Reset for Xhc

[zhoucheng]

[-- Attachment #1.1: Type: text/plain, Size: 1096 bytes --]

HI  Hao Wu：
This exception is executed

Therefore, the modifications provided may not be applicable. Resetting a Root Hub Port cause the exception.
Although the probability is not high.So I think it still needs to be placed after the Root Hub prot reset.
//
// 4.3.1 Resetting a Root Hub Port
// 1) Write the PORTSC register with the Port Reset (PR) bit set to '1'.
//
State |= XHC_PORTSC_RESET;
XhcWriteOpReg (Xhc, Offset, State);
XhcWaitOpRegBit (Xhc, Offset, XHC_PORTSC_PRC, TRUE, XHC_GENERIC_TIMEOUT);

if ((((State & 0x1e0) >> 5) == 6) && ((State & 3) == 0)) {
//
// Usb 3.2 spec 7.5.2
// When the USB state machine is Inactive state, the device is abnormal.
// eSS.Inactive is a state where a link has failed Enhanced SuperSpeed operation.Software
// is required for warm reset intervention.This flag only applies to USB3 protocol ports.
//
State |= 0x80000000;
XhcWriteOpReg (Xhc, Offset, State);
XhcWaitOpRegBit (Xhc, Offset, XHC_PORTSC_PRC, TRUE, XHC_GENERIC_TIMEOUT);
DEBUG ((DEBUG_INFO, "Warm Reset Successful! \n"));
}
break;

Best Regards
Thanks！

[-- Attachment #1.2: Type: text/html, Size: 1753 bytes --]

[-- Attachment #2: dummyfile.0.part --]
[-- Type: image/png, Size: 40633 bytes --]