From mboxrd@z Thu Jan  1 00:00:00 1970
From: Valentin Longchamp <valentin.longchamp@keymile.com>
Date: Wed, 04 Apr 2012 08:40:57 +0200
Subject: [U-Boot] [PATCH 0/2] SPI flash update command
In-Reply-To: <1333466098-31783-1-git-send-email-gerlando.falauto@keymile.com>
References: <4F5F9103.1030807@keymile.com>
	<1333466098-31783-1-git-send-email-gerlando.falauto@keymile.com>
Message-ID: <4F7BECF9.9040705@keymile.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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