coreboot-gerrit June 2019

coreboot-gerrit@coreboot.org

1 participants
1454 discussions

Change in ...coreboot[master]: mb/google/octopus: Override VBT selection for Garg
by Kevin Chiu (Code Review) 21 Jun '19

21 Jun '19

Hello Kevin Chiu, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/33395 to review the following change. Change subject: mb/google/octopus: Override VBT selection for Garg ...................................................................... mb/google/octopus: Override VBT selection for Garg Garg proto build has 3 SKUs: garg 2A2C DB: SKU ID - 1 garg HDMI DB: SKU ID - 9 garg LTE DB: SKU ID - 17 For SKU#9, VBT will need to be overridden to enable DDI_C output to HDMI BUG=b:134912735 BRANCH=octopus TEST=emerge-octopus coreboot chromeos-bootimage Change-Id: I6c0ec086496eaf217ea8e326f5084d886d0e698f Signed-off-by: Kevin Chiu <Kevin.Chiu(a)quantatw.com> --- M src/mainboard/google/octopus/variants/garg/Makefile.inc A src/mainboard/google/octopus/variants/garg/mainboard.c 2 files changed, 41 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/33395/1 diff --git a/src/mainboard/google/octopus/variants/garg/Makefile.inc b/src/mainboard/google/octopus/variants/garg/Makefile.inc index 9fb63f5..5b657f4 100644 --- a/src/mainboard/google/octopus/variants/garg/Makefile.inc +++ b/src/mainboard/google/octopus/variants/garg/Makefile.inc @@ -1,3 +1,3 @@ bootblock-y += gpio.c - +ramstage-y += mainboard.c ramstage-y += gpio.c diff --git a/src/mainboard/google/octopus/variants/garg/mainboard.c b/src/mainboard/google/octopus/variants/garg/mainboard.c new file mode 100644 index 0000000..73f0ae5 --- /dev/null +++ b/src/mainboard/google/octopus/variants/garg/mainboard.c @@ -0,0 +1,40 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2018 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <boardid.h> +#include <ec/google/chromeec/ec.h> +#include <drivers/intel/gma/opregion.h> + +enum { + SKU_1_2A2C = 1, + SKU_9_HDMI = 9, + SKU_17_LTE = 17, +}; + +const char *mainboard_vbt_filename(void) +{ + uint32_t sku_id; + + if (google_chromeec_cbi_get_sku_id(&sku_id)) + sku_id = -1; + + switch (sku_id) { + case SKU_9_HDMI: + return "vbt-garg.bin"; + default: + return "vbt.bin"; + break; + } +} -- To view, visit https://review.coreboot.org/c/coreboot/+/33395 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6c0ec086496eaf217ea8e326f5084d886d0e698f Gerrit-Change-Number: 33395 Gerrit-PatchSet: 1 Gerrit-Owner: Kevin Chiu <Kevin.Chiu(a)quantatw.com> Gerrit-Reviewer: Kevin Chiu <kevin.chiu(a)quanta.corp-partner.google.com> Gerrit-MessageType: newchange

7 22

Change in ...coreboot[master]: mb/google/octopus: rename get_board_sku and make it overridable
by Kevin Chiu (Code Review) 21 Jun '19

21 Jun '19

Kevin Chiu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33448 Change subject: mb/google/octopus: rename get_board_sku and make it overridable ...................................................................... mb/google/octopus: rename get_board_sku and make it overridable BUG=b:134912735 BRANCH=octopus TEST=emerge-octopus coreboot chromeos-bootimage Change-Id: I1fb7b5eeac48f2cd9c24fa1d3ac3fe4b390762d2 Signed-off-by: Kevin Chiu <Kevin.Chiu(a)quantatw.com> --- M src/mainboard/google/octopus/mainboard.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h 2 files changed, 4 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/33448/1 diff --git a/src/mainboard/google/octopus/mainboard.c b/src/mainboard/google/octopus/mainboard.c index 4316ffe..31a2fd1 100644 --- a/src/mainboard/google/octopus/mainboard.c +++ b/src/mainboard/google/octopus/mainboard.c @@ -118,7 +118,7 @@ #define SKU_UNKNOWN 0xFFFFFFFF #define SKU_MAX 255 -static uint32_t get_board_sku(void) +uint32_t __weak variant_board_sku(void) { static uint32_t sku_id = SKU_UNKNOWN; @@ -134,7 +134,7 @@ const char *smbios_system_sku(void) { static char sku_str[7]; /* sku{0..255} */ - uint32_t sku_id = get_board_sku(); + uint32_t sku_id = variant_board_sku(); if ((sku_id == SKU_UNKNOWN) || (sku_id > SKU_MAX)) { printk(BIOS_ERR, "%s: Unexpected SKU ID %u\n", diff --git a/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h index 9638118..1df6dea 100644 --- a/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h @@ -34,7 +34,8 @@ const struct lpddr4_cfg *variant_lpddr4_config(void); /* Return memory SKU for the board. */ size_t variant_memory_sku(void); - +/* Return board SKU */ +uint32_t variant_board_sku(void); /* Return ChromeOS gpio table and fill in number of entries. */ const struct cros_gpio *variant_cros_gpios(size_t *num); -- To view, visit https://review.coreboot.org/c/coreboot/+/33448 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1fb7b5eeac48f2cd9c24fa1d3ac3fe4b390762d2 Gerrit-Change-Number: 33448 Gerrit-PatchSet: 1 Gerrit-Owner: Kevin Chiu <Kevin.Chiu(a)quantatw.com> Gerrit-MessageType: newchange

6 14

Change in ...coreboot[master]: {drivers,soc/intel/braswell}: Implement C_ENVIRONMENT_BOOTBLOCK support
by Nico Huber (Code Review) 21 Jun '19

21 Jun '19

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29662 ) Change subject: {drivers,soc/intel/braswell}: Implement C_ENVIRONMENT_BOOTBLOCK support ...................................................................... Patch Set 47: (1 comment) https://review.coreboot.org/#/c/29662/47/src/soc/intel/braswell/bootblock/b… File src/soc/intel/braswell/bootblock/bootblock.c: https://review.coreboot.org/#/c/29662/47/src/soc/intel/braswell/bootblock/b… PS47, Line 40: Far fetched, but maybe this makes a difference. In theory, FSP-T could check if something is already set up and behave differently. Actually, that applies to the whole block and not just the odd 4MiB. -- To view, visit https://review.coreboot.org/c/coreboot/+/29662 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iab48ad72f1514c93f20d70db5ef4fd8fa2383e8c Gerrit-Change-Number: 29662 Gerrit-PatchSet: 47 Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: York Yang <yyang024(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Fri, 21 Jun 2019 09:26:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in ...coreboot[master]: mb/emulation/qemu-riscv: Support arbitrary ROM sizes
by Patrick Rudolph (Code Review) 21 Jun '19

21 Jun '19

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33412 Change subject: mb/emulation/qemu-riscv: Support arbitrary ROM sizes ...................................................................... mb/emulation/qemu-riscv: Support arbitrary ROM sizes Make the linker script dynamic to support non default ROM sizes. Prevents weird runtime issues due to stages overwriting parts of the CBFS while decompressing stages. Change-Id: I37b9187c719b907959f02a272ec0459aabbcda3c Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- M src/mainboard/emulation/qemu-riscv/memlayout.ld 1 file changed, 7 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/33412/1 diff --git a/src/mainboard/emulation/qemu-riscv/memlayout.ld b/src/mainboard/emulation/qemu-riscv/memlayout.ld index 032fbdd..57a7da4 100644 --- a/src/mainboard/emulation/qemu-riscv/memlayout.ld +++ b/src/mainboard/emulation/qemu-riscv/memlayout.ld @@ -17,13 +17,16 @@ #include <arch/header.ld> #define START 0x80000000 +//Stages start after CBFS in DRAM +#define STAGES_START (START + CONFIG_COREBOOT_ROMSIZE_KB * 1024) SECTIONS { DRAM_START(START) BOOTBLOCK(START, 64K) - STACK(START + 4M, 4K) - ROMSTAGE(START + 4M + 64K, 128K) - PRERAM_CBMEM_CONSOLE(START + 4M + 192K, 8K) - RAMSTAGE(START + 4M + 200K, 16M) + // CBFS goes here + STACK(STAGES_START, 4K) + ROMSTAGE(STAGES_START + 64K, 128K) + PRERAM_CBMEM_CONSOLE(STAGES_START + 192K, 8K) + RAMSTAGE(STAGES_START + 200K, 16M) } -- To view, visit https://review.coreboot.org/c/coreboot/+/33412 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I37b9187c719b907959f02a272ec0459aabbcda3c Gerrit-Change-Number: 33412 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

3 5

Change in ...coreboot[master]: device_tree: Update comment style to C89
by Julius Werner (Code Review) 21 Jun '19

21 Jun '19

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/32875 to review the following change. Change subject: device_tree: Update comment style to C89 ...................................................................... device_tree: Update comment style to C89 This code was copied from depthcharge which uses C99 comment style, but coreboot uses C89 comment style. Update to match coreboot. Change-Id: Ib67bb9ff17b7688826071453ab58894a0835ce10 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/include/device_tree.h M src/lib/device_tree.c M src/lib/fit.c 3 files changed, 119 insertions(+), 109 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/32875/1 diff --git a/src/include/device_tree.h b/src/include/device_tree.h index 8d86bc5..56a9361 100644 --- a/src/include/device_tree.h +++ b/src/include/device_tree.h @@ -74,9 +74,9 @@ const char *name; uint32_t phandle; - // List of struct device_tree_property-s. + /* List of struct device_tree_property-s. */ struct list_node properties; - // List of struct device_tree_nodes. + /* List of struct device_tree_nodes. */ struct list_node children; struct list_node list_node; @@ -108,18 +108,18 @@ * which were consumed reading the requested value. */ -// Read the property, if any, at offset offset. +/* Read the property, if any, at offset offset. */ int fdt_next_property(const void *blob, uint32_t offset, struct fdt_property *prop); -// Read the name of the node, if any, at offset offset. +/* Read the name of the node, if any, at offset offset. */ int fdt_node_name(const void *blob, uint32_t offset, const char **name); void fdt_print_node(const void *blob, uint32_t offset); int fdt_skip_node(const void *blob, uint32_t offset); -// Read a flattened device tree into a heirarchical structure which refers to -// the contents of the flattened tree in place. Modifying the flat tree -// invalidates the unflattened one. +/* Read a flattened device tree into a heirarchical structure which refers to + the contents of the flattened tree in place. Modifying the flat tree + invalidates the unflattened one. */ struct device_tree *fdt_unflatten(const void *blob); @@ -128,42 +128,42 @@ * Unflattened device tree functions. */ -// Figure out how big a device tree would be if it were flattened. +/* Figure out how big a device tree would be if it were flattened. */ uint32_t dt_flat_size(const struct device_tree *tree); -// Flatten a device tree into the buffer pointed to by dest. +/* Flatten a device tree into the buffer pointed to by dest. */ void dt_flatten(const struct device_tree *tree, void *dest); void dt_print_node(const struct device_tree_node *node); -// Read #address-cells and #size-cells properties from a node. +/* Read #address-cells and #size-cells properties from a node. */ void dt_read_cell_props(const struct device_tree_node *node, u32 *addrcp, u32 *sizecp); -// Look up or create a node relative to a parent node, through its path -// represented as an array of strings. +/* Look up or create a node relative to a parent node, through its path + represented as an array of strings. */ 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 *dt_find_node_by_phandle(struct device_tree_node *root, uint32_t phandle); -// Look up or create a node in the tree, through its path -// represented as a string of '/' separated node names. +/* Look up or create a node in the tree, through its path + represented as a string of '/' separated node names. */ struct device_tree_node *dt_find_node_by_path(struct device_tree *tree, const char *path, u32 *addrcp, u32 *sizecp, int create); -// Look up a node through an alias. +/* Look up a node through an alias. */ struct device_tree_node *dt_find_node_by_alias(struct device_tree *tree, const char *alias); -// Look up a node relative to a parent node, through its compatible string. +/* Look up a node relative to a parent node, through its compatible string. */ struct device_tree_node *dt_find_compat(struct device_tree_node *parent, const char *compatible); -// Look up the next child of a parent node, through its compatible string. It -// uses child pointer as the marker to find next. +/* Look up the next child of a parent node, through its compatible string. It + uses child pointer as the marker to find next. */ struct device_tree_node *dt_find_next_compat_child(struct device_tree_node *parent, struct device_tree_node *child, const char *compat); -// Look up a node relative to a parent node, through its property value. +/* Look up a node relative to a parent node, through its property value. */ struct device_tree_node *dt_find_prop_value(struct device_tree_node *parent, const char *name, void *data, size_t size); -// Write src into *dest as a 'length'-byte big-endian integer. +/* Write src into *dest as a 'length'-byte big-endian integer. */ void dt_write_int(u8 *dest, u64 src, size_t length); -// Delete a property +/* Delete a property */ void dt_delete_prop(struct device_tree_node *node, const char *name); -// Add different kinds of properties to a node, or update existing ones. +/* Add different kinds of properties to a node, or update existing ones. */ void dt_add_bin_prop(struct device_tree_node *node, const char *name, void *data, size_t size); void dt_add_string_prop(struct device_tree_node *node, const char *name, diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index 05f2ea9..338ac45 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -133,7 +133,7 @@ offset += size; } - printk(BIOS_DEBUG, "\n"); // empty line between props and nodes + printk(BIOS_DEBUG, "\n"); /* empty line between props and nodes */ while ((size = print_flat_node(blob, offset, depth + 1))) offset += size; @@ -277,9 +277,9 @@ uint32_t min_offset = 0; min_offset = MIN(struct_offset, strings_offset); min_offset = MIN(min_offset, reserve_offset); - // Assume everything up to the first non-header component is part of - // the header and needs to be preserved. This will protect us against - // new elements being added in the future. + /* Assume everything up to the first non-header component is part of + the header and needs to be preserved. This will protect us against + new elements being added in the future. */ tree->header_size = min_offset; struct device_tree_reserve_map_entry *entry; @@ -307,25 +307,25 @@ static void dt_flat_prop_size(struct device_tree_property *prop, uint32_t *struct_size, uint32_t *strings_size) { - // Starting token. + /* Starting token. */ *struct_size += sizeof(uint32_t); - // Size. + /* Size. */ *struct_size += sizeof(uint32_t); - // Name offset. + /* Name offset. */ *struct_size += sizeof(uint32_t); - // Property value. + /* Property value. */ *struct_size += ALIGN_UP(prop->prop.size, sizeof(uint32_t)); - // Property name. + /* Property name. */ *strings_size += strlen(prop->prop.name) + 1; } static void dt_flat_node_size(struct device_tree_node *node, uint32_t *struct_size, uint32_t *strings_size) { - // Starting token. + /* Starting token. */ *struct_size += sizeof(uint32_t); - // Node name. + /* Node name. */ *struct_size += ALIGN_UP(strlen(node->name) + 1, sizeof(uint32_t)); struct device_tree_property *prop; @@ -336,7 +336,7 @@ list_for_each(child, node->children, list_node) dt_flat_node_size(child, struct_size, strings_size); - // End token. + /* End token. */ *struct_size += sizeof(uint32_t); } @@ -353,7 +353,7 @@ dt_flat_node_size(tree->root, &struct_size, &strings_size); size += struct_size; - // End token. + /* End token. */ size += sizeof(uint32_t); size += strings_size; @@ -478,7 +478,7 @@ static void print_node(const struct device_tree_node *node, int depth) { print_indent(depth); - if (depth == 0) // root node has no name, print a starting slash + if (depth == 0) /* root node has no name, print a starting slash */ printk(BIOS_DEBUG, "/"); printk(BIOS_DEBUG, "%s {\n", node->name); @@ -486,7 +486,7 @@ list_for_each(prop, node->properties, list_node) print_property(&prop->prop, depth + 1); - printk(BIOS_DEBUG, "\n"); // empty line between props and nodes + printk(BIOS_DEBUG, "\n"); /* empty line between props and nodes */ struct device_tree_node *child; list_for_each(child, node->children, list_node) @@ -546,13 +546,13 @@ { struct device_tree_node *node, *found = NULL; - // Update #address-cells and #size-cells for this level. + /* Update #address-cells and #size-cells for this level. */ dt_read_cell_props(parent, addrcp, sizecp); if (!*path) return parent; - // Find the next node in the path, if it exists. + /* Find the next node in the path, if it exists. */ list_for_each(node, parent->children, list_node) { if (!strcmp(node->name, *path)) { found = node; @@ -560,7 +560,7 @@ } } - // Otherwise create it or return NULL. + /* Otherwise create it or return NULL. */ if (!found) { if (!create) return NULL; @@ -607,8 +607,8 @@ int i; struct device_tree_node *node = NULL; - if (path[0] == '/') { // regular path - if (path[1] == '\0') { // special case: "/" is root node + if (path[0] == '/') { /* regular path */ + if (path[1] == '\0') { /* special case: "/" is root node */ dt_read_cell_props(tree->root, addrcp, sizecp); return tree->root; } @@ -618,7 +618,7 @@ return NULL; parent = tree->root; - } else { // alias + } else { /* alias */ char *alias; alias = duped_str = strdup(path); @@ -639,7 +639,7 @@ } if (!sub_path) { - // it's just the alias, no sub-path + /* it's just the alias, no sub-path */ free(duped_str); return parent; } @@ -754,7 +754,7 @@ struct device_tree_node *dt_find_compat(struct device_tree_node *parent, const char *compat) { - // Check if the parent node itself is compatible. + /* Check if the parent node itself is compatible. */ if (dt_check_compat_match(parent, compat)) return parent; @@ -1089,11 +1089,11 @@ if (!reserved) return NULL; - // Binding doc says this should have the same #{address,size}-cells as - // the root. + /* Binding doc says this should have the same #{address,size}-cells as + the root. */ dt_add_u32_prop(reserved, "#address-cells", addr); dt_add_u32_prop(reserved, "#size-cells", size); - // Binding doc says this should be empty (i.e., 1:1 mapping from root). + /* Binding doc says this should be empty (1:1 mapping from root). */ dt_add_bin_prop(reserved, "ranges", NULL, 0); return reserved; @@ -1139,7 +1139,7 @@ static uint32_t dt_adjust_all_phandles(struct device_tree_node *node, uint32_t base) { - uint32_t new_max = MAX(base, 1); // make sure we don't return 0 + uint32_t new_max = MAX(base, 1); /* make sure we don't return 0 */ struct device_tree_property *prop; struct device_tree_node *child; @@ -1152,7 +1152,7 @@ if (!node->phandle) return 0; new_max = MAX(new_max, node->phandle); - } // no break -- can have more than one phandle prop + } /* no break -- can have more than one phandle prop */ list_for_each(child, node->children, list_node) new_max = MAX(new_max, dt_adjust_all_phandles(child, base)); @@ -1178,11 +1178,13 @@ struct device_tree_node *fixup_child; int i; - // For local fixups the /__local_fixup__ subtree contains the same node - // hierarchy as the main tree we're fixing up. Each property contains - // the fixup offsets for the respective property in the main tree. For - // each property in the fixup node, find the corresponding property in - // the base node and apply fixups to all offsets it specifies. + /* + * For local fixups the /__local_fixup__ subtree contains the same node + * hierarchy as the main tree we're fixing up. Each property contains + * the fixup offsets for the respective property in the main tree. For + * each property in the fixup node, find the corresponding property in + * the base node and apply fixups to all offsets it specifies. + */ list_for_each(fixup_prop, fixup->properties, list_node) { struct device_tree_property *base_prop = NULL; list_for_each(prop, node->properties, list_node) @@ -1191,8 +1193,8 @@ break; } - // We should always find a corresponding base prop for a fixup, - // and fixup props contain a list of 32-bit fixup offsets. + /* We should always find a corresponding base prop for a fixup, + and fixup props contain a list of 32-bit fixup offsets. */ if (!base_prop || fixup_prop->prop.size % sizeof(uint32_t)) return -1; @@ -1202,8 +1204,8 @@ return -1; } - // Now recursively descend both the base tree and the /__local_fixups__ - // subtree in sync to apply all fixups. + /* Now recursively descend both the base tree and the /__local_fixups__ + subtree in sync to apply all fixups. */ list_for_each(fixup_child, fixup->children, list_node) { struct device_tree_node *base_child = NULL; list_for_each(child, node->children, list_node) @@ -1212,7 +1214,7 @@ break; } - // All fixup nodes should have a corresponding base node. + /* All fixup nodes should have a corresponding base node. */ if (!base_child) return -1; @@ -1236,10 +1238,10 @@ const char *base_path) { struct device_tree_property *prop; - char buf[512]; // Should be enough for maximum DT path length? - char node_path[64]; // easily enough for /fragment@XXXX/__overlay__ + char buf[512]; /* Should be enough for maximum DT path length? */ + char node_path[64]; /* easily enough for /fragment@XXXX/__overlay__ */ - if (!symbols) // If the overlay has no /__symbols__ node, we're done! + if (!symbols) /* If the overlay has no /__symbols__ node, we're done! */ return; int len = snprintf(node_path, sizeof(node_path), "/%s/__overlay__", @@ -1273,10 +1275,10 @@ { struct device_tree_property *prop; - // External fixup properties are encoded as "<path>:<prop>:<offset>". + /* External fixup properties are encoded as "<path>:<prop>:<offset>". */ char *entry = fixup->prop.data; while ((void *)entry < fixup->prop.data + fixup->prop.size) { - // okay to destroy fixup property value, won't be needed again + /* okay to destroy fixup property value, won't need it again */ char *node_path = entry; entry = strchr(node_path, ':'); if (!entry) @@ -1301,18 +1303,18 @@ break; } - // Move entry to first char after number, must be a '\0'. + /* Move entry to first char after number, must be a '\0'. */ uint32_t offset = skip_atoi((const char **)&entry); if (!ovl_prop || offset + 4 > ovl_prop->prop.size || entry[0]) return -1; - entry++; // jump over '\0' to potential next fixup + entry++; /* jump over '\0' to potential next fixup */ be32enc(ovl_prop->prop.data + offset, phandle); - // If this is a /fragment@X:target property, update - // references to this fragment in the overlay __symbols__ now. + /* If this is a /fragment@X:target property, update references + to this fragment in the overlay __symbols__ now. */ if (offset == 0 && !strcmp(prop_name, "target") && - !strchr(node_path + 1, '/')) // only toplevel nodes + !strchr(node_path + 1, '/')) /* only toplevel nodes */ dt_fix_symbols(overlay_symbols, ovl_node, base_path); } @@ -1339,27 +1341,29 @@ { struct device_tree_property *fix; - // If we have any external fixups, the base tree must have /__symbols__. + /* If we have any external fixups, base tree must have /__symbols__. */ if (!symbols) return -1; - // Unlike /__local_fixups__, /__fixups__ is not a whole subtree that - // mirrors the node hierarchy. It's just a directory of fixup properties - // that each directly contain all information necessary to apply them. + /* + * Unlike /__local_fixups__, /__fixups__ is not a whole subtree that + * mirrors the node hierarchy. It's just a directory of fixup properties + * that each directly contain all information necessary to apply them. + */ list_for_each(fix, fixups->properties, list_node) { - // The name of a fixup property is the label of the node we want - // a property to phandle-reference. Look it up in /__symbols__. + /* The name of a fixup property is the label of the node we want + a property to phandle-reference. Look up in /__symbols__. */ const char *path = dt_find_string_prop(symbols, fix->prop.name); if (!path) return -1; - // Find the node the label pointed to to figure out its phandle. + /* Find node the label pointed to to figure out its phandle. */ struct device_tree_node *node = dt_find_node_by_path(tree, path, NULL, NULL, 0); if (!node) return -1; - // Write it into the overlay property(s) pointing to that node. + /* Write into the overlay property(s) pointing to that node. */ if (dt_fixup_external(overlay, fix, node->phandle, path, overlay_symbols) < 0) return -1; @@ -1446,23 +1450,23 @@ struct device_tree_node *fragment, struct device_tree_node *overlay_symbols) { - // The actually overlaid nodes/props are in an __overlay__ child node. + /* The actual overlaid nodes/props are in an __overlay__ child node. */ static const char *overlay_path[] = { "__overlay__", NULL }; struct device_tree_node *overlay = dt_find_node(fragment, overlay_path, NULL, NULL, 0); - // If it doesn't have an __overlay__ child, it's not a fragment. + /* If it doesn't have an __overlay__ child, it's not a fragment. */ if (!overlay) return 0; - // The target node of the fragment can be given by path or by phandle. + /* Target node of the fragment can be given by path or by phandle. */ struct device_tree_property *prop; struct device_tree_property *phandle = NULL; struct device_tree_property *path = NULL; list_for_each(prop, fragment->properties, list_node) { if (!strcmp(prop->prop.name, "target")) { phandle = prop; - break; // phandle target has priority, stop looking here + break; /* phandle target has priority, stop looking */ } if (!strcmp(prop->prop.name, "target-path")) path = prop; @@ -1474,7 +1478,7 @@ return -1; target = dt_find_node_by_phandle(tree->root, be32dec(phandle->prop.data)); - // Symbols already updated as part of dt_fixup_external(target). + /* Symbols already updated as part of dt_fixup_external(). */ } else if (path) { target = dt_find_node_by_path(tree, path->prop.data, NULL, NULL, 0); @@ -1499,10 +1503,12 @@ */ int dt_apply_overlay(struct device_tree *tree, struct device_tree *overlay) { - // First, we need to make sure phandles inside the overlay don't clash - // with those in the base tree. We just define the highest phandle value - // in the base tree as the "phandle offset" for this overlay and - // increment all phandles in it by that value. + /* + * First, we need to make sure phandles inside the overlay don't clash + * with those in the base tree. We just define the highest phandle value + * in the base tree as the "phandle offset" for this overlay and + * increment all phandles in it by that value. + */ uint32_t phandle_base = tree->max_phandle; uint32_t new_max = dt_adjust_all_phandles(overlay->root, phandle_base); if (!new_max) { @@ -1511,8 +1517,8 @@ } tree->max_phandle = new_max; - // Now that we changed phandles in the overlay, we need to update any - // nodes referring to them. Those are listed in /__local_fixups__. + /* Now that we changed phandles in the overlay, we need to update any + nodes referring to them. Those are listed in /__local_fixups__. */ struct device_tree_node *local_fixups = dt_find_node_by_path(overlay, "/__local_fixups__", NULL, NULL, 0); if (local_fixups && dt_fixup_locals(overlay->root, local_fixups, @@ -1521,16 +1527,18 @@ return -1; } - // Besides local phandle references (from nodes within the overlay to - // other nodes within the overlay), the overlay may also contain phandle - // references to the base tree. These are stored with invalid values and - // must be updated now. /__symbols__ contains a list of all labels in - // the base tree, and /__fixups__ describes all nodes in the overlay - // that contain external phandle references. - // We also take this opportunity to update all /fragment@X/__overlay__/ - // prefixes in the overlay's /__symbols__ node to the correct path that - // the fragment will be placed in later, since this is the only step - // where we have all necessary information for that easily available. + /* + * Besides local phandle references (from nodes within the overlay to + * other nodes within the overlay), the overlay may also contain phandle + * references to the base tree. These are stored with invalid values and + * must be updated now. /__symbols__ contains a list of all labels in + * the base tree, and /__fixups__ describes all nodes in the overlay + * that contain external phandle references. + * We also take this opportunity to update all /fragment@X/__overlay__/ + * prefixes in the overlay's /__symbols__ node to the correct path that + * the fragment will be placed in later, since this is the only step + * where we have all necessary information for that easily available. + */ struct device_tree_node *symbols = dt_find_node_by_path(tree, "/__symbols__", NULL, NULL, 0); struct device_tree_node *fixups = dt_find_node_by_path(overlay, @@ -1544,8 +1552,8 @@ return -1; } - // After all this fixing up, we can finally merge the overlay into the - // tree (one fragment at a time, because for some reason it's split up). + /* After all this fixing up, we can finally merge overlay into the tree + (one fragment at a time, because for some reason it's split up). */ struct device_tree_node *fragment; list_for_each(fragment, overlay->root->children, list_node) if (dt_import_fragment(tree, fragment, overlay_symbols) < 0) { @@ -1554,11 +1562,13 @@ return -1; } - // We need to also update /__symbols__ to include labels from this - // overlay, in case we want to load further overlays with external - // phandle references to it. If the base tree already has a /__symbols__ - // we merge them together, otherwise we just insert the overlay's - // /__symbols__ node into the base tree root. + /* + * We need to also update /__symbols__ to include labels from this + * overlay, in case we want to load further overlays with external + * phandle references to it. If the base tree already has a /__symbols__ + * we merge them together, otherwise we just insert the overlay's + * /__symbols__ node into the base tree root. + */ if (overlay_symbols) { if (symbols) dt_copy_subtree(symbols, overlay_symbols, 0); diff --git a/src/lib/fit.c b/src/lib/fit.c index 0d3cb2d..be243d7 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -419,8 +419,8 @@ */ static int fit_update_compat(struct fit_config_node *config) { - // If there was no "compatible" property in config node, this is a - // legacy FIT image. Must extract compat prop from FDT itself. + /* If there was no "compatible" property in config node, this is a + legacy FIT image. Must extract compat prop from FDT itself. */ if (!config->compat.name) { void *fdt_blob = config->fdt->data; const struct fdt_header *fdt_header = fdt_blob; @@ -434,7 +434,7 @@ return -1; } - // FDT overlays are not supported in legacy FIT images. + /* FDT overlays are not supported in legacy FIT images. */ if (config->overlays.next) { printk(BIOS_ERR, "ERROR: config %s has overlay but no compat!\n", -- To view, visit https://review.coreboot.org/c/coreboot/+/32875 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib67bb9ff17b7688826071453ab58894a0835ce10 Gerrit-Change-Number: 32875 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

3 3

Change in ...coreboot[master]: fit: Check all compat strings for highest match
by Julius Werner (Code Review) 21 Jun '19

21 Jun '19

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/32874 to review the following change. Change subject: fit: Check all compat strings for highest match ...................................................................... fit: Check all compat strings for highest match The compat string matching code was mostly copied from depthcharge. One of the few differences is that we now store the list of compat strings we're willing to match in a list rather than an array. Since our lists insert at the front, that means the strings are now ordered lowest to highest (not highest to lowest like in depthcharge). We did rewrite the compat_rank matching code to accomodate for that... however, what we didn't do is remove the break-statement in the loop that matches all compat strings. When we search the lowest priority first, we can't abort the search as soon as we found a match -- we have to keep looking because we might find a higher priority match later. This patch fixes the issue so that my Kevin can actually match for google,kevin-rev5 (and doesn't just jump at the first best google,kevin match). Change-Id: Ibe3d84bbce6de3cd49c746a667ae1ccfdc843105 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/lib/fit.c 1 file changed, 0 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/32874/1 diff --git a/src/lib/fit.c b/src/lib/fit.c index 1b02c6a..0d3cb2d 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -463,7 +463,6 @@ config->compat_rank = i; config->compat_string = compat_node->compat_string; - break; } i++; } -- To view, visit https://review.coreboot.org/c/coreboot/+/32874 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibe3d84bbce6de3cd49c746a667ae1ccfdc843105 Gerrit-Change-Number: 32874 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

3 4

Change in ...coreboot[master]: fit: Add overlay support
by Julius Werner (Code Review) 21 Jun '19

21 Jun '19

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/32873 to review the following change. Change subject: fit: Add overlay support ...................................................................... fit: Add overlay support This patch adds support to boot FIT image configurations consisting of a base device tree and one or more overlays. Since extracting the right compatible string from overlay FDTs is problematic, we'll only support this for FIT images that have the compatible string pulled out into the config node. This patch was adapted from depthcharge's http://crosreview.com/1555293 Change-Id: I0943f9a1869c9e416887c7ff16e33f7d91b74989 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/include/fit.h M src/lib/fit.c M src/lib/fit_payload.c 3 files changed, 54 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/32873/1 diff --git a/src/include/fit.h b/src/include/fit.h index 758ee70..a5a9e69 100644 --- a/src/include/fit.h +++ b/src/include/fit.h @@ -39,6 +39,7 @@ const char *name; struct fit_image_node *kernel; struct fit_image_node *fdt; + struct list_node overlays; struct fit_image_node *ramdisk; struct fdt_property compat; int compat_rank; @@ -48,6 +49,12 @@ struct list_node list_node; }; +struct fit_overlay_chain +{ + struct fit_image_node *overlay; + struct list_node list_node; +}; + /* * Updates the cmdline in the devicetree. */ diff --git a/src/lib/fit.c b/src/lib/fit.c index 464be3a..1b02c6a 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -96,6 +96,31 @@ return NULL; } +static struct fit_image_node *find_image_with_overlays(const char *name, + int bytes, struct list_node *prev) +{ + struct fit_image_node *base = find_image(name); + if (!base) + return NULL; + + int len = strnlen(name, bytes) + 1; + bytes -= len; + name += len; + while (bytes > 0) { + struct fit_overlay_chain *next = xzalloc(sizeof(*next)); + next->overlay = find_image(name); + if (!next->overlay) + return NULL; + list_insert_after(&next->list_node, prev); + prev = &next->list_node; + len = strnlen(name, bytes) + 1; + bytes -= len; + name += len; + } + + return base; +} + static void image_node(struct device_tree_node *node) { struct fit_image_node *image = xzalloc(sizeof(*image)); @@ -133,7 +158,8 @@ if (!strcmp("kernel", prop->prop.name)) config->kernel = find_image(prop->prop.data); else if (!strcmp("fdt", prop->prop.name)) - config->fdt = find_image(prop->prop.data); + config->fdt = find_image_with_overlays(prop->prop.data, + prop->prop.size, &config->overlays); else if (!strcmp("ramdisk", prop->prop.name)) config->ramdisk = find_image(prop->prop.data); else if (!strcmp("compatible", prop->prop.name)) @@ -408,6 +434,14 @@ return -1; } + // FDT overlays are not supported in legacy FIT images. + if (config->overlays.next) { + printk(BIOS_ERR, + "ERROR: config %s has overlay but no compat!\n", + config->name); + return -1; + } + if (fdt_find_compat(fdt_blob, fdt_offset, &config->compat)) { printk(BIOS_ERR, "ERROR: Can't find compat string in FDT %s " @@ -442,6 +476,7 @@ struct fit_image_node *image; struct fit_config_node *config; struct compat_string_entry *compat_node; + struct fit_overlay_chain *overlay_chain; printk(BIOS_DEBUG, "FIT: Loading FIT from %p\n", fit); @@ -505,6 +540,8 @@ } printk(BIOS_DEBUG, ", kernel %s", config->kernel->name); printk(BIOS_DEBUG, ", fdt %s", config->fdt->name); + list_for_each(overlay_chain, config->overlays, list_node) + printk(BIOS_DEBUG, " %s", overlay_chain->overlay->name); if (config->ramdisk) printk(BIOS_DEBUG, ", ramdisk %s", config->ramdisk->name); if (config->compat.name) { diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index b049188..8e75915 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -206,6 +206,15 @@ return; } + struct fit_overlay_chain *chain; + list_for_each(chain, config->overlays, list_node) { + struct device_tree *overlay = unpack_fdt(chain->overlay); + if (!overlay || dt_apply_overlay(dt, overlay)) { + printk(BIOS_ERR, "ERROR: Failed to apply overlay %s!\n", + chain->overlay->name); + } + } + dt_apply_fixups(dt); /* Insert coreboot specific information */ -- To view, visit https://review.coreboot.org/c/coreboot/+/32873 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0943f9a1869c9e416887c7ff16e33f7d91b74989 Gerrit-Change-Number: 32873 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

4 4

Change in ...coreboot[master]: fit: Add device tree compression
by Julius Werner (Code Review) 21 Jun '19

21 Jun '19

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/32872 to review the following change. Change subject: fit: Add device tree compression ...................................................................... fit: Add device tree compression This patch adds support for compressing individual device trees in the FIT image. In order to make this efficient, we'll have to pull the compatible property out of the FDT and store it directly in the config node of the FIT image, so that we don't have to scan (and therefore decompress) every single FDT on boot. Device tree compression is only supported for FIT images that have this external compatible property. For older images with no compression, we still support fallback to scanning the FDT for the property. This patch was adapted from depthcharge's http://crosreview.com/1553458 Change-Id: Ifcb6997782c480c8ef6692df17b66ad96264e623 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/lib/fit.c M src/lib/fit_payload.c 2 files changed, 55 insertions(+), 25 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/32872/1 diff --git a/src/lib/fit.c b/src/lib/fit.c index aa3fee7..464be3a 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -136,6 +136,8 @@ config->fdt = find_image(prop->prop.data); else if (!strcmp("ramdisk", prop->prop.name)) config->ramdisk = find_image(prop->prop.data); + else if (!strcmp("compatible", prop->prop.name)) + config->compat = prop->prop; } list_insert_after(&config->list_node, &config_nodes); @@ -391,35 +393,45 @@ */ static int fit_update_compat(struct fit_config_node *config) { - if (config->fdt->compression != CBFS_COMPRESS_NONE) { - printk(BIOS_ERR, - "FDT compression not yet supported, skipping %s.\n", - config->name); - return -1; - } + // If there was no "compatible" property in config node, this is a + // legacy FIT image. Must extract compat prop from FDT itself. + if (!config->compat.name) { + void *fdt_blob = config->fdt->data; + const struct fdt_header *fdt_header = fdt_blob; + uint32_t fdt_offset = be32_to_cpu(fdt_header->structure_offset); - void *fdt_blob = config->fdt->data; - struct compat_string_entry *compat_node; - const struct fdt_header *fdt_header = - (const struct fdt_header *)fdt_blob; - uint32_t fdt_offset = be32_to_cpu(fdt_header->structure_offset); - size_t i = 0; + if (config->fdt->compression != CBFS_COMPRESS_NONE) { + printk(BIOS_ERR, + "ERROR: config %s has a compressed FDT without " + "external compatible property, skipping.\n", + config->name); + return -1; + } + + if (fdt_find_compat(fdt_blob, fdt_offset, &config->compat)) { + printk(BIOS_ERR, + "ERROR: Can't find compat string in FDT %s " + "for config %s, skipping.\n", + config->fdt->name, config->name); + return -1; + } + } config->compat_pos = -1; config->compat_rank = -1; - if (!fdt_find_compat(fdt_blob, fdt_offset, &config->compat)) { - list_for_each(compat_node, compat_strings, list_node) { - int pos = fit_check_compat(&config->compat, - compat_node->compat_string); - if (pos >= 0) { - config->compat_pos = pos; - config->compat_rank = i; - config->compat_string = - compat_node->compat_string; - break; - } - i++; + size_t i = 0; + struct compat_string_entry *compat_node; + list_for_each(compat_node, compat_strings, list_node) { + int pos = fit_check_compat(&config->compat, + compat_node->compat_string); + if (pos >= 0) { + config->compat_pos = pos; + config->compat_rank = i; + config->compat_string = + compat_node->compat_string; + break; } + i++; } return 0; diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index c977903..b049188 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -96,6 +96,24 @@ return false; } +static struct device_tree *unpack_fdt(struct fit_image_node *image_node) +{ + void *data = image_node->data; + + if (image_node->compression != CBFS_COMPRESS_NONE) { + /* TODO: This is an ugly heuristic for how much the size will + expand on decompression, fix once FIT images support storing + the real uncompressed size. */ + struct region r = { .offset = 0, .size = image_node->size * 5 }; + data = malloc(r.size); + r.offset = (uintptr_t)data; + if (!data || extract(&r, image_node)) + return NULL; + } + + return fdt_unflatten(data); +} + /** * Add coreboot tables, CBMEM information and optional board specific strapping * IDs to the device tree loaded via FIT. @@ -181,7 +199,7 @@ return; } - dt = fdt_unflatten(config->fdt->data); + dt = unpack_fdt(config->fdt); if (!dt) { printk(BIOS_ERR, "ERROR: Failed to unflatten the FDT.\n"); rdev_munmap(prog_rdev(payload), data); -- To view, visit https://review.coreboot.org/c/coreboot/+/32872 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ifcb6997782c480c8ef6692df17b66ad96264e623 Gerrit-Change-Number: 32872 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

3 2

Change in ...coreboot[master]: fit_payload: Don't call prog_segment_loaded() on extracted images
by Julius Werner (Code Review) 21 Jun '19

21 Jun '19

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/32871 to review the following change. Change subject: fit_payload: Don't call prog_segment_loaded() on extracted images ...................................................................... fit_payload: Don't call prog_segment_loaded() on extracted images Kernel handoff on all architectures supporting FIT images already includes flushing and disabling the cache. No need to waste any more time flushing individual components (especially since in the case of compressed DT overlays they will still get accessed again afterwards). Change-Id: I7b483e920c5a71663b024b5b50804ffc84939830 Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/lib/fit_payload.c 1 file changed, 2 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/32871/1 diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index 4bc6622..c977903 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -88,12 +88,11 @@ } if (!true_size) { - printk(BIOS_ERR, "ERROR: %s node failed!\n", comp_name); + printk(BIOS_ERR, "ERROR: %s decompression failed!\n", + comp_name); return true; } - prog_segment_loaded(region->offset, true_size, 0); - return false; } -- To view, visit https://review.coreboot.org/c/coreboot/+/32871 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7b483e920c5a71663b024b5b50804ffc84939830 Gerrit-Change-Number: 32871 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

3 2

Change in ...coreboot[master]: fit: Refactor config node handling
by Julius Werner (Code Review) 21 Jun '19

21 Jun '19

Hello Patrick Rudolph, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/32870 to review the following change. Change subject: fit: Refactor config node handling ...................................................................... fit: Refactor config node handling This patch makes some minor refactoring to the way the FIT parser handles config nodes. A lot of this code was written in the dawn age of depthcharge when its device tree library wasn't as well-stocked yet, so some of it can be rewritten nicer with more high-level primitives. There's no point in storing both the string name and the actual FDT node of a FIT image node separately, since the latter also contains the former, so remove that. Also eliminate code for the case of not having an FDT (which makes no sense), and move some more FDT validity/compat checking into fit_update_compat() (mostly in anticipation of later changes). This patch was adapted from depthcharge's http://crosreview.com/1553456 with a couple of modifications specific to coreboot's custom FIT loading code. Change-Id: Ia79e0fd0e1159c4aca64c453b82a0379b133350d Signed-off-by: Julius Werner <jwerner(a)chromium.org> --- M src/arch/arm64/fit_payload.c M src/include/fit.h M src/lib/fit.c M src/lib/fit_payload.c 4 files changed, 92 insertions(+), 117 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/32870/1 diff --git a/src/arch/arm64/fit_payload.c b/src/arch/arm64/fit_payload.c index c4bbcee..fd1bae1 100644 --- a/src/arch/arm64/fit_payload.c +++ b/src/arch/arm64/fit_payload.c @@ -184,20 +184,14 @@ bool place_anywhere; void *arg = NULL; - if (!config->fdt || !fdt) { - printk(BIOS_CRIT, "CRIT: Providing a valid FDT is mandatory to " - "boot an ARM64 kernel!\n"); - return false; - } - - if (!decompress_kernel_header(config->kernel_node)) { + if (!decompress_kernel_header(config->kernel)) { printk(BIOS_CRIT, "CRIT: Payload doesn't look like an ARM64" " kernel Image.\n"); return false; } /* Update kernel size from image header, if possible */ - kernel->size = get_kernel_size(config->kernel_node); + kernel->size = get_kernel_size(config->kernel); printk(BIOS_DEBUG, "FIT: Using kernel size of 0x%zx bytes\n", kernel->size); diff --git a/src/include/fit.h b/src/include/fit.h index 6e0667f..758ee70 100644 --- a/src/include/fit.h +++ b/src/include/fit.h @@ -27,7 +27,7 @@ struct fit_image_node { const char *name; - const void *data; + void *data; uint32_t size; int compression; @@ -37,12 +37,9 @@ struct fit_config_node { const char *name; - const char *kernel; - struct fit_image_node *kernel_node; - const char *fdt; - struct fit_image_node *fdt_node; - const char *ramdisk; - struct fit_image_node *ramdisk_node; + struct fit_image_node *kernel; + struct fit_image_node *fdt; + struct fit_image_node *ramdisk; struct fdt_property compat; int compat_rank; int compat_pos; diff --git a/src/lib/fit.c b/src/lib/fit.c index c98ba2f..aa3fee7 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -85,6 +85,17 @@ fit_add_compat_string(compat_string); } +static struct fit_image_node *find_image(const char *name) +{ + struct fit_image_node *image; + list_for_each(image, image_nodes, list_node) { + if (!strcmp(image->name, name)) + return image; + } + printk(BIOS_ERR, "ERROR: Cannot find image node %s!\n", name); + return NULL; +} + static void image_node(struct device_tree_node *node) { struct fit_image_node *image = xzalloc(sizeof(*image)); @@ -120,11 +131,11 @@ struct device_tree_property *prop; list_for_each(prop, node->properties, list_node) { if (!strcmp("kernel", prop->prop.name)) - config->kernel = prop->prop.data; + config->kernel = find_image(prop->prop.data); else if (!strcmp("fdt", prop->prop.name)) - config->fdt = prop->prop.data; + config->fdt = find_image(prop->prop.data); else if (!strcmp("ramdisk", prop->prop.name)) - config->ramdisk = prop->prop.data; + config->ramdisk = find_image(prop->prop.data); } list_insert_after(&config->list_node, &config_nodes); @@ -132,40 +143,22 @@ static void fit_unpack(struct device_tree *tree, const char **default_config) { - assert(tree && tree->root); + struct device_tree_node *child; + struct device_tree_node *images = dt_find_node_by_path(tree, "/images", + NULL, NULL, 0); + if (images) + list_for_each(child, images->children, list_node) + image_node(child); - struct device_tree_node *top; - list_for_each(top, tree->root->children, list_node) { - struct device_tree_node *child; - if (!strcmp("images", top->name)) { - - list_for_each(child, top->children, list_node) - image_node(child); - - } else if (!strcmp("configurations", top->name)) { - struct device_tree_property *prop; - list_for_each(prop, top->properties, list_node) { - if (!strcmp("default", prop->prop.name) && - default_config) - *default_config = prop->prop.data; - } - - list_for_each(child, top->children, list_node) - config_node(child); - } + struct device_tree_node *configs = dt_find_node_by_path(tree, + "/configurations", NULL, NULL, 0); + if (configs) { + *default_config = dt_find_string_prop(configs, "default"); + list_for_each(child, configs->children, list_node) + config_node(child); } } -static struct fit_image_node *find_image(const char *name) -{ - struct fit_image_node *image; - list_for_each(image, image_nodes, list_node) { - if (!strcmp(image->name, name)) - return image; - } - return NULL; -} - static int fdt_find_compat(const void *blob, uint32_t start_offset, struct fdt_property *prop) { @@ -393,18 +386,27 @@ /* * Finds a compat string and updates the compat position and rank. - * @param fdt_blob Pointer to FDT * @param config The current config node to operate on + * @return 0 if compat updated, -1 if this FDT cannot be used. */ -static void fit_update_compat(const void *fdt_blob, - struct fit_config_node *config) +static int fit_update_compat(struct fit_config_node *config) { + if (config->fdt->compression != CBFS_COMPRESS_NONE) { + printk(BIOS_ERR, + "FDT compression not yet supported, skipping %s.\n", + config->name); + return -1; + } + + void *fdt_blob = config->fdt->data; struct compat_string_entry *compat_node; const struct fdt_header *fdt_header = (const struct fdt_header *)fdt_blob; uint32_t fdt_offset = be32_to_cpu(fdt_header->structure_offset); size_t i = 0; + config->compat_pos = -1; + config->compat_rank = -1; if (!fdt_find_compat(fdt_blob, fdt_offset, &config->compat)) { list_for_each(compat_node, compat_strings, list_node) { int pos = fit_check_compat(&config->compat, @@ -419,6 +421,8 @@ i++; } } + + return 0; } struct fit_config_node *fit_load(void *fit) @@ -457,55 +461,40 @@ printk(BIOS_DEBUG, "\n"); /* Process and list the configs. */ list_for_each(config, config_nodes, list_node) { - if (config->kernel) - config->kernel_node = find_image(config->kernel); - if (config->fdt) - config->fdt_node = find_image(config->fdt); - if (config->ramdisk) - config->ramdisk_node = find_image(config->ramdisk); + if (!config->kernel) { + printk(BIOS_ERR, + "ERROR: config %s has no kernel, skipping.\n", + config->name); + continue; + } + if (!config->fdt) { + printk(BIOS_ERR, + "ERROR: config %s has no FDT, skipping.\n", + config->name); + continue; + } - if (config->ramdisk_node && - config->ramdisk_node->compression < 0) { + if (config->ramdisk && + config->ramdisk->compression < 0) { printk(BIOS_WARNING, "WARN: Ramdisk is compressed with " "an unsupported algorithm, discarding config %s." "\n", config->name); - list_remove(&config->list_node); continue; } - if (!config->kernel_node || - (config->fdt && !config->fdt_node)) { - printk(BIOS_DEBUG, "FIT: Missing image, discarding " - "config %s.\n", config->name); - list_remove(&config->list_node); + if (fit_update_compat(config)) continue; - } - if (config->fdt_node) { - if (config->fdt_node->compression != - CBFS_COMPRESS_NONE) { - printk(BIOS_DEBUG, - "FIT: FDT compression not yet supported," - " skipping config %s.\n", config->name); - list_remove(&config->list_node); - continue; - } - - config->compat_pos = -1; - config->compat_rank = -1; - - fit_update_compat(config->fdt_node->data, config); - } printk(BIOS_DEBUG, "FIT: config %s", config->name); if (default_config_name && !strcmp(config->name, default_config_name)) { printk(BIOS_DEBUG, " (default)"); default_config = config; } - if (config->fdt) - printk(BIOS_DEBUG, ", fdt %s", config->fdt); - if (config->ramdisk) - printk(BIOS_DEBUG, ", ramdisk %s", config->ramdisk); + printk(BIOS_DEBUG, ", kernel %s", config->kernel->name); + printk(BIOS_DEBUG, ", fdt %s", config->fdt->name); + if (config->ramdisk) printk(BIOS_DEBUG, + ", ramdisk %s", config->ramdisk->name); if (config->compat.name) { printk(BIOS_DEBUG, ", compat"); int bytes = config->compat.size; diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index e0158f2..4bc6622 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -176,37 +176,34 @@ struct fit_config_node *config = fit_load(data); - if (!config || !config->kernel_node) { + if (!config) { printk(BIOS_ERR, "ERROR: Could not load FIT\n"); rdev_munmap(prog_rdev(payload), data); return; } - if (config->fdt_node) { - dt = fdt_unflatten(config->fdt_node->data); - if (!dt) { - printk(BIOS_ERR, - "ERROR: Failed to unflatten the FDT.\n"); - rdev_munmap(prog_rdev(payload), data); - return; - } - - dt_apply_fixups(dt); - - /* Insert coreboot specific information */ - add_cb_fdt_data(dt); - - /* Update device_tree */ -#if defined(CONFIG_LINUX_COMMAND_LINE) - fit_update_chosen(dt, (char *)CONFIG_LINUX_COMMAND_LINE); -#endif - fit_update_memory(dt); + dt = fdt_unflatten(config->fdt->data); + if (!dt) { + printk(BIOS_ERR, "ERROR: Failed to unflatten the FDT.\n"); + rdev_munmap(prog_rdev(payload), data); + return; } + dt_apply_fixups(dt); + + /* Insert coreboot specific information */ + add_cb_fdt_data(dt); + + /* Update device_tree */ +#if defined(CONFIG_LINUX_COMMAND_LINE) + fit_update_chosen(dt, (char *)CONFIG_LINUX_COMMAND_LINE); +#endif + fit_update_memory(dt); + /* Collect infos for fit_payload_arch */ - kernel.size = config->kernel_node->size; + kernel.size = config->kernel->size; fdt.size = dt ? dt_flat_size(dt) : 0; - initrd.size = config->ramdisk_node ? config->ramdisk_node->size : 0; + initrd.size = config->ramdisk ? config->ramdisk->size : 0; /* Invoke arch specific payload placement and fixups */ if (!fit_payload_arch(payload, config, &kernel, &fdt, &initrd)) { @@ -216,17 +213,15 @@ return; } - /* Load the images to given position */ - if (config->fdt_node) { - /* Update device_tree */ - if (config->ramdisk_node) - fit_add_ramdisk(dt, (void *)initrd.offset, initrd.size); + /* Update ramdisk location in FDT */ + if (config->ramdisk) + fit_add_ramdisk(dt, (void *)initrd.offset, initrd.size); - pack_fdt(&fdt, dt); - } + /* Repack FDT for handoff to kernel */ + pack_fdt(&fdt, dt); - if (config->ramdisk_node && - extract(&initrd, config->ramdisk_node)) { + if (config->ramdisk && + extract(&initrd, config->ramdisk)) { printk(BIOS_ERR, "ERROR: Failed to extract initrd\n"); prog_set_entry(payload, NULL, NULL); rdev_munmap(prog_rdev(payload), data); @@ -235,7 +230,7 @@ timestamp_add_now(TS_KERNEL_DECOMPRESSION); - if (extract(&kernel, config->kernel_node)) { + if (extract(&kernel, config->kernel)) { printk(BIOS_ERR, "ERROR: Failed to extract kernel\n"); prog_set_entry(payload, NULL, NULL); rdev_munmap(prog_rdev(payload), data); -- To view, visit https://review.coreboot.org/c/coreboot/+/32870 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia79e0fd0e1159c4aca64c453b82a0379b133350d Gerrit-Change-Number: 32870 Gerrit-PatchSet: 1 Gerrit-Owner: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

4 9

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2019