Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-27 Thread Øyvind Harboe
I can't test the code anytime soon, but it looks OK to me so I
think you should commit it if you've run tests on it.

I would like the same padding logic added to flash erase_address
to restore that functionality.

Qualified by an pad option to round up/down to nearest sector boundary,
if you prefer.





-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-27 Thread David Brownell
On Sunday 27 December 2009, Øyvind Harboe wrote:
 I can't test the code anytime soon, but it looks OK to me so I
 think you should commit it if you've run tests on it.

My thoughts exactly.  Done.


 I would like the same padding logic added to flash erase_address
 to restore that functionality.
 
 Qualified by an pad option to round up/down to nearest sector boundary,
 if you prefer.

I'll think about that.  Any such dangerous erase extra stuff
should certainly not be a default behavior, when someone has gone
through the effort to specify exactly the address range they want
to erase!

- Dave
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-27 Thread Øyvind Harboe
 Qualified by an pad option to round up/down to nearest sector boundary,
 if you prefer.

 I'll think about that.  Any such dangerous erase extra stuff
 should certainly not be a default behavior, when someone has gone
 through the effort to specify exactly the address range they want
 to erase!

The feature of being able to erase all sectors in an address
range is to allow users to erase without remembering
the precise sector boundaries. From scripts this
can be *especially* annoying.

This feature should be restored before 0.4, but I'm fine
by adding a new option to force alignment to nearest sector
boundaries. This would be in line with the fine write_image
traditions of unlock/erase.


-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-24 Thread David Brownell
On Wednesday 23 December 2009, Øyvind Harboe wrote:
 Now, there is a small problem you pointed out: that two elf images
 that are too close together won't work with the flash write
 image erase option,

Don't forget the original one:  flash erase_address was
erasing data which it was told to leave alone.


 but here the user can use flash erase_address 
 range in one pass and then multiple invocations of flash
 write_image, so I think we should just ignore this problem
 as we have up until now and nobody raised the problem.

That's sort of beside the point.  The write_image command
is a layer up from the bug that got fixed.

What we're coping with now is the usual thing that crops up
when low level bugs get fixed ... higher-level bugs which
they papered over now become visible.

- Dave

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-24 Thread David Brownell
On Tuesday 22 December 2009, Austin, Alex wrote:
 Read-modify-erase-write on per-sector basis?

That's more work than I want to do for now, but it would be a
more general solution.

My conclusion from looking at that code is that the intelligence
of the auto-erase code is ... a lot less than I've applied when
sorting these things out by hand.

Not also that the auto-unprotect doesn't include auto-reprotect.

 
  I'm going to sort through the messages here and come up with
  a solution that's not broken-by-design.

Looks like the simplest fix for now is to update the existing
padding code in write_image to just pad to end-of-last-sector.

That code is kind of rude -- its notion of padding includes
arbitrary numbers of sectors between sections -- but can be
easily tweaked to handle the two last section unaligned
cases reported here.

- Dave
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-24 Thread David Brownell
On Tuesday 22 December 2009, Yegor Yefremov wrote:
 flash write_image erase image.bin 0x1000 bin
 auto erase enabled
 address range 0x1000 .. 0x000135d3 is not sector-aligned
 Command handler execution failed

The following patch should help in at least some cases;
it doesn't insert sector pre-padding but does update the
existing post-pad logic.

- Dave

== CUT HERE
From: David Brownell dbrown...@users.sourceforge.net
Subject:  NOR: make flash_write_unlock() pad to sector end

Resolve a regression when using new automagic write_image modes,
by always padding to the end of affected sectors.  Also:

 - Allocate the right amount of memory on 64-bit hosts (previous
   code could corrupt server memory), switching to calloc() to
   simplify review and initialization.

 - Document issues associated with the newish automagic options,
   adding some code comments too.

 - Fix syntax error:  wrote N bytes; writing a single byte is
   an unusual case, not the normal one.

We might need similar padding at the *beginning* of some sectors,
but this is a minimalist fix for the problems which have currently
been reported (plus that bug on 64-bit hosts, and doc fixes).
---
 doc/openocd.texi |   25 +++--
 src/flash/nor/core.c |   46 +++---
 src/flash/nor/tcl.c  |4 ++--
 3 files changed, 68 insertions(+), 7 deletions(-)

--- a/doc/openocd.texi
+++ b/doc/openocd.texi
@@ -3797,8 +3797,29 @@ explicitly as @option{bin} (binary), @op
 The relevant flash sectors will be erased prior to programming
 if the @option{erase} parameter is given. If @option{unlock} is
 provided, then the flash banks are unlocked before erase and
-program. The flash bank to use is inferred from the @var{address} of
-each image segment.
+program. The flash bank to use is inferred from the address of
+each image section.
+
+...@quotation Warning
+Be careful using the @option{erase} flag when the flash is holding
+data you want to preserve.
+Portions of the flash outside those described in the image's
+sections might be erased with no notice.
+...@itemize
+...@item
+When a section of the image being written does not fill out all the
+sectors it uses, the unwritten parts of those sectors are necessarily
+also erased, because sectors can't be partially erased.
+...@item
+Data stored in sector holes between image sections is also affected.
+For example, @command{flash write_image erase ...} of an image with
+one byte at the beginning of a flash bank and one byte at the end
+erases the entire bank -- not just the two sectors being written.
+...@end itemize
+Also, when flash protection is important, you must re-apply it after
+it has been removed by the @option{unlock} flag.
+...@end quotation
+
 @end deffn
 
 @section Other Flash commands
--- a/src/flash/nor/core.c
+++ b/src/flash/nor/core.c
@@ -401,7 +401,7 @@ int flash_write_unlock(struct target *ta
}
 
/* allocate padding array */
-   padding = malloc(image-num_sections * sizeof(padding));
+   padding = calloc(image-num_sections, sizeof(*padding));
 
/* loop until we reach end of the image */
while (section  image-num_sections)
@@ -439,9 +439,26 @@ int flash_write_unlock(struct target *ta
{
if (image-sections[section_last + 1].base_address  
(run_address + run_size))
{
-   LOG_DEBUG(section %d out of order(very 
slightly surprising, but supported), section_last + 1);
+   LOG_DEBUG(section %d out of order 
+   (surprising, but supported),
+   section_last + 1);
+   /* REVISIT this can break with autoerase ...
+* clobbering data after it's written.
+*/
break;
}
+
+   /* REVISIT This needlessly touches sectors BETWEEN the
+* sections it's writing.  Without auto erase, it just
+* writes ones; unlikely to destroy data.
+*
+* With auto erase enabled, data in those sectors will
+* be needlessly destroyed; and some of the limited
+* number of flash erase cycles will be wasted.
+*
+* In both cases, the extra writes slow things down.
+*/
+
/* if we have multiple sections within our image, flash 
programming could fail due to alignment issues
 * attempt to rebuild a consecutive buffer for the 
flash loader */
pad_bytes = (image-sections[section_last + 
1].base_address) - (run_address + run_size);
@@ -450,7 +467,6 @@ int flash_write_unlock(struct 

Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-23 Thread Øyvind Harboe
What flash erase address range does is to a) identify sectors
within an address range b) erase all those sectors. There is
certainly nothing broken about a) and if I wanted to do b) then
there is nothing broken about that either.

At most, there could be a non-default option to flash erase_address
that would force non-sector aligned writes without complaints.

Non-contrived examples of elf images will never align to sector
sizes so issuing even a warning when running flash write_image
here is wrong.

Hooking up a JTAG debugger in the first place is dangerous.

The whole point of flash write_image is that you can tell it
flash write_image dammit: unlock, erase, maim, whatever,
just do it.

Now, there is a small problem you pointed out: that two elf images
that are too close together won't work with the flash write
image erase option, but here the user can use flash erase_address
range in one pass and then multiple invocations of flash
write_image, so I think we should just ignore this problem
as we have up until now and nobody raised the problem.


-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-23 Thread Øyvind Harboe
I do think it might be useful to print actual address
ranges erased in some reasonably terse and non
warning format.

This would help the user learn what flash address_range
/ write_image erase is doing and should cover your concern
as well, I think.

-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-23 Thread Michael Schwingen
Øyvind Harboe wrote:
 Now, there is a small problem you pointed out: that two elf images
 that are too close together won't work with the flash write
 image erase option, but here the user can use flash erase_address
 range in one pass and then multiple invocations of flash
 write_image, so I think we should just ignore this problem
 as we have up until now and nobody raised the problem.
   
Agreed. I do not think this case is really relevant. In all cases I have
seen until now, images always have separate sectors, never two images
sharing one sector - because sharing a sector will damage the second
image if the first is upgraded in-system and power fails.

If such a case really is needed, the separate erase should be enough.

cu
Michael


___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-23 Thread David Brownell
On Wednesday 23 December 2009, Øyvind Harboe wrote:
 I do think it might be useful to print actual address
 ranges erased in some reasonably terse and non
 warning format.

If it's going to be erasing data beyond what it was told
to erase, we deserve at the very least a warning.

That's pretty basic:  don't destroy user data.  And if
you must, at least warn that something may now be broken.


 This would help the user learn what flash address_range
 / write_image erase is doing and should cover your concern
 as well, I think.

My concern is that it erases the wrong address range:
a bigger one than I told it to erase.  Just a warning
from flash erase_address ... would NOT be enough.


What we have right now is some upper layer calls (at
least write_image) that are trying to rely on a lower
level interface that was dangerously broken ... when it
should just be telling those lower level calls exactly
what to do.

- Dave
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-23 Thread Øyvind Harboe
On Wed, Dec 23, 2009 at 1:47 PM, David Brownell davi...@pacbell.net wrote:
 On Wednesday 23 December 2009, Řyvind Harboe wrote:
 I do think it might be useful to print actual address
 ranges erased in some reasonably terse and non
 warning format.

 If it's going to be erasing data beyond what it was told
 to erase, we deserve at the very least a warning.

 That's pretty basic:  don't destroy user data.  And if
 you must, at least warn that something may now be broken.

I think a note of actual address ranges erased could be good, but what
flash erase_address does is to identify *all* sectors in that range
and erase those
sectors.

So if you wanted some other functionality, then this command wasn't
for you.

 What we have right now is some upper layer calls (at
 least write_image) that are trying to rely on a lower
 level interface that was dangerously broken ... when it
 should just be telling those lower level calls exactly
 what to do.

The flash erase address is doing precisely what it is told to:
erase all sectors in that address range. If that isn't what you wanted,
then use another function.

By making it do something else, the contract to the upper levels
is broken.

flash write_image erase will erase *all* sectors in the address ranges
of the image and then write the image to flash.

If this isn't clear from the documentation, then we should make it clear.


-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-22 Thread Yegor Yefremov
On Fri, Dec 18, 2009 at 12:49 AM, David Brownell davi...@pacbell.net wrote:
 Much to my surprise, I observed a flash erase_address ...
 command erasing data which I didn't say should be erased.

 The issue turns out to be generic NOR flash code which was
 silently, and rather dangerously, morphing partial-sector
 references into unrequested whole-sector ones.

 This patch removes that low-level morphing.  If desired, it
 can and should be done in higher level code.
 ---
 I suspect there's one GDB server path which should do that
 morphing, but it looks like a later one already handles it.
 Other than that, does anyone object to fixing this?

  NEWS                 |    2 ++
  src/flash/nor/core.c |   44 
  2 files changed, 38 insertions(+), 8 deletions(-)

 --- a/NEWS
 +++ b/NEWS
 @@ -39,6 +39,8 @@ Flash Layer:
                - bank_name: reference the bank with its defined name
                - driver_name[.N]: reference the driver's Nth bank
        New 'nand verify' command to check bank against an image file.
 +       The flash erase_address command now rejects partial sectors;
 +               previously it would silently erase data you did not request.

  Board, Target, and Interface Configuration Scripts:
        ARM9
 --- a/src/flash/nor/core.c
 +++ b/src/flash/nor/core.c
 @@ -279,11 +279,13 @@ int default_flash_blank_check(struct fla

        return ERROR_OK;
  }
 +
  /* erase given flash region, selects proper bank according to target and 
 address */
  static int flash_iterate_address_range(struct target *target, uint32_t addr, 
 uint32_t length,
                int (*callback)(struct flash_bank *bank, int first, int last))
  {
        struct flash_bank *c;
 +       uint32_t last_addr = addr + length;     /* first address AFTER end */
        int first = -1;
        int last = -1;
        int i;
 @@ -306,26 +308,52 @@ static int flash_iterate_address_range(s
                return callback(c, 0, c-num_sectors - 1);
        }

 -       /* check whether it fits */
 +       /* check whether it all fits in this bank */
        if (addr + length - 1  c-base + c-size - 1)
                return ERROR_FLASH_DST_BREAKS_ALIGNMENT;

 +       /** @todo: handle erasures that cross into adjacent banks */
 +
        addr -= c-base;

        for (i = 0; i  c-num_sectors; i++)
        {
 -               /* check whether sector overlaps with the given range and is 
 not yet erased */
 -               if (addr  c-sectors[i].offset + c-sectors[i].size  addr 
 + length  c-sectors[i].offset  c-sectors[i].is_erased != 1) {
 -                       /* if first is not set yet then this is the first 
 sector */
 -                       if (first == -1)
 +               struct flash_sector *f = c-sectors + i;
 +
 +               /* start only on a sector boundary */
 +               if (first  0) {
 +                       /* is this the first sector? */
 +                       if (addr == f-offset)
                                first = i;
 -                       last = i; /* and it is the last one so far in any 
 case */
 +                       else if (addr  f-offset)
 +                               break;
 +               }
 +
 +               /* is this (also?) the last sector? */
 +               if (last_addr == f-offset + f-size) {
 +                       last = i;
 +                       break;
                }
 +
 +               /* MUST finish on a sector boundary */
 +               if (last_addr = f-offset)
 +                       break;
        }

 -       if (first == -1 || last == -1)
 -               return ERROR_OK;
 +       /* invalid start or end address? */
 +       if (first == -1 || last == -1) {
 +               LOG_ERROR(address range 0x%8.8x .. 0x%8.8x 
 +                               is not sector-aligned,
 +                               (unsigned) c-base + addr,
 +                               (unsigned) last_addr - 1);
 +               return ERROR_FLASH_DST_BREAKS_ALIGNMENT;
 +       }

 +       /* The NOR driver may trim this range down, based on
 +        * whether or not a given sector is already erased.
 +        *
 +        * REVISIT should *we* trim it... ?
 +        */
        return callback(c, first, last);
  }

 ___
 Openocd-development mailing list
 Openocd-development@lists.berlios.de
 https://lists.berlios.de/mailman/listinfo/openocd-development


After applying the latest patches 0.4-rc1 I encountered the following error:

halt
flash write_image erase image.bin 0x1000 bin
auto erase enabled
address range 0x1000 .. 0x000135d3 is not sector-aligned
Command handler execution failed
in procedure 'flash' called at file command.c, line 637
called at file command.c, line 352

my image has 75220 bytes. According to your patch
3f18900b19f49a84ba9df56f2e089c255e610011, only whole sectors are
allowed. As far as I understand if my image has only one section, it
will not be padded (see 

Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-22 Thread Øyvind Harboe
On Tue, Dec 22, 2009 at 3:47 PM, Yegor Yefremov
yegorsli...@googlemail.com wrote:
 On Fri, Dec 18, 2009 at 12:49 AM, David Brownell davi...@pacbell.net wrote:
 Much to my surprise, I observed a flash erase_address ...
 command erasing data which I didn't say should be erased.

 The issue turns out to be generic NOR flash code which was
 silently, and rather dangerously, morphing partial-sector
 references into unrequested whole-sector ones.

 This patch removes that low-level morphing.  If desired, it
 can and should be done in higher level code.

I missed this patch in the flurry... Don't push it(if it has already,
then it should be
reverted).

The whole point of flash erase_address is to allow an address range to be
erased without knowing the precise sector boundaries.

There are low level commands that can erase only sectors in a specific
bank if that is what you are looking for.

-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-22 Thread Øyvind Harboe
An alternative would be to add an option that is disabled by
default that will check that the address range matches sector
boundaries. This is incompatible, but perhaps a better way to
go?

This would be along the lines of the options we have for flash
write_image.

flash erase_address force  = no error messages upon
address range not matching sector boundaries.

-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-22 Thread Øyvind Harboe
Ouch.

This is a bit worse than I thought.

This breaks flash write_image as well.

Flash write_image will first erase all sectors in address ranges of
all segments to be written, then it will write all segments.



-- 
Øyvind Harboe
US toll free 1-866-980-3434 / International +47 51 63 25 00
http://www.zylin.com/zy1000.html
ARM7 ARM9 ARM11 XScale Cortex
JTAG debugger and flash programmer
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-22 Thread David Brownell
On Tuesday 22 December 2009, Yegor Yefremov wrote:
 After applying the latest patches 0.4-rc1 I encountered the following error:
 
 halt
 flash write_image erase image.bin 0x1000 bin
 auto erase enabled
 address range 0x1000 .. 0x000135d3 is not sector-aligned
 Command handler execution failed
 in procedure 'flash' called at file command.c, line 637
 called at file command.c, line 352
 
 my image has 75220 bytes. According to your patch
 3f18900b19f49a84ba9df56f2e089c255e610011, only whole sectors are
 allowed. As far as I understand if my image has only one section, it
 will not be padded (see flash_write_unlock) or in general the last
 section will not be padded. Due to the new constraint of whole sectors
 the last section will be rejected if it has the wrong size. Do I see
 it right?

Right.  I confess that I expected this to break some things,
which would then need to be fixed.  Here's the first.  :)

The problem is that automatically erasing things outside
of the specified range is actively dangerous; it will break
things.

In this write_image case it might make sense to pass a flag to
the low level code saying it's OK to just issue a warning.

I'll look at the details on this later.

- Dave



___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-22 Thread David Brownell
On Tuesday 22 December 2009, Øyvind Harboe wrote:
 This is a bit worse than I thought.
 
 This breaks flash write_image as well.

That was *this* problem report, in fact...


 Flash write_image will first erase all sectors in address ranges of
 all segments to be written, then it will write all segments.

In short, kind of broken-by-design.  When other portions
of one of those sector should not have been erased -- maybe
the portion being written was still erased! -- it's trashing
stuff that should have been left alone.

Example:  I might have two images to write, which don't
overlap.  Erase, write one, write the next.  Whoops ... it
needlessly erased part of the first image in order to write
the second one, because they used different parts of a sector.


I'm going to sort through the messages here and come up with
a solution that's not broken-by-design.

- Dave
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-22 Thread Austin, Alex
Read-modify-erase-write on per-sector basis?

 -Original Message-
 From: openocd-development-boun...@lists.berlios.de [mailto:openocd-
 development-boun...@lists.berlios.de] On Behalf Of David Brownell
 Sent: Tuesday, December 22, 2009 8:23 PM
 To: Øyvind Harboe
 Cc: openocd-development@lists.berlios.de
 Subject: Re: [Openocd-development] [patch/rfc] NOR FLASH: only
 erase/unlock whole sectors
 
 On Tuesday 22 December 2009, Øyvind Harboe wrote:
  This is a bit worse than I thought.
 
  This breaks flash write_image as well.
 
 That was *this* problem report, in fact...
 
 
  Flash write_image will first erase all sectors in address ranges of
  all segments to be written, then it will write all segments.
 
 In short, kind of broken-by-design.  When other portions
 of one of those sector should not have been erased -- maybe
 the portion being written was still erased! -- it's trashing
 stuff that should have been left alone.
 
 Example:  I might have two images to write, which don't
 overlap.  Erase, write one, write the next.  Whoops ... it
 needlessly erased part of the first image in order to write
 the second one, because they used different parts of a sector.
 
 
 I'm going to sort through the messages here and come up with
 a solution that's not broken-by-design.
 
 - Dave
 ___
 Openocd-development mailing list
 Openocd-development@lists.berlios.de
 https://lists.berlios.de/mailman/listinfo/openocd-development
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


[Openocd-development] [patch/rfc] NOR FLASH: only erase/unlock whole sectors

2009-12-17 Thread David Brownell
Much to my surprise, I observed a flash erase_address ...
command erasing data which I didn't say should be erased.

The issue turns out to be generic NOR flash code which was
silently, and rather dangerously, morphing partial-sector
references into unrequested whole-sector ones.

This patch removes that low-level morphing.  If desired, it
can and should be done in higher level code.
---
I suspect there's one GDB server path which should do that
morphing, but it looks like a later one already handles it.
Other than that, does anyone object to fixing this?

 NEWS |2 ++
 src/flash/nor/core.c |   44 
 2 files changed, 38 insertions(+), 8 deletions(-)

--- a/NEWS
+++ b/NEWS
@@ -39,6 +39,8 @@ Flash Layer:
- bank_name: reference the bank with its defined name
- driver_name[.N]: reference the driver's Nth bank
New 'nand verify' command to check bank against an image file.
+   The flash erase_address command now rejects partial sectors;
+   previously it would silently erase data you did not request.
 
 Board, Target, and Interface Configuration Scripts:
ARM9
--- a/src/flash/nor/core.c
+++ b/src/flash/nor/core.c
@@ -279,11 +279,13 @@ int default_flash_blank_check(struct fla
 
return ERROR_OK;
 }
+
 /* erase given flash region, selects proper bank according to target and 
address */
 static int flash_iterate_address_range(struct target *target, uint32_t addr, 
uint32_t length,
int (*callback)(struct flash_bank *bank, int first, int last))
 {
struct flash_bank *c;
+   uint32_t last_addr = addr + length; /* first address AFTER end */
int first = -1;
int last = -1;
int i;
@@ -306,26 +308,52 @@ static int flash_iterate_address_range(s
return callback(c, 0, c-num_sectors - 1);
}
 
-   /* check whether it fits */
+   /* check whether it all fits in this bank */
if (addr + length - 1  c-base + c-size - 1)
return ERROR_FLASH_DST_BREAKS_ALIGNMENT;
 
+   /** @todo: handle erasures that cross into adjacent banks */
+
addr -= c-base;
 
for (i = 0; i  c-num_sectors; i++)
{
-   /* check whether sector overlaps with the given range and is 
not yet erased */
-   if (addr  c-sectors[i].offset + c-sectors[i].size  addr + 
length  c-sectors[i].offset  c-sectors[i].is_erased != 1) {
-   /* if first is not set yet then this is the first 
sector */
-   if (first == -1)
+   struct flash_sector *f = c-sectors + i;
+
+   /* start only on a sector boundary */
+   if (first  0) {
+   /* is this the first sector? */
+   if (addr == f-offset)
first = i;
-   last = i; /* and it is the last one so far in any case 
*/
+   else if (addr  f-offset)
+   break;
+   }
+
+   /* is this (also?) the last sector? */
+   if (last_addr == f-offset + f-size) {
+   last = i;
+   break;
}
+
+   /* MUST finish on a sector boundary */
+   if (last_addr = f-offset)
+   break;
}
 
-   if (first == -1 || last == -1)
-   return ERROR_OK;
+   /* invalid start or end address? */
+   if (first == -1 || last == -1) {
+   LOG_ERROR(address range 0x%8.8x .. 0x%8.8x 
+   is not sector-aligned,
+   (unsigned) c-base + addr,
+   (unsigned) last_addr - 1);
+   return ERROR_FLASH_DST_BREAKS_ALIGNMENT;
+   }
 
+   /* The NOR driver may trim this range down, based on
+* whether or not a given sector is already erased.
+*
+* REVISIT should *we* trim it... ?
+*/
return callback(c, first, last);
 }
 
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development