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.120, mailfrom: hao.a.wu@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Mon, 17 Jun 2019 20:23:17 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Jun 2019 20:23:16 -0700 X-ExtLoop1: 1 Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga008.fm.intel.com with ESMTP; 17 Jun 2019 20:23:16 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 17 Jun 2019 20:23:16 -0700 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 17 Jun 2019 20:23:15 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.185]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.173]) with mapi id 14.03.0439.000; Tue, 18 Jun 2019 11:23:13 +0800 From: "Wu, Hao A" To: "Albecki, Mateusz" , "devel@edk2.groups.io" CC: Ard Biesheuvel , "'mw@semihalf.com'" Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: Implement revision 3 of SdMmcOverrideProtocol Thread-Topic: [edk2-devel] [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: Implement revision 3 of SdMmcOverrideProtocol Thread-Index: AQHVGgBQSVRhuFXjG0uiZvMcFRSdRKaUMaiAgAV5/kCABlzqgIAAzLRQ Date: Tue, 18 Jun 2019 03:23:13 +0000 Message-ID: References: <20190603113346.1288-1-mateusz.albecki@intel.com> <20190603113346.1288-3-mateusz.albecki@intel.com> <15A7C8F163ADE317.25258@groups.io> <92CF190FF2351747A47C1708F0E09C0875E6902B@IRSMSX102.ger.corp.intel.com> In-Reply-To: <92CF190FF2351747A47C1708F0E09C0875E6902B@IRSMSX102.ger.corp.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 > -----Original Message----- > From: Albecki, Mateusz > Sent: Monday, June 17, 2019 11:12 PM > To: devel@edk2.groups.io; Albecki, Mateusz > Cc: Wu, Hao A > Subject: RE: [edk2-devel] [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: > Implement revision 3 of SdMmcOverrideProtocol >=20 > Actually I would like to back out of the proposed solution with mapping = SD > driver strength to eMMC driver strength. Since SD specification doesn't = even > specify enough space in driver strength field in host control 2 register= to > cover for all 5 driver strength types defined by eMMC specification and = I also > don't know about any controller that would use the mapping below. The > controller I am working on currently just ignores controller side of the= driver > strength setting altogether so for now I would like to leave the driver > strength setting for eMMC as is(check card capability and platform selec= tion) > and update only SD case. I am fine with this approach. Best Regards, Hao Wu >=20 > Thanks, > Mateusz >=20 > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Albecki, Mateusz > > Sent: Thursday, June 13, 2019 4:39 PM > > To: devel@edk2.groups.io > > Cc: Wu, Hao A > > Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: > > Implement revision 3 of SdMmcOverrideProtocol > > > > Resending to group. Inline reply to some of the comments. > > > > As for the mapping between eMMC driver strength and SD controller > driver > > strength should we use this one: > > > > SD controller -> eMMC card > > B -> Type 0 > > A -> Type 1 > > C -> Type 2 > > D -> Type 3 > > (none) -> Type4 > > > > And Type 4 would be identified by bit 39 in capability register which = is > > currently marked as reserved in SD controller specification. This is c= urrent > > capability register definition in the driver. > > > > Thanks, > > Mateusz > > > > > -----Original Message----- > > > From: Wu, Hao A > > > Sent: Monday, June 10, 2019 5:19 AM > > > To: Albecki, Mateusz > > > Subject: RE: [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: Implement > > revision > > > 3 of SdMmcOverrideProtocol > > > > > > Hello Mateusz, > > > > > > Some inline comments below: > > > > > > > -----Original Message----- > > > > From: Albecki, Mateusz > > > > Sent: Monday, June 03, 2019 7:34 PM > > > > Cc: Albecki, Mateusz; Wu, Hao A; Albecki > > > > Subject: [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: Implement > revision > > 3 > > > > of SdMmcOverrideProtocol > > > > > > > > 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 proce= ss > > > > 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 operatin= g > > > > parameters passed in GetOperatingParameters phase to choose > > preferred > > > > bus settings in GetTargetBusSettings function. Those settings are = later > > > > on to start bus training to high speeds. If user passes incompatib= le > > > > setting with selected bus timing driver will assume it's standard > behavior > > > 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 set= ting > of > > > > 4 and 1 bit won't be respected and 8 bit setting will be chosen in= stead. > > > > > > > > > > > > Cc: hao.a.wu@intel.com > > > > Signed-off-by: Albecki, Mateusz > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > --- > > > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 522 > > > > +++++++++++++++------ > > > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c | 381 > > > > ++++++++++++--- > > > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 52 +- > > > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 18 +- > > > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 8 +- > > > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 8 +- > > > > 6 files changed, 750 insertions(+), 239 deletions(-) > > > > > > > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > > > index deaf4468c9..1a81f6d250 100644 > > > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > > > @@ -449,7 +449,7 @@ EFI_STATUS > > > > EmmcSendTuningBlk ( > > > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > > > IN UINT8 Slot, > > > > - IN UINT8 BusWidth > > > > + IN UINT16 BusWidth > > > > ) > > > > { > > > > EFI_SD_MMC_COMMAND_BLOCK SdMmcCmdBlk; > > > > @@ -506,7 +506,7 @@ EmmcTuningClkForHs200 ( > > > > IN EFI_PCI_IO_PROTOCOL *PciIo, > > > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > > > IN UINT8 Slot, > > > > - IN UINT8 BusWidth > > > > + IN UINT16 BusWidth > > > > ) > > > > { > > > > EFI_STATUS Status; > > > > @@ -583,7 +583,7 @@ EmmcSwitchBusWidth ( > > > > IN UINT8 Slot, > > > > IN UINT16 Rca, > > > > IN BOOLEAN IsDdr, > > > > - IN UINT8 BusWidth > > > > + IN UINT16 BusWidth > > > > ) > > > > { > > > > EFI_STATUS Status; > > > > @@ -641,13 +641,13 @@ EmmcSwitchBusWidth ( > > > > Refer to EMMC Electrical Standard Spec 5.1 Section 6.6 and SD H= ost > > > > Controller > > > > Simplified Spec 3.0 Figure 3-3 for details. > > > > > > > > - @param[in] PciIo A pointer to the EFI_PCI_IO_PROTOCOL > instance. > > > > - @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 sen= d the > > > command > > > > to. > > > > - @param[in] Rca The relative device address to be ass= igned. > > > > - @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, th= e unit > is > > > > MHz. > > > > + @param[in] PciIo A pointer to the EFI_PCI_IO_PROTOCOL > > instance. > > > > + @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 se= nd the > > > > command to. > > > > + @param[in] Rca The relative device address to be as= signed. > > > > + @param[in] DriverStrength Driver strength to set for speed mod= es > > that > > > > support it. > > > > + @param[in] Timing The bus mode timing indicator. > > > > > > > > > Please help to update the parameter comment to reflect the name > change > > > from > > > 'Timing' to 'BusTiming'. > > > > > > > > > > + @param[in] ClockFreq The max clock frequency to be set, t= he unit > 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, Cm= dSet); > > > > 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 (( > > > > @@ -754,25 +771,34 @@ EmmcSwitchToHighSpeed ( > > > > > > > > > Please help to update the function comments for > > > EmmcSwitchToHighSpeed() after > > > the parameter changes. > > > > > > > > > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > > > IN UINT8 Slot, > > > > IN UINT16 Rca, > > > > - IN UINT32 ClockFreq, > > > > - IN BOOLEAN IsDdr, > > > > - IN UINT8 BusWidth > > > > + IN EDKII_SD_MMC_BUS_MODE *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, IsDd= r, > > > 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, IsDd= r, > > > 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 +806,27 @@ 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, PciI= o, Slot, > > > > Timing); > > > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciI= o, > Slot, > > > > BusMode->BusTiming); > > > > if (EFI_ERROR (Status)) { > > > > return Status; > > > > } > > > > > > > > - HsTiming =3D 1; > > > > - Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsT= iming, > > > 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 > > still > > > 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 > instance. > > > > - @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 sen= d the > > > command > > > > to. > > > > - @param[in] Rca The relative device address to be ass= igned. > > > > - @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 > > instance. > > > > + @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 se= nd the > > > > command to. > > > > + @param[in] Rca The relative device address to be as= signed. > > > > + @param[in] DriverStrength The driver strength to be selected. > > > > + @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. > > > > > > > > @retval EFI_SUCCESS The operation is done correctly. > > > > @retval Others The operation fails. > > > > @@ -822,30 +838,25 @@ EmmcSwitchToHS200 ( > > > > > > > > > Please help to update the function comments for EmmcSwitchToHS200() > > > after the > > > parameter changes. > > > > > > > > > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > > > IN UINT8 Slot, > > > > IN UINT16 Rca, > > > > - IN UINT32 ClockFreq, > > > > - IN UINT8 BusWidth > > > > + IN EDKII_SD_MMC_BUS_MODE *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, FALS= E, > > > BusWidth); > > > > + Status =3D EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, FALS= E, > > > > 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 +864,7 @@ EmmcSwitchToHS200 ( > > > > return Status; > > > > } > > > > > > > > - Timing =3D SdMmcMmcHs200; > > > > - > > > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciI= o, Slot, > > > > Timing); > > > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciI= o, > Slot, > > > > BusMode->BusTiming); > > > > if (EFI_ERROR (Status)) { > > > > return Status; > > > > } > > > > @@ -881,28 +890,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, HsT= iming, > > > Timing, > > > > ClockFreq); > > > > + Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, Bus= Mode- > > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq); > > > > > > > > > After the proposed change, the preferred bus timing and driver stren= gth > > > value > > > will be set for the device through the EMMC_SWITCH command. > > > > > > However, on the host controller side, only the bus timing value is a= lso > > > updated > > > in the Host Control 2 Register of the SD HC (via function > > > SdMmcHcUhsSignaling()). > > > It seems to me that the driver strength value does not get updated i= n this > > > register. > > > > > > I think the SD case has a similar issue. > > > > For SD I agree that there is an issue but for eMMC there is no clear m= apping > > between > > eMMC defined driver strength types and SD defined driver strength type= s. > > The controller > > I happen to be working with seems to ignore the values in this registe= r. If > we > > can agree on > > one mapping that will be able to service majority of the eMMC controll= ers > on > > market I don't > > mind adding that. We can maybe add driver strength setting to > > SdMmcHcUhsSignaling so that > > platform has an opportunity to correct core driver during > > EdkiiSdMmcUhsSignaling notify. > > > > > > > > > > > > if (EFI_ERROR (Status)) { > > > > return Status; > > > > } > > > > > > > > - Status =3D EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusWid= th); > > > > + Status =3D EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusMod= e- > > > > >BusWidth); > > > > > > > > return Status; > > > > } > > > > > > > > /** > > > > - Switch to the HS400 timing according to request. > > > > + Switch to the HS400 timing. This function assumes that eMMC bus= is > > still > > > 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 > instance. > > > > - @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 sen= d the > > > command > > > > to. > > > > - @param[in] Rca The relative device address to be ass= igned. > > > > - @param[in] ClockFreq The max clock frequency to be set. > > > > + @param[in] PciIo A pointer to the EFI_PCI_IO_PROTOCOL > > instance. > > > > + @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 se= nd the > > > > command to. > > > > + @param[in] Rca The relative device address to be as= signed. > > > > + @param[in] ClockFreq The max clock frequency to be set. > > > > > > > > > Please help to update the function comments for EmmcSwitchToHS400() > > > after the > > > parameter changes. > > > > > > > > > > > > > > @retval EFI_SUCCESS The operation is done correctly. > > > > @retval Others The operation fails. > > > > @@ -914,47 +922,321 @@ EmmcSwitchToHS400 ( > > > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > > > IN UINT8 Slot, > > > > IN UINT16 Rca, > > > > - IN UINT32 ClockFreq > > > > + IN EDKII_SD_MMC_BUS_MODE *BusMode > > > > ) > > > > { > > > > EFI_STATUS Status; > > > > - UINT8 HsTiming; > > > > - SD_MMC_BUS_MODE Timing; > > > > SD_MMC_HC_PRIVATE_DATA *Private; > > > > + EDKII_SD_MMC_BUS_MODE 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, Clock= Freq, 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 v= alue > less > > > than > > > > 52MHz. > > > > + // Set to High Speed timing and set the clock frequency to a va= lue less > > > than > > > > or equal to 52MHz. > > > > + // This step is necessary to be able to switch Bus into 8 bit D= DR mode > > > which > > > > is unsupported in HS200. > > > > // > > > > - HsTiming =3D 1; > > > > - Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsT= iming, > > > > SdMmcMmcHsSdr, 52); > > > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciI= o, > Slot, > > > > SdMmcMmcHsSdr); > > > > > > > > > The above SdMmcHcUhsSignaling() is newly added, is this a bug for th= e > > origin > > > code? > > > > > > I searched the eMMC spec, it seems to me the spec does not explicitl= y > > > mention > > > the Host Control 2 Register at all, I am not very sure if such chang= e will > > > bring any effect to devices. Did you have a try without adding such > change? > > > > > > > This is aligned to the behavior of EmmcSwitchToHighSpeed function. My > > eMMC > > controller doesn't need this register to be programmed for switch to h= igh > > speed > > but calling SdMmcUhsSignaling here gives override protocol a chance to > finish > > switch to high speed for eMMC controllers that need such tweaking. > > > > > > > > > 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, Bus= Mode- > > > > >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, PciI= o, Slot, > > > > Timing); > > > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciI= o, > Slot, > > > > BusMode->BusTiming); > > > > if (EFI_ERROR (Status)) { > > > > return Status; > > > > } > > > > > > > > - HsTiming =3D 3; > > > > - Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsT= iming, > > > Timing, > > > > ClockFreq); > > > > + return EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, BusMode= - > > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq); > > > > > > > > > After the proposed change, the preferred bus timing and driver stren= gth > > > value > > > will be set for the device through the EMMC_SWITCH command. > > > > > > However, on the host controller side, only the bus timing value is a= lso > > > updated > > > in the Host Control 2 Register of the SD HC (via function > > > SdMmcHcUhsSignaling()). > > > It seems to me that the driver strength value does not get updated i= n this > > > register. > > > > > > I think the SD case has a similar issue. > > > > > > > > > > +} > > > > > > > > - return Status; > > > > +/** > > > > + Check if passed BusTiming is supported in both controller and c= ard. > > > > + > > > > + @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 BusTimin= g > > > > +**/ > > > > +STATIC > > > > +BOOLEAN > > > > +IsBusTimingSupported ( > > > > > > > > > How about renaming it to 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) && (Cap= abilities- > > > > >Hs400 !=3D 0)) && Capabilities->BusWidth8) { > > > > + Supported =3D TRUE; > > > > + } > > > > + break; > > > > + case SdMmcMmcHs200: > > > > + if ((((ExtCsd->DeviceType & (BIT4 | BIT5)) !=3D 0) && (Cap= abilities- > > > > >Sdr104 !=3D 0))) { > > > > + Supported =3D TRUE; > > > > + } > > > > + break; > > > > + case SdMmcMmcHsDdr: > > > > + if ((((ExtCsd->DeviceType & (BIT2 | BIT3)) !=3D 0) && (Cap= abilities- > > > > >Ddr50 !=3D 0))) { > > > > + Supported =3D TRUE; > > > > + } > > > > + break; > > > > + case SdMmcMmcHsSdr: > > > > + if ((((ExtCsd->DeviceType & BIT1) !=3D 0) && (Capabilities= - > > >HighSpeed !=3D > > > > 0))) { > > > > + Supported =3D TRUE; > > > > + } else if ((((ExtCsd->DeviceType & BIT0) !=3D 0) && (Capab= ilities- > > > > >HighSpeed !=3D 0))) { > > > > > > > > > For this case, I think the bus timing should be classified to > > SdMmcMmcLegacy. > > > Since under this timing, the maximum frequency allowed is 26Mhz. If = set > to > > > SdMmcMmcHsSdr the driver will try to set the frequency to 52Mhz late= r > in > > > EmmcGetTargetClockFreq(). > > > > > > > > > > + Supported =3D TRUE; > > > > + } > > > > + break; > > > > + case SdMmcMmcLegacy: > > > > + 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, contro= ller > > > > + 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 > > > > +**/ > > > > +STATIC > > > > +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 = support > > and > > > > + // return as soon as we find supported timing. > > > > + // > > > > + BusTiming =3D SdMmcMmcHs400; > > > > + while (BusTiming > SdMmcMmcLegacy) { > > > > + if (IsBusTimingSupported (Private, SlotIndex, ExtCsd, BusTimi= ng)) { > > > > + break; > > > > + } > > > > + BusTiming--; > > > > + } > > > > + > > > > + return BusTiming; > > > > +} > > > > + > > > > +/** > > > > + Check if the passed bus width is supported by controller and ca= rd. > > > > + > > > > + @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 > > > > +**/ > > > > +STATIC > > > > +BOOLEAN > > > > +IsBusWidthSupported ( > > > > + 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].BusWidt= h8) { > > > > + 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 > > > > +**/ > > > > +STATIC > > > > +UINT16 > > > > +EmmcGetTargetBusWidth ( > > > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > > > + IN UINT8 SlotIndex, > > > > + IN EMMC_EXT_CSD *ExtCsd, > > > > + IN SD_MMC_BUS_MODE BusTiming > > > > + ) > > > > +{ > > > > + UINT16 BusWidth; > > > > + UINT16 PreferedBusWidth; > > > > + > > > > + PreferedBusWidth =3D Private- > > > > >Slot[SlotIndex].OperatingParameters.BusWidth; > > > > + > > > > + if (PreferedBusWidth !=3D EDKII_SD_MMC_BUS_WIDTH_IGNORE && > > > > + IsBusWidthSupported (Private, SlotIndex, BusTiming, > > > > PreferedBusWidth)) { > > > > + BusWidth =3D PreferedBusWidth; > > > > + } else if (IsBusWidthSupported (Private, SlotIndex, BusTiming, = 8)) { > > > > + BusWidth =3D 8; > > > > + } else if (IsBusWidthSupported (Private, SlotIndex, BusTiming, = 4)) { > > > > + BusWidth =3D 4; > > > > + } else if (IsBusWidthSupported (Private, SlotIndex, BusTiming, = 1)) { > > > > + BusWidth =3D 1; > > > > + } > > > > + > > > > + 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 > > > > +**/ > > > > +STATIC > > > > +UINT32 > > > > +EmmcGetTargetClockFreq ( > > > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > > > + IN UINT8 SlotIndex, > > > > + IN EMMC_EXT_CSD *ExtCsd, > > > > + IN SD_MMC_BUS_MODE BusTiming > > > > + ) > > > > +{ > > > > + UINT32 PreferedClockFreq; > > > > + UINT32 MaxClockFreq; > > > > + > > > > + PreferedClockFreq =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 (PreferedClockFreq !=3D EDKII_SD_MMC_CLOCK_FREQ_IGNORE && > > > > PreferedClockFreq < MaxClockFreq) { > > > > + return PreferedClockFreq; > > > > + } else { > > > > + return MaxClockFreq; > > > > + } > > > > +} > > > > + > > > > +/** > > > > + Get the driver strength to be set on bus > > > > > > Please help to append a '.' character for the above line. > > > It is required for the edk2 coding style. > > > > > > > > > > + > > > > + @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 > > > > +**/ > > > > +STATIC > > > > +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 PreferedDriverStrength; > > > > + EDKII_SD_MMC_DRIVER_STRENGTH DriverStrength; > > > > + > > > > + PreferedDriverStrength =3D Private- > > > > >Slot[SlotIndex].OperatingParameters.DriverStrength; > > > > + DriverStrength.Emmc =3D EmmcDriverStrengthType0; > > > > + > > > > + if (PreferedDriverStrength.Emmc !=3D > > > > EDKII_SD_MMC_DRIVER_STRENGTH_IGNORE && > > > > + (ExtCsd->DriverStrength & (BIT0 << > PreferedDriverStrength.Emmc))) > > { > > > > > > > > > The host controller also has a capability for different types of dri= ver > > strength. > > > (Capabilities Register, bit 36~38) > > > > > > The check should take it into consideration as well. > > > > For SD it should. For eMMC it's not as obvious since as I mentioned we > don't > > have a mapping defined. > > If we agree on one we can add this and for all eMMC controllers that w= on't > > follow it I guess we can > > leave it to the platform code to override capabilities. > > > > > > > > > > > > + DriverStrength.Emmc =3D PreferedDriverStrength.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 > > > > +**/ > > > > +STATIC > > > > +VOID > > > > +EmmcGetTargetBusMode ( > > > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > > > + IN UINT8 SlotIndex, > > > > + IN EMMC_EXT_CSD *ExtCsd, > > > > + OUT EDKII_SD_MMC_BUS_MODE *BusMode > > > > + ) > > > > +{ > > > > + BusMode->BusTiming =3D EmmcGetTargetBusTiming (Private, > SlotIndex, > > > > ExtCsd); > > > > + BusMode->BusWidth =3D EmmcGetTargetBusWidth (Private, SlotIndex= , > > > > ExtCsd, BusMode->BusTiming); > > > > + BusMode->ClockFreq =3D EmmcGetTargetClockFreq (Private, SlotInd= ex, > > > > ExtCsd, BusMode->BusTiming); > > > > + BusMode->DriverStrength =3D EmmcGetTargetDriverStrength (Privat= e, > > > > SlotIndex, ExtCsd, BusMode->BusTiming); > > > > } > > > > > > > > /** > > > > @@ -983,10 +1265,7 @@ EmmcSetBusMode ( > > > > EFI_STATUS Status; > > > > EMMC_CSD Csd; > > > > EMMC_EXT_CSD ExtCsd; > > > > - UINT8 HsTiming; > > > > - BOOLEAN IsDdr; > > > > - UINT32 ClockFreq; > > > > - UINT8 BusWidth; > > > > + EDKII_SD_MMC_BUS_MODE BusMode; > > > > SD_MMC_HC_PRIVATE_DATA *Private; > > > > > > > > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > > > @@ -1004,85 +1283,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) && (Pr= ivate- > > > > >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, Clo= ckFreq); > > > > - } else if (HsTiming =3D=3D 2) { > > > > - // > > > > - // Execute HS200 timing switch procedure > > > > - // > > > > - Status =3D EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, Clo= ckFreq, > > > > 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, > > ClockFreq, > > > > 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"= ), Status)); > > > > + 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..36d2b487df 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. > > > > > > > > @@ -422,7 +418,7 @@ SdCardSetBusWidth ( > > > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > > > IN UINT8 Slot, > > > > IN UINT16 Rca, > > > > - IN UINT8 BusWidth > > > > + IN UINT16 BusWidth > > > > ) > > > > { > > > > EFI_SD_MMC_COMMAND_BLOCK SdMmcCmdBlk; > > > > @@ -476,7 +472,7 @@ SdCardSetBusWidth ( > > > > @param[in] Slot The slot number of the SD card to sen= d the > > > command > > > > to. > > > > @param[in] AccessMode The value for access mode group. > > > > > > > > > Please help to update the parameter comment to reflect the function > > > interface changes. > > > > > > > > > > @param[in] CommandSystem The value for command set group. > > > > - @param[in] DriveStrength The value for drive length group. > > > > + @param[in] DriveStrength The value for driver strength group. > > > > > > > > > Please help to update the parameter name in the above comment from > > > 'DriveStrength' to 'DriverStrength'. > > > > > > > > > > @param[in] PowerLimit The value for power limit group. > > > > @param[in] Mode Switch or check function. > > > > @param[out] SwitchResp The return switch function status. > > > > @@ -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,8 +513,31 @@ SdCardSwitch ( > > > > SdMmcCmdBlk.ResponseType =3D SdMmcResponseTypeR1; > > > > > > > > ModeValue =3D Mode ? BIT31 : 0; > > > > - SdMmcCmdBlk.CommandArgument =3D (AccessMode & 0xF) | > > > ((PowerLimit > > > > & 0xF) << 4) | \ > > > > - ((DriveStrength & 0xF) << 8) | ((= DriveStrength & 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) | (= (PowerLimit & 0xF) << > > 12) > > > | > > > > \ > > > > ModeValue; > > > > > > > > Packet.InDataBuffer =3D SwitchResp; > > > > @@ -718,7 +738,7 @@ SdCardSwitchBusWidth ( > > > > IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > > > > IN UINT8 Slot, > > > > IN UINT16 Rca, > > > > - IN UINT8 BusWidth > > > > + IN UINT16 BusWidth > > > > ) > > > > { > > > > EFI_STATUS Status; > > > > @@ -748,6 +768,261 @@ SdCardSwitchBusWidth ( > > > > return Status; > > > > } > > > > > > > > +/** > > > > + Check if passed BusTiming is supported in both controller and c= ard. > > > > + > > > > + @param[in] Private Pointer to controller priva= te data > > > > + @param[in] SlotIndex Index of the slot in the co= ntroller > > > > + @param[in] CardSupportedBusTimings Bitmask indicating which bu= s > > > > timings are supported by card > > > > + @param[in] IsInUhsI Flag indicating if link is = in UHS-I > > > > + > > > > + @retval TRUE Both card and controller support given BusTiming > > > > + @retval FALSE Card or controller doesn't support given BusTimin= g > > > > +**/ > > > > +STATIC > > > > +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) { > > > > > > > > > Please help to add a 'default' handle for this 'switch' statement, t= he > > > proposed > > > code will cause a GCC5 build failure. > > > > > > > > > > + case SdMmcUhsSdr104: > > > > + if (Capability->Sdr104 && ((CardSupportedBusTimings & BIT= 3) !=3D 0)) > > { > > > > + 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; > > > > + } > > > > + } else { > > > > + switch (BusTiming) { > > > > > > > > > Please help to add a 'default' handle for this 'switch' statement, t= he > > > proposed > > > code will cause a GCC5 build failure. > > > > > > > > > > + case SdMmcSdHs: > > > > + if (Capability->HighSpeed && (CardSupportedBusTimings & > BIT1) !=3D > > 0) > > > { > > > > + return TRUE; > > > > + } > > > > + break; > > > > + case SdMmcSdDs: > > > > + if ((CardSupportedBusTimings & BIT0) !=3D 0) { > > > > + return TRUE; > > > > + } > > > > + 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, contro= ller > > > > + and the driver. > > > > + > > > > + @param[in] Private Pointer to controller priva= te data > > > > + @param[in] SlotIndex Index of the slot in the co= ntroller > > > > + @param[in] CardSupportedBusTimings Bitmask indicating which bu= s > > > > timings are supported by card > > > > + @param[in] IsInUhsI Flag indicating if link is = in UHS-I > > > > + > > > > + @return Bus timing value that should be set on link > > > > +**/ > > > > +STATIC > > > > +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--; > > > > + } > > > > > > > > > The origin logic has the below preference when selecting bus timing > > > (if supported): > > > > > > SdMmcUhsSdr104 > > > SdMmcUhsSdr50 > > > SdMmcUhsDdr50 > > > SdMmcUhsSdr25 > > > SdMmcUhsSdr12 > > > > > > So we need to reorder the types in 'SD_MMC_BUS_MODE' to: > > > > > > typedef enum { > > > SdMmcSdDs, > > > SdMmcSdHs, > > > SdMmcUhsSdr12, > > > SdMmcUhsSdr25, > > > SdMmcUhsDdr50, > > > SdMmcUhsSdr50, > > > SdMmcUhsSdr104, > > > ... > > > } SD_MMC_BUS_MODE; > > > > > > Otherwise, the Ddr50 mode will never be selected. > > > > > > > > > > + > > > > + 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 > > > > +**/ > > > > +STATIC > > > > +UINT16 > > > > +SdGetTargetBusWidth ( > > > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > > > + IN UINT8 SlotIndex, > > > > + IN SD_MMC_BUS_MODE BusTiming > > > > + ) > > > > +{ > > > > + UINT16 BusWidth; > > > > + UINT16 PreferedBusWidth; > > > > + > > > > + PreferedBusWidth =3D Private- > > > > >Slot[SlotIndex].OperatingParameters.BusWidth; > > > > + > > > > + if (BusTiming =3D=3D SdMmcSdDs || BusTiming =3D=3D SdMmcSdHs) { > > > > + if (PreferedBusWidth !=3D EDKII_SD_MMC_BUS_WIDTH_IGNORE) { > > > > + BusWidth =3D PreferedBusWidth; > > > > > > > > > The supported bus width is either 1 or 4 bit for 3.3V signal cards. > > > I think we need a more strict check here. > > > > > > > > > > + } else { > > > > + BusWidth =3D 4; > > > > + } > > > > + } else { > > > > + // > > > > + // UHS-I modes support only 4-bit width. > > > > + // Switch to 4-bit has been done before calling this function= anyway > 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 > > > > +**/ > > > > +STATIC > > > > +UINT32 > > > > +SdGetTargetBusClockFreq ( > > > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > > > + IN UINT8 SlotIndex, > > > > + IN SD_MMC_BUS_MODE BusTiming > > > > + ) > > > > +{ > > > > + UINT32 PreferedClockFreq; > > > > + UINT32 MaxClockFreq; > > > > + > > > > + PreferedClockFreq =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; > > > > > > > > > Minor comment, need an extra space for the above line. > > > > > > > > > > + } > > > > + > > > > + if (PreferedClockFreq !=3D EDKII_SD_MMC_CLOCK_FREQ_IGNORE && > > > > PreferedClockFreq < MaxClockFreq) { > > > > + return PreferedClockFreq; > > > > + } else { > > > > + return MaxClockFreq; > > > > + } > > > > +} > > > > + > > > > +/** > > > > + Get the driver strength to be set on bus > > > > > > > > > Please help to append a '.' character for the above line. > > > It is required for the edk2 coding style. > > > > > > > > > > + > > > > + @param[in] Private Pointer to controller = private data > > > > + @param[in] SlotIndex Index of the slot in t= he controller > > > > + @param[in] CardSupportedDriverStrengths Bitmask indicating whi= ch > > > > 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 > > > > +**/ > > > > +STATIC > > > > +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 > > > > + ) > > > > > > > > > One question here, the driver strength configuration is only for UHS= -I > cards. > > > Does the check on 'CardSupportedDriverStrengths' enough to ensure th= e > > > above > > > requirement or a boolean input parameter 'IsInUhsI' is needed here? > > > > > > > I think we can leave it to the code that actually sets the bus paramet= ers > > to simply skip driver strength programming for legacy and select drive= r > > strength here > > based only on HW support and platform preference. The alternative is t= o > > return > > SdDriverStrengthIgnore here and handle that in code that actually sets > driver > > strength. > > > > > > > > > +{ > > > > + EDKII_SD_MMC_DRIVER_STRENGTH PreferedDriverStrength; > > > > + EDKII_SD_MMC_DRIVER_STRENGTH DriverStrength; > > > > + > > > > + PreferedDriverStrength =3D Private- > > > > >Slot[SlotIndex].OperatingParameters.DriverStrength; > > > > + DriverStrength.Sd =3D SdDriverStrengthTypeB; > > > > + > > > > + if (PreferedDriverStrength.Sd !=3D > > > > EDKII_SD_MMC_DRIVER_STRENGTH_IGNORE && > > > > + (CardSupportedDriverStrengths & (BIT0 << > > > PreferedDriverStrength.Sd))) > > > > > > > > > The host controller also has a capability for different types of dri= ver > > strength. > > > (Capabilities Register, bit 36~38) > > > > > > The check should take it into consideration as well. > > > > > > > > > > { > > > > + DriverStrength.Sd =3D PreferedDriverStrength.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 controlle= r > > > > + @param[in] SwitchQueryResp Pointer to switch query response > > > > + @param[in] IsInUhsI Flag indicating if link is in UHS-= I mode > > > > + @param[out] BusMode Target configuration of the bus > > > > +**/ > > > > +STATIC > > > > +VOID > > > > +SdGetTargetBusMode ( > > > > + IN SD_MMC_HC_PRIVATE_DATA *Private, > > > > + IN UINT8 SlotIndex, > > > > + IN UINT8 *SwitchQueryResp, > > > > + IN BOOLEAN IsInUhsI, > > > > + OUT EDKII_SD_MMC_BUS_MODE *BusMode > > > > + ) > > > > +{ > > > > + BusMode->BusTiming =3D SdGetTargetBusTiming (Private, SlotIndex= , > > > > SwitchQueryResp[13], IsInUhsI); > > > > + BusMode->BusWidth =3D SdGetTargetBusWidth (Private, SlotIndex, > > > > BusMode->BusTiming); > > > > + BusMode->ClockFreq =3D SdGetTargetBusClockFreq (Private, SlotIn= dex, > > > > BusMode->BusTiming); > > > > + BusMode->DriverStrength =3D SdGetTargetDriverStrength (Private, > > > > SlotIndex, SwitchQueryResp[9], BusMode->BusTiming); > > > > +} > > > > + > > > > /** > > > > Switch the high speed timing according to request. > > > > > > > > @@ -775,13 +1050,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; > > > > + EDKII_SD_MMC_BUS_MODE BusMode; > > > > > > > > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > > > > > > > @@ -792,61 +1064,46 @@ SdCardSetBusMode ( > > > > return Status; > > > > } > > > > > > > > - BusWidth =3D 4; > > > > - > > > > - Status =3D SdCardSwitchBusWidth (PciIo, PassThru, Slot, Rca, Bu= sWidth); > > > > - 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 grou= p > #1. > > > > // > > > > - Status =3D SdCardSwitch (PassThru, Slot, 0xF, 0xF, 0xF, 0xF, FA= LSE, > > > > SwitchResp); > > > > + Status =3D SdCardSwitch (PassThru, Slot, 0xFF, 0xF, > > SdDriverStrengthIgnore, > > > > 0xF, FALSE, SwitchResp); > > > > if (EFI_ERROR (Status)) { > > > > return Status; > > > > } > > > > - // > > > > - // Calculate supported bus speed/bus width/clock frequency by h= ost > > and > > > > device capability. > > > > - // > > > > - ClockFreq =3D 0; > > > > - if (S18A && (Capability->Sdr104 !=3D 0) && ((SwitchResp[13] & B= IT3) !=3D > 0)) > > { > > > > - ClockFreq =3D 208; > > > > - AccessMode =3D 3; > > > > - Timing =3D SdMmcUhsSdr104; > > > > - } else if (S18A && (Capability->Sdr50 !=3D 0) && ((SwitchResp[1= 3] & > > BIT2) !=3D > > > 0)) > > > > { > > > > - ClockFreq =3D 100; > > > > - AccessMode =3D 2; > > > > - Timing =3D SdMmcUhsSdr50; > > > > - } else if (S18A && (Capability->Ddr50 !=3D 0) && ((SwitchResp[1= 3] & > > 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; > > > > + } > > > > } > > > > > > > > > Is it feasible to set the bus width for the 3.3V signaled cards & th= e UHS-I > > > cards together here? I think the SdGetTargetBusWidth() already handl= es > > the > > > UHS-I cards case well by always setting the bus width to 4. > > > > > > > So basically to skip if (!S18A) condition and do bus width switch > > unconditionally? > > I am not sure how the bus will react to additional bus width switch fo= r UHS-I > > case > > and I am not able to test it locally as I only have controller with le= gacy > support > > available for tests. > > I guess I will give it a try and we will revert it if it causes any pr= oblems. > > > > > > > > > > > > > - 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); > > > > > > > > > After the proposed change, the preferred bus timing and driver stren= gth > > > value > > > will be set for the device through the SD_SWITCH command. > > > > > > However, on the host controller side, only the bus timing value is a= lso > > > updated > > > in the Host Control 2 Register of the SD HC (via function > > > SdMmcHcUhsSignaling()). > > > It seems to me that the driver strength value does not get updated i= n this > > > register. > > > > > > I think the eMMC case has a similar issue. > > > > > > > > > > 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; > > > > - } > > > > > > > > > The above check may still be needed. Also, the whether the target dr= iver > > > strength is successfully set needs to be checked as well. > > > > > > How about adding checks in function SdCardSwitch() for mode 1 of the > > > switch > > > command? > > > > > > > > > > - > > > > - 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 +1111,12 @@ SdCardSetBusMode ( > > > > } > > > > } > > > > > > > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciI= o, Slot, > > > > Timing); > > > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciI= o, > Slot, > > > > BusMode.BusTiming); > > > > if (EFI_ERROR (Status)) { > > > > return Status; > > > > } > > > > > > > > - Status =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, P= rivate- > > > > >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 +1126,7 @@ SdCardSetBusMode ( > > > > Private->ControllerHandle, > > > > Slot, > > > > EdkiiSdMmcSwitchClockFreqPost, > > > > - &Timing > > > > + &BusMode.BusTiming > > > > ); > > > > if (EFI_ERROR (Status)) { > > > > DEBUG (( > > > > @@ -882,7 +1139,7 @@ SdCardSetBusMode ( > > > > } > > > > } > > > > > > > > - if ((AccessMode =3D=3D 3) || ((AccessMode =3D=3D 2) && (Capabil= ity- > > > > >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; > > > > @@ -1025,10 +1282,8 @@ SdCardIdentification ( > > > > // (One of support bits is set to 1: SDR50, SDR104 or DDR50 = in the > > > > // Capabilities register), switch its voltage to 1.8V. > > > > // > > > > - if ((Private->Capability[Slot].Sdr50 !=3D 0 || > > > > - Private->Capability[Slot].Sdr104 !=3D 0 || > > > > - Private->Capability[Slot].Ddr50 !=3D 0) && > > > > - ((Ocr & BIT24) !=3D 0)) { > > > > + if ((Private->Capability[Slot].Sdr50 || Private->Capability[Slo= t].Ddr50 > || > > > > Private->Capability[Slot].Sdr104) && > > > > + ((Ocr & BIT24) !=3D 0)) { > > > > > > > > > Minor comment, for: > > > Private->Capability[Slot].Sdr50 > > > Private->Capability[Slot].Ddr50 > > > Private->Capability[Slot].Sdr104 > > > > > > They are of type UINT32, should use operators '=3D=3D' or '!=3D' in = condition > > > statement. I think the origin logic is good here. > > > > > > > > > > Status =3D SdCardVoltageSwitch (PassThru, Slot); > > > > if (EFI_ERROR (Status)) { > > > > DEBUG ((DEBUG_ERROR, "SdCardIdentification: Executing > > > > SdCardVoltageSwitch fails with %r\n", Status)); > > > > diff --git > a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > > > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > > > > index 4881ee44cc..8f49954b7f 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_IGNOR= E}} > > > > > > > > > Please help to update the above definition to: > > > #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}= }} > > > > > > The proposed one will cause GCC5 build failure. > > > > > > > > > > + > > > > // > > > > // 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, Unk= nownSlot, > 0, > > 0, > > > > 0}, > > > > - {0, UnknownSlot, 0, 0, 0}, {0, UnknownSlot, 0, 0, 0}, {0, Unk= nownSlot, > 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 c= hild device is > > > > provided, > > > > it further tests to see if this driver supports creating a hand= le for 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].Base= ClkFreq; > > > > > > > > - 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].Operati= ngParameters > > > > + ); > > > > + if (EFI_ERROR (Status)) { > > > > + DEBUG ((DEBUG_WARN, "%a: Failed to get operating parame= ters, > > > > using defaults\n" __FUNCTION__)); > > > > > > > > > Please update the above line to (missed one ',' before '__FUNCTION__= '): > > > DEBUG ((DEBUG_WARN, "%a: Failed to get operating parameters, using > > > defaults\n", __FUNCTION__)); > > > > > > It will cause GCC5 build failure for the proposed change. > > > > > > > > > > + } > > > > } > > > > } > > > > + > > > > 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..b768f3942f 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_S= LOT]; > > > > } SD_MMC_HC_PRIVATE_DATA; > > > > > > > > +typedef struct { > > > > + EDKII_SD_MMC_BUS_TIMING BusTiming; > > > > > > > > > Should be: > > > SD_MMC_BUS_MODE BusTiming; > > > > > > > > > > + UINT16 BusWidth; > > > > + UINT32 ClockFreq; > > > > + EDKII_SD_MMC_DRIVER_STRENGTH DriverStrength; > > > > +} EDKII_SD_MMC_BUS_MODE; > > > > > > > > > Since the above structure is a driver internal one, I prefer droppin= g the > > > prefix 'EDKII_'. But unfortunately, SD_MMC_BUS_MODE is already used > in > > > Protocol/SdMmcOverride.h, so how about 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..863ae3db34 100644 > > > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > > > > @@ -1263,10 +1263,10 @@ SdMmcHcInitHost ( > > > > **/ > > > > EFI_STATUS > > > > SdMmcHcUhsSignaling ( > > > > - IN EFI_HANDLE ControllerHandle, > > > > - IN EFI_PCI_IO_PROTOCOL *PciIo, > > > > - IN UINT8 Slot, > > > > - IN SD_MMC_BUS_MODE Timing > > > > + IN EFI_HANDLE ControllerHandle, > > > > + IN EFI_PCI_IO_PROTOCOL *PciIo, > > > > + IN UINT8 Slot, > > > > + IN SD_MMC_BUS_MODE Timing > > > > ) > > > > > > > > > I think the changes made here (space indentation) can be dropped. > > > > > > > > > > { > > > > EFI_STATUS Status; > > > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > > > index 0b0d415256..c4d4aef73a 100644 > > > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > > > > @@ -611,10 +611,10 @@ SdMmcHcInitTimeoutCtrl ( > > > > **/ > > > > EFI_STATUS > > > > SdMmcHcUhsSignaling ( > > > > - IN EFI_HANDLE ControllerHandle, > > > > - IN EFI_PCI_IO_PROTOCOL *PciIo, > > > > - IN UINT8 Slot, > > > > - IN SD_MMC_BUS_MODE Timing > > > > + IN EFI_HANDLE ControllerHandle, > > > > + IN EFI_PCI_IO_PROTOCOL *PciIo, > > > > + IN UINT8 Slot, > > > > + IN SD_MMC_BUS_MODE Timing > > > > > > > > > I think the changes made here (space indentation) can be dropped. > > > > > > Best Regards, > > > Hao Wu > > > > > > > > > > ); > > > > > > > > #endif > > > > -- > > > > 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 rec= ipient, > > please contact the sender and delete all copies; any review or distrib= ution > by > > others is strictly prohibited. > > > > > >=20