[platforms: PATCH 0/6] Armada 7k/8k SPI improvements pt 2.

[platforms: PATCH 0/6] Armada 7k/8k SPI improvements pt 2.

[Marcin Wojtas]

Hi,

I submit a series, which comprises a major rework allowing
to make use of dynamic SPI NOR flash detection with recently
implemented NorFlashInfoLib. Comparing to initial version
of the patches, apart from the flash info table extraction,
also the functional changes around ReadId are taken into
a separate commit. This, I hope, results in readable patchset.

Patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/spi-upstream-r20171031
NorFlashInfoLib:
https://github.com/MarvellEmbeddedProcessors/edk2/commits/norlib-upstream-r20171030

I'm looking forward to the comments or remarks.

Best regards,
Marcin

Marcin Wojtas (6):
  Marvell/Drivers: MvSpiFlash: Improve ReadId
  Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection
  Marvell/Drivers: MvSpiFlash: Remove duplicated macros
  Marvell/Applications: SpiTool: Do not override existing slave device
  Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion
  Marvell/Drivers: MvSpiDxe: Keep data in SPI_DEVICE structure

 Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c   |  25 +----
 Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf |   4 +-
 Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c      |  44 +++-----
 Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf    |   2 +-
 Platform/Marvell/Armada/Armada.dsc.inc                   |   1 +
 Platform/Marvell/Armada/Armada70x0.dsc                   |   5 -
 Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c        | 108 ++++++++++++--------
 Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h        |   6 ++
 Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf      |   9 +-
 Platform/Marvell/Drivers/Spi/MvSpiDxe.c                  |  63 +++++++-----
 Platform/Marvell/Drivers/Spi/MvSpiDxe.h                  |   1 +
 Platform/Marvell/Drivers/Spi/MvSpiDxe.inf                |   2 +
 Platform/Marvell/Include/Protocol/Spi.h                  |   6 ++
 Platform/Marvell/Include/Protocol/SpiFlash.h             |  14 +--
 Platform/Marvell/Marvell.dec                             |   6 --
 Silicon/Marvell/Documentation/PortingGuide.txt           |  18 ----
 16 files changed, 141 insertions(+), 173 deletions(-)

-- 
2.7.4

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

[Marcin Wojtas]

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);
+  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

[platforms: PATCH 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection

[Marcin Wojtas]

Hitherto mechanism of fixing SPI flash model in the PCDs,
occured to be very inefficient and problematic. Enable
dynamic detection by reworking MvSpiFlashReadId() command,
which now uses newly added NorFlashInfoLib, that helps to
obtain description of the JEDEC compliant devices.

This patch updates the MvSpiFlashProtocol ReadId() protocol
callback on both producer's (MvFlashDxe) and consumers' sides
(FirmwareUpdate and SpiTool applications). Because all
information about detected SPI NOR flash is now stored in
the obtained NorFlashInfo structure fields, use them instead
of the PCDs.

Enable compilation of the NorFlashInfoLib and update
PortingGuide documentation accordingly.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c   |  5 +-
 Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf |  4 +-
 Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c      |  5 +-
 Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf    |  2 +-
 Platform/Marvell/Armada/Armada.dsc.inc                   |  1 +
 Platform/Marvell/Armada/Armada70x0.dsc                   |  5 --
 Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c        | 68 +++++++++++---------
 Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf      |  9 +--
 Platform/Marvell/Drivers/Spi/MvSpiDxe.inf                |  2 +
 Platform/Marvell/Include/Protocol/Spi.h                  |  3 +
 Platform/Marvell/Include/Protocol/SpiFlash.h             |  6 +-
 Platform/Marvell/Marvell.dec                             |  6 --
 Silicon/Marvell/Documentation/PortingGuide.txt           | 18 ------
 13 files changed, 51 insertions(+), 83 deletions(-)

diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
index d70645d..750e52a 100644
--- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
+++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
@@ -94,12 +94,9 @@ SpiFlashProbe (
   )
 {
   EFI_STATUS       Status;
-  UINT8           *FlashId;
-
-  FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
 
   // Read SPI flash ID
-  Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
+  Status = SpiFlashProtocol->ReadId (Slave, FALSE);
   if (EFI_ERROR (Status)) {
     return SHELL_ABORTED;
   }
diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
index 92c3333..53ea491 100644
--- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
+++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
@@ -44,6 +44,7 @@
   FUpdate.uni
 
 [Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
   MdeModulePkg/MdeModulePkg.dec
   MdePkg/MdePkg.dec
   Platform/Marvell/Marvell.dec
@@ -64,9 +65,6 @@
   UefiLib
   UefiRuntimeServicesTableLib
 
-[Pcd]
- gMarvellTokenSpaceGuid.PcdSpiFlashId
-
 [Protocols]
  gMarvellSpiFlashProtocolGuid
  gMarvellSpiMasterProtocolGuid
diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
index a12f2ec..68a6cf7 100644
--- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
+++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
@@ -166,11 +166,8 @@ FlashProbe (
   )
 {
   EFI_STATUS Status;
-  UINT8      *FlashId;
 
-  FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
-
-  Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
+  Status = SpiFlashProtocol->ReadId (Slave, FALSE);
   if (EFI_ERROR (Status)) {
     return SHELL_ABORTED;
   }
diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
index 887b9a5..a52906b 100644
--- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
+++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
@@ -44,6 +44,7 @@
  SpiFlashCmd.uni
 
 [Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
  MdePkg/MdePkg.dec
  ShellPkg/ShellPkg.dec
  MdeModulePkg/MdeModulePkg.dec
@@ -66,7 +67,6 @@
 
 [Pcd]
  gMarvellTokenSpaceGuid.PcdSpiFlashCs
- gMarvellTokenSpaceGuid.PcdSpiFlashId
  gMarvellTokenSpaceGuid.PcdSpiFlashMode
 
 [Protocols]
diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
index b9fc384..2cd96e6 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -33,6 +33,7 @@
   ArmPlatformLib|Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
   ComPhyLib|Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
   MppLib|Platform/Marvell/Library/MppLib/MppLib.inf
+  NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
   UtmiPhyLib|Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
 
   DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
index 4d5f55f..8e4cdb2 100644
--- a/Platform/Marvell/Armada/Armada70x0.dsc
+++ b/Platform/Marvell/Armada/Armada70x0.dsc
@@ -90,11 +90,6 @@
   gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|10000000
   gMarvellTokenSpaceGuid.PcdSpiClockFrequency|200000000
 
-  gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd|0x70
-  gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|3
-  gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|65536
-  gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|256
-  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 ab3ed6a..703994c 100755
--- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
+++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
@@ -107,10 +107,10 @@ MvSpiFlashWriteCommon (
   UINT8 PollBit = STATUS_REG_POLL_WIP;
   UINT8 CheckStatus = 0x0;
 
-  CmdStatus = (UINT8)PcdGet32 (PcdSpiFlashPollCmd);
-  if (CmdStatus == CMD_FLAG_STATUS) {
+  if (Slave->Info->Flags & NF_WRITE_FSR) {
+    CmdStatus = CMD_FLAG_STATUS;
     PollBit = STATUS_REG_POLL_PEC;
-    CheckStatus = PollBit;
+    CheckStatus = STATUS_REG_POLL_PEC;
   }
 
   // Send command
@@ -181,8 +181,19 @@ MvSpiFlashErase (
   UINTN EraseSize;
   UINT8 Cmd[5];
 
-  AddrSize  = PcdGet32 (PcdSpiFlashAddressCycles);
-  EraseSize = PcdGet64 (PcdSpiFlashEraseSize);
+  if (Slave->Info->Flags & NF_4B_ADDR) {
+    AddrSize = 4;
+  } else {
+    AddrSize = 3;
+  }
+
+  if (Slave->Info->Flags & NF_ERASE_4K) {
+    Cmd[0] = CMD_ERASE_4K;
+    EraseSize = SIZE_4KB;
+  } else {
+    Cmd[0] = CMD_ERASE_64K;
+    EraseSize = Slave->Info->SectorSize;
+  }
 
   // Check input parameters
   if (Offset % EraseSize || Length % EraseSize) {
@@ -191,21 +202,6 @@ MvSpiFlashErase (
     return EFI_DEVICE_ERROR;
   }
 
-  switch (EraseSize) {
-  case SIZE_4KB:
-    Cmd[0] = CMD_ERASE_4K;
-    break;
-  case SIZE_32KB:
-    Cmd[0] = CMD_ERASE_32K;
-    break;
-  case SIZE_64KB:
-    Cmd[0] = CMD_ERASE_64K;
-    break;
-  default:
-    DEBUG ((DEBUG_ERROR, "MvSpiFlash: Invalid EraseSize parameter\n"));
-    return EFI_INVALID_PARAMETER;
-  }
-
   while (Length) {
     EraseAddr = Offset;
 
@@ -239,7 +235,11 @@ MvSpiFlashRead (
   UINT32 AddrSize, ReadAddr, ReadLength, RemainLength;
   UINTN BankSel = 0;
 
-  AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
+  if (Slave->Info->Flags & NF_4B_ADDR) {
+    AddrSize = 4;
+  } else {
+    AddrSize = 3;
+  }
 
   Cmd[0] = CMD_READ_ARRAY_FAST;
 
@@ -282,8 +282,13 @@ MvSpiFlashWrite (
   UINT32 WriteAddr;
   UINT8 Cmd[5], AddrSize;
 
-  AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
-  PageSize = PcdGet32 (PcdSpiFlashPageSize);
+  if (Slave->Info->Flags & NF_4B_ADDR) {
+    AddrSize = 4;
+  } else {
+    AddrSize = 3;
+  }
+
+  PageSize = Slave->Info->PageSize;
 
   Cmd[0] = CMD_PAGE_PROGRAM;
 
@@ -370,7 +375,7 @@ MvSpiFlashUpdate (
   UINT64 SectorSize, ToUpdate, Scale = 1;
   UINT8 *TmpBuf, *End;
 
-  SectorSize = PcdGet64 (PcdSpiFlashSectorSize);
+  SectorSize = Slave->Info->SectorSize;
 
   End = Buf + ByteCount;
 
@@ -404,8 +409,7 @@ EFI_STATUS
 EFIAPI
 MvSpiFlashReadId (
   IN     SPI_DEVICE *SpiDev,
-  IN     UINT32     DataByteCount,
-  IN OUT UINT8      *Buffer
+  IN     BOOLEAN     UseInRuntime
   )
 {
   EFI_STATUS Status;
@@ -425,9 +429,7 @@ MvSpiFlashReadId (
     return Status;
   }
 
-  if (CompareMem (Id, Buffer, DataByteCount) != 0) {
-    Status = EFI_NOT_FOUND;
-  }
+  Status = GetNorFlashInfo (Id, &SpiDev->Info, UseInRuntime);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR,
       "%a: Unrecognized JEDEC Id bytes: 0x%02x%02x%02x\n",
@@ -438,6 +440,8 @@ MvSpiFlashReadId (
     return Status;
   }
 
+  PrintNorFlashInfo (SpiDev->Info);
+
   return EFI_SUCCESS;
 }
 
@@ -452,7 +456,11 @@ MvSpiFlashInit (
   UINT8 Cmd, StatusRegister;
   UINT32 AddrSize;
 
-  AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
+  if (Slave->Info->Flags & NF_4B_ADDR) {
+    AddrSize = 4;
+  } else {
+    AddrSize = 3;
+  }
 
   if (AddrSize == 4) {
     // Set 4 byte address mode
diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
index 4519b02..6587f69 100644
--- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
+++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
@@ -42,10 +42,12 @@
   MvSpiFlash.h
 
 [Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
   Platform/Marvell/Marvell.dec
 
 [LibraryClasses]
+  NorFlashInfoLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   TimerLib
@@ -53,13 +55,6 @@
   DebugLib
   MemoryAllocationLib
 
-[FixedPcd]
-  gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles
-  gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize
-  gMarvellTokenSpaceGuid.PcdSpiFlashPageSize
-  gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd
-  gMarvellTokenSpaceGuid.PcdSpiFlashSectorSize
-
 [Protocols]
   gMarvellSpiMasterProtocolGuid
   gMarvellSpiFlashProtocolGuid
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf b/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
index d38d331..08c6c04 100644
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
@@ -42,10 +42,12 @@
   MvSpiDxe.h
 
 [Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
   Platform/Marvell/Marvell.dec
 
 [LibraryClasses]
+  NorFlashInfoLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   TimerLib
diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
index ae78a31..93a8ec0 100644
--- a/Platform/Marvell/Include/Protocol/Spi.h
+++ b/Platform/Marvell/Include/Protocol/Spi.h
@@ -34,6 +34,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #ifndef __MARVELL_SPI_MASTER_PROTOCOL_H__
 #define __MARVELL_SPI_MASTER_PROTOCOL_H__
 
+#include <Library/NorFlashInfoLib.h>
+
 extern EFI_GUID gMarvellSpiMasterProtocolGuid;
 
 typedef struct _MARVELL_SPI_MASTER_PROTOCOL MARVELL_SPI_MASTER_PROTOCOL;
@@ -49,6 +51,7 @@ typedef struct {
   INTN Cs;
   INTN MaxFreq;
   SPI_MODE Mode;
+  NOR_FLASH_INFO *Info;
 } SPI_DEVICE;
 
 typedef
diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h
index e0d62cc..4a3053e 100644
--- a/Platform/Marvell/Include/Protocol/SpiFlash.h
+++ b/Platform/Marvell/Include/Protocol/SpiFlash.h
@@ -47,9 +47,6 @@ 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;
@@ -65,8 +62,7 @@ typedef
 EFI_STATUS
 (EFIAPI *MV_SPI_FLASH_READ_ID) (
   IN     SPI_DEVICE *SpiDev,
-  IN     UINT32     DataByteCount,
-  IN OUT UINT8      *Buffer
+  IN     BOOLEAN     UseInRuntime
   );
 
 typedef
diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
index 679a9d0..8255895 100644
--- a/Platform/Marvell/Marvell.dec
+++ b/Platform/Marvell/Marvell.dec
@@ -128,12 +128,6 @@
   gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|0|UINT32|0x30000052
   gMarvellTokenSpaceGuid.PcdSpiClockFrequency|0|UINT32|0x30000053
 
-  gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd|0|UINT32|0x3000052
-  gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|0|UINT32|0x3000053
-  gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|0|UINT64|0x3000054
-  gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|0|UINT32|0x3000055
-  gMarvellTokenSpaceGuid.PcdSpiFlashSectorSize|65536|UINT64|0x3000059
-  gMarvellTokenSpaceGuid.PcdSpiFlashId|{ 0x0 }|VOID*|0x3000056
   gMarvellTokenSpaceGuid.PcdSpiFlashCs|0|UINT32|0x3000057
   gMarvellTokenSpaceGuid.PcdSpiFlashMode|0|UINT32|0x3000058
 
diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
index cbe3bed..d5deed5 100644
--- a/Silicon/Marvell/Documentation/PortingGuide.txt
+++ b/Silicon/Marvell/Documentation/PortingGuide.txt
@@ -312,24 +312,6 @@ SpiFlash configuration
 ======================
 Folowing PCDs for spi flash driver configuration must be set properly:
 
-  - gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles
-        (Size of SPI flash address in bytes (3 or 4) )
-
-  - gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize
-        (Size of minimal erase block in bytes)
-
-  - gMarvellTokenSpaceGuid.PcdSpiFlashPageSize
-        (Size of SPI flash page)
-
-  - gMarvellTokenSpaceGuid.PcdSpiFlashSectorSize
-        (Size of SPI flash sector, 65536 bytes by default)
-
-  - gMarvellTokenSpaceGuid.PcdSpiFlashId
-        (Id of SPI flash)
-
-  - gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd
-        (Spi flash polling flag)
-
   - gMarvellTokenSpaceGuid.PcdSpiFlashMode
         (Default SCLK mode (see SPI_MODE enum in file
          edk2-platforms/Platform/Marvell/Drivers/Spi/MvSpi.h))
-- 
2.7.4

[platforms: PATCH 3/6] Marvell/Drivers: MvSpiFlash: Remove duplicated macros

[Marcin Wojtas]

Flash commands macros are already defined locally, so
remove them from the protocol header.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Include/Protocol/SpiFlash.h | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h
index 4a3053e..4ba29ba 100644
--- a/Platform/Marvell/Include/Protocol/SpiFlash.h
+++ b/Platform/Marvell/Include/Protocol/SpiFlash.h
@@ -36,17 +36,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include <Protocol/Spi.h>
 
-#define CMD_READ_ID                     0x9f
-#define READ_STATUS_REG_CMD             0x0b
-#define CMD_WRITE_ENABLE                0x06
-#define CMD_FLAG_STATUS                 0x70
-#define CMD_WRITE_STATUS_REG            0x01
-#define CMD_READ_ARRAY_FAST             0x0b
-#define CMD_PAGE_PROGRAM                0x02
-#define CMD_BANK_WRITE                  0xc5
-#define CMD_ERASE_64K                   0xd8
-#define CMD_4B_ADDR_ENABLE              0xb7
-
 extern EFI_GUID gMarvellSpiFlashProtocolGuid;
 
 typedef struct _MARVELL_SPI_FLASH_PROTOCOL MARVELL_SPI_FLASH_PROTOCOL;
-- 
2.7.4

[platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device

[Marcin Wojtas]

Current usage of sf command requires running 'sf probe' prior to
executing any other option. Because it is done in two separate steps,
it turned out that SpiMasterProtocol->SetupDevice could easily
overwrite valid Slave pointer when performing second operation.

Fix the issue by allocating Slave device only once and keep it
as global variable in the SpiTool application. This patch
also updates FirmwareUpdate command to follow the modified
SetupDevice operation.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c |  4 ++--
 Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c    |  8 ++++----
 Platform/Marvell/Drivers/Spi/MvSpiDxe.c                | 17 +++++++++--------
 Platform/Marvell/Drivers/Spi/MvSpiDxe.h                |  1 +
 Platform/Marvell/Include/Protocol/Spi.h                |  1 +
 5 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
index 750e52a..9ccb1c7 100644
--- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
+++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
@@ -240,7 +240,7 @@ ShellCommandRunFUpdate (
   )
 {
   IN SHELL_FILE_HANDLE    FileHandle;
-  SPI_DEVICE              *Slave;
+  SPI_DEVICE              *Slave = NULL;
   UINT64                  FileSize;
   UINTN                   *FileBuffer = NULL;
   CHAR16                  *ProblemParam;
@@ -302,7 +302,7 @@ ShellCommandRunFUpdate (
   }
 
   // Setup and probe SPI flash
-  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, 0, 0);
+  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, 0, 0);
   if (Slave == NULL) {
     Print(L"%s: Cannot allocate SPI device!\n", CMD_NAME_STRING);
     goto HeaderError;
diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
index 68a6cf7..1084f68 100644
--- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
+++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
@@ -191,7 +191,7 @@ ShellCommandRunSpiFlash (
   )
 {
 EFI_STATUS              Status;
-  SPI_DEVICE            *Slave;
+  STATIC SPI_DEVICE     *Slave;
   LIST_ENTRY            *CheckPackage;
   EFI_PHYSICAL_ADDRESS  Address = 0, Offset = 0;
   SHELL_FILE_HANDLE     FileHandle = NULL;
@@ -273,7 +273,7 @@ EFI_STATUS              Status;
   Cs = PcdGet32 (PcdSpiFlashCs);
 
   // Setup new spi device
-  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Cs, Mode);
+  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, Cs, Mode);
     if (Slave == NULL) {
       Print(L"sf: Cannot allocate SPI device!\n");
       return SHELL_ABORTED;
@@ -285,6 +285,8 @@ EFI_STATUS              Status;
     Status = FlashProbe (Slave);
     if (EFI_ERROR(Status)) {
       // No supported spi flash detected
+      SpiMasterProtocol->FreeDevice(Slave);
+      Slave = NULL;
       return SHELL_ABORTED;
     } else {
       return Status;
@@ -426,8 +428,6 @@ EFI_STATUS              Status;
     break;
   }
 
-  SpiMasterProtocol->FreeDevice(Slave);
-
   if (EFI_ERROR (Status)) {
     Print (L"sf: Error while performing spi transfer\n");
     return SHELL_ABORTED;
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
index 3b49147..a7db5f2 100755
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
@@ -296,21 +296,22 @@ SPI_DEVICE *
 EFIAPI
 MvSpiSetupSlave (
   IN MARVELL_SPI_MASTER_PROTOCOL *This,
+  IN SPI_DEVICE *Slave,
   IN UINTN Cs,
   IN SPI_MODE Mode
   )
 {
-  SPI_DEVICE  *Slave;
+  if (!Slave) {
+    Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
+    if (Slave == NULL) {
+      DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
+      return NULL;
+    }
 
-  Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
-  if (Slave == NULL) {
-    DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
-    return NULL;
+    Slave->Cs   = Cs;
+    Slave->Mode = Mode;
   }
 
-  Slave->Cs   = Cs;
-  Slave->Mode = Mode;
-
   SpiSetupTransfer (This, Slave);
 
   return Slave;
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
index 1401f62..e7e280a 100644
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
@@ -125,6 +125,7 @@ SPI_DEVICE *
 EFIAPI
 MvSpiSetupSlave (
   IN MARVELL_SPI_MASTER_PROTOCOL     * This,
+  IN SPI_DEVICE *Slave,
   IN UINTN Cs,
   IN SPI_MODE Mode
   );
diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
index 93a8ec0..0cf7914 100644
--- a/Platform/Marvell/Include/Protocol/Spi.h
+++ b/Platform/Marvell/Include/Protocol/Spi.h
@@ -87,6 +87,7 @@ typedef
 SPI_DEVICE *
 (EFIAPI *MV_SPI_SETUP_DEVICE) (
   IN MARVELL_SPI_MASTER_PROTOCOL *This,
+  IN SPI_DEVICE *Slave,
   IN UINTN    Cs,
   IN SPI_MODE Mode
   );
-- 
2.7.4

[platforms: PATCH 5/6] Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion

[Marcin Wojtas]

Spansion SPI flash devices use different command for bank
selection. Update it, basing on the first byte of flash ID.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 5 +++++
 Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
index 703994c..a00fc305 100755
--- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
+++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
@@ -150,6 +150,11 @@ SpiFlashCmdBankaddrWrite (
 {
   UINT8 Cmd = CMD_BANK_WRITE;
 
+  /* Update bank selection command for Spansion */
+  if (Slave->Info->Id[0] == SPI_FLASH_MFR_SPANSION) {
+    Cmd = CMD_BANKADDR_BRWR;
+  }
+
   MvSpiFlashWriteCommon (Slave, &Cmd, 1, &BankSel, 1);
 }
 
diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
index 2583484..00af188 100755
--- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
+++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
@@ -57,6 +57,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #define CMD_READ_ARRAY_FAST             0x0b
 #define CMD_PAGE_PROGRAM                0x02
 #define CMD_BANK_WRITE                  0xc5
+#define CMD_BANKADDR_BRWR               0x17
 #define CMD_ERASE_4K                    0x20
 #define CMD_ERASE_32K                   0x52
 #define CMD_ERASE_64K                   0xd8
@@ -72,6 +73,9 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #define SPI_FLASH_16MB_BOUN             0x1000000
 
+/* Manufacturer ID's */
+#define SPI_FLASH_MFR_SPANSION          0x01
+
 typedef enum {
   SPI_FLASH_READ_ID,
   SPI_FLASH_READ, // Read from SPI flash with address
-- 
2.7.4

[platforms: PATCH 6/6] Marvell/Drivers: MvSpiDxe: Keep data in SPI_DEVICE structure

[Marcin Wojtas]

In the MvSpiDxe driver obtaining host register base address,
controller clock and device maximum frequency directly from PCDs
was done all over the code. This patch cleans up the parameters'
handling and enables accessing them from SPI_DEVICE structure fields.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 48 ++++++++++++--------
 Platform/Marvell/Include/Protocol/Spi.h |  2 +
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
index a7db5f2..c60a520 100755
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
@@ -38,12 +38,13 @@ SPI_MASTER *mSpiMasterInstance;
 STATIC
 EFI_STATUS
 SpiSetBaudRate (
+  IN SPI_DEVICE *Slave,
   IN UINT32 CpuClock,
   IN UINT32 MaxFreq
   )
 {
   UINT32 Spr, BestSpr, Sppr, BestSppr, ClockDivider, Match, Reg, MinBaudDiff;
-  UINTN SpiRegBase = PcdGet32 (PcdSpiRegBase);
+  UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
 
   MinBaudDiff = 0xFFFFFFFF;
   BestSppr = 0;
@@ -93,26 +94,28 @@ SpiSetBaudRate (
 STATIC
 VOID
 SpiSetCs (
-  UINT8 CsId
+  IN SPI_DEVICE *Slave
   )
 {
-  UINT32 Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
+  UINT32 Reg;
+  UINTN  SpiRegBase = Slave->HostRegisterBaseAddress;
 
   Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
   Reg &= ~SPI_CS_NUM_MASK;
-  Reg |= (CsId << SPI_CS_NUM_OFFSET);
+  Reg |= (Slave->Cs << SPI_CS_NUM_OFFSET);
   MmioWrite32 (SpiRegBase + SPI_CTRL_REG, Reg);
 }
 
 STATIC
 VOID
 SpiActivateCs (
-  UINT8 IN CsId
+  IN SPI_DEVICE *Slave
   )
 {
-  UINT32  Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
+  UINT32 Reg;
+  UINTN  SpiRegBase = Slave->HostRegisterBaseAddress;
 
-  SpiSetCs(CsId);
+  SpiSetCs(Slave);
   Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
   Reg |= SPI_CS_EN_MASK;
   MmioWrite32(SpiRegBase + SPI_CTRL_REG, Reg);
@@ -121,10 +124,11 @@ SpiActivateCs (
 STATIC
 VOID
 SpiDeactivateCs (
-  VOID
+  IN SPI_DEVICE *Slave
   )
 {
-  UINT32  Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
+  UINT32 Reg;
+  UINTN  SpiRegBase = Slave->HostRegisterBaseAddress;
 
   Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
   Reg &= ~SPI_CS_EN_MASK;
@@ -139,14 +143,15 @@ SpiSetupTransfer (
   )
 {
   SPI_MASTER *SpiMaster;
-  UINT32 Reg, SpiRegBase, CoreClock, SpiMaxFreq;
+  UINT32 Reg, CoreClock, SpiMaxFreq;
+  UINTN SpiRegBase;
 
   SpiMaster = SPI_MASTER_FROM_SPI_MASTER_PROTOCOL (This);
 
   // Initialize values from PCDs
-  SpiRegBase  = PcdGet32 (PcdSpiRegBase);
-  CoreClock   = PcdGet32 (PcdSpiClockFrequency);
-  SpiMaxFreq  = PcdGet32 (PcdSpiMaxFrequency);
+  SpiRegBase  = Slave->HostRegisterBaseAddress;
+  CoreClock   = Slave->CoreClock;
+  SpiMaxFreq  = Slave->MaxFreq;
 
   EfiAcquireLock (&SpiMaster->Lock);
 
@@ -154,9 +159,9 @@ SpiSetupTransfer (
   Reg |= SPI_BYTE_LENGTH;
   MmioWrite32 (SpiRegBase + SPI_CONF_REG, Reg);
 
-  SpiSetCs(Slave->Cs);
+  SpiSetCs(Slave);
 
-  SpiSetBaudRate (CoreClock, SpiMaxFreq);
+  SpiSetBaudRate (Slave, CoreClock, SpiMaxFreq);
 
   Reg = MmioRead32 (SpiRegBase + SPI_CONF_REG);
   Reg &= ~(SPI_CPOL_MASK | SPI_CPHA_MASK | SPI_TXLSBF_MASK | SPI_RXLSBF_MASK);
@@ -194,21 +199,22 @@ MvSpiTransfer (
 {
   SPI_MASTER *SpiMaster;
   UINT64  Length;
-  UINT32  Iterator, Reg, SpiRegBase;
+  UINT32  Iterator, Reg;
   UINT8   *DataOutPtr = (UINT8 *)DataOut;
   UINT8   *DataInPtr  = (UINT8 *)DataIn;
   UINT8   DataToSend  = 0;
+  UINTN   SpiRegBase;
 
   SpiMaster = SPI_MASTER_FROM_SPI_MASTER_PROTOCOL (This);
 
-  SpiRegBase = PcdGet32 (PcdSpiRegBase);
+  SpiRegBase = Slave->HostRegisterBaseAddress;
 
   Length = 8 * DataByteCount;
 
   EfiAcquireLock (&SpiMaster->Lock);
 
   if (Flag & SPI_TRANSFER_BEGIN) {
-    SpiActivateCs (Slave->Cs);
+    SpiActivateCs (Slave);
   }
 
   // Set 8-bit mode
@@ -245,7 +251,7 @@ MvSpiTransfer (
   }
 
   if (Flag & SPI_TRANSFER_END) {
-    SpiDeactivateCs ();
+    SpiDeactivateCs (Slave);
   }
 
   EfiReleaseLock (&SpiMaster->Lock);
@@ -312,6 +318,10 @@ MvSpiSetupSlave (
     Slave->Mode = Mode;
   }
 
+  Slave->HostRegisterBaseAddress = PcdGet32 (PcdSpiRegBase);
+  Slave->CoreClock = PcdGet32 (PcdSpiClockFrequency);
+  Slave->MaxFreq = PcdGet32 (PcdSpiMaxFrequency);
+
   SpiSetupTransfer (This, Slave);
 
   return Slave;
diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
index 0cf7914..b8981f3 100644
--- a/Platform/Marvell/Include/Protocol/Spi.h
+++ b/Platform/Marvell/Include/Protocol/Spi.h
@@ -52,6 +52,8 @@ typedef struct {
   INTN MaxFreq;
   SPI_MODE Mode;
   NOR_FLASH_INFO *Info;
+  UINTN HostRegisterBaseAddress;
+  UINTN CoreClock;
 } SPI_DEVICE;
 
 typedef
-- 
2.7.4

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

[Leif Lindholm]

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
> 

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

[Marcin Wojtas]

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

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

[Leif Lindholm]

On Tue, Oct 31, 2017 at 10:22:50AM +0100, Marcin Wojtas wrote:
> 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).

Right, OK, that sort of thing is useful to mention in the cover
letter.

But there's also MAX_ID_LEN, which is added here only to allocate
buffers that are hard-coded to always contain value of ID_DEFAULT_LEN.
Surely this could just be left out?

/
    Leif

Re: [platforms: PATCH 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection

[Leif Lindholm]

On Tue, Oct 31, 2017 at 04:59:31AM +0100, Marcin Wojtas wrote:
> Hitherto mechanism of fixing SPI flash model in the PCDs,
> occured to be very inefficient and problematic. Enable
> dynamic detection by reworking MvSpiFlashReadId() command,
> which now uses newly added NorFlashInfoLib, that helps to
> obtain description of the JEDEC compliant devices.
> 
> This patch updates the MvSpiFlashProtocol ReadId() protocol
> callback on both producer's (MvFlashDxe) and consumers' sides
> (FirmwareUpdate and SpiTool applications). Because all
> information about detected SPI NOR flash is now stored in
> the obtained NorFlashInfo structure fields, use them instead
> of the PCDs.
> 
> Enable compilation of the NorFlashInfoLib and update
> PortingGuide documentation accordingly.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c   |  5 +-
>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf |  4 +-
>  Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c      |  5 +-
>  Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf    |  2 +-
>  Platform/Marvell/Armada/Armada.dsc.inc                   |  1 +
>  Platform/Marvell/Armada/Armada70x0.dsc                   |  5 --
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c        | 68 +++++++++++---------
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf      |  9 +--
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.inf                |  2 +
>  Platform/Marvell/Include/Protocol/Spi.h                  |  3 +
>  Platform/Marvell/Include/Protocol/SpiFlash.h             |  6 +-
>  Platform/Marvell/Marvell.dec                             |  6 --
>  Silicon/Marvell/Documentation/PortingGuide.txt           | 18 ------
>  13 files changed, 51 insertions(+), 83 deletions(-)
> 
> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> index d70645d..750e52a 100644
> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> @@ -94,12 +94,9 @@ SpiFlashProbe (
>    )
>  {
>    EFI_STATUS       Status;
> -  UINT8           *FlashId;
> -
> -  FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
>  
>    // Read SPI flash ID
> -  Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
> +  Status = SpiFlashProtocol->ReadId (Slave, FALSE);
>    if (EFI_ERROR (Status)) {
>      return SHELL_ABORTED;
>    }
> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
> index 92c3333..53ea491 100644
> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
> @@ -44,6 +44,7 @@
>    FUpdate.uni
>  
>  [Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
>    Platform/Marvell/Marvell.dec
> @@ -64,9 +65,6 @@
>    UefiLib
>    UefiRuntimeServicesTableLib
>  
> -[Pcd]
> - gMarvellTokenSpaceGuid.PcdSpiFlashId
> -
>  [Protocols]
>   gMarvellSpiFlashProtocolGuid
>   gMarvellSpiMasterProtocolGuid
> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> index a12f2ec..68a6cf7 100644
> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> @@ -166,11 +166,8 @@ FlashProbe (
>    )
>  {
>    EFI_STATUS Status;
> -  UINT8      *FlashId;
>  
> -  FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
> -
> -  Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
> +  Status = SpiFlashProtocol->ReadId (Slave, FALSE);
>    if (EFI_ERROR (Status)) {
>      return SHELL_ABORTED;
>    }
> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
> index 887b9a5..a52906b 100644
> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
> @@ -44,6 +44,7 @@
>   SpiFlashCmd.uni
>  
>  [Packages]
> + EmbeddedPkg/EmbeddedPkg.dec
>   MdePkg/MdePkg.dec
>   ShellPkg/ShellPkg.dec
>   MdeModulePkg/MdeModulePkg.dec
> @@ -66,7 +67,6 @@
>  
>  [Pcd]
>   gMarvellTokenSpaceGuid.PcdSpiFlashCs
> - gMarvellTokenSpaceGuid.PcdSpiFlashId
>   gMarvellTokenSpaceGuid.PcdSpiFlashMode
>  
>  [Protocols]
> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
> index b9fc384..2cd96e6 100644
> --- a/Platform/Marvell/Armada/Armada.dsc.inc
> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
> @@ -33,6 +33,7 @@
>    ArmPlatformLib|Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
>    ComPhyLib|Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
>    MppLib|Platform/Marvell/Library/MppLib/MppLib.inf
> +  NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
>    UtmiPhyLib|Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
>  
>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index 4d5f55f..8e4cdb2 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -90,11 +90,6 @@
>    gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|10000000
>    gMarvellTokenSpaceGuid.PcdSpiClockFrequency|200000000
>  
> -  gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd|0x70
> -  gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|3
> -  gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|65536
> -  gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|256
> -  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 ab3ed6a..703994c 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> @@ -107,10 +107,10 @@ MvSpiFlashWriteCommon (
>    UINT8 PollBit = STATUS_REG_POLL_WIP;
>    UINT8 CheckStatus = 0x0;
>  
> -  CmdStatus = (UINT8)PcdGet32 (PcdSpiFlashPollCmd);
> -  if (CmdStatus == CMD_FLAG_STATUS) {
> +  if (Slave->Info->Flags & NF_WRITE_FSR) {
> +    CmdStatus = CMD_FLAG_STATUS;
>      PollBit = STATUS_REG_POLL_PEC;
> -    CheckStatus = PollBit;
> +    CheckStatus = STATUS_REG_POLL_PEC;
>    }
>  
>    // Send command
> @@ -181,8 +181,19 @@ MvSpiFlashErase (
>    UINTN EraseSize;
>    UINT8 Cmd[5];
>  
> -  AddrSize  = PcdGet32 (PcdSpiFlashAddressCycles);
> -  EraseSize = PcdGet64 (PcdSpiFlashEraseSize);
> +  if (Slave->Info->Flags & NF_4B_ADDR) {
> +    AddrSize = 4;
> +  } else {
> +    AddrSize = 3;
> +  }
> +
> +  if (Slave->Info->Flags & NF_ERASE_4K) {
> +    Cmd[0] = CMD_ERASE_4K;
> +    EraseSize = SIZE_4KB;
> +  } else {

Are 4 and 64K the only legal sizes?
This patch deletes a detection of 32K size and the error message (and
return) if an unknown size is encountered.

> +    Cmd[0] = CMD_ERASE_64K;
> +    EraseSize = Slave->Info->SectorSize;
> +  }
>  
>    // Check input parameters
>    if (Offset % EraseSize || Length % EraseSize) {
> @@ -191,21 +202,6 @@ MvSpiFlashErase (
>      return EFI_DEVICE_ERROR;
>    }
>  
> -  switch (EraseSize) {
> -  case SIZE_4KB:
> -    Cmd[0] = CMD_ERASE_4K;
> -    break;
> -  case SIZE_32KB:
> -    Cmd[0] = CMD_ERASE_32K;
> -    break;
> -  case SIZE_64KB:
> -    Cmd[0] = CMD_ERASE_64K;
> -    break;
> -  default:
> -    DEBUG ((DEBUG_ERROR, "MvSpiFlash: Invalid EraseSize parameter\n"));
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
>    while (Length) {
>      EraseAddr = Offset;
>  
> @@ -239,7 +235,11 @@ MvSpiFlashRead (
>    UINT32 AddrSize, ReadAddr, ReadLength, RemainLength;
>    UINTN BankSel = 0;
>  
> -  AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
> +  if (Slave->Info->Flags & NF_4B_ADDR) {
> +    AddrSize = 4;
> +  } else {
> +    AddrSize = 3;
> +  }

I see this sequence repeated many times.
Could it be replaced by a macro or static helper function? Or cached
in a device structure?

/
    Leif

Re: [platforms: PATCH 3/6] Marvell/Drivers: MvSpiFlash: Remove duplicated macros

[Leif Lindholm]

On Tue, Oct 31, 2017 at 04:59:32AM +0100, Marcin Wojtas wrote:
> Flash commands macros are already defined locally, so
> remove them from the protocol header.

Locally?
I have no issue with the patch, but the commit message can be a bit
more descriptive.

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Include/Protocol/SpiFlash.h | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h
> index 4a3053e..4ba29ba 100644
> --- a/Platform/Marvell/Include/Protocol/SpiFlash.h
> +++ b/Platform/Marvell/Include/Protocol/SpiFlash.h
> @@ -36,17 +36,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
>  #include <Protocol/Spi.h>
>  
> -#define CMD_READ_ID                     0x9f
> -#define READ_STATUS_REG_CMD             0x0b
> -#define CMD_WRITE_ENABLE                0x06
> -#define CMD_FLAG_STATUS                 0x70
> -#define CMD_WRITE_STATUS_REG            0x01
> -#define CMD_READ_ARRAY_FAST             0x0b
> -#define CMD_PAGE_PROGRAM                0x02
> -#define CMD_BANK_WRITE                  0xc5
> -#define CMD_ERASE_64K                   0xd8
> -#define CMD_4B_ADDR_ENABLE              0xb7
> -
>  extern EFI_GUID gMarvellSpiFlashProtocolGuid;
>  
>  typedef struct _MARVELL_SPI_FLASH_PROTOCOL MARVELL_SPI_FLASH_PROTOCOL;
> -- 
> 2.7.4
> 

Re: [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device

[Leif Lindholm]

On Tue, Oct 31, 2017 at 04:59:33AM +0100, Marcin Wojtas wrote:
> Current usage of sf command requires running 'sf probe' prior to
> executing any other option. Because it is done in two separate steps,
> it turned out that SpiMasterProtocol->SetupDevice could easily
> overwrite valid Slave pointer when performing second operation.
> 
> Fix the issue by allocating Slave device only once and keep it
> as global variable in the SpiTool application. This patch
> also updates FirmwareUpdate command to follow the modified
> SetupDevice operation.

Really an unrelated question, but would we not expect to use capsule
updates instead now? Do we have other uses for this tool?

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c |  4 ++--
>  Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c    |  8 ++++----
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.c                | 17 +++++++++--------
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.h                |  1 +
>  Platform/Marvell/Include/Protocol/Spi.h                |  1 +
>  5 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> index 750e52a..9ccb1c7 100644
> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> @@ -240,7 +240,7 @@ ShellCommandRunFUpdate (
>    )
>  {
>    IN SHELL_FILE_HANDLE    FileHandle;
> -  SPI_DEVICE              *Slave;
> +  SPI_DEVICE              *Slave = NULL;
>    UINT64                  FileSize;
>    UINTN                   *FileBuffer = NULL;
>    CHAR16                  *ProblemParam;
> @@ -302,7 +302,7 @@ ShellCommandRunFUpdate (
>    }
>  
>    // Setup and probe SPI flash
> -  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, 0, 0);
> +  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, 0, 0);
>    if (Slave == NULL) {
>      Print(L"%s: Cannot allocate SPI device!\n", CMD_NAME_STRING);
>      goto HeaderError;
> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> index 68a6cf7..1084f68 100644
> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> @@ -191,7 +191,7 @@ ShellCommandRunSpiFlash (
>    )
>  {
>  EFI_STATUS              Status;
> -  SPI_DEVICE            *Slave;
> +  STATIC SPI_DEVICE     *Slave;

If this is a safe thing to do, please use a global variable called
mSlave instead, to make it clear that it persists across calls.

>    LIST_ENTRY            *CheckPackage;
>    EFI_PHYSICAL_ADDRESS  Address = 0, Offset = 0;
>    SHELL_FILE_HANDLE     FileHandle = NULL;
> @@ -273,7 +273,7 @@ EFI_STATUS              Status;
>    Cs = PcdGet32 (PcdSpiFlashCs);
>  
>    // Setup new spi device
> -  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Cs, Mode);
> +  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, Cs, Mode);
>      if (Slave == NULL) {
>        Print(L"sf: Cannot allocate SPI device!\n");
>        return SHELL_ABORTED;
> @@ -285,6 +285,8 @@ EFI_STATUS              Status;
>      Status = FlashProbe (Slave);
>      if (EFI_ERROR(Status)) {
>        // No supported spi flash detected
> +      SpiMasterProtocol->FreeDevice(Slave);

Space before (.

/
    Leif

> +      Slave = NULL;
>        return SHELL_ABORTED;
>      } else {
>        return Status;
> @@ -426,8 +428,6 @@ EFI_STATUS              Status;
>      break;
>    }
>  
> -  SpiMasterProtocol->FreeDevice(Slave);
> -
>    if (EFI_ERROR (Status)) {
>      Print (L"sf: Error while performing spi transfer\n");
>      return SHELL_ABORTED;
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> index 3b49147..a7db5f2 100755
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> @@ -296,21 +296,22 @@ SPI_DEVICE *
>  EFIAPI
>  MvSpiSetupSlave (
>    IN MARVELL_SPI_MASTER_PROTOCOL *This,
> +  IN SPI_DEVICE *Slave,
>    IN UINTN Cs,
>    IN SPI_MODE Mode
>    )
>  {
> -  SPI_DEVICE  *Slave;
> +  if (!Slave) {
> +    Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
> +    if (Slave == NULL) {
> +      DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
> +      return NULL;
> +    }
>  
> -  Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
> -  if (Slave == NULL) {
> -    DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
> -    return NULL;
> +    Slave->Cs   = Cs;
> +    Slave->Mode = Mode;
>    }
>  
> -  Slave->Cs   = Cs;
> -  Slave->Mode = Mode;
> -
>    SpiSetupTransfer (This, Slave);
>  
>    return Slave;
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> index 1401f62..e7e280a 100644
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> @@ -125,6 +125,7 @@ SPI_DEVICE *
>  EFIAPI
>  MvSpiSetupSlave (
>    IN MARVELL_SPI_MASTER_PROTOCOL     * This,
> +  IN SPI_DEVICE *Slave,
>    IN UINTN Cs,
>    IN SPI_MODE Mode
>    );
> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
> index 93a8ec0..0cf7914 100644
> --- a/Platform/Marvell/Include/Protocol/Spi.h
> +++ b/Platform/Marvell/Include/Protocol/Spi.h
> @@ -87,6 +87,7 @@ typedef
>  SPI_DEVICE *
>  (EFIAPI *MV_SPI_SETUP_DEVICE) (
>    IN MARVELL_SPI_MASTER_PROTOCOL *This,
> +  IN SPI_DEVICE *Slave,
>    IN UINTN    Cs,
>    IN SPI_MODE Mode
>    );
> -- 
> 2.7.4
> 

Re: [platforms: PATCH 5/6] Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion

[Leif Lindholm]

On Tue, Oct 31, 2017 at 04:59:34AM +0100, Marcin Wojtas wrote:
> Spansion SPI flash devices use different command for bank
> selection. Update it, basing on the first byte of flash ID.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 5 +++++
>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> index 703994c..a00fc305 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> @@ -150,6 +150,11 @@ SpiFlashCmdBankaddrWrite (
>  {
>    UINT8 Cmd = CMD_BANK_WRITE;
>  
> +  /* Update bank selection command for Spansion */
> +  if (Slave->Info->Id[0] == SPI_FLASH_MFR_SPANSION) {
> +    Cmd = CMD_BANKADDR_BRWR;
> +  }
> +
>    MvSpiFlashWriteCommon (Slave, &Cmd, 1, &BankSel, 1);
>  }
>  
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> index 2583484..00af188 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> @@ -57,6 +57,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #define CMD_READ_ARRAY_FAST             0x0b
>  #define CMD_PAGE_PROGRAM                0x02
>  #define CMD_BANK_WRITE                  0xc5
> +#define CMD_BANKADDR_BRWR               0x17
>  #define CMD_ERASE_4K                    0x20
>  #define CMD_ERASE_32K                   0x52
>  #define CMD_ERASE_64K                   0xd8
> @@ -72,6 +73,9 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
>  #define SPI_FLASH_16MB_BOUN             0x1000000
>  
> +/* Manufacturer ID's */
> +#define SPI_FLASH_MFR_SPANSION          0x01
> +

Please move this definition to NorFlashInfoLib.
Otherwise this patch looks OK.

/
    Leif

>  typedef enum {
>    SPI_FLASH_READ_ID,
>    SPI_FLASH_READ, // Read from SPI flash with address
> -- 
> 2.7.4
> 

Re: [platforms: PATCH 6/6] Marvell/Drivers: MvSpiDxe: Keep data in SPI_DEVICE structure

[Leif Lindholm]

On Tue, Oct 31, 2017 at 04:59:35AM +0100, Marcin Wojtas wrote:
> In the MvSpiDxe driver obtaining host register base address,
> controller clock and device maximum frequency directly from PCDs
> was done all over the code. This patch cleans up the parameters'
> handling and enables accessing them from SPI_DEVICE structure fields.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 48 ++++++++++++--------
>  Platform/Marvell/Include/Protocol/Spi.h |  2 +
>  2 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> index a7db5f2..c60a520 100755
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> @@ -38,12 +38,13 @@ SPI_MASTER *mSpiMasterInstance;
>  STATIC
>  EFI_STATUS
>  SpiSetBaudRate (
> +  IN SPI_DEVICE *Slave,
>    IN UINT32 CpuClock,
>    IN UINT32 MaxFreq
>    )
>  {
>    UINT32 Spr, BestSpr, Sppr, BestSppr, ClockDivider, Match, Reg, MinBaudDiff;
> -  UINTN SpiRegBase = PcdGet32 (PcdSpiRegBase);
> +  UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
>  
>    MinBaudDiff = 0xFFFFFFFF;
>    BestSppr = 0;
> @@ -93,26 +94,28 @@ SpiSetBaudRate (
>  STATIC
>  VOID
>  SpiSetCs (
> -  UINT8 CsId
> +  IN SPI_DEVICE *Slave
>    )
>  {
> -  UINT32 Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
> +  UINT32 Reg;
> +  UINTN  SpiRegBase = Slave->HostRegisterBaseAddress;
>  
>    Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
>    Reg &= ~SPI_CS_NUM_MASK;
> -  Reg |= (CsId << SPI_CS_NUM_OFFSET);
> +  Reg |= (Slave->Cs << SPI_CS_NUM_OFFSET);
>    MmioWrite32 (SpiRegBase + SPI_CTRL_REG, Reg);
>  }
>  
>  STATIC
>  VOID
>  SpiActivateCs (
> -  UINT8 IN CsId
> +  IN SPI_DEVICE *Slave
>    )
>  {
> -  UINT32  Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
> +  UINT32 Reg;
> +  UINTN  SpiRegBase = Slave->HostRegisterBaseAddress;
>  
> -  SpiSetCs(CsId);
> +  SpiSetCs(Slave);
>    Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
>    Reg |= SPI_CS_EN_MASK;
>    MmioWrite32(SpiRegBase + SPI_CTRL_REG, Reg);
> @@ -121,10 +124,11 @@ SpiActivateCs (
>  STATIC
>  VOID
>  SpiDeactivateCs (
> -  VOID
> +  IN SPI_DEVICE *Slave
>    )
>  {
> -  UINT32  Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
> +  UINT32 Reg;
> +  UINTN  SpiRegBase = Slave->HostRegisterBaseAddress;
>  
>    Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
>    Reg &= ~SPI_CS_EN_MASK;
> @@ -139,14 +143,15 @@ SpiSetupTransfer (
>    )
>  {
>    SPI_MASTER *SpiMaster;
> -  UINT32 Reg, SpiRegBase, CoreClock, SpiMaxFreq;
> +  UINT32 Reg, CoreClock, SpiMaxFreq;
> +  UINTN SpiRegBase;
>  
>    SpiMaster = SPI_MASTER_FROM_SPI_MASTER_PROTOCOL (This);
>  
>    // Initialize values from PCDs
> -  SpiRegBase  = PcdGet32 (PcdSpiRegBase);
> -  CoreClock   = PcdGet32 (PcdSpiClockFrequency);
> -  SpiMaxFreq  = PcdGet32 (PcdSpiMaxFrequency);
> +  SpiRegBase  = Slave->HostRegisterBaseAddress;
> +  CoreClock   = Slave->CoreClock;
> +  SpiMaxFreq  = Slave->MaxFreq;
>  
>    EfiAcquireLock (&SpiMaster->Lock);
>  
> @@ -154,9 +159,9 @@ SpiSetupTransfer (
>    Reg |= SPI_BYTE_LENGTH;
>    MmioWrite32 (SpiRegBase + SPI_CONF_REG, Reg);
>  
> -  SpiSetCs(Slave->Cs);
> +  SpiSetCs(Slave);
>  
> -  SpiSetBaudRate (CoreClock, SpiMaxFreq);
> +  SpiSetBaudRate (Slave, CoreClock, SpiMaxFreq);
>  
>    Reg = MmioRead32 (SpiRegBase + SPI_CONF_REG);
>    Reg &= ~(SPI_CPOL_MASK | SPI_CPHA_MASK | SPI_TXLSBF_MASK | SPI_RXLSBF_MASK);
> @@ -194,21 +199,22 @@ MvSpiTransfer (
>  {
>    SPI_MASTER *SpiMaster;
>    UINT64  Length;
> -  UINT32  Iterator, Reg, SpiRegBase;
> +  UINT32  Iterator, Reg;
>    UINT8   *DataOutPtr = (UINT8 *)DataOut;
>    UINT8   *DataInPtr  = (UINT8 *)DataIn;
>    UINT8   DataToSend  = 0;
> +  UINTN   SpiRegBase;
>  
>    SpiMaster = SPI_MASTER_FROM_SPI_MASTER_PROTOCOL (This);
>  
> -  SpiRegBase = PcdGet32 (PcdSpiRegBase);
> +  SpiRegBase = Slave->HostRegisterBaseAddress;
>  
>    Length = 8 * DataByteCount;
>  
>    EfiAcquireLock (&SpiMaster->Lock);
>  
>    if (Flag & SPI_TRANSFER_BEGIN) {
> -    SpiActivateCs (Slave->Cs);
> +    SpiActivateCs (Slave);
>    }
>  
>    // Set 8-bit mode
> @@ -245,7 +251,7 @@ MvSpiTransfer (
>    }
>  
>    if (Flag & SPI_TRANSFER_END) {
> -    SpiDeactivateCs ();
> +    SpiDeactivateCs (Slave);
>    }
>  
>    EfiReleaseLock (&SpiMaster->Lock);
> @@ -312,6 +318,10 @@ MvSpiSetupSlave (
>      Slave->Mode = Mode;
>    }
>  
> +  Slave->HostRegisterBaseAddress = PcdGet32 (PcdSpiRegBase);
> +  Slave->CoreClock = PcdGet32 (PcdSpiClockFrequency);
> +  Slave->MaxFreq = PcdGet32 (PcdSpiMaxFrequency);
> +
>    SpiSetupTransfer (This, Slave);
>  
>    return Slave;
> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
> index 0cf7914..b8981f3 100644
> --- a/Platform/Marvell/Include/Protocol/Spi.h
> +++ b/Platform/Marvell/Include/Protocol/Spi.h
> @@ -52,6 +52,8 @@ typedef struct {
>    INTN MaxFreq;
>    SPI_MODE Mode;
>    NOR_FLASH_INFO *Info;
> +  UINTN HostRegisterBaseAddress;
> +  UINTN CoreClock;
>  } SPI_DEVICE;
>  
>  typedef
> -- 
> 2.7.4
> 

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

[Marcin Wojtas]

2017-11-01 4:14 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 31, 2017 at 10:22:50AM +0100, Marcin Wojtas wrote:
>> 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).
>
> Right, OK, that sort of thing is useful to mention in the cover
> letter.
>
> But there's also MAX_ID_LEN, which is added here only to allocate
> buffers that are hard-coded to always contain value of ID_DEFAULT_LEN.
> Surely this could just be left out?
>

The id can be either 3 or 5 bytes in the extended variant, which is
used by some flash devices. In theory we can define such pcd and
PcdGetSize ensures flexibility. However with the next commit and
NorFlashInfoLib table, MAX_ID_LEN is suitable and used with new
implementation. In the new version of commit I already removed
ID_DEFAULT_LEN.

Marcin

Re: [platforms: PATCH 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection

[Marcin Wojtas]

Leif,

2017-11-01 4:25 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 31, 2017 at 04:59:31AM +0100, Marcin Wojtas wrote:
>> Hitherto mechanism of fixing SPI flash model in the PCDs,
>> occured to be very inefficient and problematic. Enable
>> dynamic detection by reworking MvSpiFlashReadId() command,
>> which now uses newly added NorFlashInfoLib, that helps to
>> obtain description of the JEDEC compliant devices.
>>
>> This patch updates the MvSpiFlashProtocol ReadId() protocol
>> callback on both producer's (MvFlashDxe) and consumers' sides
>> (FirmwareUpdate and SpiTool applications). Because all
>> information about detected SPI NOR flash is now stored in
>> the obtained NorFlashInfo structure fields, use them instead
>> of the PCDs.
>>
>> Enable compilation of the NorFlashInfoLib and update
>> PortingGuide documentation accordingly.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c   |  5 +-
>>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf |  4 +-
>>  Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c      |  5 +-
>>  Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf    |  2 +-
>>  Platform/Marvell/Armada/Armada.dsc.inc                   |  1 +
>>  Platform/Marvell/Armada/Armada70x0.dsc                   |  5 --
>>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c        | 68 +++++++++++---------
>>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf      |  9 +--
>>  Platform/Marvell/Drivers/Spi/MvSpiDxe.inf                |  2 +
>>  Platform/Marvell/Include/Protocol/Spi.h                  |  3 +
>>  Platform/Marvell/Include/Protocol/SpiFlash.h             |  6 +-
>>  Platform/Marvell/Marvell.dec                             |  6 --
>>  Silicon/Marvell/Documentation/PortingGuide.txt           | 18 ------
>>  13 files changed, 51 insertions(+), 83 deletions(-)
>>
>> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> index d70645d..750e52a 100644
>> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> @@ -94,12 +94,9 @@ SpiFlashProbe (
>>    )
>>  {
>>    EFI_STATUS       Status;
>> -  UINT8           *FlashId;
>> -
>> -  FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
>>
>>    // Read SPI flash ID
>> -  Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
>> +  Status = SpiFlashProtocol->ReadId (Slave, FALSE);
>>    if (EFI_ERROR (Status)) {
>>      return SHELL_ABORTED;
>>    }
>> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
>> index 92c3333..53ea491 100644
>> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
>> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
>> @@ -44,6 +44,7 @@
>>    FUpdate.uni
>>
>>  [Packages]
>> +  EmbeddedPkg/EmbeddedPkg.dec
>>    MdeModulePkg/MdeModulePkg.dec
>>    MdePkg/MdePkg.dec
>>    Platform/Marvell/Marvell.dec
>> @@ -64,9 +65,6 @@
>>    UefiLib
>>    UefiRuntimeServicesTableLib
>>
>> -[Pcd]
>> - gMarvellTokenSpaceGuid.PcdSpiFlashId
>> -
>>  [Protocols]
>>   gMarvellSpiFlashProtocolGuid
>>   gMarvellSpiMasterProtocolGuid
>> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> index a12f2ec..68a6cf7 100644
>> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> @@ -166,11 +166,8 @@ FlashProbe (
>>    )
>>  {
>>    EFI_STATUS Status;
>> -  UINT8      *FlashId;
>>
>> -  FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
>> -
>> -  Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
>> +  Status = SpiFlashProtocol->ReadId (Slave, FALSE);
>>    if (EFI_ERROR (Status)) {
>>      return SHELL_ABORTED;
>>    }
>> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
>> index 887b9a5..a52906b 100644
>> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
>> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
>> @@ -44,6 +44,7 @@
>>   SpiFlashCmd.uni
>>
>>  [Packages]
>> + EmbeddedPkg/EmbeddedPkg.dec
>>   MdePkg/MdePkg.dec
>>   ShellPkg/ShellPkg.dec
>>   MdeModulePkg/MdeModulePkg.dec
>> @@ -66,7 +67,6 @@
>>
>>  [Pcd]
>>   gMarvellTokenSpaceGuid.PcdSpiFlashCs
>> - gMarvellTokenSpaceGuid.PcdSpiFlashId
>>   gMarvellTokenSpaceGuid.PcdSpiFlashMode
>>
>>  [Protocols]
>> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
>> index b9fc384..2cd96e6 100644
>> --- a/Platform/Marvell/Armada/Armada.dsc.inc
>> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
>> @@ -33,6 +33,7 @@
>>    ArmPlatformLib|Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
>>    ComPhyLib|Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
>>    MppLib|Platform/Marvell/Library/MppLib/MppLib.inf
>> +  NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
>>    UtmiPhyLib|Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
>>
>>    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
>> index 4d5f55f..8e4cdb2 100644
>> --- a/Platform/Marvell/Armada/Armada70x0.dsc
>> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
>> @@ -90,11 +90,6 @@
>>    gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|10000000
>>    gMarvellTokenSpaceGuid.PcdSpiClockFrequency|200000000
>>
>> -  gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd|0x70
>> -  gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|3
>> -  gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|65536
>> -  gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|256
>> -  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 ab3ed6a..703994c 100755
>> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> @@ -107,10 +107,10 @@ MvSpiFlashWriteCommon (
>>    UINT8 PollBit = STATUS_REG_POLL_WIP;
>>    UINT8 CheckStatus = 0x0;
>>
>> -  CmdStatus = (UINT8)PcdGet32 (PcdSpiFlashPollCmd);
>> -  if (CmdStatus == CMD_FLAG_STATUS) {
>> +  if (Slave->Info->Flags & NF_WRITE_FSR) {
>> +    CmdStatus = CMD_FLAG_STATUS;
>>      PollBit = STATUS_REG_POLL_PEC;
>> -    CheckStatus = PollBit;
>> +    CheckStatus = STATUS_REG_POLL_PEC;
>>    }
>>
>>    // Send command
>> @@ -181,8 +181,19 @@ MvSpiFlashErase (
>>    UINTN EraseSize;
>>    UINT8 Cmd[5];
>>
>> -  AddrSize  = PcdGet32 (PcdSpiFlashAddressCycles);
>> -  EraseSize = PcdGet64 (PcdSpiFlashEraseSize);
>> +  if (Slave->Info->Flags & NF_4B_ADDR) {
>> +    AddrSize = 4;
>> +  } else {
>> +    AddrSize = 3;
>> +  }
>> +
>> +  if (Slave->Info->Flags & NF_ERASE_4K) {
>> +    Cmd[0] = CMD_ERASE_4K;
>> +    EraseSize = SIZE_4KB;
>> +  } else {
>
> Are 4 and 64K the only legal sizes?
> This patch deletes a detection of 32K size and the error message (and
> return) if an unknown size is encountered.

I didn't find 32K flash in Linux and U-Boot, maybe there some exotic
models, which use it. Anyway, there is no size check, as there are two
valid options now - 64KB and 4KB size, which is determined by
Info->Flag. Previously it was explicit EraseSize value, so the check
for wrong value was needed. Although unused, I can add NF_ERASE_32K
flag to NorFlashInfoLib and extend a check here.

>
>> +    Cmd[0] = CMD_ERASE_64K;
>> +    EraseSize = Slave->Info->SectorSize;
>> +  }
>>
>>    // Check input parameters
>>    if (Offset % EraseSize || Length % EraseSize) {
>> @@ -191,21 +202,6 @@ MvSpiFlashErase (
>>      return EFI_DEVICE_ERROR;
>>    }
>>
>> -  switch (EraseSize) {
>> -  case SIZE_4KB:
>> -    Cmd[0] = CMD_ERASE_4K;
>> -    break;
>> -  case SIZE_32KB:
>> -    Cmd[0] = CMD_ERASE_32K;
>> -    break;
>> -  case SIZE_64KB:
>> -    Cmd[0] = CMD_ERASE_64K;
>> -    break;
>> -  default:
>> -    DEBUG ((DEBUG_ERROR, "MvSpiFlash: Invalid EraseSize parameter\n"));
>> -    return EFI_INVALID_PARAMETER;
>> -  }
>> -
>>    while (Length) {
>>      EraseAddr = Offset;
>>
>> @@ -239,7 +235,11 @@ MvSpiFlashRead (
>>    UINT32 AddrSize, ReadAddr, ReadLength, RemainLength;
>>    UINTN BankSel = 0;
>>
>> -  AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
>> +  if (Slave->Info->Flags & NF_4B_ADDR) {
>> +    AddrSize = 4;
>> +  } else {
>> +    AddrSize = 3;
>> +  }
>
> I see this sequence repeated many times.
> Could it be replaced by a macro or static helper function? Or cached
> in a device structure?
>

Ok.

Thanks,
Marcin

Re: [platforms: PATCH 3/6] Marvell/Drivers: MvSpiFlash: Remove duplicated macros

[Marcin Wojtas]

2017-11-01 4:27 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 31, 2017 at 04:59:32AM +0100, Marcin Wojtas wrote:
>> Flash commands macros are already defined locally, so
>> remove them from the protocol header.
>
> Locally?
> I have no issue with the patch, but the commit message can be a bit
> more descriptive.
>

In a local header. I'll improve the message.

Marcin

> /
>     Leif
>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Include/Protocol/SpiFlash.h | 11 -----------
>>  1 file changed, 11 deletions(-)
>>
>> diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h
>> index 4a3053e..4ba29ba 100644
>> --- a/Platform/Marvell/Include/Protocol/SpiFlash.h
>> +++ b/Platform/Marvell/Include/Protocol/SpiFlash.h
>> @@ -36,17 +36,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>
>>  #include <Protocol/Spi.h>
>>
>> -#define CMD_READ_ID                     0x9f
>> -#define READ_STATUS_REG_CMD             0x0b
>> -#define CMD_WRITE_ENABLE                0x06
>> -#define CMD_FLAG_STATUS                 0x70
>> -#define CMD_WRITE_STATUS_REG            0x01
>> -#define CMD_READ_ARRAY_FAST             0x0b
>> -#define CMD_PAGE_PROGRAM                0x02
>> -#define CMD_BANK_WRITE                  0xc5
>> -#define CMD_ERASE_64K                   0xd8
>> -#define CMD_4B_ADDR_ENABLE              0xb7
>> -
>>  extern EFI_GUID gMarvellSpiFlashProtocolGuid;
>>
>>  typedef struct _MARVELL_SPI_FLASH_PROTOCOL MARVELL_SPI_FLASH_PROTOCOL;
>> --
>> 2.7.4
>>

Re: [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device

[Marcin Wojtas]

Leif,

2017-11-01 4:45 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 31, 2017 at 04:59:33AM +0100, Marcin Wojtas wrote:
>> Current usage of sf command requires running 'sf probe' prior to
>> executing any other option. Because it is done in two separate steps,
>> it turned out that SpiMasterProtocol->SetupDevice could easily
>> overwrite valid Slave pointer when performing second operation.
>>
>> Fix the issue by allocating Slave device only once and keep it
>> as global variable in the SpiTool application. This patch
>> also updates FirmwareUpdate command to follow the modified
>> SetupDevice operation.
>
> Really an unrelated question, but would we not expect to use capsule
> updates instead now? Do we have other uses for this tool?

I use it massively for the development. Since the variables work now,
I have a plan to implement capsule update, but it's definitely not at
the top of the list now. I'm also wondering if there will be no
problems with the binaries concatenation and their handling by the
bootrom. BTW. Is capsule update supposed to replace only UEFI part or
the entire image stored in flash?

>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c |  4 ++--
>>  Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c    |  8 ++++----
>>  Platform/Marvell/Drivers/Spi/MvSpiDxe.c                | 17 +++++++++--------
>>  Platform/Marvell/Drivers/Spi/MvSpiDxe.h                |  1 +
>>  Platform/Marvell/Include/Protocol/Spi.h                |  1 +
>>  5 files changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> index 750e52a..9ccb1c7 100644
>> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> @@ -240,7 +240,7 @@ ShellCommandRunFUpdate (
>>    )
>>  {
>>    IN SHELL_FILE_HANDLE    FileHandle;
>> -  SPI_DEVICE              *Slave;
>> +  SPI_DEVICE              *Slave = NULL;
>>    UINT64                  FileSize;
>>    UINTN                   *FileBuffer = NULL;
>>    CHAR16                  *ProblemParam;
>> @@ -302,7 +302,7 @@ ShellCommandRunFUpdate (
>>    }
>>
>>    // Setup and probe SPI flash
>> -  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, 0, 0);
>> +  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, 0, 0);
>>    if (Slave == NULL) {
>>      Print(L"%s: Cannot allocate SPI device!\n", CMD_NAME_STRING);
>>      goto HeaderError;
>> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> index 68a6cf7..1084f68 100644
>> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> @@ -191,7 +191,7 @@ ShellCommandRunSpiFlash (
>>    )
>>  {
>>  EFI_STATUS              Status;
>> -  SPI_DEVICE            *Slave;
>> +  STATIC SPI_DEVICE     *Slave;
>
> If this is a safe thing to do, please use a global variable called
> mSlave instead, to make it clear that it persists across calls.

Right, this way it will be more clear.

>
>>    LIST_ENTRY            *CheckPackage;
>>    EFI_PHYSICAL_ADDRESS  Address = 0, Offset = 0;
>>    SHELL_FILE_HANDLE     FileHandle = NULL;
>> @@ -273,7 +273,7 @@ EFI_STATUS              Status;
>>    Cs = PcdGet32 (PcdSpiFlashCs);
>>
>>    // Setup new spi device
>> -  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Cs, Mode);
>> +  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, Cs, Mode);
>>      if (Slave == NULL) {
>>        Print(L"sf: Cannot allocate SPI device!\n");
>>        return SHELL_ABORTED;
>> @@ -285,6 +285,8 @@ EFI_STATUS              Status;
>>      Status = FlashProbe (Slave);
>>      if (EFI_ERROR(Status)) {
>>        // No supported spi flash detected
>> +      SpiMasterProtocol->FreeDevice(Slave);
>
> Space before (.

Ok.

Thanks,
Marcin

>
> /
>     Leif
>
>> +      Slave = NULL;
>>        return SHELL_ABORTED;
>>      } else {
>>        return Status;
>> @@ -426,8 +428,6 @@ EFI_STATUS              Status;
>>      break;
>>    }
>>
>> -  SpiMasterProtocol->FreeDevice(Slave);
>> -
>>    if (EFI_ERROR (Status)) {
>>      Print (L"sf: Error while performing spi transfer\n");
>>      return SHELL_ABORTED;
>> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>> index 3b49147..a7db5f2 100755
>> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>> @@ -296,21 +296,22 @@ SPI_DEVICE *
>>  EFIAPI
>>  MvSpiSetupSlave (
>>    IN MARVELL_SPI_MASTER_PROTOCOL *This,
>> +  IN SPI_DEVICE *Slave,
>>    IN UINTN Cs,
>>    IN SPI_MODE Mode
>>    )
>>  {
>> -  SPI_DEVICE  *Slave;
>> +  if (!Slave) {
>> +    Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
>> +    if (Slave == NULL) {
>> +      DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
>> +      return NULL;
>> +    }
>>
>> -  Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
>> -  if (Slave == NULL) {
>> -    DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
>> -    return NULL;
>> +    Slave->Cs   = Cs;
>> +    Slave->Mode = Mode;
>>    }
>>
>> -  Slave->Cs   = Cs;
>> -  Slave->Mode = Mode;
>> -
>>    SpiSetupTransfer (This, Slave);
>>
>>    return Slave;
>> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>> index 1401f62..e7e280a 100644
>> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>> @@ -125,6 +125,7 @@ SPI_DEVICE *
>>  EFIAPI
>>  MvSpiSetupSlave (
>>    IN MARVELL_SPI_MASTER_PROTOCOL     * This,
>> +  IN SPI_DEVICE *Slave,
>>    IN UINTN Cs,
>>    IN SPI_MODE Mode
>>    );
>> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
>> index 93a8ec0..0cf7914 100644
>> --- a/Platform/Marvell/Include/Protocol/Spi.h
>> +++ b/Platform/Marvell/Include/Protocol/Spi.h
>> @@ -87,6 +87,7 @@ typedef
>>  SPI_DEVICE *
>>  (EFIAPI *MV_SPI_SETUP_DEVICE) (
>>    IN MARVELL_SPI_MASTER_PROTOCOL *This,
>> +  IN SPI_DEVICE *Slave,
>>    IN UINTN    Cs,
>>    IN SPI_MODE Mode
>>    );
>> --
>> 2.7.4
>>

Re: [platforms: PATCH 5/6] Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion

[Marcin Wojtas]

2017-11-01 4:50 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 31, 2017 at 04:59:34AM +0100, Marcin Wojtas wrote:
>> Spansion SPI flash devices use different command for bank
>> selection. Update it, basing on the first byte of flash ID.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 5 +++++
>>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h | 4 ++++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> index 703994c..a00fc305 100755
>> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> @@ -150,6 +150,11 @@ SpiFlashCmdBankaddrWrite (
>>  {
>>    UINT8 Cmd = CMD_BANK_WRITE;
>>
>> +  /* Update bank selection command for Spansion */
>> +  if (Slave->Info->Id[0] == SPI_FLASH_MFR_SPANSION) {
>> +    Cmd = CMD_BANKADDR_BRWR;
>> +  }
>> +
>>    MvSpiFlashWriteCommon (Slave, &Cmd, 1, &BankSel, 1);
>>  }
>>
>> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
>> index 2583484..00af188 100755
>> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
>> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
>> @@ -57,6 +57,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>  #define CMD_READ_ARRAY_FAST             0x0b
>>  #define CMD_PAGE_PROGRAM                0x02
>>  #define CMD_BANK_WRITE                  0xc5
>> +#define CMD_BANKADDR_BRWR               0x17
>>  #define CMD_ERASE_4K                    0x20
>>  #define CMD_ERASE_32K                   0x52
>>  #define CMD_ERASE_64K                   0xd8
>> @@ -72,6 +73,9 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>
>>  #define SPI_FLASH_16MB_BOUN             0x1000000
>>
>> +/* Manufacturer ID's */
>> +#define SPI_FLASH_MFR_SPANSION          0x01
>> +
>
> Please move this definition to NorFlashInfoLib.
> Otherwise this patch looks OK.
>

Ok.

Thanks,
Marcin

Re: [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device

[Ard Biesheuvel]

On 1 November 2017 at 16:40, Marcin Wojtas <mw@semihalf.com> wrote:
> Leif,
>
> 2017-11-01 4:45 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> On Tue, Oct 31, 2017 at 04:59:33AM +0100, Marcin Wojtas wrote:
>>> Current usage of sf command requires running 'sf probe' prior to
>>> executing any other option. Because it is done in two separate steps,
>>> it turned out that SpiMasterProtocol->SetupDevice could easily
>>> overwrite valid Slave pointer when performing second operation.
>>>
>>> Fix the issue by allocating Slave device only once and keep it
>>> as global variable in the SpiTool application. This patch
>>> also updates FirmwareUpdate command to follow the modified
>>> SetupDevice operation.
>>
>> Really an unrelated question, but would we not expect to use capsule
>> updates instead now? Do we have other uses for this tool?
>
> I use it massively for the development. Since the variables work now,
> I have a plan to implement capsule update, but it's definitely not at
> the top of the list now. I'm also wondering if there will be no
> problems with the binaries concatenation and their handling by the
> bootrom. BTW. Is capsule update supposed to replace only UEFI part or
> the entire image stored in flash?
>

That depends on your use case, but you can easily update your .fdf to
incorporate other binaries into the UEFI build (ARM-TF etc), and you
can update everything using a single capsule. That also allows you to
use the standard 'fwupdate' tool in Linux, and even allows you to host
firmware updates at fwupd.org, and they can be installed automatically
by the ordinary Software Update GUI that runs in the OS.

>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> ---
>>>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c |  4 ++--
>>>  Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c    |  8 ++++----
>>>  Platform/Marvell/Drivers/Spi/MvSpiDxe.c                | 17 +++++++++--------
>>>  Platform/Marvell/Drivers/Spi/MvSpiDxe.h                |  1 +
>>>  Platform/Marvell/Include/Protocol/Spi.h                |  1 +
>>>  5 files changed, 17 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>>> index 750e52a..9ccb1c7 100644
>>> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>>> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>>> @@ -240,7 +240,7 @@ ShellCommandRunFUpdate (
>>>    )
>>>  {
>>>    IN SHELL_FILE_HANDLE    FileHandle;
>>> -  SPI_DEVICE              *Slave;
>>> +  SPI_DEVICE              *Slave = NULL;
>>>    UINT64                  FileSize;
>>>    UINTN                   *FileBuffer = NULL;
>>>    CHAR16                  *ProblemParam;
>>> @@ -302,7 +302,7 @@ ShellCommandRunFUpdate (
>>>    }
>>>
>>>    // Setup and probe SPI flash
>>> -  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, 0, 0);
>>> +  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, 0, 0);
>>>    if (Slave == NULL) {
>>>      Print(L"%s: Cannot allocate SPI device!\n", CMD_NAME_STRING);
>>>      goto HeaderError;
>>> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>>> index 68a6cf7..1084f68 100644
>>> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>>> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>>> @@ -191,7 +191,7 @@ ShellCommandRunSpiFlash (
>>>    )
>>>  {
>>>  EFI_STATUS              Status;
>>> -  SPI_DEVICE            *Slave;
>>> +  STATIC SPI_DEVICE     *Slave;
>>
>> If this is a safe thing to do, please use a global variable called
>> mSlave instead, to make it clear that it persists across calls.
>
> Right, this way it will be more clear.
>
>>
>>>    LIST_ENTRY            *CheckPackage;
>>>    EFI_PHYSICAL_ADDRESS  Address = 0, Offset = 0;
>>>    SHELL_FILE_HANDLE     FileHandle = NULL;
>>> @@ -273,7 +273,7 @@ EFI_STATUS              Status;
>>>    Cs = PcdGet32 (PcdSpiFlashCs);
>>>
>>>    // Setup new spi device
>>> -  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Cs, Mode);
>>> +  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, Cs, Mode);
>>>      if (Slave == NULL) {
>>>        Print(L"sf: Cannot allocate SPI device!\n");
>>>        return SHELL_ABORTED;
>>> @@ -285,6 +285,8 @@ EFI_STATUS              Status;
>>>      Status = FlashProbe (Slave);
>>>      if (EFI_ERROR(Status)) {
>>>        // No supported spi flash detected
>>> +      SpiMasterProtocol->FreeDevice(Slave);
>>
>> Space before (.
>
> Ok.
>
> Thanks,
> Marcin
>
>>
>> /
>>     Leif
>>
>>> +      Slave = NULL;
>>>        return SHELL_ABORTED;
>>>      } else {
>>>        return Status;
>>> @@ -426,8 +428,6 @@ EFI_STATUS              Status;
>>>      break;
>>>    }
>>>
>>> -  SpiMasterProtocol->FreeDevice(Slave);
>>> -
>>>    if (EFI_ERROR (Status)) {
>>>      Print (L"sf: Error while performing spi transfer\n");
>>>      return SHELL_ABORTED;
>>> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>>> index 3b49147..a7db5f2 100755
>>> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>>> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>>> @@ -296,21 +296,22 @@ SPI_DEVICE *
>>>  EFIAPI
>>>  MvSpiSetupSlave (
>>>    IN MARVELL_SPI_MASTER_PROTOCOL *This,
>>> +  IN SPI_DEVICE *Slave,
>>>    IN UINTN Cs,
>>>    IN SPI_MODE Mode
>>>    )
>>>  {
>>> -  SPI_DEVICE  *Slave;
>>> +  if (!Slave) {
>>> +    Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
>>> +    if (Slave == NULL) {
>>> +      DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
>>> +      return NULL;
>>> +    }
>>>
>>> -  Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
>>> -  if (Slave == NULL) {
>>> -    DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
>>> -    return NULL;
>>> +    Slave->Cs   = Cs;
>>> +    Slave->Mode = Mode;
>>>    }
>>>
>>> -  Slave->Cs   = Cs;
>>> -  Slave->Mode = Mode;
>>> -
>>>    SpiSetupTransfer (This, Slave);
>>>
>>>    return Slave;
>>> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>>> index 1401f62..e7e280a 100644
>>> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>>> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>>> @@ -125,6 +125,7 @@ SPI_DEVICE *
>>>  EFIAPI
>>>  MvSpiSetupSlave (
>>>    IN MARVELL_SPI_MASTER_PROTOCOL     * This,
>>> +  IN SPI_DEVICE *Slave,
>>>    IN UINTN Cs,
>>>    IN SPI_MODE Mode
>>>    );
>>> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
>>> index 93a8ec0..0cf7914 100644
>>> --- a/Platform/Marvell/Include/Protocol/Spi.h
>>> +++ b/Platform/Marvell/Include/Protocol/Spi.h
>>> @@ -87,6 +87,7 @@ typedef
>>>  SPI_DEVICE *
>>>  (EFIAPI *MV_SPI_SETUP_DEVICE) (
>>>    IN MARVELL_SPI_MASTER_PROTOCOL *This,
>>> +  IN SPI_DEVICE *Slave,
>>>    IN UINTN    Cs,
>>>    IN SPI_MODE Mode
>>>    );
>>> --
>>> 2.7.4
>>>