From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22a.google.com (mail-wm0-x22a.google.com [IPv6:2a00:1450:400c:c09::22a]) (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 8575481E10 for ; Mon, 14 Nov 2016 08:36:48 -0800 (PST) Received: by mail-wm0-x22a.google.com with SMTP id f82so108821274wmf.1 for ; Mon, 14 Nov 2016 08:36:53 -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=W0D8Yfsx1h+zZKhholTDz1cdOgF3Of1SWFfNxHXDgpw=; b=FBM7bv7lqABARAbowPWz9p83mj9HxZjWTXUJAuClRuHibn6I5hluo4kyApQdxiZI9V O6Dqop+cQ2YpaAj5VNIzHtYVqJMs75ZyEXJMuKB1ms0D7yr0rfP2Vp5Vp+o4wAVHMlI8 yXzJwFqagjmdUjTzySz9OhFDu14BumUjbE8tY= 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=W0D8Yfsx1h+zZKhholTDz1cdOgF3Of1SWFfNxHXDgpw=; b=QYJ+AZpyJs7jlRx65BxdxskVbwzbPDAvWd5FbrJzHkxMvIGjgbvDMTO3WuI47pUlEO gUwl64Jc/nwArQrsiYFl8+6JVPPtHWfL9Nm5+x0Xw4QKsFt/SVXdKta2+4jEQgTqznbC 7uoq0TIoN63Wr3jOaLbMujm+sXrQEgmZIrv4L32SJpNZInDvWWsb6+DkptwptBHg/Gmu vEAZ1//muNEYqiPWXQwxtNiL7U/aDvzK9udVFRoZqKJF/Wewcpj6nLZK/K56z+cECVOi 4+F0p4LaG2OcWNHcsqvdbO33IpGqJTWYgM40krb2UUJP02t6AafdCmYcy6A/vvjv852X FUVA== X-Gm-Message-State: ABUngvc65wowRWhmF81qFwCbvCapoTbA2pAn3wtGFiRY7WJ0sOvdmqSfzicZOrZVxgdEs6G3 X-Received: by 10.28.37.2 with SMTP id l2mr6922860wml.86.1479141411623; Mon, 14 Nov 2016 08:36:51 -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 d10sm13752306wja.20.2016.11.14.08.36.50 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Nov 2016 08:36:51 -0800 (PST) Date: Mon, 14 Nov 2016 16:36:49 +0000 From: Leif Lindholm To: Haojian Zhuang Cc: ryan.harkin@linaro.org, edk2-devel@lists.01.org, ard.biesheuvel@linaro.org Message-ID: <20161114163649.GY27644@bivouac.eciton.net> References: <1479019678-12621-1-git-send-email-haojian.zhuang@linaro.org> <1479019678-12621-7-git-send-email-haojian.zhuang@linaro.org> MIME-Version: 1.0 In-Reply-To: <1479019678-12621-7-git-send-email-haojian.zhuang@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v5 6/9] MmcDxe: set io bus width before reading EXTCSD 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 16:36:49 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > Tested-by: Ryan Harkin > --- > 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 >