[PATCH 1/2] MMC : Recieve response was missing after CMD12

[PATCH 1/2] MMC : Recieve response was missing after CMD12

[Meenakshi Aggarwal]

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

[PATCH 2/2] SD : Updated CMD 6 implememtation.

[Meenakshi Aggarwal]

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

Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12

[Meenakshi Aggarwal]

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

Re: [PATCH 2/2] SD : Updated CMD 6 implememtation.

[Meenakshi Aggarwal]

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

Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12

[Leif Lindholm]

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

Re: [PATCH 2/2] SD : Updated CMD 6 implememtation.

[Leif Lindholm]

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

Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12

[Jun Nie]

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>

Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12

[Meenakshi Aggarwal]

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>

Re: [PATCH 2/2] SD : Updated CMD 6 implememtation.

[Jun Nie]

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

Re: [PATCH 2/2] SD : Updated CMD 6 implememtation.

[Meenakshi Aggarwal]

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

Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12

[Leif Lindholm]

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

Re: [PATCH 1/2] MMC : Recieve response was missing after CMD12

[Meenakshi Aggarwal]

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

Re: [PATCH 2/2] SD : Updated CMD 6 implememtation.

[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