* [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId
2017-10-31 3:59 [platforms: PATCH 0/6] Armada 7k/8k SPI improvements pt 2 Marcin Wojtas
@ 2017-10-31 3:59 ` Marcin Wojtas
2017-10-31 9:07 ` Leif Lindholm
2017-10-31 3:59 ` [platforms: PATCH 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection Marcin Wojtas
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-31 3:59 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId
2017-10-31 3:59 ` [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId Marcin Wojtas
@ 2017-10-31 9:07 ` Leif Lindholm
2017-10-31 9:22 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-10-31 9:07 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId
2017-10-31 9:07 ` Leif Lindholm
@ 2017-10-31 9:22 ` Marcin Wojtas
2017-11-01 3:14 ` Leif Lindholm
0 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-31 9:22 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
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
>>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId
2017-10-31 9:22 ` Marcin Wojtas
@ 2017-11-01 3:14 ` Leif Lindholm
2017-11-01 16:28 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-11-01 3:14 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId
2017-11-01 3:14 ` Leif Lindholm
@ 2017-11-01 16:28 ` Marcin Wojtas
0 siblings, 0 replies; 21+ messages in thread
From: Marcin Wojtas @ 2017-11-01 16:28 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [platforms: PATCH 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection
2017-10-31 3:59 [platforms: PATCH 0/6] Armada 7k/8k SPI improvements pt 2 Marcin Wojtas
2017-10-31 3:59 ` [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId Marcin Wojtas
@ 2017-10-31 3:59 ` Marcin Wojtas
2017-11-01 3:25 ` Leif Lindholm
2017-10-31 3:59 ` [platforms: PATCH 3/6] Marvell/Drivers: MvSpiFlash: Remove duplicated macros Marcin Wojtas
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-31 3:59 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
Hitherto mechanism of fixing SPI flash model in the PCDs,
occured to be very inefficient and problematic. Enable
dynamic detection by reworking MvSpiFlashReadId() command,
which now uses newly added NorFlashInfoLib, that helps to
obtain description of the JEDEC compliant devices.
This patch updates the MvSpiFlashProtocol ReadId() protocol
callback on both producer's (MvFlashDxe) and consumers' sides
(FirmwareUpdate and SpiTool applications). Because all
information about detected SPI NOR flash is now stored in
the obtained NorFlashInfo structure fields, use them instead
of the PCDs.
Enable compilation of the NorFlashInfoLib and update
PortingGuide documentation accordingly.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 5 +-
Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf | 4 +-
Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c | 5 +-
Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf | 2 +-
Platform/Marvell/Armada/Armada.dsc.inc | 1 +
Platform/Marvell/Armada/Armada70x0.dsc | 5 --
Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 68 +++++++++++---------
Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf | 9 +--
Platform/Marvell/Drivers/Spi/MvSpiDxe.inf | 2 +
Platform/Marvell/Include/Protocol/Spi.h | 3 +
Platform/Marvell/Include/Protocol/SpiFlash.h | 6 +-
Platform/Marvell/Marvell.dec | 6 --
Silicon/Marvell/Documentation/PortingGuide.txt | 18 ------
13 files changed, 51 insertions(+), 83 deletions(-)
diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
index d70645d..750e52a 100644
--- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
+++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
@@ -94,12 +94,9 @@ SpiFlashProbe (
)
{
EFI_STATUS Status;
- UINT8 *FlashId;
-
- FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
// Read SPI flash ID
- Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
+ Status = SpiFlashProtocol->ReadId (Slave, FALSE);
if (EFI_ERROR (Status)) {
return SHELL_ABORTED;
}
diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
index 92c3333..53ea491 100644
--- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
+++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
@@ -44,6 +44,7 @@
FUpdate.uni
[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
Platform/Marvell/Marvell.dec
@@ -64,9 +65,6 @@
UefiLib
UefiRuntimeServicesTableLib
-[Pcd]
- gMarvellTokenSpaceGuid.PcdSpiFlashId
-
[Protocols]
gMarvellSpiFlashProtocolGuid
gMarvellSpiMasterProtocolGuid
diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
index a12f2ec..68a6cf7 100644
--- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
+++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
@@ -166,11 +166,8 @@ FlashProbe (
)
{
EFI_STATUS Status;
- UINT8 *FlashId;
- FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
-
- Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
+ Status = SpiFlashProtocol->ReadId (Slave, FALSE);
if (EFI_ERROR (Status)) {
return SHELL_ABORTED;
}
diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
index 887b9a5..a52906b 100644
--- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
+++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
@@ -44,6 +44,7 @@
SpiFlashCmd.uni
[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
MdePkg/MdePkg.dec
ShellPkg/ShellPkg.dec
MdeModulePkg/MdeModulePkg.dec
@@ -66,7 +67,6 @@
[Pcd]
gMarvellTokenSpaceGuid.PcdSpiFlashCs
- gMarvellTokenSpaceGuid.PcdSpiFlashId
gMarvellTokenSpaceGuid.PcdSpiFlashMode
[Protocols]
diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
index b9fc384..2cd96e6 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -33,6 +33,7 @@
ArmPlatformLib|Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
ComPhyLib|Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
MppLib|Platform/Marvell/Library/MppLib/MppLib.inf
+ NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
UtmiPhyLib|Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
index 4d5f55f..8e4cdb2 100644
--- a/Platform/Marvell/Armada/Armada70x0.dsc
+++ b/Platform/Marvell/Armada/Armada70x0.dsc
@@ -90,11 +90,6 @@
gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|10000000
gMarvellTokenSpaceGuid.PcdSpiClockFrequency|200000000
- gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd|0x70
- gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|3
- gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|65536
- gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|256
- 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 ab3ed6a..703994c 100755
--- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
+++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
@@ -107,10 +107,10 @@ MvSpiFlashWriteCommon (
UINT8 PollBit = STATUS_REG_POLL_WIP;
UINT8 CheckStatus = 0x0;
- CmdStatus = (UINT8)PcdGet32 (PcdSpiFlashPollCmd);
- if (CmdStatus == CMD_FLAG_STATUS) {
+ if (Slave->Info->Flags & NF_WRITE_FSR) {
+ CmdStatus = CMD_FLAG_STATUS;
PollBit = STATUS_REG_POLL_PEC;
- CheckStatus = PollBit;
+ CheckStatus = STATUS_REG_POLL_PEC;
}
// Send command
@@ -181,8 +181,19 @@ MvSpiFlashErase (
UINTN EraseSize;
UINT8 Cmd[5];
- AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
- EraseSize = PcdGet64 (PcdSpiFlashEraseSize);
+ if (Slave->Info->Flags & NF_4B_ADDR) {
+ AddrSize = 4;
+ } else {
+ AddrSize = 3;
+ }
+
+ if (Slave->Info->Flags & NF_ERASE_4K) {
+ Cmd[0] = CMD_ERASE_4K;
+ EraseSize = SIZE_4KB;
+ } else {
+ Cmd[0] = CMD_ERASE_64K;
+ EraseSize = Slave->Info->SectorSize;
+ }
// Check input parameters
if (Offset % EraseSize || Length % EraseSize) {
@@ -191,21 +202,6 @@ MvSpiFlashErase (
return EFI_DEVICE_ERROR;
}
- switch (EraseSize) {
- case SIZE_4KB:
- Cmd[0] = CMD_ERASE_4K;
- break;
- case SIZE_32KB:
- Cmd[0] = CMD_ERASE_32K;
- break;
- case SIZE_64KB:
- Cmd[0] = CMD_ERASE_64K;
- break;
- default:
- DEBUG ((DEBUG_ERROR, "MvSpiFlash: Invalid EraseSize parameter\n"));
- return EFI_INVALID_PARAMETER;
- }
-
while (Length) {
EraseAddr = Offset;
@@ -239,7 +235,11 @@ MvSpiFlashRead (
UINT32 AddrSize, ReadAddr, ReadLength, RemainLength;
UINTN BankSel = 0;
- AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
+ if (Slave->Info->Flags & NF_4B_ADDR) {
+ AddrSize = 4;
+ } else {
+ AddrSize = 3;
+ }
Cmd[0] = CMD_READ_ARRAY_FAST;
@@ -282,8 +282,13 @@ MvSpiFlashWrite (
UINT32 WriteAddr;
UINT8 Cmd[5], AddrSize;
- AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
- PageSize = PcdGet32 (PcdSpiFlashPageSize);
+ if (Slave->Info->Flags & NF_4B_ADDR) {
+ AddrSize = 4;
+ } else {
+ AddrSize = 3;
+ }
+
+ PageSize = Slave->Info->PageSize;
Cmd[0] = CMD_PAGE_PROGRAM;
@@ -370,7 +375,7 @@ MvSpiFlashUpdate (
UINT64 SectorSize, ToUpdate, Scale = 1;
UINT8 *TmpBuf, *End;
- SectorSize = PcdGet64 (PcdSpiFlashSectorSize);
+ SectorSize = Slave->Info->SectorSize;
End = Buf + ByteCount;
@@ -404,8 +409,7 @@ EFI_STATUS
EFIAPI
MvSpiFlashReadId (
IN SPI_DEVICE *SpiDev,
- IN UINT32 DataByteCount,
- IN OUT UINT8 *Buffer
+ IN BOOLEAN UseInRuntime
)
{
EFI_STATUS Status;
@@ -425,9 +429,7 @@ MvSpiFlashReadId (
return Status;
}
- if (CompareMem (Id, Buffer, DataByteCount) != 0) {
- Status = EFI_NOT_FOUND;
- }
+ Status = GetNorFlashInfo (Id, &SpiDev->Info, UseInRuntime);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR,
"%a: Unrecognized JEDEC Id bytes: 0x%02x%02x%02x\n",
@@ -438,6 +440,8 @@ MvSpiFlashReadId (
return Status;
}
+ PrintNorFlashInfo (SpiDev->Info);
+
return EFI_SUCCESS;
}
@@ -452,7 +456,11 @@ MvSpiFlashInit (
UINT8 Cmd, StatusRegister;
UINT32 AddrSize;
- AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
+ if (Slave->Info->Flags & NF_4B_ADDR) {
+ AddrSize = 4;
+ } else {
+ AddrSize = 3;
+ }
if (AddrSize == 4) {
// Set 4 byte address mode
diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
index 4519b02..6587f69 100644
--- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
+++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
@@ -42,10 +42,12 @@
MvSpiFlash.h
[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
MdePkg/MdePkg.dec
Platform/Marvell/Marvell.dec
[LibraryClasses]
+ NorFlashInfoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
TimerLib
@@ -53,13 +55,6 @@
DebugLib
MemoryAllocationLib
-[FixedPcd]
- gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles
- gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize
- gMarvellTokenSpaceGuid.PcdSpiFlashPageSize
- gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd
- gMarvellTokenSpaceGuid.PcdSpiFlashSectorSize
-
[Protocols]
gMarvellSpiMasterProtocolGuid
gMarvellSpiFlashProtocolGuid
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf b/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
index d38d331..08c6c04 100644
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
@@ -42,10 +42,12 @@
MvSpiDxe.h
[Packages]
+ EmbeddedPkg/EmbeddedPkg.dec
MdePkg/MdePkg.dec
Platform/Marvell/Marvell.dec
[LibraryClasses]
+ NorFlashInfoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
TimerLib
diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
index ae78a31..93a8ec0 100644
--- a/Platform/Marvell/Include/Protocol/Spi.h
+++ b/Platform/Marvell/Include/Protocol/Spi.h
@@ -34,6 +34,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#ifndef __MARVELL_SPI_MASTER_PROTOCOL_H__
#define __MARVELL_SPI_MASTER_PROTOCOL_H__
+#include <Library/NorFlashInfoLib.h>
+
extern EFI_GUID gMarvellSpiMasterProtocolGuid;
typedef struct _MARVELL_SPI_MASTER_PROTOCOL MARVELL_SPI_MASTER_PROTOCOL;
@@ -49,6 +51,7 @@ typedef struct {
INTN Cs;
INTN MaxFreq;
SPI_MODE Mode;
+ NOR_FLASH_INFO *Info;
} SPI_DEVICE;
typedef
diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h
index e0d62cc..4a3053e 100644
--- a/Platform/Marvell/Include/Protocol/SpiFlash.h
+++ b/Platform/Marvell/Include/Protocol/SpiFlash.h
@@ -47,9 +47,6 @@ 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;
@@ -65,8 +62,7 @@ typedef
EFI_STATUS
(EFIAPI *MV_SPI_FLASH_READ_ID) (
IN SPI_DEVICE *SpiDev,
- IN UINT32 DataByteCount,
- IN OUT UINT8 *Buffer
+ IN BOOLEAN UseInRuntime
);
typedef
diff --git a/Platform/Marvell/Marvell.dec b/Platform/Marvell/Marvell.dec
index 679a9d0..8255895 100644
--- a/Platform/Marvell/Marvell.dec
+++ b/Platform/Marvell/Marvell.dec
@@ -128,12 +128,6 @@
gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|0|UINT32|0x30000052
gMarvellTokenSpaceGuid.PcdSpiClockFrequency|0|UINT32|0x30000053
- gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd|0|UINT32|0x3000052
- gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|0|UINT32|0x3000053
- gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|0|UINT64|0x3000054
- gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|0|UINT32|0x3000055
- gMarvellTokenSpaceGuid.PcdSpiFlashSectorSize|65536|UINT64|0x3000059
- gMarvellTokenSpaceGuid.PcdSpiFlashId|{ 0x0 }|VOID*|0x3000056
gMarvellTokenSpaceGuid.PcdSpiFlashCs|0|UINT32|0x3000057
gMarvellTokenSpaceGuid.PcdSpiFlashMode|0|UINT32|0x3000058
diff --git a/Silicon/Marvell/Documentation/PortingGuide.txt b/Silicon/Marvell/Documentation/PortingGuide.txt
index cbe3bed..d5deed5 100644
--- a/Silicon/Marvell/Documentation/PortingGuide.txt
+++ b/Silicon/Marvell/Documentation/PortingGuide.txt
@@ -312,24 +312,6 @@ SpiFlash configuration
======================
Folowing PCDs for spi flash driver configuration must be set properly:
- - gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles
- (Size of SPI flash address in bytes (3 or 4) )
-
- - gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize
- (Size of minimal erase block in bytes)
-
- - gMarvellTokenSpaceGuid.PcdSpiFlashPageSize
- (Size of SPI flash page)
-
- - gMarvellTokenSpaceGuid.PcdSpiFlashSectorSize
- (Size of SPI flash sector, 65536 bytes by default)
-
- - gMarvellTokenSpaceGuid.PcdSpiFlashId
- (Id of SPI flash)
-
- - gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd
- (Spi flash polling flag)
-
- gMarvellTokenSpaceGuid.PcdSpiFlashMode
(Default SCLK mode (see SPI_MODE enum in file
edk2-platforms/Platform/Marvell/Drivers/Spi/MvSpi.h))
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection
2017-10-31 3:59 ` [platforms: PATCH 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection Marcin Wojtas
@ 2017-11-01 3:25 ` Leif Lindholm
2017-11-01 16:35 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-11-01 3:25 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Tue, Oct 31, 2017 at 04:59:31AM +0100, Marcin Wojtas wrote:
> Hitherto mechanism of fixing SPI flash model in the PCDs,
> occured to be very inefficient and problematic. Enable
> dynamic detection by reworking MvSpiFlashReadId() command,
> which now uses newly added NorFlashInfoLib, that helps to
> obtain description of the JEDEC compliant devices.
>
> This patch updates the MvSpiFlashProtocol ReadId() protocol
> callback on both producer's (MvFlashDxe) and consumers' sides
> (FirmwareUpdate and SpiTool applications). Because all
> information about detected SPI NOR flash is now stored in
> the obtained NorFlashInfo structure fields, use them instead
> of the PCDs.
>
> Enable compilation of the NorFlashInfoLib and update
> PortingGuide documentation accordingly.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 5 +-
> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf | 4 +-
> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c | 5 +-
> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf | 2 +-
> Platform/Marvell/Armada/Armada.dsc.inc | 1 +
> Platform/Marvell/Armada/Armada70x0.dsc | 5 --
> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 68 +++++++++++---------
> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf | 9 +--
> Platform/Marvell/Drivers/Spi/MvSpiDxe.inf | 2 +
> Platform/Marvell/Include/Protocol/Spi.h | 3 +
> Platform/Marvell/Include/Protocol/SpiFlash.h | 6 +-
> Platform/Marvell/Marvell.dec | 6 --
> Silicon/Marvell/Documentation/PortingGuide.txt | 18 ------
> 13 files changed, 51 insertions(+), 83 deletions(-)
>
> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> index d70645d..750e52a 100644
> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> @@ -94,12 +94,9 @@ SpiFlashProbe (
> )
> {
> EFI_STATUS Status;
> - UINT8 *FlashId;
> -
> - FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
>
> // Read SPI flash ID
> - Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
> + Status = SpiFlashProtocol->ReadId (Slave, FALSE);
> if (EFI_ERROR (Status)) {
> return SHELL_ABORTED;
> }
> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
> index 92c3333..53ea491 100644
> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
> @@ -44,6 +44,7 @@
> FUpdate.uni
>
> [Packages]
> + EmbeddedPkg/EmbeddedPkg.dec
> MdeModulePkg/MdeModulePkg.dec
> MdePkg/MdePkg.dec
> Platform/Marvell/Marvell.dec
> @@ -64,9 +65,6 @@
> UefiLib
> UefiRuntimeServicesTableLib
>
> -[Pcd]
> - gMarvellTokenSpaceGuid.PcdSpiFlashId
> -
> [Protocols]
> gMarvellSpiFlashProtocolGuid
> gMarvellSpiMasterProtocolGuid
> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> index a12f2ec..68a6cf7 100644
> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> @@ -166,11 +166,8 @@ FlashProbe (
> )
> {
> EFI_STATUS Status;
> - UINT8 *FlashId;
>
> - FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
> -
> - Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
> + Status = SpiFlashProtocol->ReadId (Slave, FALSE);
> if (EFI_ERROR (Status)) {
> return SHELL_ABORTED;
> }
> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
> index 887b9a5..a52906b 100644
> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
> @@ -44,6 +44,7 @@
> SpiFlashCmd.uni
>
> [Packages]
> + EmbeddedPkg/EmbeddedPkg.dec
> MdePkg/MdePkg.dec
> ShellPkg/ShellPkg.dec
> MdeModulePkg/MdeModulePkg.dec
> @@ -66,7 +67,6 @@
>
> [Pcd]
> gMarvellTokenSpaceGuid.PcdSpiFlashCs
> - gMarvellTokenSpaceGuid.PcdSpiFlashId
> gMarvellTokenSpaceGuid.PcdSpiFlashMode
>
> [Protocols]
> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
> index b9fc384..2cd96e6 100644
> --- a/Platform/Marvell/Armada/Armada.dsc.inc
> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
> @@ -33,6 +33,7 @@
> ArmPlatformLib|Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
> ComPhyLib|Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
> MppLib|Platform/Marvell/Library/MppLib/MppLib.inf
> + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
> UtmiPhyLib|Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
>
> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index 4d5f55f..8e4cdb2 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -90,11 +90,6 @@
> gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|10000000
> gMarvellTokenSpaceGuid.PcdSpiClockFrequency|200000000
>
> - gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd|0x70
> - gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|3
> - gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|65536
> - gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|256
> - 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 ab3ed6a..703994c 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> @@ -107,10 +107,10 @@ MvSpiFlashWriteCommon (
> UINT8 PollBit = STATUS_REG_POLL_WIP;
> UINT8 CheckStatus = 0x0;
>
> - CmdStatus = (UINT8)PcdGet32 (PcdSpiFlashPollCmd);
> - if (CmdStatus == CMD_FLAG_STATUS) {
> + if (Slave->Info->Flags & NF_WRITE_FSR) {
> + CmdStatus = CMD_FLAG_STATUS;
> PollBit = STATUS_REG_POLL_PEC;
> - CheckStatus = PollBit;
> + CheckStatus = STATUS_REG_POLL_PEC;
> }
>
> // Send command
> @@ -181,8 +181,19 @@ MvSpiFlashErase (
> UINTN EraseSize;
> UINT8 Cmd[5];
>
> - AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
> - EraseSize = PcdGet64 (PcdSpiFlashEraseSize);
> + if (Slave->Info->Flags & NF_4B_ADDR) {
> + AddrSize = 4;
> + } else {
> + AddrSize = 3;
> + }
> +
> + if (Slave->Info->Flags & NF_ERASE_4K) {
> + Cmd[0] = CMD_ERASE_4K;
> + EraseSize = SIZE_4KB;
> + } else {
Are 4 and 64K the only legal sizes?
This patch deletes a detection of 32K size and the error message (and
return) if an unknown size is encountered.
> + Cmd[0] = CMD_ERASE_64K;
> + EraseSize = Slave->Info->SectorSize;
> + }
>
> // Check input parameters
> if (Offset % EraseSize || Length % EraseSize) {
> @@ -191,21 +202,6 @@ MvSpiFlashErase (
> return EFI_DEVICE_ERROR;
> }
>
> - switch (EraseSize) {
> - case SIZE_4KB:
> - Cmd[0] = CMD_ERASE_4K;
> - break;
> - case SIZE_32KB:
> - Cmd[0] = CMD_ERASE_32K;
> - break;
> - case SIZE_64KB:
> - Cmd[0] = CMD_ERASE_64K;
> - break;
> - default:
> - DEBUG ((DEBUG_ERROR, "MvSpiFlash: Invalid EraseSize parameter\n"));
> - return EFI_INVALID_PARAMETER;
> - }
> -
> while (Length) {
> EraseAddr = Offset;
>
> @@ -239,7 +235,11 @@ MvSpiFlashRead (
> UINT32 AddrSize, ReadAddr, ReadLength, RemainLength;
> UINTN BankSel = 0;
>
> - AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
> + if (Slave->Info->Flags & NF_4B_ADDR) {
> + AddrSize = 4;
> + } else {
> + AddrSize = 3;
> + }
I see this sequence repeated many times.
Could it be replaced by a macro or static helper function? Or cached
in a device structure?
/
Leif
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection
2017-11-01 3:25 ` Leif Lindholm
@ 2017-11-01 16:35 ` Marcin Wojtas
0 siblings, 0 replies; 21+ messages in thread
From: Marcin Wojtas @ 2017-11-01 16:35 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
Leif,
2017-11-01 4:25 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 31, 2017 at 04:59:31AM +0100, Marcin Wojtas wrote:
>> Hitherto mechanism of fixing SPI flash model in the PCDs,
>> occured to be very inefficient and problematic. Enable
>> dynamic detection by reworking MvSpiFlashReadId() command,
>> which now uses newly added NorFlashInfoLib, that helps to
>> obtain description of the JEDEC compliant devices.
>>
>> This patch updates the MvSpiFlashProtocol ReadId() protocol
>> callback on both producer's (MvFlashDxe) and consumers' sides
>> (FirmwareUpdate and SpiTool applications). Because all
>> information about detected SPI NOR flash is now stored in
>> the obtained NorFlashInfo structure fields, use them instead
>> of the PCDs.
>>
>> Enable compilation of the NorFlashInfoLib and update
>> PortingGuide documentation accordingly.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 5 +-
>> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf | 4 +-
>> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c | 5 +-
>> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf | 2 +-
>> Platform/Marvell/Armada/Armada.dsc.inc | 1 +
>> Platform/Marvell/Armada/Armada70x0.dsc | 5 --
>> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 68 +++++++++++---------
>> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf | 9 +--
>> Platform/Marvell/Drivers/Spi/MvSpiDxe.inf | 2 +
>> Platform/Marvell/Include/Protocol/Spi.h | 3 +
>> Platform/Marvell/Include/Protocol/SpiFlash.h | 6 +-
>> Platform/Marvell/Marvell.dec | 6 --
>> Silicon/Marvell/Documentation/PortingGuide.txt | 18 ------
>> 13 files changed, 51 insertions(+), 83 deletions(-)
>>
>> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> index d70645d..750e52a 100644
>> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> @@ -94,12 +94,9 @@ SpiFlashProbe (
>> )
>> {
>> EFI_STATUS Status;
>> - UINT8 *FlashId;
>> -
>> - FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
>>
>> // Read SPI flash ID
>> - Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
>> + Status = SpiFlashProtocol->ReadId (Slave, FALSE);
>> if (EFI_ERROR (Status)) {
>> return SHELL_ABORTED;
>> }
>> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
>> index 92c3333..53ea491 100644
>> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
>> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.inf
>> @@ -44,6 +44,7 @@
>> FUpdate.uni
>>
>> [Packages]
>> + EmbeddedPkg/EmbeddedPkg.dec
>> MdeModulePkg/MdeModulePkg.dec
>> MdePkg/MdePkg.dec
>> Platform/Marvell/Marvell.dec
>> @@ -64,9 +65,6 @@
>> UefiLib
>> UefiRuntimeServicesTableLib
>>
>> -[Pcd]
>> - gMarvellTokenSpaceGuid.PcdSpiFlashId
>> -
>> [Protocols]
>> gMarvellSpiFlashProtocolGuid
>> gMarvellSpiMasterProtocolGuid
>> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> index a12f2ec..68a6cf7 100644
>> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> @@ -166,11 +166,8 @@ FlashProbe (
>> )
>> {
>> EFI_STATUS Status;
>> - UINT8 *FlashId;
>>
>> - FlashId = (UINT8 *)PcdGetPtr (PcdSpiFlashId);
>> -
>> - Status = SpiFlashProtocol->ReadId (Slave, NOR_FLASH_ID_DEFAULT_LEN, FlashId);
>> + Status = SpiFlashProtocol->ReadId (Slave, FALSE);
>> if (EFI_ERROR (Status)) {
>> return SHELL_ABORTED;
>> }
>> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
>> index 887b9a5..a52906b 100644
>> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
>> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.inf
>> @@ -44,6 +44,7 @@
>> SpiFlashCmd.uni
>>
>> [Packages]
>> + EmbeddedPkg/EmbeddedPkg.dec
>> MdePkg/MdePkg.dec
>> ShellPkg/ShellPkg.dec
>> MdeModulePkg/MdeModulePkg.dec
>> @@ -66,7 +67,6 @@
>>
>> [Pcd]
>> gMarvellTokenSpaceGuid.PcdSpiFlashCs
>> - gMarvellTokenSpaceGuid.PcdSpiFlashId
>> gMarvellTokenSpaceGuid.PcdSpiFlashMode
>>
>> [Protocols]
>> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
>> index b9fc384..2cd96e6 100644
>> --- a/Platform/Marvell/Armada/Armada.dsc.inc
>> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
>> @@ -33,6 +33,7 @@
>> ArmPlatformLib|Platform/Marvell/Armada/Library/Armada70x0Lib/Armada70x0Lib.inf
>> ComPhyLib|Platform/Marvell/Library/ComPhyLib/ComPhyLib.inf
>> MppLib|Platform/Marvell/Library/MppLib/MppLib.inf
>> + NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
>> UtmiPhyLib|Platform/Marvell/Library/UtmiPhyLib/UtmiPhyLib.inf
>>
>> DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
>> index 4d5f55f..8e4cdb2 100644
>> --- a/Platform/Marvell/Armada/Armada70x0.dsc
>> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
>> @@ -90,11 +90,6 @@
>> gMarvellTokenSpaceGuid.PcdSpiMaxFrequency|10000000
>> gMarvellTokenSpaceGuid.PcdSpiClockFrequency|200000000
>>
>> - gMarvellTokenSpaceGuid.PcdSpiFlashPollCmd|0x70
>> - gMarvellTokenSpaceGuid.PcdSpiFlashAddressCycles|3
>> - gMarvellTokenSpaceGuid.PcdSpiFlashEraseSize|65536
>> - gMarvellTokenSpaceGuid.PcdSpiFlashPageSize|256
>> - 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 ab3ed6a..703994c 100755
>> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> @@ -107,10 +107,10 @@ MvSpiFlashWriteCommon (
>> UINT8 PollBit = STATUS_REG_POLL_WIP;
>> UINT8 CheckStatus = 0x0;
>>
>> - CmdStatus = (UINT8)PcdGet32 (PcdSpiFlashPollCmd);
>> - if (CmdStatus == CMD_FLAG_STATUS) {
>> + if (Slave->Info->Flags & NF_WRITE_FSR) {
>> + CmdStatus = CMD_FLAG_STATUS;
>> PollBit = STATUS_REG_POLL_PEC;
>> - CheckStatus = PollBit;
>> + CheckStatus = STATUS_REG_POLL_PEC;
>> }
>>
>> // Send command
>> @@ -181,8 +181,19 @@ MvSpiFlashErase (
>> UINTN EraseSize;
>> UINT8 Cmd[5];
>>
>> - AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
>> - EraseSize = PcdGet64 (PcdSpiFlashEraseSize);
>> + if (Slave->Info->Flags & NF_4B_ADDR) {
>> + AddrSize = 4;
>> + } else {
>> + AddrSize = 3;
>> + }
>> +
>> + if (Slave->Info->Flags & NF_ERASE_4K) {
>> + Cmd[0] = CMD_ERASE_4K;
>> + EraseSize = SIZE_4KB;
>> + } else {
>
> Are 4 and 64K the only legal sizes?
> This patch deletes a detection of 32K size and the error message (and
> return) if an unknown size is encountered.
I didn't find 32K flash in Linux and U-Boot, maybe there some exotic
models, which use it. Anyway, there is no size check, as there are two
valid options now - 64KB and 4KB size, which is determined by
Info->Flag. Previously it was explicit EraseSize value, so the check
for wrong value was needed. Although unused, I can add NF_ERASE_32K
flag to NorFlashInfoLib and extend a check here.
>
>> + Cmd[0] = CMD_ERASE_64K;
>> + EraseSize = Slave->Info->SectorSize;
>> + }
>>
>> // Check input parameters
>> if (Offset % EraseSize || Length % EraseSize) {
>> @@ -191,21 +202,6 @@ MvSpiFlashErase (
>> return EFI_DEVICE_ERROR;
>> }
>>
>> - switch (EraseSize) {
>> - case SIZE_4KB:
>> - Cmd[0] = CMD_ERASE_4K;
>> - break;
>> - case SIZE_32KB:
>> - Cmd[0] = CMD_ERASE_32K;
>> - break;
>> - case SIZE_64KB:
>> - Cmd[0] = CMD_ERASE_64K;
>> - break;
>> - default:
>> - DEBUG ((DEBUG_ERROR, "MvSpiFlash: Invalid EraseSize parameter\n"));
>> - return EFI_INVALID_PARAMETER;
>> - }
>> -
>> while (Length) {
>> EraseAddr = Offset;
>>
>> @@ -239,7 +235,11 @@ MvSpiFlashRead (
>> UINT32 AddrSize, ReadAddr, ReadLength, RemainLength;
>> UINTN BankSel = 0;
>>
>> - AddrSize = PcdGet32 (PcdSpiFlashAddressCycles);
>> + if (Slave->Info->Flags & NF_4B_ADDR) {
>> + AddrSize = 4;
>> + } else {
>> + AddrSize = 3;
>> + }
>
> I see this sequence repeated many times.
> Could it be replaced by a macro or static helper function? Or cached
> in a device structure?
>
Ok.
Thanks,
Marcin
^ permalink raw reply [flat|nested] 21+ messages in thread
* [platforms: PATCH 3/6] Marvell/Drivers: MvSpiFlash: Remove duplicated macros
2017-10-31 3:59 [platforms: PATCH 0/6] Armada 7k/8k SPI improvements pt 2 Marcin Wojtas
2017-10-31 3:59 ` [platforms: PATCH 1/6] Marvell/Drivers: MvSpiFlash: Improve ReadId Marcin Wojtas
2017-10-31 3:59 ` [platforms: PATCH 2/6] Marvell/Drivers: MvSpiFlash: Enable dynamic SPI Flash detection Marcin Wojtas
@ 2017-10-31 3:59 ` Marcin Wojtas
2017-11-01 3:27 ` Leif Lindholm
2017-10-31 3:59 ` [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device Marcin Wojtas
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-31 3:59 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
Flash commands macros are already defined locally, so
remove them from the protocol header.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Include/Protocol/SpiFlash.h | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h
index 4a3053e..4ba29ba 100644
--- a/Platform/Marvell/Include/Protocol/SpiFlash.h
+++ b/Platform/Marvell/Include/Protocol/SpiFlash.h
@@ -36,17 +36,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <Protocol/Spi.h>
-#define CMD_READ_ID 0x9f
-#define READ_STATUS_REG_CMD 0x0b
-#define CMD_WRITE_ENABLE 0x06
-#define CMD_FLAG_STATUS 0x70
-#define CMD_WRITE_STATUS_REG 0x01
-#define CMD_READ_ARRAY_FAST 0x0b
-#define CMD_PAGE_PROGRAM 0x02
-#define CMD_BANK_WRITE 0xc5
-#define CMD_ERASE_64K 0xd8
-#define CMD_4B_ADDR_ENABLE 0xb7
-
extern EFI_GUID gMarvellSpiFlashProtocolGuid;
typedef struct _MARVELL_SPI_FLASH_PROTOCOL MARVELL_SPI_FLASH_PROTOCOL;
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 3/6] Marvell/Drivers: MvSpiFlash: Remove duplicated macros
2017-10-31 3:59 ` [platforms: PATCH 3/6] Marvell/Drivers: MvSpiFlash: Remove duplicated macros Marcin Wojtas
@ 2017-11-01 3:27 ` Leif Lindholm
2017-11-01 16:36 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-11-01 3:27 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Tue, Oct 31, 2017 at 04:59:32AM +0100, Marcin Wojtas wrote:
> Flash commands macros are already defined locally, so
> remove them from the protocol header.
Locally?
I have no issue with the patch, but the commit message can be a bit
more descriptive.
/
Leif
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Include/Protocol/SpiFlash.h | 11 -----------
> 1 file changed, 11 deletions(-)
>
> diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h
> index 4a3053e..4ba29ba 100644
> --- a/Platform/Marvell/Include/Protocol/SpiFlash.h
> +++ b/Platform/Marvell/Include/Protocol/SpiFlash.h
> @@ -36,17 +36,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>
> #include <Protocol/Spi.h>
>
> -#define CMD_READ_ID 0x9f
> -#define READ_STATUS_REG_CMD 0x0b
> -#define CMD_WRITE_ENABLE 0x06
> -#define CMD_FLAG_STATUS 0x70
> -#define CMD_WRITE_STATUS_REG 0x01
> -#define CMD_READ_ARRAY_FAST 0x0b
> -#define CMD_PAGE_PROGRAM 0x02
> -#define CMD_BANK_WRITE 0xc5
> -#define CMD_ERASE_64K 0xd8
> -#define CMD_4B_ADDR_ENABLE 0xb7
> -
> extern EFI_GUID gMarvellSpiFlashProtocolGuid;
>
> typedef struct _MARVELL_SPI_FLASH_PROTOCOL MARVELL_SPI_FLASH_PROTOCOL;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 3/6] Marvell/Drivers: MvSpiFlash: Remove duplicated macros
2017-11-01 3:27 ` Leif Lindholm
@ 2017-11-01 16:36 ` Marcin Wojtas
0 siblings, 0 replies; 21+ messages in thread
From: Marcin Wojtas @ 2017-11-01 16:36 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
2017-11-01 4:27 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 31, 2017 at 04:59:32AM +0100, Marcin Wojtas wrote:
>> Flash commands macros are already defined locally, so
>> remove them from the protocol header.
>
> Locally?
> I have no issue with the patch, but the commit message can be a bit
> more descriptive.
>
In a local header. I'll improve the message.
Marcin
> /
> Leif
>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Include/Protocol/SpiFlash.h | 11 -----------
>> 1 file changed, 11 deletions(-)
>>
>> diff --git a/Platform/Marvell/Include/Protocol/SpiFlash.h b/Platform/Marvell/Include/Protocol/SpiFlash.h
>> index 4a3053e..4ba29ba 100644
>> --- a/Platform/Marvell/Include/Protocol/SpiFlash.h
>> +++ b/Platform/Marvell/Include/Protocol/SpiFlash.h
>> @@ -36,17 +36,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>
>> #include <Protocol/Spi.h>
>>
>> -#define CMD_READ_ID 0x9f
>> -#define READ_STATUS_REG_CMD 0x0b
>> -#define CMD_WRITE_ENABLE 0x06
>> -#define CMD_FLAG_STATUS 0x70
>> -#define CMD_WRITE_STATUS_REG 0x01
>> -#define CMD_READ_ARRAY_FAST 0x0b
>> -#define CMD_PAGE_PROGRAM 0x02
>> -#define CMD_BANK_WRITE 0xc5
>> -#define CMD_ERASE_64K 0xd8
>> -#define CMD_4B_ADDR_ENABLE 0xb7
>> -
>> extern EFI_GUID gMarvellSpiFlashProtocolGuid;
>>
>> typedef struct _MARVELL_SPI_FLASH_PROTOCOL MARVELL_SPI_FLASH_PROTOCOL;
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device
2017-10-31 3:59 [platforms: PATCH 0/6] Armada 7k/8k SPI improvements pt 2 Marcin Wojtas
` (2 preceding siblings ...)
2017-10-31 3:59 ` [platforms: PATCH 3/6] Marvell/Drivers: MvSpiFlash: Remove duplicated macros Marcin Wojtas
@ 2017-10-31 3:59 ` Marcin Wojtas
2017-11-01 3:45 ` Leif Lindholm
2017-10-31 3:59 ` [platforms: PATCH 5/6] Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion Marcin Wojtas
2017-10-31 3:59 ` [platforms: PATCH 6/6] Marvell/Drivers: MvSpiDxe: Keep data in SPI_DEVICE structure Marcin Wojtas
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-31 3:59 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
Current usage of sf command requires running 'sf probe' prior to
executing any other option. Because it is done in two separate steps,
it turned out that SpiMasterProtocol->SetupDevice could easily
overwrite valid Slave pointer when performing second operation.
Fix the issue by allocating Slave device only once and keep it
as global variable in the SpiTool application. This patch
also updates FirmwareUpdate command to follow the modified
SetupDevice operation.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 4 ++--
Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c | 8 ++++----
Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 17 +++++++++--------
Platform/Marvell/Drivers/Spi/MvSpiDxe.h | 1 +
Platform/Marvell/Include/Protocol/Spi.h | 1 +
5 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
index 750e52a..9ccb1c7 100644
--- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
+++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
@@ -240,7 +240,7 @@ ShellCommandRunFUpdate (
)
{
IN SHELL_FILE_HANDLE FileHandle;
- SPI_DEVICE *Slave;
+ SPI_DEVICE *Slave = NULL;
UINT64 FileSize;
UINTN *FileBuffer = NULL;
CHAR16 *ProblemParam;
@@ -302,7 +302,7 @@ ShellCommandRunFUpdate (
}
// Setup and probe SPI flash
- Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, 0, 0);
+ Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, 0, 0);
if (Slave == NULL) {
Print(L"%s: Cannot allocate SPI device!\n", CMD_NAME_STRING);
goto HeaderError;
diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
index 68a6cf7..1084f68 100644
--- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
+++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
@@ -191,7 +191,7 @@ ShellCommandRunSpiFlash (
)
{
EFI_STATUS Status;
- SPI_DEVICE *Slave;
+ STATIC SPI_DEVICE *Slave;
LIST_ENTRY *CheckPackage;
EFI_PHYSICAL_ADDRESS Address = 0, Offset = 0;
SHELL_FILE_HANDLE FileHandle = NULL;
@@ -273,7 +273,7 @@ EFI_STATUS Status;
Cs = PcdGet32 (PcdSpiFlashCs);
// Setup new spi device
- Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Cs, Mode);
+ Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, Cs, Mode);
if (Slave == NULL) {
Print(L"sf: Cannot allocate SPI device!\n");
return SHELL_ABORTED;
@@ -285,6 +285,8 @@ EFI_STATUS Status;
Status = FlashProbe (Slave);
if (EFI_ERROR(Status)) {
// No supported spi flash detected
+ SpiMasterProtocol->FreeDevice(Slave);
+ Slave = NULL;
return SHELL_ABORTED;
} else {
return Status;
@@ -426,8 +428,6 @@ EFI_STATUS Status;
break;
}
- SpiMasterProtocol->FreeDevice(Slave);
-
if (EFI_ERROR (Status)) {
Print (L"sf: Error while performing spi transfer\n");
return SHELL_ABORTED;
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
index 3b49147..a7db5f2 100755
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
@@ -296,21 +296,22 @@ SPI_DEVICE *
EFIAPI
MvSpiSetupSlave (
IN MARVELL_SPI_MASTER_PROTOCOL *This,
+ IN SPI_DEVICE *Slave,
IN UINTN Cs,
IN SPI_MODE Mode
)
{
- SPI_DEVICE *Slave;
+ if (!Slave) {
+ Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
+ if (Slave == NULL) {
+ DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
+ return NULL;
+ }
- Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
- if (Slave == NULL) {
- DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
- return NULL;
+ Slave->Cs = Cs;
+ Slave->Mode = Mode;
}
- Slave->Cs = Cs;
- Slave->Mode = Mode;
-
SpiSetupTransfer (This, Slave);
return Slave;
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
index 1401f62..e7e280a 100644
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
@@ -125,6 +125,7 @@ SPI_DEVICE *
EFIAPI
MvSpiSetupSlave (
IN MARVELL_SPI_MASTER_PROTOCOL * This,
+ IN SPI_DEVICE *Slave,
IN UINTN Cs,
IN SPI_MODE Mode
);
diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
index 93a8ec0..0cf7914 100644
--- a/Platform/Marvell/Include/Protocol/Spi.h
+++ b/Platform/Marvell/Include/Protocol/Spi.h
@@ -87,6 +87,7 @@ typedef
SPI_DEVICE *
(EFIAPI *MV_SPI_SETUP_DEVICE) (
IN MARVELL_SPI_MASTER_PROTOCOL *This,
+ IN SPI_DEVICE *Slave,
IN UINTN Cs,
IN SPI_MODE Mode
);
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device
2017-10-31 3:59 ` [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device Marcin Wojtas
@ 2017-11-01 3:45 ` Leif Lindholm
2017-11-01 16:40 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-11-01 3:45 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Tue, Oct 31, 2017 at 04:59:33AM +0100, Marcin Wojtas wrote:
> Current usage of sf command requires running 'sf probe' prior to
> executing any other option. Because it is done in two separate steps,
> it turned out that SpiMasterProtocol->SetupDevice could easily
> overwrite valid Slave pointer when performing second operation.
>
> Fix the issue by allocating Slave device only once and keep it
> as global variable in the SpiTool application. This patch
> also updates FirmwareUpdate command to follow the modified
> SetupDevice operation.
Really an unrelated question, but would we not expect to use capsule
updates instead now? Do we have other uses for this tool?
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 4 ++--
> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c | 8 ++++----
> Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 17 +++++++++--------
> Platform/Marvell/Drivers/Spi/MvSpiDxe.h | 1 +
> Platform/Marvell/Include/Protocol/Spi.h | 1 +
> 5 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> index 750e52a..9ccb1c7 100644
> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> @@ -240,7 +240,7 @@ ShellCommandRunFUpdate (
> )
> {
> IN SHELL_FILE_HANDLE FileHandle;
> - SPI_DEVICE *Slave;
> + SPI_DEVICE *Slave = NULL;
> UINT64 FileSize;
> UINTN *FileBuffer = NULL;
> CHAR16 *ProblemParam;
> @@ -302,7 +302,7 @@ ShellCommandRunFUpdate (
> }
>
> // Setup and probe SPI flash
> - Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, 0, 0);
> + Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, 0, 0);
> if (Slave == NULL) {
> Print(L"%s: Cannot allocate SPI device!\n", CMD_NAME_STRING);
> goto HeaderError;
> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> index 68a6cf7..1084f68 100644
> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> @@ -191,7 +191,7 @@ ShellCommandRunSpiFlash (
> )
> {
> EFI_STATUS Status;
> - SPI_DEVICE *Slave;
> + STATIC SPI_DEVICE *Slave;
If this is a safe thing to do, please use a global variable called
mSlave instead, to make it clear that it persists across calls.
> LIST_ENTRY *CheckPackage;
> EFI_PHYSICAL_ADDRESS Address = 0, Offset = 0;
> SHELL_FILE_HANDLE FileHandle = NULL;
> @@ -273,7 +273,7 @@ EFI_STATUS Status;
> Cs = PcdGet32 (PcdSpiFlashCs);
>
> // Setup new spi device
> - Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Cs, Mode);
> + Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, Cs, Mode);
> if (Slave == NULL) {
> Print(L"sf: Cannot allocate SPI device!\n");
> return SHELL_ABORTED;
> @@ -285,6 +285,8 @@ EFI_STATUS Status;
> Status = FlashProbe (Slave);
> if (EFI_ERROR(Status)) {
> // No supported spi flash detected
> + SpiMasterProtocol->FreeDevice(Slave);
Space before (.
/
Leif
> + Slave = NULL;
> return SHELL_ABORTED;
> } else {
> return Status;
> @@ -426,8 +428,6 @@ EFI_STATUS Status;
> break;
> }
>
> - SpiMasterProtocol->FreeDevice(Slave);
> -
> if (EFI_ERROR (Status)) {
> Print (L"sf: Error while performing spi transfer\n");
> return SHELL_ABORTED;
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> index 3b49147..a7db5f2 100755
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> @@ -296,21 +296,22 @@ SPI_DEVICE *
> EFIAPI
> MvSpiSetupSlave (
> IN MARVELL_SPI_MASTER_PROTOCOL *This,
> + IN SPI_DEVICE *Slave,
> IN UINTN Cs,
> IN SPI_MODE Mode
> )
> {
> - SPI_DEVICE *Slave;
> + if (!Slave) {
> + Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
> + if (Slave == NULL) {
> + DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
> + return NULL;
> + }
>
> - Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
> - if (Slave == NULL) {
> - DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
> - return NULL;
> + Slave->Cs = Cs;
> + Slave->Mode = Mode;
> }
>
> - Slave->Cs = Cs;
> - Slave->Mode = Mode;
> -
> SpiSetupTransfer (This, Slave);
>
> return Slave;
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> index 1401f62..e7e280a 100644
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> @@ -125,6 +125,7 @@ SPI_DEVICE *
> EFIAPI
> MvSpiSetupSlave (
> IN MARVELL_SPI_MASTER_PROTOCOL * This,
> + IN SPI_DEVICE *Slave,
> IN UINTN Cs,
> IN SPI_MODE Mode
> );
> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
> index 93a8ec0..0cf7914 100644
> --- a/Platform/Marvell/Include/Protocol/Spi.h
> +++ b/Platform/Marvell/Include/Protocol/Spi.h
> @@ -87,6 +87,7 @@ typedef
> SPI_DEVICE *
> (EFIAPI *MV_SPI_SETUP_DEVICE) (
> IN MARVELL_SPI_MASTER_PROTOCOL *This,
> + IN SPI_DEVICE *Slave,
> IN UINTN Cs,
> IN SPI_MODE Mode
> );
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device
2017-11-01 3:45 ` Leif Lindholm
@ 2017-11-01 16:40 ` Marcin Wojtas
2017-11-01 17:15 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-11-01 16:40 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
Leif,
2017-11-01 4:45 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 31, 2017 at 04:59:33AM +0100, Marcin Wojtas wrote:
>> Current usage of sf command requires running 'sf probe' prior to
>> executing any other option. Because it is done in two separate steps,
>> it turned out that SpiMasterProtocol->SetupDevice could easily
>> overwrite valid Slave pointer when performing second operation.
>>
>> Fix the issue by allocating Slave device only once and keep it
>> as global variable in the SpiTool application. This patch
>> also updates FirmwareUpdate command to follow the modified
>> SetupDevice operation.
>
> Really an unrelated question, but would we not expect to use capsule
> updates instead now? Do we have other uses for this tool?
I use it massively for the development. Since the variables work now,
I have a plan to implement capsule update, but it's definitely not at
the top of the list now. I'm also wondering if there will be no
problems with the binaries concatenation and their handling by the
bootrom. BTW. Is capsule update supposed to replace only UEFI part or
the entire image stored in flash?
>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 4 ++--
>> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c | 8 ++++----
>> Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 17 +++++++++--------
>> Platform/Marvell/Drivers/Spi/MvSpiDxe.h | 1 +
>> Platform/Marvell/Include/Protocol/Spi.h | 1 +
>> 5 files changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> index 750e52a..9ccb1c7 100644
>> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>> @@ -240,7 +240,7 @@ ShellCommandRunFUpdate (
>> )
>> {
>> IN SHELL_FILE_HANDLE FileHandle;
>> - SPI_DEVICE *Slave;
>> + SPI_DEVICE *Slave = NULL;
>> UINT64 FileSize;
>> UINTN *FileBuffer = NULL;
>> CHAR16 *ProblemParam;
>> @@ -302,7 +302,7 @@ ShellCommandRunFUpdate (
>> }
>>
>> // Setup and probe SPI flash
>> - Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, 0, 0);
>> + Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, 0, 0);
>> if (Slave == NULL) {
>> Print(L"%s: Cannot allocate SPI device!\n", CMD_NAME_STRING);
>> goto HeaderError;
>> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> index 68a6cf7..1084f68 100644
>> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>> @@ -191,7 +191,7 @@ ShellCommandRunSpiFlash (
>> )
>> {
>> EFI_STATUS Status;
>> - SPI_DEVICE *Slave;
>> + STATIC SPI_DEVICE *Slave;
>
> If this is a safe thing to do, please use a global variable called
> mSlave instead, to make it clear that it persists across calls.
Right, this way it will be more clear.
>
>> LIST_ENTRY *CheckPackage;
>> EFI_PHYSICAL_ADDRESS Address = 0, Offset = 0;
>> SHELL_FILE_HANDLE FileHandle = NULL;
>> @@ -273,7 +273,7 @@ EFI_STATUS Status;
>> Cs = PcdGet32 (PcdSpiFlashCs);
>>
>> // Setup new spi device
>> - Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Cs, Mode);
>> + Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, Cs, Mode);
>> if (Slave == NULL) {
>> Print(L"sf: Cannot allocate SPI device!\n");
>> return SHELL_ABORTED;
>> @@ -285,6 +285,8 @@ EFI_STATUS Status;
>> Status = FlashProbe (Slave);
>> if (EFI_ERROR(Status)) {
>> // No supported spi flash detected
>> + SpiMasterProtocol->FreeDevice(Slave);
>
> Space before (.
Ok.
Thanks,
Marcin
>
> /
> Leif
>
>> + Slave = NULL;
>> return SHELL_ABORTED;
>> } else {
>> return Status;
>> @@ -426,8 +428,6 @@ EFI_STATUS Status;
>> break;
>> }
>>
>> - SpiMasterProtocol->FreeDevice(Slave);
>> -
>> if (EFI_ERROR (Status)) {
>> Print (L"sf: Error while performing spi transfer\n");
>> return SHELL_ABORTED;
>> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>> index 3b49147..a7db5f2 100755
>> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>> @@ -296,21 +296,22 @@ SPI_DEVICE *
>> EFIAPI
>> MvSpiSetupSlave (
>> IN MARVELL_SPI_MASTER_PROTOCOL *This,
>> + IN SPI_DEVICE *Slave,
>> IN UINTN Cs,
>> IN SPI_MODE Mode
>> )
>> {
>> - SPI_DEVICE *Slave;
>> + if (!Slave) {
>> + Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
>> + if (Slave == NULL) {
>> + DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
>> + return NULL;
>> + }
>>
>> - Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
>> - if (Slave == NULL) {
>> - DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
>> - return NULL;
>> + Slave->Cs = Cs;
>> + Slave->Mode = Mode;
>> }
>>
>> - Slave->Cs = Cs;
>> - Slave->Mode = Mode;
>> -
>> SpiSetupTransfer (This, Slave);
>>
>> return Slave;
>> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>> index 1401f62..e7e280a 100644
>> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>> @@ -125,6 +125,7 @@ SPI_DEVICE *
>> EFIAPI
>> MvSpiSetupSlave (
>> IN MARVELL_SPI_MASTER_PROTOCOL * This,
>> + IN SPI_DEVICE *Slave,
>> IN UINTN Cs,
>> IN SPI_MODE Mode
>> );
>> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
>> index 93a8ec0..0cf7914 100644
>> --- a/Platform/Marvell/Include/Protocol/Spi.h
>> +++ b/Platform/Marvell/Include/Protocol/Spi.h
>> @@ -87,6 +87,7 @@ typedef
>> SPI_DEVICE *
>> (EFIAPI *MV_SPI_SETUP_DEVICE) (
>> IN MARVELL_SPI_MASTER_PROTOCOL *This,
>> + IN SPI_DEVICE *Slave,
>> IN UINTN Cs,
>> IN SPI_MODE Mode
>> );
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device
2017-11-01 16:40 ` Marcin Wojtas
@ 2017-11-01 17:15 ` Ard Biesheuvel
0 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-11-01 17:15 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Leif Lindholm, edk2-devel-01, Nadav Haklai, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
On 1 November 2017 at 16:40, Marcin Wojtas <mw@semihalf.com> wrote:
> Leif,
>
> 2017-11-01 4:45 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> On Tue, Oct 31, 2017 at 04:59:33AM +0100, Marcin Wojtas wrote:
>>> Current usage of sf command requires running 'sf probe' prior to
>>> executing any other option. Because it is done in two separate steps,
>>> it turned out that SpiMasterProtocol->SetupDevice could easily
>>> overwrite valid Slave pointer when performing second operation.
>>>
>>> Fix the issue by allocating Slave device only once and keep it
>>> as global variable in the SpiTool application. This patch
>>> also updates FirmwareUpdate command to follow the modified
>>> SetupDevice operation.
>>
>> Really an unrelated question, but would we not expect to use capsule
>> updates instead now? Do we have other uses for this tool?
>
> I use it massively for the development. Since the variables work now,
> I have a plan to implement capsule update, but it's definitely not at
> the top of the list now. I'm also wondering if there will be no
> problems with the binaries concatenation and their handling by the
> bootrom. BTW. Is capsule update supposed to replace only UEFI part or
> the entire image stored in flash?
>
That depends on your use case, but you can easily update your .fdf to
incorporate other binaries into the UEFI build (ARM-TF etc), and you
can update everything using a single capsule. That also allows you to
use the standard 'fwupdate' tool in Linux, and even allows you to host
firmware updates at fwupd.org, and they can be installed automatically
by the ordinary Software Update GUI that runs in the OS.
>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> ---
>>> Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 4 ++--
>>> Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c | 8 ++++----
>>> Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 17 +++++++++--------
>>> Platform/Marvell/Drivers/Spi/MvSpiDxe.h | 1 +
>>> Platform/Marvell/Include/Protocol/Spi.h | 1 +
>>> 5 files changed, 17 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>>> index 750e52a..9ccb1c7 100644
>>> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>>> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
>>> @@ -240,7 +240,7 @@ ShellCommandRunFUpdate (
>>> )
>>> {
>>> IN SHELL_FILE_HANDLE FileHandle;
>>> - SPI_DEVICE *Slave;
>>> + SPI_DEVICE *Slave = NULL;
>>> UINT64 FileSize;
>>> UINTN *FileBuffer = NULL;
>>> CHAR16 *ProblemParam;
>>> @@ -302,7 +302,7 @@ ShellCommandRunFUpdate (
>>> }
>>>
>>> // Setup and probe SPI flash
>>> - Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, 0, 0);
>>> + Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, 0, 0);
>>> if (Slave == NULL) {
>>> Print(L"%s: Cannot allocate SPI device!\n", CMD_NAME_STRING);
>>> goto HeaderError;
>>> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>>> index 68a6cf7..1084f68 100644
>>> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>>> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
>>> @@ -191,7 +191,7 @@ ShellCommandRunSpiFlash (
>>> )
>>> {
>>> EFI_STATUS Status;
>>> - SPI_DEVICE *Slave;
>>> + STATIC SPI_DEVICE *Slave;
>>
>> If this is a safe thing to do, please use a global variable called
>> mSlave instead, to make it clear that it persists across calls.
>
> Right, this way it will be more clear.
>
>>
>>> LIST_ENTRY *CheckPackage;
>>> EFI_PHYSICAL_ADDRESS Address = 0, Offset = 0;
>>> SHELL_FILE_HANDLE FileHandle = NULL;
>>> @@ -273,7 +273,7 @@ EFI_STATUS Status;
>>> Cs = PcdGet32 (PcdSpiFlashCs);
>>>
>>> // Setup new spi device
>>> - Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Cs, Mode);
>>> + Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, Cs, Mode);
>>> if (Slave == NULL) {
>>> Print(L"sf: Cannot allocate SPI device!\n");
>>> return SHELL_ABORTED;
>>> @@ -285,6 +285,8 @@ EFI_STATUS Status;
>>> Status = FlashProbe (Slave);
>>> if (EFI_ERROR(Status)) {
>>> // No supported spi flash detected
>>> + SpiMasterProtocol->FreeDevice(Slave);
>>
>> Space before (.
>
> Ok.
>
> Thanks,
> Marcin
>
>>
>> /
>> Leif
>>
>>> + Slave = NULL;
>>> return SHELL_ABORTED;
>>> } else {
>>> return Status;
>>> @@ -426,8 +428,6 @@ EFI_STATUS Status;
>>> break;
>>> }
>>>
>>> - SpiMasterProtocol->FreeDevice(Slave);
>>> -
>>> if (EFI_ERROR (Status)) {
>>> Print (L"sf: Error while performing spi transfer\n");
>>> return SHELL_ABORTED;
>>> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>>> index 3b49147..a7db5f2 100755
>>> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>>> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
>>> @@ -296,21 +296,22 @@ SPI_DEVICE *
>>> EFIAPI
>>> MvSpiSetupSlave (
>>> IN MARVELL_SPI_MASTER_PROTOCOL *This,
>>> + IN SPI_DEVICE *Slave,
>>> IN UINTN Cs,
>>> IN SPI_MODE Mode
>>> )
>>> {
>>> - SPI_DEVICE *Slave;
>>> + if (!Slave) {
>>> + Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
>>> + if (Slave == NULL) {
>>> + DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
>>> + return NULL;
>>> + }
>>>
>>> - Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
>>> - if (Slave == NULL) {
>>> - DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
>>> - return NULL;
>>> + Slave->Cs = Cs;
>>> + Slave->Mode = Mode;
>>> }
>>>
>>> - Slave->Cs = Cs;
>>> - Slave->Mode = Mode;
>>> -
>>> SpiSetupTransfer (This, Slave);
>>>
>>> return Slave;
>>> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>>> index 1401f62..e7e280a 100644
>>> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>>> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
>>> @@ -125,6 +125,7 @@ SPI_DEVICE *
>>> EFIAPI
>>> MvSpiSetupSlave (
>>> IN MARVELL_SPI_MASTER_PROTOCOL * This,
>>> + IN SPI_DEVICE *Slave,
>>> IN UINTN Cs,
>>> IN SPI_MODE Mode
>>> );
>>> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
>>> index 93a8ec0..0cf7914 100644
>>> --- a/Platform/Marvell/Include/Protocol/Spi.h
>>> +++ b/Platform/Marvell/Include/Protocol/Spi.h
>>> @@ -87,6 +87,7 @@ typedef
>>> SPI_DEVICE *
>>> (EFIAPI *MV_SPI_SETUP_DEVICE) (
>>> IN MARVELL_SPI_MASTER_PROTOCOL *This,
>>> + IN SPI_DEVICE *Slave,
>>> IN UINTN Cs,
>>> IN SPI_MODE Mode
>>> );
>>> --
>>> 2.7.4
>>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [platforms: PATCH 5/6] Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion
2017-10-31 3:59 [platforms: PATCH 0/6] Armada 7k/8k SPI improvements pt 2 Marcin Wojtas
` (3 preceding siblings ...)
2017-10-31 3:59 ` [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device Marcin Wojtas
@ 2017-10-31 3:59 ` Marcin Wojtas
2017-11-01 3:50 ` Leif Lindholm
2017-10-31 3:59 ` [platforms: PATCH 6/6] Marvell/Drivers: MvSpiDxe: Keep data in SPI_DEVICE structure Marcin Wojtas
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-31 3:59 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
Spansion SPI flash devices use different command for bank
selection. Update it, basing on the first byte of flash ID.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 5 +++++
Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h | 4 ++++
2 files changed, 9 insertions(+)
diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
index 703994c..a00fc305 100755
--- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
+++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
@@ -150,6 +150,11 @@ SpiFlashCmdBankaddrWrite (
{
UINT8 Cmd = CMD_BANK_WRITE;
+ /* Update bank selection command for Spansion */
+ if (Slave->Info->Id[0] == SPI_FLASH_MFR_SPANSION) {
+ Cmd = CMD_BANKADDR_BRWR;
+ }
+
MvSpiFlashWriteCommon (Slave, &Cmd, 1, &BankSel, 1);
}
diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
index 2583484..00af188 100755
--- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
+++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
@@ -57,6 +57,7 @@ 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_BANKADDR_BRWR 0x17
#define CMD_ERASE_4K 0x20
#define CMD_ERASE_32K 0x52
#define CMD_ERASE_64K 0xd8
@@ -72,6 +73,9 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#define SPI_FLASH_16MB_BOUN 0x1000000
+/* Manufacturer ID's */
+#define SPI_FLASH_MFR_SPANSION 0x01
+
typedef enum {
SPI_FLASH_READ_ID,
SPI_FLASH_READ, // Read from SPI flash with address
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 5/6] Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion
2017-10-31 3:59 ` [platforms: PATCH 5/6] Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion Marcin Wojtas
@ 2017-11-01 3:50 ` Leif Lindholm
2017-11-01 16:41 ` Marcin Wojtas
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2017-11-01 3:50 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Tue, Oct 31, 2017 at 04:59:34AM +0100, Marcin Wojtas wrote:
> Spansion SPI flash devices use different command for bank
> selection. Update it, basing on the first byte of flash ID.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 5 +++++
> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h | 4 ++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> index 703994c..a00fc305 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
> @@ -150,6 +150,11 @@ SpiFlashCmdBankaddrWrite (
> {
> UINT8 Cmd = CMD_BANK_WRITE;
>
> + /* Update bank selection command for Spansion */
> + if (Slave->Info->Id[0] == SPI_FLASH_MFR_SPANSION) {
> + Cmd = CMD_BANKADDR_BRWR;
> + }
> +
> MvSpiFlashWriteCommon (Slave, &Cmd, 1, &BankSel, 1);
> }
>
> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> index 2583484..00af188 100755
> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
> @@ -57,6 +57,7 @@ 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_BANKADDR_BRWR 0x17
> #define CMD_ERASE_4K 0x20
> #define CMD_ERASE_32K 0x52
> #define CMD_ERASE_64K 0xd8
> @@ -72,6 +73,9 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>
> #define SPI_FLASH_16MB_BOUN 0x1000000
>
> +/* Manufacturer ID's */
> +#define SPI_FLASH_MFR_SPANSION 0x01
> +
Please move this definition to NorFlashInfoLib.
Otherwise this patch looks OK.
/
Leif
> typedef enum {
> SPI_FLASH_READ_ID,
> SPI_FLASH_READ, // Read from SPI flash with address
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 5/6] Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion
2017-11-01 3:50 ` Leif Lindholm
@ 2017-11-01 16:41 ` Marcin Wojtas
0 siblings, 0 replies; 21+ messages in thread
From: Marcin Wojtas @ 2017-11-01 16:41 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel-01, Ard Biesheuvel, nadavh, Neta Zur Hershkovits,
Kostya Porotchkin, Hua Jing, semihalf-dabros-jan
2017-11-01 4:50 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Oct 31, 2017 at 04:59:34AM +0100, Marcin Wojtas wrote:
>> Spansion SPI flash devices use different command for bank
>> selection. Update it, basing on the first byte of flash ID.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c | 5 +++++
>> Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h | 4 ++++
>> 2 files changed, 9 insertions(+)
>>
>> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> index 703994c..a00fc305 100755
>> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.c
>> @@ -150,6 +150,11 @@ SpiFlashCmdBankaddrWrite (
>> {
>> UINT8 Cmd = CMD_BANK_WRITE;
>>
>> + /* Update bank selection command for Spansion */
>> + if (Slave->Info->Id[0] == SPI_FLASH_MFR_SPANSION) {
>> + Cmd = CMD_BANKADDR_BRWR;
>> + }
>> +
>> MvSpiFlashWriteCommon (Slave, &Cmd, 1, &BankSel, 1);
>> }
>>
>> diff --git a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
>> index 2583484..00af188 100755
>> --- a/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
>> +++ b/Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.h
>> @@ -57,6 +57,7 @@ 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_BANKADDR_BRWR 0x17
>> #define CMD_ERASE_4K 0x20
>> #define CMD_ERASE_32K 0x52
>> #define CMD_ERASE_64K 0xd8
>> @@ -72,6 +73,9 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>
>> #define SPI_FLASH_16MB_BOUN 0x1000000
>>
>> +/* Manufacturer ID's */
>> +#define SPI_FLASH_MFR_SPANSION 0x01
>> +
>
> Please move this definition to NorFlashInfoLib.
> Otherwise this patch looks OK.
>
Ok.
Thanks,
Marcin
^ permalink raw reply [flat|nested] 21+ messages in thread
* [platforms: PATCH 6/6] Marvell/Drivers: MvSpiDxe: Keep data in SPI_DEVICE structure
2017-10-31 3:59 [platforms: PATCH 0/6] Armada 7k/8k SPI improvements pt 2 Marcin Wojtas
` (4 preceding siblings ...)
2017-10-31 3:59 ` [platforms: PATCH 5/6] Marvell/Drivers: MvSpiFlash: Fix bank selection for Spansion Marcin Wojtas
@ 2017-10-31 3:59 ` Marcin Wojtas
2017-11-01 3:54 ` Leif Lindholm
5 siblings, 1 reply; 21+ messages in thread
From: Marcin Wojtas @ 2017-10-31 3:59 UTC (permalink / raw)
To: edk2-devel
Cc: leif.lindholm, ard.biesheuvel, nadavh, neta, kostap, jinghua, mw,
jsd
In the MvSpiDxe driver obtaining host register base address,
controller clock and device maximum frequency directly from PCDs
was done all over the code. This patch cleans up the parameters'
handling and enables accessing them from SPI_DEVICE structure fields.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 48 ++++++++++++--------
Platform/Marvell/Include/Protocol/Spi.h | 2 +
2 files changed, 31 insertions(+), 19 deletions(-)
diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
index a7db5f2..c60a520 100755
--- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
+++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
@@ -38,12 +38,13 @@ SPI_MASTER *mSpiMasterInstance;
STATIC
EFI_STATUS
SpiSetBaudRate (
+ IN SPI_DEVICE *Slave,
IN UINT32 CpuClock,
IN UINT32 MaxFreq
)
{
UINT32 Spr, BestSpr, Sppr, BestSppr, ClockDivider, Match, Reg, MinBaudDiff;
- UINTN SpiRegBase = PcdGet32 (PcdSpiRegBase);
+ UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
MinBaudDiff = 0xFFFFFFFF;
BestSppr = 0;
@@ -93,26 +94,28 @@ SpiSetBaudRate (
STATIC
VOID
SpiSetCs (
- UINT8 CsId
+ IN SPI_DEVICE *Slave
)
{
- UINT32 Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
+ UINT32 Reg;
+ UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
Reg &= ~SPI_CS_NUM_MASK;
- Reg |= (CsId << SPI_CS_NUM_OFFSET);
+ Reg |= (Slave->Cs << SPI_CS_NUM_OFFSET);
MmioWrite32 (SpiRegBase + SPI_CTRL_REG, Reg);
}
STATIC
VOID
SpiActivateCs (
- UINT8 IN CsId
+ IN SPI_DEVICE *Slave
)
{
- UINT32 Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
+ UINT32 Reg;
+ UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
- SpiSetCs(CsId);
+ SpiSetCs(Slave);
Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
Reg |= SPI_CS_EN_MASK;
MmioWrite32(SpiRegBase + SPI_CTRL_REG, Reg);
@@ -121,10 +124,11 @@ SpiActivateCs (
STATIC
VOID
SpiDeactivateCs (
- VOID
+ IN SPI_DEVICE *Slave
)
{
- UINT32 Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
+ UINT32 Reg;
+ UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
Reg &= ~SPI_CS_EN_MASK;
@@ -139,14 +143,15 @@ SpiSetupTransfer (
)
{
SPI_MASTER *SpiMaster;
- UINT32 Reg, SpiRegBase, CoreClock, SpiMaxFreq;
+ UINT32 Reg, CoreClock, SpiMaxFreq;
+ UINTN SpiRegBase;
SpiMaster = SPI_MASTER_FROM_SPI_MASTER_PROTOCOL (This);
// Initialize values from PCDs
- SpiRegBase = PcdGet32 (PcdSpiRegBase);
- CoreClock = PcdGet32 (PcdSpiClockFrequency);
- SpiMaxFreq = PcdGet32 (PcdSpiMaxFrequency);
+ SpiRegBase = Slave->HostRegisterBaseAddress;
+ CoreClock = Slave->CoreClock;
+ SpiMaxFreq = Slave->MaxFreq;
EfiAcquireLock (&SpiMaster->Lock);
@@ -154,9 +159,9 @@ SpiSetupTransfer (
Reg |= SPI_BYTE_LENGTH;
MmioWrite32 (SpiRegBase + SPI_CONF_REG, Reg);
- SpiSetCs(Slave->Cs);
+ SpiSetCs(Slave);
- SpiSetBaudRate (CoreClock, SpiMaxFreq);
+ SpiSetBaudRate (Slave, CoreClock, SpiMaxFreq);
Reg = MmioRead32 (SpiRegBase + SPI_CONF_REG);
Reg &= ~(SPI_CPOL_MASK | SPI_CPHA_MASK | SPI_TXLSBF_MASK | SPI_RXLSBF_MASK);
@@ -194,21 +199,22 @@ MvSpiTransfer (
{
SPI_MASTER *SpiMaster;
UINT64 Length;
- UINT32 Iterator, Reg, SpiRegBase;
+ UINT32 Iterator, Reg;
UINT8 *DataOutPtr = (UINT8 *)DataOut;
UINT8 *DataInPtr = (UINT8 *)DataIn;
UINT8 DataToSend = 0;
+ UINTN SpiRegBase;
SpiMaster = SPI_MASTER_FROM_SPI_MASTER_PROTOCOL (This);
- SpiRegBase = PcdGet32 (PcdSpiRegBase);
+ SpiRegBase = Slave->HostRegisterBaseAddress;
Length = 8 * DataByteCount;
EfiAcquireLock (&SpiMaster->Lock);
if (Flag & SPI_TRANSFER_BEGIN) {
- SpiActivateCs (Slave->Cs);
+ SpiActivateCs (Slave);
}
// Set 8-bit mode
@@ -245,7 +251,7 @@ MvSpiTransfer (
}
if (Flag & SPI_TRANSFER_END) {
- SpiDeactivateCs ();
+ SpiDeactivateCs (Slave);
}
EfiReleaseLock (&SpiMaster->Lock);
@@ -312,6 +318,10 @@ MvSpiSetupSlave (
Slave->Mode = Mode;
}
+ Slave->HostRegisterBaseAddress = PcdGet32 (PcdSpiRegBase);
+ Slave->CoreClock = PcdGet32 (PcdSpiClockFrequency);
+ Slave->MaxFreq = PcdGet32 (PcdSpiMaxFrequency);
+
SpiSetupTransfer (This, Slave);
return Slave;
diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
index 0cf7914..b8981f3 100644
--- a/Platform/Marvell/Include/Protocol/Spi.h
+++ b/Platform/Marvell/Include/Protocol/Spi.h
@@ -52,6 +52,8 @@ typedef struct {
INTN MaxFreq;
SPI_MODE Mode;
NOR_FLASH_INFO *Info;
+ UINTN HostRegisterBaseAddress;
+ UINTN CoreClock;
} SPI_DEVICE;
typedef
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [platforms: PATCH 6/6] Marvell/Drivers: MvSpiDxe: Keep data in SPI_DEVICE structure
2017-10-31 3:59 ` [platforms: PATCH 6/6] Marvell/Drivers: MvSpiDxe: Keep data in SPI_DEVICE structure Marcin Wojtas
@ 2017-11-01 3:54 ` Leif Lindholm
0 siblings, 0 replies; 21+ messages in thread
From: Leif Lindholm @ 2017-11-01 3:54 UTC (permalink / raw)
To: Marcin Wojtas
Cc: edk2-devel, ard.biesheuvel, nadavh, neta, kostap, jinghua, jsd
On Tue, Oct 31, 2017 at 04:59:35AM +0100, Marcin Wojtas wrote:
> In the MvSpiDxe driver obtaining host register base address,
> controller clock and device maximum frequency directly from PCDs
> was done all over the code. This patch cleans up the parameters'
> handling and enables accessing them from SPI_DEVICE structure fields.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> Platform/Marvell/Drivers/Spi/MvSpiDxe.c | 48 ++++++++++++--------
> Platform/Marvell/Include/Protocol/Spi.h | 2 +
> 2 files changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> index a7db5f2..c60a520 100755
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> @@ -38,12 +38,13 @@ SPI_MASTER *mSpiMasterInstance;
> STATIC
> EFI_STATUS
> SpiSetBaudRate (
> + IN SPI_DEVICE *Slave,
> IN UINT32 CpuClock,
> IN UINT32 MaxFreq
> )
> {
> UINT32 Spr, BestSpr, Sppr, BestSppr, ClockDivider, Match, Reg, MinBaudDiff;
> - UINTN SpiRegBase = PcdGet32 (PcdSpiRegBase);
> + UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
>
> MinBaudDiff = 0xFFFFFFFF;
> BestSppr = 0;
> @@ -93,26 +94,28 @@ SpiSetBaudRate (
> STATIC
> VOID
> SpiSetCs (
> - UINT8 CsId
> + IN SPI_DEVICE *Slave
> )
> {
> - UINT32 Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
> + UINT32 Reg;
> + UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
>
> Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
> Reg &= ~SPI_CS_NUM_MASK;
> - Reg |= (CsId << SPI_CS_NUM_OFFSET);
> + Reg |= (Slave->Cs << SPI_CS_NUM_OFFSET);
> MmioWrite32 (SpiRegBase + SPI_CTRL_REG, Reg);
> }
>
> STATIC
> VOID
> SpiActivateCs (
> - UINT8 IN CsId
> + IN SPI_DEVICE *Slave
> )
> {
> - UINT32 Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
> + UINT32 Reg;
> + UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
>
> - SpiSetCs(CsId);
> + SpiSetCs(Slave);
> Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
> Reg |= SPI_CS_EN_MASK;
> MmioWrite32(SpiRegBase + SPI_CTRL_REG, Reg);
> @@ -121,10 +124,11 @@ SpiActivateCs (
> STATIC
> VOID
> SpiDeactivateCs (
> - VOID
> + IN SPI_DEVICE *Slave
> )
> {
> - UINT32 Reg, SpiRegBase = PcdGet32 (PcdSpiRegBase);
> + UINT32 Reg;
> + UINTN SpiRegBase = Slave->HostRegisterBaseAddress;
>
> Reg = MmioRead32 (SpiRegBase + SPI_CTRL_REG);
> Reg &= ~SPI_CS_EN_MASK;
> @@ -139,14 +143,15 @@ SpiSetupTransfer (
> )
> {
> SPI_MASTER *SpiMaster;
> - UINT32 Reg, SpiRegBase, CoreClock, SpiMaxFreq;
> + UINT32 Reg, CoreClock, SpiMaxFreq;
> + UINTN SpiRegBase;
>
> SpiMaster = SPI_MASTER_FROM_SPI_MASTER_PROTOCOL (This);
>
> // Initialize values from PCDs
> - SpiRegBase = PcdGet32 (PcdSpiRegBase);
> - CoreClock = PcdGet32 (PcdSpiClockFrequency);
> - SpiMaxFreq = PcdGet32 (PcdSpiMaxFrequency);
> + SpiRegBase = Slave->HostRegisterBaseAddress;
> + CoreClock = Slave->CoreClock;
> + SpiMaxFreq = Slave->MaxFreq;
>
> EfiAcquireLock (&SpiMaster->Lock);
>
> @@ -154,9 +159,9 @@ SpiSetupTransfer (
> Reg |= SPI_BYTE_LENGTH;
> MmioWrite32 (SpiRegBase + SPI_CONF_REG, Reg);
>
> - SpiSetCs(Slave->Cs);
> + SpiSetCs(Slave);
>
> - SpiSetBaudRate (CoreClock, SpiMaxFreq);
> + SpiSetBaudRate (Slave, CoreClock, SpiMaxFreq);
>
> Reg = MmioRead32 (SpiRegBase + SPI_CONF_REG);
> Reg &= ~(SPI_CPOL_MASK | SPI_CPHA_MASK | SPI_TXLSBF_MASK | SPI_RXLSBF_MASK);
> @@ -194,21 +199,22 @@ MvSpiTransfer (
> {
> SPI_MASTER *SpiMaster;
> UINT64 Length;
> - UINT32 Iterator, Reg, SpiRegBase;
> + UINT32 Iterator, Reg;
> UINT8 *DataOutPtr = (UINT8 *)DataOut;
> UINT8 *DataInPtr = (UINT8 *)DataIn;
> UINT8 DataToSend = 0;
> + UINTN SpiRegBase;
>
> SpiMaster = SPI_MASTER_FROM_SPI_MASTER_PROTOCOL (This);
>
> - SpiRegBase = PcdGet32 (PcdSpiRegBase);
> + SpiRegBase = Slave->HostRegisterBaseAddress;
>
> Length = 8 * DataByteCount;
>
> EfiAcquireLock (&SpiMaster->Lock);
>
> if (Flag & SPI_TRANSFER_BEGIN) {
> - SpiActivateCs (Slave->Cs);
> + SpiActivateCs (Slave);
> }
>
> // Set 8-bit mode
> @@ -245,7 +251,7 @@ MvSpiTransfer (
> }
>
> if (Flag & SPI_TRANSFER_END) {
> - SpiDeactivateCs ();
> + SpiDeactivateCs (Slave);
> }
>
> EfiReleaseLock (&SpiMaster->Lock);
> @@ -312,6 +318,10 @@ MvSpiSetupSlave (
> Slave->Mode = Mode;
> }
>
> + Slave->HostRegisterBaseAddress = PcdGet32 (PcdSpiRegBase);
> + Slave->CoreClock = PcdGet32 (PcdSpiClockFrequency);
> + Slave->MaxFreq = PcdGet32 (PcdSpiMaxFrequency);
> +
> SpiSetupTransfer (This, Slave);
>
> return Slave;
> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
> index 0cf7914..b8981f3 100644
> --- a/Platform/Marvell/Include/Protocol/Spi.h
> +++ b/Platform/Marvell/Include/Protocol/Spi.h
> @@ -52,6 +52,8 @@ typedef struct {
> INTN MaxFreq;
> SPI_MODE Mode;
> NOR_FLASH_INFO *Info;
> + UINTN HostRegisterBaseAddress;
> + UINTN CoreClock;
> } SPI_DEVICE;
>
> typedef
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 21+ messages in thread