Re: [PATCH] dt-bindings: firmware: coreboot: Convert to YAML

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/11/2025 11:41, Chen-Yu Tsai wrote:
>>> 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?

Just choose one. Here was capitals, but in description not. Preferably
Coreboot or coreboot


> 
>>> +
>>> +maintainers:
>>> +  - Julius Werner <jwerner@chromium.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@fdfea000 {
>>> +            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.

Lovely, I don't think we should accept growing this binding at all until
coreboot fixes this mess (duplicated entry and missing unit address).


Best regards,
Krzysztof