From: "Ard Biesheuvel" <ard.biesheuvel@linaro.org>
To: Marcin Wojtas <mw@semihalf.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
"Leif Lindholm" <leif.lindholm@linaro.org>,
"Nadav Haklai" <nadavh@marvell.com>,
"Jan Dąbroś" <jsd@semihalf.com>,
"Grzegorz Jaszczyk" <jaz@semihalf.com>,
"Kostya Porotchkin" <kostap@marvell.com>,
"Jici Gao" <Jici.Gao@arm.com>,
"Kornel Duleba" <mindal@semihalf.com>
Subject: Re: [edk2-platforms: PATCH 3/3] Marvell/Drivers: Add non-mmio mode to MvFvbDxe
Date: Sat, 20 Apr 2019 19:45:47 +0200 [thread overview]
Message-ID: <CAKv+Gu-bfQ9UyEAuO-m6NDmVmKGKdCnrJz-ac2qZC63boc+R2g@mail.gmail.com> (raw)
In-Reply-To: <1555524356-1740-4-git-send-email-mw@semihalf.com>
On Wed, 17 Apr 2019 at 20:06, Marcin Wojtas <mw@semihalf.com> wrote:
>
> From: Kornel Duleba <mindal@semihalf.com>
>
> This path enables support for reading variables directly from flash without
> relying on it to be memory mapped. It adds PcdSpiMemoryMapped PCD that
> allows to switch between the modes. When in non-memory-mapped mode the
> driver will copy the variables from flash to previously allocated buffer
> and set PcdFlashNvStorageVariableBase64, PcdFlashNvStorageFtwWorkingBase64
> and PcdFlashNvStorageFtwSpareBase64 accordingly.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
How does this work with VariablePei?
> ---
> Silicon/Marvell/Marvell.dec | 8 ++
> Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc | 10 +-
> Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf | 17 ++-
> Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h | 1 +
> Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c | 132 +++++++++++++++-----
> 5 files changed, 132 insertions(+), 36 deletions(-)
>
> diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
> index 201a62f..70929d2 100644
> --- a/Silicon/Marvell/Marvell.dec
> +++ b/Silicon/Marvell/Marvell.dec
> @@ -58,6 +58,12 @@
>
> gMarvellFvbDxeGuid = { 0x42903750, 0x7e61, 0x4aaf, { 0x83, 0x29, 0xbf, 0x42, 0x36, 0x4e, 0x24, 0x85 } }
> gMarvellSpiFlashDxeGuid = { 0x49d7fb74, 0x306d, 0x42bd, { 0x94, 0xc8, 0xc0, 0xc5, 0x4b, 0x18, 0x1d, 0xd7 } }
> + #
> + # Generic FaultTolerantWriteDxe driver use variables,
> + # whose setting is done in MvFvbDxe driver in case
> + # the SPI contents are not mapped in memory.
> + #
> + gFaultTolerantWriteDxeFileGuid = { 0xfe5cea76, 0x4f72, 0x49e8, { 0x98, 0x6f, 0x2c, 0xd8, 0x99, 0xdf, 0xfe, 0x5d} }
>
> [LibraryClasses]
> ArmadaBoardDescLib|Include/Library/ArmadaBoardDescLib.h
> @@ -143,6 +149,8 @@
> gMarvellTokenSpaceGuid.PcdSpiClockRegBase|0|UINT64|0x30000056
> gMarvellTokenSpaceGuid.PcdSpiRegBase|0|UINT32|0x3000051
> gMarvellTokenSpaceGuid.PcdSpiMemoryBase|0|UINT64|0x3000059
> + gMarvellTokenSpaceGuid.PcdSpiMemoryMapped|TRUE|BOOLEAN|0x3000060
> + gMarvellTokenSpaceGuid.PcdSpiVariableOffset|0|UINT32|0x3000061
> gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|0|UINT32|0x30000052
> gMarvellTokenSpaceGuid.PcdSpiClockFrequency|0|UINT32|0x30000053
>
> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> index a1ebb81..9974d5f 100644
> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> @@ -256,6 +256,11 @@
> # USB support
> gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
>
> +[PcdsDynamicDefault.common]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0xF93C0000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0xF93E0000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0xF93D0000
> +
> [PcdsFixedAtBuild.common]
> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"MARVELL_EFI"
> gArmPlatformTokenSpaceGuid.PcdCoreCount|4
> @@ -396,11 +401,10 @@
> # Variable store - default values
> #
> gMarvellTokenSpaceGuid.PcdSpiMemoryBase|0xF9000000
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0xF93C0000
> + gMarvellTokenSpaceGuid.PcdSpiMemoryMapped|TRUE
> + gMarvellTokenSpaceGuid.PcdSpiVariableOffset|0x3C0000
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize|0x00010000
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|0xF93D0000
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize|0x00010000
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|0xF93E0000
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize|0x00010000
>
> !if $(CAPSULE_ENABLE)
> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
> index ef10bfd..c85e8a6 100644
> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.inf
> @@ -76,13 +76,22 @@
> gMarvellSpiMasterProtocolGuid
>
> [FixedPcd]
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> gMarvellTokenSpaceGuid.PcdSpiMemoryBase
> + gMarvellTokenSpaceGuid.PcdSpiMemoryMapped
> + gMarvellTokenSpaceGuid.PcdSpiVariableOffset
> +
> +[Pcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
>
> [Depex]
> - gEfiCpuArchProtocolGuid
> + #
> + # Generic FaultTolerantWriteDxe driver use variables,
> + # whose setting is done in MvFvbDxe driver in case
> + # the SPI contents are not mapped in memory.
> + #
> + BEFORE gFaultTolerantWriteDxeFileGuid
> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h
> index 31e6e44..e8df9a5 100644
> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h
> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.h
> @@ -55,6 +55,7 @@ typedef struct {
>
> UINT32 Signature;
>
> + BOOLEAN IsMemoryMapped;
> UINTN DeviceBaseAddress;
> UINTN RegionBaseAddress;
> UINTN Size;
> diff --git a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
> index b57f415..b1dfd62 100644
> --- a/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
> +++ b/Silicon/Marvell/Drivers/Spi/MvFvbDxe/MvFvbDxe.c
> @@ -52,6 +52,7 @@ STATIC CONST FVB_DEVICE mMvFvbFlashInstanceTemplate = {
>
> FVB_FLASH_SIGNATURE, // Signature
>
> + FALSE, // IsMemoryMapped ... NEED TO BE FILLED
> 0, // DeviceBaseAddress ... NEED TO BE FILLED
> 0, // RegionBaseAddress ... NEED TO BE FILLED
> SIZE_256KB, // Size
> @@ -175,11 +176,14 @@ MvFvbInitFvAndVariableStoreHeaders (
> FirmwareVolumeHeader->Attributes = EFI_FVB2_READ_ENABLED_CAP |
> EFI_FVB2_READ_STATUS |
> EFI_FVB2_STICKY_WRITE |
> - EFI_FVB2_MEMORY_MAPPED |
> EFI_FVB2_ERASE_POLARITY |
> EFI_FVB2_WRITE_STATUS |
> EFI_FVB2_WRITE_ENABLED_CAP;
>
> + if (FlashInstance->IsMemoryMapped) {
> + FirmwareVolumeHeader->Attributes |= EFI_FVB2_MEMORY_MAPPED;
> + }
> +
> FirmwareVolumeHeader->HeaderLength = sizeof (EFI_FIRMWARE_VOLUME_HEADER) +
> sizeof (EFI_FV_BLOCK_MAP_ENTRY);
> FirmwareVolumeHeader->Revision = EFI_FVH_REVISION;
> @@ -382,12 +386,15 @@ MvFvbSetAttributes (
> EFI_FVB2_WRITE_ENABLED_CAP | \
> EFI_FVB2_LOCK_CAP | \
> EFI_FVB2_STICKY_WRITE | \
> - EFI_FVB2_MEMORY_MAPPED | \
> EFI_FVB2_ERASE_POLARITY | \
> EFI_FVB2_READ_LOCK_CAP | \
> EFI_FVB2_WRITE_LOCK_CAP | \
> EFI_FVB2_ALIGNMENT;
>
> + if (FlashInstance->IsMemoryMapped) {
> + UnchangedAttributes |= EFI_FVB2_MEMORY_MAPPED;
> + }
> +
> //
> // Some attributes of FV is read only can *not* be set
> //
> @@ -714,6 +721,7 @@ MvFvbWrite (
> IN UINT8 *Buffer
> )
> {
> + EFI_STATUS Status;
> FVB_DEVICE *FlashInstance;
> UINTN DataOffset;
>
> @@ -728,10 +736,27 @@ MvFvbWrite (
> FlashInstance->StartLba + Lba,
> FlashInstance->Media.BlockSize);
>
> - return FlashInstance->SpiFlashProtocol->Write (&FlashInstance->SpiDevice,
> - DataOffset,
> - *NumBytes,
> - Buffer);
> + Status = FlashInstance->SpiFlashProtocol->Write (&FlashInstance->SpiDevice,
> + DataOffset,
> + *NumBytes,
> + Buffer);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR,
> + "%a: Failed to write to Spi device\n",
> + __FUNCTION__));
> + return Status;
> + }
> +
> + // Update shadow buffer
> + if (!FlashInstance->IsMemoryMapped) {
> + DataOffset = GET_DATA_OFFSET (FlashInstance->RegionBaseAddress + Offset,
> + FlashInstance->StartLba + Lba,
> + FlashInstance->Media.BlockSize);
> +
> + CopyMem ((UINTN *)DataOffset, Buffer, *NumBytes);
> + }
> +
> + return EFI_SUCCESS;
> }
>
> /**
> @@ -1009,6 +1034,9 @@ MvFvbConfigureFlashInstance (
> )
> {
> EFI_STATUS Status;
> + UINTN *NumBytes;
> + UINTN DataOffset;
> + UINTN VariableSize, FtwWorkingSize, FtwSpareSize, MemorySize;
>
>
> // Locate SPI protocols
> @@ -1043,25 +1071,62 @@ MvFvbConfigureFlashInstance (
> }
>
> // Fill remaining flash description
> - FlashInstance->DeviceBaseAddress = PcdGet64 (PcdSpiMemoryBase);
> - FlashInstance->RegionBaseAddress = FixedPcdGet64 (PcdFlashNvStorageVariableBase64);
> - FlashInstance->FvbOffset = FlashInstance->RegionBaseAddress -
> - FlashInstance->DeviceBaseAddress;
> - FlashInstance->FvbSize = PcdGet32(PcdFlashNvStorageVariableSize) +
> - PcdGet32(PcdFlashNvStorageFtwWorkingSize) +
> - PcdGet32(PcdFlashNvStorageFtwSpareSize);
> + VariableSize = PcdGet32 (PcdFlashNvStorageVariableSize);
> + FtwWorkingSize = PcdGet32 (PcdFlashNvStorageFtwWorkingSize);
> + FtwSpareSize = PcdGet32 (PcdFlashNvStorageFtwSpareSize);
> +
> + FlashInstance->IsMemoryMapped = PcdGetBool (PcdSpiMemoryMapped);
> + FlashInstance->FvbSize = VariableSize + FtwWorkingSize + FtwSpareSize;
> + FlashInstance->FvbOffset = PcdGet32 (PcdSpiVariableOffset);
>
> FlashInstance->Media.MediaId = 0;
> FlashInstance->Media.BlockSize = FlashInstance->SpiDevice.Info->SectorSize;
> FlashInstance->Media.LastBlock = FlashInstance->Size /
> FlashInstance->Media.BlockSize - 1;
>
> + if (FlashInstance->IsMemoryMapped) {
> + FlashInstance->DeviceBaseAddress = PcdGet64 (PcdSpiMemoryBase);
> + FlashInstance->RegionBaseAddress = PcdGet64 (PcdFlashNvStorageVariableBase64);
> + } else {
> + MemorySize = EFI_SIZE_TO_PAGES (FlashInstance->FvbSize);
> +
> + // FaultTolerantWriteDxe requires memory to be aligned to FtwWorkingSize
> + FlashInstance->RegionBaseAddress = (UINTN) AllocateAlignedRuntimePages (MemorySize,
> + SIZE_64KB);
> + if (FlashInstance->RegionBaseAddress == (UINTN) NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + PcdSet64 (PcdFlashNvStorageVariableBase64,
> + (UINT64) FlashInstance->RegionBaseAddress);
> + PcdSet64 (PcdFlashNvStorageFtwWorkingBase64,
> + (UINT64) FlashInstance->RegionBaseAddress
> + + VariableSize);
> + PcdSet64 (PcdFlashNvStorageFtwSpareBase64,
> + (UINT64) FlashInstance->RegionBaseAddress
> + + VariableSize
> + + FtwWorkingSize);
> +
> + // Fill the buffer with data from flash
> + DataOffset = GET_DATA_OFFSET (FlashInstance->FvbOffset,
> + FlashInstance->StartLba,
> + FlashInstance->Media.BlockSize);
> + *NumBytes = FlashInstance->FvbSize;
> + Status = FlashInstance->SpiFlashProtocol->Read (&FlashInstance->SpiDevice,
> + DataOffset,
> + *NumBytes,
> + (VOID *)FlashInstance->RegionBaseAddress);
> + if (EFI_ERROR (Status)) {
> + goto ErrorFreeAllocatedPages;
> + }
> + }
> +
> Status = gBS->InstallMultipleProtocolInterfaces (&FlashInstance->Handle,
> &gEfiDevicePathProtocolGuid, &FlashInstance->DevicePath,
> &gEfiFirmwareVolumeBlockProtocolGuid, &FlashInstance->FvbProtocol,
> NULL);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto ErrorFreeAllocatedPages;
> }
>
> Status = MvFvbPrepareFvHeader (FlashInstance);
> @@ -1077,6 +1142,12 @@ ErrorPrepareFvbHeader:
> &gEfiFirmwareVolumeBlockProtocolGuid,
> NULL);
>
> +ErrorFreeAllocatedPages:
> + if (!FlashInstance->IsMemoryMapped) {
> + FreeAlignedPages ((VOID *)FlashInstance->RegionBaseAddress,
> + MemorySize);
> + }
> +
> return Status;
> }
>
> @@ -1128,24 +1199,27 @@ MvFvbEntryPoint (
> //
> // Declare the Non-Volatile storage as EFI_MEMORY_RUNTIME
> //
> - RuntimeMmioRegionSize = mFvbDevice->FvbSize;
> RegionBaseAddress = mFvbDevice->RegionBaseAddress;
>
> - Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
> - RegionBaseAddress,
> - RuntimeMmioRegionSize,
> - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> - if (EFI_ERROR (Status)) {
> - DEBUG ((DEBUG_ERROR, "%a: Failed to add memory space\n", __FUNCTION__));
> - goto ErrorAddSpace;
> - }
> + if (mFvbDevice->IsMemoryMapped) {
> + RuntimeMmioRegionSize = mFvbDevice->FvbSize;
> + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeMemoryMappedIo,
> + RegionBaseAddress,
> + RuntimeMmioRegionSize,
> + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: Failed to add memory space\n", __FUNCTION__));
> + goto ErrorAddSpace;
> + }
>
> - Status = gDS->SetMemorySpaceAttributes (RegionBaseAddress,
> - RuntimeMmioRegionSize,
> - EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> - if (EFI_ERROR (Status)) {
> - DEBUG ((DEBUG_ERROR, "%a: Failed to set memory attributes\n", __FUNCTION__));
> - goto ErrorSetMemAttr;
> +
> + Status = gDS->SetMemorySpaceAttributes (RegionBaseAddress,
> + RuntimeMmioRegionSize,
> + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: Failed to set memory attributes\n", __FUNCTION__));
> + goto ErrorSetMemAttr;
> + }
> }
>
> //
> --
> 2.7.4
>
prev parent reply other threads:[~2019-04-20 17:46 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-17 18:05 [edk2-platforms: PATCH 0/3] Armada7k8k FVB improvements Marcin Wojtas
2019-04-17 18:05 ` [edk2-platforms: PATCH 1/3] Marvell/Drivers: MvFvbDxe: Change Pcd parameters to be 64 bit Marcin Wojtas
2019-04-17 18:05 ` [edk2-platforms: PATCH 2/3] Marvell/Drivers: MvSpiOrionDxe: Keep clock enabled at runtime Marcin Wojtas
2019-04-17 18:27 ` Ard Biesheuvel
2019-04-17 18:30 ` Ard Biesheuvel
2019-04-17 19:33 ` Marcin Wojtas
2019-04-17 18:05 ` [edk2-platforms: PATCH 3/3] Marvell/Drivers: Add non-mmio mode to MvFvbDxe Marcin Wojtas
2019-04-20 17:45 ` Ard Biesheuvel [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu-bfQ9UyEAuO-m6NDmVmKGKdCnrJz-ac2qZC63boc+R2g@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox