[PATCH v2 0/7] Fix some bugs with DwEmmc

[PATCH v2 0/7] Fix some bugs with DwEmmc

[Loh, Tien Hock]

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

There are some issues with DwEmmc when being used with SD protocol.
These series of patches fixes the issues.

TIen Hock, Loh (1):
  EmbeddedPkg: Add SD command support for DwEmmc

Tien Hock, Loh (6):
  EmbeddedPkg: Fix DwEmmc CMD8 support for SD
  EmbeddedPkg: Send command when MMC ask for response
  EmbeddedPkg: Fix response check flag
  EmbeddedPkg: Clear CTYPE on initialization
  EmbeddedPkg: Fix DwEmmc SendCommand polling
  EmbeddedPkg: Fix DwEmmc read/write size in preparing DMA size

 EmbeddedPkg/Include/Protocol/MmcHost.h        |   6 +
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c     | 103 +++++++++++++++---
 .../Universal/MmcDxe/MmcIdentification.c      |   2 +
 3 files changed, 93 insertions(+), 18 deletions(-)

-- 
2.19.0

[PATCH v2 1/7] EmbeddedPkg: Add SD command support for DwEmmc

[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>

--
v2:
- Move IsACmd as a local static variable in function
- Fix some coding standard issue with spacing
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index 68c523a99f..420487757d 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -307,6 +307,7 @@ DwEmmcSendCommand (
 {
   UINT32       Cmd = 0;
   EFI_STATUS   Status = EFI_SUCCESS;
+  STATIC BOOLEAN IsACmd = FALSE;
 
   switch (MMC_GET_INDX(MmcCmd)) {
   case MMC_INDX(0):
@@ -323,6 +324,15 @@ DwEmmcSendCommand (
     Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
            BIT_CMD_SEND_INIT;
     break;
+  case MMC_INDX (6):
+    if(IsACmd) {
+      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;
@@ -367,12 +377,22 @@ 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;
   }
 
   Cmd |= MMC_GET_INDX(MmcCmd) | BIT_CMD_USE_HOLD_REG | BIT_CMD_START;
+
+  if(MMC_INDX (55) == MMC_GET_INDX (MmcCmd))
+    IsACmd = TRUE;
+  else
+    IsACmd = FALSE;
+
   if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) {
     mDwEmmcCommand = Cmd;
     mDwEmmcArgument = Argument;
-- 
2.19.0

Re: [PATCH v2 1/7] EmbeddedPkg: Add SD command support for DwEmmc

[Leif Lindholm]

Hi Tien Hock,

Thanks for resubmitting this.

On Mon, May 27, 2019 at 05:30:22PM +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>
> 
> --
> v2:
> - Move IsACmd as a local static variable in function
> - Fix some coding standard issue with spacing
> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index 68c523a99f..420487757d 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -307,6 +307,7 @@ DwEmmcSendCommand (
>  {
>    UINT32       Cmd = 0;
>    EFI_STATUS   Status = EFI_SUCCESS;
> +  STATIC BOOLEAN IsACmd = FALSE;
>  
>    switch (MMC_GET_INDX(MmcCmd)) {
>    case MMC_INDX(0):
> @@ -323,6 +324,15 @@ DwEmmcSendCommand (
>      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
>             BIT_CMD_SEND_INIT;
>      break;
> +  case MMC_INDX (6):
> +    if(IsACmd) {
> +      Cmd = BIT_CMD_RESPONSE_EXPECT;
> +    }
> +    else {

I remarked last time that the else should be on the same line as }.

> +      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;
> @@ -367,12 +377,22 @@ 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;
>    }
>  
>    Cmd |= MMC_GET_INDX(MmcCmd) | BIT_CMD_USE_HOLD_REG | BIT_CMD_START;
> +
> +  if(MMC_INDX (55) == MMC_GET_INDX (MmcCmd))

Space after if.

> +    IsACmd = TRUE;
> +  else
> +    IsACmd = FALSE;

Even though some code in this file takes liberties with the use of {
and }, please add them for both if and else.

/
    Leif

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

[PATCH v2 2/7] EmbeddedPkg: Fix DwEmmc CMD8 support for SD

[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>

--
v2
- Change IsEmmc to EFI_MMC_HOST_CARD_TYPE
---
 EmbeddedPkg/Include/Protocol/MmcHost.h           | 6 ++++++
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c        | 9 ++++++---
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 2 ++
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
index 9e07082680..7807744721 100644
--- a/EmbeddedPkg/Include/Protocol/MmcHost.h
+++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
@@ -151,6 +151,11 @@ typedef BOOLEAN (EFIAPI *MMC_ISMULTIBLOCK) (
   IN  EFI_MMC_HOST_PROTOCOL     *This
   );
 
+typedef enum {
+  EMMC,
+  SD
+} EFI_MMC_HOST_CARD_TYPE;
+
 struct _EFI_MMC_HOST_PROTOCOL {
 
   UINT32                  Revision;
@@ -169,6 +174,7 @@ struct _EFI_MMC_HOST_PROTOCOL {
   MMC_SETIOS              SetIos;
   MMC_ISMULTIBLOCK        IsMultiBlock;
 
+  EFI_MMC_HOST_CARD_TYPE  HostCardType;
 };
 
 #define MMC_HOST_PROTOCOL_REVISION    0x00010002    // 1.2
diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index 420487757d..fd3a5bf685 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -340,9 +340,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->HostCardType == SD)
+      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/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index 4dc0be125c..c816ae09ee 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->HostCardType = SD;
     Status = InitializeSdMmcDevice (MmcHostInstance);
   } else {
+    MmcHostInstance->MmcHost->HostCardType = EMMC;
     Status = InitializeEmmcDevice (MmcHostInstance);
   }
   if (EFI_ERROR (Status)) {
-- 
2.19.0

Re: [PATCH v2 2/7] EmbeddedPkg: Fix DwEmmc CMD8 support for SD

[Leif Lindholm]

On Mon, May 27, 2019 at 05:30:23PM +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>
> 
> --
> v2
> - Change IsEmmc to EFI_MMC_HOST_CARD_TYPE

Version information goes below ---.
Never type this manually - sooner or later you will leave out a like
above and accidentally have it included in the commit message.

It is also helpful to include this information in a cover letter
[0/7]. For simple changes like this, there is no need to list it
individually in each patch.

> ---
>  EmbeddedPkg/Include/Protocol/MmcHost.h           | 6 ++++++
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c        | 9 ++++++---
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 2 ++
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
> index 9e07082680..7807744721 100644
> --- a/EmbeddedPkg/Include/Protocol/MmcHost.h
> +++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
> @@ -151,6 +151,11 @@ typedef BOOLEAN (EFIAPI *MMC_ISMULTIBLOCK) (
>    IN  EFI_MMC_HOST_PROTOCOL     *This
>    );
>  
> +typedef enum {
> +  EMMC,
> +  SD
> +} EFI_MMC_HOST_CARD_TYPE;
> +
>  struct _EFI_MMC_HOST_PROTOCOL {
>  
>    UINT32                  Revision;
> @@ -169,6 +174,7 @@ struct _EFI_MMC_HOST_PROTOCOL {
>    MMC_SETIOS              SetIos;
>    MMC_ISMULTIBLOCK        IsMultiBlock;
>  
> +  EFI_MMC_HOST_CARD_TYPE  HostCardType;
>  };
>  
>  #define MMC_HOST_PROTOCOL_REVISION    0x00010002    // 1.2
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index 420487757d..fd3a5bf685 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -340,9 +340,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->HostCardType == SD)
> +      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 saw no comment on my feedback from previous revision that
---
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;
  }
---
and the request was not followed.

Also, always use { and } with if/else.

/
    Leif

>      break;
>    case MMC_INDX(9):
>      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 4dc0be125c..c816ae09ee 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->HostCardType = SD;
>      Status = InitializeSdMmcDevice (MmcHostInstance);
>    } else {
> +    MmcHostInstance->MmcHost->HostCardType = EMMC;
>      Status = InitializeEmmcDevice (MmcHostInstance);
>    }
>    if (EFI_ERROR (Status)) {
> -- 
> 2.19.0
> 

[PATCH v2 3/7] EmbeddedPkg: Send command when MMC ask for response

[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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index fd3a5bf685..c44e310c04 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -400,6 +400,8 @@ DwEmmcSendCommand (
     mDwEmmcCommand = Cmd;
     mDwEmmcArgument = Argument;
   } else {
+    mDwEmmcCommand = Cmd;
+    mDwEmmcArgument = Argument;
     Status = SendCommand (Cmd, Argument);
   }
   return Status;
@@ -412,10 +414,15 @@ DwEmmcReceiveResponse (
   IN UINT32*                    Buffer
   )
 {
+  EFI_STATUS Status = EFI_SUCCESS;
+
   if (Buffer == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
+  if(IsPendingReadCommand (mDwEmmcCommand) || IsPendingWriteCommand(mDwEmmcCommand))
+    Status = SendCommand (mDwEmmcCommand, mDwEmmcArgument);
+
   if (   (Type == MMC_RESPONSE_TYPE_R1)
       || (Type == MMC_RESPONSE_TYPE_R1b)
       || (Type == MMC_RESPONSE_TYPE_R3)
@@ -429,7 +436,7 @@ DwEmmcReceiveResponse (
     Buffer[2] = MmioRead32 (DWEMMC_RESP2);
     Buffer[3] = MmioRead32 (DWEMMC_RESP3);
   }
-  return EFI_SUCCESS;
+  return Status;
 }
 
 VOID
-- 
2.19.0

Re: [PATCH v2 3/7] EmbeddedPkg: Send command when MMC ask for response

[Leif Lindholm]

On Mon, May 27, 2019 at 05:30:24PM +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 | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index fd3a5bf685..c44e310c04 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -400,6 +400,8 @@ DwEmmcSendCommand (
>      mDwEmmcCommand = Cmd;
>      mDwEmmcArgument = Argument;
>    } else {
> +    mDwEmmcCommand = Cmd;
> +    mDwEmmcArgument = Argument;

OK, sorry, I didn't spot this last time around - but these two lines
are now present for both sides of the conditional, so should be moved
outside. Which (I think) would turn it into

  mDwEmmcCommand = Cmd;
  mDwEmmcArgument = Argument;
  if (!IsPendingReadCommand (Cmd) && !IsPendingWriteCommand (Cmd)) {
    Status = SendCommand (Cmd, Argument);
  }


>      Status = SendCommand (Cmd, Argument);
>    }
>    return Status;
> @@ -412,10 +414,15 @@ DwEmmcReceiveResponse (
>    IN UINT32*                    Buffer
>    )
>  {
> +  EFI_STATUS Status = EFI_SUCCESS;
> +
>    if (Buffer == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> +  if(IsPendingReadCommand (mDwEmmcCommand) || IsPendingWriteCommand(mDwEmmcCommand))

Space after if. Space after IsPendingWriteCommand.

/
    Leif

> +    Status = SendCommand (mDwEmmcCommand, mDwEmmcArgument);
> +
>    if (   (Type == MMC_RESPONSE_TYPE_R1)
>        || (Type == MMC_RESPONSE_TYPE_R1b)
>        || (Type == MMC_RESPONSE_TYPE_R3)
> @@ -429,7 +436,7 @@ DwEmmcReceiveResponse (
>      Buffer[2] = MmioRead32 (DWEMMC_RESP2);
>      Buffer[3] = MmioRead32 (DWEMMC_RESP3);
>    }
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>  
>  VOID
> -- 
> 2.19.0
> 

[PATCH v2 4/7] EmbeddedPkg: Fix response check flag

[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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index c44e310c04..e0068655ca 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -385,7 +385,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;
   }
 
@@ -396,6 +396,10 @@ DwEmmcSendCommand (
   else
     IsACmd = FALSE;
 
+  if (!(MmcCmd & MMC_CMD_NO_CRC_RESPONSE)) {
+    Cmd |= BIT_CMD_CHECK_RESPONSE_CRC;
+  }
+
   if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) {
     mDwEmmcCommand = Cmd;
     mDwEmmcArgument = Argument;
-- 
2.19.0

Re: [PATCH v2 4/7] EmbeddedPkg: Fix response check flag

[Leif Lindholm]

On Mon, May 27, 2019 at 05:30:25PM +0800, tien.hock.loh@intel.com wrote:
> 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>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index c44e310c04..e0068655ca 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -385,7 +385,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;
>    }
>  
> @@ -396,6 +396,10 @@ DwEmmcSendCommand (
>    else
>      IsACmd = FALSE;
>  
> +  if (!(MmcCmd & MMC_CMD_NO_CRC_RESPONSE)) {
> +    Cmd |= BIT_CMD_CHECK_RESPONSE_CRC;
> +  }
> +
>    if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) {
>      mDwEmmcCommand = Cmd;
>      mDwEmmcArgument = Argument;
> -- 
> 2.19.0
> 

[PATCH v2 5/7] EmbeddedPkg: Clear CTYPE on initialization

[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 e0068655ca..c6c8e04917 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -205,6 +205,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.19.0

Re: [PATCH v2 5/7] EmbeddedPkg: Clear CTYPE on initialization

[Leif Lindholm]

On Mon, May 27, 2019 at 05:30:26PM +0800, tien.hock.loh@intel.com wrote:
> 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>

Reviewed-by: Leif Lindholm <leif.lindholm@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 e0068655ca..c6c8e04917 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -205,6 +205,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.19.0
> 

[PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling

[Loh, Tien Hock]

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

Change SendCommand polling mode to remove unnecessary delay, and check
for transfer done only when block data is to be read/write. This would
also increase performance slightly.

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 | 43 +++++++++++++++-----
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index c6c8e04917..b57833458f 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -286,16 +286,13 @@ SendCommand (
             DWEMMC_INT_RCRC | DWEMMC_INT_RE;
   ErrMask |= DWEMMC_INT_DCRC | DWEMMC_INT_DRT | DWEMMC_INT_SBE;
   do {
-    MicroSecondDelay(500);
     Data = MmioRead32 (DWEMMC_RINTSTS);
-
-    if (Data & ErrMask) {
-      return EFI_DEVICE_ERROR;
-    }
-    if (Data & DWEMMC_INT_DTO) {     // Transfer Done
-      break;
-    }
   } while (!(Data & DWEMMC_INT_CMD_DONE));
+
+  if (Data & ErrMask) {
+    return EFI_DEVICE_ERROR;
+  }
+
   return EFI_SUCCESS;
 }
 
@@ -550,8 +547,9 @@ DwEmmcReadBlockData (
   )
 {
   EFI_STATUS  Status;
-  UINT32      DescPages, CountPerPage, Count;
+  UINT32      DescPages, CountPerPage, Count, ErrMask;
   EFI_TPL     Tpl;
+  UINTN Rintsts = 0;
 
   Tpl = gBS->RaiseTPL (TPL_NOTIFY);
 
@@ -574,6 +572,18 @@ DwEmmcReadBlockData (
     DEBUG ((DEBUG_ERROR, "Failed to read data, mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n", mDwEmmcCommand, mDwEmmcArgument, Status));
     goto out;
   }
+
+  while(!((MmioRead32(DWEMMC_RINTSTS) & (DWEMMC_INT_DTO)))) {
+    Rintsts = MmioRead32 (DWEMMC_RINTSTS);
+  }
+  ErrMask = DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
+            DWEMMC_INT_RCRC | DWEMMC_INT_RE | DWEMMC_INT_DCRC |
+            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
+
+  if (Rintsts & ErrMask) {
+    Status = EFI_DEVICE_ERROR;
+    goto out;
+  }
 out:
   // Restore Tpl
   gBS->RestoreTPL (Tpl);
@@ -589,8 +599,9 @@ DwEmmcWriteBlockData (
   )
 {
   EFI_STATUS  Status;
-  UINT32      DescPages, CountPerPage, Count;
+  UINT32      DescPages, CountPerPage, Count, ErrMask;
   EFI_TPL     Tpl;
+  UINTN Rintsts = 0;
 
   Tpl = gBS->RaiseTPL (TPL_NOTIFY);
 
@@ -613,6 +624,18 @@ DwEmmcWriteBlockData (
     DEBUG ((DEBUG_ERROR, "Failed to write data, mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n", mDwEmmcCommand, mDwEmmcArgument, Status));
     goto out;
   }
+
+  while(!((MmioRead32(DWEMMC_RINTSTS) & (DWEMMC_INT_DTO)))) {
+    Rintsts = MmioRead32 (DWEMMC_RINTSTS);
+  }
+  ErrMask = DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
+            DWEMMC_INT_RCRC | DWEMMC_INT_RE | DWEMMC_INT_DCRC |
+            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
+
+  if (Rintsts & ErrMask) {
+    Status = EFI_DEVICE_ERROR;
+    goto out;
+  }
 out:
   // Restore Tpl
   gBS->RestoreTPL (Tpl);
-- 
2.19.0

Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling

[Leif Lindholm]

+Haojian,

Haojian - since you are the original author, can you comment on the
delays? Are these silicon bug workarounds (so we need to add a Pcd),
or does these changes work on your platforms too?

Regards,

Leif

On Mon, May 27, 2019 at 05:30:27PM +0800, tien.hock.loh@intel.com wrote:
> From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> 
> Change SendCommand polling mode to remove unnecessary delay, and check
> for transfer done only when block data is to be read/write. This would
> also increase performance slightly.
> 
> 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 | 43 +++++++++++++++-----
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index c6c8e04917..b57833458f 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -286,16 +286,13 @@ SendCommand (
>              DWEMMC_INT_RCRC | DWEMMC_INT_RE;
>    ErrMask |= DWEMMC_INT_DCRC | DWEMMC_INT_DRT | DWEMMC_INT_SBE;
>    do {
> -    MicroSecondDelay(500);
>      Data = MmioRead32 (DWEMMC_RINTSTS);
> -
> -    if (Data & ErrMask) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -    if (Data & DWEMMC_INT_DTO) {     // Transfer Done
> -      break;
> -    }
>    } while (!(Data & DWEMMC_INT_CMD_DONE));
> +
> +  if (Data & ErrMask) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
>    return EFI_SUCCESS;
>  }
>  
> @@ -550,8 +547,9 @@ DwEmmcReadBlockData (
>    )
>  {
>    EFI_STATUS  Status;
> -  UINT32      DescPages, CountPerPage, Count;
> +  UINT32      DescPages, CountPerPage, Count, ErrMask;
>    EFI_TPL     Tpl;
> +  UINTN Rintsts = 0;
>  
>    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
>  
> @@ -574,6 +572,18 @@ DwEmmcReadBlockData (
>      DEBUG ((DEBUG_ERROR, "Failed to read data, mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n", mDwEmmcCommand, mDwEmmcArgument, Status));
>      goto out;
>    }
> +
> +  while(!((MmioRead32(DWEMMC_RINTSTS) & (DWEMMC_INT_DTO)))) {
> +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);
> +  }
> +  ErrMask = DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> +            DWEMMC_INT_RCRC | DWEMMC_INT_RE | DWEMMC_INT_DCRC |
> +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> +
> +  if (Rintsts & ErrMask) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto out;
> +  }
>  out:
>    // Restore Tpl
>    gBS->RestoreTPL (Tpl);
> @@ -589,8 +599,9 @@ DwEmmcWriteBlockData (
>    )
>  {
>    EFI_STATUS  Status;
> -  UINT32      DescPages, CountPerPage, Count;
> +  UINT32      DescPages, CountPerPage, Count, ErrMask;
>    EFI_TPL     Tpl;
> +  UINTN Rintsts = 0;
>  
>    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
>  
> @@ -613,6 +624,18 @@ DwEmmcWriteBlockData (
>      DEBUG ((DEBUG_ERROR, "Failed to write data, mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n", mDwEmmcCommand, mDwEmmcArgument, Status));
>      goto out;
>    }
> +
> +  while(!((MmioRead32(DWEMMC_RINTSTS) & (DWEMMC_INT_DTO)))) {
> +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);
> +  }
> +  ErrMask = DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> +            DWEMMC_INT_RCRC | DWEMMC_INT_RE | DWEMMC_INT_DCRC |
> +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> +
> +  if (Rintsts & ErrMask) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto out;
> +  }
>  out:
>    // Restore Tpl
>    gBS->RestoreTPL (Tpl);
> -- 
> 2.19.0
> 

Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling

[Haojian Zhuang]

On Tue, May 28, 2019 at 07:04:09PM +0100, Leif Lindholm wrote:
> +Haojian,
> 
> Haojian - since you are the original author, can you comment on the
> delays? Are these silicon bug workarounds (so we need to add a Pcd),
> or does these changes work on your platforms too?

I'm not in the loop, so I missed the patch series.

The patch series can't work on my platform for the eMMC. Although a
variable is created to identify whether it's a SD or eMMC device, it
doesn't identify the eMMC device by the right way. So the eMMC device
isn't initialized successfully on my platform.

1. Since MMC framework could identify whether it's eMMC device or SD
device, we need to make device driver gets this kind of information from
the MMC framework. And we need to support multiple eMMC/SD instances in
MMC framework.

2. I sent a patch series to support both eMMC device and SD device before.
https://edk2.groups.io/g/devel/message/28572
&&
https://edk2.groups.io/g/devel/message/28615
Maybe it's missed. Could you help to review that patch series?

Best Regards
Haojian

> 
> Regards,
> 
> Leif
> 
> On Mon, May 27, 2019 at 05:30:27PM +0800, tien.hock.loh@intel.com wrote:
> > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > 
> > Change SendCommand polling mode to remove unnecessary delay, and check
> > for transfer done only when block data is to be read/write. This would
> > also increase performance slightly.
> > 
> > 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 | 43 +++++++++++++++-----
> >  1 file changed, 33 insertions(+), 10 deletions(-)
> > 
> > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > index c6c8e04917..b57833458f 100644
> > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > @@ -286,16 +286,13 @@ SendCommand (
> >              DWEMMC_INT_RCRC | DWEMMC_INT_RE;
> >    ErrMask |= DWEMMC_INT_DCRC | DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> >    do {
> > -    MicroSecondDelay(500);
> >      Data = MmioRead32 (DWEMMC_RINTSTS);
> > -
> > -    if (Data & ErrMask) {
> > -      return EFI_DEVICE_ERROR;
> > -    }
> > -    if (Data & DWEMMC_INT_DTO) {     // Transfer Done
> > -      break;
> > -    }
> >    } while (!(Data & DWEMMC_INT_CMD_DONE));
> > +
> > +  if (Data & ErrMask) {
> > +    return EFI_DEVICE_ERROR;
> > +  }
> > +
> >    return EFI_SUCCESS;
> >  }
> >  
> > @@ -550,8 +547,9 @@ DwEmmcReadBlockData (
> >    )
> >  {
> >    EFI_STATUS  Status;
> > -  UINT32      DescPages, CountPerPage, Count;
> > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> >    EFI_TPL     Tpl;
> > +  UINTN Rintsts = 0;
> >  
> >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> >  
> > @@ -574,6 +572,18 @@ DwEmmcReadBlockData (
> >      DEBUG ((DEBUG_ERROR, "Failed to read data, mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n", mDwEmmcCommand, mDwEmmcArgument, Status));
> >      goto out;
> >    }
> > +
> > +  while(!((MmioRead32(DWEMMC_RINTSTS) & (DWEMMC_INT_DTO)))) {
> > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);
> > +  }
> > +  ErrMask = DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE | DWEMMC_INT_DCRC |
> > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > +
> > +  if (Rintsts & ErrMask) {
> > +    Status = EFI_DEVICE_ERROR;
> > +    goto out;
> > +  }
> >  out:
> >    // Restore Tpl
> >    gBS->RestoreTPL (Tpl);
> > @@ -589,8 +599,9 @@ DwEmmcWriteBlockData (
> >    )
> >  {
> >    EFI_STATUS  Status;
> > -  UINT32      DescPages, CountPerPage, Count;
> > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> >    EFI_TPL     Tpl;
> > +  UINTN Rintsts = 0;
> >  
> >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> >  
> > @@ -613,6 +624,18 @@ DwEmmcWriteBlockData (
> >      DEBUG ((DEBUG_ERROR, "Failed to write data, mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n", mDwEmmcCommand, mDwEmmcArgument, Status));
> >      goto out;
> >    }
> > +
> > +  while(!((MmioRead32(DWEMMC_RINTSTS) & (DWEMMC_INT_DTO)))) {
> > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);
> > +  }
> > +  ErrMask = DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE | DWEMMC_INT_DCRC |
> > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > +
> > +  if (Rintsts & ErrMask) {
> > +    Status = EFI_DEVICE_ERROR;
> > +    goto out;
> > +  }
> >  out:
> >    // Restore Tpl
> >    gBS->RestoreTPL (Tpl);
> > -- 
> > 2.19.0
> > 

Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling

[Loh, Tien Hock]

> -----Original Message-----
> From: Haojian Zhuang <haojian.zhuang@linaro.org>
> Sent: Thursday, May 30, 2019 3:06 PM
> To: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Loh, Tien Hock <tien.hock.loh@intel.com>; devel@edk2.groups.io;
> thloh85@gmail.com; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> polling
> 
> On Tue, May 28, 2019 at 07:04:09PM +0100, Leif Lindholm wrote:
> > +Haojian,
> >
> > Haojian - since you are the original author, can you comment on the
> > delays? Are these silicon bug workarounds (so we need to add a Pcd),
> > or does these changes work on your platforms too?
> 
> I'm not in the loop, so I missed the patch series.
> 
> The patch series can't work on my platform for the eMMC. Although a
> variable is created to identify whether it's a SD or eMMC device, it doesn't
> identify the eMMC device by the right way. So the eMMC device isn't
> initialized successfully on my platform.
> 
> 1. Since MMC framework could identify whether it's eMMC device or SD
> device, we need to make device driver gets this kind of information from the
> MMC framework. And we need to support multiple eMMC/SD instances in
> MMC framework.

Yeah my bad I didn't read through the SD/MMC specs on that. Now I check the specs and you're right, the information should be read from MMC framework.

> 
> 2. I sent a patch series to support both eMMC device and SD device before.
> https://edk2.groups.io/g/devel/message/28572
> &&
> https://edk2.groups.io/g/devel/message/28615
> Maybe it's missed. Could you help to review that patch series?

> 
> Best Regards
> Haojian
> 
> >
> > Regards,
> >
> > Leif
> >
> > On Mon, May 27, 2019 at 05:30:27PM +0800, tien.hock.loh@intel.com
> wrote:
> > > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > >
> > > Change SendCommand polling mode to remove unnecessary delay, and
> > > check for transfer done only when block data is to be read/write.
> > > This would also increase performance slightly.
> > >
> > > 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 | 43
> +++++++++++++++-----
> > >  1 file changed, 33 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > index c6c8e04917..b57833458f 100644
> > > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > @@ -286,16 +286,13 @@ SendCommand (
> > >              DWEMMC_INT_RCRC | DWEMMC_INT_RE;
> > >    ErrMask |= DWEMMC_INT_DCRC | DWEMMC_INT_DRT |
> DWEMMC_INT_SBE;
> > >    do {
> > > -    MicroSecondDelay(500);
> > >      Data = MmioRead32 (DWEMMC_RINTSTS);
> > > -
> > > -    if (Data & ErrMask) {
> > > -      return EFI_DEVICE_ERROR;
> > > -    }
> > > -    if (Data & DWEMMC_INT_DTO) {     // Transfer Done
> > > -      break;
> > > -    }
> > >    } while (!(Data & DWEMMC_INT_CMD_DONE));
> > > +
> > > +  if (Data & ErrMask) {
> > > +    return EFI_DEVICE_ERROR;
> > > +  }
> > > +
> > >    return EFI_SUCCESS;
> > >  }
> > >
> > > @@ -550,8 +547,9 @@ DwEmmcReadBlockData (
> > >    )
> > >  {
> > >    EFI_STATUS  Status;
> > > -  UINT32      DescPages, CountPerPage, Count;
> > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > >    EFI_TPL     Tpl;
> > > +  UINTN Rintsts = 0;
> > >
> > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > >
> > > @@ -574,6 +572,18 @@ DwEmmcReadBlockData (
> > >      DEBUG ((DEBUG_ERROR, "Failed to read data,
> mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> mDwEmmcCommand, mDwEmmcArgument, Status));
> > >      goto out;
> > >    }
> > > +
> > > +  while(!((MmioRead32(DWEMMC_RINTSTS) & (DWEMMC_INT_DTO))))
> {
> > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> DWEMMC_INT_DCRC |
> > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > +
> > > +  if (Rintsts & ErrMask) {
> > > +    Status = EFI_DEVICE_ERROR;
> > > +    goto out;
> > > +  }
> > >  out:
> > >    // Restore Tpl
> > >    gBS->RestoreTPL (Tpl);
> > > @@ -589,8 +599,9 @@ DwEmmcWriteBlockData (
> > >    )
> > >  {
> > >    EFI_STATUS  Status;
> > > -  UINT32      DescPages, CountPerPage, Count;
> > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > >    EFI_TPL     Tpl;
> > > +  UINTN Rintsts = 0;
> > >
> > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > >
> > > @@ -613,6 +624,18 @@ DwEmmcWriteBlockData (
> > >      DEBUG ((DEBUG_ERROR, "Failed to write data,
> mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> mDwEmmcCommand, mDwEmmcArgument, Status));
> > >      goto out;
> > >    }
> > > +
> > > +  while(!((MmioRead32(DWEMMC_RINTSTS) & (DWEMMC_INT_DTO))))
> {
> > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> DWEMMC_INT_DCRC |
> > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > +
> > > +  if (Rintsts & ErrMask) {
> > > +    Status = EFI_DEVICE_ERROR;
> > > +    goto out;
> > > +  }
> > >  out:
> > >    // Restore Tpl
> > >    gBS->RestoreTPL (Tpl);
> > > --
> > > 2.19.0
> > >

Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling

[Loh, Tien Hock]

Leif,

Since Haojian have a newer driver model that uses the NonDiscoverableDeviceDxe, I believe we should be moving to use the new driver.
I'll try to test out the patches submitted by Haojian in the mean time. 
Can you help review the patch? Thanks!

> -----Original Message-----
> From: Loh, Tien Hock
> Sent: Thursday, May 30, 2019 3:56 PM
> To: Haojian Zhuang <haojian.zhuang@linaro.org>; Leif Lindholm
> <leif.lindholm@linaro.org>
> Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: RE: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> polling
> 
> > -----Original Message-----
> > From: Haojian Zhuang <haojian.zhuang@linaro.org>
> > Sent: Thursday, May 30, 2019 3:06 PM
> > To: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Loh, Tien Hock <tien.hock.loh@intel.com>; devel@edk2.groups.io;
> > thloh85@gmail.com; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Subject: Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> > polling
> >
> > On Tue, May 28, 2019 at 07:04:09PM +0100, Leif Lindholm wrote:
> > > +Haojian,
> > >
> > > Haojian - since you are the original author, can you comment on the
> > > delays? Are these silicon bug workarounds (so we need to add a Pcd),
> > > or does these changes work on your platforms too?
> >
> > I'm not in the loop, so I missed the patch series.
> >
> > The patch series can't work on my platform for the eMMC. Although a
> > variable is created to identify whether it's a SD or eMMC device, it
> > doesn't identify the eMMC device by the right way. So the eMMC device
> > isn't initialized successfully on my platform.
> >
> > 1. Since MMC framework could identify whether it's eMMC device or SD
> > device, we need to make device driver gets this kind of information
> > from the MMC framework. And we need to support multiple eMMC/SD
> > instances in MMC framework.
> 
> Yeah my bad I didn't read through the SD/MMC specs on that. Now I check
> the specs and you're right, the information should be read from MMC
> framework.
> 
> >
> > 2. I sent a patch series to support both eMMC device and SD device before.
> > https://edk2.groups.io/g/devel/message/28572
> > &&
> > https://edk2.groups.io/g/devel/message/28615
> > Maybe it's missed. Could you help to review that patch series?

Leif, can you help review the patch series? Since Haojian have moved the driver to NonDiscoverableDeviceDxe, I think that would be a more appropriate driver to be used going forward. Thanks!

> 
> >
> > Best Regards
> > Haojian
> >
> > >
> > > Regards,
> > >
> > > Leif
> > >
> > > On Mon, May 27, 2019 at 05:30:27PM +0800, tien.hock.loh@intel.com
> > wrote:
> > > > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > > >
> > > > Change SendCommand polling mode to remove unnecessary delay, and
> > > > check for transfer done only when block data is to be read/write.
> > > > This would also increase performance slightly.
> > > >
> > > > 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 | 43
> > +++++++++++++++-----
> > > >  1 file changed, 33 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > index c6c8e04917..b57833458f 100644
> > > > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > @@ -286,16 +286,13 @@ SendCommand (
> > > >              DWEMMC_INT_RCRC | DWEMMC_INT_RE;
> > > >    ErrMask |= DWEMMC_INT_DCRC | DWEMMC_INT_DRT |
> > DWEMMC_INT_SBE;
> > > >    do {
> > > > -    MicroSecondDelay(500);
> > > >      Data = MmioRead32 (DWEMMC_RINTSTS);
> > > > -
> > > > -    if (Data & ErrMask) {
> > > > -      return EFI_DEVICE_ERROR;
> > > > -    }
> > > > -    if (Data & DWEMMC_INT_DTO) {     // Transfer Done
> > > > -      break;
> > > > -    }
> > > >    } while (!(Data & DWEMMC_INT_CMD_DONE));
> > > > +
> > > > +  if (Data & ErrMask) {
> > > > +    return EFI_DEVICE_ERROR;
> > > > +  }
> > > > +
> > > >    return EFI_SUCCESS;
> > > >  }
> > > >
> > > > @@ -550,8 +547,9 @@ DwEmmcReadBlockData (
> > > >    )
> > > >  {
> > > >    EFI_STATUS  Status;
> > > > -  UINT32      DescPages, CountPerPage, Count;
> > > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > > >    EFI_TPL     Tpl;
> > > > +  UINTN Rintsts = 0;
> > > >
> > > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > >
> > > > @@ -574,6 +572,18 @@ DwEmmcReadBlockData (
> > > >      DEBUG ((DEBUG_ERROR, "Failed to read data,
> > mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> mDwEmmcCommand,
> > mDwEmmcArgument, Status));
> > > >      goto out;
> > > >    }
> > > > +
> > > > +  while(!((MmioRead32(DWEMMC_RINTSTS) &
> (DWEMMC_INT_DTO))))
> > {
> > > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> > DWEMMC_INT_DCRC |
> > > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > > +
> > > > +  if (Rintsts & ErrMask) {
> > > > +    Status = EFI_DEVICE_ERROR;
> > > > +    goto out;
> > > > +  }
> > > >  out:
> > > >    // Restore Tpl
> > > >    gBS->RestoreTPL (Tpl);
> > > > @@ -589,8 +599,9 @@ DwEmmcWriteBlockData (
> > > >    )
> > > >  {
> > > >    EFI_STATUS  Status;
> > > > -  UINT32      DescPages, CountPerPage, Count;
> > > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > > >    EFI_TPL     Tpl;
> > > > +  UINTN Rintsts = 0;
> > > >
> > > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > >
> > > > @@ -613,6 +624,18 @@ DwEmmcWriteBlockData (
> > > >      DEBUG ((DEBUG_ERROR, "Failed to write data,
> > mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> mDwEmmcCommand,
> > mDwEmmcArgument, Status));
> > > >      goto out;
> > > >    }
> > > > +
> > > > +  while(!((MmioRead32(DWEMMC_RINTSTS) &
> (DWEMMC_INT_DTO))))
> > {
> > > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> > DWEMMC_INT_DCRC |
> > > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > > +
> > > > +  if (Rintsts & ErrMask) {
> > > > +    Status = EFI_DEVICE_ERROR;
> > > > +    goto out;
> > > > +  }
> > > >  out:
> > > >    // Restore Tpl
> > > >    gBS->RestoreTPL (Tpl);
> > > > --
> > > > 2.19.0
> > > >

Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling

[Leif Lindholm]

On Tue, Jun 11, 2019 at 02:40:51AM +0000, Loh, Tien Hock wrote:
> Leif,
> 
> Since Haojian have a newer driver model that uses the
> NonDiscoverableDeviceDxe, I believe we should be moving to use the
> new driver.
>
> I'll try to test out the patches submitted by Haojian in the mean time. 
> Can you help review the patch? Thanks!

I can have another look at the patch, but I would really appreciate if
you could also review it please?

My problem is that I really don't have much understanding of SD/MMC
protocols.

/
    Leif

> > -----Original Message-----
> > From: Loh, Tien Hock
> > Sent: Thursday, May 30, 2019 3:56 PM
> > To: Haojian Zhuang <haojian.zhuang@linaro.org>; Leif Lindholm
> > <leif.lindholm@linaro.org>
> > Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>
> > Subject: RE: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> > polling
> > 
> > > -----Original Message-----
> > > From: Haojian Zhuang <haojian.zhuang@linaro.org>
> > > Sent: Thursday, May 30, 2019 3:06 PM
> > > To: Leif Lindholm <leif.lindholm@linaro.org>
> > > Cc: Loh, Tien Hock <tien.hock.loh@intel.com>; devel@edk2.groups.io;
> > > thloh85@gmail.com; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Subject: Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> > > polling
> > >
> > > On Tue, May 28, 2019 at 07:04:09PM +0100, Leif Lindholm wrote:
> > > > +Haojian,
> > > >
> > > > Haojian - since you are the original author, can you comment on the
> > > > delays? Are these silicon bug workarounds (so we need to add a Pcd),
> > > > or does these changes work on your platforms too?
> > >
> > > I'm not in the loop, so I missed the patch series.
> > >
> > > The patch series can't work on my platform for the eMMC. Although a
> > > variable is created to identify whether it's a SD or eMMC device, it
> > > doesn't identify the eMMC device by the right way. So the eMMC device
> > > isn't initialized successfully on my platform.
> > >
> > > 1. Since MMC framework could identify whether it's eMMC device or SD
> > > device, we need to make device driver gets this kind of information
> > > from the MMC framework. And we need to support multiple eMMC/SD
> > > instances in MMC framework.
> > 
> > Yeah my bad I didn't read through the SD/MMC specs on that. Now I check
> > the specs and you're right, the information should be read from MMC
> > framework.
> > 
> > >
> > > 2. I sent a patch series to support both eMMC device and SD device before.
> > > https://edk2.groups.io/g/devel/message/28572
> > > &&
> > > https://edk2.groups.io/g/devel/message/28615
> > > Maybe it's missed. Could you help to review that patch series?
> 
> Leif, can you help review the patch series? Since Haojian have moved the driver to NonDiscoverableDeviceDxe, I think that would be a more appropriate driver to be used going forward. Thanks!
> 
> > 
> > >
> > > Best Regards
> > > Haojian
> > >
> > > >
> > > > Regards,
> > > >
> > > > Leif
> > > >
> > > > On Mon, May 27, 2019 at 05:30:27PM +0800, tien.hock.loh@intel.com
> > > wrote:
> > > > > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > > > >
> > > > > Change SendCommand polling mode to remove unnecessary delay, and
> > > > > check for transfer done only when block data is to be read/write.
> > > > > This would also increase performance slightly.
> > > > >
> > > > > 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 | 43
> > > +++++++++++++++-----
> > > > >  1 file changed, 33 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > index c6c8e04917..b57833458f 100644
> > > > > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > @@ -286,16 +286,13 @@ SendCommand (
> > > > >              DWEMMC_INT_RCRC | DWEMMC_INT_RE;
> > > > >    ErrMask |= DWEMMC_INT_DCRC | DWEMMC_INT_DRT |
> > > DWEMMC_INT_SBE;
> > > > >    do {
> > > > > -    MicroSecondDelay(500);
> > > > >      Data = MmioRead32 (DWEMMC_RINTSTS);
> > > > > -
> > > > > -    if (Data & ErrMask) {
> > > > > -      return EFI_DEVICE_ERROR;
> > > > > -    }
> > > > > -    if (Data & DWEMMC_INT_DTO) {     // Transfer Done
> > > > > -      break;
> > > > > -    }
> > > > >    } while (!(Data & DWEMMC_INT_CMD_DONE));
> > > > > +
> > > > > +  if (Data & ErrMask) {
> > > > > +    return EFI_DEVICE_ERROR;
> > > > > +  }
> > > > > +
> > > > >    return EFI_SUCCESS;
> > > > >  }
> > > > >
> > > > > @@ -550,8 +547,9 @@ DwEmmcReadBlockData (
> > > > >    )
> > > > >  {
> > > > >    EFI_STATUS  Status;
> > > > > -  UINT32      DescPages, CountPerPage, Count;
> > > > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > > > >    EFI_TPL     Tpl;
> > > > > +  UINTN Rintsts = 0;
> > > > >
> > > > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >
> > > > > @@ -574,6 +572,18 @@ DwEmmcReadBlockData (
> > > > >      DEBUG ((DEBUG_ERROR, "Failed to read data,
> > > mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> > mDwEmmcCommand,
> > > mDwEmmcArgument, Status));
> > > > >      goto out;
> > > > >    }
> > > > > +
> > > > > +  while(!((MmioRead32(DWEMMC_RINTSTS) &
> > (DWEMMC_INT_DTO))))
> > > {
> > > > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > > > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> > > DWEMMC_INT_DCRC |
> > > > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > > > +
> > > > > +  if (Rintsts & ErrMask) {
> > > > > +    Status = EFI_DEVICE_ERROR;
> > > > > +    goto out;
> > > > > +  }
> > > > >  out:
> > > > >    // Restore Tpl
> > > > >    gBS->RestoreTPL (Tpl);
> > > > > @@ -589,8 +599,9 @@ DwEmmcWriteBlockData (
> > > > >    )
> > > > >  {
> > > > >    EFI_STATUS  Status;
> > > > > -  UINT32      DescPages, CountPerPage, Count;
> > > > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > > > >    EFI_TPL     Tpl;
> > > > > +  UINTN Rintsts = 0;
> > > > >
> > > > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > > >
> > > > > @@ -613,6 +624,18 @@ DwEmmcWriteBlockData (
> > > > >      DEBUG ((DEBUG_ERROR, "Failed to write data,
> > > mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> > mDwEmmcCommand,
> > > mDwEmmcArgument, Status));
> > > > >      goto out;
> > > > >    }
> > > > > +
> > > > > +  while(!((MmioRead32(DWEMMC_RINTSTS) &
> > (DWEMMC_INT_DTO))))
> > > {
> > > > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > > > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> > > DWEMMC_INT_DCRC |
> > > > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > > > +
> > > > > +  if (Rintsts & ErrMask) {
> > > > > +    Status = EFI_DEVICE_ERROR;
> > > > > +    goto out;
> > > > > +  }
> > > > >  out:
> > > > >    // Restore Tpl
> > > > >    gBS->RestoreTPL (Tpl);
> > > > > --
> > > > > 2.19.0
> > > > >

Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling

[Loh, Tien Hock]

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Tuesday, June 11, 2019 5:09 PM
> To: Loh, Tien Hock <tien.hock.loh@intel.com>
> Cc: 'Haojian Zhuang' <haojian.zhuang@linaro.org>; 'devel@edk2.groups.io'
> <devel@edk2.groups.io>; 'thloh85@gmail.com' <thloh85@gmail.com>; 'Ard
> Biesheuvel' <ard.biesheuvel@linaro.org>
> Subject: Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> polling
> 
> On Tue, Jun 11, 2019 at 02:40:51AM +0000, Loh, Tien Hock wrote:
> > Leif,
> >
> > Since Haojian have a newer driver model that uses the
> > NonDiscoverableDeviceDxe, I believe we should be moving to use the new
> > driver.
> >
> > I'll try to test out the patches submitted by Haojian in the mean time.
> > Can you help review the patch? Thanks!
> 
> I can have another look at the patch, but I would really appreciate if you
> could also review it please?
> 
> My problem is that I really don't have much understanding of SD/MMC
> protocols.

Sure. I'll test it out on the SoCFPGA platform first. It is quite a long patch, so I might take a bit of time to review.
Thanks Leif!

> 
> /
>     Leif
> 
> > > -----Original Message-----
> > > From: Loh, Tien Hock
> > > Sent: Thursday, May 30, 2019 3:56 PM
> > > To: Haojian Zhuang <haojian.zhuang@linaro.org>; Leif Lindholm
> > > <leif.lindholm@linaro.org>
> > > Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org>
> > > Subject: RE: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> > > polling
> > >
> > > > -----Original Message-----
> > > > From: Haojian Zhuang <haojian.zhuang@linaro.org>
> > > > Sent: Thursday, May 30, 2019 3:06 PM
> > > > To: Leif Lindholm <leif.lindholm@linaro.org>
> > > > Cc: Loh, Tien Hock <tien.hock.loh@intel.com>;
> > > > devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> > > > <ard.biesheuvel@linaro.org>
> > > > Subject: Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> > > > polling
> > > >
> > > > On Tue, May 28, 2019 at 07:04:09PM +0100, Leif Lindholm wrote:
> > > > > +Haojian,
> > > > >
> > > > > Haojian - since you are the original author, can you comment on
> > > > > the delays? Are these silicon bug workarounds (so we need to add
> > > > > a Pcd), or does these changes work on your platforms too?
> > > >
> > > > I'm not in the loop, so I missed the patch series.
> > > >
> > > > The patch series can't work on my platform for the eMMC. Although
> > > > a variable is created to identify whether it's a SD or eMMC
> > > > device, it doesn't identify the eMMC device by the right way. So
> > > > the eMMC device isn't initialized successfully on my platform.
> > > >
> > > > 1. Since MMC framework could identify whether it's eMMC device or
> > > > SD device, we need to make device driver gets this kind of
> > > > information from the MMC framework. And we need to support
> > > > multiple eMMC/SD instances in MMC framework.
> > >
> > > Yeah my bad I didn't read through the SD/MMC specs on that. Now I
> > > check the specs and you're right, the information should be read
> > > from MMC framework.
> > >
> > > >
> > > > 2. I sent a patch series to support both eMMC device and SD device
> before.
> > > > https://edk2.groups.io/g/devel/message/28572
> > > > &&
> > > > https://edk2.groups.io/g/devel/message/28615
> > > > Maybe it's missed. Could you help to review that patch series?
> >
> > Leif, can you help review the patch series? Since Haojian have moved the
> driver to NonDiscoverableDeviceDxe, I think that would be a more
> appropriate driver to be used going forward. Thanks!
> >
> > >
> > > >
> > > > Best Regards
> > > > Haojian
> > > >
> > > > >
> > > > > Regards,
> > > > >
> > > > > Leif
> > > > >
> > > > > On Mon, May 27, 2019 at 05:30:27PM +0800,
> > > > > tien.hock.loh@intel.com
> > > > wrote:
> > > > > > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > > > > >
> > > > > > Change SendCommand polling mode to remove unnecessary delay,
> > > > > > and check for transfer done only when block data is to be
> read/write.
> > > > > > This would also increase performance slightly.
> > > > > >
> > > > > > 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 | 43
> > > > +++++++++++++++-----
> > > > > >  1 file changed, 33 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > > index c6c8e04917..b57833458f 100644
> > > > > > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > > @@ -286,16 +286,13 @@ SendCommand (
> > > > > >              DWEMMC_INT_RCRC | DWEMMC_INT_RE;
> > > > > >    ErrMask |= DWEMMC_INT_DCRC | DWEMMC_INT_DRT |
> > > > DWEMMC_INT_SBE;
> > > > > >    do {
> > > > > > -    MicroSecondDelay(500);
> > > > > >      Data = MmioRead32 (DWEMMC_RINTSTS);
> > > > > > -
> > > > > > -    if (Data & ErrMask) {
> > > > > > -      return EFI_DEVICE_ERROR;
> > > > > > -    }
> > > > > > -    if (Data & DWEMMC_INT_DTO) {     // Transfer Done
> > > > > > -      break;
> > > > > > -    }
> > > > > >    } while (!(Data & DWEMMC_INT_CMD_DONE));
> > > > > > +
> > > > > > +  if (Data & ErrMask) {
> > > > > > +    return EFI_DEVICE_ERROR;
> > > > > > +  }
> > > > > > +
> > > > > >    return EFI_SUCCESS;
> > > > > >  }
> > > > > >
> > > > > > @@ -550,8 +547,9 @@ DwEmmcReadBlockData (
> > > > > >    )
> > > > > >  {
> > > > > >    EFI_STATUS  Status;
> > > > > > -  UINT32      DescPages, CountPerPage, Count;
> > > > > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > > > > >    EFI_TPL     Tpl;
> > > > > > +  UINTN Rintsts = 0;
> > > > > >
> > > > > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > > > >
> > > > > > @@ -574,6 +572,18 @@ DwEmmcReadBlockData (
> > > > > >      DEBUG ((DEBUG_ERROR, "Failed to read data,
> > > > mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> > > mDwEmmcCommand,
> > > > mDwEmmcArgument, Status));
> > > > > >      goto out;
> > > > > >    }
> > > > > > +
> > > > > > +  while(!((MmioRead32(DWEMMC_RINTSTS) &
> > > (DWEMMC_INT_DTO))))
> > > > {
> > > > > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > > > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > > > > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> > > > DWEMMC_INT_DCRC |
> > > > > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > > > > +
> > > > > > +  if (Rintsts & ErrMask) {
> > > > > > +    Status = EFI_DEVICE_ERROR;
> > > > > > +    goto out;
> > > > > > +  }
> > > > > >  out:
> > > > > >    // Restore Tpl
> > > > > >    gBS->RestoreTPL (Tpl);
> > > > > > @@ -589,8 +599,9 @@ DwEmmcWriteBlockData (
> > > > > >    )
> > > > > >  {
> > > > > >    EFI_STATUS  Status;
> > > > > > -  UINT32      DescPages, CountPerPage, Count;
> > > > > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > > > > >    EFI_TPL     Tpl;
> > > > > > +  UINTN Rintsts = 0;
> > > > > >
> > > > > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > > > >
> > > > > > @@ -613,6 +624,18 @@ DwEmmcWriteBlockData (
> > > > > >      DEBUG ((DEBUG_ERROR, "Failed to write data,
> > > > mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> > > mDwEmmcCommand,
> > > > mDwEmmcArgument, Status));
> > > > > >      goto out;
> > > > > >    }
> > > > > > +
> > > > > > +  while(!((MmioRead32(DWEMMC_RINTSTS) &
> > > (DWEMMC_INT_DTO))))
> > > > {
> > > > > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > > > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO |
> > > > > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> > > > DWEMMC_INT_DCRC |
> > > > > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > > > > +
> > > > > > +  if (Rintsts & ErrMask) {
> > > > > > +    Status = EFI_DEVICE_ERROR;
> > > > > > +    goto out;
> > > > > > +  }
> > > > > >  out:
> > > > > >    // Restore Tpl
> > > > > >    gBS->RestoreTPL (Tpl);
> > > > > > --
> > > > > > 2.19.0
> > > > > >

Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling

[Loh, Tien Hock]

Leif,
Some comments inlined.

> -----Original Message-----
> From: Loh, Tien Hock
> Sent: Tuesday, June 11, 2019 5:12 PM
> To: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: 'Haojian Zhuang' <haojian.zhuang@linaro.org>; 'devel@edk2.groups.io'
> <devel@edk2.groups.io>; 'thloh85@gmail.com' <thloh85@gmail.com>; 'Ard
> Biesheuvel' <ard.biesheuvel@linaro.org>
> Subject: RE: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> polling
> 
> > -----Original Message-----
> > From: Leif Lindholm <leif.lindholm@linaro.org>
> > Sent: Tuesday, June 11, 2019 5:09 PM
> > To: Loh, Tien Hock <tien.hock.loh@intel.com>
> > Cc: 'Haojian Zhuang' <haojian.zhuang@linaro.org>; 'devel@edk2.groups.io'
> > <devel@edk2.groups.io>; 'thloh85@gmail.com' <thloh85@gmail.com>;
> 'Ard
> > Biesheuvel' <ard.biesheuvel@linaro.org>
> > Subject: Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> > polling
> >
> > On Tue, Jun 11, 2019 at 02:40:51AM +0000, Loh, Tien Hock wrote:
> > > Leif,
> > >
> > > Since Haojian have a newer driver model that uses the
> > > NonDiscoverableDeviceDxe, I believe we should be moving to use the
> > > new driver.
> > >
> > > I'll try to test out the patches submitted by Haojian in the mean time.
> > > Can you help review the patch? Thanks!
> >
> > I can have another look at the patch, but I would really appreciate if
> > you could also review it please?
> >
> > My problem is that I really don't have much understanding of SD/MMC
> > protocols.
> 
> Sure. I'll test it out on the SoCFPGA platform first. It is quite a long patch, so I
> might take a bit of time to review.
> Thanks Leif!

I've reviewed and tested the driver. For SD portion the patch works correctly on the SoCFPGA platform. How should I ACK the patch?

> 
> >
> > /
> >     Leif
> >
> > > > -----Original Message-----
> > > > From: Loh, Tien Hock
> > > > Sent: Thursday, May 30, 2019 3:56 PM
> > > > To: Haojian Zhuang <haojian.zhuang@linaro.org>; Leif Lindholm
> > > > <leif.lindholm@linaro.org>
> > > > Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> > > > <ard.biesheuvel@linaro.org>
> > > > Subject: RE: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand
> > > > polling
> > > >
> > > > > -----Original Message-----
> > > > > From: Haojian Zhuang <haojian.zhuang@linaro.org>
> > > > > Sent: Thursday, May 30, 2019 3:06 PM
> > > > > To: Leif Lindholm <leif.lindholm@linaro.org>
> > > > > Cc: Loh, Tien Hock <tien.hock.loh@intel.com>;
> > > > > devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> > > > > <ard.biesheuvel@linaro.org>
> > > > > Subject: Re: [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc
> SendCommand
> > > > > polling
> > > > >
> > > > > On Tue, May 28, 2019 at 07:04:09PM +0100, Leif Lindholm wrote:
> > > > > > +Haojian,
> > > > > >
> > > > > > Haojian - since you are the original author, can you comment
> > > > > > on the delays? Are these silicon bug workarounds (so we need
> > > > > > to add a Pcd), or does these changes work on your platforms too?
> > > > >
> > > > > I'm not in the loop, so I missed the patch series.
> > > > >
> > > > > The patch series can't work on my platform for the eMMC.
> > > > > Although a variable is created to identify whether it's a SD or
> > > > > eMMC device, it doesn't identify the eMMC device by the right
> > > > > way. So the eMMC device isn't initialized successfully on my platform.
> > > > >
> > > > > 1. Since MMC framework could identify whether it's eMMC device
> > > > > or SD device, we need to make device driver gets this kind of
> > > > > information from the MMC framework. And we need to support
> > > > > multiple eMMC/SD instances in MMC framework.
> > > >
> > > > Yeah my bad I didn't read through the SD/MMC specs on that. Now I
> > > > check the specs and you're right, the information should be read
> > > > from MMC framework.
> > > >
> > > > >
> > > > > 2. I sent a patch series to support both eMMC device and SD
> > > > > device
> > before.
> > > > > https://edk2.groups.io/g/devel/message/28572
> > > > > &&
> > > > > https://edk2.groups.io/g/devel/message/28615
> > > > > Maybe it's missed. Could you help to review that patch series?
> > >
> > > Leif, can you help review the patch series? Since Haojian have moved
> > > the
> > driver to NonDiscoverableDeviceDxe, I think that would be a more
> > appropriate driver to be used going forward. Thanks!
> > >
> > > >
> > > > >
> > > > > Best Regards
> > > > > Haojian
> > > > >
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Leif
> > > > > >
> > > > > > On Mon, May 27, 2019 at 05:30:27PM +0800,
> > > > > > tien.hock.loh@intel.com
> > > > > wrote:
> > > > > > > From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> > > > > > >
> > > > > > > Change SendCommand polling mode to remove unnecessary
> delay,
> > > > > > > and check for transfer done only when block data is to be
> > read/write.
> > > > > > > This would also increase performance slightly.
> > > > > > >
> > > > > > > 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 | 43
> > > > > +++++++++++++++-----
> > > > > > >  1 file changed, 33 insertions(+), 10 deletions(-)
> > > > > > >
> > > > > > > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > > > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > > > index c6c8e04917..b57833458f 100644
> > > > > > > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > > > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > > > > > > @@ -286,16 +286,13 @@ SendCommand (
> > > > > > >              DWEMMC_INT_RCRC | DWEMMC_INT_RE;
> > > > > > >    ErrMask |= DWEMMC_INT_DCRC | DWEMMC_INT_DRT |
> > > > > DWEMMC_INT_SBE;
> > > > > > >    do {
> > > > > > > -    MicroSecondDelay(500);
> > > > > > >      Data = MmioRead32 (DWEMMC_RINTSTS);
> > > > > > > -
> > > > > > > -    if (Data & ErrMask) {
> > > > > > > -      return EFI_DEVICE_ERROR;
> > > > > > > -    }
> > > > > > > -    if (Data & DWEMMC_INT_DTO) {     // Transfer Done
> > > > > > > -      break;
> > > > > > > -    }
> > > > > > >    } while (!(Data & DWEMMC_INT_CMD_DONE));
> > > > > > > +
> > > > > > > +  if (Data & ErrMask) {
> > > > > > > +    return EFI_DEVICE_ERROR;  }
> > > > > > > +
> > > > > > >    return EFI_SUCCESS;
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -550,8 +547,9 @@ DwEmmcReadBlockData (
> > > > > > >    )
> > > > > > >  {
> > > > > > >    EFI_STATUS  Status;
> > > > > > > -  UINT32      DescPages, CountPerPage, Count;
> > > > > > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > > > > > >    EFI_TPL     Tpl;
> > > > > > > +  UINTN Rintsts = 0;
> > > > > > >
> > > > > > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > > > > >
> > > > > > > @@ -574,6 +572,18 @@ DwEmmcReadBlockData (
> > > > > > >      DEBUG ((DEBUG_ERROR, "Failed to read data,
> > > > > mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> > > > mDwEmmcCommand,
> > > > > mDwEmmcArgument, Status));
> > > > > > >      goto out;
> > > > > > >    }
> > > > > > > +
> > > > > > > +  while(!((MmioRead32(DWEMMC_RINTSTS) &
> > > > (DWEMMC_INT_DTO))))
> > > > > {
> > > > > > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > > > > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO
> |
> > > > > > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> > > > > DWEMMC_INT_DCRC |
> > > > > > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > > > > > +
> > > > > > > +  if (Rintsts & ErrMask) {
> > > > > > > +    Status = EFI_DEVICE_ERROR;
> > > > > > > +    goto out;
> > > > > > > +  }
> > > > > > >  out:
> > > > > > >    // Restore Tpl
> > > > > > >    gBS->RestoreTPL (Tpl);
> > > > > > > @@ -589,8 +599,9 @@ DwEmmcWriteBlockData (
> > > > > > >    )
> > > > > > >  {
> > > > > > >    EFI_STATUS  Status;
> > > > > > > -  UINT32      DescPages, CountPerPage, Count;
> > > > > > > +  UINT32      DescPages, CountPerPage, Count, ErrMask;
> > > > > > >    EFI_TPL     Tpl;
> > > > > > > +  UINTN Rintsts = 0;
> > > > > > >
> > > > > > >    Tpl = gBS->RaiseTPL (TPL_NOTIFY);
> > > > > > >
> > > > > > > @@ -613,6 +624,18 @@ DwEmmcWriteBlockData (
> > > > > > >      DEBUG ((DEBUG_ERROR, "Failed to write data,
> > > > > mDwEmmcCommand:%x, mDwEmmcArgument:%x, Status:%r\n",
> > > > mDwEmmcCommand,
> > > > > mDwEmmcArgument, Status));
> > > > > > >      goto out;
> > > > > > >    }
> > > > > > > +
> > > > > > > +  while(!((MmioRead32(DWEMMC_RINTSTS) &
> > > > (DWEMMC_INT_DTO))))
> > > > > {
> > > > > > > +    Rintsts = MmioRead32 (DWEMMC_RINTSTS);  }  ErrMask =
> > > > > > > + DWEMMC_INT_EBE | DWEMMC_INT_HLE | DWEMMC_INT_RTO
> |
> > > > > > > +            DWEMMC_INT_RCRC | DWEMMC_INT_RE |
> > > > > DWEMMC_INT_DCRC |
> > > > > > > +            DWEMMC_INT_DRT | DWEMMC_INT_SBE;
> > > > > > > +
> > > > > > > +  if (Rintsts & ErrMask) {
> > > > > > > +    Status = EFI_DEVICE_ERROR;
> > > > > > > +    goto out;
> > > > > > > +  }
> > > > > > >  out:
> > > > > > >    // Restore Tpl
> > > > > > >    gBS->RestoreTPL (Tpl);
> > > > > > > --
> > > > > > > 2.19.0
> > > > > > >

[PATCH v2 7/7] EmbeddedPkg: Fix DwEmmc read/write size in preparing DMA size

[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>

--
v2:
- Fix white space issue
---
 EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
index b57833458f..ec2fa7923b 100644
--- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
+++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
@@ -493,7 +493,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 |
@@ -534,8 +537,14 @@ StartDma (
   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.19.0

Re: [PATCH v2 7/7] EmbeddedPkg: Fix DwEmmc read/write size in preparing DMA size

[Leif Lindholm]

On Mon, May 27, 2019 at 05:30:28PM +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>
> 
> --
> v2:
> - Fix white space issue

Drop the version blurb from the commit message.
Below --- only.

> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index b57833458f..ec2fa7923b 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -493,7 +493,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 |
> @@ -534,8 +537,14 @@ StartDma (
>    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);

There is still a duplicated line here

> +  }
> +  else {

} else {

> +    MmioWrite32 (DWEMMC_BLKSIZ, DWEMMC_BLOCK_SIZE);
> +    MmioWrite32 (DWEMMC_BYTCNT, Length);

and here

> +  }

Which could be outside of the conditional clause, since it is always
executed the same.

/
    Leif

>  }
>  
>  EFI_STATUS
> -- 
> 2.19.0
>