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);
+ 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
/
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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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).
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
>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
2017-11-01 4:14 GMT+01:00 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?
>
The id can be either 3 or 5 bytes in the extended variant, which is
used by some flash devices. In theory we can define such pcd and
PcdGetSize ensures flexibility. However with the next commit and
NorFlashInfoLib table, MAX_ID_LEN is suitable and used with new
implementation. In the new version of commit I already removed
ID_DEFAULT_LEN.
Marcin
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.