Re: [platforms: PATCH 09/11] Drivers/Spi/Devices/MvSpiFlash: Fix usage of erase size parameter

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Sep 01, 2017 at 05:25:00PM +0200, Marcin Wojtas wrote:
> 2017-09-01 17:21 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Fri, Sep 01, 2017 at 03:08:21PM +0200, Marcin Wojtas wrote:
> >> Although, hitherto support allowed for using configurable EraseSize,
> >> the erase command was fixed to CMD_ERASE_64K. Also it was
> >> assumed that EraseSize equals SectorSize, which is not true
> >> for some flash devices.
> >
> > Another thing I immediately wish I had never learned.
> > Thanks :/
> 
> Do you mean the code or flash quirks? :)

The flash quirks...

/
    Leif

> >> Fix both issues by adding new PCD
> >> (gMarvellTokenSpaceGuid.PcdSpiFlashPageSize) and using
> >> this parameter properly in MvSpiFlashUpdate routine instead
> >> of the EraseSize. Also erase command is adjusted to the settings.
> >> Update PortingGuide accordingly.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >>  Platform/Marvell/Documentation/PortingGuide.txt    |  3 +++
> >>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c  | 26 +++++++++++++++++-----
> >>  Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h  |  6 +++++
> >>  .../Marvell/Drivers/Spi/Devices/MvSpiFlash.inf     |  1 +
> >>  Platform/Marvell/Marvell.dec                       |  1 +
> >>  5 files changed, 31 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/Platform/Marvell/Documentation/PortingGuide.txt b/Platform/Marvell/Documentation/PortingGuide.txt
> >> index 3b79bd2..f637fee 100644
> >> --- a/Platform/Marvell/Documentation/PortingGuide.txt
> >> +++ b/Platform/Marvell/Documentation/PortingGuide.txt
> >> @@ -297,6 +297,9 @@ Folowing PCDs for spi flash driver configuration must be set properly:
> >>    - gMarvellTokenSpaceGuid.PcdSpiFlashPageSize
> >>       (Size of SPI flash page)
> >>
> >> +  - gMarvellTokenSpaceGuid.PcdSpiFlashSectorSize
> >> +     (Size of SPI flash sector, 65536 bytes by default)
> >> +
> >>    - gMarvellTokenSpaceGuid.PcdSpiFlashId
> >>       (Id of SPI flash)
> >>
> >> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> >> index 9a04493..f3fdba4 100755
> >> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> >> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> >> @@ -191,7 +191,21 @@ MvSpiFlashErase (
> >>      return EFI_DEVICE_ERROR;
> >>    }
> >>
> >> -  Cmd[0] = CMD_ERASE_64K;
> >> +  switch (EraseSize) {
> >> +  case SPI_ERASE_SIZE_4K:
> >> +    Cmd[0] = CMD_ERASE_4K;
> >> +    break;
> >> +  case SPI_ERASE_SIZE_32K:
> >> +    Cmd[0] = CMD_ERASE_32K;
> >> +    break;
> >> +  case SPI_ERASE_SIZE_64K:
> >> +    Cmd[0] = CMD_ERASE_64K;
> >> +    break;
> >> +  default:
> >> +    DEBUG ((DEBUG_ERROR, "MvSpiFlash: Invalid EraseSize parameter\n"));
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +
> >>    while (Length) {
> >>      EraseAddr = Offset;
> >>
> >> @@ -353,14 +367,14 @@ MvSpiFlashUpdate (
> >>    )
> >>  {
> >>    EFI_STATUS Status;
> >> -  UINT64 EraseSize, ToUpdate, Scale = 1;
> >> +  UINT64 SectorSize, ToUpdate, Scale = 1;
> >>    UINT8 *TmpBuf, *End;
> >>
> >> -  EraseSize = PcdGet64 (PcdSpiFlashEraseSize);
> >> +  SectorSize = PcdGet64 (PcdSpiFlashSectorSize);
> >>
> >>    End = Buf + ByteCount;
> >>
> >> -  TmpBuf = (UINT8 *)AllocateZeroPool (EraseSize);
> >> +  TmpBuf = (UINT8 *)AllocateZeroPool (SectorSize);
> >>    if (TmpBuf == NULL) {
> >>      DEBUG((DEBUG_ERROR, "SpiFlash: Cannot allocate memory\n"));
> >>      return EFI_OUT_OF_RESOURCES;
> >> @@ -370,9 +384,9 @@ MvSpiFlashUpdate (
> >>      Scale = (End - Buf) / 100;
> >>
> >>    for (; Buf < End; Buf += ToUpdate, Offset += ToUpdate) {
> >> -    ToUpdate = MIN((UINT64)(End - Buf), EraseSize);
> >> +    ToUpdate = MIN((UINT64)(End - Buf), SectorSize);
> >>      Print (L"   \rUpdating, %d%%", 100 - (End - Buf) / Scale);
> >> -    Status = MvSpiFlashUpdateBlock (Slave, Offset, ToUpdate, Buf, TmpBuf, EraseSize);
> >> +    Status = MvSpiFlashUpdateBlock (Slave, Offset, ToUpdate, Buf, TmpBuf, SectorSize);
> >>
> >>      if (EFI_ERROR (Status)) {
> >>        DEBUG((DEBUG_ERROR, "SpiFlash: Error while updating\n"));
> >> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> >> index 3889643..646598a 100755
> >> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> >> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> >> @@ -57,6 +57,8 @@ 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_ERASE_4K                    0x20
> >> +#define CMD_ERASE_32K                   0x52
> >>  #define CMD_ERASE_64K                   0xd8
> >>  #define CMD_4B_ADDR_ENABLE              0xb7
> >>
> >> @@ -66,6 +68,10 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >>  #define SPI_TRANSFER_BEGIN              0x01  // Assert CS before transfer
> >>  #define SPI_TRANSFER_END                0x02  // Deassert CS after transfers
> >>
> >> +#define SPI_ERASE_SIZE_4K               4096
> >> +#define SPI_ERASE_SIZE_32K              32768
> >> +#define SPI_ERASE_SIZE_64K              65536
> >> +
> >
> > Maybe just replace these with SIZE_4KB, SIZE_32KB and SIZE_64KB?
> >
> 
> Sure, will do.
> 
> Marcin