From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x230.google.com (mail-wm0-x230.google.com [IPv6:2a00:1450:400c:c09::230]) (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 6BC4221EB5283 for ; Thu, 31 Aug 2017 05:03:36 -0700 (PDT) Received: by mail-wm0-x230.google.com with SMTP id v2so3396698wmf.0 for ; Thu, 31 Aug 2017 05:06:19 -0700 (PDT) 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=+/EAJJISA5Vz+NTOFPPZpr2ZuBncOzJy0g+F7Dh5GTg=; b=UhmBlR7b/tVPaIA3CrsQvhJwypKTNAHetYyAtbcBmdlqr9Tevu61XffwfCi24qPG6r eypkrfepxi2Bd/ywXEoKccJ3YIb/Aw99UOwmEacGyEmNjR6o6gX6ZhvTnEZ49hB3OA09 /kUU0QRDIWtTouprws8NO5PNuYemHqRV5wnA4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=+/EAJJISA5Vz+NTOFPPZpr2ZuBncOzJy0g+F7Dh5GTg=; b=JwReHUHnyvkTt7s5QpNQI6XtK+Wki5Hr3yrVb1DA/vXA5lliRWz9hplF8moY/z6alm T1nSZC3RUVQoCl5lux7PXslhXAapA9dXZbinx6ZzqN010EfOQzRG90EayZ5eispVVuJA +6sHWbtYh1RnSZOnrDy17QSzMaLvutAMmdfIIKm8AWtmye7LxXsK0Ai1KBmMzbSzuyVb /yZvVNVeqWsnASKpbeJfqm6RyTbseDtdFm4Tkl3PhIYYZWtdQveaXCjbEXCaAyozlWvz tGDPEe3ckiTBPsuxvqztfUdnhhOrbPhjR2scLXnk9u7E8XPTGJ/c09+rc973DMxkWrwk 64wg== X-Gm-Message-State: AHPjjUgSUz08urSwr/fdphvHRiY5l/lg4Tns++p0QV4kQxx2wH+CTREF 0VgiYb1T0sBuYdhD X-Received: by 10.28.55.16 with SMTP id e16mr447831wma.142.1504181178278; Thu, 31 Aug 2017 05:06:18 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id k10sm3502631wre.38.2017.08.31.05.06.16 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 31 Aug 2017 05:06:17 -0700 (PDT) Date: Thu, 31 Aug 2017 13:06:14 +0100 From: Leif Lindholm To: Meenakshi Aggarwal Cc: edk2-devel@lists.01.org, Jun Nie , Haojian Zhuang Message-ID: <20170831120614.zhnmv34s4wuvsub2@bivouac.eciton.net> References: <1504102859-13477-1-git-send-email-meenakshi.aggarwal@nxp.com> <1504102859-13477-2-git-send-email-meenakshi.aggarwal@nxp.com> MIME-Version: 1.0 In-Reply-To: <1504102859-13477-2-git-send-email-meenakshi.aggarwal@nxp.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH 2/2] SD : Updated CMD 6 implememtation. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Aug 2017 12:03:36 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Aug 30, 2017 at 07:50:59PM +0530, Meenakshi Aggarwal wrote: > For setting high speed in SD card, > First CMD 6 (Switch) is send to check if card supports High Speed and > Second command is send to switch card to high speed mode. > > In current inplementation, CMD 6 was sent only once to switch the > card into HS mode without checking if card supports HS or not, which is > not as per specification and also we are not setting the HS i.e. 50000000 > but directly asking the card to switch to 26000000 which is incorrect as > SD card supports either 25000000 or 50000000. Same as previous one: Jun, Haojian? I do have a couple of style comments below. > Signed-off-by: Meenakshi Aggarwal > --- > EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 64 ++++++++++++++++++++---- > 1 file changed, 55 insertions(+), 9 deletions(-) > > diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c > index 7f74c54..3071b3b 100644 > --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c > +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c > @@ -317,6 +317,24 @@ InitializeEmmcDevice ( > return Status; > } > > + > +STATIC > +UINT32 > +CreateSwitchCmdArgument ( This helper function is a good addition, thanks. > + IN UINT8 Mode, > + IN UINT8 Group, > + IN UINT8 Value > + ) > +{ > + UINT32 Argument; > + > + Argument = Mode << 31 | 0x00FFFFFF; Just because I hate implicit type promotion, could you make Mode UINT32 in the input, please? > + Argument &= ~(0xF << (Group * 4)); > + Argument |= Value << (Group * 4); > + > + return Argument; > +} > + > STATIC > EFI_STATUS > InitializeSdMmcDevice ( > @@ -326,6 +344,7 @@ InitializeSdMmcDevice ( > UINT32 CmdArg; > UINT32 Response[4]; > UINT32 Buffer[128]; > + UINT32 Speed; > UINTN BlockSize; > UINTN CardSize; > UINTN NumBlocks; > @@ -334,6 +353,7 @@ InitializeSdMmcDevice ( > EFI_STATUS Status; > EFI_MMC_HOST_PROTOCOL *MmcHost; > > + Speed = 25000000; Could this be given a #define with a descriptive name, in Mmc.h? > MmcHost = MmcHostInstance->MmcHost; > > // Send a command to get Card specific data > @@ -439,43 +459,69 @@ InitializeSdMmcDevice ( > } > } > if (CccSwitch) { > + /* SD Switch, Mode:0, Group:0, Value:0 */ > + CmdArg = CreateSwitchCmdArgument(0, 0, 0); > + Status = MmcHost->SendCommand (MmcHost, MMC_CMD6, CmdArg); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Failed with Status = %r\n", __func__, Status)); > + return Status; > + } else { > + Status = MmcHost->ReadBlockData (MmcHost, 0, 64, Buffer); What are 0 and 64? I guess 64 is a size? Is there a #define or a sizeof() that could make it more descriptive? > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): ReadBlockData Failed with Status = %r\n", __func__, Status)); > + return Status; > + } > + } > + > + if (!(Buffer[3] & 0x20000)) { Is there no struct available to access this information in more human readable form? And a #define for the 0x20000, please. > + DEBUG ((EFI_D_ERROR, "%aHigh Speed not supported by Card %r\n", __func__, Status)); > + return Status; > + } > + > + Speed = 50000000; //High Speed for SD card is 50 MHZ Could this be given a #define with a descriptive name, in Mmc.h? > + > /* SD Switch, Mode:1, Group:0, Value:1 */ > - CmdArg = 1 << 31 | 0x00FFFFFF; > - CmdArg &= ~(0xF << (0 * 4)); > - CmdArg |= 1 << (0 * 4); > + CmdArg = CreateSwitchCmdArgument(1, 0, 1); > Status = MmcHost->SendCommand (MmcHost, MMC_CMD6, CmdArg); > if (EFI_ERROR (Status)) { > - DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", Status)); > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", __func__, Status)); This looks like an unrelated bugfix? It is good, and thank you, but could you break it out into its own patch please? Also, __FUNCTION__ matches the coding style better (I know we have both, but __func__ appears to be losing, and I would like to keep that momentum up. > 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)); > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): ReadBlockData Error and Status = %r\n",__func__, Status)); Unrelated bugfix (same as comment above, and same patch please). > + return Status; > + } > + > + if ((Buffer[4] & 0x0f000000) != 0x01000000) { Is there no struct available to access this information in more human readable form? And a #define for both the magic values, please. > + DEBUG((EFI_D_ERROR, "Problem switching SD card into high-speed mode\n")); > 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)); > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD55): Error and Status = %r\n", __func__, Status)); Unrelated bugfix (same as comment above, and same patch please). > 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)); > + DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", __func__, Status)); Unrelated bugfix (same as comment above, and same patch please). > return Status; > } > } > if (MMC_HOST_HAS_SETIOS(MmcHost)) { > - Status = MmcHost->SetIos (MmcHost, 26 * 1000 * 1000, 4, EMMCBACKWARD); > + Status = MmcHost->SetIos (MmcHost, Speed, 4, EMMCBACKWARD); > if (EFI_ERROR (Status)) { > - DEBUG ((EFI_D_ERROR, "%a(SetIos): Error and Status = %r\n", Status)); > + DEBUG ((EFI_D_ERROR, "%a(SetIos): Error and Status = %r\n", __func__, Status)); Unrelated bugfix (same as comment above, and same patch please). / Leif > return Status; > } > } > + > return EFI_SUCCESS; > } > > -- > 1.9.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel