Re: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error handling of Ufs Pass Thru driver

["Wu, Hao A" <hao.a.wu@intel.com>]

Thanks Liming,

I will sync with Purna for other possible solutions.

Best Regards,
Hao Wu

> -----Original Message-----
> From: gaoliming <gaoliming@byosoft.com.cn>
> Sent: Thursday, February 25, 2021 9:46 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; Bandaru, Purna Chandra Rao
> <purna.chandra.rao.bandaru@intel.com>; devel@edk2.groups.io
> Cc: 'Laszlo Ersek' <lersek@redhat.com>; 'Leif Lindholm (Nuvia address)'
> <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> 'Andrew Fish' <afish@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.wu@intel.com>
> > 发送时间: 2021年2月24日 9:21
> > 收件人: gaoliming <gaoliming@byosoft.com.cn>; Bandaru, Purna Chandra
> Rao
> > <purna.chandra.rao.bandaru@intel.com>; devel@edk2.groups.io
> > 抄送: Laszlo Ersek <lersek@redhat.com>; Leif Lindholm (Nuvia address)
> > <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> > Andrew Fish <afish@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.bandaru@intel.com>
> > > Sent: Tuesday, February 23, 2021 10:36 PM
> > > To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> > > Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> > > <ray.ni@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.wu@intel.com>
> > > Sent: Tuesday, February 23, 2021 6:46 AM
> > > To: Bandaru, Purna Chandra Rao
> > > <purna.chandra.rao.bandaru@intel.com>;
> > > devel@edk2.groups.io
> > > Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> > > <ray.ni@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.bandaru@intel.com>
> > > > Sent: Tuesday, February 23, 2021 1:11 AM
> > > > To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> > > > Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> > > > <ray.ni@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.wu@intel.com>
> > > > Sent: Monday, February 22, 2021 2:10 PM
> > > > To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Bandaru,
> > > > Purna Chandra Rao <purna.chandra.rao.bandaru@intel.com>
> > > > Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> > > > <ray.ni@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.bandaru@intel.com>;
> > > > > devel@edk2.groups.io
> > > > > Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> > > > > <ray.ni@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.bandaru@intel.com>
> > > > > > Sent: Wednesday, February 17, 2021 5:02 PM
> > > > > > To: devel@edk2.groups.io
> > > > > > Cc: Bandaru, Purna Chandra Rao
> > > > > > <purna.chandra.rao.bandaru@intel.com>;
> > > > > > Albecki, Mateusz <mateusz.albecki@intel.com>; Ni, Ray
> > > > > > <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > > > > > Subject: [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error
> > > > handling
> > > > > > of Ufs Pass Thru driver
> > > > > >
> > > > > > From: Bandaru <purna.chandra.rao.bandaru@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.bandaru@intel.com>
> > > > > > Cc: Mateusz Albecki <mateusz.albecki@intel.com>
> > > > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > > > Cc: Hao A Wu <hao.a.wu@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
> > > > >
> > > > >
> > > > >
> > > > > 
> > > > >
> 
>