[edk2-devel] [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add system/user defined reset delay
Ard Biesheuvel
ard.biesheuvel at arm.com
Mon Jan 4 17:10:50 UTC 2021
On 12/25/20 7:10 PM, Samer El-Haj-Mahmoud wrote:
> I tested the patch using the procedure documented in https://github.com/pftf/RPi4/issues/95 (which is relatively easy to reproduce), and did not se the async exception/hang with SD card. The SCT Runtime Variables test complete without any hangs. It looks like this may be a sufficient workaround for that issue.
>
> Tested-By: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>
>
>
Pushed as a34e27a1fa79..94e9fba43d7e
Thanks all!
>> -----Original Message-----
>> From: Pete Batard <pete at akeo.ie>
>> Sent: Tuesday, December 22, 2020 8:45 AM
>> To: devel at edk2.groups.io
>> Cc: Ard Biesheuvel <Ard.Biesheuvel at arm.com>; leif at nuviainc.com; Samer
>> El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>; Andrei Warkentin
>> (awarkentin at vmware.com) <awarkentin at vmware.com>
>> Subject: [edk2-platforms][PATCH 1/1] Platform/RaspberryPi: Add
>> system/user defined reset delay
>>
>> Due to the method in which NV variables are stored on removable media
>> for the Raspberry Pi platform, and the manner in which we dump updated
>> variables right before reset, it is possible, and has been repeatedly
>> demonstrated with SSD-based USB 3.0 devices, that the updated file does
>> not actually end up being written to permanent storage, due to the
>> device write-cache not having enough time to be flushed before reset.
>>
>> To compensate for this, since we don't know of a generic method that
>> would allow turning off USB mass storage devices write cache (and also
>> because we are seeing an issue that seems related for SD-based media),
>> we add a new reset delay PCD, which can be set by the user, and which
>> we also set as required when NV variables are being dumped.
>>
>> Our testing show that, with more than 3 seconds of extra delay, the
>> storage media has enough time to finalize its internal write, thus
>> solving the issue of configuration changes not being persisted.
>>
>> Signed-off-by: Pete Batard <pete at akeo.ie>
>> ---
>> Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c |
>> 24 ++++++++++++++++++++
>> Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf |
>> 1 +
>> Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 11
>> +++++++++
>> Platform/RaspberryPi/Library/ResetLib/ResetLib.inf | 5 ++++
>> Platform/RaspberryPi/RPi3/RPi3.dsc | 6 +++++
>> Platform/RaspberryPi/RPi4/RPi4.dsc | 6 +++++
>> Platform/RaspberryPi/RaspberryPi.dec | 1 +
>> 7 files changed, 54 insertions(+)
>>
>> diff --git
>> a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c
>> b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c
>> index 07f3f1c24295..4071a3fca468 100644
>> ---
>> a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c
>> +++
>> b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.c
>> @@ -10,6 +10,18 @@
>>
>>
>> #include "VarBlockService.h"
>>
>>
>>
>> +//
>>
>> +// Minimum delay to enact before reset, when variables are dirty (in μs).
>>
>> +// Needed to ensure that SSD-based USB 3.0 devices have time to flush
>> their
>>
>> +// write cache after updating the NV vars. A much smaller delay is applied
>>
>> +// on Pi 3 compared to Pi 4, as we haven't had reports of issues there yet.
>>
>> +//
>>
>> +#if (RPI_MODEL == 3)
>>
>> +#define PLATFORM_RESET_DELAY 500000
>>
>> +#else
>>
>> +#define PLATFORM_RESET_DELAY 3500000
>>
>> +#endif
>>
>> +
>>
>> VOID *mSFSRegistration;
>>
>>
>>
>>
>>
>> @@ -154,6 +166,7 @@ DumpVars (
>> )
>>
>> {
>>
>> EFI_STATUS Status;
>>
>> + RETURN_STATUS PcdStatus;
>>
>>
>>
>> if (mFvInstance->Device == NULL) {
>>
>> DEBUG ((DEBUG_INFO, "Variable store not found?\n"));
>>
>> @@ -173,6 +186,17 @@ DumpVars (
>> }
>>
>>
>>
>> DEBUG ((DEBUG_INFO, "Variables dumped!\n"));
>>
>> +
>>
>> + //
>>
>> + // Add a reset delay to give time for slow/cached devices
>>
>> + // to flush the NV variables write to permanent storage.
>>
>> + // But only do so if this won't reduce an existing user-set delay.
>>
>> + //
>>
>> + if (PcdGet32 (PcdPlatformResetDelay) < PLATFORM_RESET_DELAY) {
>>
>> + PcdStatus = PcdSet32S (PcdPlatformResetDelay,
>> PLATFORM_RESET_DELAY);
>>
>> + ASSERT_RETURN_ERROR (PcdStatus);
>>
>> + }
>>
>> +
>>
>> mFvInstance->Dirty = FALSE;
>>
>> }
>>
>>
>>
>> diff --git
>> a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf
>> b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf
>> index ecfb8f85c9c1..c2edb25bd41d 100644
>> ---
>> a/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf
>> +++
>> b/Platform/RaspberryPi/Drivers/VarBlockServiceDxe/VarBlockServiceDxe.inf
>> @@ -79,6 +79,7 @@ [Pcd]
>> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
>>
>> gRaspberryPiTokenSpaceGuid.PcdNvStorageEventLogBase
>>
>> + gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay
>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
>>
>>
>>
>> [FeaturePcd]
>>
>> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
>> b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
>> index c62a92321ecb..4a50166dd63b 100644
>> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
>> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c
>> @@ -16,6 +16,7 @@
>>
>>
>> #include <Library/BaseLib.h>
>>
>> #include <Library/DebugLib.h>
>>
>> +#include <Library/TimerLib.h>
>>
>> #include <Library/EfiResetSystemLib.h>
>>
>> #include <Library/ArmSmcLib.h>
>>
>> #include <Library/UefiLib.h>
>>
>> @@ -44,6 +45,7 @@ LibResetSystem (
>> )
>>
>> {
>>
>> ARM_SMC_ARGS ArmSmcArgs;
>>
>> + UINT32 Delay;
>>
>>
>>
>> if (!EfiAtRuntime ()) {
>>
>> /*
>>
>> @@ -52,6 +54,15 @@ LibResetSystem (
>> EfiEventGroupSignal (&gRaspberryPiEventResetGuid);
>>
>> }
>>
>>
>>
>> + Delay = PcdGet32 (PcdPlatformResetDelay);
>>
>> + if (Delay != 0) {
>>
>> + DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n",
>>
>> + Delay / 1000000, (Delay % 1000000) / 100000));
>>
>> + MicroSecondDelay (Delay);
>>
>> + }
>>
>> + DEBUG ((DEBUG_INFO, "Platform %a.\n",
>>
>> + (ResetType == EfiResetShutdown) ? "shutdown" : "reset"));
>>
>> +
>>
>> switch (ResetType) {
>>
>> case EfiResetPlatformSpecific:
>>
>> // Map the platform specific reset as reboot
>>
>> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf
>> b/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf
>> index b02a06d9d0bf..9bdb94a52ebf 100644
>> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf
>> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.inf
>> @@ -33,8 +33,13 @@ [LibraryClasses]
>> DebugLib
>>
>> BaseLib
>>
>> ArmSmcLib
>>
>> + PcdLib
>>
>> + TimerLib
>>
>> UefiLib
>>
>> UefiRuntimeLib
>>
>>
>>
>> [Guids]
>>
>> gRaspberryPiEventResetGuid
>>
>> +
>>
>> +[Pcd]
>>
>> + gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay ## CONSUMES
>>
>> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc
>> b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> index 9408138d0a09..530b42796a0d 100644
>> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
>> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
>> @@ -508,6 +508,12 @@ [PcdsDynamicHii.common.DEFAULT]
>>
>> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFor
>> mSetGuid|0x0|0
>>
>>
>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormS
>> etGuid|0x0|0
>>
>>
>>
>> + #
>>
>> + # Reset-related.
>>
>> + #
>>
>> +
>>
>> +
>> gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|L"ResetDelay"|gRasp
>> berryPiTokenSpaceGuid|0x0|0
>>
>> +
>>
>> #
>>
>> # Common UEFI ones.
>>
>> #
>>
>> diff --git a/Platform/RaspberryPi/RPi4/RPi4.dsc
>> b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> index ddf4dd6a416e..5f8452aa0b76 100644
>> --- a/Platform/RaspberryPi/RPi4/RPi4.dsc
>> +++ b/Platform/RaspberryPi/RPi4/RPi4.dsc
>> @@ -522,6 +522,12 @@ [PcdsDynamicHii.common.DEFAULT]
>>
>> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|L"FanOnGpio"|gConfigDxeFor
>> mSetGuid|0x0|0
>>
>>
>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|L"FanTemp"|gConfigDxeFormS
>> etGuid|0x0|60
>>
>>
>>
>> + #
>>
>> + # Reset-related.
>>
>> + #
>>
>> +
>>
>> +
>> gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|L"ResetDelay"|gRasp
>> berryPiTokenSpaceGuid|0x0|0
>>
>> +
>>
>> #
>>
>> # Common UEFI ones.
>>
>> #
>>
>> diff --git a/Platform/RaspberryPi/RaspberryPi.dec
>> b/Platform/RaspberryPi/RaspberryPi.dec
>> index c64c61930ea8..10723036aa31 100644
>> --- a/Platform/RaspberryPi/RaspberryPi.dec
>> +++ b/Platform/RaspberryPi/RaspberryPi.dec
>> @@ -68,3 +68,4 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
>> PcdsDynamic, PcdsDynamicEx]
>> gRaspberryPiTokenSpaceGuid.PcdRamLimitTo3GB|0|UINT32|0x0000001A
>>
>> gRaspberryPiTokenSpaceGuid.PcdFanOnGpio|0|UINT32|0x0000001C
>>
>> gRaspberryPiTokenSpaceGuid.PcdFanTemp|0|UINT32|0x0000001D
>>
>> +
>> gRaspberryPiTokenSpaceGuid.PcdPlatformResetDelay|0|UINT32|0x0000001
>> E
>>
>> --
>> 2.29.2.windows.2
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69600): https://edk2.groups.io/g/devel/message/69600
Mute This Topic: https://groups.io/mt/79148769/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list