Hi Alper, On Tue, 25 Aug 2020 at 12:01, Alper Nebi Yasak <alpernebiya...@gmail.com> wrote: > > When reading subentries of each image, the FIT entry type directly > concatenates their contents without padding them according to their > offset, size, align, align-size, align-end, pad-before, pad-after > properties. > > This patch makes sure these properties are respected by offloading this > image-data building to the section etype, where each subnode of the > "images" node is processed as a section. Alignments and offsets are > respective to the beginning of each image. For example, the following > fragment can end up having "u-boot-spl" start at 0x88 within the final > FIT binary, while "u-boot" would then end up starting at e.g. 0x20088. > > fit { > description = "example"; > > images { > kernel-1 { > description = "U-Boot with SPL"; > type = "kernel"; > arch = "arm64"; > os = "linux"; > compression = "none"; > > u-boot-spl { > }; > u-boot { > align = <0x10000>; > }; > }; > }; > } > > Signed-off-by: Alper Nebi Yasak <alpernebiya...@gmail.com> > --- > > tools/binman/etype/fit.py | 22 +++---- > tools/binman/ftest.py | 24 ++++++++ > .../test/166_fit_image_subentry_alignment.dts | 57 +++++++++++++++++++ > 3 files changed, 93 insertions(+), 10 deletions(-) > create mode 100644 tools/binman/test/166_fit_image_subentry_alignment.dts
This is a nice enhancement. A few nits below. > > diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py > index 75712f4409..f136a2c254 100644 > --- a/tools/binman/etype/fit.py > +++ b/tools/binman/etype/fit.py > @@ -62,7 +62,7 @@ class Entry_fit(Entry): > """ > super().__init__(section, etype, node) > self._fit = None > - self._fit_content = defaultdict(list) > + self._fit_images = {} Can you add a Properties comment above for this? > self._fit_props = {} > > def ReadNode(self): > @@ -91,15 +91,18 @@ class Entry_fit(Entry): > > rel_path = node.path[len(base_node.path):] > has_images = depth == 2 and rel_path.startswith('/images/') > + if has_images: > + entry = Entry.Create(self.section, node, etype='section') > + entry.ReadNode() > + self._fit_images[rel_path] = entry > + > for subnode in node.subnodes: > if has_images and not (subnode.name.startswith('hash') or > subnode.name.startswith('signature')): > # This is a content node. We collect all of these > together > # and put them in the 'data' property. They do not appear > # in the FIT. This comment should move along with the code you moved. Perhaps here you can mention that it is not a content node. > - entry = Entry.Create(self.section, subnode) > - entry.ReadNode() > - self._fit_content[rel_path].append(entry) > + pass > else: > with fsw.add_node(subnode.name): > _AddNode(base_node, depth + 1, subnode) > @@ -150,13 +153,12 @@ class Entry_fit(Entry): > Returns: > New fdt contents (bytes) > """ > - for path, entries in self._fit_content.items(): > + for path, image in self._fit_images.items(): I think section is a better name than image. In binman, 'image' means the final output file, containing a collection of binaries. The FIT itself therefore ends up in an image, so calling the components of the FIT 'images' is confusing. > node = fdt.GetNode(path) > - data = b'' > - for entry in entries: > - if not entry.ObtainContents(): > - return False > - data += entry.GetData() > + if not image.ObtainContents(): > + return False > + image.Pack(0) > + data = image.GetData() > node.AddData('data', data) > > fdt.Sync(auto_resize=True) [..] Please also do check code coverage: binman test -T Regards, Simon