| Message ID | 20241013210925.123632-2-beanhuo@iokpp.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add multiple FFU modes in mmc-utils based on eMMC specification 6.6.18 for flexible firmware updates | expand |
> mmc_cmds.c | 287 +++++++++++++++++++++++++++++++---------------------- > 1 file changed, 171 insertions(+), 116 deletions(-) > > diff --git a/mmc_cmds.c b/mmc_cmds.c > index 3b1bcf4..72921a7 100644 > --- a/mmc_cmds.c > +++ b/mmc_cmds.c > @@ -29,6 +29,7 @@ > #include <stdint.h> > #include <assert.h> > #include <linux/fs.h> /* for BLKGETSIZE */ > +#include <stdbool.h> > > #include "mmc.h" > #include "mmc_cmds.h" > @@ -2810,15 +2811,13 @@ out: > return ret; > } > > -static void set_ffu_single_cmd(struct mmc_ioc_multi_cmd *multi_cmd, > - __u8 *ext_csd, unsigned int bytes, __u8 *buf, > - off_t offset) > +static void set_ffu_download_cmd(struct mmc_ioc_multi_cmd *multi_cmd, > __u8 *ext_csd, > + unsigned int bytes, __u8 *buf, off_t > +offset) > { > __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); > + 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; > > /* > @@ -2826,23 +2825,141 @@ static void set_ffu_single_cmd(struct > mmc_ioc_multi_cmd *multi_cmd, > * 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); > + 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); } It is not recommended to mix functional & formatting changes. > > +static int get_ffu_sectors_programmed(int *dev_fd, __u8 *ext_csd) { > + int ret; > + > + ret = read_extcsd(*dev_fd, ext_csd); > + if (ret) { > + fprintf(stderr, "Could not read EXT_CSD\n"); > + return ret; > + } > + > + ret = per_byte_htole32((__u8 > + *)&ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]); > + > + return ret; > +} ret is not really needed. It is not common for the return value to indicate both error and expected functionality. > + > +static bool ffu_is_supported(__u8 *ext_csd, char *device) { > + if (ext_csd == NULL) { > + fprintf(stderr, "ext_cst is NULL\n"); ^^^^ s/cst/csd/g > + return false; > + } > + > + if (ext_csd[EXT_CSD_REV] < EXT_CSD_REV_V5_0) { > + fprintf(stderr, "The FFU feature is only available on devices >= " > + "MMC 5.0, not supported in %s\n", device); > + return false; > + } > + > + if (!(ext_csd[EXT_CSD_SUPPORTED_MODES] & EXT_CSD_FFU)) { > + fprintf(stderr, "FFU is not supported in %s\n", device); > + return false; > + } > + > + if (ext_csd[EXT_CSD_FW_CONFIG] & EXT_CSD_UPDATE_DISABLE) { > + fprintf(stderr, "Firmware update was disabled in %s\n", device); > + return false; > + } > + > + return true; > +} > + > +static int do_ffu_download(int *dev_fd, __u8 *ext_csd, __u8 *fw_buf, off_t > fw_size, > + unsigned int > +chunk_size) { Why dev_fd can't be just an integer? > + int ret; > + __u8 num_of_cmds = 4; > + off_t bytes_left, off; > + unsigned int bytes_per_loop, sect_done, retry = 3; > + struct mmc_ioc_multi_cmd *multi_cmd = NULL; > + > + if (!dev_fd || !fw_buf || !ext_csd) { > + fprintf(stderr, "unexpected NULL pointer\n"); > + return -EINVAL; > + } > + /* allocate maximum required */ > + multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + > num_of_cmds * sizeof(struct mmc_ioc_cmd)); > + if (!multi_cmd) { > + perror("failed to allocate memory"); > + return -ENOMEM; > + } I was expecting that do_ffu_download will be called with struct mmc_ioc_multi_cmd *multi_cmd argument as well. That each ffu<x> mode fills it according to its own logic. This you won't be needing that ffu_mode additional parameter. > + > + /* 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: > + bytes_left = fw_size; > + off = 0; > + multi_cmd->num_of_cmds = num_of_cmds; > + > + while (bytes_left) { > + 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); > + > + /* send ioctl with multi-cmd, download firmware bundle */ > + ret = ioctl(*dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); > + > + if (ret) { > + perror("Multi-cmd ioctl"); > + /* > + * In case multi-cmd ioctl failed before exiting from > + * ffu mode > + */ > + ioctl(*dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]); > + goto out; > + } > + > + sect_done = get_ffu_sectors_programmed(dev_fd, ext_csd); > + if (sect_done <= 0) { > + /* By spec, host should re-start download from the first sector > if sect_done is 0 */ > + ioctl(*dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]); > + if (retry > 0) { > + retry--; > + fprintf(stderr, "Programming failed. Retrying... (%d)\n", > retry); > + goto do_retry; > + } > + fprintf(stderr, "Programming failed! Aborting...\n"); > + ret = sect_done; > + goto out; > + } else { > + fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done * 512, > (intmax_t)fw_size); > + } > + > + bytes_left -= bytes_per_loop; > + off += bytes_per_loop; > + } > + > + ret = get_ffu_sectors_programmed(dev_fd, ext_csd); > +out: > + free(multi_cmd); > + return ret; > + > +} > + > int do_ffu(int nargs, char **argv) > { > + off_t fw_size; > + char *device; > + int sect_done = 0; > int dev_fd, img_fd; > - int retry = 3, ret = -EINVAL; > + int ret = -EINVAL; > unsigned int sect_size; > __u8 ext_csd[512]; > - __u8 *buf = NULL; > - off_t fw_size, bytes_left, off; > - char *device; > + __u8 *fw_buf = NULL; > struct mmc_ioc_multi_cmd *multi_cmd = NULL; > unsigned int default_chunk = MMC_IOC_MAX_BYTES; > - __u32 sect_done = 0; It is not recommended to mix functional & formatting changes. > > assert (nargs == 3 || nargs == 4); > > @@ -2852,6 +2969,7 @@ int do_ffu(int nargs, char **argv) > perror("device open failed"); > exit(1); > } > + > img_fd = open(argv[1], O_RDONLY); > if (img_fd < 0) { > perror("image open failed"); @@ -2859,28 +2977,22 @@ int > do_ffu(int nargs, char **argv) > exit(1); > } > > + if (nargs == 4) { > + default_chunk = strtol(argv[3], NULL, 10); > + if (default_chunk > MMC_IOC_MAX_BYTES || default_chunk % > 512) { > + fprintf(stderr, "Invalid chunk size"); > + goto out; > + } > + } > + Can the argument parsing part be common to all modes as well? > ret = read_extcsd(dev_fd, ext_csd); > if (ret) { > fprintf(stderr, "Could not read EXT_CSD from %s\n", device); > goto out; > } can this part also be part of ffu_is_supported()? > > - if (ext_csd[EXT_CSD_REV] < EXT_CSD_REV_V5_0) { > - fprintf(stderr, > - "The FFU feature is only available on devices >= " > - "MMC 5.0, not supported in %s\n", device); > - goto out; > - } > - > - if (!(ext_csd[EXT_CSD_SUPPORTED_MODES] & EXT_CSD_FFU)) { > - fprintf(stderr, "FFU is not supported in %s\n", device); > - goto out; > - } > - > - if (ext_csd[EXT_CSD_FW_CONFIG] & EXT_CSD_UPDATE_DISABLE) { > - fprintf(stderr, "Firmware update was disabled in %s\n", device); > + if (ffu_is_supported(ext_csd, device) != true) If (!ffu_is_supported(ext_csd, device)) > goto out; > - } > > fw_size = lseek(img_fd, 0, SEEK_END); > if (fw_size == 0) { > @@ -2888,15 +3000,6 @@ int do_ffu(int nargs, char **argv) > goto out; > } > > - /* allocate maximum required */ > - buf = malloc(fw_size); > - multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + > - 4 * sizeof(struct mmc_ioc_cmd)); > - if (!buf || !multi_cmd) { > - perror("failed to allocate memory"); > - goto out; > - } > - > /* ensure fw is multiple of native sector size */ > sect_size = (ext_csd[EXT_CSD_DATA_SECTOR_SIZE] == 0) ? 512 : 4096; > if (fw_size % sect_size) { > @@ -2904,61 +3007,32 @@ int do_ffu(int nargs, char **argv) > goto out; > } > > - if (nargs == 4) { > - default_chunk = strtol(argv[3], NULL, 10); > - if (default_chunk > MMC_IOC_MAX_BYTES || default_chunk % 512) > { > - fprintf(stderr, "Invalid chunk size"); > - goto out; > - } > + /* allocate maximum required */ > + fw_buf = malloc(fw_size); > + if (!fw_buf) { > + perror("failed to allocate memory"); > + goto out; > } > > - /* prepare multi_cmd for FFU based on cmd to be used */ > - > - multi_cmd->num_of_cmds = 4; > - > - /* 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); > - > /* read firmware */ > lseek(img_fd, 0, SEEK_SET); > - if (read(img_fd, buf, fw_size) != fw_size) { > + if (read(img_fd, fw_buf, fw_size) != fw_size) { > perror("Could not read the firmware file: "); > ret = -ENOSPC; > goto out; > } > > -do_retry: > - bytes_left = fw_size; > - off = 0; > - while (bytes_left) { > - unsigned int chunk_size = bytes_left < default_chunk ? > - bytes_left : default_chunk; > - > - /* prepare multi_cmd for FFU based on cmd to be used */ > - set_ffu_single_cmd(multi_cmd, ext_csd, chunk_size, buf, off); > - > - /* send ioctl with multi-cmd */ > - ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); > + sect_done = do_ffu_download((int *)&dev_fd, ext_csd, fw_buf, > + fw_size, default_chunk); > > - if (ret) { > - perror("Multi-cmd ioctl"); > - /* > - * In case multi-cmd ioctl failed before exiting from > - * ffu mode > - */ > - ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]); > - goto out; > - } > - > - bytes_left -= chunk_size; > - off += chunk_size; > + /* Check programmed sectors */ > + if (sect_done > 0 && (sect_done * 512) == fw_size) { > + fprintf(stderr, "Programmed %jd/%jd bytes\n", (intmax_t)fw_size, > (intmax_t)fw_size); > + fprintf(stderr, "Programming finished with status %d \n", ret); > + } else { > + fprintf(stderr, "Firmware bundle download failed. Operation status > %d\n", sect_done); > + ret = -EIO; > + goto out; > } > - > /* > * By spec - check if mode operation codes are supported in ffu features, > * if not then skip checking number of sectors programmed after install > @@ -2969,48 +3043,29 @@ do_retry: > goto out; > } > > - ret = read_extcsd(dev_fd, ext_csd); > - if (ret) { > - fprintf(stderr, "Could not read EXT_CSD from %s\n", device); > - goto out; > - } > - > - /* Test if we need to restart the download */ > - sect_done = > per_byte_htole32(&ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]); > - /* By spec, host should re-start download from the first sector if > sect_done is 0 */ > - if (sect_done == 0) { > - if (retry--) { > - fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry); > - goto do_retry; > - } > - fprintf(stderr, "Programming failed! Aborting...\n"); > - goto out; > - } > - > - if ((sect_done * sect_size) == fw_size) { > - fprintf(stderr, "Programmed %jd/%jd bytes\n", (intmax_t)fw_size, > (intmax_t)fw_size); > - fprintf(stderr, "Programming finished with status %d \n", ret); > - } > - else { > - fprintf(stderr, "FW size and number of sectors written mismatch. > Status return %d\n", ret); > + fprintf(stderr, "Installing firmware on %s...\n", device); > + multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + 2 * > sizeof(struct mmc_ioc_cmd)); > + if (!multi_cmd) { > + perror("failed to allocate memory"); > + ret = -ENOMEM; > goto out; > } > > - fprintf(stderr, "Installing firmware on %s...\n", device); > /* Re-enter ffu mode and install the firmware */ > multi_cmd->num_of_cmds = 2; > - > - /* set ext_csd to install mode */ > - fill_switch_cmd(&multi_cmd->cmds[1], > EXT_CSD_MODE_OPERATION_CODES, > - EXT_CSD_FFU_INSTALL); > + /* put device into ffu mode */ > + fill_switch_cmd(&multi_cmd->cmds[0], EXT_CSD_MODE_CONFIG, > EXT_CSD_FFU_MODE); > + /* Re-enter ffu mode and set ext_csd to install mode */ > + fill_switch_cmd(&multi_cmd->cmds[1], > + EXT_CSD_MODE_OPERATION_CODES, EXT_CSD_FFU_INSTALL); > > /* send ioctl with multi-cmd */ > ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); Can wrapping up be common to all modes as well? Thanks, Avri > > if (ret) { > perror("Multi-cmd ioctl failed setting install mode"); > + fill_switch_cmd(&multi_cmd->cmds[1], > + EXT_CSD_MODE_CONFIG, EXT_CSD_NORMAL_MODE); > /* In case multi-cmd ioctl failed before exiting from ffu mode */ > - ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]); > + ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[1]); > goto out; > } > > @@ -3022,16 +3077,16 @@ do_retry: > > /* return status */ > ret = ext_csd[EXT_CSD_FFU_STATUS]; > - if (ret) { > + if (ret) > fprintf(stderr, "%s: error %d during FFU install:\n", device, ret); > - goto out; > - } else { > + else > fprintf(stderr, "FFU finished successfully\n"); > - } > > out: > - free(buf); > - free(multi_cmd); > + if (fw_buf) > + free(fw_buf); > + if (multi_cmd) > + free(multi_cmd); > close(img_fd); > close(dev_fd); > return ret; > -- > 2.34.1
On Mon, 2024-10-14 at 06:55 +0000, Avri Altman wrote: > > + __u8 num_of_cmds = 4; > > + off_t bytes_left, off; > > + unsigned int bytes_per_loop, sect_done, retry = 3; > > + struct mmc_ioc_multi_cmd *multi_cmd = NULL; > > + > > + if (!dev_fd || !fw_buf || !ext_csd) { > > + fprintf(stderr, "unexpected NULL pointer\n"); > > + return -EINVAL; > > + } > > + /* allocate maximum required */ > > + multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + > > num_of_cmds * sizeof(struct mmc_ioc_cmd)); > > + if (!multi_cmd) { > > + perror("failed to allocate memory"); > > + return -ENOMEM; > > + } > I was expecting that do_ffu_download will be called with struct > mmc_ioc_multi_cmd *multi_cmd argument as well. > That each ffu<x> mode fills it according to its own logic. > This you won't be needing that ffu_mode additional parameter. I wanted to clarify why the ffu_mode parameter is necessary during the download phase. By extracting the logic into a common approach and using ffu_mode to dynamically update the multi_cmd structure across iterations per ffu-mode, I can handle the variations between different FFU modes more effectively. This allows me to extract more common code and avoid duplication, as the ffu_mode helps determine which specific updates or adjustments need to be made to the multi_cmd at each stage. Without this, it would be difficult to manage multiple loops or iterations of the download process, especially when the command structure needs to be modified in different ways depending on the mode. The use of ffu_mode centralizes this control, making the code cleaner and more maintainable. Kind regards, Bean
> On Mon, 2024-10-14 at 06:55 +0000, Avri Altman wrote: > > > + __u8 num_of_cmds = 4; > > > + off_t bytes_left, off; > > > + unsigned int bytes_per_loop, sect_done, retry = 3; > > > + struct mmc_ioc_multi_cmd *multi_cmd = NULL; > > > + > > > + if (!dev_fd || !fw_buf || !ext_csd) { > > > + fprintf(stderr, "unexpected NULL pointer\n"); > > > + return -EINVAL; > > > + } > > > + /* allocate maximum required */ > > > + multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + > > > num_of_cmds * sizeof(struct mmc_ioc_cmd)); > > > + if (!multi_cmd) { > > > + perror("failed to allocate memory"); > > > + return -ENOMEM; > > > + } > > I was expecting that do_ffu_download will be called with struct > > mmc_ioc_multi_cmd *multi_cmd argument as well. > > That each ffu<x> mode fills it according to its own logic. > > This you won't be needing that ffu_mode additional parameter. > > I wanted to clarify why the ffu_mode parameter is necessary during the > download phase. By extracting the logic into a common approach and using > ffu_mode to dynamically update the multi_cmd structure across iterations > per ffu-mode, I can handle the variations between different FFU modes more > effectively. This allows me to extract more common code and avoid > duplication, as the ffu_mode helps determine which specific updates or > adjustments need to be made to the multi_cmd at each stage. > > Without this, it would be difficult to manage multiple loops or iterations of > the download process, especially when the command structure needs to be > modified in different ways depending on the mode. > The use of ffu_mode centralizes this control, making the code cleaner and > more maintainable. I see your point. Each mode is packing differently the multi-ioctl, and you need to update different offset of the cmds part. So how about instead of ffu mode, adjust set_ffu_single_cmd to do just that. And maybe in few preparation phases - to make the review process more concise: First let's make it get the cmds part of the multi-ioctl instead of the multi-ioctl: +static void set_ffu_write_cmd(struct mmc_ioc_cmd *cmds, __u8 *ext_csd, unsigned int bytes, + __u8 *buf, off_t offset) { __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; + set_single_cmd(&cmds[0], MMC_SET_BLOCK_COUNT, 0, 0, bytes / 512); + cmds[0].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); + set_single_cmd(&cmds[1], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / 512, arg); + mmc_ioc_cmd_set_data(cmds[1], buf + offset); +} And then, allow it to carry sbc or not: +static void set_ffu_sbc_cmd(struct mmc_ioc_cmd *cmds, int bytes) { - __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; + set_single_cmd(&cmds[0], MMC_SET_BLOCK_COUNT, 0, 0, bytes / 512); + cmds[0].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC; +} +static void set_ffu_write_cmd(struct mmc_ioc_cmd *cmds, __u8 *ext_csd, unsigned int bytes, + __u8 *buf, off_t offset, bool sbc) +{ + __u32 arg = per_byte_htole32(&ext_csd[EXT_CSD_FFU_ARG_0]); + + if (sbc) + set_ffu_sbc_cmd(cmds, bytes); /* * 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); + set_single_cmd(&cmds[1], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / 512, arg); + mmc_ioc_cmd_set_data(cmds[1], buf + offset); +} + And only then, as a 3rd preparation phase move the do_retry out. And you need to tell it what offset the write commands are. I guess this is the equivalent of your ffu_mode. But the difference is that when I look into do_ffu2, do_ffu3 etc. - The logic of pointing to the write command is there. +static int __do_ffu(int dev_fd, char *device, struct mmc_ioc_multi_cmd *multi_cmd, int cmd_off, + off_t fw_size, unsigned int sect_size, unsigned int default_chunk, + __u8 *ext_csd, __u8 *buf) Maybe we can pack all the ffu arguments into a single control struct to get passed to __do_ffu as struct ffu_params *. +{ + off_t bytes_left, off; + __u32 sect_done = 0; + __u8 ffu_feature = ext_csd[EXT_CSD_FFU_FEATURES]; + int write_command_offset = cmd_off; + int retry = 3; + int ret; + +do_retry: + bytes_left = fw_size; + off = 0; + while (bytes_left) { + unsigned int chunk_size = bytes_left < default_chunk ? + bytes_left : default_chunk; + + /* prepare multi_cmd for FFU based on cmd to be used */ + set_ffu_write_cmd(&multi_cmd->cmds[write_command_offset], + ext_csd, chunk_size, buf, off, 1); + + /* send ioctl with multi-cmd */ + ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); + if (ret) { + perror("Multi-cmd ioctl"); + /* + * In case multi-cmd ioctl failed before exiting from + * ffu mode + */ + ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]); + return ret; + } + + bytes_left -= chunk_size; + off += chunk_size; + } + + /* + * By spec - check if mode operation codes are supported in ffu features, + * if not then skip checking number of sectors programmed after install + */ + if (!ffu_feature) { + fprintf(stderr, "Please reboot to complete firmware installation on %s\n", device); + return 0; + } + + ret = read_extcsd(dev_fd, ext_csd); + if (ret) { + fprintf(stderr, "Could not read EXT_CSD from %s\n", device); + return ret; + } + + /* Test if we need to restart the download */ + sect_done = per_byte_htole32(&ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]); + /* By spec, host should re-start download from the first sector if sect_done is 0 */ + if (sect_done == 0) { + if (retry--) { + fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry); + goto do_retry; + } + fprintf(stderr, "Programming failed! Aborting...\n"); + return -EINVAL; + } + + fprintf(stderr, "Programmed %jd/%jd bytes\n", (intmax_t)sect_done * sect_size, (intmax_t)fw_size); + + if ((sect_done * sect_size) == fw_size) { + fprintf(stderr, "Programming finished successfully\n"); + return 0; + } + + fprintf(stderr, "FW size and number of sectors written mismatch\n"); + return -EINVAL; } Each mode can have its own arguments - it’s a different de-facto util so you are not bounded by do_ffu arguments. Thus the argument parsing is done for each mode differently. Also allocating the multi-ioctl is privet to each mode. Some have 4, some have 2, and some 1. Thanks, Avri > > Kind regards, > Bean
On Mon, 2024-10-21 at 07:39 +0000, Avri Altman wrote: > > > > + return -ENOMEM; > > > > + } > > > I was expecting that do_ffu_download will be called with struct > > > mmc_ioc_multi_cmd *multi_cmd argument as well. > > > That each ffu<x> mode fills it according to its own logic. > > > This you won't be needing that ffu_mode additional parameter. > > > > I wanted to clarify why the ffu_mode parameter is necessary during > > the > > download phase. By extracting the logic into a common approach and > > using > > ffu_mode to dynamically update the multi_cmd structure across > > iterations > > per ffu-mode, I can handle the variations between different FFU > > modes more > > effectively. This allows me to extract more common code and avoid > > duplication, as the ffu_mode helps determine which specific updates > > or > > adjustments need to be made to the multi_cmd at each stage. > > > > Without this, it would be difficult to manage multiple loops or > > iterations of > > the download process, especially when the command structure needs > > to be > > modified in different ways depending on the mode. > > The use of ffu_mode centralizes this control, making the code > > cleaner and > > more maintainable. > I see your point. > Each mode is packing differently the multi-ioctl, and you need to > update different offset of the cmds part. > So how about instead of ffu mode, adjust set_ffu_single_cmd to do > just that. > > And maybe in few preparation phases - to make the review process more > concise: > First let's make it get the cmds part of the multi-ioctl instead of > the multi-ioctl: > +static void set_ffu_write_cmd(struct mmc_ioc_cmd *cmds, __u8 > *ext_csd, unsigned int bytes, > + __u8 *buf, off_t offset) > { > __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; > + set_single_cmd(&cmds[0], MMC_SET_BLOCK_COUNT, 0, 0, bytes / > 512); > + cmds[0].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); > + set_single_cmd(&cmds[1], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / > 512, arg); > + mmc_ioc_cmd_set_data(cmds[1], buf + offset); > +} > > And then, allow it to carry sbc or not: > +static void set_ffu_sbc_cmd(struct mmc_ioc_cmd *cmds, int bytes) > { > - __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; > + set_single_cmd(&cmds[0], MMC_SET_BLOCK_COUNT, 0, 0, bytes / > 512); > + cmds[0].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC; > +} > > +static void set_ffu_write_cmd(struct mmc_ioc_cmd *cmds, __u8 > *ext_csd, unsigned int bytes, > + __u8 *buf, off_t offset, bool sbc) > +{ > + __u32 arg = per_byte_htole32(&ext_csd[EXT_CSD_FFU_ARG_0]); > + > + if (sbc) > + set_ffu_sbc_cmd(cmds, bytes); > /* > * 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); > + set_single_cmd(&cmds[1], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / > 512, arg); > + mmc_ioc_cmd_set_data(cmds[1], buf + offset); > +} > + > > And only then, as a 3rd preparation phase move the do_retry out. > And you need to tell it what offset the write commands are. I guess > this is the equivalent of your ffu_mode. > But the difference is that when I look into do_ffu2, do_ffu3 etc. - > The logic of pointing to the write command is there. > > +static int __do_ffu(int dev_fd, char *device, struct > mmc_ioc_multi_cmd *multi_cmd, int cmd_off, > + off_t fw_size, unsigned int sect_size, unsigned > int default_chunk, > + __u8 *ext_csd, __u8 *buf) > Maybe we can pack all the ffu arguments into a single control struct > to get passed to __do_ffu as struct ffu_params *. > > +{ > + off_t bytes_left, off; > + __u32 sect_done = 0; > + __u8 ffu_feature = ext_csd[EXT_CSD_FFU_FEATURES]; > + int write_command_offset = cmd_off; > + int retry = 3; > + int ret; > + > +do_retry: > + bytes_left = fw_size; > + off = 0; > + while (bytes_left) { > + unsigned int chunk_size = bytes_left < default_chunk > ? > + bytes_left : default_chunk; > + > + /* prepare multi_cmd for FFU based on cmd to be used > */ > + set_ffu_write_cmd(&multi_cmd- > >cmds[write_command_offset], > + ext_csd, chunk_size, buf, > off, 1); > + > + /* send ioctl with multi-cmd */ > + ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); > + if (ret) { > + perror("Multi-cmd ioctl"); > + /* > + * In case multi-cmd ioctl failed before > exiting from > + * ffu mode > + */ > + ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd- > >cmds[3]); > + return ret; > + } > + > + bytes_left -= chunk_size; > + off += chunk_size; > + } > + > + /* > + * By spec - check if mode operation codes are supported in > ffu features, > + * if not then skip checking number of sectors programmed > after install > + */ > + if (!ffu_feature) { > + fprintf(stderr, "Please reboot to complete firmware > installation on %s\n", device); > + return 0; > + } > + > + ret = read_extcsd(dev_fd, ext_csd); > + if (ret) { > + fprintf(stderr, "Could not read EXT_CSD from %s\n", > device); > + return ret; > + } > + > + /* Test if we need to restart the download */ > + sect_done = > per_byte_htole32(&ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]); > + /* By spec, host should re-start download from the first > sector if sect_done is 0 */ > + if (sect_done == 0) { > + if (retry--) { > + fprintf(stderr, "Programming failed. > Retrying... (%d)\n", retry); > + goto do_retry; > + } > + fprintf(stderr, "Programming failed! Aborting...\n"); > + return -EINVAL; > + } > + > + fprintf(stderr, "Programmed %jd/%jd bytes\n", > (intmax_t)sect_done * sect_size, (intmax_t)fw_size); > + > + if ((sect_done * sect_size) == fw_size) { > + fprintf(stderr, "Programming finished > successfully\n"); > + return 0; > + } > + > + fprintf(stderr, "FW size and number of sectors written > mismatch\n"); > + return -EINVAL; > } > > Each mode can have its own arguments - it’s a different de-facto util > so you are not bounded by do_ffu arguments. > Thus the argument parsing is done for each mode differently. > Also allocating the multi-ioctl is privet to each mode. Some have 4, > some have 2, and some 1. > > Thanks, > Avri I can change your suggestions, before changing, I would like to have a solid agreement, The approach in my patch is to concentrate differences in do_ffu_download() while keeping common functions outside it, the reason for this becuase the main difference btw ffu_modes are FW budnle download. void _do_ffu(int ffu_mode) { // Declare parameters specific to the FFU mode do_ffu_prepare(); ffu_is_supported(); do_ffu_download(ffu_mode); do_ffu_installation(); } void do_ffu1() { _do_ffu(FFU_MODE_1); } void do_ffu2() { _do_ffu(FFU_MODE_2); } Advantages: Reduced Duplication: Each do_ffu<x>() avoids repeating the same logic. Improved Maintainability: Changes only need to be made in one location. Clear Separation: Common operations remain consistent across all modes. The approach you suggested is to sperate them at the begigning of do_ffu<x>, then pass the different parameters to each different function: It risks unnecessary duplication and increased complexity, as seen in this structure: void do_ffu1() { // Duplicate logic here do_ffu_prepare(ffu1); ffu_is_supported(); do_ffu_download(ffu_structure); do_ffu_installation(); } void do_ffu2() { // Duplicate logic here do_ffu_prepare(ffu2); ffu_is_supported(); do_ffu_download(ffu_structure); do_ffu_installation(); } Please tell me was the second one what you expected? Kind regards, Bean
> On Mon, 2024-10-21 at 07:39 +0000, Avri Altman wrote: > > > > > + return -ENOMEM; > > > > > + } > > > > I was expecting that do_ffu_download will be called with struct > > > > mmc_ioc_multi_cmd *multi_cmd argument as well. > > > > That each ffu<x> mode fills it according to its own logic. > > > > This you won't be needing that ffu_mode additional parameter. > > > > > > I wanted to clarify why the ffu_mode parameter is necessary during > > > the download phase. By extracting the logic into a common approach > > > and using ffu_mode to dynamically update the multi_cmd structure > > > across iterations per ffu-mode, I can handle the variations between > > > different FFU modes more effectively. This allows me to extract more > > > common code and avoid duplication, as the ffu_mode helps determine > > > which specific updates or adjustments need to be made to the > > > multi_cmd at each stage. > > > > > > Without this, it would be difficult to manage multiple loops or > > > iterations of the download process, especially when the command > > > structure needs to be modified in different ways depending on the > > > mode. > > > The use of ffu_mode centralizes this control, making the code > > > cleaner and more maintainable. > > I see your point. > > Each mode is packing differently the multi-ioctl, and you need to > > update different offset of the cmds part. > > So how about instead of ffu mode, adjust set_ffu_single_cmd to do just > > that. > > > > And maybe in few preparation phases - to make the review process more > > concise: > > First let's make it get the cmds part of the multi-ioctl instead of > > the multi-ioctl: > > +static void set_ffu_write_cmd(struct mmc_ioc_cmd *cmds, __u8 > > *ext_csd, unsigned int bytes, > > + __u8 *buf, off_t offset) > > { > > __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; > > + set_single_cmd(&cmds[0], MMC_SET_BLOCK_COUNT, 0, 0, bytes / > > 512); > > + cmds[0].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); > > + set_single_cmd(&cmds[1], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / > > 512, arg); > > + mmc_ioc_cmd_set_data(cmds[1], buf + offset); } > > > > And then, allow it to carry sbc or not: > > +static void set_ffu_sbc_cmd(struct mmc_ioc_cmd *cmds, int bytes) > > { > > - __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; > > + set_single_cmd(&cmds[0], MMC_SET_BLOCK_COUNT, 0, 0, bytes / > > 512); > > + cmds[0].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC; } > > > > +static void set_ffu_write_cmd(struct mmc_ioc_cmd *cmds, __u8 > > *ext_csd, unsigned int bytes, > > + __u8 *buf, off_t offset, bool sbc) { > > + __u32 arg = per_byte_htole32(&ext_csd[EXT_CSD_FFU_ARG_0]); > > + > > + if (sbc) > > + set_ffu_sbc_cmd(cmds, bytes); > > /* > > * 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); > > + set_single_cmd(&cmds[1], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes / > > 512, arg); > > + mmc_ioc_cmd_set_data(cmds[1], buf + offset); } > > + > > > > And only then, as a 3rd preparation phase move the do_retry out. > > And you need to tell it what offset the write commands are. I guess > > this is the equivalent of your ffu_mode. > > But the difference is that when I look into do_ffu2, do_ffu3 etc. - > > The logic of pointing to the write command is there. > > > > +static int __do_ffu(int dev_fd, char *device, struct > > mmc_ioc_multi_cmd *multi_cmd, int cmd_off, > > + off_t fw_size, unsigned int sect_size, unsigned > > int default_chunk, > > + __u8 *ext_csd, __u8 *buf) > > Maybe we can pack all the ffu arguments into a single control struct > > to get passed to __do_ffu as struct ffu_params *. > > > > +{ > > + off_t bytes_left, off; > > + __u32 sect_done = 0; > > + __u8 ffu_feature = ext_csd[EXT_CSD_FFU_FEATURES]; > > + int write_command_offset = cmd_off; > > + int retry = 3; > > + int ret; > > + > > +do_retry: > > + bytes_left = fw_size; > > + off = 0; > > + while (bytes_left) { > > + unsigned int chunk_size = bytes_left < default_chunk > > ? > > + bytes_left : default_chunk; > > + > > + /* prepare multi_cmd for FFU based on cmd to be used > > */ > > + set_ffu_write_cmd(&multi_cmd- > > >cmds[write_command_offset], > > + ext_csd, chunk_size, buf, > > off, 1); > > + > > + /* send ioctl with multi-cmd */ > > + ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); > > + if (ret) { > > + perror("Multi-cmd ioctl"); > > + /* > > + * In case multi-cmd ioctl failed before > > exiting from > > + * ffu mode > > + */ > > + ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd- > > >cmds[3]); > > + return ret; > > + } > > + > > + bytes_left -= chunk_size; > > + off += chunk_size; > > + } > > + > > + /* > > + * By spec - check if mode operation codes are supported in > > ffu features, > > + * if not then skip checking number of sectors programmed > > after install > > + */ > > + if (!ffu_feature) { > > + fprintf(stderr, "Please reboot to complete firmware > > installation on %s\n", device); > > + return 0; > > + } > > + > > + ret = read_extcsd(dev_fd, ext_csd); > > + if (ret) { > > + fprintf(stderr, "Could not read EXT_CSD from %s\n", > > device); > > + return ret; > > + } > > + > > + /* Test if we need to restart the download */ > > + sect_done = > > per_byte_htole32(&ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]); > > + /* By spec, host should re-start download from the first > > sector if sect_done is 0 */ > > + if (sect_done == 0) { > > + if (retry--) { > > + fprintf(stderr, "Programming failed. > > Retrying... (%d)\n", retry); > > + goto do_retry; > > + } > > + fprintf(stderr, "Programming failed! Aborting...\n"); > > + return -EINVAL; > > + } > > + > > + fprintf(stderr, "Programmed %jd/%jd bytes\n", > > (intmax_t)sect_done * sect_size, (intmax_t)fw_size); > > + > > + if ((sect_done * sect_size) == fw_size) { > > + fprintf(stderr, "Programming finished > > successfully\n"); > > + return 0; > > + } > > + > > + fprintf(stderr, "FW size and number of sectors written > > mismatch\n"); > > + return -EINVAL; > > } > > > > Each mode can have its own arguments - it’s a different de-facto util > > so you are not bounded by do_ffu arguments. > > Thus the argument parsing is done for each mode differently. > > Also allocating the multi-ioctl is privet to each mode. Some have 4, > > some have 2, and some 1. > > > > Thanks, > > Avri > > I can change your suggestions, before changing, I would like to have a solid > agreement, > > > The approach in my patch is to concentrate differences in > do_ffu_download() while keeping common functions outside it, the reason > for this becuase the main difference btw ffu_modes are FW budnle > download. > > void _do_ffu(int ffu_mode) { > > // Declare parameters specific to the FFU mode > do_ffu_prepare(); > ffu_is_supported(); > > do_ffu_download(ffu_mode); > > do_ffu_installation(); > } > > void do_ffu1() { > _do_ffu(FFU_MODE_1); > } > > void do_ffu2() { > _do_ffu(FFU_MODE_2); > } > > > Advantages: > > Reduced Duplication: Each do_ffu<x>() avoids repeating the same logic. > Improved Maintainability: Changes only need to be made in one location. > Clear Separation: Common operations remain consistent across all modes. > > > The approach you suggested is to sperate them at the begigning of > do_ffu<x>, then pass the different parameters to each different > function: > > It risks unnecessary duplication and increased complexity, as seen in this > structure: > > void do_ffu1() { > // Duplicate logic here > do_ffu_prepare(ffu1); > ffu_is_supported(); > do_ffu_download(ffu_structure); > do_ffu_installation(); > } > > void do_ffu2() { > // Duplicate logic here > do_ffu_prepare(ffu2); > ffu_is_supported(); > do_ffu_download(ffu_structure); > do_ffu_installation(); > } > > Please tell me was the second one what you expected? I see that you still think that the ffu_mode is better. I don't want to block the progress of the series - as I am aware that there are customers that are waiting. Please feel free to add my Acked-by tag to the whole series. Thanks, Avri > > > Kind regards, > Bean
On Mon, 2024-10-21 at 13:02 +0000, Avri Altman wrote: > > Reduced Duplication: Each do_ffu<x>() avoids repeating the same > > logic. > > Improved Maintainability: Changes only need to be made in one > > location. > > Clear Separation: Common operations remain consistent across all > > modes. > > > > > > The approach you suggested is to sperate them at the begigning of > > do_ffu<x>, then pass the different parameters to each different > > function: > > > > It risks unnecessary duplication and increased complexity, as seen > > in this > > structure: > > > > void do_ffu1() { > > // Duplicate logic here > > do_ffu_prepare(ffu1); > > ffu_is_supported(); > > do_ffu_download(ffu_structure); > > do_ffu_installation(); > > } > > > > void do_ffu2() { > > // Duplicate logic here > > do_ffu_prepare(ffu2); > > ffu_is_supported(); > > do_ffu_download(ffu_structure); > > do_ffu_installation(); > > } > > > > Please tell me was the second one what you expected? > I see that you still think that the ffu_mode is better. > I don't want to block the progress of the series - as I am aware that > there are customers that are waiting. > Please feel free to add my Acked-by tag to the whole series. > > Thanks, > Avri Thanks, I plan to send out the v4 patch this week and will incorporate your inputs inline as possible as it can. Kind regards, Bean
diff --git a/mmc_cmds.c b/mmc_cmds.c index 3b1bcf4..72921a7 100644 --- a/mmc_cmds.c +++ b/mmc_cmds.c @@ -29,6 +29,7 @@ #include <stdint.h> #include <assert.h> #include <linux/fs.h> /* for BLKGETSIZE */ +#include <stdbool.h> #include "mmc.h" #include "mmc_cmds.h" @@ -2810,15 +2811,13 @@ out: return ret; } -static void set_ffu_single_cmd(struct mmc_ioc_multi_cmd *multi_cmd, - __u8 *ext_csd, unsigned int bytes, __u8 *buf, - off_t offset) +static void set_ffu_download_cmd(struct mmc_ioc_multi_cmd *multi_cmd, __u8 *ext_csd, + unsigned int bytes, __u8 *buf, off_t offset) { __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); + 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; /* @@ -2826,23 +2825,141 @@ static void set_ffu_single_cmd(struct mmc_ioc_multi_cmd *multi_cmd, * 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); + 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); } +static int get_ffu_sectors_programmed(int *dev_fd, __u8 *ext_csd) +{ + int ret; + + ret = read_extcsd(*dev_fd, ext_csd); + if (ret) { + fprintf(stderr, "Could not read EXT_CSD\n"); + return ret; + } + + ret = per_byte_htole32((__u8 *)&ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]); + + return ret; +} + +static bool ffu_is_supported(__u8 *ext_csd, char *device) +{ + if (ext_csd == NULL) { + fprintf(stderr, "ext_cst is NULL\n"); + return false; + } + + if (ext_csd[EXT_CSD_REV] < EXT_CSD_REV_V5_0) { + fprintf(stderr, "The FFU feature is only available on devices >= " + "MMC 5.0, not supported in %s\n", device); + return false; + } + + if (!(ext_csd[EXT_CSD_SUPPORTED_MODES] & EXT_CSD_FFU)) { + fprintf(stderr, "FFU is not supported in %s\n", device); + return false; + } + + if (ext_csd[EXT_CSD_FW_CONFIG] & EXT_CSD_UPDATE_DISABLE) { + fprintf(stderr, "Firmware update was disabled in %s\n", device); + return false; + } + + return true; +} + +static int do_ffu_download(int *dev_fd, __u8 *ext_csd, __u8 *fw_buf, off_t fw_size, + unsigned int chunk_size) +{ + int ret; + __u8 num_of_cmds = 4; + off_t bytes_left, off; + unsigned int bytes_per_loop, sect_done, retry = 3; + struct mmc_ioc_multi_cmd *multi_cmd = NULL; + + if (!dev_fd || !fw_buf || !ext_csd) { + fprintf(stderr, "unexpected NULL pointer\n"); + return -EINVAL; + } + /* allocate maximum required */ + multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + num_of_cmds * sizeof(struct mmc_ioc_cmd)); + if (!multi_cmd) { + perror("failed to allocate memory"); + 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: + bytes_left = fw_size; + off = 0; + multi_cmd->num_of_cmds = num_of_cmds; + + while (bytes_left) { + 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); + + /* send ioctl with multi-cmd, download firmware bundle */ + ret = ioctl(*dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); + + if (ret) { + perror("Multi-cmd ioctl"); + /* + * In case multi-cmd ioctl failed before exiting from + * ffu mode + */ + ioctl(*dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]); + goto out; + } + + sect_done = get_ffu_sectors_programmed(dev_fd, ext_csd); + if (sect_done <= 0) { + /* By spec, host should re-start download from the first sector if sect_done is 0 */ + ioctl(*dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]); + if (retry > 0) { + retry--; + fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry); + goto do_retry; + } + fprintf(stderr, "Programming failed! Aborting...\n"); + ret = sect_done; + goto out; + } else { + fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done * 512, (intmax_t)fw_size); + } + + bytes_left -= bytes_per_loop; + off += bytes_per_loop; + } + + ret = get_ffu_sectors_programmed(dev_fd, ext_csd); +out: + free(multi_cmd); + return ret; + +} + int do_ffu(int nargs, char **argv) { + off_t fw_size; + char *device; + int sect_done = 0; int dev_fd, img_fd; - int retry = 3, ret = -EINVAL; + int ret = -EINVAL; unsigned int sect_size; __u8 ext_csd[512]; - __u8 *buf = NULL; - off_t fw_size, bytes_left, off; - char *device; + __u8 *fw_buf = NULL; struct mmc_ioc_multi_cmd *multi_cmd = NULL; unsigned int default_chunk = MMC_IOC_MAX_BYTES; - __u32 sect_done = 0; assert (nargs == 3 || nargs == 4); @@ -2852,6 +2969,7 @@ int do_ffu(int nargs, char **argv) perror("device open failed"); exit(1); } + img_fd = open(argv[1], O_RDONLY); if (img_fd < 0) { perror("image open failed"); @@ -2859,28 +2977,22 @@ int do_ffu(int nargs, char **argv) exit(1); } + if (nargs == 4) { + default_chunk = strtol(argv[3], NULL, 10); + if (default_chunk > MMC_IOC_MAX_BYTES || default_chunk % 512) { + fprintf(stderr, "Invalid chunk size"); + goto out; + } + } + ret = read_extcsd(dev_fd, ext_csd); if (ret) { fprintf(stderr, "Could not read EXT_CSD from %s\n", device); goto out; } - if (ext_csd[EXT_CSD_REV] < EXT_CSD_REV_V5_0) { - fprintf(stderr, - "The FFU feature is only available on devices >= " - "MMC 5.0, not supported in %s\n", device); - goto out; - } - - if (!(ext_csd[EXT_CSD_SUPPORTED_MODES] & EXT_CSD_FFU)) { - fprintf(stderr, "FFU is not supported in %s\n", device); - goto out; - } - - if (ext_csd[EXT_CSD_FW_CONFIG] & EXT_CSD_UPDATE_DISABLE) { - fprintf(stderr, "Firmware update was disabled in %s\n", device); + if (ffu_is_supported(ext_csd, device) != true) goto out; - } fw_size = lseek(img_fd, 0, SEEK_END); if (fw_size == 0) { @@ -2888,15 +3000,6 @@ int do_ffu(int nargs, char **argv) goto out; } - /* allocate maximum required */ - buf = malloc(fw_size); - multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + - 4 * sizeof(struct mmc_ioc_cmd)); - if (!buf || !multi_cmd) { - perror("failed to allocate memory"); - goto out; - } - /* ensure fw is multiple of native sector size */ sect_size = (ext_csd[EXT_CSD_DATA_SECTOR_SIZE] == 0) ? 512 : 4096; if (fw_size % sect_size) { @@ -2904,61 +3007,32 @@ int do_ffu(int nargs, char **argv) goto out; } - if (nargs == 4) { - default_chunk = strtol(argv[3], NULL, 10); - if (default_chunk > MMC_IOC_MAX_BYTES || default_chunk % 512) { - fprintf(stderr, "Invalid chunk size"); - goto out; - } + /* allocate maximum required */ + fw_buf = malloc(fw_size); + if (!fw_buf) { + perror("failed to allocate memory"); + goto out; } - /* prepare multi_cmd for FFU based on cmd to be used */ - - multi_cmd->num_of_cmds = 4; - - /* 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); - /* read firmware */ lseek(img_fd, 0, SEEK_SET); - if (read(img_fd, buf, fw_size) != fw_size) { + if (read(img_fd, fw_buf, fw_size) != fw_size) { perror("Could not read the firmware file: "); ret = -ENOSPC; goto out; } -do_retry: - bytes_left = fw_size; - off = 0; - while (bytes_left) { - unsigned int chunk_size = bytes_left < default_chunk ? - bytes_left : default_chunk; - - /* prepare multi_cmd for FFU based on cmd to be used */ - set_ffu_single_cmd(multi_cmd, ext_csd, chunk_size, buf, off); - - /* send ioctl with multi-cmd */ - ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); + sect_done = do_ffu_download((int *)&dev_fd, ext_csd, fw_buf, fw_size, default_chunk); - if (ret) { - perror("Multi-cmd ioctl"); - /* - * In case multi-cmd ioctl failed before exiting from - * ffu mode - */ - ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]); - goto out; - } - - bytes_left -= chunk_size; - off += chunk_size; + /* Check programmed sectors */ + if (sect_done > 0 && (sect_done * 512) == fw_size) { + fprintf(stderr, "Programmed %jd/%jd bytes\n", (intmax_t)fw_size, (intmax_t)fw_size); + fprintf(stderr, "Programming finished with status %d \n", ret); + } else { + fprintf(stderr, "Firmware bundle download failed. Operation status %d\n", sect_done); + ret = -EIO; + goto out; } - /* * By spec - check if mode operation codes are supported in ffu features, * if not then skip checking number of sectors programmed after install @@ -2969,48 +3043,29 @@ do_retry: goto out; } - ret = read_extcsd(dev_fd, ext_csd); - if (ret) { - fprintf(stderr, "Could not read EXT_CSD from %s\n", device); - goto out; - } - - /* Test if we need to restart the download */ - sect_done = per_byte_htole32(&ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]); - /* By spec, host should re-start download from the first sector if sect_done is 0 */ - if (sect_done == 0) { - if (retry--) { - fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry); - goto do_retry; - } - fprintf(stderr, "Programming failed! Aborting...\n"); - goto out; - } - - if ((sect_done * sect_size) == fw_size) { - fprintf(stderr, "Programmed %jd/%jd bytes\n", (intmax_t)fw_size, (intmax_t)fw_size); - fprintf(stderr, "Programming finished with status %d \n", ret); - } - else { - fprintf(stderr, "FW size and number of sectors written mismatch. Status return %d\n", ret); + fprintf(stderr, "Installing firmware on %s...\n", device); + multi_cmd = calloc(1, sizeof(struct mmc_ioc_multi_cmd) + 2 * sizeof(struct mmc_ioc_cmd)); + if (!multi_cmd) { + perror("failed to allocate memory"); + ret = -ENOMEM; goto out; } - fprintf(stderr, "Installing firmware on %s...\n", device); /* Re-enter ffu mode and install the firmware */ multi_cmd->num_of_cmds = 2; - - /* set ext_csd to install mode */ - fill_switch_cmd(&multi_cmd->cmds[1], EXT_CSD_MODE_OPERATION_CODES, - EXT_CSD_FFU_INSTALL); + /* put device into ffu mode */ + fill_switch_cmd(&multi_cmd->cmds[0], EXT_CSD_MODE_CONFIG, EXT_CSD_FFU_MODE); + /* Re-enter ffu mode and set ext_csd to install mode */ + fill_switch_cmd(&multi_cmd->cmds[1], EXT_CSD_MODE_OPERATION_CODES, EXT_CSD_FFU_INSTALL); /* send ioctl with multi-cmd */ ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd); if (ret) { perror("Multi-cmd ioctl failed setting install mode"); + fill_switch_cmd(&multi_cmd->cmds[1], EXT_CSD_MODE_CONFIG, EXT_CSD_NORMAL_MODE); /* In case multi-cmd ioctl failed before exiting from ffu mode */ - ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]); + ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[1]); goto out; } @@ -3022,16 +3077,16 @@ do_retry: /* return status */ ret = ext_csd[EXT_CSD_FFU_STATUS]; - if (ret) { + if (ret) fprintf(stderr, "%s: error %d during FFU install:\n", device, ret); - goto out; - } else { + else fprintf(stderr, "FFU finished successfully\n"); - } out: - free(buf); - free(multi_cmd); + if (fw_buf) + free(fw_buf); + if (multi_cmd) + free(multi_cmd); close(img_fd); close(dev_fd); return ret;