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

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

Leif & Mike

I will help to submit another tracker to Bugzilla to track this request.

1. create below new clear time definitions
2. remove those separate definitions in modules to get code clean.

Thanks
Feng

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif Lindholm
Sent: Thursday, September 15, 2016 6:20 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: 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 Wed, Sep 14, 2016 at 05:14:19PM +0000, Kinney, Michael D wrote:
> 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.

Correct.

> 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)
> 

Either (or both) of those two look good to me. The latter has the benefit of a smaller call site, at the cost of perhaps obscuring the dependency on UefiRuntimeServicesTableLib.

> 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) 

I'm less concerned about those, but it could make sense for completeness.

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

It would certainly be good to reduce duplication, and consistency would help with the readability of the code. (Which is good for
reviewing.)

Regards,

Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel