> Hey Avri,
> Thanks for the comments.
>
> >dont
> >^^^^
> >Don't
> My bad, will correct in v2
>
> >
> >
> >> FFU used to divide the fw_size by native sector size.
> >> If native sector size is 4K the accesses need to be aligned and a
> >> multiple of 4K, other than that CMD23 SET_BLOCK_COUNT does not
> change.
> >I am not sure what are you fixing, aside of violating a spec requirement:
> >" Downloaded firmware bundle must be DATA_SECTOR_SIZE size aligned
> (internal padding of the bundle might be required)."
> >So if the fw fluf is not sector-size aligned, you need to make it so.
>
> So clearly the commit message does not a good job explaining either the
> problem or the fix, so let me try again.
> First of all I only care about 4K native sector size devices here, the rest works
> fine, the behavior should not be changed by the patch
> for 512B sector size devices. (If you think it does, please speak up!)
> 4k native sector size device FFU is currently broken in mmc-utils.
So maybe make the title say just that, and also add a fixes tag?
> Why?
> We get some firmware of fw_size bytes, we want to tranfer it to the device.
>
> Let's go through the code:
> First we have an "alignment" check, which ensures multiple and sets
> sect_size accordingly.
> sect_size = (ext_csd[EXT_CSD_DATA_SECTOR_SIZE] == 0) ? 512 : 4096;
> if (fw_size % sect_size) {
> fprintf(stderr, "Firmware data size (%jd) is not aligned!\n",
> (intmax_t)fw_size);
> goto out;
> }
> Then there is the following
>
> /* calculate required fw blocks for CMD25 */
> blocks = fw_size / sect_size;
>
> for native 4k device this is now the mount of 4k blocks of fw.
>
> The fw is now transferred in one CMD23/25 transfer:
> /* send block count */
> multi_cmd->cmds[1].opcode = MMC_SET_BLOCK_COUNT;
> multi_cmd->cmds[1].arg = blocks;
> multi_cmd->cmds[1].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
> MMC_CMD_AC;
>
> Here we have the problem already, it sets blocks, the fw_size / 4k as block
> count, this is wrong, should also be fw_size / 512,
Right
> the patch addresses this.
Not quite. The patch set:
multi_cmd->cmds[1].arg = fw_size;
instead of:
multi_cmd->cmds[1].arg = (fw_size >> 9);
>
> Now we have the transfer, with an incorrectly set CMD23 as we will see.
> /* send image chunk */
> multi_cmd->cmds[2].opcode = MMC_WRITE_MULTIPLE_BLOCK;
> multi_cmd->cmds[2].blksz = sect_size;
> multi_cmd->cmds[2].blocks = blocks;
> multi_cmd->cmds[2].arg = arg;
> multi_cmd->cmds[2].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
> MMC_CMD_ADTC;
> multi_cmd->cmds[2].write_flag = 1;
> mmc_ioc_cmd_set_data(multi_cmd->cmds[2], buf);
>
> blocks*sect_size (data and fw_size size in total) is intended to be transferred,
> but CMD23 has just set it to fw_size/8 sectors.
> Furthermore I think since we are transferring everything in one chunk, blksz
> and blocks should reflect this, even though the kernel does not really care in
> that case.
Right - here it's not a bug, but I see your point.
> The patch addresses this (this part should not change actual 512B sector size
> behavior).
>
> The error is just a misunderstanding I would say that CMD23 acts in chunks
> of native sector size, which is not the case, its argument just needs to be
> divisible by 8 in case of native 4k.
>
> >So if the fw fluf is not sector-size aligned, you need to make it so.
> For now the FFU will cancel for non-aligned fw_size, I did not change this
> behavior, so I'd say that is fine.
OK.
Thanks,
Avri
>
> Any future comments appreciated, also I will try to make the commit
> message more understandable, but if you have suggestions, go ahead.
>
> Regards,
> Christian
> Hyperstone GmbH | Reichenaustr. 39a | 78467 Konstanz
> Managing Director: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
