> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Thursday, September 26, 2019 9:29 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Marcin Wojtas
> Subject: RE: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus timing
> switch sequence
>
>
>
> > -----Original Message-----
> > From: Wu, Hao A <hao.a...@intel.com>
> > Sent: Thursday, September 26, 2019 3:36 AM
> > To: Albecki, Mateusz <mateusz.albe...@intel.com>; devel@edk2.groups.io
> > Cc: Marcin Wojtas <m...@semihalf.com>
> > Subject: RE: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus timing
> > switch sequence
> >
> > > -----Original Message-----
> > > From: Albecki, Mateusz
> > > Sent: Wednesday, September 25, 2019 10:54 PM
> > > To: Wu, Hao A; devel@edk2.groups.io
> > > Cc: Marcin Wojtas
> > > Subject: RE: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus
> timing
> > > switch sequence
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wu, Hao A <hao.a...@intel.com>
> > > > Sent: Wednesday, September 25, 2019 5:34 AM
> > > > To: Albecki, Mateusz <mateusz.albe...@intel.com>;
> > > > devel@edk2.groups.io
> > > > Cc: Marcin Wojtas <m...@semihalf.com>
> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus
> > timing
> > > > switch sequence
> > > >
> > > > > -----Original Message-----
> > > > > From: Albecki, Mateusz
> > > > > Sent: Monday, September 23, 2019 4:37 PM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas
> > > > > Subject: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus
> timing
> > > > switch
> > > > > sequence
> > > > >
> > > > > SD specification recommends switching card bus timing before
> > > > > switching bus timing in controller. Emmc driver used to do this
> > > > > switch other way around. This commit adds controller timing switch
> > > > > in EmmcSwitchBusTiming function to enforce this order and removes
> > > > > all controller timing programing from EmmcSwitchToXXX functions.
> > > >
> > > >
> > > > Hello Mateusz,
> > > >
> > > > I think the changes in the patch look good.
> > > >
> > > > Could you help to address the below things:
> > > >
> > > > 1. The 'Private' local variable is no longer being used by below
> > > > functions:
> > > > EmmcSwitchToHighSpeed()
> > > > EmmcSwitchToHS200()
> > > > EmmcSwitchToHS400()
> > > >
> > > > The GCC compiler seems not happy with it, could you help to resolve it?
> > >
> > > Sure
> > > >
> > > > 2. I have submitted a Bugzilla tracker for this issue at:
> > > > https://bugzilla.tianocore.org/show_bug.cgi?id=2218
> > > >
> > > > Could you help to add this information in the commit log message?
> > >
> > > Sure
> > > >
> > > > 3. For the removal of bus clock stopping codes in
> > > > EmmcSwitchToHS200(), could you split them into another separate
> > > > patch? I think they are not
> > > directly
> > > > related with the bus speed mode changing sequence.
> > > >
> > >
> > > I could do that sure, but since I have to remove the call to
> > > SdMmcHcUhsSignaling from this function stopping and starting clock no
> > > longer makes any sense so it would result in a nonsensical code in
> > > this one patch. Anyway let me know if you still want to split it and I
> > > will.
> >
> >
> > My initial thought was to split the removal of bus clock stopping and
> > resuming codes (before and after the call of function
> > SdMmcHcUhsSignaling()) into the 1st patch of the series. Then a 2nd patch
> > will relocate the call of function
> > SdMmcHcUhsSignaling() to fix the bus mode switching sequence issue.
> >
> > I think from your description in the cover-letter, such bus clock stopping
> and
> > resuming is only required when the preset values are being used (which is
> > not the case for the current implementation of the driver).
> >
> > What do you think of this?
> >
>
> Yeah that makes sense. I will split it like that.
>
> >
> > >
> > > > 4. I have performed the below tests on the eMMC device on my side,
> > > > for different eMMC bus modes:
> > > > HS400 (good)
> > > > HS200 (good)
> > > > High Speed DDR (good)
> > > > High Speed SDR (good)
> > > > Legacy MMC (NOT good)
> > > >
> > > > For the Legacy MMC mode case, the error comes from the
> > > > EmmcSwitchToHighSpeed() function returning
> EFI_INVALID_PARAMETER
> > > for
> > > > bus mode 'SdMmcMmcLegacy'. I think for the EmmcSetBusMode()
> > > function,
> > > > the below updates can be made:
> > > >
> > > > if (BusMode.BusTiming == SdMmcMmcHs400) {
> > > > Status = EmmcSwitchToHS400 (PciIo, PassThru, Slot, Rca, &BusMode);
> > > > } else if (BusMode.BusTiming == SdMmcMmcHs200) {
> > > > Status = EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, &BusMode);
> > > > } else { <== should be updated to:
> > > > } else if (BusMode.BusTiming == SdMmcMmcHsSdr ||
> > > BusMode.BusTiming
> > > > == SdMmcMmcHsDdr) {
> > > > Status = EmmcSwitchToHighSpeed (PciIo, PassThru, Slot, Rca,
> > > &BusMode);
> > > > }
> > > >
> > > > think for the Legacy MMC mode, no additional bus mode switch is
> > needed.
> > > If
> > > > you agree with this, could you help to add another patch to change
> > > > the above logic (also the debug message after the above-shown code)?
> > > >
> > > > Best Regards,
> > > > Hao Wu
> > > >
> > >
> > > That is actually another bug introduced by the previous patch, right?
> > > I will fix it in the separate patch(but in this series).
> >
> >
> > Before the previous patch, I found that in EmmcSetBusMode(), the Legacy
> > MMC mode will be handled in EmmcSwitchToHighSpeed():
> >
> > } else if (((ExtCsd.DeviceType & BIT0) != 0) && (Private-
> > >Capability[Slot].HighSpeed != 0)) {
> > HsTiming = 1;
> > IsDdr = FALSE;
> > ClockFreq = 26;
> > }
> >
> > ...
> >
> > 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);
> > } else {
> > //
> > // Execute High Speed timing switch procedure
> > //
> > Status = EmmcSwitchToHighSpeed (PciIo, PassThru, Slot, Rca, ClockFreq,
> > IsDdr, BusWidth);
> > }
> >
> > Also, there is logic for handling the case that no high speed mode is
> > supported:
> >
> > if ((ClockFreq == 0) || (HsTiming == 0)) {
> > //
> > // Continue using default setting.
> > //
> > return EFI_SUCCESS;
> > }
> >
> > After a second thought, the previous suggested solution does not seem
> > proper (parameters like bus width and working clock frequency will not get
> > customized for the Legacy MMC mode).
> >
> > I think we can add a 3rd patch to:
> >
> > A. Match the check in EmmcIsBusTimingSupported() for the
> > SdMmcMmcLegacy case
> > with the above shown code.
> > B. Restore to the previous handling approach that let the
> > EmmcSwitchToHighSpeed()
> > function to handle Legacy MMC mode.
> > C. Add back codes to return directly when no high speed mode is
> supported.
> >
> > Best Regards,
> > Hao Wu
> >
>
> After closer inspection I think the problem might be a little more
> complicated.
> According to eMMC 5.1 specification section 5.3.2 high speed SDR mode
> supports clock frequency in the range 0-52MHz while 0-26MHz is reserved for
> the backwards compatibility with MMC card(SdMmcMmcLegacy). This
> suggests that frequencies <=26MHz are not high speed so we do not have to
> switch link to high speed timing. In the same specification it says that if
> BIT0 in
> DEVICE_TYPE field in EXT_CSD register is set then the card is "High speed
> eMMC at 26MHz" which would suggest that high speed mode should be
> enabled for those cards and we should just be careful to not exceed the
> clock frequency of 26MHz.
Yes. The mention of the:
High-Speed eMMC at 26 MHz - at rated device voltage(s)
by the EMMC spec under the description for Extended CSD register looks
inconsistent with the high speed bus mode defined in other places in the spec.
>
> That said in section 6.6.2 which actually talks about high speed mode
> selection spec is very clear that host should switch the card to high speed
> only if it wishes to operate at frequencies greater then 26MHz. Given that I
> think we should follow your proposition with minor modification. We will
> keep treating the eMMC devices that have only set BIT0 in DEVICE_TYPE as
> legacy devices and we will not switch the to high speed. The reason for that
> is
> that we will never exceed legacy clock frequencies anyway. The change will
> be basically limited to allowing SdMmcMmcLegacy bus timing in
> EmmcSwitchToHighSpeed since I think EmmcSwitchBusTiming function is
> ready to handle SdMmcMmcLegacy argument.
Before commit adec1f5deb89c68d82598074500006bd575e8f58:
* MdeModulePkg/SdMmcHcDxe: Implement revision 3 of SdMmcOverrideProtocol
The driver attempted to switch to high speed bus mode for device whose maximum
working frequency is 26MHz (which maps to 'SdMmcMmcLegacy' bus timing in current
driver implementation):
} else if (((ExtCsd.DeviceType & BIT0) != 0) &&
(Private->Capability[Slot].HighSpeed != 0)) {
HsTiming = 1;
IsDdr = FALSE;
ClockFreq = 26;
}
(where setting 'HsTiming' to 1 means switching to high speed bus mode)
However, I think the above codes do not have a chance to be executed, due to
the lack of eMMC device that ONLY supports this bus timing mode.
So based on the above findings, I agree with your proposal to NOT switching to
the high speed mode for SdMmcMmcLegacy bus timing.
Best Regards,
Hao Wu
>
> >
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Mateusz Albecki <mateusz.albe...@intel.com>
> > > > > Cc: Hao A Wu <hao.a...@intel.com>
> > > > > Cc: Marcin Wojtas <m...@semihalf.com>
> > > > > ---
> > > > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 79
> > +++++++-
> > > --
> > > > ----
> > > > > -----------
> > > > > 1 file changed, 20 insertions(+), 59 deletions(-)
> > > > >
> > > > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > > > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > > > > index 3f4a8e5413..06ee1208be 100644
> > > > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > > > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > > > > @@ -671,6 +671,7 @@ EmmcSwitchBusTiming (
> > > > > UINT8 CmdSet;
> > > > > UINT32 DevStatus;
> > > > > SD_MMC_HC_PRIVATE_DATA *Private;
> > > > > + UINT8 HostCtrl1;
> > > > >
> > > > > Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> > > > > //
> > > > > @@ -704,6 +705,25 @@ EmmcSwitchBusTiming (
> > > > > return Status;
> > > > > }
> > > > >
> > > > > + if (BusTiming == SdMmcMmcHsSdr || BusTiming ==
> > SdMmcMmcHsDdr)
> > > {
> > > > > + HostCtrl1 = BIT2;
> > > > > + Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1,
> > > > sizeof
> > > > > (HostCtrl1), &HostCtrl1);
> > > > > + if (EFI_ERROR (Status)) {
> > > > > + return Status;
> > > > > + }
> > > > > + } else {
> > > > > + HostCtrl1 = (UINT8)~BIT2;
> > > > > + Status = SdMmcHcAndMmio (PciIo, Slot,
> SD_MMC_HC_HOST_CTRL1,
> > > > > sizeof (HostCtrl1), &HostCtrl1);
> > > > > + if (EFI_ERROR (Status)) {
> > > > > + return Status;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> > > > > + Slot,
> > > > > BusTiming);
> > > > > + if (EFI_ERROR (Status)) {
> > > > > + return Status;
> > > > > + }
> > > > > +
> > > > > //
> > > > > // Convert the clock freq unit from MHz to KHz.
> > > > > //
> > > > > @@ -772,7 +792,6 @@ EmmcSwitchToHighSpeed (
> > > > > )
> > > > > {
> > > > > EFI_STATUS Status;
> > > > > - UINT8 HostCtrl1;
> > > > > SD_MMC_HC_PRIVATE_DATA *Private;
> > > > > BOOLEAN IsDdr;
> > > > >
> > > > > @@ -794,20 +813,6 @@ EmmcSwitchToHighSpeed (
> > > > > return Status;
> > > > > }
> > > > >
> > > > > - //
> > > > > - // Set to High Speed timing
> > > > > - //
> > > > > - HostCtrl1 = BIT2;
> > > > > - Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1,
> > > > sizeof
> > > > > (HostCtrl1), &HostCtrl1);
> > > > > - if (EFI_ERROR (Status)) {
> > > > > - return Status;
> > > > > - }
> > > > > -
> > > > > - Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> > > > > Slot,
> > > > > BusMode->BusTiming);
> > > > > - if (EFI_ERROR (Status)) {
> > > > > - return Status;
> > > > > - }
> > > > > -
> > > > > return EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca,
> > > > > BusMode-
> > > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq);
> > > > > }
> > > > >
> > > > > @@ -837,7 +842,6 @@ EmmcSwitchToHS200 (
> > > > > )
> > > > > {
> > > > > EFI_STATUS Status;
> > > > > - UINT16 ClockCtrl;
> > > > > SD_MMC_HC_PRIVATE_DATA *Private;
> > > > >
> > > > > Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); @@ -
> > 851,39
> > > > +855,6
> > > > > @@ EmmcSwitchToHS200 (
> > > > > if (EFI_ERROR (Status)) {
> > > > > return Status;
> > > > > }
> > > > > - //
> > > > > - // Stop bus clock at first
> > > > > - //
> > > > > - Status = SdMmcHcStopClock (PciIo, Slot);
> > > > > - if (EFI_ERROR (Status)) {
> > > > > - return Status;
> > > > > - }
> > > > > -
> > > > > - Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> > > > > Slot,
> > > > > BusMode->BusTiming);
> > > > > - if (EFI_ERROR (Status)) {
> > > > > - return Status;
> > > > > - }
> > > > > -
> > > > > - //
> > > > > - // Wait Internal Clock Stable in the Clock Control register to
> > > > > be 1 before set SD Clock Enable bit
> > > > > - //
> > > > > - Status = SdMmcHcWaitMmioSet (
> > > > > - PciIo,
> > > > > - Slot,
> > > > > - SD_MMC_HC_CLOCK_CTRL,
> > > > > - sizeof (ClockCtrl),
> > > > > - BIT1,
> > > > > - BIT1,
> > > > > - SD_MMC_HC_GENERIC_TIMEOUT
> > > > > - );
> > > > > - if (EFI_ERROR (Status)) {
> > > > > - return Status;
> > > > > - }
> > > > > - //
> > > > > - // Set SD Clock Enable in the Clock Control register to 1
> > > > > - //
> > > > > - ClockCtrl = BIT2;
> > > > > - Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL,
> > > > sizeof
> > > > > (ClockCtrl), &ClockCtrl);
> > > > >
> > > > > Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca,
> > > > > BusMode-
> > > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq);
> > > > > if (EFI_ERROR (Status)) {
> > > > > @@ -945,11 +916,6 @@ EmmcSwitchToHS400 (
> > > > > // 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.
> > > > > //
> > > > > - Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> > > > > Slot, SdMmcMmcHsSdr);
> > > > > - if (EFI_ERROR (Status)) {
> > > > > - return Status;
> > > > > - }
> > > > > -
> > > > > HsFreq = BusMode->ClockFreq < 52 ? BusMode->ClockFreq : 52;
> > > > > Status = EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca,
> > > > > BusMode-
> > > > > >DriverStrength, SdMmcMmcHsSdr, HsFreq);
> > > > > if (EFI_ERROR (Status)) {
> > > > > @@ -961,11 +927,6 @@ EmmcSwitchToHS400 (
> > > > > return Status;
> > > > > }
> > > > >
> > > > > - Status = SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo,
> > > > > Slot,
> > > > > BusMode->BusTiming);
> > > > > - if (EFI_ERROR (Status)) {
> > > > > - return Status;
> > > > > - }
> > > > > -
> > > > > return EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca,
> > > > > BusMode-
> > > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq);
> > > > > }
> > > > >
> > > > > --
> > > > > 2.14.1.windows.1
> > > >
> > >
> >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48209): https://edk2.groups.io/g/devel/message/48209
Mute This Topic: https://groups.io/mt/34262004/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
