Re: [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId

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

On Tue, Oct 31, 2017 at 04:59:30AM +0100, Marcin Wojtas wrote:
> Fix the ReadId routine by using master's ReadWrite callback
> instead of the raw Transfer - no longer swapping and byte
> shifting is needed. Simplify code by using local array
> instead of dynamic allocation. Moreover store the FlashId
> in an UINT8 array PCD instead of the concatenated UINT32
> format - this way less overhead in the driver is needed
> for comparing the buffers.
> 
> The new handling allowed for cleaning Fupdate and Sf
> shell commands FlashProbe routines.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 22 +++--------
>  Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c    | 37 ++++++------------
>  Platform/Marvell/Armada/Armada70x0.dsc                 |  2 +-
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c      | 41 ++++++++++++--------
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h      |  2 +
>  Platform/Marvell/Include/Protocol/SpiFlash.h           |  3 ++
>  Platform/Marvell/Marvell.dec                           |  2 +-
>  7 files changed, 48 insertions(+), 61 deletions(-)
> 
> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> index 664411a..d70645d 100644
> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> @@ -94,28 +94,16 @@ SpiFlashProbe (
>    )
>  {
>    EFI_STATUS       Status;
> -  UINT32           IdBuffer, Id, RefId;
> +  UINT8           *FlashId;
>  
> -  Id = PcdGet32 (PcdSpiFlashId);
> -
> -  IdBuffer = CMD_READ_ID & 0xff;
> +  FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
>  
>    // Read SPI flash ID
> -  SpiFlashProtocol->ReadId (Slave, sizeof (UINT32), (UINT8 *)&IdBuffer);
> -
> -  // Swap and extract 3 bytes of the ID
> -  RefId = SwapBytes32 (IdBuffer) >> 8;
> -
> -  if (RefId == 0) {
> -    Print (L"%s: No SPI flash detected");
> -    return EFI_DEVICE_ERROR;
> -  } else if (RefId != Id) {
> -    Print (L"%s: Unsupported SPI flash detected with ID=%2x\n", CMD_NAME_STRING, RefId);
> -    return EFI_DEVICE_ERROR;
> +  Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);

Is the length not possible to calculate somehow?
Having a MAX_LEN defined and then using a DEFAULT_LEN or explicitly
extracting 3 bytes from somewhere feels suboptimal.

/
    Leif

> +  if (EFI_ERROR (Status)) {
> +    return SHELL_ABORTED;
>    }
>  
> -  Print (L"%s: Detected supported SPI flash with ID=%3x\n", CMD_NAME_STRING, RefId);
> -
>    Status = SpiFlashProtocol->Init (SpiFlashProtocol, Slave);
>    if (EFI_ERROR(Status)) {
>      Print (L"%s: Cannot initialize flash device\n", CMD_NAME_STRING);
> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> index 9321f6b..a12f2ec 100644
> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> @@ -166,37 +166,24 @@ FlashProbe (
>    )
>  {
>    EFI_STATUS Status;
> -  UINT8  IdBuffer[4];
> -  UINT32 Id, RefId;
> +  UINT8      *FlashId;
>  
> -  Id = PcdGet32 (PcdSpiFlashId);
> +  FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
>  
> -  IdBuffer[0] = CMD_READ_ID;
> -
> -  SpiFlashProtocol->ReadId (
> -    Slave,
> -    4,
> -    IdBuffer
> -    );
> -
> -  RefId = (IdBuffer[0] << 16) + (IdBuffer[1] << 8) + IdBuffer[2];
> +  Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
> +  if (EFI_ERROR (Status)) {
> +    return SHELL_ABORTED;
> +  }
>  
> -  if (RefId == Id) {
> -    Print (L"sf: Detected supported SPI flash with ID=%3x\n", RefId);
> -    Status = SpiFlashProtocol->Init (SpiFlashProtocol, Slave);
> -    if (EFI_ERROR(Status)) {
> -      Print (L"sf: Cannot initialize flash device\n");
> -      return SHELL_ABORTED;
> -    }
> -    InitFlag = 0;
> -    return EFI_SUCCESS;
> -  } else if (RefId != 0) {
> -    Print (L"sf: Unsupported SPI flash detected with ID=%2x\n", RefId);
> +  Status = SpiFlashProtocol->Init (SpiFlashProtocol, Slave);
> +  if (EFI_ERROR (Status)) {
> +    Print (L"sf: Cannot initialize flash device\n");
>      return SHELL_ABORTED;
>    }
>  
> -  Print (L"sf: No SPI flash detected");
> -  return SHELL_ABORTED;
> +  InitFlag = 0;
> +
> +  return SHELL_SUCCESS;
>  }
>  
>  SHELL_STATUS
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index 0396e8e..4d5f55f 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -94,7 +94,7 @@
>    gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|3
>    gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|65536
>    gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|256
> -  gMarvellTokenSpaceGuid.PcdSpiFlashId|0x20BA18
> +  gMarvellTokenSpaceGuid.PcdSpiFlashId|{ 0x20, 0xBA, 0x18 }
>    gMarvellTokenSpaceGuid.PcdSpiFlashMode|3
>    gMarvellTokenSpaceGuid.PcdSpiFlashCs|0
>  
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> index 6f7d9c7..ab3ed6a 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> @@ -409,28 +409,35 @@ MvSpiFlashReadId (
>    )
>  {
>    EFI_STATUS Status;
> -  UINT8 *DataOut;
> +  UINT8 Id[NOR_FLASH_MAX_ID_LEN];
> +  UINT8 Cmd;
> +
> +  Cmd = CMD_READ_ID;
> +  Status = SpiMasterProtocol->ReadWrite (SpiMasterProtocol,
> +                                SpiDev,
> +                                &Cmd,
> +                                SPI_CMD_LEN,
> +                                NULL,
> +                                Id,
> +                                NOR_FLASH_MAX_ID_LEN);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "ReadId: Spi transfer error\n"));
> +    return Status;
> +  }
>  
> -  DataOut = (UINT8 *) AllocateZeroPool (DataByteCount);
> -  if (DataOut == NULL) {
> -    DEBUG((DEBUG_ERROR, "SpiFlash: Cannot allocate memory\n"));
> -    return EFI_OUT_OF_RESOURCES;
> +  if (CompareMem (Id, Buffer, DataByteCount) != 0) {
> +    Status = EFI_NOT_FOUND;
>    }
> -  Status = SpiMasterProtocol->Transfer (SpiMasterProtocol, SpiDev,
> -    DataByteCount, Buffer, DataOut, SPI_TRANSFER_BEGIN | SPI_TRANSFER_END);
> -  if (EFI_ERROR(Status)) {
> -    FreePool (DataOut);
> -    DEBUG((DEBUG_ERROR, "SpiFlash: Spi transfer error\n"));
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Unrecognized JEDEC Id bytes: 0x%02x%02x%02x\n",
> +      __FUNCTION__,
> +      Id[0],
> +      Id[1],
> +      Id[2]));
>      return Status;
>    }
>  
> -  // Bytes 1,2 and 3 contain SPI flash ID
> -  Buffer[0] = DataOut[1];
> -  Buffer[1] = DataOut[2];
> -  Buffer[2] = DataOut[3];
> -
> -  FreePool (DataOut);
> -
>    return EFI_SUCCESS;
>  }
>  
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> index f90abb7..2583484 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> @@ -62,6 +62,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #define CMD_ERASE_64K                   0xd8
>  #define CMD_4B_ADDR_ENABLE              0xb7
>  
> +#define SPI_CMD_LEN                     1
> +
>  #define STATUS_REG_POLL_WIP             (1 << 0)
>  #define STATUS_REG_POLL_PEC             (1 << 7)
>  
> diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h
> index 743bb87..e0d62cc 100644
> --- a/Platform/Marvell/Include/Protocol/SpiFlash.h
> +++ b/Platform/Marvell/Include/Protocol/SpiFlash.h
> @@ -47,6 +47,9 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #define CMD_ERASE_64K                   0xd8
>  #define CMD_4B_ADDR_ENABLE              0xb7
>  
> +#define NOR_FLASH_MAX_ID_LEN            6
> +#define NOR_FLASH_ID_DEFAULT_LEN        3
> +
>  extern EFI_GUID gMarvellSpiFlashProtocolGuid;
>  
>  typedef struct _MARVELL_SPI_FLASH_PROTOCOL MARVELL_SPI_FLASH_PROTOCOL;
> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
> index cd800c8..679a9d0 100644
> --- a/Platform/Marvell/Marvell.dec
> +++ b/Platform/Marvell/Marvell.dec
> @@ -133,7 +133,7 @@
>    gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|0|UINT64|0x3000054
>    gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|0|UINT32|0x3000055
>    gMarvellTokenSpaceGuid.PcdSpiFlashSectorSize|65536|UINT64|0x3000059
> -  gMarvellTokenSpaceGuid.PcdSpiFlashId|0|UINT32|0x3000056
> +  gMarvellTokenSpaceGuid.PcdSpiFlashId|{ 0x0 }|VOID*|0x3000056
>    gMarvellTokenSpaceGuid.PcdSpiFlashCs|0|UINT32|0x3000057
>    gMarvellTokenSpaceGuid.PcdSpiFlashMode|0|UINT32|0x3000058
>  
> -- 
> 2.7.4
>