NOR: make flash_write_unlock() pad to sector end

Resolve a regression when using newish automagic "write_image"
modes, by always padding to the end of affected sectors.

Also document some issues associated with those automagic options,
in the User's Guide and also some related code comments.

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 doc updates).

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

2 files changed, 65 insertions, 4 deletions

diff --git a/doc/openocd.texi b/doc/openocd.texi
index 1c20716c..154ecbca 100644
--- a/doc/openocd.texi
+++ b/doc/openocd.texi
@@ -3857,8 +3857,29 @@ explicitly as @option{bin} (binary), @option{ihex} (Intel hex),
 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 are 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
diff --git a/src/flash/nor/core.c b/src/flash/nor/core.c
index fe5372b6..5eb51cd2 100644
--- a/src/flash/nor/core.c
+++ b/src/flash/nor/core.c
@@ -439,9 +439,26 @@ int flash_write_unlock(struct target *target, struct image *image,
 		{
 			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 target *target, struct image *image,
 			padding[section_last] = pad_bytes;
 			run_size += image->sections[++section_last].size;
 			run_size += pad_bytes;
-			padding[section_last] = 0;
 
 			LOG_INFO("Padding image section %d with %d bytes", section_last-1, pad_bytes);
 		}
@@ -458,11 +474,35 @@ int flash_write_unlock(struct target *target, struct image *image,
 		/* fit the run into bank constraints */
 		if (run_address + run_size - 1 > c->base + c->size - 1)
 		{
+			/* REVISIT isn't this superfluous, given the while()
+			 * loop conditions above??
+			 */
 			LOG_WARNING("writing %d bytes only - as image section is %d bytes and bank is only %d bytes", \
 				    (int)(c->base + c->size - run_address), (int)(run_size), (int)(c->size));
 			run_size = c->base + c->size - run_address;
 		}
 
+		/* If we're applying any sector automagic, then pad this
+		 * (maybe-combined) segment to the end of its last sector.
+		 */
+		if (unlock || erase) {
+			int sector;
+			uint32_t offset_start = run_address - c->base;
+			uint32_t offset_end = offset_start + run_size;
+			uint32_t end = offset_end, delta;
+
+			for (sector = 0; sector < c->num_sectors; sector++) {
+				end = c->sectors[sector].offset
+						+ c->sectors[sector].size;
+				if (offset_end <= end)
+					break;
+			}
+
+			delta = end - offset_end;
+			padding[section_last] += delta;
+			run_size += delta;
+		}
+
 		/* allocate buffer */
 		buffer = malloc(run_size);
 		buffer_size = 0;