[PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver

[PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver

[Albecki, Mateusz]

UFS specification allows for additional, implementation specific, programming to be done during host controller initialization. Since some hosts
might require it to allow for device detection we add a way for platform/silicon specific code to pass required attributes and their values to
generic UFS driver. Please see UFS 2.0 specification for details of the host controller initialization sequence(JESD223B section 7.1.1 step 4).
Patchset consists of 3 patches to achieve this. The first one is a bugfix for data transfers that are not aligned to DWORD boundary such as SCSI SENSE
command. The second one is code refactoring for device presence detection that cleans up the function that sends UIC commands. Final patch defines
UFS info protocol and implements driver logic for servicing it.

Tests performed:
-Test UFS LU enumeration with UFS 2.0 card with 3 enabled LUs
-Test that no unaligned data transfers are performed during enumeration
-Test that UIC is programmed according to platform table
-Test that driver operation is not impacted when no UFS info protocol is installed

CC: Wu, Hao A <hao.a.wu@intel.com>

Albecki, Mateusz (3):
  MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
  MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection
  MdeModulePkg/UfsPassThruDxe: Add UFS info protocol

 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  |  62 +++++
 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |   3 +
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf      |   1 +
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 299 +++++++++++++++------
 .../Include/Protocol/UfsHostControllerInfo.h       |  75 ++++++
 MdeModulePkg/MdeModulePkg.dec                      |   3 +
 6 files changed, 356 insertions(+), 87 deletions(-)
 create mode 100644 MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h

-- 
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling

[Albecki, Mateusz]

Since UFS spcification requiers the data buffer specified
in PRDT to be DWORD aligned in size we had a code in
UfsInitUtpPrdt that aligned the data buffer by rounding down
the buffer size to DWORD boundary. This meant that for SCSI
commands that wanted to perform unaligned data transfer(such as
SENSE command) we specified to small buffer for the data to fit
and transfer was aborted. This change introduces code that allocates
auxilary DWORD aligned data buffer for unaligned transfer. Device
transfers data to aligned buffer and when data transfer is over driver copies
data from aligned buffer to data buffer passed by user.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Albecki, Mateusz <mateusz.albecki@intel.com>
---
 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |   1 +
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 190 +++++++++++++++------
 2 files changed, 135 insertions(+), 56 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
index b8937842b8..e591e78661 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
@@ -98,6 +98,7 @@ typedef struct {
   UINT32                                        CmdDescSize;
   VOID                                          *CmdDescHost;
   VOID                                          *CmdDescMapping;
+  VOID                                          *AlignedDataBuf;
   VOID                                          *DataBufMapping;
 
   EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet;
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
index 0238264878..9d0793c6be 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
@@ -400,17 +400,13 @@ UfsInitUtpPrdt (
   UINT8      *Remaining;
   UINTN      PrdtNumber;
 
-  if ((BufferSize & (BIT0 | BIT1)) != 0) {
-    BufferSize &= ~(BIT0 | BIT1);
-    DEBUG ((DEBUG_WARN, "UfsInitUtpPrdt: The BufferSize [%d] is not dword-aligned!\n", BufferSize));
-  }
+  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
+  ASSERT ((BufferSize & (BIT1 | BIT0)) == 0);
 
   if (BufferSize == 0) {
     return EFI_SUCCESS;
   }
 
-  ASSERT (((UINTN)Buffer & (BIT0 | BIT1)) == 0);
-
   RemainingLen = BufferSize;
   Remaining    = Buffer;
   PrdtNumber   = (UINTN)DivU64x32 ((UINT64)BufferSize + UFS_MAX_DATA_LEN_PER_PRD - 1, UFS_MAX_DATA_LEN_PER_PRD);
@@ -1212,8 +1208,6 @@ UfsSetFlag (
   return Status;
 }
 
-
-
 /**
   Read specified flag from a UFS device.
 
@@ -1323,6 +1317,129 @@ Exit:
   return Status;
 }
 
+/**
+  Cleanup data buffers after data transfer. This function
+  also takes care to copy all data to user memory pool for
+  unaligned data transfers.
+
+  @param[in] Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
+  @param[in] TransReq  Pointer to the transfer request
+**/
+VOID
+UfsReconcileDataTransferBuffer (
+  IN UFS_PASS_THRU_PRIVATE_DATA  *Private,
+  IN UFS_PASS_THRU_TRANS_REQ     *TransReq
+  )
+{
+  if (TransReq->DataBufMapping != NULL) {
+    Private->UfsHostController->Unmap (
+                                  Private->UfsHostController,
+                                  TransReq->DataBufMapping
+                                  );
+  }
+
+  //
+  // Check if unaligned transfer was performed. If it was and we read
+  // data from device copy memory to user data buffers before cleanup.
+  // The assumption is if auxilary aligned data buffer is not NULL then
+  // unaligned transfer has been performed.
+  //
+  if (TransReq->AlignedDataBuf != NULL) {
+    if (TransReq->Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+      CopyMem (TransReq->Packet->InDataBuffer, TransReq->AlignedDataBuf, TransReq->Packet->InTransferLength);
+    }
+    FreePool (TransReq->AlignedDataBuf);
+    TransReq->AlignedDataBuf = NULL;
+  }
+}
+
+/**
+  Prepare data buffer for transfer
+
+  @param[in] Private   Pointer to the UFS_PASS_THRU_PRIVATE_DATA
+  @param[in] TransReq  Pointer to the transfer request
+
+  @retval EFI_DEVICE_ERROR  Failed to prepare buffer for transfer
+  @retval EFI_SUCCESS       Buffer ready for transfer
+**/
+EFI_STATUS
+UfsPrepareDataTransferBuffer (
+  IN     UFS_PASS_THRU_PRIVATE_DATA  *Private,
+  IN OUT UFS_PASS_THRU_TRANS_REQ     *TransReq
+  )
+{
+  EFI_STATUS                           Status;
+  VOID                                 *DataBuf;
+  VOID                                 *AlignedDataBuf;
+  UINT32                               DataLen;
+  UINTN                                MapLength;
+  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
+  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
+  UTP_TR_PRD                           *PrdtBase;
+
+  DataBufPhyAddr = (EFI_PHYSICAL_ADDRESS) NULL;
+  AlignedDataBuf = NULL;
+  DataBuf        = NULL;
+
+  //
+  // For unaligned data transfers we allocate auxilary DWORD aligned memory pool.
+  // When command is finished auxilary memory pool is copied into actual user memory.
+  // This is requiered to assure data transfer safety(DWORD alignment required by UFS spec.)
+  //
+  if (TransReq->Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
+    if (TransReq->Packet->InTransferLength % 4 != 0) {
+      DataLen = TransReq->Packet->InTransferLength + (4 - (TransReq->Packet->InTransferLength % 4));
+      DataBuf = AllocateZeroPool (DataLen);
+      if (DataBuf == NULL) {
+        return EFI_DEVICE_ERROR;
+      }
+      TransReq->AlignedDataBuf = DataBuf;
+    } else {
+      DataLen       = TransReq->Packet->InTransferLength;
+      DataBuf       = TransReq->Packet->InDataBuffer;
+    }
+    Flag          = EdkiiUfsHcOperationBusMasterWrite;
+  } else {
+    if (TransReq->Packet->OutTransferLength % 4 != 0) {
+      DataLen = TransReq->Packet->OutTransferLength + (4 - (TransReq->Packet->OutTransferLength % 4));
+      DataBuf = AllocateCopyPool (TransReq->Packet->OutTransferLength, TransReq->Packet->OutDataBuffer);
+      if (DataBuf == NULL) {
+        return EFI_DEVICE_ERROR;
+      }
+      TransReq->AlignedDataBuf = DataBuf;
+    } else {
+      DataLen       = TransReq->Packet->OutTransferLength;
+      DataBuf       = TransReq->Packet->OutDataBuffer;
+    }
+    Flag          = EdkiiUfsHcOperationBusMasterRead;
+  }
+
+  if (DataLen != 0) {
+    MapLength = DataLen;
+    Status    = Private->UfsHostController->Map (
+                                              Private->UfsHostController,
+                                              Flag,
+                                              DataBuf,
+                                              &MapLength,
+                                              &DataBufPhyAddr,
+                                              &TransReq->DataBufMapping
+                                              );
+
+    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
+      return EFI_DEVICE_ERROR;
+    }
+  }
+
+  //
+  // Fill PRDT table of Command UPIU for executed SCSI cmd.
+  //
+  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost + ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof (UTP_RESPONSE_UPIU)));
+  ASSERT (PrdtBase != NULL);
+  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
+
+  return EFI_SUCCESS;
+}
+
 /**
   Sends a UFS-supported SCSI Request Packet to a UFS device that is attached to the UFS host controller.
 
@@ -1359,24 +1476,19 @@ UfsExecScsiCmds (
   UTP_RESPONSE_UPIU                    *Response;
   UINT16                               SenseDataLen;
   UINT32                               ResTranCount;
-  VOID                                 *DataBuf;
-  EFI_PHYSICAL_ADDRESS                 DataBufPhyAddr;
-  UINT32                               DataLen;
-  UINTN                                MapLength;
-  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
-  EDKII_UFS_HOST_CONTROLLER_OPERATION  Flag;
-  UTP_TR_PRD                           *PrdtBase;
   EFI_TPL                              OldTpl;
   UFS_PASS_THRU_TRANS_REQ              *TransReq;
+  EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
 
-  TransReq       = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
+  TransReq = AllocateZeroPool (sizeof (UFS_PASS_THRU_TRANS_REQ));
   if (TransReq == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
 
   TransReq->Signature     = UFS_PASS_THRU_TRANS_REQ_SIG;
   TransReq->TimeoutRemain = Packet->Timeout;
-  DataBufPhyAddr = 0;
+  TransReq->Packet        = Packet;
+
   UfsHc          = Private->UfsHostController;
   //
   // Find out which slot of transfer request list is available.
@@ -1405,44 +1517,16 @@ UfsExecScsiCmds (
 
   TransReq->CmdDescSize = TransReq->Trd->PrdtO * sizeof (UINT32) + TransReq->Trd->PrdtL * sizeof (UTP_TR_PRD);
 
-  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
-    DataBuf       = Packet->InDataBuffer;
-    DataLen       = Packet->InTransferLength;
-    Flag          = EdkiiUfsHcOperationBusMasterWrite;
-  } else {
-    DataBuf       = Packet->OutDataBuffer;
-    DataLen       = Packet->OutTransferLength;
-    Flag          = EdkiiUfsHcOperationBusMasterRead;
-  }
-
-  if (DataLen != 0) {
-    MapLength = DataLen;
-    Status    = UfsHc->Map (
-                         UfsHc,
-                         Flag,
-                         DataBuf,
-                         &MapLength,
-                         &DataBufPhyAddr,
-                         &TransReq->DataBufMapping
-                         );
-
-    if (EFI_ERROR (Status) || (DataLen != MapLength)) {
-      goto Exit1;
-    }
+  Status = UfsPrepareDataTransferBuffer (Private, TransReq);
+  if (EFI_ERROR (Status)) {
+    goto Exit1;
   }
-  //
-  // Fill PRDT table of Command UPIU for executed SCSI cmd.
-  //
-  PrdtBase = (UTP_TR_PRD*)((UINT8*)TransReq->CmdDescHost + ROUNDUP8 (sizeof (UTP_COMMAND_UPIU)) + ROUNDUP8 (sizeof (UTP_RESPONSE_UPIU)));
-  ASSERT (PrdtBase != NULL);
-  UfsInitUtpPrdt (PrdtBase, (VOID*)(UINTN)DataBufPhyAddr, DataLen);
 
   //
   // Insert the async SCSI cmd to the Async I/O list
   //
   if (Event != NULL) {
     OldTpl = gBS->RaiseTPL (TPL_NOTIFY);
-    TransReq->Packet      = Packet;
     TransReq->CallerEvent = Event;
     InsertTailList (&Private->Queue, &TransReq->TransferList);
     gBS->RestoreTPL (OldTpl);
@@ -1521,9 +1605,7 @@ Exit:
 
   UfsStopExecCmd (Private, TransReq->Slot);
 
-  if (TransReq->DataBufMapping != NULL) {
-    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
-  }
+  UfsReconcileDataTransferBuffer (Private, TransReq);
 
 Exit1:
   if (TransReq->CmdDescMapping != NULL) {
@@ -1538,7 +1620,6 @@ Exit1:
   return Status;
 }
 
-
 /**
   Sent UIC DME_LINKSTARTUP command to start the link startup procedure.
 
@@ -2106,7 +2187,6 @@ UfsControllerStop (
   return EFI_SUCCESS;
 }
 
-
 /**
   Internal helper function which will signal the caller event and clean up
   resources.
@@ -2138,9 +2218,7 @@ SignalCallerEvent (
 
   UfsStopExecCmd (Private, TransReq->Slot);
 
-  if (TransReq->DataBufMapping != NULL) {
-    UfsHc->Unmap (UfsHc, TransReq->DataBufMapping);
-  }
+  UfsReconcileDataTransferBuffer (Private, TransReq);
 
   if (TransReq->CmdDescMapping != NULL) {
     UfsHc->Unmap (UfsHc, TransReq->CmdDescMapping);
-- 
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection

[Albecki, Mateusz]

In current implementation we are checking for device presence every
time we execute UIC command. To make UfsExecUicCommands more generic
checking device presence has been moved to UfsDeviceDetection.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Hao Wu <hao.a.wu@intel.com>
Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
---
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 49 ++++++++--------------
 1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
index 9d0793c6be..c37161c4ae 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
@@ -1621,7 +1621,7 @@ Exit1:
 }
 
 /**
-  Sent UIC DME_LINKSTARTUP command to start the link startup procedure.
+  Send UIC command.
 
   @param[in] Private          The pointer to the UFS_PASS_THRU_PRIVATE_DATA data structure.
   @param[in] UicOpcode        The opcode of the UIC command.
@@ -1716,24 +1716,6 @@ UfsExecUicCommands (
     }
   }
 
-  //
-  // Check value of HCS.DP and make sure that there is a device attached to the Link.
-  //
-  Status = UfsMmioRead32 (Private, UFS_HC_STATUS_OFFSET, &Data);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  if ((Data & UFS_HC_HCS_DP) == 0) {
-    Status  = UfsWaitMemSet (Private, UFS_HC_IS_OFFSET, 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, "UfsPassThruDxe: found a attached UFS device\n"));
-
   return EFI_SUCCESS;
 }
 
@@ -1907,8 +1889,9 @@ UfsDeviceDetection (
   IN  UFS_PASS_THRU_PRIVATE_DATA     *Private
   )
 {
-  UINTN                  Retry;
-  EFI_STATUS             Status;
+  UINTN       Retry;
+  EFI_STATUS  Status;
+  UINT32      Data;
 
   //
   // Start UFS device detection.
@@ -1916,22 +1899,26 @@ 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;
+    Status = UfsMmioRead32 (Private, UFS_HC_STATUS_OFFSET, &Data);
+    if (EFI_ERROR (Status)) {
+      return EFI_DEVICE_ERROR;
     }
 
-    return EFI_DEVICE_ERROR;
-  }
-
-  if (Retry == 3) {
-    return EFI_NOT_FOUND;
+    if ((Data & UFS_HC_HCS_DP) == 0) {
+      Status = UfsWaitMemSet (Private, UFS_HC_IS_OFFSET, UFS_HC_IS_ULSS, UFS_HC_IS_ULSS, UFS_TIMEOUT);
+      if (EFI_ERROR (Status)) {
+        return EFI_DEVICE_ERROR;
+      }
+    } else {
+      return EFI_SUCCESS;
+    }
   }
 
-  return EFI_SUCCESS;
+  return EFI_NOT_FOUND;
 }
 
 /**
-- 
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

Re: [PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection

[Wu, Hao A]

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Thursday, March 28, 2019 9:56 PM
> To: edk2-devel@lists.01.org
> Cc: Albecki, Mateusz; Wu, Hao A
> Subject: [PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device
> presence detection
> 
> In current implementation we are checking for device presence every
> time we execute UIC command. To make UfsExecUicCommands more generic
> checking device presence has been moved to UfsDeviceDetection.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Hao Wu <hao.a.wu@intel.com>
> Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
> ---
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 49 ++++++++--------------
>  1 file changed, 18 insertions(+), 31 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index 9d0793c6be..c37161c4ae 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -1621,7 +1621,7 @@ Exit1:
>  }
> 
>  /**
> -  Sent UIC DME_LINKSTARTUP command to start the link startup procedure.
> +  Send UIC command.
> 
>    @param[in] Private          The pointer to the UFS_PASS_THRU_PRIVATE_DATA
> data structure.
>    @param[in] UicOpcode        The opcode of the UIC command.

Please help to update remove the line:
  @return EFI_NOT_FOUND    The presence of the UFS device isn't detected.

from the description of functions UfsExecUicCommands().

Other than this, the patch is good to me:
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> @@ -1716,24 +1716,6 @@ UfsExecUicCommands (
>      }
>    }
> 
> -  //
> -  // Check value of HCS.DP and make sure that there is a device attached to
> the Link.
> -  //
> -  Status = UfsMmioRead32 (Private, UFS_HC_STATUS_OFFSET, &Data);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  if ((Data & UFS_HC_HCS_DP) == 0) {
> -    Status  = UfsWaitMemSet (Private, UFS_HC_IS_OFFSET, 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, "UfsPassThruDxe: found a attached UFS device\n"));
> -
>    return EFI_SUCCESS;
>  }
> 
> @@ -1907,8 +1889,9 @@ UfsDeviceDetection (
>    IN  UFS_PASS_THRU_PRIVATE_DATA     *Private
>    )
>  {
> -  UINTN                  Retry;
> -  EFI_STATUS             Status;
> +  UINTN       Retry;
> +  EFI_STATUS  Status;
> +  UINT32      Data;
> 
>    //
>    // Start UFS device detection.
> @@ -1916,22 +1899,26 @@ 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;
> +    Status = UfsMmioRead32 (Private, UFS_HC_STATUS_OFFSET, &Data);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_DEVICE_ERROR;
>      }
> 
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  if (Retry == 3) {
> -    return EFI_NOT_FOUND;
> +    if ((Data & UFS_HC_HCS_DP) == 0) {
> +      Status = UfsWaitMemSet (Private, UFS_HC_IS_OFFSET, UFS_HC_IS_ULSS,
> UFS_HC_IS_ULSS, UFS_TIMEOUT);
> +      if (EFI_ERROR (Status)) {
> +        return EFI_DEVICE_ERROR;
> +      }
> +    } else {
> +      return EFI_SUCCESS;
> +    }
>    }
> 
> -  return EFI_SUCCESS;
> +  return EFI_NOT_FOUND;
>  }
> 
>  /**
> --
> 2.14.1.windows.1

[PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol

[Albecki, Mateusz]

UFS host controller specification allows for implementation specific
UIC programming to take place just after host controller enable and before
device detection. Since there is no way for generic driver to anticipate
implementation specific programming we add a UFS info protocol
which allows the implementation specific code to pass this information
to generic driver. UFS info protocol is located by the driver at the
BindingStart call and the supported instance is retained throught entire
driver lifetime. Presence of the protocol is optional.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Hao Wu <hao.a.wu@intel.com>
Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
---
 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  | 62 ++++++++++++++++++
 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |  2 +
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf      |  1 +
 .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 60 +++++++++++++++++
 .../Include/Protocol/UfsHostControllerInfo.h       | 75 ++++++++++++++++++++++
 MdeModulePkg/MdeModulePkg.dec                      |  3 +
 6 files changed, 203 insertions(+)
 create mode 100644 MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h

diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
index ea329618dc..a41079e311 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
@@ -40,6 +40,7 @@ UFS_PASS_THRU_PRIVATE_DATA gUfsPassThruTemplate = {
     UfsRwUfsAttribute
   },
   0,                              // UfsHostController
+  NULL,                           // UfsInfo
   0,                              // UfsHcBase
   0,                              // Capabilities
   0,                              // TaskTag
@@ -776,6 +777,66 @@ UfsFinishDeviceInitialization (
   return EFI_SUCCESS;
 }
 
+/**
+  Locates UFS HC infor protocol suitable for this controller.
+
+  @param[in] DriverBinding     Pointer to driver binding protocol
+  @param[in] Private           Pointer to host controller private data
+  @param[in] ControllerHandle  Controller to which driver is bound
+**/
+VOID
+GetUfsHcInfoProtocol (
+  IN EFI_DRIVER_BINDING_PROTOCOL  *DriverBinding,
+  IN UFS_PASS_THRU_PRIVATE_DATA   *Private,
+  IN EFI_HANDLE                   ControllerHandle
+  )
+{
+  EFI_STATUS            Status;
+  EFI_HANDLE            *ProtocolHandleArray;
+  UINTN                 NoHandles;
+  UINTN                 HandleIndex;
+  UFS_HC_INFO_PROTOCOL  *UfsHcInfo;
+
+  Status = gBS->LocateHandleBuffer (
+                  ByProtocol,
+                  &gUfsHcInfoProtocolGuid,
+                  NULL,
+                  &NoHandles,
+                  &ProtocolHandleArray
+                  );
+  if (EFI_ERROR (Status)) {
+    return;
+  }
+
+  for (HandleIndex = 0; HandleIndex < NoHandles; HandleIndex++) {
+    Status = gBS->OpenProtocol (
+                    ProtocolHandleArray[HandleIndex],
+                    &gUfsHcInfoProtocolGuid,
+                    &UfsHcInfo,
+                    DriverBinding->DriverBindingHandle,
+                    ControllerHandle,
+                    EFI_OPEN_PROTOCOL_BY_DRIVER
+                    );
+    if (EFI_ERROR (Status)) {
+      continue;
+    }
+    if (!EFI_ERROR(UfsHcInfo->Supported (UfsHcInfo, ControllerHandle))) {
+      Private->UfsHcInfo = UfsHcInfo;
+      break;
+    } else {
+      Status = gBS->CloseProtocol (
+                      ProtocolHandleArray[HandleIndex],
+                      &gUfsHcInfoProtocolGuid,
+                      DriverBinding->DriverBindingHandle,
+                      ControllerHandle
+                      );
+      ASSERT_EFI_ERROR (Status);
+    }
+  }
+
+  FreePool (ProtocolHandleArray);
+}
+
 /**
   Starts a device controller or a bus controller.
 
@@ -871,6 +932,7 @@ UfsPassThruDriverBindingStart (
   Private->UfsHostController    = UfsHc;
   Private->UfsHcBase            = UfsHcBase;
   InitializeListHead (&Private->Queue);
+  GetUfsHcInfoProtocol (This, Private, Controller);
 
   //
   // Initialize UFS Host Controller H/W.
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
index e591e78661..55a8ed9bdf 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
@@ -29,6 +29,7 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/DevicePathLib.h>
 #include <Library/TimerLib.h>
+#include <Protocol/UfsHostControllerInfo.h>
 
 #include "UfsPassThruHci.h"
 
@@ -66,6 +67,7 @@ typedef struct _UFS_PASS_THRU_PRIVATE_DATA {
   EFI_EXT_SCSI_PASS_THRU_PROTOCOL     ExtScsiPassThru;
   EFI_UFS_DEVICE_CONFIG_PROTOCOL      UfsDevConfig;
   EDKII_UFS_HOST_CONTROLLER_PROTOCOL  *UfsHostController;
+  UFS_HC_INFO_PROTOCOL                *UfsHcInfo;
   UINTN                               UfsHcBase;
   UINT32                              Capabilities;
 
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
index e550cd02b4..12b01771ff 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
@@ -59,6 +59,7 @@
   gEfiExtScsiPassThruProtocolGuid               ## BY_START
   gEfiUfsDeviceConfigProtocolGuid               ## BY_START
   gEdkiiUfsHostControllerProtocolGuid           ## TO_START
+  gUfsHcInfoProtocolGuid
 
 [UserExtensions.TianoCore."ExtraFiles"]
   UfsPassThruExtra.uni
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
index c37161c4ae..125f2f2516 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
@@ -2068,6 +2068,61 @@ UfsInitTransferRequestList (
   return EFI_SUCCESS;
 }
 
+/**
+  Performs additional UIC programming if it has been specified by platform in UFS info protocol.
+
+  @param[in] Private  Pointer to host controller private data
+
+  @retval EFI_SUCCESS  Programming finished successfully or not requested
+  @retval others       Programming failed
+**/
+EFI_STATUS
+UfsProgramAdditionalUicAttributes (
+  IN UFS_PASS_THRU_PRIVATE_DATA  *Private
+  )
+{
+  EFI_STATUS     Status;
+  UIC_ATTRIBUTE  *UicAttrArray;
+  UINTN          NoAttributes;
+  UINTN          AttributeIndex;
+
+  //
+  // No info protocol means that no additional programming should be performed
+  //
+  if (Private->UfsHcInfo == NULL) {
+    return EFI_SUCCESS;
+  }
+
+  //
+  // If we failed to get the programming table we assume that platform doesn't want to do any additional programming
+  //
+  Status = Private->UfsHcInfo->GetUicProgrammingTable (Private->UfsHcInfo, &UicAttrArray, &NoAttributes);
+  if (EFI_ERROR (Status)) {
+    return EFI_SUCCESS;
+  }
+
+  for (AttributeIndex = 0; AttributeIndex < NoAttributes; AttributeIndex++) {
+    DEBUG ((DEBUG_ERROR, "Programing UIC attribute = %d, selector index = %d, set type = %d, value = %d\n",
+                          UicAttrArray[AttributeIndex].MibAttribute, UicAttrArray[AttributeIndex].GenSelectorIndex,
+                          UicAttrArray[AttributeIndex].AttrSetType, UicAttrArray[AttributeIndex].AttributeValue));
+    Status = UfsExecUicCommands (
+               Private,
+               UfsUicDmeSet,
+               (UicAttrArray[AttributeIndex].MibAttribute << 16) | (UicAttrArray[AttributeIndex].GenSelectorIndex),
+               UicAttrArray[AttributeIndex].AttrSetType << 16,
+               UicAttrArray[AttributeIndex].AttributeValue
+               );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "Failed to program UIC attribute = %d, selector index = %d, set type = %d, value = %d\n",
+                            UicAttrArray[AttributeIndex].MibAttribute, UicAttrArray[AttributeIndex].GenSelectorIndex,
+                            UicAttrArray[AttributeIndex].AttrSetType, UicAttrArray[AttributeIndex].AttributeValue));
+      return Status;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
 /**
   Initialize the UFS host controller.
 
@@ -2090,6 +2145,11 @@ UfsControllerInit (
     return Status;
   }
 
+  Status = UfsProgramAdditionalUicAttributes (Private);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "UfsControllerInit: Failed to perform additional UIC programming\n"));
+  }
+
   Status = UfsDeviceDetection (Private);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "UfsControllerInit: Device Detection Fails, Status = %r\n", Status));
diff --git a/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
new file mode 100644
index 0000000000..c730127482
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
@@ -0,0 +1,75 @@
+/** @file
+
+  Universal Flash Storage Host Controller info Protocol.
+
+Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+This program and the accompanying materials are licensed and made available under
+the terms and conditions of the BSD License that accompanies this distribution.
+The full text of the license may be found at
+http://opensource.org/licenses/bsd-license.php.
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __UFS_HC_INFO_PROTOCOL_H__
+#define __UFS_HC_INFO_PROTOCOL_H__
+
+#define UFS_HC_INFO_PROTOCOL_VERSION  1
+
+extern EFI_GUID gUfsHcInfoProtocolGuid;
+
+typedef struct _UFS_HC_INFO_PROTOCOL UFS_HC_INFO_PROTOCOL;
+
+typedef struct {
+  UINT16  MibAttribute;
+  UINT16  GenSelectorIndex;
+  UINT8   AttrSetType;
+  UINT32  AttributeValue;
+} UIC_ATTRIBUTE;
+
+/**
+  Checks if this instance of the info protocol supports
+  given host controller.
+
+  @param[in] This              Pointer to this instance of UFS_HC_INFO_PROTOCOL
+  @param[in] ControllerHandle  Controller to check
+
+  @retval EFI_SUCCESS  This instance of info protocol supports given controller
+  @retval others       This instance of info protocol doesn't support given controller
+**/
+typedef
+EFI_STATUS
+(*UFS_INFO_CONTROLLER_SUPPORTED) (
+  IN UFS_HC_INFO_PROTOCOL  *This,
+  IN EFI_HANDLE            ControllerHandle
+  );
+
+/**
+  Get the UIC programming table to be used just after host controller
+  enabling and before device detection.
+
+  @param[in] This               Pointer to this instance of UFS_HC_INFO_PROTOCOL
+  @param[in] UicAttributeArray  Out variable for address of the table
+  @param[in] NoAttributes       Out variable for number of attributes in the array
+
+  @retval EFI_SUCCESS  Table successfully found and returned
+  @retval others       Table wasn't located successfully UIC programming shouldn't be performed.
+**/
+typedef
+EFI_STATUS
+(*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
+  IN  UFS_HC_INFO_PROTOCOL  *This,
+  OUT UIC_ATTRIBUTE         **UicAttributeArray,
+  OUT UINTN                 *NoAttributes
+  );
+
+struct _UFS_HC_INFO_PROTOCOL {
+  UINT32                              Version;
+  UFS_INFO_CONTROLLER_SUPPORTED       Supported;
+  UFS_INFO_GET_UIC_PROGRAMMING_TABLE  GetUicProgrammingTable;
+};
+
+#endif
+
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index a2130bc439..c6be8f12a4 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -581,6 +581,9 @@
   ## Include/Protocol/UfsHostController.h
   gEdkiiUfsHostControllerProtocolGuid = { 0xebc01af5, 0x7a9, 0x489e, { 0xb7, 0xce, 0xdc, 0x8, 0x9e, 0x45, 0x9b, 0x2f } }
 
+  ## Include/Protocol/UfsHostControllerInfo.h
+  gUfsHcInfoProtocolGuid = { 0x3d18ba13, 0xd9b1, 0x4dd4, {0xb9, 0x16, 0xd3, 0x07, 0x96, 0x53, 0x9e, 0xd8}}
+
   ## Include/Protocol/EsrtManagement.h
   gEsrtManagementProtocolGuid         = { 0xa340c064, 0x723c, 0x4a9c, { 0xa4, 0xdd, 0xd5, 0xb4, 0x7a, 0x26, 0xfb, 0xb0 }}
 
-- 
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol

[Wu, Hao A]

Hello Mateusz,

A couple of general comments:

A. I suggest to break this commit into 2 patches:

The first one will just introduce the new protocol.
The second one will update the UfsPassThruDxe driver to consume this new
protocol.


B. There has been a Bugzilla tracker for the feature you add in this
patch:
https://bugzilla.tianocore.org/show_bug.cgi?id=1343

Could you help to add this information in the commit log message? Thanks.


C. I prefer the new protocol to have platform level singleton instance:

I do not see much value for a platform to produce multiple instances of
this protocol, I think the additional UIC commands needed during the UFS
HC initialization for a specific platform is deterministic.

Also, the selection of protocol instance implemented in function
GetUfsHcInfoProtocol() is by:
  1) LocateHandleBuffer() to get all protocol instances;
  2) Choose the 1st instance if the 'Supported' service returns without
     error.

I think this will bring some kind of confusion to the protocol producers,
since they do not know which one will be selected when there are multiple
instances of the protocol.


> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Thursday, March 28, 2019 9:56 PM
> To: edk2-devel@lists.01.org
> Cc: Albecki, Mateusz; Wu, Hao A
> Subject: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol
> 
> UFS host controller specification allows for implementation specific
> UIC programming to take place just after host controller enable and before
> device detection. Since there is no way for generic driver to anticipate
> implementation specific programming we add a UFS info protocol
> which allows the implementation specific code to pass this information
> to generic driver. UFS info protocol is located by the driver at the
> BindingStart call and the supported instance is retained throught entire

throught -> through


> driver lifetime. Presence of the protocol is optional.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Hao Wu <hao.a.wu@intel.com>
> Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
> ---
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  | 62
> ++++++++++++++++++
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |  2 +
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf      |  1 +
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 60 +++++++++++++++++
>  .../Include/Protocol/UfsHostControllerInfo.h       | 75
> ++++++++++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec                      |  3 +
>  6 files changed, 203 insertions(+)
>  create mode 100644 MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> index ea329618dc..a41079e311 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> @@ -40,6 +40,7 @@ UFS_PASS_THRU_PRIVATE_DATA gUfsPassThruTemplate
> = {
>      UfsRwUfsAttribute
>    },
>    0,                              // UfsHostController
> +  NULL,                           // UfsInfo
>    0,                              // UfsHcBase
>    0,                              // Capabilities
>    0,                              // TaskTag
> @@ -776,6 +777,66 @@ UfsFinishDeviceInitialization (
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Locates UFS HC infor protocol suitable for this controller.

infor -> info


> +
> +  @param[in] DriverBinding     Pointer to driver binding protocol
> +  @param[in] Private           Pointer to host controller private data
> +  @param[in] ControllerHandle  Controller to which driver is bound
> +**/
> +VOID
> +GetUfsHcInfoProtocol (
> +  IN EFI_DRIVER_BINDING_PROTOCOL  *DriverBinding,
> +  IN UFS_PASS_THRU_PRIVATE_DATA   *Private,
> +  IN EFI_HANDLE                   ControllerHandle
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_HANDLE            *ProtocolHandleArray;
> +  UINTN                 NoHandles;
> +  UINTN                 HandleIndex;
> +  UFS_HC_INFO_PROTOCOL  *UfsHcInfo;
> +
> +  Status = gBS->LocateHandleBuffer (
> +                  ByProtocol,
> +                  &gUfsHcInfoProtocolGuid,
> +                  NULL,
> +                  &NoHandles,
> +                  &ProtocolHandleArray
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +
> +  for (HandleIndex = 0; HandleIndex < NoHandles; HandleIndex++) {
> +    Status = gBS->OpenProtocol (
> +                    ProtocolHandleArray[HandleIndex],
> +                    &gUfsHcInfoProtocolGuid,
> +                    &UfsHcInfo,
> +                    DriverBinding->DriverBindingHandle,
> +                    ControllerHandle,
> +                    EFI_OPEN_PROTOCOL_BY_DRIVER
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      continue;
> +    }
> +    if (!EFI_ERROR(UfsHcInfo->Supported (UfsHcInfo, ControllerHandle))) {
> +      Private->UfsHcInfo = UfsHcInfo;
> +      break;
> +    } else {
> +      Status = gBS->CloseProtocol (
> +                      ProtocolHandleArray[HandleIndex],
> +                      &gUfsHcInfoProtocolGuid,
> +                      DriverBinding->DriverBindingHandle,
> +                      ControllerHandle
> +                      );
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +  }
> +
> +  FreePool (ProtocolHandleArray);
> +}
> +
>  /**
>    Starts a device controller or a bus controller.
> 
> @@ -871,6 +932,7 @@ UfsPassThruDriverBindingStart (
>    Private->UfsHostController    = UfsHc;
>    Private->UfsHcBase            = UfsHcBase;
>    InitializeListHead (&Private->Queue);
> +  GetUfsHcInfoProtocol (This, Private, Controller);
> 
>    //
>    // Initialize UFS Host Controller H/W.
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> index e591e78661..55a8ed9bdf 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> @@ -29,6 +29,7 @@
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/DevicePathLib.h>
>  #include <Library/TimerLib.h>
> +#include <Protocol/UfsHostControllerInfo.h>

A minor comment here:
You can put the above '#include' together with other protocol header files
rather than appending it after the libraries includes.


> 
>  #include "UfsPassThruHci.h"
> 
> @@ -66,6 +67,7 @@ typedef struct _UFS_PASS_THRU_PRIVATE_DATA {
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL     ExtScsiPassThru;
>    EFI_UFS_DEVICE_CONFIG_PROTOCOL      UfsDevConfig;
>    EDKII_UFS_HOST_CONTROLLER_PROTOCOL  *UfsHostController;
> +  UFS_HC_INFO_PROTOCOL                *UfsHcInfo;
>    UINTN                               UfsHcBase;
>    UINT32                              Capabilities;
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> index e550cd02b4..12b01771ff 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> @@ -59,6 +59,7 @@
>    gEfiExtScsiPassThruProtocolGuid               ## BY_START
>    gEfiUfsDeviceConfigProtocolGuid               ## BY_START
>    gEdkiiUfsHostControllerProtocolGuid           ## TO_START
> +  gUfsHcInfoProtocolGuid

gUfsHcInfoProtocolGuid ->
gUfsHcInfoProtocolGuid    ## SOMETIMES_CONSUMES


> 
>  [UserExtensions.TianoCore."ExtraFiles"]
>    UfsPassThruExtra.uni
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index c37161c4ae..125f2f2516 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -2068,6 +2068,61 @@ UfsInitTransferRequestList (
>    return EFI_SUCCESS;
>  }
> 
> +/**
> +  Performs additional UIC programming if it has been specified by platform in
> UFS info protocol.
> +
> +  @param[in] Private  Pointer to host controller private data
> +
> +  @retval EFI_SUCCESS  Programming finished successfully or not requested
> +  @retval others       Programming failed
> +**/
> +EFI_STATUS
> +UfsProgramAdditionalUicAttributes (
> +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private
> +  )
> +{
> +  EFI_STATUS     Status;
> +  UIC_ATTRIBUTE  *UicAttrArray;
> +  UINTN          NoAttributes;
> +  UINTN          AttributeIndex;
> +
> +  //
> +  // No info protocol means that no additional programming should be
> performed
> +  //
> +  if (Private->UfsHcInfo == NULL) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  //
> +  // If we failed to get the programming table we assume that platform
> doesn't want to do any additional programming
> +  //
> +  Status = Private->UfsHcInfo->GetUicProgrammingTable (Private->UfsHcInfo,
> &UicAttrArray, &NoAttributes);
> +  if (EFI_ERROR (Status)) {
> +    return EFI_SUCCESS;
> +  }

Please help to free the content pointed by 'UicAttrArray'.


> +
> +  for (AttributeIndex = 0; AttributeIndex < NoAttributes; AttributeIndex++) {
> +    DEBUG ((DEBUG_ERROR, "Programing UIC attribute = %d, selector index

DEBUG_INFO should be enough.


> = %d, set type = %d, value = %d\n",
> +                          UicAttrArray[AttributeIndex].MibAttribute,
> UicAttrArray[AttributeIndex].GenSelectorIndex,
> +                          UicAttrArray[AttributeIndex].AttrSetType,
> UicAttrArray[AttributeIndex].AttributeValue));
> +    Status = UfsExecUicCommands (
> +               Private,
> +               UfsUicDmeSet,
> +               (UicAttrArray[AttributeIndex].MibAttribute << 16) |
> (UicAttrArray[AttributeIndex].GenSelectorIndex),
> +               UicAttrArray[AttributeIndex].AttrSetType << 16,
> +               UicAttrArray[AttributeIndex].AttributeValue
> +               );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "Failed to program UIC attribute = %d, selector
> index = %d, set type = %d, value = %d\n",
> +                            UicAttrArray[AttributeIndex].MibAttribute,
> UicAttrArray[AttributeIndex].GenSelectorIndex,
> +                            UicAttrArray[AttributeIndex].AttrSetType,
> UicAttrArray[AttributeIndex].AttributeValue));
> +      return Status;
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Initialize the UFS host controller.
> 
> @@ -2090,6 +2145,11 @@ UfsControllerInit (
>      return Status;
>    }
> 
> +  Status = UfsProgramAdditionalUicAttributes (Private);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "UfsControllerInit: Failed to perform additional
> UIC programming\n"));
> +  }
> +
>    Status = UfsDeviceDetection (Private);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "UfsControllerInit: Device Detection Fails, Status
> = %r\n", Status));
> diff --git a/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> new file mode 100644
> index 0000000000..c730127482
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> @@ -0,0 +1,75 @@
> +/** @file
> +
> +  Universal Flash Storage Host Controller info Protocol.
> +
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +This program and the accompanying materials are licensed and made
> available under
> +the terms and conditions of the BSD License that accompanies this distribution.
> +The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php.
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
> OR IMPLIED.
> +
> +**/
> +
> +#ifndef __UFS_HC_INFO_PROTOCOL_H__
> +#define __UFS_HC_INFO_PROTOCOL_H__
> +
> +#define UFS_HC_INFO_PROTOCOL_VERSION  1
> +
> +extern EFI_GUID gUfsHcInfoProtocolGuid;

I suggest to rename gUfsHcInfoProtocolGuid to:
gEdkiiUfsHcInfoProtocolGuid

which I think is a common naming convention for non UEFI spec protocols
within MdeModulePkg.


> +
> +typedef struct _UFS_HC_INFO_PROTOCOL UFS_HC_INFO_PROTOCOL;
> +
> +typedef struct {
> +  UINT16  MibAttribute;
> +  UINT16  GenSelectorIndex;
> +  UINT8   AttrSetType;
> +  UINT32  AttributeValue;
> +} UIC_ATTRIBUTE;
> +
> +/**
> +  Checks if this instance of the info protocol supports
> +  given host controller.
> +
> +  @param[in] This              Pointer to this instance of
> UFS_HC_INFO_PROTOCOL
> +  @param[in] ControllerHandle  Controller to check
> +
> +  @retval EFI_SUCCESS  This instance of info protocol supports given controller
> +  @retval others       This instance of info protocol doesn't support given
> controller
> +**/
> +typedef
> +EFI_STATUS
> +(*UFS_INFO_CONTROLLER_SUPPORTED) (
> +  IN UFS_HC_INFO_PROTOCOL  *This,
> +  IN EFI_HANDLE            ControllerHandle
> +  );
> +
> +/**
> +  Get the UIC programming table to be used just after host controller
> +  enabling and before device detection.
> +
> +  @param[in] This               Pointer to this instance of
> UFS_HC_INFO_PROTOCOL
> +  @param[in] UicAttributeArray  Out variable for address of the table
> +  @param[in] NoAttributes       Out variable for number of attributes in the
> array

'@param[out]' for UicAttributeArray & NoAttributes.

Also, for 'UicAttributeArray', I think the content pointed by this pointer
is allocated by the protocol producer and should be freed by the consumer
after being used, right?

If so, please help to refine the description comments for
'UicAttributeArray'.


> +
> +  @retval EFI_SUCCESS  Table successfully found and returned
> +  @retval others       Table wasn't located successfully UIC programming
> shouldn't be performed.
> +**/
> +typedef
> +EFI_STATUS
> +(*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
> +  IN  UFS_HC_INFO_PROTOCOL  *This,
> +  OUT UIC_ATTRIBUTE         **UicAttributeArray,
> +  OUT UINTN                 *NoAttributes
> +  );

Judging from the usage of this service in function
UfsProgramAdditionalUicAttributes(), I think there is an assumption that
platforms will only require additional 'DME_SET' UIC commands.

I am not sure if we need to make this service a bit more general. So how
about changing the 'UIC_ATTRIBUTE' structure to:

typedef struct {
  UINT8   UicOpcode;
  UINT32  Arg1;
  UINT32  Arg2;
  UINT32  Arg3;
} UIC_COMMAND;

and update the service to:

typedef
EFI_STATUS
(*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
  IN  UFS_HC_INFO_PROTOCOL  *This,
  OUT UIC_COMMAND           **UicCommandArray,
  OUT UINTN                 *NoCommand
  );


> +
> +struct _UFS_HC_INFO_PROTOCOL {
> +  UINT32                              Version;
> +  UFS_INFO_CONTROLLER_SUPPORTED       Supported;

If there will be only one protocol instance within system, do you think we
can drop the above 'Supported' service?

Best Regards,
Hao Wu


> +  UFS_INFO_GET_UIC_PROGRAMMING_TABLE  GetUicProgrammingTable;
> +};
> +
> +#endif
> +
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index a2130bc439..c6be8f12a4 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -581,6 +581,9 @@
>    ## Include/Protocol/UfsHostController.h
>    gEdkiiUfsHostControllerProtocolGuid = { 0xebc01af5, 0x7a9, 0x489e, { 0xb7,
> 0xce, 0xdc, 0x8, 0x9e, 0x45, 0x9b, 0x2f } }
> 
> +  ## Include/Protocol/UfsHostControllerInfo.h
> +  gUfsHcInfoProtocolGuid = { 0x3d18ba13, 0xd9b1, 0x4dd4, {0xb9, 0x16, 0xd3,
> 0x07, 0x96, 0x53, 0x9e, 0xd8}}
> +
>    ## Include/Protocol/EsrtManagement.h
>    gEsrtManagementProtocolGuid         = { 0xa340c064, 0x723c, 0x4a9c, { 0xa4,
> 0xdd, 0xd5, 0xb4, 0x7a, 0x26, 0xfb, 0xb0 }}
> 
> --
> 2.14.1.windows.1

Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol

[Wu, Hao A]

Resend this one to the new mailing list.

Hello Mateusz,

Incline comments below.

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Thursday, April 11, 2019 9:39 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Ni, Ray
> Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info
> protocol
> 
> Hi,
> 
> I will be addressing most of the comments in next patch however I want to
> discuss 2 things.
> 
> 1. I really thing that supporting multiple instances of the protocol is a better
> way here. I can imagine a board which has 2 UFS controllers one integrated
> into the SoC and the second one connected through PCI(maybe for debug or
> maybe that's the production configuration due to problems with the
> integrated controller). I would like the company that produced the discrete
> UFS controller to be able to deliver a efi binary which will install this protocol
> just for their device(identified by device id) while not conflicting with the
> code that installs configuration for the integrated controller. With single
> instance we would need BIOS team working on the product to modify their
> code instead of just dropping in the binary. This use case is a little exotic but I
> think it's valid especially if we ever consider extending this protocol. Choosing
> the first instance that returns supported should be clear once I add
> description in the protocol header.

For controllers attached through the PCI, my understanding is that this
binary will reside in the option ROM. If the device produce company want
to customize the initialization process, I think they can directly put a
different controller/device driver binary in the ROM instead. The Bus
Specific Driver Override Protocol ensures the driver in the option ROM
will manage the device. So I think for BIOS, it is reasonable to focus on
the integrated one.

> 
> 2. About making the service more general and allowing the producer of the
> protocol to choose UIC command to execute. I only tried to satisfy the UFS
> specification and I am not sure what possible use cases platform could have
> that would require sending commands other than DME_SET so maybe we
> should refrain from extending it now? Extension would also raise questions
> how to handle DME_GET command for instance since right now we do not
> have any callback to platform producer.

I am not very sure about this one, what I found in the UFSHCI spec seems
that the spec does not explicitly forbid other UIC to be sent.

According to UFSHCI spec Version 2.1, Section 7.1.1:

> Additional commands, such as DME_SET commands may be sent from the
> system host to the UFS host controller to provide configuration
> flexibility.

If you think commands other than 'DME_SET' will not make much sense here,
I am okay with the current 'UIC_ATTRIBUTE' definition.

(I slightly prefer using 'Arg1'~'Arg3' in structure 'UIC_ATTRIBUTE', it
seems a little bit cleaner to me. But this is only my thought and you may
choose your own implementation for this.)

Best Regards,
Hao Wu

> 
> Thanks,
> Mateusz
> 
> -----Original Message-----
> From: Wu, Hao A
> Sent: Monday, April 1, 2019 8:53 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info
> protocol
> 
> Hello Mateusz,
> 
> A couple of general comments:
> 
> A. I suggest to break this commit into 2 patches:
> 
> The first one will just introduce the new protocol.
> The second one will update the UfsPassThruDxe driver to consume this new
> protocol.
> 
> 
> B. There has been a Bugzilla tracker for the feature you add in this
> patch:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1343
> 
> Could you help to add this information in the commit log message? Thanks.
> 
> 
> C. I prefer the new protocol to have platform level singleton instance:
> 
> I do not see much value for a platform to produce multiple instances of this
> protocol, I think the additional UIC commands needed during the UFS HC
> initialization for a specific platform is deterministic.
> 
> Also, the selection of protocol instance implemented in function
> GetUfsHcInfoProtocol() is by:
>   1) LocateHandleBuffer() to get all protocol instances;
>   2) Choose the 1st instance if the 'Supported' service returns without
>      error.
> 
> I think this will bring some kind of confusion to the protocol producers, since
> they do not know which one will be selected when there are multiple
> instances of the protocol.
> 
> 
> > -----Original Message-----
> > From: Albecki, Mateusz
> > Sent: Thursday, March 28, 2019 9:56 PM
> > To: edk2-devel@lists.01.org
> > Cc: Albecki, Mateusz; Wu, Hao A
> > Subject: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info
> > protocol
> >
> > UFS host controller specification allows for implementation specific
> > UIC programming to take place just after host controller enable and
> > before device detection. Since there is no way for generic driver to
> > anticipate implementation specific programming we add a UFS info
> > protocol which allows the implementation specific code to pass this
> > information to generic driver. UFS info protocol is located by the
> > driver at the BindingStart call and the supported instance is retained
> > throught entire
> 
> throught -> through
> 
> 
> > driver lifetime. Presence of the protocol is optional.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
> > ---
> >  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  | 62
> > ++++++++++++++++++
> >  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |  2 +
> >  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf      |  1 +
> >  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 60
> +++++++++++++++++
> >  .../Include/Protocol/UfsHostControllerInfo.h       | 75
> > ++++++++++++++++++++++
> >  MdeModulePkg/MdeModulePkg.dec                      |  3 +
> >  6 files changed, 203 insertions(+)
> >  create mode 100644
> > MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> >
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > index ea329618dc..a41079e311 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > @@ -40,6 +40,7 @@ UFS_PASS_THRU_PRIVATE_DATA
> gUfsPassThruTemplate = {
> >      UfsRwUfsAttribute
> >    },
> >    0,                              // UfsHostController
> > +  NULL,                           // UfsInfo
> >    0,                              // UfsHcBase
> >    0,                              // Capabilities
> >    0,                              // TaskTag
> > @@ -776,6 +777,66 @@ UfsFinishDeviceInitialization (
> >    return EFI_SUCCESS;
> >  }
> >
> > +/**
> > +  Locates UFS HC infor protocol suitable for this controller.
> 
> infor -> info
> 
> 
> > +
> > +  @param[in] DriverBinding     Pointer to driver binding protocol
> > +  @param[in] Private           Pointer to host controller private data
> > +  @param[in] ControllerHandle  Controller to which driver is bound
> > +**/ VOID GetUfsHcInfoProtocol (
> > +  IN EFI_DRIVER_BINDING_PROTOCOL  *DriverBinding,
> > +  IN UFS_PASS_THRU_PRIVATE_DATA   *Private,
> > +  IN EFI_HANDLE                   ControllerHandle
> > +  )
> > +{
> > +  EFI_STATUS            Status;
> > +  EFI_HANDLE            *ProtocolHandleArray;
> > +  UINTN                 NoHandles;
> > +  UINTN                 HandleIndex;
> > +  UFS_HC_INFO_PROTOCOL  *UfsHcInfo;
> > +
> > +  Status = gBS->LocateHandleBuffer (
> > +                  ByProtocol,
> > +                  &gUfsHcInfoProtocolGuid,
> > +                  NULL,
> > +                  &NoHandles,
> > +                  &ProtocolHandleArray
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    return;
> > +  }
> > +
> > +  for (HandleIndex = 0; HandleIndex < NoHandles; HandleIndex++) {
> > +    Status = gBS->OpenProtocol (
> > +                    ProtocolHandleArray[HandleIndex],
> > +                    &gUfsHcInfoProtocolGuid,
> > +                    &UfsHcInfo,
> > +                    DriverBinding->DriverBindingHandle,
> > +                    ControllerHandle,
> > +                    EFI_OPEN_PROTOCOL_BY_DRIVER
> > +                    );
> > +    if (EFI_ERROR (Status)) {
> > +      continue;
> > +    }
> > +    if (!EFI_ERROR(UfsHcInfo->Supported (UfsHcInfo, ControllerHandle))) {
> > +      Private->UfsHcInfo = UfsHcInfo;
> > +      break;
> > +    } else {
> > +      Status = gBS->CloseProtocol (
> > +                      ProtocolHandleArray[HandleIndex],
> > +                      &gUfsHcInfoProtocolGuid,
> > +                      DriverBinding->DriverBindingHandle,
> > +                      ControllerHandle
> > +                      );
> > +      ASSERT_EFI_ERROR (Status);
> > +    }
> > +  }
> > +
> > +  FreePool (ProtocolHandleArray);
> > +}
> > +
> >  /**
> >    Starts a device controller or a bus controller.
> >
> > @@ -871,6 +932,7 @@ UfsPassThruDriverBindingStart (
> >    Private->UfsHostController    = UfsHc;
> >    Private->UfsHcBase            = UfsHcBase;
> >    InitializeListHead (&Private->Queue);
> > +  GetUfsHcInfoProtocol (This, Private, Controller);
> >
> >    //
> >    // Initialize UFS Host Controller H/W.
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > index e591e78661..55a8ed9bdf 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > @@ -29,6 +29,7 @@
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/DevicePathLib.h>
> >  #include <Library/TimerLib.h>
> > +#include <Protocol/UfsHostControllerInfo.h>
> 
> A minor comment here:
> You can put the above '#include' together with other protocol header files
> rather than appending it after the libraries includes.
> 
> 
> >
> >  #include "UfsPassThruHci.h"
> >
> > @@ -66,6 +67,7 @@ typedef struct _UFS_PASS_THRU_PRIVATE_DATA {
> >    EFI_EXT_SCSI_PASS_THRU_PROTOCOL     ExtScsiPassThru;
> >    EFI_UFS_DEVICE_CONFIG_PROTOCOL      UfsDevConfig;
> >    EDKII_UFS_HOST_CONTROLLER_PROTOCOL  *UfsHostController;
> > +  UFS_HC_INFO_PROTOCOL                *UfsHcInfo;
> >    UINTN                               UfsHcBase;
> >    UINT32                              Capabilities;
> >
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > index e550cd02b4..12b01771ff 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > @@ -59,6 +59,7 @@
> >    gEfiExtScsiPassThruProtocolGuid               ## BY_START
> >    gEfiUfsDeviceConfigProtocolGuid               ## BY_START
> >    gEdkiiUfsHostControllerProtocolGuid           ## TO_START
> > +  gUfsHcInfoProtocolGuid
> 
> gUfsHcInfoProtocolGuid ->
> gUfsHcInfoProtocolGuid    ## SOMETIMES_CONSUMES
> 
> 
> >
> >  [UserExtensions.TianoCore."ExtraFiles"]
> >    UfsPassThruExtra.uni
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > index c37161c4ae..125f2f2516 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > @@ -2068,6 +2068,61 @@ UfsInitTransferRequestList (
> >    return EFI_SUCCESS;
> >  }
> >
> > +/**
> > +  Performs additional UIC programming if it has been specified by
> > +platform in
> > UFS info protocol.
> > +
> > +  @param[in] Private  Pointer to host controller private data
> > +
> > +  @retval EFI_SUCCESS  Programming finished successfully or not
> requested
> > +  @retval others       Programming failed
> > +**/
> > +EFI_STATUS
> > +UfsProgramAdditionalUicAttributes (
> > +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private
> > +  )
> > +{
> > +  EFI_STATUS     Status;
> > +  UIC_ATTRIBUTE  *UicAttrArray;
> > +  UINTN          NoAttributes;
> > +  UINTN          AttributeIndex;
> > +
> > +  //
> > +  // No info protocol means that no additional programming should be
> > performed
> > +  //
> > +  if (Private->UfsHcInfo == NULL) {
> > +    return EFI_SUCCESS;
> > +  }
> > +
> > +  //
> > +  // If we failed to get the programming table we assume that
> > + platform
> > doesn't want to do any additional programming
> > +  //
> > +  Status = Private->UfsHcInfo->GetUicProgrammingTable
> > + (Private->UfsHcInfo,
> > &UicAttrArray, &NoAttributes);
> > +  if (EFI_ERROR (Status)) {
> > +    return EFI_SUCCESS;
> > +  }
> 
> Please help to free the content pointed by 'UicAttrArray'.
> 
> 
> > +
> > +  for (AttributeIndex = 0; AttributeIndex < NoAttributes; AttributeIndex++)
> {
> > +    DEBUG ((DEBUG_ERROR, "Programing UIC attribute = %d, selector
> > + index
> 
> DEBUG_INFO should be enough.
> 
> 
> > = %d, set type = %d, value = %d\n",
> > +                          UicAttrArray[AttributeIndex].MibAttribute,
> > UicAttrArray[AttributeIndex].GenSelectorIndex,
> > +                          UicAttrArray[AttributeIndex].AttrSetType,
> > UicAttrArray[AttributeIndex].AttributeValue));
> > +    Status = UfsExecUicCommands (
> > +               Private,
> > +               UfsUicDmeSet,
> > +               (UicAttrArray[AttributeIndex].MibAttribute << 16) |
> > (UicAttrArray[AttributeIndex].GenSelectorIndex),
> > +               UicAttrArray[AttributeIndex].AttrSetType << 16,
> > +               UicAttrArray[AttributeIndex].AttributeValue
> > +               );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR, "Failed to program UIC attribute = %d,
> > + selector
> > index = %d, set type = %d, value = %d\n",
> > +
> > + UicAttrArray[AttributeIndex].MibAttribute,
> > UicAttrArray[AttributeIndex].GenSelectorIndex,
> > +                            UicAttrArray[AttributeIndex].AttrSetType,
> > UicAttrArray[AttributeIndex].AttributeValue));
> > +      return Status;
> > +    }
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> >  /**
> >    Initialize the UFS host controller.
> >
> > @@ -2090,6 +2145,11 @@ UfsControllerInit (
> >      return Status;
> >    }
> >
> > +  Status = UfsProgramAdditionalUicAttributes (Private);  if
> > + (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "UfsControllerInit: Failed to perform
> > + additional
> > UIC programming\n"));
> > +  }
> > +
> >    Status = UfsDeviceDetection (Private);
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((DEBUG_ERROR, "UfsControllerInit: Device Detection Fails,
> > Status = %r\n", Status)); diff --git
> > a/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > new file mode 100644
> > index 0000000000..c730127482
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > @@ -0,0 +1,75 @@
> > +/** @file
> > +
> > +  Universal Flash Storage Host Controller info Protocol.
> > +
> > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> This
> > +program and the accompanying materials are licensed and made
> > available under
> > +the terms and conditions of the BSD License that accompanies this
> distribution.
> > +The full text of the license may be found at
> > +http://opensource.org/licenses/bsd-license.php.
> > +
> > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS
> > OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef __UFS_HC_INFO_PROTOCOL_H__
> > +#define __UFS_HC_INFO_PROTOCOL_H__
> > +
> > +#define UFS_HC_INFO_PROTOCOL_VERSION  1
> > +
> > +extern EFI_GUID gUfsHcInfoProtocolGuid;
> 
> I suggest to rename gUfsHcInfoProtocolGuid to:
> gEdkiiUfsHcInfoProtocolGuid
> 
> which I think is a common naming convention for non UEFI spec protocols
> within MdeModulePkg.
> 
> 
> > +
> > +typedef struct _UFS_HC_INFO_PROTOCOL UFS_HC_INFO_PROTOCOL;
> > +
> > +typedef struct {
> > +  UINT16  MibAttribute;
> > +  UINT16  GenSelectorIndex;
> > +  UINT8   AttrSetType;
> > +  UINT32  AttributeValue;
> > +} UIC_ATTRIBUTE;
> > +
> > +/**
> > +  Checks if this instance of the info protocol supports
> > +  given host controller.
> > +
> > +  @param[in] This              Pointer to this instance of
> > UFS_HC_INFO_PROTOCOL
> > +  @param[in] ControllerHandle  Controller to check
> > +
> > +  @retval EFI_SUCCESS  This instance of info protocol supports given
> controller
> > +  @retval others       This instance of info protocol doesn't support given
> > controller
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(*UFS_INFO_CONTROLLER_SUPPORTED) (
> > +  IN UFS_HC_INFO_PROTOCOL  *This,
> > +  IN EFI_HANDLE            ControllerHandle
> > +  );
> > +
> > +/**
> > +  Get the UIC programming table to be used just after host controller
> > +  enabling and before device detection.
> > +
> > +  @param[in] This               Pointer to this instance of
> > UFS_HC_INFO_PROTOCOL
> > +  @param[in] UicAttributeArray  Out variable for address of the table
> > +  @param[in] NoAttributes       Out variable for number of attributes in the
> > array
> 
> '@param[out]' for UicAttributeArray & NoAttributes.
> 
> Also, for 'UicAttributeArray', I think the content pointed by this pointer is
> allocated by the protocol producer and should be freed by the consumer
> after being used, right?
> 
> If so, please help to refine the description comments for 'UicAttributeArray'.
> 
> 
> > +
> > +  @retval EFI_SUCCESS  Table successfully found and returned
> > +  @retval others       Table wasn't located successfully UIC programming
> > shouldn't be performed.
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
> > +  IN  UFS_HC_INFO_PROTOCOL  *This,
> > +  OUT UIC_ATTRIBUTE         **UicAttributeArray,
> > +  OUT UINTN                 *NoAttributes
> > +  );
> 
> Judging from the usage of this service in function
> UfsProgramAdditionalUicAttributes(), I think there is an assumption that
> platforms will only require additional 'DME_SET' UIC commands.
> 
> I am not sure if we need to make this service a bit more general. So how
> about changing the 'UIC_ATTRIBUTE' structure to:
> 
> typedef struct {
>   UINT8   UicOpcode;
>   UINT32  Arg1;
>   UINT32  Arg2;
>   UINT32  Arg3;
> } UIC_COMMAND;
> 
> and update the service to:
> 
> typedef
> EFI_STATUS
> (*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
>   IN  UFS_HC_INFO_PROTOCOL  *This,
>   OUT UIC_COMMAND           **UicCommandArray,
>   OUT UINTN                 *NoCommand
>   );
> 
> 
> > +
> > +struct _UFS_HC_INFO_PROTOCOL {
> > +  UINT32                              Version;
> > +  UFS_INFO_CONTROLLER_SUPPORTED       Supported;
> 
> If there will be only one protocol instance within system, do you think we can
> drop the above 'Supported' service?
> 
> Best Regards,
> Hao Wu
> 
> 
> > +  UFS_INFO_GET_UIC_PROGRAMMING_TABLE
> GetUicProgrammingTable; };
> > +
> > +#endif
> > +
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index a2130bc439..c6be8f12a4
> 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -581,6 +581,9 @@
> >    ## Include/Protocol/UfsHostController.h
> >    gEdkiiUfsHostControllerProtocolGuid = { 0xebc01af5, 0x7a9, 0x489e,
> > { 0xb7, 0xce, 0xdc, 0x8, 0x9e, 0x45, 0x9b, 0x2f } }
> >
> > +  ## Include/Protocol/UfsHostControllerInfo.h
> > +  gUfsHcInfoProtocolGuid = { 0x3d18ba13, 0xd9b1, 0x4dd4, {0xb9, 0x16,
> > + 0xd3,
> > 0x07, 0x96, 0x53, 0x9e, 0xd8}}
> > +
> >    ## Include/Protocol/EsrtManagement.h
> >    gEsrtManagementProtocolGuid         = { 0xa340c064, 0x723c, 0x4a9c, { 0xa4,
> > 0xdd, 0xd5, 0xb4, 0x7a, 0x26, 0xfb, 0xb0 }}
> >
> > --
> > 2.14.1.windows.1

Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol

[Albecki, Mateusz]

I wasn't really thinking about efi code that can come in from device. I was thinking more of a discrete device that you can solder down on the board and manufacturer of such device can distribute a efi binary with it that will contain only this small piece of code instead of the full blown driver. Single instance is also OK it is just that I think it is slightly less comfortable for producers of the protocol.

I can't think of anything outside of DME_SET that would make sense but you are right that specification doesn't forbid other opcodes. Maybe we need to be a little more flexible here and introduce different protocol to handle that? How about something like this

UFS_HC_INFO_PROTOCOL {
  Supported // If we do decide to go with multi instance
  GetNextCommand
  CommandFinishedCallback (This, CommandResults)
}

I think this would be able to support DME_GET commands. Let me know what you think.

Thanks,
Mateusz

-----Original Message-----
From: Wu, Hao A 
Sent: Friday, April 12, 2019 8:35 AM
To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>
Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol

Resend this one to the new mailing list.

Hello Mateusz,

Incline comments below.

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Thursday, April 11, 2019 9:39 AM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Ni, Ray
> Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info 
> protocol
> 
> Hi,
> 
> I will be addressing most of the comments in next patch however I want 
> to discuss 2 things.
> 
> 1. I really thing that supporting multiple instances of the protocol 
> is a better way here. I can imagine a board which has 2 UFS 
> controllers one integrated into the SoC and the second one connected 
> through PCI(maybe for debug or maybe that's the production 
> configuration due to problems with the integrated controller). I would 
> like the company that produced the discrete UFS controller to be able 
> to deliver a efi binary which will install this protocol just for 
> their device(identified by device id) while not conflicting with the 
> code that installs configuration for the integrated controller. With 
> single instance we would need BIOS team working on the product to 
> modify their code instead of just dropping in the binary. This use 
> case is a little exotic but I think it's valid especially if we ever 
> consider extending this protocol. Choosing the first instance that returns supported should be clear once I add description in the protocol header.

For controllers attached through the PCI, my understanding is that this binary will reside in the option ROM. If the device produce company want to customize the initialization process, I think they can directly put a different controller/device driver binary in the ROM instead. The Bus Specific Driver Override Protocol ensures the driver in the option ROM will manage the device. So I think for BIOS, it is reasonable to focus on the integrated one.

> 
> 2. About making the service more general and allowing the producer of 
> the protocol to choose UIC command to execute. I only tried to satisfy 
> the UFS specification and I am not sure what possible use cases 
> platform could have that would require sending commands other than 
> DME_SET so maybe we should refrain from extending it now? Extension 
> would also raise questions how to handle DME_GET command for instance 
> since right now we do not have any callback to platform producer.

I am not very sure about this one, what I found in the UFSHCI spec seems that the spec does not explicitly forbid other UIC to be sent.

According to UFSHCI spec Version 2.1, Section 7.1.1:

> Additional commands, such as DME_SET commands may be sent from the 
> system host to the UFS host controller to provide configuration 
> flexibility.

If you think commands other than 'DME_SET' will not make much sense here, I am okay with the current 'UIC_ATTRIBUTE' definition.

(I slightly prefer using 'Arg1'~'Arg3' in structure 'UIC_ATTRIBUTE', it seems a little bit cleaner to me. But this is only my thought and you may choose your own implementation for this.)

Best Regards,
Hao Wu

> 
> Thanks,
> Mateusz
> 
> -----Original Message-----
> From: Wu, Hao A
> Sent: Monday, April 1, 2019 8:53 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; 
> edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info 
> protocol
> 
> Hello Mateusz,
> 
> A couple of general comments:
> 
> A. I suggest to break this commit into 2 patches:
> 
> The first one will just introduce the new protocol.
> The second one will update the UfsPassThruDxe driver to consume this 
> new protocol.
> 
> 
> B. There has been a Bugzilla tracker for the feature you add in this
> patch:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1343
> 
> Could you help to add this information in the commit log message? Thanks.
> 
> 
> C. I prefer the new protocol to have platform level singleton instance:
> 
> I do not see much value for a platform to produce multiple instances 
> of this protocol, I think the additional UIC commands needed during 
> the UFS HC initialization for a specific platform is deterministic.
> 
> Also, the selection of protocol instance implemented in function
> GetUfsHcInfoProtocol() is by:
>   1) LocateHandleBuffer() to get all protocol instances;
>   2) Choose the 1st instance if the 'Supported' service returns without
>      error.
> 
> I think this will bring some kind of confusion to the protocol 
> producers, since they do not know which one will be selected when 
> there are multiple instances of the protocol.
> 
> 
> > -----Original Message-----
> > From: Albecki, Mateusz
> > Sent: Thursday, March 28, 2019 9:56 PM
> > To: edk2-devel@lists.01.org
> > Cc: Albecki, Mateusz; Wu, Hao A
> > Subject: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info 
> > protocol
> >
> > UFS host controller specification allows for implementation specific 
> > UIC programming to take place just after host controller enable and 
> > before device detection. Since there is no way for generic driver to 
> > anticipate implementation specific programming we add a UFS info 
> > protocol which allows the implementation specific code to pass this 
> > information to generic driver. UFS info protocol is located by the 
> > driver at the BindingStart call and the supported instance is 
> > retained throught entire
> 
> throught -> through
> 
> 
> > driver lifetime. Presence of the protocol is optional.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
> > ---
> >  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  | 62
> > ++++++++++++++++++
> >  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |  2 +
> >  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf      |  1 +
> >  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 60
> +++++++++++++++++
> >  .../Include/Protocol/UfsHostControllerInfo.h       | 75
> > ++++++++++++++++++++++
> >  MdeModulePkg/MdeModulePkg.dec                      |  3 +
> >  6 files changed, 203 insertions(+)
> >  create mode 100644
> > MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> >
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > index ea329618dc..a41079e311 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > @@ -40,6 +40,7 @@ UFS_PASS_THRU_PRIVATE_DATA
> gUfsPassThruTemplate = {
> >      UfsRwUfsAttribute
> >    },
> >    0,                              // UfsHostController
> > +  NULL,                           // UfsInfo
> >    0,                              // UfsHcBase
> >    0,                              // Capabilities
> >    0,                              // TaskTag
> > @@ -776,6 +777,66 @@ UfsFinishDeviceInitialization (
> >    return EFI_SUCCESS;
> >  }
> >
> > +/**
> > +  Locates UFS HC infor protocol suitable for this controller.
> 
> infor -> info
> 
> 
> > +
> > +  @param[in] DriverBinding     Pointer to driver binding protocol
> > +  @param[in] Private           Pointer to host controller private data
> > +  @param[in] ControllerHandle  Controller to which driver is bound 
> > +**/ VOID GetUfsHcInfoProtocol (
> > +  IN EFI_DRIVER_BINDING_PROTOCOL  *DriverBinding,
> > +  IN UFS_PASS_THRU_PRIVATE_DATA   *Private,
> > +  IN EFI_HANDLE                   ControllerHandle
> > +  )
> > +{
> > +  EFI_STATUS            Status;
> > +  EFI_HANDLE            *ProtocolHandleArray;
> > +  UINTN                 NoHandles;
> > +  UINTN                 HandleIndex;
> > +  UFS_HC_INFO_PROTOCOL  *UfsHcInfo;
> > +
> > +  Status = gBS->LocateHandleBuffer (
> > +                  ByProtocol,
> > +                  &gUfsHcInfoProtocolGuid,
> > +                  NULL,
> > +                  &NoHandles,
> > +                  &ProtocolHandleArray
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    return;
> > +  }
> > +
> > +  for (HandleIndex = 0; HandleIndex < NoHandles; HandleIndex++) {
> > +    Status = gBS->OpenProtocol (
> > +                    ProtocolHandleArray[HandleIndex],
> > +                    &gUfsHcInfoProtocolGuid,
> > +                    &UfsHcInfo,
> > +                    DriverBinding->DriverBindingHandle,
> > +                    ControllerHandle,
> > +                    EFI_OPEN_PROTOCOL_BY_DRIVER
> > +                    );
> > +    if (EFI_ERROR (Status)) {
> > +      continue;
> > +    }
> > +    if (!EFI_ERROR(UfsHcInfo->Supported (UfsHcInfo, ControllerHandle))) {
> > +      Private->UfsHcInfo = UfsHcInfo;
> > +      break;
> > +    } else {
> > +      Status = gBS->CloseProtocol (
> > +                      ProtocolHandleArray[HandleIndex],
> > +                      &gUfsHcInfoProtocolGuid,
> > +                      DriverBinding->DriverBindingHandle,
> > +                      ControllerHandle
> > +                      );
> > +      ASSERT_EFI_ERROR (Status);
> > +    }
> > +  }
> > +
> > +  FreePool (ProtocolHandleArray);
> > +}
> > +
> >  /**
> >    Starts a device controller or a bus controller.
> >
> > @@ -871,6 +932,7 @@ UfsPassThruDriverBindingStart (
> >    Private->UfsHostController    = UfsHc;
> >    Private->UfsHcBase            = UfsHcBase;
> >    InitializeListHead (&Private->Queue);
> > +  GetUfsHcInfoProtocol (This, Private, Controller);
> >
> >    //
> >    // Initialize UFS Host Controller H/W.
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > index e591e78661..55a8ed9bdf 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > @@ -29,6 +29,7 @@
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Library/DevicePathLib.h>
> >  #include <Library/TimerLib.h>
> > +#include <Protocol/UfsHostControllerInfo.h>
> 
> A minor comment here:
> You can put the above '#include' together with other protocol header 
> files rather than appending it after the libraries includes.
> 
> 
> >
> >  #include "UfsPassThruHci.h"
> >
> > @@ -66,6 +67,7 @@ typedef struct _UFS_PASS_THRU_PRIVATE_DATA {
> >    EFI_EXT_SCSI_PASS_THRU_PROTOCOL     ExtScsiPassThru;
> >    EFI_UFS_DEVICE_CONFIG_PROTOCOL      UfsDevConfig;
> >    EDKII_UFS_HOST_CONTROLLER_PROTOCOL  *UfsHostController;
> > +  UFS_HC_INFO_PROTOCOL                *UfsHcInfo;
> >    UINTN                               UfsHcBase;
> >    UINT32                              Capabilities;
> >
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > index e550cd02b4..12b01771ff 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > @@ -59,6 +59,7 @@
> >    gEfiExtScsiPassThruProtocolGuid               ## BY_START
> >    gEfiUfsDeviceConfigProtocolGuid               ## BY_START
> >    gEdkiiUfsHostControllerProtocolGuid           ## TO_START
> > +  gUfsHcInfoProtocolGuid
> 
> gUfsHcInfoProtocolGuid ->
> gUfsHcInfoProtocolGuid    ## SOMETIMES_CONSUMES
> 
> 
> >
> >  [UserExtensions.TianoCore."ExtraFiles"]
> >    UfsPassThruExtra.uni
> > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > index c37161c4ae..125f2f2516 100644
> > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > @@ -2068,6 +2068,61 @@ UfsInitTransferRequestList (
> >    return EFI_SUCCESS;
> >  }
> >
> > +/**
> > +  Performs additional UIC programming if it has been specified by 
> > +platform in
> > UFS info protocol.
> > +
> > +  @param[in] Private  Pointer to host controller private data
> > +
> > +  @retval EFI_SUCCESS  Programming finished successfully or not
> requested
> > +  @retval others       Programming failed
> > +**/
> > +EFI_STATUS
> > +UfsProgramAdditionalUicAttributes (
> > +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private
> > +  )
> > +{
> > +  EFI_STATUS     Status;
> > +  UIC_ATTRIBUTE  *UicAttrArray;
> > +  UINTN          NoAttributes;
> > +  UINTN          AttributeIndex;
> > +
> > +  //
> > +  // No info protocol means that no additional programming should 
> > + be
> > performed
> > +  //
> > +  if (Private->UfsHcInfo == NULL) {
> > +    return EFI_SUCCESS;
> > +  }
> > +
> > +  //
> > +  // If we failed to get the programming table we assume that 
> > + platform
> > doesn't want to do any additional programming
> > +  //
> > +  Status = Private->UfsHcInfo->GetUicProgrammingTable
> > + (Private->UfsHcInfo,
> > &UicAttrArray, &NoAttributes);
> > +  if (EFI_ERROR (Status)) {
> > +    return EFI_SUCCESS;
> > +  }
> 
> Please help to free the content pointed by 'UicAttrArray'.
> 
> 
> > +
> > +  for (AttributeIndex = 0; AttributeIndex < NoAttributes; 
> > + AttributeIndex++)
> {
> > +    DEBUG ((DEBUG_ERROR, "Programing UIC attribute = %d, selector 
> > + index
> 
> DEBUG_INFO should be enough.
> 
> 
> > = %d, set type = %d, value = %d\n",
> > +                          
> > + UicAttrArray[AttributeIndex].MibAttribute,
> > UicAttrArray[AttributeIndex].GenSelectorIndex,
> > +                          UicAttrArray[AttributeIndex].AttrSetType,
> > UicAttrArray[AttributeIndex].AttributeValue));
> > +    Status = UfsExecUicCommands (
> > +               Private,
> > +               UfsUicDmeSet,
> > +               (UicAttrArray[AttributeIndex].MibAttribute << 16) |
> > (UicAttrArray[AttributeIndex].GenSelectorIndex),
> > +               UicAttrArray[AttributeIndex].AttrSetType << 16,
> > +               UicAttrArray[AttributeIndex].AttributeValue
> > +               );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR, "Failed to program UIC attribute = %d, 
> > + selector
> > index = %d, set type = %d, value = %d\n",
> > +
> > + UicAttrArray[AttributeIndex].MibAttribute,
> > UicAttrArray[AttributeIndex].GenSelectorIndex,
> > +                            
> > + UicAttrArray[AttributeIndex].AttrSetType,
> > UicAttrArray[AttributeIndex].AttributeValue));
> > +      return Status;
> > +    }
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +}
> > +
> >  /**
> >    Initialize the UFS host controller.
> >
> > @@ -2090,6 +2145,11 @@ UfsControllerInit (
> >      return Status;
> >    }
> >
> > +  Status = UfsProgramAdditionalUicAttributes (Private);  if 
> > + (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "UfsControllerInit: Failed to perform 
> > + additional
> > UIC programming\n"));
> > +  }
> > +
> >    Status = UfsDeviceDetection (Private);
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((DEBUG_ERROR, "UfsControllerInit: Device Detection 
> > Fails, Status = %r\n", Status)); diff --git 
> > a/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > new file mode 100644
> > index 0000000000..c730127482
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > @@ -0,0 +1,75 @@
> > +/** @file
> > +
> > +  Universal Flash Storage Host Controller info Protocol.
> > +
> > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> 
> > +This program and the accompanying materials are licensed and made
> > available under
> > +the terms and conditions of the BSD License that accompanies this
> distribution.
> > +The full text of the license may be found at 
> > +http://opensource.org/licenses/bsd-license.php.
> > +
> > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS
> > OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef __UFS_HC_INFO_PROTOCOL_H__
> > +#define __UFS_HC_INFO_PROTOCOL_H__
> > +
> > +#define UFS_HC_INFO_PROTOCOL_VERSION  1
> > +
> > +extern EFI_GUID gUfsHcInfoProtocolGuid;
> 
> I suggest to rename gUfsHcInfoProtocolGuid to:
> gEdkiiUfsHcInfoProtocolGuid
> 
> which I think is a common naming convention for non UEFI spec 
> protocols within MdeModulePkg.
> 
> 
> > +
> > +typedef struct _UFS_HC_INFO_PROTOCOL UFS_HC_INFO_PROTOCOL;
> > +
> > +typedef struct {
> > +  UINT16  MibAttribute;
> > +  UINT16  GenSelectorIndex;
> > +  UINT8   AttrSetType;
> > +  UINT32  AttributeValue;
> > +} UIC_ATTRIBUTE;
> > +
> > +/**
> > +  Checks if this instance of the info protocol supports
> > +  given host controller.
> > +
> > +  @param[in] This              Pointer to this instance of
> > UFS_HC_INFO_PROTOCOL
> > +  @param[in] ControllerHandle  Controller to check
> > +
> > +  @retval EFI_SUCCESS  This instance of info protocol supports 
> > + given
> controller
> > +  @retval others       This instance of info protocol doesn't support given
> > controller
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(*UFS_INFO_CONTROLLER_SUPPORTED) (
> > +  IN UFS_HC_INFO_PROTOCOL  *This,
> > +  IN EFI_HANDLE            ControllerHandle
> > +  );
> > +
> > +/**
> > +  Get the UIC programming table to be used just after host 
> > +controller
> > +  enabling and before device detection.
> > +
> > +  @param[in] This               Pointer to this instance of
> > UFS_HC_INFO_PROTOCOL
> > +  @param[in] UicAttributeArray  Out variable for address of the table
> > +  @param[in] NoAttributes       Out variable for number of attributes in the
> > array
> 
> '@param[out]' for UicAttributeArray & NoAttributes.
> 
> Also, for 'UicAttributeArray', I think the content pointed by this 
> pointer is allocated by the protocol producer and should be freed by 
> the consumer after being used, right?
> 
> If so, please help to refine the description comments for 'UicAttributeArray'.
> 
> 
> > +
> > +  @retval EFI_SUCCESS  Table successfully found and returned
> > +  @retval others       Table wasn't located successfully UIC programming
> > shouldn't be performed.
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
> > +  IN  UFS_HC_INFO_PROTOCOL  *This,
> > +  OUT UIC_ATTRIBUTE         **UicAttributeArray,
> > +  OUT UINTN                 *NoAttributes
> > +  );
> 
> Judging from the usage of this service in function 
> UfsProgramAdditionalUicAttributes(), I think there is an assumption 
> that platforms will only require additional 'DME_SET' UIC commands.
> 
> I am not sure if we need to make this service a bit more general. So 
> how about changing the 'UIC_ATTRIBUTE' structure to:
> 
> typedef struct {
>   UINT8   UicOpcode;
>   UINT32  Arg1;
>   UINT32  Arg2;
>   UINT32  Arg3;
> } UIC_COMMAND;
> 
> and update the service to:
> 
> typedef
> EFI_STATUS
> (*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
>   IN  UFS_HC_INFO_PROTOCOL  *This,
>   OUT UIC_COMMAND           **UicCommandArray,
>   OUT UINTN                 *NoCommand
>   );
> 
> 
> > +
> > +struct _UFS_HC_INFO_PROTOCOL {
> > +  UINT32                              Version;
> > +  UFS_INFO_CONTROLLER_SUPPORTED       Supported;
> 
> If there will be only one protocol instance within system, do you 
> think we can drop the above 'Supported' service?
> 
> Best Regards,
> Hao Wu
> 
> 
> > +  UFS_INFO_GET_UIC_PROGRAMMING_TABLE
> GetUicProgrammingTable; };
> > +
> > +#endif
> > +
> > diff --git a/MdeModulePkg/MdeModulePkg.dec 
> > b/MdeModulePkg/MdeModulePkg.dec index a2130bc439..c6be8f12a4
> 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -581,6 +581,9 @@
> >    ## Include/Protocol/UfsHostController.h
> >    gEdkiiUfsHostControllerProtocolGuid = { 0xebc01af5, 0x7a9, 
> > 0x489e, { 0xb7, 0xce, 0xdc, 0x8, 0x9e, 0x45, 0x9b, 0x2f } }
> >
> > +  ## Include/Protocol/UfsHostControllerInfo.h
> > +  gUfsHcInfoProtocolGuid = { 0x3d18ba13, 0xd9b1, 0x4dd4, {0xb9, 
> > + 0x16, 0xd3,
> > 0x07, 0x96, 0x53, 0x9e, 0xd8}}
> > +
> >    ## Include/Protocol/EsrtManagement.h
> >    gEsrtManagementProtocolGuid         = { 0xa340c064, 0x723c, 0x4a9c, { 0xa4,
> > 0xdd, 0xd5, 0xb4, 0x7a, 0x26, 0xfb, 0xb0 }}
> >
> > --
> > 2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol

[Wu, Hao A]

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Saturday, April 13, 2019 7:23 AM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Ni, Ray
> Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info
> protocol
> 
> I wasn't really thinking about efi code that can come in from device. I was
> thinking more of a discrete device that you can solder down on the board and
> manufacturer of such device can distribute a efi binary with it that will contain
> only this small piece of code instead of the full blown driver. Single instance is

Hello Mateusz,

Judging from the existing cases like ATA and SD/MMC,
(MdeModulePkg/Bus/Ata/AtaAtapiPassThru, MdeModulePkg/Bus/Pci/SdMmcPciHcDxe)
it seems that we have not met such request yet.

Since this one will not bring big impact to the driver implementation, how
about make this protocol a platform singleton at this moment (using a
global variable to store the protocol instance)? We may update the codes
if there comes an actual request.

How does this sound to you?

> also OK it is just that I think it is slightly less comfortable for producers of the
> protocol.
> 
> I can't think of anything outside of DME_SET that would make sense but you are
> right that specification doesn't forbid other opcodes. Maybe we need to be a
> little more flexible here and introduce different protocol to handle that? How
> about something like this
> 
> UFS_HC_INFO_PROTOCOL {
>   Supported // If we do decide to go with multi instance
>   GetNextCommand
>   CommandFinishedCallback (This, CommandResults)
> }
> 
> I think this would be able to support DME_GET commands. Let me know what
> you think.

For this one I was thinking commands like DME_PEER_SET (or other commands
maybe added in future) which does not require subsequent process by the
system host.

Handling commands like DME_GET seems a bit overshot to me.

Best Regards,
Hao Wu

> 
> Thanks,
> Mateusz
> 
> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, April 12, 2019 8:35 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info
> protocol
> 
> Resend this one to the new mailing list.
> 
> Hello Mateusz,
> 
> Incline comments below.
> 
> > -----Original Message-----
> > From: Albecki, Mateusz
> > Sent: Thursday, April 11, 2019 9:39 AM
> > To: Wu, Hao A; edk2-devel@lists.01.org
> > Cc: Ni, Ray
> > Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info
> > protocol
> >
> > Hi,
> >
> > I will be addressing most of the comments in next patch however I want
> > to discuss 2 things.
> >
> > 1. I really thing that supporting multiple instances of the protocol
> > is a better way here. I can imagine a board which has 2 UFS
> > controllers one integrated into the SoC and the second one connected
> > through PCI(maybe for debug or maybe that's the production
> > configuration due to problems with the integrated controller). I would
> > like the company that produced the discrete UFS controller to be able
> > to deliver a efi binary which will install this protocol just for
> > their device(identified by device id) while not conflicting with the
> > code that installs configuration for the integrated controller. With
> > single instance we would need BIOS team working on the product to
> > modify their code instead of just dropping in the binary. This use
> > case is a little exotic but I think it's valid especially if we ever
> > consider extending this protocol. Choosing the first instance that returns
> supported should be clear once I add description in the protocol header.
> 
> For controllers attached through the PCI, my understanding is that this binary
> will reside in the option ROM. If the device produce company want to
> customize the initialization process, I think they can directly put a different
> controller/device driver binary in the ROM instead. The Bus Specific Driver
> Override Protocol ensures the driver in the option ROM will manage the device.
> So I think for BIOS, it is reasonable to focus on the integrated one.
> 
> >
> > 2. About making the service more general and allowing the producer of
> > the protocol to choose UIC command to execute. I only tried to satisfy
> > the UFS specification and I am not sure what possible use cases
> > platform could have that would require sending commands other than
> > DME_SET so maybe we should refrain from extending it now? Extension
> > would also raise questions how to handle DME_GET command for instance
> > since right now we do not have any callback to platform producer.
> 
> I am not very sure about this one, what I found in the UFSHCI spec seems that
> the spec does not explicitly forbid other UIC to be sent.
> 
> According to UFSHCI spec Version 2.1, Section 7.1.1:
> 
> > Additional commands, such as DME_SET commands may be sent from the
> > system host to the UFS host controller to provide configuration
> > flexibility.
> 
> If you think commands other than 'DME_SET' will not make much sense here, I
> am okay with the current 'UIC_ATTRIBUTE' definition.
> 
> (I slightly prefer using 'Arg1'~'Arg3' in structure 'UIC_ATTRIBUTE', it seems a
> little bit cleaner to me. But this is only my thought and you may choose your
> own implementation for this.)
> 
> Best Regards,
> Hao Wu
> 
> >
> > Thanks,
> > Mateusz
> >
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Monday, April 1, 2019 8:53 AM
> > To: Albecki, Mateusz <mateusz.albecki@intel.com>;
> > edk2-devel@lists.01.org
> > Cc: Ni, Ray <ray.ni@intel.com>
> > Subject: RE: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info
> > protocol
> >
> > Hello Mateusz,
> >
> > A couple of general comments:
> >
> > A. I suggest to break this commit into 2 patches:
> >
> > The first one will just introduce the new protocol.
> > The second one will update the UfsPassThruDxe driver to consume this
> > new protocol.
> >
> >
> > B. There has been a Bugzilla tracker for the feature you add in this
> > patch:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1343
> >
> > Could you help to add this information in the commit log message? Thanks.
> >
> >
> > C. I prefer the new protocol to have platform level singleton instance:
> >
> > I do not see much value for a platform to produce multiple instances
> > of this protocol, I think the additional UIC commands needed during
> > the UFS HC initialization for a specific platform is deterministic.
> >
> > Also, the selection of protocol instance implemented in function
> > GetUfsHcInfoProtocol() is by:
> >   1) LocateHandleBuffer() to get all protocol instances;
> >   2) Choose the 1st instance if the 'Supported' service returns without
> >      error.
> >
> > I think this will bring some kind of confusion to the protocol
> > producers, since they do not know which one will be selected when
> > there are multiple instances of the protocol.
> >
> >
> > > -----Original Message-----
> > > From: Albecki, Mateusz
> > > Sent: Thursday, March 28, 2019 9:56 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Albecki, Mateusz; Wu, Hao A
> > > Subject: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info
> > > protocol
> > >
> > > UFS host controller specification allows for implementation specific
> > > UIC programming to take place just after host controller enable and
> > > before device detection. Since there is no way for generic driver to
> > > anticipate implementation specific programming we add a UFS info
> > > protocol which allows the implementation specific code to pass this
> > > information to generic driver. UFS info protocol is located by the
> > > driver at the BindingStart call and the supported instance is
> > > retained throught entire
> >
> > throught -> through
> >
> >
> > > driver lifetime. Presence of the protocol is optional.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Cc: Hao Wu <hao.a.wu@intel.com>
> > > Signed-off-by: Albecki Mateusz <mateusz.albecki@intel.com>
> > > ---
> > >  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  | 62
> > > ++++++++++++++++++
> > >  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |  2 +
> > >  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf      |  1 +
> > >  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 60
> > +++++++++++++++++
> > >  .../Include/Protocol/UfsHostControllerInfo.h       | 75
> > > ++++++++++++++++++++++
> > >  MdeModulePkg/MdeModulePkg.dec                      |  3 +
> > >  6 files changed, 203 insertions(+)
> > >  create mode 100644
> > > MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > >
> > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > > index ea329618dc..a41079e311 100644
> > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> > > @@ -40,6 +40,7 @@ UFS_PASS_THRU_PRIVATE_DATA
> > gUfsPassThruTemplate = {
> > >      UfsRwUfsAttribute
> > >    },
> > >    0,                              // UfsHostController
> > > +  NULL,                           // UfsInfo
> > >    0,                              // UfsHcBase
> > >    0,                              // Capabilities
> > >    0,                              // TaskTag
> > > @@ -776,6 +777,66 @@ UfsFinishDeviceInitialization (
> > >    return EFI_SUCCESS;
> > >  }
> > >
> > > +/**
> > > +  Locates UFS HC infor protocol suitable for this controller.
> >
> > infor -> info
> >
> >
> > > +
> > > +  @param[in] DriverBinding     Pointer to driver binding protocol
> > > +  @param[in] Private           Pointer to host controller private data
> > > +  @param[in] ControllerHandle  Controller to which driver is bound
> > > +**/ VOID GetUfsHcInfoProtocol (
> > > +  IN EFI_DRIVER_BINDING_PROTOCOL  *DriverBinding,
> > > +  IN UFS_PASS_THRU_PRIVATE_DATA   *Private,
> > > +  IN EFI_HANDLE                   ControllerHandle
> > > +  )
> > > +{
> > > +  EFI_STATUS            Status;
> > > +  EFI_HANDLE            *ProtocolHandleArray;
> > > +  UINTN                 NoHandles;
> > > +  UINTN                 HandleIndex;
> > > +  UFS_HC_INFO_PROTOCOL  *UfsHcInfo;
> > > +
> > > +  Status = gBS->LocateHandleBuffer (
> > > +                  ByProtocol,
> > > +                  &gUfsHcInfoProtocolGuid,
> > > +                  NULL,
> > > +                  &NoHandles,
> > > +                  &ProtocolHandleArray
> > > +                  );
> > > +  if (EFI_ERROR (Status)) {
> > > +    return;
> > > +  }
> > > +
> > > +  for (HandleIndex = 0; HandleIndex < NoHandles; HandleIndex++) {
> > > +    Status = gBS->OpenProtocol (
> > > +                    ProtocolHandleArray[HandleIndex],
> > > +                    &gUfsHcInfoProtocolGuid,
> > > +                    &UfsHcInfo,
> > > +                    DriverBinding->DriverBindingHandle,
> > > +                    ControllerHandle,
> > > +                    EFI_OPEN_PROTOCOL_BY_DRIVER
> > > +                    );
> > > +    if (EFI_ERROR (Status)) {
> > > +      continue;
> > > +    }
> > > +    if (!EFI_ERROR(UfsHcInfo->Supported (UfsHcInfo, ControllerHandle))) {
> > > +      Private->UfsHcInfo = UfsHcInfo;
> > > +      break;
> > > +    } else {
> > > +      Status = gBS->CloseProtocol (
> > > +                      ProtocolHandleArray[HandleIndex],
> > > +                      &gUfsHcInfoProtocolGuid,
> > > +                      DriverBinding->DriverBindingHandle,
> > > +                      ControllerHandle
> > > +                      );
> > > +      ASSERT_EFI_ERROR (Status);
> > > +    }
> > > +  }
> > > +
> > > +  FreePool (ProtocolHandleArray);
> > > +}
> > > +
> > >  /**
> > >    Starts a device controller or a bus controller.
> > >
> > > @@ -871,6 +932,7 @@ UfsPassThruDriverBindingStart (
> > >    Private->UfsHostController    = UfsHc;
> > >    Private->UfsHcBase            = UfsHcBase;
> > >    InitializeListHead (&Private->Queue);
> > > +  GetUfsHcInfoProtocol (This, Private, Controller);
> > >
> > >    //
> > >    // Initialize UFS Host Controller H/W.
> > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > index e591e78661..55a8ed9bdf 100644
> > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> > > @@ -29,6 +29,7 @@
> > >  #include <Library/UefiBootServicesTableLib.h>
> > >  #include <Library/DevicePathLib.h>
> > >  #include <Library/TimerLib.h>
> > > +#include <Protocol/UfsHostControllerInfo.h>
> >
> > A minor comment here:
> > You can put the above '#include' together with other protocol header
> > files rather than appending it after the libraries includes.
> >
> >
> > >
> > >  #include "UfsPassThruHci.h"
> > >
> > > @@ -66,6 +67,7 @@ typedef struct _UFS_PASS_THRU_PRIVATE_DATA {
> > >    EFI_EXT_SCSI_PASS_THRU_PROTOCOL     ExtScsiPassThru;
> > >    EFI_UFS_DEVICE_CONFIG_PROTOCOL      UfsDevConfig;
> > >    EDKII_UFS_HOST_CONTROLLER_PROTOCOL  *UfsHostController;
> > > +  UFS_HC_INFO_PROTOCOL                *UfsHcInfo;
> > >    UINTN                               UfsHcBase;
> > >    UINT32                              Capabilities;
> > >
> > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > > index e550cd02b4..12b01771ff 100644
> > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf
> > > @@ -59,6 +59,7 @@
> > >    gEfiExtScsiPassThruProtocolGuid               ## BY_START
> > >    gEfiUfsDeviceConfigProtocolGuid               ## BY_START
> > >    gEdkiiUfsHostControllerProtocolGuid           ## TO_START
> > > +  gUfsHcInfoProtocolGuid
> >
> > gUfsHcInfoProtocolGuid ->
> > gUfsHcInfoProtocolGuid    ## SOMETIMES_CONSUMES
> >
> >
> > >
> > >  [UserExtensions.TianoCore."ExtraFiles"]
> > >    UfsPassThruExtra.uni
> > > diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > index c37161c4ae..125f2f2516 100644
> > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> > > @@ -2068,6 +2068,61 @@ UfsInitTransferRequestList (
> > >    return EFI_SUCCESS;
> > >  }
> > >
> > > +/**
> > > +  Performs additional UIC programming if it has been specified by
> > > +platform in
> > > UFS info protocol.
> > > +
> > > +  @param[in] Private  Pointer to host controller private data
> > > +
> > > +  @retval EFI_SUCCESS  Programming finished successfully or not
> > requested
> > > +  @retval others       Programming failed
> > > +**/
> > > +EFI_STATUS
> > > +UfsProgramAdditionalUicAttributes (
> > > +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private
> > > +  )
> > > +{
> > > +  EFI_STATUS     Status;
> > > +  UIC_ATTRIBUTE  *UicAttrArray;
> > > +  UINTN          NoAttributes;
> > > +  UINTN          AttributeIndex;
> > > +
> > > +  //
> > > +  // No info protocol means that no additional programming should
> > > + be
> > > performed
> > > +  //
> > > +  if (Private->UfsHcInfo == NULL) {
> > > +    return EFI_SUCCESS;
> > > +  }
> > > +
> > > +  //
> > > +  // If we failed to get the programming table we assume that
> > > + platform
> > > doesn't want to do any additional programming
> > > +  //
> > > +  Status = Private->UfsHcInfo->GetUicProgrammingTable
> > > + (Private->UfsHcInfo,
> > > &UicAttrArray, &NoAttributes);
> > > +  if (EFI_ERROR (Status)) {
> > > +    return EFI_SUCCESS;
> > > +  }
> >
> > Please help to free the content pointed by 'UicAttrArray'.
> >
> >
> > > +
> > > +  for (AttributeIndex = 0; AttributeIndex < NoAttributes;
> > > + AttributeIndex++)
> > {
> > > +    DEBUG ((DEBUG_ERROR, "Programing UIC attribute = %d, selector
> > > + index
> >
> > DEBUG_INFO should be enough.
> >
> >
> > > = %d, set type = %d, value = %d\n",
> > > +
> > > + UicAttrArray[AttributeIndex].MibAttribute,
> > > UicAttrArray[AttributeIndex].GenSelectorIndex,
> > > +                          UicAttrArray[AttributeIndex].AttrSetType,
> > > UicAttrArray[AttributeIndex].AttributeValue));
> > > +    Status = UfsExecUicCommands (
> > > +               Private,
> > > +               UfsUicDmeSet,
> > > +               (UicAttrArray[AttributeIndex].MibAttribute << 16) |
> > > (UicAttrArray[AttributeIndex].GenSelectorIndex),
> > > +               UicAttrArray[AttributeIndex].AttrSetType << 16,
> > > +               UicAttrArray[AttributeIndex].AttributeValue
> > > +               );
> > > +    if (EFI_ERROR (Status)) {
> > > +      DEBUG ((DEBUG_ERROR, "Failed to program UIC attribute = %d,
> > > + selector
> > > index = %d, set type = %d, value = %d\n",
> > > +
> > > + UicAttrArray[AttributeIndex].MibAttribute,
> > > UicAttrArray[AttributeIndex].GenSelectorIndex,
> > > +
> > > + UicAttrArray[AttributeIndex].AttrSetType,
> > > UicAttrArray[AttributeIndex].AttributeValue));
> > > +      return Status;
> > > +    }
> > > +  }
> > > +
> > > +  return EFI_SUCCESS;
> > > +}
> > > +
> > >  /**
> > >    Initialize the UFS host controller.
> > >
> > > @@ -2090,6 +2145,11 @@ UfsControllerInit (
> > >      return Status;
> > >    }
> > >
> > > +  Status = UfsProgramAdditionalUicAttributes (Private);  if
> > > + (EFI_ERROR (Status)) {
> > > +    DEBUG ((DEBUG_ERROR, "UfsControllerInit: Failed to perform
> > > + additional
> > > UIC programming\n"));
> > > +  }
> > > +
> > >    Status = UfsDeviceDetection (Private);
> > >    if (EFI_ERROR (Status)) {
> > >      DEBUG ((DEBUG_ERROR, "UfsControllerInit: Device Detection
> > > Fails, Status = %r\n", Status)); diff --git
> > > a/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > > b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > > new file mode 100644
> > > index 0000000000..c730127482
> > > --- /dev/null
> > > +++ b/MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> > > @@ -0,0 +1,75 @@
> > > +/** @file
> > > +
> > > +  Universal Flash Storage Host Controller info Protocol.
> > > +
> > > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > > +This program and the accompanying materials are licensed and made
> > > available under
> > > +the terms and conditions of the BSD License that accompanies this
> > distribution.
> > > +The full text of the license may be found at
> > > +http://opensource.org/licenses/bsd-license.php.
> > > +
> > > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > BASIS,
> > > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> > EXPRESS
> > > OR IMPLIED.
> > > +
> > > +**/
> > > +
> > > +#ifndef __UFS_HC_INFO_PROTOCOL_H__
> > > +#define __UFS_HC_INFO_PROTOCOL_H__
> > > +
> > > +#define UFS_HC_INFO_PROTOCOL_VERSION  1
> > > +
> > > +extern EFI_GUID gUfsHcInfoProtocolGuid;
> >
> > I suggest to rename gUfsHcInfoProtocolGuid to:
> > gEdkiiUfsHcInfoProtocolGuid
> >
> > which I think is a common naming convention for non UEFI spec
> > protocols within MdeModulePkg.
> >
> >
> > > +
> > > +typedef struct _UFS_HC_INFO_PROTOCOL UFS_HC_INFO_PROTOCOL;
> > > +
> > > +typedef struct {
> > > +  UINT16  MibAttribute;
> > > +  UINT16  GenSelectorIndex;
> > > +  UINT8   AttrSetType;
> > > +  UINT32  AttributeValue;
> > > +} UIC_ATTRIBUTE;
> > > +
> > > +/**
> > > +  Checks if this instance of the info protocol supports
> > > +  given host controller.
> > > +
> > > +  @param[in] This              Pointer to this instance of
> > > UFS_HC_INFO_PROTOCOL
> > > +  @param[in] ControllerHandle  Controller to check
> > > +
> > > +  @retval EFI_SUCCESS  This instance of info protocol supports
> > > + given
> > controller
> > > +  @retval others       This instance of info protocol doesn't support given
> > > controller
> > > +**/
> > > +typedef
> > > +EFI_STATUS
> > > +(*UFS_INFO_CONTROLLER_SUPPORTED) (
> > > +  IN UFS_HC_INFO_PROTOCOL  *This,
> > > +  IN EFI_HANDLE            ControllerHandle
> > > +  );
> > > +
> > > +/**
> > > +  Get the UIC programming table to be used just after host
> > > +controller
> > > +  enabling and before device detection.
> > > +
> > > +  @param[in] This               Pointer to this instance of
> > > UFS_HC_INFO_PROTOCOL
> > > +  @param[in] UicAttributeArray  Out variable for address of the table
> > > +  @param[in] NoAttributes       Out variable for number of attributes in the
> > > array
> >
> > '@param[out]' for UicAttributeArray & NoAttributes.
> >
> > Also, for 'UicAttributeArray', I think the content pointed by this
> > pointer is allocated by the protocol producer and should be freed by
> > the consumer after being used, right?
> >
> > If so, please help to refine the description comments for 'UicAttributeArray'.
> >
> >
> > > +
> > > +  @retval EFI_SUCCESS  Table successfully found and returned
> > > +  @retval others       Table wasn't located successfully UIC programming
> > > shouldn't be performed.
> > > +**/
> > > +typedef
> > > +EFI_STATUS
> > > +(*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
> > > +  IN  UFS_HC_INFO_PROTOCOL  *This,
> > > +  OUT UIC_ATTRIBUTE         **UicAttributeArray,
> > > +  OUT UINTN                 *NoAttributes
> > > +  );
> >
> > Judging from the usage of this service in function
> > UfsProgramAdditionalUicAttributes(), I think there is an assumption
> > that platforms will only require additional 'DME_SET' UIC commands.
> >
> > I am not sure if we need to make this service a bit more general. So
> > how about changing the 'UIC_ATTRIBUTE' structure to:
> >
> > typedef struct {
> >   UINT8   UicOpcode;
> >   UINT32  Arg1;
> >   UINT32  Arg2;
> >   UINT32  Arg3;
> > } UIC_COMMAND;
> >
> > and update the service to:
> >
> > typedef
> > EFI_STATUS
> > (*UFS_INFO_GET_UIC_PROGRAMMING_TABLE) (
> >   IN  UFS_HC_INFO_PROTOCOL  *This,
> >   OUT UIC_COMMAND           **UicCommandArray,
> >   OUT UINTN                 *NoCommand
> >   );
> >
> >
> > > +
> > > +struct _UFS_HC_INFO_PROTOCOL {
> > > +  UINT32                              Version;
> > > +  UFS_INFO_CONTROLLER_SUPPORTED       Supported;
> >
> > If there will be only one protocol instance within system, do you
> > think we can drop the above 'Supported' service?
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > > +  UFS_INFO_GET_UIC_PROGRAMMING_TABLE
> > GetUicProgrammingTable; };
> > > +
> > > +#endif
> > > +
> > > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > > b/MdeModulePkg/MdeModulePkg.dec index a2130bc439..c6be8f12a4
> > 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > @@ -581,6 +581,9 @@
> > >    ## Include/Protocol/UfsHostController.h
> > >    gEdkiiUfsHostControllerProtocolGuid = { 0xebc01af5, 0x7a9,
> > > 0x489e, { 0xb7, 0xce, 0xdc, 0x8, 0x9e, 0x45, 0x9b, 0x2f } }
> > >
> > > +  ## Include/Protocol/UfsHostControllerInfo.h
> > > +  gUfsHcInfoProtocolGuid = { 0x3d18ba13, 0xd9b1, 0x4dd4, {0xb9,
> > > + 0x16, 0xd3,
> > > 0x07, 0x96, 0x53, 0x9e, 0xd8}}
> > > +
> > >    ## Include/Protocol/EsrtManagement.h
> > >    gEsrtManagementProtocolGuid         = { 0xa340c064, 0x723c, 0x4a9c,
> { 0xa4,
> > > 0xdd, 0xd5, 0xb4, 0x7a, 0x26, 0xfb, 0xb0 }}
> > >
> > > --
> > > 2.14.1.windows.1

Re: [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver

[Wu, Hao A]

> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Thursday, March 28, 2019 9:49 PM
> To: edk2-devel@lists.01.org
> Cc: Albecki, Mateusz; Wu; Wu, Hao A
> Subject: [PATCH 0/3] Implement UFS info protocol to pass additional UIC
> programming data from platform/silicon specific driver
> 
> UFS specification allows for additional, implementation specific, programming
> to be done during host controller initialization. Since some hosts
> might require it to allow for device detection we add a way for platform/silicon
> specific code to pass required attributes and their values to
> generic UFS driver. Please see UFS 2.0 specification for details of the host
> controller initialization sequence(JESD223B section 7.1.1 step 4).
> Patchset consists of 3 patches to achieve this. The first one is a bugfix for data
> transfers that are not aligned to DWORD boundary such as SCSI SENSE
> command. The second one is code refactoring for device presence detection
> that cleans up the function that sends UIC commands. Final patch defines
> UFS info protocol and implements driver logic for servicing it.
> 
> Tests performed:
> -Test UFS LU enumeration with UFS 2.0 card with 3 enabled LUs
> -Test that no unaligned data transfers are performed during enumeration
> -Test that UIC is programmed according to platform table

Hello Mateusz,

For the additional UIC during HC initialization change, could you help to
verify a 'reconnect -r' under Shell to see whether the UFS devices work
fine in such case? Thanks.

Best Regards,
Hao Wu


> -Test that driver operation is not impacted when no UFS info protocol is
> installed
> 
> CC: Wu, Hao A <hao.a.wu@intel.com>
> 
> Albecki, Mateusz (3):
>   MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling
>   MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection
>   MdeModulePkg/UfsPassThruDxe: Add UFS info protocol
> 
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  |  62 +++++
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |   3 +
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruDxe.inf      |   1 +
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 299 +++++++++++++++-----
> -
>  .../Include/Protocol/UfsHostControllerInfo.h       |  75 ++++++
>  MdeModulePkg/MdeModulePkg.dec                      |   3 +
>  6 files changed, 356 insertions(+), 87 deletions(-)
>  create mode 100644 MdeModulePkg/Include/Protocol/UfsHostControllerInfo.h
> 
> --
> 2.14.1.windows.1