diff mbox series

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

Message ID 20181117004524.31851-2-ard.biesheuvel@linaro.org
State New
Headers show
Series ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB | expand

Commit Message

Ard Biesheuvel Nov. 17, 2018, 12:45 a.m. UTC 
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(-)

-- 
2.17.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Nov. 19, 2018, 7:05 p.m. UTC | #1 
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

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 19, 2018, 7:09 p.m. UTC | #2 
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

> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Nov. 19, 2018, 7:16 p.m. UTC | #3 
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Nov. 19, 2018, 7:23 p.m. UTC | #4 
On Mon, Nov 19, 2018 at 11:09:32AM -0800, Ard Biesheuvel wrote:
> > > @@ -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)


OK, I'm happy with that.
If Thomas/Nariman don't object:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


/
    Leif

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

> > >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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