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 firstentry 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
Thanks, Jagdish