[U-Boot] [PATCH v3 0/2] SPI flash update command

[U-Boot] [PATCH v3 0/2] SPI flash update command

[Gerlando Falauto]

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.

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.

Changes from v2:
 - Perform two write operations but have cleaner code
Changes from v1 [April, 03, 2012 (!)]:
 - Rebased on top of u-boot-spi/master

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 | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

-- 
1.8.0.1

[U-Boot] [PATCH v3 1/2] cmd_sf: let "sf update" erase last sector as a whole

[Gerlando Falauto]

make "sf update" work with unaligned `len' parameter, by deleting the
whole last sector before writing, so to allow for:

 sf update ${load_addr_r} 0 ${filesize}

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
Cc: Holger Brunck <holger.brunck@keymile.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
 common/cmd_sf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 19b0dc9..ab35a94 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -160,7 +160,8 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
 		*skipped += len;
 		return NULL;
 	}
-	if (spi_flash_erase(flash, offset, len))
+	/* Erase the entire sector */
+	if (spi_flash_erase(flash, offset, flash->sector_size))
 		return "erase";
 	if (spi_flash_write(flash, offset, len, buf))
 		return "write";
-- 
1.8.0.1

[U-Boot] [PATCH v3 1/2] cmd_sf: let "sf update" erase last sector as a whole

[Jagan Teki]

On 04-07-2013 00:03, Gerlando Falauto wrote:
> make "sf update" work with unaligned `len' parameter, by deleting the
> whole last sector before writing, so to allow for:
>
>   sf update ${load_addr_r} 0 ${filesize}
>
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> Cc: Holger Brunck <holger.brunck@keymile.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> ---
>   common/cmd_sf.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index 19b0dc9..ab35a94 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -160,7 +160,8 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
>   		*skipped += len;
>   		return NULL;
>   	}
> -	if (spi_flash_erase(flash, offset, len))
> +	/* Erase the entire sector */
> +	if (spi_flash_erase(flash, offset, flash->sector_size))
>   		return "erase";
>   	if (spi_flash_write(flash, offset, len, buf))
>   		return "write";
>
Reviewed-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>

--
Thanks,
Jagan.

[U-Boot] [PATCH v3 1/2] cmd_sf: let "sf update" erase last sector as a whole

[Jagan Teki]

On 07-08-2013 00:27, Jagan Teki wrote:
> On 04-07-2013 00:03, Gerlando Falauto wrote:
>> make "sf update" work with unaligned `len' parameter, by deleting the
>> whole last sector before writing, so to allow for:
>>
>>   sf update ${load_addr_r} 0 ${filesize}
>>
>> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
>> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
>> Cc: Holger Brunck <holger.brunck@keymile.com>
>> Acked-by: Simon Glass <sjg@chromium.org>
>> ---
>>   common/cmd_sf.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
>> index 19b0dc9..ab35a94 100644
>> --- a/common/cmd_sf.c
>> +++ b/common/cmd_sf.c
>> @@ -160,7 +160,8 @@ static const char *spi_flash_update_block(struct
>> spi_flash *flash, u32 offset,
>>           *skipped += len;
>>           return NULL;
>>       }
>> -    if (spi_flash_erase(flash, offset, len))
>> +    /* Erase the entire sector */
>> +    if (spi_flash_erase(flash, offset, flash->sector_size))
>>           return "erase";
>>       if (spi_flash_write(flash, offset, len, buf))
>>           return "write";
>>
> Reviewed-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
>
> --
> Thanks,
> Jagan.

Applied to u-boot-spi/master

--
Thanks,
Jagan.

[U-Boot] [PATCH v3 2/2] cmd_sf: let "sf update" preserve the final part of the last sector

[Gerlando Falauto]

Since "sf update" erases the last block as a whole, but only rewrites
the meaningful initial part of it, the rest would be left erased,
potentially erasing meaningful information.
So, as a safety measure, have it rewrite the original content.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
Cc: Holger Brunck <holger.brunck@keymile.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
 common/cmd_sf.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index ab35a94..1141dc1 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -152,8 +152,10 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
 {
 	debug("offset=%#x, sector_size=%#x, len=%#zx\n",
 		offset, flash->sector_size, len);
-	if (spi_flash_read(flash, offset, len, cmp_buf))
+	/* Read the entire sector so to allow for rewriting */
+	if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
 		return "read";
+	/* Compare only what is meaningful (len) */
 	if (memcmp(cmp_buf, buf, len) == 0) {
 		debug("Skip region %x size %zx: no change\n",
 			offset, len);
@@ -163,8 +165,16 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
 	/* Erase the entire sector */
 	if (spi_flash_erase(flash, offset, flash->sector_size))
 		return "erase";
+	/* Write the initial part of the block from the source */
 	if (spi_flash_write(flash, offset, len, buf))
 		return "write";
+	/* If it's a partial sector, rewrite the existing part */
+	if (len != flash->sector_size) {
+		/* Rewrite the original data to the end of the sector */
+		if (spi_flash_write(flash, offset + len,
+			flash->sector_size - len, &cmp_buf[len]))
+			return "write";
+	}
 	return NULL;
 }
 
-- 
1.8.0.1

[U-Boot] [PATCH v3 2/2] cmd_sf: let "sf update" preserve the final part of the last sector

[Jagan Teki]

On Thu, Jul 4, 2013 at 12:03 AM, Gerlando Falauto
<gerlando.falauto@keymile.com> wrote:
> Since "sf update" erases the last block as a whole, but only rewrites
> the meaningful initial part of it, the rest would be left erased,
> potentially erasing meaningful information.
> So, as a safety measure, have it rewrite the original content.
>
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
> Cc: Holger Brunck <holger.brunck@keymile.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> ---
>  common/cmd_sf.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index ab35a94..1141dc1 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -152,8 +152,10 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
>  {
>         debug("offset=%#x, sector_size=%#x, len=%#zx\n",
>                 offset, flash->sector_size, len);
> -       if (spi_flash_read(flash, offset, len, cmp_buf))
> +       /* Read the entire sector so to allow for rewriting */
> +       if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
>                 return "read";
> +       /* Compare only what is meaningful (len) */
>         if (memcmp(cmp_buf, buf, len) == 0) {
>                 debug("Skip region %x size %zx: no change\n",
>                         offset, len);
> @@ -163,8 +165,16 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
>         /* Erase the entire sector */
>         if (spi_flash_erase(flash, offset, flash->sector_size))
>                 return "erase";
> +       /* Write the initial part of the block from the source */
>         if (spi_flash_write(flash, offset, len, buf))
>                 return "write";

I din't understand why the below write is required again-
As erase ops requires only sector operation and read + write will do
the operations on partial sizes

Can you send the failure case w/o this.

--
Thanks,
Jagan.
> +       /* If it's a partial sector, rewrite the existing part */
> +       if (len != flash->sector_size) {
> +               /* Rewrite the original data to the end of the sector */
> +               if (spi_flash_write(flash, offset + len,
> +                       flash->sector_size - len, &cmp_buf[len]))
> +                       return "write";
> +       }
>         return NULL;
>  }
>
> --
> 1.8.0.1
>

[U-Boot] [PATCH v3 2/2] cmd_sf: let "sf update" preserve the final part of the last sector

[Gerlando Falauto]

Hi Jagan,

On 07/04/2013 06:26 PM, Jagan Teki wrote:
> On Thu, Jul 4, 2013 at 12:03 AM, Gerlando Falauto
> <gerlando.falauto@keymile.com> wrote:
>> Since "sf update" erases the last block as a whole, but only rewrites
>> the meaningful initial part of it, the rest would be left erased,
>> potentially erasing meaningful information.
>> So, as a safety measure, have it rewrite the original content.
>>
>> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
>> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
>> Cc: Holger Brunck <holger.brunck@keymile.com>
>> Acked-by: Simon Glass <sjg@chromium.org>
>> ---
>>   common/cmd_sf.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
>> index ab35a94..1141dc1 100644
>> --- a/common/cmd_sf.c
>> +++ b/common/cmd_sf.c
>> @@ -152,8 +152,10 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
>>   {
>>          debug("offset=%#x, sector_size=%#x, len=%#zx\n",
>>                  offset, flash->sector_size, len);
>> -       if (spi_flash_read(flash, offset, len, cmp_buf))
>> +       /* Read the entire sector so to allow for rewriting */
>> +       if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
>>                  return "read";
>> +       /* Compare only what is meaningful (len) */
>>          if (memcmp(cmp_buf, buf, len) == 0) {
>>                  debug("Skip region %x size %zx: no change\n",
>>                          offset, len);
>> @@ -163,8 +165,16 @@ static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
>>          /* Erase the entire sector */
>>          if (spi_flash_erase(flash, offset, flash->sector_size))
>>                  return "erase";
>> +       /* Write the initial part of the block from the source */
>>          if (spi_flash_write(flash, offset, len, buf))
>>                  return "write";
>
> I din't understand why the below write is required again-
> As erase ops requires only sector operation and read + write will do
> the operations on partial sizes
>
> Can you send the failure case w/o this.

I'm not sure I understand your question.
I thought the commit message above was clear enough.

In any case, the idea is to have "sf update" be as agnostic as possible 
wrt to sector size, so it virtually only erases the same amount of data 
as it is going to overwrite. The rest will be preserved.

This way you could, for instance, store some binary proprietary firmware 
towards the end of the space reserved for u-boot, without having to 
reserve a whole flash sector for it. The reason for doing such a thing 
(as opposed to just embedding it within u-boot itself) is licensing 
issues, so you might want to keep the firmware as close as possible to 
the u-boot binary yet not link it.
Then when you update u-boot (GPL), your firmware is preserved.

Another extreme use case that comes to my mind would be where you have 
the u-boot environment within the same sector where u-boot lies, though
a) I'm not sure it's even possible
b) is of course a BAD, BAD idea
c) See b)
d) See c) and then b), plus is a BAD idea and therefore discouraged
e) it would only make sense if the u-boot environment is never meant to 
be altered except during production

To be honest with you, I don't remember if there was a real use case 
leading me to write this or if it was just all hypothetical or I just 
thought it was nicer that way.

As for changes of v3 wrt v2, the two should be functionally equivalent:
- In v2 I used a memcpy() to write the whole sector at once
- In v3 I avoided the memcpy() but this requires writing the two 
portions separately.

Hope this answers your question.

Thank you,
Gerlando
>
> --
> Thanks,
> Jagan.
>> +       /* If it's a partial sector, rewrite the existing part */
>> +       if (len != flash->sector_size) {
>> +               /* Rewrite the original data to the end of the sector */
>> +               if (spi_flash_write(flash, offset + len,
>> +                       flash->sector_size - len, &cmp_buf[len]))
>> +                       return "write";
>> +       }
>>          return NULL;
>>   }
>>
>> --
>> 1.8.0.1
>>

[U-Boot] [PATCH v3 2/2] cmd_sf: let "sf update" preserve the final part of the last sector

[Jagan Teki]

On Fri, Jul 5, 2013 at 1:01 PM, Gerlando Falauto
<gerlando.falauto@keymile.com> wrote:
> Hi Jagan,
>
>
> On 07/04/2013 06:26 PM, Jagan Teki wrote:
>>
>> On Thu, Jul 4, 2013 at 12:03 AM, Gerlando Falauto
>> <gerlando.falauto@keymile.com> wrote:
>>>
>>> Since "sf update" erases the last block as a whole, but only rewrites
>>> the meaningful initial part of it, the rest would be left erased,
>>> potentially erasing meaningful information.
>>> So, as a safety measure, have it rewrite the original content.
>>>
>>> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
>>> Cc: Valentin Longchamp <valentin.longchamp@keymile.com>
>>> Cc: Holger Brunck <holger.brunck@keymile.com>
>>> Acked-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>   common/cmd_sf.c | 12 +++++++++++-
>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
>>> index ab35a94..1141dc1 100644
>>> --- a/common/cmd_sf.c
>>> +++ b/common/cmd_sf.c
>>> @@ -152,8 +152,10 @@ static const char *spi_flash_update_block(struct
>>> spi_flash *flash, u32 offset,
>>>   {
>>>          debug("offset=%#x, sector_size=%#x, len=%#zx\n",
>>>                  offset, flash->sector_size, len);
>>> -       if (spi_flash_read(flash, offset, len, cmp_buf))
>>> +       /* Read the entire sector so to allow for rewriting */
>>> +       if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
>>>                  return "read";
>>> +       /* Compare only what is meaningful (len) */
>>>          if (memcmp(cmp_buf, buf, len) == 0) {
>>>                  debug("Skip region %x size %zx: no change\n",
>>>                          offset, len);
>>> @@ -163,8 +165,16 @@ static const char *spi_flash_update_block(struct
>>> spi_flash *flash, u32 offset,
>>>          /* Erase the entire sector */
>>>          if (spi_flash_erase(flash, offset, flash->sector_size))
>>>                  return "erase";
>>> +       /* Write the initial part of the block from the source */
>>>          if (spi_flash_write(flash, offset, len, buf))
>>>                  return "write";
>>
>>
>> I din't understand why the below write is required again-
>> As erase ops requires only sector operation and read + write will do
>> the operations on partial sizes
>>
>> Can you send the failure case w/o this.
>
>
> I'm not sure I understand your question.
> I thought the commit message above was clear enough.
>
> In any case, the idea is to have "sf update" be as agnostic as possible wrt
> to sector size, so it virtually only erases the same amount of data as it is
> going to overwrite. The rest will be preserved.
>
> This way you could, for instance, store some binary proprietary firmware
> towards the end of the space reserved for u-boot, without having to reserve
> a whole flash sector for it. The reason for doing such a thing (as opposed
> to just embedding it within u-boot itself) is licensing issues, so you might
> want to keep the firmware as close as possible to the u-boot binary yet not
> link it.
> Then when you update u-boot (GPL), your firmware is preserved.
>
> Another extreme use case that comes to my mind would be where you have the
> u-boot environment within the same sector where u-boot lies, though
> a) I'm not sure it's even possible
> b) is of course a BAD, BAD idea
> c) See b)
> d) See c) and then b), plus is a BAD idea and therefore discouraged
> e) it would only make sense if the u-boot environment is never meant to be
> altered except during production
>
> To be honest with you, I don't remember if there was a real use case leading
> me to write this or if it was just all hypothetical or I just thought it was
> nicer that way.
>
> As for changes of v3 wrt v2, the two should be functionally equivalent:
> - In v2 I used a memcpy() to write the whole sector at once
> - In v3 I avoided the memcpy() but this requires writing the two portions
> separately.

What if don't use second write, is that a condition where we are not
sure the write gonna be happen
properly in case of  partial sectors.

-- 
Thanks,
Jagan.

>
> Hope this answers your question.
>
> Thank you,
> Gerlando
>
>>
>> --
>> Thanks,
>> Jagan.
>>>
>>> +       /* If it's a partial sector, rewrite the existing part */
>>> +       if (len != flash->sector_size) {
>>> +               /* Rewrite the original data to the end of the sector */
>>> +               if (spi_flash_write(flash, offset + len,
>>> +                       flash->sector_size - len, &cmp_buf[len]))
>>> +                       return "write";
>>> +       }
>>>          return NULL;
>>>   }
>>>
>>> --
>>> 1.8.0.1
>>>
>