Hi,
> -----Original Message-----
> From: s...@google.com <s...@google.com> On Behalf Of Simon Glass
> Sent: Thursday, August 30, 2018 8:21 AM
> To: Jagdish Gediya <jagdish.ged...@nxp.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Prabhakar Kushwaha
> <prabhakar.kushw...@nxp.com>; York Sun <york....@nxp.com>; Poonam
> Aggrwal <poonam.aggr...@nxp.com>; Bin Meng <bmeng...@gmail.com>;
> Tom Rini <tr...@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.ged...@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.ged...@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
Thanks,
Jagdish
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
