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

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

Hi Leif and Jun,


Thanks for your review.


My comments are inlined.

Regards,
Meenakshi

> -----Original Message-----
> From: Jun Nie [mailto:jun.nie@linaro.org]
> Sent: Thursday, August 31, 2017 8:13 PM
> To: Leif Lindholm <leif.lindholm@linaro.org>; Meenakshi Aggarwal
> <meenakshi.aggarwal@nxp.com>
> Cc: edk2-devel@lists.01.org; Haojian Zhuang <haojian.zhuang@linaro.org>
> Subject: Re: [edk2] [PATCH 2/2] SD : Updated CMD 6 implememtation.
> 
> On 2017年08月31日 20:06, Leif Lindholm wrote:
> > 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.
> 
> Good catch, check should be done before setting function. And the setting
> result should be checked before return. Logic is correct in this patch.
> 
> >
> > 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?
> >
I will surely do this.

> >> +  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?
> >
ok
> >>     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);
> 
> A SD_MODE_CHECK/GET macro is clearer than 0 and 1 value for Mode.
> 
> >> +    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?
> >
Yes 64 is the number of bytes we want to read, and 0 is the block offset.
I will add a macro for size.

> >> +      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)) {
> 
> Bit 401 is HS support status. So bit in Buffer[12] should be tested.
> Or I miss anything? I am checking "SD Specifications Part 1 Physical Layer
> Specification Version 2.00".
> 
Ah... You are correct, my SD host controller is Big Endian and so is the difference, I will update the patch and soon send V2.
> >
> > Is there no struct available to access this information in more human
> > readable form?
> >
No ☹

> > And a #define for the 0x20000, please.
> >
For sure

> >> +      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?
> >
Ok

> >> +
> >>       /* 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.
> >
Ok... will send a separate patch

> >>          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) {
> 
> HS function busy status is bit 287:272 in response, bit 273 actually.
> Bit 379:376 is error status or function number if no error. So I guess you
> should test bit in other two elements of Buffer[].
> 
Again... You are correct, I will update the patch and send V2 soon.
> >
> > Is there no struct available to access this information in more human
> > readable form?
> >
No
> > And a #define for both the magic values, please.
> >
Ok

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