* [PATCH 1/2] MMC : Recieve response was missing after CMD12 @ 2017-08-30 14:20 Meenakshi Aggarwal 2017-08-30 14:20 ` [PATCH 2/2] SD : Updated CMD 6 implememtation Meenakshi Aggarwal ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Meenakshi Aggarwal @ 2017-08-30 14:20 UTC (permalink / raw) To: edk2-devel We are not recieving the response from memory card after sending CMD 12. It was not resulting in any failure but we should recieve response after sending a command. Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> --- EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c | 1 + 1 file changed, 1 insertion(+) diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c index 403db96..a2b9232 100644 --- a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c +++ b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c @@ -206,6 +206,7 @@ MmcTransferBlock ( if (EFI_ERROR (Status)) { DEBUG ((EFI_D_BLKIO, "%a(): Error and Status:%r\n", __func__, Status)); } + MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1b, Response); } Status = MmcNotifyState (MmcHostInstance, MmcTransferState); -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] SD : Updated CMD 6 implememtation. 2017-08-30 14:20 [PATCH 1/2] MMC : Recieve response was missing after CMD12 Meenakshi Aggarwal @ 2017-08-30 14:20 ` Meenakshi Aggarwal 2017-08-31 6:06 ` Meenakshi Aggarwal 2017-08-31 12:06 ` Leif Lindholm 2017-08-31 6:05 ` [PATCH 1/2] MMC : Recieve response was missing after CMD12 Meenakshi Aggarwal 2017-08-31 11:22 ` Leif Lindholm 2 siblings, 2 replies; 13+ messages in thread From: Meenakshi Aggarwal @ 2017-08-30 14:20 UTC (permalink / raw) To: edk2-devel 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. Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@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 ( + IN UINT8 Mode, + IN UINT8 Group, + IN UINT8 Value + ) +{ + UINT32 Argument; + + Argument = Mode << 31 | 0x00FFFFFF; + 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; 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); + 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)) { + 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 + /* 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)); 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)); + return Status; + } + + if ((Buffer[4] & 0x0f000000) != 0x01000000) { + 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)); 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)); 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)); return Status; } } + return EFI_SUCCESS; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] SD : Updated CMD 6 implememtation. 2017-08-30 14:20 ` [PATCH 2/2] SD : Updated CMD 6 implememtation Meenakshi Aggarwal @ 2017-08-31 6:06 ` Meenakshi Aggarwal 2017-08-31 12:06 ` Leif Lindholm 1 sibling, 0 replies; 13+ messages in thread From: Meenakshi Aggarwal @ 2017-08-31 6:06 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: Meenakshi Aggarwal, Ard Biesheuvel, leif.lindholm@linaro.org Hi, Please review this patch. Thanks & Regards, Meenakshi -----Original Message----- From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] Sent: Wednesday, August 30, 2017 7:51 PM To: edk2-devel@lists.01.org Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> Subject: [PATCH 2/2] SD : Updated CMD 6 implememtation. 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. Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@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 ( + IN UINT8 Mode, + IN UINT8 Group, + IN UINT8 Value + ) +{ + UINT32 Argument; + + Argument = Mode << 31 | 0x00FFFFFF; + 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; 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); + 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)) { + 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 + /* 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)); 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)); + return Status; + } + + if ((Buffer[4] & 0x0f000000) != 0x01000000) { + 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)); 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)); 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)); return Status; } } + return EFI_SUCCESS; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] SD : Updated CMD 6 implememtation. 2017-08-30 14:20 ` [PATCH 2/2] SD : Updated CMD 6 implememtation Meenakshi Aggarwal 2017-08-31 6:06 ` Meenakshi Aggarwal @ 2017-08-31 12:06 ` Leif Lindholm 2017-08-31 14:43 ` Jun Nie 1 sibling, 1 reply; 13+ messages in thread From: Leif Lindholm @ 2017-08-31 12:06 UTC (permalink / raw) To: Meenakshi Aggarwal; +Cc: edk2-devel, Jun Nie, Haojian Zhuang 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 <meenakshi.aggarwal@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? > + 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] SD : Updated CMD 6 implememtation. 2017-08-31 12:06 ` Leif Lindholm @ 2017-08-31 14:43 ` Jun Nie 2017-09-01 9:32 ` Meenakshi Aggarwal 0 siblings, 1 reply; 13+ messages in thread From: Jun Nie @ 2017-08-31 14:43 UTC (permalink / raw) To: Leif Lindholm, Meenakshi Aggarwal; +Cc: edk2-devel, Haojian Zhuang 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.aggarwal@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? > >> + 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] SD : Updated CMD 6 implememtation. 2017-08-31 14:43 ` Jun Nie @ 2017-09-01 9:32 ` Meenakshi Aggarwal 2017-09-04 8:37 ` Meenakshi Aggarwal 0 siblings, 1 reply; 13+ messages in thread From: Meenakshi Aggarwal @ 2017-09-01 9:32 UTC (permalink / raw) To: Jun Nie, Leif Lindholm; +Cc: edk2-devel@lists.01.org, Haojian Zhuang Hi Leif and Jun, Thanks for your review. My comments are inlined. Regards, Meenakshi > -----Original Message----- > From: Jun Nie [mailto:jun.nie@linaro.org] > Sent: Thursday, August 31, 2017 8:13 PM > To: Leif Lindholm <leif.lindholm@linaro.org>; Meenakshi Aggarwal > <meenakshi.aggarwal@nxp.com> > Cc: edk2-devel@lists.01.org; Haojian Zhuang <haojian.zhuang@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.aggarwal@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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] SD : Updated CMD 6 implememtation. 2017-09-01 9:32 ` Meenakshi Aggarwal @ 2017-09-04 8:37 ` Meenakshi Aggarwal 0 siblings, 0 replies; 13+ messages in thread From: Meenakshi Aggarwal @ 2017-09-04 8:37 UTC (permalink / raw) To: Jun Nie, Leif Lindholm Cc: edk2-devel@lists.01.org, Haojian Zhuang, Meenakshi Aggarwal 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-bounces@lists.01.org] On Behalf Of > Meenakshi Aggarwal > Sent: Friday, September 01, 2017 3:03 PM > To: Jun Nie <jun.nie@linaro.org>; Leif Lindholm <leif.lindholm@linaro.org> > Cc: edk2-devel@lists.01.org; Haojian Zhuang <haojian.zhuang@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.nie@linaro.org] > > Sent: Thursday, August 31, 2017 8:13 PM > > To: Leif Lindholm <leif.lindholm@linaro.org>; Meenakshi Aggarwal > > <meenakshi.aggarwal@nxp.com> > > Cc: edk2-devel@lists.01.org; Haojian Zhuang > > <haojian.zhuang@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.aggarwal@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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12 2017-08-30 14:20 [PATCH 1/2] MMC : Recieve response was missing after CMD12 Meenakshi Aggarwal 2017-08-30 14:20 ` [PATCH 2/2] SD : Updated CMD 6 implememtation Meenakshi Aggarwal @ 2017-08-31 6:05 ` Meenakshi Aggarwal 2017-08-31 11:22 ` Leif Lindholm 2 siblings, 0 replies; 13+ messages in thread From: Meenakshi Aggarwal @ 2017-08-31 6:05 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: Meenakshi Aggarwal, Ard Biesheuvel, leif.lindholm@linaro.org Hi, Please review this patch. Thanks & Regards, Meenakshi -----Original Message----- From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] Sent: Wednesday, August 30, 2017 7:51 PM To: edk2-devel@lists.01.org Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> Subject: [PATCH 1/2] MMC : Recieve response was missing after CMD12 We are not recieving the response from memory card after sending CMD 12. It was not resulting in any failure but we should recieve response after sending a command. Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> --- EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c | 1 + 1 file changed, 1 insertion(+) diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c index 403db96..a2b9232 100644 --- a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c +++ b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c @@ -206,6 +206,7 @@ MmcTransferBlock ( if (EFI_ERROR (Status)) { DEBUG ((EFI_D_BLKIO, "%a(): Error and Status:%r\n", __func__, Status)); } + MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1b, + Response); } Status = MmcNotifyState (MmcHostInstance, MmcTransferState); -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12 2017-08-30 14:20 [PATCH 1/2] MMC : Recieve response was missing after CMD12 Meenakshi Aggarwal 2017-08-30 14:20 ` [PATCH 2/2] SD : Updated CMD 6 implememtation Meenakshi Aggarwal 2017-08-31 6:05 ` [PATCH 1/2] MMC : Recieve response was missing after CMD12 Meenakshi Aggarwal @ 2017-08-31 11:22 ` Leif Lindholm 2017-08-31 13:33 ` Jun Nie 2 siblings, 1 reply; 13+ messages in thread From: Leif Lindholm @ 2017-08-31 11:22 UTC (permalink / raw) To: Meenakshi Aggarwal; +Cc: edk2-devel, Jun Nie, Haojian Zhuang On Wed, Aug 30, 2017 at 07:50:58PM +0530, Meenakshi Aggarwal wrote: > We are not recieving the response from memory card after > sending CMD 12. It was not resulting in any failure but > we should recieve response after sending a command. This looks sensible to me, but I'm not very familiar with MMC. Jun, Haojian - could you comment on this? / Leif > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c > index 403db96..a2b9232 100644 > --- a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c > +++ b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c > @@ -206,6 +206,7 @@ MmcTransferBlock ( > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_BLKIO, "%a(): Error and Status:%r\n", __func__, Status)); > } > + MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1b, Response); > } > > Status = MmcNotifyState (MmcHostInstance, MmcTransferState); > -- > 1.9.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12 2017-08-31 11:22 ` Leif Lindholm @ 2017-08-31 13:33 ` Jun Nie 2017-08-31 14:14 ` Meenakshi Aggarwal 2017-09-01 10:45 ` Leif Lindholm 0 siblings, 2 replies; 13+ messages in thread From: Jun Nie @ 2017-08-31 13:33 UTC (permalink / raw) To: Leif Lindholm, Meenakshi Aggarwal; +Cc: edk2-devel, Haojian Zhuang On 2017年08月31日 19:22, Leif Lindholm wrote: > On Wed, Aug 30, 2017 at 07:50:58PM +0530, Meenakshi Aggarwal wrote: >> We are not recieving the response from memory card after >> sending CMD 12. It was not resulting in any failure but >> we should recieve response after sending a command. Per spec, there is response data for CMD12. It is reasonable to read it. Reviewed-by: Jun Nie <jun.nie@linaro.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12 2017-08-31 13:33 ` Jun Nie @ 2017-08-31 14:14 ` Meenakshi Aggarwal 2017-09-01 10:45 ` Leif Lindholm 1 sibling, 0 replies; 13+ messages in thread From: Meenakshi Aggarwal @ 2017-08-31 14:14 UTC (permalink / raw) To: Jun Nie, Leif Lindholm; +Cc: edk2-devel@lists.01.org, Haojian Zhuang Hi Leif and Jun, Thanks for the review. -----Original Message----- From: Jun Nie [mailto:jun.nie@linaro.org] Sent: Thursday, August 31, 2017 7:03 PM To: Leif Lindholm <leif.lindholm@linaro.org>; Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> Cc: edk2-devel@lists.01.org; Haojian Zhuang <haojian.zhuang@linaro.org> Subject: Re: [edk2] [PATCH 1/2] MMC : Recieve response was missing after CMD12 On 2017年08月31日 19:22, Leif Lindholm wrote: > On Wed, Aug 30, 2017 at 07:50:58PM +0530, Meenakshi Aggarwal wrote: >> We are not recieving the response from memory card after sending CMD >> 12. It was not resulting in any failure but we should recieve >> response after sending a command. Per spec, there is response data for CMD12. It is reasonable to read it. Reviewed-by: Jun Nie <jun.nie@linaro.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12 2017-08-31 13:33 ` Jun Nie 2017-08-31 14:14 ` Meenakshi Aggarwal @ 2017-09-01 10:45 ` Leif Lindholm 2017-09-01 10:46 ` Meenakshi Aggarwal 1 sibling, 1 reply; 13+ messages in thread From: Leif Lindholm @ 2017-09-01 10:45 UTC (permalink / raw) To: Meenakshi Aggarwal; +Cc: edk2-devel, Haojian Zhuang, Jun Nie On Thu, Aug 31, 2017 at 09:33:08PM +0800, Jun Nie wrote: > On 2017年08月31日 19:22, Leif Lindholm wrote: > > On Wed, Aug 30, 2017 at 07:50:58PM +0530, Meenakshi Aggarwal wrote: > > > We are not recieving the response from memory card after > > > sending CMD 12. It was not resulting in any failure but > > > we should recieve response after sending a command. > > Per spec, there is response data for CMD12. It is reasonable to read it. > > Reviewed-by: Jun Nie <jun.nie@linaro.org> Thanks Jun for the review! Since this patch is valid on its own, I have pushed it as 1cc0f69bbe. No need to include in a v2 submission. However, I noticed after pushing (Doh! and for some reason my pre-push hook didn't catch this), that your submission was missing the required Contributed-under tag, as described by Contributions.txt. Can you confirm that you consider your contribution to be under this agreement, as well as ensure to include this in any further submissions? / Leif ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12 2017-09-01 10:45 ` Leif Lindholm @ 2017-09-01 10:46 ` Meenakshi Aggarwal 0 siblings, 0 replies; 13+ messages in thread From: Meenakshi Aggarwal @ 2017-09-01 10:46 UTC (permalink / raw) To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Haojian Zhuang, Jun Nie Hi Leif, Sorry I forgot to add the contribution agreement. Will make sure to add this in next patch. Yes, I agree to this. Regards, Meenakshi > -----Original Message----- > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > Sent: Friday, September 01, 2017 4:15 PM > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > Cc: edk2-devel@lists.01.org; Haojian Zhuang <haojian.zhuang@linaro.org>; > Jun Nie <jun.nie@linaro.org> > Subject: Re: [edk2] [PATCH 1/2] MMC : Recieve response was missing after > CMD12 > > On Thu, Aug 31, 2017 at 09:33:08PM +0800, Jun Nie wrote: > > On 2017年08月31日 19:22, Leif Lindholm wrote: > > > On Wed, Aug 30, 2017 at 07:50:58PM +0530, Meenakshi Aggarwal wrote: > > > > We are not recieving the response from memory card after sending > > > > CMD 12. It was not resulting in any failure but we should recieve > > > > response after sending a command. > > > > Per spec, there is response data for CMD12. It is reasonable to read it. > > > > Reviewed-by: Jun Nie <jun.nie@linaro.org> > > Thanks Jun for the review! > Since this patch is valid on its own, I have pushed it as 1cc0f69bbe. > No need to include in a v2 submission. > > However, I noticed after pushing (Doh! and for some reason my pre-push > hook didn't catch this), that your submission was missing the required > Contributed-under tag, as described by Contributions.txt. > > Can you confirm that you consider your contribution to be under this > agreement, as well as ensure to include this in any further submissions? > > / > Leif ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-09-04 8:34 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-30 14:20 [PATCH 1/2] MMC : Recieve response was missing after CMD12 Meenakshi Aggarwal 2017-08-30 14:20 ` [PATCH 2/2] SD : Updated CMD 6 implememtation Meenakshi Aggarwal 2017-08-31 6:06 ` Meenakshi Aggarwal 2017-08-31 12:06 ` Leif Lindholm 2017-08-31 14:43 ` Jun Nie 2017-09-01 9:32 ` Meenakshi Aggarwal 2017-09-04 8:37 ` Meenakshi Aggarwal 2017-08-31 6:05 ` [PATCH 1/2] MMC : Recieve response was missing after CMD12 Meenakshi Aggarwal 2017-08-31 11:22 ` Leif Lindholm 2017-08-31 13:33 ` Jun Nie 2017-08-31 14:14 ` Meenakshi Aggarwal 2017-09-01 10:45 ` Leif Lindholm 2017-09-01 10:46 ` Meenakshi Aggarwal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox