Hi Sahil,
Thank you for this patch.
Please find my feedback inline marked [SAMI].
Regards,
Sami Mujawarnd
From: sahil <sahil@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 <sahil@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.
[SAMI] You have the documentation header already in P30NorFlashDeviceLib.. Can you put that in the header file here as well, please?+ 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. + );
+
#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;
+}