On Tue, Oct 31, 2017 at 10:22:50AM +0100, Marcin Wojtas wrote:
> 2017-10-31 10:07 GMT+01:00 Leif Lindholm <leif.lindh...@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 <m...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
