Hi,
We had discussed this internally with Gerlando, but I will repeat my position here on the ML for the sake of the discussion.
On 04/03/2012 05:14 PM, Gerlando Falauto wrote:
Hi,
this patchset allows "sf update" to erase+write a number of bytes which is not a multiple of the sector size. Start address must still be sector-aligned though.
First I think this feature is really nice: the u-boot update command then becomes really simple. But I have some remarks on how we should implement it.
That's my first remark here: some arguments have to be sector aligned and some not. That's not really consistent.
The first patch trivially makes it such it will always erase an entire sector before writing, regardless of the amount of data to write (i.e. the last sector is erased completely before writing it only partially).
The second patch just makes sure that the original data at the end of the sector is written back so to apparently remain unchanged.
I anticipate two potential objections already:
whether it is really worth writing back the portion of the sector which was erased but shouldn't have been overwritten (whole purpose of the second patch)
these changes make the semantics of "sf update" and "sf erase" somewhat different, in that "sf erase" needs a "+" to deal with odd lengths, whereas "sf update" does not. I think this is only partially true. After all, "sf update" is already somehow special. It's not a standard operation for a flash (erase, read, write). It combines two of those operations, and takes care of optimizing the process by removing unneeded erase/write operations. So it might as well deserve to be "special", in my opinion. Plus, it makes the command for updating u-boot as easy as
The sector aligned arguments are mandatory for the erase command, and update is as explained above erase + write commands (some erase/writes may be spared, but that's transparent to the users). So why have a different nomenclature for update and erase, when update is only putting erase and write commands together ?
With the "+" approach for the update command, we would achieve exactly the same as proposed by these 2 patches, plus we are consistent with the "+" nomenclature of the erase.
sf update ${load_addr_r} 0 ${filesize}
It would then only be:
sf update ${load_addr_r} 0 +${filesize}
Not a lot more complicated.
We have to think about 2 points here:
1) Do we want our sf commands to remain tied to the sectors ? If we want low level control on the flash, that may be something desirable and then the "+" approach makes more sense (you know that you may not be sector aligned and imply with the "+".
2) Do we want our sf commands that are concerned by sector size (erase and update) to have consistence in the arguments ? Why would an erase require a "+" that an update does not require ? (This means that if we add the same functionality than the "+" in update without the "+", why keep it for erase ?).
Feedback and criticism more than welcome, of course.
Thank you, Gerlando
Gerlando Falauto (2): cmd_sf: let "sf update" erase last sector as a whole cmd_sf: "sf update" preserve the final part of the last sector
common/cmd_sf.c | 18 ++++++++++++++++-- 1 files changed, 16 insertions(+), 2 deletions(-)
Best Regards
Valentin