Re: [PATCH v3 1/3] MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence

[Marcin Wojtas <mw@semihalf.com>]

Hi Hao,

pon., 17 wrz 2018 o 09:18 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
>
> > -----Original Message-----
> > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > Sent: Sunday, September 16, 2018 6:26 AM
> > To: edk2-devel@lists.01.org
> > Cc: Tian, Feng; Kinney, Michael D; Gao, Liming; leif.lindholm@linaro.org; Wu,
> > Hao A; ard.biesheuvel@linaro.org; nadavh@marvell.com; mw@semihalf.com;
> > jsd@semihalf.com; tm@semihalf.com
> > Subject: [PATCH v3 1/3] MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock
> > and bus width sequence
> >
> > According to JESD84-B50-1 chapter A.6 (documentation about eMMC4.5
> > standard) step "Changing the data bus width" (A.6.3) should be
> > executed after step "Switching to high-speed mode" (A.6.2).
>
> Hi,
>
> My understanding to the spec is that the spec seems do not impose a
> sequence for 'Switching to high-speed mode' and 'Changing the data bus
> width'.
>
> My find is that both operation (A.6.2 & A.6.3 of JESD84-B50-1 ) requires:
>
> * Do these steps after the bus is initialized according to A.6.1, Bus
> * initialization.
>
> Also, for HS200 mode selection, the flow chart in Section 6.6.4 shows the
> bus width set before bus mode switch. So I think the current code is
> taking this as a reference when switching the 'High Speed' mode.
>
> I tried the eMMC device on my side, the current code implementation and
> codes after applying your patch both work. So do you met with issues with
> certain device with this sequence? If not, I prefer to keep the current
> logic.
>

I re-checked on my boards with unchanged logic - all 3 can work in
High Speed. Patch is pretty old, I can't recall exact circumstances.
I'll drop it in v4.

Best regards,
Marcin

> Best Regards,
> Hao Wu
>
> >
> > This patch fixes the bus-width/clock-setting sequence
> > in EmmcSwitchToHighSpeed ().
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index c5fd214..12935ef 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -745,10 +745,6 @@ EmmcSwitchToHighSpeed (
> >    UINT8               HostCtrl1;
> >    UINT8               HostCtrl2;
> >
> > -  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
> > -  if (EFI_ERROR (Status)) {
> > -    return Status;
> > -  }
> >    //
> >    // Set to Hight Speed timing
> >    //
> > @@ -783,6 +779,11 @@ EmmcSwitchToHighSpeed (
> >
> >    HsTiming = 1;
> >    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming,
> > ClockFreq);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
> >
> >    return Status;
> >  }
> > --
> > 2.7.4
>