From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: hao.a.wu@intel.com) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by groups.io with SMTP; Wed, 07 Aug 2019 19:37:19 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Aug 2019 19:37:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,358,1559545200"; d="scan'208";a="168835360" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga008.jf.intel.com with ESMTP; 07 Aug 2019 19:37:18 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 7 Aug 2019 19:37:18 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 7 Aug 2019 19:37:17 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.112]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.80]) with mapi id 14.03.0439.000; Thu, 8 Aug 2019 10:37:16 +0800 From: "Wu, Hao A" To: "devel@edk2.groups.io" , "Albecki, Mateusz" Subject: Re: [edk2-devel] [PATCH 3/4] MdeModulePkg/UfsPassThruDxe: Refactor private data to use EDKII_UFS_HC_INFO Thread-Topic: [edk2-devel] [PATCH 3/4] MdeModulePkg/UfsPassThruDxe: Refactor private data to use EDKII_UFS_HC_INFO Thread-Index: AQHVTUBnvv3wqMSQPUa3/KVVuEUpGabwhG/Q Date: Thu, 8 Aug 2019 02:37:16 +0000 Message-ID: References: <20190807165107.688-1-mateusz.albecki@intel.com> <20190807165107.688-4-mateusz.albecki@intel.com> In-Reply-To: <20190807165107.688-4-mateusz.albecki@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D1343 >=20 > 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. >=20 > Cc: Hao A Wu > Signed-off-by: Mateusz Albecki > --- > 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(-) >=20 > 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 >=20 > - Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved. > + Copyright (c) 2014 - 2019, Intel Corporation. All rights reserved. > SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > **/ > @@ -35,7 +35,7 @@ UFS_PASS_THRU_PRIVATE_DATA > gUfsPassThruTemplate =3D { > }, > 0, // UfsHostController > 0, // UfsHcBase > - 0, // Capabilities > + {0, 0}, // UfsHcInfo > 0, // TaskTag > 0, // UtpTrlBase > 0, // Nutrs > @@ -865,6 +865,10 @@ UfsPassThruDriverBindingStart ( > Private->UfsHostController =3D UfsHc; > Private->UfsHcBase =3D UfsHcBase; > InitializeListHead (&Private->Queue); > + Status =3D GetUfsHcInfo (Private); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to initialize UfsHcInfo\n")); > + } >=20 I think when the driver fails to read the CAP & VER registers of the UFS H= C, the initialization process should be aborted. Do you agree to change the code to: Status =3D 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 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; >=20 > UINT8 TaskTag; >=20 > @@ -959,6 +959,19 @@ UfsRwUfsAttribute ( > IN OUT UINT32 *AttrSize > ); >=20 > +/** > + 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; > } >=20 > - Nutrs =3D (UINT8)((Private->Capabilities & UFS_HC_CAP_NUTRS) + 1); > + Nutrs =3D (UINT8)((Private->UfsHcInfo.Capabilities & UFS_HC_CAP_NUT= RS) > + 1); >=20 > for (Index =3D 0; Index < Nutrs; Index++) { > if ((Data & (BIT0 << Index)) =3D=3D 0) { > @@ -1754,7 +1754,7 @@ UfsAllocateAlignCommonBuffer ( > BOOLEAN Is32BitAddr; > EDKII_UFS_HOST_CONTROLLER_PROTOCOL *UfsHc; >=20 > - if ((Private->Capabilities & UFS_HC_CAP_64ADDR) =3D=3D > UFS_HC_CAP_64ADDR) { > + if ((Private->UfsHcInfo.Capabilities & UFS_HC_CAP_64ADDR) =3D=3D > UFS_HC_CAP_64ADDR) { > Is32BitAddr =3D FALSE; > } else { > Is32BitAddr =3D 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 =3D NULL; > CmdDescPhyAddr =3D 0; >=20 > - Status =3D UfsMmioRead32 (Private, UFS_HC_CAP_OFFSET, &Data); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > - Private->Capabilities =3D Data; > - > // > // Allocate and initialize UTP Task Management Request List. > // > - Nutmrs =3D (UINT8) (RShiftU64 ((Private->Capabilities & > UFS_HC_CAP_NUTMRS), 16) + 1); > + Nutmrs =3D (UINT8) (RShiftU64 ((Private->UfsHcInfo.Capabilities & > UFS_HC_CAP_NUTMRS), 16) + 1); > Status =3D 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 =3D NULL; > CmdDescPhyAddr =3D 0; >=20 > - Status =3D UfsMmioRead32 (Private, UFS_HC_CAP_OFFSET, &Data); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > - Private->Capabilities =3D Data; > - > // > // Allocate and initialize UTP Transfer Request List. > // > - Nutrs =3D (UINT8)((Private->Capabilities & UFS_HC_CAP_NUTRS) + 1); > + Nutrs =3D (UINT8)((Private->UfsHcInfo.Capabilities & UFS_HC_CAP_NUTR= S) > + 1); > Status =3D UfsAllocateAlignCommonBuffer (Private, Nutrs * sizeof (UTP= _TRD), > &CmdDescHost, &CmdDescPhyAddr, &CmdDescMapping); > if (EFI_ERROR (Status)) { > return Status; > @@ -2366,3 +2350,36 @@ ProcessAsyncTaskList ( > } > } >=20 > +/** > + 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 =3D UfsMmioRead32 (Private, UFS_HC_VER_OFFSET, &Data); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Private->UfsHcInfo.Version =3D Data; > + > + Status =3D UfsMmioRead32 (Private, UFS_HC_CAP_OFFSET, &Data); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Private->UfsHcInfo.Capabilities =3D Data; > + > + return EFI_SUCCESS; > +} > + > -- > 2.14.1.windows.1 >=20 > -------------------------------------------------------------------- >=20 > 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. >=20 > 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 th= e > sole use of the intended recipient(s). If you are not the intended recip= ient, > please contact the sender and delete all copies; any review or distribut= ion by > others is strictly prohibited. >=20 >=20 >=20