1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v2 0/7] Fix some bugs with DwEmmc
  4. View patch

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

Loh, Tien Hock posted 7 patches 6 years, 8 months ago
[edk2-devel] [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling
Posted by Loh, Tien Hock 6 years, 8 months ago 
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41414): https://edk2.groups.io/g/devel/message/41414
Mute This Topic: https://groups.io/mt/31807965/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling
Posted by Leif Lindholm 6 years, 8 months ago 
+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
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41539): https://edk2.groups.io/g/devel/message/41539
Mute This Topic: https://groups.io/mt/31807965/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling
Posted by Haojian Zhuang 6 years, 8 months ago 
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
> > 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41687): https://edk2.groups.io/g/devel/message/41687
Mute This Topic: https://groups.io/mt/31807965/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling
Posted by Loh, Tien Hock 6 years, 8 months ago 
> -----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
> > >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41667): https://edk2.groups.io/g/devel/message/41667
Mute This Topic: https://groups.io/mt/31807965/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling
Posted by Loh, Tien Hock 6 years, 8 months ago 
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
> > > >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42156): https://edk2.groups.io/g/devel/message/42156
Mute This Topic: https://groups.io/mt/31807965/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling
Posted by Leif Lindholm 6 years, 8 months ago 
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
> > > > >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42194): https://edk2.groups.io/g/devel/message/42194
Mute This Topic: https://groups.io/mt/31807965/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling
Posted by Loh, Tien Hock 6 years, 8 months ago 
> -----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
> > > > > >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42195): https://edk2.groups.io/g/devel/message/42195
Mute This Topic: https://groups.io/mt/31807965/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/7] EmbeddedPkg: Fix DwEmmc SendCommand polling
Posted by Loh, Tien Hock 6 years, 6 months ago 
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
> > > > > > >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43639): https://edk2.groups.io/g/devel/message/43639
Mute This Topic: https://groups.io/mt/31807965/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.