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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Leif,

MdePkg/Include/Library/UefiLib.h does have some helper macros for setting timer events periods 
that are in 100 nS units:

  #define EFI_TIMER_PERIOD_MICROSECONDS(Microseconds) MultU64x32((UINT64)(Microseconds), 10)
  #define EFI_TIMER_PERIOD_MILLISECONDS(Milliseconds) MultU64x32((UINT64)(Milliseconds), 10000)
  #define EFI_TIMER_PERIOD_SECONDS(Seconds)           MultU64x32((UINT64)(Seconds), 10000000)

I believe the examples you show are for use with the gBS->Stall() service which is in 1 uS units.

Maybe we should consider some additional macros in UefiLib.h

  #define EFI_STALL_PERIOD_MICROSECONDS(Microseconds) (Microseconds)
  #define EFI_STALL_PERIOD_MILLISECONDS(Milliseconds) ((Milliseconds) * 1000)
  #define EFI_STALL_PERIOD_SECONDS(Seconds)           ((Seconds) * 1000000)

Or maybe some macros that actually do the call to gBS->Stall() too.

  #define EFI_STALL_MICROSECONDS(Microseconds) gBS->Stall (Microseconds)
  #define EFI_STALL_MILLISECONDS(Milliseconds) gBS->Stall ((Milliseconds) * 1000)
  #define EFI_STALL_SECONDS(Seconds)           gBS->Stall ((Seconds) * 1000000)

The other method of generating timed delays for PEI/DXE/SMM modules is using the 
Services in MdePkg/Include/Library/TimerLib.h:

  UINTN
  EFIAPI
  NanoSecondDelay (
    IN      UINTN                     NanoSeconds
    );

  UINTN
  EFIAPI
  MicroSecondDelay (
    IN      UINTN                     MicroSeconds
    );

If we wanted macros helper to use these services to match UEFI ones, maybe 
add the following to TimerLib.h:

  #define DELAY_NANOSECONDS(Nanoseconds)   NanoSecondDelay (Nanoseconds)
  #define DELAY_MICROSECONDS(Microseconds) MicroSecondDelay (Microseconds)
  #define DELAY_MILLISECONDS(Milliseconds) MicroSecondDelay ((Microseconds) * 1000)
  #define DELAY_SECONDS(Seconds)           MicroSecondDelay ((Microseconds) * 1000000) 

Do you think it would improve the maintenance of the code if macros like these
were used consistently?

Thanks,

Mike


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif Lindholm
> Sent: Wednesday, September 14, 2016 6:09 AM
> To: Tian, Feng <feng.tian@intel.com>
> Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>
> 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel