* [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver @ 2019-03-28 13:48 Albecki, Mateusz 2019-03-28 13:48 ` [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Albecki, Mateusz ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Albecki, Mateusz @ 2019-03-28 13:48 UTC (permalink / raw) To: edk2-devel; +Cc: Albecki, Mateusz, Wu, Hao A 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling 2019-03-28 13:48 [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Albecki, Mateusz @ 2019-03-28 13:48 ` Albecki, Mateusz 2019-03-28 13:56 ` [PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection Albecki, Mateusz ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Albecki, Mateusz @ 2019-03-28 13:48 UTC (permalink / raw) To: edk2-devel; +Cc: Albecki, Mateusz, Albecki 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. ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection 2019-03-28 13:48 [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Albecki, Mateusz 2019-03-28 13:48 ` [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Albecki, Mateusz @ 2019-03-28 13:56 ` Albecki, Mateusz 2019-04-01 6:53 ` Wu, Hao A 2019-03-28 13:56 ` [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol Albecki, Mateusz 2019-04-01 6:53 ` [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Wu, Hao A 3 siblings, 1 reply; 10+ messages in thread From: Albecki, Mateusz @ 2019-03-28 13:56 UTC (permalink / raw) To: edk2-devel; +Cc: Albecki, Mateusz, Hao Wu 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. ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection 2019-03-28 13:56 ` [PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection Albecki, Mateusz @ 2019-04-01 6:53 ` Wu, Hao A 0 siblings, 0 replies; 10+ messages in thread From: Wu, Hao A @ 2019-04-01 6:53 UTC (permalink / raw) To: Albecki, Mateusz, edk2-devel@lists.01.org > -----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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol 2019-03-28 13:48 [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Albecki, Mateusz 2019-03-28 13:48 ` [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Albecki, Mateusz 2019-03-28 13:56 ` [PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection Albecki, Mateusz @ 2019-03-28 13:56 ` Albecki, Mateusz 2019-04-01 6:53 ` Wu, Hao A 2019-04-01 6:53 ` [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Wu, Hao A 3 siblings, 1 reply; 10+ messages in thread From: Albecki, Mateusz @ 2019-03-28 13:56 UTC (permalink / raw) To: edk2-devel; +Cc: Albecki, Mateusz, Hao Wu 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. ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol 2019-03-28 13:56 ` [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol Albecki, Mateusz @ 2019-04-01 6:53 ` Wu, Hao A [not found] ` <92CF190FF2351747A47C1708F0E09C0875DE7165@IRSMSX102.ger.corp.intel.com> 0 siblings, 1 reply; 10+ messages in thread From: Wu, Hao A @ 2019-04-01 6:53 UTC (permalink / raw) To: Albecki, Mateusz, edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <92CF190FF2351747A47C1708F0E09C0875DE7165@IRSMSX102.ger.corp.intel.com>]
* Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol [not found] ` <92CF190FF2351747A47C1708F0E09C0875DE7165@IRSMSX102.ger.corp.intel.com> @ 2019-04-12 6:35 ` Wu, Hao A 2019-04-12 23:22 ` Albecki, Mateusz 0 siblings, 1 reply; 10+ messages in thread From: Wu, Hao A @ 2019-04-12 6:35 UTC (permalink / raw) To: Albecki, Mateusz, devel@edk2.groups.io; +Cc: Ni, Ray 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol 2019-04-12 6:35 ` Wu, Hao A @ 2019-04-12 23:22 ` Albecki, Mateusz 2019-04-15 5:56 ` Wu, Hao A 0 siblings, 1 reply; 10+ messages in thread From: Albecki, Mateusz @ 2019-04-12 23:22 UTC (permalink / raw) To: Wu, Hao A, devel@edk2.groups.io; +Cc: Ni, Ray 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol 2019-04-12 23:22 ` Albecki, Mateusz @ 2019-04-15 5:56 ` Wu, Hao A 0 siblings, 0 replies; 10+ messages in thread From: Wu, Hao A @ 2019-04-15 5:56 UTC (permalink / raw) To: Albecki, Mateusz, devel@edk2.groups.io; +Cc: Ni, Ray > -----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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver 2019-03-28 13:48 [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Albecki, Mateusz ` (2 preceding siblings ...) 2019-03-28 13:56 ` [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol Albecki, Mateusz @ 2019-04-01 6:53 ` Wu, Hao A 3 siblings, 0 replies; 10+ messages in thread From: Wu, Hao A @ 2019-04-01 6:53 UTC (permalink / raw) To: Albecki, Mateusz, edk2-devel@lists.01.org > -----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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-15 5:56 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-03-28 13:48 [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Albecki, Mateusz 2019-03-28 13:48 ` [PATCH 1/3] MdeModulePkg/UfsPassThruDxe: Fix unaligned data transfer handling Albecki, Mateusz 2019-03-28 13:56 ` [PATCH 2/3] MdeModulePkg/UfsPassThruDxe: Refactor UFS device presence detection Albecki, Mateusz 2019-04-01 6:53 ` Wu, Hao A 2019-03-28 13:56 ` [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: Add UFS info protocol Albecki, Mateusz 2019-04-01 6:53 ` Wu, Hao A [not found] ` <92CF190FF2351747A47C1708F0E09C0875DE7165@IRSMSX102.ger.corp.intel.com> 2019-04-12 6:35 ` Wu, Hao A 2019-04-12 23:22 ` Albecki, Mateusz 2019-04-15 5:56 ` Wu, Hao A 2019-04-01 6:53 ` [PATCH 0/3] Implement UFS info protocol to pass additional UIC programming data from platform/silicon specific driver Wu, Hao A
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox