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

[Haojian Zhuang <haojian.zhuang@linaro.org>]

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