Re: [PATCH 2/2] SD : Updated CMD 6 implememtation.

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

On Wed, Aug 30, 2017 at 07:50:59PM +0530, Meenakshi Aggarwal wrote:
> For setting high speed in SD card,
> First CMD 6 (Switch) is send to check if card supports High Speed and
> Second command is send to switch card to high speed mode.
> 
> In current inplementation, CMD 6 was sent only once to switch the
> card into HS mode without checking if card supports HS or not, which is
> not as per specification and also we are not setting the HS i.e. 50000000
> but directly asking the card to switch to 26000000 which is incorrect as
> SD card supports either 25000000 or 50000000.

Same as previous one: Jun, Haojian?

I do have a couple of style comments below.

> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> ---
>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 64 ++++++++++++++++++++----
>  1 file changed, 55 insertions(+), 9 deletions(-)
> 
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> index 7f74c54..3071b3b 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
> @@ -317,6 +317,24 @@ InitializeEmmcDevice (
>    return Status;
>  }
>  
> +
> +STATIC
> +UINT32
> +CreateSwitchCmdArgument (

This helper function is a good addition, thanks.

> +  IN  UINT8  Mode,
> +  IN  UINT8  Group,
> +  IN  UINT8  Value
> +  )
> +{
> +  UINT32 Argument;
> +
> +  Argument = Mode << 31 | 0x00FFFFFF;

Just because I hate implicit type promotion, could you make Mode
UINT32 in the input, please?

> +  Argument &= ~(0xF << (Group * 4));
> +  Argument |= Value << (Group * 4);
> +
> +  return Argument;
> +}
> +
>  STATIC
>  EFI_STATUS
>  InitializeSdMmcDevice (
> @@ -326,6 +344,7 @@ InitializeSdMmcDevice (
>    UINT32        CmdArg;
>    UINT32        Response[4];
>    UINT32        Buffer[128];
> +  UINT32        Speed;
>    UINTN         BlockSize;
>    UINTN         CardSize;
>    UINTN         NumBlocks;
> @@ -334,6 +353,7 @@ InitializeSdMmcDevice (
>    EFI_STATUS    Status;
>    EFI_MMC_HOST_PROTOCOL     *MmcHost;
>  
> +  Speed = 25000000;

Could this be given a #define with a descriptive name, in Mmc.h?

>    MmcHost = MmcHostInstance->MmcHost;
>  
>    // Send a command to get Card specific data
> @@ -439,43 +459,69 @@ InitializeSdMmcDevice (
>      }
>    }
>    if (CccSwitch) {
> +    /* SD Switch, Mode:0, Group:0, Value:0 */
> +    CmdArg = CreateSwitchCmdArgument(0, 0, 0);
> +    Status = MmcHost->SendCommand (MmcHost, MMC_CMD6, CmdArg);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Failed with Status = %r\n", __func__, Status));
> +       return Status;
> +    } else {
> +      Status = MmcHost->ReadBlockData (MmcHost, 0, 64, Buffer);

What are 0 and 64?
I guess 64 is a size?
Is there a #define or a sizeof() that could make it more descriptive?

> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): ReadBlockData Failed with Status = %r\n", __func__, Status));
> +        return Status;
> +      }
> +    }
> +
> +    if (!(Buffer[3] & 0x20000)) {

Is there no struct available to access this information in more human
readable form?

And a #define for the 0x20000, please.

> +      DEBUG ((EFI_D_ERROR, "%aHigh Speed not supported by Card %r\n", __func__, Status));
> +      return Status;
> +    }
> +
> +    Speed = 50000000;       //High Speed for SD card is 50 MHZ

Could this be given a #define with a descriptive name, in Mmc.h?

> +
>      /* SD Switch, Mode:1, Group:0, Value:1 */
> -    CmdArg = 1 << 31 | 0x00FFFFFF;
> -    CmdArg &= ~(0xF << (0 * 4));
> -    CmdArg |= 1 << (0 * 4);
> +    CmdArg = CreateSwitchCmdArgument(1, 0, 1);
>      Status = MmcHost->SendCommand (MmcHost, MMC_CMD6, CmdArg);
>      if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", Status));
> +      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", __func__, Status));

This looks like an unrelated bugfix? It is good, and thank you, but
could you break it out into its own patch please?
Also, __FUNCTION__ matches the coding style better (I know we have
both, but __func__ appears to be losing, and I would like to keep that
momentum up.

>         return Status;
>      } else {
>        Status = MmcHost->ReadBlockData (MmcHost, 0, 64, Buffer);
>        if (EFI_ERROR (Status)) {
> -        DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): ReadBlockData Error and Status = %r\n", Status));
> +        DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): ReadBlockData Error and Status = %r\n",__func__, Status));

Unrelated bugfix (same as comment above, and same patch please).

> +        return Status;
> +      }
> +
> +      if ((Buffer[4] & 0x0f000000) != 0x01000000) {

Is there no struct available to access this information in more human
readable form?

And a #define for both the magic values, please.

> +        DEBUG((EFI_D_ERROR, "Problem switching SD card into high-speed mode\n"));
>          return Status;
>        }
>      }
>    }
> +
>    if (Scr.SD_BUS_WIDTHS & SD_BUS_WIDTH_4BIT) {
>      CmdArg = MmcHostInstance->CardInfo.RCA << 16;
>      Status = MmcHost->SendCommand (MmcHost, MMC_CMD55, CmdArg);
>      if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD55): Error and Status = %r\n", Status));
> +      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD55): Error and Status = %r\n", __func__, Status));

Unrelated bugfix (same as comment above, and same patch please).

>        return Status;
>      }
>      /* Width: 4 */
>      Status = MmcHost->SendCommand (MmcHost, MMC_CMD6, 2);
>      if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", Status));
> +      DEBUG ((EFI_D_ERROR, "%a(MMC_CMD6): Error and Status = %r\n", __func__, Status));

Unrelated bugfix (same as comment above, and same patch please).

>        return Status;
>      }
>    }
>    if (MMC_HOST_HAS_SETIOS(MmcHost)) {
> -    Status = MmcHost->SetIos (MmcHost, 26 * 1000 * 1000, 4, EMMCBACKWARD);
> +    Status = MmcHost->SetIos (MmcHost, Speed, 4, EMMCBACKWARD);
>      if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "%a(SetIos): Error and Status = %r\n", Status));
> +      DEBUG ((EFI_D_ERROR, "%a(SetIos): Error and Status = %r\n", __func__, Status));

Unrelated bugfix (same as comment above, and same patch please).

/
    Leif

>        return Status;
>      }
>    }
> +
>    return EFI_SUCCESS;
>  }
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel