[edk2-devel] [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs
Loh, Tien Hock
tien.hock.loh at intel.com
Wed May 8 02:39:00 UTC 2019
Hi Leif,
Sorry for the late reply, I were held up in other areas of the work.
> -----Original Message-----
> From: Leif Lindholm <leif.lindholm at linaro.org>
> Sent: Friday, May 3, 2019 7:52 PM
> To: Loh, Tien Hock <tien.hock.loh at intel.com>
> Cc: devel at edk2.groups.io; thloh85 at gmail.com; Ard Biesheuvel
> <ard.biesheuvel at 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 at intel.com wrote:
> > From: "Tien Hock, Loh" <tien.hock.loh at 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 at intel.com>
> > Cc: Leif Lindholm <leif.lindholm at linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel at 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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#40164): https://edk2.groups.io/g/devel/message/40164
Mute This Topic: https://groups.io/mt/31480077/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list