Inline comment below:
> -----Original Message----- > From: Ke, VincentX <vincentx...@intel.com> > Sent: Thursday, December 23, 2021 12:19 PM > To: devel@edk2.groups.io > Cc: Ke, VincentX <vincentx...@intel.com>; Wu, Hao A > <hao.a...@intel.com>; Ni, Ray <ray...@intel.com>; Chiu, Ian > <ian.c...@intel.com>; Chu, Maggie <maggie....@intel.com> > Subject: [PATCH v6 2/3] MdeModulePkg: Refactoring UFS DME request and > fix timing problem > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3775 > > Refactoring UFS DME request function and retry up to 5 times. > > Cc: Hao A Wu <hao.a...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Ian Chiu <ian.c...@intel.com> > Cc: Maggie Chu <maggie....@intel.com> > Signed-off-by: VincentX Ke <vincentx...@intel.com> > --- > MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c | 312 +++++++++++-------- > - > 1 file changed, 179 insertions(+), 133 deletions(-) > > diff --git a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c > b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c > index 2baa57593e..cffe8e02a7 100644 > --- a/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c > +++ b/MdeModulePkg/Bus/Ufs/UfsBlockIoPei/UfsHci.c > @@ -1,6 +1,6 @@ > /** @file > > - Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -557,7 +557,7 @@ UfsCreateDMCommandDesc ( > return EFI_INVALID_PARAMETER; > } > > - if ( ((Opcode == UtpQueryFuncOpcodeSetFlag) || (Opcode == > UtpQueryFuncOpcodeClrFlag) || (Opcode == UtpQueryFuncOpcodeTogFlag)) > + if ( ((Opcode == UtpQueryFuncOpcodeClrFlag) || (Opcode == > UtpQueryFuncOpcodeTogFlag)) Please help to just remove the above check. > && ((DataSize != 0) || (Data != NULL))) > { > return EFI_INVALID_PARAMETER; > @@ -747,60 +747,91 @@ UfsStopExecCmd ( > } > > /** > - Read or write specified device descriptor of a UFS device. > + Extracts return data from query response upiu. > > - @param[in] Private The pointer to the > UFS_PEIM_HC_PRIVATE_DATA data structure. > - @param[in] Read The boolean variable to show r/w direction. > - @param[in] DescId The ID of device descriptor. > - @param[in] Index The Index of device descriptor. > - @param[in] Selector The Selector of device descriptor. > - @param[in, out] Descriptor The buffer of device descriptor to be read or > written. > - @param[in] DescSize The size of device descriptor buffer. > + @param[in, out] Packet Pointer to the > UFS_DEVICE_MANAGEMENT_REQUEST_PACKET. > + @param[in] QueryResp Pointer to the query response. > > - @retval EFI_SUCCESS The device descriptor was read/written > successfully. > - @retval EFI_DEVICE_ERROR A device error occurred while attempting to > r/w the device descriptor. > - @retval EFI_TIMEOUT A timeout occurred while waiting for the > completion of r/w the device descriptor. > + @retval EFI_INVALID_PARAMETER Packet or QueryResp are empty or > opcode is invalid. > + @retval EFI_DEVICE_ERROR Data returned from device is invalid. > + @retval EFI_SUCCESS Data extracted. > > **/ > EFI_STATUS > -UfsRwDeviceDesc ( > - IN UFS_PEIM_HC_PRIVATE_DATA *Private, > - IN BOOLEAN Read, > - IN UINT8 DescId, > - IN UINT8 Index, > - IN UINT8 Selector, > - IN OUT VOID *Descriptor, > - IN UINT32 DescSize > +UfsGetReturnDataFromQueryResponse ( > + IN OUT UFS_DEVICE_MANAGEMENT_REQUEST_PACKET *Packet, > + IN UTP_QUERY_RESP_UPIU *QueryResp > ) > { > - EFI_STATUS Status; > - UFS_DEVICE_MANAGEMENT_REQUEST_PACKET Packet; > - UINT8 Slot; > - UTP_TRD *Trd; > - UINTN Address; > - UTP_QUERY_RESP_UPIU *QueryResp; > - UINT8 *CmdDescBase; > - UINT32 CmdDescSize; > - UINT16 ReturnDataSize; > + UINT16 ReturnDataSize; > > - ZeroMem (&Packet, sizeof > (UFS_DEVICE_MANAGEMENT_REQUEST_PACKET)); > + ReturnDataSize = 0; > > - if (Read) { > - Packet.DataDirection = UfsDataIn; > - Packet.InDataBuffer = Descriptor; > - Packet.InTransferLength = DescSize; > - Packet.Opcode = UtpQueryFuncOpcodeRdDesc; > - } else { > - Packet.DataDirection = UfsDataOut; > - Packet.OutDataBuffer = Descriptor; > - Packet.OutTransferLength = DescSize; > - Packet.Opcode = UtpQueryFuncOpcodeWrDesc; > + if ((Packet == NULL) || (QueryResp == NULL)) { > + return EFI_INVALID_PARAMETER; > } > > - Packet.DescId = DescId; > - Packet.Index = Index; > - Packet.Selector = Selector; > - Packet.Timeout = UFS_TIMEOUT; > + switch (Packet->Opcode) { > + case UtpQueryFuncOpcodeRdDesc: > + ReturnDataSize = QueryResp->Tsf.Length; > + SwapLittleEndianToBigEndian ((UINT8 *)&ReturnDataSize, sizeof > (UINT16)); > + // > + // Make sure the hardware device does not return more data than > expected. > + // > + if (ReturnDataSize > Packet->InTransferLength) { > + return EFI_DEVICE_ERROR; > + } > + > + CopyMem (Packet->InDataBuffer, (QueryResp + 1), ReturnDataSize); > + Packet->InTransferLength = ReturnDataSize; > + break; > + case UtpQueryFuncOpcodeWrDesc: > + ReturnDataSize = QueryResp->Tsf.Length; > + SwapLittleEndianToBigEndian ((UINT8 *)&ReturnDataSize, sizeof > (UINT16)); > + Packet->OutTransferLength = ReturnDataSize; > + break; > + case UtpQueryFuncOpcodeRdFlag: > + case UtpQueryFuncOpcodeSetFlag: > + case UtpQueryFuncOpcodeClrFlag: > + case UtpQueryFuncOpcodeTogFlag: > + // > + // The 'FLAG VALUE' field is at byte offset 3 of QueryResp->Tsf.Value > + // > + *((UINT8 *)(Packet->OutDataBuffer)) = *((UINT8 *)&(QueryResp- > >Tsf.Value) + 3); Use "OutDataBuffer" only for UtpQueryFuncOpcodeSetFlag, UtpQueryFuncOpcodeClrFlag and UtpQueryFuncOpcodeTogFlag. Use "InDataBuffer" for UtpQueryFuncOpcodeRdFlag. > + break; > + default: > + return EFI_INVALID_PARAMETER; > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + Creates Transfer Request descriptor and sends Query Request to the > device. > + > + @param[in] Private Pointer to the UFS_PEIM_HC_PRIVATE_DATA. > + @param[in, out] Packet Pointer to the > UFS_DEVICE_MANAGEMENT_REQUEST_PACKET. > + > + @retval EFI_SUCCESS The device descriptor was read/written > successfully. > + @retval EFI_INVALID_PARAMETER The DescId, Index and Selector fields in > Packet are invalid > + combination to point to a type of UFS device > descriptor. > + @retval EFI_DEVICE_ERROR A device error occurred while attempting to > r/w the device descriptor. > + @retval EFI_TIMEOUT A timeout occurred while waiting for the > completion of r/w the device descriptor. > + > +**/ > +EFI_STATUS > +UfsSendDmRequestRetry ( > + IN UFS_PEIM_HC_PRIVATE_DATA *Private, > + IN OUT UFS_DEVICE_MANAGEMENT_REQUEST_PACKET *Packet > + ) > +{ > + UINT8 Slot; > + EFI_STATUS Status; > + UTP_TRD *Trd; > + UINTN Address; > + UTP_QUERY_RESP_UPIU *QueryResp; > + UINT8 *CmdDescBase; > + UINT32 CmdDescSize; > > // > // Find out which slot of transfer request list is available. > @@ -814,8 +845,9 @@ UfsRwDeviceDesc ( > // > // Fill transfer request descriptor to this slot. > // > - Status = UfsCreateDMCommandDesc (Private, &Packet, Trd); > + Status = UfsCreateDMCommandDesc (Private, Packet, Trd); > if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to create DM command descriptor\n")); > return Status; > } > > @@ -835,43 +867,116 @@ UfsRwDeviceDesc ( > // Wait for the completion of the transfer request. > // > Address = Private->UfsHcBase + UFS_HC_UTRLDBR_OFFSET; > - Status = UfsWaitMemSet (Address, BIT0 << Slot, 0, Packet.Timeout); > + Status = UfsWaitMemSet (Address, (BIT0 << Slot), 0, Packet->Timeout); > if (EFI_ERROR (Status)) { > goto Exit; > } > > - if (QueryResp->QueryResp != 0) { > + if ((Trd->Ocs != 0) || (QueryResp->QueryResp != 0)) { Please use: if ((Trd->Ocs != 0) || (QueryResp->QueryResp != UfsUtpQueryResponseSuccess)) { > + DEBUG ((DEBUG_ERROR, "Failed to send query request, OCS = %X, > QueryResp = %X\n", Trd->Ocs, QueryResp->QueryResp)); > DumpQueryResponseResult (QueryResp->QueryResp); > Status = EFI_DEVICE_ERROR; > goto Exit; > } > > - if (Trd->Ocs == 0) { > - ReturnDataSize = QueryResp->Tsf.Length; > - SwapLittleEndianToBigEndian ((UINT8 *)&ReturnDataSize, sizeof > (UINT16)); > + Status = UfsGetReturnDataFromQueryResponse (Packet, QueryResp); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to get return data from query > response\n")); > + goto Exit; > + } > > - if (Read) { > - // > - // Make sure the hardware device does not return more data than > expected. > - // > - if (ReturnDataSize > Packet.InTransferLength) { > - Status = EFI_DEVICE_ERROR; > - goto Exit; > - } > +Exit: > + UfsStopExecCmd (Private, Slot); > + UfsPeimFreeMem (Private->Pool, CmdDescBase, CmdDescSize); > > - CopyMem (Packet.InDataBuffer, (QueryResp + 1), ReturnDataSize); > - Packet.InTransferLength = ReturnDataSize; > - } else { > - Packet.OutTransferLength = ReturnDataSize; > + return Status; > +} > + > +/** > + Sends Query Request to the device. Query is sent until device responds > correctly or counter runs out. > + > + @param[in] Private Pointer to the UFS_PEIM_HC_PRIVATE_DATA. > + @param[in, out] Packet Pointer to the > UFS_DEVICE_MANAGEMENT_REQUEST_PACKET. > + > + @retval EFI_SUCCESS The device responded correctly to the Query > request. > + @retval EFI_INVALID_PARAMETER The DescId, Index and Selector fields in > Packet are invalid > + combination to point to a type of UFS device > descriptor. > + @retval EFI_DEVICE_ERROR A device error occurred while waiting for > the response from the device. > + @retval EFI_TIMEOUT A timeout occurred while waiting for the > completion of the operation. > + > +**/ > +EFI_STATUS > +UfsSendDmRequest ( > + IN UFS_PEIM_HC_PRIVATE_DATA *Private, > + IN OUT UFS_DEVICE_MANAGEMENT_REQUEST_PACKET *Packet > + ) > +{ > + EFI_STATUS Status; > + UINT8 Retry; > + > + Status = EFI_SUCCESS; > + > + for (Retry = 0; Retry < 5; Retry++) { > + Status = UfsSendDmRequestRetry (Private, Packet); > + if (!EFI_ERROR (Status)) { > + return EFI_SUCCESS; > } > + } > + > + DEBUG ((DEBUG_ERROR, "Failed to get response from the device after %d > retries\n", Retry)); > + return Status; > +} > + > +/** > + Read or write specified device descriptor of a UFS device. > + > + @param[in] Private The pointer to the > UFS_PEIM_HC_PRIVATE_DATA data structure. > + @param[in] Read The boolean variable to show r/w direction. > + @param[in] DescId The ID of device descriptor. > + @param[in] Index The Index of device descriptor. > + @param[in] Selector The Selector of device descriptor. > + @param[in, out] Descriptor The buffer of device descriptor to be read or > written. > + @param[in] DescSize The size of device descriptor buffer. > + > + @retval EFI_SUCCESS The device descriptor was read/written > successfully. > + @retval EFI_DEVICE_ERROR A device error occurred while attempting to > r/w the device descriptor. > + @retval EFI_TIMEOUT A timeout occurred while waiting for the > completion of r/w the device descriptor. > + > +**/ > +EFI_STATUS > +UfsRwDeviceDesc ( > + IN UFS_PEIM_HC_PRIVATE_DATA *Private, > + IN BOOLEAN Read, > + IN UINT8 DescId, > + IN UINT8 Index, > + IN UINT8 Selector, > + IN OUT VOID *Descriptor, > + IN UINT32 DescSize > + ) > +{ > + EFI_STATUS Status; > + UFS_DEVICE_MANAGEMENT_REQUEST_PACKET Packet; > + > + ZeroMem (&Packet, sizeof > (UFS_DEVICE_MANAGEMENT_REQUEST_PACKET)); > + > + if (Read) { > + Packet.DataDirection = UfsDataIn; > + Packet.InDataBuffer = Descriptor; > + Packet.InTransferLength = DescSize; > + Packet.Opcode = UtpQueryFuncOpcodeRdDesc; > } else { > - Status = EFI_DEVICE_ERROR; > + Packet.DataDirection = UfsDataOut; > + Packet.OutDataBuffer = Descriptor; > + Packet.OutTransferLength = DescSize; > + Packet.Opcode = UtpQueryFuncOpcodeWrDesc; > } > > -Exit: > - UfsStopExecCmd (Private, Slot); > - UfsPeimFreeMem (Private->Pool, CmdDescBase, CmdDescSize); > + Packet.DescId = DescId; > + Packet.Index = Index; > + Packet.Selector = Selector; > + Packet.Timeout = UFS_TIMEOUT; > > + Status = UfsSendDmRequest (Private, &Packet); > return Status; > } > > @@ -898,12 +1003,6 @@ UfsRwFlags ( > { > EFI_STATUS Status; > UFS_DEVICE_MANAGEMENT_REQUEST_PACKET Packet; > - UINT8 Slot; > - UTP_TRD *Trd; > - UINTN Address; > - UTP_QUERY_RESP_UPIU *QueryResp; > - UINT8 *CmdDescBase; > - UINT32 CmdDescSize; > > if (Value == NULL) { > return EFI_INVALID_PARAMETER; > @@ -926,67 +1025,14 @@ UfsRwFlags ( > } > } > > - Packet.DescId = FlagId; > - Packet.Index = 0; > - Packet.Selector = 0; > - Packet.Timeout = UFS_TIMEOUT; > + Packet.OutDataBuffer = (VOID *)Value; Please use "Packet.OutDataBuffer" only for the write case, use "Packet.InDataBuffer" for read case. Also, I suggest to set OutTransferLength and InTransferLength to 0 respectively. > + Packet.DescId = FlagId; > + Packet.Index = 0; > + Packet.Selector = 0; > + Packet.Timeout = UFS_TIMEOUT; > > - // > - // Find out which slot of transfer request list is available. > - // > - Status = UfsFindAvailableSlotInTrl (Private, &Slot); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > - // > - // Fill transfer request descriptor to this slot. > - // > - Trd = ((UTP_TRD *)Private->UtpTrlBase) + Slot; > - Status = UfsCreateDMCommandDesc (Private, &Packet, Trd); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > - // > - // Check the transfer request result. > - // > - CmdDescBase = (UINT8 *)(UINTN)(LShiftU64 ((UINT64)Trd->UcdBaU, 32) | > LShiftU64 ((UINT64)Trd->UcdBa, 7)); > - QueryResp = (UTP_QUERY_RESP_UPIU *)(CmdDescBase + Trd->RuO * > sizeof (UINT32)); > - CmdDescSize = Trd->RuO * sizeof (UINT32) + Trd->RuL * sizeof (UINT32); > - > - // > - // Start to execute the transfer request. > - // > - UfsStartExecCmd (Private, Slot); > - > - // > - // Wait for the completion of the transfer request. > - // > - Address = Private->UfsHcBase + UFS_HC_UTRLDBR_OFFSET; > - Status = UfsWaitMemSet (Address, BIT0 << Slot, 0, Packet.Timeout); > - if (EFI_ERROR (Status)) { > - goto Exit; > - } > - > - if (QueryResp->QueryResp != 0) { > - DumpQueryResponseResult (QueryResp->QueryResp); > - Status = EFI_DEVICE_ERROR; > - goto Exit; > - } > - > - if (Trd->Ocs == 0) { > - // > - // The 'FLAG VALUE' field is at byte offset 3 of QueryResp->Tsf.Value > - // > - *Value = *((UINT8 *)&(QueryResp->Tsf.Value) + 3); > - } else { > - Status = EFI_DEVICE_ERROR; > - } > - > -Exit: > - UfsStopExecCmd (Private, Slot); > - UfsPeimFreeMem (Private->Pool, CmdDescBase, CmdDescSize); > + Status = UfsSendDmRequest (Private, &Packet); > + *Value = *((UINT8 *)(Packet.OutDataBuffer)); Similar comment as above, please use "Packet.InDataBuffer" for read case. Best Regards, Hao Wu > > return Status; > } > -- > 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#85190): https://edk2.groups.io/g/devel/message/85190 Mute This Topic: https://groups.io/mt/87913153/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
