Re: [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Mon, 19 Nov 2018 at 11:05, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> 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)
>

Yes.

Before:

Mapping table
     BLK1: Alias(s):
          VenHw(F9B94AE2-8BA6-409B-9D56-B9B417F53CB3)
     BLK0: Alias(s):
          VenHw(8047DB4B-7E9C-4C0C-8EBC-DFBBAACACE8F)

After:

Mapping table
     BLK0: Alias(s):
          VenHw(93E34C7E-B50E-11DF-9223-2443DFD72085,00)
     BLK1: Alias(s):
          VenHw(93E34C7E-B50E-11DF-9223-2443DFD72085,01)


> 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
> >