Re: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs

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

Hi Leif,

Sorry for the late reply, I were held up in other areas of the work.

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Friday, May 3, 2019 7:52 PM
> To: Loh, Tien Hock <tien.hock.loh@intel.com>
> Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs
> 
> Hi Tien Hock,
> 
> Thanks for splitting the patches back up, and sorry for taking so long to get
> around to reviewing. But could you please add some descriptive subject lines
> back as well?

OK noted. My bad, I thought the content of the commit would suffice.

> 
> Most of my comments on this series are purely coding style related, a couple
> are not.
> 
> On Fri, May 03, 2019 at 11:26:57AM +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>
> > ---
> >  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 15
> +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > index adc6b06..fa24802 100644
> > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > @@ -39,6 +39,7 @@ DWEMMC_IDMAC_DESCRIPTOR   *gpIdmacDesc;
> >  EFI_GUID mDwEmmcDevicePathGuid = EFI_CALLER_ID_GUID;  STATIC
> UINT32
> > mDwEmmcCommand;  STATIC UINT32 mDwEmmcArgument;
> > +STATIC BOOLEAN mIsACmd = FALSE;
> 
> Could we move this variable into DwEmmcSendCommand and drop the 'm'?
OK, so static variable in the function it is.

> 
> >
> >  EFI_STATUS
> >  DwEmmcReadBlockData (
> > @@ -321,6 +322,15 @@ DwEmmcSendCommand (
> >      Cmd = BIT_CMD_RESPONSE_EXPECT |
> BIT_CMD_CHECK_RESPONSE_CRC |
> >             BIT_CMD_SEND_INIT;
> >      break;
> > +  case MMC_INDX(6):
> > +    if(mIsACmd) {
> > +      Cmd = BIT_CMD_RESPONSE_EXPECT ;
> 
> Drop space before ;.
OK
> 
> > +    }
> > +    else {
> 
> } 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;
> > @@ -371,6 +381,11 @@ DwEmmcSendCommand (
> >    }
> >
> >    Cmd |= MMC_GET_INDX(MmcCmd) | BIT_CMD_USE_HOLD_REG |
> BIT_CMD_START;
> > +
> > +  if(MMC_INDX(55) == MMC_GET_INDX(MmcCmd))
> > +    mIsACmd = TRUE;
> > +  else
> > +    mIsACmd = FALSE;
> 
>   if () {
>   } else {
>   }
> 
> There should also be spaces between MMC_INDX and (55), as well as
> between MMC_GET_INDX (MmcCmd). Surrounding code does not follow
> the
> style, but I would still prefer to keep to this for new/modified code.
> 
> I also think we should add IsACmd = FALSE to the default: case.
OK, I'll fix this. Thanks for the review.

> 
> /
>     Leif
> 
> >    if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) {
> >      mDwEmmcCommand = Cmd;
> >      mDwEmmcArgument = Argument;
> > --
> > 2.2.2
> >