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

[Simon Glass]

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.

> 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).

> 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.

> 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.

> 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.

> 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.

> 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.

Regards,
Simon