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

[Elliot Berman]

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@xxxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxx>
> > +
> > +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