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

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

Andrew,

The 100 ns vs. 1 us differences are what I tried to highlight here and propose some helpers to hide that difference.

Do you have some alternatives in mind?

Mike

> -----Original Message-----
> From: afish@apple.com [mailto:afish@apple.com]
> Sent: Wednesday, September 14, 2016 10:37 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Tian, Feng <feng.tian@intel.com>; 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 Sep 14, 2016, at 10:14 AM, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> >
> > 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?
> >
> 
> MIke,
> 
> We have some APIs that are 100ns vs. MIcroSeconds and that causes confusion for
> folks. Maybe we should have some helpers for both APIs and comments about which are
> which to help with the confusion. For example list the APIs that take the 100ns
> argument, and list the APIs that take Microseconds.
> 
> Thanks,
> 
> Andrew Fish
> 
> 
> > 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
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel