On Tue, Jun 06, 2023 at 10:02:08AM -0700, Tuan Phan wrote:
> 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?
>
I have not used crutinize tool. But this change looks unnecessary. I
would use uncrustify tool locally for any formatting fixes since the
same tool gets used in CI tests also.

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