* [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error handling of Ufs Pass Thru driver
@ 2021-02-11 12:59 Purna Chandra Rao Bandaru
2021-02-17 14:10 ` [edk2-devel] " Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Purna Chandra Rao Bandaru @ 2021-02-11 12:59 UTC (permalink / raw)
To: devel; +Cc: Bandaru
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;
+ }
}
}
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error handling of Ufs Pass Thru driver
2021-02-11 12:59 [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error handling of Ufs Pass Thru driver Purna Chandra Rao Bandaru
@ 2021-02-17 14:10 ` Laszlo Ersek
2021-02-18 1:57 ` Wu, Hao A
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2021-02-17 14:10 UTC (permalink / raw)
To: Jian J Wang, Hao A Wu, Ray Ni; +Cc: devel, purna.chandra.rao.bandaru
Jian, Hao, Ray -- any feedback on this?
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;
> + }
> }
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error handling of Ufs Pass Thru driver
2021-02-17 14:10 ` [edk2-devel] " Laszlo Ersek
@ 2021-02-18 1:57 ` Wu, Hao A
0 siblings, 0 replies; 4+ messages in thread
From: Wu, Hao A @ 2021-02-18 1:57 UTC (permalink / raw)
To: Laszlo Ersek, Bandaru, Purna Chandra Rao
Cc: devel@edk2.groups.io, Wang, Jian J, Ni, Ray
> -----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;
> > + }
> > }
> > }
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error handling of Ufs Pass Thru driver
[not found] <20210217090143.20032-1-purna.chandra.rao.bandaru@intel.com>
@ 2021-02-22 8:38 ` Wu, Hao A
0 siblings, 0 replies; 4+ messages in thread
From: Wu, Hao A @ 2021-02-22 8:38 UTC (permalink / raw)
To: Bandaru, Purna Chandra Rao, devel@edk2.groups.io
Cc: Albecki, Mateusz, Ni, Ray
> -----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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-22 8:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-11 12:59 [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error handling of Ufs Pass Thru driver Purna Chandra Rao Bandaru
2021-02-17 14:10 ` [edk2-devel] " Laszlo Ersek
2021-02-18 1:57 ` Wu, Hao A
[not found] <20210217090143.20032-1-purna.chandra.rao.bandaru@intel.com>
2021-02-22 8:38 ` Wu, Hao A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox