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

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

> -----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.

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