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 :) > 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? > > > 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. Thanks Simon, -Sam > > Regards, > Simon >
