[[PATCH v2] 0/7] Fix DwEmmc driver bugs

[[PATCH v2] 0/7] Fix DwEmmc driver bugs

[Loh, Tien Hock]

From: "Tien Hock, Loh" <tien.hock.loh@intel.com>

This fixes a few issues with DwEmmc driver:
- - Change SendCommand polling mode and check for DTO when read/write
  - Add CMD6 for SD support
  - Clear CTYPE on initialization
  - Add CMD8 support for SD
  - Add ACMD51 for SD
  - Call send command when receive response is called
  - Add check CRC response only when MMC command supports it
  - Add support for reading data less than DWEMMC_BLOCK_SIZE

Tien Hock, Loh (7):
  EmbeddedPkg: Fix DwEmmc driver bugs
  EmbeddedPkg: Fix DwEmmc driver bugs
  EmbeddedPkg: Fix DwEmmc driver bugs
  EmbeddedPkg: Fix DwEmmc driver bugs
  EmbeddedPkg: Fix DwEmmc driver bugs
  EmbeddedPkg: Fix DwEmmc driver bugs
  EmbeddedPkg: Fix DwEmmc driver bugs

 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c        | 62 +++++++++++++++++++++---
 EmbeddedPkg/Include/Protocol/MmcHost.h           |  1 +
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c |  2 +
 3 files changed, 57 insertions(+), 8 deletions(-)

-- 
2.2.2

[[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

From: "Tien Hock, Loh" <tien.hock.loh@intel.com>

Added ACMD6 for SD support. For SD, after CMD55 is sent, the next command
should be an application command, which should not expect data

Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index adc6b06..fa24802 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -39,6 +39,7 @@ DWEMMC_IDMAC_DESCRIPTOR   *gpIdmacDesc;
 EFI_GUID mDwEmmcDevicePathGuid = EFI_CALLER_ID_GUID;
 STATIC UINT32 mDwEmmcCommand;
 STATIC UINT32 mDwEmmcArgument;
+STATIC BOOLEAN mIsACmd = FALSE;
 
 EFI_STATUS
 DwEmmcReadBlockData (
@@ -321,6 +322,15 @@ DwEmmcSendCommand (
     Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
            BIT_CMD_SEND_INIT;
     break;
+  case MMC_INDX(6):
+    if(mIsACmd) {
+      Cmd = BIT_CMD_RESPONSE_EXPECT ;
+    }
+    else {
+      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_DATA_EXPECTED |
+            BIT_CMD_READ;
+    }
+    break;
   case MMC_INDX(7):
     if (Argument)
         Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC;
@@ -371,6 +381,11 @@ DwEmmcSendCommand (
   }
 
   Cmd |= MMC_GET_INDX(MmcCmd) | BIT_CMD_USE_HOLD_REG | BIT_CMD_START;
+
+  if(MMC_INDX(55) == MMC_GET_INDX(MmcCmd))
+    mIsACmd = TRUE;
+  else
+    mIsACmd = FALSE;
   if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) {
     mDwEmmcCommand = Cmd;
     mDwEmmcArgument = Argument;
-- 
2.2.2

[[PATCH v2] 2/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

From: "Tien Hock, Loh" <tien.hock.loh@intel.com>

Clear CTYPE on initialization. This is important if previous bootloader
changes the CTYPE can cause the controller to not initialize correctly
if CTYPE is not reset to 0

Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index fa24802..058665b 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -206,6 +206,7 @@ DwEmmcNotifyState (
     MmioWrite32 (DWEMMC_TMOUT, ~0);
     MmioWrite32 (DWEMMC_IDINTEN, 0);
     MmioWrite32 (DWEMMC_BMOD, DWEMMC_IDMAC_SWRESET);
+    MmioWrite32 (DWEMMC_CTYPE, 0);
 
     MmioWrite32 (DWEMMC_BLKSIZ, DWEMMC_BLOCK_SIZE);
     do {
-- 
2.2.2

[[PATCH v2] 3/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

From: "Tien Hock, Loh" <tien.hock.loh@intel.com>

On CMD8, for SD, the controller should not expect data as this is a
SEND_IF_COND command to verify SD operating condition, and does not have
data

Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c        | 9 ++++++---
 EmbeddedPkg/Include/Protocol/MmcHost.h           | 1 +
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 2 ++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index 058665b..04fdcbf 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -339,9 +339,12 @@ DwEmmcSendCommand (
         Cmd = 0;
     break;
   case MMC_INDX(8):
-    Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
-           BIT_CMD_DATA_EXPECTED | BIT_CMD_READ |
-           BIT_CMD_WAIT_PRVDATA_COMPLETE;
+    if (!This->IsEmmc)
+      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
+             BIT_CMD_WAIT_PRVDATA_COMPLETE ;
+    else
+      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
+             BIT_CMD_WAIT_PRVDATA_COMPLETE | BIT_CMD_READ | BIT_CMD_DATA_EXPECTED;
     break;
   case MMC_INDX(9):
     Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
index 9e07082..ae8ea5d 100644
--- a/EmbeddedPkg/Include/Protocol/MmcHost.h
+++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
@@ -169,6 +169,7 @@ struct _EFI_MMC_HOST_PROTOCOL {
   MMC_SETIOS              SetIos;
   MMC_ISMULTIBLOCK        IsMultiBlock;
 
+  BOOLEAN                 IsEmmc;
 };
 
 #define MMC_HOST_PROTOCOL_REVISION    0x00010002    // 1.2
diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index 4dc0be1..fa1eda2 100755
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -770,8 +770,10 @@ InitializeMmcDevice (
   }
 
   if (MmcHostInstance->CardInfo.CardType != EMMC_CARD) {
+    MmcHostInstance->MmcHost->IsEmmc = FALSE;
     Status = InitializeSdMmcDevice (MmcHostInstance);
   } else {
+    MmcHostInstance->MmcHost->IsEmmc = TRUE;
     Status = InitializeEmmcDevice (MmcHostInstance);
   }
   if (EFI_ERROR (Status)) {
-- 
2.2.2

[[PATCH v2] 4/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

From: "Tien Hock, Loh" <tien.hock.loh@intel.com>

Add ACMD51 support for SD, this is needed to do SEND_SCR command correctly

Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index 04fdcbf..32572a9 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -379,6 +379,10 @@ DwEmmcSendCommand (
     Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
            BIT_CMD_DATA_EXPECTED;
     break;
+  case MMC_INDX(51):
+    Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_DATA_EXPECTED |
+           BIT_CMD_READ | BIT_CMD_WAIT_PRVDATA_COMPLETE;
+    break;
   default:
     Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC;
     break;
-- 
2.2.2

[[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

From: "Tien Hock, Loh" <tien.hock.loh@intel.com>

Send command when MMC ask for response in DwEmmcReceiveResponse, and
command is a pending command (eg. DMA needs to be set up first)

Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index 32572a9..a69d9ab 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -398,8 +398,11 @@ DwEmmcSendCommand (
     mDwEmmcCommand = Cmd;
     mDwEmmcArgument = Argument;
   } else {
+    mDwEmmcCommand = Cmd;
+    mDwEmmcArgument = Argument;
     Status = SendCommand (Cmd, Argument);
   }
+
   return Status;
 }
 
@@ -410,6 +413,11 @@ DwEmmcReceiveResponse (
   IN UINT32*                    Buffer
   )
 {
+  EFI_STATUS Status = EFI_SUCCESS;
+
+  if(IsPendingReadCommand (mDwEmmcCommand) || IsPendingWriteCommand(mDwEmmcCommand))
+    Status = SendCommand (mDwEmmcCommand, mDwEmmcArgument);
+
   if (Buffer == NULL) {
     return EFI_INVALID_PARAMETER;
   }
@@ -427,7 +435,7 @@ DwEmmcReceiveResponse (
     Buffer[2] = MmioRead32 (DWEMMC_RESP2);
     Buffer[3] = MmioRead32 (DWEMMC_RESP3);
   }
-  return EFI_SUCCESS;
+  return Status;
 }
 
 VOID
-- 
2.2.2

[[PATCH v2] 6/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

From: "Tien Hock, Loh" <tien.hock.loh@intel.com>

Do not send CRC response check if the MMC command does not support CRC
response

Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index a69d9ab..c38b5a4 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -384,7 +384,7 @@ DwEmmcSendCommand (
            BIT_CMD_READ | BIT_CMD_WAIT_PRVDATA_COMPLETE;
     break;
   default:
-    Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC;
+    Cmd = BIT_CMD_RESPONSE_EXPECT;
     break;
   }
 
@@ -394,6 +394,11 @@ DwEmmcSendCommand (
     mIsACmd = TRUE;
   else
     mIsACmd = FALSE;
+
+  if (!(MmcCmd & MMC_CMD_NO_CRC_RESPONSE)) {
+    Cmd |= BIT_CMD_CHECK_RESPONSE_CRC;
+  }
+
   if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) {
     mDwEmmcCommand = Cmd;
     mDwEmmcArgument = Argument;
-- 
2.2.2

[[PATCH v2] 7/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

From: "Tien Hock, Loh" <tien.hock.loh@intel.com>

Add support for reading data that is less than DWEMMC_BLOCK_SIZE, otherwise
it would read bigger data than requested and cause errors

Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index c38b5a4..4183ad4 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -495,7 +495,10 @@ PrepareDmaData (
 
   Cnt = (Length + DWEMMC_DMA_BUF_SIZE - 1) / DWEMMC_DMA_BUF_SIZE;
   Blks = (Length + DWEMMC_BLOCK_SIZE - 1) / DWEMMC_BLOCK_SIZE;
-  Length = DWEMMC_BLOCK_SIZE * Blks;
+
+  if(Length >= DWEMMC_BLOCK_SIZE) {
+    Length = DWEMMC_BLOCK_SIZE * Blks;
+  }
 
   for (Idx = 0; Idx < Cnt; Idx++) {
     (IdmacDesc + Idx)->Des0 = DWEMMC_IDMAC_DES0_OWN | DWEMMC_IDMAC_DES0_CH |
@@ -533,11 +536,18 @@ StartDma (
   Data |= DWEMMC_CTRL_INT_EN | DWEMMC_CTRL_DMA_EN | DWEMMC_CTRL_IDMAC_EN;
   MmioWrite32 (DWEMMC_CTRL, Data);
   Data = MmioRead32 (DWEMMC_BMOD);
+
   Data |= DWEMMC_IDMAC_ENABLE | DWEMMC_IDMAC_FB;
   MmioWrite32 (DWEMMC_BMOD, Data);
 
-  MmioWrite32 (DWEMMC_BLKSIZ, DWEMMC_BLOCK_SIZE);
-  MmioWrite32 (DWEMMC_BYTCNT, Length);
+  if(Length < DWEMMC_BLOCK_SIZE) {
+    MmioWrite32 (DWEMMC_BLKSIZ, Length);
+    MmioWrite32 (DWEMMC_BYTCNT, Length);
+  }
+  else {
+    MmioWrite32 (DWEMMC_BLKSIZ, DWEMMC_BLOCK_SIZE);
+    MmioWrite32 (DWEMMC_BYTCNT, Length);
+  }
 }
 
 EFI_STATUS
-- 
2.2.2

Re: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Leif Lindholm]

Hi Tien Hock,

Thanks for splitting the patches back up, and sorry for taking so long
to get around to reviewing. But could you please add some descriptive
subject lines back as well?

Most of my comments on this series are purely coding style related, a
couple are not.

On Fri, May 03, 2019 at 11:26:57AM +0800, tien.hock.loh@intel.com wrote:
> From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> 
> Added ACMD6 for SD support. For SD, after CMD55 is sent, the next command
> should be an application command, which should not expect data
> 
> Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index adc6b06..fa24802 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -39,6 +39,7 @@ DWEMMC_IDMAC_DESCRIPTOR   *gpIdmacDesc;
>  EFI_GUID mDwEmmcDevicePathGuid = EFI_CALLER_ID_GUID;
>  STATIC UINT32 mDwEmmcCommand;
>  STATIC UINT32 mDwEmmcArgument;
> +STATIC BOOLEAN mIsACmd = FALSE;

Could we move this variable into DwEmmcSendCommand and drop the 'm'?

>  
>  EFI_STATUS
>  DwEmmcReadBlockData (
> @@ -321,6 +322,15 @@ DwEmmcSendCommand (
>      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
>             BIT_CMD_SEND_INIT;
>      break;
> +  case MMC_INDX(6):
> +    if(mIsACmd) {
> +      Cmd = BIT_CMD_RESPONSE_EXPECT ;

Drop space before ;.

> +    }
> +    else {

} else {

> +      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_DATA_EXPECTED |
> +            BIT_CMD_READ;
> +    }
> +    break;
>    case MMC_INDX(7):
>      if (Argument)
>          Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC;
> @@ -371,6 +381,11 @@ DwEmmcSendCommand (
>    }
>  
>    Cmd |= MMC_GET_INDX(MmcCmd) | BIT_CMD_USE_HOLD_REG | BIT_CMD_START;
> +
> +  if(MMC_INDX(55) == MMC_GET_INDX(MmcCmd))
> +    mIsACmd = TRUE;
> +  else
> +    mIsACmd = FALSE;

  if () {
  } else {
  }

There should also be spaces between MMC_INDX and (55), as well as
between MMC_GET_INDX (MmcCmd). Surrounding code does not follow the
style, but I would still prefer to keep to this for new/modified code.

I also think we should add IsACmd = FALSE to the default: case.

/
    Leif

>    if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) {
>      mDwEmmcCommand = Cmd;
>      mDwEmmcArgument = Argument;
> -- 
> 2.2.2
> 

Re: [[PATCH v2] 3/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Leif Lindholm]

On Fri, May 03, 2019 at 11:26:59AM +0800, tien.hock.loh@intel.com wrote:
> From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> 
> On CMD8, for SD, the controller should not expect data as this is a
> SEND_IF_COND command to verify SD operating condition, and does not have
> data
> 
> Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c        | 9 ++++++---
>  EmbeddedPkg/Include/Protocol/MmcHost.h           | 1 +
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 2 ++

If you follow the guidelines at
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers
when generating patches, that means interface changes (like for the .h
file here) get reviewed before their uses, which makes for a much more
natural review flow.

>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index 058665b..04fdcbf 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -339,9 +339,12 @@ DwEmmcSendCommand (
>          Cmd = 0;
>      break;
>    case MMC_INDX(8):
> -    Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
> -           BIT_CMD_DATA_EXPECTED | BIT_CMD_READ |
> -           BIT_CMD_WAIT_PRVDATA_COMPLETE;
> +    if (!This->IsEmmc)
> +      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
> +             BIT_CMD_WAIT_PRVDATA_COMPLETE ;
> +    else
> +      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
> +             BIT_CMD_WAIT_PRVDATA_COMPLETE | BIT_CMD_READ | BIT_CMD_DATA_EXPECTED;

I think this would be more clear as
       Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
             BIT_CMD_WAIT_PRVDATA_COMPLETE;
       if (...) {
         Cmd |= BIT_CMD_READ | BIT_CMD_DATA_EXPECTED;
       }

>      break;
>    case MMC_INDX(9):
>      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
> diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
> index 9e07082..ae8ea5d 100644
> --- a/EmbeddedPkg/Include/Protocol/MmcHost.h
> +++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
> @@ -169,6 +169,7 @@ struct _EFI_MMC_HOST_PROTOCOL {
>    MMC_SETIOS              SetIos;
>    MMC_ISMULTIBLOCK        IsMultiBlock;
>  
> +  BOOLEAN                 IsEmmc;

Case in point w.r.t. natural order.

But I don't think a BOOLEAN is the right solution here. (We would just
need to add another for each device type special case handling.)

Could you instead add an EFI_MMC_HOST_CARD_TYPE enum like in
EmbeddedPkg/Universal/MmcDxe/Mmc.h?

/
    Leif

>  };
>  
>  #define MMC_HOST_PROTOCOL_REVISION    0x00010002    // 1.2
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 4dc0be1..fa1eda2 100755
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -770,8 +770,10 @@ InitializeMmcDevice (
>    }
>  
>    if (MmcHostInstance->CardInfo.CardType != EMMC_CARD) {
> +    MmcHostInstance->MmcHost->IsEmmc = FALSE;
>      Status = InitializeSdMmcDevice (MmcHostInstance);
>    } else {
> +    MmcHostInstance->MmcHost->IsEmmc = TRUE;
>      Status = InitializeEmmcDevice (MmcHostInstance);
>    }
>    if (EFI_ERROR (Status)) {
> -- 
> 2.2.2
> 

Re: [[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Leif Lindholm]

On Fri, May 03, 2019 at 11:27:01AM +0800, tien.hock.loh@intel.com wrote:
> From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> 
> Send command when MMC ask for response in DwEmmcReceiveResponse, and
> command is a pending command (eg. DMA needs to be set up first)
> 
> Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index 32572a9..a69d9ab 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -398,8 +398,11 @@ DwEmmcSendCommand (
>      mDwEmmcCommand = Cmd;
>      mDwEmmcArgument = Argument;
>    } else {
> +    mDwEmmcCommand = Cmd;
> +    mDwEmmcArgument = Argument;
>      Status = SendCommand (Cmd, Argument);
>    }
> +

I agree a space looks better here, but please don't add unrelated
whitespace as part of a functional change.

>    return Status;
>  }
>  
> @@ -410,6 +413,11 @@ DwEmmcReceiveResponse (
>    IN UINT32*                    Buffer
>    )
>  {
> +  EFI_STATUS Status = EFI_SUCCESS;
> +
> +  if(IsPendingReadCommand (mDwEmmcCommand) || IsPendingWriteCommand(mDwEmmcCommand))

  {

> +    Status = SendCommand (mDwEmmcCommand, mDwEmmcArgument);

  }

> +
>    if (Buffer == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }

Should this test not come first in the function?
If the code is relying on the side effect of the SendCommand () being
performed even if Buffer is invalid, that needs a very detailed
comment.

/
    Leif


> @@ -427,7 +435,7 @@ DwEmmcReceiveResponse (
>      Buffer[2] = MmioRead32 (DWEMMC_RESP2);
>      Buffer[3] = MmioRead32 (DWEMMC_RESP3);
>    }
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>  
>  VOID
> -- 
> 2.2.2
> 

Re: [[PATCH v2] 7/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Leif Lindholm]

On Fri, May 03, 2019 at 11:27:03AM +0800, tien.hock.loh@intel.com wrote:
> From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> 
> Add support for reading data that is less than DWEMMC_BLOCK_SIZE, otherwise
> it would read bigger data than requested and cause errors
> 
> Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index c38b5a4..4183ad4 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -495,7 +495,10 @@ PrepareDmaData (
>  
>    Cnt = (Length + DWEMMC_DMA_BUF_SIZE - 1) / DWEMMC_DMA_BUF_SIZE;
>    Blks = (Length + DWEMMC_BLOCK_SIZE - 1) / DWEMMC_BLOCK_SIZE;

Could we add a BlockSize variable instead?...

> -  Length = DWEMMC_BLOCK_SIZE * Blks;
> +
> +  if(Length >= DWEMMC_BLOCK_SIZE) {
> +    Length = DWEMMC_BLOCK_SIZE * Blks;
> +  }
>  
>    for (Idx = 0; Idx < Cnt; Idx++) {
>      (IdmacDesc + Idx)->Des0 = DWEMMC_IDMAC_DES0_OWN | DWEMMC_IDMAC_DES0_CH |
> @@ -533,11 +536,18 @@ StartDma (
>    Data |= DWEMMC_CTRL_INT_EN | DWEMMC_CTRL_DMA_EN | DWEMMC_CTRL_IDMAC_EN;
>    MmioWrite32 (DWEMMC_CTRL, Data);
>    Data = MmioRead32 (DWEMMC_BMOD);
> +

Drop unrelated whitespace addition.

>    Data |= DWEMMC_IDMAC_ENABLE | DWEMMC_IDMAC_FB;
>    MmioWrite32 (DWEMMC_BMOD, Data);
>

And do
  if (Length < DWEMMC_BLOCK_SIZE) {
    BlockSize = Length;
  } else {
    BlockSize = DWEMMC_BLOCK_SIZE;
  }

  MmioWrite32 (DWEMMC_BLKSIZ, BlockSize);
  MmioWrite32 (DWEMMC_BYTCNT, Length);      
instead?

(I have no comments on the patches I have not responded to at this
point, but I want to see their proper subject lines before giving a R-b:)

/
    Leif

> -  MmioWrite32 (DWEMMC_BLKSIZ, DWEMMC_BLOCK_SIZE);
> -  MmioWrite32 (DWEMMC_BYTCNT, Length);
> +  if(Length < DWEMMC_BLOCK_SIZE) {
> +    MmioWrite32 (DWEMMC_BLKSIZ, Length);
> +    MmioWrite32 (DWEMMC_BYTCNT, Length);
> +  }
> +  else {
> +    MmioWrite32 (DWEMMC_BLKSIZ, DWEMMC_BLOCK_SIZE);
> +    MmioWrite32 (DWEMMC_BYTCNT, Length);
> +  }
>  }
>  
>  EFI_STATUS
> -- 
> 2.2.2
> 

Re: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

Hi Leif,

Sorry for the late reply, I were held up in other areas of the work.

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Friday, May 3, 2019 7:52 PM
> To: Loh, Tien Hock <tien.hock.loh@intel.com>
> Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs
> 
> Hi Tien Hock,
> 
> Thanks for splitting the patches back up, and sorry for taking so long to get
> around to reviewing. But could you please add some descriptive subject lines
> back as well?

OK noted. My bad, I thought the content of the commit would suffice.

> 
> Most of my comments on this series are purely coding style related, a couple
> are not.
> 
> On Fri, May 03, 2019 at 11:26:57AM +0800, tien.hock.loh@intel.com wrote:
> > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> >
> > Added ACMD6 for SD support. For SD, after CMD55 is sent, the next
> > command should be an application command, which should not expect
> data
> >
> > Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 15
> +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > index adc6b06..fa24802 100644
> > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > @@ -39,6 +39,7 @@ DWEMMC_IDMAC_DESCRIPTOR   *gpIdmacDesc;
> >  EFI_GUID mDwEmmcDevicePathGuid = EFI_CALLER_ID_GUID;  STATIC
> UINT32
> > mDwEmmcCommand;  STATIC UINT32 mDwEmmcArgument;
> > +STATIC BOOLEAN mIsACmd = FALSE;
> 
> Could we move this variable into DwEmmcSendCommand and drop the 'm'?
OK, so static variable in the function it is.

> 
> >
> >  EFI_STATUS
> >  DwEmmcReadBlockData (
> > @@ -321,6 +322,15 @@ DwEmmcSendCommand (
> >      Cmd = BIT_CMD_RESPONSE_EXPECT |
> BIT_CMD_CHECK_RESPONSE_CRC |
> >             BIT_CMD_SEND_INIT;
> >      break;
> > +  case MMC_INDX(6):
> > +    if(mIsACmd) {
> > +      Cmd = BIT_CMD_RESPONSE_EXPECT ;
> 
> Drop space before ;.
OK
> 
> > +    }
> > +    else {
> 
> } else {
> 
> > +      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_DATA_EXPECTED |
> > +            BIT_CMD_READ;
> > +    }
> > +    break;
> >    case MMC_INDX(7):
> >      if (Argument)
> >          Cmd = BIT_CMD_RESPONSE_EXPECT |
> BIT_CMD_CHECK_RESPONSE_CRC;
> > @@ -371,6 +381,11 @@ DwEmmcSendCommand (
> >    }
> >
> >    Cmd |= MMC_GET_INDX(MmcCmd) | BIT_CMD_USE_HOLD_REG |
> BIT_CMD_START;
> > +
> > +  if(MMC_INDX(55) == MMC_GET_INDX(MmcCmd))
> > +    mIsACmd = TRUE;
> > +  else
> > +    mIsACmd = FALSE;
> 
>   if () {
>   } else {
>   }
> 
> There should also be spaces between MMC_INDX and (55), as well as
> between MMC_GET_INDX (MmcCmd). Surrounding code does not follow
> the
> style, but I would still prefer to keep to this for new/modified code.
> 
> I also think we should add IsACmd = FALSE to the default: case.
OK, I'll fix this. Thanks for the review.

> 
> /
>     Leif
> 
> >    if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) {
> >      mDwEmmcCommand = Cmd;
> >      mDwEmmcArgument = Argument;
> > --
> > 2.2.2
> >

Re: [[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Friday, May 3, 2019 8:11 PM
> To: Loh, Tien Hock <tien.hock.loh@intel.com>
> Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs
> 
> On Fri, May 03, 2019 at 11:27:01AM +0800, tien.hock.loh@intel.com wrote:
> > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> >
> > Send command when MMC ask for response in
> DwEmmcReceiveResponse, and
> > command is a pending command (eg. DMA needs to be set up first)
> >
> > Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > index 32572a9..a69d9ab 100644
> > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > @@ -398,8 +398,11 @@ DwEmmcSendCommand (
> >      mDwEmmcCommand = Cmd;
> >      mDwEmmcArgument = Argument;
> >    } else {
> > +    mDwEmmcCommand = Cmd;
> > +    mDwEmmcArgument = Argument;
> >      Status = SendCommand (Cmd, Argument);
> >    }
> > +
> 
> I agree a space looks better here, but please don't add unrelated whitespace
> as part of a functional change.
OK noted. 
> 
> >    return Status;
> >  }
> >
> > @@ -410,6 +413,11 @@ DwEmmcReceiveResponse (
> >    IN UINT32*                    Buffer
> >    )
> >  {
> > +  EFI_STATUS Status = EFI_SUCCESS;
> > +
> > +  if(IsPendingReadCommand (mDwEmmcCommand) ||
> > + IsPendingWriteCommand(mDwEmmcCommand))
> 
>   {
> 
> > +    Status = SendCommand (mDwEmmcCommand, mDwEmmcArgument);
> 
>   }
> 
> > +
> >    if (Buffer == NULL) {
> >      return EFI_INVALID_PARAMETER;
> >    }
> 
> Should this test not come first in the function?
> If the code is relying on the side effect of the SendCommand () being
> performed even if Buffer is invalid, that needs a very detailed comment.

Yes I'll move it to after the buffer null check. 

> 
> /
>     Leif
> 
> 
> > @@ -427,7 +435,7 @@ DwEmmcReceiveResponse (
> >      Buffer[2] = MmioRead32 (DWEMMC_RESP2);
> >      Buffer[3] = MmioRead32 (DWEMMC_RESP3);
> >    }
> > -  return EFI_SUCCESS;
> > +  return Status;
> >  }
> >
> >  VOID
> > --
> > 2.2.2
> >

Re: [[PATCH v2] 7/7] EmbeddedPkg: Fix DwEmmc driver bugs

[Loh, Tien Hock]

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Friday, May 3, 2019 8:19 PM
> To: Loh, Tien Hock <tien.hock.loh@intel.com>
> Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [[PATCH v2] 7/7] EmbeddedPkg: Fix DwEmmc driver bugs
> 
> On Fri, May 03, 2019 at 11:27:03AM +0800, tien.hock.loh@intel.com wrote:
> > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> >
> > Add support for reading data that is less than DWEMMC_BLOCK_SIZE,
> > otherwise it would read bigger data than requested and cause errors
> >
> > Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 16
> +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > index c38b5a4..4183ad4 100644
> > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > @@ -495,7 +495,10 @@ PrepareDmaData (
> >
> >    Cnt = (Length + DWEMMC_DMA_BUF_SIZE - 1) /
> DWEMMC_DMA_BUF_SIZE;
> >    Blks = (Length + DWEMMC_BLOCK_SIZE - 1) / DWEMMC_BLOCK_SIZE;
> 
> Could we add a BlockSize variable instead?...

Can you clarify further on this? I didn't change anything on DWEMMC_BLOCK_SIZE. Do you want me to assign DWEMMC_BLOCK_SIZE to a BlockSize variable and use it? 

> 
> > -  Length = DWEMMC_BLOCK_SIZE * Blks;
> > +
> > +  if(Length >= DWEMMC_BLOCK_SIZE) {
> > +    Length = DWEMMC_BLOCK_SIZE * Blks;  }
> >
> >    for (Idx = 0; Idx < Cnt; Idx++) {
> >      (IdmacDesc + Idx)->Des0 = DWEMMC_IDMAC_DES0_OWN |
> > DWEMMC_IDMAC_DES0_CH | @@ -533,11 +536,18 @@ StartDma (
> >    Data |= DWEMMC_CTRL_INT_EN | DWEMMC_CTRL_DMA_EN |
> DWEMMC_CTRL_IDMAC_EN;
> >    MmioWrite32 (DWEMMC_CTRL, Data);
> >    Data = MmioRead32 (DWEMMC_BMOD);
> > +
> 
> Drop unrelated whitespace addition.
OK noted.

> 
> >    Data |= DWEMMC_IDMAC_ENABLE | DWEMMC_IDMAC_FB;
> >    MmioWrite32 (DWEMMC_BMOD, Data);
> >
> 
> And do
>   if (Length < DWEMMC_BLOCK_SIZE) {
>     BlockSize = Length;
>   } else {
>     BlockSize = DWEMMC_BLOCK_SIZE;
>   }
> 
>   MmioWrite32 (DWEMMC_BLKSIZ, BlockSize);
>   MmioWrite32 (DWEMMC_BYTCNT, Length);
> instead?
> 
> (I have no comments on the patches I have not responded to at this point,
> but I want to see their proper subject lines before giving a R-b:)
OK, noted with thanks!
> 
> /
>     Leif
> 
> > -  MmioWrite32 (DWEMMC_BLKSIZ, DWEMMC_BLOCK_SIZE);
> > -  MmioWrite32 (DWEMMC_BYTCNT, Length);
> > +  if(Length < DWEMMC_BLOCK_SIZE) {
> > +    MmioWrite32 (DWEMMC_BLKSIZ, Length);
> > +    MmioWrite32 (DWEMMC_BYTCNT, Length);  }  else {
> > +    MmioWrite32 (DWEMMC_BLKSIZ, DWEMMC_BLOCK_SIZE);
> > +    MmioWrite32 (DWEMMC_BYTCNT, Length);  }
> >  }
> >
> >  EFI_STATUS
> > --
> > 2.2.2
> >