From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.65, mailfrom: tien.hock.loh@intel.com) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by groups.io with SMTP; Tue, 07 May 2019 19:39:06 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 May 2019 19:39:05 -0700 X-ExtLoop1: 1 Received: from kmsmsx154.gar.corp.intel.com ([172.21.73.14]) by orsmga001.jf.intel.com with ESMTP; 07 May 2019 19:39:04 -0700 Received: from pgsmsx110.gar.corp.intel.com ([169.254.13.159]) by KMSMSX154.gar.corp.intel.com ([169.254.12.227]) with mapi id 14.03.0415.000; Wed, 8 May 2019 10:39:00 +0800 From: "Loh, Tien Hock" To: Leif Lindholm CC: "devel@edk2.groups.io" , "thloh85@gmail.com" , Ard Biesheuvel Subject: Re: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs Thread-Topic: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs Thread-Index: AQHVAWBFQsseqZDKyEin0DvdnPsBPKZYxFcAgAfAXPA= Date: Wed, 8 May 2019 02:39:00 +0000 Message-ID: References: <1556854023-5486-1-git-send-email-tien.hock.loh@intel.com> <1556854023-5486-2-git-send-email-tien.hock.loh@intel.com> <20190503115154.zc6x37xupwl7v7jr@bivouac.eciton.net> In-Reply-To: <20190503115154.zc6x37xupwl7v7jr@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDdjZDBmMjQtYjYyMC00NGU0LWFmYmQtMTNiNGQxY2QzZGRmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiQzgwMkFnaVpRXC9GeDVyT3pjMXBlVDllcFdhRTFtcWJNVDFLRXZZem9JeTV2djJtSUVibEx3NWpKdmd2ZG96RW8ifQ== x-originating-ip: [172.30.20.205] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Leif, Sorry for the late reply, I were held up in other areas of the work. > -----Original Message----- > From: Leif Lindholm > Sent: Friday, May 3, 2019 7:52 PM > To: Loh, Tien Hock > Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel > > Subject: Re: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs >=20 > Hi Tien Hock, >=20 > 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 li= nes > back as well? OK noted. My bad, I thought the content of the commit would suffice. >=20 > Most of my comments on this series are purely coding style related, a cou= ple > are not. >=20 > On Fri, May 03, 2019 at 11:26:57AM +0800, tien.hock.loh@intel.com wrote: > > From: "Tien Hock, Loh" > > > > 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" > > Cc: Leif Lindholm > > Cc: Ard Biesheuvel > > --- > > 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 =3D EFI_CALLER_ID_GUID; STATIC > UINT32 > > mDwEmmcCommand; STATIC UINT32 mDwEmmcArgument; > > +STATIC BOOLEAN mIsACmd =3D FALSE; >=20 > Could we move this variable into DwEmmcSendCommand and drop the 'm'? OK, so static variable in the function it is. >=20 > > > > EFI_STATUS > > DwEmmcReadBlockData ( > > @@ -321,6 +322,15 @@ DwEmmcSendCommand ( > > Cmd =3D BIT_CMD_RESPONSE_EXPECT | > BIT_CMD_CHECK_RESPONSE_CRC | > > BIT_CMD_SEND_INIT; > > break; > > + case MMC_INDX(6): > > + if(mIsACmd) { > > + Cmd =3D BIT_CMD_RESPONSE_EXPECT ; >=20 > Drop space before ;. OK >=20 > > + } > > + else { >=20 > } else { >=20 > > + Cmd =3D BIT_CMD_RESPONSE_EXPECT | BIT_CMD_DATA_EXPECTED | > > + BIT_CMD_READ; > > + } > > + break; > > case MMC_INDX(7): > > if (Argument) > > Cmd =3D BIT_CMD_RESPONSE_EXPECT | > BIT_CMD_CHECK_RESPONSE_CRC; > > @@ -371,6 +381,11 @@ DwEmmcSendCommand ( > > } > > > > Cmd |=3D MMC_GET_INDX(MmcCmd) | BIT_CMD_USE_HOLD_REG | > BIT_CMD_START; > > + > > + if(MMC_INDX(55) =3D=3D MMC_GET_INDX(MmcCmd)) > > + mIsACmd =3D TRUE; > > + else > > + mIsACmd =3D FALSE; >=20 > if () { > } else { > } >=20 > 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. >=20 > I also think we should add IsACmd =3D FALSE to the default: case. OK, I'll fix this. Thanks for the review. >=20 > / > Leif >=20 > > if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) { > > mDwEmmcCommand =3D Cmd; > > mDwEmmcArgument =3D Argument; > > -- > > 2.2.2 > >