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

["Albecki, Mateusz" <mateusz.albecki@intel.com>]

Hi,

Sure I agree. That was my original intention and then I forgot to add goto Error.

Thanks,
Mateusz

> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, August 8, 2019 4:37 AM
> To: devel@edk2.groups.io; Albecki, Mateusz <mateusz.albecki@intel.com>
> Subject: RE: [edk2-devel] [PATCH 3/4] MdeModulePkg/UfsPassThruDxe:
> Refactor private data to use EDKII_UFS_HC_INFO
> 
> 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.
> >
> >
> > 

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

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.