Hi,

On 28 August 2018 at 08:49, Jagdish Gediya jagdish.gediya@nxp.com wrote:

Currently binman calculates '_skip_at_start' based on 'end-at-4gb' property and it is used for x86 images.

For Powerpc mpc85xx based CPU, CONFIG_SYS_TEXT_BASE is the first entry offset which can be 0xeff40000 or 0xfff40000 for nor flash boot, 0x201000 for sd boot etc, so "_skip_at_start" should be set to CONFIG_SYS_TEXT_BASE.

'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE + Image size != 4gb.

Add new property "skip-at-start" in Section class so that '_skip_at_start' can be calculated either based on "end-at-4gb" or based on "skip-at-start".

Signed-off-by: Jagdish Gediya jagdish.gediya@nxp.com

Changes for v2: - Renamed 'start-pos' property to 'skip-at-start' - Updated README

tools/binman/README | 9 +++++++++ tools/binman/bsection.py | 1 + 2 files changed, 10 insertions(+)

Please add a test for this feature. You will need to add a new numbered dts in tools/binman/tests and test in ftest.py

diff --git a/tools/binman/README b/tools/binman/README index cb34171..7b4bf2e 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -397,6 +397,15 @@ end-at-4gb: 8MB ROM, the offset of the first entry would be 0xfff80000 with this option, instead of 0 without this option.

+skip-at-start:

•   This property specifies the first entry offset if not 0.
  

•   For Powerpc Book-E architecture, CONFIG_SYS_TEXT_BASE is the first
  

•   entry offset which can be 0xeff40000 or 0xfff40000 for nor flash boot,
  

•   0x201000 for sd boot etc.
  

Can you say 'entry offset of the first entry. It can be ...'. I think it is clearer.

•   'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
  

•   Image size != 4gb.
  
  

Examples of the above options can be found in the tests. See the tools/binman/test directory. diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py index a0bd1b6..68997b7 100644 --- a/tools/binman/bsection.py +++ b/tools/binman/bsection.py @@ -79,6 +79,7 @@ class Section(object): self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0) self._sort = fdt_util.GetBool(self._node, 'sort-by-offset') self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')

•    self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start', 0)
  

This is a bit pedantic, but...

I think this needs to drop the '0' default value. Also in the __init__ constructor, set _skip_at_start to None....

     if self._end_4gb and not self._size:
         self._Raise("Section size must be provided when using end-at-4gb")
     if self._end_4gb:

...here you need to check that self._skip_at_start is None, so people don't set both properties. Then set it to 0 if not set and not end_4gb. Something like:

if self._end_4gb: if if self._skip_at_start is not None: self.Raise... self._skip_at_start = 0x100000000 - self._size else: self._skip_at_start = 0

Does that make sense? This needs a test too...

-- 2.7.4

Regards, Simon

Hi,

On 30 August 2018 at 23:22, Jagdish Gediya jagdish.gediya@nxp.com wrote:

Hi,

...

-----Original Message----- From: sjg@google.com sjg@google.com On Behalf Of Simon Glass Sent: Thursday, August 30, 2018 8:21 AM To: Jagdish Gediya jagdish.gediya@nxp.com Cc: U-Boot Mailing List u-boot@lists.denx.de; Prabhakar Kushwaha prabhakar.kushwaha@nxp.com; York Sun york.sun@nxp.com; Poonam Aggrwal poonam.aggrwal@nxp.com; Bin Meng bmeng.cn@gmail.com; Tom Rini trini@konsulko.com Subject: Re: [PATCH v2 3/8] binman: Add a new "skip-at-start" property in Section class

Hi,

On 28 August 2018 at 08:49, Jagdish Gediya jagdish.gediya@nxp.com wrote:

...

Currently binman calculates '_skip_at_start' based on 'end-at-4gb' property and it is used for x86 images.

For Powerpc mpc85xx based CPU, CONFIG_SYS_TEXT_BASE is the first entry offset which can be 0xeff40000 or 0xfff40000 for nor flash boot, 0x201000 for sd boot etc, so "_skip_at_start" should be set to CONFIG_SYS_TEXT_BASE.

'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE + Image size != 4gb.

Add new property "skip-at-start" in Section class so that '_skip_at_start' can be calculated either based on "end-at-4gb" or based on "skip-at-start".

Signed-off-by: Jagdish Gediya jagdish.gediya@nxp.com

Changes for v2: - Renamed 'start-pos' property to 'skip-at-start' - Updated README

tools/binman/README | 9 +++++++++ tools/binman/bsection.py | 1 + 2 files changed, 10 insertions(+)

Please add a test for this feature. You will need to add a new numbered dts in tools/binman/tests and test in ftest.py

...

diff --git a/tools/binman/README b/tools/binman/README index cb34171..7b4bf2e 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -397,6 +397,15 @@ end-at-4gb: 8MB ROM, the offset of the first entry would be 0xfff80000 with this option, instead of 0 without this option.

+skip-at-start:

•   This property specifies the first entry offset if not 0.
  

•   For Powerpc Book-E architecture, CONFIG_SYS_TEXT_BASE is the first
  

•   entry offset which can be 0xeff40000 or 0xfff40000 for nor flash boot,
  

•   0x201000 for sd boot etc.
  

Can you say 'entry offset of the first entry. It can be ...'. I think it is clearer.

...

•   'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE
  

...

•   Image size != 4gb.
  
  

Examples of the above options can be found in the tests. See the tools/binman/test directory. diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py index a0bd1b6..68997b7 100644 --- a/tools/binman/bsection.py +++ b/tools/binman/bsection.py @@ -79,6 +79,7 @@ class Section(object): self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0) self._sort = fdt_util.GetBool(self._node, 'sort-by-offset') self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')

•    self._skip_at_start = fdt_util.GetInt(self._node,
  

• 'skip-at-start', 0)

This is a bit pedantic, but...

I think this needs to drop the '0' default value. Also in the __init__ constructor, set _skip_at_start to None....

...

     if self._end_4gb and not self._size:
         self._Raise("Section size must be provided when using end-at-4gb")
     if self._end_4gb:

...here you need to check that self._skip_at_start is None, so people don't set both properties. Then set it to 0 if not set and not end_4gb. Something like:

if self._end_4gb: if if self._skip_at_start is not None: self.Raise... self._skip_at_start = 0x100000000 - self._size else: self._skip_at_start = 0

Does that make sense? This needs a test too...

I think it should be checked that self._skip_at_start is None before setting it to 0 in else part. What's your opinion on below implementation?

    self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
    self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start')
    if self._end_4gb:
        if not self._size:
            self._Raise("Section size must be provided when using end-at-4gb")
        if self._skip_at_start is not None:
            self._Raise("Provide either 'end-at-4gb' or 'skip-at-start'")
        else:
            self._skip_at_start = 0x100000000 - self._size
    else:
        if self._skip_at_start is None:
            self._skip_at_start = 0

That looks OK to me. Make sure you change __init__() to set it to None instead of 0.

Regards, Simon

On Tue, Aug 28, 2018 at 11:53 AM Jagdish Gediya jagdish.gediya@nxp.com wrote:

This entry contains the Powerpc mpc85xx boot page and resetvec

nits: PowerPC

Please fix the title as well

sections.

Signed-off-by: Jagdish Gediya jagdish.gediya@nxp.com

Changes for v2: - Updated README for new binman entry - Added test

tools/binman/README.entries | 14 +++++++++++- .../etype/powerpc_mpc85xx_bootpg_resetvec.py | 25 ++++++++++++++++++++++ tools/binman/ftest.py | 8 +++++++ .../test/80_powerpc_mpc85xx_bootpg_resetvec.dts | 16 ++++++++++++++ 4 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py create mode 100644 tools/binman/test/80_powerpc_mpc85xx_bootpg_resetvec.dts

diff --git a/tools/binman/README.entries b/tools/binman/README.entries index c6e7b22..99440d6 100644 --- a/tools/binman/README.entries +++ b/tools/binman/README.entries @@ -221,6 +221,18 @@ See README.x86 for information about Intel binary blobs.

+Entry: powerpc-mpc85xx-bootpg-resetvec: Powerpc mpc85xx bootpg + resetvec code for U-Boot

nits: PowerPC, please fix this globally in this patch

+-----------------------------------------------------------------------------------------

+Properties / Entry arguments:

• • filename: Filename of u-boot-br.bin (default 'u-boot-br.bin')

+This enrty is valid for Powerpc mpc85xx cpus. This entry holds +'bootpg + resetvec' code for Powerpc mpc85xx CPUs which needs to be +placed at offset 'RESET_VECTOR_ADDRESS - 0xffc'.

Entry: section: Entry that contains other entries

@@ -543,7 +555,7 @@ Properties / Entry arguments: - kernelkey: Name of the kernel key to use (inside keydir) - preamble-flags: Value of the vboot preamble flags (typically 0)

-Chromium OS signs the read-write firmware and kernel, writing the signature +Chromium OS signs the read-write firmware and kernel, writing the signature in this block. This allows U-Boot to verify that the next firmware stage and kernel are genuine.

diff --git a/tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py b/tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py new file mode 100644 index 0000000..37cf408 --- /dev/null +++ b/tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py @@ -0,0 +1,25 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright 2018 NXP +# +# Entry-type module for the Powerpc mpc85xx bootpg and resetvec code for U-Boot +#

+from entry import Entry +from blob import Entry_blob

+class Entry_powerpc_mpc85xx_bootpg_resetvec(Entry_blob):

• """Powerpc mpc85xx bootpg + resetvec code for U-Boot
• Properties / Entry arguments:

•    - filename: Filename of u-boot-br.bin (default 'u-boot-br.bin')
  

• This enrty is valid for Powerpc mpc85xx cpus. This entry holds
• 'bootpg + resetvec' code for Powerpc mpc85xx CPUs which needs to be
• placed at offset 'RESET_VECTOR_ADDRESS - 0xffc'.
• """
• def __init__(self, section, etype, node):

•    Entry_blob.__init__(self, section, etype, node)
  

• def GetDefaultFilename(self):

•    return 'u-boot-br.bin'
  

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index a8456c2..7a6a5ae 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -39,6 +39,7 @@ U_BOOT_SPL_DTB_DATA = 'spldtb' U_BOOT_TPL_DTB_DATA = 'tpldtb' X86_START16_DATA = 'start16' X86_START16_SPL_DATA = 'start16spl' +PPC_MPC85XX_BR_DATA = 'ppcmpc85xxbr' U_BOOT_NODTB_DATA = 'nodtb with microcode pointer somewhere in here' U_BOOT_SPL_NODTB_DATA = 'splnodtb with microcode pointer somewhere in here' FSP_DATA = 'fsp' @@ -90,6 +91,7 @@ class TestFunctional(unittest.TestCase): TestFunctional._MakeInputFile('vga.bin', VGA_DATA) self._ResetDtbs() TestFunctional._MakeInputFile('u-boot-x86-16bit.bin', X86_START16_DATA)

•    TestFunctional._MakeInputFile('u-boot-br.bin', PPC_MPC85XX_BR_DATA)
     TestFunctional._MakeInputFile('spl/u-boot-x86-16bit-spl.bin',
                                   X86_START16_SPL_DATA)
     TestFunctional._MakeInputFile('u-boot-nodtb.bin', U_BOOT_NODTB_DATA)
  

@@ -756,6 +758,12 @@ class TestFunctional(unittest.TestCase): data = self._DoReadFile('33_x86-start16.dts') self.assertEqual(X86_START16_DATA, data[:len(X86_START16_DATA)])

• def testPackPowerpcMpc85xxBootpgResetvec(self):

•    """Test that an image with powerpc-mpc85xx-bootpg-resetvec can be
  

•    created"""
  

•    data = self._DoReadFile('80_powerpc_mpc85xx_bootpg_resetvec.dts')
  

•    self.assertEqual(PPC_MPC85XX_BR_DATA, data[:len(PPC_MPC85XX_BR_DATA)])
  

• def _RunMicrocodeTest(self, dts_fname, nodtb_data, ucode_second=False): """Handle running a test for insertion of microcode

diff --git a/tools/binman/test/80_powerpc_mpc85xx_bootpg_resetvec.dts b/tools/binman/test/80_powerpc_mpc85xx_bootpg_resetvec.dts new file mode 100644 index 0000000..8f4b16c --- /dev/null +++ b/tools/binman/test/80_powerpc_mpc85xx_bootpg_resetvec.dts @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright 2018 NXP
• */

+/dts-v1/;

+/ {

•   #address-cells = <1>;
  

•   #size-cells = <1>;
  

•   binman {
  

•           powerpc-mpc85xx-bootpg-resetvec {
  

•           };
  

•   };
  

+};

Regards, Bin

On Tue, Aug 28, 2018 at 11:53 AM Jagdish Gediya jagdish.gediya@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@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.

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

•   depends on MPC85xx
  

config BOOKE bool default y --

Regards, Bin