[PATCH v3 1/4] Improve user feedback in case of FPGA bitstream load failure
Michal Simek
michal.simek at amd.com
Wed Jul 2 10:51:57 CEST 2025
On 6/26/25 11:35, vtpieter at gmail.com wrote:
> From: Pieter Van Trappen <pieter.van.trappen at cern.ch>
>
> In cmd/fpga.c, change some `debug` calls to `log_err` for important
> user feedback and use CMD_RET_FAILURE in favor of CMD_RET_USAGE due to
> its long output which hides the actual, useful return message. Change
> the remaining `debug` calls to `log_debug`. Remove all 'fpga:' and
> __func__ strings as `log_*` has this covered.
>
> For `do_fpga_loads`, move up the `do_fpga_check_params` call for more
> consistent command output; use a constant instead of multiple '5' use.
>
> In drivers/fpga/zynq*.c, change 'up to' to 'above' which corrects this
> confusing/wrong statement.
>
> Signed-off-by: Pieter Van Trappen <pieter.van.trappen at cern.ch>
> ---
> cmd/fpga.c | 99 +++++++++++++++++++++--------------------
> drivers/fpga/zynqmppl.c | 4 +-
> drivers/fpga/zynqpl.c | 4 +-
> 3 files changed, 54 insertions(+), 53 deletions(-)
>
> diff --git a/cmd/fpga.c b/cmd/fpga.c
> index 9dc7b63db5d..9cc51f04698 100644
> --- a/cmd/fpga.c
> +++ b/cmd/fpga.c
> @@ -28,7 +28,7 @@ static long do_fpga_get_device(char *arg)
> if (dev == FPGA_INVALID_DEVICE && arg)
> dev = simple_strtol(arg, NULL, 16);
>
> - debug("%s: device = %ld\n", __func__, dev);
> + log_debug("device = %ld\n", dev);
>
> return dev;
> }
> @@ -40,26 +40,26 @@ static int do_fpga_check_params(long *dev, long *fpga_data, size_t *data_size,
> size_t local_data_size;
> long local_fpga_data;
>
> - debug("%s %d, %d\n", __func__, argc, cmdtp->maxargs);
> + log_debug("%d, %d\n", argc, cmdtp->maxargs);
>
> if (argc != cmdtp->maxargs) {
> - debug("fpga: incorrect parameters passed\n");
> - return CMD_RET_USAGE;
> + log_err("Incorrect number of parameters passed\n");
> + return CMD_RET_FAILURE;
> }
>
> *dev = do_fpga_get_device(argv[0]);
>
> local_fpga_data = simple_strtol(argv[1], NULL, 16);
> if (!local_fpga_data) {
> - debug("fpga: zero fpga_data address\n");
> - return CMD_RET_USAGE;
> + log_err("Zero fpga_data address\n");
> + return CMD_RET_FAILURE;
> }
> *fpga_data = local_fpga_data;
>
> local_data_size = hextoul(argv[2], NULL);
> if (!local_data_size) {
> - debug("fpga: zero size\n");
> - return CMD_RET_USAGE;
> + log_err("Zero size\n");
> + return CMD_RET_FAILURE;
> }
> *data_size = local_data_size;
>
> @@ -70,51 +70,52 @@ static int do_fpga_check_params(long *dev, long *fpga_data, size_t *data_size,
> static int do_fpga_loads(struct cmd_tbl *cmdtp, int flag, int argc,
> char *const argv[])
> {
> + struct fpga_secure_info fpga_sec_info;
> + const int pos_userkey = 5;
> size_t data_size = 0;
> long fpga_data, dev;
> int ret;
> - struct fpga_secure_info fpga_sec_info;
>
> memset(&fpga_sec_info, 0, sizeof(fpga_sec_info));
>
> - if (argc < 5) {
> - debug("fpga: incorrect parameters passed\n");
> - return CMD_RET_USAGE;
> + if (argc < pos_userkey) {
> + log_err("Too few parameters passed\n");
> + return CMD_RET_FAILURE;
> }
>
> - if (argc == 6)
> + if (argc == pos_userkey + 1)
> fpga_sec_info.userkey_addr = (u8 *)(uintptr_t)
> - simple_strtoull(argv[5],
> + simple_strtoull(argv[pos_userkey],
> NULL, 16);
> else
> /*
> * If 6th parameter is not passed then do_fpga_check_params
> * will get 5 instead of expected 6 which means that function
> - * return CMD_RET_USAGE. Increase number of params +1 to pass
> + * return CMD_RET_FAILURE. Increase number of params +1 to pass
> * this.
> */
> argc++;
>
> + ret = do_fpga_check_params(&dev, &fpga_data, &data_size,
> + cmdtp, argc, argv);
> + if (ret)
> + return ret;
> +
> fpga_sec_info.encflag = (u8)hextoul(argv[4], NULL);
> fpga_sec_info.authflag = (u8)hextoul(argv[3], NULL);
>
> if (fpga_sec_info.authflag >= FPGA_NO_ENC_OR_NO_AUTH &&
> fpga_sec_info.encflag >= FPGA_NO_ENC_OR_NO_AUTH) {
> - debug("fpga: Use <fpga load> for NonSecure bitstream\n");
> - return CMD_RET_USAGE;
> + log_err("Use <fpga load> for NonSecure bitstream\n");
> + return CMD_RET_FAILURE;
> }
>
> if (fpga_sec_info.encflag == FPGA_ENC_USR_KEY &&
> !fpga_sec_info.userkey_addr) {
> - debug("fpga: User key not provided\n");
> - return CMD_RET_USAGE;
> + log_err("User key not provided\n");
> + return CMD_RET_FAILURE;
> }
>
> - ret = do_fpga_check_params(&dev, &fpga_data, &data_size,
> - cmdtp, argc, argv);
> - if (ret)
> - return ret;
> -
> return fpga_loads(dev, (void *)fpga_data, data_size, &fpga_sec_info);
> }
> #endif
> @@ -245,23 +246,23 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
> ulong dev = do_fpga_get_device(argv[0]);
> char *datastr = env_get("fpgadata");
>
> - debug("fpga: argc %x, dev %lx, datastr %s\n", argc, dev, datastr);
> + log_debug("argc %x, dev %lx, datastr %s\n", argc, dev, datastr);
>
> if (dev == FPGA_INVALID_DEVICE) {
> - debug("fpga: Invalid fpga device\n");
> - return CMD_RET_USAGE;
> + log_err("Invalid fpga device\n");
> + return CMD_RET_FAILURE;
> }
>
> if (argc == 0 && !datastr) {
> - debug("fpga: No datastr passed\n");
> - return CMD_RET_USAGE;
> + log_err("No datastr passed\n");
> + return CMD_RET_FAILURE;
> }
>
> if (argc == 2) {
> datastr = argv[1];
> - debug("fpga: Full command with two args\n");
> + log_debug("Full command with two args\n");
> } else if (argc == 1 && !datastr) {
> - debug("fpga: Dev is setup - fpgadata passed\n");
> + log_debug("Dev is setup - fpgadata passed\n");
> datastr = argv[0];
> }
>
> @@ -269,20 +270,20 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
> if (fit_parse_subimage(datastr, (ulong)fpga_data,
> &fit_addr, &fit_uname)) {
> fpga_data = (void *)fit_addr;
> - debug("* fpga: subimage '%s' from FIT image ",
> - fit_uname);
> - debug("at 0x%08lx\n", fit_addr);
> + log_debug("* fpga: subimage '%s' from FIT image ",
> + fit_uname);
> + log_debug("at 0x%08lx\n", fit_addr);
> } else
> #endif
> {
> fpga_data = (void *)hextoul(datastr, NULL);
> - debug("* fpga: cmdline image address = 0x%08lx\n",
> - (ulong)fpga_data);
> + log_debug("* fpga: cmdline image address = 0x%08lx\n",
> + (ulong)fpga_data);
> }
> - debug("%s: fpga_data = 0x%lx\n", __func__, (ulong)fpga_data);
> + log_debug("fpga_data = 0x%lx\n", (ulong)fpga_data);
> if (!fpga_data) {
> - puts("Zero fpga_data address\n");
> - return CMD_RET_USAGE;
> + log_err("Zero fpga_data address\n");
> + return CMD_RET_FAILURE;
> }
>
> switch (genimg_get_format(fpga_data)) {
> @@ -303,13 +304,13 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
>
> if (gunzip((void *)data, ~0UL, (void *)image_buf,
> &image_size) != 0) {
> - puts("GUNZIP: error\n");
> + log_err("Gunzip error\n");
> return CMD_RET_FAILURE;
> }
> data_size = image_size;
> #else
> - puts("Gunzip image is not supported\n");
> - return 1;
> + log_err("Gunzip image is not supported\n");
> + return CMD_RET_FAILURE;
> #endif
> } else {
> data = (ulong)image_get_data(hdr);
> @@ -327,12 +328,12 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
> const void *fit_data;
>
> if (!fit_uname) {
> - puts("No FIT subimage unit name\n");
> + log_err("No FIT subimage unit name\n");
> return CMD_RET_FAILURE;
> }
>
> if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) {
> - puts("Bad FIT image format\n");
> + log_err("Bad FIT image format\n");
> return CMD_RET_FAILURE;
> }
>
> @@ -348,7 +349,7 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
> }
> #endif
> default:
> - puts("** Unknown image type\n");
> + log_err("Unknown image type\n");
> return CMD_RET_FAILURE;
> }
> }
> @@ -390,16 +391,16 @@ static int do_fpga_wrapper(struct cmd_tbl *cmdtp, int flag, int argc,
> fpga_cmd = find_cmd_tbl(argv[1], fpga_commands,
> ARRAY_SIZE(fpga_commands));
> if (!fpga_cmd) {
> - debug("fpga: non existing command\n");
> - return CMD_RET_USAGE;
> + log_err("Non existing command\n");
> + return CMD_RET_FAILURE;
> }
>
> argc -= 2;
> argv += 2;
>
> if (argc > fpga_cmd->maxargs) {
> - debug("fpga: more parameters passed\n");
> - return CMD_RET_USAGE;
> + log_err("Too many parameters passed\n");
> + return CMD_RET_FAILURE;
> }
>
> ret = fpga_cmd->cmd(fpga_cmd, flag, argc, argv);
> diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
> index 2b62bbbe3cf..1199b249e36 100644
> --- a/drivers/fpga/zynqmppl.c
> +++ b/drivers/fpga/zynqmppl.c
> @@ -191,8 +191,8 @@ static int zynqmp_validate_bitstream(xilinx_desc *desc, const void *buf,
> }
>
> if ((ulong)buf < SZ_1M) {
> - printf("%s: Bitstream has to be placed up to 1MB (%px)\n",
> - __func__, buf);
> + log_err("Bitstream has to be placed above 1MB (%px)\n",
> + buf);
> return FPGA_FAIL;
> }
>
> diff --git a/drivers/fpga/zynqpl.c b/drivers/fpga/zynqpl.c
> index 3e86d854a01..a8ca015a8a6 100644
> --- a/drivers/fpga/zynqpl.c
> +++ b/drivers/fpga/zynqpl.c
> @@ -360,8 +360,8 @@ static int zynq_validate_bitstream(xilinx_desc *desc, const void *buf,
> }
>
> if ((u32)buf < SZ_1M) {
> - printf("%s: Bitstream has to be placed up to 1MB (%x)\n",
> - __func__, (u32)buf);
> + log_err("Bitstream has to be placed above 1MB (%px)\n",
> + (u32)buf);
> return FPGA_FAIL;
> }
>
I would be the best if you can create also cover letter.
But let me reply to this patch but issue is likely related to others. I am also
not in CC on 3/4.
Anyway CI is failing with
$ tools/buildman/buildman -o ${UBOOT_TRAVIS_BUILD_DIR} -w -E -W -e --board
${TEST_PY_BD} ${OVERRIDE}
Building current source for 1 boards (1 thread, 64 jobs per thread)
sandbox: + sandbox
+drivers/fpga/fpga.c:23:13: error: ‘fpga_no_sup’ defined but not used
[-Werror=unused-function]
+ 23 | static void fpga_no_sup(char *fn, char *msg)
+ | ^~~~~~~~~~~
+cc1: all warnings being treated as errors
+make[3]: *** [scripts/Makefile.build:249: drivers/fpga/fpga.o] Error 1
+make[2]: *** [scripts/Makefile.build:388: drivers/fpga] Error 2
+make[1]: *** [Makefile:1930: drivers] Error 2
which should be fixed.
Second thing is that all of your subjects are missing prefixes.
That's why please use fpga: or cmd: fpga: there to see subsystems you are
targetting.
Thakns,
Michal
More information about the U-Boot
mailing list