On 1/4/24 16:01, Sunil V L wrote:
> Hi Laszlo,
>
> Thank you very much for the review!.
>
> On Thu, Jan 04, 2024 at 03:38:17PM +0100, Laszlo Ersek wrote:
>> On 1/3/24 14:58, Sunil V L wrote:
>>> Sstc extension allows to program the timer and receive the interrupt
>>> without using an SBI call. This reduces the latency to generate the timer
>>> interrupt. So, detect whether Sstc extension is supported and use the
>>> stimecmp register directly to program the timer interrupt.
>>>
>>> Cc: Gerd Hoffmann <[email protected]>
>>> Cc: Rahul Kumar <[email protected]>
>>> Cc: Laszlo Ersek <[email protected]>
>>> Cc: Ray Ni <[email protected]>
>>> Cc: Andrei Warkentin <[email protected]>
>>> Signed-off-by: Sunil V L <[email protected]>
>>> ---
>>> .../CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf | 1 +
>>> UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h | 2 ++
>>> UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c | 30 +++++++++++++++++--
>>> 3 files changed, 31 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
>>> b/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
>>> index aba660186dc0..f2a2cf12caef 100644
>>> --- a/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
>>> +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf
>>> @@ -41,6 +41,7 @@ [Sources.RISCV64]
>>> Timer.c
>>>
>>> [Pcd]
>>> + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency ## CONSUMES
>>>
>>> [Protocols]
>>> diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
>>> b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
>>> index 9b3542230cb5..5e5071b3f0b2 100644
>>> --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
>>> +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.h
>>> @@ -26,6 +26,8 @@
>>> //
>>> #define DEFAULT_TIMER_TICK_DURATION 100000
>>>
>>> +#define RISCV_CPU_FEATURE_SSTC_BITMASK 0x2
>>
>> (1) Not a bug by any means, but BIT1 might read more idiomatic.
>>
> Agree. Would RISCV_CPU_FEATURE_BIT1_SSTC be better?
Sorry, I was unclear: the macro *name* was fine, IMO; my proposal was to
change the *replacement text* from 0x2 to BIT1. (Because BIT1 is another
macro, from "MdePkg/Include/Base.h"; those are frequently used all over
edk2.)
Thanks!
Laszlo
>
>>> +
>>> extern VOID
>>> RiscvSetTimerPeriod (
>>> UINT32 TimerPeriod
>>> diff --git a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
>>> b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
>>> index 30e48061cd06..4babfb4bfc60 100644
>>> --- a/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
>>> +++ b/UefiCpuPkg/CpuTimerDxeRiscV64/Timer.c
>>> @@ -44,6 +44,19 @@ STATIC EFI_TIMER_NOTIFY mTimerNotifyFunction;
>>> STATIC UINT64 mTimerPeriod = 0;
>>> STATIC UINT64 mLastPeriodStart = 0;
>>>
>>> +/**
>>> + Check whether Sstc is enabled in PCD.
>>> +
>>> +**/
>>> +STATIC
>>> +BOOLEAN
>>> +RiscVIsSstcEnabled (
>>> + VOID
>>> + )
>>> +{
>>> + return ((PcdGet64 (PcdRiscVFeatureOverride) &
>>> RISCV_CPU_FEATURE_SSTC_BITMASK) != 0);
>>> +}
>>> +
>>> /**
>>> Timer Interrupt Handler.
>>>
>>> @@ -94,7 +107,12 @@ TimerInterruptHandler (
>>> ),
>>> 1000000u
>>> ); // convert to tick
>>> - SbiSetTimer (PeriodStart);
>>> + if (RiscVIsSstcEnabled ()) {
>>
>> (2) Even though the PCD is currently declared as fixed or
>> patchable-in-module, seeing a PcdGet64() call on the call stack of the
>> timer interrupt handler (and at a high TPL) makes me uncomfortable. It
>> carries a risk that later on we relax the PCD decl to dynamic, and then
>> this code would become brittle.
>>
>> I propose: either replace the PcdGet64 call above with FixedPcdGet64 (so
>> it can never land in the runtime / dynamic PCD protocol), or perform the
>> PCD check in the entry point function of the driver, and store the
>> result in a STATIC BOOLEAN variable. Then further PCD accesses (dynamic
>> or otherwise) will not be needed.
>>
> Ahh yes. Good point. Let me use a static variable as you suggested.
>
>>> + RiscVSetSupervisorTimeCompareRegister (PeriodStart);
>>> + } else {
>>> + SbiSetTimer (PeriodStart);
>>> + }
>>> +
>>> RiscVEnableTimerInterrupt (); // enable SMode timer int
>>> gBS->RestoreTPL (OriginalTPL);
>>> }
>>> @@ -197,7 +215,11 @@ TimerDriverSetTimerPeriod (
>>> ),
>>> 1000000u
>>> ); // convert to tick
>>> - SbiSetTimer (PeriodStart);
>>> + if (RiscVIsSstcEnabled ()) {
>>> + RiscVSetSupervisorTimeCompareRegister (PeriodStart);
>>> + } else {
>>> + SbiSetTimer (PeriodStart);
>>> + }
>>>
>>> mCpu->EnableInterrupt (mCpu);
>>> RiscVEnableTimerInterrupt (); // enable SMode timer int
>>
>> (3) This seems like duplicated code. How about replacing the
>> RiscVIsSstcEnabled() function with a more substantive function that
>> incorporates both the feature check *and* the "PeriodStart" setting?
>> Then you can easily call that function from both TimerInterruptHandler()
>> and TimerDriverSetTimerPeriod().
>>
> I agree. Let me update in the next version.
>
>>> @@ -282,6 +304,10 @@ TimerDriverInitialize (
>>> //
>>> mTimerNotifyFunction = NULL;
>>>
>>> + if (RiscVIsSstcEnabled ()) {
>>> + DEBUG ((DEBUG_INFO, "%a: Timer interrupt is via Sstc extension\n",
>>> __func__));
>>> + }
>>> +
>>
>> Right, this would be the place to fetch the PCD explicitly and to store
>> the result (based on bit-masking) into the global boolean.
>>
> Yes!
>
> Thanks!
> Sunil
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113284): https://edk2.groups.io/g/devel/message/113284
Mute This Topic: https://groups.io/mt/103501843/21656
Group Owner: [email protected]
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
