* [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 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
* 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
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