coreboot-gerrit January 2025

coreboot-gerrit@coreboot.org

1 participants
3269 discussions

[S] Change in coreboot[main]: commonlib/device_tree: Initialize cells to default values on find()
by Doug Anderson (Code Review) 14 Jan '25

14 Jan '25

Attention is currently required from: Julius Werner, Maximilian Brune. Doug Anderson has posted comments on this change by Julius Werner. ( https://review.coreboot.org/c/coreboot/+/85988?usp=email ) Change subject: commonlib/device_tree: Initialize cells to default values on find() ...................................................................... Patch Set 1: Code-Review+1 (1 comment) Patchset: PS1: I thought I had +2 in the past, but I don't seem to now so +1. -- To view, visit https://review.coreboot.org/c/coreboot/+/85988?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: I39ba2c82d3a0d0b39a2ed5eba2420a04fbccb2f7 Gerrit-Change-Number: 85988 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Doug Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Comment-Date: Tue, 14 Jan 2025 22:10:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[XS] Change in coreboot[main]: commonlib/device_tree: Return cells properties of parent node
by Julius Werner (Code Review) 14 Jan '25

14 Jan '25

1 0

[XS] Change in coreboot[main]: commonlib/device_tree: Skip dt_read_cell_props() when not needed
by Julius Werner (Code Review) 14 Jan '25

14 Jan '25

Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/85989?usp=email ) Change subject: commonlib/device_tree: Skip dt_read_cell_props() when not needed ...................................................................... commonlib/device_tree: Skip dt_read_cell_props() when not needed dt_find_node() calls dt_read_cell_props() for every node it walks, but this is only actually necessary when the caller is interested in the `#address-cells` and `#size-cells` values and passed out-parameters to receive them. Most callers don't actually do that, and we scan through all properties needlessly on every node. This patch adds a fast path to skip that. Change-Id: I114f824a7d88b0bac4a96aca3f7dced459503b02 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/commonlib/device_tree.c 1 file changed, 4 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/85989/1 diff --git a/src/commonlib/device_tree.c b/src/commonlib/device_tree.c index 2ee0316..a7007d9 100644 --- a/src/commonlib/device_tree.c +++ b/src/commonlib/device_tree.c @@ -1050,7 +1050,8 @@ /* Update #address-cells and #size-cells for the parent level (cells properties always count for the direct children of their node). */ - dt_read_cell_props(parent, addrcp, sizecp); + if (addrcp || sizecp) + dt_read_cell_props(parent, addrcp, sizecp); /* Find the next node in the path, if it exists. */ list_for_each(node, parent->children, list_node) { @@ -1135,7 +1136,8 @@ if (path[0] == '/') { /* regular path */ if (path[1] == '\0') { /* special case: "/" is root node */ - dt_read_cell_props(tree->root, addrcp, sizecp); + if (addrcp || sizecp) + dt_read_cell_props(tree->root, addrcp, sizecp); return tree->root; } -- To view, visit https://review.coreboot.org/c/coreboot/+/85989?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: I114f824a7d88b0bac4a96aca3f7dced459503b02 Gerrit-Change-Number: 85989 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>

1 0

[S] Change in coreboot[main]: commonlib/device_tree: Initialize cells to default values on find()
by Julius Werner (Code Review) 14 Jan '25

14 Jan '25

Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/85988?usp=email ) Change subject: commonlib/device_tree: Initialize cells to default values on find() ...................................................................... commonlib/device_tree: Initialize cells to default values on find() This patch wraps `dt_find_node()` in a function that initializes the addr_cells and size_cells values to the defaults provided in the FDT specification before potentially updating them from found values, so that we always return the correct result and remove the burden of correctly initializing them from the caller. Change-Id: I39ba2c82d3a0d0b39a2ed5eba2420a04fbccb2f7 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/commonlib/device_tree.c 1 file changed, 31 insertions(+), 17 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/85988/1 diff --git a/src/commonlib/device_tree.c b/src/commonlib/device_tree.c index c73e258..2ee0316 100644 --- a/src/commonlib/device_tree.c +++ b/src/commonlib/device_tree.c @@ -1039,23 +1039,9 @@ } } -/* - * Find a node from a device tree path, relative to a parent node. - * - * @param parent The node from which to start the relative path lookup. - * @param path An array of path component strings that will be looked - * up in order to find the node. Must be terminated with - * a NULL pointer. Example: {'firmware', 'coreboot', NULL} - * @param addrcp Pointer that will be updated with any #address-cells - * value found in the path. May be NULL to ignore. - * @param sizecp Pointer that will be updated with any #size-cells - * value found in the path. May be NULL to ignore. - * @param create 1: Create node(s) if not found. 0: Return NULL instead. - * @return The found/created node, or NULL. - */ -struct device_tree_node *dt_find_node(struct device_tree_node *parent, - const char **path, u32 *addrcp, - u32 *sizecp, int create) +static struct device_tree_node *_dt_find_node(struct device_tree_node *parent, + const char **path, u32 *addrcp, + u32 *sizecp, int create) { struct device_tree_node *node, *found = NULL; @@ -1091,6 +1077,34 @@ } /* + * Find a node from a device tree path, relative to a parent node. + * + * @param parent The node from which to start the relative path lookup. + * @param path An array of path component strings that will be looked + * up in order to find the node. Must be terminated with + * a NULL pointer. Example: {'firmware', 'coreboot', NULL} + * @param addrcp Pointer that will be updated with any #address-cells + * value found in the path. May be NULL to ignore. + * @param sizecp Pointer that will be updated with any #size-cells + * value found in the path. May be NULL to ignore. + * @param create 1: Create node(s) if not found. 0: Return NULL instead. + * @return The found/created node, or NULL. + */ +struct device_tree_node *dt_find_node(struct device_tree_node *parent, + const char **path, u32 *addrcp, + u32 *sizecp, int create) +{ + /* Initialize cells to default values according to FDT spec. */ + if (addrcp) + *addrcp = 2; + + if (sizecp) + *sizecp = 1; + + return _dt_find_node(parent, path, addrcp, sizecp, create); +} + +/* * Find a node in the tree from a string device tree path. * * @param tree The device tree to search. -- To view, visit https://review.coreboot.org/c/coreboot/+/85988?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: I39ba2c82d3a0d0b39a2ed5eba2420a04fbccb2f7 Gerrit-Change-Number: 85988 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>

1 0

[M] Change in coreboot[main]: CFR: add dependencies based on specific option values
by Filip Brozovic (Code Review) 14 Jan '25

14 Jan '25

Filip Brozovic has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/85987?usp=email ) Change subject: CFR: add dependencies based on specific option values ...................................................................... CFR: add dependencies based on specific option values Implements a way for CFR options to depend on another option being set to one or more specific values. This is achieved by writing a list of values as a varbinary struct. Change-Id: Iaf7965551490969052eb27c207fa524470d4dd6a Signed-off-by: Filip Brozovic <fbrozovic(a)gmail.com> --- M Documentation/drivers/cfr_internal.md M src/commonlib/include/commonlib/cfr.h M src/drivers/option/cfr.c M src/drivers/option/cfr_frontend.h 4 files changed, 117 insertions(+), 29 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/85987/1 diff --git a/Documentation/drivers/cfr_internal.md b/Documentation/drivers/cfr_internal.md index 95fef66..b2355d7 100644 --- a/Documentation/drivers/cfr_internal.md +++ b/Documentation/drivers/cfr_internal.md @@ -50,8 +50,9 @@ The CFR options can have a dependency that must be evaluated at runtime by the OS/payload that parses the CFR record and displays the UI. -By using the `WITH_DEP()` macro you can specify another numberic option that -is checked to hide the current option. +By using the `WITH_DEP()` macro you can specify another numeric option that +is checked to hide the current option. The `WITH_DEP_VALUES()` macro allows +specifying one or more values that cause the dependent option to be displayed. **Example:** Declares a dependency from `sata_disable_port0` to `sata_enable`. The option `sata_disable_port0` will be hidden as long as "sata_enable" is 0. @@ -76,6 +77,37 @@ }, WITH_DEP(&sata_enable)); ``` +**Example:** Declares a dependency from `com1_termination` to `com1_mode`. +The option `com1_termination` will only be shown if `com1_mode` is set to RS-485. + +``` +#define COM_MODE_DISABLED 3 +#define COM_MODE_RS232 0 +#define COM_MODE_RS485 1 + +static struct sm_object com1_mode = SM_DECLARE_ENUM({ + .flags = CFR_OPTFLAG_RUNTIME, + .opt_name = "com1_mode", + .ui_name = "COM1 Mode", + .ui_helptext = NULL, + .default_value = 1, + .values = (const struct sm_enum_value[]) { + { "Disabled", COM_MODE_DISABLED }, + { "RS-232", COM_MODE_RS232 }, + { "RS-485", COM_MODE_RS485 }, + SM_ENUM_VALUE_END }, +}); + +static struct sm_object com1_termination = SM_DECLARE_BOOL({ + .flags = CFR_OPTFLAG_RUNTIME, + .opt_name = "com1_termination", + .ui_name = "Enable COM1 termination resistors", + .ui_helptext = NULL, + .default_value = false, +}, WITH_DEP_VALUES(&com1_mode, COM_MODE_RS485)); + +``` + ### Providing mainboard custom options A mainboard that uses CFR can provide a list of custom options diff --git a/src/commonlib/include/commonlib/cfr.h b/src/commonlib/include/commonlib/cfr.h index cc8431a..564c8a1 100644 --- a/src/commonlib/include/commonlib/cfr.h +++ b/src/commonlib/include/commonlib/cfr.h @@ -61,6 +61,7 @@ CFR_TAG_VARCHAR_UI_HELPTEXT = 9, CFR_TAG_VARCHAR_DEF_VALUE = 10, CFR_TAG_OPTION_COMMENT = 11, + CFR_TAG_DEP_VALUES = 12, }; /* @@ -102,7 +103,8 @@ struct __packed lb_cfr_varbinary { uint32_t tag; /* * CFR_TAG_VARCHAR_OPT_NAME, CFR_TAG_VARCHAR_UI_NAME, - * CFR_TAG_VARCHAR_UI_HELPTEXT or CFR_TAG_VARCHAR_DEF_VALUE + * CFR_TAG_VARCHAR_UI_HELPTEXT, CFR_TAG_VARCHAR_DEF_VALUE + * or CFR_TAG_DEP_VALUES */ uint32_t size; /* Length of the entire structure */ uint32_t data_length; /* Length of data, including NULL terminator for strings */ @@ -134,6 +136,7 @@ * struct lb_cfr_varbinary opt_name * struct lb_cfr_varbinary ui_name * struct lb_cfr_varbinary ui_helptext (Optional) + * struct lb_cfr_varbinary dependency_values (Optional) * struct lb_cfr_enum_value enum_values[] */ }; @@ -151,6 +154,7 @@ * struct lb_cfr_varbinary opt_name * struct lb_cfr_varbinary ui_name * struct lb_cfr_varbinary ui_helptext (Optional) + * struct lb_cfr_varbinary dependency_values (Optional) */ }; @@ -170,6 +174,7 @@ /* * struct lb_cfr_varbinary ui_name * struct lb_cfr_varbinary ui_helptext (Optional) + * struct lb_cfr_varbinary dependency_values (Optional) */ }; @@ -184,6 +189,7 @@ uint32_t flags; /* enum cfr_option_flags */ /* * struct lb_cfr_varbinary ui_name + * struct lb_cfr_varbinary dependency_values (Optional) * struct lb_cfr_varchar_option options[] */ }; diff --git a/src/drivers/option/cfr.c b/src/drivers/option/cfr.c index e833d7b..6d59ca9 100644 --- a/src/drivers/option/cfr.c +++ b/src/drivers/option/cfr.c @@ -69,6 +69,25 @@ return write_cfr_varchar(current, string, CFR_TAG_VARCHAR_UI_HELPTEXT); } +static uint32_t sm_write_dep_values(char *current, + const uint32_t *dep_values, const uint32_t num_dep_values) +{ + /* Dependency values are optional */ + if (!dep_values || !num_dep_values) + return 0; + + struct lb_cfr_varbinary *cfr_values = (struct lb_cfr_varbinary *)current; + cfr_values->tag = CFR_TAG_DEP_VALUES; + cfr_values->data_length = sizeof(*dep_values) * num_dep_values; + char *data = current + sizeof(*cfr_values); + memcpy(data, dep_values, cfr_values->data_length); + + /* Make sure that every TAG/SIZE field is always aligned to LB_ENTRY_ALIGN */ + cfr_values->size = ALIGN_UP(sizeof(*cfr_values) + cfr_values->data_length, LB_ENTRY_ALIGN); + + return cfr_values->size; +} + static uint32_t sm_write_enum_value(char *current, const struct sm_enum_value *e) { struct lb_cfr_enum_value *enum_val = (struct lb_cfr_enum_value *)current; @@ -86,7 +105,7 @@ static uint32_t write_numeric_option(char *current, uint32_t tag, const uint64_t object_id, const char *opt_name, const char *ui_name, const char *ui_helptext, uint32_t flags, uint32_t default_value, const struct sm_enum_value *values, - const uint64_t dep_id) + const uint64_t dep_id, const uint32_t *dep_values, const uint32_t num_dep_values) { struct lb_cfr_numeric_option *option = (struct lb_cfr_numeric_option *)current; size_t len; @@ -110,6 +129,7 @@ return 0; current += len; current += sm_write_ui_helptext(current, ui_helptext); + current += sm_write_dep_values(current, dep_values, num_dep_values); if (option->tag == CFR_TAG_OPTION_ENUM && values) { for (const struct sm_enum_value *e = values; e->ui_name; e++) { @@ -122,35 +142,41 @@ } static uint32_t sm_write_opt_enum(char *current, const struct sm_obj_enum *sm_enum, - const uint64_t object_id, const uint64_t dep_id) + const uint64_t object_id, const uint64_t dep_id, + const uint32_t *dep_values, const uint32_t num_dep_values) { return write_numeric_option(current, CFR_TAG_OPTION_ENUM, object_id, sm_enum->opt_name, sm_enum->ui_name, sm_enum->ui_helptext, sm_enum->flags, sm_enum->default_value, sm_enum->values, - dep_id); + dep_id, dep_values, num_dep_values); } static uint32_t sm_write_opt_number(char *current, const struct sm_obj_number *sm_number, - const uint64_t object_id, const uint64_t dep_id) + const uint64_t object_id, const uint64_t dep_id, + const uint32_t *dep_values, const uint32_t num_dep_values) { return write_numeric_option(current, CFR_TAG_OPTION_NUMBER, object_id, sm_number->opt_name, sm_number->ui_name, sm_number->ui_helptext, - sm_number->flags, sm_number->default_value, NULL, dep_id); + sm_number->flags, sm_number->default_value, NULL, dep_id, + dep_values, num_dep_values); } static uint32_t sm_write_opt_bool(char *current, const struct sm_obj_bool *sm_bool, - const uint64_t object_id, const uint64_t dep_id) + const uint64_t object_id, const uint64_t dep_id, + const uint32_t *dep_values, const uint32_t num_dep_values) { return write_numeric_option(current, CFR_TAG_OPTION_BOOL, object_id, sm_bool->opt_name, sm_bool->ui_name, sm_bool->ui_helptext, - sm_bool->flags, sm_bool->default_value, NULL, dep_id); + sm_bool->flags, sm_bool->default_value, NULL, dep_id, + dep_values, num_dep_values); } static uint32_t sm_write_opt_varchar(char *current, const struct sm_obj_varchar *sm_varchar, - const uint64_t object_id, const uint64_t dep_id) + const uint64_t object_id, const uint64_t dep_id, + const uint32_t *dep_values, const uint32_t num_dep_values) { struct lb_cfr_varchar_option *option = (struct lb_cfr_varchar_option *)current; @@ -175,13 +201,15 @@ return 0; current += len; current += sm_write_ui_helptext(current, sm_varchar->ui_helptext); + current += sm_write_dep_values(current, dep_values, num_dep_values); option->size = cfr_record_size((char *)option, current); return option->size; } static uint32_t sm_write_opt_comment(char *current, const struct sm_obj_comment *sm_comment, - const uint32_t object_id, const uint32_t dep_id) + const uint32_t object_id, const uint32_t dep_id, + const uint32_t *dep_values, const uint32_t num_dep_values) { struct lb_cfr_option_comment *comment = (struct lb_cfr_option_comment *)current; size_t len; @@ -200,6 +228,7 @@ return 0; current += len; current += sm_write_ui_helptext(current, sm_comment->ui_helptext); + current += sm_write_dep_values(current, dep_values, num_dep_values); comment->size = cfr_record_size((char *)comment, current); return comment->size; @@ -215,7 +244,8 @@ static uint32_t sm_write_object(char *current, const struct sm_object *sm_obj); static uint32_t sm_write_form(char *current, struct sm_obj_form *sm_form, - const uint64_t object_id, const uint64_t dep_id) + const uint64_t object_id, const uint64_t dep_id, + const uint32_t *dep_values, const uint32_t num_dep_values) { struct lb_cfr_option_form *form = (struct lb_cfr_option_form *)current; size_t len; @@ -234,6 +264,7 @@ if (!len) return 0; current += len; + current += sm_write_dep_values(current, dep_values, num_dep_values); while (sm_form->obj_list[i]) current += sm_write_object(current, sm_form->obj_list[i++]); @@ -245,6 +276,8 @@ static uint32_t sm_write_object(char *current, const struct sm_object *sm_obj) { uint64_t dep_id, obj_id; + const uint32_t *dep_values; + uint32_t num_dep_values; struct sm_object sm_obj_copy; assert(sm_obj); @@ -253,10 +286,14 @@ /* Set dependency ID */ dep_id = 0; + dep_values = NULL; + num_dep_values = 0; if (sm_obj->dep) { - assert(sm_obj->dep->kind == SM_OBJ_BOOL); - if (sm_obj->dep->kind == SM_OBJ_BOOL) + if (sm_obj->dep->kind == SM_OBJ_BOOL || sm_obj->dep->kind == SM_OBJ_ENUM) { dep_id = sm_gen_obj_id((void *)sm_obj->dep); + dep_values = sm_obj->dep_values; + num_dep_values = sm_obj->num_dep_values; + } } /* Invoke callback to update fields */ @@ -273,21 +310,22 @@ return 0; case SM_OBJ_ENUM: return sm_write_opt_enum(current, &sm_obj->sm_enum, obj_id, - dep_id); + dep_id, dep_values, num_dep_values); case SM_OBJ_NUMBER: return sm_write_opt_number(current, &sm_obj->sm_number, obj_id, - dep_id); + dep_id, dep_values, num_dep_values); case SM_OBJ_BOOL: return sm_write_opt_bool(current, &sm_obj->sm_bool, obj_id, - dep_id); + dep_id, dep_values, num_dep_values); case SM_OBJ_VARCHAR: return sm_write_opt_varchar(current, &sm_obj->sm_varchar, obj_id, - dep_id); + dep_id, dep_values, num_dep_values); case SM_OBJ_COMMENT: return sm_write_opt_comment(current, &sm_obj->sm_comment, obj_id, - dep_id); + dep_id, dep_values, num_dep_values); case SM_OBJ_FORM: - return sm_write_form(current, (struct sm_obj_form *)&sm_obj->sm_form, obj_id, dep_id); + return sm_write_form(current, (struct sm_obj_form *)&sm_obj->sm_form, obj_id, + dep_id, dep_values, num_dep_values); default: BUG(); printk(BIOS_ERR, "Unknown setup menu object kind %u, ignoring\n", sm_obj->kind); @@ -310,13 +348,13 @@ current += cfr_root->size; while (sm_root && sm_root[i]) - current += sm_write_form(current, sm_root[i++], 0, 0); + current += sm_write_form(current, sm_root[i++], 0, 0, NULL, 0); /* * Add generic forms. */ for (obj = &_cfr_forms[0]; obj != &_ecfr_forms[0]; obj++) - current += sm_write_form(current, obj, 0, 0); + current += sm_write_form(current, obj, 0, 0, NULL, 0); cfr_root->size = cfr_record_size((char *)cfr_root, current); diff --git a/src/drivers/option/cfr_frontend.h b/src/drivers/option/cfr_frontend.h index 2161853..21c18e7 100644 --- a/src/drivers/option/cfr_frontend.h +++ b/src/drivers/option/cfr_frontend.h @@ -75,6 +75,8 @@ struct sm_object { enum sm_object_kind kind; const struct sm_object *dep; + const uint32_t *dep_values; + const uint32_t num_dep_values; void (*ctor)(const struct sm_object *obj, struct sm_object *new); /* Called on object creation */ union { struct sm_obj_enum sm_enum; @@ -87,21 +89,31 @@ }; /* sm_object helpers with type checking */ -#define SM_DECLARE_ENUM(...) { .kind = SM_OBJ_ENUM, .dep = NULL, \ +#define SM_DECLARE_ENUM(...) { .kind = SM_OBJ_ENUM, .dep = NULL, \ + .dep_values = NULL, .num_dep_values = 0, \ .ctor = NULL, .sm_enum = __VA_ARGS__ } -#define SM_DECLARE_NUMBER(...) { .kind = SM_OBJ_NUMBER, .dep = NULL, \ +#define SM_DECLARE_NUMBER(...) { .kind = SM_OBJ_NUMBER, .dep = NULL, \ + .dep_values = NULL, .num_dep_values = 0, \ .ctor = NULL, .sm_number = __VA_ARGS__ } -#define SM_DECLARE_BOOL(...) { .kind = SM_OBJ_BOOL, .dep = NULL, \ +#define SM_DECLARE_BOOL(...) { .kind = SM_OBJ_BOOL, .dep = NULL, \ + .dep_values = NULL, .num_dep_values = 0, \ .ctor = NULL, .sm_bool = __VA_ARGS__ } -#define SM_DECLARE_VARCHAR(...) { .kind = SM_OBJ_VARCHAR, .dep = NULL, \ +#define SM_DECLARE_VARCHAR(...) { .kind = SM_OBJ_VARCHAR, .dep = NULL, \ + .dep_values = NULL, .num_dep_values = 0, \ .ctor = NULL, .sm_varchar = __VA_ARGS__ } -#define SM_DECLARE_COMMENT(...) { .kind = SM_OBJ_COMMENT, .dep = NULL, \ +#define SM_DECLARE_COMMENT(...) { .kind = SM_OBJ_COMMENT, .dep = NULL, \ + .dep_values = NULL, .num_dep_values = 0, \ .ctor = NULL, .sm_comment = __VA_ARGS__ } -#define SM_DECLARE_FORM(...) { .kind = SM_OBJ_FORM, .dep = NULL, \ +#define SM_DECLARE_FORM(...) { .kind = SM_OBJ_FORM, .dep = NULL, \ + .dep_values = NULL, .num_dep_values = 0, \ .ctor = NULL, .sm_form = __VA_ARGS__ } #define WITH_CALLBACK(c) .ctor = (c) #define WITH_DEP(d) .dep = (d) +#define WITH_DEP_VALUES(d, ...) \ + .dep = (d), \ + .dep_values = ((const uint32_t[]) { __VA_ARGS__ }), \ + .num_dep_values = sizeof((uint32_t[]) { __VA_ARGS__ }) / sizeof(uint32_t) void cfr_write_setup_menu(struct lb_cfr *cfr_root, struct sm_obj_form *sm_root[]); -- To view, visit https://review.coreboot.org/c/coreboot/+/85987?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: Iaf7965551490969052eb27c207fa524470d4dd6a Gerrit-Change-Number: 85987 Gerrit-PatchSet: 1 Gerrit-Owner: Filip Brozovic <fbrozovic(a)gmail.com>

1 0

[S] Change in coreboot[main]: MAINTAINERS: Add Intel reviewers mailing list to relevant sections
by Julius Werner (Code Review) 14 Jan '25

14 Jan '25

Attention is currently required from: Cliff Huang, Jérémy Compostella, Patrick Georgi, Zhixing Ma. Julius Werner has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/85982?usp=email ) Change subject: MAINTAINERS: Add Intel reviewers mailing list to relevant sections ...................................................................... Patch Set 6: (1 comment) Patchset: PS6: FYI, while it's good to keep the MAINTAINERS file up to date anyway so that people can see who to reach out to and who can give authoritative +2s about a certain code section, you don't _need_ to be in MAINTAINERS just to get notifications. Gerrit has an extensive notification system built-in that anyone can configure for their account without having to commit anything to the repo. Just go to Gerrit's Settings page in the UI (gear in top right), scroll to Notifications, put "coreboot" in the Repo field and write something like `path:"^src/soc/intel/.*"` in the "branch:name, or other search expression" field. Click Add and Save Changes and then you'll get notified of any new patch under the soc/intel directory. -- To view, visit https://review.coreboot.org/c/coreboot/+/85982?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: I2d90d10902dba30222d213632bd0d6d0226b855d Gerrit-Change-Number: 85982 Gerrit-PatchSet: 6 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Name of user not set #1005776 Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Comment-Date: Tue, 14 Jan 2025 21:36:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in coreboot[main]: commonlib/device_tree: Return cells properties of parent node
by Douglas Anderson (Code Review) 14 Jan '25

14 Jan '25

Attention is currently required from: Julius Werner, Maximilian Brune, Paul Menzel, Yu-Ping Wu. Douglas Anderson has posted comments on this change by Julius Werner. ( https://review.coreboot.org/c/coreboot/+/85964?usp=email ) Change subject: commonlib/device_tree: Return cells properties of parent node ...................................................................... Patch Set 2: Code-Review+1 (1 comment) File src/commonlib/device_tree.c: https://review.coreboot.org/c/coreboot/+/85964/comment/6f8a9470_8d12d54d?us… : PS1, Line 1061: > Maybe it would be nice to make sure props are always initialized to something but I don't think that could be done with the current recursive architecture of the API. Not that I'm insisting, but the recursion issue is simple to solve by adding a wrapper. Rename the existing function to `_dt_find_node`, then: ``` struct device_tree_node *dt_find_node(...) { if (addrcp) *addrcp = 2; if (sizecp) *sizecp = 1; return _dt_find_node(...); } ``` ;-) ...but given that you've researched it then I'm file with it how it is... -- To view, visit https://review.coreboot.org/c/coreboot/+/85964?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: I323066477a4d4be17225e0915a81ce2ff39c1e40 Gerrit-Change-Number: 85964 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Tue, 14 Jan 2025 21:32:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Douglas Anderson <dianders(a)chromium.org> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>

1 0

[XS] Change in coreboot[main]: commonlib/device_tree: Return cells properties of parent node
by Julius Werner (Code Review) 14 Jan '25

14 Jan '25

Attention is currently required from: Douglas Anderson, Maximilian Brune, Paul Menzel, Yu-Ping Wu. Julius Werner has posted comments on this change by Julius Werner. ( https://review.coreboot.org/c/coreboot/+/85964?usp=email ) Change subject: commonlib/device_tree: Return cells properties of parent node ...................................................................... Patch Set 2: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/85964/comment/982af942_f965e2d8?us… : PS1, Line 31: recently > Maybe just reference the commit? Done File src/commonlib/device_tree.c: https://review.coreboot.org/c/coreboot/+/85964/comment/fb7917b5_fb77c51d?us… : PS1, Line 1061: > Sure, it's useless for the purpose of finding a child DT node, but what if old code was using this function as a shortcut for finding the #address-cells and #size-cells of a given node. AKA: I've looked through all callers (both in coreboot and depthcharge) already, there are very few cases that even care about cells at all and those don't do anything that would be affected by this. In general, cases that only want to probe the current node just call `dt_read_cell_props()` directly. Maybe it would be nice to make sure props are always initialized to something but I don't think that could be done with the current recursive architecture of the API. We could do it in `dt_find_node_by_path()`, but then that just makes that function act differently from `dt_find_node()` which could also be confusing. Right now, callers seem to generally all initialize the variables before passing them in (although they unfortunately don't always do it to 2/1 like the spec says, but that's their problem), and don't expect the function to write back to them in all cases, which also matches the API documentation. https://review.coreboot.org/c/coreboot/+/85964/comment/1aa344ec_43291cf4?us… : PS1, Line 1066: . > Remove the space? Done -- To view, visit https://review.coreboot.org/c/coreboot/+/85964?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: I323066477a4d4be17225e0915a81ce2ff39c1e40 Gerrit-Change-Number: 85964 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Douglas Anderson <dianders(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Tue, 14 Jan 2025 21:27:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Douglas Anderson <dianders(a)chromium.org> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>

1 0

[XS] Change in coreboot[main]: commonlib/device_tree: Return cells properties of parent node
by Julius Werner (Code Review) 14 Jan '25

14 Jan '25

Attention is currently required from: Julius Werner, Maximilian Brune, Paul Menzel, Yu-Ping Wu. Hello Douglas Anderson, Maximilian Brune, Paul Menzel, Yu-Ping Wu, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/85964?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Code-Review+1 by Paul Menzel, Code-Review+2 by Maximilian Brune, Verified+1 by build bot (Jenkins) Change subject: commonlib/device_tree: Return cells properties of parent node ...................................................................... commonlib/device_tree: Return cells properties of parent node In Flattened Device Trees, there exist special properties called `#address-cells` and `#size-cells` that determine how large addresses and sizes in `reg` properties are. According to the FDT specification, each `reg` node cares about the `...cells` property in the _parent_ of its node. Our current implementation looks for those properties in the node it finds and returns, which would presumably be the node with the `reg` property itself. Therefore, we're returning the wrong `...cells` values. This isn't really a problem in practice because we also allow inheriting these properties from the parent when they don't exist in the child, and nodes that contain `reg` properties usually don't contain `...cells` properties themselves (because those properties would be incorrect and useless there), so we usually just end up falling back to the (correct) value we inherited from the parent. But it's still better to just fix the mistake, and if we ever happen to have a situation where the node containing the `reg` property still has children that require different `...cells` values as well, it could make a difference. (The fact that we're inheriting these properties is also technically incorrect according to the spec, but we're doing that intentionally to match behavior in the Linux kernel.) This issue was already correctly implemented in the recently added fdt_find_node() from commit 33079b8174e6 ("lib/device_tree: Add some FDT helper functions"), and this patch also fixes it in the older dt_find_node(). Change-Id: I323066477a4d4be17225e0915a81ce2ff39c1e40 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/commonlib/device_tree.c 1 file changed, 4 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/85964/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/85964?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: I323066477a4d4be17225e0915a81ce2ff39c1e40 Gerrit-Change-Number: 85964 Gerrit-PatchSet: 2 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Douglas Anderson <dianders(a)chromium.org> Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>

1 0

[S] Change in coreboot[main]: MAINTAINERS: Add Intel reviewers mailing list to relevant sections
by Felix Singer (Code Review) 14 Jan '25

14 Jan '25

Attention is currently required from: Cliff Huang, Jérémy Compostella, Patrick Georgi, Zhixing Ma. Felix Singer has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/85982?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: MAINTAINERS: Add Intel reviewers mailing list to relevant sections ...................................................................... Patch Set 6: (1 comment) Patchset: PS6: Hi, sorry for being a bit late. Please read my reply here https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/message/T3V… -- To view, visit https://review.coreboot.org/c/coreboot/+/85982?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: I2d90d10902dba30222d213632bd0d6d0226b855d Gerrit-Change-Number: 85982 Gerrit-PatchSet: 6 Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Name of user not set #1005776 Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Zhixing Ma <zhixing.ma(a)intel.com> Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Comment-Date: Tue, 14 Jan 2025 21:17:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2025