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