Re: [edk2-devel] [PATCH RESEND edk2-platforms][PATCH V2 11/14] Silicon/ARM/NeoverseN1Soc: NOR flash library for N1Sdp

["Sami Mujawar" <sami.mujawar@arm.com>]

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
> <sami.mujawar=arm.com@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 <sami.mujawar@arm.com>
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 23/04/2024 06:56 am, Sahil Kaushal wrote:
>>
>> From: sahil <sahil@arm.com>
>>
>> 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 <sahil@arm.com>
>> ---
>>   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 000000000000..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.<BR>
>>
>> +#
>>
>> +#  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 000000000000..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.<BR>
>>
>> +*
>>
>> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> +*
>>
>> +**/
>>
>> +
>>
>> +#include <Library/NorFlashPlatformLib.h>
>>
>> +#include <NeoverseN1Soc.h>
>>
>> +#include <PiDxe.h>
>>
>> +
>>
>> +#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.
>>
>> +
>>
>> +  @retval        EFI_SUCCESS        Success.
>>
>> +**/
>>
>> +EFI_STATUS
>>
>> +NorFlashPlatformInitialization (
>>
>> +  VOID
>>
>> +  )
>>
>> +{
>>
>> +  return EFI_SUCCESS;
>>
>> +}
>>
>> +
>>
>> +/**
>>
>> +  Get NOR flash region info
>>
>> +
>>
>> +  @param[out]    NorFlashDevices        NOR flash regions info.
>>
>> +  @param[out]    Count                  number of flash instance.
>>
>> +
>>
>> +  @retval        EFI_SUCCESS            Success.
>>
>> +  @retval        EFI_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 (#119102): https://edk2.groups.io/g/devel/message/119102
Mute This Topic: https://groups.io/mt/105690946/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-