[PATCH v2 6/8] binman: Add DTBH support

[Sam Day]

Greetings Simon,

On Monday, 8 June 2026 at 11:27 PM, Simon Glass <sjg at chromium.org> wrote:

> Hi Sam,
>
> On 2026-06-07T23:37:09, Sam Day via B4 Relay
> <devnull+me.samcday.com at kernel.org> wrote:
> > binman: Add DTBH support
> >
> > DTBH, used by Samsung S-BOOT bootloaders, is similar to QCDT. That is,
> > it's a multi-record container that carries vendor-specific magic values
> > to assist the previous bootloader in picking the appropriate FDT for the
> > booting device.
> >
> > dtbTool-exynos was used as a reference for this implementation.
> >
> > Link: https://github.com/dsankouski/dtbtool-exynos
> > Signed-off-by: Sam Day <me at samcday.com>
> >
> > tools/binman/etype/android_boot.py               |  20 +++
> >  tools/binman/etype/dtbh.py                       | 173 +++++++++++++++++++++++
> >  tools/binman/ftest.py                            |  24 ++++
> >  tools/binman/test/vendor/dtbh.dts                |  29 ++++
> >  tools/binman/test/vendor/dtbh_bad_model_info.dts |  19 +++
> >  5 files changed, 265 insertions(+)
>
> > diff --git a/tools/binman/etype/dtbh.py b/tools/binman/etype/dtbh.py
> > new file mode 100644
> > index 00000000000..90769e5d016
> > --- /dev/null
> > +++ b/tools/binman/etype/dtbh.py
> > @@ -0,0 +1,173 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# Entry-type module for Samsung Android DTBH tables
> > +
> > +import struct
> > +
> > +from binman.entry import Entry
> > +from binman.etype.section import Entry_section
> > +from dtoc import fdt_util
>
> A lot of duplication between this file and qcdt.py - _align_up(),
> _pad(), _DtbEntryName(), _GetPayloadSubnodes(), ReadEntries(),
> _GetPageSize(), _GetDtbData() are essentially identical. Perhaps you
> can factor the common bits into a helper or even a shared base class
> (something like Entry_vendor_dt_table), with the two etypes only
> carrying their format-specific bits.

Affirmative. v3 introduces a Entry_Android_vendor_dt_table base class to
reduce the duplication between QCDT and DTBH impls.

>
> > diff --git a/tools/binman/etype/dtbh.py b/tools/binman/etype/dtbh.py
> > @@ -0,0 +1,173 @@
> > +    def BuildSectionData(self, required):
> > +...
> > +        header_size = _align_up(12 + len(dtbs) * 32 + 4, page_size)
>
> What is the trailing '+ 4' for? Nothing gets written after the
> records, and the QCDT calculation uses just '12 + len(dtbs) * 24'. If
> it's meant to reserve a terminator cell, the code should emit it too;
> otherwise please drop it (or add a comment explaining the layout
> invariant from dtbTool-exynos).

dtbTool-exynos refers to it as an "end of table marker". v3 introduces
named constants with comments to de-mystify this spooky behaviour.

>
> > diff --git a/tools/binman/etype/dtbh.py b/tools/binman/etype/dtbh.py
> > @@ -0,0 +1,173 @@
> > +    @staticmethod
> > +    def _GetDtbRootU32(node, data, propname):
> > +        import libfdt
> > +
> > +        try:
> > +            fdt = libfdt.Fdt(data)
> > +            root = fdt.path_offset('/')
> > +            prop = fdt.getprop(root, propname)
> > +        except libfdt.FdtException as exc:
> > +            raise ValueError("Node '%s': Missing required DTB root property "
> > +                             ''%s'' % (node.path, propname)) from exc
>
> Please move 'import libfdt' to module-level imports - that's the
> convention in binman. Also, catching FdtException to report 'Missing
> required DTB root property' will mis-report any other libfdt failure
> (e.g. a malformed FDT). Validate the FDT once up front and only wrap
> the getprop() call, so genuine parse failures surface with their real
> cause.

Fair. Discussed further, below.

>
> > diff --git a/tools/binman/etype/dtbh.py b/tools/binman/etype/dtbh.py
> > @@ -0,0 +1,173 @@
> > +DTBH_MAGIC = b'DTBH'
> > +DTBH_VERSION = 2
> > +DTBH_PLATFORM_CODE_DEF = 0x50a6
> > +DTBH_SUBTYPE_CODE_DEF = 0x217584da
> > +DTBH_SPACE = 0x20
>
> DTBH_SPACE is unexplained. Please add a comment describing what this
> field means in the DTBH record layout (and why 0x20 is the right
> constant), or make it a documented property like 'platform' and
> 'subtype' if it varies in the wild.

It's just a magic 0x20 referred to as a "space delimiter" in dtbTool-exynos.
Added a comment to that effect in v3.

>
> > diff --git a/tools/binman/etype/android_boot.py b/tools/binman/etype/android_boot.py
> > @@ -141,6 +141,26 @@ class Entry_android_boot(Entry_section):
> > +        A legacy DTBH abootimg, the kind some Samsung bootloaders expect:
> > +
> > +        android-boot {
> > +            header-version = <0>;
> > +...
>
> The QCDT example added in patch 4 does not set header-version
> explicitly, while this one does. Please make them consistent.

Fixed in v3.

>
> > diff --git a/tools/binman/test/vendor/dtbh.dts b/tools/binman/test/vendor/dtbh.dts
> > @@ -0,0 +1,29 @@
> > +     model_info-chip = <7870>;
> > +     model_info-hw_rev = <6>;
> > +     model_info-hw_rev_end = <6>;
>
> These properties end up at the root of the test DTB, which is what
> 'u-boot-dtb' embeds, so _GetDtbRootU32() picks them back out of the
> payload. That works but is subtle - a one-line comment at the top of
> the test DTS noting that the test relies on the binman test DTB being
> its own DTBH payload would save the next person a few minutes.
>

It's subtle indeed, and also quite confusing. There's no need
for these values to leak into the control FDT, I was just following the
prior art too closely. v3 opts to pluck these values from the dtbh {} node
rather than hunting for them in the root of the DTB.

The end result is that QCDT and DTBH can share even more code, since they're
both hunting for these record-level values in the same place and same way.

> > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > @@ -5701,6 +5701,30 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> > +    def testAndroidBootDtbh(self):
> > +        """Test that binman can produce a DTBH container"""
> > +        data, dtb_data, _map, _dtb = self._DoReadFileDtb(
> > +            'vendor/dtbh.dts', use_real_dtb=True)
> > +
> > +        dtb_size = tools.align(len(dtb_data), 0x800)
> > +
> > +        self.assertEqual(b'DTBH', data[:4])
> > +        self.assertEqual((2, 1), struct.unpack_from('<II', data, 4))
> > +        self.assertEqual((7870, 0x50a6, 0x217584da, 6, 6, 0x800,
> > +                          dtb_size, 0x20),
> > +                         struct.unpack_from('<8I', data, 12))
>
> Please also add a test exercising non-default 'platform', 'subtype'
> and 'page-size', and one with more than one dtb-* subnode - at the
> moment only the single-record default-everything path is covered,
> which leaves the multi-record offset arithmetic untested.

Done and done.

Thanks again Simon,
-Sam

>
> Regards,
> Simon
>