From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-x236.google.com (mail-pf0-x236.google.com [IPv6:2607:f8b0:400e:c00::236]) (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 7C28321EB529C for ; Thu, 31 Aug 2017 07:40:49 -0700 (PDT) Received: by mail-pf0-x236.google.com with SMTP id g13so2509651pfm.2 for ; Thu, 31 Aug 2017 07:43:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=WpW+BYYlTvtaB3Xln7fZoKuvuftHEOe3ej7M0Q90wYs=; b=CjBV6G9EUMIq7ScRxdUGj9fGLIMp0welZwlllu/ZfX7aWGQsZ9t8eYWf2SLJ4l/sU4 Hrbutgl0I8j0C3TmTpSlreZFaS4Egp/lnfvXRXGoKaR0mo0+4G5bugAxBZIlYEXudGyY 3Sf+yh7lzUCZDzxyIB7wU1FhwnjB8b7xPMCFE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=WpW+BYYlTvtaB3Xln7fZoKuvuftHEOe3ej7M0Q90wYs=; b=n+bZaYtd6/lSApWBi+1gW0/jBPNaYRWWn05STgnVtonWKnkQOzLh1sdwrIeDHrcc9Y +3o/+DG03V9PGlt3y7Wer6DzY/3EtaanhlgjQoBrd0NiSos7A3D2Mxydi1Wmvnl7NLBL GhzcQWtmqTB79xQNFxTCovTPOdiZuoR4+Bm/JvQWnj0PM7KQDpeV9K2APTiozWVifugn X7hdHAEYtDaWDKhUm/lsZmnnzLX3+OHxrGA8u0VSCQZz6Jp2O0CcqJL2VqSS/+0TKpSj MqDh1k2ela+Yu0cUupf9d2Rc1usFV+x3UbopLSJDtQvGSTvxWi17WLZbHPtpv6uWeoL3 A2Wg== X-Gm-Message-State: AHYfb5g0zo//VC23hEmMK4HxEMUbyFz/WQiquxYF0CVt0U9c+8a39TQT Tup39aKnd5GsDtFt X-Google-Smtp-Source: ADKCNb4iEG9FChkN1WvVO1hBzwOZULnXOZMpBl2K1UlqonPmdEPao3sxW9KQ97KN4R7EzjnaHRgcZg== X-Received: by 10.84.175.195 with SMTP id t61mr2848217plb.254.1504190612007; Thu, 31 Aug 2017 07:43:32 -0700 (PDT) Received: from [10.56.6.22] ([113.53.228.63]) by smtp.googlemail.com with ESMTPSA id o79sm13777479pfi.98.2017.08.31.07.43.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 31 Aug 2017 07:43:30 -0700 (PDT) To: Leif Lindholm , Meenakshi Aggarwal Cc: edk2-devel@lists.01.org, Haojian Zhuang References: <1504102859-13477-1-git-send-email-meenakshi.aggarwal@nxp.com> <1504102859-13477-2-git-send-email-meenakshi.aggarwal@nxp.com> <20170831120614.zhnmv34s4wuvsub2@bivouac.eciton.net> From: Jun Nie Message-ID: Date: Thu, 31 Aug 2017 22:43:24 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170831120614.zhnmv34s4wuvsub2@bivouac.eciton.net> 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 14:40:49 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit On 2017年08月31日 20:06, Leif Lindholm wrote: > 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. Good catch, check should be done before setting function. And the setting result should be checked before return. Logic is correct in this patch. > > 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); A SD_MODE_CHECK/GET macro is clearer than 0 and 1 value for Mode. >> + 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)) { Bit 401 is HS support status. So bit in Buffer[12] should be tested. Or I miss anything? I am checking "SD Specifications Part 1 Physical Layer Specification Version 2.00". > > 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) { HS function busy status is bit 287:272 in response, bit 273 actually. Bit 379:376 is error status or function number if no error. So I guess you should test bit in other two elements of Buffer[]. > > 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