Re: [PATCH RFC v3 2/9] dt-bindings: board: Introduce board-id

Elliot Berman
2024-05-29T15:43:58Z

On Sat, May 25, 2024 at 05:54:52PM +0100, Conor Dooley wrote:
> On Wed, May 22, 2024 at 04:54:23PM -0700, Elliot Berman wrote:
> > Device manufcturers frequently ship multiple boards or SKUs under a
> > single softwre package. These software packages ship multiple devicetree
> > blobs and require some mechanims to pick the correct DTB for the boards
> > that use the software package. This patch introduces a common language
> > for adding board identifiers to devicetrees.
> >
> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> > ---
> > .../devicetree/bindings/board/board-id.yaml | 71 ++++++++++++++++++++++
> > 1 file changed, 71 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/board/board-id.yaml b/Documentation/devicetree/bindings/board/board-id.yaml
> > new file mode 100644
> > index 000000000000..894c1e310cbd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/board/board-id.yaml
> > @@ -0,0 +1,71 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/board/board-id.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Board identifiers
> > +description: |
> > + This node contains a list of identifier values for the board(s) supported by
> > + this devicetree. Identifier values are either N-tuples of integers or a
> > + string. The number of items for an N-tuple identifer is determined by the
> > + property name. String identifiers must be suffixed with "-string".
> > +
> > + Every identifier in the devicetree must have a matching value from the board
> > + to be considered a valid devicetree for the board. In other words: if
> > + multiple identifiers are present in the board-id and one identifier doesn't
> > + match against the board, then the devicetree is not applicable. Note this is
> > + not the case where the the board can provide more identifiers than the
> > + devicetree describes: those additional identifers can be ignored.
> > +
> > + Identifiers in the devicetree can describe multiple possible valid values,
> > + such as revision 1 and revision 2.
> > +
> > +maintainers:
> > + - Elliot Berman <quic_eberman@quicinc.com>
> > +
> > +properties:
> > + $nodename:
> > + const: '/'
> > + board-id:
>
>
> Does this need to be
> properties:
> $nodename:
> const: board-id
> ? That's the pattern I see for all top level nodes.
>
> > + type: object
> > + patternProperties:
> > + "^.*(?<!-string)$":
>
> At least this regex now actually works :)
>
> > + $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > + description: |
> > + List of values that match boards this devicetree applies to.
> > + A bootloader checks whether any of the values in this list
> > + match against the board's value.
> > +
> > + The number of items per tuple is determined by the property name,
> > + see the vendor-specific board-id bindings.
> > + "^.*-string$":
> > + $ref: /schemas/types.yaml#/definitions/string-array
>
> Your description above doesn't match a string-array, just a single
> string. That said I'm far from sold on the "thou shalt have -string"
> edict. If every vendor is expected to go and define their own set of
> properties (and provide their own callback in your libfdt PoC) there's
> little to no reason to inflict property naming on them, AFAICT all that
> is gained is a being able to share
> if (string) {
> return fdt_stringlist_contains(prop->data,
> fdt32_to_cpu(prop->len),
> data);
> } else {
> // exact data comparison. data_len is the size of each entry
> if (fdt32_to_cpu(prop->len) % data_len || data_len % 4)
> return -FDT_ERR_BADVALUE;
>
> for (int i = 0; i < fdt32_to_cpu(prop->len); i += data_len) {
> if (!memcmp(&prop->data[i], data, data_len))
> return 1;
> }
>
> return 0;
> }
> in the libfdt PoC? I'd be expecting that a common mechanism would use
> the same "callback" for boards shipped by both Qualcomm and
> $other_vendor. Every vendor having different properties and only sharing
> the board-id node name seems a wee bit like paying lip-service to a
> common mechanism to me. What am I missing?

One way I thought to get the real board-id values from firmware to OS
loader is via DT itself. A firmware-provided DT provides the real
board-id values. In this case, firmware doesn't have any way to say the
board-id property is a string or a number, so I put that info in the DT
property name.

Another way I thought to get the real board-id values from firmware is
via a UEFI protocol. In that case, we could easily share whether the
value is a string or number and we can drop the "-string" suffix bit.

Thanks,
Elliot