[edk2-devel] [edk2-platform][PATCH 1/1] MdeModulePkg: SdMmcPciHcDxe: Fix issue that SD1.0 cards can't be recognized

[Wu, Hao A]

Hello,

The proposed patch will cause CI test failure: https://github.com/tianocore/edk2/pull/3724. Could you help to resolve them?
May I know what kind of tests have been performed for this patch? Have you tried with non SD1.0 cards and make sure that they still work fine after the patch?

Also, serveral more inline comments below:


> -----Original Message-----
> From: Chevron Li <chevron.li at bayhubtech.com>
> Sent: Tuesday, December 6, 2022 2:48 PM
> To: devel at edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>; Wang,
> Jian J <jian.j.wang at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>;
> shirley.her at bayhubtech.com; shaper.liu at bayhubtech.com;
> xiaoguang.yu at bayhubtech.com; Chevron Li (WH)
> <chevron.li at bayhubtech.com>
> Subject: [edk2-platform][PATCH 1/1] MdeModulePkg: SdMmcPciHcDxe: Fix
> issue that SD1.0 cards can't be recognized
> 
> From: "Chevron Li (WH)" <chevron.li at bayhubtech.com>
> 
> 
> SD1.0 cards don't support CMD8 and CMD6
> 
> CMD8 result can be used to distinguish the card is SD1.0 or not.
> 
> CMD8 result can be used to decide following CMD6 is sent or skip.
> 
> 
> 
> Cc: Hao A Wu <hao.a.wu at intel.com>
> 
> Cc: Ray Ni <ray.ni at intel.com>
> 
> Cc: Jian J Wang <jian.j.wang at intel.com>
> 
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> 
> Signed-off-by: Chevron Li <chevron.li at bayhubtech.com>
> 
> ---
> 
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c | 30 ++++++++++++---
> ----
> 
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> 
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> 
> index f5a3607e47..281aa38170 100644
> 
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> 
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> 
> @@ -1085,7 +1085,8 @@ SdCardSetBusMode (
> 
>    IN EFI_SD_MMC_PASS_THRU_PROTOCOL  *PassThru,
> 
>    IN UINT8                          Slot,
> 
>    IN UINT16                         Rca,
> 
> -  IN BOOLEAN                        S18A
> 
> +  IN BOOLEAN                        S18A,
> 
> +  IN BOOLEAN                        SdVersion1


For the newly added input parameter 'SdVersion1', please help to update the function description comments to reflect it.


> 
>    )
> 
>  {
> 
>    EFI_STATUS              Status;
> 
> @@ -1117,10 +1118,13 @@ SdCardSetBusMode (
> 
> 
> 
>    //
> 
>    // Get the supported bus speed from SWITCH cmd return data group #1.
> 
> +  // SdVersion1 don't support the SWITCH cmd
> 
>    //
> 
> -  Status = SdCardSwitch (PassThru, Slot, 0xFF, 0xF, SdDriverStrengthIgnore,
> 0xF, FALSE, SwitchResp);
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    return Status;
> 
> +  if(SdVersion1 == FALSE) {
> 
> +    Status = SdCardSwitch (PassThru, Slot, 0xFF, 0xF, SdDriverStrengthIgnore,
> 0xF, FALSE, SwitchResp);


The local variable 'SwitchResp' is an output parameter for SdCardSwitch().
When SdVersion1 is TRUE, SdCardSwitch() call will be skipped and 'SwitchResp' will have uninitialized value.
This will probably cause issues when 'SwitchResp' later being used as an input parameter for SdGetTargetBusMode() function call.
Could you help to check on this?


> 
> +    if (EFI_ERROR (Status)) {
> 
> +      return Status;
> 
> +    }
> 
>    }
> 
> 
> 
>    SdGetTargetBusMode (Private, Slot, SwitchResp, S18A, &BusMode);
> 
> @@ -1141,9 +1145,14 @@ SdCardSetBusMode (
> 
>      }
> 
>    }
> 
> 
> 
> -  Status = SdCardSwitch (PassThru, Slot, BusMode.BusTiming, 0xF,
> BusMode.DriverStrength.Sd, 0xF, TRUE, SwitchResp);
> 
> -  if (EFI_ERROR (Status)) {
> 
> -    return Status;
> 
> +  //
> 
> +  // SdVersion1 don't support the SWITCH cmd
> 
> +  //
> 
> +  if(SdVersion1 == FALSE){
> 
> +    Status = SdCardSwitch (PassThru, Slot, BusMode.BusTiming, 0xF,
> BusMode.DriverStrength.Sd, 0xF, TRUE, SwitchResp);
> 
> +    if (EFI_ERROR (Status)) {
> 
> +      return Status;
> 
> +    }
> 
>    }
> 
> 
> 
>    Status = SdMmcSetDriverStrength (Private->PciIo, Slot,
> BusMode.DriverStrength.Sd);
> 
> @@ -1214,6 +1223,7 @@ SdCardIdentification (
> 
>    UINT8                          HostCtrl2;
> 
>    UINTN                          Retry;
> 
>    BOOLEAN                        ForceVoltage33;
> 
> +  BOOLEAN                        SdVersion1;
> 
> 
> 
>    ForceVoltage33 = FALSE;


Please help to assign a initial value for the newly added local variable 'SdVersion1'.
If the card supports cmd8, this local variable will have uninitialized value when calling SdCardSetBusMode().


> 
> 
> 
> @@ -1231,12 +1241,12 @@ Voltage33Retry:
> 
>    }
> 
> 
> 
>    //
> 
> -  // 2. Send Cmd8 to the device
> 
> +  // 2. Send Cmd8 to the device, the command will fail for SdVersion1;
> 
>    //
> 
>    Status = SdCardVoltageCheck (PassThru, Slot, 0x1, 0xFF);
> 
>    if (EFI_ERROR (Status)) {
> 
> +    SdVersion1 = 1;


Please use "SdVersion1 = TRUE;".

Best Regards,
Hao Wu


> 
>      DEBUG ((DEBUG_INFO, "SdCardIdentification: Executing Cmd8 fails
> with %r\n", Status));
> 
> -    return Status;
> 
>    }
> 
> 
> 
>    //
> 
> @@ -1426,7 +1436,7 @@ Voltage33Retry:
> 
>    DEBUG ((DEBUG_INFO, "SdCardIdentification: Found a SD device at slot
> [%d]\n", Slot));
> 
>    Private->Slot[Slot].CardType = SdCardType;
> 
> 
> 
> -  Status = SdCardSetBusMode (PciIo, PassThru, Slot, Rca, ((Ocr & BIT24) !=
> 0));
> 
> +  Status = SdCardSetBusMode (PciIo, PassThru, Slot, Rca, ((Ocr & BIT24) != 0),
> SdVersion1);
> 
> 
> 
>    return Status;
> 
> 
> 
> 
> 
> base-commit: 7bee2498910a9034faaf90802c49188afb7582dc
> 
> --
> 
> 2.18.0.windows.1
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97013): https://edk2.groups.io/g/devel/message/97013
Mute This Topic: https://groups.io/mt/95489530/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-