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

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

/
    Leif