Hi Sam,
On Wed, 10 Jun 2026 at 02:43, Sam Day <[email protected]> wrote:
>
> Greetings Simon,
>
> On Monday, 8 June 2026 at 11:27 PM, Simon Glass <[email protected]> wrote:
>
> > Hi Sam,
> >
> > On 2026-06-07T23:37:09, Sam Day via B4 Relay
> > <[email protected]> wrote:
> > > binman: Add QCDT support
> > >
> > > This vendor-specific format is used by many bootloaders on older qcom
> > > SoCs, such as msm8916. It's a container for N FDTs. Each one is
> > > contained in a record that includes metadata about the platform/variant
> > > it targets. The previous bootloader picks the 'right' record based on
> > > this metadata.
> > >
> > > This initial impl targets a streamlined v2 path, with no support for
> > > different versions or multiple qcom,msm-id/qcom,board-id tuples. If/when
> > > that's needed it will be implemented in a follow-up.
> > >
> > > Signed-off-by: Sam Day <[email protected]>
> > >
> > > tools/binman/etype/android_boot.py | 30 +++++
> > > tools/binman/etype/qcdt.py | 160
> > > +++++++++++++++++++++++++++
> > > tools/binman/ftest.py | 22 ++++
> > > tools/binman/test/vendor/qcdt.dts | 20 ++++
> > > tools/binman/test/vendor/qcdt_bad_msm_id.dts | 17 +++
> > > 5 files changed, 249 insertions(+)
> >
> > Your comment says "A legacy QCDT abootimg, the kind msm8916 bootloaders
> > expect"
> >
> > Please can you spell out the acronym...
>
> I'd love to, but I think nobody can actually do that. There's not
> really any canonical impl or reference point for this stuff. I believe the
> reason everyone calls it "QCDT" is because that's the magic identifier in
> the header.
>
> Maybe it means "QualComm Device(tree) Table"? Or perhaps it means
> "Qualcomm/CodeAurora Device Trees"? We may never truly know :)
Hilarious :-)
>
> > I would also prefer qcom_xxx
> > for the etype name where xxx is whatever this means, since presumably
> > this is Qualcomm-specific.
>
> Given the acronym doesn't have a canonical definition, and the limited
> references to it use the "QCDT" moniker directly, I opted to keep it as-is
> for v3, since `qcom_cdt` is confusing and would be a unique reference that
> exists only in U-Boot. Or would you prefer qcom_qcdt?
Let me look. Mostly it is about being unique...I quite like the idea
of a qcom_ prefix but it isn't very important.
>
> >
> > > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > > @@ -0,0 +1,160 @@
> > > +from binman.entry import Entry
> > > +from binman.etype.section import Entry_section
> > > +from dtoc import fdt_util
> >
> > The _align_up()/_pad() helpers at the top are duplicated from
> > android_boot.py in patch 1, and will be again in dtbh.py in patch 6.
> > Please factor them into a shared helper module (or stick them on
> > Entry_section).
>
> I opted for the latter. v3 introduces a new patch to put these
> shared helpers in Entry_section.
>
> >
> > > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > > @@ -0,0 +1,160 @@
> > > + def ReadEntries(self):
> > > + for node in self._node.subnodes:
> > > + if self.IsSpecialSubnode(node):
> > > + continue
> > > +
> > > + payloads = self._GetPayloadSubnodes(node)
> > > + if len(payloads) > 1:
> > > + raise ValueError("Node '%s': must contain exactly one
> > > DTB "
> > > + "payload subnode" % node.path)
> >
> > Please use self.Raise() throughout - that's the binman convention and
> > it adds the node path for you. The static/classmethod helpers should
> > become plain methods so they can use self.Raise() too.
>
> Ack, done in v3.
>
> >
> > > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > > @@ -0,0 +1,160 @@
> > > + dtb_offset = _align_up(12 + len(dtbs) * 24, page_size)
> > > + records = []
> > > + payloads = bytearray()
> > > + for msm_id, board_id, dtb in dtbs:
> > > + dtb_size = _align_up(len(dtb), page_size)
> > > + records.append((*msm_id, *board_id, dtb_offset, dtb_size))
> > > + payloads += _pad(dtb, page_size)
> > > + dtb_offset += dtb_size
> > > +
> > > + qcdt = bytearray(struct.pack('<4sII', QCDT_MAGIC, QCDT_VERSION,
> > > + len(records)))
> > > + for platform_id, soc_rev, variant_id, subtype, offset, size in
> > > records:
> > > + qcdt += struct.pack('<IIIIII', platform_id, variant_id,
> > > subtype,
> > > + soc_rev, offset, size)
> >
> > The 12 and 24 magic numbers should be struct.calcsize() of named
> > header/record format strings, the way android_boot.py does for
> > BOOT_IMAGE_HEADER_V0_SIZE. That also documents the layout.
>
> Hm yeah, I got lazy here. v3 removes the magic values and formalizes
> the format/size calculations like android_boot does.
>
> >
> > Also, unpacking msm-id as (platform_id, soc_rev) and board-id as
> > (variant_id, subtype), then packing in the swapped order (platform_id,
> > variant_id, subtype, soc_rev) is hard to read. Please build each
> > record directly with the on-disk field order and add a comment
> > referencing the LK/AOSP source for the QCDT v2 record layout.
>
> Ack - this isn't very readable, I agree. Overhauled in v3.
>
> >
> > > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > > @@ -0,0 +1,160 @@
> > > + def BuildSectionData(self, required):
> > > + if not self._node.subnodes:
> > > + raise ValueError("Node '%s': Missing required DTB subnodes" %
> > > + self._node.path)
> >
> > Redundant with the 'if not dtbs:' check further down (which correctly
> > handles the case where the only subnodes are special). Please drop it.
>
> Ack. v3 refactors this heavily and the redundant check is gone.
>
> >
> > > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > > @@ -0,0 +1,160 @@
> > > + def _GetPageSize(self):
> > > + if self._page_size is not None:
> > > + return self._page_size
> > > +
> > > + return getattr(self.section, 'page_size', 2048)
> >
> > The docstring says "defaults to the parent android-boot page size",
> > but in practice this picks it up from the vendor-dt section that
> > _ReadVendorDtEntries() stamps with a page_size attribute. Please
> > update the docstring - or better, but really you should look up the
> > enclosing android-boot section directly rather than relying on an
> > attribute being monkey-patched onto an intermediate generic section.
>
> Indeed. v3 refactors this heavily to introduce a shared base class, which
> prefers a locally set page-size, falling back to traversing the ancestry
> to find an android-boot preference (and finally falling back to 2048).
>
> >
> > > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > > @@ -5673,6 +5673,28 @@ fdt fdtmap Extract the
> > > devicetree blob from the fdtmap
> > > + def testAndroidBootQcdt(self):
> > > + """Test that binman can produce a QCDT container"""
> > > + data, dtb_data, _map, _dtb = self._DoReadFileDtb(
> > > + 'vendor/qcdt.dts', use_real_dtb=True)
> >
> > Only one record is covered. Since the whole point of QCDT is to hold
> > multiple DTBs picked by msm-id/board-id, please add a test with at
> > least two dtb-* records so the offset/size table-walking code is
> > actually exercised.
>
> Agree. Done in v3.
>
> >
> > > + binman: Add QCDT support
> > > +
> > > + This initial impl targets a streamlined v2 path, with no support for
> > > + different versions or multiple qcom,msm-id/qcom,board-id tuples.
> > > If/when
> > > + that's needed it will be implemented in a follow-up.
> >
> > How abouta Link: to the LK/AOSP reference for the QCDT v2 record format?
>
> Good idea, done in v3.
>
Regards,
Simon
