cmd: fpga: improve user feedback in case of bitstream load failure

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@cern.ch>
Link: https://lore.kernel.org/r/20250708152455.1214487-2-vtpieter@gmail.com
Signed-off-by: Michal Simek <michal.simek@amd.com>

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");
+		log_err("Incorrect number of parameters passed\n");
-		return CMD_RET_USAGE;
+		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");
+		log_err("Zero fpga_data address\n");
-		return CMD_RET_USAGE;
+		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");
+		log_err("Zero size\n");
-		return CMD_RET_USAGE;
+		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) {
+	if (argc < pos_userkey) {
-		debug("fpga: incorrect parameters passed\n");
+		log_err("Too few parameters passed\n");
-		return CMD_RET_USAGE;
+		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");
+		log_err("Use <fpga load> for NonSecure bitstream\n");
-		return CMD_RET_USAGE;
+		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");
+		log_err("User key not provided\n");
-		return CMD_RET_USAGE;
+		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");
+		log_err("Invalid fpga device\n");
-		return CMD_RET_USAGE;
+		return CMD_RET_FAILURE;
 	}
 
 	if (argc == 0 && !datastr) {
-		debug("fpga: No datastr passed\n");
+		log_err("No datastr passed\n");
-		return CMD_RET_USAGE;
+		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 ",
+		log_debug("*  fpga: subimage '%s' from FIT image ",
 			  fit_uname);
-		debug("at 0x%08lx\n", fit_addr);
+		log_debug("at 0x%08lx\n", fit_addr);
 	} else
 #endif
 	{
 		fpga_data = (void *)hextoul(datastr, NULL);
-		debug("*  fpga: cmdline image address = 0x%08lx\n",
+		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");
+		log_err("Zero fpga_data address\n");
-		return CMD_RET_USAGE;
+		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");
+			log_err("Gunzip image is not supported\n");
-			return 1;
+			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");
+		log_err("Non existing command\n");
-		return CMD_RET_USAGE;
+		return CMD_RET_FAILURE;
 	}
 
 	argc -= 2;
 	argv += 2;
 
 	if (argc > fpga_cmd->maxargs) {
-		debug("fpga: more parameters passed\n");
+		log_err("Too many parameters passed\n");
-		return CMD_RET_USAGE;
+		return CMD_RET_FAILURE;
 	}
 
 	ret = fpga_cmd->cmd(fpga_cmd, flag, argc, argv);

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",
+		log_err("Bitstream has to be placed above 1MB (%px)\n",
-		       __func__, buf);
+			buf);
 		return FPGA_FAIL;
 	}
 

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",
+		log_err("Bitstream has to be placed above 1MB (%x)\n",
-		       __func__, (u32)buf);
+			(u32)buf);
 		return FPGA_FAIL;
 	}