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

[Marcin Wojtas <mw@semihalf.com>]

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