Hi Sahil,
Thank you for this patch.
Please find my feedback inline marked [SAMI].
Regards,
Sami Mujawarnd
On 23/04/2024 06:56 am, Sahil Kaushal wrote:
From: sahil<sa...@arm.com>
This patch adds an optional functionality in NorFlashDxe to fetch and
print NOR Flash information from NorFlashInfoLib using its JEDEC ID.
NOR Flash libraries will implement a function "NorFlashReadID" which
will fetch and return JEDEC ID. This JEDEC ID can be then used to
print NOR Flash info using NorFlashInfoLib. If this functionality is
[SAMI] Can you explain how this information is useful, please? Is it
just for debugging or it can be used for some other purpose.
If it is just for printing the information, then maybe the above
sentence could be silghtly modified, e.g.
This JEDEC ID can be then printed along with the NOR Flash info by
NorFlashInfoLib.
[/SAMI]
not needed then the function can just return EFI_UNSUPPORTED.
Signed-off-by: sahil<sa...@arm.com>
---
Platform/ARM/SgiPkg/SgiPlatform.dsc.inc | 2 ++
Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc | 2 ++
Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc | 2 ++
Platform/ARM/JunoPkg/ArmJuno.dsc | 2 ++
Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc | 2 ++
Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf | 1 +
Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf | 1 +
Platform/ARM/Include/Library/NorFlashDeviceLib.h | 6 ++++++
Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c | 19
+++++++++++++++++++
Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c | 19
+++++++++++++++++++
Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c | 18
++++++++++++++++++
11 files changed, 74 insertions(+)
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
index 3dcf422eab4b..aef7cba5449e 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc
@@ -36,6 +36,8 @@
LcdPlatformLib|Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgiLib.inf
NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf
NorFlashPlatformLib|Platform/ARM/SgiPkg/Library/NorFlashLib/NorFlashLib.inf
+ # NOR flash support
+ NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
diff --git a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
index ab0e2a957a1b..02d684adaebd 100644
--- a/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
+++ b/Platform/ARM/SgiPkg/SgiPlatformMm.dsc.inc
@@ -65,6 +65,8 @@
IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf
NorFlashPlatformLib|Platform/ARM/SgiPkg/Library/NorFlashLib/StandaloneMmNorFlashLib.inf
+ # NOR flash support
+ NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf
diff --git a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
index 70ff049d3248..4e208c539a88 100644
--- a/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
+++ b/Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc
@@ -95,6 +95,8 @@
ArmPlatformSysConfigLib|Platform/ARM/VExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf
NorFlashPlatformLib|Platform/ARM/VExpressPkg/Library/NorFlashArmVExpressLib/NorFlashArmVExpressLib.inf
+ # NOR flash support
+ NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
ResetSystemLib|ArmPkg/Library/ArmSmcPsciResetSystemLib/ArmSmcPsciResetSystemLib.inf
# ARM PL031 RTC Driver
diff --git a/Platform/ARM/JunoPkg/ArmJuno.dsc b/Platform/ARM/JunoPkg/ArmJuno.dsc
index 81d2cbe4359f..946b8680c8c2 100644
--- a/Platform/ARM/JunoPkg/ArmJuno.dsc
+++ b/Platform/ARM/JunoPkg/ArmJuno.dsc
@@ -42,6 +42,8 @@
NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf
NorFlashPlatformLib|Platform/ARM/JunoPkg/Library/NorFlashJunoLib/NorFlashJunoLib.inf
+ # NOR flash support
+ NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
diff --git a/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc
b/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc
index a5805da49c92..ee71bbb1fc09 100644
--- a/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc
+++ b/Platform/ARM/VExpressPkg/PlatformStandaloneMm.dsc
@@ -102,6 +102,8 @@
!if $(ENABLE_UEFI_SECURE_VARIABLE) == TRUE
NorFlashDeviceLib|Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.inf
NorFlashPlatformLib|Platform/ARM/VExpressPkg/Library/NorFlashArmVExpressLib/NorFlashStMmLib.inf
+ # NOR flash support
[SAMI] Nit. Should the above comment be 'NOR flash identification
support'? Same comment for other places in this patch.
+ NorFlashInfoLib|EmbeddedPkg/Library/NorFlashInfoLib/NorFlashInfoLib.inf
VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
VariableFlashInfoLib|MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.inf
AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf
b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf
index 6522968d6c5a..4ab4d6a26926 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -35,6 +35,7 @@
HobLib
IoLib
NorFlashDeviceLib
+ NorFlashInfoLib
NorFlashPlatformLib
UefiBootServicesTableLib
UefiDriverEntryPoint
diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
index eb86d423f106..8b583f77d927 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
@@ -37,6 +37,7 @@
MemoryAllocationLib
MmServicesTableLib
NorFlashDeviceLib
+ NorFlashInfoLib
NorFlashPlatformLib
StandaloneMmDriverEntryPoint
diff --git a/Platform/ARM/Include/Library/NorFlashDeviceLib.h
b/Platform/ARM/Include/Library/NorFlashDeviceLib.h
index 29b8b8901525..1e61f7c935ae 100644
--- a/Platform/ARM/Include/Library/NorFlashDeviceLib.h
+++ b/Platform/ARM/Include/Library/NorFlashDeviceLib.h
@@ -154,4 +154,10 @@ NorFlashUnlock (
IN EFI_TPL OriginalTPL
);
+EFI_STATUS
+NorFlashReadID (
+ IN NOR_FLASH_INSTANCE *Instance,
+ OUT UINT8 *JedecId // Maximum length of JedecId can be upto 6
bytes.
+ );
[SAMI] You have the documentation header already in
P30NorFlashDeviceLib.. Can you put that in the header file here as well,
please?
+
#endif /* NOR_FLASH_DEVICE_LIB_H_ */
diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
index f5c0dadf84e0..1084f4731ce4 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -9,6 +9,7 @@
#include <Library/UefiLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/NorFlashInfoLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/PcdLib.h>
#include <Library/HobLib.h>
@@ -112,6 +113,8 @@ NorFlashCreateInstance (
{
EFI_STATUS Status;
NOR_FLASH_INSTANCE *Instance;
+ NOR_FLASH_INFO *FlashInfo;
+ UINT8 JedecId[6];
ASSERT (NorFlashInstance != NULL);
@@ -138,6 +141,22 @@ NorFlashCreateInstance (
return EFI_OUT_OF_RESOURCES;
}
+ Status = NorFlashReadID (Instance, JedecId);
+ if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) {
+ FreePool (Instance);
[SAMI] What about freeing the Instance->ShadowBuffer buffer? This issue
appears to be present even before your changes. Can you add another
patch before patch 9/14 to fix the memory leak in this funciton, please?
I think it may be better to handle the error at the end of the function
and cleanup the resources allocated, i.e. implement an exit_handler<1,2>
label as needed that frees up the resources and add goto
exit_handler<1,2> from whereever the error occurs, setting up the Status
appropriately to be returned at the end of the function.
+ return Status;
+ }
+
+ if (Status == EFI_SUCCESS) {
+ Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE);
[SAMI] It looks like the memory allocated for FlashInfo is leaked. Also
do you need the memory allocation to be rom the runtime pool? Can you
check, please?
+ if (EFI_ERROR (Status)) {
+ FreePool (Instance);
[SAMI] Same comment as above.
+ return Status;
+ }
+
+ NorFlashPrintInfo (FlashInfo);
+ }
+
[SAMI] I think the above code can be slightly optimised, see snippet below.
<SNIP>
Status = NorFlashReadID (Instance, JedecId);
if (EFI_ERROR (Status)) {
if (Status != EFI_UNSUPPORTED) {
goto error_handler1;
}
} else {
Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE);
if (EFI_ERROR (Status)) {
[SAMI] Question? How critical is this error? Can it not be treated as
JedecID not supported, and the flash driver can proceed without printing
the flash info?
In other words, is it worth adding a warning message that the flash info
cannot be read and proceed with loading the driver?
goto error_handler1;
}
NorFlashPrintInfo (FlashInfo);
}
</SNIP>
if (SupportFvb) {
NorFlashFvbInitialize (Instance);
diff --git a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
index ef9bed37d140..a621b29f34e2 100644
--- a/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
+++ b/Platform/ARM/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
@@ -10,6 +10,7 @@
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/MmServicesTableLib.h>
+#include <Library/NorFlashInfoLib.h>
#include "NorFlashCommon.h"
@@ -106,6 +107,8 @@ NorFlashCreateInstance (
{
EFI_STATUS Status;
NOR_FLASH_INSTANCE *Instance;
+ NOR_FLASH_INFO *FlashInfo;
+ UINT8 JedecId[6];
ASSERT (NorFlashInstance != NULL);
@@ -132,6 +135,22 @@ NorFlashCreateInstance (
return EFI_OUT_OF_RESOURCES;
}
+ Status = NorFlashReadID (Instance, JedecId);
+ if (EFI_ERROR (Status) && (Status != EFI_UNSUPPORTED)) {
+ FreePool (Instance);
+ return Status;
+ }
+
[SAMI] Same comments as in NorFlashDxe.
+ if (Status == EFI_SUCCESS) {
+ Status = NorFlashGetInfo (JedecId, &FlashInfo, TRUE);
+ if (EFI_ERROR (Status)) {
+ FreePool (Instance);
+ return Status;
+ }
+
+ NorFlashPrintInfo (FlashInfo);
+ }
+
if (SupportFvb) {
NorFlashFvbInitialize (Instance);
diff --git a/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
b/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
index d68e237e2e26..5f8f137482ee 100644
--- a/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
+++ b/Platform/ARM/Library/P30NorFlashDeviceLib/P30NorFlashDeviceLib.c
@@ -947,3 +947,21 @@ NorFlashReset (
SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
return EFI_SUCCESS;
}
+
+/**
+ Read JEDEC ID of NOR flash device.
+
+ @param[in] Instance NOR flash Instance of variable store
region.
+ @param[out] JedecId JEDEC ID of NOR flash device.
+ Maximum length of JedecId can be upto
6 bytes.
+
+ @retval EFI_UNSUPPORTED JEDEC ID retrievel not implemented.
+**/
+EFI_STATUS
+NorFlashReadID (
[SAMI] Nit. Rename NorFlashReadID() => NorFlashReadId().
+ IN NOR_FLASH_INSTANCE *Instance,
+ OUT UINT8 *JedecId
+ )
+{
+ return EFI_UNSUPPORTED;
+}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118965): https://edk2.groups.io/g/devel/message/118965
Mute This Topic: https://groups.io/mt/105690944/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
