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

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

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