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

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

[Wu, Hao A]

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

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

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

[Wu, Hao A]

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

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

[Bandaru, Purna Chandra Rao]

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

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

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

[Wu, Hao A]

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

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

[Bandaru, Purna Chandra Rao]

[-- Attachment #1: Type: text/plain, Size: 12672 bytes --]

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


[-- Attachment #2: Type: message/rfc822, Size: 9081 bytes --]

From: "Bandaru, Purna Chandra Rao" <purna.chandra.rao.bandaru@intel.com>
To: "devel@edk2.groups.io" <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 Device initialization polling Loop
Date: Tue, 23 Feb 2021 14:32:15 +0000
Message-ID: <20210223143215.26752-1-purna.chandra.rao.bandaru@intel.com>

https://bugzilla.tianocore.org/show_bug.cgi?id=3217

Current Ufs Pass thru driver polls for 5us and return success even when
the timeout occurs.
There are cards that can take upto 600ms for Init and hence increased
the time out for fDeviceInit polling loop.

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: I6cb063b43bdf37790db8e60c3919153cd2f3c086
---
 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c | 17 +++++++++++++----
 MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h |  3 ++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.c
index 9768c2e6fb..92ff958f16 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;
+  UINT32 Timeout;

   DeviceInitStatus = 0xFF;

@@ -761,7 +761,10 @@ UfsFinishDeviceInitialization (
     return Status;
   }

-  Timeout = 5;
+  //
+  // There are cards that can take upto 600ms to clear fDeviceInit flag.
+  //
+  Timeout = UFS_INIT_COMPLETION_TIMEOUT;
   do {
     Status = UfsReadFlag (Private, UfsFlagDevInit, &DeviceInitStatus);
     if (EFI_ERROR (Status)) {
@@ -771,7 +774,13 @@ UfsFinishDeviceInitialization (
     Timeout--;
   } while (DeviceInitStatus != 0 && Timeout != 0);

-  return EFI_SUCCESS;
+  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;
+  }
 }

 /**
diff --git a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
index ef33250c89..275351cf8e 100644
--- a/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
+++ b/MdeModulePkg/Bus/Ufs/UfsPassThruDxe/UfsPassThru.h
@@ -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

@@ -40,6 +40,7 @@
 //
 #define UFS_MAX_LUNS                12
 #define UFS_WLUN_PREFIX             0xC1
+#define UFS_INIT_COMPLETION_TIMEOUT 600000

 typedef struct {
   UINT8    Lun[UFS_MAX_LUNS];
--
2.16.2.windows.1

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

[Wu, Hao A]

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

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

[gaoliming]

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

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

[Wu, Hao A]

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