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 a8b2b44b492ead6a27849d7a4cab7aeaee44c11f..ce4896fb8b0da345cb401d2497823307288384ec 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 0eb40b13cece40b7192c2d081fbdb5c4201e275e..3e0b5db7c8c43e94f0fd54e1c67ff7118e24211d 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 9083ed15ef92a95dda02aa4f0d9362922c2b4905..aedaa8661c66ecfa69e4f4dcb69f9e254a63c9a3 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 36e163dfc085d69bd157f8c65539d2bb8ef1467c..b164b8dd03384769a46561bcc29f68c89142a4dd 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 b7e80df25d9802b0e93f44cec079208172744728..cf40a81a75e4b49145d3d7f44be6eb02af9a6b1c 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 8018e6f94b42d1f9c7cc5c15fc7db7f3190c4c57..4191cc2a7c664ac699fb353b4d3ac219d0b0e5c3 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)

Linking to existing account procedure

If you already have an account and want to add another login method you MUST first sign in with your existing account and then change URL to read https://review.openocd.org/login/?link to get to this page again but this time it'll work for linking. Thank you.

SSH host keys fingerprints

1024 SHA256:YKx8b7u5ZWdcbp7/4AeXNaqElP49m6QrwfXaqQGJAOk gerrit-code-review@openocd.zylin.com (DSA)
384 SHA256:jHIbSQa4REvwCFG4cq5LBlBLxmxSqelQPem/EXIrxjk gerrit-code-review@openocd.org (ECDSA)
521 SHA256:UAOPYkU9Fjtcao0Ul/Rrlnj/OsQvt+pgdYSZ4jOYdgs gerrit-code-review@openocd.org (ECDSA)
256 SHA256:A13M5QlnozFOvTllybRZH6vm7iSt0XLxbA48yfc2yfY gerrit-code-review@openocd.org (ECDSA)
256 SHA256:spYMBqEYoAOtK7yZBrcwE8ZpYt6b68Cfh9yEVetvbXg gerrit-code-review@openocd.org (ED25519)
+--[ED25519 256]--+
|=..              |
|+o..   .         |
|*.o   . .        |
|+B . . .         |
|Bo. = o S        |
|Oo.+ + =         |
|oB=.* = . o      |
| =+=.+   + E     |
|. .=o   . o      |
+----[SHA256]-----+
2048 SHA256:0Onrb7/PHjpo6iVZ7xQX2riKN83FJ3KGU0TvI0TaFG4 gerrit-code-review@openocd.zylin.com (RSA)