From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Bandaru,
Purna Chandra Rao" <purna.chandra.rao.bandaru@intel.com>,
"devel@edk2.groups.io" <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
Date: Tue, 23 Feb 2021 01:16:03 +0000 [thread overview]
Message-ID: <BN8PR11MB3666A56144D85EB18F7EBC3DCA809@BN8PR11MB3666.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CO1PR11MB51712DB3404D394DD6799202F9819@CO1PR11MB5171.namprd11.prod.outlook.com>
> -----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
> >
> >
> >
> >
> >
next prev parent reply other threads:[~2021-02-23 1:16 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20210217090143.20032-1-purna.chandra.rao.bandaru@intel.com>
2021-02-22 8:38 ` [PATCH] MdeModulePkg/UfsPassThruDxe: Improve Error handling of Ufs Pass Thru driver Wu, Hao A
2021-02-22 8:39 ` [edk2-devel] " Wu, Hao A
2021-02-22 17:10 ` Bandaru, Purna Chandra Rao
2021-02-23 1:16 ` Wu, Hao A [this message]
2021-02-23 14:35 ` Bandaru, Purna Chandra Rao
2021-02-24 1:20 ` Wu, Hao A
2021-02-25 1:45 ` 回复: " gaoliming
2021-02-25 1:52 ` Wu, Hao A
2021-02-11 12:59 Purna Chandra Rao Bandaru
2021-02-17 14:10 ` [edk2-devel] " Laszlo Ersek
2021-02-18 1:57 ` Wu, Hao A
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN8PR11MB3666A56144D85EB18F7EBC3DCA809@BN8PR11MB3666.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox