> 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
