From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: hao.a.wu@intel.com) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by groups.io with SMTP; Mon, 24 Jun 2019 01:46:21 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Jun 2019 01:46:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,411,1557212400"; d="scan'208";a="187857379" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga002.fm.intel.com with ESMTP; 24 Jun 2019 01:46:20 -0700 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 24 Jun 2019 01:46:20 -0700 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 24 Jun 2019 01:46:19 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 24 Jun 2019 01:46:19 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.185]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.225]) with mapi id 14.03.0439.000; Mon, 24 Jun 2019 16:46:18 +0800 From: "Wu, Hao A" To: "Albecki, Mateusz" , "devel@edk2.groups.io" Subject: Re: [PATCH v3 2/2] MdeModulePkg/SdMmcHcDxe: Implement revision 3 of SdMmcOverrideProtocol Thread-Topic: [PATCH v3 2/2] MdeModulePkg/SdMmcHcDxe: Implement revision 3 of SdMmcOverrideProtocol Thread-Index: AQHVKEPLMl+YKlQELESlkF0rcl4h9aaqGvCggABmM9A= Date: Mon, 24 Jun 2019 08:46:18 +0000 Message-ID: References: <20190621151205.2444-1-mateusz.albecki@intel.com> <20190621151205.2444-3-mateusz.albecki@intel.com> In-Reply-To: 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 > -----Original Message----- > From: Wu, Hao A > Sent: Monday, June 24, 2019 4:15 PM > To: Albecki, Mateusz; devel@edk2.groups.io > Subject: RE: [PATCH v3 2/2] MdeModulePkg/SdMmcHcDxe: Implement > revision 3 of SdMmcOverrideProtocol >=20 > > -----Original Message----- > > From: Albecki, Mateusz > > Sent: Friday, June 21, 2019 11:12 PM > > To: devel@edk2.groups.io > > Cc: Albecki, Mateusz; Wu, Hao A > > Subject: [PATCH v3 2/2] MdeModulePkg/SdMmcHcDxe: Implement > revision > > 3 of SdMmcOverrideProtocol > > > > From: "Albecki, Mateusz" >=20 >=20 > Hello, >=20 > Some inline comments below: >=20 One more minor comment for EmmcGetTargetBusWidth() below: >=20 > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=3D1882 > > > > Implement support for GetOperatingParamters notify phase > > in SdMmcHcDxe driver. GetOperatingParameters notify phase > > is signaled before we start card detection and initialization. > > Code has been updated for both eMMC and SD card controllers to > > take into consideration those new parameters. Initialization process > > has been divided into 2 steps. In the first step we bring the link > > up to the point where we can get card identification data(Extended > > CSD in eMMC case and SWITCH command response in SD card case). This > > data is later used along with controller capabilities and operating > > parameters passed in GetOperatingParameters phase to choose prefered > > bus settings in GetTargetBusSettings function. Those settings are later > > on to start bus training to high speeds. If user passes incompatible > > setting with selected bus timing driver will assume it's standard behav= ior > > with respect to that setting. For instance if HS400 has been selected a= s a > > target bus timing due to card and controller support bus width setting = of > > 4 and 1 bit won't be respected and 8 bit setting will be choosen instea= d. > > > > Cc: Hao A Wu > > Signed-off-by: Mateusz Albecki > > --- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 512 > > +++++++++++++++------ > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c | 409 > > +++++++++++++--- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 52 ++- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 18 +- > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 34 ++ > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 19 + > > 6 files changed, 813 insertions(+), 231 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > index deaf4468c9..5645a32906 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > @@ -641,13 +641,13 @@ EmmcSwitchBusWidth ( > > Refer to EMMC Electrical Standard Spec 5.1 Section 6.6 and SD Host > > Controller > > Simplified Spec 3.0 Figure 3-3 for details. > > > > - @param[in] PciIo A pointer to the EFI_PCI_IO_PROTOCOL insta= nce. > > - @param[in] PassThru A pointer to the > > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. > > - @param[in] Slot The slot number of the SD card to send the > command > > to. > > - @param[in] Rca The relative device address to be assigned= . > > - @param[in] HsTiming The value to be written to HS_TIMING field= of > > EXT_CSD register. > > - @param[in] Timing The bus mode timing indicator. > > - @param[in] ClockFreq The max clock frequency to be set, the uni= t is > > MHz. > > + @param[in] PciIo A pointer to the EFI_PCI_IO_PROTOCOL inst= ance. > > + @param[in] PassThru A pointer to the > > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. > > + @param[in] Slot The slot number of the SD card to send th= e > > command to. > > + @param[in] Rca The relative device address to be assigne= d. > > + @param[in] DriverStrength Driver strength to set for speed modes th= at > > support it. > > + @param[in] BusTiming The bus mode timing indicator. > > + @param[in] ClockFreq The max clock frequency to be set, the un= it is > > MHz. > > > > @retval EFI_SUCCESS The operation is done correctly. > > @retval Others The operation fails. > > @@ -659,8 +659,8 @@ EmmcSwitchBusTiming ( > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > IN UINT8 Slot, > > IN UINT16 Rca, > > - IN UINT8 HsTiming, > > - IN SD_MMC_BUS_MODE Timing, > > + IN EDKII_SD_MMC_DRIVER_STRENGTH DriverStrength, > > + IN SD_MMC_BUS_MODE BusTiming, > > IN UINT32 ClockFreq > > ) > > { > > @@ -678,12 +678,29 @@ EmmcSwitchBusTiming ( > > // > > Access =3D 0x03; > > Index =3D OFFSET_OF (EMMC_EXT_CSD, HsTiming); > > - Value =3D HsTiming; > > CmdSet =3D 0; > > + switch (BusTiming) { > > + case SdMmcMmcHs400: > > + Value =3D (UINT8)((DriverStrength.Emmc << 4) | 3); > > + break; > > + case SdMmcMmcHs200: > > + Value =3D (UINT8)((DriverStrength.Emmc << 4) | 2); > > + break; > > + case SdMmcMmcHsSdr: > > + case SdMmcMmcHsDdr: > > + Value =3D 1; > > + break; > > + case SdMmcMmcLegacy: > > + Value =3D 0; > > + break; > > + default: > > + DEBUG ((DEBUG_ERROR, "EmmcSwitchBusTiming: Unsupported > > BusTiming(%d\n)", BusTiming)); > > + return EFI_INVALID_PARAMETER; > > + } > > > > Status =3D EmmcSwitch (PassThru, Slot, Access, Index, Value, CmdSet)= ; > > if (EFI_ERROR (Status)) { > > - DEBUG ((DEBUG_ERROR, "EmmcSwitchBusTiming: Switch to hstiming %d > > fails with %r\n", HsTiming, Status)); > > + DEBUG ((DEBUG_ERROR, "EmmcSwitchBusTiming: Switch to bus > > timing %d fails with %r\n", BusTiming, Status)); > > return Status; > > } > > > > @@ -713,7 +730,7 @@ EmmcSwitchBusTiming ( > > Private->ControllerHandle, > > Slot, > > EdkiiSdMmcSwitchClockFreqPost, > > - &Timing > > + &BusTiming > > ); > > if (EFI_ERROR (Status)) { > > DEBUG (( > > @@ -739,10 +756,7 @@ EmmcSwitchBusTiming ( > > @param[in] PassThru A pointer to the > > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. > > @param[in] Slot The slot number of the SD card to send the > command > > to. > > @param[in] Rca The relative device address to be assigned= . > > - @param[in] ClockFreq The max clock frequency to be set. > > - @param[in] IsDdr If TRUE, use dual data rate data simpling = method. > > Otherwise > > - use single data rate data simpling method. > > - @param[in] BusWidth The bus width to be set, it could be 4 or = 8. > > + @param[in] BusMode Pointer to SD_MMC_BUS_SETTINGS structure > > containing bus settings. > > > > @retval EFI_SUCCESS The operation is done correctly. > > @retval Others The operation fails. > > @@ -754,25 +768,34 @@ EmmcSwitchToHighSpeed ( > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > IN UINT8 Slot, > > IN UINT16 Rca, > > - IN UINT32 ClockFreq, > > - IN BOOLEAN IsDdr, > > - IN UINT8 BusWidth > > + IN SD_MMC_BUS_SETTINGS *BusMode > > ) > > { > > EFI_STATUS Status; > > - UINT8 HsTiming; > > UINT8 HostCtrl1; > > - SD_MMC_BUS_MODE Timing; > > SD_MMC_HC_PRIVATE_DATA *Private; > > + BOOLEAN IsDdr; > > > > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > > > - Status =3D EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, > BusWidth); > > + if ((BusMode->BusTiming !=3D SdMmcMmcHsSdr && BusMode- > > >BusTiming !=3D SdMmcMmcHsDdr) || > > + BusMode->ClockFreq > 52) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (BusMode->BusTiming =3D=3D SdMmcMmcHsDdr) { > > + IsDdr =3D TRUE; > > + } else { > > + IsDdr =3D FALSE; > > + } > > + > > + Status =3D EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, > BusMode- > > >BusWidth); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > + > > // > > - // Set to Hight Speed timing > > + // Set to High Speed timing > > // > > HostCtrl1 =3D BIT2; > > Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1, sizeof > > (HostCtrl1), &HostCtrl1); > > @@ -780,37 +803,25 @@ EmmcSwitchToHighSpeed ( > > return Status; > > } > > > > - if (IsDdr) { > > - Timing =3D SdMmcMmcHsDdr; > > - } else if (ClockFreq =3D=3D 52) { > > - Timing =3D SdMmcMmcHsSdr; > > - } else { > > - Timing =3D SdMmcMmcLegacy; > > - } > > - > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Sl= ot, > > Timing); > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Sl= ot, > > BusMode->BusTiming); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > > > - HsTiming =3D 1; > > - Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming= , > Timing, > > ClockFreq); > > - > > - return Status; > > + return EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, BusMode- > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq); > > } > > > > /** > > - Switch to the HS200 timing according to request. > > + Switch to the HS200 timing. This function assumes that eMMC bus is s= till > in > > legacy mode. > > > > Refer to EMMC Electrical Standard Spec 5.1 Section 6.6.8 and SD Host > > Controller > > Simplified Spec 3.0 Figure 2-29 for details. > > > > - @param[in] PciIo A pointer to the EFI_PCI_IO_PROTOCOL insta= nce. > > - @param[in] PassThru A pointer to the > > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. > > - @param[in] Slot The slot number of the SD card to send the > command > > to. > > - @param[in] Rca The relative device address to be assigned= . > > - @param[in] ClockFreq The max clock frequency to be set. > > - @param[in] BusWidth The bus width to be set, it could be 4 or = 8. > > + @param[in] PciIo A pointer to the EFI_PCI_IO_PROTOCOL inst= ance. > > + @param[in] PassThru A pointer to the > > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. > > + @param[in] Slot The slot number of the SD card to send th= e > > command to. > > + @param[in] Rca The relative device address to be assigne= d. > > + @param[in] BusMode Pointer to SD_MMC_BUS_SETTINGS structure > > containing bus settings. > > > > @retval EFI_SUCCESS The operation is done correctly. > > @retval Others The operation fails. > > @@ -822,30 +833,25 @@ EmmcSwitchToHS200 ( > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > IN UINT8 Slot, > > IN UINT16 Rca, > > - IN UINT32 ClockFreq, > > - IN UINT8 BusWidth > > + IN SD_MMC_BUS_SETTINGS *BusMode > > ) > > { > > EFI_STATUS Status; > > - UINT8 HsTiming; > > UINT16 ClockCtrl; > > - SD_MMC_BUS_MODE Timing; > > SD_MMC_HC_PRIVATE_DATA *Private; > > > > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > > > - if ((BusWidth !=3D 4) && (BusWidth !=3D 8)) { > > + if (BusMode->BusTiming !=3D SdMmcMmcHs200 || > > + (BusMode->BusWidth !=3D 4 && BusMode->BusWidth !=3D 8)) { > > return EFI_INVALID_PARAMETER; > > } > > > > - Status =3D EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, FALSE, > BusWidth); > > + Status =3D EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, FALSE, > > BusMode->BusWidth); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > // > > - // Set to HS200/SDR104 timing > > - // > > - // > > // Stop bus clock at first > > // > > Status =3D SdMmcHcStopClock (PciIo, Slot); > > @@ -853,9 +859,7 @@ EmmcSwitchToHS200 ( > > return Status; > > } > > > > - Timing =3D SdMmcMmcHs200; > > - > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Sl= ot, > > Timing); > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Sl= ot, > > BusMode->BusTiming); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -881,28 +885,27 @@ EmmcSwitchToHS200 ( > > ClockCtrl =3D BIT2; > > Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof > > (ClockCtrl), &ClockCtrl); > > > > - HsTiming =3D 2; > > - Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming= , > Timing, > > ClockFreq); > > + Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, BusMode- > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > > > - Status =3D EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusWidth); > > + Status =3D EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusMode- > > >BusWidth); > > > > return Status; > > } > > > > /** > > - Switch to the HS400 timing according to request. > > + Switch to the HS400 timing. This function assumes that eMMC bus is s= till > in > > legacy mode. > > > > Refer to EMMC Electrical Standard Spec 5.1 Section 6.6.8 and SD Host > > Controller > > Simplified Spec 3.0 Figure 2-29 for details. > > > > - @param[in] PciIo A pointer to the EFI_PCI_IO_PROTOCOL insta= nce. > > - @param[in] PassThru A pointer to the > > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. > > - @param[in] Slot The slot number of the SD card to send the > command > > to. > > - @param[in] Rca The relative device address to be assigned= . > > - @param[in] ClockFreq The max clock frequency to be set. > > + @param[in] PciIo A pointer to the EFI_PCI_IO_PROTOCOL inst= ance. > > + @param[in] PassThru A pointer to the > > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. > > + @param[in] Slot The slot number of the SD card to send th= e > > command to. > > + @param[in] Rca The relative device address to be assigne= d. > > + @param[in] BusMode Pointer to SD_MMC_BUS_SETTINGS structure > > containing bus settings. > > > > @retval EFI_SUCCESS The operation is done correctly. > > @retval Others The operation fails. > > @@ -914,47 +917,314 @@ EmmcSwitchToHS400 ( > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > IN UINT8 Slot, > > IN UINT16 Rca, > > - IN UINT32 ClockFreq > > + IN SD_MMC_BUS_SETTINGS *BusMode > > ) > > { > > EFI_STATUS Status; > > - UINT8 HsTiming; > > - SD_MMC_BUS_MODE Timing; > > SD_MMC_HC_PRIVATE_DATA *Private; > > + SD_MMC_BUS_SETTINGS Hs200BusMode; > > + UINT32 HsFreq; > > + > > + if (BusMode->BusTiming !=3D SdMmcMmcHs400 || > > + BusMode->BusWidth !=3D 8) { > > + return EFI_INVALID_PARAMETER; > > + } > > > > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > + Hs200BusMode.BusTiming =3D SdMmcMmcHs200; > > + Hs200BusMode.BusWidth =3D BusMode->BusWidth; > > + Hs200BusMode.ClockFreq =3D BusMode->ClockFreq; > > + Hs200BusMode.DriverStrength =3D BusMode->DriverStrength; > > > > - Status =3D EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFreq,= 8); > > + Status =3D EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, > &Hs200BusMode); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > + > > // > > - // Set to Hight Speed timing and set the clock frequency to a value = less > than > > 52MHz. > > + // Set to High Speed timing and set the clock frequency to a value l= ess > than > > or equal to 52MHz. > > + // This step is necessary to be able to switch Bus into 8 bit DDR mo= de > which > > is unsupported in HS200. > > // > > - HsTiming =3D 1; > > - Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming= , > > SdMmcMmcHsSdr, 52); > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Sl= ot, > > SdMmcMmcHsSdr); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > - // > > - // HS400 mode must use 8 data lines. > > - // > > - Status =3D EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, TRUE, 8); > > + > > + HsFreq =3D BusMode->ClockFreq < 52 ? BusMode->ClockFreq : 52; > > + Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, BusMode- > > >DriverStrength, SdMmcMmcHsSdr, HsFreq); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > > > - Timing =3D SdMmcMmcHs400; > > + Status =3D EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, TRUE, > BusMode- > > >BusWidth); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Sl= ot, > > Timing); > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Sl= ot, > > BusMode->BusTiming); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > > > - HsTiming =3D 3; > > - Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming= , > Timing, > > ClockFreq); > > + return EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, BusMode- > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq); > > +} > > > > - return Status; > > +/** > > + Check if passed BusTiming is supported in both controller and card. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] ExtCsd Pointer to the card's extended CSD > > + @param[in] BusTiming Bus timing to check > > + > > + @retval TRUE Both card and controller support given BusTiming > > + @retval FALSE Card or controller doesn't support given BusTiming > > +**/ > > +BOOLEAN > > +EmmcIsBusTimingSupported ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN EMMC_EXT_CSD *ExtCsd, > > + IN SD_MMC_BUS_MODE BusTiming > > + ) > > +{ > > + BOOLEAN Supported; > > + SD_MMC_HC_SLOT_CAP *Capabilities; > > + > > + Capabilities =3D &Private->Capability[SlotIndex]; > > + > > + Supported =3D FALSE; > > + switch (BusTiming) { > > + case SdMmcMmcHs400: > > + if ((((ExtCsd->DeviceType & (BIT6 | BIT7)) !=3D 0) && (Capabili= ties- > > >Hs400 !=3D 0)) && Capabilities->BusWidth8) { >=20 >=20 > Please use: > (Capabilities->BusWidth8 !=3D 0) > instead of: > Capabilities->BusWidth8 > for non-BOOLEAN type in 'if' statement. >=20 > Really sorry for missing this in previous reply. >=20 >=20 > > + Supported =3D TRUE; > > + } > > + break; > > + case SdMmcMmcHs200: > > + if ((((ExtCsd->DeviceType & (BIT4 | BIT5)) !=3D 0) && (Capabili= ties- > > >Sdr104 !=3D 0))) { > > + Supported =3D TRUE; > > + } > > + break; > > + case SdMmcMmcHsDdr: > > + if ((((ExtCsd->DeviceType & (BIT2 | BIT3)) !=3D 0) && (Capabili= ties- > > >Ddr50 !=3D 0))) { > > + Supported =3D TRUE; > > + } > > + break; > > + case SdMmcMmcHsSdr: > > + if ((((ExtCsd->DeviceType & BIT1) !=3D 0) && (Capabilities->Hig= hSpeed !=3D > > 0))) { > > + Supported =3D TRUE; > > + } > > + break; > > + case SdMmcMmcLegacy: > > + if ((ExtCsd->DeviceType & BIT0) !=3D 0) { > > + Supported =3D TRUE; > > + } > > + break; > > + default: > > + ASSERT (FALSE); > > + } > > + > > + return Supported; > > +} > > + > > +/** > > + Get the target bus timing to set on the link. This function > > + will try to select highest bus timing supported by card, controller > > + and the driver. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] ExtCsd Pointer to the card's extended CSD > > + > > + @return Bus timing value that should be set on link > > +**/ > > +SD_MMC_BUS_MODE > > +EmmcGetTargetBusTiming ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN EMMC_EXT_CSD *ExtCsd > > + ) > > +{ > > + SD_MMC_BUS_MODE BusTiming; > > + > > + // > > + // We start with highest bus timing that this driver currently suppo= rts and > > + // return as soon as we find supported timing. > > + // > > + BusTiming =3D SdMmcMmcHs400; > > + while (BusTiming > SdMmcMmcLegacy) { > > + if (EmmcIsBusTimingSupported (Private, SlotIndex, ExtCsd, BusTimin= g)) > { > > + break; > > + } > > + BusTiming--; > > + } > > + > > + return BusTiming; > > +} > > + > > +/** > > + Check if the passed bus width is supported by controller and card. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] BusTiming Bus timing set on the link > > + @param[in] BusWidth Bus width to check > > + > > + @retval TRUE Passed bus width is supported in current bus > configuration > > + @retval FALSE Passed bus width is not supported in current bus > > configuration > > +**/ > > +BOOLEAN > > +EmmcIsBusWidthSupported ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN SD_MMC_BUS_MODE BusTiming, > > + IN UINT16 BusWidth > > + ) > > +{ > > + if (BusWidth =3D=3D 8 && Private->Capability[SlotIndex].BusWidth8) { >=20 >=20 > Please use: > (Private->Capability[SlotIndex].BusWidth8 !=3D 0) > instead of: > Private->Capability[SlotIndex].BusWidth8 > for non-BOOLEAN type in 'if' statement. >=20 > Really sorry for missing this in previous reply. >=20 >=20 > > + return TRUE; > > + } else if (BusWidth =3D=3D 4 && BusTiming !=3D SdMmcMmcHs400) { > > + return TRUE; > > + } else if (BusWidth =3D=3D 1 && (BusTiming =3D=3D SdMmcMmcHsSdr || > BusTiming > > =3D=3D SdMmcMmcLegacy)) { > > + return TRUE; > > + } > > + > > + return FALSE; > > +} > > + > > +/** > > + Get the target bus width to be set on the bus. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] ExtCsd Pointer to card's extended CSD > > + @param[in] BusTiming Bus timing set on the bus > > + > > + @return Bus width to be set on the bus > > +**/ > > +UINT8 > > +EmmcGetTargetBusWidth ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN EMMC_EXT_CSD *ExtCsd, > > + IN SD_MMC_BUS_MODE BusTiming > > + ) > > +{ > > + UINT8 BusWidth; > > + UINT8 PreferredBusWidth; > > + > > + PreferredBusWidth =3D Private- > > >Slot[SlotIndex].OperatingParameters.BusWidth; > > + > > + if (PreferredBusWidth !=3D EDKII_SD_MMC_BUS_WIDTH_IGNORE && > > + EmmcIsBusWidthSupported (Private, SlotIndex, BusTiming, > > PreferredBusWidth)) { > > + BusWidth =3D PreferredBusWidth; > > + } else if (EmmcIsBusWidthSupported (Private, SlotIndex, BusTiming, 8= )) { > > + BusWidth =3D 8; > > + } else if (EmmcIsBusWidthSupported (Private, SlotIndex, BusTiming, 4= )) { > > + BusWidth =3D 4; > > + } else if (EmmcIsBusWidthSupported (Private, SlotIndex, BusTiming, 1= )) { > > + BusWidth =3D 1; > > + } > > + As complaint by GCC49 tool chain and some static code checkers: 'BusWidth' is possible to be uninitialized upon return if all the above 'if' statements are not satisfied. Best Regards, Hao Wu > > + return BusWidth; > > +} > > + > > +/** > > + Get the target clock frequency to be set on the bus. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] ExtCsd Pointer to card's extended CSD > > + @param[in] BusTiming Bus timing to be set on the bus > > + > > + @return Value of the clock frequency to be set on bus in MHz > > +**/ > > +UINT32 > > +EmmcGetTargetClockFreq ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN EMMC_EXT_CSD *ExtCsd, > > + IN SD_MMC_BUS_MODE BusTiming > > + ) > > +{ > > + UINT32 PreferredClockFreq; > > + UINT32 MaxClockFreq; > > + > > + PreferredClockFreq =3D Private- > > >Slot[SlotIndex].OperatingParameters.ClockFreq; > > + > > + switch (BusTiming) { > > + case SdMmcMmcHs400: > > + case SdMmcMmcHs200: > > + MaxClockFreq =3D 200; > > + break; > > + case SdMmcMmcHsSdr: > > + case SdMmcMmcHsDdr: > > + MaxClockFreq =3D 52; > > + break; > > + default: > > + MaxClockFreq =3D 26; > > + break; > > + } > > + > > + if (PreferredClockFreq !=3D EDKII_SD_MMC_CLOCK_FREQ_IGNORE && > > PreferredClockFreq < MaxClockFreq) { > > + return PreferredClockFreq; > > + } else { > > + return MaxClockFreq; > > + } > > +} > > + > > +/** > > + Get the driver strength to be set on bus. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] ExtCsd Pointer to card's extended CSD > > + @param[in] BusTiming Bus timing set on the bus > > + > > + @return Value of the driver strength to be set on the bus > > +**/ > > +EDKII_SD_MMC_DRIVER_STRENGTH > > +EmmcGetTargetDriverStrength ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN EMMC_EXT_CSD *ExtCsd, > > + IN SD_MMC_BUS_MODE BusTiming > > + ) > > +{ > > + EDKII_SD_MMC_DRIVER_STRENGTH PreferredDriverStrength; > > + EDKII_SD_MMC_DRIVER_STRENGTH DriverStrength; > > + > > + PreferredDriverStrength =3D Private- > > >Slot[SlotIndex].OperatingParameters.DriverStrength; > > + DriverStrength.Emmc =3D EmmcDriverStrengthType0; > > + > > + if (PreferredDriverStrength.Emmc !=3D > > EDKII_SD_MMC_DRIVER_STRENGTH_IGNORE && > > + (ExtCsd->DriverStrength & (BIT0 << PreferredDriverStrength.Emmc)= )) { > > + DriverStrength.Emmc =3D PreferredDriverStrength.Emmc; > > + } > > + > > + return DriverStrength; > > +} > > + > > +/** > > + Get the target settings for the bus mode. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] ExtCsd Pointer to card's extended CSD > > + @param[out] BusMode Target configuration of the bus > > +**/ > > +VOID > > +EmmcGetTargetBusMode ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN EMMC_EXT_CSD *ExtCsd, > > + OUT SD_MMC_BUS_SETTINGS *BusMode > > + ) > > +{ > > + BusMode->BusTiming =3D EmmcGetTargetBusTiming (Private, SlotIndex, > > ExtCsd); > > + BusMode->BusWidth =3D EmmcGetTargetBusWidth (Private, SlotIndex, > > ExtCsd, BusMode->BusTiming); > > + BusMode->ClockFreq =3D EmmcGetTargetClockFreq (Private, SlotIndex, > > ExtCsd, BusMode->BusTiming); > > + BusMode->DriverStrength =3D EmmcGetTargetDriverStrength (Private, > > SlotIndex, ExtCsd, BusMode->BusTiming); > > } > > > > /** > > @@ -983,10 +1253,7 @@ EmmcSetBusMode ( > > EFI_STATUS Status; > > EMMC_CSD Csd; > > EMMC_EXT_CSD ExtCsd; > > - UINT8 HsTiming; > > - BOOLEAN IsDdr; > > - UINT32 ClockFreq; > > - UINT8 BusWidth; > > + SD_MMC_BUS_SETTINGS BusMode; > > SD_MMC_HC_PRIVATE_DATA *Private; > > > > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > @@ -1004,85 +1271,30 @@ EmmcSetBusMode ( > > } > > > > ASSERT (Private->BaseClkFreq[Slot] !=3D 0); > > + > > // > > - // Check if the Host Controller support 8bits bus width. > > - // > > - if (Private->Capability[Slot].BusWidth8 !=3D 0) { > > - BusWidth =3D 8; > > - } else { > > - BusWidth =3D 4; > > - } > > - // > > - // Get Deivce_Type from EXT_CSD register. > > + // Get Device_Type from EXT_CSD register. > > // > > Status =3D EmmcGetExtCsd (PassThru, Slot, &ExtCsd); > > if (EFI_ERROR (Status)) { > > DEBUG ((DEBUG_ERROR, "EmmcSetBusMode: GetExtCsd fails > with %r\n", > > Status)); > > return Status; > > } > > - // > > - // Calculate supported bus speed/bus width/clock frequency. > > - // > > - HsTiming =3D 0; > > - IsDdr =3D FALSE; > > - ClockFreq =3D 0; > > - if (((ExtCsd.DeviceType & (BIT4 | BIT5)) !=3D 0) && (Private- > > >Capability[Slot].Sdr104 !=3D 0)) { > > - HsTiming =3D 2; > > - IsDdr =3D FALSE; > > - ClockFreq =3D 200; > > - } else if (((ExtCsd.DeviceType & (BIT2 | BIT3)) !=3D 0) && (Private= - > > >Capability[Slot].Ddr50 !=3D 0)) { > > - HsTiming =3D 1; > > - IsDdr =3D TRUE; > > - ClockFreq =3D 52; > > - } else if (((ExtCsd.DeviceType & BIT1) !=3D 0) && (Private- > > >Capability[Slot].HighSpeed !=3D 0)) { > > - HsTiming =3D 1; > > - IsDdr =3D FALSE; > > - ClockFreq =3D 52; > > - } else if (((ExtCsd.DeviceType & BIT0) !=3D 0) && (Private- > > >Capability[Slot].HighSpeed !=3D 0)) { > > - HsTiming =3D 1; > > - IsDdr =3D FALSE; > > - ClockFreq =3D 26; > > - } > > - // > > - // Check if both of the device and the host controller support HS400= DDR > > mode. > > - // > > - if (((ExtCsd.DeviceType & (BIT6 | BIT7)) !=3D 0) && (Private- > > >Capability[Slot].Hs400 !=3D 0)) { > > - // > > - // The host controller supports 8bits bus. > > - // > > - ASSERT (BusWidth =3D=3D 8); > > - HsTiming =3D 3; > > - IsDdr =3D TRUE; > > - ClockFreq =3D 200; > > - } > > > > - if ((ClockFreq =3D=3D 0) || (HsTiming =3D=3D 0)) { > > - // > > - // Continue using default setting. > > - // > > - return EFI_SUCCESS; > > - } > > + EmmcGetTargetBusMode (Private, Slot, &ExtCsd, &BusMode); > > > > - DEBUG ((DEBUG_INFO, "EmmcSetBusMode: HsTiming %d ClockFreq %d > > BusWidth %d Ddr %a\n", HsTiming, ClockFreq, BusWidth, IsDdr ? > > "TRUE":"FALSE")); > > + DEBUG ((DEBUG_INFO, "EmmcSetBusMode: Target bus mode: timing > > =3D %d, width =3D %d, clock freq =3D %d, driver strength =3D %d\n", > > + BusMode.BusTiming, BusMode.BusWidth, > BusMode.ClockFreq, > > BusMode.DriverStrength.Emmc)); > > > > - if (HsTiming =3D=3D 3) { > > - // > > - // Execute HS400 timing switch procedure > > - // > > - Status =3D EmmcSwitchToHS400 (PciIo, PassThru, Slot, Rca, ClockFre= q); > > - } else if (HsTiming =3D=3D 2) { > > - // > > - // Execute HS200 timing switch procedure > > - // > > - Status =3D EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFre= q, > > BusWidth); > > + if (BusMode.BusTiming =3D=3D SdMmcMmcHs400) { > > + Status =3D EmmcSwitchToHS400 (PciIo, PassThru, Slot, Rca, &BusMode= ); > > + } else if (BusMode.BusTiming =3D=3D SdMmcMmcHs200) { > > + Status =3D EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, &BusMode= ); > > } else { > > - // > > - // Execute High Speed timing switch procedure > > - // > > - Status =3D EmmcSwitchToHighSpeed (PciIo, PassThru, Slot, Rca, Cloc= kFreq, > > IsDdr, BusWidth); > > + Status =3D EmmcSwitchToHighSpeed (PciIo, PassThru, Slot, Rca, > &BusMode); > > } > > > > - DEBUG ((DEBUG_INFO, "EmmcSetBusMode: Switch to %a %r\n", > (HsTiming > > =3D=3D 3) ? "HS400" : ((HsTiming =3D=3D 2) ? "HS200" : "HighSpeed"), St= atus)); > > + DEBUG ((DEBUG_INFO, "EmmcSetBusMode: Switch to %a %r\n", > > (BusMode.BusTiming =3D=3D SdMmcMmcHs400) ? "HS400" : > > ((BusMode.BusTiming =3D=3D SdMmcMmcHs200) ? "HS200" : "HighSpeed"), > > Status)); > > > > return Status; > > } > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c > > index 17936a5492..27a13e1008 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c > > @@ -313,10 +313,6 @@ SdCardSetRca ( > > return Status; > > } > > > > - > > - > > - > > - > > /** > > Send command SELECT_DESELECT_CARD to the SD device to > > select/deselect it. > > > > @@ -472,14 +468,14 @@ SdCardSetBusWidth ( > > > > Refer to SD Physical Layer Simplified Spec 4.1 Section 4.7 for detai= ls. > > > > - @param[in] PassThru A pointer to the > > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. > > - @param[in] Slot The slot number of the SD card to send the > command > > to. > > - @param[in] AccessMode The value for access mode group. > > - @param[in] CommandSystem The value for command set group. > > - @param[in] DriveStrength The value for drive length group. > > - @param[in] PowerLimit The value for power limit group. > > - @param[in] Mode Switch or check function. > > - @param[out] SwitchResp The return switch function status. > > + @param[in] PassThru A pointer to the > > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. > > + @param[in] Slot The slot number of the SD card to send th= e > > command to. > > + @param[in] BusTiming Target bus timing based on which access g= roup > > value will be set. > > + @param[in] CommandSystem The value for command set group. > > + @param[in] DriverStrength The value for driver strength group. > > + @param[in] PowerLimit The value for power limit group. > > + @param[in] Mode Switch or check function. > > + @param[out] SwitchResp The return switch function status. > > > > @retval EFI_SUCCESS The operation is done correctly. > > @retval Others The operation fails. > > @@ -489,9 +485,9 @@ EFI_STATUS > > SdCardSwitch ( > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > IN UINT8 Slot, > > - IN UINT8 AccessMode, > > + IN SD_MMC_BUS_MODE BusTiming, > > IN UINT8 CommandSystem, > > - IN UINT8 DriveStrength, > > + IN SD_DRIVER_STRENGTH_TYPE DriverStrength, > > IN UINT8 PowerLimit, > > IN BOOLEAN Mode, > > OUT UINT8 *SwitchResp > > @@ -502,6 +498,7 @@ SdCardSwitch ( > > EFI_SD_MMC_PASS_THRU_COMMAND_PACKET Packet; > > EFI_STATUS Status; > > UINT32 ModeValue; > > + UINT8 AccessMode; > > > > ZeroMem (&SdMmcCmdBlk, sizeof (SdMmcCmdBlk)); > > ZeroMem (&SdMmcStatusBlk, sizeof (SdMmcStatusBlk)); > > @@ -516,14 +513,48 @@ SdCardSwitch ( > > SdMmcCmdBlk.ResponseType =3D SdMmcResponseTypeR1; > > > > ModeValue =3D Mode ? BIT31 : 0; > > - SdMmcCmdBlk.CommandArgument =3D (AccessMode & 0xF) | > ((PowerLimit > > & 0xF) << 4) | \ > > - ((DriveStrength & 0xF) << 8) | ((Drive= Strength & 0xF) << 12) > > | \ > > + > > + switch (BusTiming) { > > + case SdMmcUhsDdr50: > > + AccessMode =3D 0x4; > > + break; > > + case SdMmcUhsSdr104: > > + AccessMode =3D 0x3; > > + break; > > + case SdMmcUhsSdr50: > > + AccessMode =3D 0x2; > > + break; > > + case SdMmcUhsSdr25: > > + case SdMmcSdHs: > > + AccessMode =3D 0x1; > > + break; > > + case SdMmcUhsSdr12: > > + case SdMmcSdDs: > > + AccessMode =3D 0; > > + break; > > + default: > > + AccessMode =3D 0xF; > > + } > > + > > + SdMmcCmdBlk.CommandArgument =3D (AccessMode & 0xF) | > > ((CommandSystem & 0xF) << 4) | \ > > + ((DriverStrength & 0xF) << 8) | ((Powe= rLimit & 0xF) << 12) > | > > \ > > ModeValue; > > > > Packet.InDataBuffer =3D SwitchResp; > > Packet.InTransferLength =3D 64; > > > > Status =3D SdMmcPassThruPassThru (PassThru, Slot, &Packet, NULL); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + if (!Mode) { > > + if ((((AccessMode & 0xF) !=3D 0xF) && (SwitchResp[16] =3D=3D 0xF))= || > > + (((CommandSystem & 0xF) !=3D 0xF) && (SwitchResp[15] =3D=3D 0x= F)) || > > + (((DriverStrength & 0xF) !=3D 0xF) && (SwitchResp[14] =3D=3D 0= xF)) || > > + (((PowerLimit & 0xF) !=3D 0xF) && (SwitchResp[13] =3D=3D 0xF))= ) > > + return EFI_DEVICE_ERROR; > > + } >=20 >=20 > A couple of comments for the above check. >=20 > Per my understanding, the check is used to verify whether the target > values for function groups have been successfully set in Mode 1 (Set). > Hence, I think the below: > if (!Mode) {...} > should be: > if (Mode) {...} >=20 > The result of the Mode 1 Switch command is 4-bit wide for each function > group, so for: >=20 > Access Mode (Function group 1, bits 379:376) - SwitchResp[16] & 0xF > Command System (Function group 2, bits 383:380) - (SwitchResp[16] >> 4) & > 0xF > Driver Strength(Function group 3, bits 387:384) - SwitchResp[15] & 0xF > Current Limit (Function group 4, bits 391:388) - (SwitchResp[15] >> 4) &= 0xF >=20 > Also, according to the SD spec: > ''' > CMD6 mode 1 ... > ... > When a function cannot be switched because it is busy, the card returns > the current function number (not returns 0xF), the other functions in the > other groups may still be switched. > ''' >=20 > So how about comparing the Switch command results with the target value > rather than value '0xF'? >=20 >=20 > > > > return Status; > > } > > @@ -748,6 +779,273 @@ SdCardSwitchBusWidth ( > > return Status; > > } > > > > +/** > > + Check if passed BusTiming is supported in both controller and card. > > + > > + @param[in] Private Pointer to controller private da= ta > > + @param[in] SlotIndex Index of the slot in the control= ler > > + @param[in] CardSupportedBusTimings Bitmask indicating which bus > > timings are supported by card > > + @param[in] IsInUhsI Flag indicating if link is in UH= S-I > > + > > + @retval TRUE Both card and controller support given BusTiming > > + @retval FALSE Card or controller doesn't support given BusTiming > > +**/ > > +BOOLEAN > > +SdIsBusTimingSupported ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN UINT8 CardSupportedBusTimings, > > + IN BOOLEAN IsInUhsI, > > + IN SD_MMC_BUS_MODE BusTiming > > + ) > > +{ > > + SD_MMC_HC_SLOT_CAP *Capability; > > + > > + Capability =3D &Private->Capability[SlotIndex]; > > + > > + if (IsInUhsI) { > > + switch (BusTiming) { > > + case SdMmcUhsSdr104: > > + if (Capability->Sdr104 && ((CardSupportedBusTimings & BIT3) != =3D 0)) { >=20 >=20 > Please help to update to: > Capability->Sdr104 !=3D 0 >=20 > Three more similar cases below in this function. > Really sorry for missing this in previous reply. >=20 >=20 > > + return TRUE; > > + } > > + break; > > + case SdMmcUhsDdr50: > > + if (Capability->Ddr50 && ((CardSupportedBusTimings & BIT4) != =3D 0)) { > > + return TRUE; > > + } > > + break; > > + case SdMmcUhsSdr50: > > + if (Capability->Sdr50 && ((CardSupportedBusTimings & BIT2) != =3D 0)) { > > + return TRUE; > > + } > > + break; > > + case SdMmcUhsSdr25: > > + if ((CardSupportedBusTimings & BIT1) !=3D 0) { > > + return TRUE; > > + } > > + break; > > + case SdMmcUhsSdr12: > > + if ((CardSupportedBusTimings & BIT0) !=3D 0) { > > + return TRUE; > > + } > > + break; > > + default: > > + break; > > + } > > + } else { > > + switch (BusTiming) { > > + case SdMmcSdHs: > > + if (Capability->HighSpeed && (CardSupportedBusTimings & BIT1) = !=3D 0) > { > > + return TRUE; > > + } > > + break; > > + case SdMmcSdDs: > > + if ((CardSupportedBusTimings & BIT0) !=3D 0) { > > + return TRUE; > > + } > > + break; > > + default: > > + break; > > + } > > + } > > + > > + return FALSE; > > +} > > + > > +/** > > + Get the target bus timing to set on the link. This function > > + will try to select highest bus timing supported by card, controller > > + and the driver. > > + > > + @param[in] Private Pointer to controller private da= ta > > + @param[in] SlotIndex Index of the slot in the control= ler > > + @param[in] CardSupportedBusTimings Bitmask indicating which bus > > timings are supported by card > > + @param[in] IsInUhsI Flag indicating if link is in UH= S-I > > + > > + @return Bus timing value that should be set on link > > +**/ > > +SD_MMC_BUS_MODE > > +SdGetTargetBusTiming ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN UINT8 CardSupportedBusTimings, > > + IN BOOLEAN IsInUhsI > > + ) > > +{ > > + SD_MMC_BUS_MODE BusTiming; > > + > > + if (IsInUhsI) { > > + BusTiming =3D SdMmcUhsSdr104; > > + } else { > > + BusTiming =3D SdMmcSdHs; > > + } > > + > > + while (BusTiming > SdMmcSdDs) { > > + if (SdIsBusTimingSupported (Private, SlotIndex, > > CardSupportedBusTimings, IsInUhsI, BusTiming)) { > > + break; > > + } > > + BusTiming--; > > + } > > + > > + return BusTiming; > > +} > > + > > +/** > > + Get the target bus width to be set on the bus. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] BusTiming Bus timing set on the bus > > + > > + @return Bus width to be set on the bus > > +**/ > > +UINT8 > > +SdGetTargetBusWidth ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN SD_MMC_BUS_MODE BusTiming > > + ) > > +{ > > + UINT8 BusWidth; > > + UINT8 PreferredBusWidth; > > + > > + PreferredBusWidth =3D Private- > > >Slot[SlotIndex].OperatingParameters.BusWidth; > > + > > + if (BusTiming =3D=3D SdMmcSdDs || BusTiming =3D=3D SdMmcSdHs) { > > + if (PreferredBusWidth !=3D EDKII_SD_MMC_BUS_WIDTH_IGNORE && > > + (PreferredBusWidth =3D=3D 1 || PreferredBusWidth =3D=3D 4)) { > > + BusWidth =3D PreferredBusWidth; > > + } else { > > + BusWidth =3D 4; > > + } > > + } else { > > + // > > + // UHS-I modes support only 4-bit width. > > + // Switch to 4-bit has been done before calling this function anyw= ay so > > + // this is purely informational. > > + // > > + BusWidth =3D 4; > > + } > > + > > + return BusWidth; > > +} > > + > > +/** > > + Get the target clock frequency to be set on the bus. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] BusTiming Bus timing to be set on the bus > > + > > + @return Value of the clock frequency to be set on bus in MHz > > +**/ > > +UINT32 > > +SdGetTargetBusClockFreq ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN SD_MMC_BUS_MODE BusTiming > > + ) > > +{ > > + UINT32 PreferredClockFreq; > > + UINT32 MaxClockFreq; > > + > > + PreferredClockFreq =3D Private- > > >Slot[SlotIndex].OperatingParameters.ClockFreq; > > + > > + switch (BusTiming) { > > + case SdMmcUhsSdr104: > > + MaxClockFreq =3D 208; > > + break; > > + case SdMmcUhsSdr50: > > + MaxClockFreq =3D 100; > > + break; > > + case SdMmcUhsDdr50: > > + case SdMmcUhsSdr25: > > + case SdMmcSdHs: > > + MaxClockFreq =3D 50; > > + break; > > + case SdMmcUhsSdr12: > > + case SdMmcSdDs: > > + default: > > + MaxClockFreq =3D 25; > > + } > > + > > + if (PreferredClockFreq !=3D EDKII_SD_MMC_CLOCK_FREQ_IGNORE && > > PreferredClockFreq < MaxClockFreq) { > > + return PreferredClockFreq; > > + } else { > > + return MaxClockFreq; > > + } > > +} > > + > > +/** > > + Get the driver strength to be set on bus. > > + > > + @param[in] Private Pointer to controller priva= te data > > + @param[in] SlotIndex Index of the slot in the co= ntroller > > + @param[in] CardSupportedDriverStrengths Bitmask indicating which > > driver strengths are supported on the card > > + @param[in] BusTiming Bus timing set on the bus > > + > > + @return Value of the driver strength to be set on the bus > > +**/ > > +EDKII_SD_MMC_DRIVER_STRENGTH > > +SdGetTargetDriverStrength ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN UINT8 CardSupportedDriverStrengths, > > + IN SD_MMC_BUS_MODE BusTiming > > + ) > > +{ > > + EDKII_SD_MMC_DRIVER_STRENGTH PreferredDriverStrength; > > + EDKII_SD_MMC_DRIVER_STRENGTH DriverStrength; > > + > > + if (BusTiming =3D=3D SdMmcSdDs || BusTiming =3D=3D SdMmcSdHs) { > > + DriverStrength.Sd =3D SdDriverStrengthIgnore; > > + return DriverStrength; > > + } > > + > > + PreferredDriverStrength =3D Private- > > >Slot[SlotIndex].OperatingParameters.DriverStrength; > > + DriverStrength.Sd =3D SdDriverStrengthTypeB; > > + > > + if (PreferredDriverStrength.Sd !=3D > > EDKII_SD_MMC_DRIVER_STRENGTH_IGNORE && > > + (CardSupportedDriverStrengths & (BIT0 << > PreferredDriverStrength.Sd))) > > { > > + > > + if ((PreferredDriverStrength.Sd =3D=3D SdDriverStrengthTypeA && > > + Private->Capability[SlotIndex].DriverTypeA) || >=20 >=20 > Please help to update to: > Private->Capability[SlotIndex].DriverTypeA !=3D 0 >=20 > Two more similar cases below in this function. > Really sorry for missing this in previous reply. >=20 >=20 > > + (PreferredDriverStrength.Sd =3D=3D SdDriverStrengthTypeC && > > + Private->Capability[SlotIndex].DriverTypeC) || > > + (PreferredDriverStrength.Sd =3D=3D SdDriverStrengthTypeD && > > + Private->Capability[SlotIndex].DriverTypeD)) { > > + DriverStrength.Sd =3D PreferredDriverStrength.Sd; > > + } > > + } > > + > > + return DriverStrength; > > +} > > + > > +/** > > + Get the target settings for the bus mode. > > + > > + @param[in] Private Pointer to controller private data > > + @param[in] SlotIndex Index of the slot in the controller > > + @param[in] SwitchQueryResp Pointer to switch query response > > + @param[in] IsInUhsI Flag indicating if link is in UHS-I mod= e > > + @param[out] BusMode Target configuration of the bus > > +**/ > > +VOID > > +SdGetTargetBusMode ( > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > + IN UINT8 SlotIndex, > > + IN UINT8 *SwitchQueryResp, > > + IN BOOLEAN IsInUhsI, > > + OUT SD_MMC_BUS_SETTINGS *BusMode > > + ) > > +{ > > + BusMode->BusTiming =3D SdGetTargetBusTiming (Private, SlotIndex, > > SwitchQueryResp[13], IsInUhsI); > > + BusMode->BusWidth =3D SdGetTargetBusWidth (Private, SlotIndex, > > BusMode->BusTiming); > > + BusMode->ClockFreq =3D SdGetTargetBusClockFreq (Private, SlotIndex, > > BusMode->BusTiming); > > + BusMode->DriverStrength =3D SdGetTargetDriverStrength (Private, > > SlotIndex, SwitchQueryResp[9], BusMode->BusTiming); > > +} > > + > > /** > > Switch the high speed timing according to request. > > > > @@ -775,13 +1073,10 @@ SdCardSetBusMode ( > > { > > EFI_STATUS Status; > > SD_MMC_HC_SLOT_CAP *Capability; > > - UINT32 ClockFreq; > > - UINT8 BusWidth; > > - UINT8 AccessMode; > > UINT8 HostCtrl1; > > UINT8 SwitchResp[64]; > > - SD_MMC_BUS_MODE Timing; > > SD_MMC_HC_PRIVATE_DATA *Private; > > + SD_MMC_BUS_SETTINGS BusMode; > > > > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > > > @@ -792,61 +1087,51 @@ SdCardSetBusMode ( > > return Status; > > } > > > > - BusWidth =3D 4; > > - > > - Status =3D SdCardSwitchBusWidth (PciIo, PassThru, Slot, Rca, BusWidt= h); > > - if (EFI_ERROR (Status)) { > > - return Status; > > + if (S18A) { > > + // > > + // For UHS-I speed modes 4-bit data bus is requiered so we > > + // switch here irrespective of platform preference. > > + // > > + Status =3D SdCardSwitchBusWidth (PciIo, PassThru, Slot, Rca, 4); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > } > > + > > // > > // Get the supported bus speed from SWITCH cmd return data group #1. > > // > > - Status =3D SdCardSwitch (PassThru, Slot, 0xF, 0xF, 0xF, 0xF, FALSE, > > SwitchResp); > > + Status =3D SdCardSwitch (PassThru, Slot, 0xFF, 0xF, SdDriverStrength= Ignore, > > 0xF, FALSE, SwitchResp); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > - // > > - // Calculate supported bus speed/bus width/clock frequency by host a= nd > > device capability. > > - // > > - ClockFreq =3D 0; > > - if (S18A && (Capability->Sdr104 !=3D 0) && ((SwitchResp[13] & BIT3) = !=3D 0)) { > > - ClockFreq =3D 208; > > - AccessMode =3D 3; > > - Timing =3D SdMmcUhsSdr104; > > - } else if (S18A && (Capability->Sdr50 !=3D 0) && ((SwitchResp[13] & = BIT2) !=3D > 0)) > > { > > - ClockFreq =3D 100; > > - AccessMode =3D 2; > > - Timing =3D SdMmcUhsSdr50; > > - } else if (S18A && (Capability->Ddr50 !=3D 0) && ((SwitchResp[13] & = BIT4) !=3D > > 0)) { > > - ClockFreq =3D 50; > > - AccessMode =3D 4; > > - Timing =3D SdMmcUhsDdr50; > > - } else if ((SwitchResp[13] & BIT1) !=3D 0) { > > - ClockFreq =3D 50; > > - AccessMode =3D 1; > > - Timing =3D SdMmcUhsSdr25; > > - } else { > > - ClockFreq =3D 25; > > - AccessMode =3D 0; > > - Timing =3D SdMmcUhsSdr12; > > + > > + SdGetTargetBusMode (Private, Slot, SwitchResp, S18A, &BusMode); > > + > > + DEBUG ((DEBUG_INFO, "SdCardSetBusMode: Target bus mode: bus > timing > > =3D %d, bus width =3D %d, clock freq[MHz] =3D %d, driver strength =3D %= d\n", > > + BusMode.BusTiming, BusMode.BusWidth, > BusMode.ClockFreq, > > BusMode.DriverStrength.Sd)); > > + > > + if (!S18A) { > > + Status =3D SdCardSwitchBusWidth (PciIo, PassThru, Slot, Rca, > > BusMode.BusWidth); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > } >=20 > I am sorry for not being clear in the previous reply. > Do you think it is okay to combine the below 2 chunks of codes (bus width > setting) together? >=20 > if (S18A) { > // > // For UHS-I speed modes 4-bit data bus is requiered so we > // switch here irrespective of platform preference. > // > Status =3D SdCardSwitchBusWidth (PciIo, PassThru, Slot, Rca, 4); > if (EFI_ERROR (Status)) { > return Status; > } > } >=20 > and >=20 > if (!S18A) { > Status =3D SdCardSwitchBusWidth (PciIo, PassThru, Slot, Rca, > BusMode.BusWidth); > if (EFI_ERROR (Status)) { > return Status; > } > } >=20 >=20 > The returned bus width is guaranteed to be 4 after calling > SdGetTargetBusMode() for UHS-I speed modes. >=20 >=20 > > > > - Status =3D SdCardSwitch (PassThru, Slot, AccessMode, 0xF, 0xF, 0xF, = TRUE, > > SwitchResp); > > + Status =3D SdCardSwitch (PassThru, Slot, BusMode.BusTiming, 0xF, > > BusMode.DriverStrength.Sd, 0xF, TRUE, SwitchResp); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > > > - if ((SwitchResp[16] & 0xF) !=3D AccessMode) { > > - DEBUG ((DEBUG_ERROR, "SdCardSetBusMode: Switch to > AccessMode %d > > ClockFreq %d BusWidth %d fails! The Switch response is 0x%1x\n", > > AccessMode, ClockFreq, BusWidth, SwitchResp[16] & 0xF)); > > - return EFI_DEVICE_ERROR; > > + Status =3D SdMmcSetDriverStrength (Private->PciIo, Slot, > > BusMode.DriverStrength.Sd); > > + if (EFI_ERROR (Status)) { > > + return Status; > > } > > > > - DEBUG ((DEBUG_INFO, "SdCardSetBusMode: Switch to AccessMode %d > > ClockFreq %d BusWidth %d\n", AccessMode, ClockFreq, BusWidth)); > > - > > // > > - // Set to Hight Speed timing > > + // Set to High Speed timing > > // > > - if (AccessMode =3D=3D 1) { > > + if (BusMode.BusTiming =3D=3D SdMmcSdHs) { > > HostCtrl1 =3D BIT2; > > Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1, > sizeof > > (HostCtrl1), &HostCtrl1); > > if (EFI_ERROR (Status)) { > > @@ -854,12 +1139,12 @@ SdCardSetBusMode ( > > } > > } > > > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Sl= ot, > > Timing); > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Sl= ot, > > BusMode.BusTiming); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > > > - Status =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Privat= e- > > >BaseClkFreq[Slot], Private->ControllerVersion[Slot]); > > + Status =3D SdMmcHcClockSupply (PciIo, Slot, BusMode.ClockFreq * 1000= , > > Private->BaseClkFreq[Slot], Private->ControllerVersion[Slot]); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -869,7 +1154,7 @@ SdCardSetBusMode ( > > Private->ControllerHandle, > > Slot, > > EdkiiSdMmcSwitchClockFreqPost, > > - &Timing > > + &BusMode.BusTiming > > ); > > if (EFI_ERROR (Status)) { > > DEBUG (( > > @@ -882,7 +1167,7 @@ SdCardSetBusMode ( > > } > > } > > > > - if ((AccessMode =3D=3D 3) || ((AccessMode =3D=3D 2) && (Capability- > > >TuningSDR50 !=3D 0))) { > > + if ((BusMode.BusTiming =3D=3D SdMmcUhsSdr104) || ((BusMode.BusTiming > > =3D=3D SdMmcUhsSdr50) && (Capability->TuningSDR50 !=3D 0))) { > > Status =3D SdCardTuningClock (PciIo, PassThru, Slot); > > if (EFI_ERROR (Status)) { > > return Status; > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > > index 4881ee44cc..373f1bed45 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > > @@ -28,6 +28,11 @@ EFI_DRIVER_BINDING_PROTOCOL > > gSdMmcPciHcDriverBinding =3D { > > NULL > > }; > > > > +#define SLOT_INIT_TEMPLATE {0, UnknownSlot, 0, 0, 0, \ > > + {EDKII_SD_MMC_BUS_WIDTH_IGNORE,\ > > + EDKII_SD_MMC_CLOCK_FREQ_IGNORE,\ > > + {EDKII_SD_MMC_DRIVER_STRENGTH_IGNORE}}} > > + > > // > > // Template for SD/MMC host controller private data. > > // > > @@ -50,8 +55,12 @@ SD_MMC_HC_PRIVATE_DATA > gSdMmcPciHcTemplate > > =3D { > > // Queue > > INITIALIZE_LIST_HEAD_VARIABLE (gSdMmcPciHcTemplate.Queue), > > { // Slot > > - {0, UnknownSlot, 0, 0, 0}, {0, UnknownSlot, 0, 0, 0}, {0, UnknownS= lot, 0, 0, > > 0}, > > - {0, UnknownSlot, 0, 0, 0}, {0, UnknownSlot, 0, 0, 0}, {0, UnknownS= lot, 0, 0, > > 0} > > + SLOT_INIT_TEMPLATE, > > + SLOT_INIT_TEMPLATE, > > + SLOT_INIT_TEMPLATE, > > + SLOT_INIT_TEMPLATE, > > + SLOT_INIT_TEMPLATE, > > + SLOT_INIT_TEMPLATE > > }, > > { // Capability > > {0}, > > @@ -328,6 +337,7 @@ SdMmcPciHcEnumerateDevice ( > > > > return; > > } > > + > > /** > > Tests to see if this driver supports a given controller. If a child = device is > > provided, > > it further tests to see if this driver supports creating a handle fo= r the > > specified child device. > > @@ -619,7 +629,6 @@ SdMmcPciHcDriverBindingStart ( > > Support64BitDma =3D TRUE; > > for (Slot =3D FirstBar; Slot < (FirstBar + SlotNum); Slot++) { > > Private->Slot[Slot].Enable =3D TRUE; > > - > > // > > // Get SD/MMC Pci Host Controller Version > > // > > @@ -635,19 +644,34 @@ SdMmcPciHcDriverBindingStart ( > > > > Private->BaseClkFreq[Slot] =3D Private->Capability[Slot].BaseClkFr= eq; > > > > - if (mOverride !=3D NULL && mOverride->Capability !=3D NULL) { > > - Status =3D mOverride->Capability ( > > - Controller, > > - Slot, > > - &Private->Capability[Slot], > > - &Private->BaseClkFreq[Slot] > > - ); > > - if (EFI_ERROR (Status)) { > > - DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\n"= , > > - __FUNCTION__, Status)); > > - continue; > > + if (mOverride !=3D NULL) { > > + if (mOverride->Capability !=3D NULL) { > > + Status =3D mOverride->Capability ( > > + Controller, > > + Slot, > > + &Private->Capability[Slot], > > + &Private->BaseClkFreq[Slot] > > + ); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\= n", > > + __FUNCTION__, Status)); > > + continue; > > + } > > + } > > + > > + if (mOverride->NotifyPhase !=3D NULL) { > > + Status =3D mOverride->NotifyPhase ( > > + Controller, > > + Slot, > > + EdkiiSdMmcGetOperatingParam, > > + (VOID*)&Private->Slot[Slot].OperatingPar= ameters > > + ); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_WARN, "%a: Failed to get operating parameters, > > using defaults\n", __FUNCTION__)); > > + } > > } > > } > > + > > DumpCapabilityReg (Slot, &Private->Capability[Slot]); > > DEBUG (( > > DEBUG_INFO, > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > > index 77bbf83b76..c29e48767e 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > > @@ -78,11 +78,12 @@ typedef enum { > > } EFI_SD_MMC_SLOT_TYPE; > > > > typedef struct { > > - BOOLEAN Enable; > > - EFI_SD_MMC_SLOT_TYPE SlotType; > > - BOOLEAN MediaPresent; > > - BOOLEAN Initialized; > > - SD_MMC_CARD_TYPE CardType; > > + BOOLEAN Enable; > > + EFI_SD_MMC_SLOT_TYPE SlotType; > > + BOOLEAN MediaPresent; > > + BOOLEAN Initialized; > > + SD_MMC_CARD_TYPE CardType; > > + EDKII_SD_MMC_OPERATING_PARAMETERS OperatingParameters; > > } SD_MMC_HC_SLOT; > > > > typedef struct { > > @@ -120,6 +121,13 @@ typedef struct { > > UINT32 BaseClkFreq[SD_MMC_HC_MAX_SLOT]; > > } SD_MMC_HC_PRIVATE_DATA; > > > > +typedef struct { > > + SD_MMC_BUS_MODE BusTiming; > > + UINT8 BusWidth; > > + UINT32 ClockFreq; > > + EDKII_SD_MMC_DRIVER_STRENGTH DriverStrength; > > +} SD_MMC_BUS_SETTINGS; > > + > > #define SD_MMC_HC_TRB_SIG SIGNATURE_32 ('T', 'R', 'B', 'T'= ) > > > > // > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > index 5d1f977e55..75deb79ae4 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > @@ -1339,6 +1339,40 @@ SdMmcHcUhsSignaling ( > > return EFI_SUCCESS; > > } > > > > +/** > > + Set driver strength in host controller. > > + > > + @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA > > instance. >=20 >=20 > Please help to update the comments to match the function parameter. >=20 >=20 > > + @param[in] SlotIndex The slot index of the card. > > + @param[in] DriverStrength DriverStrength to set in the controller. > > + > > + @retval EFI_SUCCESS Driver strength programmed successfully. > > + @retval Others Failed to set driver strength. > > +**/ > > +EFI_STATUS > > +SdMmcSetDriverStrength ( > > + IN EFI_PCI_IO_PROTOCOL *PciIo, > > + IN UINT8 SlotIndex, > > + IN SD_DRIVER_STRENGTH_TYPE DriverStrength > > + ) > > +{ > > + EFI_STATUS Status; > > + UINT16 HostCtrl2; > > + > > + HostCtrl2 =3D (UINT16)~SD_MMC_HC_CTRL_DRIVER_STRENGTH_MASK; > > + Status =3D SdMmcHcAndMmio (PciIo, SlotIndex, > SD_MMC_HC_HOST_CTRL2, > > sizeof (HostCtrl2), &HostCtrl2); > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } > > + > > + if (DriverStrength =3D=3D SdDriverStrengthIgnore) { > > + return EFI_SUCCESS; > > + } >=20 >=20 > I suggest to put the above check at the entry of this function. >=20 >=20 > > + > > + HostCtrl2 =3D (DriverStrength << 4) & > > SD_MMC_HC_CTRL_DRIVER_STRENGTH_MASK; > > + return SdMmcHcOrMmio (PciIo, SlotIndex, SD_MMC_HC_HOST_CTRL2, > > sizeof (HostCtrl2), &HostCtrl2); > > +} > > + > > /** > > Turn on/off LED. > > > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > index 0b0d415256..bf2add1748 100644 > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > @@ -72,6 +72,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #define SD_MMC_HC_CTRL_MMC_HS200 0x0003 > > #define SD_MMC_HC_CTRL_MMC_HS400 0x0005 > > > > +#define SD_MMC_HC_CTRL_DRIVER_STRENGTH_MASK 0x0030 > > + > > // > > // The transfer modes supported by SD Host Controller > > // > > @@ -617,4 +619,21 @@ SdMmcHcUhsSignaling ( > > IN SD_MMC_BUS_MODE Timing > > ); > > > > +/** > > + Set driver strength in host controller. > > + > > + @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA > > instance. >=20 >=20 > Please help to update the comments to match the function parameter. >=20 > Best Regards, > Hao Wu >=20 >=20 > > + @param[in] SlotIndex The slot index of the card. > > + @param[in] DriverStrength DriverStrength to set in the controller. > > + > > + @retval EFI_SUCCESS Driver strength programmed successfully. > > + @retval Others Failed to set driver strength. > > +**/ > > +EFI_STATUS > > +SdMmcSetDriverStrength ( > > + IN EFI_PCI_IO_PROTOCOL *PciIo, > > + IN UINT8 SlotIndex, > > + IN SD_DRIVER_STRENGTH_TYPE DriverStrength > > + ); > > + > > #endif > > -- > > 2.14.1.windows.1