[U-Boot] [PATCH v2 6/8] powerpc: mpc85xx: Use binman to embed dtb inside u-boot

[Jagdish Gediya]

Hi,

> -----Original Message-----
> From: Bin Meng <bmeng.cn at gmail.com>
> Sent: Tuesday, August 28, 2018 2:47 PM
> To: Jagdish Gediya <jagdish.gediya at nxp.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Prabhakar Kushwaha
> <prabhakar.kushwaha at nxp.com>; York Sun <york.sun at nxp.com>; Poonam
> Aggrwal <poonam.aggrwal at nxp.com>; Simon Glass <sjg at chromium.org>;
> Tom Rini <trini at konsulko.com>
> Subject: Re: [PATCH v2 6/8] powerpc: mpc85xx: Use binman to embed dtb
> inside u-boot
> 
> On Tue, Aug 28, 2018 at 11:53 AM Jagdish Gediya <jagdish.gediya at nxp.com>
> wrote:
> >
> > Below is the sequence to embed dtb inside u-boot,
> 
> nits: U-Boot
> 
> > 1. Remove bootpg and resetvec section if needed 2. Append dtb 3.
> > Append bootpg and resetvec section back if removed in step 1
> >
> > Above procedure is required only when CONFIG_MPC85xx and
> > CONFIG_OF_SEPARATE are defined.
> >
> > Add new config CONFIG_MPC85XX_HAVE_RESET_VECTOR to indicate that
> image
> > have resetvec section. step 1 and step 3 described above are
> 
> have -> has. step 1 -> Step 1
> 
> > required only if this config is y.
> >
> > Signed-off-by: Jagdish Gediya <jagdish.gediya at nxp.com>
> > ---
> > Changes for v2:
> >         - Don't change the generic target
> >         - Add new config option to use binman
> >
> >  Makefile                         | 23 ++++++++++++++++++++++-
> >  arch/powerpc/cpu/mpc85xx/Kconfig |  4 ++++
> >  2 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index b5bf8ab..03baa74 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -861,6 +861,10 @@ ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),)
> >  ALL-y += init_sp_bss_offset_check
> >  endif
> >
> > +ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy)
> > +ALL-y += u-boot-with-dtb.bin
> > +endif
> > +
> >  LDFLAGS_u-boot += $(LDFLAGS_FINAL)
> >
> >  # Avoid 'Not enough room for program headers' error on binutils 2.28
> onwards.
> > @@ -983,7 +987,8 @@ spl/u-boot-spl.srec: spl/u-boot-spl FORCE
> >         $(call if_changed,objcopy)
> >
> >  OBJCOPYFLAGS_u-boot-nodtb.bin := -O binary \
> > -               $(if $(CONFIG_X86_16BIT_INIT),-R .start16 -R .resetvec)
> > +               $(if $(CONFIG_X86_16BIT_INIT),-R .start16 -R .resetvec) \
> > +               $(if $(CONFIG_MPC85XX_HAVE_RESET_VECTOR),-R .bootpg -R
> > + .resetvec)
> >
> >  binary_size_check: u-boot-nodtb.bin FORCE
> >         @file_size=$(shell wc -c u-boot-nodtb.bin | awk '{print $$1}')
> > ; \ @@ -1202,6 +1207,18 @@ u-boot-with-spl.sfp: spl/u-boot-spl.sfp u-
> boot.img FORCE
> >         $(call if_changed,socboot)
> >  endif
> >
> > +ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy)
> 
> This looks odd. Both CONFIG_OF_SEPARATE and CONFIG_OF_EMBED should
> be supported by binman.

Entry type 'u-boot-dtb-with-ucode' requires file 'u-boot.dtb' which is built only for CONFIG_OF_SEPARATE.

> > +u-boot-with-dtb.bin: u-boot.bin u-boot.dtb \
> > +       $(if $(CONFIG_MPC85XX_HAVE_RESET_VECTOR), u-boot-br.bin)
> FORCE
> > +       $(call if_changed,binman)
> > +
> > +ifeq ($(CONFIG_MPC85XX_HAVE_RESET_VECTOR),y)
> > +OBJCOPYFLAGS_u-boot-br.bin := -O binary -j .bootpg -j .resetvec
> > +u-boot-br.bin: u-boot FORCE
> > +       $(call if_changed,objcopy)
> > +endif
> > +endif
> > +
> >  # x86 uses a large ROM. We fill it with 0xff, put the 16-bit stuff
> > (including  # reset vector) at the top, Intel ME descriptor at the
> > bottom, and U-Boot in  # the middle. This is handled by binman based
> > on an image description in the @@ -1296,8 +1313,12 @@
> > spl/u-boot-spl.pbl: spl/u-boot-spl.bin FORCE  ifeq ($(ARCH),arm)
> > UBOOT_BINLOAD := u-boot.img  else
> > +ifeq ($(CONFIG_MPC85xx)$(CONFIG_OF_SEPARATE),yy)
> > +UBOOT_BINLOAD := u-boot-with-dtb.bin
> > +else
> >  UBOOT_BINLOAD := u-boot.bin
> >  endif
> > +endif
> >
> >  OBJCOPYFLAGS_u-boot-with-spl-pbl.bin = -I binary -O binary --pad-
> to=$(CONFIG_SPL_PAD_TO) \
> >                           --gap-fill=0xff diff --git
> > a/arch/powerpc/cpu/mpc85xx/Kconfig
> b/arch/powerpc/cpu/mpc85xx/Kconfig
> > index 19e8d02..7d139ff 100644
> > --- a/arch/powerpc/cpu/mpc85xx/Kconfig
> > +++ b/arch/powerpc/cpu/mpc85xx/Kconfig
> > @@ -1143,6 +1143,10 @@ config ARCH_T4240
> >         imply CMD_REGINFO
> >         imply FSL_SATA
> >
> > +config MPC85XX_HAVE_RESET_VECTOR
> > +       bool "Indicate reset vector at CONFIG_RESET_VECTOR_ADDRESS -
> 0xffc"
> 
> I don't think you want people to turn this option on and off, no? You
> probably need move the "string" to the help paragraph.

This option need to be turned on/off.  Only NOR boot have the reset vector and bootpg section at CONFIG_RESET_VECTOR_ADDRESS - 0xffc. Other boot do not have the reset vector but they have the bootpg section at some other address where it don't need to be removed and joined.

This option indicates that reset vector and bootpg sections are placed at CONFIG_RESET_VECTOR_ADDRESS - 0xffc and it need to be turned on/off according to boot device.

> > +       depends on MPC85xx
> > +
> >  config BOOKE
> >         bool
> >         default y
> > --
> 
> Regards,
> Bin