From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f42.google.com (mail-ej1-f42.google.com [209.85.218.42]) by mx.groups.io with SMTP id smtpd.web11.1623.1686088064619283373 for ; Tue, 06 Jun 2023 14:47:45 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=LmFnq8pk; spf=pass (domain: ventanamicro.com, ip: 209.85.218.42, mailfrom: tphan@ventanamicro.com) Received: by mail-ej1-f42.google.com with SMTP id a640c23a62f3a-977e83d536fso359660066b.3 for ; Tue, 06 Jun 2023 14:47:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1686088063; x=1688680063; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=bEboWGPjgc0Yh7Twire8adjqwiWEbQxpmAUqWBUTcV4=; b=LmFnq8pkZ10v9f3LlyRFZkO/rwtb8UiXsHZN0K5l8qgg1E/4ron5P0L5jwXglsD4Jy bnwIpMPLlFsOzRf4W8N7fbUQidXqxQDiTW3ts0Xh9vhmKDHPSqqyufSLiOq2wKWI4IQ3 CSTtGC+UzKHTHlMZ00hiRxCizzHu3idJ2UV1pY6bKrn24rqeZRvDt1R1QWzOQxECNW4Z d74RQsj0VA4W2ltHrgycDsl1b3oels1lBWG2+TqzqSB76gZAzuBPXwi+VfCrB+DvMDFc t+FgLf+WY7BS+sy1hgkPyG6/iy27dlrRb9C1UKHwtXH7cOYvwtLFL6vyQm+0RlKXE7j1 x0aQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686088063; x=1688680063; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=bEboWGPjgc0Yh7Twire8adjqwiWEbQxpmAUqWBUTcV4=; b=B578NynI3rMkwX6IMUflo7F2r1V40YqlMmnaDBS8UZGwhZ4qAI7OOMTUbHtindwOXb 3ydXfekCFmtM23InR0Q99qnVG3KJUP2hUR300EKnMwJ7HFq9Gk79QM3Ju74ZPgTsTdJN r2xCyrtem4wY32T0yt0ljoxCrmNaKkniTOoiieNe/YjGxrAsim9JEM+xWZkHGG1Ooduh FrG+ebJ+w/STIEpssVDmkMm0iJDBS5Eaj2UDHrkZymKgPszTn4N4QgfHxN2yau3G1GJ7 wrcHvJedAGbLL6f1oejD7KmJzcuPLQrC/sK9knrxXakYGES+/PRCgwVU4JQSbXpmrGrl zjtg== X-Gm-Message-State: AC+VfDyH8eYl4c+uGyX0HwveUqceOputHWLQ/M313izcdqFjQix6d3hs JYogDuvJLlsPrEPHtCmGPxAaJ9MpuQkyZQigVygIfw== X-Google-Smtp-Source: ACHHUZ7hwBiRiRzJTbIPsRPUw1p4RiI56cWhfWe//BoyR06spwCtDeg8hOI1NsDCnUbUitPvuZl7nIEX9MHMUjVV9ZQ= X-Received: by 2002:a17:907:7e84:b0:958:4c75:705e with SMTP id qb4-20020a1709077e8400b009584c75705emr3616828ejc.17.1686088063011; Tue, 06 Jun 2023 14:47:43 -0700 (PDT) MIME-Version: 1.0 References: <20230526232518.7141-1-tphan@ventanamicro.com> <20230526232518.7141-2-tphan@ventanamicro.com> In-Reply-To: From: "Tuan Phan" Date: Tue, 6 Jun 2023 14:47:31 -0700 Message-ID: Subject: Re: [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit To: Sunil V L Cc: devel@edk2.groups.io, eric.dong@intel.com, ray.ni@intel.com, rahul1.kumar@intel.com, git@danielschaefer.me, abner.chang@amd.com, kraxel@redhat.com, andrei.warkentin@intel.com Content-Type: multipart/alternative; boundary="0000000000000e632105fd7cf800" --0000000000000e632105fd7cf800 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Jun 6, 2023 at 10:10=E2=80=AFAM Sunil V L wrote: > On Tue, Jun 06, 2023 at 10:02:08AM -0700, Tuan Phan wrote: > > On Tue, Jun 6, 2023 at 3:27=E2=80=AFAM Sunil V L > 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 > > > > --- > > > > 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 =3D Delay >> (RISCV_TIMER_COMPARE_BITS - 2); > > > > - Delay &=3D ((1 << (RISCV_TIMER_COMPARE_BITS - 2)) - 1); > > > > - do { > > > > - // > > > > - // The target timer count is calculated here > > > > - // > > > > - Ticks =3D RiscVReadTimer () + Delay; > > > > - Delay =3D 1 << (RISCV_TIMER_COMPARE_BITS - 2); > > > > - while (((Ticks - RiscVReadTimer ()) & (1 << > > > (RISCV_TIMER_COMPARE_BITS - 1))) =3D=3D 0) { > > > > - CpuPause (); > > > > - } > > > > - } while (Times-- > 0); > > > > + UINT64 Ticks; > > > > + > > > > + Ticks =3D RiscVReadTimer () + Delay; > > > > + > > > > + while (RiscVReadTimer () <=3D 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 ru= n > > > CI tests? > > > > > =3D> That is the result of running crutinize tool with edk2 config. Sho= uld > 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. > =3D> 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 > > > > --0000000000000e632105fd7cf800 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Jun 6, 2023 at 10:10=E2=80=AF= AM Sunil V L <sunilvl@ventan= amicro.com> wrote:
On Tue, Jun 06, 2023 a= t 10:02:08AM -0700, Tuan Phan wrote:
> On Tue, Jun 6, 2023 at 3:27=E2=80=AFAM Sunil V L <sunilvl@ventanamicro.com&g= t; wrote:
>
> > On Fri, May 26, 2023 at 04:25:18PM -0700, Tuan Phan wrote:
> > > The timer compare register is 64-bit so simplifying the dela= y
> > > function.
> > >
> > > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> > > ---
> > >=C2=A0 MdePkg/Include/Register/RiscV64/RiscVImpl.h=C2=A0 =C2= =A0|=C2=A0 1 -
> > >=C2=A0 .../BaseRiscV64CpuTimerLib/CpuTimerLib.c=C2=A0 =C2=A0 = =C2=A0 | 62 +++++++++----------
> > >=C2=A0 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 @@
> > >=C2=A0 =C2=A0 Name:
> > >
> > >=C2=A0 #define ASM_FUNC(Name)=C2=A0 _ASM_FUNC(ASM_PFX(Name), = .text. ## Name)
> > > -#define RISCV_TIMER_COMPARE_BITS=C2=A0 32
> > >
> > >=C2=A0 #endif
> > > diff --git a/UefiCpuPkg/Library/BaseRiscV64CpuTimerLib/CpuTi= merLib.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 @@
> > >=C2=A0 =C2=A0 @param=C2=A0 Delay=C2=A0 =C2=A0 =C2=A0A period = of time to delay in ticks.
> > >
> > >=C2=A0 **/
> > > +STATIC
> > >=C2=A0 VOID
> > >=C2=A0 InternalRiscVTimerDelay (
> > > -=C2=A0 IN UINT32=C2=A0 Delay
> > > +=C2=A0 IN UINT64=C2=A0 Delay
> > >=C2=A0 =C2=A0 )
> > >=C2=A0 {
> > > -=C2=A0 UINT32=C2=A0 Ticks;
> > > -=C2=A0 UINT32=C2=A0 Times;
> > > -
> > > -=C2=A0 Times=C2=A0 =3D Delay >> (RISCV_TIMER_COMPARE_= BITS - 2);
> > > -=C2=A0 Delay &=3D ((1 << (RISCV_TIMER_COMPARE_BIT= S - 2)) - 1);
> > > -=C2=A0 do {
> > > -=C2=A0 =C2=A0 //
> > > -=C2=A0 =C2=A0 // The target timer count is calculated here<= br> > > > -=C2=A0 =C2=A0 //
> > > -=C2=A0 =C2=A0 Ticks =3D RiscVReadTimer () + Delay;
> > > -=C2=A0 =C2=A0 Delay =3D 1 << (RISCV_TIMER_COMPARE_BIT= S - 2);
> > > -=C2=A0 =C2=A0 while (((Ticks - RiscVReadTimer ()) & (1 = <<
> > (RISCV_TIMER_COMPARE_BITS - 1))) =3D=3D 0) {
> > > -=C2=A0 =C2=A0 =C2=A0 CpuPause ();
> > > -=C2=A0 =C2=A0 }
> > > -=C2=A0 } while (Times-- > 0);
> > > +=C2=A0 UINT64=C2=A0 Ticks;
> > > +
> > > +=C2=A0 Ticks =3D RiscVReadTimer () + Delay;
> > > +
> > > +=C2=A0 while (RiscVReadTimer () <=3D Ticks) {
> > > +=C2=A0 =C2=A0 CpuPause ();
> > > +=C2=A0 }
> > >=C2=A0 }
> > >
> > >=C2=A0 /**
> > > @@ -61,14 +54,14 @@ MicroSecondDelay (
> > >=C2=A0 =C2=A0 )
> > >=C2=A0 {
> > >=C2=A0 =C2=A0 InternalRiscVTimerDelay (
> > > -=C2=A0 =C2=A0 (UINT32)DivU64x32 (
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 MultU64x32= (
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Mic= roSeconds,
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Pcd= Get64 (PcdCpuCoreCrystalClockFrequency)
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ),<= br> > > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 1000000u > > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> > > -=C2=A0 =C2=A0 );
> > > +=C2=A0 =C2=A0 DivU64x32 (
> > > +=C2=A0 =C2=A0 =C2=A0 MultU64x32 (
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 MicroSeconds,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 PcdGet64 (PcdCpuCoreCrystalCloc= kFrequency)
> > > +=C2=A0 =C2=A0 =C2=A0 ),
> > > +=C2=A0 =C2=A0 =C2=A0 1000000u
> > > +=C2=A0 =C2=A0 )
> > > +=C2=A0 );
> > >=C2=A0 =C2=A0 return MicroSeconds;
> > >=C2=A0 }
> > >
> > > @@ -89,14 +82,14 @@ NanoSecondDelay (
> > >=C2=A0 =C2=A0 )
> > >=C2=A0 {
> > >=C2=A0 =C2=A0 InternalRiscVTimerDelay (
> > > -=C2=A0 =C2=A0 (UINT32)DivU64x32 (
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 MultU64x32= (
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Nan= oSeconds,
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Pcd= Get64 (PcdCpuCoreCrystalClockFrequency)
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ),<= br> > > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 1000000000= u
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
> > > -=C2=A0 =C2=A0 );
> > > +=C2=A0 =C2=A0 DivU64x32 (
> > > +=C2=A0 =C2=A0 =C2=A0 MultU64x32 (
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 NanoSeconds,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 PcdGet64 (PcdCpuCoreCrystalCloc= kFrequency)
> > > +=C2=A0 =C2=A0 =C2=A0 ),
> > > +=C2=A0 =C2=A0 =C2=A0 1000000000u
> > > +=C2=A0 =C2=A0 )
> > > +=C2=A0 );
> > >=C2=A0 =C2=A0 return NanoSeconds;
> > >=C2=A0 }
> > >
> > > @@ -147,8 +140,9 @@ GetPerformanceCounter (
> > >=C2=A0 UINT64
> > >=C2=A0 EFIAPI
> > >=C2=A0 GetPerformanceCounterProperties (
> > > -=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0 UINT64 *StartValue, OPTIONAL=
> > > -=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0 UINT64=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *EndValue=C2=A0 =C2=A0 =C2= =A0OPTIONAL
> > > +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0 UINT64=C2=A0 *StartValue, > > > +=C2=A0 OPTIONAL
> > > +=C2=A0 OUT=C2=A0 =C2=A0 =C2=A0 UINT64=C2=A0 *EndValue=C2=A0= =C2=A0 =C2=A0OPTIONAL
> >
> > Hi Tuan,
> >
> > What is this change? The formatting doesn't look correct. Hav= e you run
> > CI tests?
> >
> =3D> 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.
=3D> The prob= lem 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.<= /div>

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