Message-ID: <CAD=FV=W3CTMkWPMN5GqGg_L_bUT2Q9vLpc43p5kWAf+j5HBEGA@mail.gmail.com>
Date: Wed, 12 Nov 2025 12:59:35 -0800
From: Doug Anderson <dianders@...omium.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Chen-Yu Tsai <wenst@...omium.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Peter Griffin <peter.griffin@...aro.org>, André Draszik <andre.draszik@...aro.org>, 
	Tudor Ambarus <tudor.ambarus@...aro.org>, linux-samsung-soc@...r.kernel.org, 
	Roy Luo <royluo@...gle.com>, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, Julius Werner <jwerner@...omium.org>, 
	William McVicker <willmcvicker@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] arm64: dts: google: Add initial dts for frankel,
 blazer, and mustang

Hi,

On Wed, Nov 12, 2025 at 1:49 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> >>> To avoid fragmenting the discussion, IMO:
> >>> * Let's have the discussion about using the "dts" for SoC and the
> >>>   "dtso" for the boards in response to the bindings (patch #1).
> >>
> >> That's discussion here, bindings are irrelevant to this.

Ummm, OK. In any case, I'm going to wait until our discussion in the
bindings patch about whether SoCs can be final compatibles, then if I
still think extra discussion is needed I'll respond more on this
thread.


> >>> * If we want to have a discussion about putting "board-id" and
> >>>   "model-id" at the root of the board overlays, we can have it
> >>>   here. I'll preemptively note that the "board-id" and "model-id"
> >>>   won't show up in the final combined device tree and they are just
> >>>   used by the tool (mkdtimg). We could change mkdtimg to parse the
> >>>   "compatible" strings of the overlays files (since I've put the IDs
> >>>   there too), but official the docs [1] seem to indicate that
> >>>   top-level properties like this are OK.
> >>>
> >>> In order for these device trees to pass validation without warnings,
> >>> it's assumed you have my dtc patches:
> >>> * https://lore.kernel.org/r/20251110204529.2838248-1-dianders@chromium.org
> >>> * https://lore.kernel.org/r/20251110204529.2838248-2-dianders@chromium.org
> >>>
> >>> [1] https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/Documentation/dt-object-internal.txt?h=main

> >>> +     board-id = <0x070306>;
> >>> +     board-rev = <0x010000>;
> >>
> >> Undocumented ABI, which you cannot document because these properties are
> >> not allowed. You cannot have them.
> >
> > This is part of the discussion I want to have at Plumbers. But I suppose
> > we can start here.
>
> Then the patch should be called RFC as not yet ready for merging. :)
>
> >
> > The Android DTB partition format uses six 32-bit integers for matching,
> > as opposed to a compatible string used in FIT images. Two of the integers
> > are the "id" and "rev" numbers in the example above. The remaining four
> > are custom and left up to the (vendor) bootloader implementation.
> >
> > The values for these fields need to be stored somewhere with the .dts.
> > The compiled DTB is useless if the user cannot build a proper image for
> > the bootloader to consume, and that involves putting in the right numbers
> > in these fields. The android "mkdtimg" tool can either take the values
> > from some known properties within the DTB, or have them fed to it
> > externally.
> >
> > So if we don't want these numbers in the dts itself, then we should come
> > up with some format to store them beside the dts files.
>
> Re-iterating comment from Rob long time ago: adding such new properties
> is fine, but they must come for more than one user and be universal
> across these users.
>
> And of course the ABI needs to be documented which did not happen here.
>
> I indeed said incorrectly that "properties are not allowed". The
> properties could be allowed if we document them according to above Rob's
> comment, but that did not happen.
>
> Adding these properties per one SoC vendor is not really allowed, like
> qcom,board-id and qcom,msm-id, but maybe you intend to make it generic.

Perhaps you have a link to Rob's comment from a long time ago?

As per my comment "after the cut" in the original patch (see above),
for my use case, I'm OK with removing the "board-id" and "board-rev"
here. It wouldn't be terribly hard to teach my tool to parse the
top-level compatible. That being said, it would be nice to allow them
at a top-level like this. As Chen-Yu says, there are other interested
parties.

The official documentation that I referred to in my comment "after the
cut" says this about properties directly in the overlays:

```
/dts-v1/;
/plugin/; /* allow undefined references and record them */
/ {
    .... /* various properties for loader use; i.e. part id etc. */
    fragment@0 {
```

So properties are clearly documented to be allowed here. When I read
the above, I interpret it as the properties are "whatever the expected
loader of this overlay would find convenient".

I am more than happy to document which properties my "loader"
(mkdtimg) needs if you have some proposed place or way for me to
document them. I'm happy to do it in freeform text (or markup) for now
if that's what people would accept. Maybe that lets us get started yet
still document things while we figure out what the needs are?


> > On a similar note, we would have a similar problem with FIT images and
> > overlays. The FIT image format maps a (series of) compatible string(s)
> > to one DTB and any number of overlays. If overlays are involved, then
> > the compatible string cannot come from the DTB itself, and the mapping
> > must be stored somewhere.
>
> I recall, although cannot find now references to, a email talk on the
> list saying that such overlays should have their own compatible, thus
> solving this mapping problem.

If you have more details or if Rob wants to re-iterate his thoughts,
I'm happy to discuss.

In my mind, I'd rather this not be a "compatible" but I'm also not
dead set on that. IMO, though it can be made to work, having a
"compatible" here is sorta backwards from what we want. We faced this
issue in ChromeOS when we used the top-level "compatible" to pick the
device tree. Specifically, the normal usage of "compatible" is to
start with the device tree which has a list of compatible strings.
>From there, we pick a driver that matches. AKA: we start with some
"compatible" strings and find a matching "thing" (a driver). When
using a "compatible" to pick an overlay / device tree, we start with a
"thing" (a known board) and then pick a list of "compatible" strings
that matches it. Hopefully it's clear how that's a bit different.

As I said, the problems are mostly subtle, but this is how we ended up
with the weirdness on Chromebooks where we had a pile of all equal
"compatible" strings at the top level and that made all the DT folks
grumpy. See, for instance, the sc7180-trogdor-lazor-r1.dts file where
"google,lazor-rev1" and "google,lazor-rev2" are both there. We'll pick
this same DTS for both revisions.

It can be made to work, but IMO it's not a perfect fit.

I'd rather us just pick some standard property that documents the
information that the expected loader should need. Maybe you could even
handle more than one loader type?

/ {
  loaders {
    mkdtimg {
      board-id = <0x1234>;
      board-rev = <0x5678>;
    };
    other-loader {
      something-else  <0xaaaa>;
    };
  };

Then under the "loaders" node we have node names that need to match
exactly for various loaders and then properties that they need.

I haven't tested this, but I believe that since the "loaders" isn't
under any "fragment" that it will just be ignored when the overlay is
applied, so it will just be for the consumption of the loader.


-Doug

Powered by blists - more mailing lists