public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Leif Lindholm <leif.lindholm@linaro.org>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: edk2-devel@lists.01.org, lersek@redhat.com, philmd@redhat.com,
	hongbo.zhang@linaro.org,
	Thomas Panakamattam Abraham <thomas.abraham@arm.com>,
	Nariman Poushin <nariman.poushin@linaro.org>
Subject: Re: [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks
Date: Mon, 19 Nov 2018 19:05:55 +0000	[thread overview]
Message-ID: <20181119190555.3hlnyds46waiited@bivouac.eciton.net> (raw)
In-Reply-To: <20181117004524.31851-2-ard.biesheuvel@linaro.org>

On Fri, Nov 16, 2018 at 04:45:23PM -0800, 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)
> +        }
>        },
>        { 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,
>    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;
>    Instance->Media.BlockSize = BlockSize;
>    Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1;
>  
> -  CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid);
> +  CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);

Just sanity checking: this sets the VendorGuid to the NorFlashDxe
GUID? (93E34C7E-B50E-11DF-9223-2443DFD72085)

If not, can you explain it to me slowly? :)

Thomas, Nariman: would this change cause any transient issues for
anything that has set Boot#### options in any of your configurations?
And if it would, is that a big deal?
(Ard has a separate patch that fixes up any default values.)

/
    Leif

> +  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;
> -- 
> 2.17.1
> 


  reply	other threads:[~2018-11-19 19:06 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 [this message]
2018-11-19 19:09     ` Ard Biesheuvel
2018-11-19 19:23       ` Leif Lindholm
2018-11-19 19:16   ` Laszlo Ersek
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=20181119190555.3hlnyds46waiited@bivouac.eciton.net \
    --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