Message-ID: <CAGXv+5FPaJMuN7wCP7g0Rxa0mXD3Ru0rxka=m8B_rv+XUkJPWA@mail.gmail.com>
Date: Tue, 25 Nov 2025 18:41:07 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org, 
	chrome-platform@...ts.linux.dev, Julius Werner <jwerner@...omium.org>, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: firmware: coreboot: Convert to YAML

On Tue, Nov 25, 2025 at 6:08 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On Tue, Nov 25, 2025 at 02:48:49PM +0800, Chen-Yu Tsai wrote:
> > Convert the existing text binding to YAML.
> >
> > The description has been change to reflect the possibility of coreboot
> > inserting the node itself.
> >
> > The example has been modified to compile and pass validation without
> > any errors. A comment was added to note what the firmware actually
> > emits.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@...omium.org>
> > ---
> >  .../devicetree/bindings/firmware/coreboot.txt | 33 ----------
> >  .../bindings/firmware/coreboot.yaml           | 60 +++++++++++++++++++
> >  2 files changed, 60 insertions(+), 33 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/firmware/coreboot.txt
> >  create mode 100644 Documentation/devicetree/bindings/firmware/coreboot.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/coreboot.txt b/Documentation/devicetree/bindings/firmware/coreboot.txt
> > deleted file mode 100644
> > index 4c955703cea8..000000000000
> > --- a/Documentation/devicetree/bindings/firmware/coreboot.txt
> > +++ /dev/null
> > @@ -1,33 +0,0 @@
> > -COREBOOT firmware information
> > -
> > -The device tree node to communicate the location of coreboot's memory-resident
> > -bookkeeping structures to the kernel. Since coreboot itself cannot boot a
> > -device-tree-based kernel (yet), this node needs to be inserted by a
> > -second-stage bootloader (a coreboot "payload").
> > -
> > -Required properties:
> > - - compatible: Should be "coreboot"
> > - - reg: Address and length of the following two memory regions, in order:
> > -     1.) The coreboot table. This is a list of variable-sized descriptors
> > -     that contain various compile- and run-time generated firmware
> > -     parameters. It is identified by the magic string "LBIO" in its first
> > -     four bytes.
> > -     See coreboot's src/commonlib/include/commonlib/coreboot_tables.h for
> > -     details.
> > -     2.) The CBMEM area. This is a downward-growing memory region used by
> > -     coreboot to dynamically allocate data structures that remain resident.
> > -     It may or may not include the coreboot table as one of its members. It
> > -     is identified by a root node descriptor with the magic number
> > -     0xc0389481 that resides in the topmost 8 bytes of the area.
> > -     See coreboot's src/include/imd.h for details.
> > -
> > -Example:
> > -     firmware {
> > -             ranges;
> > -
> > -             coreboot {
> > -                     compatible = "coreboot";
> > -                     reg = <0xfdfea000 0x264>,
> > -                           <0xfdfea000 0x16000>;
> > -             }
> > -     };
> > diff --git a/Documentation/devicetree/bindings/firmware/coreboot.yaml b/Documentation/devicetree/bindings/firmware/coreboot.yaml
> > new file mode 100644
> > index 000000000000..568afd1abb92
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/coreboot.yaml
> > @@ -0,0 +1,60 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/firmware/coreboot.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: COREBOOT firmware information
>
> Coreboot

OK. Side note, coreboot is stylized in all lowercase letters.
Should I follow that or just use standard English rules?

> > +
> > +maintainers:
> > +  - Julius Werner <jwerner@...omium.org>
> > +
> > +description:
> > +  The device tree node to communicate the location of coreboot's
> > +  memory-resident bookkeeping structures to the kernel. Coreboot's
> > +  FIT image payload can insert the node into the device tree. If a
> > +  second-stage bootloader (a coreboot "payload") is used, then it
> > +  is responsible for inserting the node.
> > +
> > +properties:
> > +  compatible:
> > +    const: coreboot
>
> Blank line (it is always here, there is no example without, which makes
> me wonder which file you took as starting point)

I actually converted the existing text file directly, copying
boilerplate, i.e. the top few lines, from another file.

> > +  reg:
> > +    description: Address and length of the following two memory regions
>
> Drop description, redundant.

Ack.

> > +    items:
> > +      - description:
> > +          The coreboot table. This is a list of variable-sized descriptors
> > +          that contain various compile- and run-time generated firmware
> > +          parameters. It is identified by the magic string "LBIO" in its first
> > +          four bytes.
> > +
> > +          See coreboot's src/commonlib/include/commonlib/coreboot_tables.h for
> > +          details.
> > +      - description:
> > +          The CBMEM area. This is a downward-growing memory region used by
> > +          coreboot to dynamically allocate data structures that remain resident.
> > +          It may or may not include the coreboot table as one of its members. It
> > +          is identified by a root node descriptor with the magic number
> > +          0xc0389481 that resides in the topmost 8 bytes of the area.
> > +
> > +          See coreboot's src/include/imd.h for details.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    firmware {
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +        ranges;
> > +
> > +        /* Firmware actually emits "coreboot" node without unit name */
> > +        coreboot@...ea000 {
> > +            compatible = "coreboot";
> > +            reg = <0xfdfea000 0x264>, <0xfdfea000 0x16000>;
>
> That's the same address in both places, so the same one entry. You need
> two distinctive addresses or binding needs changes to have only one item
> as well.

The description does mention that the latter block can include the
former. It's really up to the firmware. If you like I can include
two examples to cover both cases.


ChenYu

Powered by blists - more mailing lists