Re: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus timing switch sequence

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----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.wu@intel.com>
> > Sent: Thursday, September 26, 2019 3:36 AM
> > To: Albecki, Mateusz <mateusz.albecki@intel.com>; devel@edk2.groups.io
> > Cc: Marcin Wojtas <mw@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.wu@intel.com>
> > > > Sent: Wednesday, September 25, 2019 5:34 AM
> > > > To: Albecki, Mateusz <mateusz.albecki@intel.com>;
> > > > devel@edk2.groups.io
> > > > Cc: Marcin Wojtas <mw@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.albecki@intel.com>
> > > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > > Cc: Marcin Wojtas <mw@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
> > > >
> > >
> >
>