NOR: add optional "flash erase_address" sector padding

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>

4 files changed, 117 insertions, 26 deletions

diff --git a/src/flash/nor/core.c b/src/flash/nor/core.c
index 9083ed15..aedaa866 100644
--- a/src/flash/nor/core.c
+++ b/src/flash/nor/core.c
@@ -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);
 			}
 		}
 
diff --git a/src/flash/nor/core.h b/src/flash/nor/core.h
index 36e163df..b164b8dd 100644
--- a/src/flash/nor/core.h
+++ b/src/flash/nor/core.h
@@ -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
diff --git a/src/flash/nor/tcl.c b/src/flash/nor/tcl.c
index b7e80df2..cf40a81a 100644
--- a/src/flash/nor/tcl.c
+++ b/src/flash/nor/tcl.c
@@ -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",
diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c
index 8018e6f9..4191cc2a 100644
--- a/src/server/gdb_server.c
+++ b/src/server/gdb_server.c
@@ -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)