NOR: add optional "flash erase_address" sector padding
authorDavid Brownell <dbrownell@users.sourceforge.net>
Thu, 14 Jan 2010 07:33:25 +0000 (23:33 -0800)
committerDavid Brownell <dbrownell@users.sourceforge.net>
Thu, 14 Jan 2010 07:33:25 +0000 (23:33 -0800)
Add a NOR flash mechanism where erase_address ranges can be padded
out to sector boundaries, triggering a diagnostic:

  > flash erase_address 0x0001f980 16
  address range 0x0001f980 .. 0x0001f98f is not sector-aligned
  Command handler execution failed
  in procedure 'flash' called at file "command.c", line 647
  called at file "command.c", line 361
  >

  > flash erase_address pad 0x0001f980 16
  Adding extra erase range, 0x0001f800 to 0x0001f97f
  Adding extra erase range, 0x0001f990 to 0x0001fbff
  erased address 0x0001f980 (length 16) in 0.095975s (0.163 kb/s)
  >

This addresses what would otherwise be something of a functional
regression.  An earlier version of the interface had a dangerous
problem:  it would silently erase data outside the range it was
told to erase.  Fixing that bug turned up some folk who relied on
that unsafe behavior.  (The classic problem with interface bugs!)
Now they can get that behavior again.  If they really need it,
just specify "pad".

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
NEWS
doc/openocd.texi
src/flash/nor/core.c
src/flash/nor/core.h
src/flash/nor/tcl.c
src/server/gdb_server.c

diff --git a/NEWS b/NEWS
index a8b2b44..ce4896f 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -50,7 +50,9 @@ Flash Layer:
                - <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 extra data.
+               previously it would silently erase extra data.  If you
+               want to erase the rest of the first and/or last sectors
+               instead of failing, you must pass an explicit "pad" flag.
        New at91sam9 NAND controller driver.
 
 Board, Target, and Interface Configuration Scripts:
index 0eb40b1..3e0b5db 100644 (file)
@@ -3841,8 +3841,12 @@ specifies "to the end of the flash bank".
 The @var{num} parameter is a value shown by @command{flash banks}.
 @end deffn
 
-@deffn Command {flash erase_address} address length
+@deffn Command {flash erase_address} [@option{pad}] address length
 Erase sectors starting at @var{address} for @var{length} bytes.
+Unless @option{pad} is specified, @math{address} must begin a
+flash sector, and @math{address + length - 1} must end a sector.
+Specifying @option{pad} erases extra data at the beginning and/or
+end of the specified region, as needed to erase only full sectors.
 The flash bank to use is inferred from the @var{address}, and
 the specified length must stay within that bank.
 As a special case, when @var{length} is zero and @var{address} is
index 9083ed1..aedaa86 100644 (file)
@@ -287,9 +287,22 @@ int default_flash_blank_check(struct flash_bank *bank)
        return ERROR_OK;
 }
 
-/* erase given flash region, selects proper bank according to target and address */
+/* Manipulate given flash region, selecting the bank according to target
+ * and address.  Maps an address range to a set of sectors, and issues
+ * the callback() on that set ... e.g. to erase or unprotect its members.
+ *
+ * (Note a current bad assumption:  that protection operates on the same
+ * size sectors as erase operations use.)
+ *
+ * The "pad_reason" parameter is a kind of boolean:  when it's NULL, the
+ * range must fit those sectors exactly.  This is clearly safe; it can't
+ * erase data which the caller said to leave alone, for example.  If it's
+ * non-NULL, rather than failing, extra data in the first and/or last
+ * sectors will be added to the range, and that reason string is used when
+ * warning about those additions.
+ */
 static int flash_iterate_address_range(struct target *target,
-               uint32_t addr, uint32_t length,
+               char *pad_reason, uint32_t addr, uint32_t length,
                int (*callback)(struct flash_bank *bank, int first, int last))
 {
        struct flash_bank *c;
@@ -328,18 +341,53 @@ static int flash_iterate_address_range(struct target *target,
        for (i = 0; i < c->num_sectors; i++)
        {
                struct flash_sector *f = c->sectors + i;
+               uint32_t end = f->offset + f->size;
 
                /* start only on a sector boundary */
                if (first < 0) {
+                       /* scanned past the first sector? */
+                       if (addr < f->offset)
+                               break;
+
                        /* is this the first sector? */
                        if (addr == f->offset)
                                first = i;
-                       else if (addr < f->offset)
-                               break;
+
+                       /* Does this need head-padding?  If so, pad and warn;
+                        * or else force an error.
+                        *
+                        * Such padding can make trouble, since *WE* can't
+                        * ever know if that data was in use.  The warning
+                        * should help users sort out messes later.
+                        */
+                       else if (addr < end && pad_reason) {
+                               /* FIXME say how many bytes (e.g. 80 KB) */
+                               LOG_WARNING("Adding extra %s range, "
+                                               "%#8.8x to %#8.8x",
+                                       pad_reason,
+                                       (unsigned) f->offset,
+                                       (unsigned) addr - 1);
+                               first = i;
+                       } else
+                               continue;
                }
 
                /* is this (also?) the last sector? */
-               if (last_addr == f->offset + f->size) {
+               if (last_addr == end) {
+                       last = i;
+                       break;
+               }
+
+               /* Does this need tail-padding?  If so, pad and warn;
+                * or else force an error.
+                */
+               if (last_addr < end && pad_reason) {
+                       /* FIXME say how many bytes (e.g. 80 KB) */
+                       LOG_WARNING("Adding extra %s range, "
+                                       "%#8.8x to %#8.8x",
+                               pad_reason,
+                               (unsigned) last_addr,
+                               (unsigned) end - 1);
                        last = i;
                        break;
                }
@@ -358,18 +406,17 @@ static int flash_iterate_address_range(struct target *target,
                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... ?
+       /* The NOR driver may trim this range down, based on what
+        * sectors are already erased/unprotected.  GDB currently
+        * blocks such optimizations.
         */
        return callback(c, first, last);
 }
 
 int flash_erase_address_range(struct target *target,
-               uint32_t addr, uint32_t length)
+               bool pad, uint32_t addr, uint32_t length)
 {
-       return flash_iterate_address_range(target,
+       return flash_iterate_address_range(target, pad ? "erase" : NULL,
                        addr, length, &flash_driver_erase);
 }
 
@@ -380,7 +427,11 @@ static int flash_driver_unprotect(struct flash_bank *bank, int first, int last)
 
 static int flash_unlock_address_range(struct target *target, uint32_t addr, uint32_t length)
 {
-       return flash_iterate_address_range(target,
+       /* By default, pad to sector boundaries ... the real issue here
+        * is that our (only) caller *permanently* removes protection,
+        * and doesn't restore it.
+        */
+       return flash_iterate_address_range(target, "unprotect",
                        addr, length, &flash_driver_unprotect);
 }
 
@@ -394,6 +445,12 @@ int flash_write_unlock(struct target *target, struct image *image,
        struct flash_bank *c;
        int *padding;
 
+       /* REVISIT do_pad should perhaps just be another parameter.
+        * GDB wouldn't ever need it, since it erases separately.
+        * But "flash write_image" commands might want that option.
+        */
+       bool do_pad = false;
+
        section = 0;
        section_offset = 0;
 
@@ -470,7 +527,8 @@ int flash_write_unlock(struct target *target, struct image *image,
                         * 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
+                       /* 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);
                        if ((run_address + run_size + pad_bytes) > (c->base + c->size))
@@ -560,7 +618,8 @@ int flash_write_unlock(struct target *target, struct image *image,
                        if (erase)
                        {
                                /* calculate and erase sectors */
-                               retval = flash_erase_address_range(target, run_address, run_size);
+                               retval = flash_erase_address_range(target,
+                                               do_pad, run_address, run_size);
                        }
                }
 
index 36e163d..b164b8d 100644 (file)
@@ -102,10 +102,14 @@ int flash_init_drivers(struct command_context *cmd_ctx);
 
 /**
  * Erases @a length bytes in the @a target flash, starting at @a addr.
+ * The range @a addr to @a addr + @a length - 1 must be strictly
+ * sector aligned, unless @a pad is true.  Setting @a pad true extends
+ * the range, at beginning and/or end, if needed for sector alignment.
  * @returns ERROR_OK if successful; otherwise, an error code.
  */
 int flash_erase_address_range(struct target *target,
-               uint32_t addr, uint32_t length);
+               bool pad, uint32_t addr, uint32_t length);
+
 /**
  * Writes @a image into the @a target flash.  The @a written parameter
  * will contain the
index b7e80df..cf40a81 100644 (file)
@@ -203,14 +203,29 @@ COMMAND_HANDLER(handle_flash_erase_address_command)
        int retval;
        int address;
        int length;
-
+       bool do_pad = false;
        struct target *target = get_current_target(CMD_CTX);
 
-       if (CMD_ARGC != 2)
+       switch (CMD_ARGC) {
+       case 3:
+               /* Optionally pad out the address range to block/sector
+                * boundaries.  We can't know if there's data in that part
+                * of the flash; only do padding if we're told to.
+                */
+               if (strcmp("pad", CMD_ARGV[0]) != 0)
+                       return ERROR_COMMAND_SYNTAX_ERROR;
+               do_pad = true;
+               CMD_ARGC--;
+               CMD_ARGV++;
+               /* FALL THROUGH */
+       case 2:
+               COMMAND_PARSE_NUMBER(int, CMD_ARGV[0], address);
+               COMMAND_PARSE_NUMBER(int, CMD_ARGV[1], length);
+               break;
+       default:
                return ERROR_COMMAND_SYNTAX_ERROR;
+       }
 
-       COMMAND_PARSE_NUMBER(int, CMD_ARGV[0], address);
-       COMMAND_PARSE_NUMBER(int, CMD_ARGV[1], length);
        if (length <= 0)
        {
                command_print(CMD_CTX, "Length must be >0");
@@ -229,7 +244,7 @@ COMMAND_HANDLER(handle_flash_erase_address_command)
        struct duration bench;
        duration_start(&bench);
 
-       retval = flash_erase_address_range(target, address, length);
+       retval = flash_erase_address_range(target, do_pad, address, length);
 
        if ((ERROR_OK == retval) && (duration_measure(&bench) == ERROR_OK))
        {
@@ -698,9 +713,12 @@ static const struct command_registration flash_exec_command_handlers[] = {
                .name = "erase_address",
                .handler = handle_flash_erase_address_command,
                .mode = COMMAND_EXEC,
-               .usage = "address length",
-               .help = "Erase flash blocks starting at address "
-                       "and continuing for length bytes.",
+               .usage = "['pad'] address length",
+               .help = "Erase flash sectors starting at address and "
+                       "continuing for length bytes.  If 'pad' is specified, "
+                       "data outside that range may also be erased: the start "
+                       "address may be decreased, and length increased, so "
+                       "that all of the first and last sectors are erased.",
        },
        {
                .name = "fillw",
index 8018e6f..4191cc2 100644 (file)
@@ -1928,9 +1928,19 @@ int gdb_v_packet(struct connection *connection, struct target *target, char *pac
                flash_set_dirty();
 
                /* perform any target specific operations before the erase */
-               target_call_event_callbacks(gdb_service->target, TARGET_EVENT_GDB_FLASH_ERASE_START);
-               result = flash_erase_address_range(gdb_service->target, addr, length);
-               target_call_event_callbacks(gdb_service->target, TARGET_EVENT_GDB_FLASH_ERASE_END);
+               target_call_event_callbacks(gdb_service->target,
+                               TARGET_EVENT_GDB_FLASH_ERASE_START);
+
+               /* vFlashErase:addr,length messages require region start and
+                * end to be "block" aligned ... if padding is ever needed,
+                * GDB will have become dangerously confused.
+                */
+               result = flash_erase_address_range(gdb_service->target,
+                               false, addr, length);
+
+               /* perform any target specific operations after the erase */
+               target_call_event_callbacks(gdb_service->target,
+                               TARGET_EVENT_GDB_FLASH_ERASE_END);
 
                /* perform erase */
                if (result != ERROR_OK)