Re: [edk2-devel] [PATCH 3/4] MdeModulePkg/UfsPassThruDxe: Refactor private data to use EDKII_UFS_HC_INFO

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

Hello Mateusz,

One inline comment below:


> -----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 3/4] MdeModulePkg/UfsPassThruDxe:
> Refactor private data to use EDKII_UFS_HC_INFO
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1343
> 
> Private data has been refactored to use EDKII_UFS_HC_INFO structure
> to store host controller capabilities and version
> information. Getting host controller data has been moved
> into single place and is done before host controller enable.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
> ---
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c  |  8 ++-
>  MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h  | 15 +++++-
>  .../Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c        | 57 ++++++++++++++----
> ----
>  3 files changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> index 1518b251d8..09333c51d6 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
> @@ -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
> 
>  **/
> @@ -35,7 +35,7 @@ UFS_PASS_THRU_PRIVATE_DATA
> gUfsPassThruTemplate = {
>    },
>    0,                              // UfsHostController
>    0,                              // UfsHcBase
> -  0,                              // Capabilities
> +  {0, 0},                         // UfsHcInfo
>    0,                              // TaskTag
>    0,                              // UtpTrlBase
>    0,                              // Nutrs
> @@ -865,6 +865,10 @@ UfsPassThruDriverBindingStart (
>    Private->UfsHostController    = UfsHc;
>    Private->UfsHcBase            = UfsHcBase;
>    InitializeListHead (&Private->Queue);
> +  Status = GetUfsHcInfo (Private);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "Failed to initialize UfsHcInfo\n"));
> +  }
> 

I think when the driver fails to read the CAP & VER registers of the UFS HC,
the initialization process should be aborted.

Do you agree to change the code to:

  Status = GetUfsHcInfo (Private);
  if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Failed to initialize UfsHcInfo\n"));
    goto Error;
    ^^^^^^^^^^^
  }

when I push this patch?

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

Best Regards,
Hao Wu


>    //
>    // Initialize UFS Host Controller H/W.
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> index b79be77709..c511aa8c7a 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
> @@ -62,7 +62,7 @@ typedef struct _UFS_PASS_THRU_PRIVATE_DATA {
>    EFI_UFS_DEVICE_CONFIG_PROTOCOL      UfsDevConfig;
>    EDKII_UFS_HOST_CONTROLLER_PROTOCOL  *UfsHostController;
>    UINTN                               UfsHcBase;
> -  UINT32                              Capabilities;
> +  EDKII_UFS_HC_INFO                   UfsHcInfo;
> 
>    UINT8                               TaskTag;
> 
> @@ -959,6 +959,19 @@ UfsRwUfsAttribute (
>    IN OUT UINT32                        *AttrSize
>    );
> 
> +/**
> +  Initializes UfsHcInfo field in private data.
> +
> +  @param[in] Private  Pointer to host controller private data.
> +
> +  @retval EFI_SUCCESS  UfsHcInfo initialized successfully.
> +  @retval Others       Failed to initalize UfsHcInfo.
> +**/
> +EFI_STATUS
> +GetUfsHcInfo (
> +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private
> +  );
> +
>  extern EFI_COMPONENT_NAME_PROTOCOL
> gUfsPassThruComponentName;
>  extern EFI_COMPONENT_NAME2_PROTOCOL
> gUfsPassThruComponentName2;
>  extern EFI_DRIVER_BINDING_PROTOCOL  gUfsPassThruDriverBinding;
> diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> index 6ea27e473c..74be3efc41 100644
> --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c
> @@ -731,7 +731,7 @@ UfsFindAvailableSlotInTrl (
>      return Status;
>    }
> 
> -  Nutrs   = (UINT8)((Private->Capabilities & UFS_HC_CAP_NUTRS) + 1);
> +  Nutrs   = (UINT8)((Private->UfsHcInfo.Capabilities & UFS_HC_CAP_NUTRS)
> + 1);
> 
>    for (Index = 0; Index < Nutrs; Index++) {
>      if ((Data & (BIT0 << Index)) == 0) {
> @@ -1754,7 +1754,7 @@ UfsAllocateAlignCommonBuffer (
>    BOOLEAN                              Is32BitAddr;
>    EDKII_UFS_HOST_CONTROLLER_PROTOCOL   *UfsHc;
> 
> -  if ((Private->Capabilities & UFS_HC_CAP_64ADDR) ==
> UFS_HC_CAP_64ADDR) {
> +  if ((Private->UfsHcInfo.Capabilities & UFS_HC_CAP_64ADDR) ==
> UFS_HC_CAP_64ADDR) {
>      Is32BitAddr = FALSE;
>    } else {
>      Is32BitAddr = TRUE;
> @@ -1947,7 +1947,6 @@ UfsInitTaskManagementRequestList (
>    IN  UFS_PASS_THRU_PRIVATE_DATA     *Private
>    )
>  {
> -  UINT32                 Data;
>    UINT8                  Nutmrs;
>    VOID                   *CmdDescHost;
>    EFI_PHYSICAL_ADDRESS   CmdDescPhyAddr;
> @@ -1961,17 +1960,10 @@ UfsInitTaskManagementRequestList (
>    CmdDescMapping = NULL;
>    CmdDescPhyAddr = 0;
> 
> -  Status = UfsMmioRead32 (Private, UFS_HC_CAP_OFFSET, &Data);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  Private->Capabilities = Data;
> -
>    //
>    // Allocate and initialize UTP Task Management Request List.
>    //
> -  Nutmrs = (UINT8) (RShiftU64 ((Private->Capabilities &
> UFS_HC_CAP_NUTMRS), 16) + 1);
> +  Nutmrs = (UINT8) (RShiftU64 ((Private->UfsHcInfo.Capabilities &
> UFS_HC_CAP_NUTMRS), 16) + 1);
>    Status = UfsAllocateAlignCommonBuffer (Private, Nutmrs * sizeof
> (UTP_TMRD), &CmdDescHost, &CmdDescPhyAddr, &CmdDescMapping);
>    if (EFI_ERROR (Status)) {
>      return Status;
> @@ -2020,7 +2012,6 @@ UfsInitTransferRequestList (
>    IN  UFS_PASS_THRU_PRIVATE_DATA     *Private
>    )
>  {
> -  UINT32                 Data;
>    UINT8                  Nutrs;
>    VOID                   *CmdDescHost;
>    EFI_PHYSICAL_ADDRESS   CmdDescPhyAddr;
> @@ -2034,17 +2025,10 @@ UfsInitTransferRequestList (
>    CmdDescMapping = NULL;
>    CmdDescPhyAddr = 0;
> 
> -  Status = UfsMmioRead32 (Private, UFS_HC_CAP_OFFSET, &Data);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -
> -  Private->Capabilities = Data;
> -
>    //
>    // Allocate and initialize UTP Transfer Request List.
>    //
> -  Nutrs  = (UINT8)((Private->Capabilities & UFS_HC_CAP_NUTRS) + 1);
> +  Nutrs  = (UINT8)((Private->UfsHcInfo.Capabilities & UFS_HC_CAP_NUTRS)
> + 1);
>    Status = UfsAllocateAlignCommonBuffer (Private, Nutrs * sizeof (UTP_TRD),
> &CmdDescHost, &CmdDescPhyAddr, &CmdDescMapping);
>    if (EFI_ERROR (Status)) {
>      return Status;
> @@ -2366,3 +2350,36 @@ ProcessAsyncTaskList (
>    }
>  }
> 
> +/**
> +  Initializes UfsHcInfo field in private data.
> +
> +  @param[in] Private  Pointer to host controller private data.
> +
> +  @retval EFI_SUCCESS  UfsHcInfo initialized successfully.
> +  @retval Others       Failed to initalize UfsHcInfo.
> +**/
> +EFI_STATUS
> +GetUfsHcInfo (
> +  IN UFS_PASS_THRU_PRIVATE_DATA  *Private
> +  )
> +{
> +  UINT32      Data;
> +  EFI_STATUS  Status;
> +
> +  Status = UfsMmioRead32 (Private, UFS_HC_VER_OFFSET, &Data);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Private->UfsHcInfo.Version = Data;
> +
> +  Status = UfsMmioRead32 (Private, UFS_HC_CAP_OFFSET, &Data);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Private->UfsHcInfo.Capabilities = Data;
> +
> +  return EFI_SUCCESS;
> +}
> +
> --
> 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.
> 
> 
>