[U-Boot] [PATCH v2 3/8] binman: Add a new "skip-at-start" property in Section class

[Simon Glass]

Hi,

On 28 August 2018 at 08:49, Jagdish Gediya <jagdish.gediya at 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 at 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