Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41007 )
Change subject: util/sconfig: Drop use of ref_count for chip_instance ......................................................................
util/sconfig: Drop use of ref_count for chip_instance
chip_instance structure currently uses a ref_count to determine how many devices hold reference to that instance. If the count drops to zero, then it is assumed that the chip instance is a duplicate in override tree and has a similar instance that is already overriden in base device tree.
ref_count is currently decremented whenever a device in override tree matches the one in base device tree and the registers from the override tree instance are copied over to the base tree instance. On the other hand, if a device in override tree does not match any device in base tree under a given parent, then the device is added to base tree and all the devices in its subtree that hold pointers to its parent chip instance are updated to point to the parent's chip instance in base tree. This is done as part of update_chip_pointers.
However, there are a couple of issues that this suffers from: a) If a device is present only in override tree and it does not have its own chip (i.e. pointing to parent's chip instance), then it results in sconfig emiiting parent's chip instance (which can be the SoC chip instance) in static.c even though it is unused. This is because update_chip_pointers() does not call delete_chip_instance() before reassigning the chip instance pointer. b) If a device is added under root device only in the override tree and it does not have its own chip instance (i.e. uses SoC chip instance), then it results in sconfig emitting a copy of the SoC chip instance and setting that as chip_ops for this new device in the override tree.
In order to fix the above issues, this change drops the ref_count field from chip_instance structure and instead adds a forwarding pointer `base_chip_instance`. This is setup as per the following rules: 1. If the instance belongs to base devicetree, base_chip_instance is set to NULL. 2. If the instance belongs to override tree, then it is set to its corresponding chip instance in base tree (if present), else set to NULL.
State of base_chip_instance is then used when emitting chips and devices using the following rules: 1. If a chip_instance has non-NULL base_chip_instance, then that chip instance is not emitted to static.c 2. When emitting chip_ops for a device, base_chip_instance is used to determine the correct chip instance name to emit.
BUG=b:155549176 TEST=Verified that the static.c file generated for base/override tree combination is correct when new devices without chips are added only to override tree.
Change-Id: Idbb5b34f49bf874da3f30ebb6a6a0e2d8d091fe5 Signed-off-by: Furquan Shaikh furquan@google.com --- M util/sconfig/main.c M util/sconfig/sconfig.h 2 files changed, 30 insertions(+), 89 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/41007/1
diff --git a/util/sconfig/main.c b/util/sconfig/main.c index 951ad6d..186eedb 100644 --- a/util/sconfig/main.c +++ b/util/sconfig/main.c @@ -128,7 +128,6 @@ static struct chip_instance mainboard_instance = { .id = 0, .chip = &mainboard_chip, - .ref_count = 2, };
/* This is the parent of all devices added by parsing the devicetree file. */ @@ -337,54 +336,6 @@ return instance; }
-static void delete_chip_instance(struct chip_instance *ins) -{ - - if (ins->ref_count == 0) { - printf("ERROR: ref count for chip instance is zero!!\n"); - exit(1); - } - - if (--ins->ref_count) - return; - - struct chip *c = ins->chip; - - /* Get pointer to first instance of the chip. */ - struct chip_instance *i = c->instance; - - /* - * If chip instance to be deleted is the first instance, then update - * instance pointer of the chip as well. - */ - if (i == ins) { - c->instance = ins->next; - free(ins); - return; - } - - /* - * Loop through the instances list of the chip to find and remove the - * given instance. - */ - while (1) { - if (i == NULL) { - printf("ERROR: chip instance not found!\n"); - exit(1); - } - - if (i->next != ins) { - i = i->next; - continue; - } - - i->next = ins->next; - break; - } - - free(ins); -} - /* * Allocate a new bus for the provided device. * - If this is the first bus being allocated under this device, then its id @@ -504,7 +455,6 @@ new_d->hidden = (status >> 1) & 0x01; new_d->mandatory = (status >> 2) & 0x01; new_d->chip_instance = chip_instance; - chip_instance->ref_count++;
set_new_child(parent, new_d);
@@ -780,6 +730,13 @@ struct chip_instance *chip_ins = ptr->chip_instance; int has_children = dev_has_children(ptr);
+ /* + * If the chip instance of device has base_chip_instance pointer set, then follow that + * to update the chip instance for current device. + */ + if (chip_ins->base_chip_instance) + chip_ins = chip_ins->base_chip_instance; + if (ptr == &base_root_dev) fprintf(fil, "DEVTREE_CONST struct device %s = {\n", ptr->name); else @@ -993,8 +950,14 @@ chip_id = 1; instance = chip->instance; while (instance) { - instance->id = chip_id++; - emit_chip_instance(fil, instance); + /* + * Emit this chip instance only if there is no forwarding pointer to the + * base tree chip instance. + */ + if (instance->base_chip_instance == NULL) { + instance->id = chip_id++; + emit_chip_instance(fil, instance); + } instance = instance->next; } } @@ -1082,29 +1045,6 @@ }
/* - * Walk through the override subtree in breadth-first manner starting at node to - * see if chip_instance pointer of the node is same as chip_instance pointer of - * override parent that is passed into the function. If yes, then update the - * chip_instance pointer of the node to chip_instance pointer of the base - * parent. - */ -static void update_chip_pointers(struct device *node, - struct chip_instance *base_parent_ci, - struct chip_instance *override_parent_ci) -{ - struct queue_entry *q_head = NULL; - - enqueue_tail(&q_head, node); - - while ((node = dequeue_head(&q_head))) { - if (node->chip_instance != override_parent_ci) - continue; - node->chip_instance = base_parent_ci; - add_children_to_queue(&q_head, node); - } -} - -/* * Add resource to device. If resource is already present, then update its base * and index. If not, then add a new resource to the device. */ @@ -1287,6 +1227,12 @@ }
/* + * Update base_chip_instance member in chip instance of override tree to forward it to + * the chip instance in base tree. + */ + override_dev->chip_instance->base_chip_instance = base_dev->chip_instance; + + /* * Now that the device properties are all copied over, look at each bus * of the override device and run override_devicetree in a recursive * manner. The assumption here is that first bus of override device @@ -1312,9 +1258,6 @@ override_bus = override_bus->next_bus; base_bus = base_bus->next_bus; } - - delete_chip_instance(override_dev->chip_instance); - override_dev->chip_instance = NULL; }
/* @@ -1359,14 +1302,6 @@ * as a new child of base_parent. */ set_new_child(base_parent, override_child); - /* - * Ensure all nodes in override tree pointing to - * override parent chip_instance now point to base - * parent chip_instance. - */ - update_chip_pointers(override_child, - base_parent->dev->chip_instance, - override_parent->dev->chip_instance); }
override_child = next_child; diff --git a/util/sconfig/sconfig.h b/util/sconfig/sconfig.h index a76506d..2603904 100644 --- a/util/sconfig/sconfig.h +++ b/util/sconfig/sconfig.h @@ -56,10 +56,16 @@ struct chip_instance *next;
/* - * Reference count - Indicates how many devices hold pointer to this - * chip instance. + * Pointer to corresponding chip instance in base devicetree. + * a) If the chip instance belongs to the base devicetree, then this pointer is set to + * NULL. + * b) If the chip instance belongs to override tree, then this pointer is set to its + * corresponding chip instance in base devicetree (if it exists), else to NULL. + * + * This is useful when generating chip instances and chip_ops for a device to determine + * if this is the instance to emit or if there is a base chip instance to use instead. */ - int ref_count; + struct chip_instance *base_chip_instance; };
struct chip {