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

["Chevron Li (WH)" <chevron.li@bayhubtech.com>]

Thanks for your response.

I will update patch later according to your suggestions.

Answer your questions below.

BR,
Chevron

> -----邮件原件-----
> 发件人: Wu, Hao A <hao.a.wu@intel.com>
> 发送时间: Tuesday, December 6, 2022 16:28
> 收件人: Chevron Li (WH) <chevron.li@bayhubtech.com>;
> devel@edk2.groups.io
> 抄送: Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>; Shirley Her(SC)
> <shirley.her@bayhubtech.com>; Shaper Liu (WH)
> <shaper.liu@bayhubtech.com>; XiaoGuang Yu (WH)
> <xiaoguang.yu@bayhubtech.com>
> 主题: RE: [edk2-platform][PATCH 1/1] MdeModulePkg: SdMmcPciHcDxe: Fix
> issue that SD1.0 cards can't be recognized
> 
> Hello,
> 
> The proposed patch will cause CI test failure:
> https://github.com/tianocore/edk2/pull/3724. Could you help to resolve
> them?

Yes, I will update according to your suggestions.

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

We have tested this patch with SD1.0 cards and SD2.0, SD3.0 cards, all of them can work right.
Test items are application level:
Such as card identify, create file, delete file.

> Also, serveral more inline comments below:
> 
> 
> > -----Original Message-----
> > From: Chevron Li <chevron.li@bayhubtech.com>
> > Sent: Tuesday, December 6, 2022 2:48 PM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang,
> > Jian J <jian.j.wang@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; shirley.her@bayhubtech.com;
> > shaper.liu@bayhubtech.com; xiaoguang.yu@bayhubtech.com; Chevron Li
> > (WH) <chevron.li@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@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@intel.com>
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> >
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >
> > Signed-off-by: Chevron Li <chevron.li@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
> >
> >