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...@intel.com> > Signed-off-by: Mateusz Albecki <mateusz.albe...@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...@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. > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45108): https://edk2.groups.io/g/devel/message/45108 Mute This Topic: https://groups.io/mt/32784358/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
