Hi, On 30 August 2018 at 23:22, Jagdish Gediya <[email protected]> wrote: > Hi, > >> -----Original Message----- >> From: [email protected] <[email protected]> On Behalf Of Simon Glass >> Sent: Thursday, August 30, 2018 8:21 AM >> To: Jagdish Gediya <[email protected]> >> Cc: U-Boot Mailing List <[email protected]>; Prabhakar Kushwaha >> <[email protected]>; York Sun <[email protected]>; Poonam >> Aggrwal <[email protected]>; Bin Meng <[email protected]>; >> Tom Rini <[email protected]> >> 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 <[email protected]> >> 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 <[email protected]> >> > --- >> > 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
