Message ID | 20230316163442.3120729-1-peter.hoyes@arm.com |
---|---|
State | Superseded |
Delegated to: | Simon Glass |
Headers | show |
Series | fdt: Make fdt addr -q quieter | expand |
On 3/16/23 17:34, Peter Hoyes wrote: > From: Peter Hoyes <Peter.Hoyes@arm.com> > > 64597346 "fdt: Add -q option to fdt addr for distro_bootcmd" introduced > the -q option for fdt addr, which sets the current working fdt address > without printing any output. > > baf41410 "fdt: Show a message when the working FDT changes" made the > utility function set_working_fdt_addr (in cmd/fdt.c) output a message > on each invocation, even if called via fdt addr -q, in which case its > output is now slightly noisier. > > To fix this, move the printf outside of set_working_fdt_addr to three > call sites: > * bootm_find_images (the use case for which the additional output was > added in baf41410). > * fdt addr, but only if the 'quiet' argument is not set. > * fdt move. > > This also has the advantage of printing the specified address instead of > the translated address when using the sandbox. > > Remove assertions from the fdt addr test case when: > * Calling set_working_fdt_addr directly. > * Calling fdt addr with the -q argument. Why not just pass the 'quiet' flag to set_working_fdt_addr() to avoid duplication ?
On 17/03/2023 12:17, Marek Vasut wrote: > On 3/16/23 17:34, Peter Hoyes wrote: >> From: Peter Hoyes <Peter.Hoyes@arm.com> >> >> 64597346 "fdt: Add -q option to fdt addr for distro_bootcmd" introduced >> the -q option for fdt addr, which sets the current working fdt address >> without printing any output. >> >> baf41410 "fdt: Show a message when the working FDT changes" made the >> utility function set_working_fdt_addr (in cmd/fdt.c) output a message >> on each invocation, even if called via fdt addr -q, in which case its >> output is now slightly noisier. >> >> To fix this, move the printf outside of set_working_fdt_addr to three >> call sites: >> * bootm_find_images (the use case for which the additional output was >> added in baf41410). >> * fdt addr, but only if the 'quiet' argument is not set. >> * fdt move. >> >> This also has the advantage of printing the specified address instead of >> the translated address when using the sandbox. >> >> Remove assertions from the fdt addr test case when: >> * Calling set_working_fdt_addr directly. >> * Calling fdt addr with the -q argument. > > Why not just pass the 'quiet' flag to set_working_fdt_addr() to avoid > duplication ? I considered this too, but there are other machine-specific call sites and I was trying to avoid adding an extra argument everywhere just for logging. I'll send a v2 with a 'quiet' argument on Monday. Peter
On 3/17/23 18:27, Peter Hoyes wrote: > On 17/03/2023 12:17, Marek Vasut wrote: >> On 3/16/23 17:34, Peter Hoyes wrote: >>> From: Peter Hoyes <Peter.Hoyes@arm.com> >>> >>> 64597346 "fdt: Add -q option to fdt addr for distro_bootcmd" introduced >>> the -q option for fdt addr, which sets the current working fdt address >>> without printing any output. >>> >>> baf41410 "fdt: Show a message when the working FDT changes" made the >>> utility function set_working_fdt_addr (in cmd/fdt.c) output a message >>> on each invocation, even if called via fdt addr -q, in which case its >>> output is now slightly noisier. >>> >>> To fix this, move the printf outside of set_working_fdt_addr to three >>> call sites: >>> * bootm_find_images (the use case for which the additional output was >>> added in baf41410). >>> * fdt addr, but only if the 'quiet' argument is not set. >>> * fdt move. >>> >>> This also has the advantage of printing the specified address instead of >>> the translated address when using the sandbox. >>> >>> Remove assertions from the fdt addr test case when: >>> * Calling set_working_fdt_addr directly. >>> * Calling fdt addr with the -q argument. >> >> Why not just pass the 'quiet' flag to set_working_fdt_addr() to avoid >> duplication ? > > I considered this too, but there are other machine-specific call sites > and I was trying to avoid adding an extra argument everywhere just for > logging. > > I'll send a v2 with a 'quiet' argument on Monday. Hmmmm, what about creating a wrapper with the extra logging ? That way, you can have the logging in one place, without introducing the extra parameter. int set_working_fdt_addr_verbose(...) { ret = set_working_fdt_addr(); printf(...); return ret; }
diff --git a/boot/bootm.c b/boot/bootm.c index 2eec60ec7b..be5e29088d 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -313,8 +313,11 @@ int bootm_find_images(int flag, int argc, char *const argv[], ulong start, return 1; } - if (IS_ENABLED(CONFIG_CMD_FDT)) + if (IS_ENABLED(CONFIG_CMD_FDT)) { + printf("Working FDT set to %lx\n", (ulong)images.ft_addr); set_working_fdt_addr(map_to_sysmem(images.ft_addr)); + } + #endif #if CONFIG_IS_ENABLED(FIT) diff --git a/cmd/fdt.c b/cmd/fdt.c index f38fe909c3..39f94759d9 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -40,7 +40,6 @@ void set_working_fdt_addr(ulong addr) { void *buf; - printf("Working FDT set to %lx\n", addr); buf = map_sysmem(addr, 0); working_fdt = buf; env_set_hex("fdtaddr", addr); @@ -192,10 +191,13 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if ((quiet && fdt_check_header(blob)) || (!quiet && !fdt_valid(&blob))) return 1; - if (control) + if (control) { gd->fdt_blob = blob; - else + } else { + if (!quiet) + printf("Working FDT set to %lx\n", addr); set_working_fdt_addr(addr); + } if (argc >= 2) { int len; @@ -223,6 +225,7 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) * Move the working_fdt */ } else if (strncmp(argv[1], "mo", 2) == 0) { + unsigned long addr; struct fdt_header *newaddr; int len; int err; @@ -237,7 +240,8 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if (!fdt_valid(&working_fdt)) return 1; - newaddr = map_sysmem(hextoul(argv[3], NULL), 0); + addr = hextoul(argv[3], NULL); + newaddr = map_sysmem(addr, 0); /* * If the user specifies a length, use that. Otherwise use the @@ -264,6 +268,7 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) fdt_strerror(err)); return 1; } + printf("Working FDT set to %lx\n", addr); set_working_fdt_addr(map_to_sysmem(newaddr)); return CMD_RET_SUCCESS; diff --git a/test/cmd/fdt.c b/test/cmd/fdt.c index cdbaf8c425..d2839d6dee 100644 --- a/test/cmd/fdt.c +++ b/test/cmd/fdt.c @@ -151,7 +151,6 @@ static int fdt_test_addr(struct unit_test_state *uts) /* The working fdt is not set, so this should fail */ set_working_fdt_addr(0); - ut_assert_nextline("Working FDT set to 0"); ut_asserteq(CMD_RET_FAILURE, run_command("fdt addr", 0)); ut_assert_nextline("libfdt fdt_check_header(): FDT_ERR_BADMAGIC"); ut_assertok(ut_check_console_end(uts)); @@ -160,20 +159,17 @@ static int fdt_test_addr(struct unit_test_state *uts) ut_assertok(make_test_fdt(uts, fdt, sizeof(fdt))); addr = map_to_sysmem(fdt); set_working_fdt_addr(addr); - ut_assert_nextline("Working FDT set to %lx", addr); ut_assertok(run_command("fdt addr", 0)); ut_assert_nextline("Working fdt: %08lx", (ulong)map_to_sysmem(fdt)); ut_assertok(ut_check_console_end(uts)); /* Set the working FDT */ set_working_fdt_addr(0); - ut_assert_nextline("Working FDT set to 0"); ut_assertok(run_commandf("fdt addr %08x", addr)); ut_assert_nextline("Working FDT set to %lx", addr); ut_asserteq(addr, map_to_sysmem(working_fdt)); ut_assertok(ut_check_console_end(uts)); set_working_fdt_addr(0); - ut_assert_nextline("Working FDT set to 0"); /* Set the control FDT */ fdt_blob = gd->fdt_blob; @@ -194,7 +190,6 @@ static int fdt_test_addr(struct unit_test_state *uts) /* Test detecting an invalid FDT */ fdt[0] = 123; set_working_fdt_addr(addr); - ut_assert_nextline("Working FDT set to %lx", addr); ut_asserteq(1, run_commandf("fdt addr")); ut_assert_nextline("libfdt fdt_check_header(): FDT_ERR_BADMAGIC"); ut_assertok(ut_check_console_end(uts)); @@ -229,7 +224,6 @@ static int fdt_test_addr_resize(struct unit_test_state *uts) /* ...quietly */ ut_assertok(run_commandf("fdt addr -q %08x %x", addr, sizeof(fdt) / 4)); - ut_assert_nextline("Working FDT set to %lx", addr); ut_assertok(ut_check_console_end(uts)); /* We cannot easily provoke errors in fdt_open_into(), so ignore that */