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

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

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 17, 2021 10:11 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Ni, Ray <ray.ni@intel.com>
> Cc: devel@edk2.groups.io; Bandaru, Purna Chandra Rao
> <purna.chandra.rao.bandaru@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: Improve
> Error handling of Ufs Pass Thru driver
> 
> Jian, Hao, Ray -- any feedback on this?


Hello Laszlo and Bandaru,

I will try to provide feedbacks for this patch once I am back to the office (early next week).
Sorry for the delayed response.

Best Regards,
Hao Wu


> 
> Thanks
> Laszlo
> 
> On 02/11/21 13:59, Purna Chandra Rao Bandaru wrote:
> > 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
> >
> > Signed-off-by: Bandaru <purna.chandra.rao.bandaru@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.
> >    do {
> > +    MicroSecondDelay (100); //Give 100 us and then start polling.
> >      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;
> > +  }
> >  }
> >
> >  /**
> > @@ -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 (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++) {
> >      LinkStartupCommand.Opcode = UfsUicDmeLinkStartup;
> >      LinkStartupCommand.Arg1 = 0;
> >      LinkStartupCommand.Arg2 = 0;
> >      LinkStartupCommand.Arg3 = 0;
> >      Status = UfsExecUicCommands (Private, &LinkStartupCommand);
> > -    if (EFI_ERROR (Status)) {
> > -      return EFI_DEVICE_ERROR;
> > -    }
> > +    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) {
> > +      Status = UfsEnableHostController (Private);
> > +      if (EFI_ERROR (Status)) {
> > +        DEBUG ((DEBUG_ERROR, "UfsDeviceDetection: Enable Host Controller
> Fails, Status = %r\n", Status));
> > +        return Status;
> > +      }
> >      }
> >    }
> >
> >