Re: [PATCH] ArmPlatformPkg/NorFlashDxe: Change Flash memory attributes before writes

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 20 January 2017 at 11:54,  <achin.gupta@arm.com> wrote:
> From: Achin Gupta <achin.gupta@arm.com>
>
> In NorFlashFvbInitialize() if a valid Firmware Volume header is not found at the
> start of NOR Flash, the Flash memory is written before it has been remapped with
> EFI_MEMORY_UC attributes to allow write commands. Since the flash memory was
> previously mapped with Normal and possibly cacheable memory attributes, the
> Flash commands might never reach the device.
>
> This patch fixes this issue by remapping the Flash memory region with correct
> memory attributes before writing to it.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Achin Gupta <achin.gupta@arm.com>

Thanks Achin!

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Pushed as 90d1f671cdad


> ---
>  .../Drivers/NorFlashDxe/NorFlashFvbDxe.c           | 45 +++++++++++-----------
>  1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> index 42be5c2..12a8612 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> @@ -719,6 +719,29 @@ NorFlashFvbInitialize (
>    UINTN       RuntimeMmioRegionSize;
>
>    DEBUG((DEBUG_BLKIO,"NorFlashFvbInitialize\n"));
> +  ASSERT((Instance != NULL));
> +
> +  //
> +  // Declare the Non-Volatile storage as EFI_MEMORY_RUNTIME
> +  //
> +
> +  // Note: all the NOR Flash region needs to be reserved into the UEFI Runtime memory;
> +  //       even if we only use the small block region at the top of the NOR Flash.
> +  //       The reason is when the NOR Flash memory is set into program mode, the command
> +  //       is written as the base of the flash region (ie: Instance->DeviceBaseAddress)
> +  RuntimeMmioRegionSize = (Instance->RegionBaseAddress - Instance->DeviceBaseAddress) + Instance->Size;
> +
> +  Status = gDS->AddMemorySpace (
> +      EfiGcdMemoryTypeMemoryMappedIo,
> +      Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
> +      EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> +      );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Status = gDS->SetMemorySpaceAttributes (
> +      Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
> +      EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> +  ASSERT_EFI_ERROR (Status);
>
>    Instance->Initialized = TRUE;
>    mFlashNvStorageVariableBase = FixedPcdGet32 (PcdFlashNvStorageVariableBase);
> @@ -757,28 +780,6 @@ NorFlashFvbInitialize (
>    }
>
>    //
> -  // Declare the Non-Volatile storage as EFI_MEMORY_RUNTIME
> -  //
> -
> -  // Note: all the NOR Flash region needs to be reserved into the UEFI Runtime memory;
> -  //       even if we only use the small block region at the top of the NOR Flash.
> -  //       The reason is when the NOR Flash memory is set into program mode, the command
> -  //       is written as the base of the flash region (ie: Instance->DeviceBaseAddress)
> -  RuntimeMmioRegionSize = (Instance->RegionBaseAddress - Instance->DeviceBaseAddress) + Instance->Size;
> -
> -  Status = gDS->AddMemorySpace (
> -      EfiGcdMemoryTypeMemoryMappedIo,
> -      Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
> -      EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> -      );
> -  ASSERT_EFI_ERROR (Status);
> -
> -  Status = gDS->SetMemorySpaceAttributes (
> -      Instance->DeviceBaseAddress, RuntimeMmioRegionSize,
> -      EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> -  ASSERT_EFI_ERROR (Status);
> -
> -  //
>    // Register for the virtual address change event
>    //
>    Status = gBS->CreateEventEx (
> --
> 1.9.1
>