Re: [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Fix HS200 operation

[Marcin Wojtas <mw@semihalf.com>]

Hi Hao,

Thank you for your input. In v3, I'll withdraw this patch. I tested
and I can use HS200 successfully on my boards without it. Both changes
(HS enable bit and removing clock disabling) were residues from an old
debug, whereas the acutal fix was an additional HW reconfiguration
after switching the clock frequency.

Best regards,
Marcin

czw., 13 wrz 2018 o 16:43 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Marcin Wojtas
> > Sent: Friday, September 07, 2018 5:10 PM
> > To: edk2-devel@lists.01.org
> > Cc: Tian, Feng; tm@semihalf.com; nadavh@marvell.com; Gao, Liming; Kinney,
> > Michael D
> > Subject: [edk2] [PATCH v2 1/4] MdeModulePkg/SdMmcPciHcDxe: Fix HS200
> > operation
> >
> > When switching to any of high speed modes (HS, HS200, HS400)
> > there is need to set HS_ENABLE bit in Host Control 1 register
> > which allow Host Controller to output CMD and DAT lines on
> > both edges of clock. In Linux it is done after switching bus
> > width in sdhci_set_ios().
>
> I am checking both the SD and eMMC specs for this. As far as I can tell,
> since the eMMC cards that support HS200 & HS400 will have 1.8V/1.2V
> signaling. This makes a match within the SD specs for UHS-I mode (rather
> than High Speed mode which is 3.3V signaling).
>
> Switching the host controller to UHS-I mode requires configuring the '1.8V
> Signaling Enable' (BIT3) and 'UHS Mode Select' (BIT2~0) fields of the Host
> Control 2 Register. And the setting of the 'High Speed Enable' (BIT2)
> field of the Host Control 1 Register seems not required to me.
>
> Do you met with issues when switching the eMMC device to HS200/HS400 mode
> when not setting BIT2 of the Host Control Register?
>
> >
> > Also according to JESD84-B50-1 chapter 6.6.4 "HS200 timing mode
> > selection" (documentation about eMMC4.5 standard) there is
> > no need to disable clock when switching to HS200.
>
> Yes, you are right that the eMMC spec does not require such a reset of the
> reset SD Clock Enable (BIT2) of the Clock Control Register.
>
> I think the code here is taking reference to the SD Host Controller
> Simplified Spec 3.0:
>
> Under the description of 'High Speed Enable' (BIT2) of the Host Control 1
> Register (Table 2-16):
>
> * If Preset Value Enable in the Host Control 2 register is set to 1, Host
> * Driver needs to reset SD Clock Enable before changing this field to
> * avoid generating clock glitches. After setting this field, the Host
> * Driver sets SD Clock Enable again.
>
> The spec makes very clear that the reset SD Clock Enable is only needed
> when Preset Value Enable in the Host Control 2 register is set to 1.
>
> But for the description of 'UHS Mode Select' (BIT2~0) of the Host Control
> 2 Register (Table 2-32):
>
> * If Preset Value Enable in the Host Control 2 register is set to 1, Host
> * Controller sets SDCLK Frequency Select, Clock Generator Select in the
> * Clock Control register and Driver Strength Select according to Preset
> * Value registers. In this case, one of preset value registers is selected
> * by this field. Host Driver needs to reset SD Clock Enable before changing
> * this field to avoid generating clock glitch. After setting this field,
> * Host Driver sets SD Clock Enable again.
>
> It's not that clear whether this reset is needed under all cases. So the
> driver just play it safe here to added reset of the SD Clock Enable bit
> during the switch of HS200 mode for eMMC devices.
>
> So do you met with issues here for this? If not, I prefer to keep the
> re-enabling of the clock logic here for stability consideration.
>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  5 ++++
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 30 +++----------------
> > -
> >  2 files changed, 9 insertions(+), 26 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > index e389d52..e3fadb5 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > @@ -63,6 +63,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> > KIND, EITHER EXPRESS OR IMPLIED.
> >  #define SD_MMC_HC_CTRL_VER            0xFE
> >
> >  //
> > +// SD Host Control 1 Register bits description
> > +//
> > +#define SD_MMC_HC_HOST_CTRL1_HS_ENABLE  (1 << 2)
> > +
> > +//
> >  // The transfer modes supported by SD Host Controller
> >  // Simplified Spec 3.0 Table 1-2
> >  //
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index c5fd214..b3903b4 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -816,8 +816,8 @@ EmmcSwitchToHS200 (
> >  {
> >    EFI_STATUS          Status;
> >    UINT8               HsTiming;
> > +  UINT8               HostCtrl1;
> >    UINT8               HostCtrl2;
> > -  UINT16              ClockCtrl;
> >
> >    if ((BusWidth != 4) && (BusWidth != 8)) {
> >      return EFI_INVALID_PARAMETER;
> > @@ -828,12 +828,10 @@ EmmcSwitchToHS200 (
> >      return Status;
> >    }
> >    //
> > -  // Set to HS200/SDR104 timing
> > -  //
> > -  //
> > -  // Stop bus clock at first
> > +  // Set to High Speed timing
> >    //
> > -  Status = SdMmcHcStopClock (PciIo, Slot);
> > +  HostCtrl1 = SD_MMC_HC_HOST_CTRL1_HS_ENABLE;
> > +  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1, sizeof
> > (HostCtrl1), &HostCtrl1);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > @@ -853,26 +851,6 @@ EmmcSwitchToHS200 (
> >    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);
> >
> >    HsTiming = 2;
> >    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming,
> > ClockFreq);
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel