1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs

Loh, Tien Hock posted 1 patch 4 years, 11 months ago
Failed in applying to current master (apply log)
EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
[edk2-devel] [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs
Posted by Loh, Tien Hock 4 years, 11 months ago 
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;
 
 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 ;
+    }
+    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 (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 (#39935): https://edk2.groups.io/g/devel/message/39935
Mute This Topic: https://groups.io/mt/31480077/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs
Posted by Leif Lindholm 4 years, 11 months ago 
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?

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

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

> +    }
> +    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.

/
    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 (#39955): https://edk2.groups.io/g/devel/message/39955
Mute This Topic: https://groups.io/mt/31480077/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs
Posted by Loh, Tien Hock 4 years, 11 months ago 
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
> >

-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.