Re: [PATCH edk2-platforms 17/41] Silicon/NXP : Add NOR driver.

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, Nov 28, 2018 at 08:31:31PM +0530, Meenakshi Aggarwal wrote:
> Add NOR DXE phase driver, it installs BlockIO and Fvp
> protocol.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> ---
>  Platform/NXP/LS1043aRdbPkg/VarStore.fdf.inc        |  98 +++
>  .../NXP/Drivers/NorFlashDxe/NorFlashBlockIoDxe.c   | 252 +++++++
>  Silicon/NXP/Drivers/NorFlashDxe/NorFlashDxe.c      | 503 +++++++++++++
>  Silicon/NXP/Drivers/NorFlashDxe/NorFlashDxe.h      | 146 ++++
>  Silicon/NXP/Drivers/NorFlashDxe/NorFlashDxe.inf    |  65 ++
>  Silicon/NXP/Drivers/NorFlashDxe/NorFlashFvbDxe.c   | 816 +++++++++++++++++++++
>  6 files changed, 1880 insertions(+)
>  create mode 100644 Platform/NXP/LS1043aRdbPkg/VarStore.fdf.inc
>  create mode 100644 Silicon/NXP/Drivers/NorFlashDxe/NorFlashBlockIoDxe.c
>  create mode 100644 Silicon/NXP/Drivers/NorFlashDxe/NorFlashDxe.c
>  create mode 100644 Silicon/NXP/Drivers/NorFlashDxe/NorFlashDxe.h
>  create mode 100644 Silicon/NXP/Drivers/NorFlashDxe/NorFlashDxe.inf
>  create mode 100644 Silicon/NXP/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> 
> diff --git a/Platform/NXP/LS1043aRdbPkg/VarStore.fdf.inc b/Platform/NXP/LS1043aRdbPkg/VarStore.fdf.inc
> new file mode 100644
> index 0000000..e254337
> --- /dev/null
> +++ b/Platform/NXP/LS1043aRdbPkg/VarStore.fdf.inc
> @@ -0,0 +1,98 @@
> +## @file
> +#  FDF include file with FD definition that defines an empty variable store.
> +#
> +#  Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.
> +#  Copyright (C) 2014, Red Hat, Inc.
> +#  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
> +#  Copyright (c) 2016, Freescale Semiconductor. All rights reserved.
> +#  Copyright 2017 NXP
> +#
> +#  This program and the accompanying materials are licensed and made available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> +#  IMPLIED.
> +#
> +##
> +
> +[FD.LS1043aRdbNv_EFI]
> +BaseAddress   = 0x60300000  #The base address of the FLASH device
> +Size          = 0x000C0000  #The size in bytes of the FLASH device
> +ErasePolarity = 1
> +BlockSize     = 0x1
> +NumBlocks     = 0xC0000
> +
> +#
> +# Place NV Storage just above Platform Data Base
> +#
> +DEFINE NVRAM_AREA_VARIABLE_BASE                = 0x00000000
> +DEFINE NVRAM_AREA_VARIABLE_SIZE                = 0x00040000
> +DEFINE FTW_WORKING_BASE                        = $(NVRAM_AREA_VARIABLE_BASE) + $(NVRAM_AREA_VARIABLE_SIZE)
> +DEFINE FTW_WORKING_SIZE                        = 0x00040000
> +DEFINE FTW_SPARE_BASE                          = $(FTW_WORKING_BASE) + $(FTW_WORKING_SIZE)
> +DEFINE FTW_SPARE_SIZE                          = 0x00040000
> +
> +#############################################################################
> +# LS1043ARDB NVRAM Area
> +# LS1043ARDB NVRAM Area contains: Variable + FTW Working + FTW Spare
> +#############################################################################
> +
> +
> +$(NVRAM_AREA_VARIABLE_BASE)|$(NVRAM_AREA_VARIABLE_SIZE)
> +gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> +#NV_VARIABLE_STORE
> +DATA = {
> +  ## This is the EFI_FIRMWARE_VOLUME_HEADER
> +  # ZeroVector []
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  # FileSystemGuid: gEfiSystemNvDataFvGuid         =
> +  #   { 0xFFF12B8D, 0x7696, 0x4C8B,
> +  #     { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }}
> +  0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,
> +  0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,
> +  # FvLength: 0xC0000
> +  0x00, 0x00, 0x0C, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  # Signature "_FVH"       # Attributes
> +  0x5f, 0x46, 0x56, 0x48, 0x36, 0x0E, 0x00, 0x00,
> +  # HeaderLength # CheckSum # ExtHeaderOffset #Reserved #Revision
> +  0x48, 0x00, 0xC2, 0xF9, 0x00, 0x00, 0x00, 0x02,
> +  # Blockmap[0]: 0x3 Blocks * 0x40000 Bytes / Block
> +  0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00,
> +  # Blockmap[1]: End
> +  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +  ## This is the VARIABLE_STORE_HEADER
> +  # It is compatible with SECURE_BOOT_ENABLE == FALSE as well.
> +  # Signature: gEfiAuthenticatedVariableGuid =
> +  #   { 0xaaf32c78, 0x947b, 0x439a,
> +  #     { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
> +  0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
> +  0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
> +  # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
> +  #         0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3ffb8
> +  # This can speed up the Variable Dispatch a bit.
> +  0xB8, 0xFF, 0x03, 0x00,
> +  # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32
> +  0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +}
> +
> +$(FTW_WORKING_BASE)|$(FTW_WORKING_SIZE)
> +gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> +#NV_FTW_WORKING
> +DATA = {
> +  # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid         =
> +  #  { 0x9e58292b, 0x7c68, 0x497d, { 0xa0, 0xce, 0x65,  0x0, 0xfd, 0x9f, 0x1b, 0x95 }}
> +  0x2b, 0x29, 0x58, 0x9e, 0x68, 0x7c, 0x7d, 0x49,
> +  0xa0, 0xce, 0x65,  0x0, 0xfd, 0x9f, 0x1b, 0x95,
> +  # Crc:UINT32            #WorkingBlockValid:1, WorkingBlockInvalid:1, Reserved
> +  0x5b, 0xe7, 0xc6, 0x86, 0xFE, 0xFF, 0xFF, 0xFF,
> +  # WriteQueueSize: UINT64
> +  0xE0, 0xFF, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00
> +}
> +
> +$(FTW_SPARE_BASE)|$(FTW_SPARE_SIZE)
> +gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64|gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> +#NV_FTW_SPARE
> diff --git a/Silicon/NXP/Drivers/NorFlashDxe/NorFlashBlockIoDxe.c b/Silicon/NXP/Drivers/NorFlashDxe/NorFlashBlockIoDxe.c
> new file mode 100644
> index 0000000..bc49fdc
> --- /dev/null
> +++ b/Silicon/NXP/Drivers/NorFlashDxe/NorFlashBlockIoDxe.c
> @@ -0,0 +1,252 @@
> +/** @NorFlashBlockIoDxe.c
> +
> +  Based on NorFlash implementation available in
> +  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashBlockIoDxe.c
> +
> +  Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.
> +  Copyright 2017 NXP
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Library/DebugLib.h>
> +#include <Library/NorFlashLib.h>
> +
> +#include <NorFlash.h>
> +#include "NorFlashDxe.h"
> +
> +//
> +// BlockIO Protocol function EFI_BLOCK_IO_PROTOCOL.Reset
> +//
> +EFI_STATUS
> +EFIAPI
> +NorFlashBlockIoReset (
> +  IN EFI_BLOCK_IO_PROTOCOL  *This,
> +  IN BOOLEAN                ExtendedVerification
> +  )
> +{
> +  NOR_FLASH_INSTANCE        *Instance;
> +
> +  Instance = INSTANCE_FROM_BLKIO_THIS (This);
> +
> +  DEBUG ((DEBUG_INFO, "NorFlashBlockIoReset (MediaId=0x%x)\n",
> +    This->Media->MediaId));
> +
> +  return NorFlashPlatformReset (Instance->DeviceBaseAddress);
> +}
> +
> +//
> +// BlockIO Protocol function EFI_BLOCK_IO_PROTOCOL.ReadBlocks
> +//
> +EFI_STATUS
> +EFIAPI
> +NorFlashBlockIoReadBlocks (
> +  IN  EFI_BLOCK_IO_PROTOCOL   *This,
> +  IN  UINT32                  MediaId,
> +  IN  EFI_LBA                 Lba,
> +  IN  UINTN                   BufferSizeInBytes,
> +  OUT VOID                    *Buffer
> +  )
> +{
> +  NOR_FLASH_INSTANCE          *Instance;
> +  EFI_STATUS                  Status;
> +  EFI_BLOCK_IO_MEDIA          *Media;
> +  UINTN                       NumBlocks;
> +  UINT8                       *ReadBuffer;
> +  UINTN                       BlockCount;
> +  UINTN                       BlockSizeInBytes;
> +  EFI_LBA                     CurrentBlock;
> +
> +  Status = EFI_SUCCESS;
> +
> +  if ((This == NULL) || (Buffer == NULL)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Instance = INSTANCE_FROM_BLKIO_THIS (This);
> +  Media = This->Media;
> +
> +  if (Media  == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a : Media is NULL\n", __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  NumBlocks = BufferSizeInBytes / Instance->Media.BlockSize ;
> +
> +  DEBUG ((DEBUG_BLKIO,
> +    "%a : (MediaId=0x%x, Lba=%ld, BufferSize=0x%x bytes (%d kB), BufferPtr @ 0x%p)\n",
> +    __FUNCTION__, MediaId, Lba, BufferSizeInBytes, Buffer));
> +
> +  if (!Media->MediaPresent) {
> +    return EFI_NO_MEDIA;
> +  }
> +  if (Media->MediaId != MediaId) {
> +    return EFI_MEDIA_CHANGED;
> +  }
> +  if ((Media->IoAlign >= 2) && (((UINTN)Buffer & (Media->IoAlign - 1)) != 0)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  if (BufferSizeInBytes == 0) {
> +    // Return if we have not any byte to read
> +    return EFI_SUCCESS;
> +  }
> +  if ((BufferSizeInBytes % Media->BlockSize) != 0) {
> +    // The size of the buffer must be a multiple of the block size
> +    DEBUG ((DEBUG_ERROR, "%a : BlockSize in bytes = 0x%x\n", __FUNCTION__, BufferSizeInBytes));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  if ((Lba + NumBlocks - 1) > Media->LastBlock) {
> +    // All blocks must be within the device
> +    DEBUG ((DEBUG_ERROR, "%a : Read will exceed last block %d, %d, %d \n",
> +      __FUNCTION__, Lba, NumBlocks, Media->LastBlock));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  BlockSizeInBytes = Instance->Media.BlockSize;
> +
> +  /* Because the target *Buffer is a pointer to VOID,
> +   * we must put all the data into a pointer
> +   * to a proper data type, so use *ReadBuffer */
> +  ReadBuffer = (UINT8 *)Buffer;
> +
> +  CurrentBlock = Lba;
> +  // Read data block by Block
> +  for (BlockCount = 0; BlockCount < NumBlocks; BlockCount++) {
> +    DEBUG ((DEBUG_BLKIO, "%a: Reading block #%d\n", __FUNCTION__, (UINTN)CurrentBlock));
> +
> +    Status = NorFlashPlatformRead (Instance, CurrentBlock, (UINTN)0 ,
> +               BlockSizeInBytes, ReadBuffer);
> +    if (EFI_ERROR (Status)) {
> +      break;
> +    }
> +
> +    CurrentBlock++;
> +    ReadBuffer = ReadBuffer + BlockSizeInBytes;
> +  }
> +
> +  return Status;
> +}
> +
> +//
> +// BlockIO Protocol function EFI_BLOCK_IO_PROTOCOL.WriteBlocks
> +//
> +EFI_STATUS
> +EFIAPI
> +NorFlashBlockIoWriteBlocks (
> +  IN  EFI_BLOCK_IO_PROTOCOL   *This,
> +  IN  UINT32                  MediaId,
> +  IN  EFI_LBA                 Lba,
> +  IN  UINTN                   BufferSizeInBytes,
> +  IN  VOID                    *Buffer
> +  )
> +{
> +  NOR_FLASH_INSTANCE          *Instance;
> +  EFI_STATUS                   Status;
> +  EFI_BLOCK_IO_MEDIA           *Media;
> +  UINTN                        NumBlocks;
> +  EFI_LBA                      CurrentBlock;
> +  UINTN                        BlockSizeInBytes;
> +  UINT32                       BlockCount;
> +  UINTN                        SectorAddress;
> +  UINT8                        *WriteBuffer;
> +
> +  Status = EFI_SUCCESS;
> +
> +  if ((This == NULL) || (Buffer == NULL)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Instance = INSTANCE_FROM_BLKIO_THIS (This);
> +  Media = This->Media;
> +
> +  if (Media  == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a : Media is NULL\n",  __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  NumBlocks = BufferSizeInBytes / Instance->Media.BlockSize ;
> +
> +  DEBUG ((DEBUG_BLKIO,
> +    "%a : (MediaId=0x%x, Lba=%ld, BufferSize=0x%x bytes (%d kB) BufferPtr @ 0x%08x)\n",
> +    __FUNCTION__, MediaId, Lba, BufferSizeInBytes, Buffer));
> +
> +  if (!Media->MediaPresent) {
> +    return EFI_NO_MEDIA;
> +  }
> +  if (Media->MediaId != MediaId) {
> +    return EFI_MEDIA_CHANGED;
> +  }
> +  if (Media->ReadOnly) {
> +    return EFI_WRITE_PROTECTED;
> +  }
> +  if (BufferSizeInBytes == 0) {
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +  if ((BufferSizeInBytes % Media->BlockSize) != 0) {
> +    // The size of the buffer must be a multiple of the block size
> +    DEBUG ((DEBUG_ERROR, "%a : BlockSize in bytes = 0x%x\n",__FUNCTION__, BufferSizeInBytes));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  if ((Lba + NumBlocks - 1) > Media->LastBlock) {
> +    // All blocks must be within the device
> +    DEBUG ((DEBUG_ERROR, "%a: Write will exceed last block %d, %d, %d  \n",
> +      __FUNCTION__,Lba, NumBlocks, Media->LastBlock));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  BlockSizeInBytes = Instance->Media.BlockSize;
> +
> +  WriteBuffer = (UINT8 *)Buffer;
> +
> +  CurrentBlock = Lba;
> +  // Program data block by Block
> +  for (BlockCount = 0; BlockCount < NumBlocks; BlockCount++) {
> +    DEBUG ((DEBUG_BLKIO, "%a: Writing block #%d\n", __FUNCTION__, (UINTN)CurrentBlock));
> +    // Erase the Block(Sector) to be written to
> +    SectorAddress = GET_NOR_BLOCK_ADDRESS (
> +                      Instance->RegionBaseAddress,
> +                      CurrentBlock,
> +                      Instance->Media.BlockSize
> +                      );
> +
> +    Status = NorFlashPlatformEraseSector (Instance, (UINTN)SectorAddress);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Failed to erase Target 0x%x (0x%x) \n",
> +        __FUNCTION__,SectorAddress, Status));
> +      break;
> +    }
> +
> +    // Program Block(Sector) to be written to
> +    Status = NorFlashWrite (Instance, CurrentBlock, (UINTN)0, &BlockSizeInBytes, WriteBuffer);
> +    if (EFI_ERROR (Status)) {
> +      break;
> +    }
> +
> +    CurrentBlock++;
> +    WriteBuffer = WriteBuffer + BlockSizeInBytes;
> +  }
> +
> +  return Status;
> +}
> +
> +//
> +// BlockIO Protocol function EFI_BLOCK_IO_PROTOCOL.FlushBlocks
> +//
> +EFI_STATUS
> +EFIAPI
> +NorFlashBlockIoFlushBlocks (
> +  IN EFI_BLOCK_IO_PROTOCOL  *This
> +  )
> +{
> +  DEBUG ((DEBUG_BLKIO, "%a NOT IMPLEMENTED (not required)\n", __FUNCTION__));
> +
> +  // Nothing to do so just return without error
> +  return EFI_SUCCESS;
> +}
> diff --git a/Silicon/NXP/Drivers/NorFlashDxe/NorFlashDxe.c b/Silicon/NXP/Drivers/NorFlashDxe/NorFlashDxe.c
> new file mode 100644
> index 0000000..ab94662
> --- /dev/null
> +++ b/Silicon/NXP/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -0,0 +1,503 @@
> +/** @file
> +
> +  Based on NorFlash implementation available in
> +  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +
> +  Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.
> +  Copyright 2017 NXP
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/NorFlashLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeLib.h>
> +
> +#include "NorFlashDxe.h"
> +
> +STATIC EFI_EVENT mNorFlashVirtualAddrChangeEvent;
> +
> +//
> +// Global variable declarations
> +//
> +NOR_FLASH_INSTANCE **mNorFlashInstances;
> +UINT32               mNorFlashDeviceCount;

Can the two above be STATIC?

Other than that, the only comment I have on the remainder of this
patch is that there are some _very_ long lines here, across multiple
files.

Some, like the "| // ...." comments are probably more helpful to keep
like that.
But for the actual functional bits of code, please try to stay below
(or near) 80 characters per line.

/
    Leif