From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4001:c06::244; helo=mail-io0-x244.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-io0-x244.google.com (mail-io0-x244.google.com [IPv6:2607:f8b0:4001:c06::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6B73620347175 for ; Tue, 31 Oct 2017 02:19:00 -0700 (PDT) Received: by mail-io0-x244.google.com with SMTP id m16so33528538iod.1 for ; Tue, 31 Oct 2017 02:22:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=w6E03hbjbp/nDMmm9jPTDWdfMXvZTwxGinMbdDtgWr4=; b=Myk2lHYVu6/WpMSi2CO9f0/rxVl/EGK9FWywLJPqBBLtZ+qAAnk1dLyN7xwwYYBOre B08f1UUd69uc383K9g33itkzulNP04QccROMfGrFmTMsoM05brw1j/sVxifia+72s4q3 bLqeXkZmoJZZFSOTfFTRgEQTgM0wOc1/O5XNE7bjPIS37cvgiJDVe9oW6EpdQa3051HB GCKbbgW5NxvqymArCnQJ5wcg7Kt2rOIQYtChPKyCcKEd2AHCxKklOEmwTDMA8Mgels4N RaL3t+07EBGL0EhpHtTVpVh+o/l77jDsVpQP7Vr1Hm68h+xVFiBGT6IfXhDU837RlZb1 ngEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=w6E03hbjbp/nDMmm9jPTDWdfMXvZTwxGinMbdDtgWr4=; b=p/+OHkdoO/GjP4GEQyHFXq6C8Uq0WwuEfqmZMAFYC18MQnVKFL/3PJwurpkYTsQsLx e+U70/xMlR1RCOAAprMibmP8GcsWXPRNCHy3eQXndSTXHeGaDnbfKYDskF+HEenN/3e1 QHnw6JkJl4lg+78d5JdkJ8B7gcGDL5j05d7FQ71xs95OgK67eJeRouzSoiC2NL1rfnk3 VkPwCQ4F3iIAHrFXDXmA/a5TovuTzgkRfNvn80btcdUghdXilOmVo5cU2s2asQ9G3IRV GcT54UliufWWM6OixJi7Y5XfPTWyaYeNPItsxFCOOAib3ZJKM3KnubpTjN4Rn8Tmuo2l n1Yw== X-Gm-Message-State: AMCzsaVucrjnlpFoeddJzqKks7aabeDuOk8P1kRidlrKq0qSGNCdP7Ux Ch4YG59IryGGdjBfAhAdDqdL4XeyF4hNvxIY0eT9Xg== X-Google-Smtp-Source: ABhQp+RFxNBSSsdunDs3yJentCGMDkSY02WdjDXy8cwYeixNMkC8AZRDC0BLtLuxMeHVR+rLc1PrsCP2TlXVnMBlb20= X-Received: by 10.36.110.71 with SMTP id w68mr2237481itc.113.1509441771178; Tue, 31 Oct 2017 02:22:51 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.167.208 with HTTP; Tue, 31 Oct 2017 02:22:50 -0700 (PDT) In-Reply-To: <20171031090717.xwbjaitebmcfdiue@bivouac.eciton.net> References: <1509422375-20198-1-git-send-email-mw@semihalf.com> <1509422375-20198-2-git-send-email-mw@semihalf.com> <20171031090717.xwbjaitebmcfdiue@bivouac.eciton.net> From: Marcin Wojtas Date: Tue, 31 Oct 2017 10:22:50 +0100 Message-ID: To: Leif Lindholm Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , semihalf-dabros-jan Subject: Re: [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 31 Oct 2017 09:19:00 -0000 Content-Type: text/plain; charset="UTF-8" 2017-10-31 10:07 GMT+01:00 Leif Lindholm : > 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 >> --- >> 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). Marcin > / > Leif > >> + if (EFI_ERROR (Status)) { >> + return SHELL_ABORTED; >> } >> >> - Print (L"%s: Detected supported SPI flash with ID=%3x\n", CMD_NAME_STRING, RefId); >> - >> Status = SpiFlashProtocol->Init (SpiFlashProtocol, Slave); >> if (EFI_ERROR(Status)) { >> Print (L"%s: Cannot initialize flash device\n", CMD_NAME_STRING); >> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c >> index 9321f6b..a12f2ec 100644 >> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c >> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c >> @@ -166,37 +166,24 @@ FlashProbe ( >> ) >> { >> EFI_STATUS Status; >> - UINT8 IdBuffer[4]; >> - UINT32 Id, RefId; >> + UINT8 *FlashId; >> >> - Id = PcdGet32 (PcdSpiFlashId); >> + FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId); >> >> - IdBuffer[0] = CMD_READ_ID; >> - >> - SpiFlashProtocol->ReadId ( >> - Slave, >> - 4, >> - IdBuffer >> - ); >> - >> - RefId = (IdBuffer[0] << 16) + (IdBuffer[1] << 8) + IdBuffer[2]; >> + Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId); >> + if (EFI_ERROR (Status)) { >> + return SHELL_ABORTED; >> + } >> >> - if (RefId == Id) { >> - Print (L"sf: Detected supported SPI flash with ID=%3x\n", RefId); >> - Status = SpiFlashProtocol->Init (SpiFlashProtocol, Slave); >> - if (EFI_ERROR(Status)) { >> - Print (L"sf: Cannot initialize flash device\n"); >> - return SHELL_ABORTED; >> - } >> - InitFlag = 0; >> - return EFI_SUCCESS; >> - } else if (RefId != 0) { >> - Print (L"sf: Unsupported SPI flash detected with ID=%2x\n", RefId); >> + Status = SpiFlashProtocol->Init (SpiFlashProtocol, Slave); >> + if (EFI_ERROR (Status)) { >> + Print (L"sf: Cannot initialize flash device\n"); >> return SHELL_ABORTED; >> } >> >> - Print (L"sf: No SPI flash detected"); >> - return SHELL_ABORTED; >> + InitFlag = 0; >> + >> + return SHELL_SUCCESS; >> } >> >> SHELL_STATUS >> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc >> index 0396e8e..4d5f55f 100644 >> --- a/Platform/Marvell/Armada/Armada70x0.dsc >> +++ b/Platform/Marvell/Armada/Armada70x0.dsc >> @@ -94,7 +94,7 @@ >> gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|3 >> gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|65536 >> gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|256 >> - gMarvellTokenSpaceGuid.PcdSpiFlashId|0x20BA18 >> + 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 6f7d9c7..ab3ed6a 100755 >> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c >> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c >> @@ -409,28 +409,35 @@ MvSpiFlashReadId ( >> ) >> { >> EFI_STATUS Status; >> - UINT8 *DataOut; >> + UINT8 Id[NOR_FLASH_MAX_ID_LEN]; >> + UINT8 Cmd; >> + >> + Cmd = CMD_READ_ID; >> + Status = SpiMasterProtocol->ReadWrite (SpiMasterProtocol, >> + SpiDev, >> + &Cmd, >> + SPI_CMD_LEN, >> + NULL, >> + Id, >> + NOR_FLASH_MAX_ID_LEN); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "ReadId: Spi transfer error\n")); >> + return Status; >> + } >> >> - DataOut = (UINT8 *) AllocateZeroPool (DataByteCount); >> - if (DataOut == NULL) { >> - DEBUG((DEBUG_ERROR, "SpiFlash: Cannot allocate memory\n")); >> - return EFI_OUT_OF_RESOURCES; >> + if (CompareMem (Id, Buffer, DataByteCount) != 0) { >> + Status = EFI_NOT_FOUND; >> } >> - Status = SpiMasterProtocol->Transfer (SpiMasterProtocol, SpiDev, >> - DataByteCount, Buffer, DataOut, SPI_TRANSFER_BEGIN | SPI_TRANSFER_END); >> - if (EFI_ERROR(Status)) { >> - FreePool (DataOut); >> - DEBUG((DEBUG_ERROR, "SpiFlash: Spi transfer error\n")); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, >> + "%a: Unrecognized JEDEC Id bytes: 0x%02x%02x%02x\n", >> + __FUNCTION__, >> + Id[0], >> + Id[1], >> + Id[2])); >> return Status; >> } >> >> - // Bytes 1,2 and 3 contain SPI flash ID >> - Buffer[0] = DataOut[1]; >> - Buffer[1] = DataOut[2]; >> - Buffer[2] = DataOut[3]; >> - >> - FreePool (DataOut); >> - >> return EFI_SUCCESS; >> } >> >> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h >> index f90abb7..2583484 100755 >> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h >> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h >> @@ -62,6 +62,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. >> #define CMD_ERASE_64K 0xd8 >> #define CMD_4B_ADDR_ENABLE 0xb7 >> >> +#define SPI_CMD_LEN 1 >> + >> #define STATUS_REG_POLL_WIP (1 << 0) >> #define STATUS_REG_POLL_PEC (1 << 7) >> >> diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h >> index 743bb87..e0d62cc 100644 >> --- a/Platform/Marvell/Include/Protocol/SpiFlash.h >> +++ b/Platform/Marvell/Include/Protocol/SpiFlash.h >> @@ -47,6 +47,9 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. >> #define CMD_ERASE_64K 0xd8 >> #define CMD_4B_ADDR_ENABLE 0xb7 >> >> +#define NOR_FLASH_MAX_ID_LEN 6 >> +#define NOR_FLASH_ID_DEFAULT_LEN 3 >> + >> extern EFI_GUID gMarvellSpiFlashProtocolGuid; >> >> typedef struct _MARVELL_SPI_FLASH_PROTOCOL MARVELL_SPI_FLASH_PROTOCOL; >> diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec >> index cd800c8..679a9d0 100644 >> --- a/Platform/Marvell/Marvell.dec >> +++ b/Platform/Marvell/Marvell.dec >> @@ -133,7 +133,7 @@ >> gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|0|UINT64|0x3000054 >> gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|0|UINT32|0x3000055 >> gMarvellTokenSpaceGuid.PcdSpiFlashSectorSize|65536|UINT64|0x3000059 >> - gMarvellTokenSpaceGuid.PcdSpiFlashId|0|UINT32|0x3000056 >> + gMarvellTokenSpaceGuid.PcdSpiFlashId|{ 0x0 }|VOID*|0x3000056 >> gMarvellTokenSpaceGuid.PcdSpiFlashCs|0|UINT32|0x3000057 >> gMarvellTokenSpaceGuid.PcdSpiFlashMode|0|UINT32|0x3000058 >> >> -- >> 2.7.4 >>