From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f47.google.com (mail-ed1-f47.google.com [209.85.208.47]) by mx.groups.io with SMTP id smtpd.web11.2138.1686070941551036002 for ; Tue, 06 Jun 2023 10:02:21 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=fZktNAu9; spf=pass (domain: ventanamicro.com, ip: 209.85.208.47, mailfrom: tphan@ventanamicro.com) Received: by mail-ed1-f47.google.com with SMTP id 4fb4d7f45d1cf-51491b87565so9717810a12.1 for ; Tue, 06 Jun 2023 10:02:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1686070940; x=1688662940; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=zbgDnDWNuxt506ZX2I7MVgAbYgpDg2UMZvZQ+Kp0gMM=; b=fZktNAu9GPmWqiyTsK7D/od0DHqa8mUCsglNX/vPX2C3r99lzq1DG6uK7s5UQm8n0c ncZP7usZdeY7JemioyfNrgBf4RRakZbJfbA8yOSKs5C6NBrSBPJeoSVETbmTLARmI0GR gxLMz9T2wtQ7omILjgC/xRzAt2kgKUNPv9CYzImwwh0VFXIqBN3LshSoTvYLwwW9rZts tlxIgmmeAWa/Lqd0vkRw7gOwBl/0m38AlKXe9EE4A+HxCn7s69FoNG64/WDqUrh7+2h2 G5NRPYtr23nJYSgwF2ATLoqfLtahrC58AWIlsoKKKL7WDUoP7tFW6Uy2HhTJcg3qI0Kd 54bA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686070940; x=1688662940; 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=zbgDnDWNuxt506ZX2I7MVgAbYgpDg2UMZvZQ+Kp0gMM=; b=BfDe1HUA3Z7KCBbWWmnHD11rtRUA6dcNa3nvZ03jv7AJ/V7B5QuGMfCV5fuBofkcy+ FaxZV127Wwyl1KsFnWjZWCQvh0UKDzUWvE9ehZpbzW/YnKXPOQGizrLGyvtsDfk8NsVV USfapfyC/BB4vV43QzGnxu8hFlG21c+uWVmjv5Ta+xqMAvAUXgJddAgH7iA59ZV38lGb egY3KhP1SZN6G9BjAEDQKL4v+6hSQe2oC76G07Uw5aVwEfBGJ2xrvGAVcdJj7S40QvOR 9aYYu0BAcc4SSHkpjYu6HuvdAgawDTrrK/p0WHHVBvmeD7i5SutG2lE5n86M9Irunatx hx5Q== X-Gm-Message-State: AC+VfDxSEHwyNFbJl4yk+frb1TAp0eHMaPHN5GXBSzx8ANL9GEklSycG c14geZuM0k7fzEX2pH/pB+j96rtONXizWL6pgMzLKQ== X-Google-Smtp-Source: ACHHUZ5b1rWeZgr5D5pvBVIpzuqC8IeMIqrjx7g3ZHOjzAp/5/mHc6KBitec1ib5TsGXFC8YIJbAoGBHkQYWDmxUITc= X-Received: by 2002:aa7:ce04:0:b0:514:9bb7:d0bd with SMTP id d4-20020aa7ce04000000b005149bb7d0bdmr2770104edv.24.1686070940007; Tue, 06 Jun 2023 10:02:20 -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 10:02:08 -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="00000000000072265705fd78fb71" --00000000000072265705fd78fb71 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 run > CI tests? > =3D> 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 > --00000000000072265705fd78fb71 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Jun 6, 2023 at 3:27=E2=80=AFA= M Sunil V L <sunilvl@ventana= micro.com> wrote:
On Fri, May 26, 2023 a= t 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>
> ---
>=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 | 6= 2 +++++++++----------
>=C2=A0 2 files changed, 28 insertions(+), 35 deletions(-)
>
> diff --git a/MdePkg/Include/Register/RiscV64/RiscVImpl.h b/MdePkg/Incl= ude/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
>=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
>=C2=A0 #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 @@
>=C2=A0 =C2=A0 @param=C2=A0 Delay=C2=A0 =C2=A0 =C2=A0A period of time to= delay in ticks.
>=C2=A0
>=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_BITS - 2)) - = 1);
> -=C2=A0 do {
> -=C2=A0 =C2=A0 //
> -=C2=A0 =C2=A0 // The target timer count is calculated here
> -=C2=A0 =C2=A0 //
> -=C2=A0 =C2=A0 Ticks =3D RiscVReadTimer () + Delay;
> -=C2=A0 =C2=A0 Delay =3D 1 << (RISCV_TIMER_COMPARE_BITS - 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
>=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 MicroSeconds,=
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 PcdGet64 (Pcd= CpuCoreCrystalClockFrequency)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ),
> -=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 (PcdCpuCoreCrystalClockFrequency= )
> +=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 }
>=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 NanoSeconds,<= br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 PcdGet64 (Pcd= CpuCoreCrystalClockFrequency)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ),
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 1000000000u
> -=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 (PcdCpuCoreCrystalClockFrequency= )
> +=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 }
>=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=A0OPTIONA= L
> +=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. Have you run<= br> CI tests?
=3D> That is the result of running crutin= ize tool with edk2 config. Should I leave it as before?

Otherwise LGTM. Thanks for the fix!

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