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

[Jun Nie <jun.nie@linaro.org>]

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?
> 
>> +  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);

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?
> 
>> +      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".

> 
> 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) {

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[].

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