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

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Fri, May 03, 2019 at 11:26:59AM +0800, tien.hock.loh@intel.com wrote:
> From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> 
> On CMD8, for SD, the controller should not expect data as this is a
> SEND_IF_COND command to verify SD operating condition, and does not have
> 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        | 9 ++++++---
>  EmbeddedPkg/Include/Protocol/MmcHost.h           | 1 +
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 2 ++

If you follow the guidelines at
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers
when generating patches, that means interface changes (like for the .h
file here) get reviewed before their uses, which makes for a much more
natural review flow.

>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index 058665b..04fdcbf 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -339,9 +339,12 @@ DwEmmcSendCommand (
>          Cmd = 0;
>      break;
>    case MMC_INDX(8):
> -    Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
> -           BIT_CMD_DATA_EXPECTED | BIT_CMD_READ |
> -           BIT_CMD_WAIT_PRVDATA_COMPLETE;
> +    if (!This->IsEmmc)
> +      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
> +             BIT_CMD_WAIT_PRVDATA_COMPLETE ;
> +    else
> +      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
> +             BIT_CMD_WAIT_PRVDATA_COMPLETE | BIT_CMD_READ | BIT_CMD_DATA_EXPECTED;

I think this would be more clear as
       Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
             BIT_CMD_WAIT_PRVDATA_COMPLETE;
       if (...) {
         Cmd |= BIT_CMD_READ | BIT_CMD_DATA_EXPECTED;
       }

>      break;
>    case MMC_INDX(9):
>      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
> diff --git a/EmbeddedPkg/Include/Protocol/MmcHost.h b/EmbeddedPkg/Include/Protocol/MmcHost.h
> index 9e07082..ae8ea5d 100644
> --- a/EmbeddedPkg/Include/Protocol/MmcHost.h
> +++ b/EmbeddedPkg/Include/Protocol/MmcHost.h
> @@ -169,6 +169,7 @@ struct _EFI_MMC_HOST_PROTOCOL {
>    MMC_SETIOS              SetIos;
>    MMC_ISMULTIBLOCK        IsMultiBlock;
>  
> +  BOOLEAN                 IsEmmc;

Case in point w.r.t. natural order.

But I don't think a BOOLEAN is the right solution here. (We would just
need to add another for each device type special case handling.)

Could you instead add an EFI_MMC_HOST_CARD_TYPE enum like in
EmbeddedPkg/Universal/MmcDxe/Mmc.h?

/
    Leif

>  };
>  
>  #define MMC_HOST_PROTOCOL_REVISION    0x00010002    // 1.2
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 4dc0be1..fa1eda2 100755
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -770,8 +770,10 @@ InitializeMmcDevice (
>    }
>  
>    if (MmcHostInstance->CardInfo.CardType != EMMC_CARD) {
> +    MmcHostInstance->MmcHost->IsEmmc = FALSE;
>      Status = InitializeSdMmcDevice (MmcHostInstance);
>    } else {
> +    MmcHostInstance->MmcHost->IsEmmc = TRUE;
>      Status = InitializeEmmcDevice (MmcHostInstance);
>    }
>    if (EFI_ERROR (Status)) {
> -- 
> 2.2.2
>