From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web09.60077.1606743822660832017 for ; Mon, 30 Nov 2020 05:43:42 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: vijayenthiran.subramaniam@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B7A531042 for ; Mon, 30 Nov 2020 05:43:41 -0800 (PST) Received: from mail-wm1-f47.google.com (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8E8DB3F918 for ; Mon, 30 Nov 2020 05:43:41 -0800 (PST) Received: by mail-wm1-f47.google.com with SMTP id v14so12933028wml.1 for ; Mon, 30 Nov 2020 05:43:41 -0800 (PST) X-Gm-Message-State: AOAM533aU+zkwl5rn9ECQ9w7XkNdIs+kK5omauDZt9JfJcpMWrkl+nL2 6hA8ASdMxloaKxZ5weFP52JPALQ/2kHIxE8tIUs= X-Google-Smtp-Source: ABdhPJw0J/LA5Fw8kus3PZwMnEcNPfjhKs+ApDZ1/IsbEVKV1FiqEpuzDanJDs+CE8VRrpLzYc+4P+Itz+2Z3FCkam4= X-Received: by 2002:a1c:f017:: with SMTP id a23mr23104251wmb.56.1606743816389; Mon, 30 Nov 2020 05:43:36 -0800 (PST) MIME-Version: 1.0 References: <1606310988-10772-1-git-send-email-vijayenthiran.subramaniam@arm.com> In-Reply-To: From: "Vijayenthiran Subramaniam" Date: Mon, 30 Nov 2020 13:43:23 +0000 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH] ArmPlatformPkg: Enable support for flash in 64-bit address space To: devel@edk2.groups.io, sami.mujawar@arm.com Cc: Vijayenthiran Subramaniam , "leif@nuviainc.com" , Ard Biesheuvel , Thomas Abraham , Aditya Angadi Content-Type: text/plain; charset="UTF-8" Hi Sami, On Wed, Nov 25, 2020 at 2:45 PM Sami Mujawar wrote: > > Hi Vijay, > > Thank you for this patch. > > Please see my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Vijayenthiran Subramaniam via groups.io > Sent: 25 November 2020 01:30 PM > To: devel@edk2.groups.io; leif@nuviainc.com; Ard Biesheuvel > Cc: Thomas Abraham ; Aditya Angadi > Subject: [edk2-devel] [PATCH] ArmPlatformPkg: Enable support for flash in 64-bit address space > > The existing NOR flash Dxe driver supports NOR flash devices connected > in the 32-bit address space. Extend this driver to allow NOR flash > devices connected to 64-bit address space to be usable as well. > > Signed-off-by: Vijayenthiran Subramaniam > --- > .../Drivers/NorFlashDxe/NorFlashDxe.c | 13 ++++++-- > .../Drivers/NorFlashDxe/NorFlashDxe.inf | 3 ++ > .../Drivers/NorFlashDxe/NorFlashFvbDxe.c | 31 ++++++++++++++----- > 3 files changed, 37 insertions(+), 10 deletions(-) > > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > index d9e196cbf1..f3fbbafb7d 100644 > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c > @@ -1298,9 +1298,16 @@ NorFlashInitialise ( > > for (Index = 0; Index < mNorFlashDeviceCount; Index++) { > // Check if this NOR Flash device contain the variable storage region > - ContainVariableStorage = > - (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) && > - (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size); > + > + if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) { > + ContainVariableStorage = > + (NorFlashDevices[Index].RegionBaseAddress <= PcdGet64 (PcdFlashNvStorageVariableBase64)) && > + (PcdGet64 (PcdFlashNvStorageVariableBase64) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size); > + } else { > + ContainVariableStorage = > + (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) && > + (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size); > + } > [SAMI] Since you are updating the code here, can you limit the maximum line length, please? > I prefer 80 chars but coding guidelines allows up to 120 see https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/3_quick_reference#3-3-files-general-rules > [/SAMI] > > Status = NorFlashCreateInstance ( > NorFlashDevices[Index].DeviceBaseAddress, > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf > index a647c01687..b2a941d672 100644 > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf > @@ -54,10 +54,13 @@ > gEfiDiskIoProtocolGuid > > [Pcd.common] > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize > + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c > index 9cdd85096a..ecbe009495 100644 > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c > @@ -58,8 +58,17 @@ InitializeFvAndVariableStoreHeaders ( > Headers = AllocateZeroPool(HeadersLength); > > // FirmwareVolumeHeader->FvLength is declared to have the Variable area AND the FTW working area AND the FTW Spare contiguous. > - ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet32(PcdFlashNvStorageFtwWorkingBase)); > - ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet32(PcdFlashNvStorageFtwSpareBase)); > + if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) { > + ASSERT(PcdGet64(PcdFlashNvStorageVariableBase64) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet64(PcdFlashNvStorageFtwWorkingBase64)); > > [SAMI] Please put a space between PcdGetxx and '('. Same applies to ASSERT () as well. > [/SAMI] > > + } else { > + ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet32(PcdFlashNvStorageFtwWorkingBase)); > + } > + > + if (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) != 0) { > + ASSERT(PcdGet64(PcdFlashNvStorageFtwWorkingBase64) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet64(PcdFlashNvStorageFtwSpareBase64)); > + } else { > + ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet32(PcdFlashNvStorageFtwSpareBase)); > + } > > > // Check if the size of the area is at least one block size > ASSERT((PcdGet32(PcdFlashNvStorageVariableSize) > 0) && (PcdGet32(PcdFlashNvStorageVariableSize) / Instance->Media.BlockSize > 0)); > @@ -67,9 +76,16 @@ InitializeFvAndVariableStoreHeaders ( > ASSERT((PcdGet32(PcdFlashNvStorageFtwSpareSize) > 0) && (PcdGet32(PcdFlashNvStorageFtwSpareSize) / Instance->Media.BlockSize > 0)); > > // Ensure the Variable area Base Addresses are aligned on a block size boundaries > - ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) % Instance->Media.BlockSize == 0); > - ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) % Instance->Media.BlockSize == 0); > - ASSERT(PcdGet32(PcdFlashNvStorageFtwSpareBase) % Instance->Media.BlockSize == 0); > + if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) { > + ASSERT(PcdGet64(PcdFlashNvStorageVariableBase64) % Instance->Media.BlockSize == 0); > + ASSERT(PcdGet64(PcdFlashNvStorageFtwWorkingBase64) % Instance->Media.BlockSize == 0); > + ASSERT(PcdGet64(PcdFlashNvStorageFtwSpareBase64) % Instance->Media.BlockSize == 0); > + } > + else { > > [SAMI] Move else to previous line e.g. '} else {' > [/SAMI] > > + ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) % Instance->Media.BlockSize == 0); > + ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) % Instance->Media.BlockSize == 0); > + ASSERT(PcdGet32(PcdFlashNvStorageFtwSpareBase) % Instance->Media.BlockSize == 0); > + } > > [SAMI] This entire code block from line 60 to 90 is asserting when an invalid parameter is detected so could ideally be in DEBUG_CODE (). > But more importantly, in release builds the asserts would be turned off leading to incorrect values being used. I think this code can be improved to return an error code if a parameter is invalid. This would be useful as some of these PCDs are dynamic. > [/SAMI] > > > // > // EFI_FIRMWARE_VOLUME_HEADER > @@ -736,10 +752,11 @@ NorFlashFvbInitialize ( > EFI_MEMORY_UC | EFI_MEMORY_RUNTIME); > ASSERT_EFI_ERROR (Status); > > - mFlashNvStorageVariableBase = PcdGet32 (PcdFlashNvStorageVariableBase); > + mFlashNvStorageVariableBase = (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ? > + FixedPcdGet64 (PcdFlashNvStorageVariableBase64) : FixedPcdGet32 (PcdFlashNvStorageVariableBase); > > // Set the index of the first LBA for the FVB > - Instance->StartLba = (PcdGet32 (PcdFlashNvStorageVariableBase) - Instance->RegionBaseAddress) / Instance->Media.BlockSize; > + Instance->StartLba = (mFlashNvStorageVariableBase - Instance->RegionBaseAddress) / Instance->Media.BlockSize; > > BootMode = GetBootModeHob (); > if (BootMode == BOOT_WITH_DEFAULT_SETTINGS) { > -- > 2.17.1 > Thanks for the review. I will address the changes in the next version of the patch. Regards, Vijay