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

[Leif Lindholm <leif.lindholm@linaro.org>]

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