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 <[email protected]>
> Tested-by: Ryan Harkin <[email protected]>
> ---
> 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
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
