From 68ff6d365539fd0bb13a219bb4c5bb885ee1f30f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 13 Mar 2022 16:22:48 -0600 Subject: bloblist: Describe the design goals Add a comment explaining the design goals of bloblist, to make it easier for people to understand and comment on the structure. Signed-off-by: Simon Glass --- include/bloblist.h | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/bloblist.h b/include/bloblist.h index d0e128acf10..9684bfd5f4b 100644 --- a/include/bloblist.h +++ b/include/bloblist.h @@ -3,8 +3,66 @@ * This provides a standard way of passing information between boot phases * (TPL -> SPL -> U-Boot proper.) * - * A list of blobs of data, tagged with their owner. The list resides in memory - * and can be updated by SPL, U-Boot, etc. + * It consists of a list of blobs of data, tagged with their owner / contents. + * The list resides in memory and can be updated by SPL, U-Boot, etc. + * + * Design goals for bloblist: + * + * 1. Small and efficient structure. This avoids UUIDs or 16-byte name fields, + * since a 32-bit tag provides enough space for all the tags we will even need. + * If UUIDs are desired, they can be added inside a particular blob. + * + * 2. Avoids use of pointers, so the structure can be relocated in memory. The + * data in each blob is inline, rather than using pointers. + * + * 3. Bloblist is designed to start small in TPL or SPL, when only a few things + * are needed, like the memory size or whether console output should be enabled. + * Then it can grow in U-Boot proper, e.g. to include space for ACPI tables. + * + * 4. The bloblist structure is simple enough that it can be implemented in a + * small amount of C code. The API does not require use of strings or UUIDs, + * which would add to code size. For Thumb-2 the code size needed in SPL is + * approximately 940 bytes (e.g. for chromebook_bob). + * + * 5. Bloblist uses 16-byte alignment internally and is designed to start on a + * 16-byte boundary. Its headers are multiples of 16 bytes. This makes it easier + * to deal with data structures which need this level of alignment, such as ACPI + * tables. For use in SPL and TPL the alignment can be relaxed, since it can be + * relocated to an aligned address in U-Boot proper. + * + * 6. Bloblist is designed to be passed to Linux as reserved memory. While linux + * doesn't understand the bloblist header, it can be passed the indivdual blobs. + * For example, ACPI tables can reside in a blob and the address of those is + * passed to Linux, without Linux ever being away of the existence of a + * bloblist. Having all the blobs contiguous in memory simplifies the + * reserved-memory space. + * + * 7. Bloblist tags are defined in the enum below. There is an area for + * project-specific stuff (e.g. U-Boot, TF-A) and vendor-specific stuff, e.g. + * something used only on a particular SoC. There is also a private area for + * temporary, local use. + * + * 8. Bloblist includes a simple checksum, so that each boot phase can update + * this and allow the next phase to check that all is well. While the bloblist + * is small, this is quite cheap to calculate. When it grows (e.g. in U-Boot\ + * proper), the CPU is likely running faster, so it is not prohibitive. Having + * said that, U-Boot is often the last phase that uses bloblist, so calculating + * the checksum there may not be necessary. + * + * 9. It would be possible to extend bloblist to support a non-contiguous + * structure, e.g. by creating a blob type that points to the next bloblist. + * This does not seem necessary for now. It adds complexity and code. We can + * always just copy it. + * + * 10. Bloblist is designed for simple structures, those that can be defined by + * a single C struct. More complex structures should be passed in a device tree. + * There are some exceptions, chiefly the various binary structures that Intel + * is fond of creating. But device tree provides a dictionary-type format which + * is fairly efficient (for use in U-Boot proper and Linux at least), along with + * a schema and a good set of tools. New formats should be designed around + * device tree rather than creating new binary formats, unless they are needed + * early in boot (where libfdt's 3KB of overhead is too large) and are trival + * enough to be described by a C struct. * * Copyright 2018 Google, Inc * Written by Simon Glass -- cgit v1.3.1 From 8b52f237f0f8ae63c930fa8459c39ab87367bad1 Mon Sep 17 00:00:00 2001 From: Sean Anderson Date: Mon, 28 Mar 2022 18:14:37 -0400 Subject: dm: core: Provide fallbacks for ofnode_conf_read_... Because fdt_get_config_str et al. were moved/renamed to ofnode_conf_read_str, they now depend on CONFIG_DM as well as CONFIG_OF_CONTROL. Add some fallback implementations, preventing a linker error when CONFIG_SPL_OF_CONTROL and CONFIG_SPL_ENV_IS_IN_MMC are enabled and CONFIG_SPL_DM is disabled. Fixes: 7de8bd03c3 ("treewide: fdt: Move fdt_get_config_... to ofnode_conf_read...") Signed-off-by: Sean Anderson --- include/dm/ofnode.h | 66 ++++++++++++++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 24 deletions(-) (limited to 'include') diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h index 2c4d72d77f5..bb60433124b 100644 --- a/include/dm/ofnode.h +++ b/include/dm/ofnode.h @@ -1181,6 +1181,33 @@ int ofnode_write_string(ofnode node, const char *propname, const char *value); */ int ofnode_set_enabled(ofnode node, bool value); +/** + * ofnode_get_phy_node() - Get PHY node for a MAC (if not fixed-link) + * + * This function parses PHY handle from the Ethernet controller's ofnode + * (trying all possible PHY handle property names), and returns the PHY ofnode. + * + * Before this is used, ofnode_phy_is_fixed_link() should be checked first, and + * if the result to that is true, this function should not be called. + * + * @eth_node: ofnode belonging to the Ethernet controller + * Return: ofnode of the PHY, if it exists, otherwise an invalid ofnode + */ +ofnode ofnode_get_phy_node(ofnode eth_node); + +/** + * ofnode_read_phy_mode() - Read PHY connection type from a MAC node + * + * This function parses the "phy-mode" / "phy-connection-type" property and + * returns the corresponding PHY interface type. + * + * @mac_node: ofnode containing the property + * Return: one of PHY_INTERFACE_MODE_* constants, PHY_INTERFACE_MODE_NA on + * error + */ +phy_interface_t ofnode_read_phy_mode(ofnode mac_node); + +#if CONFIG_IS_ENABLED(DM) /** * ofnode_conf_read_bool() - Read a boolean value from the U-Boot config * @@ -1218,30 +1245,21 @@ int ofnode_conf_read_int(const char *prop_name, int default_val); */ const char *ofnode_conf_read_str(const char *prop_name); -/** - * ofnode_get_phy_node() - Get PHY node for a MAC (if not fixed-link) - * - * This function parses PHY handle from the Ethernet controller's ofnode - * (trying all possible PHY handle property names), and returns the PHY ofnode. - * - * Before this is used, ofnode_phy_is_fixed_link() should be checked first, and - * if the result to that is true, this function should not be called. - * - * @eth_node: ofnode belonging to the Ethernet controller - * Return: ofnode of the PHY, if it exists, otherwise an invalid ofnode - */ -ofnode ofnode_get_phy_node(ofnode eth_node); +#else /* CONFIG_DM */ +static inline bool ofnode_conf_read_bool(const char *prop_name) +{ + return false; +} -/** - * ofnode_read_phy_mode() - Read PHY connection type from a MAC node - * - * This function parses the "phy-mode" / "phy-connection-type" property and - * returns the corresponding PHY interface type. - * - * @mac_node: ofnode containing the property - * Return: one of PHY_INTERFACE_MODE_* constants, PHY_INTERFACE_MODE_NA on - * error - */ -phy_interface_t ofnode_read_phy_mode(ofnode mac_node); +static inline int ofnode_conf_read_int(const char *prop_name, int default_val) +{ + return default_val; +} + +static inline const char *ofnode_conf_read_str(const char *prop_name) +{ + return NULL; +} +#endif /* CONFIG_DM */ #endif -- cgit v1.3.1 From 7750ee45a62c7834f170f817232e432b8d2a14d3 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Mon, 4 Apr 2022 22:45:03 +0200 Subject: sandbox: add function os_printf() Before setting up the devices U-Boot's printf() function cannot be used for console output. Provide function os_printf() to print to stderr. Signed-off-by: Heinrich Schuchardt --- arch/sandbox/cpu/os.c | 13 +++++++++++++ include/os.h | 7 +++++++ 2 files changed, 20 insertions(+) (limited to 'include') diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 3b230606a97..f937991139c 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -54,6 +55,18 @@ ssize_t os_write(int fd, const void *buf, size_t count) return write(fd, buf, count); } +int os_printf(const char *fmt, ...) +{ + va_list args; + int i; + + va_start(args, fmt); + i = vfprintf(stdout, fmt, args); + va_end(args); + + return i; +} + off_t os_lseek(int fd, off_t offset, int whence) { if (whence == OS_SEEK_SET) diff --git a/include/os.h b/include/os.h index 10e198cf503..148178787bc 100644 --- a/include/os.h +++ b/include/os.h @@ -16,6 +16,13 @@ struct rtc_time; struct sandbox_state; +/** + * os_printf() - print directly to OS console + * + * @format: format string + */ +int os_printf(const char *format, ...); + /** * Access to the OS read() system call * -- cgit v1.3.1 From 1452870404804210db1d797ec046e24a99c101bf Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 8 May 2022 04:39:19 -0600 Subject: dm: core: Rename dm_dump_all() This is not a good name anymore as it does not dump everything. Rename it to dm_dump_tree() to avoid confusion. Signed-off-by: Simon Glass --- cmd/dm.c | 8 ++++---- drivers/core/dump.c | 2 +- include/dm/util.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) (limited to 'include') diff --git a/cmd/dm.c b/cmd/dm.c index ca609224f55..03674cd0864 100644 --- a/cmd/dm.c +++ b/cmd/dm.c @@ -10,10 +10,10 @@ #include #include -static int do_dm_dump_all(struct cmd_tbl *cmdtp, int flag, int argc, - char *const argv[]) +static int do_dm_dump_tree(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) { - dm_dump_all(); + dm_dump_tree(); return 0; } @@ -70,7 +70,7 @@ static char dm_help_text[] = #endif U_BOOT_CMD_WITH_SUBCMDS(dm, "Driver model low level access", dm_help_text, - U_BOOT_SUBCMD_MKENT(tree, 1, 1, do_dm_dump_all), + U_BOOT_SUBCMD_MKENT(tree, 1, 1, do_dm_dump_tree), U_BOOT_SUBCMD_MKENT(uclass, 1, 1, do_dm_dump_uclass), U_BOOT_SUBCMD_MKENT(devres, 1, 1, do_dm_dump_devres), U_BOOT_SUBCMD_MKENT(drivers, 1, 1, do_dm_dump_drivers), diff --git a/drivers/core/dump.c b/drivers/core/dump.c index 21d9e7a91f7..994e308f057 100644 --- a/drivers/core/dump.c +++ b/drivers/core/dump.c @@ -45,7 +45,7 @@ static void show_devices(struct udevice *dev, int depth, int last_flag) } } -void dm_dump_all(void) +void dm_dump_tree(void) { struct udevice *root; diff --git a/include/dm/util.h b/include/dm/util.h index 4428f045b72..c52daa87ef3 100644 --- a/include/dm/util.h +++ b/include/dm/util.h @@ -25,7 +25,7 @@ struct list_head; int list_count_items(struct list_head *head); /* Dump out a tree of all devices */ -void dm_dump_all(void); +void dm_dump_tree(void); /* Dump out a list of uclasses and their devices */ void dm_dump_uclass(void); -- cgit v1.3.1 From 53c20bebb2215caaadc58b2eee2c80c61456b93d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 8 May 2022 04:39:23 -0600 Subject: dm: core: Switch the testbus driver to use a new struct At present this driver uses 'priv' struct to hold 'plat' data, which is confusing. The contents of the strct don't matter, since only dtoc is using it. Create a new struct with the correct name. Signed-off-by: Simon Glass --- drivers/misc/test_drv.c | 2 +- include/dm/test.h | 7 +++++++ tools/dtoc/test_dtoc.py | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/drivers/misc/test_drv.c b/drivers/misc/test_drv.c index 5d72982f258..b6df1189032 100644 --- a/drivers/misc/test_drv.c +++ b/drivers/misc/test_drv.c @@ -109,7 +109,7 @@ UCLASS_DRIVER(testbus) = { .child_post_probe = testbus_child_post_probe_uclass, /* This is for dtoc testing only */ - .per_device_plat_auto = sizeof(struct dm_test_uclass_priv), + .per_device_plat_auto = sizeof(struct dm_test_uclass_plat), }; static int testfdt_drv_ping(struct udevice *dev, int pingval, int *pingret) diff --git a/include/dm/test.h b/include/dm/test.h index 4919064cc02..b5937509212 100644 --- a/include/dm/test.h +++ b/include/dm/test.h @@ -92,6 +92,13 @@ struct dm_test_uclass_priv { int total_add; }; +/** + * struct dm_test_uclass_plat - private plat data for test uclass + */ +struct dm_test_uclass_plat { + char dummy[32]; +}; + /** * struct dm_test_parent_data - parent's information on each child * diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index c81bcc9c32f..8bac2076214 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -616,7 +616,7 @@ struct dm_test_pdata __attribute__ ((section (".priv_data"))) u8 _denx_u_boot_test_bus_priv_some_bus[sizeof(struct dm_test_priv)] \t__attribute__ ((section (".priv_data"))); #include -u8 _denx_u_boot_test_bus_ucplat_some_bus[sizeof(struct dm_test_uclass_priv)] +u8 _denx_u_boot_test_bus_ucplat_some_bus[sizeof(struct dm_test_uclass_plat)] \t__attribute__ ((section (".priv_data"))); #include -- cgit v1.3.1 From 930a3ddadebf3660cc3163081671de189300afdd Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 8 May 2022 04:39:24 -0600 Subject: dm: core: Support accessing core tags At present tag numbers are only allocated for non-core data, meaning that the 'core' data, like priv and plat, are accessed through dedicated functions. For debugging and consistency it is convenient to use tags for this 'core' data too. Add support for this, with new tag numbers and functions to access the pointer and size for each. Update one of the test drivers so that the uclass-private data can be tested here. There is some code duplication with functions like device_alloc_priv() but this is not addressed for now. At some point, some rationalisation may help to reduce code size, but more thought it needed on that. Signed-off-by: Simon Glass --- drivers/core/device.c | 65 ++++++++++++++++++++++++++++++++++++++++ drivers/misc/test_drv.c | 4 ++- include/dm/device.h | 25 ++++++++++++++++ include/dm/tag.h | 13 +++++++- test/dm/core.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++ tools/dtoc/test_dtoc.py | 4 +++ 6 files changed, 189 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/drivers/core/device.c b/drivers/core/device.c index 7f71570a940..d9ce546c0c4 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -674,6 +674,71 @@ void *dev_get_parent_priv(const struct udevice *dev) return dm_priv_to_rw(dev->parent_priv_); } +void *dev_get_attach_ptr(const struct udevice *dev, enum dm_tag_t tag) +{ + switch (tag) { + case DM_TAG_PLAT: + return dev_get_plat(dev); + case DM_TAG_PARENT_PLAT: + return dev_get_parent_plat(dev); + case DM_TAG_UC_PLAT: + return dev_get_uclass_plat(dev); + case DM_TAG_PRIV: + return dev_get_priv(dev); + case DM_TAG_PARENT_PRIV: + return dev_get_parent_priv(dev); + case DM_TAG_UC_PRIV: + return dev_get_uclass_priv(dev); + default: + return NULL; + } +} + +int dev_get_attach_size(const struct udevice *dev, enum dm_tag_t tag) +{ + const struct udevice *parent = dev_get_parent(dev); + const struct uclass *uc = dev->uclass; + const struct uclass_driver *uc_drv = uc->uc_drv; + const struct driver *parent_drv = NULL; + int size = 0; + + if (parent) + parent_drv = parent->driver; + + switch (tag) { + case DM_TAG_PLAT: + size = dev->driver->plat_auto; + break; + case DM_TAG_PARENT_PLAT: + if (parent) { + size = parent_drv->per_child_plat_auto; + if (!size) + size = parent->uclass->uc_drv->per_child_plat_auto; + } + break; + case DM_TAG_UC_PLAT: + size = uc_drv->per_device_plat_auto; + break; + case DM_TAG_PRIV: + size = dev->driver->priv_auto; + break; + case DM_TAG_PARENT_PRIV: + if (parent) { + size = parent_drv->per_child_auto; + if (!size) + size = parent->uclass->uc_drv->per_child_auto; + } + break; + case DM_TAG_UC_PRIV: + size = uc_drv->per_device_auto; + break; + default: + break; + } + + return size; +} + static int device_get_device_tail(struct udevice *dev, int ret, struct udevice **devp) { diff --git a/drivers/misc/test_drv.c b/drivers/misc/test_drv.c index b6df1189032..927618256f0 100644 --- a/drivers/misc/test_drv.c +++ b/drivers/misc/test_drv.c @@ -108,7 +108,9 @@ UCLASS_DRIVER(testbus) = { .child_pre_probe = testbus_child_pre_probe_uclass, .child_post_probe = testbus_child_post_probe_uclass, - /* This is for dtoc testing only */ + .per_device_auto = sizeof(struct dm_test_uclass_priv), + + /* Note: this is for dtoc testing as well as tags*/ .per_device_plat_auto = sizeof(struct dm_test_uclass_plat), }; diff --git a/include/dm/device.h b/include/dm/device.h index 5bdb10653f8..12c6ba37ff3 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -11,6 +11,7 @@ #define _DM_DEVICE_H #include +#include #include #include #include @@ -546,6 +547,30 @@ void *dev_get_parent_priv(const struct udevice *dev); */ void *dev_get_uclass_priv(const struct udevice *dev); +/** + * dev_get_attach_ptr() - Get the value of an attached pointed tag + * + * The tag is assumed to hold a pointer, if it exists + * + * @dev: Device to look at + * @tag: Tag to access + * @return value of tag, or NULL if there is no tag of this type + */ +void *dev_get_attach_ptr(const struct udevice *dev, enum dm_tag_t tag); + +/** + * dev_get_attach_size() - Get the size of an attached tag + * + * Core tags have an automatic-allocation mechanism where the allocated size is + * defined by the device, parent or uclass. This returns the size associated + * with a particular tag + * + * @dev: Device to look at + * @tag: Tag to access + * @return size of auto-allocated data, 0 if none + */ +int dev_get_attach_size(const struct udevice *dev, enum dm_tag_t tag); + /** * dev_get_parent() - Get the parent of a device * diff --git a/include/dm/tag.h b/include/dm/tag.h index 54fc31eb153..9cb5d68f0a3 100644 --- a/include/dm/tag.h +++ b/include/dm/tag.h @@ -13,8 +13,19 @@ struct udevice; enum dm_tag_t { + /* Types of core tags that can be attached to devices */ + DM_TAG_PLAT, + DM_TAG_PARENT_PLAT, + DM_TAG_UC_PLAT, + + DM_TAG_PRIV, + DM_TAG_PARENT_PRIV, + DM_TAG_UC_PRIV, + DM_TAG_DRIVER_DATA, + DM_TAG_ATTACH_COUNT, + /* EFI_LOADER */ - DM_TAG_EFI = 0, + DM_TAG_EFI = DM_TAG_ATTACH_COUNT, DM_TAG_COUNT, }; diff --git a/test/dm/core.c b/test/dm/core.c index ebd504427d1..26e2fd56619 100644 --- a/test/dm/core.c +++ b/test/dm/core.c @@ -1275,3 +1275,83 @@ static int dm_test_uclass_find_device(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_uclass_find_device, UT_TESTF_SCAN_FDT); + +/* Test getting information about tags attached to devices */ +static int dm_test_dev_get_attach(struct unit_test_state *uts) +{ + struct udevice *dev; + + ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev)); + ut_asserteq_str("a-test", dev->name); + + ut_assertnonnull(dev_get_attach_ptr(dev, DM_TAG_PLAT)); + ut_assertnonnull(dev_get_attach_ptr(dev, DM_TAG_PRIV)); + ut_assertnull(dev_get_attach_ptr(dev, DM_TAG_UC_PRIV)); + ut_assertnull(dev_get_attach_ptr(dev, DM_TAG_UC_PLAT)); + ut_assertnull(dev_get_attach_ptr(dev, DM_TAG_PARENT_PLAT)); + ut_assertnull(dev_get_attach_ptr(dev, DM_TAG_PARENT_PRIV)); + + ut_asserteq(sizeof(struct dm_test_pdata), + dev_get_attach_size(dev, DM_TAG_PLAT)); + ut_asserteq(sizeof(struct dm_test_priv), + dev_get_attach_size(dev, DM_TAG_PRIV)); + ut_asserteq(0, dev_get_attach_size(dev, DM_TAG_UC_PRIV)); + ut_asserteq(0, dev_get_attach_size(dev, DM_TAG_UC_PLAT)); + ut_asserteq(0, dev_get_attach_size(dev, DM_TAG_PARENT_PLAT)); + ut_asserteq(0, dev_get_attach_size(dev, DM_TAG_PARENT_PRIV)); + + return 0; +} +DM_TEST(dm_test_dev_get_attach, UT_TESTF_SCAN_FDT); + +/* Test getting information about tags attached to bus devices */ +static int dm_test_dev_get_attach_bus(struct unit_test_state *uts) +{ + struct udevice *dev, *child; + + ut_assertok(uclass_first_device_err(UCLASS_TEST_BUS, &dev)); + ut_asserteq_str("some-bus", dev->name); + + ut_assertnonnull(dev_get_attach_ptr(dev, DM_TAG_PLAT)); + ut_assertnonnull(dev_get_attach_ptr(dev, DM_TAG_PRIV)); + ut_assertnonnull(dev_get_attach_ptr(dev, DM_TAG_UC_PRIV)); + ut_assertnonnull(dev_get_attach_ptr(dev, DM_TAG_UC_PLAT)); + ut_assertnull(dev_get_attach_ptr(dev, DM_TAG_PARENT_PLAT)); + ut_assertnull(dev_get_attach_ptr(dev, DM_TAG_PARENT_PRIV)); + + ut_asserteq(sizeof(struct dm_test_pdata), + dev_get_attach_size(dev, DM_TAG_PLAT)); + ut_asserteq(sizeof(struct dm_test_priv), + dev_get_attach_size(dev, DM_TAG_PRIV)); + ut_asserteq(sizeof(struct dm_test_uclass_priv), + dev_get_attach_size(dev, DM_TAG_UC_PRIV)); + ut_asserteq(sizeof(struct dm_test_uclass_plat), + dev_get_attach_size(dev, DM_TAG_UC_PLAT)); + ut_asserteq(0, dev_get_attach_size(dev, DM_TAG_PARENT_PLAT)); + ut_asserteq(0, dev_get_attach_size(dev, DM_TAG_PARENT_PRIV)); + + /* Now try the child of the bus */ + ut_assertok(device_first_child_err(dev, &child)); + ut_asserteq_str("c-test@5", child->name); + + ut_assertnonnull(dev_get_attach_ptr(child, DM_TAG_PLAT)); + ut_assertnonnull(dev_get_attach_ptr(child, DM_TAG_PRIV)); + ut_assertnull(dev_get_attach_ptr(child, DM_TAG_UC_PRIV)); + ut_assertnull(dev_get_attach_ptr(child, DM_TAG_UC_PLAT)); + ut_assertnonnull(dev_get_attach_ptr(child, DM_TAG_PARENT_PLAT)); + ut_assertnonnull(dev_get_attach_ptr(child, DM_TAG_PARENT_PRIV)); + + ut_asserteq(sizeof(struct dm_test_pdata), + dev_get_attach_size(child, DM_TAG_PLAT)); + ut_asserteq(sizeof(struct dm_test_priv), + dev_get_attach_size(child, DM_TAG_PRIV)); + ut_asserteq(0, dev_get_attach_size(child, DM_TAG_UC_PRIV)); + ut_asserteq(0, dev_get_attach_size(child, DM_TAG_UC_PLAT)); + ut_asserteq(sizeof(struct dm_test_parent_plat), + dev_get_attach_size(child, DM_TAG_PARENT_PLAT)); + ut_asserteq(sizeof(struct dm_test_parent_data), + dev_get_attach_size(child, DM_TAG_PARENT_PRIV)); + + return 0; +} +DM_TEST(dm_test_dev_get_attach_bus, UT_TESTF_SCAN_FDT); diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index 8bac2076214..879ca2ab2bf 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -618,6 +618,9 @@ u8 _denx_u_boot_test_bus_priv_some_bus[sizeof(struct dm_test_priv)] #include u8 _denx_u_boot_test_bus_ucplat_some_bus[sizeof(struct dm_test_uclass_plat)] \t__attribute__ ((section (".priv_data"))); +#include +u8 _denx_u_boot_test_bus_uc_priv_some_bus[sizeof(struct dm_test_uclass_priv)] + __attribute__ ((section (".priv_data"))); #include DM_DEVICE_INST(some_bus) = { @@ -628,6 +631,7 @@ DM_DEVICE_INST(some_bus) = { \t.driver_data\t= DM_TEST_TYPE_FIRST, \t.priv_\t\t= _denx_u_boot_test_bus_priv_some_bus, \t.uclass\t\t= DM_UCLASS_REF(testbus), +\t.uclass_priv_ = _denx_u_boot_test_bus_uc_priv_some_bus, \t.uclass_node\t= { \t\t.prev = &DM_UCLASS_REF(testbus)->dev_head, \t\t.next = &DM_UCLASS_REF(testbus)->dev_head, -- cgit v1.3.1 From 0dfda34ca594c701955cfcb71711a7599f97bae3 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 8 May 2022 04:39:25 -0600 Subject: dm: core: Add a way to collect memory usage Add a function for collecting the amount of memory used by driver model, including devices, uclasses and attached data and tags. This information can provide insights into how to reduce the memory required by driver model. Future work may look at execution speed also. Signed-off-by: Simon Glass --- drivers/core/root.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/core/tag.c | 11 +++++++++++ include/dm/root.h | 45 +++++++++++++++++++++++++++++++++++++++++++++ include/dm/tag.h | 11 +++++++++++ test/dm/core.c | 11 +++++++++++ 5 files changed, 131 insertions(+) (limited to 'include') diff --git a/drivers/core/root.c b/drivers/core/root.c index 17dd1205a32..f24ddfa5218 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -449,6 +449,59 @@ void dm_get_stats(int *device_countp, int *uclass_countp) *uclass_countp = uclass_get_count(); } +void dev_collect_stats(struct dm_stats *stats, const struct udevice *parent) +{ + const struct udevice *dev; + int i; + + stats->dev_count++; + stats->dev_size += sizeof(struct udevice); + stats->dev_name_size += strlen(parent->name) + 1; + for (i = 0; i < DM_TAG_ATTACH_COUNT; i++) { + int size = dev_get_attach_size(parent, i); + + if (size || + (i == DM_TAG_DRIVER_DATA && parent->driver_data)) { + stats->attach_count[i]++; + stats->attach_size[i] += size; + stats->attach_count_total++; + stats->attach_size_total += size; + } + } + + list_for_each_entry(dev, &parent->child_head, sibling_node) + dev_collect_stats(stats, dev); +} + +void uclass_collect_stats(struct dm_stats *stats) +{ + struct uclass *uc; + + list_for_each_entry(uc, gd->uclass_root, sibling_node) { + int size; + + stats->uc_count++; + stats->uc_size += sizeof(struct uclass); + size = uc->uc_drv->priv_auto; + if (size) { + stats->uc_attach_count++; + stats->uc_attach_size += size; + } + } +} + +void dm_get_mem(struct dm_stats *stats) +{ + memset(stats, '\0', sizeof(*stats)); + dev_collect_stats(stats, gd->dm_root); + uclass_collect_stats(stats); + dev_tag_collect_stats(stats); + + stats->total_size = stats->dev_size + stats->uc_size + + stats->attach_size_total + stats->uc_attach_size + + stats->tag_size; +} + #ifdef CONFIG_ACPIGEN static int root_acpi_get_name(const struct udevice *dev, char *out_name) { diff --git a/drivers/core/tag.c b/drivers/core/tag.c index 22999193a5a..2961725b658 100644 --- a/drivers/core/tag.c +++ b/drivers/core/tag.c @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -137,3 +138,13 @@ int dev_tag_del_all(struct udevice *dev) return -ENOENT; } + +void dev_tag_collect_stats(struct dm_stats *stats) +{ + struct dmtag_node *node; + + list_for_each_entry(node, &gd->dmtag_list, sibling) { + stats->tag_count++; + stats->tag_size += sizeof(struct dmtag_node); + } +} diff --git a/include/dm/root.h b/include/dm/root.h index e888fb993c0..382f83c7f5b 100644 --- a/include/dm/root.h +++ b/include/dm/root.h @@ -9,11 +9,49 @@ #ifndef _DM_ROOT_H_ #define _DM_ROOT_H_ +#include + struct udevice; /* Head of the uclass list if CONFIG_OF_PLATDATA_INST is enabled */ extern struct list_head uclass_head; +/** + * struct dm_stats - Information about driver model memory usage + * + * @total_size: All data + * @dev_count: Number of devices + * @dev_size: Size of all devices (just the struct udevice) + * @dev_name_size: Bytes used by device names + * @uc_count: Number of uclasses + * @uc_size: Size of all uclasses (just the struct uclass) + * @tag_count: Number of tags + * @tag_size: Bytes used by all tags + * @uc_attach_count: Number of uclasses with attached data (priv) + * @uc_attach_size: Total size of that attached data + * @attach_count_total: Total number of attached data items for all udevices and + * uclasses + * @attach_size_total: Total number of bytes of attached data + * @attach_count: Number of devices with attached, for each type + * @attach_size: Total number of bytes of attached data, for each type + */ +struct dm_stats { + int total_size; + int dev_count; + int dev_size; + int dev_name_size; + int uc_count; + int uc_size; + int tag_count; + int tag_size; + int uc_attach_count; + int uc_attach_size; + int attach_count_total; + int attach_size_total; + int attach_count[DM_TAG_ATTACH_COUNT]; + int attach_size[DM_TAG_ATTACH_COUNT]; +}; + /** * dm_root() - Return pointer to the top of the driver tree * @@ -141,4 +179,11 @@ static inline int dm_remove_devices_flags(uint flags) { return 0; } */ void dm_get_stats(int *device_countp, int *uclass_countp); +/** + * dm_get_mem() - Get stats on memory usage in driver model + * + * @mem: Place to put the information + */ +void dm_get_mem(struct dm_stats *stats); + #endif diff --git a/include/dm/tag.h b/include/dm/tag.h index 9cb5d68f0a3..1ea3c9f7af3 100644 --- a/include/dm/tag.h +++ b/include/dm/tag.h @@ -10,6 +10,7 @@ #include #include +struct dm_stats; struct udevice; enum dm_tag_t { @@ -118,4 +119,14 @@ int dev_tag_del(struct udevice *dev, enum dm_tag_t tag); */ int dev_tag_del_all(struct udevice *dev); +/** + * dev_tag_collect_stats() - Collect information on driver model performance + * + * This collects information on how driver model is performing. For now it only + * includes memory usage + * + * @stats: Place to put the collected information + */ +void dev_tag_collect_stats(struct dm_stats *stats); + #endif /* _DM_TAG_H */ diff --git a/test/dm/core.c b/test/dm/core.c index 26e2fd56619..fd4d7569728 100644 --- a/test/dm/core.c +++ b/test/dm/core.c @@ -1355,3 +1355,14 @@ static int dm_test_dev_get_attach_bus(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_dev_get_attach_bus, UT_TESTF_SCAN_FDT); + +/* Test getting information about tags attached to bus devices */ +static int dm_test_dev_get_mem(struct unit_test_state *uts) +{ + struct dm_stats stats; + + dm_get_mem(&stats); + + return 0; +} +DM_TEST(dm_test_dev_get_mem, UT_TESTF_SCAN_FDT); -- cgit v1.3.1 From 2cb4ddb91ec9fcb77c895e4a1192a15aece700c6 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 8 May 2022 04:39:26 -0600 Subject: dm: core: Add a command to show driver model statistics This command shows the memory used by driver model along with various hints as to what it might be if some 'core' tags were moved to use the tag list instead of a core (i.e. always-there) pointer. This may help with future work to reduce memory usage. Signed-off-by: Simon Glass --- cmd/dm.c | 24 +++++++++++++++++ drivers/core/Kconfig | 11 ++++++++ drivers/core/dump.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/core/tag.c | 18 +++++++++++++ include/dm/root.h | 2 +- include/dm/tag.h | 8 ++++++ include/dm/util.h | 9 +++++++ 7 files changed, 144 insertions(+), 1 deletion(-) (limited to 'include') diff --git a/cmd/dm.c b/cmd/dm.c index 0c7554a1b1d..eb40f0865fe 100644 --- a/cmd/dm.c +++ b/cmd/dm.c @@ -8,6 +8,7 @@ #include #include +#include #include static int do_dm_dump_driver_compat(struct cmd_tbl *cmdtp, int flag, int argc, @@ -34,6 +35,19 @@ static int do_dm_dump_drivers(struct cmd_tbl *cmdtp, int flag, int argc, return 0; } +#if CONFIG_IS_ENABLED(DM_STATS) +static int do_dm_dump_mem(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + struct dm_stats mem; + + dm_get_mem(&mem); + dm_dump_mem(&mem); + + return 0; +} +#endif /* DM_STATS */ + static int do_dm_dump_static_driver_info(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) { @@ -58,11 +72,20 @@ static int do_dm_dump_uclass(struct cmd_tbl *cmdtp, int flag, int argc, return 0; } +#if CONFIG_IS_ENABLED(DM_STATS) +#define DM_MEM_HELP "dm mem Provide a summary of memory usage\n" +#define DM_MEM U_BOOT_SUBCMD_MKENT(mem, 1, 1, do_dm_dump_mem), +#else +#define DM_MEM_HELP +#define DM_MEM +#endif + #if CONFIG_IS_ENABLED(SYS_LONGHELP) static char dm_help_text[] = "compat Dump list of drivers with compatibility strings\n" "dm devres Dump list of device resources for each device\n" "dm drivers Dump list of drivers with uclass and instances\n" + DM_MEM_HELP "dm static Dump list of drivers with static platform data\n" "dn tree Dump tree of driver model devices ('*' = activated)\n" "dm uclass Dump list of instances for each uclass" @@ -73,6 +96,7 @@ U_BOOT_CMD_WITH_SUBCMDS(dm, "Driver model low level access", dm_help_text, U_BOOT_SUBCMD_MKENT(compat, 1, 1, do_dm_dump_driver_compat), U_BOOT_SUBCMD_MKENT(devres, 1, 1, do_dm_dump_devres), U_BOOT_SUBCMD_MKENT(drivers, 1, 1, do_dm_dump_drivers), + DM_MEM U_BOOT_SUBCMD_MKENT(static, 1, 1, do_dm_dump_static_driver_info), U_BOOT_SUBCMD_MKENT(tree, 1, 1, do_dm_dump_tree), U_BOOT_SUBCMD_MKENT(uclass, 1, 1, do_dm_dump_uclass)); diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig index 9b9a7148a1a..97dc699e969 100644 --- a/drivers/core/Kconfig +++ b/drivers/core/Kconfig @@ -75,6 +75,17 @@ config DM_DEBUG help Say Y here if you want to compile in debug messages in DM core. +config DM_STATS + bool "Collect and show driver model stats" + depends on DM + default y if SANDBOX + help + Enable this to collect and display memory statistics about driver + model. This can help to figure out where all the memory is going and + to find optimisations. + + To display the memory stats, use the 'dm mem' command. + config DM_DEVICE_REMOVE bool "Support device removal" depends on DM diff --git a/drivers/core/dump.c b/drivers/core/dump.c index e434fe04728..1c1f7e4d308 100644 --- a/drivers/core/dump.c +++ b/drivers/core/dump.c @@ -172,3 +172,76 @@ void dm_dump_static_driver_info(void) for (entry = drv; entry != drv + n_ents; entry++) printf("%-25.25s %p\n", entry->name, entry->plat); } + +void dm_dump_mem(struct dm_stats *stats) +{ + int total, total_delta; + int i; + + /* Support SPL printf() */ + printf("Struct sizes: udevice %x, driver %x, uclass %x, uc_driver %x\n", + (int)sizeof(struct udevice), (int)sizeof(struct driver), + (int)sizeof(struct uclass), (int)sizeof(struct uclass_driver)); + printf("Memory: device %x:%x, device names %x, uclass %x:%x\n", + stats->dev_count, stats->dev_size, stats->dev_name_size, + stats->uc_count, stats->uc_size); + printf("\n"); + printf("%-15s %5s %5s %5s %5s %5s\n", "Attached type", "Count", + "Size", "Cur", "Tags", "Save"); + printf("%-15s %5s %5s %5s %5s %5s\n", "---------------", "-----", + "-----", "-----", "-----", "-----"); + total_delta = 0; + for (i = 0; i < DM_TAG_ATTACH_COUNT; i++) { + int cur_size, new_size, delta; + + cur_size = stats->dev_count * sizeof(struct udevice); + new_size = stats->dev_count * (sizeof(struct udevice) - + sizeof(void *)); + /* + * Let's assume we can fit each dmtag_node into 32 bits. We can + * limit the 'tiny tags' feature to SPL with + * CONFIG_SPL_SYS_MALLOC_F_LEN <= 64KB, so needing 14 bits to + * point to anything in that region (with 4-byte alignment). + * So: + * 4 bits for tag + * 14 bits for offset of dev + * 14 bits for offset of data + */ + new_size += stats->attach_count[i] * sizeof(u32); + delta = cur_size - new_size; + total_delta += delta; + printf("%-16s %5x %6x %6x %6x %6x (%d)\n", tag_get_name(i), + stats->attach_count[i], stats->attach_size[i], + cur_size, new_size, delta > 0 ? delta : 0, delta); + } + printf("%-16s %5x %6x\n", "uclass", stats->uc_attach_count, + stats->uc_attach_size); + printf("%-16s %5x %6x %5s %5s %6x (%d)\n", "Attached total", + stats->attach_count_total + stats->uc_attach_count, + stats->attach_size_total + stats->uc_attach_size, "", "", + total_delta > 0 ? total_delta : 0, total_delta); + printf("%-16s %5x %6x\n", "tags", stats->tag_count, stats->tag_size); + printf("\n"); + printf("Total size: %x (%d)\n", stats->total_size, stats->total_size); + printf("\n"); + + total = stats->total_size; + total -= total_delta; + printf("With tags: %x (%d)\n", total, total); + + /* Use singly linked lists in struct udevice (3 nodes in each) */ + total -= sizeof(void *) * 3 * stats->dev_count; + printf("- singly-linked: %x (%d)\n", total, total); + + /* Use an index into the struct_driver list instead of a pointer */ + total = total + stats->dev_count * (1 - sizeof(void *)); + printf("- driver index: %x (%d)\n", total, total); + + /* Same with the uclass */ + total = total + stats->dev_count * (1 - sizeof(void *)); + printf("- uclass index: %x (%d)\n", total, total); + + /* Drop the device name */ + printf("Drop device name (not SRAM): %x (%d)\n", stats->dev_name_size, + stats->dev_name_size); +} diff --git a/drivers/core/tag.c b/drivers/core/tag.c index 2961725b658..a3c5cb7e57c 100644 --- a/drivers/core/tag.c +++ b/drivers/core/tag.c @@ -16,6 +16,24 @@ struct udevice; DECLARE_GLOBAL_DATA_PTR; +static const char *const tag_name[] = { + [DM_TAG_PLAT] = "plat", + [DM_TAG_PARENT_PLAT] = "parent_plat", + [DM_TAG_UC_PLAT] = "uclass_plat", + + [DM_TAG_PRIV] = "priv", + [DM_TAG_PARENT_PRIV] = "parent_priv", + [DM_TAG_UC_PRIV] = "uclass_priv", + [DM_TAG_DRIVER_DATA] = "driver_data", + + [DM_TAG_EFI] = "efi", +}; + +const char *tag_get_name(enum dm_tag_t tag) +{ + return tag_name[tag]; +} + int dev_tag_set_ptr(struct udevice *dev, enum dm_tag_t tag, void *ptr) { struct dmtag_node *node; diff --git a/include/dm/root.h b/include/dm/root.h index 382f83c7f5b..b2f30a842f5 100644 --- a/include/dm/root.h +++ b/include/dm/root.h @@ -182,7 +182,7 @@ void dm_get_stats(int *device_countp, int *uclass_countp); /** * dm_get_mem() - Get stats on memory usage in driver model * - * @mem: Place to put the information + * @stats: Place to put the information */ void dm_get_mem(struct dm_stats *stats); diff --git a/include/dm/tag.h b/include/dm/tag.h index 1ea3c9f7af3..745088ffcff 100644 --- a/include/dm/tag.h +++ b/include/dm/tag.h @@ -129,4 +129,12 @@ int dev_tag_del_all(struct udevice *dev); */ void dev_tag_collect_stats(struct dm_stats *stats); +/** + * tag_get_name() - Get the name of a tag + * + * @tag: Tag to look up, which must be valid + * Returns: Name of tag + */ +const char *tag_get_name(enum dm_tag_t tag); + #endif /* _DM_TAG_H */ diff --git a/include/dm/util.h b/include/dm/util.h index c52daa87ef3..e10c6060ce0 100644 --- a/include/dm/util.h +++ b/include/dm/util.h @@ -6,6 +6,8 @@ #ifndef __DM_UTIL_H #define __DM_UTIL_H +struct dm_stats; + #if CONFIG_IS_ENABLED(DM_WARN) #define dm_warn(fmt...) log(LOGC_DM, LOGL_WARNING, ##fmt) #else @@ -48,6 +50,13 @@ void dm_dump_driver_compat(void); /* Dump out a list of drivers with static platform data */ void dm_dump_static_driver_info(void); +/** + * dm_dump_mem() - Dump stats on memory usage in driver model + * + * @mem: Stats to dump + */ +void dm_dump_mem(struct dm_stats *stats); + #if CONFIG_IS_ENABLED(OF_PLATDATA_INST) && CONFIG_IS_ENABLED(READ_ONLY) void *dm_priv_to_rw(void *priv); #else -- cgit v1.3.1 From 5204823c81c2be4ef3abfcaae351401f8dd5f058 Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sat, 18 Jun 2022 15:13:08 +0300 Subject: spl: binman: Declare extern symbols for VPL as well The binman extern symbol declarations in spl.h are missing the VPL symbols recently added to spl.c, add them like the others. Signed-off-by: Alper Nebi Yasak --- include/spl.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'include') diff --git a/include/spl.h b/include/spl.h index 83ac583e0b4..1778e0f5368 100644 --- a/include/spl.h +++ b/include/spl.h @@ -288,6 +288,8 @@ binman_sym_extern(ulong, u_boot_any, image_pos); binman_sym_extern(ulong, u_boot_any, size); binman_sym_extern(ulong, u_boot_spl, image_pos); binman_sym_extern(ulong, u_boot_spl, size); +binman_sym_extern(ulong, u_boot_vpl, image_pos); +binman_sym_extern(ulong, u_boot_vpl, size); /** * spl_get_image_pos() - get the image position of the next phase -- cgit v1.3.1 From d8830cf84035120562bb490be276fab9e43d6414 Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sat, 18 Jun 2022 15:13:09 +0300 Subject: spl: binman: Split binman symbols support from enabling binman Enabling CONFIG_BINMAN makes binman run after a build to package any images specified in the device-tree. It also enables a mechanism for SPL/TPL to declare and use special linker symbols that refer to other entries in the same binman image. A similar feature that gets this info from the device-tree exists for U-Boot proper, but it is gated behind a CONFIG_BINMAN_FDT unlike the symbols. Confusingly, CONFIG_SPL/TPL_BINMAN_SYMBOLS also exist. These configs don't actually enable/disable the symbols mechanism as one would expect, but declare some symbols for U-Boot using this mechanism. Reuse the BINMAN_SYMBOLS configs to make them toggle the symbols mechanism, and declare symbols for the U-Boot phases in a dependent BINMAN_UBOOT_SYMBOLS config. Extend it to cover symbols of all phases. Update the config prompt and help message to make it clearer about this. Fix binman test binaries to work with CONFIG_IS_ENABLED(BINMAN_SYMBOLS). Co-developed-by: Peng Fan [Alper: New config for phase symbols, update Kconfigs, commit message] Signed-off-by: Alper Nebi Yasak --- common/spl/Kconfig | 22 +++++++++++++++++----- common/spl/Kconfig.tpl | 24 ++++++++++++++++++------ common/spl/spl.c | 9 +++++---- include/binman_sym.h | 6 +++--- tools/binman/test/Makefile | 2 +- tools/binman/test/generated/autoconf.h | 3 +++ tools/binman/test/u_boot_binman_syms.c | 2 +- tools/binman/test/u_boot_binman_syms_size.c | 2 +- 8 files changed, 49 insertions(+), 21 deletions(-) create mode 100644 tools/binman/test/generated/autoconf.h (limited to 'include') diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 2ad2351c6eb..46d9be73bb1 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -190,12 +190,24 @@ config SPL_BINMAN_SYMBOLS depends on SPL_FRAMEWORK && BINMAN default y help - This enables use of symbols in SPL which refer to U-Boot, enabling SPL - to obtain the location of U-Boot simply by calling spl_get_image_pos() - and spl_get_image_size(). + This enables use of symbols in SPL which refer to other entries in + the same binman image as the SPL. These can be declared with the + binman_sym_declare(type, entry, prop) macro and accessed by the + binman_sym(type, entry, prop) macro defined in binman_sym.h. - For this to work, you must have a U-Boot image in the binman image, so - binman can update SPL with the location of it. + See tools/binman/binman.rst for a detailed explanation. + +config SPL_BINMAN_UBOOT_SYMBOLS + bool "Declare binman symbols for U-Boot phases in SPL" + depends on SPL_BINMAN_SYMBOLS + default y + help + This enables use of symbols in SPL which refer to U-Boot phases, + enabling SPL to obtain the location and size of its next phase simply + by calling spl_get_image_pos() and spl_get_image_size(). + + For this to work, you must have all U-Boot phases in the same binman + image, so binman can update SPL with the locations of everything. source "common/spl/Kconfig.nxp" diff --git a/common/spl/Kconfig.tpl b/common/spl/Kconfig.tpl index 834cb6b6dd8..8c59c767302 100644 --- a/common/spl/Kconfig.tpl +++ b/common/spl/Kconfig.tpl @@ -9,16 +9,28 @@ config TPL_SIZE_LIMIT If this value is zero, it is ignored. config TPL_BINMAN_SYMBOLS - bool "Declare binman symbols in TPL" + bool "Support binman symbols in TPL" depends on TPL_FRAMEWORK && BINMAN default y help - This enables use of symbols in TPL which refer to U-Boot, enabling TPL - to obtain the location of U-Boot simply by calling spl_get_image_pos() - and spl_get_image_size(). + This enables use of symbols in TPL which refer to other entries in + the same binman image as the TPL. These can be declared with the + binman_sym_declare(type, entry, prop) macro and accessed by the + binman_sym(type, entry, prop) macro defined in binman_sym.h. - For this to work, you must have a U-Boot image in the binman image, so - binman can update TPL with the location of it. + See tools/binman/binman.rst for a detailed explanation. + +config TPL_BINMAN_UBOOT_SYMBOLS + bool "Declare binman symbols for U-Boot phases in TPL" + depends on TPL_BINMAN_SYMBOLS + default y + help + This enables use of symbols in TPL which refer to U-Boot phases, + enabling TPL to obtain the location and size of its next phase simply + by calling spl_get_image_pos() and spl_get_image_size(). + + For this to work, you must have all U-Boot phases in the same binman + image, so binman can update TPL with the locations of everything. config TPL_FRAMEWORK bool "Support TPL based upon the common SPL framework" diff --git a/common/spl/spl.c b/common/spl/spl.c index 75051656249..5dd122611c2 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -52,11 +52,10 @@ DECLARE_GLOBAL_DATA_PTR; u32 *boot_params_ptr = NULL; -#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) +#if CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS) /* See spl.h for information about this */ binman_sym_declare(ulong, u_boot_any, image_pos); binman_sym_declare(ulong, u_boot_any, size); -#endif #ifdef CONFIG_TPL binman_sym_declare(ulong, u_boot_spl, image_pos); @@ -68,6 +67,8 @@ binman_sym_declare(ulong, u_boot_vpl, image_pos); binman_sym_declare(ulong, u_boot_vpl, size); #endif +#endif /* BINMAN_UBOOT_SYMBOLS */ + /* Define board data structure */ static struct bd_info bdata __attribute__ ((section(".data"))); @@ -152,7 +153,7 @@ void spl_fixup_fdt(void *fdt_blob) ulong spl_get_image_pos(void) { - if (!CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) + if (!CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS)) return BINMAN_SYM_MISSING; #ifdef CONFIG_VPL @@ -166,7 +167,7 @@ ulong spl_get_image_pos(void) ulong spl_get_image_size(void) { - if (!CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) + if (!CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS)) return BINMAN_SYM_MISSING; #ifdef CONFIG_VPL diff --git a/include/binman_sym.h b/include/binman_sym.h index 72e6765fe52..8586ef8731b 100644 --- a/include/binman_sym.h +++ b/include/binman_sym.h @@ -13,7 +13,7 @@ #define BINMAN_SYM_MISSING (-1UL) -#ifdef CONFIG_BINMAN +#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) /** * binman_symname() - Internal function to get a binman symbol name @@ -77,7 +77,7 @@ #define binman_sym(_type, _entry_name, _prop_name) \ (*(_type *)&binman_symname(_entry_name, _prop_name)) -#else /* !BINMAN */ +#else /* !CONFIG_IS_ENABLED(BINMAN_SYMBOLS) */ #define binman_sym_declare(_type, _entry_name, _prop_name) @@ -87,6 +87,6 @@ #define binman_sym(_type, _entry_name, _prop_name) BINMAN_SYM_MISSING -#endif /* BINMAN */ +#endif /* CONFIG_IS_ENABLED(BINMAN_SYMBOLS) */ #endif diff --git a/tools/binman/test/Makefile b/tools/binman/test/Makefile index 57057e2d588..bea8567c9b4 100644 --- a/tools/binman/test/Makefile +++ b/tools/binman/test/Makefile @@ -21,7 +21,7 @@ CC = $(CROSS_COMPILE)gcc OBJCOPY = $(CROSS_COMPILE)objcopy VPATH := $(SRC) -CFLAGS := -march=i386 -m32 -nostdlib -I $(SRC)../../../include \ +CFLAGS := -march=i386 -m32 -nostdlib -I $(SRC)../../../include -I $(SRC) \ -Wl,--no-dynamic-linker LDS_UCODE := -T $(SRC)u_boot_ucode_ptr.lds diff --git a/tools/binman/test/generated/autoconf.h b/tools/binman/test/generated/autoconf.h new file mode 100644 index 00000000000..6a23039f469 --- /dev/null +++ b/tools/binman/test/generated/autoconf.h @@ -0,0 +1,3 @@ +#define CONFIG_BINMAN 1 +#define CONFIG_SPL_BUILD 1 +#define CONFIG_SPL_BINMAN_SYMBOLS 1 diff --git a/tools/binman/test/u_boot_binman_syms.c b/tools/binman/test/u_boot_binman_syms.c index 37fc339ce84..89fee5567e1 100644 --- a/tools/binman/test/u_boot_binman_syms.c +++ b/tools/binman/test/u_boot_binman_syms.c @@ -5,7 +5,7 @@ * Simple program to create some binman symbols. This is used by binman tests. */ -#define CONFIG_BINMAN +#include #include binman_sym_declare(unsigned long, u_boot_spl_any, offset); diff --git a/tools/binman/test/u_boot_binman_syms_size.c b/tools/binman/test/u_boot_binman_syms_size.c index 7224bc1863c..c4a053f96f1 100644 --- a/tools/binman/test/u_boot_binman_syms_size.c +++ b/tools/binman/test/u_boot_binman_syms_size.c @@ -5,7 +5,7 @@ * Simple program to create some binman symbols. This is used by binman tests. */ -#define CONFIG_BINMAN +#include #include binman_sym_declare(char, u_boot_spl, pos); -- cgit v1.3.1 From 367ecbf2d3b1c16a3b98b9f6430b8197d2bddbf9 Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sat, 18 Jun 2022 15:13:11 +0300 Subject: spl: binman: Check at runtime if binman symbols were filled in Binman lets us declare symbols in SPL/TPL that refer to other entries in the same binman image as them. These symbols are filled in with the correct values while binman assembles the images, but this is done in-memory only. Symbols marked as optional can be filled with BINMAN_SYM_MISSING as an error value if their referred entry is missing. However, the unmodified SPL/TPL binaries are still available on disk, and can be used by people. For these files, nothing ensures that the symbols are set to this error value, and they will be considered valid when they are not. Empirically, all symbols show up as zero in a sandbox_vpl build when we run e.g. tpl/u-boot-tpl directly. On the other hand, zero is a perfectly fine value for a binman-written symbol, so we cannot say the symbols have wrong values based on that. Declare a magic symbol that binman always fills in with a fixed value. Check this value as an indicator that symbols were filled in correctly. Return the error value for all symbols when this magic symbol has the wrong value. For binman tests, we need to make room for the new symbol in the mocked SPL/TPL data by extending them by four bytes. This messes up some test image layouts. Fix the affected values, and check the magic symbol wherever it makes sense. Signed-off-by: Alper Nebi Yasak --- common/spl/spl.c | 1 + include/binman_sym.h | 45 +++++++++++++++++++++++++++-- tools/binman/elf.py | 12 ++++++-- tools/binman/elf_test.py | 12 ++++---- tools/binman/ftest.py | 33 +++++++++++---------- tools/binman/test/021_image_pad.dts | 2 +- tools/binman/test/024_sorted.dts | 2 +- tools/binman/test/028_pack_4gb_outside.dts | 2 +- tools/binman/test/029_x86_rom.dts | 6 ++-- tools/binman/test/053_symbols.dts | 2 +- tools/binman/test/149_symbols_tpl.dts | 4 +-- tools/binman/test/155_symbols_tpl_x86.dts | 4 +-- tools/binman/test/187_symbols_sub.dts | 2 +- tools/binman/test/u_boot_binman_syms.c | 4 +++ tools/binman/test/u_boot_binman_syms_size.c | 4 +++ 15 files changed, 97 insertions(+), 38 deletions(-) (limited to 'include') diff --git a/common/spl/spl.c b/common/spl/spl.c index 5dd122611c2..29e0898f03d 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -41,6 +41,7 @@ #include DECLARE_GLOBAL_DATA_PTR; +DECLARE_BINMAN_MAGIC_SYM; #ifndef CONFIG_SYS_UBOOT_START #define CONFIG_SYS_UBOOT_START CONFIG_SYS_TEXT_BASE diff --git a/include/binman_sym.h b/include/binman_sym.h index 8586ef8731b..528d7e4e90e 100644 --- a/include/binman_sym.h +++ b/include/binman_sym.h @@ -11,6 +11,8 @@ #ifndef __BINMAN_SYM_H #define __BINMAN_SYM_H +/* BSYM in little endian, keep in sync with tools/binman/elf.py */ +#define BINMAN_SYM_MAGIC_VALUE (0x4d595342UL) #define BINMAN_SYM_MISSING (-1UL) #if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) @@ -62,6 +64,37 @@ __attribute__((aligned(4), weak, unused, \ section(".binman_sym"))) +/** + * _binman_sym_magic - Internal magic symbol for validity checks + * + * When building images, binman fills in this symbol with the magic + * value #defined above. This is used to check at runtime if the + * symbol values were filled in and are OK to use. + */ +extern ulong _binman_sym_magic; + +/** + * DECLARE_BINMAN_MAGIC_SYM - Declare the internal magic symbol + * + * This macro declares the _binman_sym_magic symbol so that it exists. + * Declaring it here would cause errors during linking due to multiple + * definitions of the symbol. + */ +#define DECLARE_BINMAN_MAGIC_SYM \ + ulong _binman_sym_magic \ + __attribute__((aligned(4), section(".binman_sym"))) + +/** + * BINMAN_SYMS_OK - Check if the symbol values are valid + * + * This macro checks if the magic symbol's value is filled properly, + * which indicates that other symbols are OK to use as well. + * + * Return: 1 if binman symbol values are usable, 0 if not + */ +#define BINMAN_SYMS_OK \ + (*(ulong *)&_binman_sym_magic == BINMAN_SYM_MAGIC_VALUE) + /** * binman_sym() - Access a previously declared symbol * @@ -72,10 +105,14 @@ * @_type: Type f the symbol (e.g. unsigned long) * @entry_name: Name of the entry to look for (e.g. 'u_boot_spl') * @_prop_name: Property value to get from that entry (e.g. 'pos') - * @returns value of that property (filled in by binman) + * + * Return: value of that property (filled in by binman), or + * BINMAN_SYM_MISSING if the value is unavailable */ #define binman_sym(_type, _entry_name, _prop_name) \ - (*(_type *)&binman_symname(_entry_name, _prop_name)) + (BINMAN_SYMS_OK ? \ + (*(_type *)&binman_symname(_entry_name, _prop_name)) : \ + BINMAN_SYM_MISSING) #else /* !CONFIG_IS_ENABLED(BINMAN_SYMBOLS) */ @@ -85,6 +122,10 @@ #define binman_sym_extern(_type, _entry_name, _prop_name) +#define DECLARE_BINMAN_MAGIC_SYM + +#define BINMAN_SYMS_OK (0) + #define binman_sym(_type, _entry_name, _prop_name) BINMAN_SYM_MISSING #endif /* CONFIG_IS_ENABLED(BINMAN_SYMBOLS) */ diff --git a/tools/binman/elf.py b/tools/binman/elf.py index afa05e58fdd..6d440ddf21d 100644 --- a/tools/binman/elf.py +++ b/tools/binman/elf.py @@ -25,6 +25,9 @@ try: except: # pragma: no cover ELF_TOOLS = False +# BSYM in little endian, keep in sync with include/binman_sym.h +BINMAN_SYM_MAGIC_VALUE = 0x4d595342 + # Information about an EFL symbol: # section (str): Name of the section containing this symbol # address (int): Address of the symbol (its value) @@ -223,9 +226,12 @@ def LookupAndWriteSymbols(elf_fname, entry, section): raise ValueError('%s has size %d: only 4 and 8 are supported' % (msg, sym.size)) - # Look up the symbol in our entry tables. - value = section.GetImage().LookupImageSymbol(name, sym.weak, msg, - base.address) + if name == '_binman_sym_magic': + value = BINMAN_SYM_MAGIC_VALUE + else: + # Look up the symbol in our entry tables. + value = section.GetImage().LookupImageSymbol(name, sym.weak, + msg, base.address) if value is None: value = -1 pack_string = pack_string.lower() diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py index 02bc1083749..5a51c64cfee 100644 --- a/tools/binman/elf_test.py +++ b/tools/binman/elf_test.py @@ -127,7 +127,7 @@ class TestElf(unittest.TestCase): elf_fname = self.ElfTestFile('u_boot_binman_syms') with self.assertRaises(ValueError) as e: elf.LookupAndWriteSymbols(elf_fname, entry, section) - self.assertIn('entry_path has offset 4 (size 8) but the contents size ' + self.assertIn('entry_path has offset 8 (size 8) but the contents size ' 'is a', str(e.exception)) def testMissingImageStart(self): @@ -161,18 +161,20 @@ class TestElf(unittest.TestCase): This should produce -1 values for all thress symbols, taking up the first 16 bytes of the image. """ - entry = FakeEntry(24) + entry = FakeEntry(28) section = FakeSection(sym_value=None) elf_fname = self.ElfTestFile('u_boot_binman_syms') elf.LookupAndWriteSymbols(elf_fname, entry, section) - self.assertEqual(tools.get_bytes(255, 20) + tools.get_bytes(ord('a'), 4), - entry.data) + expected = (struct.pack('; + offset = <28>; }; }; }; diff --git a/tools/binman/test/024_sorted.dts b/tools/binman/test/024_sorted.dts index b79d9adf682..b54f9b14191 100644 --- a/tools/binman/test/024_sorted.dts +++ b/tools/binman/test/024_sorted.dts @@ -7,7 +7,7 @@ binman { sort-by-offset; u-boot { - offset = <26>; + offset = <30>; }; u-boot-spl { diff --git a/tools/binman/test/028_pack_4gb_outside.dts b/tools/binman/test/028_pack_4gb_outside.dts index 11a1f6059e2..b6ad7fb56a5 100644 --- a/tools/binman/test/028_pack_4gb_outside.dts +++ b/tools/binman/test/028_pack_4gb_outside.dts @@ -13,7 +13,7 @@ }; u-boot-spl { - offset = <0xffffffe7>; + offset = <0xffffffe3>; }; }; }; diff --git a/tools/binman/test/029_x86_rom.dts b/tools/binman/test/029_x86_rom.dts index 88aa007bbae..ad8f9d6e1bd 100644 --- a/tools/binman/test/029_x86_rom.dts +++ b/tools/binman/test/029_x86_rom.dts @@ -7,13 +7,13 @@ binman { sort-by-offset; end-at-4gb; - size = <32>; + size = <36>; u-boot { - offset = <0xffffffe0>; + offset = <0xffffffdc>; }; u-boot-spl { - offset = <0xffffffe7>; + offset = <0xffffffe3>; }; }; }; diff --git a/tools/binman/test/053_symbols.dts b/tools/binman/test/053_symbols.dts index 29658092764..b28f34a72fa 100644 --- a/tools/binman/test/053_symbols.dts +++ b/tools/binman/test/053_symbols.dts @@ -10,7 +10,7 @@ }; u-boot { - offset = <0x18>; + offset = <0x1c>; }; u-boot-spl2 { diff --git a/tools/binman/test/149_symbols_tpl.dts b/tools/binman/test/149_symbols_tpl.dts index 0a4ab3f1fab..4e649c45978 100644 --- a/tools/binman/test/149_symbols_tpl.dts +++ b/tools/binman/test/149_symbols_tpl.dts @@ -11,12 +11,12 @@ }; u-boot-spl2 { - offset = <0x1c>; + offset = <0x20>; type = "u-boot-spl"; }; u-boot { - offset = <0x34>; + offset = <0x3c>; }; section { diff --git a/tools/binman/test/155_symbols_tpl_x86.dts b/tools/binman/test/155_symbols_tpl_x86.dts index 9d7dc51b3d9..e1ce33e67fb 100644 --- a/tools/binman/test/155_symbols_tpl_x86.dts +++ b/tools/binman/test/155_symbols_tpl_x86.dts @@ -14,12 +14,12 @@ }; u-boot-spl2 { - offset = <0xffffff1c>; + offset = <0xffffff20>; type = "u-boot-spl"; }; u-boot { - offset = <0xffffff34>; + offset = <0xffffff3c>; }; section { diff --git a/tools/binman/test/187_symbols_sub.dts b/tools/binman/test/187_symbols_sub.dts index 54511a73711..3ab62d37215 100644 --- a/tools/binman/test/187_symbols_sub.dts +++ b/tools/binman/test/187_symbols_sub.dts @@ -11,7 +11,7 @@ }; u-boot { - offset = <24>; + offset = <28>; }; }; diff --git a/tools/binman/test/u_boot_binman_syms.c b/tools/binman/test/u_boot_binman_syms.c index 89fee5567e1..ed761246aec 100644 --- a/tools/binman/test/u_boot_binman_syms.c +++ b/tools/binman/test/u_boot_binman_syms.c @@ -5,9 +5,13 @@ * Simple program to create some binman symbols. This is used by binman tests. */ +typedef unsigned long ulong; + #include #include +DECLARE_BINMAN_MAGIC_SYM; + binman_sym_declare(unsigned long, u_boot_spl_any, offset); binman_sym_declare(unsigned long long, u_boot_spl2, offset); binman_sym_declare(unsigned long, u_boot_any, image_pos); diff --git a/tools/binman/test/u_boot_binman_syms_size.c b/tools/binman/test/u_boot_binman_syms_size.c index c4a053f96f1..fa41b3d9a33 100644 --- a/tools/binman/test/u_boot_binman_syms_size.c +++ b/tools/binman/test/u_boot_binman_syms_size.c @@ -5,7 +5,11 @@ * Simple program to create some binman symbols. This is used by binman tests. */ +typedef unsigned long ulong; + #include #include +DECLARE_BINMAN_MAGIC_SYM; + binman_sym_declare(char, u_boot_spl, pos); -- cgit v1.3.1