Re: [PATCH v5 6/9] MmcDxe: set io bus width before reading EXTCSD

[Leif Lindholm <leif.lindholm@linaro.org>]

On Sun, Nov 13, 2016 at 02:47:55PM +0800, Haojian Zhuang wrote:
> Set io bus width on both MMC controller and EXTCSD. Otherwise, it may
> cause unmatched failure case. And support more timing mode, high speed,
> HS200 & HS400 mode.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
> ---
>  EmbeddedPkg/Include/Protocol/MmcHost.h           |  16 +-
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 229 ++++++++++++++++++++---
>  2 files changed, 221 insertions(+), 24 deletions(-)
> 
> diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
> index a242291..01c0bf4 100644
> --- a/EmbeddedPkg/Include/Protocol/MmcHost.h
> +++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
> @@ -49,6 +49,7 @@ typedef UINT32 MMC_CMD;
>  #define MMC_CMD2              (MMC_INDX(2) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_LONG_RESPONSE)
>  #define MMC_CMD3              (MMC_INDX(3) | MMC_CMD_WAIT_RESPONSE)
>  #define MMC_CMD5              (MMC_INDX(5) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_NO_CRC_RESPONSE)
> +#define MMC_CMD6              (MMC_INDX(6) | MMC_CMD_WAIT_RESPONSE)
>  #define MMC_CMD7              (MMC_INDX(7) | MMC_CMD_WAIT_RESPONSE)
>  #define MMC_CMD8              (MMC_INDX(8) | MMC_CMD_WAIT_RESPONSE)
>  #define MMC_CMD9              (MMC_INDX(9) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_LONG_RESPONSE)
> @@ -82,6 +83,16 @@ typedef enum _MMC_STATE {
>      MmcDisconnectState,
>  } MMC_STATE;
>  
> +#define EMMCBACKWARD         (0)
> +#define EMMCHS26             (1 << 0)      // High-Speed @26MHz at rated device voltages
> +#define EMMCHS52             (1 << 1)      // High-Speed @52MHz at rated device voltages
> +#define EMMCHS52DDR1V8       (1 << 2)      // High-Speed Dual Data Rate @52MHz 1.8V or 3V I/O
> +#define EMMCHS52DDR1V2       (1 << 3)      // High-Speed Dual Data Rate @52MHz 1.2V I/O
> +#define EMMCHS200SDR1V8      (1 << 4)      // HS200 Single Data Rate @200MHz 1.8V I/O
> +#define EMMCHS200SDR1V2      (1 << 5)      // HS200 Single Data Rate @200MHz 1.2V I/O
> +#define EMMCHS400DDR1V8      (1 << 6)      // HS400 Dual Data Rate @400MHz 1.8V I/O
> +#define EMMCHS400DDR1V2      (1 << 7)      // HS400 Dual Data Rate @400MHz 1.2V I/O
> +
>  ///
>  /// Forward declaration for EFI_MMC_HOST_PROTOCOL
>  ///
> @@ -133,8 +144,9 @@ typedef EFI_STATUS (EFIAPI *MMC_WRITEBLOCKDATA) (
>  
>  typedef EFI_STATUS (EFIAPI *MMC_SETIOS) (
>    IN  EFI_MMC_HOST_PROTOCOL     *This,
> -  IN  UINT32                    BusClockRate,
> -  IN  UINT32                    BusWidth
> +  IN  UINT32                    BusClockFreq,
> +  IN  UINT32                    BusWidth,
> +  IN  UINT32                    TimingMode
>    );
>  
>  
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 378e438..9a79767 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -25,11 +25,109 @@ typedef union {
>  #define EMMC_CARD_SIZE          512
>  #define EMMC_ECSD_SIZE_OFFSET   53
>  
> +#define EXTCSD_BUS_WIDTH        183
> +#define EXTCSD_HS_TIMING        185
> +
> +#define EMMC_TIMING_BACKWARD    0
> +#define EMMC_TIMING_HS          1
> +#define EMMC_TIMING_HS200       2
> +#define EMMC_TIMING_HS400       3
> +
> +#define EMMC_BUS_WIDTH_1BIT     0
> +#define EMMC_BUS_WIDTH_4BIT     1
> +#define EMMC_BUS_WIDTH_8BIT     2
> +#define EMMC_BUS_WIDTH_DDR_4BIT 5
> +#define EMMC_BUS_WIDTH_DDR_8BIT 6
> +
> +#define EMMC_SWITCH_ERROR       (1 << 7)
> +
> +#define DEVICE_STATE(x)         (((x) >> 9) & 0xf)
> +typedef enum _EMMC_DEVICE_STATE {
> +  EMMC_IDLE_STATE = 0,
> +  EMMC_READY_STATE,
> +  EMMC_IDENT_STATE,
> +  EMMC_STBY_STATE,
> +  EMMC_TRAN_STATE,
> +  EMMC_DATA_STATE,
> +  EMMC_RCV_STATE,
> +  EMMC_PRG_STATE,
> +  EMMC_DIS_STATE,
> +  EMMC_BTST_STATE,
> +  EMMC_SLP_STATE
> +} EMMC_DEVICE_STATE;
> +
>  UINT32 mEmmcRcaCount = 0;
>  
>  STATIC
>  EFI_STATUS
>  EFIAPI
> +EmmcGetDeviceState (
> +  IN  MMC_HOST_INSTANCE    *MmcHostInstance,
> +  OUT EMMC_DEVICE_STATE    *State
> +  )
> +{
> +  EFI_MMC_HOST_PROTOCOL *Host;
> +  EFI_STATUS Status;
> +  UINT32     Data, RCA;
> +
> +  if (State == NULL)
> +    return EFI_INVALID_PARAMETER;

Always { and }.

> +
> +  Host  = MmcHostInstance->MmcHost;

Double space.

> +  RCA = MmcHostInstance->CardInfo.RCA << RCA_SHIFT_OFFSET;
> +  Status = Host->SendCommand (Host, MMC_CMD13, RCA);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to get card status, Status=%r.\n", Status));
> +    return Status;
> +  }
> +  Status = Host->ReceiveResponse (Host, MMC_RESPONSE_TYPE_R1, &Data);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to get response of CMD13, Status=%r.\n", Status));
> +    return Status;
> +  }
> +  if (Data & EMMC_SWITCH_ERROR) {
> +    DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to switch expected mode, Status=%r.\n", Status));
> +    return EFI_DEVICE_ERROR;
> +  }
> +  *State = DEVICE_STATE(Data);
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +EmmcSetEXTCSD (
> +  IN MMC_HOST_INSTANCE     *MmcHostInstance,
> +  UINT32                   ExtCmdIndex,
> +  UINT32                   Value
> +  )
> +{
> +  EFI_MMC_HOST_PROTOCOL *Host;
> +  EMMC_DEVICE_STATE     State;
> +  EFI_STATUS Status;
> +  UINT32     Argument;
> +
> +  Host  = MmcHostInstance->MmcHost;
> +  Argument = (3 << 24) | ((ExtCmdIndex & 0xff) << 16) | ((Value & 0xff) << 8) | 1;

Can these magic number operations be cleaned up with some #defines?

> +  Status = Host->SendCommand (Host, MMC_CMD6, Argument);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "EmmcSetEXTCSD(): Failed to send CMD6, Status=%r.\n", Status));
> +    return Status;
> +  }
> +  // Make sure device exiting prog mode
> +  do {
> +    Status = EmmcGetDeviceState (MmcHostInstance, &State);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_ERROR, "EmmcSetEXTCSD(): Failed to get device state, Status=%r.\n", Status));
> +      return Status;
> +    }
> +  } while (State == EMMC_PRG_STATE);
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
>  EmmcIdentificationMode (
>    IN MMC_HOST_INSTANCE     *MmcHostInstance,
>    IN OCR_RESPONSE           Response
> @@ -37,6 +135,7 @@ EmmcIdentificationMode (
>  {
>    EFI_MMC_HOST_PROTOCOL *Host;
>    EFI_BLOCK_IO_MEDIA    *Media;
> +  EMMC_DEVICE_STATE     State;
>    EFI_STATUS Status;
>    UINT32     RCA;
>  
> @@ -84,35 +183,117 @@ EmmcIdentificationMode (
>      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): Card selection error, Status=%r.\n", Status));
>    }
>  
> -  // Fetch ECSD
> -  Status = Host->SendCommand (Host, MMC_CMD8, RCA);
> -  if (EFI_ERROR (Status)) {
> -    DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));
> -  }
> -
> -  Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData));
> -  if (EFI_ERROR (Status)) {
> -    DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));
> -    return Status;
> -  }
> -
> +  // MMC v4 specific

Does this mean this code was previously unsafe for pre-MMC v4?
If so, that would be good to point out in commit message.

Or is it the case that all eMMC devices are MMCv4+?

> +  if (MmcHostInstance->CardInfo.CSDData.SPEC_VERS == 4) {
> +    if (Host->SetIos) {
> +      // Set 1-bit bus width
> +      Status = Host->SetIos (Host, 0, 1, EMMCBACKWARD);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): Set 1-bit bus width error, Status=%r.\n", Status));
> +        return Status;
> +      }
> +
> +      // Set 1-bit bus width for EXTCSD
> +      Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_1BIT);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): Set extcsd bus width error, Status=%r.\n", Status));
> +        return Status;
> +      }
> +    }
> +
> +    // Fetch ECSD
> +    Status = Host->SendCommand (Host, MMC_CMD8, RCA);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD fetch error, Status=%r.\n", Status));
> +    }
> +    Status = Host->ReadBlockData (Host, 0, 512, (UINT32 *)&(MmcHostInstance->CardInfo.ECSDData));
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): ECSD read error, Status=%r.\n", Status));
> +      return Status;
> +    }
> +
> +    // Make sure device exiting data mode
> +    do {
> +      Status = EmmcGetDeviceState (MmcHostInstance, &State);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((EFI_D_ERROR, "EmmcIdentificationMode(): Failed to get device state, Status=%r.\n", Status));
> +        return Status;
> +      }
> +    } while (State == EMMC_DATA_STATE);
> +
> +    // Compute last block using bits [215:212] of the ECSD
> +    Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SECTOR_COUNT - 1; // eMMC isn't supposed to report this for
> +    // Cards <2GB in size, but the model does.
> +
> +    // Setup card type
> +    MmcHostInstance->CardInfo.CardType = EMMC_CARD;
> +  }
>    // Set up media
>    Media->BlockSize = EMMC_CARD_SIZE; // 512-byte support is mandatory for eMMC cards
>    Media->MediaId = MmcHostInstance->CardInfo.CIDData.PSN;
>    Media->ReadOnly = MmcHostInstance->CardInfo.CSDData.PERM_WRITE_PROTECT;
>    Media->LogicalBlocksPerPhysicalBlock = 1;
>    Media->IoAlign = 4;
> -  // Compute last block using bits [215:212] of the ECSD
> -  Media->LastBlock = MmcHostInstance->CardInfo.ECSDData.SECTOR_COUNT - 1; // eMMC isn't supposed to report this for
> -  // Cards <2GB in size, but the model does.
> -
> -  // Setup card type
> -  MmcHostInstance->CardInfo.CardType = EMMC_CARD;

Does this need to be initialised to something else if not EMMC?

>    return EFI_SUCCESS;
>  }
>  
>  STATIC
>  EFI_STATUS
> +InitializeEmmcDevice (
> +  IN  MMC_HOST_INSTANCE   *MmcHostInstance
> +  )
> +{
> +  EFI_MMC_HOST_PROTOCOL *Host;
> +  EFI_STATUS Status = EFI_SUCCESS;
> +  ECSD       *ECSDData;
> +  BOOLEAN    Found = FALSE;
> +  UINT32     BusClockFreq, Idx;
> +  UINT32     TimingMode[4] = {EMMCHS52DDR1V2, EMMCHS52DDR1V8, EMMCHS52, EMMCHS26};
> +
> +  Host  = MmcHostInstance->MmcHost;
> +  if (MmcHostInstance->CardInfo.CSDData.SPEC_VERS < 4)
> +    return EFI_SUCCESS;
> +  ECSDData = &MmcHostInstance->CardInfo.ECSDData;
> +  if (ECSDData->DEVICE_TYPE == EMMCBACKWARD)
> +    return EFI_SUCCESS;

Always { and } (2X).

> +
> +  if (Host->SetIos) {

How about
  If (!Host->SetIos) {
    return EFI_SUCCESS;
  }
?

And then we can lose a level of indentation in the rest of the function.

> +    Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_HS_TIMING, EMMC_TIMING_HS);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to switch high speed mode, Status:%r.\n", Status));
> +      return Status;
> +    }
> +
> +    for (Idx = 0; Idx < 4; Idx++) {
> +      switch (TimingMode[Idx]) {
> +      case EMMCHS52DDR1V2:
> +      case EMMCHS52DDR1V8:
> +      case EMMCHS52:
> +        BusClockFreq = 52000000;
> +        break;
> +      case EMMCHS26:
> +        BusClockFreq = 26000000;
> +        break;
> +      default:
> +        return EFI_UNSUPPORTED;
> +      }
> +      Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);
> +      if (!EFI_ERROR (Status)) {
> +        Found = TRUE;
> +        break;
> +      }
> +    }
> +    if (Found) {
> +      Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
> +      if (EFI_ERROR (Status))
> +        DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));

{ and }

But also, just inserting this code in the "if (!EFI_ERROR (Status)) {"
statement and dropping the Found flag completely would be cleaner.

> +    }
> +  }
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
>  InitializeSdMmcDevice (
>    IN  MMC_HOST_INSTANCE   *MmcHostInstance
>    )
> @@ -429,11 +610,15 @@ InitializeMmcDevice (
>      return Status;
>    }
>  
> -  if (MmcHostInstance->CardInfo.CardType != EMMC_CARD) {
> -    Status = InitializeSdMmcDevice (MmcHostInstance);
> -    if (EFI_ERROR (Status)) {
> -      return Status;
> +  if (MmcHostInstance->CardInfo.CardType == EMMC_CARD) {

Flipping the comparison logic here makes the diff look a lot more
invasive than it is. While I'm not opposed to the change, could you
break it out as a separat patch if you want to keep it in?

> +    if (MmcHost->SetIos) {

This is being checked also in SetIos, so this added check can be
dropped.

> +      Status = InitializeEmmcDevice (MmcHostInstance);
>      }
> +  } else {
> +    Status = InitializeSdMmcDevice (MmcHostInstance);
> +  }
> +  if (EFI_ERROR (Status)) {
> +    return Status;
>    }
>  
>    // Set Block Length
> -- 
> 2.7.4
>