Thanks Liming, I will sync with Purna for other possible solutions.
Best Regards, Hao Wu > -----Original Message----- > From: gaoliming <gaolim...@byosoft.com.cn> > Sent: Thursday, February 25, 2021 9:46 AM > To: Wu, Hao A <hao.a...@intel.com>; Bandaru, Purna Chandra Rao > <purna.chandra.rao.band...@intel.com>; devel@edk2.groups.io > Cc: 'Laszlo Ersek' <ler...@redhat.com>; 'Leif Lindholm (Nuvia address)' > <l...@nuviainc.com>; Kinney, Michael D <michael.d.kin...@intel.com>; > 'Andrew Fish' <af...@apple.com> > Subject: 回复: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: > Improve Error handling of Ufs Pass Thru driver > > Hao: > I see v2 patch set was sent after SFF (Soft Feature Freeze). According to > SFF, > no feature will be added in this period. > > The request is to merge [PATCH 3/3] MdeModulePkg/UfsPassThruDxe: > Improve UFS device Readiness check for this stable tag. > This change is to add retry times for the device readiness check. This is an > improvement, not bug fix. If this patch needs to catch this stable tag, we > have to defer stable tag. > > Thanks > Liming > > -----邮件原件----- > > 发件人: Wu, Hao A <hao.a...@intel.com> > > 发送时间: 2021年2月24日 9:21 > > 收件人: gaoliming <gaolim...@byosoft.com.cn>; Bandaru, Purna Chandra > Rao > > <purna.chandra.rao.band...@intel.com>; devel@edk2.groups.io > > 抄送: Laszlo Ersek <ler...@redhat.com>; Leif Lindholm (Nuvia address) > > <l...@nuviainc.com>; Kinney, Michael D <michael.d.kin...@intel.com>; > > Andrew Fish <af...@apple.com> > > 主题: RE: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: Improve > > Error handling of Ufs Pass Thru driver > > > > Hello Liming, > > > > I have a patch that would like to confirm with you that whether it can > > be merged into the upcoming edk2-stable202102 tag. > > > > This is a feature request: > > https://bugzilla.tianocore.org/show_bug.cgi?id=3217 > > In the BZ tracker, there are 3 improvements mentioned for > UfsPassThruDxe. > > According to Purna, he would like to have 1 of the improvements > > (improvement #3 in BZ-3217) be merged and catch the stable tag. > > I have given the 'R-b' tag for improvement #3 already > > (https://edk2.groups.io/g/devel/message/72121) > > > > My thought is that we can break BZ-3217 into multiple feature requests: > > 1. BZ-3217: Updated its title and description to only cover > > improvement #3 2. File new BZ feature requests to cover improvement #1 > > & #2 > > > > What is your suggestion for this case? Thanks in advance. > > > > Best Regards, > > Hao Wu > > > > > -----Original Message----- > > > From: Bandaru, Purna Chandra Rao > > <purna.chandra.rao.band...@intel.com> > > > Sent: Tuesday, February 23, 2021 10:36 PM > > > To: Wu, Hao A <hao.a...@intel.com>; devel@edk2.groups.io > > > Cc: Albecki, Mateusz <mateusz.albe...@intel.com>; Ni, Ray > > > <ray...@intel.com> > > > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: > > > Improve Error handling of Ufs Pass Thru driver > > > > > > Hi Wu, Hao A > > > > > > I am trying to focus on merging patch#1 for now to unblock boot issues. > > > March 6th might be too late, May I request you to expedite any other > > > alternatives like exceptions/overrides? > > > For the remaining two patches I will get back to you with the plan > > > after discussing with WSIV and MVE teams on the protocol analyzer tools > etc. > > > > > > Thanks > > > ~Purna > > > > > > -----Original Message----- > > > From: Wu, Hao A <hao.a...@intel.com> > > > Sent: Tuesday, February 23, 2021 6:46 AM > > > To: Bandaru, Purna Chandra Rao > > > <purna.chandra.rao.band...@intel.com>; > > > devel@edk2.groups.io > > > Cc: Albecki, Mateusz <mateusz.albe...@intel.com>; Ni, Ray > > > <ray...@intel.com> > > > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: > > > Improve Error handling of Ufs Pass Thru driver > > > > > > > -----Original Message----- > > > > From: Bandaru, Purna Chandra Rao > > > <purna.chandra.rao.band...@intel.com> > > > > Sent: Tuesday, February 23, 2021 1:11 AM > > > > To: Wu, Hao A <hao.a...@intel.com>; devel@edk2.groups.io > > > > Cc: Albecki, Mateusz <mateusz.albe...@intel.com>; Ni, Ray > > > > <ray...@intel.com> > > > > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: > > > > Improve Error handling of Ufs Pass Thru driver > > > > > > > > Thank you Hai Bu for the response. > > > > > > > > I have broken this into three separate patches. There were no > > > > specific recommendation in the speciation for seen multiple issues > > > > on all the UFS platforms like LKF, ADP-P and EHK. > > > > > > > > > Hello, > > > > > > After quickly going through the new series sent, I do not see my > previous > > > inline comments and questions get addressed. > > > Could you please help to provide your feedbacks and update the patches? > > > > > > > > > > And these changes worked on all the three with various UFS cards. > > > > Can you please review and help to get this changes at the earliest > > > > in master as well as Downstream/master. > > > > > > > > > Sorry, since there is an upcoming stable tag approaching, at this > moment, I > > > prefer to hold this feature after the stable tag (March 6th). > > > > > > Best Regards, > > > Hao Wu > > > > > > > > > > > > > > Thanks > > > > ~Purna > > > > > > > > -----Original Message----- > > > > From: Wu, Hao A <hao.a...@intel.com> > > > > Sent: Monday, February 22, 2021 2:10 PM > > > > To: devel@edk2.groups.io; Wu, Hao A <hao.a...@intel.com>; Bandaru, > > > > Purna Chandra Rao <purna.chandra.rao.band...@intel.com> > > > > Cc: Albecki, Mateusz <mateusz.albe...@intel.com>; Ni, Ray > > > > <ray...@intel.com> > > > > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: > > > > Improve Error handling of Ufs Pass Thru driver > > > > > > > > > -----Original Message----- > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > > > > > Wu, > > > > Hao > > > > > A > > > > > Sent: Monday, February 22, 2021 4:38 PM > > > > > To: Bandaru, Purna Chandra Rao > > > > > <purna.chandra.rao.band...@intel.com>; > > > > > devel@edk2.groups.io > > > > > Cc: Albecki, Mateusz <mateusz.albe...@intel.com>; Ni, Ray > > > > > <ray...@intel.com> > > > > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: > > > > > Improve Error handling of Ufs Pass Thru driver > > > > > > > > > > > -----Original Message----- > > > > > > From: Bandaru, Purna Chandra Rao > > > > > <purna.chandra.rao.band...@intel.com> > > > > > > Sent: Wednesday, February 17, 2021 5:02 PM > > > > > > To: devel@edk2.groups.io > > > > > > Cc: Bandaru, Purna Chandra Rao > > > > > > <purna.chandra.rao.band...@intel.com>; > > > > > > Albecki, Mateusz <mateusz.albe...@intel.com>; Ni, Ray > > > > > > <ray...@intel.com>; Wu, Hao A <hao.a...@intel.com> > > > > > > Subject: [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error > > > > handling > > > > > > of Ufs Pass Thru driver > > > > > > > > > > > > From: Bandaru <purna.chandra.rao.band...@intel.com> > > > > > > > > > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=3217 > > > > > > > > > > > > Following is the brief description of the changes > > > > > > 1) There are cards that can take upto 600ms for Init and > > > > > > hence > > increase > > > > > > the time out for fDeviceInit polling loop. > > > > > > 2) Add UFS host conctroller reset in the last retry of Link > > > > > > start > up. > > > > > > 3) Retry sending NOP OUT command upto 10 times > > > > > > > > > > > > > > > Hello Bandaru, > > > > > > > > > > Could you help to break this patch into a 3-patch series in V2? > > > > > With each patch handling just one of the above 3 improvements > > > > mentioned. > > > > > > > > > > For improvement 2) above, I do not see such UFS host controller > > > > > re-enabling process being mentioned in UFSHCI 3.0 spec section > 7.1.1. > > > > > Is this process being documented somewhere else in the spec or > > > > > suggested by device vender? > > > > > > > > > > > > Sorry for missing one comment. > > > > Could you help to add the information on what kind of tests have > > > > been performed for the code changes? > > > > > > > > Thanks in advance. > > > > > > > > Best Regards, > > > > Hao Wu > > > > > > > > > > > > > > > > > > More inline comments below: > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Bandaru <purna.chandra.rao.band...@intel.com> > > > > > > Cc: Mateusz Albecki <mateusz.albe...@intel.com> > > > > > > Cc: Ray Ni <ray...@intel.com> > > > > > > Cc: Hao A Wu <hao.a...@intel.com> > > > > > > > > > > > > Change-Id: I6c0dbc1c147487e51f0ed5f2425957ae089b0160 > > > > > > --- > > > > > > MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c | 26 > > > > > > +++++++++++++++++++++----- > > > > > > MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c | 18 > > > > > > ++++++++++++------ > > > > > > 2 files changed, 33 insertions(+), 11 deletions(-) > > > > > > > > > > > > diff --git > a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c > > > > > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c > > > > > > index 9768c2e6fb..89048745be 100644 > > > > > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c > > > > > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c > > > > > > @@ -1,6 +1,6 @@ > > > > > > /** @file > > > > > > > > > > > > - Copyright (c) 2014 - 2019, Intel Corporation. All rights > > > > > > reserved.<BR> > > > > > > + Copyright (c) 2014 - 2021, Intel Corporation. All rights > > > > > > + reserved.<BR> > > > > > > Copyright (c) Microsoft Corporation.<BR> > > > > > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > > > > > > > > @@ -749,7 +749,7 @@ UfsFinishDeviceInitialization ( { > > > > > > EFI_STATUS Status; > > > > > > UINT8 DeviceInitStatus; > > > > > > - UINT8 Timeout; > > > > > > + UINT16 Timeout; > > > > > > > > > > > > DeviceInitStatus = 0xFF; > > > > > > > > > > > > @@ -761,17 +761,23 @@ UfsFinishDeviceInitialization ( > > > > > > return Status; > > > > > > } > > > > > > > > > > > > - Timeout = 5; > > > > > > + Timeout = 6000; //There are cards that can take upto 600ms. > > > > > > > > > > > > > > > Please help to add a macro in file UfsPassThru.h: > > > > > #define UFS_INIT_COMPLETION_TIMEOUT 6000 And use the macro > > here. > > > > > > > > > > Also a minor comment, could you help to use the below comment > > format? > > > > > // > > > > > // There are UFS devices that can take up to 600ms to clear the > > > > > fDeviceInit flag // Timeout = UFS_INIT_COMPLETION_TIMEOUT; > > > > > > > > > > > > > > > > do { > > > > > > + MicroSecondDelay (100); //Give 100 us and then start polling. > > > > > > > > > > > > > > > For the above delay movement, do you observe any side effect for > > > > > the origin code? > > > > > If not, I prefer to leave the origin behavior: > > > > > do { > > > > > UfsReadFlag(); > > > > > ... > > > > > MicroSecondDelay (1); > > > > > } while (...) > > > > > since doing so will have the least performance penalty for > > > > > devices that respond fast. > > > > > > > > > > > > > > > > Status = UfsReadFlag (Private, UfsFlagDevInit, > > &DeviceInitStatus); > > > > > > if (EFI_ERROR (Status)) { > > > > > > return Status; > > > > > > } > > > > > > - MicroSecondDelay (1); > > > > > > Timeout--; > > > > > > } while (DeviceInitStatus != 0 && Timeout != 0); > > > > > > > > > > > > + if (Timeout == 0) { > > > > > > + DEBUG ((DEBUG_ERROR, "UfsFinishDeviceInitialization > > > > > > DeviceInitStatus=%x EFI_TIMEOUT \n", DeviceInitStatus)); > > > > > > + return EFI_TIMEOUT; > > > > > > + } else { > > > > > > + DEBUG ((DEBUG_INFO, "UfsFinishDeviceInitialization > > > > > > + Timeout left=%x EFI_SUCCESS \n", Timeout)); > > > > > > return EFI_SUCCESS; > > > > > > > > > > > > > > > Please help to add two spaces for text alignment in the above line. > > > > > > > > > > > > > > > > + } > > > > > > } > > > > > > > > > > > > /** > > > > > > @@ -905,9 +911,19 @@ UfsPassThruDriverBindingStart ( > > > > > > // At the end of the UFS Interconnect Layer initialization > > > > > > on both host and device side, > > > > > > // the host shall send a NOP OUT UPIU to verify that the > > > > > > device UTP Layer is ready. > > > > > > // > > > > > > > > > > > > > > > For the NOP OUT - NOP IN improvement, could you help to provide > > more > > > > > information on what is the current issue for some devices? > > > > > Is it a timeout happened for: > > > > > Status = UfsWaitMemSet (Private, UFS_HC_UTRLDBR_OFFSET, BIT0 > > << > > > > > Slot, 0, UFS_TIMEOUT); (If so, have you tried increasing the > > > > > last parameter like > > > > > '10*UFS_TIMEOUT'?) Or the case is that NopInUpiu->Resp has a > > > > > non-zero value? > > > > > > > > > > I found that in the UFS 3.0 spec: > > > > > |> For some implementations, the device UTP layer may not be > > > > > |> initialized yet, therefore the device may not respond > > > > > |> promptly to NOP OUT UPIU sending NOP IN UPIU. > > > > > |> The host waits until it receives the NOP IN UPIU from the > device... > > > > > And there is no mention for the retry scheme. > > > > > > > > > > > > > > > > + for (Index = 10; Index > 0; Index--) { > > > > > > Status = UfsExecNopCmds (Private); > > > > > > if (EFI_ERROR (Status)) { > > > > > > - DEBUG ((DEBUG_ERROR, "Ufs Sending NOP IN command Error, > > > Status > > > > > > = %r\n", Status)); > > > > > > + DEBUG ((DEBUG_ERROR, "Ufs Sending NOP IN command > > Error, > > > > Index > > > > > > = %x Status = %r\n", Index, Status)); > > > > > > + MicroSecondDelay (100); //100 us > > > > > > + continue; > > > > > > + } else { > > > > > > + DEBUG ((DEBUG_INFO, "Ufs Sent NOP OUT successfully and > > > > > > + received > > > > > > NOP IN, Status = %r\n", Status)); > > > > > > + break; > > > > > > + } > > > > > > + } > > > > > > + if (!Index) { > > > > > > + DEBUG ((DEBUG_INFO, "NOP OUT failed all the 10 times > > > > > > + Status > > = > > > > > > + %r\n", Status)); > > > > > > goto Error; > > > > > > } > > > > > > > > > > > > diff --git > > a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > > > > > > b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > > > > > > index 0b1030ab47..4fa5689196 100644 > > > > > > --- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > > > > > > +++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThruHci.c > > > > > > @@ -2,7 +2,7 @@ > > > > > > UfsPassThruDxe driver is used to produce > > EFI_EXT_SCSI_PASS_THRU > > > > > > protocol interface > > > > > > for upper layer application to execute UFS-supported SCSI cmds. > > > > > > > > > > > > - Copyright (c) 2014 - 2019, Intel Corporation. All rights > > > > > > reserved.<BR> > > > > > > + Copyright (c) 2014 - 2021, Intel Corporation. All rights > > > > > > + reserved.<BR> > > > > > > Copyright (c) Microsoft Corporation.<BR> > > > > > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > > > > > > > > @@ -1929,17 +1929,15 @@ UfsDeviceDetection ( > > > > > > > > > > > > // > > > > > > // Start UFS device detection. > > > > > > - // Try up to 3 times for establishing data link with device. > > > > > > + // Try up to 4 times for establishing data link with device. > > > > > > // > > > > > > - for (Retry = 0; Retry < 3; Retry++) { > > > > > > + for (Retry = 0; Retry < 4; Retry++) { > > > > > > > > > > > > > > > Please introduce a macro in file UfsPassThru.h: > > > > > #define UFS_LINK_STARTUP_RETRIES 4 And use the macro here. > > > > > > > > > > Also, is it necessary to increase the retry number by 1? > > > > > Or the device can be successfully brought up by adding a host > > > > > controller re- enabling? > > > > > > > > > > > > > > > > LinkStartupCommand.Opcode = UfsUicDmeLinkStartup; > > > > > > LinkStartupCommand.Arg1 = 0; > > > > > > LinkStartupCommand.Arg2 = 0; > > > > > > LinkStartupCommand.Arg3 = 0; > > > > > > Status = UfsExecUicCommands (Private, > > &LinkStartupCommand); > > > > > > - if (EFI_ERROR (Status)) { > > > > > > - return EFI_DEVICE_ERROR; > > > > > > - } > > > > > > > > > > > > > > > Will the DME_LINKSTARTUP command execution fail at first and > > > > > then succeed after retry? > > > > > If not, I prefer to keep the origin code logic to return error > status directly. > > > > > > > > > > > > > > > > + if (!EFI_ERROR (Status)) { > > > > > > > > > > > > Status = UfsMmioRead32 (Private, UFS_HC_STATUS_OFFSET, > > &Data); > > > > > > if (EFI_ERROR (Status)) { @@ -1960,6 +1958,14 @@ > > > > > > UfsDeviceDetection ( > > > > > > } > > > > > > } > > > > > > return EFI_SUCCESS; > > > > > > + } > > > > > > + } > > > > > > + if (Retry == 2) { > > > > > > > > > > > > > > > Please help to update to: > > > > > if (Retry == UFS_LINK_STARTUP_RETRIES - 1) { > > > > > > > > > > And add comments like: > > > > > // > > > > > // Try re-enabling the UFS host controller in the last retry > > > > > attempt // > > > > > > > > > > > > > > > Best Regards, > > > > > Hao Wu > > > > > > > > > > > > > > > > + Status = UfsEnableHostController (Private); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + DEBUG ((DEBUG_ERROR, "UfsDeviceDetection: Enable > > Host > > > > > Controller > > > > > > Fails, Status = %r\n", Status)); > > > > > > + return Status; > > > > > > + } > > > > > > } > > > > > > } > > > > > > > > > > > > -- > > > > > > 2.16.2.windows.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#72164): https://edk2.groups.io/g/devel/message/72164 Mute This Topic: https://groups.io/mt/80893320/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
