From f4f4123708cd3699aa3981afca9511beb760c7fb Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 27 Sep 2020 18:46:19 -0600 Subject: 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 e0b9207..8b89a96 100644 --- a/include/binman.h +++ b/include/binman.h @@ -43,6 +43,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 * * This searches the binman information in the device tree for a symbol of the diff --git a/lib/binman.c b/lib/binman.c index 7a8ad62..79e497f 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)); -- cgit v1.1 From 83187546ae14e266c745c3bb24db492eb5433b89 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 27 Sep 2020 18:46:20 -0600 Subject: 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 79e497f..e71c1b9 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; -- cgit v1.1 From 26e408fe121016b8cfacddc03a23093bf867e7a2 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:18 -0600 Subject: 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 579a674..bd1daa4 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') -- cgit v1.1 From 97136eb5354c28c475cd2fb72d5cc1029ce9e743 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:19 -0600 Subject: 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 bd1daa4..1b52198 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())) -- cgit v1.1 From 3c14083f20679c526f3a133b2f71f0aad996fdc8 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:20 -0600 Subject: 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 cab93b0..25479b0 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) -- cgit v1.1 From 08c3b88dd145d3f7f06e7ad8458905bde7a286ef Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:21 -0600 Subject: 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 924d51f..eed8273 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 -- cgit v1.1 From a652d9c73a6eea1fdfb901c66178a4d804fac95d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:22 -0600 Subject: 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 b9217b2..0829622 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 10f4bec..5edd4e4 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 0726be6..de23161 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 0157af1..cadfc05 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 6bef73b..5cb6f44 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 38b7b148..9959887 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); -- cgit v1.1 From 1cadc7611755bcd5e28e98515f4814e598196daa Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:23 -0600 Subject: 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 7c40399..13ee661 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 -- cgit v1.1 From 82c468a049d8887b70b1826c9fda2388f14704ef Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:24 -0600 Subject: 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 13ee661..73bddb4 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 -- cgit v1.1 From 16a5068340f8d28e26436acf64e4dc5f652aa2a6 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:25 -0600 Subject: 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 5b80eb2..89c42c1 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 73bddb4..ed3e882 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 -- cgit v1.1 From 627988f9f9b5533cedb3e9d2de0e0b905bb3f45f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 09:25:26 -0600 Subject: 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 | 11 +++++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/test/Makefile b/test/Makefile index ed3e882..1c930b3 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 8b3d77e..c0dc632 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -2,15 +2,16 @@ # # Copyright (c) 2013 Google, Inc +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),) 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. 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 -- cgit v1.1 From c14732984759d64634350d01d58daca8cf44d23f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:23 -0600 Subject: 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 937ce28..18a605d 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 f09d291..db2fbac 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 69c22a7..71cabd1 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 -- cgit v1.1 From e4fb5faa044ffc66170f8776794386e5e11e13a2 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:24 -0600 Subject: 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 1b52198..72725bb 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') -- cgit v1.1 From 1b27273e090590e7944a3f7a3b33ac9f90aae765 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:25 -0600 Subject: 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 72725bb..31a9b38 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 c2ff267..857a98e 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, }; -- cgit v1.1 From abb9cd30b23858125ce015c1dffdae4d7895bae8 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:26 -0600 Subject: 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 07d3a6a..ffae6f9 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 25b0d76..91e93b0 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 31da621..af7e3e4 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 9773db6..01a0449 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, ...) -- cgit v1.1 From e144cafe43c8298bd41c044329857c3068cd845b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:27 -0600 Subject: 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 263334b..1315493 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 11bfc4c..fd168cb 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 d058c59..03b8677 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 857a98e..8dcac91 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 b4f9b7f..cfe3e04 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'] -- cgit v1.1 From 18bca1f172473649aed03a9fa797c453b2a04ee4 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Fri, 9 Oct 2020 23:44:11 +0200 Subject: 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 7dc3dab..d4dab36 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()); -- cgit v1.1 From 970cd91e8c4d9c812857601475dc2972ad1cd1b4 Mon Sep 17 00:00:00 2001 From: Dario Binacchi Date: Sun, 11 Oct 2020 14:27:07 +0200 Subject: 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 ac3b6c1..993c9e6 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. -- cgit v1.1 From ba96be48ad34180debcfbc11434be7329d530701 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:26 -0600 Subject: 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 1d49e81..f59e4f2 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 d528c99..2e3a443 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 28704a2..2646e7d 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 -- cgit v1.1 From 5b448ce687c0f34b0b55ffe5785b14d762b2296d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:27 -0600 Subject: 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 9959887..5560572 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; } -- cgit v1.1 From b25ff5cbaaaa7f144eb6e087b4bdd7362c58029d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:28 -0600 Subject: 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 7ab8919..48fd126 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 c6a2bbe..f5e104b 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 1bfad30..f828d9d 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 67c7d69..03e2929 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 5560572..9d22df8 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); -- cgit v1.1 From 217293e399148e704d6c318b19b1f69959831f7b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:29 -0600 Subject: 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 c0dc632..57e13ad 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 0000000..7a864eb --- /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); -- cgit v1.1 From 60251b1d536abec8e163a00c41d801d69faa429c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:30 -0600 Subject: 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 2e3a443..9f1f744 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 $@ -- cgit v1.1 From bc84d585ec0edf1705832fd431fc6a0ded978524 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:31 -0600 Subject: 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 3092047..dc92c0b 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 0000000..990cc9b --- /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() -- cgit v1.1 From 7b51bf770af349aa03f49cb9f445173ff2dc7fc7 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:32 -0600 Subject: 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 de87e75..735628e 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 -- cgit v1.1 From 22b29cc8fb15f611e8e2af6fde8627a32abea76d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:33 -0600 Subject: 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 48fd126..81b217a 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 f5e104b..569dafb 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 f828d9d..7547602 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] -- cgit v1.1 From e1e54ffe9956449d79697d31648217065e033045 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:34 -0600 Subject: 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 990cc9b..bd273da 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: -- cgit v1.1 From a8b1fbc14d064d7b14bf887730300f4dbf856473 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:35 -0600 Subject: 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 81b217a..9a77da1 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 569dafb..58ada13 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 7547602..bca1306 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 9990316..3ae6707 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 1315493..78837a3 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""" -- cgit v1.1 From afb26ba9a7c0812c56d40ba00554bd1e2f3be460 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 25 Oct 2020 20:38:36 -0600 Subject: 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 473ddee..a78c8d6 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 9ac2b33..b1e0b3b 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 fb8f731..cb48ff3 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" -- cgit v1.1 From b325248c9368c518d68d00d700404eeac801c98d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:28 -0600 Subject: 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 7a864eb..8763005 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); -- cgit v1.1 From 36af37b9367677f16b09c7d57fb84674979a7a2b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:31 -0600 Subject: 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 e90d701..746c619 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 8763005..80900e4 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); -- cgit v1.1 From 88280529bddf0bd05c90db42b6c8e48de954cf66 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:32 -0600 Subject: 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 0faad3f..6a0338b 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 f59e4f2..db723cd 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 55e1f8c..f86b4a0 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 768fbb7..0ff1b49 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 80900e4..57f9036 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); -- cgit v1.1 From a294ead8d2531a641f87bf182fee257029973ac0 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:33 -0600 Subject: 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 746c619..2e5767e 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 5beba91..2e6bd50 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 de23161..e8df5ae 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 cadfc05..0aa1144 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 1dcc22f..c5d7ec0 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 25479b0..2c3cc90 100644 --- a/include/dm/platdata.h +++ b/include/dm/platdata.h @@ -22,18 +22,28 @@ * @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 * available). U-Boot's driver model uses device tree for configuration. diff --git a/test/dm/of_platdata.c b/test/dm/of_platdata.c index 57f9036..e827d45 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)); } -- cgit v1.1 From 67507e4aab5b58c62cd57b855ce97a83d882479c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:34 -0600 Subject: 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 bd248cb..b371980 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 1b70e14..84b6a21 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 8035f7f..b406bab 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 }; -- cgit v1.1 From e41651fffda7da55f6d74afdf4b784088184c543 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:35 -0600 Subject: 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 2e6bd50..b23ee30 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 86ea8ce..aeda542 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 2c3cc90..f800a866 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 31a9b38..8832e6e 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 8dcac91..fee9853 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) -- cgit v1.1 From fbe27a54ebbe7433bbccf242f4edda61e2c1ba3e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:36 -0600 Subject: 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 20f6893..8b50a40 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 6a0338b..81cdc55 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 db723cd..d47a625 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 57b1c60..2cbdaf9 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 852770a..d0864b1 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 e827d45..bad733f 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 -- cgit v1.1 From 9e948b9b99d17ea6c2204a1b6251a5c33ee83dd9 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:37 -0600 Subject: 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 e8df5ae..5f10d7a 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) { -- cgit v1.1 From d07f31aea07ec642e8072fe0b83ba56ec83cc561 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:38 -0600 Subject: 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 35a425c..c6c1350 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 e1336b8..a91cb78 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); -- cgit v1.1 From bb44ebdd0f3eccece2081ec51cf3b3554dafd801 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:39 -0600 Subject: 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 | 27 ++++++++++++++------------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/arch/x86/cpu/apollolake/spl.c b/arch/x86/cpu/apollolake/spl.c index 5a53831..089b37c 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 b5219df..12abcff 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; - - /* - * 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; + 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. + */ + dev->parent_platdata = malloc(sizeof(*pplat)); + if (!dev->parent_platdata) + return -ENOMEM; + } pplat = dev_get_parent_platdata(dev); pplat->pid = portid; -- cgit v1.1 From 8a38abfc43f94a92b63e428738714111710bda53 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:40 -0600 Subject: 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 31c5997..ac954a3 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 2e5767e..4b3dcb3 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 94fa233..24b2796 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 1c015ab..22040c6 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 993c9e6..5bef484 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -554,6 +554,20 @@ 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 * * @parent: Parent device to search diff --git a/include/dt-structs.h b/include/dt-structs.h index eed8273..f0e1c9c 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 bad733f..4f3cc15 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 9d22df8..fd24635 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 8832e6e..2be11ff 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 fee9853..8e16dc0 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) -- cgit v1.1 From 4e28a259fd3a2025f42ad005b375aea2912c81de Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 11:08:25 -0600 Subject: 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 e129286..a636107 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, -- cgit v1.1 From cb43ac184f71f0ba696d0effcd39a746a6f3a456 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:41 -0600 Subject: 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 f800a866..216efa8 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 2be11ff..9b27aec 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 8e16dc0..a5836e0 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 -- cgit v1.1 From 7b82ce882c3871636fd18205e4684896104b740d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 3 Oct 2020 11:31:42 -0600 Subject: 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 1e3fad1..5848166 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 -- cgit v1.1 From f3243303a01eb58eef2a0cd320d562a7af486ddb Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:39:59 -0600 Subject: 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 bdb4fd6..13b930d 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 -------------------------------------- -- cgit v1.1 From a4dfe3e473ea53c44db50d07c34815e24069d39d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:00 -0600 Subject: 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 75f6ca3..d23967e 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 -- cgit v1.1 From f2c0dd85ad6cbf8b53ad5fc423a0177a0eb7fe6f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:01 -0600 Subject: 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 ++++++++ .../binman/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 d23967e..adc1603 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 0000000..021460b --- /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 0000000..deda3c8 --- /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 0000000..bf2f8f6 --- /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 { + }; + }; + }; +}; -- cgit v1.1 From 680e3c6edb9aa5cf400a5b22ecfa3a40b0b247e5 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:02 -0600 Subject: 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 bb4d9d1..c007d0a 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', -- cgit v1.1 From a81294671ca190102a06db8ac3004dae7e49aa46 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:03 -0600 Subject: 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 4462293..b4cb8cd 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 -- cgit v1.1 From 72628cdf58ec2737637b41ec3dad4a30fe5090e0 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:04 -0600 Subject: 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 515c97f..3277504 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) -- cgit v1.1 From e6bed4f1812c1666eeab794694c4b3765f41c143 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:05 -0600 Subject: 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 3277504..a3e37c3 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 adc1603..4f7e226 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): -- cgit v1.1 From 87c962943aec7aef8849c60f018a5e7756e5b7ba Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:06 -0600 Subject: 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 f7adc3b..173c913 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 ecfb1e4..301ac55 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 -- cgit v1.1 From 9248c8d9c9674d705216e51161c1f0f473fc7fa3 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:07 -0600 Subject: 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 13b930d..999b776 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 9adb3af..ce3832e 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 a3e37c3..9222042 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 847b398..5aeead2 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"; }; }; }; -- cgit v1.1 From ef439ed191a0655a86f5b4035cec9cc57e97d8b1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:08 -0600 Subject: 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 4f7e226..481b8fd 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 0765707..1b31555 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>; }; }; -- cgit v1.1 From f90d906a275f85e7077d9a30ab82b20676b54645 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:09 -0600 Subject: 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 fbcfdc7..0433cab 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 173c913..e5d0aa5 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 481b8fd..35c1420 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 = { -- cgit v1.1 From 4eec34c91f84d6c3915b300f0688f9b055e04dda Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:10 -0600 Subject: 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 0433cab..0dee71d 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 e5d0aa5..0421129 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 35c1420..c99852d 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: -- cgit v1.1 From 17ea9f35e780d073820a770b8e65641761a31d1f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:11 -0600 Subject: 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 9222042..d05adf0 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 c99852d..82be997 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) -- cgit v1.1 From 4a655c9bd7d0e58206ed4417e8da31ad5da4926b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:12 -0600 Subject: 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 d05adf0..f804329 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 a8772c3..d65ab88 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)) -- cgit v1.1 From d1d3ad7d1fea137a6fe0709458947ca84ffb124c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:13 -0600 Subject: 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 f804329..7cbb500 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 82be997..31e93c6 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) -- cgit v1.1 From 7d398bb1c71580da2182f0b820d91950bf4b3d24 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:14 -0600 Subject: 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 7cbb500..c423a22 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 31e93c6..830b610 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 0000000..7e4ebf2 --- /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 0000000..90795d1 --- /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>; + }; + }; + }; +}; -- cgit v1.1 From 97c3e9a6faa4483a700b26988bc48c2f9efe8dd6 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:15 -0600 Subject: 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 0421129..d701eaf 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 830b610..4c94bea 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), -- cgit v1.1 From 63e7ba6c1820def06e7ba88ce357cb605285e70c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:16 -0600 Subject: 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 d701eaf..01a5fde 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 c423a22..6e6f674 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 4c94bea..6f47dea 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'] -- cgit v1.1 From a9fad07d4b862b4b5d52e04950a374371936f7eb Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:17 -0600 Subject: 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 ee5771e..26f1cf4 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 01a5fde..8fa1dce 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 650ab2c..6cdbaa0 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 6e6f674..2812989 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() -- cgit v1.1 From 6ddd61131f3206c5e385dc7e763aea0dc5caa24c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:18 -0600 Subject: 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 8fa1dce..8946d2b 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 2812989..fb4bf64 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 -- cgit v1.1 From c1af7a86b380c75c0718c80e1997293913c6095a Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:19 -0600 Subject: 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 fb4bf64..c883f0d 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(): -- cgit v1.1 From 601b69aa8a56168ff25988bbf9cd2d457f13861d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:20 -0600 Subject: 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 0dee71d..c7c787c 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 26f1cf4..9eeac5d 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: -- cgit v1.1 From 93f3c2ea1463f5ad4fff9ada8675078d326db347 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:21 -0600 Subject: 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 c883f0d..f93469a 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""" -- cgit v1.1 From 0b65769c2e31ae7b3ee6015b1f7602e4e1ac56a6 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:22 -0600 Subject: 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 c7c787c..c14cee5 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 9eeac5d..072417f 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 f93469a..1618beb 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""" -- cgit v1.1 From b004bf3906e6f80b80558424e4a3ed8767103e37 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:23 -0600 Subject: 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 1618beb..b146239 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)" % -- cgit v1.1 From 0ff83da634c4e4a8e510c5b2434cab88cb33c164 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:24 -0600 Subject: 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 b146239..570dbfc 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 6f47dea..5bcdb70 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) -- cgit v1.1 From 8f5ef89f00133df6381e60f0415269ded39647c1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:25 -0600 Subject: 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 ++ tools/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 c14cee5..de1eedf 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 5bcdb70..e753a34 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 0000000..4176b7f --- /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 0000000..1d9d57b --- /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 0000000..95ed30a --- /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 0000000..dc3e340 --- /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 0000000..59aae82 --- /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"; + }; + }; + }; +}; -- cgit v1.1 From 2424057b2ad0eacdd2d81e35e7bea5df97802b8f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 26 Oct 2020 17:40:26 -0600 Subject: 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 570dbfc..3dd5f58 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) -- cgit v1.1