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?
+ 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
