Re: [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

["Tian, Feng" <feng.tian@intel.com>]

Lefi,

This delay is just a workaround for some XHCI HCs, I will add such info into commit log.

Thanks
Feng

-----Original Message-----
From: Leif Lindholm [mailto:leif.lindholm@linaro.org] 
Sent: Wednesday, September 14, 2016 9:09 PM
To: Tian, Feng <feng.tian@intel.com>
Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [patch] MdeModulePkg/Xhci: add 1ms delay before access MMIO reg during reset

On Wed, Sep 14, 2016 at 09:37:13AM +0800, Feng Tian wrote:
> Some XHCI host controllers require to have extra 1ms delay before 
> accessing any MMIO register during HC reset.

Is this compliant with the XHCI specification or a bug workaround?
I am not going to argue about the delay, but I would like to see it spelled out in the commit message.

> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Feng Tian <feng.tian@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c 
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index d0f2205..cb822a6 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -2,7 +2,7 @@
>  
>    The XHCI register operation routines.
>  
> -Copyright (c) 2011 - 2015, Intel Corporation. All rights 
> reserved.<BR>
> +Copyright (c) 2011 - 2016, Intel Corporation. All rights 
> +reserved.<BR>
>  This program and the accompanying materials  are licensed and made 
> available under the terms and conditions of the BSD License  which 
> accompanies this distribution.  The full text of the license may be 
> found at @@ -687,6 +687,10 @@ XhcResetHC (
>    if ((Xhc->DebugCapSupOffset == 0xFFFFFFFF) || ((XhcReadExtCapReg (Xhc, Xhc->DebugCapSupOffset) & 0xFF) != XHC_CAP_USB_DEBUG) ||
>        ((XhcReadExtCapReg (Xhc, Xhc->DebugCapSupOffset + XHC_DC_DCCTRL) & BIT0) == 0)) {
>      XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RESET);
> +    //
> +    // some XHCI host controllers require to have extra 1ms delay before accessing any MMIO register during reset.
> +    //
> +    gBS->Stall (XHC_1_MILLISECOND);

OK, so this is not actually a comment on this patch, but why do we have so many separate definitions of how many microseconds go in a milisecond or second?

USB_BUS_1_MILLISECOND (Pei and Dxe)
XHC_1_MILLISECOND     (Pei and Dxe)
EHC_1_MILLISECOND     (Pei and Dxe)
UHC_1_MILLISECOND     (Dxe, Pei defines STALL_1_MILLI_SECOND instead)

And IdeBusPei/PciBusDxe do similar things.

Could we add something common to Base.h instead, like the SIZE_ ones, droppping a lot of duplication, gaining uniformity, and correcting spelling?

/
    Leif

>      Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_RESET, FALSE, Timeout);
>    }
>  
> --
> 2.7.1.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel