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

[Marcin Wojtas <mw@semihalf.com>]

2017-10-31 10:07 GMT+01:00 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.
>

I know. It is however a change that was somewhat artificially
extracted, so that to make the next patch more readable
(NOR_FLASH_ID_DEFAULT_LEN is removed there). I will substitute it with
PcdGetSize (PcdSpiFlashId).

Marcin

> /
>     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
>>