Thanks for your response. I will update patch later according to your suggestions.
Answer your questions below. BR, Chevron > -----邮件原件----- > 发件人: Wu, Hao A <hao.a...@intel.com> > 发送时间: Tuesday, December 6, 2022 16:28 > 收件人: Chevron Li (WH) <chevron...@bayhubtech.com>; > devel@edk2.groups.io > 抄送: Ni, Ray <ray...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Gao, > Liming <gaolim...@byosoft.com.cn>; Shirley Her(SC) > <shirley....@bayhubtech.com>; Shaper Liu (WH) > <shaper....@bayhubtech.com>; XiaoGuang Yu (WH) > <xiaoguang...@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...@bayhubtech.com> > > Sent: Tuesday, December 6, 2022 2:48 PM > > To: devel@edk2.groups.io > > Cc: Wu, Hao A <hao.a...@intel.com>; Ni, Ray <ray...@intel.com>; Wang, > > Jian J <jian.j.w...@intel.com>; Gao, Liming > > <gaolim...@byosoft.com.cn>; shirley....@bayhubtech.com; > > shaper....@bayhubtech.com; xiaoguang...@bayhubtech.com; Chevron Li > > (WH) <chevron...@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...@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...@intel.com> > > > > Cc: Ray Ni <ray...@intel.com> > > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > > > Signed-off-by: Chevron Li <chevron...@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 (#97027): https://edk2.groups.io/g/devel/message/97027 Mute This Topic: https://groups.io/mt/95490144/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
