From: "Albecki, Mateusz" <mateusz.albecki@intel.com>
To: "Wu, Hao A" <hao.a.wu@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>,
"mw@semihalf.com" <mw@semihalf.com>
Subject: Re: [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: Implement revision 3 of SdMmcOverrideProtocol
Date: Tue, 18 Jun 2019 15:14:43 +0000 [thread overview]
Message-ID: <92CF190FF2351747A47C1708F0E09C0875E6C2DB@IRSMSX102.ger.corp.intel.com> (raw)
In-Reply-To: <B80AF82E9BFB8E4FBD8C89DA810C6A093C8F064E@SHSMSX104.ccr.corp.intel.com>
Response inline to the bus width switch before SWITCH command in UHS-I mode. As I mentioned in forked message mapping SD driver strength to eMMC driver strength seems like a bad idea right now as I don't know of any eMMC host controller that would actually follow this map and would require any host programming.
Thanks,
Mateusz
> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, June 14, 2019 5:15 AM
> To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io;
> mw@semihalf.com
> Subject: RE: [PATCH 2/2] MdeModulePkg/SdMmcHcDxe: Implement revision
> 3 of SdMmcOverrideProtocol
>
> > -----Original Message-----
> > From: Albecki, Mateusz
> > Sent: Thursday, June 13, 2019 10:39 PM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A
> > Subject: RE: [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
>
>
> Yes, I think this matching is fine. Inspecting the SD & eMMC specs:
>
> For SD Physical Layer Specification:
> Driver Type Nominal Driving
> Impedance Capability
> A 33Ω x1.5
> B 50Ω x1
> C 66Ω x0.75
> D 100Ω x0.5
>
> For Embedded Multi-Media Card (e.MMC) Electrical Standard:
> Driver Type Nominal Approximated driver capability
> Values Impedance compared to Type-0
> 0x0 50Ω x1
> 0x1 33Ω x1.5
> 0x2 66Ω x0.75
> 0x3 100Ω x0.5
> 0x4 40Ω x1.2
>
> The above mapping is consistent with regard to the Nominal Impedance &
> Driving Capability parameters.
>
>
> >
> > 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.
>
>
> I am not sure if the bit39 in the capability register works for all SD
> host controller (because it is not documented in specs, as far as I know).
> I suggest we just ignore the HC capability when setting driver strength
> type 4 for eMMC devices.
>
> Response to other inline comments below:
>
>
> >
> > 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.
>
>
> How about using the mapping at the beginning of the thread to set the HC2
> register and ignore this step for eMMC driver type 4?
>
>
> >
> > >
> > >
> > > > 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.
>
>
> Hello Marcin,
> Do you see any concern for this specific change?
>
>
> >
> > >
> > > > 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.
>
>
> Similar as one comment above, the mapping of eMMC driver type 0~3 looks
> consistent to SD driver type A~D. As for eMMC driver type 4, I think just
> checking the capability of the device is enough.
>
>
> >
> > >
> > >
> > > > + 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.
>
>
> Either approach is okay to me, since both of them need extra handling in
> SdCardSwitch() & SdMmcHcUhsSignaling() when setting driver strength.
>
> I slightly prefer the 1st option, since the logic is a bit more straight-
> forward.
>
> >
> > >
> > > > +{
> > > > + 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.
>
>
> Sorry for not being clear.
> I suggest to just remove the below codes:
>
> 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;
> }
> }
>
> And set the bus width in one place after SdGetTargetBusMode().
> (Also remove the 'if' condition - if (!S18A), do it together for UHS-I &
> 3.3V signaling cards.)
>
> Best Regards,
> Hao Wu
I am not sure if that would work. My understanding is that SWITCH command needs to be executed on 4-bit mode as that includes data transfer with DAT lanes so that bus width switch beforehand is necessary.
>
>
> >
> > >
> > > >
> > > > - 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.
next prev parent reply other threads:[~2019-06-18 15:14 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 [this message]
[not found] ` <15A7C8F163ADE317.25258@groups.io>
2019-06-17 15:11 ` [edk2-devel] " Albecki, Mateusz
2019-06-18 3:23 ` Wu, Hao A
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=92CF190FF2351747A47C1708F0E09C0875E6C2DB@IRSMSX102.ger.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