Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 11/14] Silicon/ARM/NeoverseN1Soc: NOR flash library for N1Sdp
Hi Sahil,
Please see my response marked inline as [SAMI].
Regards,
Sami Mujawar
On 21/05/2024 10:24 am, sahil via groups.io wrote:
Hi Sami,
Thank you for reviewing the patches.
Please find my comments inline below marked as [SAHIL].
On Thu, 16 May 2024 at 20:54, Sami Mujawar via groups.io
wrote:
Hi Sahil,
Thank you for this patch.
I have some suggestions marked inline below, otherwise this patch looks good to
me.
With that fixed,
Reviewed-by: Sami Mujawar
Regards,
Sami Mujawar
On 23/04/2024 06:56 am, Sahil Kaushal wrote:
From: sahil
Add NOR flash library, this library provides APIs for getting the list
of NOR flash devices on the platform.
[SAMI] I think the information in the commit message of patch 10/14 would be
more useful here.
Not mandatory, but it may be useful to have an ASCII diagram to explain the
flash partitioning.
[/SAMI]
[SAHIL]
I will add more information to the commit message.
As for the ASCII diagram, as the size of the images can change, it
will not be possible to make an accurate diagram.
[/SAHIL]
[SAMI] See if you can represent the layout, otherwise ignore my comment.
[/SAMI]
Signed-off-by: sahil
---
Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf | 34 ++
Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c | 65
2 files changed, 99 insertions(+)
diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
new file mode 100644
index ..fad3bca79d3a
--- /dev/null
+++ b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
@@ -0,0 +1,34 @@
+## @file
+# NOR flash lib for ARM Neoverse N1 platform.
+#
+# Copyright (c) 2024, ARM Limited. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION= 0x0001001B
+ BASE_NAME = NorFlashNeoverseN1SocLib
+ FILE_GUID = 7006fcf1-a585-4272-92e3-b286b1dff5bb
+ MODULE_TYPE= DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = NorFlashPlatformLib
+
+[Sources.common]
+ NorFlashLib.c
+
+[Packages]
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Platform/ARM/ARM.dec
+ Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
+
+[LibraryClasses]
+ BaseLib
+
+[FixedPcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
new file mode 100644
index ..a48db9c74548
--- /dev/null
+++ b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
@@ -0,0 +1,65 @@
+/** @file
+* NOR flash lib for ARM Neoverse N1 platform
+*
+* Copyright (c) 2024, ARM Limited. All rights reserved.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include
+#include
+#include
+
+#define FW_ENV_REGION_BASE FixedPcdGet32 (PcdFlashNvStorageVariableBase)
+#define FW_ENV_REGION_SIZE (FixedPcdGet32 (PcdFlashNvStorageVariableSize) + \
+FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) + \
+FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
[SAMI] Would it be an issue if someone were to increase the storage variable
sizes above?
How can you prevent someone overwriting the flash region used by the SCP?
Would it make sense to add a check in NorFlashPlatformInitialization() ?
[/SAMI]
[SAHIL]
With the current code, the firmware images take ~2.5MB and the
variable storage region starts at 15MB
offset from the base. The firmware image's size is unlikely to change much.
Therefore, it is unlikely to cause any issue if someone tries to
increase the variable
region by decreasing the PcdFlashNvStorageVariableBase.
If we still want to add a check, I can add an assert checking
PcdFlashNvStorageVariableBase offset is
not <= to a particular offset (maybe 8MB) in NorFlashPlatformInitialization().
Would this be fine?
[SAMI] Yes, an assert at the appropriate place would be good. Also think
if it would make sense to add a check and return an error. [/SAMI]
[/SAHIL]
+
+STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
+ {
+/// Environment variable region
+NEOVERSEN1SOC_SCP_QSPI_AHB_BASE,///< device base
+FW_ENV_REGION_BASE, ///< region base
+FW_ENV_REGION_SIZE, ///< region size
+SIZE_4KB, ///< block size
+ },
+};
+
+/**
+ Dummy implementation of NorFlashPlatformInitialization to
+ comply with NorFlashPlatformLib structure.
+
Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 11/14] Silicon/ARM/NeoverseN1Soc: NOR flash library for N1Sdp
Hi Sami,
Thank you for reviewing the patches.
Please find my comments inline below marked as [SAHIL].
On Thu, 16 May 2024 at 20:54, Sami Mujawar via groups.io
wrote:
>
> Hi Sahil,
>
> Thank you for this patch.
>
> I have some suggestions marked inline below, otherwise this patch looks good
> to me.
>
> With that fixed,
>
> Reviewed-by: Sami Mujawar
>
> Regards,
>
> Sami Mujawar
>
> On 23/04/2024 06:56 am, Sahil Kaushal wrote:
>
> From: sahil
>
> Add NOR flash library, this library provides APIs for getting the list
> of NOR flash devices on the platform.
>
> [SAMI] I think the information in the commit message of patch 10/14 would be
> more useful here.
>
> Not mandatory, but it may be useful to have an ASCII diagram to explain the
> flash partitioning.
>
> [/SAMI]
[SAHIL]
I will add more information to the commit message.
As for the ASCII diagram, as the size of the images can change, it
will not be possible to make an accurate diagram.
[/SAHIL]
>
> Signed-off-by: sahil
> ---
> Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf | 34 ++
> Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c | 65
>
> 2 files changed, 99 insertions(+)
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
> b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
> new file mode 100644
> index ..fad3bca79d3a
> --- /dev/null
> +++ b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
> @@ -0,0 +1,34 @@
> +## @file
>
> +# NOR flash lib for ARM Neoverse N1 platform.
>
> +#
>
> +# Copyright (c) 2024, ARM Limited. All rights reserved.
>
> +#
>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +#
>
> +##
>
> +
>
> +[Defines]
>
> + INF_VERSION= 0x0001001B
>
> + BASE_NAME = NorFlashNeoverseN1SocLib
>
> + FILE_GUID = 7006fcf1-a585-4272-92e3-b286b1dff5bb
>
> + MODULE_TYPE= DXE_DRIVER
>
> + VERSION_STRING = 1.0
>
> + LIBRARY_CLASS = NorFlashPlatformLib
>
> +
>
> +[Sources.common]
>
> + NorFlashLib.c
>
> +
>
> +[Packages]
>
> + MdeModulePkg/MdeModulePkg.dec
>
> + MdePkg/MdePkg.dec
>
> + Platform/ARM/ARM.dec
>
> + Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
>
> +
>
> +[LibraryClasses]
>
> + BaseLib
>
> +
>
> +[FixedPcd]
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
>
> diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
> b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
> new file mode 100644
> index ..a48db9c74548
> --- /dev/null
> +++ b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
> @@ -0,0 +1,65 @@
> +/** @file
>
> +* NOR flash lib for ARM Neoverse N1 platform
>
> +*
>
> +* Copyright (c) 2024, ARM Limited. All rights reserved.
>
> +*
>
> +* SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +*
>
> +**/
>
> +
>
> +#include
>
> +#include
>
> +#include
>
> +
>
> +#define FW_ENV_REGION_BASE FixedPcdGet32 (PcdFlashNvStorageVariableBase)
>
> +#define FW_ENV_REGION_SIZE (FixedPcdGet32 (PcdFlashNvStorageVariableSize) +
> \
>
> +FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize)
> + \
>
> +FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
>
> [SAMI] Would it be an issue if someone were to increase the storage variable
> sizes above?
>
> How can you prevent someone overwriting the flash region used by the SCP?
>
> Would it make sense to add a check in NorFlashPlatformInitialization() ?
>
> [/SAMI]
[SAHIL]
With the current code, the firmware images take ~2.5MB and the
variable storage region starts at 15MB
offset from the base. The firmware image's size is unlikely to change much.
Therefore, it is unlikely to cause any issue if someone tries to
increase the variable
region by decreasing the PcdFlashNvStorageVariableBase.
If we still want to add a check, I can add an assert checking
PcdFlashNvStorageVariableBase offset is
not <= to a particular offset (maybe 8MB) in NorFlashPlatformInitialization().
Would this be fine?
[/SAHIL]
>
> +
>
> +STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
>
> + {
>
> +/// Environment variable region
>
> +NEOVERSEN1SOC_SCP_QSPI_AHB_BASE,///< device base
>
> +FW_ENV_REGION_BASE, ///< region base
>
> +FW_ENV_REGION_SIZE, ///< region size
>
> +SIZE_4KB, ///< block size
>
> + },
>
> +};
>
> +
>
> +/**
>
> + Dummy implementation of NorFlashPlatformInitialization to
>
> + comply with NorFlashPlatformLib structure.
>
> +
>
> + @retvalEFI_SUCCESSSuccess.
>
> +**/
Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 11/14] Silicon/ARM/NeoverseN1Soc: NOR flash library for N1Sdp
Hi Sahil,
Thank you for this patch.
I have some suggestions marked inline below, otherwise this patch looks
good to me.
With that fixed,
Reviewed-by: Sami Mujawar
Regards,
Sami Mujawar
On 23/04/2024 06:56 am, Sahil Kaushal wrote:
From: sahil
Add NOR flash library, this library provides APIs for getting the list
of NOR flash devices on the platform.
[SAMI] I think the information in the commit message of patch 10/14
would be more useful here.
Not mandatory, but it may be useful to have an ASCII diagram to explain
the flash partitioning.
[/SAMI]
Signed-off-by: sahil
---
Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf | 34 ++
Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c | 65
2 files changed, 99 insertions(+)
diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
new file mode 100644
index ..fad3bca79d3a
--- /dev/null
+++ b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
@@ -0,0 +1,34 @@
+## @file
+# NOR flash lib for ARM Neoverse N1 platform.
+#
+# Copyright (c) 2024, ARM Limited. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION= 0x0001001B
+ BASE_NAME = NorFlashNeoverseN1SocLib
+ FILE_GUID = 7006fcf1-a585-4272-92e3-b286b1dff5bb
+ MODULE_TYPE= DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = NorFlashPlatformLib
+
+[Sources.common]
+ NorFlashLib.c
+
+[Packages]
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Platform/ARM/ARM.dec
+ Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
+
+[LibraryClasses]
+ BaseLib
+
+[FixedPcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
new file mode 100644
index ..a48db9c74548
--- /dev/null
+++ b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
@@ -0,0 +1,65 @@
+/** @file
+* NOR flash lib for ARM Neoverse N1 platform
+*
+* Copyright (c) 2024, ARM Limited. All rights reserved.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include
+#include
+#include
+
+#define FW_ENV_REGION_BASE FixedPcdGet32 (PcdFlashNvStorageVariableBase)
+#define FW_ENV_REGION_SIZE (FixedPcdGet32 (PcdFlashNvStorageVariableSize) + \
+FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) + \
+FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
[SAMI] Would it be an issue if someone were to increase the storage
variable sizes above?
How can you prevent someone overwriting the flash region used by the SCP?
Would it make sense to add a check in NorFlashPlatformInitialization() ?
[/SAMI]
+
+STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
+ {
+/// Environment variable region
+NEOVERSEN1SOC_SCP_QSPI_AHB_BASE,///< device base
+FW_ENV_REGION_BASE, ///< region base
+FW_ENV_REGION_SIZE, ///< region size
+SIZE_4KB, ///< block size
+ },
+};
+
+/**
+ Dummy implementation of NorFlashPlatformInitialization to
+ comply with NorFlashPlatformLib structure.
+
+ @retvalEFI_SUCCESSSuccess.
+**/
+EFI_STATUS
+NorFlashPlatformInitialization (
+ VOID
+ )
+{
+ return EFI_SUCCESS;
+}
+
+/**
+ Get NOR flash region info
+
+ @param[out]NorFlashDevicesNOR flash regions info.
+ @param[out]Count number of flash instance.
+
+ @retvalEFI_SUCCESSSuccess.
+ @retvalEFI_INVALID_PARAMETER The parameters specified are not valid.
+**/
+EFI_STATUS
+NorFlashPlatformGetDevices (
+ OUT NOR_FLASH_DESCRIPTION **NorFlashDevices,
+ OUT UINT32 *Count
+ )
+{
+ if ((NorFlashDevices == NULL) || (Count == NULL)) {
+return EFI_INVALID_PARAMETER;
+ }
+
+ *NorFlashDevices = mNorFlashDevices;
+ *Count = ARRAY_SIZE (mNorFlashDevices);
+ return EFI_SUCCESS;
+}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118967): https://edk2.groups.io/g/devel/message/118967
Mute This Topic: https://groups.io/mt/105690946/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 11/14] Silicon/ARM/NeoverseN1Soc: NOR flash library for N1Sdp
Reviewed-by: levi.yun
From: devel@edk2.groups.io on behalf of Sahil Kaushal
via groups.io
Sent: 23 April 2024 06:56
To: devel@edk2.groups.io
Cc: Ard Biesheuvel; Leif Lindholm ; Sami Mujawar; Sahil Kaushal
Subject: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 11/14]
Silicon/ARM/NeoverseN1Soc: NOR flash library for N1Sdp
From: sahil
Add NOR flash library, this library provides APIs for getting the list
of NOR flash devices on the platform.
Signed-off-by: sahil
---
Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf | 34 ++
Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c | 65
2 files changed, 99 insertions(+)
diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
new file mode 100644
index ..fad3bca79d3a
--- /dev/null
+++ b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.inf
@@ -0,0 +1,34 @@
+## @file
+# NOR flash lib for ARM Neoverse N1 platform.
+#
+# Copyright (c) 2024, ARM Limited. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION= 0x0001001B
+ BASE_NAME = NorFlashNeoverseN1SocLib
+ FILE_GUID = 7006fcf1-a585-4272-92e3-b286b1dff5bb
+ MODULE_TYPE= DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = NorFlashPlatformLib
+
+[Sources.common]
+ NorFlashLib.c
+
+[Packages]
+ MdeModulePkg/MdeModulePkg.dec
+ MdePkg/MdePkg.dec
+ Platform/ARM/ARM.dec
+ Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec
+
+[LibraryClasses]
+ BaseLib
+
+[FixedPcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
diff --git a/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
new file mode 100644
index ..a48db9c74548
--- /dev/null
+++ b/Silicon/ARM/NeoverseN1Soc/Library/NorFlashLib/NorFlashLib.c
@@ -0,0 +1,65 @@
+/** @file
+* NOR flash lib for ARM Neoverse N1 platform
+*
+* Copyright (c) 2024, ARM Limited. All rights reserved.
+*
+* SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include
+#include
+#include
+
+#define FW_ENV_REGION_BASE FixedPcdGet32 (PcdFlashNvStorageVariableBase)
+#define FW_ENV_REGION_SIZE (FixedPcdGet32 (PcdFlashNvStorageVariableSize) + \
+FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) + \
+FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
+
+STATIC NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
+ {
+/// Environment variable region
+NEOVERSEN1SOC_SCP_QSPI_AHB_BASE,///< device base
+FW_ENV_REGION_BASE, ///< region base
+FW_ENV_REGION_SIZE, ///< region size
+SIZE_4KB, ///< block size
+ },
+};
+
+/**
+ Dummy implementation of NorFlashPlatformInitialization to
+ comply with NorFlashPlatformLib structure.
+
+ @retvalEFI_SUCCESSSuccess.
+**/
+EFI_STATUS
+NorFlashPlatformInitialization (
+ VOID
+ )
+{
+ return EFI_SUCCESS;
+}
+
+/**
+ Get NOR flash region info
+
+ @param[out]NorFlashDevicesNOR flash regions info.
+ @param[out]Count number of flash instance.
+
+ @retvalEFI_SUCCESSSuccess.
+ @retvalEFI_INVALID_PARAMETER The parameters specified are not valid.
+**/
+EFI_STATUS
+NorFlashPlatformGetDevices (
+ OUT NOR_FLASH_DESCRIPTION **NorFlashDevices,
+ OUT UINT32 *Count
+ )
+{
+ if ((NorFlashDevices == NULL) || (Count == NULL)) {
+return EFI_INVALID_PARAMETER;
+ }
+
+ *NorFlashDevices = mNorFlashDevices;
+ *Count = ARRAY_SIZE (mNorFlashDevices);
+ return EFI_SUCCESS;
+}
--
2.25.1
-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118161): https://edk2.groups.io/g/devel/message/118161
Mute This Topic: https://groups.io/mt/105690946/7717249
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [yeoreum@arm.com]
-=-=-=-=-=-=
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent
