[RESENT,v3,2/5] mmc-utils: Add FFU mode 2

Commit Message

Bean Huo Oct. 13, 2024, 9:09 p.m. UTC

From: Bean Huo <[e-mail]>

Added a new FFU mode 2 that ensures atomic firmware image download to improve reliability
and provide a smoother FFU process. In this mode, begins with CMD6, followed by repeated
CMD23+CMD25 for downloading the firmware image. Once the entire firmware image is downloaded,
the FFU mode is exited with CMD6, ensuring the download is treated as an atomic operation.

Signed-off-by: Bean Huo <[e-mail]>
---
 mmc.1      |  3 ++
 mmc.c      |  5 +++
 mmc_cmds.c | 92 +++++++++++++++++++++++++++++++++++++++++-------------
 mmc_cmds.h |  1 +
 4 files changed, 79 insertions(+), 22 deletions(-)

Comments

Avri Altman Oct. 14, 2024, 6:56 a.m. UTC | #1

> +       } else if (ffu_mode == 2) {
> +               set_single_cmd(&multi_cmd->cmds[0], MMC_SET_BLOCK_COUNT,
> 0, 0, bytes / 512);
> +               multi_cmd->cmds[0].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
> MMC_CMD_AC;
> +               set_single_cmd(&multi_cmd->cmds[1],
> MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / 512, arg);
> +               mmc_ioc_cmd_set_data(multi_cmd->cmds[1], buf + offset);
> +       }
> +}
Actually, I was hoping to avoid the "ffu_mode" extra argument.
So that each do_ffu<x> has its own fluf encapsulation logic.

Thanks,
Avri

Avri Altman Oct. 14, 2024, 7:51 a.m. UTC | #2

> +static int enter_ffu_mode(int *dev_fd)
> +{
> +       int ret;
> +       struct mmc_ioc_cmd cmd;
> +       memset(&cmd, 0, sizeof(cmd));
> 
> -       /*
> -        * send image chunk: blksz and blocks essentially do not matter, as
> -        * long as the product is fw_size, but some hosts don't handle larger
> -        * blksz well.
> -        */
> -       set_single_cmd(&multi_cmd->cmds[2], MMC_WRITE_MULTIPLE_BLOCK,
> 1, bytes / 512, arg);
> -       mmc_ioc_cmd_set_data(multi_cmd->cmds[2], buf + offset);
> +       fill_switch_cmd(&cmd, EXT_CSD_MODE_CONFIG, EXT_CSD_FFU_MODE);
> +       ret = ioctl(*dev_fd, MMC_IOC_CMD, &cmd);
> +       if (ret)
> +               perror("enter FFU mode faled\n");
> +
> +       return ret;
> +}
Thinking aloud if we need the enter phase at all, because:
1) The first cmd6 is always part of the sequence, and
2) is it even safe to send it as a single ioctl?

I guess the answer to 2) is yes, and having a start phase actually make sense:
it makes clearer of what the ffu process comprised of:
Start (enter ffu mode), repeat...., finish up.

I would even add a comment in the code (and in the commit log) for all modes other than 1,
emphasizing that from this point onward, the device is blocked in ffu mode.

Thanks,
Avri

Bean Huo Oct. 20, 2024, 7:53 p.m. UTC | #3

On Mon, 2024-10-14 at 06:56 +0000, Avri Altman wrote:
> > +       } else if (ffu_mode == 2) {
> > +               set_single_cmd(&multi_cmd->cmds[0],
> > MMC_SET_BLOCK_COUNT,
> > 0, 0, bytes / 512);
> > +               multi_cmd->cmds[0].flags = MMC_RSP_SPI_R1 |
> > MMC_RSP_R1 |
> > MMC_CMD_AC;
> > +               set_single_cmd(&multi_cmd->cmds[1],
> > MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / 512, arg);
> > +               mmc_ioc_cmd_set_data(multi_cmd->cmds[1], buf +
> > offset);
> > +       }
> > +}
> Actually, I was hoping to avoid the "ffu_mode" extra argument.
> So that each do_ffu<x> has its own fluf encapsulation logic.
> 
> Thanks,
> Avri

Thank you for the suggestion. After reviewing the approach, I believe
it's not feasible to encapsulate the above code logic entirely within
each do_ffu<x> function.

The primary reason is that during the download phase, especially when
multiple loops are required, I need to update certain state variables
dynamically, such as the ffu_mode. This allows for handling the
different stages of the process efficiently across iterations. The
encapsulation logic would prevent us from adjusting these variables
between loops, which is crucial for ensuring the proper progression and
handling of larger data sets.

Without this flexibility, we'd run into issues when the process
requires multiple passes or loops, making it difficult to adjust the
behavior mid-operation.

Let me know if you have any further suggestions, or if you'd like to
discuss alternate solutions.

Kind regards,
Bean

Patch

diff --git a/mmc.1 b/mmc.1
index e153557..b98b63f 100644
--- a/mmc.1
+++ b/mmc.1
@@ -192,6 +192,9 @@  Run Field Firmware Update with \fIimage\-file\-name\fR on the device.
 .br
 if [\fIchunk\-bytes\fR] is omitted, mmc-utils will try to run ffu using the largest possible chunks: max(image-file, 512k).
 .TP
+.BI ffu2 " \fIimage\-file\-name\fR " " \fIdevice\fR " " [\fIchunk\-bytes\fR]
+Same as 'ffu', but uses CMD23+CMD25 for repeated downloads and remains in FFU mode until completion.
+.TP
 .BI erase " " \fItype\fR " " \fIstart-address\fR " " \fIend\-address\fR " " \fIdevice\fR
 Send Erase CMD38 with specific argument to the device.
 .br
diff --git a/mmc.c b/mmc.c
index 2c5b9b5..f1d98e6 100644
--- a/mmc.c
+++ b/mmc.c
@@ -234,6 +234,11 @@  static struct Command commands[] = {
 		"should be in decimal bytes and sector aligned.\n",
 	  NULL
 	},
+	{ do_ffu2, -2,
+	  "ffu2", "<image name> <device> [chunk-bytes]\n"
+		"Same as 'ffu', but uses CMD23+CMD25 for repeated downloads and remains in FFU mode until completion.\n",
+	  NULL
+	},
 	{ do_erase, -4,
 	"erase", "<type> " "<start address> " "<end address> " "<device>\n"
 		"Send Erase CMD38 with specific argument to the <device>\n\n"
diff --git a/mmc_cmds.c b/mmc_cmds.c
index 72921a7..b507bff 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -2812,21 +2812,55 @@  out:
 }
 
 static void set_ffu_download_cmd(struct mmc_ioc_multi_cmd *multi_cmd, __u8 *ext_csd,
-				unsigned int bytes, __u8 *buf, off_t offset)
+				unsigned int bytes, __u8 *buf, off_t offset, __u8 ffu_mode)
 {
 	__u32 arg = per_byte_htole32(&ext_csd[EXT_CSD_FFU_ARG_0]);
 
-	/* send block count */
-	set_single_cmd(&multi_cmd->cmds[1], MMC_SET_BLOCK_COUNT, 0, 0, bytes / 512);
-	multi_cmd->cmds[1].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
+	if (ffu_mode == 1) {
+		/* send block count */
+		set_single_cmd(&multi_cmd->cmds[1], MMC_SET_BLOCK_COUNT, 0, 0, bytes / 512);
+		multi_cmd->cmds[1].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
+
+		/*
+		 * send image chunk: blksz and blocks essentially do not matter, as
+		 * long as the product is fw_size, but some hosts don't handle larger
+		 * blksz well.
+		 */
+		set_single_cmd(&multi_cmd->cmds[2], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / 512, arg);
+		mmc_ioc_cmd_set_data(multi_cmd->cmds[2], buf + offset);
+	} else if (ffu_mode == 2) {
+		set_single_cmd(&multi_cmd->cmds[0], MMC_SET_BLOCK_COUNT, 0, 0, bytes / 512);
+		multi_cmd->cmds[0].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
+		set_single_cmd(&multi_cmd->cmds[1], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / 512, arg);
+		mmc_ioc_cmd_set_data(multi_cmd->cmds[1], buf + offset);
+	}
+}
+static int enter_ffu_mode(int *dev_fd)
+{
+       int ret;
+       struct mmc_ioc_cmd cmd;
+       memset(&cmd, 0, sizeof(cmd));
 
-	/*
-	 * send image chunk: blksz and blocks essentially do not matter, as
-	 * long as the product is fw_size, but some hosts don't handle larger
-	 * blksz well.
-	 */
-	set_single_cmd(&multi_cmd->cmds[2], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / 512, arg);
-	mmc_ioc_cmd_set_data(multi_cmd->cmds[2], buf + offset);
+       fill_switch_cmd(&cmd, EXT_CSD_MODE_CONFIG, EXT_CSD_FFU_MODE);
+       ret = ioctl(*dev_fd, MMC_IOC_CMD, &cmd);
+       if (ret)
+               perror("enter FFU mode faled\n");
+
+       return ret;
+}
+
+static int exit_ffu_mode(int *dev_fd)
+{
+       int ret;
+       struct mmc_ioc_cmd cmd;
+       memset(&cmd, 0, sizeof(cmd));
+
+       fill_switch_cmd(&cmd, EXT_CSD_MODE_CONFIG, EXT_CSD_NORMAL_MODE);
+       ret = ioctl(*dev_fd, MMC_IOC_CMD, &cmd);
+       if (ret)
+               perror("exit FFU mode faled\n");
+
+       return ret;
 }
 
 static int get_ffu_sectors_programmed(int *dev_fd, __u8 *ext_csd)
@@ -2871,7 +2905,7 @@  static bool ffu_is_supported(__u8 *ext_csd, char *device)
 }
 
 static int do_ffu_download(int *dev_fd, __u8 *ext_csd, __u8 *fw_buf, off_t fw_size,
-							unsigned int chunk_size)
+							unsigned int chunk_size, __u8 ffu_mode)
 {
 	int ret;
 	__u8 num_of_cmds = 4;
@@ -2883,6 +2917,10 @@  static int do_ffu_download(int *dev_fd, __u8 *ext_csd, __u8 *fw_buf, off_t fw_si
 		fprintf(stderr, "unexpected NULL pointer\n");
 		return -EINVAL;
 	}
+
+	if (ffu_mode != 1) /* in FFU mode 1, mmc_ioc_multi_cmd contains 4 commands */
+		num_of_cmds = 2;
+
 	/* allocate maximum required */
 	multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + num_of_cmds * sizeof(struct mmc_ioc_cmd));
 	if (!multi_cmd) {
@@ -2890,14 +2928,12 @@  static int do_ffu_download(int *dev_fd, __u8 *ext_csd, __u8 *fw_buf, off_t fw_si
 		return -ENOMEM;
 	}
 
-	/* prepare multi_cmd for FFU based on cmd to be used */
-	/* put device into ffu mode */
-	fill_switch_cmd(&multi_cmd->cmds[0], EXT_CSD_MODE_CONFIG, EXT_CSD_FFU_MODE);
-
-	/* return device into normal mode */
-	fill_switch_cmd(&multi_cmd->cmds[3], EXT_CSD_MODE_CONFIG, EXT_CSD_NORMAL_MODE);
 
 do_retry:
+
+	if (num_of_cmds != 4 && enter_ffu_mode(dev_fd))
+		goto out;
+
 	bytes_left = fw_size;
 	off = 0;
 	multi_cmd->num_of_cmds = num_of_cmds;
@@ -2906,7 +2942,7 @@  do_retry:
 		bytes_per_loop = bytes_left < chunk_size ? bytes_left : chunk_size;
 
 		/* prepare multi_cmd for FFU based on cmd to be used */
-		set_ffu_download_cmd(multi_cmd, ext_csd, bytes_per_loop, fw_buf, off);
+		set_ffu_download_cmd(multi_cmd, ext_csd, bytes_per_loop, fw_buf, off, ffu_mode);
 
 		/* send ioctl with multi-cmd, download firmware bundle */
 		ret = ioctl(*dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
@@ -2917,7 +2953,7 @@  do_retry:
 			 * In case multi-cmd ioctl failed before exiting from
 			 * ffu mode
 			 */
-			ioctl(*dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
+			exit_ffu_mode(dev_fd);
 			goto out;
 		}
 
@@ -2941,6 +2977,9 @@  do_retry:
 		off += bytes_per_loop;
 	}
 
+	if (num_of_cmds != 4 && exit_ffu_mode(dev_fd))
+		goto out;
+
 	ret = get_ffu_sectors_programmed(dev_fd, ext_csd);
 out:
 	free(multi_cmd);
@@ -2948,7 +2987,8 @@  out:
 
 }
 
-int do_ffu(int nargs, char **argv)
+
+static int __do_ffu(int nargs, char **argv, __u8 ffu_mode)
 {
 	off_t fw_size;
 	char *device;
@@ -3022,7 +3062,7 @@  int do_ffu(int nargs, char **argv)
 		goto out;
 	}
 
-	sect_done = do_ffu_download((int *)&dev_fd, ext_csd, fw_buf, fw_size, default_chunk);
+	sect_done = do_ffu_download((int *)&dev_fd, ext_csd, fw_buf, fw_size, default_chunk, ffu_mode);
 
 	/* Check programmed sectors */
 	if (sect_done > 0 && (sect_done * 512) == fw_size) {
@@ -3092,6 +3132,14 @@  out:
 	return ret;
 }
 
+int do_ffu(int nargs, char **argv) {
+	return __do_ffu(nargs, argv, 1);
+}
+
+int do_ffu2(int nargs, char **argv) {
+	return __do_ffu(nargs, argv, 2);
+}
+
 int do_general_cmd_read(int nargs, char **argv)
 {
 	int dev_fd;
diff --git a/mmc_cmds.h b/mmc_cmds.h
index 5f2bef1..81364f4 100644
--- a/mmc_cmds.h
+++ b/mmc_cmds.h
@@ -42,6 +42,7 @@  int do_rpmb_write_block(int nargs, char **argv);
 int do_cache_en(int nargs, char **argv);
 int do_cache_dis(int nargs, char **argv);
 int do_ffu(int nargs, char **argv);
+int do_ffu2(int nargs, char **argv);
 int do_read_scr(int argc, char **argv);
 int do_read_cid(int argc, char **argv);
 int do_read_csd(int argc, char **argv);