coreboot-gerrit April 2025

coreboot-gerrit@coreboot.org

1 participants
3140 discussions

[L] Change in coreboot[main]: Documentation: Add Timers, Stopwatch, and Delays
by Martin L Roth (Code Review) 05 Apr '25

05 Apr '25

Martin L Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/87187?usp=email ) Change subject: Documentation: Add Timers, Stopwatch, and Delays ...................................................................... Documentation: Add Timers, Stopwatch, and Delays Change-Id: I3b58817c1ed06e6d7d5d5668b0e38ec8cfedf122 Signed-off-by: Martin Roth <gaumless(a)gmail.com> --- M Documentation/internals/index.md A Documentation/internals/stopwatch.md 2 files changed, 661 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/87187/1 diff --git a/Documentation/internals/index.md b/Documentation/internals/index.md index 2600069..098011c 100644 --- a/Documentation/internals/index.md +++ b/Documentation/internals/index.md @@ -19,5 +19,6 @@ :maxdepth: 1 Timestamps <timestamps.md> +Timers, Stopwatch, and delays <stopwatch.md> ``` diff --git a/Documentation/internals/stopwatch.md b/Documentation/internals/stopwatch.md new file mode 100644 index 0000000..0ca5ae5 --- /dev/null +++ b/Documentation/internals/stopwatch.md @@ -0,0 +1,660 @@ +# Devicetree + +## Introduction to the coreboot devicetree + +The first thing that may come to mind when one hears “DeviceTree" is a +different sort of description file that is generally passed to the Linux +kernel to describe a system’s components. Both that devicetree and +coreboot’s devicetree serve fundamentally the same purpose, but are +otherwise unrelated and have completely different syntax. The term +devicetree was used long before either version was created, and was +initially used in coreboot as a generic term. + +coreboot\'s devicetree’s main use is to define and describe the runtime +configuration and settings of the hardware on a board, chip or device +level. It defines which of the functions of the chips on the board are +enabled, and how they’re configured. + +The devicetree file is parsed during the build process by a utility +named `sconfig`, which translates the devicetree into a tree of C +structures containing the included devices. This code is placed in the +file `static.c` and a couple of header files, all under the `build` +directory. This file is then built into the binaries for the various +coreboot stages and is referred to during the coreboot boot process. + +For the early stages of the coreboot boot process, the data that is +generated by `sconfig` is a useful resource, but this structure is the +critical architectural glue of ramstage. This structure gets filled in +with pointers to every chip\'s initialization code, allowing ramstage to +find and initialize those devices through the `chip_operations` +structures. + +### History of coreboot’s devicetree + +The initial devicetree in coreboot was introduced in 2003 by Ron Minnich +as a part of the linuxbios config file, `config.lb`. At this point both +the devicetree and config options were in the same file. In 2009, +when Kconfig was added into the coreboot build, devicetree was split +out into its own file for each mainboard in a commit with this message: + +```text +devicetree.cb + +The devicetree that formerly resided in src/mainboard/*/*/Config.lb. + +Just without the build system crap +``` + +The devicetree structure was initially mainly used only in ramstage for +PCI device enumeration, configuration and resource allocation. It has +since expanded for use in the pre-ram stages as a read-only structure. + +The language used in the devicetree has been expanded greatly since it +was first introduced as well, adding new features every year or so. + +### Devicetree Registers + +In coreboot, the devicetree register setting is one of the two main +methods used to configure a board’s properties. In this way, devicetree +is similar in function to Kconfig. It’s more flexible in many ways as +it can specify not only single values, but also arrays or structures. +It's also even more static than Kconfig because there’s no update +mechanism for it other than editing the devicetree files. + +Chip-specific configuration values are often set using `register` +definitions within a `chip` block, corresponding to a `struct` defined +in the chip's `chip.h` file. + +For example, in a `chip drivers/gpio/acpi` block, you might set a GPE: + +```text +register "gpe0_sts" = "0x42" +``` + +### Adding new static configuration options: Devicetree or Kconfig + +When adding options for a new board or chip, there is frequently a +decision that needs to be made about how the option should be added. +Using the devicetree or Kconfig are the two typical methods of +build-time configuration. Below are some general guidelines on when to +use each. + +Kconfig should be used if the option configures the build in a Makefile, +or if the option is something that should be user selectable. Kconfig +is also preferred if the configuration is a global option and not limited +to a single chip. Another thing Kconfig excels at is handling decisions +based on other configuration options, which devicetree cannot do. + +Devicetree should obviously be used to define the hardware hierarchy. +It's also preferred if the option is only used in C code and is static +for a mainboard, or if the option is chip-specific. As mentioned +earlier, devicetree registers can also define structures or arrays, +which Kconfig cannot. + +Both Kconfig and devicetree can be used in C code for runtime +configuration, but there’s a significant difference in how they are +handled. Because Kconfig generates a `#define` for the choice, the +compiler can eliminate code paths not used by the option. Devicetree +options, however, are actual runtime selections, and the code for all +choices remains in the final build. + +## Basic Devicetree Syntax + +The coreboot devicetree uses a custom language parsed by the `sconfig` +utility. Here's a brief overview of the main keywords and concepts: + +* **`chip <directory>`**: Defines a collection of devices associated + with the code in the specified directory. `sconfig` may also parse a + `chip.h` file within this directory for register definitions. +* **`device <type> <id> [on|off] [alias <name>] ... end`**: Defines a + specific hardware device. + * `<type>`: Specifies the device type (e.g., `pci`, `cpu_cluster`, + `i2c`). + * `<id>`: A unique identifier for the device within its type/bus + (e.g., PCI BDF `17.0`, I2C address `0x50`). + * `on`/`off`: Enables or disables the device definition. + * `alias <name>`: Assigns a human-readable alias for referencing + this device elsewhere (often used in `chipset.cb`). + * `end`: Marks the end of the device definition block. Registers + and other properties are defined between `device` and `end`. +* **`register "<name>" = <value>`**: Sets the value of a configuration + register defined in the corresponding `chip.h` structure. The value + can be a number, string, or complex structure initialization. +* **`probe <field> <option>`**: Used for firmware configuration + (`fw_config`), indicating a setting should be probed at runtime. +* **`ops "<identifier>"`**: Associates a `chip_operations` structure + with the device, used primarily in ramstage for device initialization. +* **`fw_config field <name> size <bits> ... end`**: Defines a firmware + configuration field, often used for passing board-specific data to + payloads. Options within the field are defined using `option`. +* **`ref <alias>`**: Used within `device` definitions in `devicetree.cb` + or `overridetree.cb` to refer to a device defined (usually via an + `alias`) in a lower-level file like `chipset.cb`. +* **`# <comment text>`**: Single-line comments. + +Device definitions can be nested within `chip` blocks. `end` keywords +close the current block (`device` or `chip`). + +## Three levels of devicetree files + +There are currently three different levels of devicetrees used to build +up the structure of components and register values in coreboot. From +the lowest, most general level to the highest and most specific, they +are `chipset.cb`, `devicetree.cb`, and `overridetree.cb`. + +Unless there’s a specific reason to name them something other than these +names, they should be used. + +For newer SoCs and chipsets, there will generally be a `chipset.cb` file. +Every mainboard requires a `devicetree.cb` file, although it can be empty +if everything is inherited from the `chipset.cb`. An `overridetree.cb` +file is only required if variants have differences from the primary +mainboard's `devicetree.cb`. + +### SoC / chipset level, `chipset.cb` + +The `chipset.cb` file was added in October 2020, allowing a single +chipset or SoC to provide a "base level" devicetree, reducing +duplication between mainboards. + +The `chipset.cb` file also typically defines human-readable "aliases" +for particular devices so that mainboards can use those instead of PCI +device/function numbers or other hardware identifiers. + +The use of the `chipset.cb` file is specified in Kconfig by the +`CHIPSET_DEVICETREE` symbol, which provides the path to the file. + +In a `chipset.cb` file, you might see lines like this: + +```text +# Chip definition for the SoC/chipset itself +chip soc/intel/common/block + + # Define PCI device 17.0, alias it to "sata", and default it off + device pci 17.0 alias sata off end + + # Define PCI device 1e.0, alias it to "uart0", and default it off + device pci 1e.0 alias uart0 off end + +end # chip soc/intel/common/block +``` + +This defines the devices, assigns aliases, and sets their default state. + +### Primary mainboard level, `devicetree.cb` + +Each mainboard must have a `devicetree.cb` file. The filename and path are +typically set by the `DEVICETREE` Kconfig symbol, defaulting to +`src/mainboard/<VENDOR>/<BOARD>/devicetree.cb`. + +If a mainboard using the above `chipset.cb` wanted both devices enabled, +its `devicetree.cb` might contain: + +```text +# Reference the SATA device by its alias and enable it +device ref sata on end + +# Reference the UART0 device by its alias and enable it +device ref uart0 on end +``` + +The `ref` keyword looks up the device (usually by alias) defined in a +lower-level file (`chipset.cb` in this case) and modifies its properties. + +### Mainboard variant level, `overridetree.cb` + +Introduced in 2018 to reduce duplication and maintenance for board +variants, the `overridetree.cb` file is the most specific level. + +This allows a base `devicetree.cb` at the top mainboard level shared by +all variants. Each variant then only needs an `overridetree.cb` to +specify its differences. + +The override tree filename is set in Kconfig with the +`OVERRIDE_DEVICETREE` symbol and is typically named `overridetree.cb`. + +Finally, if one variant of the mainboard lacked a SATA connector, it +could disable the SATA device again using the following in its specific +`overridetree.cb`: + +```text +# Reference the SATA device by alias and disable it for this variant +device ref sata off end +``` + +## Additional files + +### `chip.h` files + +coreboot looks at a "chip" as a collection of devices. This collection +can be a single logical device or multiple different ones. The `chip` +keyword starts this collection. Following the `chip` keyword is a +directory path (relative to `src/`) containing the code for that chip +or logical block of hardware. + +There may optionally be a `chip.h` file in that directory. If present, +`sconfig` parses this file to define a C structure containing the +"register definitions" for the chip. The values for this structure's +members are set using the `register` keyword in one of the devicetree +files (`chipset.cb`, `devicetree.cb`, `overridetree.cb`). If not +explicitly set, members typically default to 0 or follow standard C +initialization rules. The `chip.h` file frequently also contains C +macros, enums, and sub-structures used for setting the members of the +main register structure. + +The C structure for the chip’s register definition is named after the +directory containing the `chip.h` file, with slashes (`/`) changed to +underscores (`_`), and `_config` appended. The leading `src/` is omitted. + +This means that a line in a devicetree file like: +`chip drivers/i2c/hid` +would cause `sconfig` to look for `src/drivers/i2c/hid/chip.h`. If found, +the register definition structure it contains would be named +`drivers_i2c_hid_config`. + +Here is the content of `src/drivers/i2c/hid/chip.h`: + +```c +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __DRIVERS_I2C_HID_CHIP_H__ +#define __DRIVERS_I2C_HID_CHIP_H__ +#include <drivers/i2c/generic/chip.h> +#define I2C_HID_CID "PNP0C50" + +struct drivers_i2c_hid_config { + struct drivers_i2c_generic_config generic; + uint8_t hid_desc_reg_offset; +}; + +#endif /* __I2C_HID_CHIP_H__ */ +``` + +In a devicetree, you could set `hid_desc_reg_offset` like this: + +```text +chip drivers/i2c/hid + device i2c 0x2c on + # Set the HID descriptor register offset + register "hid_desc_reg_offset" = "0x01" + end +end +``` + +## The `sconfig` utility and generated files + +### `util/sconfig` + +`sconfig` is the tool that parses the coreboot devicetrees and turns +them into a collection of C structures. This is a coreboot-specific +tool, built using flex & bison to define and parse the domain-specific +language used by coreboot’s devicetree. + +`sconfig` is called by the makefiles during the build process and doesn’t +generally need to be run directly. If run manually (e.g., +`build/util/sconfig/sconfig --help`), it shows its command-line options. +The exact options might vary slightly, but typically include: + +```text +usage: sconfig <options> + + -c | --output_c : Path to output static.c file (required) + -r | --output_h : Path to header static.h file (required) + -d | --output_d : Path to header static_devices.h file (required) + -f | --output_f : Path to header static_fw_config.h file (required) + -m | --mainboard_devtree : Path to mainboard devicetree file (required) + -o | --override_devtree : Path to override devicetree file (optional) + -p | --chipset_devtree : Path to chipset/SOC devicetree file (optional) +``` + +### `sconfig` inputs + +The `sconfig` input files `chip.h`, `chipset.cb`, `devicetree.cb`, and +`overridetree.cb` were discussed previously. As the usage above shows, +the only required input file is the mainboard devicetree (`-m`). The +additional devicetree files, `chipset.cb` (`-p`) and `overridetree.cb` +(`-o`), are optional. The `chip.h` files do not need to be specified on +the command line; their locations are determined by the `chip` directory +paths within the `.cb` files. + +Constructing the devicetree input files will be discussed later. + +### `sconfig` outputs + +#### `static.c` + +This is the primary C file generated by `sconfig`. It contains the static +definitions of the device tree structures, including device nodes, bus +links, and register configuration data. + +For historic reasons, `static.c` is generated in the +`build/mainboard/<VENDOR>/<BOARD>` directory. + +#### `static.h` + +The `static.h` file is the main header file included by most coreboot C +files that need access to the devicetree data. It is included by +`src/include/device/device.h`, which provides the primary API +(definitions, structures, function prototypes) for interacting with the +devicetree-generated output. + +`static.h` used to contain all generated declarations directly. As of +October 2020, it simply includes the other two generated header files +(`static_devices.h` and `static_fw_config.h`). This separation allows +the firmware config options (`fw_config`) to be used independently, for +example, by a payload. + +#### `static_devices.h` + +The file `static_devices.h` contains `extern` declarations for all the +device structures (`struct device`) defined in `static.c`. This allows +other C files to reference the generated device tree nodes. + +#### `static_fw_config.h` + +`static_fw_config.h` contains only the `FW_CONFIG_FIELD_*` macro results, +derived from `fw_config` entries in the devicetree. This makes it easily +consumable by payloads or other components needing platform `FW_CONFIG` +data without pulling in the full device tree structure. + +## Devicetree Example + +### A very simple devicetree + +This is the `devicetree.cb` file from +`src/mainboard/sifive/hifive-unleashed`, with line numbers added for +reference. Non-x86 devicetree files are often simpler than their x86 +counterparts. + +```text + 1 # SPDX-License-Identifier: GPL-2.0-only + 2 chip soc/sifive/fu540 + 3 device cpu_cluster 0 on end + 4 end +``` + +This can be broken down as follows: + +Line 1: Comments start with `#`. This line is the SPDX license +identifier for the file. + +Line 2: `chip soc/sifive/fu540` starts a block for the SiFive FU540 SoC. +`sconfig` will look for code and potentially a `chip.h` in +`src/soc/sifive/fu540/`. + +Line 3: `device cpu_cluster 0 on end` defines a device of type +`cpu_cluster` with ID `0`. It's marked as enabled (`on`). Since there are +no registers or other properties defined between `device` and `end`, this +is a simple enablement entry. + +Line 4: `end` closes the block started by the `chip` keyword on line 2. + +### Generated files + +Continuing with the simple `sifive/hifive-unleashed` mainboard example, +these are the files generated by `sconfig` from the devicetree above (as +of mid-2022; exact output can change). Because the input devicetree is +minimal, the generated files are also quite sparse. + +#### `build/static.h` + +```c +#ifndef __STATIC_DEVICE_TREE_H +#define __STATIC_DEVICE_TREE_H + +#include <static_fw_config.h> +#include <static_devices.h> + +#endif /* __STATIC_DEVICE_TREE_H */ +``` +(Includes the other generated headers.) + +#### `build/static_devices.h` + +```c +#ifndef __STATIC_DEVICES_H +#define __STATIC_DEVICES_H +#include <device/device.h> +/* expose_device_names */ +#endif /* __STATIC_DEVICE_NAMES_H */ +``` +(Includes `device/device.h` but contains no actual device externs beyond +the implicit root device, as the simple example didn't define complex +devices requiring separate structs.) + +#### `build/static_fw_config.h` + +```c +#ifndef __STATIC_FW_CONFIG_H +#define __STATIC_FW_CONFIG_H +#endif /* __STATIC_FW_CONFIG_H */ +``` +(Empty because the example `devicetree.cb` did not use `fw_config`.) + +#### `build/mainboard/sifive/hifive-unleashed/static.c` + +##### Includes + +```c +1 #include <boot/coreboot_tables.h> +2 #include <device/device.h> +3 #include <device/pci.h> +4 #include <fw_config.h> +5 #include <static.h> +``` + +Lines 1-5: Includes header files required for the following structure +definitions and macros. + +##### Declarations for chip-ops + +```c +6 +7 #if !DEVTREE_EARLY +8 __attribute__((weak)) struct chip_operations mainboard_ops = {}; +9 extern struct chip_operations soc_sifive_fu540_ops; +10 #endif +``` + +Lines 7 & 10: The `ops` structures inside this `#if !DEVTREE_EARLY` block +are only relevant and linked in ramstage. + +Lines 8-9: Declarations for `chip_operations` structures. This section +expands as more chips are added to the devicetree. +* Line 8: `mainboard_ops` is always present. It’s defined as `weak` + because the mainboard C code may or may not provide this structure. +* Line 9: This `extern` is generated by the `chip soc/sifive/fu540` + declaration in the `devicetree.cb`. There will be a similar line for + every `chip` declared. + +##### `STORAGE` definition + +```c +11 +12 #define STORAGE static __unused DEVTREE_CONST +``` + +Line 12: This macro conditionally adds `const` based on the build stage. +It resolves to `static __unused const` in early stages (pre-RAM) and +`static __unused` in ramstage, where the structures might be modified. + +##### Structure definitions + +```c +13 +14 +15 /* pass 0 */ +16 STORAGE struct bus dev_root_links[]; +17 STORAGE struct device _dev_0; +18 DEVTREE_CONST struct device * DEVTREE_CONST last_dev = &_dev_0; +``` + +Lines 16-18: Forward declarations of the static structures generated by +`sconfig` based on the devicetree input. `_dev_0` corresponds to the +`cpu_cluster 0` device. + +##### Register Structures + +```c +19 +20 /* chip configs */ +``` + +Line 20: This section is empty for this mainboard because the +`soc/sifive/fu540/chip.h` file (if it exists) does not define a register +structure, or the devicetree did not instantiate it using `register`. +Otherwise, this section would contain the static initialization of chip +configuration structures based on `register` entries. + +##### `dev_root` structure + +Lines 21-44: `dev_root`. This structure represents the root of the +coreboot device tree. It is always generated, regardless of the content +of the `devicetree.cb` file. It serves as the entry point for traversing +the tree. + +```c +21 +22 /* pass 1 */ +23 DEVTREE_CONST struct device dev_root = { +24 #if !DEVTREE_EARLY +25 .ops = &default_dev_ops_root, +26 #endif +27 .bus = &dev_root_links[0], +28 .path = { .type = DEVICE_PATH_ROOT }, +29 .enabled = 1, +30 .hidden = 0, +31 .mandatory = 0, +32 .on_mainboard = 1, +33 .link_list = &dev_root_links[0], +34 .sibling = NULL, +35 #if !DEVTREE_EARLY +36 .chip_ops = &mainboard_ops, +37 .name = mainboard_name, +38 #endif +39 .next=&_dev_0, +40 #if !DEVTREE_EARLY +41 #if CONFIG(GENERATE_SMBIOS_TABLES) +42 #endif +43 #endif +44 }; +``` + +* Lines 24-26: Points to a default ramstage `device_operation` + structure (`default_dev_ops_root`) found in + `src/device/root_device.c`. This structure typically does little by + default but can be overridden or utilized by mainboard code via the + `chip_operations->enable_dev()` hook for tasks like ACPI table + generation. +* Line 27: `.bus`: Pointer to the bus structure associated with this + device. For the root device, this points to its own bus link array, + `dev_root_links`. +* Line 28: `.path`: The unique path identifier for this device. The type + is `DEVICE_PATH_ROOT`. +* Lines 29-32: Device status flags. + * `enabled`: Set based on `on`/`off` in the devicetree (always on + for `dev_root`). Can be modified later (e.g., during enumeration + in ramstage). + * `hidden`, `mandatory`: Set only by corresponding keywords in the + devicetree (not used here). + * `on_mainboard`: Indicates the device was defined in the static + devicetree, as opposed to being discovered dynamically (e.g., via + PCI enumeration). Always true for `dev_root`. +* Line 33: `.link_list`: Pointer to the list of child buses attached to + this device. +* Line 34: `.sibling`: Pointer to the next device at the same level in + the tree. Should always be `NULL` for `dev_root`. +* Line 36: `.chip_ops`: Pointer to the mainboard's `chip_operations` + structure (the `weak` `mainboard_ops`). Although not a physical + chip, the mainboard gets this to hook into the boot process like + other chips. +* Line 37: `.name`: A string identifier, typically the mainboard name, + set at build time (from `src/device/root_device.c`). +* Line 39: `.next`: Pointer used internally by `sconfig` during tree + construction. Points to the next device structure processed (`_dev_0`). + +##### `dev_root_links` + +Lines 45-52: The `dev_root` bus structure array. + +This array (`struct bus`) holds pointers defining the bus topology. Each +element represents a link on a bus. `dev_root` acts as the bridge for the +top-level bus. + +A new bus structure array is typically created for each distinct bus type +or domain originating from a bridge device in the devicetree (e.g., PCI +domain 0, LPC bus). + +```c +45 STORAGE struct bus dev_root_links[] = { +46 [0] = { +47 .link_num = 0, +48 .dev = &dev_root, +49 .children = &_dev_0, +50 .next = NULL, +51 }, +52 }; +``` + +* Line 47: `.link_num`: Index of this link within the bus array. +* Line 48: `.dev`: Pointer back to the bridge device structure for this + bus (`dev_root`). +* Line 49: `.children`: Pointer to the first child device structure on + this bus (`_dev_0`). +* Line 50: `.next`: Pointer to the next bridge device on the *parent* + bus. Since `dev_root` has no parent bus, this is `NULL`. + +##### `_dev_0` + +Lines 53-72: The `cpu_cluster` device structure (`_dev_0`). + +This structure corresponds directly to the +`device cpu_cluster 0 on end` line in the `devicetree.cb`. + +```c +53 STORAGE struct device _dev_0 = { +54 #if !DEVTREE_EARLY +55 .ops = NULL, +56 #endif +57 .bus = &dev_root_links[0], +58 .path = {.type=DEVICE_PATH_CPU_CLUSTER,{.cpu_cluster={ .cluster = 0x0 }}}, +59 .enabled = 1, +60 .hidden = 0, +61 .mandatory = 0, +62 .on_mainboard = 1, +63 .link_list = NULL, +64 .sibling = NULL, +65 #if !DEVTREE_EARLY +66 .chip_ops = &soc_sifive_fu540_ops, +67 #endif +68 #if !DEVTREE_EARLY +69 #if CONFIG(GENERATE_SMBIOS_TABLES) +70 #endif +71 #endif +72 }; +``` + +* Lines 54-56: `.ops`: Pointer to a `device_operations` structure. This + is `NULL` because this entry represents the `chip` itself, not a + specific functional sub-device requiring device-level operations. The + chip-level operations are handled by `chip_ops`. +* Line 57: `.bus`: Pointer to the bus structure this device resides on. + Since it's directly under `dev_root`, it points to `dev_root_links[0]`. +* Line 58: `.path`: The unique device path structure (defined in + `src/include/device/path.h`). Type is `DEVICE_PATH_CPU_CLUSTER`, + and the cluster ID is `0`, matching the devicetree entry. This path + is used when searching the tree (e.g., with `dev_find_path()`). +* Lines 59-62: Enumeration Status. Similar to `dev_root`. `enabled = 1` + comes from the `on` keyword. +* Line 63: `.link_list`: Pointer to child buses. `NULL` because this + `cpu_cluster` device doesn't bridge to any further buses in this + simple example. +* Line 64: `.sibling`: Pointer to the next device at the same level + (i.e., another device directly under `dev_root`). `NULL` as it's the + only child. +* Lines 65-67: `.chip_ops`: Pointer to the processor's `chip_operations` + structure (`soc_sifive_fu540_ops`), used in ramstage for SoC/CPU + initialization steps. This link comes from the `chip soc/sifive/fu540` + declaration. +* Lines 68-71: Placeholder for SMBIOS information, enabled by Kconfig. + Not used in this example. -- To view, visit https://review.coreboot.org/c/coreboot/+/87187?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I3b58817c1ed06e6d7d5d5668b0e38ec8cfedf122 Gerrit-Change-Number: 87187 Gerrit-PatchSet: 1 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com>

1 0

[L] Change in coreboot[main]: Documentation: Add Timestamp documentation
by Martin L Roth (Code Review) 05 Apr '25

05 Apr '25

Martin L Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/87186?usp=email ) Change subject: Documentation: Add Timestamp documentation ...................................................................... Documentation: Add Timestamp documentation Change-Id: I6e4cc244edf6cc860cc66165173f134a30a81589 Signed-off-by: Martin Roth <gaumless(a)gmail.com> --- M Documentation/internals/index.md A Documentation/internals/timestamps.md 2 files changed, 338 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/87186/1 diff --git a/Documentation/internals/index.md b/Documentation/internals/index.md index 17dee55..2600069 100644 --- a/Documentation/internals/index.md +++ b/Documentation/internals/index.md @@ -12,3 +12,12 @@ coreboot devicetree language <devicetree_language.md> ``` + +## APIs + +```{toctree} +:maxdepth: 1 + +Timestamps <timestamps.md> + +``` diff --git a/Documentation/internals/timestamps.md b/Documentation/internals/timestamps.md new file mode 100644 index 0000000..82b7be8 --- /dev/null +++ b/Documentation/internals/timestamps.md @@ -0,0 +1,329 @@ +# coreboot Timestamp Implementation + + + +## Introduction + +Timestamps in coreboot are a critical feature for performance analysis, +debugging, and optimization of the boot process. They provide precise +timing information about various stages and operations during system +initialization, allowing developers to identify bottlenecks and optimize +boot performance. The timestamp system is designed to be lightweight, +accurate, and persistent across different boot stages. + + +## Background + +The timestamp implementation in coreboot has evolved over time to meet +the growing needs of firmware development and performance analysis. +Initially designed as a simple timing mechanism, it has grown into a +sophisticated system that: + +- Tracks boot stages and critical operations +- Supports multiple hardware platforms (x86, ARM, RISC-V) +- Provides persistent storage across boot stages +- Enables post-boot analysis of boot performance +- Integrates with vendor-specific firmware components + + +## Timestamp Architecture + +### Data Structures + +The timestamp system uses two main data structures: + +```c +struct timestamp_entry { + uint32_t entry_id; + int64_t entry_stamp; +} __packed; + +struct timestamp_table { + uint64_t base_time; + uint16_t max_entries; + uint16_t tick_freq_mhz; + uint32_t num_entries; + struct timestamp_entry entries[]; /* Variable number of entries */ +} __packed; +``` + +### Memory Layout + +Timestamps are stored in two locations during the boot process: + +1. **Cache Region**: Used during early boot stages (before CBMEM is + available) + - Located in the `timestamp` region defined in memlayout.ld + - Persists across stage transitions + - Limited to 192 entries by default (See `CONFIG_MAX_TIMESTAMP_CACHE_ENTRIES`) + +2. **CBMEM**: Used after CBMEM is initialized + - Provides persistent storage across warm reboots + - Supports more entries + - Automatically synchronized with cache region + + +### Storage Mechanism + +The timestamp system uses a two-phase storage approach: + +1. **Early Boot (ROMSTAGE_OR_BEFORE)**: + - Timestamps are stored in the cache region + - No CBMEM access required + - Limited to 192 entries (configurable via + `CONFIG_MAX_TIMESTAMP_CACHE_ENTRIES`) + +2. **Late Boot (After CBMEM)**: + - Timestamps are stored in CBMEM + - Cache region is synchronized to CBMEM + - Supports more entries + - Persists across warm reboots + + +## API Reference + +### Core Functions + +```c +void timestamp_init(uint64_t base); +``` +Initializes the timestamp system with a base time. Must be called once +in *one* of the `ENV_ROMSTAGE_OR_BEFORE` stages (e.g., bootblock, +romstage). On x86, this must occur before CAR (Cache-as-RAM) is torn +down if called in bootblock or separate romstage. + +```c +void timestamp_add(enum timestamp_id id, int64_t ts_time); +``` +Adds a new timestamp with the specified ID and time value. + +```c +void timestamp_add_now(enum timestamp_id id); +``` +Adds a new timestamp with the current time. + +```c +void timestamp_rescale_table(uint16_t N, uint16_t M); +``` +Applies a scaling factor N/M to all recorded timestamps. + +```c +uint32_t get_us_since_boot(void); +``` +Returns the time since boot in microseconds. + +```c +uint64_t timestamp_get(void); +``` +Returns the current raw timestamp value from the underlying hardware +timer. + +```c +uint64_t get_initial_timestamp(void); +``` +Returns the base_time stored in the timestamp table, representing the +reference point (initial timestamp) the system was initialized with. + +```c +int timestamp_tick_freq_mhz(void); +``` +Returns the timestamp tick frequency in MHz. + +### Timestamp IDs + +The system uses predefined timestamp IDs to mark various boot stages and +operations. These are organized in ranges: + +- 1-99: coreboot boot stages (e.g., `TS_ROMSTAGE_START`, `TS_RAMSTAGE_START`) +- 100-115: Miscellaneous coreboot operations (e.g., `TS_POSTCAR_START`, + `TS_DELAY_START`, `TS_READ_UCODE_START`) +- 500-600: Google/ChromeOS specific (e.g., `TS_VBOOT_START`, `TS_EC_SYNC_START`) +- 900-940: AMD specific (e.g., `TS_AGESA_INIT_EARLY_START`) +- 940-950: Intel ME specific (e.g., `TS_ME_INFORM_DRAM_START`) +- 950-989: Intel FSP specific (e.g., `TS_FSP_MEMORY_INIT_START`) +- 990-999: Intel ME specific (continued) (e.g., `TS_ME_ROM_START`) +- 1000+: Payload specific (e.g., Depthcharge: 1000-1199, ChromeOS + Hypervisor: 1200-1299) + +Refer to `src/commonlib/include/commonlib/timestamp_serialized.h` for +the complete list and descriptions. + + +## Configuration + +### Kconfig Options + +- `COLLECT_TIMESTAMPS`: Enable/disable timestamp collection +- `TIMESTAMPS_ON_CONSOLE`: Print timestamps to console during boot +- `MAX_TIMESTAMP_CACHE_ENTRIES`: Maximum entries in the early cache region + (default 192) + + +### Memory Layout + +The timestamp system requires a `timestamp` region in the memory layout: + +```ld +timestamp : { + _timestamp = .; + *(.timestamp) + . = ALIGN(8); + _etimestamp = .; +} +``` + +### Hardware Considerations + +- x86: Timestamps must be initialized before CAR (Cache-as-RAM) is + torn down if called from bootblock or separate romstage. +- ARM: No special considerations +- RISC-V: No special considerations + + +## Examples + +### Initializing Timestamps + +```c +void bootblock_mainboard_init(void) +{ + timestamp_init(timestamp_get()); +} +``` + +Note: `timestamp_get()` here provides the initial base time. + + +### Adding Custom Timestamps + +```c +void my_custom_function(void) +{ + timestamp_add_now(TS_DEVICE_INITIALIZE); + // ... perform initialization ... + timestamp_add_now(TS_DEVICE_DONE); +} +``` + +### Reading Timestamps + +```c +#include <cbmem.h> +#include <console/console.h> +#include <commonlib/timestamp_serialized.h> + +void analyze_timestamps(void) +{ + struct timestamp_table *ts_table = cbmem_find(CBMEM_ID_TIMESTAMP); + if (!ts_table) { + printk(BIOS_INFO, "Timestamp table not found in CBMEM.\n"); + return; + } + + printk(BIOS_INFO, "Timestamp Table Analysis:\n"); + printk(BIOS_INFO, " Base Time: %lld\n", ts_table->base_time); + printk(BIOS_INFO, " Tick Freq: %u MHz\n", ts_table->tick_freq_mhz); + printk(BIOS_INFO, " Entries: %u / %u\n", + ts_table->num_entries, ts_table->max_entries); + + for (int i = 0; i < ts_table->num_entries; i++) { + struct timestamp_entry *tse = &ts_table->entries[i]; + /* Convert raw timestamp to microseconds relative to base */ + uint64_t time_us = tse->entry_stamp - ts_table->base_time; + if (ts_table->tick_freq_mhz > 0) + time_us /= ts_table->tick_freq_mhz; + + printk(BIOS_INFO, " Entry %d: ID=%u, Raw=%lld, Rel Time=%llu us\n", + i, tse->entry_id, tse->entry_stamp, time_us); + } +} +``` + +## Best Practices + +1. **Initialization**: + - Initialize timestamps as early as possible + - Use a consistent base time across all stages + - Ensure proper memory region allocation + +2. **Adding Timestamps**: + - Use appropriate timestamp IDs + - Add timestamps for significant operations + - Keep timestamp frequency reasonable + - Avoid adding timestamps in performance-critical paths + +3. **Memory Management**: + - Be aware of the entry limit in early boot (default 192, check + `CONFIG_MAX_TIMESTAMP_CACHE_ENTRIES`) + - Monitor timestamp table usage via `cbmem -t` or custom code + - Handle potential overflow conditions if necessary + +4. **Platform-Specific Considerations**: + - Follow platform-specific initialization requirements + - Consider hardware timer limitations + - Account for platform-specific boot stages + +## Tools and Visualization + +### Built-in Tools + +- `cbmem -t`: Display timestamps from CBMEM +- `cbmem -c`: Display timestamps in chronological order +- `cbmem -1`: Display timestamps with relative times (microseconds) + +### External Tools + +- coreboot's timestamp analysis scripts (`util/timestamps/`) +- Custom visualization tools +- Performance analysis frameworks + + +## Troubleshooting + +### Common Issues + +1. **Missing Timestamps**: + - Check if `COLLECT_TIMESTAMPS` is enabled + - Verify memory region allocation + - Check initialization timing + +2. **Incorrect Times**: + - Verify base time initialization (`timestamp_init`) + - Check timer frequency calibration (`tick_freq_mhz` in table) + - Ensure proper scaling factors if `timestamp_rescale_table` is used + +3. **Table Overflow**: + - Monitor entry count (`num_entries` vs `max_entries` in table) + - Reduce timestamp frequency by adding fewer custom timestamps + - Increase cache size (`CONFIG_MAX_TIMESTAMP_CACHE_ENTRIES`) or CBMEM + size if possible + + +### Debugging Tips + +1. Enable `TIMESTAMPS_ON_CONSOLE` for real-time debugging +2. Use `cbmem -t` to verify timestamp collection +3. Check memory layout for proper `timestamp` region allocation +4. Verify timer initialization and calibration in platform code + + +## Future Development + +### Planned Improvements + +1. Enhanced visualization tools +2. Better integration with performance analysis frameworks +3. Support for more hardware platforms +4. Improved timestamp synchronization +5. Enhanced error handling and recovery + + +### Contributing + +Developers can contribute to the timestamp system by: + +1. Adding new timestamp IDs for platform-specific or generic operations +2. Improving timestamp analysis tools (e.g., `util/timestamps/`) +3. Enhancing documentation with more examples or platform details +4. Optimizing timestamp collection performance +5. Adding support for new hardware timer sources or platforms -- To view, visit https://review.coreboot.org/c/coreboot/+/87186?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I6e4cc244edf6cc860cc66165173f134a30a81589 Gerrit-Change-Number: 87186 Gerrit-PatchSet: 1 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com>

1 0

[XS] Change in coreboot[main]: Makefile: Finish switch to Makefile.mk from .inc
by Martin L Roth (Code Review) 05 Apr '25

05 Apr '25

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/80130?usp=email ) Change subject: Makefile: Finish switch to Makefile.mk from .inc ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/coreboot/+/80130?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: abandon Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: If0dbc7ed204047b5575b303f57f6cf607f688ad9 Gerrit-Change-Number: 80130 Gerrit-PatchSet: 3 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[M] Change in coreboot[main]: Update Maintainers file
by Martin L Roth (Code Review) 05 Apr '25

05 Apr '25

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/85883?usp=email ) Change subject: Update Maintainers file ...................................................................... Update Maintainers file - Update AMD maintainers lists to reflect current situation - Remove Ron Minnich as maintainer (at his request) - Update the Infrastructure owners to reflect the current situation. Change-Id: I2feac94595081fcea9becd9d8067ddd99a50123c Signed-off-by: Martin Roth <gaumless(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/85883 Reviewed-by: Fred Reitberger <reitbergerfred(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Matt DeVillier <matt.devillier(a)gmail.com> --- M MAINTAINERS 1 file changed, 39 insertions(+), 57 deletions(-) Approvals: build bot (Jenkins): Verified Fred Reitberger: Looks good to me, approved Matt DeVillier: Looks good to me, approved diff --git a/MAINTAINERS b/MAINTAINERS index a786879..cf6c04f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -137,12 +137,12 @@ AMD non-server family 17h and 19h reference boards M: Felix Held <felix-coreboot(a)felixheld.de> -M: Jason Glenesk <jason.glenesk(a)gmail.com> +M: Nick Kochlowski <nickkochlowski(a)gmail.com> M: Fred Reitberger <reitbergerfred(a)gmail.com> -L: amd_coreboot_org_changes(a)googlegroups.com S: Maintained F: src/mainboard/amd/bilby/ F: src/mainboard/amd/birman/ +F: src/mainboard/amd/birman_plus/ F: src/mainboard/amd/chausie/ F: src/mainboard/amd/majolica/ F: src/mainboard/amd/mandolin/ @@ -150,14 +150,11 @@ AMD server family 19h reference boards M: Felix Held <felix-coreboot(a)felixheld.de> -M: Martin Roth <gaumless(a)gmail.com> -M: Varshit Pandya <pandyavarshit(a)gmail.com> S: Maintained F: src/mainboard/amd/onyx_poc/ AMD reference boards outside of family 17h and 19h -S: Odd Fixes -L: amd_coreboot_org_changes(a)googlegroups.com +S: Orphan F: src/mainboard/amd/gardenia/ F: src/mainboard/amd/pademelon/ @@ -387,9 +384,9 @@ F: src/mainboard/google/stout/ GOOGLE AMD-BASED MAINBOARDS -M: Martin Roth <martin.roth(a)amd.corp-partner.google.com> +M: Martin Roth <gaumless(a)gmail.com> M: Jason Nien <jason.nien(a)amd.corp-partner.google.com> -L: amd_coreboot_org_changes(a)googlegroups.com +M: Matt DeVillier <matt.devillier(a)gmail.com> S: Supported F: src/mainboard/google/kahlee/ F: src/mainboard/google/zork/ @@ -684,7 +681,6 @@ F: util/rockchip/ PPC64 ARCHITECTURE -M: Ronald Minnich <rminnich(a)gmail.com> M: Timothy Pearson <tpearson(a)raptorengineeringinc.com> S: Maintained F: src/arch/ppc64/ @@ -692,7 +688,6 @@ F: src/mainboard/emulation/qemu-power8/ RISC-V ARCHITECTURE -M: Ronald Minnich <rminnich(a)gmail.com> M: Maximilian Brune <maximilian.brune(a)9elements.com> R: Philipp Hug <philipp(a)hug.cx> S: Maintained @@ -787,14 +782,24 @@ ################################################################################ AMD SUPPORT -L: amd_coreboot_org_changes(a)googlegroups.com +M: Felix Held <felix-coreboot(a)felixheld.de> +M: Martin Roth <gaumless(a)gmail.com> +M: Nick Kochlowski <nickkochlowski(a)gmail.com> +M: Ana Carolina Cabral <ana.cpmelo95(a)gmail.com> S: Odd Fixes -F: src/vendorcode/amd/ F: src/cpu/amd/ F: src/northbridge/amd/ F: src/southbridge/amd/ F: src/include/cpu/amd/ +AMD VENDORCODE-SUPPORT +M: Felix Held <felix-coreboot(a)felixheld.de> +M: Martin Roth <gaumless(a)gmail.com> +M: Nick Kochlowski <nickkochlowski(a)gmail.com> +M: Ana Carolina Cabral <ana.cpmelo95(a)gmail.com> +S: Maintained +F: src/vendorcode/amd/ + INTEL SUPPORT R: Intel_Coreboot_Reviewers <intel_coreboot_reviewers(a)intel.com> S: Maintained @@ -828,64 +833,41 @@ AMD Cezanne M: Felix Held <felix-coreboot(a)felixheld.de> -M: Jason Glenesk <jason.glenesk(a)gmail.com> -M: Fred Reitberger <reitbergerfred(a)gmail.com> -M: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> -L: amd_coreboot_org_changes(a)googlegroups.com S: Maintained F: src/soc/amd/cezanne/ F: src/vendorcode/amd/fsp/cezanne/ AMD common SoC code M: Felix Held <felix-coreboot(a)felixheld.de> -M: Jason Glenesk <jason.glenesk(a)gmail.com> -M: Fred Reitberger <reitbergerfred(a)gmail.com> -M: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> -L: amd_coreboot_org_changes(a)googlegroups.com S: Maintained F: src/soc/amd/common/ AMD Genoa Proof of Concept M: Felix Held <felix-coreboot(a)felixheld.de> M: Martin Roth <gaumless(a)gmail.com> -M: Varshit Pandya <pandyavarshit(a)gmail.com> S: Maintained F: src/soc/amd/genoa_poc/ AMD Mendocino M: Felix Held <felix-coreboot(a)felixheld.de> -M: Jason Glenesk <jason.glenesk(a)gmail.com> -M: Fred Reitberger <reitbergerfred(a)gmail.com> -M: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> -L: amd_coreboot_org_changes(a)googlegroups.com -S: Supported +S: Maintained F: src/soc/amd/mendocino/ F: src/vendorcode/amd/fsp/mendocino/ AMD Picasso M: Felix Held <felix-coreboot(a)felixheld.de> -M: Jason Glenesk <jason.glenesk(a)gmail.com> -M: Fred Reitberger <reitbergerfred(a)gmail.com> -M: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> -L: amd_coreboot_org_changes(a)googlegroups.com S: Maintained F: src/soc/amd/picasso/ F: src/vendorcode/amd/fsp/picasso/ AMD Phoenix M: Felix Held <felix-coreboot(a)felixheld.de> -M: Jason Glenesk <jason.glenesk(a)gmail.com> -M: Fred Reitberger <reitbergerfred(a)gmail.com> -M: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> -L: amd_coreboot_org_changes(a)googlegroups.com -S: Supported +S: Maintained F: src/soc/amd/phoenix/ F: src/vendorcode/amd/fsp/phoenix/ AMD Stoneyridge M: Felix Held <felix-coreboot(a)felixheld.de> -M: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> -L: amd_coreboot_org_changes(a)googlegroups.com S: Odd Fixes F: src/soc/amd/stoneyridge/ @@ -1246,47 +1228,47 @@ # *** Infrastructure Owners *** # This is intended to let people know who they should contact for issues with various infrastructure pieces. # Hardware -# Owners: Patrick -# Backups: +# Owners: Felix Singer <felixsinger(a)posteo.net> +# David Hendricks <dhendrix(a)coreboot.org> +# Backups: Martin Roth <gaumless(a)gmail.com> # Web Server -# Owners: Patrick -# Backups: +# Owners: Felix Singer <felixsinger(a)posteo.net> +# David Hendricks <dhendrix(a)coreboot.org> +# Backups: Martin Roth <gaumless(a)gmail.com> # Website -# Owners: Martin -# Backups: Patrick +# Owners: Martin Roth <gaumless(a)gmail.com> +# Backups: # Documentation Website -# Owners: Patrick +# Owners: Felix Singer <felixsinger(a)posteo.net> # Backups: CODE OF CONDUCT -M: Ronald Minnich <rminnich(a)gmail.com> -M: Martin Roth <martin(a)coreboot.org> +M: Martin Roth <gaumless(a)gmail.com> +M: David Hendricks <dhendrix(a)coreboot.org> S: Maintained F: Documentation/community/code_of_conduct.md -# Wiki -# Owners: Patrick -# Backups: - # Gerrit -# Owners: Patrick -# Backups: Martin +# Owners: Felix Singer <felixsinger(a)posteo.net> +# David Hendricks <dhendrix(a)coreboot.org> +# Backups: Martin Roth <gaumless(a)gmail.com> # Jenkins -# Owners: Patrick, Martin +# Owners: Felix Singer <felixsinger(a)posteo.net> # Backups: # Bug Tracker # Owners: Lynxis, -# Backups: Martin, +# Backups: Felix Singer <felixsinger(a)posteo.net> # Mailing List # Owners: Patrick -# Backups: Martin +# Backups: Martin Roth <gaumless(a)gmail.com> # Software Freedom Conservancy -# Main contact: Martin -# “Official” contact: David, Matt, Werner +# Main contact: Martin Roth <gaumless(a)gmail.com> +# Backup: Matt DeVillier <mrchromeboox(a)coreboot.org> +# Werner Zeh <mrchromebox(a)coreboot.org> -- To view, visit https://review.coreboot.org/c/coreboot/+/85883?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I2feac94595081fcea9becd9d8067ddd99a50123c Gerrit-Change-Number: 85883 Gerrit-PatchSet: 6 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>

1 0

[XL] Change in coreboot[main]: Documentation/internals: Add devicetree language documentation
by Martin L Roth (Code Review) 05 Apr '25

05 Apr '25

Attention is currently required from: Nicholas Chin. Martin L Roth has posted comments on this change by Martin L Roth. ( https://review.coreboot.org/c/coreboot/+/84710?usp=email ) Change subject: Documentation/internals: Add devicetree language documentation ...................................................................... Patch Set 3: (1 comment) Patchset: PS1: > Needs a toctree entry in some index.md for it to show up. […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/84710?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: If868ad9a87cb2903bf144996fe3b87d29d4fc755 Gerrit-Change-Number: 84710 Gerrit-PatchSet: 3 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Sat, 05 Apr 2025 20:54:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nicholas Chin <nic.c3.14(a)gmail.com>

1 0

[XL] Change in coreboot[main]: Documentation/internals: Add devicetree language documentation
by Martin L Roth (Code Review) 05 Apr '25

05 Apr '25

Attention is currently required from: Martin L Roth. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/84710?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: Documentation/internals: Add devicetree language documentation ...................................................................... Documentation/internals: Add devicetree language documentation Signed-off-by: Martin Roth <gaumless(a)gmail.com> Change-Id: If868ad9a87cb2903bf144996fe3b87d29d4fc755 --- A Documentation/internals/devicetree_language.md M Documentation/internals/index.md 2 files changed, 1,310 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/84710/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/84710?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: If868ad9a87cb2903bf144996fe3b87d29d4fc755 Gerrit-Change-Number: 84710 Gerrit-PatchSet: 3 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>

1 0

[M] Change in coreboot[main]: Documentation: Write down coreboot's git commit message rules
by Martin L Roth (Code Review) 05 Apr '25

05 Apr '25

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75495?usp=email ) Change subject: Documentation: Write down coreboot's git commit message rules ...................................................................... Documentation: Write down coreboot's git commit message rules This mostly lists things that long-time coreboot contributors already do. Some of these rules were captured on the wiki. Others were adopted from other git commit message guideline documents long ago. Signed-off-by: Martin Roth <gaumless(a)gmail.com> Change-Id: Ic2ba681193e302318934cc2f7f30659eac73d099 Reviewed-on: https://review.coreboot.org/c/coreboot/+/75495 Reviewed-by: Matt DeVillier <matt.devillier(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Subrata Banik <subratabanik(a)google.com> --- A Documentation/contributing/git_commit_messages.md M Documentation/contributing/index.md 2 files changed, 79 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Matt DeVillier: Looks good to me, approved Subrata Banik: Looks good to me, but someone else must approve diff --git a/Documentation/contributing/git_commit_messages.md b/Documentation/contributing/git_commit_messages.md new file mode 100644 index 0000000..4addf4c --- /dev/null +++ b/Documentation/contributing/git_commit_messages.md @@ -0,0 +1,78 @@ +# Git Commit messages + +There are a number of different recommendations for git commit +messages, so this is another one. + +It's expected that coreboot contributors already generally understand +the idea of writing good git commit messages, so this is not trying +to go over everything again. There are other tutorials that cover that. + +- [Linux Kernel tip tree Handbook](https://www.kernel.org/doc/html/latest/process/maintainer-tip.htm… +- [How to write a Git Commit Message](https://cbea.ms/git-commit/) + +## Line length + +- The subject line should be <= 65 characters, with an absolute maximum + of 75 characters +- Prose in the commit message should be <= 75 characters +- If reflowing prose to 75 characters can reduce the length of the + commit message by 2 or more lines, please reflow it. Using the entire + 75 characters on a line when reasonable is recommended, but not + required. +- Non-prose text in the body in the commit message does not need to be + wrapped at 75 characters. Examples: URLs, compiler output, or poetry. + +## Both Subject & body + +- Use the present tense. (The for loop exits too early, so ...", not + "The for loop exited too early, so ..."). +- When using acronyms, make sure they're explained in the commit + message itself or in the [acronyms list](https://doc.coreboot.org/acronyms.html). + +## Commit message Subject Line + +- Start the subject line with a prefix denoting the area of the change. + Often part of the path can be used by that. Looking through existing + commit messages summaries with `git log --oneline ...` gives a good + idea. Because this prefix takes space used by the rest of the subject, + it should be kept short while still uniquely describing the area. + - Don't include `src/` + - Use abbreviations where possible: + - mb: mainboard + - vc: vendorcode +- Don't end the subject line with a period. +- Use the imperative mood. ("Fix whitespace", not "whitespace fixes"). + +## Commit Message Body + +- Make sure the problem being solved by the commit is described. While + it may be obvious to the committer, it may not be obvious to others. +- Reference other commits with either CB:XXXXX or a 10 character hash + and the subject. +- When using a URL in a commit message, use archive.org when possible. + URLs often get changed or go stale, so this keeps them stable. +- Make sure that all changes in a patch are addressed in the commit + message. +- A TEST= tag is highly recommended, but not required. This lets people + know whether you tested it by booting the board or just by compiling. +- All but the most trivial of patches should generally have a body. +- A BUG= tag can be added when the author wants to indicate that the + patch is or is not related to a bug. This can be either in coreboot's + issue tracker, or a private issue tracker. + - `BUG=b:####` is used by the Google internal issue tracker. + - `BUG=chromium:####` indicates the Chromium public tracker at + https://issues.chromium.org/ + - `BUG=CID ####` can be used to indicate coverity error fixes. + - `BUG=https://...` can be used to link directly to a public + tracker. +- The BRANCH= tag is used in cases where a patch needs to be added to a + specific downstream branch. This is never required by the coreboot + project. + +## Footers + +- The Signed-off-by line is required (Jenkins forces this). +- The Change ID is required (Gerrit forces this.) +- When adding a patch that has already gone through another git or + gerrit, the footers from those previous commits may be added, but + keep the list reasonable. diff --git a/Documentation/contributing/index.md b/Documentation/contributing/index.md index 38a67c0..1f9e9e5 100644 --- a/Documentation/contributing/index.md +++ b/Documentation/contributing/index.md @@ -5,6 +5,7 @@ Coding Style <coding_style.md> Gerrit Guidelines <gerrit_guidelines.md> +Commit Message Guidelines <git_commit_messages.md> Project Ideas <project_ideas.md> Documentation Ideas <documentation_ideas.md> Google Summer of Code <gsoc.md> -- To view, visit https://review.coreboot.org/c/coreboot/+/75495?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ic2ba681193e302318934cc2f7f30659eac73d099 Gerrit-Change-Number: 75495 Gerrit-PatchSet: 5 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Jan Samek <samekh(a)email.cz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>

1 0

[L] Change in coreboot[main]: Documentation/internals: Add devicetree documentation
by Martin L Roth (Code Review) 05 Apr '25

05 Apr '25

Attention is currently required from: Martin L Roth, Nicholas Chin. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/84709?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: Documentation/internals: Add devicetree documentation ...................................................................... Documentation/internals: Add devicetree documentation Signed-off-by: Martin Roth <gaumless(a)gmail.com> Change-Id: I2a43a96911844bd2b682004d5423126ad00a4bf3 --- M Documentation/index.md A Documentation/internals/devicetree.md A Documentation/internals/index.md 3 files changed, 698 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/84709/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/84709?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I2a43a96911844bd2b682004d5423126ad00a4bf3 Gerrit-Change-Number: 84709 Gerrit-PatchSet: 3 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>

1 0

[M] Change in coreboot[main]: Documentation: Write down coreboot's git commit message rules
by Matt DeVillier (Code Review) 05 Apr '25

05 Apr '25

Attention is currently required from: Jan Samek, Martin L Roth, Nicholas Chin, Paul Menzel. Matt DeVillier has posted comments on this change by Martin L Roth. ( https://review.coreboot.org/c/coreboot/+/75495?usp=email ) Change subject: Documentation: Write down coreboot's git commit message rules ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/75495?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: Ic2ba681193e302318934cc2f7f30659eac73d099 Gerrit-Change-Number: 75495 Gerrit-PatchSet: 4 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Jan Samek <samekh(a)email.cz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-CC: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Attention: Jan Samek <samekh(a)email.cz> Gerrit-Comment-Date: Sat, 05 Apr 2025 20:04:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[M] Change in coreboot[main]: Update Maintainers file
by Fred Reitberger (Code Review) 05 Apr '25

05 Apr '25

Attention is currently required from: Felix Held, Felix Singer, Martin L Roth, Martin Roth, Paul Menzel. Fred Reitberger has posted comments on this change by Martin L Roth. ( https://review.coreboot.org/c/coreboot/+/85883?usp=email ) Change subject: Update Maintainers file ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/85883?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I2feac94595081fcea9becd9d8067ddd99a50123c Gerrit-Change-Number: 85883 Gerrit-PatchSet: 5 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Sat, 05 Apr 2025 20:01:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2025