* [PATCH 1/2] UefiCpuPkg: CpuTimerDxeRiscV64: Fix incorrect value sent to SbiSetTimer
@ 2023-05-26 23:25 Tuan Phan
2023-05-26 23:25 ` [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit Tuan Phan
2023-06-06 10:25 ` [PATCH 1/2] UefiCpuPkg: CpuTimerDxeRiscV64: Fix incorrect value sent to SbiSetTimer Sunil V L
0 siblings, 2 replies; 7+ messages in thread
From: Tuan Phan @ 2023-05-26 23:25 UTC (permalink / raw)
To: devel
Cc: eric.dong, ray.ni, rahul1.kumar, git, abner.chang, kraxel,
andrei.warkentin, sunilvl, Tuan Phan
SbiSetTimer expects core tick value.
Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
---
.../CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf | 3 +++
UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 26 ++++++++++++++++---
UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h | 2 +-
3 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf b/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
index c76bd9648373..cd58d3a2f86b 100644
--- a/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
+++ b/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
@@ -40,6 +40,9 @@
Timer.h
Timer.c
+[Pcd]
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency ## CONSUMES
+
[Protocols]
gEfiCpuArchProtocolGuid ## CONSUMES
gEfiTimerArchProtocolGuid ## PRODUCES
diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
index fa957ba5e3e9..a8afb649149f 100644
--- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
+++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
@@ -80,8 +80,15 @@ TimerInterruptHandler (
return;
}
- mLastPeriodStart = PeriodStart;
- SbiSetTimer (PeriodStart += mTimerPeriod);
+ mLastPeriodStart = PeriodStart;
+ PeriodStart += DivU64x32 (
+ MultU64x32 (
+ mTimerPeriod,
+ PcdGet64 (PcdCpuCoreCrystalClockFrequency)
+ ),
+ 1000000u
+ ); // convert to tick
+ SbiSetTimer (PeriodStart);
RiscVEnableTimerInterrupt (); // enable SMode timer int
gBS->RestoreTPL (OriginalTPL);
}
@@ -163,6 +170,8 @@ TimerDriverSetTimerPeriod (
IN UINT64 TimerPeriod
)
{
+ UINT64 PeriodStart;
+
DEBUG ((DEBUG_INFO, "TimerDriverSetTimerPeriod(0x%lx)\n", TimerPeriod));
if (TimerPeriod == 0) {
@@ -171,9 +180,18 @@ TimerDriverSetTimerPeriod (
return EFI_SUCCESS;
}
- mTimerPeriod = TimerPeriod / 10; // convert unit from 100ns to 1us
+ mTimerPeriod = TimerPeriod / 10; // convert unit from 100ns to 1us
+
mLastPeriodStart = RiscVReadTimer ();
- SbiSetTimer (mLastPeriodStart + mTimerPeriod);
+ PeriodStart = mLastPeriodStart;
+ PeriodStart += DivU64x32 (
+ MultU64x32 (
+ mTimerPeriod,
+ PcdGet64 (PcdCpuCoreCrystalClockFrequency)
+ ),
+ 1000000u
+ ); // convert to tick
+ SbiSetTimer (PeriodStart);
mCpu->EnableInterrupt (mCpu);
RiscVEnableTimerInterrupt (); // enable SMode timer int
diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
index 586eb0cfadb4..9b3542230cb5 100644
--- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
+++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
@@ -21,7 +21,7 @@
#include <Library/IoLib.h>
//
-// RISC-V use 100us timer.
+// RISC-V use 100ns timer.
// The default timer tick duration is set to 10 ms = 10 * 1000 * 10 100 ns units
//
#define DEFAULT_TIMER_TICK_DURATION 100000
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit
2023-05-26 23:25 [PATCH 1/2] UefiCpuPkg: CpuTimerDxeRiscV64: Fix incorrect value sent to SbiSetTimer Tuan Phan
@ 2023-05-26 23:25 ` Tuan Phan
2023-06-06 10:27 ` Sunil V L
2023-06-06 10:25 ` [PATCH 1/2] UefiCpuPkg: CpuTimerDxeRiscV64: Fix incorrect value sent to SbiSetTimer Sunil V L
1 sibling, 1 reply; 7+ messages in thread
From: Tuan Phan @ 2023-05-26 23:25 UTC (permalink / raw)
To: devel
Cc: eric.dong, ray.ni, rahul1.kumar, git, abner.chang, kraxel,
andrei.warkentin, sunilvl, Tuan Phan
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
)
{
if (StartValue != NULL) {
--
2.25.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] UefiCpuPkg: CpuTimerDxeRiscV64: Fix incorrect value sent to SbiSetTimer
2023-05-26 23:25 [PATCH 1/2] UefiCpuPkg: CpuTimerDxeRiscV64: Fix incorrect value sent to SbiSetTimer Tuan Phan
2023-05-26 23:25 ` [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit Tuan Phan
@ 2023-06-06 10:25 ` Sunil V L
1 sibling, 0 replies; 7+ messages in thread
From: Sunil V L @ 2023-06-06 10:25 UTC (permalink / raw)
To: Tuan Phan
Cc: devel, eric.dong, ray.ni, rahul1.kumar, git, abner.chang, kraxel,
andrei.warkentin
On Fri, May 26, 2023 at 04:25:17PM -0700, Tuan Phan wrote:
> SbiSetTimer expects core tick value.
>
> Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> ---
Thanks for the patch!. LGTM.
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit
2023-05-26 23:25 ` [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit Tuan Phan
@ 2023-06-06 10:27 ` Sunil V L
2023-06-06 17:02 ` Tuan Phan
0 siblings, 1 reply; 7+ messages in thread
From: Sunil V L @ 2023-06-06 10:27 UTC (permalink / raw)
To: Tuan Phan
Cc: devel, eric.dong, ray.ni, rahul1.kumar, git, abner.chang, kraxel,
andrei.warkentin
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?
Otherwise LGTM. Thanks for the fix!
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit
2023-06-06 10:27 ` Sunil V L
@ 2023-06-06 17:02 ` Tuan Phan
2023-06-06 17:10 ` Sunil V L
0 siblings, 1 reply; 7+ messages in thread
From: Tuan Phan @ 2023-06-06 17:02 UTC (permalink / raw)
To: Sunil V L
Cc: devel, eric.dong, ray.ni, rahul1.kumar, git, abner.chang, kraxel,
andrei.warkentin
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit
2023-06-06 17:02 ` Tuan Phan
@ 2023-06-06 17:10 ` Sunil V L
2023-06-06 21:47 ` Tuan Phan
0 siblings, 1 reply; 7+ messages in thread
From: Sunil V L @ 2023-06-06 17:10 UTC (permalink / raw)
To: Tuan Phan
Cc: devel, eric.dong, ray.ni, rahul1.kumar, git, abner.chang, kraxel,
andrei.warkentin
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>
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit
2023-06-06 17:10 ` Sunil V L
@ 2023-06-06 21:47 ` Tuan Phan
0 siblings, 0 replies; 7+ messages in thread
From: Tuan Phan @ 2023-06-06 21:47 UTC (permalink / raw)
To: Sunil V L
Cc: devel, eric.dong, ray.ni, rahul1.kumar, git, abner.chang, kraxel,
andrei.warkentin
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-06-06 21:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-26 23:25 [PATCH 1/2] UefiCpuPkg: CpuTimerDxeRiscV64: Fix incorrect value sent to SbiSetTimer Tuan Phan
2023-05-26 23:25 ` [PATCH 2/2] UefiCpuPkg: RISC-V: TimerLib: Fix delay function to use 64-bit Tuan Phan
2023-06-06 10:27 ` Sunil V L
2023-06-06 17:02 ` Tuan Phan
2023-06-06 17:10 ` Sunil V L
2023-06-06 21:47 ` Tuan Phan
2023-06-06 10:25 ` [PATCH 1/2] UefiCpuPkg: CpuTimerDxeRiscV64: Fix incorrect value sent to SbiSetTimer Sunil V L
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox