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

["Loh, Tien Hock" <tien.hock.loh@intel.com>]

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