From f4f4123708cd3699aa3981afca9511beb760c7fb Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 27 Sep 2020 18:46:19 -0600 Subject: [PATCH 01/71] binman: Add a way to read the ROM offset Provide a function to read the ROM offset so that we can store the value in one place and clients don't need to store it themselves after calling binman_set_rom_offset(). Signed-off-by: Simon Glass --- include/binman.h | 7 +++++++ lib/binman.c | 7 ++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/include/binman.h b/include/binman.h index e0b92075e2..8b89a9666d 100644 --- a/include/binman.h +++ b/include/binman.h @@ -42,6 +42,13 @@ int binman_entry_map(ofnode parent, const char *name, void **bufp, int *sizep); */ void binman_set_rom_offset(int rom_offset); +/** + * binman_get_rom_offset() - Get the ROM memory-map offset + * + * @returns offset from an image_pos to the memory-mapped address + */ +int binman_get_rom_offset(void); + /** * binman_entry_find() - Find a binman symbol * diff --git a/lib/binman.c b/lib/binman.c index 7a8ad62c4a..79e497fc8d 100644 --- a/lib/binman.c +++ b/lib/binman.c @@ -43,7 +43,7 @@ static int binman_entry_find_internal(ofnode node, const char *name, ret = ofnode_read_u32(node, "image-pos", &entry->image_pos); if (ret) - return log_msg_ret("import-pos", ret); + return log_msg_ret("image-pos", ret); ret = ofnode_read_u32(node, "size", &entry->size); if (ret) return log_msg_ret("size", ret); @@ -83,6 +83,11 @@ void binman_set_rom_offset(int rom_offset) binman->rom_offset = rom_offset; } +int binman_get_rom_offset(void) +{ + return binman->rom_offset; +} + int binman_init(void) { binman = malloc(sizeof(struct binman_info)); From 83187546ae14e266c745c3bb24db492eb5433b89 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 27 Sep 2020 18:46:20 -0600 Subject: [PATCH 02/71] binman: Support multiple images in the library Add support for multiple images, since these are used on x86 now. Select the first image for now, since that is generally the correct one. At some point we can add a way to determine which image is currently running. Signed-off-by: Simon Glass --- lib/binman.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/binman.c b/lib/binman.c index 79e497fc8d..e71c1b9e99 100644 --- a/lib/binman.c +++ b/lib/binman.c @@ -96,6 +96,13 @@ int binman_init(void) binman->image = ofnode_path("/binman"); if (!ofnode_valid(binman->image)) return log_msg_ret("binman node", -EINVAL); + if (ofnode_read_bool(binman->image, "multiple-images")) { + ofnode node = ofnode_first_subnode(binman->image); + + if (!ofnode_valid(node)) + return log_msg_ret("first image", -ENOENT); + binman->image = node; + } binman->rom_offset = ROM_OFFSET_NONE; return 0; From 26e408fe121016b8cfacddc03a23093bf867e7a2 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:18 -0600 Subject: [PATCH 03/71] dtoc: Extract inner loop from output_node() This function is very long. Put the inner loop in a separate function to enhance readability. Signed-off-by: Simon Glass --- tools/dtoc/dtb_platdata.py | 89 +++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 40 deletions(-) diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py index 579a6749c4..bd1daa4379 100644 --- a/tools/dtoc/dtb_platdata.py +++ b/tools/dtoc/dtb_platdata.py @@ -560,6 +560,54 @@ class DtbPlatdata(object): Args: node: node to output """ + def _output_list(node, prop): + """Output the C code for a devicetree property that holds a list + + Args: + node (fdt.Node): Node to output + prop (fdt.Prop): Prop to output + """ + self.buf('{') + vals = [] + # For phandles, output a reference to the platform data + # of the target node. + info = self.get_phandle_argc(prop, node.name) + if info: + # Process the list as pairs of (phandle, id) + pos = 0 + item = 0 + for args in info.args: + phandle_cell = prop.value[pos] + phandle = fdt_util.fdt32_to_cpu(phandle_cell) + target_node = self._fdt.phandle_to_node[phandle] + name = conv_name_to_c(target_node.name) + arg_values = [] + for i in range(args): + arg_values.append(str(fdt_util.fdt32_to_cpu(prop.value[pos + 1 + i]))) + pos += 1 + args + # node member is filled with NULL as the real value + # will be update at run-time during dm_init_and_scan() + # by dm_populate_phandle_data() + vals.append('\t{NULL, {%s}}' % (', '.join(arg_values))) + var_node = '%s%s.%s[%d].node' % \ + (VAL_PREFIX, var_name, member_name, item) + # Save the the link information to be use to define + # dm_populate_phandle_data() + self._links.append({'var_node': var_node, 'dev_name': name}) + item += 1 + for val in vals: + self.buf('\n\t\t%s,' % val) + else: + for val in prop.value: + vals.append(get_value(prop.type, val)) + + # Put 8 values per line to avoid very long lines. + for i in range(0, len(vals), 8): + if i: + self.buf(',\n\t\t') + self.buf(', '.join(vals[i:i + 8])) + self.buf('}') + struct_name, _ = self.get_normalized_compat_name(node) var_name = conv_name_to_c(node.name) self.buf('static struct %s%s %s%s = {\n' % @@ -573,46 +621,7 @@ class DtbPlatdata(object): # Special handling for lists if isinstance(prop.value, list): - self.buf('{') - vals = [] - # For phandles, output a reference to the platform data - # of the target node. - info = self.get_phandle_argc(prop, node.name) - if info: - # Process the list as pairs of (phandle, id) - pos = 0 - item = 0 - for args in info.args: - phandle_cell = prop.value[pos] - phandle = fdt_util.fdt32_to_cpu(phandle_cell) - target_node = self._fdt.phandle_to_node[phandle] - name = conv_name_to_c(target_node.name) - arg_values = [] - for i in range(args): - arg_values.append(str(fdt_util.fdt32_to_cpu(prop.value[pos + 1 + i]))) - pos += 1 + args - # node member is filled with NULL as the real value - # will be update at run-time during dm_init_and_scan() - # by dm_populate_phandle_data() - vals.append('\t{NULL, {%s}}' % (', '.join(arg_values))) - var_node = '%s%s.%s[%d].node' % \ - (VAL_PREFIX, var_name, member_name, item) - # Save the the link information to be use to define - # dm_populate_phandle_data() - self._links.append({'var_node': var_node, 'dev_name': name}) - item += 1 - for val in vals: - self.buf('\n\t\t%s,' % val) - else: - for val in prop.value: - vals.append(get_value(prop.type, val)) - - # Put 8 values per line to avoid very long lines. - for i in range(0, len(vals), 8): - if i: - self.buf(',\n\t\t') - self.buf(', '.join(vals[i:i + 8])) - self.buf('}') + _output_list(node, prop) else: self.buf(get_value(prop.type, prop.value)) self.buf(',\n') From 97136eb5354c28c475cd2fb72d5cc1029ce9e743 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:19 -0600 Subject: [PATCH 04/71] dtoc: Use a namedtuple for _links The use of strings to access a dict is a bit ugly. Use a namedtuple for this instead. Signed-off-by: Simon Glass --- tools/dtoc/dtb_platdata.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py index bd1daa4379..1b52198a94 100644 --- a/tools/dtoc/dtb_platdata.py +++ b/tools/dtoc/dtb_platdata.py @@ -54,6 +54,13 @@ VAL_PREFIX = 'dtv_' # phandles is len(args). This is a list of integers. PhandleInfo = collections.namedtuple('PhandleInfo', ['max_args', 'args']) +# Holds a single phandle link, allowing a C struct value to be assigned to point +# to a device +# +# var_node: C variable to assign (e.g. 'dtv_mmc.clocks[0].node') +# dev_name: Name of device to assign to (e.g. 'clock') +PhandleLink = collections.namedtuple('PhandleLink', ['var_node', 'dev_name']) + def conv_name_to_c(name): """Convert a device-tree name to a C identifier @@ -146,7 +153,8 @@ class DtbPlatdata(object): key: Driver alias declared with U_BOOT_DRIVER_ALIAS(driver_alias, driver_name) value: Driver name declared with U_BOOT_DRIVER(driver_name) - _links: List of links to be included in dm_populate_phandle_data() + _links: List of links to be included in dm_populate_phandle_data(), + each a PhandleLink _drivers_additional: List of additional drivers to use during scanning """ def __init__(self, dtb_fname, include_disabled, warning_disabled, @@ -593,7 +601,7 @@ class DtbPlatdata(object): (VAL_PREFIX, var_name, member_name, item) # Save the the link information to be use to define # dm_populate_phandle_data() - self._links.append({'var_node': var_node, 'dev_name': name}) + self._links.append(PhandleLink(var_node, name)) item += 1 for val in vals: self.buf('\n\t\t%s,' % val) @@ -669,9 +677,9 @@ class DtbPlatdata(object): # nodes using DM_GET_DEVICE # dtv_dmc_at_xxx.clocks[0].node = DM_GET_DEVICE(clock_controller_at_xxx) self.buf('void dm_populate_phandle_data(void) {\n') - for l in self._links: + for link in self._links: self.buf('\t%s = DM_GET_DEVICE(%s);\n' % - (l['var_node'], l['dev_name'])) + (link.var_node, link.dev_name)) self.buf('}\n') self.out(''.join(self.get_buf())) From 3c14083f20679c526f3a133b2f71f0aad996fdc8 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:20 -0600 Subject: [PATCH 05/71] dm: core: Expand the comment for DM_GET_DEVICE() The current documentation for this is not particularly enlightening. Add a little more detail. Signed-off-by: Simon Glass --- include/dm/platdata.h | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/include/dm/platdata.h b/include/dm/platdata.h index cab93b071b..25479b03d2 100644 --- a/include/dm/platdata.h +++ b/include/dm/platdata.h @@ -45,7 +45,22 @@ struct driver_info { #define U_BOOT_DEVICES(__name) \ ll_entry_declare_list(struct driver_info, __name, driver_info) -/* Get a pointer to a given driver */ +/** + * Get a pointer to a given device info given its name + * + * With the declaration U_BOOT_DEVICE(name), DM_GET_DEVICE(name) will return a + * pointer to the struct driver_info created by that declaration. + * + * if OF_PLATDATA is enabled, from this it is possible to use the @dev member of + * struct driver_info to find the device pointer itself. + * + * TODO(sjg@chromium.org): U_BOOT_DEVICE() tells U-Boot to create a device, so + * the naming seems sensible, but DM_GET_DEVICE() is a bit of misnomer, since it + * finds the driver_info record, not the device. + * + * @__name: Driver name (C identifier, not a string. E.g. gpio7_at_ff7e0000) + * @return struct driver_info * to the driver that created the device + */ #define DM_GET_DEVICE(__name) \ ll_entry_get(struct driver_info, __name, driver_info) From 08c3b88dd145d3f7f06e7ad8458905bde7a286ef Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:21 -0600 Subject: [PATCH 06/71] dm: core: Avoid void * in the of-platdata structs These pointers point to drivers. Update the definition to make this clear. Signed-off-by: Simon Glass --- include/dt-structs.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/include/dt-structs.h b/include/dt-structs.h index 924d51fc52..eed8273d18 100644 --- a/include/dt-structs.h +++ b/include/dt-structs.h @@ -8,18 +8,20 @@ /* These structures may only be used in SPL */ #if CONFIG_IS_ENABLED(OF_PLATDATA) +struct driver_info; + struct phandle_0_arg { - const void *node; + const struct driver_info *node; int arg[0]; }; struct phandle_1_arg { - const void *node; + const struct driver_info *node; int arg[1]; }; struct phandle_2_arg { - const void *node; + const struct driver_info *node; int arg[2]; }; #include From a652d9c73a6eea1fdfb901c66178a4d804fac95d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:22 -0600 Subject: [PATCH 07/71] dm: Avoid using #ifdef for CONFIG_OF_LIVE At present this option results in a number of #ifdefs due to the presence or absence of the global_data of_root member. Add a few macros to global_data.h to work around this. Update the code accordingly. Signed-off-by: Simon Glass --- common/board_r.c | 19 +++++++++---------- drivers/core/Makefile | 2 +- drivers/core/root.c | 27 +++++++++------------------ include/asm-generic/global_data.h | 13 ++++++++++++- include/dm/of.h | 9 +-------- test/dm/test-main.c | 16 +++++----------- 6 files changed, 37 insertions(+), 49 deletions(-) diff --git a/common/board_r.c b/common/board_r.c index b9217b2e27..0829622d0d 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -296,20 +296,21 @@ static int initr_noncached(void) } #endif -#ifdef CONFIG_OF_LIVE static int initr_of_live(void) { - int ret; + if (CONFIG_IS_ENABLED(OF_LIVE)) { + int ret; - bootstage_start(BOOTSTAGE_ID_ACCUM_OF_LIVE, "of_live"); - ret = of_live_build(gd->fdt_blob, (struct device_node **)&gd->of_root); - bootstage_accum(BOOTSTAGE_ID_ACCUM_OF_LIVE); - if (ret) - return ret; + bootstage_start(BOOTSTAGE_ID_ACCUM_OF_LIVE, "of_live"); + ret = of_live_build(gd->fdt_blob, + (struct device_node **)gd_of_root_ptr()); + bootstage_accum(BOOTSTAGE_ID_ACCUM_OF_LIVE); + if (ret) + return ret; + } return 0; } -#endif #ifdef CONFIG_DM static int initr_dm(void) @@ -713,9 +714,7 @@ static init_fnc_t init_sequence_r[] = { #ifdef CONFIG_SYS_NONCACHED_MEMORY initr_noncached, #endif -#ifdef CONFIG_OF_LIVE initr_of_live, -#endif #ifdef CONFIG_DM initr_dm, #endif diff --git a/drivers/core/Makefile b/drivers/core/Makefile index 10f4bece33..5edd4e4135 100644 --- a/drivers/core/Makefile +++ b/drivers/core/Makefile @@ -11,7 +11,7 @@ obj-$(CONFIG_SIMPLE_PM_BUS) += simple-pm-bus.o obj-$(CONFIG_DM) += dump.o obj-$(CONFIG_$(SPL_TPL_)REGMAP) += regmap.o obj-$(CONFIG_$(SPL_TPL_)SYSCON) += syscon-uclass.o -obj-$(CONFIG_OF_LIVE) += of_access.o of_addr.o +obj-$(CONFIG_$(SPL_)OF_LIVE) += of_access.o of_addr.o ifndef CONFIG_DM_DEV_READ_INLINE obj-$(CONFIG_OF_CONTROL) += read.o endif diff --git a/drivers/core/root.c b/drivers/core/root.c index 0726be6b79..de23161cff 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -61,7 +61,7 @@ void fix_drivers(void) for (entry = drv; entry != drv + n_ents; entry++) { if (entry->of_match) entry->of_match = (const struct udevice_id *) - ((u32)entry->of_match + gd->reloc_off); + ((ulong)entry->of_match + gd->reloc_off); if (entry->bind) entry->bind += gd->reloc_off; if (entry->probe) @@ -151,11 +151,9 @@ int dm_init(bool of_live) if (ret) return ret; #if CONFIG_IS_ENABLED(OF_CONTROL) -# if CONFIG_IS_ENABLED(OF_LIVE) - if (of_live) - DM_ROOT_NON_CONST->node = np_to_ofnode(gd->of_root); + if (CONFIG_IS_ENABLED(OF_LIVE) && of_live) + DM_ROOT_NON_CONST->node = np_to_ofnode(gd_of_root()); else -#endif DM_ROOT_NON_CONST->node = offset_to_ofnode(0); #endif ret = device_probe(DM_ROOT_NON_CONST); @@ -196,7 +194,7 @@ int dm_scan_platdata(bool pre_reloc_only) return ret; } -#if CONFIG_IS_ENABLED(OF_LIVE) +#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA) static int dm_scan_fdt_live(struct udevice *parent, const struct device_node *node_parent, bool pre_reloc_only) @@ -223,9 +221,7 @@ static int dm_scan_fdt_live(struct udevice *parent, return ret; } -#endif /* CONFIG_IS_ENABLED(OF_LIVE) */ -#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA) /** * dm_scan_fdt_node() - Scan the device tree and bind drivers for a node * @@ -272,24 +268,20 @@ int dm_scan_fdt_dev(struct udevice *dev) if (!dev_of_valid(dev)) return 0; -#if CONFIG_IS_ENABLED(OF_LIVE) if (of_live_active()) return dm_scan_fdt_live(dev, dev_np(dev), gd->flags & GD_FLG_RELOC ? false : true); - else -#endif + return dm_scan_fdt_node(dev, gd->fdt_blob, dev_of_offset(dev), gd->flags & GD_FLG_RELOC ? false : true); } int dm_scan_fdt(const void *blob, bool pre_reloc_only) { -#if CONFIG_IS_ENABLED(OF_LIVE) if (of_live_active()) - return dm_scan_fdt_live(gd->dm_root, gd->of_root, + return dm_scan_fdt_live(gd->dm_root, gd_of_root(), pre_reloc_only); - else -#endif + return dm_scan_fdt_node(gd->dm_root, blob, 0, pre_reloc_only); } @@ -302,10 +294,9 @@ static int dm_scan_fdt_ofnode_path(const void *blob, const char *path, if (!ofnode_valid(node)) return 0; -#if CONFIG_IS_ENABLED(OF_LIVE) if (of_live_active()) return dm_scan_fdt_live(gd->dm_root, node.np, pre_reloc_only); -#endif + return dm_scan_fdt_node(gd->dm_root, blob, node.of_offset, pre_reloc_only); } @@ -352,7 +343,7 @@ int dm_init_and_scan(bool pre_reloc_only) dm_populate_phandle_data(); #endif - ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE)); + ret = dm_init(CONFIG_IS_ENABLED(OF_LIVE)); if (ret) { debug("dm_init() failed: %d\n", ret); return ret; diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 0157af1aa4..cadfc05dd7 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -211,7 +211,7 @@ struct global_data { * @fdt_size: space reserved for relocated device space */ unsigned long fdt_size; -#ifdef CONFIG_OF_LIVE +#if CONFIG_IS_ENABLED(OF_LIVE) /** * @of_root: root node of the live tree */ @@ -427,6 +427,17 @@ struct global_data { #define gd_board_type() 0 #endif +/* These macros help avoid #ifdefs in the code */ +#if CONFIG_IS_ENABLED(OF_LIVE) +#define gd_of_root() gd->of_root +#define gd_of_root_ptr() &gd->of_root +#define gd_set_of_root(_root) gd->of_root = (_root) +#else +#define gd_of_root() NULL +#define gd_of_root_ptr() NULL +#define gd_set_of_root(_root) +#endif + /** * enum gd_flags - global data flags * diff --git a/include/dm/of.h b/include/dm/of.h index 6bef73b441..5cb6f44a6c 100644 --- a/include/dm/of.h +++ b/include/dm/of.h @@ -90,17 +90,10 @@ DECLARE_GLOBAL_DATA_PTR; * * @returns true if livetree is active, false it not */ -#ifdef CONFIG_OF_LIVE static inline bool of_live_active(void) { - return gd->of_root != NULL; + return gd_of_root() != NULL; } -#else -static inline bool of_live_active(void) -{ - return false; -} -#endif #define OF_BAD_ADDR ((u64)-1) diff --git a/test/dm/test-main.c b/test/dm/test-main.c index 38b7b1481a..995988723a 100644 --- a/test/dm/test-main.c +++ b/test/dm/test-main.c @@ -33,10 +33,8 @@ static int dm_test_init(struct unit_test_state *uts, bool of_live) memset(dm_testdrv_op_count, '\0', sizeof(dm_testdrv_op_count)); state_reset_for_test(state_get_current()); -#ifdef CONFIG_OF_LIVE /* Determine whether to make the live tree available */ - gd->of_root = of_live ? uts->of_root : NULL; -#endif + gd_set_of_root(of_live ? uts->of_root : NULL); ut_assertok(dm_init(of_live)); dms->root = dm_root(); @@ -152,9 +150,7 @@ static int dm_test_main(const char *test_name) printf("Running %d driver model tests\n", n_ents); found = 0; -#ifdef CONFIG_OF_LIVE - uts->of_root = gd->of_root; -#endif + uts->of_root = gd_of_root(); for (test = tests; test < tests + n_ents; test++) { const char *name = test->name; int runs; @@ -167,7 +163,7 @@ static int dm_test_main(const char *test_name) /* Run with the live tree if possible */ runs = 0; - if (IS_ENABLED(CONFIG_OF_LIVE)) { + if (CONFIG_IS_ENABLED(OF_LIVE)) { if (!(test->flags & UT_TESTF_FLAT_TREE)) { ut_assertok(dm_do_test(uts, test, true)); runs++; @@ -192,11 +188,9 @@ static int dm_test_main(const char *test_name) printf("Failures: %d\n", uts->fail_count); /* Put everything back to normal so that sandbox works as expected */ -#ifdef CONFIG_OF_LIVE - gd->of_root = uts->of_root; -#endif + gd_set_of_root(uts->of_root); gd->dm_root = NULL; - ut_assertok(dm_init(IS_ENABLED(CONFIG_OF_LIVE))); + ut_assertok(dm_init(CONFIG_IS_ENABLED(OF_LIVE))); dm_scan_platdata(false); dm_scan_fdt(gd->fdt_blob, false); From 1cadc7611755bcd5e28e98515f4814e598196daa Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:23 -0600 Subject: [PATCH 08/71] dm: test: Sort the Makefile Move everything into alphabetical order. Signed-off-by: Simon Glass --- test/Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/Makefile b/test/Makefile index 7c4039964e..13ee661f50 100644 --- a/test/Makefile +++ b/test/Makefile @@ -5,12 +5,12 @@ obj-$(CONFIG_SANDBOX) += bloblist.o obj-$(CONFIG_CMDLINE) += cmd/ obj-$(CONFIG_UNIT_TEST) += cmd_ut.o -obj-$(CONFIG_UNIT_TEST) += ut.o obj-$(CONFIG_SANDBOX) += command_ut.o obj-$(CONFIG_SANDBOX) += compression.o +obj-$(CONFIG_UNIT_TEST) += lib/ +obj-y += log/ obj-$(CONFIG_SANDBOX) += print_ut.o obj-$(CONFIG_SANDBOX) += str_ut.o obj-$(CONFIG_UT_TIME) += time_ut.o obj-$(CONFIG_UT_UNICODE) += unicode_ut.o -obj-y += log/ -obj-$(CONFIG_UNIT_TEST) += lib/ +obj-$(CONFIG_UNIT_TEST) += ut.o From 82c468a049d8887b70b1826c9fda2388f14704ef Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:24 -0600 Subject: [PATCH 09/71] dm: test: Update Makefile conditions At present most of the tests in test/Makefile are dependent on CONFIG_SANDBOX. But this is not ideal since they rely on commands being available and SPL does not support commands. Use CONFIG_COMMAND instead. This has the dual purpose of allowing these tests to be used on other boards and allowing SPL to skip them. Signed-off-by: Simon Glass --- test/Makefile | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/Makefile b/test/Makefile index 13ee661f50..73bddb4f61 100644 --- a/test/Makefile +++ b/test/Makefile @@ -2,15 +2,15 @@ # # (C) Copyright 2012 The Chromium Authors -obj-$(CONFIG_SANDBOX) += bloblist.o -obj-$(CONFIG_CMDLINE) += cmd/ -obj-$(CONFIG_UNIT_TEST) += cmd_ut.o -obj-$(CONFIG_SANDBOX) += command_ut.o -obj-$(CONFIG_SANDBOX) += compression.o +obj-$(CONFIG_$(SPL_)CMDLINE) += bloblist.o +obj-$(CONFIG_$(SPL_)CMDLINE) += cmd/ +obj-$(CONFIG_$(SPL_)CMDLINE) += cmd_ut.o +obj-$(CONFIG_$(SPL_)CMDLINE) += command_ut.o +obj-$(CONFIG_$(SPL_)CMDLINE) += compression.o obj-$(CONFIG_UNIT_TEST) += lib/ obj-y += log/ -obj-$(CONFIG_SANDBOX) += print_ut.o -obj-$(CONFIG_SANDBOX) += str_ut.o +obj-$(CONFIG_$(SPL_)CMDLINE) += print_ut.o +obj-$(CONFIG_$(SPL_)CMDLINE) += str_ut.o obj-$(CONFIG_UT_TIME) += time_ut.o obj-$(CONFIG_UT_UNICODE) += unicode_ut.o obj-$(CONFIG_UNIT_TEST) += ut.o From 16a5068340f8d28e26436acf64e4dc5f652aa2a6 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:25 -0600 Subject: [PATCH 10/71] dm: test: Make use of CONFIG_UNIT_TEST At present we always include test/dm from the main Makefile. We have a CONFIG_UNIT_TEST that should control whether the test/ directory is built, so rely on that instead. Signed-off-by: Simon Glass --- Makefile | 2 +- test/Makefile | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 5b80eb2acc..89c42c1024 100644 --- a/Makefile +++ b/Makefile @@ -807,7 +807,7 @@ libs-$(CONFIG_API) += api/ ifdef CONFIG_POST libs-y += post/ endif -libs-$(CONFIG_UNIT_TEST) += test/ test/dm/ +libs-$(CONFIG_UNIT_TEST) += test/ libs-$(CONFIG_UT_ENV) += test/env/ libs-$(CONFIG_UT_OPTEE) += test/optee/ libs-$(CONFIG_UT_OVERLAY) += test/overlay/ diff --git a/test/Makefile b/test/Makefile index 73bddb4f61..ed3e882f7a 100644 --- a/test/Makefile +++ b/test/Makefile @@ -9,8 +9,9 @@ obj-$(CONFIG_$(SPL_)CMDLINE) += command_ut.o obj-$(CONFIG_$(SPL_)CMDLINE) += compression.o obj-$(CONFIG_UNIT_TEST) += lib/ obj-y += log/ +obj-y += dm/ obj-$(CONFIG_$(SPL_)CMDLINE) += print_ut.o obj-$(CONFIG_$(SPL_)CMDLINE) += str_ut.o obj-$(CONFIG_UT_TIME) += time_ut.o obj-$(CONFIG_UT_UNICODE) += unicode_ut.o -obj-$(CONFIG_UNIT_TEST) += ut.o +obj-y += ut.o From 627988f9f9b5533cedb3e9d2de0e0b905bb3f45f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:26 -0600 Subject: [PATCH 11/71] dm: test: Disable some tests that should not run in SPL Tests are easier to run in U-Boot proper. Running them in SPL does not add test coverage in most cases. Also some tests use features that are not available in SPL. Update the build rules to disable these tests in SPL. We still need test-main to be able to actually run SPL tests. Signed-off-by: Simon Glass --- test/Makefile | 9 ++++++--- test/dm/Makefile | 13 ++++++++----- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/test/Makefile b/test/Makefile index ed3e882f7a..1c930b3148 100644 --- a/test/Makefile +++ b/test/Makefile @@ -7,11 +7,14 @@ obj-$(CONFIG_$(SPL_)CMDLINE) += cmd/ obj-$(CONFIG_$(SPL_)CMDLINE) += cmd_ut.o obj-$(CONFIG_$(SPL_)CMDLINE) += command_ut.o obj-$(CONFIG_$(SPL_)CMDLINE) += compression.o -obj-$(CONFIG_UNIT_TEST) += lib/ -obj-y += log/ obj-y += dm/ obj-$(CONFIG_$(SPL_)CMDLINE) += print_ut.o obj-$(CONFIG_$(SPL_)CMDLINE) += str_ut.o obj-$(CONFIG_UT_TIME) += time_ut.o -obj-$(CONFIG_UT_UNICODE) += unicode_ut.o obj-y += ut.o + +ifeq ($(CONFIG_SPL_BUILD),) +obj-$(CONFIG_UNIT_TEST) += lib/ +obj-y += log/ +obj-$(CONFIG_$(SPL_)UT_UNICODE) += unicode_ut.o +endif diff --git a/test/dm/Makefile b/test/dm/Makefile index 8b3d77e34e..c0dc632f23 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -2,15 +2,16 @@ # # Copyright (c) 2013 Google, Inc -obj-$(CONFIG_UT_DM) += bus.o -obj-$(CONFIG_UT_DM) += nop.o -obj-$(CONFIG_UT_DM) += test-driver.o -obj-$(CONFIG_UT_DM) += test-fdt.o obj-$(CONFIG_UT_DM) += test-main.o -obj-$(CONFIG_UT_DM) += test-uclass.o # Tests for particular subsystems - when enabling driver model for a new # subsystem you must add sandbox tests here. +ifeq ($(CONFIG_SPL_BUILD),) +obj-$(CONFIG_UT_DM) += bus.o +obj-$(CONFIG_UT_DM) += test-driver.o +obj-$(CONFIG_UT_DM) += test-fdt.o +obj-$(CONFIG_UT_DM) += test-uclass.o + obj-$(CONFIG_UT_DM) += core.o ifneq ($(CONFIG_SANDBOX),) obj-$(CONFIG_ACPIGEN) += acpi.o @@ -36,6 +37,7 @@ obj-$(CONFIG_DM_MAILBOX) += mailbox.o obj-$(CONFIG_DM_MMC) += mmc.o obj-$(CONFIG_CMD_MUX) += mux-cmd.o obj-y += fdtdec.o +obj-$(CONFIG_UT_DM) += nop.o obj-y += ofnode.o obj-y += ofread.o obj-$(CONFIG_OSD) += osd.o @@ -88,3 +90,4 @@ ifneq ($(CONFIG_PINMUX),) obj-$(CONFIG_PINCONF) += pinmux.o endif endif +endif # !SPL From c14732984759d64634350d01d58daca8cf44d23f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:23 -0600 Subject: [PATCH 12/71] sandbox: Drop ad-hoc device declarations in SPL Since sandbox's SPL is build with of-platadata, we should not use U_BOOT_DEVICE() declarations as well. Drop them. Signed-off-by: Simon Glass --- board/sandbox/sandbox.c | 2 ++ drivers/serial/sandbox.c | 3 +++ drivers/sysreset/sysreset_sandbox.c | 2 ++ 3 files changed, 7 insertions(+) diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c index 937ce28411..18a605de02 100644 --- a/board/sandbox/sandbox.c +++ b/board/sandbox/sandbox.c @@ -21,10 +21,12 @@ */ gd_t *gd; +#if !CONFIG_IS_ENABLED(OF_PLATDATA) /* Add a simple GPIO device */ U_BOOT_DEVICE(gpio_sandbox) = { .name = "sandbox_gpio", }; +#endif void flush_cache(unsigned long start, unsigned long size) { diff --git a/drivers/serial/sandbox.c b/drivers/serial/sandbox.c index f09d291e04..db2fbac629 100644 --- a/drivers/serial/sandbox.c +++ b/drivers/serial/sandbox.c @@ -267,6 +267,7 @@ U_BOOT_DRIVER(sandbox_serial) = { .flags = DM_FLAG_PRE_RELOC, }; +#if !CONFIG_IS_ENABLED(OF_PLATDATA) static const struct sandbox_serial_platdata platdata_non_fdt = { .colour = -1, }; @@ -275,4 +276,6 @@ U_BOOT_DEVICE(serial_sandbox_non_fdt) = { .name = "sandbox_serial", .platdata = &platdata_non_fdt, }; +#endif + #endif /* CONFIG_IS_ENABLED(OF_CONTROL) */ diff --git a/drivers/sysreset/sysreset_sandbox.c b/drivers/sysreset/sysreset_sandbox.c index 69c22a7000..71cabd1956 100644 --- a/drivers/sysreset/sysreset_sandbox.c +++ b/drivers/sysreset/sysreset_sandbox.c @@ -130,7 +130,9 @@ U_BOOT_DRIVER(warm_sysreset_sandbox) = { .ops = &sandbox_warm_sysreset_ops, }; +#if !CONFIG_IS_ENABLED(OF_PLATDATA) /* This is here in case we don't have a device tree */ U_BOOT_DEVICE(sysreset_sandbox_non_fdt) = { .name = "sysreset_sandbox", }; +#endif From e4fb5faa044ffc66170f8776794386e5e11e13a2 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:24 -0600 Subject: [PATCH 13/71] dtoc: Document the return value of scan_structs() Add documentation to this function as well as generate_structs(), where the return value is ultimately passed in. Signed-off-by: Simon Glass --- tools/dtoc/dtb_platdata.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py index 1b52198a94..72725bb6fa 100644 --- a/tools/dtoc/dtb_platdata.py +++ b/tools/dtoc/dtb_platdata.py @@ -466,6 +466,13 @@ class DtbPlatdata(object): Once the widest property is determined, all other properties are updated to match that width. + + Returns: + dict containing structures: + key (str): Node name, as a C identifier + value: dict containing structure fields: + key (str): Field name + value: Prop object with field information """ structs = {} for node in self._valid_nodes: @@ -536,6 +543,14 @@ class DtbPlatdata(object): This writes out the body of a header file consisting of structure definitions for node in self._valid_nodes. See the documentation in doc/driver-model/of-plat.rst for more information. + + Args: + structs: dict containing structures: + key (str): Node name, as a C identifier + value: dict containing structure fields: + key (str): Field name + value: Prop object with field information + """ self.out_header() self.out('#include \n') From 1b27273e090590e7944a3f7a3b33ac9f90aae765 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:25 -0600 Subject: [PATCH 14/71] dtoc: Order the structures internally by name At present the structures are written in name order, but parents have to be written before their children, so the file does not end up being in order. The order of nodes in _valid_nodes matches the order of the devicetree. Update the code so that _valid_nodes is in sorted order, by C name of the structure. This allows us to assign a sequential ordering to each U_BOOT_DEVICE() declaration. U-Boot's linker lists are also ordered alphabetically, which means that the order in the driver_info list will match the order used by dtoc. This defines an index ('idx') for the U_BOOT_DEVICE declarations. They appear in alphabetical order, numbered from 0 in _valid_nodes and in the driver_info linker list. Add a comment against each declaration, showing the idx value. Signed-off-by: Simon Glass --- tools/dtoc/dtb_platdata.py | 21 +++++--- tools/dtoc/test_dtoc.py | 105 ++++++++++++++++++++++++------------- 2 files changed, 83 insertions(+), 43 deletions(-) diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py index 72725bb6fa..31a9b3877e 100644 --- a/tools/dtoc/dtb_platdata.py +++ b/tools/dtoc/dtb_platdata.py @@ -143,7 +143,8 @@ class DtbPlatdata(object): Properties: _fdt: Fdt object, referencing the device tree _dtb_fname: Filename of the input device tree binary file - _valid_nodes: A list of Node object with compatible strings + _valid_nodes: A list of Node object with compatible strings. The list + is ordered by conv_name_to_c(node.name) _include_disabled: true to include nodes marked status = "disabled" _outfile: The current output file (sys.stdout or a real file) _warning_disabled: true to disable warnings about driver names not found @@ -367,23 +368,24 @@ class DtbPlatdata(object): """ self._fdt = fdt.FdtScan(self._dtb_fname) - def scan_node(self, root): + def scan_node(self, root, valid_nodes): """Scan a node and subnodes to build a tree of node and phandle info This adds each node to self._valid_nodes. Args: root: Root node for scan + valid_nodes: List of Node objects to add to """ for node in root.subnodes: if 'compatible' in node.props: status = node.props.get('status') if (not self._include_disabled and not status or status.value != 'disabled'): - self._valid_nodes.append(node) + valid_nodes.append(node) # recurse to handle any subnodes - self.scan_node(node) + self.scan_node(node, valid_nodes) def scan_tree(self): """Scan the device tree for useful information @@ -392,8 +394,12 @@ class DtbPlatdata(object): _valid_nodes: A list of nodes we wish to consider include in the platform data """ - self._valid_nodes = [] - return self.scan_node(self._fdt.GetRoot()) + valid_nodes = [] + self.scan_node(self._fdt.GetRoot(), valid_nodes) + self._valid_nodes = sorted(valid_nodes, + key=lambda x: conv_name_to_c(x.name)) + for idx, node in enumerate(self._valid_nodes): + node.idx = idx @staticmethod def get_num_cells(node): @@ -474,7 +480,7 @@ class DtbPlatdata(object): key (str): Field name value: Prop object with field information """ - structs = {} + structs = collections.OrderedDict() for node in self._valid_nodes: node_name, _ = self.get_normalized_compat_name(node) fields = {} @@ -633,6 +639,7 @@ class DtbPlatdata(object): struct_name, _ = self.get_normalized_compat_name(node) var_name = conv_name_to_c(node.name) + self.buf('/* Node %s index %d */\n' % (node.path, node.idx)) self.buf('static struct %s%s %s%s = {\n' % (STRUCT_PREFIX, struct_name, VAL_PREFIX, var_name)) for pname in sorted(node.props): diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index c2ff267de7..857a98e435 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -209,6 +209,27 @@ struct dtd_sandbox_spl_test_2 { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' +/* Node /i2c@0 index 0 */ +static struct dtd_sandbox_i2c_test dtv_i2c_at_0 = { +}; +U_BOOT_DEVICE(i2c_at_0) = { +\t.name\t\t= "sandbox_i2c_test", +\t.platdata\t= &dtv_i2c_at_0, +\t.platdata_size\t= sizeof(dtv_i2c_at_0), +}; + +/* Node /i2c@0/pmic@9 index 1 */ +static struct dtd_sandbox_pmic_test dtv_pmic_at_9 = { +\t.low_power\t\t= true, +\t.reg\t\t\t= {0x9, 0x0}, +}; +U_BOOT_DEVICE(pmic_at_9) = { +\t.name\t\t= "sandbox_pmic_test", +\t.platdata\t= &dtv_pmic_at_9, +\t.platdata_size\t= sizeof(dtv_pmic_at_9), +}; + +/* Node /spl-test index 2 */ static struct dtd_sandbox_spl_test dtv_spl_test = { \t.boolval\t\t= true, \t.bytearray\t\t= {0x6, 0x0, 0x0}, @@ -227,6 +248,7 @@ U_BOOT_DEVICE(spl_test) = { \t.platdata_size\t= sizeof(dtv_spl_test), }; +/* Node /spl-test2 index 3 */ static struct dtd_sandbox_spl_test dtv_spl_test2 = { \t.acpi_name\t\t= "\\\\_SB.GPO0", \t.bytearray\t\t= {0x1, 0x23, 0x34}, @@ -244,6 +266,7 @@ U_BOOT_DEVICE(spl_test2) = { \t.platdata_size\t= sizeof(dtv_spl_test2), }; +/* Node /spl-test3 index 4 */ static struct dtd_sandbox_spl_test dtv_spl_test3 = { \t.stringarray\t\t= {"one", "", ""}, }; @@ -253,6 +276,7 @@ U_BOOT_DEVICE(spl_test3) = { \t.platdata_size\t= sizeof(dtv_spl_test3), }; +/* Node /spl-test4 index 5 */ static struct dtd_sandbox_spl_test_2 dtv_spl_test4 = { }; U_BOOT_DEVICE(spl_test4) = { @@ -261,24 +285,6 @@ U_BOOT_DEVICE(spl_test4) = { \t.platdata_size\t= sizeof(dtv_spl_test4), }; -static struct dtd_sandbox_i2c_test dtv_i2c_at_0 = { -}; -U_BOOT_DEVICE(i2c_at_0) = { -\t.name\t\t= "sandbox_i2c_test", -\t.platdata\t= &dtv_i2c_at_0, -\t.platdata_size\t= sizeof(dtv_i2c_at_0), -}; - -static struct dtd_sandbox_pmic_test dtv_pmic_at_9 = { -\t.low_power\t\t= true, -\t.reg\t\t\t= {0x9, 0x0}, -}; -U_BOOT_DEVICE(pmic_at_9) = { -\t.name\t\t= "sandbox_pmic_test", -\t.platdata\t= &dtv_pmic_at_9, -\t.platdata_size\t= sizeof(dtv_pmic_at_9), -}; - ''' + C_EMPTY_POPULATE_PHANDLE_DATA, data) def test_driver_alias(self): @@ -300,6 +306,7 @@ struct dtd_sandbox_gpio { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' +/* Node /gpios@0 index 0 */ static struct dtd_sandbox_gpio dtv_gpios_at_0 = { \t.gpio_bank_name\t\t= "a", \t.gpio_controller\t= true, @@ -333,6 +340,7 @@ struct dtd_invalid { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' +/* Node /spl-test index 0 */ static struct dtd_invalid dtv_spl_test = { }; U_BOOT_DEVICE(spl_test) = { @@ -365,15 +373,7 @@ struct dtd_target { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' -static struct dtd_target dtv_phandle_target = { -\t.intval\t\t\t= 0x0, -}; -U_BOOT_DEVICE(phandle_target) = { -\t.name\t\t= "target", -\t.platdata\t= &dtv_phandle_target, -\t.platdata_size\t= sizeof(dtv_phandle_target), -}; - +/* Node /phandle2-target index 0 */ static struct dtd_target dtv_phandle2_target = { \t.intval\t\t\t= 0x1, }; @@ -383,6 +383,7 @@ U_BOOT_DEVICE(phandle2_target) = { \t.platdata_size\t= sizeof(dtv_phandle2_target), }; +/* Node /phandle3-target index 1 */ static struct dtd_target dtv_phandle3_target = { \t.intval\t\t\t= 0x2, }; @@ -392,6 +393,17 @@ U_BOOT_DEVICE(phandle3_target) = { \t.platdata_size\t= sizeof(dtv_phandle3_target), }; +/* Node /phandle-target index 4 */ +static struct dtd_target dtv_phandle_target = { +\t.intval\t\t\t= 0x0, +}; +U_BOOT_DEVICE(phandle_target) = { +\t.name\t\t= "target", +\t.platdata\t= &dtv_phandle_target, +\t.platdata_size\t= sizeof(dtv_phandle_target), +}; + +/* Node /phandle-source index 2 */ static struct dtd_source dtv_phandle_source = { \t.clocks\t\t\t= { \t\t\t{NULL, {}}, @@ -405,6 +417,7 @@ U_BOOT_DEVICE(phandle_source) = { \t.platdata_size\t= sizeof(dtv_phandle_source), }; +/* Node /phandle-source2 index 3 */ static struct dtd_source dtv_phandle_source2 = { \t.clocks\t\t\t= { \t\t\t{NULL, {}},}, @@ -448,6 +461,7 @@ struct dtd_target { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' +/* Node /phandle-target index 1 */ static struct dtd_target dtv_phandle_target = { }; U_BOOT_DEVICE(phandle_target) = { @@ -456,6 +470,7 @@ U_BOOT_DEVICE(phandle_target) = { \t.platdata_size\t= sizeof(dtv_phandle_target), }; +/* Node /phandle-source2 index 0 */ static struct dtd_source dtv_phandle_source2 = { \t.clocks\t\t\t= { \t\t\t{NULL, {}},}, @@ -479,15 +494,7 @@ void dm_populate_phandle_data(void) { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' -static struct dtd_target dtv_phandle_target = { -\t.intval\t\t\t= 0x0, -}; -U_BOOT_DEVICE(phandle_target) = { -\t.name\t\t= "target", -\t.platdata\t= &dtv_phandle_target, -\t.platdata_size\t= sizeof(dtv_phandle_target), -}; - +/* Node /phandle2-target index 0 */ static struct dtd_target dtv_phandle2_target = { \t.intval\t\t\t= 0x1, }; @@ -497,6 +504,7 @@ U_BOOT_DEVICE(phandle2_target) = { \t.platdata_size\t= sizeof(dtv_phandle2_target), }; +/* Node /phandle3-target index 1 */ static struct dtd_target dtv_phandle3_target = { \t.intval\t\t\t= 0x2, }; @@ -506,6 +514,17 @@ U_BOOT_DEVICE(phandle3_target) = { \t.platdata_size\t= sizeof(dtv_phandle3_target), }; +/* Node /phandle-target index 4 */ +static struct dtd_target dtv_phandle_target = { +\t.intval\t\t\t= 0x0, +}; +U_BOOT_DEVICE(phandle_target) = { +\t.name\t\t= "target", +\t.platdata\t= &dtv_phandle_target, +\t.platdata_size\t= sizeof(dtv_phandle_target), +}; + +/* Node /phandle-source index 2 */ static struct dtd_source dtv_phandle_source = { \t.cd_gpios\t\t= { \t\t\t{NULL, {}}, @@ -519,6 +538,7 @@ U_BOOT_DEVICE(phandle_source) = { \t.platdata_size\t= sizeof(dtv_phandle_source), }; +/* Node /phandle-source2 index 3 */ static struct dtd_source dtv_phandle_source2 = { \t.cd_gpios\t\t= { \t\t\t{NULL, {}},}, @@ -581,6 +601,7 @@ struct dtd_test3 { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' +/* Node /test1 index 0 */ static struct dtd_test1 dtv_test1 = { \t.reg\t\t\t= {0x1234, 0x5678}, }; @@ -590,6 +611,7 @@ U_BOOT_DEVICE(test1) = { \t.platdata_size\t= sizeof(dtv_test1), }; +/* Node /test2 index 1 */ static struct dtd_test2 dtv_test2 = { \t.reg\t\t\t= {0x1234567890123456, 0x9876543210987654}, }; @@ -599,6 +621,7 @@ U_BOOT_DEVICE(test2) = { \t.platdata_size\t= sizeof(dtv_test2), }; +/* Node /test3 index 2 */ static struct dtd_test3 dtv_test3 = { \t.reg\t\t\t= {0x1234567890123456, 0x9876543210987654, 0x2, 0x3}, }; @@ -630,6 +653,7 @@ struct dtd_test2 { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' +/* Node /test1 index 0 */ static struct dtd_test1 dtv_test1 = { \t.reg\t\t\t= {0x1234, 0x5678}, }; @@ -639,6 +663,7 @@ U_BOOT_DEVICE(test1) = { \t.platdata_size\t= sizeof(dtv_test1), }; +/* Node /test2 index 1 */ static struct dtd_test2 dtv_test2 = { \t.reg\t\t\t= {0x12345678, 0x98765432, 0x2, 0x3}, }; @@ -673,6 +698,7 @@ struct dtd_test3 { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' +/* Node /test1 index 0 */ static struct dtd_test1 dtv_test1 = { \t.reg\t\t\t= {0x123400000000, 0x5678}, }; @@ -682,6 +708,7 @@ U_BOOT_DEVICE(test1) = { \t.platdata_size\t= sizeof(dtv_test1), }; +/* Node /test2 index 1 */ static struct dtd_test2 dtv_test2 = { \t.reg\t\t\t= {0x1234567890123456, 0x98765432}, }; @@ -691,6 +718,7 @@ U_BOOT_DEVICE(test2) = { \t.platdata_size\t= sizeof(dtv_test2), }; +/* Node /test3 index 2 */ static struct dtd_test3 dtv_test3 = { \t.reg\t\t\t= {0x1234567890123456, 0x98765432, 0x2, 0x3}, }; @@ -725,6 +753,7 @@ struct dtd_test3 { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' +/* Node /test1 index 0 */ static struct dtd_test1 dtv_test1 = { \t.reg\t\t\t= {0x1234, 0x567800000000}, }; @@ -734,6 +763,7 @@ U_BOOT_DEVICE(test1) = { \t.platdata_size\t= sizeof(dtv_test1), }; +/* Node /test2 index 1 */ static struct dtd_test2 dtv_test2 = { \t.reg\t\t\t= {0x12345678, 0x9876543210987654}, }; @@ -743,6 +773,7 @@ U_BOOT_DEVICE(test2) = { \t.platdata_size\t= sizeof(dtv_test2), }; +/* Node /test3 index 2 */ static struct dtd_test3 dtv_test3 = { \t.reg\t\t\t= {0x12345678, 0x9876543210987654, 0x2, 0x3}, }; @@ -792,6 +823,7 @@ struct dtd_sandbox_spl_test { with open(output) as infile: data = infile.read() self._CheckStrings(C_HEADER + ''' +/* Node /spl-test index 0 */ static struct dtd_sandbox_spl_test dtv_spl_test = { \t.intval\t\t\t= 0x1, }; @@ -801,6 +833,7 @@ U_BOOT_DEVICE(spl_test) = { \t.platdata_size\t= sizeof(dtv_spl_test), }; +/* Node /spl-test2 index 1 */ static struct dtd_sandbox_spl_test dtv_spl_test2 = { \t.intarray\t\t= 0x5, }; From abb9cd30b23858125ce015c1dffdae4d7895bae8 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:26 -0600 Subject: [PATCH 15/71] dm: core: Allow dm_warn() to be used in SPL At present this option is disabled in SPL, meaning that warnings are not displayed. It is sometimes useful to see warnings in SPL for debugging purposes. Add a new Kconfig option to permit this. Signed-off-by: Simon Glass --- drivers/core/Kconfig | 18 ++++++++++++++++-- drivers/core/util.c | 2 +- include/config_uncmd_spl.h | 1 - include/dm/util.h | 2 +- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig index 07d3a6a7a4..ffae6f9795 100644 --- a/drivers/core/Kconfig +++ b/drivers/core/Kconfig @@ -40,10 +40,24 @@ config DM_WARN depends on DM default y help + Enable this to see warnings related to driver model. + + Warnings may help with debugging, such as when expected devices do + not bind correctly. If the option is disabled, dm_warn() is compiled + out - it will do nothing when called. + +config SPL_DM_WARN + bool "Enable warnings in driver model wuth SPL" + depends on SPL_DM + help + Enable this to see warnings related to driver model in SPL + The dm_warn() function can use up quite a bit of space for its strings. By default this is disabled for SPL builds to save space. - This will cause dm_warn() to be compiled out - it will do nothing - when called. + + Warnings may help with debugging, such as when expected devices do + not bind correctly. If the option is disabled, dm_warn() is compiled + out - it will do nothing when called. config DM_DEBUG bool "Enable debug messages in driver model core" diff --git a/drivers/core/util.c b/drivers/core/util.c index 25b0d76f43..91e93b0cf1 100644 --- a/drivers/core/util.c +++ b/drivers/core/util.c @@ -11,7 +11,7 @@ #include #include -#ifdef CONFIG_DM_WARN +#if CONFIG_IS_ENABLED(DM_WARN) void dm_warn(const char *fmt, ...) { va_list args; diff --git a/include/config_uncmd_spl.h b/include/config_uncmd_spl.h index 31da6215b3..af7e3e49fd 100644 --- a/include/config_uncmd_spl.h +++ b/include/config_uncmd_spl.h @@ -16,7 +16,6 @@ #undef CONFIG_DM_SPI #endif -#undef CONFIG_DM_WARN #undef CONFIG_DM_STDIO #endif /* CONFIG_SPL_BUILD */ diff --git a/include/dm/util.h b/include/dm/util.h index 9773db6de1..01a044992f 100644 --- a/include/dm/util.h +++ b/include/dm/util.h @@ -6,7 +6,7 @@ #ifndef __DM_UTIL_H #define __DM_UTIL_H -#ifdef CONFIG_DM_WARN +#if CONFIG_IS_ENABLED(DM_WARN) void dm_warn(const char *fmt, ...); #else static inline void dm_warn(const char *fmt, ...) From e144cafe43c8298bd41c044329857c3068cd845b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:27 -0600 Subject: [PATCH 16/71] dtoc: Fix widening of int to bytes At present an integer is converted to bytes incorrectly. The whole 32-bit integer is inserted as the first element of the byte array, and the other three bytes are skipped. This was not noticed because the unit test did not check it, and the functional test was checking for wrong values. Update the code to handle this as a special case. Add one more test to cover all code paths. Signed-off-by: Simon Glass --- test/py/tests/test_ofplatdata.py | 2 +- tools/dtoc/dtoc_test_simple.dts | 1 + tools/dtoc/fdt.py | 9 +++++++++ tools/dtoc/test_dtoc.py | 4 +++- tools/dtoc/test_fdt.py | 10 ++++++++++ 5 files changed, 24 insertions(+), 2 deletions(-) diff --git a/test/py/tests/test_ofplatdata.py b/test/py/tests/test_ofplatdata.py index 263334b074..13154935b9 100644 --- a/test/py/tests/test_ofplatdata.py +++ b/test/py/tests/test_ofplatdata.py @@ -20,7 +20,7 @@ byte 08 bytearray 01 23 34 int 3 intarray 5 0 0 0 -longbytearray 09 00 00 00 00 00 00 00 00 +longbytearray 09 0a 0b 0c 00 00 00 00 00 string message2 stringarray "another" "multi-word" "message" of-platdata probe: diff --git a/tools/dtoc/dtoc_test_simple.dts b/tools/dtoc/dtoc_test_simple.dts index 11bfc4c47a..fd168cb593 100644 --- a/tools/dtoc/dtoc_test_simple.dts +++ b/tools/dtoc/dtoc_test_simple.dts @@ -41,6 +41,7 @@ u-boot,dm-pre-reloc; compatible = "sandbox,spl-test"; stringarray = "one"; + longbytearray = [09 0a 0b 0c 0d 0e 0f 10]; }; spl-test4 { diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py index d058c59e92..03b86773d5 100644 --- a/tools/dtoc/fdt.py +++ b/tools/dtoc/fdt.py @@ -129,6 +129,15 @@ class Prop: specific. """ if newprop.type < self.type: + # Special handling to convert an int into bytes + if self.type == TYPE_INT and newprop.type == TYPE_BYTE: + if type(self.value) == list: + new_value = [] + for val in self.value: + new_value += [tools.ToChar(by) for by in val] + else: + new_value = [tools.ToChar(by) for by in self.value] + self.value = new_value self.type = newprop.type if type(newprop.value) == list and type(self.value) != list: diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index 857a98e435..8dcac91ee7 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -255,7 +255,7 @@ static struct dtd_sandbox_spl_test dtv_spl_test2 = { \t.byteval\t\t= 0x8, \t.intarray\t\t= {0x5, 0x0, 0x0, 0x0}, \t.intval\t\t\t= 0x3, -\t.longbytearray\t\t= {0x9, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, +\t.longbytearray\t\t= {0x9, 0xa, 0xb, 0xc, 0x0, 0x0, 0x0, 0x0, \t\t0x0}, \t.stringarray\t\t= {"another", "multi-word", "message"}, \t.stringval\t\t= "message2", @@ -268,6 +268,8 @@ U_BOOT_DEVICE(spl_test2) = { /* Node /spl-test3 index 4 */ static struct dtd_sandbox_spl_test dtv_spl_test3 = { +\t.longbytearray\t\t= {0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0x10, +\t\t0x0}, \t.stringarray\t\t= {"one", "", ""}, }; U_BOOT_DEVICE(spl_test3) = { diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py index b4f9b7f498..cfe3e04c7a 100755 --- a/tools/dtoc/test_fdt.py +++ b/tools/dtoc/test_fdt.py @@ -298,6 +298,7 @@ class TestProp(unittest.TestCase): def testWiden(self): """Test widening of values""" node2 = self.dtb.GetNode('/spl-test2') + node3 = self.dtb.GetNode('/spl-test3') prop = self.node.props['intval'] # No action @@ -316,11 +317,20 @@ class TestProp(unittest.TestCase): # byte array, it should turn into an array. prop = self.node.props['longbytearray'] prop2 = node2.props['longbytearray'] + prop3 = node3.props['longbytearray'] self.assertFalse(isinstance(prop2.value, list)) self.assertEqual(4, len(prop2.value)) + self.assertEqual(b'\x09\x0a\x0b\x0c', prop2.value) prop2.Widen(prop) self.assertTrue(isinstance(prop2.value, list)) self.assertEqual(9, len(prop2.value)) + self.assertEqual(['\x09', '\x0a', '\x0b', '\x0c', '\0', + '\0', '\0', '\0', '\0'], prop2.value) + prop3.Widen(prop) + self.assertTrue(isinstance(prop3.value, list)) + self.assertEqual(9, len(prop3.value)) + self.assertEqual(['\x09', '\x0a', '\x0b', '\x0c', '\x0d', + '\x0e', '\x0f', '\x10', '\0'], prop3.value) # Similarly for a string array prop = self.node.props['stringval'] From 18bca1f172473649aed03a9fa797c453b2a04ee4 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Fri, 9 Oct 2020 23:44:11 +0200 Subject: [PATCH 17/71] sandbox: make SDL window resizable Without resizing the SDL window showed by ./u-boot -D -l is not legible on a high resolution screen. Allow resizing the window Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- arch/sandbox/cpu/sdl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/sandbox/cpu/sdl.c b/arch/sandbox/cpu/sdl.c index 7dc3dab32e..d4dab36981 100644 --- a/arch/sandbox/cpu/sdl.c +++ b/arch/sandbox/cpu/sdl.c @@ -127,7 +127,8 @@ int sandbox_sdl_init_display(int width, int height, int log2_bpp, sdl.pitch = sdl.width * sdl.depth / 8; SDL_Window *screen = SDL_CreateWindow("U-Boot", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, - sdl.vis_width, sdl.vis_height, 0); + sdl.vis_width, sdl.vis_height, + SDL_WINDOW_RESIZABLE); if (!screen) { printf("Unable to initialise SDL screen: %s\n", SDL_GetError()); From 970cd91e8c4d9c812857601475dc2972ad1cd1b4 Mon Sep 17 00:00:00 2001 From: Dario Binacchi Date: Sun, 11 Oct 2020 14:27:07 +0200 Subject: [PATCH 18/71] dm: core: fix typo in device.h Replace 'a the' with 'the' in include/dm/device.h. Signed-off-by: Dario Binacchi Reviewed-by: Simon Glass --- include/dm/device.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/dm/device.h b/include/dm/device.h index ac3b6c1b8a..993c9e6c5a 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -823,7 +823,7 @@ static inline bool device_is_on_pci_bus(const struct udevice *dev) _ret = device_next_child_err(&dev)) /** - * dm_scan_fdt_dev() - Bind child device in a the device tree + * dm_scan_fdt_dev() - Bind child device in the device tree * * This handles device which have sub-nodes in the device tree. It scans all * sub-nodes and binds drivers for each node where a driver can be found. From ba96be48ad34180debcfbc11434be7329d530701 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:26 -0600 Subject: [PATCH 19/71] dm: test: Build tests for SPL We want to run unit tests in SPL. Add a new Kconfig to control this and enable it for sandbox_spl Signed-off-by: Simon Glass --- configs/sandbox_spl_defconfig | 2 +- scripts/Makefile.spl | 1 + test/Kconfig | 10 ++++++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig index 1d49e81639..f59e4f2e68 100644 --- a/configs/sandbox_spl_defconfig +++ b/configs/sandbox_spl_defconfig @@ -22,7 +22,6 @@ CONFIG_BOOTSTAGE_STASH=y CONFIG_BOOTSTAGE_STASH_SIZE=0x4096 CONFIG_CONSOLE_RECORD=y CONFIG_CONSOLE_RECORD_OUT_SIZE=0x1000 -CONFIG_SILENT_CONSOLE=y CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_HANDOFF=y CONFIG_SPL_BOARD_INIT=y @@ -221,5 +220,6 @@ CONFIG_TPM=y CONFIG_LZ4=y CONFIG_ERRNO_STR=y CONFIG_UNIT_TEST=y +CONFIG_SPL_UNIT_TEST=y CONFIG_UT_TIME=y CONFIG_UT_DM=y diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index d528c994ff..2e3a443035 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -99,6 +99,7 @@ libs-y += dts/ libs-y += fs/ libs-$(CONFIG_SPL_POST_MEM_SUPPORT) += post/drivers/ libs-$(CONFIG_SPL_NET_SUPPORT) += net/ +libs-$(CONFIG_SPL_UNIT_TEST) += test/ head-y := $(addprefix $(obj)/,$(head-y)) libs-y := $(addprefix $(obj)/,$(libs-y)) diff --git a/test/Kconfig b/test/Kconfig index 28704a25b6..2646e7d825 100644 --- a/test/Kconfig +++ b/test/Kconfig @@ -6,6 +6,16 @@ menuconfig UNIT_TEST This does not require sandbox to be included, but it is most often used there. +config SPL_UNIT_TEST + bool "Unit tests in SPL" + # We need to be able to unbind devices for tests to work + select SPL_DM_DEVICE_REMOVE + help + Select this to enable unit tests in SPL. Most test are designed for + running in U-Boot proper, but some are intended for SPL, such as + of-platdata and SPL handover. To run these tests with the sandbox_spl + board, use the -u (unit test) option. + config UT_LIB bool "Unit tests for library functions" depends on UNIT_TEST From 5b448ce687c0f34b0b55ffe5785b14d762b2296d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:27 -0600 Subject: [PATCH 20/71] dm: test: Update the test runner to support of-platdata At present DM tests assume that a devicetree is available. This is not the case with of-platadata. Update the code to add this condition. Signed-off-by: Simon Glass --- test/dm/test-main.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/test/dm/test-main.c b/test/dm/test-main.c index 995988723a..5560572c7c 100644 --- a/test/dm/test-main.c +++ b/test/dm/test-main.c @@ -30,7 +30,8 @@ static int dm_test_init(struct unit_test_state *uts, bool of_live) memset(dms, '\0', sizeof(*dms)); gd->dm_root = NULL; - memset(dm_testdrv_op_count, '\0', sizeof(dm_testdrv_op_count)); + if (!CONFIG_IS_ENABLED(OF_PLATDATA)) + memset(dm_testdrv_op_count, '\0', sizeof(dm_testdrv_op_count)); state_reset_for_test(state_get_current()); /* Determine whether to make the live tree available */ @@ -91,7 +92,8 @@ static int dm_do_test(struct unit_test_state *uts, struct unit_test *test, ut_assertok(dm_scan_platdata(false)); if (test->flags & UT_TESTF_PROBE_TEST) ut_assertok(do_autoprobe(uts)); - if (test->flags & UT_TESTF_SCAN_FDT) + if (!CONFIG_IS_ENABLED(OF_PLATDATA) && + (test->flags & UT_TESTF_SCAN_FDT)) ut_assertok(dm_extended_scan_fdt(gd->fdt_blob, false)); /* @@ -136,14 +138,16 @@ static int dm_test_main(const char *test_name) uts->priv = &_global_priv_dm_test_state; uts->fail_count = 0; - /* - * If we have no device tree, or it only has a root node, then these - * tests clearly aren't going to work... - */ - if (!gd->fdt_blob || fdt_next_node(gd->fdt_blob, 0, NULL) < 0) { - puts("Please run with test device tree:\n" - " ./u-boot -d arch/sandbox/dts/test.dtb\n"); - ut_assert(gd->fdt_blob); + if (!CONFIG_IS_ENABLED(OF_PLATDATA)) { + /* + * If we have no device tree, or it only has a root node, then + * these * tests clearly aren't going to work... + */ + if (!gd->fdt_blob || fdt_next_node(gd->fdt_blob, 0, NULL) < 0) { + puts("Please run with test device tree:\n" + " ./u-boot -d arch/sandbox/dts/test.dtb\n"); + ut_assert(gd->fdt_blob); + } } if (!test_name) @@ -192,7 +196,8 @@ static int dm_test_main(const char *test_name) gd->dm_root = NULL; ut_assertok(dm_init(CONFIG_IS_ENABLED(OF_LIVE))); dm_scan_platdata(false); - dm_scan_fdt(gd->fdt_blob, false); + if (!CONFIG_IS_ENABLED(OF_PLATDATA)) + dm_scan_fdt(gd->fdt_blob, false); return uts->fail_count ? CMD_RET_FAILURE : 0; } From b25ff5cbaaaa7f144eb6e087b4bdd7362c58029d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:28 -0600 Subject: [PATCH 21/71] dm: test: Add a way to run SPL tests Add a -u flag for U-Boot SPL which requests that unit tests be run. To make this work, export dm_test_main() and update it to skip test features that are not used with of-platdata. To run the tests: $ spl/u-boot-spl -u U-Boot SPL 2020.10-rc5 (Oct 01 2020 - 07:35:39 -0600) Running 0 driver model tests Failures: 0 At present there are no SPL unit tests. Note that there is one wrinkle with these tests. SPL has limited memory available for allocation. Also malloc_simple does not free memory (free() is a nop) and running tests repeatedly causes driver-model to reinit multiple times and allocate memory. Therefore it is not possible to run more than a few tests at a time. One solution is to increase the amount of malloc space in sandbox_spl. This is not a problem for pytest, since it runs each test individually, so for now this is left as is. Signed-off-by: Simon Glass --- arch/sandbox/cpu/spl.c | 8 ++++++++ arch/sandbox/cpu/start.c | 9 +++++++++ arch/sandbox/include/asm/state.h | 1 + include/test/test.h | 11 +++++++++++ test/dm/test-main.c | 2 +- 5 files changed, 30 insertions(+), 1 deletion(-) diff --git a/arch/sandbox/cpu/spl.c b/arch/sandbox/cpu/spl.c index 7ab8919eb9..48fd1265af 100644 --- a/arch/sandbox/cpu/spl.c +++ b/arch/sandbox/cpu/spl.c @@ -12,6 +12,7 @@ #include #include #include +#include DECLARE_GLOBAL_DATA_PTR; @@ -67,6 +68,13 @@ void spl_board_init(void) uclass_next_device(&dev)) ; } + + if (state->run_unittests) { + int ret; + + ret = dm_test_main(NULL); + /* continue execution into U-Boot */ + } } void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image) diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c index c6a2bbe468..f5e104b127 100644 --- a/arch/sandbox/cpu/start.c +++ b/arch/sandbox/cpu/start.c @@ -374,6 +374,15 @@ static int sandbox_cmdline_cb_show_of_platdata(struct sandbox_state *state, } SANDBOX_CMDLINE_OPT(show_of_platdata, 0, "Show of-platdata in SPL"); +static int sandbox_cmdline_cb_unittests(struct sandbox_state *state, + const char *arg) +{ + state->run_unittests = true; + + return 0; +} +SANDBOX_CMDLINE_OPT_SHORT(unittests, 'u', 0, "Run unit tests"); + static void setup_ram_buf(struct sandbox_state *state) { /* Zero the RAM buffer if we didn't read it, to keep valgrind happy */ diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h index 1bfad305f1..f828d9d244 100644 --- a/arch/sandbox/include/asm/state.h +++ b/arch/sandbox/include/asm/state.h @@ -92,6 +92,7 @@ struct sandbox_state { int default_log_level; /* Default log level for sandbox */ bool show_of_platdata; /* Show of-platdata in SPL */ bool ram_buf_read; /* true if we read the RAM buffer */ + bool run_unittests; /* Run unit tests */ /* Pointer to information for each SPI bus/cs */ struct sandbox_spi_info spi[CONFIG_SANDBOX_SPI_MAX_BUS] diff --git a/include/test/test.h b/include/test/test.h index 67c7d69d48..03e29290bf 100644 --- a/include/test/test.h +++ b/include/test/test.h @@ -94,4 +94,15 @@ enum { TEST_DEVRES_SIZE3 = 37, }; +/** + * dm_test_main() - Run driver model tests + * + * Run all the available driver model tests, or a selection + * + * @test_name: Name of single test to run (e.g. "dm_test_fdt_pre_reloc" or just + * "fdt_pre_reloc"), or NULL to run all + * @return 0 if all tests passed, 1 if not + */ +int dm_test_main(const char *test_name); + #endif /* __TEST_TEST_H */ diff --git a/test/dm/test-main.c b/test/dm/test-main.c index 5560572c7c..9d22df8c4d 100644 --- a/test/dm/test-main.c +++ b/test/dm/test-main.c @@ -127,7 +127,7 @@ static bool dm_test_run_on_flattree(struct unit_test *test) return !strstr(fname, "video") || strstr(test->name, "video_base"); } -static int dm_test_main(const char *test_name) +int dm_test_main(const char *test_name) { struct unit_test *tests = ll_entry_start(struct unit_test, dm_test); const int n_ents = ll_entry_count(struct unit_test, dm_test); From 217293e399148e704d6c318b19b1f69959831f7b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:29 -0600 Subject: [PATCH 22/71] dm: test: Add a very simple of-platadata test At present we have a pytest that covers of-platadata. Add a very simple unit test that just checks that a device can be found. This shows the ability to write these tests in C. Signed-off-by: Simon Glass --- test/dm/Makefile | 4 +++- test/dm/of_platdata.c | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 test/dm/of_platdata.c diff --git a/test/dm/Makefile b/test/dm/Makefile index c0dc632f23..57e13ad533 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -6,7 +6,9 @@ obj-$(CONFIG_UT_DM) += test-main.o # Tests for particular subsystems - when enabling driver model for a new # subsystem you must add sandbox tests here. -ifeq ($(CONFIG_SPL_BUILD),) +ifeq ($(CONFIG_SPL_BUILD),y) +obj-$(CONFIG_SPL_OF_PLATDATA) += of_platdata.o +else obj-$(CONFIG_UT_DM) += bus.o obj-$(CONFIG_UT_DM) += test-driver.o obj-$(CONFIG_UT_DM) += test-fdt.o diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c new file mode 100644 index 0000000000..7a864eb0fb --- /dev/null +++ b/test/dm/of_platdata.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0+ + +#include +#include +#include +#include +#include + +/* Test that we can find a device using of-platdata */ +static int dm_test_of_platdata_base(struct unit_test_state *uts) +{ + struct udevice *dev; + + ut_assertok(uclass_first_device_err(UCLASS_SERIAL, &dev)); + ut_asserteq_str("sandbox_serial", dev->name); + + return 0; +} +DM_TEST(dm_test_of_platdata_base, UT_TESTF_SCAN_PDATA); From 60251b1d536abec8e163a00c41d801d69faa429c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:30 -0600 Subject: [PATCH 23/71] Makefile: Generate a symbol file for u-boot-spl Add a rule to generate u-boot-spl.sym so that pytest can discover the available unit tests. Signed-off-by: Simon Glass --- scripts/Makefile.spl | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 2e3a443035..9f1f7445d7 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -214,7 +214,7 @@ spl/boot.bin: $(obj)/$(SPL_BIN)-align.bin FORCE $(call if_changed,mkimage) endif -INPUTS-y += $(obj)/$(SPL_BIN).bin +INPUTS-y += $(obj)/$(SPL_BIN).bin $(obj)/$(SPL_BIN).sym ifdef CONFIG_SAMSUNG INPUTS-y += $(obj)/$(BOARD)-spl.bin @@ -408,6 +408,11 @@ MKIMAGEFLAGS_u-boot-spl-mtk.bin = -T mtk_image \ $(obj)/u-boot-spl-mtk.bin: $(obj)/u-boot-spl.bin FORCE $(call if_changed,mkimage) +quiet_cmd_sym ?= SYM $@ + cmd_sym ?= $(OBJDUMP) -t $< > $@ +$(obj)/$(SPL_BIN).sym: $(obj)/$(SPL_BIN) FORCE + $(call if_changed,sym) + # Rule to link u-boot-spl # May be overridden by arch/$(ARCH)/config.mk quiet_cmd_u-boot-spl ?= LD $@ From bc84d585ec0edf1705832fd431fc6a0ded978524 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:31 -0600 Subject: [PATCH 24/71] pytest: Collect SPL unit tests Add a new test_spl fixture to handle running SPL unit tests. Signed-off-by: Simon Glass --- test/py/conftest.py | 13 ++++++++----- test/py/tests/test_spl.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 test/py/tests/test_spl.py diff --git a/test/py/conftest.py b/test/py/conftest.py index 30920474b3..dc92c0be32 100644 --- a/test/py/conftest.py +++ b/test/py/conftest.py @@ -227,7 +227,7 @@ def pytest_configure(config): console = u_boot_console_exec_attach.ConsoleExecAttach(log, ubconfig) re_ut_test_list = re.compile(r'_u_boot_list_2_(.*)_test_2_\1_test_(.*)\s*$') -def generate_ut_subtest(metafunc, fixture_name): +def generate_ut_subtest(metafunc, fixture_name, sym_path): """Provide parametrization for a ut_subtest fixture. Determines the set of unit tests built into a U-Boot binary by parsing the @@ -237,12 +237,13 @@ def generate_ut_subtest(metafunc, fixture_name): Args: metafunc: The pytest test function. fixture_name: The fixture name to test. + sym_path: Relative path to the symbol file with preceding '/' + (e.g. '/u-boot.sym') Returns: Nothing. """ - - fn = console.config.build_dir + '/u-boot.sym' + fn = console.config.build_dir + sym_path try: with open(fn, 'rt') as f: lines = f.readlines() @@ -317,10 +318,12 @@ def pytest_generate_tests(metafunc): Returns: Nothing. """ - for fn in metafunc.fixturenames: if fn == 'ut_subtest': - generate_ut_subtest(metafunc, fn) + generate_ut_subtest(metafunc, fn, '/u-boot.sym') + continue + if fn == 'ut_spl_subtest': + generate_ut_subtest(metafunc, fn, '/spl/u-boot-spl.sym') continue generate_config(metafunc, fn) diff --git a/test/py/tests/test_spl.py b/test/py/tests/test_spl.py new file mode 100644 index 0000000000..990cc9b071 --- /dev/null +++ b/test/py/tests/test_spl.py @@ -0,0 +1,34 @@ +# SPDX-License-Identifier: GPL-2.0 +# Copyright 2020 Google LLC +# Written by Simon Glass + +import os.path +import pytest + +def test_spl(u_boot_console, ut_spl_subtest): + """Execute a "ut" subtest. + + The subtests are collected in function generate_ut_subtest() from linker + generated lists by applying a regular expression to the lines of file + spl/u-boot-spl.sym. The list entries are created using the C macro + UNIT_TEST(). + + Strict naming conventions have to be followed to match the regular + expression. Use UNIT_TEST(foo_test_bar, _flags, foo_test) for a test bar in + test suite foo that can be executed via command 'ut foo bar' and is + implemented in C function foo_test_bar(). + + Args: + u_boot_console (ConsoleBase): U-Boot console + ut_subtest (str): SPL test to be executed (e.g. 'dm platdata_phandle') + """ + try: + cons = u_boot_console + cons.restart_uboot_with_flags(['-u', ut_spl_subtest]) + output = cons.get_spawn_output().replace('\r', '') + assert 'Failures: 0' in output + finally: + # Restart afterward in case a non-SPL test is run next. This should not + # happen since SPL tests are run in their own invocation of test.py, but + # the cost of doing this is not too great at present. + u_boot_console.restart_uboot() From 7b51bf770af349aa03f49cb9f445173ff2dc7fc7 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:32 -0600 Subject: [PATCH 25/71] test: Run SPL unit tests Update the 'run' script to include SPL unit tests. Signed-off-by: Simon Glass --- test/run | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/run b/test/run index de87e7530b..735628e7e3 100755 --- a/test/run +++ b/test/run @@ -28,7 +28,7 @@ fi # Run tests which require sandbox_spl run_test "sandbox_spl" ./test/py/test.py --bd sandbox_spl --build \ - -k 'test_ofplatdata or test_handoff' + -k 'test_ofplatdata or test_handoff or test_spl' if [ -z "$tools_only" ]; then # Run tests for the flat-device-tree version of sandbox. This is a special From 22b29cc8fb15f611e8e2af6fde8627a32abea76d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:33 -0600 Subject: [PATCH 26/71] sandbox: Allow selection of SPL unit tests Now that we have more than one test, add a way to select the test to run. Signed-off-by: Simon Glass --- arch/sandbox/cpu/spl.c | 2 +- arch/sandbox/cpu/start.c | 9 +++++++++ arch/sandbox/include/asm/state.h | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/arch/sandbox/cpu/spl.c b/arch/sandbox/cpu/spl.c index 48fd1265af..81b217a1d7 100644 --- a/arch/sandbox/cpu/spl.c +++ b/arch/sandbox/cpu/spl.c @@ -72,7 +72,7 @@ void spl_board_init(void) if (state->run_unittests) { int ret; - ret = dm_test_main(NULL); + ret = dm_test_main(state->select_unittests); /* continue execution into U-Boot */ } } diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c index f5e104b127..569dafbcfe 100644 --- a/arch/sandbox/cpu/start.c +++ b/arch/sandbox/cpu/start.c @@ -383,6 +383,15 @@ static int sandbox_cmdline_cb_unittests(struct sandbox_state *state, } SANDBOX_CMDLINE_OPT_SHORT(unittests, 'u', 0, "Run unit tests"); +static int sandbox_cmdline_cb_select_unittests(struct sandbox_state *state, + const char *arg) +{ + state->select_unittests = arg; + + return 0; +} +SANDBOX_CMDLINE_OPT_SHORT(select_unittests, 'k', 1, "Select unit tests to run"); + static void setup_ram_buf(struct sandbox_state *state) { /* Zero the RAM buffer if we didn't read it, to keep valgrind happy */ diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h index f828d9d244..7547602dd1 100644 --- a/arch/sandbox/include/asm/state.h +++ b/arch/sandbox/include/asm/state.h @@ -93,6 +93,7 @@ struct sandbox_state { bool show_of_platdata; /* Show of-platdata in SPL */ bool ram_buf_read; /* true if we read the RAM buffer */ bool run_unittests; /* Run unit tests */ + const char *select_unittests; /* Unit test to run */ /* Pointer to information for each SPI bus/cs */ struct sandbox_spi_info spi[CONFIG_SANDBOX_SPI_MAX_BUS] From e1e54ffe9956449d79697d31648217065e033045 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:34 -0600 Subject: [PATCH 27/71] test: Run only the selected SPL test Use the new -k option to select the test to run. Signed-off-by: Simon Glass --- test/py/tests/test_spl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/py/tests/test_spl.py b/test/py/tests/test_spl.py index 990cc9b071..bd273dad89 100644 --- a/test/py/tests/test_spl.py +++ b/test/py/tests/test_spl.py @@ -24,7 +24,7 @@ def test_spl(u_boot_console, ut_spl_subtest): """ try: cons = u_boot_console - cons.restart_uboot_with_flags(['-u', ut_spl_subtest]) + cons.restart_uboot_with_flags(['-u', '-k', ut_spl_subtest.split()[1]]) output = cons.get_spawn_output().replace('\r', '') assert 'Failures: 0' in output finally: From a8b1fbc14d064d7b14bf887730300f4dbf856473 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:35 -0600 Subject: [PATCH 28/71] dm: test: Drop of-platdata pytest Now that we have a C version of this test, drop the Python implementation. Signed-off-by: Simon Glass --- arch/sandbox/cpu/spl.c | 12 -------- arch/sandbox/cpu/start.c | 9 ------ arch/sandbox/include/asm/state.h | 1 - drivers/misc/spltest_sandbox.c | 35 ------------------------ test/py/tests/test_ofplatdata.py | 47 -------------------------------- 5 files changed, 104 deletions(-) diff --git a/arch/sandbox/cpu/spl.c b/arch/sandbox/cpu/spl.c index 81b217a1d7..9a77da1561 100644 --- a/arch/sandbox/cpu/spl.c +++ b/arch/sandbox/cpu/spl.c @@ -54,20 +54,8 @@ SPL_LOAD_IMAGE_METHOD("sandbox", 9, BOOT_DEVICE_BOARD, spl_board_load_image); void spl_board_init(void) { struct sandbox_state *state = state_get_current(); - struct udevice *dev; preloader_console_init(); - if (state->show_of_platdata) { - /* - * Scan all the devices so that we can output their platform - * data. See sandbox_spl_probe(). - */ - printf("Scanning misc devices\n"); - for (uclass_first_device(UCLASS_MISC, &dev); - dev; - uclass_next_device(&dev)) - ; - } if (state->run_unittests) { int ret; diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c index 569dafbcfe..58ada13fba 100644 --- a/arch/sandbox/cpu/start.c +++ b/arch/sandbox/cpu/start.c @@ -365,15 +365,6 @@ static int sandbox_cmdline_cb_log_level(struct sandbox_state *state, SANDBOX_CMDLINE_OPT_SHORT(log_level, 'L', 1, "Set log level (0=panic, 7=debug)"); -static int sandbox_cmdline_cb_show_of_platdata(struct sandbox_state *state, - const char *arg) -{ - state->show_of_platdata = true; - - return 0; -} -SANDBOX_CMDLINE_OPT(show_of_platdata, 0, "Show of-platdata in SPL"); - static int sandbox_cmdline_cb_unittests(struct sandbox_state *state, const char *arg) { diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h index 7547602dd1..bca1306982 100644 --- a/arch/sandbox/include/asm/state.h +++ b/arch/sandbox/include/asm/state.h @@ -90,7 +90,6 @@ struct sandbox_state { bool skip_delays; /* Ignore any time delays (for test) */ bool show_test_output; /* Don't suppress stdout in tests */ int default_log_level; /* Default log level for sandbox */ - bool show_of_platdata; /* Show of-platdata in SPL */ bool ram_buf_read; /* true if we read the RAM buffer */ bool run_unittests; /* Run unit tests */ const char *select_unittests; /* Unit test to run */ diff --git a/drivers/misc/spltest_sandbox.c b/drivers/misc/spltest_sandbox.c index 999031625b..3ae6707593 100644 --- a/drivers/misc/spltest_sandbox.c +++ b/drivers/misc/spltest_sandbox.c @@ -8,43 +8,8 @@ #include #include -static int sandbox_spl_probe(struct udevice *dev) -{ - struct dtd_sandbox_spl_test *plat = dev_get_platdata(dev); - int i; - - printf("of-platdata probe:\n"); - printf("bool %d\n", plat->boolval); - - printf("byte %02x\n", plat->byteval); - printf("bytearray"); - for (i = 0; i < sizeof(plat->bytearray); i++) - printf(" %02x", plat->bytearray[i]); - printf("\n"); - - printf("int %d\n", plat->intval); - printf("intarray"); - for (i = 0; i < ARRAY_SIZE(plat->intarray); i++) - printf(" %d", plat->intarray[i]); - printf("\n"); - - printf("longbytearray"); - for (i = 0; i < sizeof(plat->longbytearray); i++) - printf(" %02x", plat->longbytearray[i]); - printf("\n"); - - printf("string %s\n", plat->stringval); - printf("stringarray"); - for (i = 0; i < ARRAY_SIZE(plat->stringarray); i++) - printf(" \"%s\"", plat->stringarray[i]); - printf("\n"); - - return 0; -} - U_BOOT_DRIVER(sandbox_spl_test) = { .name = "sandbox_spl_test", .id = UCLASS_MISC, .flags = DM_FLAG_PRE_RELOC, - .probe = sandbox_spl_probe, }; diff --git a/test/py/tests/test_ofplatdata.py b/test/py/tests/test_ofplatdata.py index 13154935b9..78837a3c00 100644 --- a/test/py/tests/test_ofplatdata.py +++ b/test/py/tests/test_ofplatdata.py @@ -4,53 +4,6 @@ import pytest import u_boot_utils as util -OF_PLATDATA_OUTPUT = ''' -of-platdata probe: -bool 1 -byte 05 -bytearray 06 00 00 -int 1 -intarray 2 3 4 0 -longbytearray 09 0a 0b 0c 0d 0e 0f 10 11 -string message -stringarray "multi-word" "message" "" -of-platdata probe: -bool 0 -byte 08 -bytearray 01 23 34 -int 3 -intarray 5 0 0 0 -longbytearray 09 0a 0b 0c 00 00 00 00 00 -string message2 -stringarray "another" "multi-word" "message" -of-platdata probe: -bool 0 -byte 00 -bytearray 00 00 00 -int 0 -intarray 0 0 0 0 -longbytearray 00 00 00 00 00 00 00 00 00 -string -stringarray "one" "" "" -of-platdata probe: -bool 0 -byte 00 -bytearray 00 00 00 -int 0 -intarray 0 0 0 0 -longbytearray 00 00 00 00 00 00 00 00 00 -string -stringarray "spl" "" "" -''' - -@pytest.mark.buildconfigspec('spl_of_platdata') -def test_ofplatdata(u_boot_console): - """Test that of-platdata can be generated and used in sandbox""" - cons = u_boot_console - cons.restart_uboot_with_flags(['--show_of_platdata']) - output = cons.get_spawn_output().replace('\r', '') - assert OF_PLATDATA_OUTPUT in output - @pytest.mark.buildconfigspec('spl_of_platdata') def test_spl_devicetree(u_boot_console): """Test content of spl device-tree""" From afb26ba9a7c0812c56d40ba00554bd1e2f3be460 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:36 -0600 Subject: [PATCH 29/71] Azure/GitLab/Travis: Add SPL unit tests Run SPL unit tests in all test environments. Signed-off-by: Simon Glass --- .azure-pipelines.yml | 2 +- .gitlab-ci.yml | 2 +- .travis.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index 473ddee383..a78c8d6130 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -182,7 +182,7 @@ jobs: OVERRIDE: "-O clang-10" sandbox_spl: TEST_PY_BD: "sandbox_spl" - TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff" + TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff or test_spl" sandbox_flattree: TEST_PY_BD: "sandbox_flattree" evb_ast2500: diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 9ac2b336a1..b1e0b3bc9d 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -198,7 +198,7 @@ sandbox_spl test.py: tags: [ 'all' ] variables: TEST_PY_BD: "sandbox_spl" - TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff" + TEST_PY_TEST_SPEC: "test_ofplatdata or test_handoff or test_spl" <<: *buildman_and_testpy_dfn evb-ast2500 test.py: diff --git a/.travis.yml b/.travis.yml index fb8f73157d..cb48ff3023 100644 --- a/.travis.yml +++ b/.travis.yml @@ -512,7 +512,7 @@ matrix: - name: "test/py sandbox_spl" env: - TEST_PY_BD="sandbox_spl" - TEST_PY_TEST_SPEC="test_ofplatdata or test_handoff" + TEST_PY_TEST_SPEC="test_ofplatdata or test_handoff or test_spl" TOOLCHAIN="i386" TEST_PY_TOOLS="yes" - name: "test/py sandbox_flattree" From b325248c9368c518d68d00d700404eeac801c98d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:28 -0600 Subject: [PATCH 30/71] dm: Add a C test for of-platdata properties At present properties are tested in a roundabout way. The driver's probe() method writes out the values of the properties and the Python test checks the output from U-Boot SPL. Add a C test which checks these values more directly. Signed-off-by: Simon Glass --- test/dm/of_platdata.c | 69 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c index 7a864eb0fb..8763005ea9 100644 --- a/test/dm/of_platdata.c +++ b/test/dm/of_platdata.c @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -17,3 +18,71 @@ static int dm_test_of_platdata_base(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_of_platdata_base, UT_TESTF_SCAN_PDATA); + +/* Test that we can read properties from a device */ +static int dm_test_of_platdata_props(struct unit_test_state *uts) +{ + struct dtd_sandbox_spl_test *plat; + struct udevice *dev; + int i; + + ut_assertok(uclass_first_device_err(UCLASS_MISC, &dev)); + plat = dev_get_platdata(dev); + ut_assert(plat->boolval); + ut_asserteq(1, plat->intval); + ut_asserteq(4, ARRAY_SIZE(plat->intarray)); + ut_asserteq(2, plat->intarray[0]); + ut_asserteq(3, plat->intarray[1]); + ut_asserteq(4, plat->intarray[2]); + ut_asserteq(0, plat->intarray[3]); + ut_asserteq(5, plat->byteval); + ut_asserteq(3, ARRAY_SIZE(plat->bytearray)); + ut_asserteq(6, plat->bytearray[0]); + ut_asserteq(0, plat->bytearray[1]); + ut_asserteq(0, plat->bytearray[2]); + ut_asserteq(9, ARRAY_SIZE(plat->longbytearray)); + for (i = 0; i < ARRAY_SIZE(plat->longbytearray); i++) + ut_asserteq(9 + i, plat->longbytearray[i]); + ut_asserteq_str("message", plat->stringval); + ut_asserteq(3, ARRAY_SIZE(plat->stringarray)); + ut_asserteq_str("multi-word", plat->stringarray[0]); + ut_asserteq_str("message", plat->stringarray[1]); + ut_asserteq_str("", plat->stringarray[2]); + + ut_assertok(uclass_next_device_err(&dev)); + plat = dev_get_platdata(dev); + ut_assert(!plat->boolval); + ut_asserteq(3, plat->intval); + ut_asserteq(5, plat->intarray[0]); + ut_asserteq(0, plat->intarray[1]); + ut_asserteq(0, plat->intarray[2]); + ut_asserteq(0, plat->intarray[3]); + ut_asserteq(8, plat->byteval); + ut_asserteq(3, ARRAY_SIZE(plat->bytearray)); + ut_asserteq(1, plat->bytearray[0]); + ut_asserteq(0x23, plat->bytearray[1]); + ut_asserteq(0x34, plat->bytearray[2]); + for (i = 0; i < ARRAY_SIZE(plat->longbytearray); i++) + ut_asserteq(i < 4 ? 9 + i : 0, plat->longbytearray[i]); + ut_asserteq_str("message2", plat->stringval); + ut_asserteq_str("another", plat->stringarray[0]); + ut_asserteq_str("multi-word", plat->stringarray[1]); + ut_asserteq_str("message", plat->stringarray[2]); + + ut_assertok(uclass_next_device_err(&dev)); + plat = dev_get_platdata(dev); + ut_assert(!plat->boolval); + ut_asserteq_str("one", plat->stringarray[0]); + ut_asserteq_str("", plat->stringarray[1]); + ut_asserteq_str("", plat->stringarray[2]); + + ut_assertok(uclass_next_device_err(&dev)); + plat = dev_get_platdata(dev); + ut_assert(!plat->boolval); + ut_asserteq_str("spl", plat->stringarray[0]); + + ut_asserteq(-ENODEV, uclass_next_device_err(&dev)); + + return 0; +} +DM_TEST(dm_test_of_platdata_props, UT_TESTF_SCAN_PDATA); From 36af37b9367677f16b09c7d57fb84674979a7a2b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:31 -0600 Subject: [PATCH 31/71] dm: test: Add a check that all devices have a dev value With of-platdata, the driver_info struct is updated with the device pointer when it is bound. This makes it easy for a device to be found by its driver info with the device_get_by_driver_info() function. Add a test that all devices (except the root device) have such an entry. Fix a bug that the function does not set *devp to NULL on failure, which the documentation asserts. Signed-off-by: Simon Glass --- drivers/core/device.c | 1 + test/dm/of_platdata.c | 81 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/drivers/core/device.c b/drivers/core/device.c index e90d70101c..746c619cd9 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -767,6 +767,7 @@ int device_get_by_driver_info(const struct driver_info *info, struct udevice *dev; dev = info->dev; + *devp = NULL; return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); } diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c index 8763005ea9..80900e416b 100644 --- a/test/dm/of_platdata.c +++ b/test/dm/of_platdata.c @@ -86,3 +86,84 @@ static int dm_test_of_platdata_props(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_of_platdata_props, UT_TESTF_SCAN_PDATA); + +/* + * find_driver_info - recursively find the driver_info for a device + * + * This sets found[idx] to true when it finds the driver_info record for a + * device, where idx is the index in the driver_info linker list. + * + * @uts: Test state + * @parent: Parent to search + * @found: bool array to update + * @return 0 if OK, non-zero on error + */ +static int find_driver_info(struct unit_test_state *uts, struct udevice *parent, + bool found[]) +{ + struct udevice *dev; + + /* If not the root device, find the entry that caused it to be bound */ + if (parent->parent) { + const struct driver_info *info = + ll_entry_start(struct driver_info, driver_info); + const int n_ents = + ll_entry_count(struct driver_info, driver_info); + const struct driver_info *entry; + int idx = -1; + + for (entry = info; entry != info + n_ents; entry++) { + if (entry->dev == parent) { + idx = entry - info; + found[idx] = true; + break; + } + } + + ut_assert(idx != -1); + } + + device_foreach_child(dev, parent) { + int ret; + + ret = find_driver_info(uts, dev, found); + if (ret < 0) + return ret; + } + + return 0; +} + +/* Check that every device is recorded in its driver_info struct */ +static int dm_test_of_platdata_dev(struct unit_test_state *uts) +{ + const struct driver_info *info = + ll_entry_start(struct driver_info, driver_info); + const int n_ents = ll_entry_count(struct driver_info, driver_info); + bool found[n_ents]; + uint i; + + /* Record the indexes that are found */ + memset(found, '\0', sizeof(found)); + ut_assertok(find_driver_info(uts, gd->dm_root, found)); + + /* Make sure that the driver entries without devices have no ->dev */ + for (i = 0; i < n_ents; i++) { + const struct driver_info *entry = info + i; + struct udevice *dev; + + if (found[i]) { + /* Make sure we can find it */ + ut_assertnonnull(entry->dev); + ut_assertok(device_get_by_driver_info(entry, &dev)); + ut_asserteq_ptr(dev, entry->dev); + } else { + ut_assertnull(entry->dev); + ut_asserteq(-ENOENT, + device_get_by_driver_info(entry, &dev)); + } + } + + return 0; +} +DM_TEST(dm_test_of_platdata_dev, UT_TESTF_SCAN_PDATA); From 88280529bddf0bd05c90db42b6c8e48de954cf66 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:32 -0600 Subject: [PATCH 32/71] dm: test: Add a test for of-platdata phandles We have a test in dtoc for this feature, but not one in U-Boot itself. Add a simple test that checks that the information comes through correctly. Signed-off-by: Simon Glass --- arch/sandbox/dts/sandbox.dtsi | 26 ++++++++++++++++++++++++ configs/sandbox_spl_defconfig | 1 + drivers/clk/clk_fixed_rate.c | 4 ++-- drivers/clk/clk_sandbox.c | 4 ++-- test/dm/of_platdata.c | 37 +++++++++++++++++++++++++++++++++++ 5 files changed, 68 insertions(+), 4 deletions(-) diff --git a/arch/sandbox/dts/sandbox.dtsi b/arch/sandbox/dts/sandbox.dtsi index 0faad3f319..6a0338b212 100644 --- a/arch/sandbox/dts/sandbox.dtsi +++ b/arch/sandbox/dts/sandbox.dtsi @@ -29,6 +29,32 @@ }; }; + clk_fixed: clk-fixed { + u-boot,dm-pre-reloc; + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <1234>; + }; + + clk_sandbox: clk-sbox { + u-boot,dm-pre-reloc; + compatible = "sandbox,clk"; + #clock-cells = <1>; + assigned-clocks = <&clk_sandbox 3>; + assigned-clock-rates = <321>; + }; + + clk-test { + u-boot,dm-pre-reloc; + compatible = "sandbox,clk-test"; + clocks = <&clk_fixed>, + <&clk_sandbox 1>, + <&clk_sandbox 0>, + <&clk_sandbox 3>, + <&clk_sandbox 2>; + clock-names = "fixed", "i2c", "spi", "uart2", "uart1"; + }; + gpio_a: gpios@0 { u-boot,dm-pre-reloc; gpio-controller; diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig index f59e4f2e68..db723cdf98 100644 --- a/configs/sandbox_spl_defconfig +++ b/configs/sandbox_spl_defconfig @@ -104,6 +104,7 @@ CONFIG_ADC_SANDBOX=y CONFIG_AXI=y CONFIG_AXI_SANDBOX=y CONFIG_CLK=y +CONFIG_SPL_CLK=y CONFIG_CPU=y CONFIG_DM_DEMO=y CONFIG_DM_DEMO_SIMPLE=y diff --git a/drivers/clk/clk_fixed_rate.c b/drivers/clk/clk_fixed_rate.c index 55e1f8caa5..f86b4a0e92 100644 --- a/drivers/clk/clk_fixed_rate.c +++ b/drivers/clk/clk_fixed_rate.c @@ -46,8 +46,8 @@ static const struct udevice_id clk_fixed_rate_match[] = { { /* sentinel */ } }; -U_BOOT_DRIVER(clk_fixed_rate) = { - .name = "fixed_rate_clock", +U_BOOT_DRIVER(fixed_clock) = { + .name = "fixed_clock", .id = UCLASS_CLK, .of_match = clk_fixed_rate_match, .ofdata_to_platdata = clk_fixed_rate_ofdata_to_platdata, diff --git a/drivers/clk/clk_sandbox.c b/drivers/clk/clk_sandbox.c index 768fbb7c52..0ff1b49633 100644 --- a/drivers/clk/clk_sandbox.c +++ b/drivers/clk/clk_sandbox.c @@ -124,8 +124,8 @@ static const struct udevice_id sandbox_clk_ids[] = { { } }; -U_BOOT_DRIVER(clk_sandbox) = { - .name = "clk_sandbox", +U_BOOT_DRIVER(sandbox_clk) = { + .name = "sandbox_clk", .id = UCLASS_CLK, .of_match = sandbox_clk_ids, .ops = &sandbox_clk_ops, diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c index 80900e416b..57f903611a 100644 --- a/test/dm/of_platdata.c +++ b/test/dm/of_platdata.c @@ -26,7 +26,11 @@ static int dm_test_of_platdata_props(struct unit_test_state *uts) struct udevice *dev; int i; + /* Skip the clock */ ut_assertok(uclass_first_device_err(UCLASS_MISC, &dev)); + ut_asserteq_str("sandbox_clk_test", dev->name); + + ut_assertok(uclass_next_device_err(&dev)); plat = dev_get_platdata(dev); ut_assert(plat->boolval); ut_asserteq(1, plat->intval); @@ -167,3 +171,36 @@ static int dm_test_of_platdata_dev(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_of_platdata_dev, UT_TESTF_SCAN_PDATA); + +/* Test handling of phandles that point to other devices */ +static int dm_test_of_platdata_phandle(struct unit_test_state *uts) +{ + struct dtd_sandbox_clk_test *plat; + struct udevice *dev, *clk; + + ut_assertok(uclass_first_device_err(UCLASS_MISC, &dev)); + ut_asserteq_str("sandbox_clk_test", dev->name); + plat = dev_get_platdata(dev); + + ut_assertok(device_get_by_driver_info(plat->clocks[0].node, &clk)); + ut_asserteq_str("fixed_clock", clk->name); + + ut_assertok(device_get_by_driver_info(plat->clocks[1].node, &clk)); + ut_asserteq_str("sandbox_clk", clk->name); + ut_asserteq(1, plat->clocks[1].arg[0]); + + ut_assertok(device_get_by_driver_info(plat->clocks[2].node, &clk)); + ut_asserteq_str("sandbox_clk", clk->name); + ut_asserteq(0, plat->clocks[2].arg[0]); + + ut_assertok(device_get_by_driver_info(plat->clocks[3].node, &clk)); + ut_asserteq_str("sandbox_clk", clk->name); + ut_asserteq(3, plat->clocks[3].arg[0]); + + ut_assertok(device_get_by_driver_info(plat->clocks[4].node, &clk)); + ut_asserteq_str("sandbox_clk", clk->name); + ut_asserteq(2, plat->clocks[4].arg[0]); + + return 0; +} +DM_TEST(dm_test_of_platdata_phandle, UT_TESTF_SCAN_PDATA); From a294ead8d2531a641f87bf182fee257029973ac0 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:33 -0600 Subject: [PATCH 33/71] dm: Use an allocated array for run-time device info At present we update the driver_info struct with a pointer to the device that it created (i.e. caused to be bound). This works fine when U-Boot SPL is stored in read-write memory. But on some platforms, such as Intel Apollo Lake, it is not possible to update the data memory. In any case, it is bad form to put this information in a structure that is in the data region, since it expands the size of the binary. Create a new driver_rt structure which holds runtime information about drivers. Update the code to store the device pointer in this instead. Also update the test check that this works. Signed-off-by: Simon Glass --- drivers/core/device.c | 11 ++++++----- drivers/core/lists.c | 16 +++++++++++----- drivers/core/root.c | 11 +++++++++++ include/asm-generic/global_data.h | 14 ++++++++++++++ include/dm/device-internal.h | 2 +- include/dm/platdata.h | 14 ++++++++++++-- test/dm/of_platdata.c | 19 ++++++++++--------- 7 files changed, 65 insertions(+), 22 deletions(-) diff --git a/drivers/core/device.c b/drivers/core/device.c index 746c619cd9..2e5767ed99 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -249,7 +249,7 @@ int device_bind_ofnode(struct udevice *parent, const struct driver *drv, } int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, - struct driver_info *info, struct udevice **devp) + const struct driver_info *info, struct udevice **devp) { struct driver *drv; uint platdata_size = 0; @@ -269,9 +269,6 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, platdata_size, devp); if (ret) return ret; -#if CONFIG_IS_ENABLED(OF_PLATDATA) - info->dev = *devp; -#endif return ret; } @@ -764,9 +761,13 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp) int device_get_by_driver_info(const struct driver_info *info, struct udevice **devp) { + struct driver_info *info_base = + ll_entry_start(struct driver_info, driver_info); + int idx = info - info_base; + struct driver_rt *drt = gd_dm_driver_rt() + idx; struct udevice *dev; - dev = info->dev; + dev = drt->dev; *devp = NULL; return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); diff --git a/drivers/core/lists.c b/drivers/core/lists.c index 5beba9181c..2e6bd5006c 100644 --- a/drivers/core/lists.c +++ b/drivers/core/lists.c @@ -56,14 +56,20 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only) struct driver_info *info = ll_entry_start(struct driver_info, driver_info); const int n_ents = ll_entry_count(struct driver_info, driver_info); - struct driver_info *entry; - struct udevice *dev; int result = 0; - int ret; + uint idx; + + for (idx = 0; idx < n_ents; idx++) { + const struct driver_info *entry = info + idx; + struct driver_rt *drt = gd_dm_driver_rt() + idx; + struct udevice *dev; + int ret; - for (entry = info; entry != info + n_ents; entry++) { ret = device_bind_by_name(parent, pre_reloc_only, entry, &dev); - if (ret && ret != -EPERM) { + if (!ret) { + if (CONFIG_IS_ENABLED(OF_PLATDATA)) + drt->dev = dev; + } else if (ret != -EPERM) { dm_warn("No match for driver '%s'\n", entry->name); if (!result || ret != -ENOENT) result = ret; diff --git a/drivers/core/root.c b/drivers/core/root.c index de23161cff..e8df5aebe8 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -185,6 +185,17 @@ int dm_scan_platdata(bool pre_reloc_only) { int ret; + if (CONFIG_IS_ENABLED(OF_PLATDATA)) { + struct driver_rt *dyn; + int n_ents; + + n_ents = ll_entry_count(struct driver_info, driver_info); + dyn = calloc(n_ents, sizeof(struct driver_rt)); + if (!dyn) + return -ENOMEM; + gd_set_dm_driver_rt(dyn); + } + ret = lists_bind_drivers(DM_ROOT_NON_CONST, pre_reloc_only); if (ret == -ENOENT) { dm_warn("Some drivers were not found\n"); diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index cadfc05dd7..0aa1144184 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -24,6 +24,8 @@ #include #include +struct driver_rt; + typedef struct global_data gd_t; /** @@ -192,6 +194,10 @@ struct global_data { * @uclass_root: head of core tree */ struct list_head uclass_root; +# if CONFIG_IS_ENABLED(OF_PLATDATA) + /** Dynamic info about the driver */ + struct driver_rt *dm_driver_rt; +# endif #endif #ifdef CONFIG_TIMER /** @@ -438,6 +444,14 @@ struct global_data { #define gd_set_of_root(_root) #endif +#if CONFIG_IS_ENABLED(OF_PLATDATA) +#define gd_set_dm_driver_rt(dyn) gd->dm_driver_rt = dyn +#define gd_dm_driver_rt() gd->dm_driver_rt +#else +#define gd_set_dm_driver_rt(dyn) +#define gd_dm_driver_rt() NULL +#endif + /** * enum gd_flags - global data flags * diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h index 1dcc22f689..c5d7ec0650 100644 --- a/include/dm/device-internal.h +++ b/include/dm/device-internal.h @@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent, * @return 0 if OK, -ve on error */ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, - struct driver_info *info, struct udevice **devp); + const struct driver_info *info, struct udevice **devp); /** * device_reparent: reparent the device to a new parent diff --git a/include/dm/platdata.h b/include/dm/platdata.h index 25479b03d2..2c3cc90c29 100644 --- a/include/dm/platdata.h +++ b/include/dm/platdata.h @@ -22,17 +22,27 @@ * @name: Driver name * @platdata: Driver-specific platform data * @platdata_size: Size of platform data structure - * @dev: Device created from this structure data */ struct driver_info { const char *name; const void *platdata; #if CONFIG_IS_ENABLED(OF_PLATDATA) uint platdata_size; - struct udevice *dev; #endif }; +/** + * driver_rt - runtime information set up by U-Boot + * + * There is one of these for every driver_info in the linker list, indexed by + * the driver_info idx value. + * + * @dev: Device created from this idx + */ +struct driver_rt { + struct udevice *dev; +}; + /** * NOTE: Avoid using these except in extreme circumstances, where device tree * is not feasible (e.g. serial driver in SPL where <8KB of SRAM is diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c index 57f903611a..e827d45ffb 100644 --- a/test/dm/of_platdata.c +++ b/test/dm/of_platdata.c @@ -109,16 +109,16 @@ static int find_driver_info(struct unit_test_state *uts, struct udevice *parent, /* If not the root device, find the entry that caused it to be bound */ if (parent->parent) { - const struct driver_info *info = - ll_entry_start(struct driver_info, driver_info); const int n_ents = ll_entry_count(struct driver_info, driver_info); - const struct driver_info *entry; int idx = -1; + int i; - for (entry = info; entry != info + n_ents; entry++) { - if (entry->dev == parent) { - idx = entry - info; + for (i = 0; i < n_ents; i++) { + const struct driver_rt *drt = gd_dm_driver_rt() + i; + + if (drt->dev == parent) { + idx = i; found[idx] = true; break; } @@ -153,16 +153,17 @@ static int dm_test_of_platdata_dev(struct unit_test_state *uts) /* Make sure that the driver entries without devices have no ->dev */ for (i = 0; i < n_ents; i++) { + const struct driver_rt *drt = gd_dm_driver_rt() + i; const struct driver_info *entry = info + i; struct udevice *dev; if (found[i]) { /* Make sure we can find it */ - ut_assertnonnull(entry->dev); + ut_assertnonnull(drt->dev); ut_assertok(device_get_by_driver_info(entry, &dev)); - ut_asserteq_ptr(dev, entry->dev); + ut_asserteq_ptr(dev, drt->dev); } else { - ut_assertnull(entry->dev); + ut_assertnull(drt->dev); ut_asserteq(-ENOENT, device_get_by_driver_info(entry, &dev)); } From 67507e4aab5b58c62cd57b855ce97a83d882479c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:34 -0600 Subject: [PATCH 34/71] sandbox: Fix up building for of-platdata There is no devicetree with of-platdata. Update a few uclasses to allow them to be built for sandbox_spl. Also drop the i2c-gpio from SPL to avoid build errors, since it does not support of-platdata. Signed-off-by: Simon Glass --- drivers/i2c/Makefile | 2 +- drivers/i2c/i2c-emul-uclass.c | 2 ++ drivers/rtc/rtc-uclass.c | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index bd248cbf52..b37198036c 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -6,7 +6,7 @@ obj-$(CONFIG_DM_I2C) += i2c-uclass.o ifdef CONFIG_ACPIGEN obj-$(CONFIG_DM_I2C) += acpi_i2c.o endif -obj-$(CONFIG_DM_I2C_GPIO) += i2c-gpio.o +obj-$(CONFIG_$(SPL_)DM_I2C_GPIO) += i2c-gpio.o obj-$(CONFIG_$(SPL_)I2C_CROS_EC_TUNNEL) += cros_ec_tunnel.o obj-$(CONFIG_$(SPL_)I2C_CROS_EC_LDO) += cros_ec_ldo.o diff --git a/drivers/i2c/i2c-emul-uclass.c b/drivers/i2c/i2c-emul-uclass.c index 1b70e14054..84b6a219d1 100644 --- a/drivers/i2c/i2c-emul-uclass.c +++ b/drivers/i2c/i2c-emul-uclass.c @@ -76,7 +76,9 @@ UCLASS_DRIVER(i2c_emul) = { UCLASS_DRIVER(i2c_emul_parent) = { .id = UCLASS_I2C_EMUL_PARENT, .name = "i2c_emul_parent", +#if !CONFIG_IS_ENABLED(OF_PLATDATA) .post_bind = dm_scan_fdt_dev, +#endif }; static const struct udevice_id i2c_emul_parent_ids[] = { diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c index 8035f7fe9c..b406bab32d 100644 --- a/drivers/rtc/rtc-uclass.c +++ b/drivers/rtc/rtc-uclass.c @@ -174,5 +174,7 @@ int rtc_write32(struct udevice *dev, unsigned int reg, u32 value) UCLASS_DRIVER(rtc) = { .name = "rtc", .id = UCLASS_RTC, +#if !CONFIG_IS_ENABLED(OF_PLATDATA) .post_bind = dm_scan_fdt_dev, +#endif }; From e41651fffda7da55f6d74afdf4b784088184c543 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:35 -0600 Subject: [PATCH 35/71] dm: Support parent devices with of-platdata At present of-platdata does not provide parent information. But this is useful for I2C devices, for example, since it allows them to determine which bus they are on. Add support for setting the parent correctly, by storing the parent driver_info index in dtoc and reading this in lists_bind_drivers(). This needs multiple passes since we must process children after their parents already have been bound. Signed-off-by: Simon Glass --- drivers/core/lists.c | 54 ++++++++++++++++++++++++++++++++++++-- dts/Kconfig | 18 +++++++++++++ include/dm/platdata.h | 10 ++++++- tools/dtoc/dtb_platdata.py | 4 +++ tools/dtoc/test_dtoc.py | 33 +++++++++++++++++++++++ 5 files changed, 116 insertions(+), 3 deletions(-) diff --git a/drivers/core/lists.c b/drivers/core/lists.c index 2e6bd5006c..b23ee3030e 100644 --- a/drivers/core/lists.c +++ b/drivers/core/lists.c @@ -51,21 +51,48 @@ struct uclass_driver *lists_uclass_lookup(enum uclass_id id) return NULL; } -int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only) +static int bind_drivers_pass(struct udevice *parent, bool pre_reloc_only) { struct driver_info *info = ll_entry_start(struct driver_info, driver_info); const int n_ents = ll_entry_count(struct driver_info, driver_info); + bool missing_parent = false; int result = 0; uint idx; + /* + * Do one iteration through the driver_info records. For of-platdata, + * bind only devices whose parent is already bound. If we find any + * device we can't bind, set missing_parent to true, which will cause + * this function to be called again. + */ for (idx = 0; idx < n_ents; idx++) { + struct udevice *par = parent; const struct driver_info *entry = info + idx; struct driver_rt *drt = gd_dm_driver_rt() + idx; struct udevice *dev; int ret; - ret = device_bind_by_name(parent, pre_reloc_only, entry, &dev); + if (CONFIG_IS_ENABLED(OF_PLATDATA)) { + int parent_idx = driver_info_parent_id(entry); + + if (drt->dev) + continue; + + if (CONFIG_IS_ENABLED(OF_PLATDATA_PARENT) && + parent_idx != -1) { + struct driver_rt *parent_drt; + + parent_drt = gd_dm_driver_rt() + parent_idx; + if (!parent_drt->dev) { + missing_parent = true; + continue; + } + + par = parent_drt->dev; + } + } + ret = device_bind_by_name(par, pre_reloc_only, entry, &dev); if (!ret) { if (CONFIG_IS_ENABLED(OF_PLATDATA)) drt->dev = dev; @@ -76,6 +103,29 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only) } } + return result ? result : missing_parent ? -EAGAIN : 0; +} + +int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only) +{ + int result = 0; + int pass; + + /* + * 10 passes is 10 levels deep in the devicetree, which is plenty. If + * OF_PLATDATA_PARENT is not enabled, then bind_drivers_pass() will + * always succeed on the first pass. + */ + for (pass = 0; pass < 10; pass++) { + int ret; + + ret = bind_drivers_pass(parent, pre_reloc_only); + if (!ret) + break; + if (ret != -EAGAIN && !result) + result = ret; + } + return result; } diff --git a/dts/Kconfig b/dts/Kconfig index 86ea8ce887..aeda542f98 100644 --- a/dts/Kconfig +++ b/dts/Kconfig @@ -355,6 +355,15 @@ config SPL_OF_PLATDATA compatible string, then adding platform data and U_BOOT_DEVICE declarations for each node. See of-plat.txt for more information. +config SPL_OF_PLATDATA_PARENT + bool "Support parent information in devices" + depends on SPL_OF_PLATDATA + default y + help + Generally it is useful to be able to access the parent of a device + with of-platdata. To save space this can be disabled, but in that + case dev_get_parent() will always return NULL; + config TPL_OF_PLATDATA bool "Generate platform data for use in TPL" depends on TPL_OF_CONTROL @@ -376,4 +385,13 @@ config TPL_OF_PLATDATA compatible string, then adding platform data and U_BOOT_DEVICE declarations for each node. See of-plat.txt for more information. +config TPL_OF_PLATDATA_PARENT + bool "Support parent information in devices" + depends on TPL_OF_PLATDATA + default y + help + Generally it is useful to be able to access the parent of a device + with of-platdata. To save space this can be disabled, but in that + case dev_get_parent() will always return NULL; + endmenu diff --git a/include/dm/platdata.h b/include/dm/platdata.h index 2c3cc90c29..f800a866dd 100644 --- a/include/dm/platdata.h +++ b/include/dm/platdata.h @@ -22,15 +22,23 @@ * @name: Driver name * @platdata: Driver-specific platform data * @platdata_size: Size of platform data structure + * @parent_idx: Index of the parent driver_info structure */ struct driver_info { const char *name; const void *platdata; #if CONFIG_IS_ENABLED(OF_PLATDATA) - uint platdata_size; + unsigned short platdata_size; + short parent_idx; #endif }; +#if CONFIG_IS_ENABLED(OF_PLATDATA) +#define driver_info_parent_id(driver_info) driver_info->parent_idx +#else +#define driver_info_parent_id(driver_info) (-1) +#endif + /** * driver_rt - runtime information set up by U-Boot * diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py index 31a9b3877e..8832e6ebec 100644 --- a/tools/dtoc/dtb_platdata.py +++ b/tools/dtoc/dtb_platdata.py @@ -662,6 +662,10 @@ class DtbPlatdata(object): self.buf('\t.name\t\t= "%s",\n' % struct_name) self.buf('\t.platdata\t= &%s%s,\n' % (VAL_PREFIX, var_name)) self.buf('\t.platdata_size\t= sizeof(%s%s),\n' % (VAL_PREFIX, var_name)) + idx = -1 + if node.parent and node.parent in self._valid_nodes: + idx = node.parent.idx + self.buf('\t.parent_idx\t= %d,\n' % idx) self.buf('};\n') self.buf('\n') diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index 8dcac91ee7..fee9853d03 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -216,6 +216,7 @@ U_BOOT_DEVICE(i2c_at_0) = { \t.name\t\t= "sandbox_i2c_test", \t.platdata\t= &dtv_i2c_at_0, \t.platdata_size\t= sizeof(dtv_i2c_at_0), +\t.parent_idx\t= -1, }; /* Node /i2c@0/pmic@9 index 1 */ @@ -227,6 +228,7 @@ U_BOOT_DEVICE(pmic_at_9) = { \t.name\t\t= "sandbox_pmic_test", \t.platdata\t= &dtv_pmic_at_9, \t.platdata_size\t= sizeof(dtv_pmic_at_9), +\t.parent_idx\t= 0, }; /* Node /spl-test index 2 */ @@ -246,6 +248,7 @@ U_BOOT_DEVICE(spl_test) = { \t.name\t\t= "sandbox_spl_test", \t.platdata\t= &dtv_spl_test, \t.platdata_size\t= sizeof(dtv_spl_test), +\t.parent_idx\t= -1, }; /* Node /spl-test2 index 3 */ @@ -264,6 +267,7 @@ U_BOOT_DEVICE(spl_test2) = { \t.name\t\t= "sandbox_spl_test", \t.platdata\t= &dtv_spl_test2, \t.platdata_size\t= sizeof(dtv_spl_test2), +\t.parent_idx\t= -1, }; /* Node /spl-test3 index 4 */ @@ -276,6 +280,7 @@ U_BOOT_DEVICE(spl_test3) = { \t.name\t\t= "sandbox_spl_test", \t.platdata\t= &dtv_spl_test3, \t.platdata_size\t= sizeof(dtv_spl_test3), +\t.parent_idx\t= -1, }; /* Node /spl-test4 index 5 */ @@ -285,6 +290,7 @@ U_BOOT_DEVICE(spl_test4) = { \t.name\t\t= "sandbox_spl_test_2", \t.platdata\t= &dtv_spl_test4, \t.platdata_size\t= sizeof(dtv_spl_test4), +\t.parent_idx\t= -1, }; ''' + C_EMPTY_POPULATE_PHANDLE_DATA, data) @@ -318,6 +324,7 @@ U_BOOT_DEVICE(gpios_at_0) = { \t.name\t\t= "sandbox_gpio", \t.platdata\t= &dtv_gpios_at_0, \t.platdata_size\t= sizeof(dtv_gpios_at_0), +\t.parent_idx\t= -1, }; void dm_populate_phandle_data(void) { @@ -349,6 +356,7 @@ U_BOOT_DEVICE(spl_test) = { \t.name\t\t= "invalid", \t.platdata\t= &dtv_spl_test, \t.platdata_size\t= sizeof(dtv_spl_test), +\t.parent_idx\t= -1, }; void dm_populate_phandle_data(void) { @@ -383,6 +391,7 @@ U_BOOT_DEVICE(phandle2_target) = { \t.name\t\t= "target", \t.platdata\t= &dtv_phandle2_target, \t.platdata_size\t= sizeof(dtv_phandle2_target), +\t.parent_idx\t= -1, }; /* Node /phandle3-target index 1 */ @@ -393,6 +402,7 @@ U_BOOT_DEVICE(phandle3_target) = { \t.name\t\t= "target", \t.platdata\t= &dtv_phandle3_target, \t.platdata_size\t= sizeof(dtv_phandle3_target), +\t.parent_idx\t= -1, }; /* Node /phandle-target index 4 */ @@ -403,6 +413,7 @@ U_BOOT_DEVICE(phandle_target) = { \t.name\t\t= "target", \t.platdata\t= &dtv_phandle_target, \t.platdata_size\t= sizeof(dtv_phandle_target), +\t.parent_idx\t= -1, }; /* Node /phandle-source index 2 */ @@ -417,6 +428,7 @@ U_BOOT_DEVICE(phandle_source) = { \t.name\t\t= "source", \t.platdata\t= &dtv_phandle_source, \t.platdata_size\t= sizeof(dtv_phandle_source), +\t.parent_idx\t= -1, }; /* Node /phandle-source2 index 3 */ @@ -428,6 +440,7 @@ U_BOOT_DEVICE(phandle_source2) = { \t.name\t\t= "source", \t.platdata\t= &dtv_phandle_source2, \t.platdata_size\t= sizeof(dtv_phandle_source2), +\t.parent_idx\t= -1, }; void dm_populate_phandle_data(void) { @@ -470,6 +483,7 @@ U_BOOT_DEVICE(phandle_target) = { \t.name\t\t= "target", \t.platdata\t= &dtv_phandle_target, \t.platdata_size\t= sizeof(dtv_phandle_target), +\t.parent_idx\t= -1, }; /* Node /phandle-source2 index 0 */ @@ -481,6 +495,7 @@ U_BOOT_DEVICE(phandle_source2) = { \t.name\t\t= "source", \t.platdata\t= &dtv_phandle_source2, \t.platdata_size\t= sizeof(dtv_phandle_source2), +\t.parent_idx\t= -1, }; void dm_populate_phandle_data(void) { @@ -504,6 +519,7 @@ U_BOOT_DEVICE(phandle2_target) = { \t.name\t\t= "target", \t.platdata\t= &dtv_phandle2_target, \t.platdata_size\t= sizeof(dtv_phandle2_target), +\t.parent_idx\t= -1, }; /* Node /phandle3-target index 1 */ @@ -514,6 +530,7 @@ U_BOOT_DEVICE(phandle3_target) = { \t.name\t\t= "target", \t.platdata\t= &dtv_phandle3_target, \t.platdata_size\t= sizeof(dtv_phandle3_target), +\t.parent_idx\t= -1, }; /* Node /phandle-target index 4 */ @@ -524,6 +541,7 @@ U_BOOT_DEVICE(phandle_target) = { \t.name\t\t= "target", \t.platdata\t= &dtv_phandle_target, \t.platdata_size\t= sizeof(dtv_phandle_target), +\t.parent_idx\t= -1, }; /* Node /phandle-source index 2 */ @@ -538,6 +556,7 @@ U_BOOT_DEVICE(phandle_source) = { \t.name\t\t= "source", \t.platdata\t= &dtv_phandle_source, \t.platdata_size\t= sizeof(dtv_phandle_source), +\t.parent_idx\t= -1, }; /* Node /phandle-source2 index 3 */ @@ -549,6 +568,7 @@ U_BOOT_DEVICE(phandle_source2) = { \t.name\t\t= "source", \t.platdata\t= &dtv_phandle_source2, \t.platdata_size\t= sizeof(dtv_phandle_source2), +\t.parent_idx\t= -1, }; void dm_populate_phandle_data(void) { @@ -611,6 +631,7 @@ U_BOOT_DEVICE(test1) = { \t.name\t\t= "test1", \t.platdata\t= &dtv_test1, \t.platdata_size\t= sizeof(dtv_test1), +\t.parent_idx\t= -1, }; /* Node /test2 index 1 */ @@ -621,6 +642,7 @@ U_BOOT_DEVICE(test2) = { \t.name\t\t= "test2", \t.platdata\t= &dtv_test2, \t.platdata_size\t= sizeof(dtv_test2), +\t.parent_idx\t= -1, }; /* Node /test3 index 2 */ @@ -631,6 +653,7 @@ U_BOOT_DEVICE(test3) = { \t.name\t\t= "test3", \t.platdata\t= &dtv_test3, \t.platdata_size\t= sizeof(dtv_test3), +\t.parent_idx\t= -1, }; ''' + C_EMPTY_POPULATE_PHANDLE_DATA, data) @@ -663,6 +686,7 @@ U_BOOT_DEVICE(test1) = { \t.name\t\t= "test1", \t.platdata\t= &dtv_test1, \t.platdata_size\t= sizeof(dtv_test1), +\t.parent_idx\t= -1, }; /* Node /test2 index 1 */ @@ -673,6 +697,7 @@ U_BOOT_DEVICE(test2) = { \t.name\t\t= "test2", \t.platdata\t= &dtv_test2, \t.platdata_size\t= sizeof(dtv_test2), +\t.parent_idx\t= -1, }; ''' + C_EMPTY_POPULATE_PHANDLE_DATA, data) @@ -708,6 +733,7 @@ U_BOOT_DEVICE(test1) = { \t.name\t\t= "test1", \t.platdata\t= &dtv_test1, \t.platdata_size\t= sizeof(dtv_test1), +\t.parent_idx\t= -1, }; /* Node /test2 index 1 */ @@ -718,6 +744,7 @@ U_BOOT_DEVICE(test2) = { \t.name\t\t= "test2", \t.platdata\t= &dtv_test2, \t.platdata_size\t= sizeof(dtv_test2), +\t.parent_idx\t= -1, }; /* Node /test3 index 2 */ @@ -728,6 +755,7 @@ U_BOOT_DEVICE(test3) = { \t.name\t\t= "test3", \t.platdata\t= &dtv_test3, \t.platdata_size\t= sizeof(dtv_test3), +\t.parent_idx\t= -1, }; ''' + C_EMPTY_POPULATE_PHANDLE_DATA, data) @@ -763,6 +791,7 @@ U_BOOT_DEVICE(test1) = { \t.name\t\t= "test1", \t.platdata\t= &dtv_test1, \t.platdata_size\t= sizeof(dtv_test1), +\t.parent_idx\t= -1, }; /* Node /test2 index 1 */ @@ -773,6 +802,7 @@ U_BOOT_DEVICE(test2) = { \t.name\t\t= "test2", \t.platdata\t= &dtv_test2, \t.platdata_size\t= sizeof(dtv_test2), +\t.parent_idx\t= -1, }; /* Node /test3 index 2 */ @@ -783,6 +813,7 @@ U_BOOT_DEVICE(test3) = { \t.name\t\t= "test3", \t.platdata\t= &dtv_test3, \t.platdata_size\t= sizeof(dtv_test3), +\t.parent_idx\t= -1, }; ''' + C_EMPTY_POPULATE_PHANDLE_DATA, data) @@ -833,6 +864,7 @@ U_BOOT_DEVICE(spl_test) = { \t.name\t\t= "sandbox_spl_test", \t.platdata\t= &dtv_spl_test, \t.platdata_size\t= sizeof(dtv_spl_test), +\t.parent_idx\t= -1, }; /* Node /spl-test2 index 1 */ @@ -843,6 +875,7 @@ U_BOOT_DEVICE(spl_test2) = { \t.name\t\t= "sandbox_spl_test", \t.platdata\t= &dtv_spl_test2, \t.platdata_size\t= sizeof(dtv_spl_test2), +\t.parent_idx\t= -1, }; ''' + C_EMPTY_POPULATE_PHANDLE_DATA, data) From fbe27a54ebbe7433bbccf242f4edda61e2c1ba3e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:36 -0600 Subject: [PATCH 36/71] dm: Add a test for of-platdata parent information Add a simple test that we can obtain the correct parent for an I2C device. This requires updating the driver names to match the compatible strings, adding them to the devicetree and enabling a few options. Signed-off-by: Simon Glass --- arch/sandbox/dts/sandbox.dts | 1 + arch/sandbox/dts/sandbox.dtsi | 1 + configs/sandbox_spl_defconfig | 4 +++- drivers/i2c/sandbox_i2c.c | 4 ++-- drivers/rtc/sandbox_rtc.c | 4 ++-- test/dm/of_platdata.c | 15 +++++++++++++++ 6 files changed, 24 insertions(+), 5 deletions(-) diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts index 20f6893829..8b50a40289 100644 --- a/arch/sandbox/dts/sandbox.dts +++ b/arch/sandbox/dts/sandbox.dts @@ -69,6 +69,7 @@ clock-frequency = <400000>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_i2c0>; + u-boot,dm-pre-reloc; }; pcic: pci@0 { diff --git a/arch/sandbox/dts/sandbox.dtsi b/arch/sandbox/dts/sandbox.dtsi index 6a0338b212..81cdc55b0d 100644 --- a/arch/sandbox/dts/sandbox.dtsi +++ b/arch/sandbox/dts/sandbox.dtsi @@ -90,6 +90,7 @@ reg = <0x43>; compatible = "sandbox-rtc"; sandbox,emul = <&emul0>; + u-boot,dm-pre-reloc; }; sandbox_pmic: sandbox_pmic { reg = <0x40>; diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig index db723cdf98..d47a6252de 100644 --- a/configs/sandbox_spl_defconfig +++ b/configs/sandbox_spl_defconfig @@ -26,6 +26,8 @@ CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_HANDOFF=y CONFIG_SPL_BOARD_INIT=y CONFIG_SPL_ENV_SUPPORT=y +CONFIG_SPL_I2C_SUPPORT=y +CONFIG_SPL_RTC_SUPPORT=y CONFIG_CMD_CPU=y CONFIG_CMD_LICENSE=y CONFIG_CMD_BOOTZ=y @@ -120,7 +122,6 @@ CONFIG_I2C_CROS_EC_LDO=y CONFIG_DM_I2C_GPIO=y CONFIG_SYS_I2C_SANDBOX=y CONFIG_I2C_MUX=y -CONFIG_SPL_I2C_MUX=y CONFIG_I2C_ARB_GPIO_CHALLENGE=y CONFIG_CROS_EC_KEYB=y CONFIG_I8042_KEYB=y @@ -187,6 +188,7 @@ CONFIG_REMOTEPROC_SANDBOX=y CONFIG_DM_RESET=y CONFIG_SANDBOX_RESET=y CONFIG_DM_RTC=y +CONFIG_SPL_DM_RTC=y CONFIG_SANDBOX_SERIAL=y CONFIG_SOUND=y CONFIG_SOUND_SANDBOX=y diff --git a/drivers/i2c/sandbox_i2c.c b/drivers/i2c/sandbox_i2c.c index 57b1c60fde..2cbdaf9cc7 100644 --- a/drivers/i2c/sandbox_i2c.c +++ b/drivers/i2c/sandbox_i2c.c @@ -93,8 +93,8 @@ static const struct udevice_id sandbox_i2c_ids[] = { { } }; -U_BOOT_DRIVER(i2c_sandbox) = { - .name = "i2c_sandbox", +U_BOOT_DRIVER(sandbox_i2c) = { + .name = "sandbox_i2c", .id = UCLASS_I2C, .of_match = sandbox_i2c_ids, .ops = &sandbox_i2c_ops, diff --git a/drivers/rtc/sandbox_rtc.c b/drivers/rtc/sandbox_rtc.c index 852770a49c..d0864b1df9 100644 --- a/drivers/rtc/sandbox_rtc.c +++ b/drivers/rtc/sandbox_rtc.c @@ -92,8 +92,8 @@ static const struct udevice_id sandbox_rtc_ids[] = { { } }; -U_BOOT_DRIVER(rtc_sandbox) = { - .name = "rtc-sandbox", +U_BOOT_DRIVER(sandbox_rtc) = { + .name = "sandbox_rtc", .id = UCLASS_RTC, .of_match = sandbox_rtc_ids, .ops = &sandbox_rtc_ops, diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c index e827d45ffb..bad733fbee 100644 --- a/test/dm/of_platdata.c +++ b/test/dm/of_platdata.c @@ -205,3 +205,18 @@ static int dm_test_of_platdata_phandle(struct unit_test_state *uts) return 0; } DM_TEST(dm_test_of_platdata_phandle, UT_TESTF_SCAN_PDATA); + +#if CONFIG_IS_ENABLED(OF_PLATDATA_PARENT) +/* Test that device parents are correctly set up */ +static int dm_test_of_platdata_parent(struct unit_test_state *uts) +{ + struct udevice *rtc, *i2c; + + ut_assertok(uclass_first_device_err(UCLASS_RTC, &rtc)); + ut_assertok(uclass_first_device_err(UCLASS_I2C, &i2c)); + ut_asserteq_ptr(i2c, dev_get_parent(rtc)); + + return 0; +} +DM_TEST(dm_test_of_platdata_parent, UT_TESTF_SCAN_PDATA); +#endif From 9e948b9b99d17ea6c2204a1b6251a5c33ee83dd9 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:37 -0600 Subject: [PATCH 37/71] dm: core: Convert #ifdef to if() in root.c Convert a few conditions to use compile-time checks to reduce the number of build paths. Signed-off-by: Simon Glass --- drivers/core/root.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/core/root.c b/drivers/core/root.c index e8df5aebe8..5f10d7a39c 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -50,7 +50,6 @@ void dm_fixup_for_gd_move(struct global_data *new_gd) } } -#if defined(CONFIG_NEEDS_MANUAL_RELOC) void fix_drivers(void) { struct driver *drv = @@ -129,8 +128,6 @@ void fix_devices(void) } } -#endif - int dm_init(bool of_live) { int ret; @@ -141,11 +138,11 @@ int dm_init(bool of_live) } INIT_LIST_HEAD(&DM_UCLASS_ROOT_NON_CONST); -#if defined(CONFIG_NEEDS_MANUAL_RELOC) - fix_drivers(); - fix_uclass(); - fix_devices(); -#endif + if (IS_ENABLED(CONFIG_NEEDS_MANUAL_RELOC)) { + fix_drivers(); + fix_uclass(); + fix_devices(); + } ret = device_bind_by_name(NULL, false, &root_info, &DM_ROOT_NON_CONST); if (ret) @@ -350,9 +347,8 @@ int dm_init_and_scan(bool pre_reloc_only) { int ret; -#if CONFIG_IS_ENABLED(OF_PLATDATA) - dm_populate_phandle_data(); -#endif + if (CONFIG_IS_ENABLED(OF_PLATDATA)) + dm_populate_phandle_data(); ret = dm_init(CONFIG_IS_ENABLED(OF_LIVE)); if (ret) { From d07f31aea07ec642e8072fe0b83ba56ec83cc561 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:38 -0600 Subject: [PATCH 38/71] x86: apl: Enable SPI flash in TPL with APL_SPI_FLASH_BOOT At present, enabling CONFIG_APL_SPI_FLASH_BOOT does not build since SPI and SPI flash are not enabled for TPL. Add a condition to fix this and tidy up a build warning in the SPI-flash driver. Signed-off-by: Simon Glass --- arch/x86/cpu/apollolake/Kconfig | 2 ++ drivers/spi/ich.c | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/arch/x86/cpu/apollolake/Kconfig b/arch/x86/cpu/apollolake/Kconfig index 35a425cd1b..c6c1350f4f 100644 --- a/arch/x86/cpu/apollolake/Kconfig +++ b/arch/x86/cpu/apollolake/Kconfig @@ -85,6 +85,8 @@ config APL_SPI_FLASH_BOOT bool "Support booting with SPI-flash driver instead memory-mapped SPI" select TPL_SPI_FLASH_SUPPORT select TPL_SPI_SUPPORT + select TPL_DM_SPI + select TPL_DM_SPI_FLASH help This enables SPI and SPI flash in TPL. Without the this only available boot method is to use memory-mapped SPI. Since this is diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index e1336b89c5..a91cb78568 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -615,6 +615,7 @@ static int ich_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) return ret; } +#if !CONFIG_IS_ENABLED(OF_PLATDATA) /** * ich_spi_get_basics() - Get basic information about the ICH device * @@ -657,6 +658,7 @@ static int ich_spi_get_basics(struct udevice *bus, bool can_probe, return ret; } +#endif /** * ich_get_mmap_bus() - Handle the get_mmap() method for a bus @@ -946,10 +948,10 @@ static int ich_spi_child_pre_probe(struct udevice *dev) static int ich_spi_ofdata_to_platdata(struct udevice *dev) { struct ich_spi_platdata *plat = dev_get_platdata(dev); - int ret; #if !CONFIG_IS_ENABLED(OF_PLATDATA) struct ich_spi_priv *priv = dev_get_priv(dev); + int ret; ret = ich_spi_get_basics(dev, true, &priv->pch, &plat->ich_version, &plat->mmio_base); From bb44ebdd0f3eccece2081ec51cf3b3554dafd801 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:39 -0600 Subject: [PATCH 39/71] x86: apl: Take advantage of the of-platdata parent support Now that parent devices are supported with of-platadata, we don't need the messy code to fix up the parent pointers and allocations on Apollo Lake. Put the code behind a condition. Signed-off-by: Simon Glass --- arch/x86/cpu/apollolake/spl.c | 3 ++- drivers/misc/p2sb-uclass.c | 25 +++++++++++++------------ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/arch/x86/cpu/apollolake/spl.c b/arch/x86/cpu/apollolake/spl.c index 5a53831dc6..089b37c59f 100644 --- a/arch/x86/cpu/apollolake/spl.c +++ b/arch/x86/cpu/apollolake/spl.c @@ -90,7 +90,8 @@ static int apl_flash_probe(struct udevice *dev) */ static int apl_flash_bind(struct udevice *dev) { - if (CONFIG_IS_ENABLED(OF_PLATDATA)) { + if (CONFIG_IS_ENABLED(OF_PLATDATA) && + !CONFIG_IS_ENABLED(OF_PLATDATA_PARENT)) { struct dm_spi_slave_platdata *plat; struct udevice *spi; int ret; diff --git a/drivers/misc/p2sb-uclass.c b/drivers/misc/p2sb-uclass.c index b5219df46b..12abcff2da 100644 --- a/drivers/misc/p2sb-uclass.c +++ b/drivers/misc/p2sb-uclass.c @@ -174,19 +174,20 @@ int p2sb_set_port_id(struct udevice *dev, int portid) if (!CONFIG_IS_ENABLED(OF_PLATDATA)) return -ENOSYS; - uclass_find_first_device(UCLASS_P2SB, &ps2b); - if (!ps2b) - return -EDEADLK; - dev->parent = ps2b; + if (!CONFIG_IS_ENABLED(OF_PLATDATA_PARENT)) { + uclass_find_first_device(UCLASS_P2SB, &ps2b); + if (!ps2b) + return -EDEADLK; + dev->parent = ps2b; - /* - * We must allocate this, since when the device was bound it did not - * have a parent. - * TODO(sjg@chromium.org): Add a parent pointer to child devices in dtoc - */ - dev->parent_platdata = malloc(sizeof(*pplat)); - if (!dev->parent_platdata) - return -ENOMEM; + /* + * We must allocate this, since when the device was bound it did + * not have a parent. + */ + dev->parent_platdata = malloc(sizeof(*pplat)); + if (!dev->parent_platdata) + return -ENOMEM; + } pplat = dev_get_parent_platdata(dev); pplat->pid = portid; From 8a38abfc43f94a92b63e428738714111710bda53 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:40 -0600 Subject: [PATCH 40/71] dm: Use driver_info index instead of pointer At present we use a 'node' pointer in the of-platadata phandle_n_arg structs. This is a pointer to the struct driver_info for a particular device, and we can use it to obtain the struct udevice pointer itself. Since we don't know the struct udevice pointer until it is allocated in memory, we have to fix up the phandle_n_arg.node at runtime. This is annoying since it requires that SPL's data is writable and adds a small amount of extra (generated) code in the dm_populate_phandle_data() function. Now that we can find a driver_info by its index, it is easier to put the index in the phandle_n_arg structures. Update dtoc to do this, add a new device_get_by_driver_info_idx() to look up a device by drive_info index and update the tests to match. Signed-off-by: Simon Glass --- drivers/clk/clk-uclass.c | 3 +-- drivers/core/device.c | 11 +++++++++++ drivers/misc/irq-uclass.c | 2 +- drivers/mmc/fsl_esdhc_imx.c | 7 ++----- include/dm/device.h | 14 ++++++++++++++ include/dt-structs.h | 6 +++--- test/dm/of_platdata.c | 10 +++++----- test/dm/test-main.c | 24 ++++++++++++++++++++---- tools/dtoc/dtb_platdata.py | 20 ++++---------------- tools/dtoc/test_dtoc.py | 33 +++++++++++---------------------- 10 files changed, 72 insertions(+), 58 deletions(-) diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 31c5997aea..ac954a34d2 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -38,8 +38,7 @@ int clk_get_by_driver_info(struct udevice *dev, struct phandle_1_arg *cells, { int ret; - ret = device_get_by_driver_info((struct driver_info *)cells->node, - &clk->dev); + ret = device_get_by_driver_info_idx(cells->idx, &clk->dev); if (ret) return ret; clk->id = cells->arg[0]; diff --git a/drivers/core/device.c b/drivers/core/device.c index 2e5767ed99..4b3dcb3b37 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -772,6 +772,17 @@ int device_get_by_driver_info(const struct driver_info *info, return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); } + +int device_get_by_driver_info_idx(uint idx, struct udevice **devp) +{ + struct driver_rt *drt = gd_dm_driver_rt() + idx; + struct udevice *dev; + + dev = drt->dev; + *devp = NULL; + + return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); +} #endif int device_find_first_child(const struct udevice *parent, struct udevice **devp) diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c index 94fa233f19..24b27962a7 100644 --- a/drivers/misc/irq-uclass.c +++ b/drivers/misc/irq-uclass.c @@ -69,7 +69,7 @@ int irq_get_by_driver_info(struct udevice *dev, { int ret; - ret = device_get_by_driver_info(cells->node, &irq->dev); + ret = device_get_by_driver_info_idx(cells->idx, &irq->dev); if (ret) return ret; irq->id = cells->arg[0]; diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 1c015ab764..22040c67a8 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -1504,12 +1504,9 @@ static int fsl_esdhc_probe(struct udevice *dev) if (CONFIG_IS_ENABLED(DM_GPIO) && !priv->non_removable) { struct udevice *gpiodev; - struct driver_info *info; - - info = (struct driver_info *)dtplat->cd_gpios->node; - - ret = device_get_by_driver_info(info, &gpiodev); + ret = device_get_by_driver_info_idx(dtplat->cd_gpios->idx, + &gpiodev); if (ret) return ret; diff --git a/include/dm/device.h b/include/dm/device.h index 993c9e6c5a..5bef484247 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -553,6 +553,20 @@ int device_get_global_by_ofnode(ofnode node, struct udevice **devp); int device_get_by_driver_info(const struct driver_info *info, struct udevice **devp); +/** + * device_get_by_driver_info_idx() - Get a device based on driver_info index + * + * Locates a device by its struct driver_info, by using its index number which + * is written into the idx field of struct phandle_1_arg, etc. + * + * The device is probed to activate it ready for use. + * + * @idx: Index number of the driver_info structure (0=first) + * @devp: Returns pointer to device if found, otherwise this is set to NULL + * @return 0 if OK, -ve on error + */ +int device_get_by_driver_info_idx(uint idx, struct udevice **devp); + /** * device_find_first_child() - Find the first child of a device * diff --git a/include/dt-structs.h b/include/dt-structs.h index eed8273d18..f0e1c9cb90 100644 --- a/include/dt-structs.h +++ b/include/dt-structs.h @@ -11,17 +11,17 @@ struct driver_info; struct phandle_0_arg { - const struct driver_info *node; + uint idx; int arg[0]; }; struct phandle_1_arg { - const struct driver_info *node; + uint idx; int arg[1]; }; struct phandle_2_arg { - const struct driver_info *node; + uint idx; int arg[2]; }; #include diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c index bad733fbee..4f3cc159d0 100644 --- a/test/dm/of_platdata.c +++ b/test/dm/of_platdata.c @@ -183,22 +183,22 @@ static int dm_test_of_platdata_phandle(struct unit_test_state *uts) ut_asserteq_str("sandbox_clk_test", dev->name); plat = dev_get_platdata(dev); - ut_assertok(device_get_by_driver_info(plat->clocks[0].node, &clk)); + ut_assertok(device_get_by_driver_info_idx(plat->clocks[0].idx, &clk)); ut_asserteq_str("fixed_clock", clk->name); - ut_assertok(device_get_by_driver_info(plat->clocks[1].node, &clk)); + ut_assertok(device_get_by_driver_info_idx(plat->clocks[1].idx, &clk)); ut_asserteq_str("sandbox_clk", clk->name); ut_asserteq(1, plat->clocks[1].arg[0]); - ut_assertok(device_get_by_driver_info(plat->clocks[2].node, &clk)); + ut_assertok(device_get_by_driver_info_idx(plat->clocks[2].idx, &clk)); ut_asserteq_str("sandbox_clk", clk->name); ut_asserteq(0, plat->clocks[2].arg[0]); - ut_assertok(device_get_by_driver_info(plat->clocks[3].node, &clk)); + ut_assertok(device_get_by_driver_info_idx(plat->clocks[3].idx, &clk)); ut_asserteq_str("sandbox_clk", clk->name); ut_asserteq(3, plat->clocks[3].arg[0]); - ut_assertok(device_get_by_driver_info(plat->clocks[4].node, &clk)); + ut_assertok(device_get_by_driver_info_idx(plat->clocks[4].idx, &clk)); ut_asserteq_str("sandbox_clk", clk->name); ut_asserteq(2, plat->clocks[4].arg[0]); diff --git a/test/dm/test-main.c b/test/dm/test-main.c index 9d22df8c4d..fd24635006 100644 --- a/test/dm/test-main.c +++ b/test/dm/test-main.c @@ -127,6 +127,24 @@ static bool dm_test_run_on_flattree(struct unit_test *test) return !strstr(fname, "video") || strstr(test->name, "video_base"); } +static bool test_matches(const char *test_name, const char *find_name) +{ + if (!find_name) + return true; + + if (!strcmp(test_name, find_name)) + return true; + + /* All tests have this prefix */ + if (!strncmp(test_name, "dm_test_", 8)) + test_name += 8; + + if (!strcmp(test_name, find_name)) + return true; + + return false; +} + int dm_test_main(const char *test_name) { struct unit_test *tests = ll_entry_start(struct unit_test, dm_test); @@ -152,6 +170,7 @@ int dm_test_main(const char *test_name) if (!test_name) printf("Running %d driver model tests\n", n_ents); + else found = 0; uts->of_root = gd_of_root(); @@ -159,10 +178,7 @@ int dm_test_main(const char *test_name) const char *name = test->name; int runs; - /* All tests have this prefix */ - if (!strncmp(name, "dm_test_", 8)) - name += 8; - if (test_name && strcmp(test_name, name)) + if (!test_matches(name, test_name)) continue; /* Run with the live tree if possible */ diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py index 8832e6ebec..2be11fff6c 100644 --- a/tools/dtoc/dtb_platdata.py +++ b/tools/dtoc/dtb_platdata.py @@ -154,8 +154,6 @@ class DtbPlatdata(object): key: Driver alias declared with U_BOOT_DRIVER_ALIAS(driver_alias, driver_name) value: Driver name declared with U_BOOT_DRIVER(driver_name) - _links: List of links to be included in dm_populate_phandle_data(), - each a PhandleLink _drivers_additional: List of additional drivers to use during scanning """ def __init__(self, dtb_fname, include_disabled, warning_disabled, @@ -169,7 +167,6 @@ class DtbPlatdata(object): self._lines = [] self._drivers = [] self._driver_aliases = {} - self._links = [] self._drivers_additional = drivers_additional def get_normalized_compat_name(self, node): @@ -612,17 +609,11 @@ class DtbPlatdata(object): name = conv_name_to_c(target_node.name) arg_values = [] for i in range(args): - arg_values.append(str(fdt_util.fdt32_to_cpu(prop.value[pos + 1 + i]))) + arg_values.append( + str(fdt_util.fdt32_to_cpu(prop.value[pos + 1 + i]))) pos += 1 + args - # node member is filled with NULL as the real value - # will be update at run-time during dm_init_and_scan() - # by dm_populate_phandle_data() - vals.append('\t{NULL, {%s}}' % (', '.join(arg_values))) - var_node = '%s%s.%s[%d].node' % \ - (VAL_PREFIX, var_name, member_name, item) - # Save the the link information to be use to define - # dm_populate_phandle_data() - self._links.append(PhandleLink(var_node, name)) + vals.append('\t{%d, {%s}}' % (target_node.idx, + ', '.join(arg_values))) item += 1 for val in vals: self.buf('\n\t\t%s,' % val) @@ -703,9 +694,6 @@ class DtbPlatdata(object): # nodes using DM_GET_DEVICE # dtv_dmc_at_xxx.clocks[0].node = DM_GET_DEVICE(clock_controller_at_xxx) self.buf('void dm_populate_phandle_data(void) {\n') - for link in self._links: - self.buf('\t%s = DM_GET_DEVICE(%s);\n' % - (link.var_node, link.dev_name)) self.buf('}\n') self.out(''.join(self.get_buf())) diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index fee9853d03..8e16dc0f0f 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -419,10 +419,10 @@ U_BOOT_DEVICE(phandle_target) = { /* Node /phandle-source index 2 */ static struct dtd_source dtv_phandle_source = { \t.clocks\t\t\t= { -\t\t\t{NULL, {}}, -\t\t\t{NULL, {11}}, -\t\t\t{NULL, {12, 13}}, -\t\t\t{NULL, {}},}, +\t\t\t{4, {}}, +\t\t\t{0, {11}}, +\t\t\t{1, {12, 13}}, +\t\t\t{4, {}},}, }; U_BOOT_DEVICE(phandle_source) = { \t.name\t\t= "source", @@ -434,7 +434,7 @@ U_BOOT_DEVICE(phandle_source) = { /* Node /phandle-source2 index 3 */ static struct dtd_source dtv_phandle_source2 = { \t.clocks\t\t\t= { -\t\t\t{NULL, {}},}, +\t\t\t{4, {}},}, }; U_BOOT_DEVICE(phandle_source2) = { \t.name\t\t= "source", @@ -444,11 +444,6 @@ U_BOOT_DEVICE(phandle_source2) = { }; void dm_populate_phandle_data(void) { -\tdtv_phandle_source.clocks[0].node = DM_GET_DEVICE(phandle_target); -\tdtv_phandle_source.clocks[1].node = DM_GET_DEVICE(phandle2_target); -\tdtv_phandle_source.clocks[2].node = DM_GET_DEVICE(phandle3_target); -\tdtv_phandle_source.clocks[3].node = DM_GET_DEVICE(phandle_target); -\tdtv_phandle_source2.clocks[0].node = DM_GET_DEVICE(phandle_target); } ''', data) @@ -489,7 +484,7 @@ U_BOOT_DEVICE(phandle_target) = { /* Node /phandle-source2 index 0 */ static struct dtd_source dtv_phandle_source2 = { \t.clocks\t\t\t= { -\t\t\t{NULL, {}},}, +\t\t\t{1, {}},}, }; U_BOOT_DEVICE(phandle_source2) = { \t.name\t\t= "source", @@ -499,7 +494,6 @@ U_BOOT_DEVICE(phandle_source2) = { }; void dm_populate_phandle_data(void) { -\tdtv_phandle_source2.clocks[0].node = DM_GET_DEVICE(phandle_target); } ''', data) @@ -547,10 +541,10 @@ U_BOOT_DEVICE(phandle_target) = { /* Node /phandle-source index 2 */ static struct dtd_source dtv_phandle_source = { \t.cd_gpios\t\t= { -\t\t\t{NULL, {}}, -\t\t\t{NULL, {11}}, -\t\t\t{NULL, {12, 13}}, -\t\t\t{NULL, {}},}, +\t\t\t{4, {}}, +\t\t\t{0, {11}}, +\t\t\t{1, {12, 13}}, +\t\t\t{4, {}},}, }; U_BOOT_DEVICE(phandle_source) = { \t.name\t\t= "source", @@ -562,7 +556,7 @@ U_BOOT_DEVICE(phandle_source) = { /* Node /phandle-source2 index 3 */ static struct dtd_source dtv_phandle_source2 = { \t.cd_gpios\t\t= { -\t\t\t{NULL, {}},}, +\t\t\t{4, {}},}, }; U_BOOT_DEVICE(phandle_source2) = { \t.name\t\t= "source", @@ -572,11 +566,6 @@ U_BOOT_DEVICE(phandle_source2) = { }; void dm_populate_phandle_data(void) { -\tdtv_phandle_source.cd_gpios[0].node = DM_GET_DEVICE(phandle_target); -\tdtv_phandle_source.cd_gpios[1].node = DM_GET_DEVICE(phandle2_target); -\tdtv_phandle_source.cd_gpios[2].node = DM_GET_DEVICE(phandle3_target); -\tdtv_phandle_source.cd_gpios[3].node = DM_GET_DEVICE(phandle_target); -\tdtv_phandle_source2.cd_gpios[0].node = DM_GET_DEVICE(phandle_target); } ''', data) From 4e28a259fd3a2025f42ad005b375aea2912c81de Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 11:08:25 -0600 Subject: [PATCH 41/71] imx: mx6cuboxi: Disable thermal driver in SPL This feature is incompatble with of-platdata since it uses the U_BOOT_DEVICE() macro. With of-platdata the only devices permitted are those created by dtoc. The driver is not used in SPL anyway, so exclude it from that build. Signed-off-by: Simon Glass Reviewed-by: Fabio Estevam --- arch/arm/mach-imx/mx6/soc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/mach-imx/mx6/soc.c b/arch/arm/mach-imx/mx6/soc.c index e129286065..a636107410 100644 --- a/arch/arm/mach-imx/mx6/soc.c +++ b/arch/arm/mach-imx/mx6/soc.c @@ -34,7 +34,7 @@ struct scu_regs { u32 fpga_rev; }; -#if defined(CONFIG_IMX_THERMAL) +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_IMX_THERMAL) static const struct imx_thermal_plat imx6_thermal_plat = { .regs = (void *)ANATOP_BASE_ADDR, .fuse_bank = 1, From cb43ac184f71f0ba696d0effcd39a746a6f3a456 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:41 -0600 Subject: [PATCH 42/71] dm: Don't allow U_BOOT_DEVICE() when of-platdata is used With of-platdata, the devicetree is supposed to specify all the devices in the system. So far this hasn't really mattered since of-platdata still works correctly. However, new of-platdata features rely on numbering the devices in a particular order so that they can be referenced by a single integer. It is tricky to implement this efficiently when other devices are present in the build. To address this, disable use of U_BOOT_DEVICE() when of-platdata is enabled. This seems acceptable as it is not supposed to be used at all, except in SPL/TPL, where of-platdata is the recommended approach. This breaks one non-compliant boards at present: mx6cuboxi Signed-off-by: Simon Glass (disable CONFIG_IMX_THERMAL for mx6cuboxi to avoid a build error) --- include/dm/platdata.h | 8 ++++++++ tools/dtoc/dtb_platdata.py | 3 +++ tools/dtoc/test_dtoc.py | 3 +++ 3 files changed, 14 insertions(+) diff --git a/include/dm/platdata.h b/include/dm/platdata.h index f800a866dd..216efa8ef7 100644 --- a/include/dm/platdata.h +++ b/include/dm/platdata.h @@ -55,9 +55,17 @@ struct driver_rt { * NOTE: Avoid using these except in extreme circumstances, where device tree * is not feasible (e.g. serial driver in SPL where <8KB of SRAM is * available). U-Boot's driver model uses device tree for configuration. + * + * When of-platdata is in use, U_BOOT_DEVICE() cannot be used outside of the + * dt-platdata.c file created by dtoc */ +#if CONFIG_IS_ENABLED(OF_PLATDATA) && !defined(DT_PLATDATA_C) +#define U_BOOT_DEVICE(__name) _Static_assert(false, \ + "Cannot use U_BOOT_DEVICE with of-platdata. Please use devicetree instead") +#else #define U_BOOT_DEVICE(__name) \ ll_entry_declare(struct driver_info, __name, driver_info) +#endif /* Declare a list of devices. The argument is a driver_info[] array */ #define U_BOOT_DEVICES(__name) \ diff --git a/tools/dtoc/dtb_platdata.py b/tools/dtoc/dtb_platdata.py index 2be11fff6c..9b27aecc14 100644 --- a/tools/dtoc/dtb_platdata.py +++ b/tools/dtoc/dtb_platdata.py @@ -673,6 +673,9 @@ class DtbPlatdata(object): information. """ self.out_header() + self.out('/* Allow use of U_BOOT_DEVICE() in this file */\n') + self.out('#define DT_PLATDATA_C\n') + self.out('\n') self.out('#include \n') self.out('#include \n') self.out('#include \n') diff --git a/tools/dtoc/test_dtoc.py b/tools/dtoc/test_dtoc.py index 8e16dc0f0f..a5836e04b7 100755 --- a/tools/dtoc/test_dtoc.py +++ b/tools/dtoc/test_dtoc.py @@ -44,6 +44,9 @@ C_HEADER = '''/* * This file was generated by dtoc from a .dtb (device tree binary) file. */ +/* Allow use of U_BOOT_DEVICE() in this file */ +#define DT_PLATDATA_C + #include #include #include From 7b82ce882c3871636fd18205e4684896104b740d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:42 -0600 Subject: [PATCH 43/71] dm: doc: Update the of-platadata documentation Update this to incorporate the parent feature as well as other recent developments. Signed-off-by: Simon Glass --- doc/driver-model/of-plat.rst | 42 ++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/doc/driver-model/of-plat.rst b/doc/driver-model/of-plat.rst index 1e3fad137b..58481665ce 100644 --- a/doc/driver-model/of-plat.rst +++ b/doc/driver-model/of-plat.rst @@ -66,12 +66,6 @@ strictly necessary. Notable problems include: normally also supports device tree it must use #ifdef to separate out this code, since the structures are only available in SPL. - - Correct relations between nodes are not implemented. This means that - parent/child relations (like bus device iteration) do not work yet. - Some phandles (those that are recognised as such) are converted into - a pointer to struct driver_info. This pointer can be used to access - the referenced device. - How it works ------------ @@ -134,10 +128,14 @@ the following C struct declaration: fdt32_t vmmc_supply; }; -and the following device declaration: +and the following device declarations: .. code-block:: c + /* Node /clock-controller@ff760000 index 0 */ + ... + + /* Node /dwmmc@ff0c0000 index 2 */ static struct dtd_rockchip_rk3288_dw_mshc dtv_dwmmc_at_ff0c0000 = { .fifo_depth = 0x100, .cap_sd_highspeed = true, @@ -145,10 +143,10 @@ and the following device declaration: .clock_freq_min_max = {0x61a80, 0x8f0d180}, .vmmc_supply = 0xb, .num_slots = 0x1, - .clocks = {{NULL, 456}, - {NULL, 68}, - {NULL, 114}, - {NULL, 118}}, + .clocks = {{0, 456}, + {0, 68}, + {0, 114}, + {0, 118}}, .cap_mmc_highspeed = true, .disable_wp = true, .bus_width = 0x4, @@ -161,13 +159,10 @@ and the following device declaration: .name = "rockchip_rk3288_dw_mshc", .platdata = &dtv_dwmmc_at_ff0c0000, .platdata_size = sizeof(dtv_dwmmc_at_ff0c0000), + .parent_idx = -1, }; void dm_populate_phandle_data(void) { - dtv_dwmmc_at_ff0c0000.clocks[0].node = DM_GET_DEVICE(clock_controller_at_ff760000); - dtv_dwmmc_at_ff0c0000.clocks[1].node = DM_GET_DEVICE(clock_controller_at_ff760000); - dtv_dwmmc_at_ff0c0000.clocks[2].node = DM_GET_DEVICE(clock_controller_at_ff760000); - dtv_dwmmc_at_ff0c0000.clocks[3].node = DM_GET_DEVICE(clock_controller_at_ff760000); } The device is then instantiated at run-time and the platform data can be @@ -193,6 +188,13 @@ In order to make this a bit more flexible U_BOOT_DRIVER_ALIAS macro can be used to declare an alias for a driver name, typically a 'compatible' string. This macro produces no code, but it is by dtoc tool. +The parent_idx is the index of the parent driver_info structure within its +linker list (instantiated by the U_BOOT_DEVICE() macro). This is used to support +dev_get_parent(). The dm_populate_phandle_data() is included to allow for +fix-ups required by dtoc. It is not currently used. The values in 'clocks' are +the index of the driver_info for the target device followed by any phandle +arguments. This is used to support device_get_by_driver_info_idx(). + During the build process dtoc parses both U_BOOT_DRIVER and U_BOOT_DRIVER_ALIAS to build a list of valid driver names and driver aliases. If the 'compatible' string used for a device does not not match a valid driver name, it will be @@ -339,12 +341,7 @@ spl/dt-platdata.c. It additionally contains the definition of dm_populate_phandle_data() which is responsible of filling the phandle information by adding references to U_BOOT_DEVICE by using DM_GET_DEVICE -The beginnings of a libfdt Python module are provided. So far this only -implements a subset of the features. - -The 'swig' tool is needed to build the libfdt Python module. If this is not -found then the Python model is not used and a fallback is used instead, which -makes use of fdtget. +The pylibfdt Python module is used to access the devicetree. Credits @@ -357,11 +354,10 @@ Future work ----------- - Consider programmatically reading binding files instead of device tree contents -- Complete the phandle feature -- Move to using a full Python libfdt module .. Simon Glass .. Google, Inc .. 6/6/16 .. Updated Independence Day 2016 +.. Updated 1st October 2020 From f3243303a01eb58eef2a0cd320d562a7af486ddb Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:39:59 -0600 Subject: [PATCH 44/71] binman: Update the entry docs This has got out of sync with the entries. Regenerate it. Signed-off-by: Simon Glass --- tools/binman/README.entries | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tools/binman/README.entries b/tools/binman/README.entries index bdb4fd6ee5..13b930d518 100644 --- a/tools/binman/README.entries +++ b/tools/binman/README.entries @@ -406,6 +406,10 @@ The 'default' property, if present, will be automatically set to the name if of configuration whose devicetree matches the 'default-dt' entry argument, e.g. with '-a default-dt=sun50i-a64-pine64-lts'. +Available substitutions for '@' property values are: + + DEFAULT-SEQ Sequence number of the default fdt,as provided by the + 'default-dt' entry argument Properties (in the 'fit' node itself): fit,external-offset: Indicates that the contents of the FIT are external @@ -739,6 +743,16 @@ placed at offset 'RESET_VECTOR_ADDRESS - 0xffc'. +Entry: scp: Entry containing a System Control Processor (SCP) firmware blob +--------------------------------------------------------------------------- + +Properties / Entry arguments: + - scp-path: Filename of file to read into the entry, typically scp.bin + +This entry holds firmware for an external platform-specific coprocessor. + + + Entry: section: Entry that contains other entries ------------------------------------------------- @@ -878,6 +892,15 @@ relocated to any address for execution. +Entry: u-boot-env: An entry which contains a U-Boot environment +--------------------------------------------------------------- + +Properties / Entry arguments: + - filename: File containing the environment text, with each line in the + form var=value + + + Entry: u-boot-img: U-Boot legacy image -------------------------------------- From a4dfe3e473ea53c44db50d07c34815e24069d39d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:00 -0600 Subject: [PATCH 45/71] binman: Drop unused return variable for _DoTestFile() This function returns the exit code from binman, not any data. Fix up a few callers in the tests. Signed-off-by: Simon Glass --- tools/binman/ftest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 75f6ca3a89..d23967e603 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3737,14 +3737,14 @@ class TestFunctional(unittest.TestCase): def testEnvironmentNoSize(self): """Test that a missing 'size' property is detected""" with self.assertRaises(ValueError) as e: - data = self._DoTestFile('175_env_no_size.dts') + self._DoTestFile('175_env_no_size.dts') self.assertIn("'u-boot-env' entry must have a size property", str(e.exception)) def testEnvironmentTooSmall(self): """Test handling of an environment that does not fit""" with self.assertRaises(ValueError) as e: - data = self._DoTestFile('176_env_too_small.dts') + self._DoTestFile('176_env_too_small.dts') # checksum, start byte, environment with \0 terminator, final \0 need = 4 + 1 + len(ENV_DATA) + 1 + 1 From f2c0dd85ad6cbf8b53ad5fc423a0177a0eb7fe6f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:01 -0600 Subject: [PATCH 46/71] binman: Add tests for skip-at-start sections At present this feature is tested view the end-at-4gb feature. Add some tests of its own, including the operation of padding. The third test here shows binman's current, inconsistent approach to padding in the top-level section. Future patches in this series will address this. Signed-off-by: Simon Glass --- tools/binman/ftest.py | 59 +++++++++++++++++++ tools/binman/test/177_skip_at_start.dts | 19 ++++++ tools/binman/test/178_skip_at_start_pad.dts | 21 +++++++ .../test/179_skip_at_start_section_pad.dts | 22 +++++++ 4 files changed, 121 insertions(+) create mode 100644 tools/binman/test/177_skip_at_start.dts create mode 100644 tools/binman/test/178_skip_at_start_pad.dts create mode 100644 tools/binman/test/179_skip_at_start_section_pad.dts diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index d23967e603..adc16038d9 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3752,6 +3752,65 @@ class TestFunctional(unittest.TestCase): self.assertIn("too small to hold data (need %#x more bytes)" % short, str(e.exception)) + def testSkipAtStart(self): + """Test handling of skip-at-start section""" + data = self._DoReadFile('177_skip_at_start.dts') + self.assertEqual(U_BOOT_DATA, data) + + image = control.images['image'] + entries = image.GetEntries() + section = entries['section'] + self.assertEqual(0, section.offset) + self.assertEqual(len(U_BOOT_DATA), section.size) + self.assertEqual(U_BOOT_DATA, section.GetData()) + + entry = section.GetEntries()['u-boot'] + self.assertEqual(16, entry.offset) + self.assertEqual(len(U_BOOT_DATA), entry.size) + self.assertEqual(U_BOOT_DATA, entry.data) + + def testSkipAtStartPad(self): + """Test handling of skip-at-start section with padded entry""" + data = self._DoReadFile('178_skip_at_start_pad.dts') + before = tools.GetBytes(0, 8) + after = tools.GetBytes(0, 4) + all = before + U_BOOT_DATA + after + self.assertEqual(all, data) + + image = control.images['image'] + entries = image.GetEntries() + section = entries['section'] + self.assertEqual(0, section.offset) + self.assertEqual(len(all), section.size) + self.assertEqual(all, section.GetData()) + + entry = section.GetEntries()['u-boot'] + self.assertEqual(16, entry.offset) + self.assertEqual(len(all), entry.size) + self.assertEqual(U_BOOT_DATA, entry.data) + + def testSkipAtStartSectionPad(self): + """Test handling of skip-at-start section with padding""" + data = self._DoReadFile('179_skip_at_start_section_pad.dts') + before = tools.GetBytes(0, 8) + after = tools.GetBytes(0, 4) + all = before + U_BOOT_DATA + after + + # This is not correct, but it is what binman currently produces + self.assertEqual(tools.GetBytes(0, 16) + U_BOOT_DATA + after, data) + + image = control.images['image'] + entries = image.GetEntries() + section = entries['section'] + self.assertEqual(0, section.offset) + self.assertEqual(len(all), section.size) + self.assertIsNone(section.data) + self.assertEqual(all, section.GetData()) + + entry = section.GetEntries()['u-boot'] + self.assertEqual(16, entry.offset) + self.assertEqual(len(U_BOOT_DATA), entry.size) + self.assertEqual(U_BOOT_DATA, entry.data) if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/177_skip_at_start.dts b/tools/binman/test/177_skip_at_start.dts new file mode 100644 index 0000000000..021460b1a0 --- /dev/null +++ b/tools/binman/test/177_skip_at_start.dts @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2018 NXP + */ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + section { + skip-at-start = <16>; + u-boot { + }; + }; + }; +}; diff --git a/tools/binman/test/178_skip_at_start_pad.dts b/tools/binman/test/178_skip_at_start_pad.dts new file mode 100644 index 0000000000..deda3c862e --- /dev/null +++ b/tools/binman/test/178_skip_at_start_pad.dts @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2018 NXP + */ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + section { + skip-at-start = <16>; + u-boot { + pad-before = <8>; + pad-after = <4>; + }; + }; + }; +}; diff --git a/tools/binman/test/179_skip_at_start_section_pad.dts b/tools/binman/test/179_skip_at_start_section_pad.dts new file mode 100644 index 0000000000..bf2f8f69b4 --- /dev/null +++ b/tools/binman/test/179_skip_at_start_section_pad.dts @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2018 NXP + */ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + section { + skip-at-start = <16>; + pad-before = <8>; + pad-after = <4>; + + u-boot { + }; + }; + }; +}; From 680e3c6edb9aa5cf400a5b22ecfa3a40b0b247e5 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:02 -0600 Subject: [PATCH 47/71] binman: Give a sensible error if no command is given At present if 'binman' is typed on the command line, a strange error about a missing argument is displayed. Fix this. These does not seem to be standard way to add the 'required' argument in all recent Python versions, so set it manually. Signed-off-by: Simon Glass --- tools/binman/cmdline.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py index bb4d9d1288..c007d0a036 100644 --- a/tools/binman/cmdline.py +++ b/tools/binman/cmdline.py @@ -37,6 +37,7 @@ controlled by a description in the board device tree.''' '3=info, 4=detail, 5=debug') subparsers = parser.add_subparsers(dest='cmd') + subparsers.required = True build_parser = subparsers.add_parser('build', help='Build firmware image') build_parser.add_argument('-a', '--entry-arg', type=str, action='append', From a81294671ca190102a06db8ac3004dae7e49aa46 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:03 -0600 Subject: [PATCH 48/71] binman: Fix return from u-boot-ucode if there is no DT This should return empty contents, not leave it unset. Fix it. Signed-off-by: Simon Glass --- tools/binman/etype/u_boot_ucode.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/binman/etype/u_boot_ucode.py b/tools/binman/etype/u_boot_ucode.py index 4462293618..b4cb8cdb6e 100644 --- a/tools/binman/etype/u_boot_ucode.py +++ b/tools/binman/etype/u_boot_ucode.py @@ -81,6 +81,7 @@ class Entry_u_boot_ucode(Entry_blob): if fdt_entry: break if not fdt_entry: + self.data = b'' return True if not fdt_entry.ready: return False From 72628cdf58ec2737637b41ec3dad4a30fe5090e0 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:04 -0600 Subject: [PATCH 49/71] binman: Remove references to 'image' in entry_Section While a section is the base class of Image, it is more correct to refer to sections in most places in this file. Fix these comments. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 515c97f929..327750461e 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -56,7 +56,7 @@ class Entry_section(Entry): self._end_4gb = False def ReadNode(self): - """Read properties from the image node""" + """Read properties from the section node""" super().ReadNode() self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0) self._sort = fdt_util.GetBool(self._node, 'sort-by-offset') @@ -183,7 +183,7 @@ class Entry_section(Entry): return super().Pack(offset) def _PackEntries(self): - """Pack all entries into the image""" + """Pack all entries into the section""" offset = self._skip_at_start for entry in self._entries.values(): offset = entry.Pack(offset) @@ -209,7 +209,7 @@ class Entry_section(Entry): self._entries[entry._node.name] = entry def CheckEntries(self): - """Check that entries do not overlap or extend outside the image""" + """Check that entries do not overlap or extend outside the section""" if self._sort: self._SortEntries() self._ExpandEntries() @@ -456,7 +456,7 @@ class Entry_section(Entry): def CheckSize(self): - """Check that the image contents does not exceed its size, etc.""" + """Check that the section contents does not exceed its size, etc.""" contents_size = 0 for entry in self._entries.values(): contents_size = max(contents_size, entry.offset + entry.size) From e6bed4f1812c1666eeab794694c4b3765f41c143 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:05 -0600 Subject: [PATCH 50/71] binman: Expand the error message for breaching a section Add in a few more details to this error message to make it easier to see what is going on. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 10 ++++++---- tools/binman/ftest.py | 5 +++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 327750461e..a3e37c33c1 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -220,10 +220,12 @@ class Entry_section(Entry): if (entry.offset < self._skip_at_start or entry.offset + entry.size > self._skip_at_start + self.size): - entry.Raise("Offset %#x (%d) is outside the section starting " - "at %#x (%d)" % - (entry.offset, entry.offset, self._skip_at_start, - self._skip_at_start)) + entry.Raise('Offset %#x (%d) size %#x (%d) is outside the ' + "section '%s' starting at %#x (%d) " + 'of size %#x (%d)' % + (entry.offset, entry.offset, entry.size, entry.size, + self._node.path, self._skip_at_start, + self._skip_at_start, self.size, self.size)) if entry.offset < offset and entry.size: entry.Raise("Offset %#x (%d) overlaps with previous entry '%s' " "ending at %#x (%d)" % diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index adc16038d9..4f7e22612c 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -970,8 +970,9 @@ class TestFunctional(unittest.TestCase): """Test that the end-at-4gb property checks for offset boundaries""" with self.assertRaises(ValueError) as e: self._DoTestFile('028_pack_4gb_outside.dts') - self.assertIn("Node '/binman/u-boot': Offset 0x0 (0) is outside " - "the section starting at 0xffffffe0 (4294967264)", + self.assertIn("Node '/binman/u-boot': Offset 0x0 (0) size 0x4 (4) " + "is outside the section '/binman' starting at " + '0xffffffe0 (4294967264) of size 0x20 (32)', str(e.exception)) def testPackX86Rom(self): From 87c962943aec7aef8849c60f018a5e7756e5b7ba Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:06 -0600 Subject: [PATCH 51/71] binman: Move CompressData() into Entry base class At present this is only used by blobs. To allow it to be used by other entry types (such as sections), move it into the base class. Also read the compression type in the base class. Signed-off-by: Simon Glass --- tools/binman/entry.py | 17 +++++++++++++++++ tools/binman/etype/blob.py | 7 ------- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index f7adc3b1ab..173c9131cb 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -180,6 +180,9 @@ class Entry(object): self.expand_size = fdt_util.GetBool(self._node, 'expand-size') self.missing_msg = fdt_util.GetString(self._node, 'missing-msg') + # This is only supported by blobs and sections at present + self.compress = fdt_util.GetString(self._node, 'compress', 'none') + def GetDefaultFilename(self): return None @@ -836,3 +839,17 @@ features to produce new behaviours. list of possible tags, most desirable first """ return list(filter(None, [self.missing_msg, self.name, self.etype])) + + def CompressData(self, indata): + """Compress data according to the entry's compression method + + Args: + indata: Data to compress + + Returns: + Compressed data (first word is the compressed size) + """ + if self.compress != 'none': + self.uncomp_size = len(indata) + data = tools.Compress(indata, self.compress) + return data diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py index ecfb1e476e..301ac55e3b 100644 --- a/tools/binman/etype/blob.py +++ b/tools/binman/etype/blob.py @@ -33,7 +33,6 @@ class Entry_blob(Entry): def __init__(self, section, etype, node): super().__init__(section, etype, node) self._filename = fdt_util.GetString(self._node, 'filename', self.etype) - self.compress = fdt_util.GetString(self._node, 'compress', 'none') def ObtainContents(self): self._filename = self.GetDefaultFilename() @@ -48,12 +47,6 @@ class Entry_blob(Entry): self.ReadBlobContents() return True - def CompressData(self, indata): - if self.compress != 'none': - self.uncomp_size = len(indata) - data = tools.Compress(indata, self.compress) - return data - def ReadBlobContents(self): """Read blob contents into memory From 9248c8d9c9674d705216e51161c1f0f473fc7fa3 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:07 -0600 Subject: [PATCH 52/71] binman: Use 'files-compress' to set compression for files At present we use 'compress' as the property to set the compression of a 'files' entry. But this conflicts with the same property for entries, of which Entry_section is a subclass. Strictly speaking, since Entry_files is in fact a subclass of Entry_section, the files can be compressed individually but also the section (that contains all the files) can itself be compressed. With this change, it is possible to express that. Signed-off-by: Simon Glass --- tools/binman/README.entries | 2 +- tools/binman/etype/files.py | 7 ++++--- tools/binman/etype/section.py | 4 ++-- tools/binman/test/085_files_compress.dts | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/tools/binman/README.entries b/tools/binman/README.entries index 13b930d518..999b77690f 100644 --- a/tools/binman/README.entries +++ b/tools/binman/README.entries @@ -299,7 +299,7 @@ Entry: files: Entry containing a set of files Properties / Entry arguments: - pattern: Filename pattern to match the files to include - - compress: Compression algorithm to use: + - files-compress: Compression algorithm to use: none: No compression lz4: Use lz4 compression (via 'lz4' command-line utility) diff --git a/tools/binman/etype/files.py b/tools/binman/etype/files.py index 9adb3afeb1..ce3832e3cd 100644 --- a/tools/binman/etype/files.py +++ b/tools/binman/etype/files.py @@ -19,7 +19,7 @@ class Entry_files(Entry_section): Properties / Entry arguments: - pattern: Filename pattern to match the files to include - - compress: Compression algorithm to use: + - files-compress: Compression algorithm to use: none: No compression lz4: Use lz4 compression (via 'lz4' command-line utility) @@ -36,7 +36,8 @@ class Entry_files(Entry_section): self._pattern = fdt_util.GetString(self._node, 'pattern') if not self._pattern: self.Raise("Missing 'pattern' property") - self._compress = fdt_util.GetString(self._node, 'compress', 'none') + self._files_compress = fdt_util.GetString(self._node, 'files-compress', + 'none') self._require_matches = fdt_util.GetBool(self._node, 'require-matches') @@ -53,7 +54,7 @@ class Entry_files(Entry_section): subnode = state.AddSubnode(self._node, name) state.AddString(subnode, 'type', 'blob') state.AddString(subnode, 'filename', fname) - state.AddString(subnode, 'compress', self._compress) + state.AddString(subnode, 'compress', self._files_compress) # Read entries again, now that we have some self._ReadEntries() diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index a3e37c33c1..9222042f5d 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -160,7 +160,7 @@ class Entry_section(Entry): section_data += tools.GetBytes(self._pad_byte, pad) self.Detail('GetData: %d entries, total size %#x' % (len(self._entries), len(section_data))) - return section_data + return self.CompressData(section_data) def GetOffsets(self): """Handle entries that want to set the offset/size of other entries @@ -414,7 +414,7 @@ class Entry_section(Entry): return None def GetEntryContents(self): - """Call ObtainContents() for the section + """Call ObtainContents() for each entry in the section """ todo = self._entries.values() for passnum in range(3): diff --git a/tools/binman/test/085_files_compress.dts b/tools/binman/test/085_files_compress.dts index 847b398bf2..5aeead2e6e 100644 --- a/tools/binman/test/085_files_compress.dts +++ b/tools/binman/test/085_files_compress.dts @@ -5,7 +5,7 @@ binman { files { pattern = "files/*.dat"; - compress = "lz4"; + files-compress = "lz4"; }; }; }; From ef439ed191a0655a86f5b4035cec9cc57e97d8b1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:08 -0600 Subject: [PATCH 53/71] binman: Update testPackExtra with more checks Check the contents of each section to make sure it is actually in the right place. Also fix a whitespace error in the .dts file. Signed-off-by: Simon Glass --- tools/binman/ftest.py | 27 ++++++++++++++++++++++----- tools/binman/test/009_pack_extra.dts | 2 +- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 4f7e22612c..481b8fd03c 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -785,9 +785,8 @@ class TestFunctional(unittest.TestCase): def testPackExtra(self): """Test that extra packing feature works as expected""" - retcode = self._DoTestFile('009_pack_extra.dts') + data = self._DoReadFile('009_pack_extra.dts') - self.assertEqual(0, retcode) self.assertIn('image', control.images) image = control.images['image'] entries = image.GetEntries() @@ -799,30 +798,48 @@ class TestFunctional(unittest.TestCase): self.assertEqual(0, entry.offset) self.assertEqual(3, entry.pad_before) self.assertEqual(3 + 5 + len(U_BOOT_DATA), entry.size) + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(tools.GetBytes(0, 3) + U_BOOT_DATA + + tools.GetBytes(0, 5), data[:entry.size]) + pos = entry.size # Second u-boot has an aligned size, but it has no effect self.assertIn('u-boot-align-size-nop', entries) entry = entries['u-boot-align-size-nop'] - self.assertEqual(12, entry.offset) - self.assertEqual(4, entry.size) + self.assertEqual(pos, entry.offset) + self.assertEqual(len(U_BOOT_DATA), entry.size) + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(U_BOOT_DATA, data[pos:pos + entry.size]) + pos += entry.size # Third u-boot has an aligned size too self.assertIn('u-boot-align-size', entries) entry = entries['u-boot-align-size'] - self.assertEqual(16, entry.offset) + self.assertEqual(pos, entry.offset) self.assertEqual(32, entry.size) + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(U_BOOT_DATA + tools.GetBytes(0, 32 - len(U_BOOT_DATA)), + data[pos:pos + entry.size]) + pos += entry.size # Fourth u-boot has an aligned end self.assertIn('u-boot-align-end', entries) entry = entries['u-boot-align-end'] self.assertEqual(48, entry.offset) self.assertEqual(16, entry.size) + self.assertEqual(U_BOOT_DATA, entry.data[:len(U_BOOT_DATA)]) + self.assertEqual(U_BOOT_DATA + tools.GetBytes(0, 16 - len(U_BOOT_DATA)), + data[pos:pos + entry.size]) + pos += entry.size # Fifth u-boot immediately afterwards self.assertIn('u-boot-align-both', entries) entry = entries['u-boot-align-both'] self.assertEqual(64, entry.offset) self.assertEqual(64, entry.size) + self.assertEqual(U_BOOT_DATA, entry.data[:len(U_BOOT_DATA)]) + self.assertEqual(U_BOOT_DATA + tools.GetBytes(0, 64 - len(U_BOOT_DATA)), + data[pos:pos + entry.size]) self.CheckNoGaps(entries) self.assertEqual(128, image.size) diff --git a/tools/binman/test/009_pack_extra.dts b/tools/binman/test/009_pack_extra.dts index 0765707dea..1b31555771 100644 --- a/tools/binman/test/009_pack_extra.dts +++ b/tools/binman/test/009_pack_extra.dts @@ -28,7 +28,7 @@ u-boot-align-both { type = "u-boot"; - align= <64>; + align = <64>; align-end = <128>; }; }; From f90d906a275f85e7077d9a30ab82b20676b54645 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:09 -0600 Subject: [PATCH 54/71] binman: Expand docs and test for padding Padding becomes part of the entry once the image is written out, but within binman the entry contents does not include the padding. Add documentation to make this clear, as well as a test. Signed-off-by: Simon Glass --- tools/binman/README | 12 +++++++++--- tools/binman/entry.py | 11 ++++++++--- tools/binman/ftest.py | 29 ++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/tools/binman/README b/tools/binman/README index fbcfdc77c3..0433cabce4 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -290,14 +290,20 @@ size: pad-before: Padding before the contents of the entry. Normally this is 0, meaning - that the contents start at the beginning of the entry. This can be - offset the entry contents a little. Defaults to 0. + that the contents start at the beginning of the entry. This can be used + to offset the entry contents a little. While this does not affect the + contents of the entry within binman itself (the padding is performed + only when its parent section is assembled), the end result will be that + the entry starts with the padding bytes, so may grow. Defaults to 0. pad-after: Padding after the contents of the entry. Normally this is 0, meaning that the entry ends at the last byte of content (unless adjusted by other properties). This allows room to be created in the image for - this entry to expand later. Defaults to 0. + this entry to expand later. While this does not affect the contents of + the entry within binman itself (the padding is performed only when its + parent section is assembled), the end result will be that the entry ends + with the padding bytes, so may grow. Defaults to 0. align-size: This sets the alignment of the entry size. For example, to ensure diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 173c9131cb..e5d0aa52bd 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -51,9 +51,14 @@ class Entry(object): align: Entry start offset alignment, or None align_size: Entry size alignment, or None align_end: Entry end offset alignment, or None - pad_before: Number of pad bytes before the contents, 0 if none - pad_after: Number of pad bytes after the contents, 0 if none - data: Contents of entry (string of bytes) + pad_before: Number of pad bytes before the contents when it is placed + in the containing section, 0 if none. The pad bytes become part of + the entry. + pad_after: Number of pad bytes after the contents when it is placed in + the containing section, 0 if none. The pad bytes become part of + the entry. + data: Contents of entry (string of bytes). This does not include + padding created by pad_before or pad_after compress: Compression algoithm used (e.g. 'lz4'), 'none' if none orig_offset: Original offset value read from node orig_size: Original size value read from node diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 481b8fd03c..35c1420681 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3548,12 +3548,39 @@ class TestFunctional(unittest.TestCase): def testPadInSections(self): """Test pad-before, pad-after for entries in sections""" - data = self._DoReadFile('166_pad_in_sections.dts') + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '166_pad_in_sections.dts', update_dtb=True) expected = (U_BOOT_DATA + tools.GetBytes(ord('!'), 12) + U_BOOT_DATA + tools.GetBytes(ord('!'), 6) + U_BOOT_DATA) self.assertEqual(expected, data) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['size', 'image-pos', 'offset']) + expected = { + 'image-pos': 0, + 'offset': 0, + 'size': 12 + 6 + 3 * len(U_BOOT_DATA), + + 'section:image-pos': 0, + 'section:offset': 0, + 'section:size': 12 + 6 + 3 * len(U_BOOT_DATA), + + 'section/before:image-pos': 0, + 'section/before:offset': 0, + 'section/before:size': len(U_BOOT_DATA), + + 'section/u-boot:image-pos': 4, + 'section/u-boot:offset': 4, + 'section/u-boot:size': 12 + len(U_BOOT_DATA) + 6, + + 'section/after:image-pos': 26, + 'section/after:offset': 26, + 'section/after:size': len(U_BOOT_DATA), + } + self.assertEqual(expected, props) + def testFitImageSubentryAlignment(self): """Test relative alignability of FIT image subentries""" entry_args = { From 4eec34c91f84d6c3915b300f0688f9b055e04dda Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:10 -0600 Subject: [PATCH 55/71] binman: Expand docs and test for alignment Alignment does form part of the entry once the image is written out, but within binman the entry contents does not include the padding. Add documentation to make this clear, as well as a test. Signed-off-by: Simon Glass --- tools/binman/README | 29 ++++++++++++++++++++--------- tools/binman/entry.py | 6 ++++-- tools/binman/ftest.py | 33 ++++++++++++++++++++++++++++++++- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/tools/binman/README b/tools/binman/README index 0433cabce4..0dee71d1b2 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -278,9 +278,12 @@ offset: align: This sets the alignment of the entry. The entry offset is adjusted - so that the entry starts on an aligned boundary within the image. For - example 'align = <16>' means that the entry will start on a 16-byte - boundary. Alignment shold be a power of 2. If 'align' is not + so that the entry starts on an aligned boundary within the containing + section or image. For example 'align = <16>' means that the entry will + start on a 16-byte boundary. This may mean that padding is added before + the entry. The padding is part of the containing section but is not + included in the entry, meaning that an empty space may be created before + the entry starts. Alignment should be a power of 2. If 'align' is not provided, no alignment is performed. size: @@ -308,14 +311,22 @@ pad-after: align-size: This sets the alignment of the entry size. For example, to ensure that the size of an entry is a multiple of 64 bytes, set this to 64. - If 'align-size' is not provided, no alignment is performed. + While this does not affect the contents of the entry within binman + itself (the padding is performed only when its parent section is + assembled), the end result is that the entry ends with the padding + bytes, so may grow. If 'align-size' is not provided, no alignment is + performed. align-end: - This sets the alignment of the end of an entry. Some entries require - that they end on an alignment boundary, regardless of where they - start. This does not move the start of the entry, so the contents of - the entry will still start at the beginning. But there may be padding - at the end. If 'align-end' is not provided, no alignment is performed. + This sets the alignment of the end of an entry with respect to the + containing section. Some entries require that they end on an alignment + boundary, regardless of where they start. This does not move the start + of the entry, so the contents of the entry will still start at the + beginning. But there may be padding at the end. While this does not + affect the contents of the entry within binman itself (the padding is + performed only when its parent section is assembled), the end result + is that the entry ends with the padding bytes, so may grow. + If 'align-end' is not provided, no alignment is performed. filename: For 'blob' types this provides the filename containing the binary to diff --git a/tools/binman/entry.py b/tools/binman/entry.py index e5d0aa52bd..0421129c03 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -48,9 +48,11 @@ class Entry(object): uncomp_size: Size of uncompressed data in bytes, if the entry is compressed, else None contents_size: Size of contents in bytes, 0 by default - align: Entry start offset alignment, or None + align: Entry start offset alignment relative to the start of the + containing section, or None align_size: Entry size alignment, or None - align_end: Entry end offset alignment, or None + align_end: Entry end offset alignment relative to the start of the + containing section, or None pad_before: Number of pad bytes before the contents when it is placed in the containing section, 0 if none. The pad bytes become part of the entry. diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 35c1420681..c99852d581 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -785,7 +785,8 @@ class TestFunctional(unittest.TestCase): def testPackExtra(self): """Test that extra packing feature works as expected""" - data = self._DoReadFile('009_pack_extra.dts') + data, _, _, out_dtb_fname = self._DoReadFileDtb('009_pack_extra.dts', + update_dtb=True) self.assertIn('image', control.images) image = control.images['image'] @@ -844,6 +845,36 @@ class TestFunctional(unittest.TestCase): self.CheckNoGaps(entries) self.assertEqual(128, image.size) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['size', 'offset', 'image-pos']) + expected = { + 'image-pos': 0, + 'offset': 0, + 'size': 128, + + 'u-boot:image-pos': 0, + 'u-boot:offset': 0, + 'u-boot:size': 3 + 5 + len(U_BOOT_DATA), + + 'u-boot-align-size-nop:image-pos': 12, + 'u-boot-align-size-nop:offset': 12, + 'u-boot-align-size-nop:size': 4, + + 'u-boot-align-size:image-pos': 16, + 'u-boot-align-size:offset': 16, + 'u-boot-align-size:size': 32, + + 'u-boot-align-end:image-pos': 48, + 'u-boot-align-end:offset': 48, + 'u-boot-align-end:size': 16, + + 'u-boot-align-both:image-pos': 64, + 'u-boot-align-both:offset': 64, + 'u-boot-align-both:size': 64, + } + self.assertEqual(expected, props) + def testPackAlignPowerOf2(self): """Test that invalid entry alignment is detected""" with self.assertRaises(ValueError) as e: From 17ea9f35e780d073820a770b8e65641761a31d1f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:11 -0600 Subject: [PATCH 56/71] binman: Move section-building code into a function Create a new _BuildSectionData() to hold the code that is now in GetData(), so that it is clearly separated from entry.GetData() base function. Separate out the 'pad-before' processing to make this easier to understand. Unfortunately this breaks the testDual test. Rather than squash several patches into an un-reviewable glob, disable the test for now. This also affects testSkipAtStartSectionPad(), although it still not quite what it should be. Update that temporarily for now. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 35 +++++++++++++++++++++++++++++------ tools/binman/ftest.py | 4 ++-- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 9222042f5d..d05adf0027 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -144,24 +144,47 @@ class Entry_section(Entry): def ObtainContents(self): return self.GetEntryContents() - def GetData(self): + def _BuildSectionData(self): + """Build the contents of a section + + This places all entries at the right place, dealing with padding before + and after entries. It does not do padding for the section itself (the + pad-before and pad-after properties in the section items) since that is + handled by the parent section. + + Returns: + Contents of the section (bytes) + """ section_data = b'' for entry in self._entries.values(): data = entry.GetData() - base = self.pad_before + (entry.offset or 0) - self._skip_at_start - pad = base - len(section_data) + (entry.pad_before or 0) + # Handle empty space before the entry + pad = (entry.offset or 0) - self._skip_at_start - len(section_data) if pad > 0: section_data += tools.GetBytes(self._pad_byte, pad) + + # Handle padding before the entry + if entry.pad_before: + section_data += tools.GetBytes(self._pad_byte, entry.pad_before) + + # Add in the actual entry data section_data += data + + # Handle padding after the entry + if entry.pad_after: + section_data += tools.GetBytes(self._pad_byte, entry.pad_after) + if self.size: - pad = self.size - len(section_data) - if pad > 0: - section_data += tools.GetBytes(self._pad_byte, pad) + section_data += tools.GetBytes(self._pad_byte, + self.size - len(section_data)) self.Detail('GetData: %d entries, total size %#x' % (len(self._entries), len(section_data))) return self.CompressData(section_data) + def GetData(self): + return self._BuildSectionData() + def GetOffsets(self): """Handle entries that want to set the offset/size of other entries diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index c99852d581..82be9976c3 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -708,6 +708,7 @@ class TestFunctional(unittest.TestCase): """Test a simple binman run with debugging enabled""" self._DoTestFile('005_simple.dts', debug=True) + @unittest.skip('Disable for now until padding of images is supported') def testDual(self): """Test that we can handle creating two images @@ -3873,7 +3874,7 @@ class TestFunctional(unittest.TestCase): all = before + U_BOOT_DATA + after # This is not correct, but it is what binman currently produces - self.assertEqual(tools.GetBytes(0, 16) + U_BOOT_DATA + after, data) + self.assertEqual(before + U_BOOT_DATA + tools.GetBytes(0, 16), data) image = control.images['image'] entries = image.GetEntries() @@ -3881,7 +3882,6 @@ class TestFunctional(unittest.TestCase): self.assertEqual(0, section.offset) self.assertEqual(len(all), section.size) self.assertIsNone(section.data) - self.assertEqual(all, section.GetData()) entry = section.GetEntries()['u-boot'] self.assertEqual(16, entry.offset) From 4a655c9bd7d0e58206ed4417e8da31ad5da4926b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:12 -0600 Subject: [PATCH 57/71] binman: Refactor _BuildSectionData() At present this function does the padding needed around an entry. It is easier to understand what is going on if we have a function that returns the contents of an entry, with padding included. Refactor the code accordingly, adding a new GetPaddedData() method. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 59 +++++++++++++++++++++++++++++------ tools/binman/image.py | 2 +- 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index d05adf0027..f80432914f 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -16,6 +16,7 @@ from binman.entry import Entry from dtoc import fdt_util from patman import tools from patman import tout +from patman.tools import ToHexSize class Entry_section(Entry): @@ -144,6 +145,36 @@ class Entry_section(Entry): def ObtainContents(self): return self.GetEntryContents() + def GetPaddedDataForEntry(self, entry): + """Get the data for an entry including any padding + + Gets the entry data and uses the section pad-byte value to add padding + before and after as defined by the pad-before and pad-after properties. + This does not consider alignment. + + Args: + entry: Entry to check + + Returns: + Contents of the entry along with any pad bytes before and + after it (bytes) + """ + data = b'' + # Handle padding before the entry + if entry.pad_before: + data += tools.GetBytes(self._pad_byte, entry.pad_before) + + # Add in the actual entry data + data += entry.GetData() + + # Handle padding after the entry + if entry.pad_after: + data += tools.GetBytes(self._pad_byte, entry.pad_after) + + self.Detail('GetPaddedDataForEntry: size %s' % ToHexSize(self.data)) + + return data + def _BuildSectionData(self): """Build the contents of a section @@ -158,23 +189,15 @@ class Entry_section(Entry): section_data = b'' for entry in self._entries.values(): - data = entry.GetData() + data = self.GetPaddedDataForEntry(entry) # Handle empty space before the entry pad = (entry.offset or 0) - self._skip_at_start - len(section_data) if pad > 0: section_data += tools.GetBytes(self._pad_byte, pad) - # Handle padding before the entry - if entry.pad_before: - section_data += tools.GetBytes(self._pad_byte, entry.pad_before) - # Add in the actual entry data section_data += data - # Handle padding after the entry - if entry.pad_after: - section_data += tools.GetBytes(self._pad_byte, entry.pad_after) - if self.size: section_data += tools.GetBytes(self._pad_byte, self.size - len(section_data)) @@ -182,6 +205,24 @@ class Entry_section(Entry): (len(self._entries), len(section_data))) return self.CompressData(section_data) + def GetPaddedData(self): + """Get the data for a section including any padding + + Gets the section data and uses the parent section's pad-byte value to + add padding before and after as defined by the pad-before and pad-after + properties. If this is a top-level section (i.e. an image), this is the + same as GetData(), since padding is not supported. + + This does not consider alignment. + + Returns: + Contents of the section along with any pad bytes before and + after it (bytes) + """ + if self.section: + return super().GetPaddedData() + return self.GetData() + def GetData(self): return self._BuildSectionData() diff --git a/tools/binman/image.py b/tools/binman/image.py index a8772c3763..d65ab887b8 100644 --- a/tools/binman/image.py +++ b/tools/binman/image.py @@ -146,7 +146,7 @@ class Image(section.Entry_section): fname = tools.GetOutputFilename(self._filename) tout.Info("Writing image to '%s'" % fname) with open(fname, 'wb') as fd: - data = self.GetData() + data = self.GetPaddedData() fd.write(data) tout.Info("Wrote %#x bytes" % len(data)) From d1d3ad7d1fea137a6fe0709458947ca84ffb124c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:13 -0600 Subject: [PATCH 58/71] binman: Move section padding to the parent Each section is padded up to its size, if the contents are not large enough. Move this logic from _BuildSectionData() to GetPaddedDataForEntry() so that all the padding is in one place. With this, the testDual test is working again, so enable it. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 18 ++++++++++-------- tools/binman/ftest.py | 6 ++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index f80432914f..7cbb50057a 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -159,17 +159,23 @@ class Entry_section(Entry): Contents of the entry along with any pad bytes before and after it (bytes) """ + pad_byte = (entry._pad_byte if isinstance(entry, Entry_section) + else self._pad_byte) + data = b'' # Handle padding before the entry if entry.pad_before: - data += tools.GetBytes(self._pad_byte, entry.pad_before) + data += tools.GetBytes(pad_byte, entry.pad_before) # Add in the actual entry data data += entry.GetData() # Handle padding after the entry if entry.pad_after: - data += tools.GetBytes(self._pad_byte, entry.pad_after) + data += tools.GetBytes(pad_byte, entry.pad_after) + + if entry.size: + data += tools.GetBytes(pad_byte, entry.size - len(data)) self.Detail('GetPaddedDataForEntry: size %s' % ToHexSize(self.data)) @@ -198,9 +204,6 @@ class Entry_section(Entry): # Add in the actual entry data section_data += data - if self.size: - section_data += tools.GetBytes(self._pad_byte, - self.size - len(section_data)) self.Detail('GetData: %d entries, total size %#x' % (len(self._entries), len(section_data))) return self.CompressData(section_data) @@ -219,9 +222,8 @@ class Entry_section(Entry): Contents of the section along with any pad bytes before and after it (bytes) """ - if self.section: - return super().GetPaddedData() - return self.GetData() + section = self.section or self + return section.GetPaddedDataForEntry(self) def GetData(self): return self._BuildSectionData() diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 82be9976c3..31e93c647f 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -708,7 +708,6 @@ class TestFunctional(unittest.TestCase): """Test a simple binman run with debugging enabled""" self._DoTestFile('005_simple.dts', debug=True) - @unittest.skip('Disable for now until padding of images is supported') def testDual(self): """Test that we can handle creating two images @@ -3872,9 +3871,7 @@ class TestFunctional(unittest.TestCase): before = tools.GetBytes(0, 8) after = tools.GetBytes(0, 4) all = before + U_BOOT_DATA + after - - # This is not correct, but it is what binman currently produces - self.assertEqual(before + U_BOOT_DATA + tools.GetBytes(0, 16), data) + self.assertEqual(all, data) image = control.images['image'] entries = image.GetEntries() @@ -3882,6 +3879,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual(0, section.offset) self.assertEqual(len(all), section.size) self.assertIsNone(section.data) + self.assertEqual(all, section.GetPaddedData()) entry = section.GetEntries()['u-boot'] self.assertEqual(16, entry.offset) From 7d398bb1c71580da2182f0b820d91950bf4b3d24 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:14 -0600 Subject: [PATCH 59/71] binman: Make section padding consistent with other entries At present padding of sections is inconsistent with other entry types, in that different pad bytes are used. When a normal entry is padded by its parent, the parent's pad byte is used. But for sections, the section's pad byte is used. Adjust logic to always do this the same way. Note there is still a special case in entry_Section.GetPaddedData() where an image is padded with the pad byte of the top-level section. This is necessary since otherwise there would be no way to set the pad byte of the image, without adding a top-level section to every image. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 4 +-- tools/binman/ftest.py | 23 +++++++++++++++++ tools/binman/test/180_section_pad.dts | 27 ++++++++++++++++++++ tools/binman/test/181_section_align.dts | 34 +++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 tools/binman/test/180_section_pad.dts create mode 100644 tools/binman/test/181_section_align.dts diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 7cbb50057a..c423a22c80 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -165,14 +165,14 @@ class Entry_section(Entry): data = b'' # Handle padding before the entry if entry.pad_before: - data += tools.GetBytes(pad_byte, entry.pad_before) + data += tools.GetBytes(self._pad_byte, entry.pad_before) # Add in the actual entry data data += entry.GetData() # Handle padding after the entry if entry.pad_after: - data += tools.GetBytes(pad_byte, entry.pad_after) + data += tools.GetBytes(self._pad_byte, entry.pad_after) if entry.size: data += tools.GetBytes(pad_byte, entry.size - len(data)) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 31e93c647f..830b610890 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3886,5 +3886,28 @@ class TestFunctional(unittest.TestCase): self.assertEqual(len(U_BOOT_DATA), entry.size) self.assertEqual(U_BOOT_DATA, entry.data) + def testSectionPad(self): + """Testing padding with sections""" + data = self._DoReadFile('180_section_pad.dts') + expected = (tools.GetBytes(ord('&'), 3) + + tools.GetBytes(ord('!'), 5) + + U_BOOT_DATA + + tools.GetBytes(ord('!'), 1) + + tools.GetBytes(ord('&'), 2)) + self.assertEqual(expected, data) + + def testSectionAlign(self): + """Testing alignment with sections""" + data = self._DoReadFileDtb('181_section_align.dts', map=True)[0] + expected = (b'\0' + # fill section + tools.GetBytes(ord('&'), 1) + # padding to section align + b'\0' + # fill section + tools.GetBytes(ord('!'), 3) + # padding to u-boot align + U_BOOT_DATA + + tools.GetBytes(ord('!'), 4) + # padding to u-boot size + tools.GetBytes(ord('!'), 4)) # padding to section size + self.assertEqual(expected, data) + + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/180_section_pad.dts b/tools/binman/test/180_section_pad.dts new file mode 100644 index 0000000000..7e4ebf257b --- /dev/null +++ b/tools/binman/test/180_section_pad.dts @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0x26>; + section@0 { + read-only; + + /* Padding for the section uses the 0x26 pad byte */ + pad-before = <3>; + pad-after = <2>; + + /* Set the padding byte for entries, i.e. u-boot */ + pad-byte = <0x21>; + + u-boot { + pad-before = <5>; + pad-after = <1>; + }; + }; + }; +}; diff --git a/tools/binman/test/181_section_align.dts b/tools/binman/test/181_section_align.dts new file mode 100644 index 0000000000..90795d131b --- /dev/null +++ b/tools/binman/test/181_section_align.dts @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + pad-byte = <0x26>; + fill { + size = <1>; + }; + section@1 { + read-only; + + /* Padding for the section uses the 0x26 pad byte */ + align = <2>; + align-size = <0x10>; + + /* Set the padding byte for entries, i.e. u-boot */ + pad-byte = <0x21>; + + fill { + size = <1>; + }; + + u-boot { + align = <4>; + align-size = <8>; + }; + }; + }; +}; From 97c3e9a6faa4483a700b26988bc48c2f9efe8dd6 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:15 -0600 Subject: [PATCH 60/71] binman: Store the original data before compression When compressing an entry, the original uncompressed data is overwritten. Store it so it is available if needed. Signed-off-by: Simon Glass --- tools/binman/entry.py | 7 ++++++- tools/binman/ftest.py | 12 ++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 0421129c03..d701eaff8f 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -60,7 +60,10 @@ class Entry(object): the containing section, 0 if none. The pad bytes become part of the entry. data: Contents of entry (string of bytes). This does not include - padding created by pad_before or pad_after + padding created by pad_before or pad_after. If the entry is + compressed, this contains the compressed data. + uncomp_data: Original uncompressed data, if this entry is compressed, + else None compress: Compression algoithm used (e.g. 'lz4'), 'none' if none orig_offset: Original offset value read from node orig_size: Original size value read from node @@ -83,6 +86,7 @@ class Entry(object): self.pre_reset_size = None self.uncomp_size = None self.data = None + self.uncomp_data = None self.contents_size = 0 self.align = None self.align_size = None @@ -856,6 +860,7 @@ features to produce new behaviours. Returns: Compressed data (first word is the compressed size) """ + self.uncomp_data = indata if self.compress != 'none': self.uncomp_size = len(indata) data = tools.Compress(indata, self.compress) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 830b610890..4c94bea6f0 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1810,6 +1810,18 @@ class TestFunctional(unittest.TestCase): props = self._GetPropTree(dtb, ['size', 'uncomp-size']) orig = self._decompress(data) self.assertEquals(COMPRESS_DATA, orig) + + # Do a sanity check on various fields + image = control.images['image'] + entries = image.GetEntries() + self.assertEqual(1, len(entries)) + + entry = entries['blob'] + self.assertEqual(COMPRESS_DATA, entry.uncomp_data) + self.assertEqual(len(COMPRESS_DATA), entry.uncomp_size) + orig = self._decompress(entry.data) + self.assertEqual(orig, entry.uncomp_data) + expected = { 'blob:uncomp-size': len(COMPRESS_DATA), 'blob:size': len(data), From 63e7ba6c1820def06e7ba88ce357cb605285e70c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:16 -0600 Subject: [PATCH 61/71] binman: Set section contents in GetData() Section contents is not set up when ObtainContents() is called, since packing often changes the layout of the contents. Ensure that the contents are correctly recorded by making this function regenerate the section. It is normally only called by the parent section (when packing) or by the top-level image code, when writing out the image. So the performance impact is fairly small. Now that sections have their contents in their 'data' property, update testSkipAtStartSectionPad() to check it. Signed-off-by: Simon Glass --- tools/binman/entry.py | 6 ++++++ tools/binman/etype/section.py | 14 +++++++++++++- tools/binman/ftest.py | 4 +++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index d701eaff8f..01a5fde84e 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -437,6 +437,12 @@ class Entry(object): return self._node.path def GetData(self): + """Get the contents of an entry + + Returns: + bytes content of the entry, excluding any padding. If the entry is + compressed, the compressed data is returned + """ self.Detail('GetData: size %s' % ToHexSize(self.data)) return self.data diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index c423a22c80..6e6f674972 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -226,7 +226,19 @@ class Entry_section(Entry): return section.GetPaddedDataForEntry(self) def GetData(self): - return self._BuildSectionData() + """Get the contents of an entry + + This builds the contents of the section, stores this as the contents of + the section and returns it + + Returns: + bytes content of the section, made up for all all of its subentries. + This excludes any padding. If the section is compressed, the + compressed data is returned + """ + data = self._BuildSectionData() + self.SetContents(data) + return data def GetOffsets(self): """Handle entries that want to set the offset/size of other entries diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 4c94bea6f0..6f47deac3b 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1822,6 +1822,8 @@ class TestFunctional(unittest.TestCase): orig = self._decompress(entry.data) self.assertEqual(orig, entry.uncomp_data) + self.assertEqual(image.data, entry.data) + expected = { 'blob:uncomp-size': len(COMPRESS_DATA), 'blob:size': len(data), @@ -3890,7 +3892,7 @@ class TestFunctional(unittest.TestCase): section = entries['section'] self.assertEqual(0, section.offset) self.assertEqual(len(all), section.size) - self.assertIsNone(section.data) + self.assertEqual(U_BOOT_DATA, section.data) self.assertEqual(all, section.GetPaddedData()) entry = section.GetEntries()['u-boot'] From a9fad07d4b862b4b5d52e04950a374371936f7eb Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:17 -0600 Subject: [PATCH 62/71] binman: Avoid reporting image-pos with compression When a section is compressed, all entries within it are grouped together into a compressed block of data. This obscures the start of each individual child entry. Avoid reporting bogus 'image-pos' properties in this case, since it is not possible to access the entry at the location provided. The entire section must be decompressed first. CBFS does not support compressing whole sections, only individual files, so needs no special handling here. Signed-off-by: Simon Glass --- tools/binman/control.py | 2 +- tools/binman/entry.py | 18 ++++++++++++++---- tools/binman/etype/cbfs.py | 6 +++--- tools/binman/etype/section.py | 13 ++++++++----- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/tools/binman/control.py b/tools/binman/control.py index ee5771e729..26f1cf462e 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -462,7 +462,7 @@ def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt): for image in images.values(): image.ExpandEntries() if update_fdt: - image.AddMissingProperties() + image.AddMissingProperties(True) image.ProcessFdt(dtb) for dtb_item in state.GetAllFdts(): diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 01a5fde84e..8fa1dcef2d 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -213,11 +213,20 @@ class Entry(object): def ExpandEntries(self): pass - def AddMissingProperties(self): - """Add new properties to the device tree as needed for this entry""" - for prop in ['offset', 'size', 'image-pos']: + def AddMissingProperties(self, have_image_pos): + """Add new properties to the device tree as needed for this entry + + Args: + have_image_pos: True if this entry has an image position. This can + be False if its parent section is compressed, since compression + groups all entries together into a compressed block of data, + obscuring the start of each individual child entry + """ + for prop in ['offset', 'size']: if not prop in self._node.props: state.AddZeroProp(self._node, prop) + if have_image_pos and 'image-pos' not in self._node.props: + state.AddZeroProp(self._node, 'image-pos') if self.GetImage().allow_repack: if self.orig_offset is not None: state.AddZeroProp(self._node, 'orig-offset', True) @@ -235,7 +244,8 @@ class Entry(object): state.SetInt(self._node, 'offset', self.offset) state.SetInt(self._node, 'size', self.size) base = self.section.GetRootSkipAtStart() if self.section else 0 - state.SetInt(self._node, 'image-pos', self.image_pos - base) + if self.image_pos is not None: + state.SetInt(self._node, 'image-pos', self.image_pos) if self.GetImage().allow_repack: if self.orig_offset is not None: state.SetInt(self._node, 'orig-offset', self.orig_offset, True) diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py index 650ab2c292..6cdbaa085f 100644 --- a/tools/binman/etype/cbfs.py +++ b/tools/binman/etype/cbfs.py @@ -237,10 +237,10 @@ class Entry_cbfs(Entry): if entry._cbfs_compress: entry.uncomp_size = cfile.memlen - def AddMissingProperties(self): - super().AddMissingProperties() + def AddMissingProperties(self, have_image_pos): + super().AddMissingProperties(have_image_pos) for entry in self._cbfs_entries.values(): - entry.AddMissingProperties() + entry.AddMissingProperties(have_image_pos) if entry._cbfs_compress: state.AddZeroProp(entry._node, 'uncomp-size') # Store the 'compress' property, since we don't look at diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 6e6f674972..2812989ba1 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -136,11 +136,13 @@ class Entry_section(Entry): for entry in self._entries.values(): entry.ExpandEntries() - def AddMissingProperties(self): + def AddMissingProperties(self, have_image_pos): """Add new properties to the device tree as needed for this entry""" - super().AddMissingProperties() + super().AddMissingProperties(have_image_pos) + if self.compress != 'none': + have_image_pos = False for entry in self._entries.values(): - entry.AddMissingProperties() + entry.AddMissingProperties(have_image_pos) def ObtainContents(self): return self.GetEntryContents() @@ -323,8 +325,9 @@ class Entry_section(Entry): def SetImagePos(self, image_pos): super().SetImagePos(image_pos) - for entry in self._entries.values(): - entry.SetImagePos(image_pos + self.offset) + if self.compress == 'none': + for entry in self._entries.values(): + entry.SetImagePos(image_pos + self.offset) def ProcessContents(self): sizes_ok_base = super(Entry_section, self).ProcessContents() From 6ddd61131f3206c5e385dc7e763aea0dc5caa24c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:18 -0600 Subject: [PATCH 63/71] binman: Drop Entry.CheckOffset() This function just calls CheckEntries() in the only non-trivial implementation. Drop it and use CheckEntries() directly. Signed-off-by: Simon Glass --- tools/binman/entry.py | 2 +- tools/binman/etype/section.py | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 8fa1dcef2d..8946d2bc02 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -520,7 +520,7 @@ class Entry(object): """ pass - def CheckOffset(self): + def CheckEntries(self): """Check that the entry offsets are correct This is used for entries which have extra offset requirements (other diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 2812989ba1..fb4bf640bf 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -296,7 +296,7 @@ class Entry_section(Entry): offset = 0 prev_name = 'None' for entry in self._entries.values(): - entry.CheckOffset() + entry.CheckEntries() if (entry.offset < self._skip_at_start or entry.offset + entry.size > self._skip_at_start + self.size): @@ -337,9 +337,6 @@ class Entry_section(Entry): sizes_ok = False return sizes_ok and sizes_ok_base - def CheckOffset(self): - self.CheckEntries() - def WriteMap(self, fd, indent): """Write a map of the section to a .map file From c1af7a86b380c75c0718c80e1997293913c6095a Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:19 -0600 Subject: [PATCH 64/71] binman: Move sort and expand to the main Pack() function At present sorting and expanding entries are side-effects of the CheckEntries() function. This is a bit confusing, as 'checking' would not normally involve making changes. Move these steps into the Pack() function instead. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index fb4bf640bf..c883f0d67c 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -260,6 +260,10 @@ class Entry_section(Entry): def Pack(self, offset): """Pack all entries into the section""" self._PackEntries() + if self._sort: + self._SortEntries() + self._ExpandEntries() + return super().Pack(offset) def _PackEntries(self): @@ -290,9 +294,6 @@ class Entry_section(Entry): def CheckEntries(self): """Check that entries do not overlap or extend outside the section""" - if self._sort: - self._SortEntries() - self._ExpandEntries() offset = 0 prev_name = 'None' for entry in self._entries.values(): From 601b69aa8a56168ff25988bbf9cd2d457f13861d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:20 -0600 Subject: [PATCH 65/71] binman: Drop the Entry.CheckSize() method This is only used by entry_Section and that class already calls it. Avoid calling it twice. Also drop it from the documentation. Signed-off-by: Simon Glass --- tools/binman/README | 21 ++++++++++----------- tools/binman/control.py | 1 - 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/tools/binman/README b/tools/binman/README index 0dee71d1b2..c7c787c99f 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -712,41 +712,40 @@ size of an entry. The 'current' image offset is passed in, and the function returns the offset immediately after the entry being packed. The default implementation of Pack() is usually sufficient. -6. CheckSize() - checks that the contents of all the entries fits within -the image size. If the image does not have a defined size, the size is set -large enough to hold all the entries. +Note: for sections, this also sets the size of the entry to be large enough +for all entries it contains. -7. CheckEntries() - checks that the entries do not overlap, nor extend +6. CheckEntries() - checks that the entries do not overlap, nor extend outside the image. -8. SetImagePos() - sets the image position of every entry. This is the absolute +7. SetImagePos() - sets the image position of every entry. This is the absolute position 'image-pos', as opposed to 'offset' which is relative to the containing section. This must be done after all offsets are known, which is why it is quite late in the ordering. -9. SetCalculatedProperties() - update any calculated properties in the device +8. SetCalculatedProperties() - update any calculated properties in the device tree. This sets the correct 'offset' and 'size' vaues, for example. -10. ProcessEntryContents() - this calls Entry.ProcessContents() on each entry. +9. ProcessEntryContents() - this calls Entry.ProcessContents() on each entry. The default implementatoin does nothing. This can be overriden to adjust the contents of an entry in some way. For example, it would be possible to create an entry containing a hash of the contents of some other entries. At this stage the offset and size of entries should not be adjusted unless absolutely necessary, since it requires a repack (going back to PackEntries()). -11. ResetForPack() - if the ProcessEntryContents() step failed, in that an entry +10. ResetForPack() - if the ProcessEntryContents() step failed, in that an entry has changed its size, then there is no alternative but to go back to step 5 and try again, repacking the entries with the updated size. ResetForPack() removes the fixed offset/size values added by binman, so that the packing can start from scratch. -12. WriteSymbols() - write the value of symbols into the U-Boot SPL binary. +11. WriteSymbols() - write the value of symbols into the U-Boot SPL binary. See 'Access to binman entry offsets at run time' below for a description of what happens in this stage. -13. BuildImage() - builds the image and writes it to a file +12. BuildImage() - builds the image and writes it to a file -14. WriteMap() - writes a text file containing a map of the image. This is the +13. WriteMap() - writes a text file containing a map of the image. This is the final step. diff --git a/tools/binman/control.py b/tools/binman/control.py index 26f1cf462e..9eeac5db99 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -513,7 +513,6 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True, for pack_pass in range(passes): try: image.PackEntries() - image.CheckSize() image.CheckEntries() except Exception as e: if write_map: From 93f3c2ea1463f5ad4fff9ada8675078d326db347 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:21 -0600 Subject: [PATCH 66/71] binman: Call CheckSize() from the section's Pack() method At present CheckSize() is called from the function that packs the entries. Move it up to the main Pack() function so that _PackEntries() can just do the packing. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index c883f0d67c..f93469a170 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -264,6 +264,9 @@ class Entry_section(Entry): self._SortEntries() self._ExpandEntries() + size = self.CheckSize() + self.size = size + return super().Pack(offset) def _PackEntries(self): @@ -271,7 +274,7 @@ class Entry_section(Entry): offset = self._skip_at_start for entry in self._entries.values(): offset = entry.Pack(offset) - self.size = self.CheckSize() + return offset def _ExpandEntries(self): """Expand any entries that are permitted to""" From 0b65769c2e31ae7b3ee6015b1f7602e4e1ac56a6 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:22 -0600 Subject: [PATCH 67/71] binman: Drop CheckEntries() This method introduces a separation between packing and checking that is different for sections. In order to handle compression properly, we need to be able to deal with a section's size being smaller than the uncompressed size of its contents. It is easier to make this work if everything happens in the Pack() method. The only real user of CheckEntries() is entry_Section and it can call it directly. Drop the call from 'control' and handle it locally. Signed-off-by: Simon Glass --- tools/binman/README | 22 ++++++++++------------ tools/binman/control.py | 1 - tools/binman/etype/section.py | 4 +++- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/tools/binman/README b/tools/binman/README index c7c787c99f..c14cee5d11 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -712,40 +712,38 @@ size of an entry. The 'current' image offset is passed in, and the function returns the offset immediately after the entry being packed. The default implementation of Pack() is usually sufficient. -Note: for sections, this also sets the size of the entry to be large enough -for all entries it contains. +Note: for sections, this also checks that the entries do not overlap, nor extend +outside the section. If the section does not have a defined size, the size is +set large enough to hold all the entries. -6. CheckEntries() - checks that the entries do not overlap, nor extend -outside the image. - -7. SetImagePos() - sets the image position of every entry. This is the absolute +6. SetImagePos() - sets the image position of every entry. This is the absolute position 'image-pos', as opposed to 'offset' which is relative to the containing section. This must be done after all offsets are known, which is why it is quite late in the ordering. -8. SetCalculatedProperties() - update any calculated properties in the device +7. SetCalculatedProperties() - update any calculated properties in the device tree. This sets the correct 'offset' and 'size' vaues, for example. -9. ProcessEntryContents() - this calls Entry.ProcessContents() on each entry. +8. ProcessEntryContents() - this calls Entry.ProcessContents() on each entry. The default implementatoin does nothing. This can be overriden to adjust the contents of an entry in some way. For example, it would be possible to create an entry containing a hash of the contents of some other entries. At this stage the offset and size of entries should not be adjusted unless absolutely necessary, since it requires a repack (going back to PackEntries()). -10. ResetForPack() - if the ProcessEntryContents() step failed, in that an entry +9. ResetForPack() - if the ProcessEntryContents() step failed, in that an entry has changed its size, then there is no alternative but to go back to step 5 and try again, repacking the entries with the updated size. ResetForPack() removes the fixed offset/size values added by binman, so that the packing can start from scratch. -11. WriteSymbols() - write the value of symbols into the U-Boot SPL binary. +10. WriteSymbols() - write the value of symbols into the U-Boot SPL binary. See 'Access to binman entry offsets at run time' below for a description of what happens in this stage. -12. BuildImage() - builds the image and writes it to a file +11. BuildImage() - builds the image and writes it to a file -13. WriteMap() - writes a text file containing a map of the image. This is the +12. WriteMap() - writes a text file containing a map of the image. This is the final step. diff --git a/tools/binman/control.py b/tools/binman/control.py index 9eeac5db99..072417f364 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -513,7 +513,6 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True, for pack_pass in range(passes): try: image.PackEntries() - image.CheckEntries() except Exception as e: if write_map: fname = image.WriteMap() diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index f93469a170..1618bebe4b 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -267,7 +267,9 @@ class Entry_section(Entry): size = self.CheckSize() self.size = size - return super().Pack(offset) + offset = super().Pack(offset) + self.CheckEntries() + return offset def _PackEntries(self): """Pack all entries into the section""" From b004bf3906e6f80b80558424e4a3ed8767103e37 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:23 -0600 Subject: [PATCH 68/71] binman: Update CheckEntries() for compressed sections At present this function assumes that the size of a section is at least as large as its contents. With compression this is often not the case. Relax this constraint by using the uncompressed size, if available. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 1618bebe4b..b146239b77 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -299,19 +299,21 @@ class Entry_section(Entry): def CheckEntries(self): """Check that entries do not overlap or extend outside the section""" + max_size = self.size if self.uncomp_size is None else self.uncomp_size + offset = 0 prev_name = 'None' for entry in self._entries.values(): entry.CheckEntries() if (entry.offset < self._skip_at_start or entry.offset + entry.size > self._skip_at_start + - self.size): + max_size): entry.Raise('Offset %#x (%d) size %#x (%d) is outside the ' "section '%s' starting at %#x (%d) " 'of size %#x (%d)' % (entry.offset, entry.offset, entry.size, entry.size, self._node.path, self._skip_at_start, - self._skip_at_start, self.size, self.size)) + self._skip_at_start, max_size, max_size)) if entry.offset < offset and entry.size: entry.Raise("Offset %#x (%d) overlaps with previous entry '%s' " "ending at %#x (%d)" % From 0ff83da634c4e4a8e510c5b2434cab88cb33c164 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:24 -0600 Subject: [PATCH 69/71] binman: Use the actual contents in CheckSize() At present this function adds up the total size of entries to work out the size of a section's contents. With compression this is no-longer enough. We may as well bite the bullet and build the section contents instead. Call _BuildSectionData() to get the (possibly compressed) contents and GetPaddedData() to get the same but with padding added. Note that this is inefficient since the section contents is calculated twice. Future work will improve this. This affects testPackOverlapMap() since the error is reported with a different section size now (enough to hold the contents). Update that at the same time. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 11 ++++------- tools/binman/ftest.py | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index b146239b77..570dbfcfd4 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -544,16 +544,13 @@ class Entry_section(Entry): def CheckSize(self): - """Check that the section contents does not exceed its size, etc.""" - contents_size = 0 - for entry in self._entries.values(): - contents_size = max(contents_size, entry.offset + entry.size) - - contents_size -= self._skip_at_start + data = self._BuildSectionData() + contents_size = len(data) size = self.size if not size: - size = self.pad_before + contents_size + self.pad_after + data = self.GetPaddedData() + size = len(data) size = tools.Align(size, self.align_size) if self.size and contents_size > self.size: diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 6f47deac3b..5bcdb70c41 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -2024,7 +2024,7 @@ class TestFunctional(unittest.TestCase): self.assertTrue(os.path.exists(map_fname)) map_data = tools.ReadFile(map_fname, binary=False) self.assertEqual('''ImagePos Offset Size Name - 00000000 00000007 main-section + 00000000 00000008 main-section 00000000 00000004 u-boot 00000003 00000004 u-boot-align ''', map_data) From 8f5ef89f00133df6381e60f0415269ded39647c1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:25 -0600 Subject: [PATCH 70/71] binman: Support compression of sections With the previous changes, it is now possible to compress entire sections. Add some tests to check that compression works correctly, including updating the metadata. Also update the documentation. Signed-off-by: Simon Glass --- tools/binman/README | 8 + tools/binman/ftest.py | 217 ++++++++++++++++++ tools/binman/test/182_compress_image.dts | 14 ++ tools/binman/test/183_compress_image_less.dts | 14 ++ .../binman/test/184_compress_section_size.dts | 17 ++ tools/binman/test/185_compress_section.dts | 16 ++ tools/binman/test/186_compress_extra.dts | 37 +++ 7 files changed, 323 insertions(+) create mode 100644 tools/binman/test/182_compress_image.dts create mode 100644 tools/binman/test/183_compress_image_less.dts create mode 100644 tools/binman/test/184_compress_section_size.dts create mode 100644 tools/binman/test/185_compress_section.dts create mode 100644 tools/binman/test/186_compress_extra.dts diff --git a/tools/binman/README b/tools/binman/README index c14cee5d11..de1eedfc3f 100644 --- a/tools/binman/README +++ b/tools/binman/README @@ -853,6 +853,14 @@ The entry will then contain the compressed data, using the 'lz4' compression algorithm. Currently this is the only one that is supported. The uncompressed size is written to the node in an 'uncomp-size' property, if -u is used. +Compression is also supported for sections. In that case the entire section is +compressed in one block, including all its contents. This means that accessing +an entry from the section required decompressing the entire section. Also, the +size of a section indicates the space that it consumes in its parent section +(and typically the image). With compression, the section may contain more data, +and the uncomp-size property indicates that, as above. The contents of the +section is compressed first, before any padding is added. This ensures that the +padding itself is not compressed, which would be a waste of time. Map files diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 5bcdb70c41..e753a342c8 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -70,6 +70,7 @@ VBLOCK_DATA = b'vblk' FILES_DATA = (b"sorry I'm late\nOh, don't bother apologising, I'm " + b"sorry you're alive\n") COMPRESS_DATA = b'compress xxxxxxxxxxxxxxxxxxxxxx data' +COMPRESS_DATA_BIG = COMPRESS_DATA * 2 REFCODE_DATA = b'refcode' FSP_M_DATA = b'fsp_m' FSP_S_DATA = b'fsp_s' @@ -175,6 +176,7 @@ class TestFunctional(unittest.TestCase): os.path.join(cls._indir, 'files')) TestFunctional._MakeInputFile('compress', COMPRESS_DATA) + TestFunctional._MakeInputFile('compress_big', COMPRESS_DATA_BIG) TestFunctional._MakeInputFile('bl31.bin', ATF_BL31_DATA) TestFunctional._MakeInputFile('scp.bin', SCP_DATA) @@ -3922,6 +3924,221 @@ class TestFunctional(unittest.TestCase): tools.GetBytes(ord('!'), 4)) # padding to section size self.assertEqual(expected, data) + def testCompressImage(self): + """Test compression of the entire image""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '182_compress_image.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + orig = self._decompress(data) + self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, orig) + + # Do a sanity check on various fields + image = control.images['image'] + entries = image.GetEntries() + self.assertEqual(2, len(entries)) + + entry = entries['blob'] + self.assertEqual(COMPRESS_DATA, entry.data) + self.assertEqual(len(COMPRESS_DATA), entry.size) + + entry = entries['u-boot'] + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(len(U_BOOT_DATA), entry.size) + + self.assertEqual(len(data), image.size) + self.assertEqual(COMPRESS_DATA + U_BOOT_DATA, image.uncomp_data) + self.assertEqual(len(COMPRESS_DATA + U_BOOT_DATA), image.uncomp_size) + orig = self._decompress(image.data) + self.assertEqual(orig, image.uncomp_data) + + expected = { + 'blob:offset': 0, + 'blob:size': len(COMPRESS_DATA), + 'u-boot:offset': len(COMPRESS_DATA), + 'u-boot:size': len(U_BOOT_DATA), + 'uncomp-size': len(COMPRESS_DATA + U_BOOT_DATA), + 'offset': 0, + 'image-pos': 0, + 'size': len(data), + } + self.assertEqual(expected, props) + + def testCompressImageLess(self): + """Test compression where compression reduces the image size""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '183_compress_image_less.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + orig = self._decompress(data) + + self.assertEquals(COMPRESS_DATA + COMPRESS_DATA + U_BOOT_DATA, orig) + + # Do a sanity check on various fields + image = control.images['image'] + entries = image.GetEntries() + self.assertEqual(2, len(entries)) + + entry = entries['blob'] + self.assertEqual(COMPRESS_DATA_BIG, entry.data) + self.assertEqual(len(COMPRESS_DATA_BIG), entry.size) + + entry = entries['u-boot'] + self.assertEqual(U_BOOT_DATA, entry.data) + self.assertEqual(len(U_BOOT_DATA), entry.size) + + self.assertEqual(len(data), image.size) + self.assertEqual(COMPRESS_DATA_BIG + U_BOOT_DATA, image.uncomp_data) + self.assertEqual(len(COMPRESS_DATA_BIG + U_BOOT_DATA), + image.uncomp_size) + orig = self._decompress(image.data) + self.assertEqual(orig, image.uncomp_data) + + expected = { + 'blob:offset': 0, + 'blob:size': len(COMPRESS_DATA_BIG), + 'u-boot:offset': len(COMPRESS_DATA_BIG), + 'u-boot:size': len(U_BOOT_DATA), + 'uncomp-size': len(COMPRESS_DATA_BIG + U_BOOT_DATA), + 'offset': 0, + 'image-pos': 0, + 'size': len(data), + } + self.assertEqual(expected, props) + + def testCompressSectionSize(self): + """Test compression of a section with a fixed size""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '184_compress_section_size.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + orig = self._decompress(data) + self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, orig) + expected = { + 'section/blob:offset': 0, + 'section/blob:size': len(COMPRESS_DATA), + 'section/u-boot:offset': len(COMPRESS_DATA), + 'section/u-boot:size': len(U_BOOT_DATA), + 'section:offset': 0, + 'section:image-pos': 0, + 'section:uncomp-size': len(COMPRESS_DATA + U_BOOT_DATA), + 'section:size': 0x30, + 'offset': 0, + 'image-pos': 0, + 'size': 0x30, + } + self.assertEqual(expected, props) + + def testCompressSection(self): + """Test compression of a section with no fixed size""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '185_compress_section.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + orig = self._decompress(data) + self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, orig) + expected = { + 'section/blob:offset': 0, + 'section/blob:size': len(COMPRESS_DATA), + 'section/u-boot:offset': len(COMPRESS_DATA), + 'section/u-boot:size': len(U_BOOT_DATA), + 'section:offset': 0, + 'section:image-pos': 0, + 'section:uncomp-size': len(COMPRESS_DATA + U_BOOT_DATA), + 'section:size': len(data), + 'offset': 0, + 'image-pos': 0, + 'size': len(data), + } + self.assertEqual(expected, props) + + def testCompressExtra(self): + """Test compression of a section with no fixed size""" + self._CheckLz4() + data, _, _, out_dtb_fname = self._DoReadFileDtb( + '186_compress_extra.dts', use_real_dtb=True, update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, ['offset', 'image-pos', 'size', + 'uncomp-size']) + + base = data[len(U_BOOT_DATA):] + self.assertEquals(U_BOOT_DATA, base[:len(U_BOOT_DATA)]) + rest = base[len(U_BOOT_DATA):] + + # Check compressed data + section1 = self._decompress(rest) + expect1 = tools.Compress(COMPRESS_DATA + U_BOOT_DATA, 'lz4') + self.assertEquals(expect1, rest[:len(expect1)]) + self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, section1) + rest1 = rest[len(expect1):] + + section2 = self._decompress(rest1) + expect2 = tools.Compress(COMPRESS_DATA + COMPRESS_DATA, 'lz4') + self.assertEquals(expect2, rest1[:len(expect2)]) + self.assertEquals(COMPRESS_DATA + COMPRESS_DATA, section2) + rest2 = rest1[len(expect2):] + + expect_size = (len(U_BOOT_DATA) + len(U_BOOT_DATA) + len(expect1) + + len(expect2) + len(U_BOOT_DATA)) + #self.assertEquals(expect_size, len(data)) + + #self.assertEquals(U_BOOT_DATA, rest2) + + self.maxDiff = None + expected = { + 'u-boot:offset': 0, + 'u-boot:image-pos': 0, + 'u-boot:size': len(U_BOOT_DATA), + + 'base:offset': len(U_BOOT_DATA), + 'base:image-pos': len(U_BOOT_DATA), + 'base:size': len(data) - len(U_BOOT_DATA), + 'base/u-boot:offset': 0, + 'base/u-boot:image-pos': len(U_BOOT_DATA), + 'base/u-boot:size': len(U_BOOT_DATA), + 'base/u-boot2:offset': len(U_BOOT_DATA) + len(expect1) + + len(expect2), + 'base/u-boot2:image-pos': len(U_BOOT_DATA) * 2 + len(expect1) + + len(expect2), + 'base/u-boot2:size': len(U_BOOT_DATA), + + 'base/section:offset': len(U_BOOT_DATA), + 'base/section:image-pos': len(U_BOOT_DATA) * 2, + 'base/section:size': len(expect1), + 'base/section:uncomp-size': len(COMPRESS_DATA + U_BOOT_DATA), + 'base/section/blob:offset': 0, + 'base/section/blob:size': len(COMPRESS_DATA), + 'base/section/u-boot:offset': len(COMPRESS_DATA), + 'base/section/u-boot:size': len(U_BOOT_DATA), + + 'base/section2:offset': len(U_BOOT_DATA) + len(expect1), + 'base/section2:image-pos': len(U_BOOT_DATA) * 2 + len(expect1), + 'base/section2:size': len(expect2), + 'base/section2:uncomp-size': len(COMPRESS_DATA + COMPRESS_DATA), + 'base/section2/blob:offset': 0, + 'base/section2/blob:size': len(COMPRESS_DATA), + 'base/section2/blob2:offset': len(COMPRESS_DATA), + 'base/section2/blob2:size': len(COMPRESS_DATA), + + 'offset': 0, + 'image-pos': 0, + 'size': len(data), + } + self.assertEqual(expected, props) + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/182_compress_image.dts b/tools/binman/test/182_compress_image.dts new file mode 100644 index 0000000000..4176b7f2e6 --- /dev/null +++ b/tools/binman/test/182_compress_image.dts @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + compress = "lz4"; + blob { + filename = "compress"; + }; + + u-boot { + }; + }; +}; diff --git a/tools/binman/test/183_compress_image_less.dts b/tools/binman/test/183_compress_image_less.dts new file mode 100644 index 0000000000..1d9d57b78c --- /dev/null +++ b/tools/binman/test/183_compress_image_less.dts @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + compress = "lz4"; + blob { + filename = "compress_big"; + }; + + u-boot { + }; + }; +}; diff --git a/tools/binman/test/184_compress_section_size.dts b/tools/binman/test/184_compress_section_size.dts new file mode 100644 index 0000000000..95ed30add1 --- /dev/null +++ b/tools/binman/test/184_compress_section_size.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + section { + size = <0x30>; + compress = "lz4"; + blob { + filename = "compress"; + }; + + u-boot { + }; + }; + }; +}; diff --git a/tools/binman/test/185_compress_section.dts b/tools/binman/test/185_compress_section.dts new file mode 100644 index 0000000000..dc3e340c5d --- /dev/null +++ b/tools/binman/test/185_compress_section.dts @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + section { + compress = "lz4"; + blob { + filename = "compress"; + }; + + u-boot { + }; + }; + }; +}; diff --git a/tools/binman/test/186_compress_extra.dts b/tools/binman/test/186_compress_extra.dts new file mode 100644 index 0000000000..59aae82263 --- /dev/null +++ b/tools/binman/test/186_compress_extra.dts @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + u-boot { + }; + base { + type = "section"; + u-boot { + }; + section { + compress = "lz4"; + blob { + filename = "compress"; + }; + + u-boot { + }; + }; + section2 { + type = "section"; + compress = "lz4"; + blob { + filename = "compress"; + }; + blob2 { + type = "blob"; + filename = "compress"; + }; + }; + u-boot2 { + type = "u-boot"; + }; + }; + }; +}; From 2424057b2ad0eacdd2d81e35e7bea5df97802b8f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:26 -0600 Subject: [PATCH 71/71] binman: Avoid calculated section data repeatedly Refactor the implementation slightly so that section data is not rebuilt when it is already available. We still have GetData() set up to rebuild the section, since we don't currently track when things change that might affect a section. For example, if a blob is updated within a section, we must rebuild it. Tracking that would be possible but is more complex, so it left for another time. Signed-off-by: Simon Glass --- tools/binman/etype/section.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 570dbfcfd4..3dd5f58c4c 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -147,7 +147,7 @@ class Entry_section(Entry): def ObtainContents(self): return self.GetEntryContents() - def GetPaddedDataForEntry(self, entry): + def GetPaddedDataForEntry(self, entry, entry_data): """Get the data for an entry including any padding Gets the entry data and uses the section pad-byte value to add padding @@ -170,7 +170,7 @@ class Entry_section(Entry): data += tools.GetBytes(self._pad_byte, entry.pad_before) # Add in the actual entry data - data += entry.GetData() + data += entry_data # Handle padding after the entry if entry.pad_after: @@ -197,7 +197,7 @@ class Entry_section(Entry): section_data = b'' for entry in self._entries.values(): - data = self.GetPaddedDataForEntry(entry) + data = self.GetPaddedDataForEntry(entry, entry.GetData()) # Handle empty space before the entry pad = (entry.offset or 0) - self._skip_at_start - len(section_data) if pad > 0: @@ -210,7 +210,7 @@ class Entry_section(Entry): (len(self._entries), len(section_data))) return self.CompressData(section_data) - def GetPaddedData(self): + def GetPaddedData(self, data=None): """Get the data for a section including any padding Gets the section data and uses the parent section's pad-byte value to @@ -225,7 +225,9 @@ class Entry_section(Entry): after it (bytes) """ section = self.section or self - return section.GetPaddedDataForEntry(self) + if data is None: + data = self.GetData() + return section.GetPaddedDataForEntry(self, data) def GetData(self): """Get the contents of an entry @@ -264,8 +266,10 @@ class Entry_section(Entry): self._SortEntries() self._ExpandEntries() - size = self.CheckSize() - self.size = size + data = self._BuildSectionData() + self.SetContents(data) + + self.CheckSize() offset = super().Pack(offset) self.CheckEntries() @@ -542,14 +546,12 @@ class Entry_section(Entry): for name, info in offset_dict.items(): self._SetEntryOffsetSize(name, *info) - def CheckSize(self): - data = self._BuildSectionData() - contents_size = len(data) + contents_size = len(self.data) size = self.size if not size: - data = self.GetPaddedData() + data = self.GetPaddedData(self.data) size = len(data) size = tools.Align(size, self.align_size)