Re: [PATCH] fdt: Make fdt addr -q quieter
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
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.
H, 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;
}
Re: [PATCH] fdt: Make fdt addr -q quieter
On 17/03/2023 12:17, Marek Vasut wrote: On 3/16/23 17:34, Peter Hoyes wrote: From: Peter Hoyes 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
Re: [PATCH] fdt: Make fdt addr -q quieter
On 3/16/23 17:34, Peter Hoyes wrote: From: Peter Hoyes 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 ?
[PATCH] fdt: Make fdt addr -q quieter
From: Peter Hoyes
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.
Signed-off-by: Peter Hoyes
---
boot/bootm.c | 5 -
cmd/fdt.c | 13 +
test/cmd/fdt.c | 6 --
3 files changed, 13 insertions(+), 11 deletions(-)
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(a
