[PATCH v6 0/3] MdeModulePkg: Fix UfsBlockIoPei timing problem

[PATCH v6 0/3] MdeModulePkg: Fix UfsBlockIoPei timing problem

[VincentX Ke]

*** BLURB HERE ***

VincentX Ke (3):
  MdeModulePkg: Replace with UFS_UNIT_DESC to fix timeout problem
  MdeModulePkg: Refactoring UFS DME request and fix timing problem
  MdeModulePkg: Put off UFS HCS.DP checking to fix timing problem

 .../Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c     |  23 +-
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c   | 359 ++++++++++--------
 2 files changed, 209 insertions(+), 173 deletions(-)

-- 
2.31.1.windows.1

[PATCH v6 1/3] MdeModulePkg: Replace with UFS_UNIT_DESC to fix timeout problem

[VincentX Ke]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3714

Replace with UFS_UNIT_DESC to fix response timeout problem.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Ian Chiu <Ian.chiu@intel.com>
Cc: Maggie Chu <maggie.chu@intel.com>
Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
---
 .../Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c     | 23 +++++++++----------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
index b331c0f3e3..b8651ff998 100644
--- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
+++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2014 - 2019, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -1037,9 +1037,9 @@ InitializeUfsBlockIoPeim (
   UFS_PEIM_HC_PRIVATE_DATA       *Private;
   EDKII_UFS_HOST_CONTROLLER_PPI  *UfsHcPpi;
   UINT32                         Index;
-  UFS_CONFIG_DESC                Config;
   UINTN                          MmioBase;
   UINT8                          Controller;
+  UFS_UNIT_DESC                  UnitDescriptor;
 
   //
   // Shadow this PEIM to run from memory
@@ -1126,19 +1126,18 @@ InitializeUfsBlockIoPeim (
     }
 
     //
-    // Get Ufs Device's Lun Info by reading Configuration Descriptor.
+    // Check if 8 common luns are active and set corresponding bit mask.
     //
-    Status = UfsRwDeviceDesc (Private, TRUE, UfsConfigDesc, 0, 0, &Config, sizeof (UFS_CONFIG_DESC));
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "Ufs Get Configuration Descriptor Error, Status = %r\n", Status));
-      Controller++;
-      continue;
-    }
-
     for (Index = 0; Index < UFS_PEIM_MAX_LUNS; Index++) {
-      if (Config.UnitDescConfParams[Index].LunEn != 0) {
-        Private->Luns.BitMask |= (BIT0 << Index);
+      Status = UfsRwDeviceDesc (Private, TRUE, UfsUnitDesc, (UINT8)Index, 0, &UnitDescriptor, sizeof (UFS_UNIT_DESC));
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_ERROR, "Fail to read UFS Unit Descriptor, Index = %X, Status = %r\n", Index, Status));
+        continue;
+      }
+
+      if (UnitDescriptor.LunEn == 0x1) {
         DEBUG ((DEBUG_INFO, "Ufs %d Lun %d is enabled\n", Controller, Index));
+        Private->Luns.BitMask |= (BIT0 << Index);
       }
     }
 
-- 
2.31.1.windows.1

Re: [PATCH v6 1/3] MdeModulePkg: Replace with UFS_UNIT_DESC to fix timeout problem

[Wu, Hao A]

Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> -----Original Message-----
> From: Ke, VincentX <vincentx.ke@intel.com>
> Sent: Thursday, December 23, 2021 12:19 PM
> To: devel@edk2.groups.io
> Cc: Ke, VincentX <vincentx.ke@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Ian
> <Ian.chiu@intel.com>; Chu, Maggie <maggie.chu@intel.com>
> Subject: [PATCH v6 1/3] MdeModulePkg: Replace with UFS_UNIT_DESC to fix
> timeout problem
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3714
> 
> Replace with UFS_UNIT_DESC to fix response timeout problem.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Ian Chiu <Ian.chiu@intel.com>
> Cc: Maggie Chu <maggie.chu@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  .../Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c     | 23 +++++++++----------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
> b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
> index b331c0f3e3..b8651ff998 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsBlockIoPei.c
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -  Copyright (c) 2014 - 2019, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2021, Intel Corporation. All rights
> + reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -1037,9 +1037,9 @@ InitializeUfsBlockIoPeim (
>    UFS_PEIM_HC_PRIVATE_DATA       *Private;
>    EDKII_UFS_HOST_CONTROLLER_PPI  *UfsHcPpi;
>    UINT32                         Index;
> -  UFS_CONFIG_DESC                Config;
>    UINTN                          MmioBase;
>    UINT8                          Controller;
> +  UFS_UNIT_DESC                  UnitDescriptor;
> 
>    //
>    // Shadow this PEIM to run from memory @@ -1126,19 +1126,18 @@
> InitializeUfsBlockIoPeim (
>      }
> 
>      //
> -    // Get Ufs Device's Lun Info by reading Configuration Descriptor.
> +    // Check if 8 common luns are active and set corresponding bit mask.
>      //
> -    Status = UfsRwDeviceDesc (Private, TRUE, UfsConfigDesc, 0, 0, &Config,
> sizeof (UFS_CONFIG_DESC));
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_ERROR, "Ufs Get Configuration Descriptor Error, Status
> = %r\n", Status));
> -      Controller++;
> -      continue;
> -    }
> -
>      for (Index = 0; Index < UFS_PEIM_MAX_LUNS; Index++) {
> -      if (Config.UnitDescConfParams[Index].LunEn != 0) {
> -        Private->Luns.BitMask |= (BIT0 << Index);
> +      Status = UfsRwDeviceDesc (Private, TRUE, UfsUnitDesc, (UINT8)Index, 0,
> &UnitDescriptor, sizeof (UFS_UNIT_DESC));
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_ERROR, "Fail to read UFS Unit Descriptor, Index = %X,
> Status = %r\n", Index, Status));
> +        continue;
> +      }
> +
> +      if (UnitDescriptor.LunEn == 0x1) {
>          DEBUG ((DEBUG_INFO, "Ufs %d Lun %d is enabled\n", Controller,
> Index));
> +        Private->Luns.BitMask |= (BIT0 << Index);
>        }
>      }
> 
> --
> 2.31.1.windows.1

[PATCH v6 2/3] MdeModulePkg: Refactoring UFS DME request and fix timing problem

[VincentX Ke]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3775

Refactoring UFS DME request function and retry up to 5 times.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Ian Chiu <Ian.chiu@intel.com>
Cc: Maggie Chu <maggie.chu@intel.com>
Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
---
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c | 312 +++++++++++---------
 1 file changed, 179 insertions(+), 133 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
index 2baa57593e..cffe8e02a7 100644
--- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -557,7 +557,7 @@ UfsCreateDMCommandDesc (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (  ((Opcode == UtpQueryFuncOpcodeSetFlag) || (Opcode == UtpQueryFuncOpcodeClrFlag) || (Opcode == UtpQueryFuncOpcodeTogFlag))
+  if (  ((Opcode == UtpQueryFuncOpcodeClrFlag) || (Opcode == UtpQueryFuncOpcodeTogFlag))
      && ((DataSize != 0) || (Data != NULL)))
   {
     return EFI_INVALID_PARAMETER;
@@ -747,60 +747,91 @@ UfsStopExecCmd (
 }
 
 /**
-  Read or write specified device descriptor of a UFS device.
+  Extracts return data from query response upiu.
 
-  @param[in]      Private       The pointer to the UFS_PEIM_HC_PRIVATE_DATA data structure.
-  @param[in]      Read          The boolean variable to show r/w direction.
-  @param[in]      DescId        The ID of device descriptor.
-  @param[in]      Index         The Index of device descriptor.
-  @param[in]      Selector      The Selector of device descriptor.
-  @param[in, out] Descriptor    The buffer of device descriptor to be read or written.
-  @param[in]      DescSize      The size of device descriptor buffer.
+  @param[in, out] Packet        Pointer to the UFS_DEVICE_MANAGEMENT_REQUEST_PACKET.
+  @param[in]      QueryResp     Pointer to the query response.
 
-  @retval EFI_SUCCESS           The device descriptor was read/written successfully.
-  @retval EFI_DEVICE_ERROR      A device error occurred while attempting to r/w the device descriptor.
-  @retval EFI_TIMEOUT           A timeout occurred while waiting for the completion of r/w the device descriptor.
+  @retval EFI_INVALID_PARAMETER Packet or QueryResp are empty or opcode is invalid.
+  @retval EFI_DEVICE_ERROR      Data returned from device is invalid.
+  @retval EFI_SUCCESS           Data extracted.
 
 **/
 EFI_STATUS
-UfsRwDeviceDesc (
-  IN     UFS_PEIM_HC_PRIVATE_DATA  *Private,
-  IN     BOOLEAN                   Read,
-  IN     UINT8                     DescId,
-  IN     UINT8                     Index,
-  IN     UINT8                     Selector,
-  IN OUT VOID                      *Descriptor,
-  IN     UINT32                    DescSize
+UfsGetReturnDataFromQueryResponse (
+  IN OUT UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  *Packet,
+  IN     UTP_QUERY_RESP_UPIU                   *QueryResp
   )
 {
-  EFI_STATUS                            Status;
-  UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  Packet;
-  UINT8                                 Slot;
-  UTP_TRD                               *Trd;
-  UINTN                                 Address;
-  UTP_QUERY_RESP_UPIU                   *QueryResp;
-  UINT8                                 *CmdDescBase;
-  UINT32                                CmdDescSize;
-  UINT16                                ReturnDataSize;
+  UINT16  ReturnDataSize;
 
-  ZeroMem (&Packet, sizeof (UFS_DEVICE_MANAGEMENT_REQUEST_PACKET));
+  ReturnDataSize = 0;
 
-  if (Read) {
-    Packet.DataDirection    = UfsDataIn;
-    Packet.InDataBuffer     = Descriptor;
-    Packet.InTransferLength = DescSize;
-    Packet.Opcode           = UtpQueryFuncOpcodeRdDesc;
-  } else {
-    Packet.DataDirection     = UfsDataOut;
-    Packet.OutDataBuffer     = Descriptor;
-    Packet.OutTransferLength = DescSize;
-    Packet.Opcode            = UtpQueryFuncOpcodeWrDesc;
+  if ((Packet == NULL) || (QueryResp == NULL)) {
+    return EFI_INVALID_PARAMETER;
   }
 
-  Packet.DescId   = DescId;
-  Packet.Index    = Index;
-  Packet.Selector = Selector;
-  Packet.Timeout  = UFS_TIMEOUT;
+  switch (Packet->Opcode) {
+    case UtpQueryFuncOpcodeRdDesc:
+      ReturnDataSize = QueryResp->Tsf.Length;
+      SwapLittleEndianToBigEndian ((UINT8 *)&ReturnDataSize, sizeof (UINT16));
+      //
+      // Make sure the hardware device does not return more data than expected.
+      //
+      if (ReturnDataSize > Packet->InTransferLength) {
+        return EFI_DEVICE_ERROR;
+      }
+
+      CopyMem (Packet->InDataBuffer, (QueryResp + 1), ReturnDataSize);
+      Packet->InTransferLength = ReturnDataSize;
+      break;
+    case UtpQueryFuncOpcodeWrDesc:
+      ReturnDataSize = QueryResp->Tsf.Length;
+      SwapLittleEndianToBigEndian ((UINT8 *)&ReturnDataSize, sizeof (UINT16));
+      Packet->OutTransferLength = ReturnDataSize;
+      break;
+    case UtpQueryFuncOpcodeRdFlag:
+    case UtpQueryFuncOpcodeSetFlag:
+    case UtpQueryFuncOpcodeClrFlag:
+    case UtpQueryFuncOpcodeTogFlag:
+      //
+      // The 'FLAG VALUE' field is at byte offset 3 of QueryResp->Tsf.Value
+      //
+      *((UINT8 *)(Packet->OutDataBuffer)) = *((UINT8 *)&(QueryResp->Tsf.Value) + 3);
+      break;
+    default:
+      return EFI_INVALID_PARAMETER;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Creates Transfer Request descriptor and sends Query Request to the device.
+
+  @param[in]      Private       Pointer to the UFS_PEIM_HC_PRIVATE_DATA.
+  @param[in, out] Packet        Pointer to the UFS_DEVICE_MANAGEMENT_REQUEST_PACKET.
+
+  @retval EFI_SUCCESS           The device descriptor was read/written successfully.
+  @retval EFI_INVALID_PARAMETER The DescId, Index and Selector fields in Packet are invalid
+                                combination to point to a type of UFS device descriptor.
+  @retval EFI_DEVICE_ERROR      A device error occurred while attempting to r/w the device descriptor.
+  @retval EFI_TIMEOUT           A timeout occurred while waiting for the completion of r/w the device descriptor.
+
+**/
+EFI_STATUS
+UfsSendDmRequestRetry (
+  IN     UFS_PEIM_HC_PRIVATE_DATA              *Private,
+  IN OUT UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  *Packet
+  )
+{
+  UINT8                Slot;
+  EFI_STATUS           Status;
+  UTP_TRD              *Trd;
+  UINTN                Address;
+  UTP_QUERY_RESP_UPIU  *QueryResp;
+  UINT8                *CmdDescBase;
+  UINT32               CmdDescSize;
 
   //
   // Find out which slot of transfer request list is available.
@@ -814,8 +845,9 @@ UfsRwDeviceDesc (
   //
   // Fill transfer request descriptor to this slot.
   //
-  Status = UfsCreateDMCommandDesc (Private, &Packet, Trd);
+  Status = UfsCreateDMCommandDesc (Private, Packet, Trd);
   if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "Failed to create DM command descriptor\n"));
     return Status;
   }
 
@@ -835,43 +867,116 @@ UfsRwDeviceDesc (
   // Wait for the completion of the transfer request.
   //
   Address = Private->UfsHcBase + UFS_HC_UTRLDBR_OFFSET;
-  Status  = UfsWaitMemSet (Address, BIT0 << Slot, 0, Packet.Timeout);
+  Status  = UfsWaitMemSet (Address, (BIT0 << Slot), 0, Packet->Timeout);
   if (EFI_ERROR (Status)) {
     goto Exit;
   }
 
-  if (QueryResp->QueryResp != 0) {
+  if ((Trd->Ocs != 0) || (QueryResp->QueryResp != 0)) {
+    DEBUG ((DEBUG_ERROR, "Failed to send query request, OCS = %X, QueryResp = %X\n", Trd->Ocs, QueryResp->QueryResp));
     DumpQueryResponseResult (QueryResp->QueryResp);
     Status = EFI_DEVICE_ERROR;
     goto Exit;
   }
 
-  if (Trd->Ocs == 0) {
-    ReturnDataSize = QueryResp->Tsf.Length;
-    SwapLittleEndianToBigEndian ((UINT8 *)&ReturnDataSize, sizeof (UINT16));
+  Status = UfsGetReturnDataFromQueryResponse (Packet, QueryResp);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "Failed to get return data from query response\n"));
+    goto Exit;
+  }
 
-    if (Read) {
-      //
-      // Make sure the hardware device does not return more data than expected.
-      //
-      if (ReturnDataSize > Packet.InTransferLength) {
-        Status = EFI_DEVICE_ERROR;
-        goto Exit;
-      }
+Exit:
+  UfsStopExecCmd (Private, Slot);
+  UfsPeimFreeMem (Private->Pool, CmdDescBase, CmdDescSize);
 
-      CopyMem (Packet.InDataBuffer, (QueryResp + 1), ReturnDataSize);
-      Packet.InTransferLength = ReturnDataSize;
-    } else {
-      Packet.OutTransferLength = ReturnDataSize;
+  return Status;
+}
+
+/**
+  Sends Query Request to the device. Query is sent until device responds correctly or counter runs out.
+
+  @param[in]      Private       Pointer to the UFS_PEIM_HC_PRIVATE_DATA.
+  @param[in, out] Packet        Pointer to the UFS_DEVICE_MANAGEMENT_REQUEST_PACKET.
+
+  @retval EFI_SUCCESS           The device responded correctly to the Query request.
+  @retval EFI_INVALID_PARAMETER The DescId, Index and Selector fields in Packet are invalid
+                                combination to point to a type of UFS device descriptor.
+  @retval EFI_DEVICE_ERROR      A device error occurred while waiting for the response from the device.
+  @retval EFI_TIMEOUT           A timeout occurred while waiting for the completion of the operation.
+
+**/
+EFI_STATUS
+UfsSendDmRequest (
+  IN     UFS_PEIM_HC_PRIVATE_DATA              *Private,
+  IN OUT UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  *Packet
+  )
+{
+  EFI_STATUS  Status;
+  UINT8       Retry;
+
+  Status = EFI_SUCCESS;
+
+  for (Retry = 0; Retry < 5; Retry++) {
+    Status = UfsSendDmRequestRetry (Private, Packet);
+    if (!EFI_ERROR (Status)) {
+      return EFI_SUCCESS;
     }
+  }
+
+  DEBUG ((DEBUG_ERROR, "Failed to get response from the device after %d retries\n", Retry));
+  return Status;
+}
+
+/**
+  Read or write specified device descriptor of a UFS device.
+
+  @param[in]      Private       The pointer to the UFS_PEIM_HC_PRIVATE_DATA data structure.
+  @param[in]      Read          The boolean variable to show r/w direction.
+  @param[in]      DescId        The ID of device descriptor.
+  @param[in]      Index         The Index of device descriptor.
+  @param[in]      Selector      The Selector of device descriptor.
+  @param[in, out] Descriptor    The buffer of device descriptor to be read or written.
+  @param[in]      DescSize      The size of device descriptor buffer.
+
+  @retval EFI_SUCCESS           The device descriptor was read/written successfully.
+  @retval EFI_DEVICE_ERROR      A device error occurred while attempting to r/w the device descriptor.
+  @retval EFI_TIMEOUT           A timeout occurred while waiting for the completion of r/w the device descriptor.
+
+**/
+EFI_STATUS
+UfsRwDeviceDesc (
+  IN     UFS_PEIM_HC_PRIVATE_DATA  *Private,
+  IN     BOOLEAN                   Read,
+  IN     UINT8                     DescId,
+  IN     UINT8                     Index,
+  IN     UINT8                     Selector,
+  IN OUT VOID                      *Descriptor,
+  IN     UINT32                    DescSize
+  )
+{
+  EFI_STATUS                            Status;
+  UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  Packet;
+
+  ZeroMem (&Packet, sizeof (UFS_DEVICE_MANAGEMENT_REQUEST_PACKET));
+
+  if (Read) {
+    Packet.DataDirection    = UfsDataIn;
+    Packet.InDataBuffer     = Descriptor;
+    Packet.InTransferLength = DescSize;
+    Packet.Opcode           = UtpQueryFuncOpcodeRdDesc;
   } else {
-    Status = EFI_DEVICE_ERROR;
+    Packet.DataDirection     = UfsDataOut;
+    Packet.OutDataBuffer     = Descriptor;
+    Packet.OutTransferLength = DescSize;
+    Packet.Opcode            = UtpQueryFuncOpcodeWrDesc;
   }
 
-Exit:
-  UfsStopExecCmd (Private, Slot);
-  UfsPeimFreeMem (Private->Pool, CmdDescBase, CmdDescSize);
+  Packet.DescId   = DescId;
+  Packet.Index    = Index;
+  Packet.Selector = Selector;
+  Packet.Timeout  = UFS_TIMEOUT;
 
+  Status = UfsSendDmRequest (Private, &Packet);
   return Status;
 }
 
@@ -898,12 +1003,6 @@ UfsRwFlags (
 {
   EFI_STATUS                            Status;
   UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  Packet;
-  UINT8                                 Slot;
-  UTP_TRD                               *Trd;
-  UINTN                                 Address;
-  UTP_QUERY_RESP_UPIU                   *QueryResp;
-  UINT8                                 *CmdDescBase;
-  UINT32                                CmdDescSize;
 
   if (Value == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -926,67 +1025,14 @@ UfsRwFlags (
     }
   }
 
-  Packet.DescId   = FlagId;
-  Packet.Index    = 0;
-  Packet.Selector = 0;
-  Packet.Timeout  = UFS_TIMEOUT;
+  Packet.OutDataBuffer = (VOID *)Value;
+  Packet.DescId        = FlagId;
+  Packet.Index         = 0;
+  Packet.Selector      = 0;
+  Packet.Timeout       = UFS_TIMEOUT;
 
-  //
-  // Find out which slot of transfer request list is available.
-  //
-  Status = UfsFindAvailableSlotInTrl (Private, &Slot);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  //
-  // Fill transfer request descriptor to this slot.
-  //
-  Trd    = ((UTP_TRD *)Private->UtpTrlBase) + Slot;
-  Status = UfsCreateDMCommandDesc (Private, &Packet, Trd);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  //
-  // Check the transfer request result.
-  //
-  CmdDescBase = (UINT8 *)(UINTN)(LShiftU64 ((UINT64)Trd->UcdBaU, 32) | LShiftU64 ((UINT64)Trd->UcdBa, 7));
-  QueryResp   = (UTP_QUERY_RESP_UPIU *)(CmdDescBase + Trd->RuO * sizeof (UINT32));
-  CmdDescSize = Trd->RuO * sizeof (UINT32) + Trd->RuL * sizeof (UINT32);
-
-  //
-  // Start to execute the transfer request.
-  //
-  UfsStartExecCmd (Private, Slot);
-
-  //
-  // Wait for the completion of the transfer request.
-  //
-  Address = Private->UfsHcBase + UFS_HC_UTRLDBR_OFFSET;
-  Status  = UfsWaitMemSet (Address, BIT0 << Slot, 0, Packet.Timeout);
-  if (EFI_ERROR (Status)) {
-    goto Exit;
-  }
-
-  if (QueryResp->QueryResp != 0) {
-    DumpQueryResponseResult (QueryResp->QueryResp);
-    Status = EFI_DEVICE_ERROR;
-    goto Exit;
-  }
-
-  if (Trd->Ocs == 0) {
-    //
-    // The 'FLAG VALUE' field is at byte offset 3 of QueryResp->Tsf.Value
-    //
-    *Value = *((UINT8 *)&(QueryResp->Tsf.Value) + 3);
-  } else {
-    Status = EFI_DEVICE_ERROR;
-  }
-
-Exit:
-  UfsStopExecCmd (Private, Slot);
-  UfsPeimFreeMem (Private->Pool, CmdDescBase, CmdDescSize);
+  Status = UfsSendDmRequest (Private, &Packet);
+  *Value = *((UINT8 *)(Packet.OutDataBuffer));
 
   return Status;
 }
-- 
2.31.1.windows.1

Re: [PATCH v6 2/3] MdeModulePkg: Refactoring UFS DME request and fix timing problem

[Wu, Hao A]

Inline comment below:


> -----Original Message-----
> From: Ke, VincentX <vincentx.ke@intel.com>
> Sent: Thursday, December 23, 2021 12:19 PM
> To: devel@edk2.groups.io
> Cc: Ke, VincentX <vincentx.ke@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Ian
> <Ian.chiu@intel.com>; Chu, Maggie <maggie.chu@intel.com>
> Subject: [PATCH v6 2/3] MdeModulePkg: Refactoring UFS DME request and
> fix timing problem
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3775
> 
> Refactoring UFS DME request function and retry up to 5 times.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Ian Chiu <Ian.chiu@intel.com>
> Cc: Maggie Chu <maggie.chu@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c | 312 +++++++++++--------
> -
>  1 file changed, 179 insertions(+), 133 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> index 2baa57593e..cffe8e02a7 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -557,7 +557,7 @@ UfsCreateDMCommandDesc (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> -  if (  ((Opcode == UtpQueryFuncOpcodeSetFlag) || (Opcode ==
> UtpQueryFuncOpcodeClrFlag) || (Opcode == UtpQueryFuncOpcodeTogFlag))
> +  if (  ((Opcode == UtpQueryFuncOpcodeClrFlag) || (Opcode ==
> UtpQueryFuncOpcodeTogFlag))


Please help to just remove the above check.



>       && ((DataSize != 0) || (Data != NULL)))
>    {
>      return EFI_INVALID_PARAMETER;
> @@ -747,60 +747,91 @@ UfsStopExecCmd (
>  }
> 
>  /**
> -  Read or write specified device descriptor of a UFS device.
> +  Extracts return data from query response upiu.
> 
> -  @param[in]      Private       The pointer to the
> UFS_PEIM_HC_PRIVATE_DATA data structure.
> -  @param[in]      Read          The boolean variable to show r/w direction.
> -  @param[in]      DescId        The ID of device descriptor.
> -  @param[in]      Index         The Index of device descriptor.
> -  @param[in]      Selector      The Selector of device descriptor.
> -  @param[in, out] Descriptor    The buffer of device descriptor to be read or
> written.
> -  @param[in]      DescSize      The size of device descriptor buffer.
> +  @param[in, out] Packet        Pointer to the
> UFS_DEVICE_MANAGEMENT_REQUEST_PACKET.
> +  @param[in]      QueryResp     Pointer to the query response.
> 
> -  @retval EFI_SUCCESS           The device descriptor was read/written
> successfully.
> -  @retval EFI_DEVICE_ERROR      A device error occurred while attempting to
> r/w the device descriptor.
> -  @retval EFI_TIMEOUT           A timeout occurred while waiting for the
> completion of r/w the device descriptor.
> +  @retval EFI_INVALID_PARAMETER Packet or QueryResp are empty or
> opcode is invalid.
> +  @retval EFI_DEVICE_ERROR      Data returned from device is invalid.
> +  @retval EFI_SUCCESS           Data extracted.
> 
>  **/
>  EFI_STATUS
> -UfsRwDeviceDesc (
> -  IN     UFS_PEIM_HC_PRIVATE_DATA  *Private,
> -  IN     BOOLEAN                   Read,
> -  IN     UINT8                     DescId,
> -  IN     UINT8                     Index,
> -  IN     UINT8                     Selector,
> -  IN OUT VOID                      *Descriptor,
> -  IN     UINT32                    DescSize
> +UfsGetReturnDataFromQueryResponse (
> +  IN OUT UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  *Packet,
> +  IN     UTP_QUERY_RESP_UPIU                   *QueryResp
>    )
>  {
> -  EFI_STATUS                            Status;
> -  UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  Packet;
> -  UINT8                                 Slot;
> -  UTP_TRD                               *Trd;
> -  UINTN                                 Address;
> -  UTP_QUERY_RESP_UPIU                   *QueryResp;
> -  UINT8                                 *CmdDescBase;
> -  UINT32                                CmdDescSize;
> -  UINT16                                ReturnDataSize;
> +  UINT16  ReturnDataSize;
> 
> -  ZeroMem (&Packet, sizeof
> (UFS_DEVICE_MANAGEMENT_REQUEST_PACKET));
> +  ReturnDataSize = 0;
> 
> -  if (Read) {
> -    Packet.DataDirection    = UfsDataIn;
> -    Packet.InDataBuffer     = Descriptor;
> -    Packet.InTransferLength = DescSize;
> -    Packet.Opcode           = UtpQueryFuncOpcodeRdDesc;
> -  } else {
> -    Packet.DataDirection     = UfsDataOut;
> -    Packet.OutDataBuffer     = Descriptor;
> -    Packet.OutTransferLength = DescSize;
> -    Packet.Opcode            = UtpQueryFuncOpcodeWrDesc;
> +  if ((Packet == NULL) || (QueryResp == NULL)) {
> +    return EFI_INVALID_PARAMETER;
>    }
> 
> -  Packet.DescId   = DescId;
> -  Packet.Index    = Index;
> -  Packet.Selector = Selector;
> -  Packet.Timeout  = UFS_TIMEOUT;
> +  switch (Packet->Opcode) {
> +    case UtpQueryFuncOpcodeRdDesc:
> +      ReturnDataSize = QueryResp->Tsf.Length;
> +      SwapLittleEndianToBigEndian ((UINT8 *)&ReturnDataSize, sizeof
> (UINT16));
> +      //
> +      // Make sure the hardware device does not return more data than
> expected.
> +      //
> +      if (ReturnDataSize > Packet->InTransferLength) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +
> +      CopyMem (Packet->InDataBuffer, (QueryResp + 1), ReturnDataSize);
> +      Packet->InTransferLength = ReturnDataSize;
> +      break;
> +    case UtpQueryFuncOpcodeWrDesc:
> +      ReturnDataSize = QueryResp->Tsf.Length;
> +      SwapLittleEndianToBigEndian ((UINT8 *)&ReturnDataSize, sizeof
> (UINT16));
> +      Packet->OutTransferLength = ReturnDataSize;
> +      break;
> +    case UtpQueryFuncOpcodeRdFlag:
> +    case UtpQueryFuncOpcodeSetFlag:
> +    case UtpQueryFuncOpcodeClrFlag:
> +    case UtpQueryFuncOpcodeTogFlag:
> +      //
> +      // The 'FLAG VALUE' field is at byte offset 3 of QueryResp->Tsf.Value
> +      //
> +      *((UINT8 *)(Packet->OutDataBuffer)) = *((UINT8 *)&(QueryResp-
> >Tsf.Value) + 3);


Use "OutDataBuffer" only for UtpQueryFuncOpcodeSetFlag, UtpQueryFuncOpcodeClrFlag and UtpQueryFuncOpcodeTogFlag.
Use "InDataBuffer" for UtpQueryFuncOpcodeRdFlag.


> +      break;
> +    default:
> +      return EFI_INVALID_PARAMETER;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Creates Transfer Request descriptor and sends Query Request to the
> device.
> +
> +  @param[in]      Private       Pointer to the UFS_PEIM_HC_PRIVATE_DATA.
> +  @param[in, out] Packet        Pointer to the
> UFS_DEVICE_MANAGEMENT_REQUEST_PACKET.
> +
> +  @retval EFI_SUCCESS           The device descriptor was read/written
> successfully.
> +  @retval EFI_INVALID_PARAMETER The DescId, Index and Selector fields in
> Packet are invalid
> +                                combination to point to a type of UFS device descriptor.
> +  @retval EFI_DEVICE_ERROR      A device error occurred while attempting to
> r/w the device descriptor.
> +  @retval EFI_TIMEOUT           A timeout occurred while waiting for the
> completion of r/w the device descriptor.
> +
> +**/
> +EFI_STATUS
> +UfsSendDmRequestRetry (
> +  IN     UFS_PEIM_HC_PRIVATE_DATA              *Private,
> +  IN OUT UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  *Packet
> +  )
> +{
> +  UINT8                Slot;
> +  EFI_STATUS           Status;
> +  UTP_TRD              *Trd;
> +  UINTN                Address;
> +  UTP_QUERY_RESP_UPIU  *QueryResp;
> +  UINT8                *CmdDescBase;
> +  UINT32               CmdDescSize;
> 
>    //
>    // Find out which slot of transfer request list is available.
> @@ -814,8 +845,9 @@ UfsRwDeviceDesc (
>    //
>    // Fill transfer request descriptor to this slot.
>    //
> -  Status = UfsCreateDMCommandDesc (Private, &Packet, Trd);
> +  Status = UfsCreateDMCommandDesc (Private, Packet, Trd);
>    if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Failed to create DM command descriptor\n"));
>      return Status;
>    }
> 
> @@ -835,43 +867,116 @@ UfsRwDeviceDesc (
>    // Wait for the completion of the transfer request.
>    //
>    Address = Private->UfsHcBase + UFS_HC_UTRLDBR_OFFSET;
> -  Status  = UfsWaitMemSet (Address, BIT0 << Slot, 0, Packet.Timeout);
> +  Status  = UfsWaitMemSet (Address, (BIT0 << Slot), 0, Packet->Timeout);
>    if (EFI_ERROR (Status)) {
>      goto Exit;
>    }
> 
> -  if (QueryResp->QueryResp != 0) {
> +  if ((Trd->Ocs != 0) || (QueryResp->QueryResp != 0)) {


Please use:
  if ((Trd->Ocs != 0) || (QueryResp->QueryResp != UfsUtpQueryResponseSuccess)) {


> +    DEBUG ((DEBUG_ERROR, "Failed to send query request, OCS = %X,
> QueryResp = %X\n", Trd->Ocs, QueryResp->QueryResp));
>      DumpQueryResponseResult (QueryResp->QueryResp);
>      Status = EFI_DEVICE_ERROR;
>      goto Exit;
>    }
> 
> -  if (Trd->Ocs == 0) {
> -    ReturnDataSize = QueryResp->Tsf.Length;
> -    SwapLittleEndianToBigEndian ((UINT8 *)&ReturnDataSize, sizeof
> (UINT16));
> +  Status = UfsGetReturnDataFromQueryResponse (Packet, QueryResp);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Failed to get return data from query
> response\n"));
> +    goto Exit;
> +  }
> 
> -    if (Read) {
> -      //
> -      // Make sure the hardware device does not return more data than
> expected.
> -      //
> -      if (ReturnDataSize > Packet.InTransferLength) {
> -        Status = EFI_DEVICE_ERROR;
> -        goto Exit;
> -      }
> +Exit:
> +  UfsStopExecCmd (Private, Slot);
> +  UfsPeimFreeMem (Private->Pool, CmdDescBase, CmdDescSize);
> 
> -      CopyMem (Packet.InDataBuffer, (QueryResp + 1), ReturnDataSize);
> -      Packet.InTransferLength = ReturnDataSize;
> -    } else {
> -      Packet.OutTransferLength = ReturnDataSize;
> +  return Status;
> +}
> +
> +/**
> +  Sends Query Request to the device. Query is sent until device responds
> correctly or counter runs out.
> +
> +  @param[in]      Private       Pointer to the UFS_PEIM_HC_PRIVATE_DATA.
> +  @param[in, out] Packet        Pointer to the
> UFS_DEVICE_MANAGEMENT_REQUEST_PACKET.
> +
> +  @retval EFI_SUCCESS           The device responded correctly to the Query
> request.
> +  @retval EFI_INVALID_PARAMETER The DescId, Index and Selector fields in
> Packet are invalid
> +                                combination to point to a type of UFS device descriptor.
> +  @retval EFI_DEVICE_ERROR      A device error occurred while waiting for
> the response from the device.
> +  @retval EFI_TIMEOUT           A timeout occurred while waiting for the
> completion of the operation.
> +
> +**/
> +EFI_STATUS
> +UfsSendDmRequest (
> +  IN     UFS_PEIM_HC_PRIVATE_DATA              *Private,
> +  IN OUT UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  *Packet
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT8       Retry;
> +
> +  Status = EFI_SUCCESS;
> +
> +  for (Retry = 0; Retry < 5; Retry++) {
> +    Status = UfsSendDmRequestRetry (Private, Packet);
> +    if (!EFI_ERROR (Status)) {
> +      return EFI_SUCCESS;
>      }
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "Failed to get response from the device after %d
> retries\n", Retry));
> +  return Status;
> +}
> +
> +/**
> +  Read or write specified device descriptor of a UFS device.
> +
> +  @param[in]      Private       The pointer to the
> UFS_PEIM_HC_PRIVATE_DATA data structure.
> +  @param[in]      Read          The boolean variable to show r/w direction.
> +  @param[in]      DescId        The ID of device descriptor.
> +  @param[in]      Index         The Index of device descriptor.
> +  @param[in]      Selector      The Selector of device descriptor.
> +  @param[in, out] Descriptor    The buffer of device descriptor to be read or
> written.
> +  @param[in]      DescSize      The size of device descriptor buffer.
> +
> +  @retval EFI_SUCCESS           The device descriptor was read/written
> successfully.
> +  @retval EFI_DEVICE_ERROR      A device error occurred while attempting to
> r/w the device descriptor.
> +  @retval EFI_TIMEOUT           A timeout occurred while waiting for the
> completion of r/w the device descriptor.
> +
> +**/
> +EFI_STATUS
> +UfsRwDeviceDesc (
> +  IN     UFS_PEIM_HC_PRIVATE_DATA  *Private,
> +  IN     BOOLEAN                   Read,
> +  IN     UINT8                     DescId,
> +  IN     UINT8                     Index,
> +  IN     UINT8                     Selector,
> +  IN OUT VOID                      *Descriptor,
> +  IN     UINT32                    DescSize
> +  )
> +{
> +  EFI_STATUS                            Status;
> +  UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  Packet;
> +
> +  ZeroMem (&Packet, sizeof
> (UFS_DEVICE_MANAGEMENT_REQUEST_PACKET));
> +
> +  if (Read) {
> +    Packet.DataDirection    = UfsDataIn;
> +    Packet.InDataBuffer     = Descriptor;
> +    Packet.InTransferLength = DescSize;
> +    Packet.Opcode           = UtpQueryFuncOpcodeRdDesc;
>    } else {
> -    Status = EFI_DEVICE_ERROR;
> +    Packet.DataDirection     = UfsDataOut;
> +    Packet.OutDataBuffer     = Descriptor;
> +    Packet.OutTransferLength = DescSize;
> +    Packet.Opcode            = UtpQueryFuncOpcodeWrDesc;
>    }
> 
> -Exit:
> -  UfsStopExecCmd (Private, Slot);
> -  UfsPeimFreeMem (Private->Pool, CmdDescBase, CmdDescSize);
> +  Packet.DescId   = DescId;
> +  Packet.Index    = Index;
> +  Packet.Selector = Selector;
> +  Packet.Timeout  = UFS_TIMEOUT;
> 
> +  Status = UfsSendDmRequest (Private, &Packet);
>    return Status;
>  }
> 
> @@ -898,12 +1003,6 @@ UfsRwFlags (
>  {
>    EFI_STATUS                            Status;
>    UFS_DEVICE_MANAGEMENT_REQUEST_PACKET  Packet;
> -  UINT8                                 Slot;
> -  UTP_TRD                               *Trd;
> -  UINTN                                 Address;
> -  UTP_QUERY_RESP_UPIU                   *QueryResp;
> -  UINT8                                 *CmdDescBase;
> -  UINT32                                CmdDescSize;
> 
>    if (Value == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -926,67 +1025,14 @@ UfsRwFlags (
>      }
>    }
> 
> -  Packet.DescId   = FlagId;
> -  Packet.Index    = 0;
> -  Packet.Selector = 0;
> -  Packet.Timeout  = UFS_TIMEOUT;
> +  Packet.OutDataBuffer = (VOID *)Value;


Please use "Packet.OutDataBuffer" only for the write case, use "Packet.InDataBuffer" for read case.
Also, I suggest to set OutTransferLength and InTransferLength to 0 respectively.


> +  Packet.DescId        = FlagId;
> +  Packet.Index         = 0;
> +  Packet.Selector      = 0;
> +  Packet.Timeout       = UFS_TIMEOUT;
> 
> -  //
> -  // Find out which slot of transfer request list is available.
> -  //
> -  Status = UfsFindAvailableSlotInTrl (Private, &Slot);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  //
> -  // Fill transfer request descriptor to this slot.
> -  //
> -  Trd    = ((UTP_TRD *)Private->UtpTrlBase) + Slot;
> -  Status = UfsCreateDMCommandDesc (Private, &Packet, Trd);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  //
> -  // Check the transfer request result.
> -  //
> -  CmdDescBase = (UINT8 *)(UINTN)(LShiftU64 ((UINT64)Trd->UcdBaU, 32) |
> LShiftU64 ((UINT64)Trd->UcdBa, 7));
> -  QueryResp   = (UTP_QUERY_RESP_UPIU *)(CmdDescBase + Trd->RuO *
> sizeof (UINT32));
> -  CmdDescSize = Trd->RuO * sizeof (UINT32) + Trd->RuL * sizeof (UINT32);
> -
> -  //
> -  // Start to execute the transfer request.
> -  //
> -  UfsStartExecCmd (Private, Slot);
> -
> -  //
> -  // Wait for the completion of the transfer request.
> -  //
> -  Address = Private->UfsHcBase + UFS_HC_UTRLDBR_OFFSET;
> -  Status  = UfsWaitMemSet (Address, BIT0 << Slot, 0, Packet.Timeout);
> -  if (EFI_ERROR (Status)) {
> -    goto Exit;
> -  }
> -
> -  if (QueryResp->QueryResp != 0) {
> -    DumpQueryResponseResult (QueryResp->QueryResp);
> -    Status = EFI_DEVICE_ERROR;
> -    goto Exit;
> -  }
> -
> -  if (Trd->Ocs == 0) {
> -    //
> -    // The 'FLAG VALUE' field is at byte offset 3 of QueryResp->Tsf.Value
> -    //
> -    *Value = *((UINT8 *)&(QueryResp->Tsf.Value) + 3);
> -  } else {
> -    Status = EFI_DEVICE_ERROR;
> -  }
> -
> -Exit:
> -  UfsStopExecCmd (Private, Slot);
> -  UfsPeimFreeMem (Private->Pool, CmdDescBase, CmdDescSize);
> +  Status = UfsSendDmRequest (Private, &Packet);
> +  *Value = *((UINT8 *)(Packet.OutDataBuffer));


Similar comment as above, please use "Packet.InDataBuffer" for read case.

Best Regards,
Hao Wu

> 
>    return Status;
>  }
> --
> 2.31.1.windows.1

[PATCH v6 3/3] MdeModulePkg: Put off UFS HCS.DP checking to fix timing problem

[VincentX Ke]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3776

Put off UFS HCS.DP (Device Attached) checking
until UfsDeviceDetection() to fix timing problem.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Ian Chiu <Ian.chiu@intel.com>
Cc: Maggie Chu <maggie.chu@intel.com>
Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
---
 MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c | 47 +++++++++------------
 1 file changed, 19 insertions(+), 28 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
index cffe8e02a7..864bf6928d 100644
--- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
@@ -1353,23 +1353,6 @@ UfsExecUicCommands (
     }
   }
 
-  //
-  // Check value of HCS.DP and make sure that there is a device attached to the Link.
-  //
-  Address = UfsHcBase + UFS_HC_STATUS_OFFSET;
-  Data    = MmioRead32 (Address);
-  if ((Data & UFS_HC_HCS_DP) == 0) {
-    Address = UfsHcBase + UFS_HC_IS_OFFSET;
-    Status  = UfsWaitMemSet (Address, UFS_HC_IS_ULSS, UFS_HC_IS_ULSS, UFS_TIMEOUT);
-    if (EFI_ERROR (Status)) {
-      return EFI_DEVICE_ERROR;
-    }
-
-    return EFI_NOT_FOUND;
-  }
-
-  DEBUG ((DEBUG_INFO, "UfsblockioPei: found a attached UFS device\n"));
-
   return EFI_SUCCESS;
 }
 
@@ -1443,6 +1426,8 @@ UfsDeviceDetection (
   )
 {
   UINTN       Retry;
+  UINTN       Address;
+  UINT32      Data;
   EFI_STATUS  Status;
 
   //
@@ -1451,22 +1436,28 @@ UfsDeviceDetection (
   //
   for (Retry = 0; Retry < 3; Retry++) {
     Status = UfsExecUicCommands (Private, UfsUicDmeLinkStartup, 0, 0, 0);
-    if (!EFI_ERROR (Status)) {
-      break;
+    if (EFI_ERROR (Status)) {
+      return EFI_DEVICE_ERROR;
     }
 
-    if (Status == EFI_NOT_FOUND) {
-      continue;
+    //
+    // Check value of HCS.DP and make sure that there is a device attached to the Link
+    //
+    Address = Private->UfsHcBase + UFS_HC_STATUS_OFFSET;
+    Data    = MmioRead32 (Address);
+    if ((Data & UFS_HC_HCS_DP) == 0) {
+      Address = Private->UfsHcBase + UFS_HC_IS_OFFSET;
+      Status  = UfsWaitMemSet (Address, UFS_HC_IS_ULSS, UFS_HC_IS_ULSS, UFS_TIMEOUT);
+      if (EFI_ERROR (Status)) {
+        return EFI_DEVICE_ERROR;
+      }
+    } else {
+      DEBUG ((DEBUG_INFO, "UfsblockioPei: found a attached UFS device\n"));
+      return EFI_SUCCESS;
     }
-
-    return EFI_DEVICE_ERROR;
   }
 
-  if (Retry == 3) {
-    return EFI_NOT_FOUND;
-  }
-
-  return EFI_SUCCESS;
+  return EFI_NOT_FOUND;
 }
 
 /**
-- 
2.31.1.windows.1

Re: [edk2-devel] [PATCH v6 3/3] MdeModulePkg: Put off UFS HCS.DP checking to fix timing problem

[Wu, Hao A]

Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> VincentX Ke
> Sent: Thursday, December 23, 2021 12:19 PM
> To: devel@edk2.groups.io
> Cc: Ke, VincentX <vincentx.ke@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Chiu, Ian
> <Ian.chiu@intel.com>; Chu, Maggie <maggie.chu@intel.com>
> Subject: [edk2-devel] [PATCH v6 3/3] MdeModulePkg: Put off UFS HCS.DP
> checking to fix timing problem
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3776
> 
> Put off UFS HCS.DP (Device Attached) checking until UfsDeviceDetection() to
> fix timing problem.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Ian Chiu <Ian.chiu@intel.com>
> Cc: Maggie Chu <maggie.chu@intel.com>
> Signed-off-by: VincentX Ke <vincentx.ke@intel.com>
> ---
>  MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c | 47 +++++++++------------
>  1 file changed, 19 insertions(+), 28 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> index cffe8e02a7..864bf6928d 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c
> @@ -1353,23 +1353,6 @@ UfsExecUicCommands (
>      }
>    }
> 
> -  //
> -  // Check value of HCS.DP and make sure that there is a device attached to
> the Link.
> -  //
> -  Address = UfsHcBase + UFS_HC_STATUS_OFFSET;
> -  Data    = MmioRead32 (Address);
> -  if ((Data & UFS_HC_HCS_DP) == 0) {
> -    Address = UfsHcBase + UFS_HC_IS_OFFSET;
> -    Status  = UfsWaitMemSet (Address, UFS_HC_IS_ULSS, UFS_HC_IS_ULSS,
> UFS_TIMEOUT);
> -    if (EFI_ERROR (Status)) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -
> -    return EFI_NOT_FOUND;
> -  }
> -
> -  DEBUG ((DEBUG_INFO, "UfsblockioPei: found a attached UFS device\n"));
> -
>    return EFI_SUCCESS;
>  }
> 
> @@ -1443,6 +1426,8 @@ UfsDeviceDetection (
>    )
>  {
>    UINTN       Retry;
> +  UINTN       Address;
> +  UINT32      Data;
>    EFI_STATUS  Status;
> 
>    //
> @@ -1451,22 +1436,28 @@ UfsDeviceDetection (
>    //
>    for (Retry = 0; Retry < 3; Retry++) {
>      Status = UfsExecUicCommands (Private, UfsUicDmeLinkStartup, 0, 0, 0);
> -    if (!EFI_ERROR (Status)) {
> -      break;
> +    if (EFI_ERROR (Status)) {
> +      return EFI_DEVICE_ERROR;
>      }
> 
> -    if (Status == EFI_NOT_FOUND) {
> -      continue;
> +    //
> +    // Check value of HCS.DP and make sure that there is a device attached to
> the Link
> +    //
> +    Address = Private->UfsHcBase + UFS_HC_STATUS_OFFSET;
> +    Data    = MmioRead32 (Address);
> +    if ((Data & UFS_HC_HCS_DP) == 0) {
> +      Address = Private->UfsHcBase + UFS_HC_IS_OFFSET;
> +      Status  = UfsWaitMemSet (Address, UFS_HC_IS_ULSS, UFS_HC_IS_ULSS,
> UFS_TIMEOUT);
> +      if (EFI_ERROR (Status)) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +    } else {
> +      DEBUG ((DEBUG_INFO, "UfsblockioPei: found a attached UFS
> device\n"));
> +      return EFI_SUCCESS;
>      }
> -
> -    return EFI_DEVICE_ERROR;
>    }
> 
> -  if (Retry == 3) {
> -    return EFI_NOT_FOUND;
> -  }
> -
> -  return EFI_SUCCESS;
> +  return EFI_NOT_FOUND;
>  }
> 
>  /**
> --
> 2.31.1.windows.1
> 
> 
> 
> 
>