From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 527D581D96 for ; Mon, 14 Nov 2016 09:08:15 -0800 (PST) Received: by mail-wm0-x233.google.com with SMTP id t79so110288693wmt.0 for ; Mon, 14 Nov 2016 09:08:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=qc3gHteu9Yf05aOR+SYe2I0zQCP1DCkOoXvlY6uwJsk=; b=BbXp9KHpUGpFPvr42XQWrFRS2UqkKO8pJlC9yA+iHSNgDkH2ci0JCgs5FiXwHoRG91 vOvjDmgXuzCUaQb+hmveI0ifc6uTjbAGq4xihpF3zbmnu0BZ4i5j3VE3+0TsA6Sk3Li7 xrksgP2HhOf2jFsqtMS1i2MRMTqtftwJXjoAw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=qc3gHteu9Yf05aOR+SYe2I0zQCP1DCkOoXvlY6uwJsk=; b=HL+PCUzKUy/yQC0GGhvNG9BoXdp7qWUBTx5lIBc4tTjVXnHwLOU6S5nRwrj7Wr2iO/ CtAbCn0FFOSuplPZh1ZS5rQPWOiZ8aXx+4TirHskuF54iD15BUvnfIOomLD0e6e/Woq2 gKeA/o2McZnfsreovdX99Yg5oP9w/b+DVdODiCQitNg7ILbwHp2IBtHWhnlYWtdqSCih I79JrYnJCdaBvnOk/YilraRZbaSKF5drNPCdecDh7QMrtbZraiiDjoClEjFch1Lf8Bqi 2gBVF/YZIgo2AWLwBoewKycrrGcrsmz3xNXmdcAXJwPlIhsn2UK7q1FK4HEpz8zkIdtF IQHg== X-Gm-Message-State: ABUngvdkQS0fuaDGwz9+KMy9D4HKeGDzPuhrfWfSv29xvJBfJ1Qo5VeAGW4Lwt9+qbgbEoy7 X-Received: by 10.194.124.100 with SMTP id mh4mr11045361wjb.154.1479143298439; Mon, 14 Nov 2016 09:08:18 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id c202sm49712730wme.1.2016.11.14.09.08.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Nov 2016 09:08:17 -0800 (PST) Date: Mon, 14 Nov 2016 17:08:15 +0000 From: Leif Lindholm To: Haojian Zhuang Cc: ryan.harkin@linaro.org, edk2-devel@lists.01.org, ard.biesheuvel@linaro.org Message-ID: <20161114170815.GZ27644@bivouac.eciton.net> References: <1479019678-12621-1-git-send-email-haojian.zhuang@linaro.org> <1479019678-12621-8-git-send-email-haojian.zhuang@linaro.org> MIME-Version: 1.0 In-Reply-To: <1479019678-12621-8-git-send-email-haojian.zhuang@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v5 7/9] MmcDxe: set iospeed and bus width in SD stack X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Nov 2016 17:08:15 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Nov 13, 2016 at 02:47:56PM +0800, Haojian Zhuang wrote: > Add more SD commands to support 4-bit bus width & iospeed. It's not > formal code. And it needs to be updated later. Little bit concerned about this statement. Is it outdated information? If so, can you update commit message. If not, can you specify which bits are missing? > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Haojian Zhuang > Tested-by: Ryan Harkin > --- > EmbeddedPkg/Include/Protocol/MmcHost.h | 3 + > EmbeddedPkg/Universal/MmcDxe/Mmc.h | 17 +++ > EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 138 ++++++++++++++++++++--- > 3 files changed, 142 insertions(+), 16 deletions(-) > > diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h > index 01c0bf4..5028045 100644 > --- a/EmbeddedPkg/Include/Protocol/MmcHost.h > +++ b/EmbeddedPkg/Include/Protocol/MmcHost.h > @@ -64,11 +64,14 @@ typedef UINT32 MMC_CMD; > #define MMC_CMD24 (MMC_INDX(24) | MMC_CMD_WAIT_RESPONSE) > #define MMC_CMD55 (MMC_INDX(55) | MMC_CMD_WAIT_RESPONSE) > #define MMC_ACMD41 (MMC_INDX(41) | MMC_CMD_WAIT_RESPONSE | MMC_CMD_NO_CRC_RESPONSE) > +#define MMC_ACMD51 (MMC_INDX(51) | MMC_CMD_WAIT_RESPONSE) > > // Valid responses for CMD1 in eMMC > #define EMMC_CMD1_CAPACITY_LESS_THAN_2GB 0x00FF8080 // Capacity <= 2GB, byte addressing used > #define EMMC_CMD1_CAPACITY_GREATER_THAN_2GB 0x40FF8080 // Capacity > 2GB, 512-byte sector addressing used > > +#define MMC_STATUS_APP_CMD (1 << 5) > + > typedef enum _MMC_STATE { > MmcInvalidState = 0, > MmcHwInitializationState, > diff --git a/EmbeddedPkg/Universal/MmcDxe/Mmc.h b/EmbeddedPkg/Universal/MmcDxe/Mmc.h > index 67f449c..f054bb2 100644 > --- a/EmbeddedPkg/Universal/MmcDxe/Mmc.h > +++ b/EmbeddedPkg/Universal/MmcDxe/Mmc.h > @@ -75,6 +75,23 @@ typedef struct { > UINT32 PowerUp: 1; // This bit is set to LOW if the card has not finished the power up routine > } OCR; > > +/* For little endian CPU */ All formally supported architectures are little-endian, so I guess this is OK for now. Don't know if we have a good portable macro to use for detection anyway. > +typedef struct { > + UINT8 SD_SPEC: 4; // SD Memory Card - Spec. Version [59:56] > + UINT8 SCR_STRUCTURE: 4; // SCR Structure [63:60] > + UINT8 SD_BUS_WIDTHS: 4; // DAT Bus widths supported [51:48] > + UINT8 DATA_STAT_AFTER_ERASE: 1; // Data Status after erases [55] > + UINT8 SD_SECURITY: 3; // CPRM Security Support [54:52] > + UINT8 EX_SECURITY_1: 1; // Extended Security Support [43] > + UINT8 SD_SPEC4: 1; // Spec. Version 4.00 or higher [42] > + UINT8 RESERVED_1: 2; // Reserved [41:40] > + UINT8 SD_SPEC3: 1; // Spec. Version 3.00 or higher [47] > + UINT8 EX_SECURITY_2: 3; // Extended Security Support [46:44] > + UINT8 CMD_SUPPORT: 4; // Command Support bits [35:32] > + UINT8 RESERVED_2: 4; // Reserved [39:36] > + UINT32 RESERVED_3; // Manufacturer Usage [31:0] > +} SCR; > + > typedef struct { > UINT32 NOT_USED; // 1 [0:0] > UINT32 CRC; // CRC7 checksum [7:1] > diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c > index 9a79767..c469dda 100644 > --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c > +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c > @@ -12,6 +12,9 @@ > * > **/ > > +#include > +#include > + > #include "Mmc.h" > > typedef union { > @@ -41,6 +44,11 @@ typedef union { > > #define EMMC_SWITCH_ERROR (1 << 7) > > +#define SD_BUS_WIDTH_1BIT (1 << 0) > +#define SD_BUS_WIDTH_4BIT (1 << 2) > + > +#define SD_CCC_SWITCH (1 << 10) > + > #define DEVICE_STATE(x) (((x) >> 9) & 0xf) > typedef enum _EMMC_DEVICE_STATE { > EMMC_IDLE_STATE = 0, > @@ -68,28 +76,30 @@ EmmcGetDeviceState ( > { > EFI_MMC_HOST_PROTOCOL *Host; > EFI_STATUS Status; > - UINT32 Data, RCA; > + UINT32 Rsp[4], RCA; > > if (State == NULL) > return EFI_INVALID_PARAMETER; > > Host = MmcHostInstance->MmcHost; > 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); > + do { > + 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, Rsp); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to get response of CMD13, Status=%r.\n", Status)); > + return Status; > + } > + if (Rsp[0] & EMMC_SWITCH_ERROR) { > + DEBUG ((EFI_D_ERROR, "EmmcGetDeviceState(): Failed to switch expected mode, Status=%r.\n", Status)); > + return EFI_DEVICE_ERROR; > + } > + } while (!(Rsp[0] & MMC_R0_READY_FOR_DATA)); > + *State = MMC_R0_CURRENTSTATE(Rsp); These changes do not look directly related to the stated purpose of the patch? Should it be a separate one? > return EFI_SUCCESS; > } > > @@ -300,9 +310,12 @@ InitializeSdMmcDevice ( > { > UINT32 CmdArg; > UINT32 Response[4]; > + UINT32 Buffer[128]; > UINTN BlockSize; > UINTN CardSize; > UINTN NumBlocks; > + BOOLEAN CccSwitch; > + SCR Scr; > EFI_STATUS Status; > EFI_MMC_HOST_PROTOCOL *MmcHost; > > @@ -323,6 +336,10 @@ InitializeSdMmcDevice ( > return Status; > } > PrintCSD (Response); > + if (MMC_CSD_GET_CCC(Response) & SD_CCC_SWITCH) > + CccSwitch = TRUE; > + else > + CccSwitch = FALSE; > > if (MmcHostInstance->CardInfo.CardType == SD_CARD_2_HIGH) { > CardSize = HC_MMC_CSD_GET_DEVICESIZE (Response); > @@ -353,6 +370,95 @@ InitializeSdMmcDevice ( > return Status; > } > > + Status = MmcHost->SendCommand (MmcHost, MMC_CMD55, CmdArg); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD55): Error and Status = %r\n", Status)); > + return Status; > + } > + Status = MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD55): Error and Status = %r\n", Status)); > + return Status; > + } > + if ((Response[0] & MMC_STATUS_APP_CMD) == 0) > + return EFI_SUCCESS; > + > + /* SCR */ > + Status = MmcHost->SendCommand (MmcHost, MMC_ACMD51, 0); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_ACMD51): Error and Status = %r\n", __func__, Status)); > + return Status; > + } else { > + Status = MmcHost->ReadBlockData (MmcHost, 0, 8, Buffer); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_ACMD51): ReadBlockData Error and Status = %r\n", __func__, Status)); > + return Status; > + } > + CopyMem (&Scr, Buffer, 8); > + if (Scr.SD_SPEC == 2) { > + if (Scr.SD_SPEC3 == 1) { > + if (Scr.SD_SPEC4 == 1) { > + DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 4.xx\n")); > + } else { > + DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 3.0x\n")); > + } > + } else { > + if (Scr.SD_SPEC4 == 0) { > + DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 2.0\n")); > + } else { > + DEBUG ((EFI_D_ERROR, "Found invalid SD Card\n")); > + } > + } > + } else { > + if ((Scr.SD_SPEC3 == 0) && (Scr.SD_SPEC4 == 0)) { > + if (Scr.SD_SPEC == 1) { > + DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 1.10\n")); > + } else { > + DEBUG ((EFI_D_INFO, "Found SD Card for Spec Version 1.0\n")); > + } > + } else { > + DEBUG ((EFI_D_ERROR, "Found invalid SD Card\n")); > + } > + } > + } > + if (CccSwitch) { > + /* SD Switch, Mode:1, Group:0, Value:1 */ > + CmdArg = 1 << 31 | 0x00FFFFFF; > + CmdArg &= ~(0xF << (0 * 4)); > + CmdArg |= 1 << (0 * 4); Can we have some defines to clean the above up? > + Status = MmcHost->SendCommand (MmcHost, MMC_CMD6, CmdArg); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", Status)); > + return Status; > + } else { > + Status = MmcHost->ReadBlockData (MmcHost, 0, 64, Buffer); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): ReadBlockData Error and Status = %r\n", Status)); > + return Status; > + } > + } > + } > + if (Scr.SD_BUS_WIDTHS & SD_BUS_WIDTH_4BIT) { > + CmdArg = MmcHostInstance->CardInfo.RCA << 16; > + Status = MmcHost->SendCommand (MmcHost, MMC_CMD55, CmdArg); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD55): Error and Status = %r\n", Status)); > + return Status; > + } > + /* Width: 4 */ > + Status = MmcHost->SendCommand (MmcHost, MMC_CMD6, 2); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", Status)); > + return Status; > + } > + } > + if (MmcHost->SetIos) { > + Status = MmcHost->SetIos (MmcHost, 24 * 1000 * 1000, 4, EMMCBACKWARD); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(SetIos): Error and Status = %r\n", Status)); > + return Status; > + } > + } > return EFI_SUCCESS; > } > > -- > 2.7.4 >