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

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

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