diff mbox series

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

Message ID 20241023143839.108572-3-beanhuo@iokpp.de (mailing list archive)
State New
Headers show
Series Add multiple FFU modes in mmc-utils based on eMMC specification 6.6.18 for flexible firmware updates | expand

Commit Message

Bean Huo Oct. 23, 2024, 2:38 p.m. UTC
From: Bean Huo <beanhuo@micron.com>

Added a new FFU mode 2, in this mode, begins with CMD6, followed by repeated
CMD23+CMD25 for downloading the firmware bundle. 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 <beanhuo@micron.com>
---
 mmc.1      |   5 ++-
 mmc.c      |   9 +++-
 mmc_cmds.c | 126 +++++++++++++++++++++++++++++++++++++++++------------
 mmc_cmds.h |   3 +-
 4 files changed, 112 insertions(+), 31 deletions(-)

Comments

Avri Altman Oct. 24, 2024, 8 a.m. UTC | #1
> -       { do_ffu, -2,
> -         "ffu", "<image name> <device> [chunk-bytes]\n"
> +       { do_ffu1, -2,
> +         "ffu1", "<image name> <device> [chunk-bytes]\n"
Ah, but didn't we establish that the current API should be retained to act as the default mode?

Thanks,
Avri

>                 "Run Field Firmware Update with <image name> on <device>.\n"
>                 "[chunk-bytes] is optional and defaults to its max - 512k. "
>                 "should be in decimal bytes and sector aligned.\n",
>           NULL
>         },
Bean Huo Oct. 24, 2024, 9:04 a.m. UTC | #2
On Thu, 2024-10-24 at 08:00 +0000, Avri Altman wrote:
> > -       { do_ffu, -2,
> > -         "ffu", "<image name> <device> [chunk-bytes]\n"
> > +       { do_ffu1, -2,
> > +         "ffu1", "<image name> <device> [chunk-bytes]\n"
> Ah, but didn't we establish that the current API should be retained
> to act as the default mode?
> 
> Thanks,
> Avri

Avri, 

Corret, the reason for updating the default FFU mode name from 'ffu' to
'ffu1' is to avoid the error: 'ERROR: in command 'ffu', 'ffu' is
ambiguous' when using 'mmc ffu'. Without this change, the system will
encounter ambiguity.


I am considering a naming scheme like opt_ffu1 and opt_ffu2, that works
well for maintaining consistency and keeping the names concise.

ffu2 could become opt_ffu1 (indicating the first optimized or alternate
FFU mode).

ffu3 could become opt_ffu2.
ffu4 could become opt_ffu3.
ffu5 could become opt_ffu4.

then keep default ffu name as it is used to be.

how do you think?



Kind regards,
Bean
Bean Huo Oct. 24, 2024, 2:18 p.m. UTC | #3
On Thu, 2024-10-24 at 11:04 +0200, Bean Huo wrote:
> On Thu, 2024-10-24 at 08:00 +0000, Avri Altman wrote:
> > > -       { do_ffu, -2,
> > > -         "ffu", "<image name> <device> [chunk-bytes]\n"
> > > +       { do_ffu1, -2,
> > > +         "ffu1", "<image name> <device> [chunk-bytes]\n"
> > Ah, but didn't we establish that the current API should be retained
> > to act as the default mode?
> > 
> > Thanks,
> > Avri
> 
> Avri, 
> 
> Corret, the reason for updating the default FFU mode name from 'ffu'
> to
> 'ffu1' is to avoid the error: 'ERROR: in command 'ffu', 'ffu' is
> ambiguous' when using 'mmc ffu'. Without this change, the system will
> encounter ambiguity.
> 
> 
> I am considering a naming scheme like opt_ffu1 and opt_ffu2, that
> works
> well for maintaining consistency and keeping the names concise.
> 
> ffu2 could become opt_ffu1 (indicating the first optimized or
> alternate
> FFU mode).
> 
> ffu3 could become opt_ffu2.
> ffu4 could become opt_ffu3.
> ffu5 could become opt_ffu4.
> 
> then keep default ffu name as it is used to be.
> 
> how do you think?
> 
> 
> 
> Kind regards,
> Bean

Avri, 

how do you think about above my proposal of changing ffux to opt_ffux?

Kind regards,
Bean
Avri Altman Oct. 24, 2024, 6:19 p.m. UTC | #4
> On Thu, 2024-10-24 at 11:04 +0200, Bean Huo wrote:
> > On Thu, 2024-10-24 at 08:00 +0000, Avri Altman wrote:
> > > > -       { do_ffu, -2,
> > > > -         "ffu", "<image name> <device> [chunk-bytes]\n"
> > > > +       { do_ffu1, -2,
> > > > +         "ffu1", "<image name> <device> [chunk-bytes]\n"
> > > Ah, but didn't we establish that the current API should be retained
> > > to act as the default mode?
> > >
> > > Thanks,
> > > Avri
> >
> > Avri,
> >
> > Corret, the reason for updating the default FFU mode name from 'ffu'
> > to
> > 'ffu1' is to avoid the error: 'ERROR: in command 'ffu', 'ffu' is
> > ambiguous' when using 'mmc ffu'. Without this change, the system will
> > encounter ambiguity.
> >
> >
> > I am considering a naming scheme like opt_ffu1 and opt_ffu2, that
> > works well for maintaining consistency and keeping the names concise.
> >
> > ffu2 could become opt_ffu1 (indicating the first optimized or
> > alternate FFU mode).
> >
> > ffu3 could become opt_ffu2.
> > ffu4 could become opt_ffu3.
> > ffu5 could become opt_ffu4.
> >
> > then keep default ffu name as it is used to be.
> >
> > how do you think?
> >
> >
> >
> > Kind regards,
> > Bean
> 
> Avri,
> 
> how do you think about above my proposal of changing ffux to opt_ffux?
I don't really have a strong opinion about that.
As long as the current mode stay the same.

Thanks,
Avri
> 
> Kind regards,
> Bean
Bean Huo Oct. 24, 2024, 8:08 p.m. UTC | #5
On Thu, 2024-10-24 at 18:19 +0000, Avri Altman wrote:
> > On Thu, 2024-10-24 at 11:04 +0200, Bean Huo wrote:
> > > On Thu, 2024-10-24 at 08:00 +0000, Avri Altman wrote:
> > > > > -       { do_ffu, -2,
> > > > > -         "ffu", "<image name> <device> [chunk-bytes]\n"
> > > > > +       { do_ffu1, -2,
> > > > > +         "ffu1", "<image name> <device> [chunk-bytes]\n"
> > > > Ah, but didn't we establish that the current API should be
> > > > retained
> > > > to act as the default mode?
> > > > 
> > > > Thanks,
> > > > Avri
> > > 
> > > Avri,
> > > 
> > > Corret, the reason for updating the default FFU mode name from
> > > 'ffu'
> > > to
> > > 'ffu1' is to avoid the error: 'ERROR: in command 'ffu', 'ffu' is
> > > ambiguous' when using 'mmc ffu'. Without this change, the system
> > > will
> > > encounter ambiguity.
> > > 
> > > 
> > > I am considering a naming scheme like opt_ffu1 and opt_ffu2, that
> > > works well for maintaining consistency and keeping the names
> > > concise.
> > > 
> > > ffu2 could become opt_ffu1 (indicating the first optimized or
> > > alternate FFU mode).
> > > 
> > > ffu3 could become opt_ffu2.
> > > ffu4 could become opt_ffu3.
> > > ffu5 could become opt_ffu4.
> > > 
> > > then keep default ffu name as it is used to be.
> > > 
> > > how do you think?
> > > 
> > > 
> > > 
> > > Kind regards,
> > > Bean
> > 
> > Avri,
> > 
> > how do you think about above my proposal of changing ffux to
> > opt_ffux?
> I don't really have a strong opinion about that.
> As long as the current mode stay the same.
> 
> Thanks,
> Avri


To avoid ambiguity while keeping the default FFU mode as 'ffu', I
cannot use prefix ffu. I’m considering using the prefix opt_ffu for the
optional FFU modes. This would allow us to differentiate between the
default FFU implementation and the various optimized or alternative
modes without using the "ffu" prefix directly.


opt_ffu1: First optional/optimized FFU mode
opt_ffu2: Second optional/optimized FFU mode
opt_ffu3, etc.

This would keep the default ffu unchanged while allowing us to add
clarity and scalability to the naming of other FFU modes.




Hi Ulf

What’s your thought on this approach?

Kind regards, 
Bean
diff mbox series

Patch

diff --git a/mmc.1 b/mmc.1
index e153557..256852b 100644
--- a/mmc.1
+++ b/mmc.1
@@ -185,13 +185,16 @@  The device path should specify the cid sysfs file directory.
 Print SCR data from \fIdevice\-path\fR.
 The device path should specify the scr sysfs file directory.
 .TP
-.BI ffu " " \fIimage\-file\-name\fR " " \fIdevice\fR " " [\fIchunk\-bytes\fR]
+.BI ffu1 " " \fIimage\-file\-name\fR " " \fIdevice\fR " " [\fIchunk\-bytes\fR]
 Run Field Firmware Update with \fIimage\-file\-name\fR on the device.
 .br
 [\fIchunk\-bytes\fR] is optional and defaults to its max - 512k. should be in decimal bytes and sector aligned.
 .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 'ffu1', 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..ffc98dc 100644
--- a/mmc.c
+++ b/mmc.c
@@ -227,13 +227,18 @@  static struct Command commands[] = {
 		  "The device path should specify the scr file directory.",
 	  NULL
 	},
-	{ do_ffu, -2,
-	  "ffu", "<image name> <device> [chunk-bytes]\n"
+	{ do_ffu1, -2,
+	  "ffu1", "<image name> <device> [chunk-bytes]\n"
 		"Run Field Firmware Update with <image name> on <device>.\n"
 		"[chunk-bytes] is optional and defaults to its max - 512k. "
 		"should be in decimal bytes and sector aligned.\n",
 	  NULL
 	},
+	{ do_ffu2, -2,
+	 "ffu2", "<image name> <device> [chunk-bytes]\n"
+	 "Same as 'ffu1', 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 1a35431..c183a2d 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -2813,23 +2813,40 @@  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)
+			       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;
-
-	/*
-	 * 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);
+	/* prepare multi_cmd for FFU based on cmd to be used */
+	if (ffu_mode == 1) {
+		/* put device into ffu mode */
+		fill_switch_cmd(&multi_cmd->cmds[0], EXT_CSD_MODE_CONFIG, EXT_CSD_FFU_MODE);
+		/* 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);
+		/* return device into normal mode */
+		fill_switch_cmd(&multi_cmd->cmds[3], EXT_CSD_MODE_CONFIG, EXT_CSD_NORMAL_MODE);
+	} else if (ffu_mode == 2) {
+		/*
+		 * FFU mode 2 uses CMD23+CMD25 for repeated downloads and remains in FFU mode
+		 * during FW bundle downloading until completion. In this mode, multi_cmd only
+		 * has 2 sub-commands.
+		 */
+		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);
+	}
 }
 
 /*
@@ -2873,6 +2890,36 @@  static bool ffu_is_supported(__u8 *ext_csd, char *device)
 	return true;
 }
 
+static int enter_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_FFU_MODE);
+	ret = ioctl(dev_fd, MMC_IOC_CMD, &cmd);
+	if (ret)
+		perror("enter FFU mode failed!");
+
+	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 failed!");
+
+	return ret;
+}
+
 /*
  * Performs FFU download of the firmware bundle.
  *
@@ -2886,7 +2933,7 @@  static bool ffu_is_supported(__u8 *ext_csd, char *device)
  *         On failure, returns a negative error number.
  */
 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;
@@ -2898,6 +2945,10 @@  static int do_ffu_download(int dev_fd, __u8 *ext_csd, __u8 *fw_buf, off_t fw_siz
 		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));
@@ -2906,14 +2957,15 @@  static int do_ffu_download(int dev_fd, __u8 *ext_csd, __u8 *fw_buf, off_t fw_siz
 		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);
+	if (ffu_mode != 1) {
+		/*
+		 * If the device is not in FFU mode 1, the command to enter FFU mode will be sent
+		 * independently, separate from the firmware bundle download command.
+		 */
+		ret = enter_ffu_mode(dev_fd);
+		if (ret)
+			goto out;
+	}
 
 do_retry:
 	bytes_left = fw_size;
@@ -2924,7 +2976,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);
@@ -2935,7 +2987,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;
 		}
 
@@ -2962,6 +3014,16 @@  do_retry:
 		off += bytes_per_loop;
 	}
 
+	if (ffu_mode != 1) {
+		/*
+		 * If the device is not in FFU mode 1, the command to exit FFU mode will be sent
+		 * independently, separate from the firmware bundle download command.
+		 */
+		ret = exit_ffu_mode(dev_fd);
+		if (ret)
+			goto out;
+	}
+
 	ret = get_ffu_sectors_programmed(dev_fd, ext_csd);
 out:
 	free(multi_cmd);
@@ -3009,7 +3071,7 @@  out:
 	return ret;
 }
 
-int do_ffu(int nargs, char **argv)
+static int __do_ffu(int nargs, char **argv, __u8 ffu_mode)
 {
 	int dev_fd, img_fd;
 	int ret = -EINVAL;
@@ -3085,7 +3147,7 @@  int do_ffu(int nargs, char **argv)
 	}
 
 	/* Download firmware bundle */
-	ret = do_ffu_download(dev_fd, ext_csd, fw_buf, fw_size, default_chunk);
+	ret = do_ffu_download(dev_fd, ext_csd, fw_buf, fw_size, default_chunk, ffu_mode);
 	/* Check programmed sectors */
 	if (ret > 0 && (ret * 512) == fw_size) {
 		fprintf(stderr, "Programmed %jd/%jd bytes\n", (intmax_t)fw_size, (intmax_t)fw_size);
@@ -3125,6 +3187,16 @@  out:
 	return ret;
 }
 
+int do_ffu1(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..e2eba91 100644
--- a/mmc_cmds.h
+++ b/mmc_cmds.h
@@ -41,7 +41,8 @@  int do_rpmb_read_block(int nargs, char **argv);
 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_ffu1(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);