public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Albecki, Mateusz" <mateusz.albecki@intel.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	"'mw@semihalf.com'" <mw@semihalf.com>
Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: Implement revision 3 of SdMmcOverrideProtocol
Date: Tue, 18 Jun 2019 03:23:13 +0000	[thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C8F22D5@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <92CF190FF2351747A47C1708F0E09C0875E6902B@IRSMSX102.ger.corp.intel.com>

> -----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
> 
> 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 selection)
> and update only SD case.


I am fine with this approach.

Best Regards,
Hao Wu


> 
> Thanks,
> Mateusz
> 
> > -----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 <hao.a.wu@intel.com>
> > 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 current
> > 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 <mateusz.albecki@intel.com>
> > > 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 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
> > preferred
> > > > 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
> behavior
> > > with
> > > > respect to that setting. For instance if HS400 has been selected as 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 chosen instead.
> > > >
> > > >
> > > > Cc: hao.a.wu@intel.com
> > > > Signed-off-by: Albecki, Mateusz <mateusz.albecki@intel.com>
> > > > 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 Host
> > > > 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 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 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 send the
> > > > command to.
> > > > +  @param[in] Rca             The relative device address to be assigned.
> > > > +  @param[in] DriverStrength  Driver strength to set for speed modes
> > 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, the 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 = 0x03;
> > > >    Index  = OFFSET_OF (EMMC_EXT_CSD, HsTiming);
> > > > -  Value  = HsTiming;
> > > >    CmdSet = 0;
> > > > +  switch (BusTiming) {
> > > > +    case SdMmcMmcHs400:
> > > > +      Value = (UINT8)((DriverStrength.Emmc << 4) | 3);
> > > > +      break;
> > > > +    case SdMmcMmcHs200:
> > > > +      Value = (UINT8)((DriverStrength.Emmc << 4) | 2);
> > > > +      break;
> > > > +    case SdMmcMmcHsSdr:
> > > > +    case SdMmcMmcHsDdr:
> > > > +      Value = 1;
> > > > +      break;
> > > > +    case SdMmcMmcLegacy:
> > > > +      Value = 0;
> > > > +      break;
> > > > +    default:
> > > > +      DEBUG ((DEBUG_ERROR, "EmmcSwitchBusTiming: Unsupported
> > > > BusTiming(%d\n)", BusTiming));
> > > > +      return EFI_INVALID_PARAMETER;
> > > > +  }
> > > >
> > > >    Status = 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 ((
> > > > @@ -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 = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> > > >
> > > > -  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr,
> > > BusWidth);
> > > > +  if ((BusMode->BusTiming != SdMmcMmcHsSdr && BusMode-
> > > > >BusTiming != SdMmcMmcHsDdr) ||
> > > > +      BusMode->ClockFreq > 52) {
> > > > +    return EFI_INVALID_PARAMETER;
> > > > +  }
> > > > +
> > > > +  if (BusMode->BusTiming == SdMmcMmcHsDdr) {
> > > > +    IsDdr = TRUE;
> > > > +  } else {
> > > > +    IsDdr = FALSE;
> > > > +  }
> > > > +
> > > > +  Status = 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 = BIT2;
> > > >    Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1,
> > sizeof
> > > > (HostCtrl1), &HostCtrl1);
> > > > @@ -780,37 +806,27 @@ EmmcSwitchToHighSpeed (
> > > >      return Status;
> > > >    }
> > > >
> > > > -  if (IsDdr) {
> > > > -    Timing = SdMmcMmcHsDdr;
> > > > -  } else if (ClockFreq == 52) {
> > > > -    Timing = SdMmcMmcHsSdr;
> > > > -  } else {
> > > > -    Timing = SdMmcMmcLegacy;
> > > > -  }
> > > > -
> > > > -  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot,
> > > > Timing);
> > > > +  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> Slot,
> > > > BusMode->BusTiming);
> > > >    if (EFI_ERROR (Status)) {
> > > >      return Status;
> > > >    }
> > > >
> > > > -  HsTiming = 1;
> > > > -  Status = 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
> > 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 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
> > 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 send the
> > > > command to.
> > > > +  @param[in] Rca             The relative device address to be assigned.
> > > > +  @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 = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> > > >
> > > > -  if ((BusWidth != 4) && (BusWidth != 8)) {
> > > > +  if (BusMode->BusTiming != SdMmcMmcHs200 ||
> > > > +      (BusMode->BusWidth != 4 && BusMode->BusWidth != 8)) {
> > > >      return EFI_INVALID_PARAMETER;
> > > >    }
> > > >
> > > > -  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, FALSE,
> > > BusWidth);
> > > > +  Status = 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 = SdMmcHcStopClock (PciIo, Slot);
> > > > @@ -853,9 +864,7 @@ EmmcSwitchToHS200 (
> > > >      return Status;
> > > >    }
> > > >
> > > > -  Timing = SdMmcMmcHs200;
> > > > -
> > > > -  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot,
> > > > Timing);
> > > > +  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> Slot,
> > > > BusMode->BusTiming);
> > > >    if (EFI_ERROR (Status)) {
> > > >      return Status;
> > > >    }
> > > > @@ -881,28 +890,27 @@ EmmcSwitchToHS200 (
> > > >    ClockCtrl = BIT2;
> > > >    Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL,
> > sizeof
> > > > (ClockCtrl), &ClockCtrl);
> > > >
> > > > -  HsTiming = 2;
> > > > -  Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming,
> > > Timing,
> > > > ClockFreq);
> > > > +  Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, BusMode-
> > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq);
> > >
> > >
> > > After the proposed change, the preferred bus timing and driver strength
> > > value
> > > will be set for the device through the EMMC_SWITCH command.
> > >
> > > However, on the host controller side, only the bus timing value is also
> > > 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 in 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 mapping
> > between
> > eMMC defined driver strength types and SD defined driver strength types.
> > The controller
> > I happen to be working with seems to ignore the values in this register. If
> we
> > can agree on
> > one mapping that will be able to service majority of the eMMC controllers
> 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 = EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusWidth);
> > > > +  Status = 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
> > 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 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
> > 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 send the
> > > > command to.
> > > > +  @param[in] Rca             The relative device address to be assigned.
> > > > +  @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 != SdMmcMmcHs400 ||
> > > > +      BusMode->BusWidth != 8) {
> > > > +    return EFI_INVALID_PARAMETER;
> > > > +  }
> > > >
> > > >    Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> > > > +  Hs200BusMode.BusTiming = SdMmcMmcHs200;
> > > > +  Hs200BusMode.BusWidth = BusMode->BusWidth;
> > > > +  Hs200BusMode.ClockFreq = BusMode->ClockFreq;
> > > > +  Hs200BusMode.DriverStrength = BusMode->DriverStrength;
> > > >
> > > > -  Status = EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFreq, 8);
> > > > +  Status = 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 less
> > > than
> > > > or equal to 52MHz.
> > > > +  // This step is necessary to be able to switch Bus into 8 bit DDR mode
> > > which
> > > > is unsupported in HS200.
> > > >    //
> > > > -  HsTiming = 1;
> > > > -  Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming,
> > > > SdMmcMmcHsSdr, 52);
> > > > +  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> Slot,
> > > > SdMmcMmcHsSdr);
> > >
> > >
> > > The above SdMmcHcUhsSignaling() is newly added, is this a bug for the
> > origin
> > > code?
> > >
> > > I searched the eMMC spec, it seems to me the spec does not explicitly
> > > mention
> > > the Host Control 2 Register at all, I am not very sure if such change 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 high
> > 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 = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, TRUE, 8);
> > > > +
> > > > +  HsFreq = BusMode->ClockFreq < 52 ? BusMode->ClockFreq : 52;
> > > > +  Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, BusMode-
> > > > >DriverStrength, SdMmcMmcHsSdr, HsFreq);
> > > >    if (EFI_ERROR (Status)) {
> > > >      return Status;
> > > >    }
> > > >
> > > > -  Timing = SdMmcMmcHs400;
> > > > +  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, TRUE,
> > > BusMode-
> > > > >BusWidth);
> > > > +  if (EFI_ERROR (Status)) {
> > > > +    return Status;
> > > > +  }
> > > >
> > > > -  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot,
> > > > Timing);
> > > > +  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> Slot,
> > > > BusMode->BusTiming);
> > > >    if (EFI_ERROR (Status)) {
> > > >      return Status;
> > > >    }
> > > >
> > > > -  HsTiming = 3;
> > > > -  Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, HsTiming,
> > > Timing,
> > > > ClockFreq);
> > > > +  return EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, BusMode-
> > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq);
> > >
> > >
> > > After the proposed change, the preferred bus timing and driver strength
> > > value
> > > will be set for the device through the EMMC_SWITCH command.
> > >
> > > However, on the host controller side, only the bus timing value is also
> > > 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 in this
> > > register.
> > >
> > > I think the SD case has a similar issue.
> > >
> > >
> > > > +}
> > > >
> > > > -  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
> > > > +**/
> > > > +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 = &Private->Capability[SlotIndex];
> > > > +
> > > > +  Supported = FALSE;
> > > > +  switch (BusTiming) {
> > > > +    case SdMmcMmcHs400:
> > > > +      if ((((ExtCsd->DeviceType & (BIT6 | BIT7))  != 0) && (Capabilities-
> > > > >Hs400 != 0)) && Capabilities->BusWidth8) {
> > > > +        Supported = TRUE;
> > > > +      }
> > > > +      break;
> > > > +    case SdMmcMmcHs200:
> > > > +      if ((((ExtCsd->DeviceType & (BIT4 | BIT5))  != 0) && (Capabilities-
> > > > >Sdr104 != 0))) {
> > > > +        Supported = TRUE;
> > > > +      }
> > > > +      break;
> > > > +    case SdMmcMmcHsDdr:
> > > > +      if ((((ExtCsd->DeviceType & (BIT2 | BIT3))  != 0) && (Capabilities-
> > > > >Ddr50 != 0))) {
> > > > +        Supported = TRUE;
> > > > +      }
> > > > +      break;
> > > > +    case SdMmcMmcHsSdr:
> > > > +      if ((((ExtCsd->DeviceType & BIT1)  != 0) && (Capabilities-
> > >HighSpeed !=
> > > > 0))) {
> > > > +        Supported = TRUE;
> > > > +      } else if ((((ExtCsd->DeviceType & BIT0)  != 0) && (Capabilities-
> > > > >HighSpeed != 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 later
> in
> > > EmmcGetTargetClockFreq().
> > >
> > >
> > > > +        Supported = TRUE;
> > > > +      }
> > > > +      break;
> > > > +    case SdMmcMmcLegacy:
> > > > +      Supported = 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
> > > > +**/
> > > > +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 = SdMmcMmcHs400;
> > > > +  while (BusTiming > SdMmcMmcLegacy) {
> > > > +    if (IsBusTimingSupported (Private, SlotIndex, ExtCsd, BusTiming)) {
> > > > +      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
> > > > +**/
> > > > +STATIC
> > > > +BOOLEAN
> > > > +IsBusWidthSupported (
> > > > +  IN SD_MMC_HC_PRIVATE_DATA   *Private,
> > > > +  IN UINT8                    SlotIndex,
> > > > +  IN SD_MMC_BUS_MODE          BusTiming,
> > > > +  IN UINT16                   BusWidth
> > > > +  )
> > > > +{
> > > > +  if (BusWidth == 8 && Private->Capability[SlotIndex].BusWidth8) {
> > > > +    return TRUE;
> > > > +  } else if (BusWidth == 4 && BusTiming != SdMmcMmcHs400) {
> > > > +    return TRUE;
> > > > +  } else if (BusWidth == 1 && (BusTiming == SdMmcMmcHsSdr ||
> > > BusTiming
> > > > == 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 = Private-
> > > > >Slot[SlotIndex].OperatingParameters.BusWidth;
> > > > +
> > > > +  if (PreferedBusWidth != EDKII_SD_MMC_BUS_WIDTH_IGNORE &&
> > > > +      IsBusWidthSupported (Private, SlotIndex, BusTiming,
> > > > PreferedBusWidth)) {
> > > > +    BusWidth = PreferedBusWidth;
> > > > +  } else if (IsBusWidthSupported (Private, SlotIndex, BusTiming, 8)) {
> > > > +    BusWidth = 8;
> > > > +  } else if (IsBusWidthSupported (Private, SlotIndex, BusTiming, 4)) {
> > > > +    BusWidth = 4;
> > > > +  } else if (IsBusWidthSupported (Private, SlotIndex, BusTiming, 1)) {
> > > > +    BusWidth = 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 = Private-
> > > > >Slot[SlotIndex].OperatingParameters.ClockFreq;
> > > > +
> > > > +  switch (BusTiming) {
> > > > +    case SdMmcMmcHs400:
> > > > +    case SdMmcMmcHs200:
> > > > +      MaxClockFreq = 200;
> > > > +      break;
> > > > +    case SdMmcMmcHsSdr:
> > > > +    case SdMmcMmcHsDdr:
> > > > +      MaxClockFreq = 52;
> > > > +      break;
> > > > +    default:
> > > > +      MaxClockFreq = 26;
> > > > +      break;
> > > > +  }
> > > > +
> > > > +  if (PreferedClockFreq != 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 = Private-
> > > > >Slot[SlotIndex].OperatingParameters.DriverStrength;
> > > > +  DriverStrength.Emmc = EmmcDriverStrengthType0;
> > > > +
> > > > +  if (PreferedDriverStrength.Emmc !=
> > > > EDKII_SD_MMC_DRIVER_STRENGTH_IGNORE &&
> > > > +      (ExtCsd->DriverStrength & (BIT0 <<
> PreferedDriverStrength.Emmc)))
> > {
> > >
> > >
> > > The host controller also has a capability for different types of driver
> > 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 won't
> > follow it I guess we can
> > leave it to the platform code to override capabilities.
> >
> > >
> > >
> > > > +    DriverStrength.Emmc = 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 = EmmcGetTargetBusTiming (Private,
> SlotIndex,
> > > > ExtCsd);
> > > > +  BusMode->BusWidth = EmmcGetTargetBusWidth (Private, SlotIndex,
> > > > ExtCsd, BusMode->BusTiming);
> > > > +  BusMode->ClockFreq = EmmcGetTargetClockFreq (Private, SlotIndex,
> > > > ExtCsd, BusMode->BusTiming);
> > > > +  BusMode->DriverStrength = EmmcGetTargetDriverStrength (Private,
> > > > 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 = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> > > > @@ -1004,85 +1283,30 @@ EmmcSetBusMode (
> > > >    }
> > > >
> > > >    ASSERT (Private->BaseClkFreq[Slot] != 0);
> > > > +
> > > >    //
> > > > -  // Check if the Host Controller support 8bits bus width.
> > > > -  //
> > > > -  if (Private->Capability[Slot].BusWidth8 != 0) {
> > > > -    BusWidth = 8;
> > > > -  } else {
> > > > -    BusWidth = 4;
> > > > -  }
> > > > -  //
> > > > -  // Get Deivce_Type from EXT_CSD register.
> > > > +  // Get Device_Type from EXT_CSD register.
> > > >    //
> > > >    Status = 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  = 0;
> > > > -  IsDdr     = FALSE;
> > > > -  ClockFreq = 0;
> > > > -  if (((ExtCsd.DeviceType & (BIT4 | BIT5))  != 0) && (Private-
> > > > >Capability[Slot].Sdr104 != 0)) {
> > > > -    HsTiming  = 2;
> > > > -    IsDdr     = FALSE;
> > > > -    ClockFreq = 200;
> > > > -  } else if (((ExtCsd.DeviceType & (BIT2 | BIT3))  != 0) && (Private-
> > > > >Capability[Slot].Ddr50 != 0)) {
> > > > -    HsTiming  = 1;
> > > > -    IsDdr     = TRUE;
> > > > -    ClockFreq = 52;
> > > > -  } else if (((ExtCsd.DeviceType & BIT1)  != 0) && (Private-
> > > > >Capability[Slot].HighSpeed != 0)) {
> > > > -    HsTiming  = 1;
> > > > -    IsDdr     = FALSE;
> > > > -    ClockFreq = 52;
> > > > -  } else if (((ExtCsd.DeviceType & BIT0)  != 0) && (Private-
> > > > >Capability[Slot].HighSpeed != 0)) {
> > > > -    HsTiming  = 1;
> > > > -    IsDdr     = FALSE;
> > > > -    ClockFreq = 26;
> > > > -  }
> > > > -  //
> > > > -  // Check if both of the device and the host controller support HS400
> > DDR
> > > > mode.
> > > > -  //
> > > > -  if (((ExtCsd.DeviceType & (BIT6 | BIT7))  != 0) && (Private-
> > > > >Capability[Slot].Hs400 != 0)) {
> > > > -    //
> > > > -    // The host controller supports 8bits bus.
> > > > -    //
> > > > -    ASSERT (BusWidth == 8);
> > > > -    HsTiming  = 3;
> > > > -    IsDdr     = TRUE;
> > > > -    ClockFreq = 200;
> > > > -  }
> > > >
> > > > -  if ((ClockFreq == 0) || (HsTiming == 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
> > > > = %d, width = %d, clock freq = %d, driver strength = %d\n",
> > > > +                          BusMode.BusTiming, BusMode.BusWidth,
> > > BusMode.ClockFreq,
> > > > BusMode.DriverStrength.Emmc));
> > > >
> > > > -  if (HsTiming == 3) {
> > > > -    //
> > > > -    // Execute HS400 timing switch procedure
> > > > -    //
> > > > -    Status = EmmcSwitchToHS400 (PciIo, PassThru, Slot, Rca, ClockFreq);
> > > > -  } else if (HsTiming == 2) {
> > > > -    //
> > > > -    // Execute HS200 timing switch procedure
> > > > -    //
> > > > -    Status = EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFreq,
> > > > BusWidth);
> > > > +  if (BusMode.BusTiming == SdMmcMmcHs400) {
> > > > +    Status = EmmcSwitchToHS400 (PciIo, PassThru, Slot, Rca,
> &BusMode);
> > > > +  } else if (BusMode.BusTiming == SdMmcMmcHs200) {
> > > > +    Status = EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca,
> &BusMode);
> > > >    } else {
> > > > -    //
> > > > -    // Execute High Speed timing switch procedure
> > > > -    //
> > > > -    Status = EmmcSwitchToHighSpeed (PciIo, PassThru, Slot, Rca,
> > ClockFreq,
> > > > IsDdr, BusWidth);
> > > > +    Status = EmmcSwitchToHighSpeed (PciIo, PassThru, Slot, Rca,
> > > &BusMode);
> > > >    }
> > > >
> > > > -  DEBUG ((DEBUG_INFO, "EmmcSetBusMode: Switch to %a %r\n",
> > > (HsTiming
> > > > == 3) ? "HS400" : ((HsTiming == 2) ? "HS200" : "HighSpeed"), Status));
> > > > +  DEBUG ((DEBUG_INFO, "EmmcSetBusMode: Switch to %a %r\n",
> > > > (BusMode.BusTiming == SdMmcMmcHs400) ? "HS400" :
> > > > ((BusMode.BusTiming == 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 send 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 = SdMmcResponseTypeR1;
> > > >
> > > >    ModeValue = Mode ? BIT31 : 0;
> > > > -  SdMmcCmdBlk.CommandArgument = (AccessMode & 0xF) |
> > > ((PowerLimit
> > > > & 0xF) << 4) | \
> > > > -                                ((DriveStrength & 0xF) << 8) | ((DriveStrength & 0xF)
> <<
> > > 12)
> > > > | \
> > > > +
> > > > +  switch (BusTiming) {
> > > > +    case SdMmcUhsDdr50:
> > > > +      AccessMode = 0x4;
> > > > +      break;
> > > > +    case SdMmcUhsSdr104:
> > > > +      AccessMode = 0x3;
> > > > +      break;
> > > > +    case SdMmcUhsSdr50:
> > > > +      AccessMode = 0x2;
> > > > +      break;
> > > > +    case SdMmcUhsSdr25:
> > > > +    case SdMmcSdHs:
> > > > +      AccessMode = 0x1;
> > > > +      break;
> > > > +    case SdMmcUhsSdr12:
> > > > +    case SdMmcSdDs:
> > > > +      AccessMode = 0;
> > > > +      break;
> > > > +    default:
> > > > +      AccessMode = 0xF;
> > > > +  }
> > > > +
> > > > +  SdMmcCmdBlk.CommandArgument = (AccessMode & 0xF) |
> > > > ((CommandSystem & 0xF) << 4) | \
> > > > +                                ((DriverStrength & 0xF) << 8) | ((PowerLimit & 0xF) <<
> > 12)
> > > |
> > > > \
> > > >                                  ModeValue;
> > > >
> > > >    Packet.InDataBuffer     = 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 card.
> > > > +
> > > > +  @param[in] Private                  Pointer to controller private data
> > > > +  @param[in] SlotIndex                Index of the slot in the controller
> > > > +  @param[in] CardSupportedBusTimings  Bitmask indicating which bus
> > > > 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 BusTiming
> > > > +**/
> > > > +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 = &Private->Capability[SlotIndex];
> > > > +
> > > > +  if (IsInUhsI) {
> > > > +    switch (BusTiming) {
> > >
> > >
> > > Please help to add a 'default' handle for this 'switch' statement, the
> > > proposed
> > > code will cause a GCC5 build failure.
> > >
> > >
> > > > +      case SdMmcUhsSdr104:
> > > > +        if (Capability->Sdr104 && ((CardSupportedBusTimings & BIT3) != 0))
> > {
> > > > +          return TRUE;
> > > > +        }
> > > > +        break;
> > > > +      case SdMmcUhsDdr50:
> > > > +        if (Capability->Ddr50 && ((CardSupportedBusTimings & BIT4) != 0))
> {
> > > > +          return TRUE;
> > > > +        }
> > > > +        break;
> > > > +      case SdMmcUhsSdr50:
> > > > +        if (Capability->Sdr50 && ((CardSupportedBusTimings & BIT2) != 0))
> {
> > > > +          return TRUE;
> > > > +        }
> > > > +        break;
> > > > +      case SdMmcUhsSdr25:
> > > > +        if ((CardSupportedBusTimings & BIT1) != 0) {
> > > > +          return TRUE;
> > > > +        }
> > > > +        break;
> > > > +      case SdMmcUhsSdr12:
> > > > +        if ((CardSupportedBusTimings & BIT0) != 0) {
> > > > +          return TRUE;
> > > > +        }
> > > > +        break;
> > > > +    }
> > > > +  } else {
> > > > +    switch (BusTiming) {
> > >
> > >
> > > Please help to add a 'default' handle for this 'switch' statement, the
> > > proposed
> > > code will cause a GCC5 build failure.
> > >
> > >
> > > > +      case SdMmcSdHs:
> > > > +        if (Capability->HighSpeed && (CardSupportedBusTimings &
> BIT1) !=
> > 0)
> > > {
> > > > +          return TRUE;
> > > > +        }
> > > > +        break;
> > > > +      case SdMmcSdDs:
> > > > +        if ((CardSupportedBusTimings & BIT0) != 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, controller
> > > > +  and the driver.
> > > > +
> > > > +  @param[in] Private                  Pointer to controller private data
> > > > +  @param[in] SlotIndex                Index of the slot in the controller
> > > > +  @param[in] CardSupportedBusTimings  Bitmask indicating which bus
> > > > 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 = SdMmcUhsSdr104;
> > > > +  } else {
> > > > +    BusTiming = 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 = Private-
> > > > >Slot[SlotIndex].OperatingParameters.BusWidth;
> > > > +
> > > > +  if (BusTiming == SdMmcSdDs || BusTiming == SdMmcSdHs) {
> > > > +    if (PreferedBusWidth != EDKII_SD_MMC_BUS_WIDTH_IGNORE) {
> > > > +      BusWidth = 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 = 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 = 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 = Private-
> > > > >Slot[SlotIndex].OperatingParameters.ClockFreq;
> > > > +
> > > > +  switch (BusTiming) {
> > > > +    case SdMmcUhsSdr104:
> > > > +      MaxClockFreq = 208;
> > > > +      break;
> > > > +    case SdMmcUhsSdr50:
> > > > +      MaxClockFreq = 100;
> > > > +      break;
> > > > +    case SdMmcUhsDdr50:
> > > > +    case SdMmcUhsSdr25:
> > > > +    case SdMmcSdHs:
> > > > +      MaxClockFreq = 50;
> > > > +      break;
> > > > +    case SdMmcUhsSdr12:
> > > > +    case SdMmcSdDs:
> > > > +    default:
> > > > +     MaxClockFreq = 25;
> > >
> > >
> > > Minor comment, need an extra space for the above line.
> > >
> > >
> > > > +  }
> > > > +
> > > > +  if (PreferedClockFreq != 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] 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
> > > > +**/
> > > > +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 the
> > > 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 parameters
> > to simply skip driver strength programming for legacy and select driver
> > strength here
> > based only on HW support and platform preference. The alternative is to
> > 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 = Private-
> > > > >Slot[SlotIndex].OperatingParameters.DriverStrength;
> > > > +  DriverStrength.Sd = SdDriverStrengthTypeB;
> > > > +
> > > > +  if (PreferedDriverStrength.Sd !=
> > > > EDKII_SD_MMC_DRIVER_STRENGTH_IGNORE &&
> > > > +      (CardSupportedDriverStrengths & (BIT0 <<
> > > PreferedDriverStrength.Sd)))
> > >
> > >
> > > The host controller also has a capability for different types of driver
> > strength.
> > > (Capabilities Register, bit 36~38)
> > >
> > > The check should take it into consideration as well.
> > >
> > >
> > > > {
> > > > +    DriverStrength.Sd = 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 controller
> > > > +  @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 = SdGetTargetBusTiming (Private, SlotIndex,
> > > > SwitchQueryResp[13], IsInUhsI);
> > > > +  BusMode->BusWidth = SdGetTargetBusWidth (Private, SlotIndex,
> > > > BusMode->BusTiming);
> > > > +  BusMode->ClockFreq = SdGetTargetBusClockFreq (Private, SlotIndex,
> > > > BusMode->BusTiming);
> > > > +  BusMode->DriverStrength = 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 = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> > > >
> > > > @@ -792,61 +1064,46 @@ SdCardSetBusMode (
> > > >      return Status;
> > > >    }
> > > >
> > > > -  BusWidth = 4;
> > > > -
> > > > -  Status = SdCardSwitchBusWidth (PciIo, PassThru, Slot, Rca, BusWidth);
> > > > -  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 = 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 = SdCardSwitch (PassThru, Slot, 0xF, 0xF, 0xF, 0xF, FALSE,
> > > > SwitchResp);
> > > > +  Status = SdCardSwitch (PassThru, Slot, 0xFF, 0xF,
> > SdDriverStrengthIgnore,
> > > > 0xF, FALSE, SwitchResp);
> > > >    if (EFI_ERROR (Status)) {
> > > >      return Status;
> > > >    }
> > > > -  //
> > > > -  // Calculate supported bus speed/bus width/clock frequency by host
> > and
> > > > device capability.
> > > > -  //
> > > > -  ClockFreq = 0;
> > > > -  if (S18A && (Capability->Sdr104 != 0) && ((SwitchResp[13] & BIT3) !=
> 0))
> > {
> > > > -    ClockFreq = 208;
> > > > -    AccessMode = 3;
> > > > -    Timing = SdMmcUhsSdr104;
> > > > -  } else if (S18A && (Capability->Sdr50 != 0) && ((SwitchResp[13] &
> > BIT2) !=
> > > 0))
> > > > {
> > > > -    ClockFreq = 100;
> > > > -    AccessMode = 2;
> > > > -    Timing = SdMmcUhsSdr50;
> > > > -  } else if (S18A && (Capability->Ddr50 != 0) && ((SwitchResp[13] &
> > BIT4) !=
> > > > 0)) {
> > > > -    ClockFreq = 50;
> > > > -    AccessMode = 4;
> > > > -    Timing = SdMmcUhsDdr50;
> > > > -  } else if ((SwitchResp[13] & BIT1) != 0) {
> > > > -    ClockFreq = 50;
> > > > -    AccessMode = 1;
> > > > -    Timing = SdMmcUhsSdr25;
> > > > -  } else {
> > > > -    ClockFreq = 25;
> > > > -    AccessMode = 0;
> > > > -    Timing = SdMmcUhsSdr12;
> > > > +
> > > > +  SdGetTargetBusMode (Private, Slot, SwitchResp, S18A, &BusMode);
> > > > +
> > > > +  DEBUG ((DEBUG_INFO, "SdCardSetBusMode: Target bus mode: bus
> > > timing
> > > > = %d, bus width = %d, clock freq[MHz] = %d, driver strength = %d\n",
> > > > +                         BusMode.BusTiming, BusMode.BusWidth,
> > > BusMode.ClockFreq,
> > > > BusMode.DriverStrength.Sd));
> > > > +
> > > > +  if (!S18A) {
> > > > +    Status = 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 & the UHS-I
> > > cards together here? I think the SdGetTargetBusWidth() already handles
> > 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 for UHS-I
> > case
> > and I am not able to test it locally as I only have controller with legacy
> support
> > available for tests.
> > I guess I will give it a try and we will revert it if it causes any problems.
> >
> > >
> > > >
> > > > -  Status = SdCardSwitch (PassThru, Slot, AccessMode, 0xF, 0xF, 0xF,
> TRUE,
> > > > SwitchResp);
> > > > +  Status = SdCardSwitch (PassThru, Slot, BusMode.BusTiming, 0xF,
> > > > BusMode.DriverStrength.Sd, 0xF, TRUE, SwitchResp);
> > >
> > >
> > > After the proposed change, the preferred bus timing and driver strength
> > > value
> > > will be set for the device through the SD_SWITCH command.
> > >
> > > However, on the host controller side, only the bus timing value is also
> > > 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 in this
> > > register.
> > >
> > > I think the eMMC case has a similar issue.
> > >
> > >
> > > >    if (EFI_ERROR (Status)) {
> > > >      return Status;
> > > >    }
> > > >
> > > > -  if ((SwitchResp[16] & 0xF) != 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 driver
> > > 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 == 1) {
> > > > +  if (BusMode.BusTiming == SdMmcSdHs) {
> > > >      HostCtrl1 = BIT2;
> > > >      Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1,
> > > sizeof
> > > > (HostCtrl1), &HostCtrl1);
> > > >      if (EFI_ERROR (Status)) {
> > > > @@ -854,12 +1111,12 @@ SdCardSetBusMode (
> > > >      }
> > > >    }
> > > >
> > > > -  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, Slot,
> > > > Timing);
> > > > +  Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> Slot,
> > > > BusMode.BusTiming);
> > > >    if (EFI_ERROR (Status)) {
> > > >      return Status;
> > > >    }
> > > >
> > > > -  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private-
> > > > >BaseClkFreq[Slot], Private->ControllerVersion[Slot]);
> > > > +  Status = 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 == 3) || ((AccessMode == 2) && (Capability-
> > > > >TuningSDR50 != 0))) {
> > > > +  if ((BusMode.BusTiming == SdMmcUhsSdr104) ||
> > ((BusMode.BusTiming
> > > > == SdMmcUhsSdr50) && (Capability->TuningSDR50 != 0))) {
> > > >      Status = 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 != 0 ||
> > > > -       Private->Capability[Slot].Sdr104 != 0 ||
> > > > -       Private->Capability[Slot].Ddr50 != 0) &&
> > > > -       ((Ocr & BIT24) != 0)) {
> > > > +  if ((Private->Capability[Slot].Sdr50 || Private->Capability[Slot].Ddr50
> ||
> > > > Private->Capability[Slot].Sdr104) &&
> > > > +      ((Ocr & BIT24) != 0)) {
> > >
> > >
> > > Minor comment, for:
> > > Private->Capability[Slot].Sdr50
> > > Private->Capability[Slot].Ddr50
> > > Private->Capability[Slot].Sdr104
> > >
> > > They are of type UINT32, should use operators '==' or '!=' in condition
> > > statement. I think the origin logic is good here.
> > >
> > >
> > > >      Status = 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 = {
> > > >    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}}
> > >
> > >
> > > 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
> > > > = {
> > > >                                      // Queue
> > > >    INITIALIZE_LIST_HEAD_VARIABLE (gSdMmcPciHcTemplate.Queue),
> > > >    {                                 // Slot
> > > > -    {0, UnknownSlot, 0, 0, 0}, {0, UnknownSlot, 0, 0, 0}, {0, UnknownSlot,
> 0,
> > 0,
> > > > 0},
> > > > -    {0, UnknownSlot, 0, 0, 0}, {0, UnknownSlot, 0, 0, 0}, {0, UnknownSlot,
> 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 for the
> > > > specified child device.
> > > > @@ -619,7 +629,6 @@ SdMmcPciHcDriverBindingStart (
> > > >    Support64BitDma = TRUE;
> > > >    for (Slot = FirstBar; Slot < (FirstBar + SlotNum); Slot++) {
> > > >      Private->Slot[Slot].Enable = TRUE;
> > > > -
> > > >      //
> > > >      // Get SD/MMC Pci Host Controller Version
> > > >      //
> > > > @@ -635,19 +644,34 @@ SdMmcPciHcDriverBindingStart (
> > > >
> > > >      Private->BaseClkFreq[Slot] = Private->Capability[Slot].BaseClkFreq;
> > > >
> > > > -    if (mOverride != NULL && mOverride->Capability != NULL) {
> > > > -      Status = 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 != NULL) {
> > > > +      if (mOverride->Capability != NULL) {
> > > > +        Status = 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 != NULL) {
> > > > +        Status = mOverride->NotifyPhase (
> > > > +                              Controller,
> > > > +                              Slot,
> > > > +                              EdkiiSdMmcGetOperatingParam,
> > > > +                              (VOID*)&Private->Slot[Slot].OperatingParameters
> > > > +                              );
> > > > +        if (EFI_ERROR (Status)) {
> > > > +          DEBUG ((DEBUG_WARN, "%a: Failed to get operating parameters,
> > > > 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_SLOT];
> > > >  } 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 dropping 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 recipient,
> > please contact the sender and delete all copies; any review or distribution
> by
> > others is strictly prohibited.
> >
> >
> > 


      reply	other threads:[~2019-06-18  3:23 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190603113346.1288-1-mateusz.albecki@intel.com>
     [not found] ` <B80AF82E9BFB8E4FBD8C89DA810C6A093C8E4722@SHSMSX104.ccr.corp.intel.com>
2019-06-13 13:43   ` [PATCH 0/2] Add GetOperatingParam notify phase to SdMmcOverride protocol mateusz.albecki
2019-06-14  1:46     ` Wu, Hao A
2019-06-14 18:44       ` Marcin Wojtas
     [not found] ` <20190603113346.1288-2-mateusz.albecki@intel.com>
     [not found]   ` <B80AF82E9BFB8E4FBD8C89DA810C6A093C8E4729@SHSMSX104.ccr.corp.intel.com>
2019-06-13 13:56     ` [PATCH 1/2] MdeModulePkg/SdMmcOverride: Add GetOperatingParam notify phase Albecki, Mateusz
     [not found] ` <20190603113346.1288-3-mateusz.albecki@intel.com>
     [not found]   ` <B80AF82E9BFB8E4FBD8C89DA810C6A093C8E473C@SHSMSX104.ccr.corp.intel.com>
2019-06-13 14:38     ` [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: Implement revision 3 of SdMmcOverrideProtocol Albecki, Mateusz
2019-06-14  3:14       ` Wu, Hao A
2019-06-18 15:14         ` Albecki, Mateusz
     [not found]     ` <15A7C8F163ADE317.25258@groups.io>
2019-06-17 15:11       ` [edk2-devel] " Albecki, Mateusz
2019-06-18  3:23         ` Wu, Hao A [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=B80AF82E9BFB8E4FBD8C89DA810C6A093C8F22D5@SHSMSX104.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox