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: 5029 bytes --]

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

> 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.
>
=> The problem is the old version of this file has not passed the
uncrustify tool. So without the "unnecessary changes", this MR will not
pass the CI.

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

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