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

[Andrew Fish <afish@apple.com>]

> 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