From: Laszlo Ersek <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>, edk2-devel@lists.01.org
Subject: Re: [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks
Date: Mon, 19 Nov 2018 20:16:23 +0100 [thread overview]
Message-ID: <1c220cda-7f03-75ec-33c5-8bca6052034c@redhat.com> (raw)
In-Reply-To: <20181117004524.31851-2-ard.biesheuvel@linaro.org>
On 11/17/18 01:45, Ard Biesheuvel wrote:
> Currently, each flash bank controlled by ArmPlatformPkg/NorFlashDxe
> has its own VendorHw GUID, and instances of NorFlashPlatformLib
> describe each bank to the driver, along with the GUID for each.
>
> This works ok for bare metal platforms, but it would be useful for
> virtual platforms if we could obtain this information from a
> device tree, which would require us to invent GUIDs on the fly,
> given that the 'cfi-flash' binding does not include a GUID.
>
> So instead, let's switch to a single GUID for all flash banks,
> and update the driver's device path handling to include an index
> to identify each bank uniquely.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 15 +++++++++------
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 3 +++
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index 46e815beb343..60a06e4a5058 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -82,10 +82,14 @@ NOR_FLASH_INSTANCE mNorFlashInstanceTemplate = {
> {
> HARDWARE_DEVICE_PATH,
> HW_VENDOR_DP,
> - { (UINT8)sizeof(VENDOR_DEVICE_PATH), (UINT8)((sizeof(VENDOR_DEVICE_PATH)) >> 8) }
> + {
> + (UINT8)(OFFSET_OF(NOR_FLASH_DEVICE_PATH, End)),
> + (UINT8)(OFFSET_OF(NOR_FLASH_DEVICE_PATH, End) >> 8)
> + }
(1) Please add a space character after OFFSET_OF (both instances).
(2) OFFSET_OF suggests a now-missing #pragma pack (1)... added at the
bottom, great.
(3) Normally I would prefer to split this hunk to a prior patch, as a
no-op refactoring. Up to you.
> },
> { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } }, // GUID ... NEED TO BE FILLED
> },
> + 0, // Index
> {
> END_DEVICE_PATH_TYPE,
> END_ENTIRE_DEVICE_PATH_SUBTYPE,
> @@ -99,10 +103,9 @@ NorFlashCreateInstance (
> IN UINTN NorFlashDeviceBase,
> IN UINTN NorFlashRegionBase,
> IN UINTN NorFlashSize,
> - IN UINT32 MediaId,
> + IN UINT32 Index,
(4) Same as (3), also up to you.
> IN UINT32 BlockSize,
> IN BOOLEAN SupportFvb,
> - IN CONST GUID *NorFlashGuid,
> OUT NOR_FLASH_INSTANCE** NorFlashInstance
> )
> {
> @@ -121,11 +124,12 @@ NorFlashCreateInstance (
> Instance->Size = NorFlashSize;
>
> Instance->BlockIoProtocol.Media = &Instance->Media;
> - Instance->Media.MediaId = MediaId;
> + Instance->Media.MediaId = Index;
(5) Ditto.
> Instance->Media.BlockSize = BlockSize;
> Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1;
>
> - CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid);
> + CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);
> + Instance->DevicePath.Index = Index;
>
> Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
> if (Instance->ShadowBuffer == NULL) {
> @@ -1311,7 +1315,6 @@ NorFlashInitialise (
> Index,
> NorFlashDevices[Index].BlockSize,
> ContainVariableStorage,
> - &NorFlashDevices[Index].Guid,
> &mNorFlashInstances[Index]
> );
> if (EFI_ERROR(Status)) {
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> index 5c07694fbfaa..8886aa43d9f3 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> @@ -122,10 +122,13 @@
>
> typedef struct _NOR_FLASH_INSTANCE NOR_FLASH_INSTANCE;
>
> +#pragma pack(1)
> typedef struct {
> VENDOR_DEVICE_PATH Vendor;
> + UINT8 Index;
> EFI_DEVICE_PATH_PROTOCOL End;
> } NOR_FLASH_DEVICE_PATH;
> +#pragma pack()
>
> struct _NOR_FLASH_INSTANCE {
> UINT32 Signature;
>
(6) Given that you introduce this field as UINT8, the
"Instance->DevicePath.Index = Index" assignment in
NorFlashCreateInstance() is liable to trigger UINT32-->UINT8
"truncation" warnings under at least some toolchains, IMO. Can you add
an explicit cast to that assignment (assuming you deem the assignment
safe otherwise)?
If you decide to ignore (3) through (5), I think (1) and (6) can be
fixed up before pushing. In that case:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
next prev parent reply other threads:[~2018-11-19 19:16 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-17 0:45 [PATCH 0/2] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB Ard Biesheuvel
2018-11-17 0:45 ` [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks Ard Biesheuvel
2018-11-19 19:05 ` Leif Lindholm
2018-11-19 19:09 ` Ard Biesheuvel
2018-11-19 19:23 ` Leif Lindholm
2018-11-19 19:16 ` Laszlo Ersek [this message]
2018-11-17 0:45 ` [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically Ard Biesheuvel
2018-11-17 1:29 ` Ard Biesheuvel
2018-11-19 19:10 ` Leif Lindholm
2018-11-19 19:16 ` Ard Biesheuvel
2018-11-19 19:24 ` Leif Lindholm
2018-11-19 19:27 ` Ard Biesheuvel
2018-11-19 19:31 ` Leif Lindholm
2018-11-19 20:02 ` Laszlo Ersek
2018-11-19 20:24 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1c220cda-7f03-75ec-33c5-8bca6052034c@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox