Re: [PATCH 3/4] Platforms/zx: Add boot manager lib and entries

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Aug 17, 2017 at 11:45:52PM +0800, Jun Nie wrote:
> On 2017年08月10日 22:41, Leif Lindholm wrote:
> > On Wed, Aug 09, 2017 at 10:12:38PM +0800, Jun Nie wrote:
> > > Add boot manager lib and entries, including Android and Grub.
> > > 
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > > ---
> > >   .../Drivers/Zx296718EvbDxe/Zx296718EvbDxe.c        | 105 ++++++
> > >   .../Drivers/Zx296718EvbDxe/Zx296718EvbDxe.inf      |  66 ++++
> > >   .../Library/PlatformBootManagerLib/PlatformBm.c    | 404 +++++++++++++++++++++
> > >   .../Library/PlatformBootManagerLib/PlatformBm.h    |  30 ++
> > >   .../PlatformBootManagerLib.inf                     |  91 +++++
> > >   Silicon/Sanchip/SanchipPkg.dec                     |  29 ++
> > >   6 files changed, 725 insertions(+)
> > >   create mode 100644 Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.c
> > >   create mode 100644 Platform/Sanchip/Zx296718Evb/Drivers/Zx296718EvbDxe/Zx296718EvbDxe.inf
> > >   create mode 100644 Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.c
> > >   create mode 100644 Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBm.h
> > >   create mode 100644 Silicon/Sanchip/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> > >   create mode 100644 Silicon/Sanchip/SanchipPkg.dec
> > > 

> > > diff --git a/Silicon/Sanchip/SanchipPkg.dec b/Silicon/Sanchip/SanchipPkg.dec
> > > new file mode 100644
> > > index 0000000..92d439d
> > > --- /dev/null
> > > +++ b/Silicon/Sanchip/SanchipPkg.dec
> > > @@ -0,0 +1,29 @@
> > > +#
> > > +#  Copyright (C) 2017 Sanechips Technology Co., Ltd.
> > > +#  Copyright (c) 2017, Linaro Ltd.
> > > +#
> > > +#  This program and the accompanying materials
> > > +#  are licensed and made available under the terms and conditions of the BSD License
> > > +#  which accompanies this distribution.  The full text of the license may be found at
> > > +#  http://opensource.org/licenses/bsd-license.php
> > > +#
> > > +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > > +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > > +#
> > > +
> > > +[Defines]
> > > +  DEC_SPECIFICATION              = 0x00010005
> > 
> > 0x00010019/1.25.
> > 
> > > +  PACKAGE_NAME                   = SanchipPkg
> > > +  PACKAGE_GUID                   = f991248f-9e21-4e4f-b344-eaad28e42ec0
> > > +  PACKAGE_VERSION                = 0.1
> > > +
> > > +[Includes.common]
> > > +  Include                        # Root include for the package
> > 
> > This directory does not exist, causing compliation to fail.
> 
> Test shows that this include entry is unnecessary.
> > 
> > > +
> > > +[Guids.common]
> > > +  gSanchipTokenSpaceGuid          =  { 0x9589ba86, 0x58e6, 0x426c, { 0xbb, 0x39, 0x21, 0xf5, 0x68, 0x78, 0xe4, 0x27 } }
> > > +
> > > +[PcdsFixedAtBuild.common]
> > > +  gSanchipTokenSpaceGuid.PcdAndroidBootFile|{ 0x36, 0x8b, 0x73, 0x3a, 0xc5, 0xb9, 0x63, 0x47, 0xab, 0xbd, 0x6c, 0xbd, 0x4b, 0x25, 0xf9, 0xff }|VOID*|0x00000003
> > 
> > What?
> 
> PcdAndroidBootFile is FileGuid for AndroidBoot app file, as PcdShellFile
> Guid in other places. Do you want to hardcode the Guid in PlatformBm.c
> library code?

But you are not setting it in your platform description file, so
what's the point of declaring it here?

Also, it does not feel like something that belongs in a
platform-specific .dec. It would make a lot more sense to me in edk2
EmbeddedPkg/EmbeddedPkg.dec.

/
    Leif