Re: [edk2-devel] [PATCH 2/4] MdeModulePkg/UfsPassThruDxe: Refactor UfsExecUicCommand function

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Albecki, Mateusz
> Sent: Thursday, August 08, 2019 12:51 AM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz; Wu, Hao A
> Subject: [edk2-devel] [PATCH 2/4] MdeModulePkg/UfsPassThruDxe:
> Refactor UfsExecUicCommand function
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1343
> 
> UfsExecUicCommand function has been refactored to allow
> the caller to check the command results which is important
> for commands such as UIC read.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> ---
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  |  3 +-
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 47 ++++++++++++-------
> ---
>  2 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> index 9b68db5ffe..b79be77709 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2019, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -13,6 +13,7 @@
>  #include <Protocol/ScsiPassThruExt.h>
>  #include <Protocol/UfsDeviceConfig.h>
>  #include <Protocol/UfsHostController.h>
> +#include <Protocol/UfsHostControllerPlatform.h>
> 
>  #include <Library/DebugLib.h>
>  #include <Library/UefiDriverEntryPoint.h>
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index 912d6f8202..6ea27e473c 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -2,7 +2,7 @@
>    UfsPassThruDxe driver is used to produce EFI_EXT_SCSI_PASS_THRU
> protocol interface
>    for upper layer application to execute UFS-supported SCSI cmds.
> 
> -  Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2019, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -1633,11 +1633,8 @@ Exit1:
>  /**
>    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.
> -  @param[in] Arg1             The value for 1st argument of the UIC command.
> -  @param[in] Arg2             The value for 2nd argument of the UIC command.
> -  @param[in] Arg3             The value for 3rd argument of the UIC command.
> +  @param[in]      Private     The pointer to the
> UFS_PASS_THRU_PRIVATE_DATA data structure.
> +  @param[in, out] UicCommand  UIC command descriptor. On exit contains
> UIC command results.
> 
>    @return EFI_SUCCESS      Successfully execute this UIC command and
> detect attached UFS device.
>    @return EFI_DEVICE_ERROR Fail to execute this UIC command and detect
> attached UFS device.
> @@ -1646,10 +1643,7 @@ Exit1:
>  EFI_STATUS
>  UfsExecUicCommands (
>    IN  UFS_PASS_THRU_PRIVATE_DATA    *Private,
> -  IN  UINT8                         UicOpcode,
> -  IN  UINT32                        Arg1,
> -  IN  UINT32                        Arg2,
> -  IN  UINT32                        Arg3
> +  IN OUT EDKII_UIC_COMMAND          *UicCommand
>    )
>  {
>    EFI_STATUS  Status;
> @@ -1675,17 +1669,17 @@ UfsExecUicCommands (
>    // only after all the UIC command argument registers (UICCMDARG1,
> UICCMDARG2 and UICCMDARG3)
>    // are set.
>    //
> -  Status = UfsMmioWrite32 (Private, UFS_HC_UCMD_ARG1_OFFSET, Arg1);
> +  Status = UfsMmioWrite32 (Private, UFS_HC_UCMD_ARG1_OFFSET,
> UicCommand->Arg1);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> 
> -  Status = UfsMmioWrite32 (Private, UFS_HC_UCMD_ARG2_OFFSET, Arg2);
> +  Status = UfsMmioWrite32 (Private, UFS_HC_UCMD_ARG2_OFFSET,
> UicCommand->Arg2);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> 
> -  Status = UfsMmioWrite32 (Private, UFS_HC_UCMD_ARG3_OFFSET, Arg3);
> +  Status = UfsMmioWrite32 (Private, UFS_HC_UCMD_ARG3_OFFSET,
> UicCommand->Arg3);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -1698,7 +1692,7 @@ UfsExecUicCommands (
>      return Status;
>    }
> 
> -  Status = UfsMmioWrite32 (Private, UFS_HC_UIC_CMD_OFFSET,
> (UINT32)UicOpcode);
> +  Status = UfsMmioWrite32 (Private, UFS_HC_UIC_CMD_OFFSET,
> UicCommand->Opcode);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -1712,14 +1706,18 @@ UfsExecUicCommands (
>      return Status;
>    }
> 
> -  if (UicOpcode != UfsUicDmeReset) {
> -    Status = UfsMmioRead32 (Private, UFS_HC_UCMD_ARG2_OFFSET,
> &Data);
> +  if (UicCommand->Opcode != UfsUicDmeReset) {
> +    Status = UfsMmioRead32 (Private, UFS_HC_UCMD_ARG2_OFFSET,
> &UicCommand->Arg2);
>      if (EFI_ERROR (Status)) {
>        return Status;
>      }
> -    if ((Data & 0xFF) != 0) {
> +    Status = UfsMmioRead32 (Private, UFS_HC_UCMD_ARG3_OFFSET,
> &UicCommand->Arg3);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +    if ((UicCommand->Arg2 & 0xFF) != 0) {
>        DEBUG_CODE_BEGIN();
> -        DumpUicCmdExecResult (UicOpcode, (UINT8)(Data & 0xFF));
> +        DumpUicCmdExecResult ((UINT8)UicCommand->Opcode,
> (UINT8)(UicCommand->Arg2 & 0xFF));
>        DEBUG_CODE_END();
>        return EFI_DEVICE_ERROR;
>      }
> @@ -1898,16 +1896,21 @@ UfsDeviceDetection (
>    IN  UFS_PASS_THRU_PRIVATE_DATA     *Private
>    )
>  {
> -  UINTN       Retry;
> -  EFI_STATUS  Status;
> -  UINT32      Data;
> +  UINTN              Retry;
> +  EFI_STATUS         Status;
> +  UINT32             Data;
> +  EDKII_UIC_COMMAND  LinkStartupCommand;
> 
>    //
>    // Start UFS device detection.
>    // Try up to 3 times for establishing data link with device.
>    //
>    for (Retry = 0; Retry < 3; Retry++) {
> -    Status = UfsExecUicCommands (Private, UfsUicDmeLinkStartup, 0, 0, 0);
> +    LinkStartupCommand.Opcode = UfsUicDmeLinkStartup;
> +    LinkStartupCommand.Arg1 = 0;
> +    LinkStartupCommand.Arg2 = 0;
> +    LinkStartupCommand.Arg3 = 0;
> +    Status = UfsExecUicCommands (Private, &LinkStartupCommand);


The patch looks good to me,
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


>      if (EFI_ERROR (Status)) {
>        return EFI_DEVICE_ERROR;
>      }
> --
> 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.
> 
> 
>