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

[Simon Glass]

Hi,

On 30 August 2018 at 23:22, Jagdish Gediya <jagdish.gediya at nxp.com> wrote:
> Hi,
>
>> -----Original Message-----
>> From: sjg at google.com <sjg at google.com> On Behalf Of Simon Glass
>> Sent: Thursday, August 30, 2018 8:21 AM
>> 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>; Bin Meng <bmeng.cn at gmail.com>;
>> Tom Rini <trini at 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 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...
> 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