Hi Rebecca, I have a minor suggestion marked inline as [SAMI], otherwise this patch looks good to me.
Reviewed-by: Sami Mujawar <sami.muja...@arm.com> Regards, Sami Mujawar On 05/01/2024, 05:15, "Rebecca Cran" <rebe...@os.amperecomputing.com <mailto:rebe...@os.amperecomputing.com>> wrote: The calculation of the timer period was broken. Introduce a global mTimerPeriod so the calculation can be removed. Since mTimerFrequencyHz is only used in one place, remove the global and make it a local variable. Do the same with mNumTimerTicks. Signed-off-by: Rebecca Cran <rebe...@os.amperecomputing.com <mailto:rebe...@os.amperecomputing.com>> --- ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 36 ++++++++++++++---------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c index f8c39458a53a..78cee62a19d6 100644 --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c @@ -28,13 +28,10 @@ in a second */ #define TIME_UNITS_PER_SECOND 10000000 -// Tick frequency of the generic timer basis of the generic watchdog. -STATIC UINTN mTimerFrequencyHz = 0; - /* In cases where the compare register was set manually, information about how long the watchdog was asked to wait cannot be retrieved from hardware. It is therefore stored here. 0 means the timer is not running. */ -STATIC UINT64 mNumTimerTicks = 0; +STATIC UINT64 mTimerPeriod = 0; #define MAX_UINT48 0xFFFFFFFFFFFFULL @@ -91,7 +88,8 @@ WatchdogExitBootServicesEvent ( ) { WatchdogDisable (); - mNumTimerTicks = 0; + mTimerPeriod = 0; + mExitedBootServices = TRUE; } /* This function is called when the watchdog's first signal (WS0) goes high. @@ -106,7 +104,6 @@ WatchdogInterruptHandler ( ) { STATIC CONST CHAR16 ResetString[] = L"The generic watchdog timer ran out."; - UINT64 TimerPeriod; WatchdogDisable (); @@ -119,8 +116,7 @@ WatchdogInterruptHandler ( // the timer period plus 1. // if (mWatchdogNotify != NULL) { - TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks); - mWatchdogNotify (TimerPeriod + 1); + mWatchdogNotify (mTimerPeriod + 1); } gRT->ResetSystem ( @@ -200,22 +196,27 @@ WatchdogSetTimerPeriod ( IN UINT64 TimerPeriod // In 100ns units ) { - UINTN SystemCount; + UINTN SystemCount; + UINT64 TimerFrequencyHz; + UINT64 NumTimerTicks; // if TimerPeriod is 0, this is a request to stop the watchdog. if (TimerPeriod == 0) { - mNumTimerTicks = 0; + mTimerPeriod = 0; WatchdogDisable (); return EFI_SUCCESS; } // Work out how many timer ticks will equate to TimerPeriod - mNumTimerTicks = (mTimerFrequencyHz * TimerPeriod) / TIME_UNITS_PER_SECOND; + TimerFrequencyHz = ArmGenericTimerGetTimerFreq (); + ASSERT (TimerFrequencyHz != 0); + mTimerPeriod = TimerPeriod; + NumTimerTicks = (TimerFrequencyHz * TimerPeriod) / TIME_UNITS_PER_SECOND; /* If the number of required ticks is greater than the max the watchdog's offset register (WOR) can hold, we need to manually compute and set the compare register (WCV) */ - if (mNumTimerTicks > MAX_UINT48) { + if (NumTimerTicks > MAX_UINT48) { /* We need to enable the watchdog *before* writing to the compare register, because enabling the watchdog causes an "explicit refresh", which clobbers the compare register (WCV). In order to make sure this doesn't @@ -223,9 +224,9 @@ WatchdogSetTimerPeriod ( WatchdogWriteOffsetRegister (MAX_UINT48); WatchdogEnable (); SystemCount = ArmGenericTimerGetSystemCount (); - WatchdogWriteCompareRegister (SystemCount + mNumTimerTicks); + WatchdogWriteCompareRegister (SystemCount + NumTimerTicks); } else { - WatchdogWriteOffsetRegister (mNumTimerTicks); + WatchdogWriteOffsetRegister (NumTimerTicks); WatchdogEnable (); } @@ -260,7 +261,7 @@ WatchdogGetTimerPeriod ( return EFI_INVALID_PARAMETER; } - *TimerPeriod = ((TIME_UNITS_PER_SECOND / mTimerFrequencyHz) * mNumTimerTicks); + *TimerPeriod = mTimerPeriod; return EFI_SUCCESS; } @@ -327,9 +328,6 @@ GenericWatchdogEntry ( This will avoid conflicts with the universal watchdog */ ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEfiWatchdogTimerArchProtocolGuid); - mTimerFrequencyHz = ArmGenericTimerGetTimerFreq (); - ASSERT (mTimerFrequencyHz != 0); - // Install interrupt handler Status = mInterruptProtocol->RegisterInterruptSource ( mInterruptProtocol, @@ -371,7 +369,7 @@ GenericWatchdogEntry ( ); ASSERT_EFI_ERROR (Status); - mNumTimerTicks = 0; + mTimerPeriod = 0; [SAMI] I think we do not need to initialise mTimerPeriod to 0 here as it would already be 0, right? WatchdogDisable (); return EFI_SUCCESS; -- 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113273): https://edk2.groups.io/g/devel/message/113273 Mute This Topic: https://groups.io/mt/103538116/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
