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

["Tuan Phan" <tphan@ventanamicro.com>]

[-- Attachment #1: Type: text/plain, Size: 3989 bytes --]

On Tue, Jun 6, 2023 at 3:27 AM Sunil V L <sunilvl@ventanamicro.com> wrote:

> 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?
>
=> That is the result of running crutinize tool with edk2 config. Should I
leave it as before?

>
> Otherwise LGTM. Thanks for the fix!
>
> Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
>

[-- Attachment #2: Type: text/html, Size: 5567 bytes --]