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

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

Hi Jun,


I checked my host controller driver and i am taking care of its endianness in host controller driver itself.


I researched further on the bit number I am checking for High speed in CMD6 data. 

I tried to find out in SD specs if response for CMD 6 comes it LE or BE format.

I didn't find out anything mentioned directly for CMD6 but yes for ACMD51 (SCR command), it follows BE.

We are reading only 8 bytes in SCR register to get SD version, while SD version comes in bit 59:56:


SD Memory Card - Spec. Version   SD_SPEC     [59:56]


I think similar stands true for CMD6 as well. Bit 512 is coming first on DATA line.

So I am checking correct bits in patch.

I have refer linux code also, there also they are considering bit 512 is coming first.

Please comment.


Thanks,
Meenakshi


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Meenakshi Aggarwal
> Sent: Friday, September 01, 2017 3:03 PM
> To: Jun Nie <jun.nie@linaro.org>; Leif Lindholm <leif.lindholm@linaro.org>
> Cc: edk2-devel@lists.01.org; Haojian Zhuang <haojian.zhuang@linaro.org>
> Subject: Re: [edk2] [PATCH 2/2] SD : Updated CMD 6 implememtation.
> 
> [This sender failed our fraud detection checks and may not be who they
> appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
> 
> 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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel