Re: [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit

["Sunil V L" <sunilvl@ventanamicro.com>]

On Fri, May 26, 2023 at 04:25:18PM -0700, Tuan Phan wrote:
> The timer compare register is 64-bit so simplifying the delay
> function.
> 
> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> ---
>  MdePkg/Include/Register/RiscV64/RiscVImpl.h   |  1 -
>  .../BaseRiscV64CpuTimerLib/CpuTimerLib.c      | 62 +++++++++----------
>  2 files changed, 28 insertions(+), 35 deletions(-)
> 
> diff --git a/MdePkg/Include/Register/RiscV64/RiscVImpl.h b/MdePkg/Include/Register/RiscV64/RiscVImpl.h
> index ee5c2ba60377..6997de6cc001 100644
> --- a/MdePkg/Include/Register/RiscV64/RiscVImpl.h
> +++ b/MdePkg/Include/Register/RiscV64/RiscVImpl.h
> @@ -20,6 +20,5 @@
>    Name:
>  
>  #define ASM_FUNC(Name)  _ASM_FUNC(ASM_PFX(Name), .text. ## Name)
> -#define RISCV_TIMER_COMPARE_BITS  32
>  
>  #endif
> diff --git a/UefiCpuPkg/Library/BaseRiscV64CpuTimerLib/CpuTimerLib.c b/UefiCpuPkg/Library/BaseRiscV64CpuTimerLib/CpuTimerLib.c
> index 9c8efc0f3530..57800177023c 100644
> --- a/UefiCpuPkg/Library/BaseRiscV64CpuTimerLib/CpuTimerLib.c
> +++ b/UefiCpuPkg/Library/BaseRiscV64CpuTimerLib/CpuTimerLib.c
> @@ -22,26 +22,19 @@
>    @param  Delay     A period of time to delay in ticks.
>  
>  **/
> +STATIC
>  VOID
>  InternalRiscVTimerDelay (
> -  IN UINT32  Delay
> +  IN UINT64  Delay
>    )
>  {
> -  UINT32  Ticks;
> -  UINT32  Times;
> -
> -  Times  = Delay >> (RISCV_TIMER_COMPARE_BITS - 2);
> -  Delay &= ((1 << (RISCV_TIMER_COMPARE_BITS - 2)) - 1);
> -  do {
> -    //
> -    // The target timer count is calculated here
> -    //
> -    Ticks = RiscVReadTimer () + Delay;
> -    Delay = 1 << (RISCV_TIMER_COMPARE_BITS - 2);
> -    while (((Ticks - RiscVReadTimer ()) & (1 << (RISCV_TIMER_COMPARE_BITS - 1))) == 0) {
> -      CpuPause ();
> -    }
> -  } while (Times-- > 0);
> +  UINT64  Ticks;
> +
> +  Ticks = RiscVReadTimer () + Delay;
> +
> +  while (RiscVReadTimer () <= Ticks) {
> +    CpuPause ();
> +  }
>  }
>  
>  /**
> @@ -61,14 +54,14 @@ MicroSecondDelay (
>    )
>  {
>    InternalRiscVTimerDelay (
> -    (UINT32)DivU64x32 (
> -              MultU64x32 (
> -                MicroSeconds,
> -                PcdGet64 (PcdCpuCoreCrystalClockFrequency)
> -                ),
> -              1000000u
> -              )
> -    );
> +    DivU64x32 (
> +      MultU64x32 (
> +        MicroSeconds,
> +        PcdGet64 (PcdCpuCoreCrystalClockFrequency)
> +      ),
> +      1000000u
> +    )
> +  );
>    return MicroSeconds;
>  }
>  
> @@ -89,14 +82,14 @@ NanoSecondDelay (
>    )
>  {
>    InternalRiscVTimerDelay (
> -    (UINT32)DivU64x32 (
> -              MultU64x32 (
> -                NanoSeconds,
> -                PcdGet64 (PcdCpuCoreCrystalClockFrequency)
> -                ),
> -              1000000000u
> -              )
> -    );
> +    DivU64x32 (
> +      MultU64x32 (
> +        NanoSeconds,
> +        PcdGet64 (PcdCpuCoreCrystalClockFrequency)
> +      ),
> +      1000000000u
> +    )
> +  );
>    return NanoSeconds;
>  }
>  
> @@ -147,8 +140,9 @@ GetPerformanceCounter (
>  UINT64
>  EFIAPI
>  GetPerformanceCounterProperties (
> -  OUT      UINT64 *StartValue, OPTIONAL
> -  OUT      UINT64                    *EndValue     OPTIONAL
> +  OUT      UINT64  *StartValue,
> +  OPTIONAL
> +  OUT      UINT64  *EndValue     OPTIONAL

Hi Tuan,

What is this change? The formatting doesn't look correct. Have you run
CI tests?

Otherwise LGTM. Thanks for the fix!

Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>