Hi Jun,
I checked my host controller driver and i am taking care of its endianness in host controller driver itself. I researched further on the bit number I am checking for High speed in CMD6 data. I tried to find out in SD specs if response for CMD 6 comes it LE or BE format. I didn't find out anything mentioned directly for CMD6 but yes for ACMD51 (SCR command), it follows BE. We are reading only 8 bytes in SCR register to get SD version, while SD version comes in bit 59:56: SD Memory Card - Spec. Version SD_SPEC [59:56] I think similar stands true for CMD6 as well. Bit 512 is coming first on DATA line. So I am checking correct bits in patch. I have refer linux code also, there also they are considering bit 512 is coming first. Please comment. Thanks, Meenakshi > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Meenakshi Aggarwal > Sent: Friday, September 01, 2017 3:03 PM > To: Jun Nie <jun....@linaro.org>; Leif Lindholm <leif.lindh...@linaro.org> > Cc: edk2-devel@lists.01.org; Haojian Zhuang <haojian.zhu...@linaro.org> > Subject: Re: [edk2] [PATCH 2/2] SD : Updated CMD 6 implememtation. > > [This sender failed our fraud detection checks and may not be who they > appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing] > > Hi Leif and Jun, > > > Thanks for your review. > > > My comments are inlined. > > Regards, > Meenakshi > > > -----Original Message----- > > From: Jun Nie [mailto:jun....@linaro.org] > > Sent: Thursday, August 31, 2017 8:13 PM > > To: Leif Lindholm <leif.lindh...@linaro.org>; Meenakshi Aggarwal > > <meenakshi.aggar...@nxp.com> > > Cc: edk2-devel@lists.01.org; Haojian Zhuang > > <haojian.zhu...@linaro.org> > > Subject: Re: [edk2] [PATCH 2/2] SD : Updated CMD 6 implememtation. > > > > 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 <meenakshi.aggar...@nxp.com> > > >> --- > > >> 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? > > > > I will surely do this. > > > >> + 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? > > > > ok > > >> 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? > > > > Yes 64 is the number of bytes we want to read, and 0 is the block offset. > I will add a macro for size. > > > >> + 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". > > > Ah... You are correct, my SD host controller is Big Endian and so is the > difference, I will update the patch and soon send V2. > > > > > > Is there no struct available to access this information in more > > > human readable form? > > > > No ☹ > > > > And a #define for the 0x20000, please. > > > > For sure > > > >> + 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? > > > > Ok > > > >> + > > >> /* 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. > > > > Ok... will send a separate patch > > > >> 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[]. > > > Again... You are correct, I will update the patch and send V2 soon. > > > > > > Is there no struct available to access this information in more > > > human readable form? > > > > No > > > And a #define for both the magic values, please. > > > > Ok > > > >> + 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 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
