Giving to -i option some real use

Message ID 571A1184.50604@inti.gob.ar
State New
Headers show

Commit Message

Salvador Eduardo Tropea April 22, 2016, 11:56 a.m.
Hi Nico:

El 22/04/16 a las 08:35, Nico Huber escribió:
> Hi All,
>
> I'm interested too in this feature. My use case is internal flashing on
> modern Intel systems where parts of the flashchip are read protected.
> This is a priority for me during the next week, but I believe my time is
> better spent reviewing someone else' patch than reinventing my own.
>
> There are also other patches floating around that look like tackling the
> same problem. I'll look at these today:
>
> https://www.flashrom.org/pipermail/flashrom/2013-September/011599.html
> along with a branch on github:
> https://github.com/stefanct/flashrom/tree/layout
>
> https://www.flashrom.org/pipermail/flashrom/2014-October/012967.html
>
> On 21.04.2016 22:36, Salvador Eduardo Tropea wrote:
>> What about reading the whole erase regions affected by each interval?
>> (like David points out)
> Another option to enforce the whole erase region is read, is to simply
> constrain the used block erasers to those that match the layout. This
> would simplify things a lot. And, IMO, you can have a layout with erase
> block aligned regions in most use cases.
You are right, forcing the sections to be aligned makes a lot of sense 
and simplifies things a lot.
But this is an important restriction, and I think that some people will 
need to define unaligned regions.
So I think the software must support unaligned regions, even when the 
user should try to always align them.
In my case I have a 4 MiB flash with 4 kiB pages. The FPGA only needs 
32300 bytes for configuration. The rest could be used to store data. One 
really good use could be that a softcore (a CPU implemented in the FPGA) 
uses it as ROM for code execution. In this case reserving a group of 
pages makes much more sense than just starting at arbitrary addresses. 
But again: this is the most common use, but I think flashrom should be 
able to handle cases where the regions aren't aligned.
I didn't post my whole patch because I want to refine it, in fact this 
discussion already introduced a very important change ;-) Additionally, 
I have to separate 3 different features that I'm introducing ;-)
I'm attaching the output of "svn -diff", just in case you want to take a 
look at it. Is not intended to be merged because, as I said, it contains 
3 different features.

Regards, SET

Patch

Index: cli_classic.c
===================================================================
--- cli_classic.c	(revisión: 1955)
+++ cli_classic.c	(copia de trabajo)
@@ -61,7 +61,9 @@ 
 #if CONFIG_PRINT_WIKI == 1
 	       " -z | --list-supported-wiki         print supported devices in wiki syntax\n"
 #endif
-	       " -p | --programmer <name>[:<param>] specify the programmer device. One of\n");
+	       " -p | --programmer <name>[:<param>] specify the programmer device. One of\n"
+	       " -s | --show-progress               show operation progress\n"
+               " -A | --no-read-all                 don't read the whole memory\n");
 	list_programmers_linebreak(4, 80, 0);
 	printf(".\n\nYou can specify one of -h, -R, -L, "
 #if CONFIG_PRINT_WIKI == 1
@@ -105,25 +107,28 @@ 
 	int dont_verify_it = 0, list_supported = 0, operation_specified = 0;
 	enum programmer prog = PROGRAMMER_INVALID;
 	int ret = 0;
+	int read_all_first = 1, included_args = 0;
 
-	static const char optstring[] = "r:Rw:v:nVEfc:l:i:p:Lzho:";
+	static const char optstring[] = "Ac:Efhi:l:Lno:p:r:Rsv:Vw:z";
 	static const struct option long_options[] = {
-		{"read",		1, NULL, 'r'},
-		{"write",		1, NULL, 'w'},
+		{"chip",		1, NULL, 'c'},
 		{"erase",		0, NULL, 'E'},
-		{"verify",		1, NULL, 'v'},
-		{"noverify",		0, NULL, 'n'},
-		{"chip",		1, NULL, 'c'},
-		{"verbose",		0, NULL, 'V'},
 		{"force",		0, NULL, 'f'},
+		{"help",		0, NULL, 'h'},
+		{"image",		1, NULL, 'i'},
 		{"layout",		1, NULL, 'l'},
-		{"image",		1, NULL, 'i'},
 		{"list-supported",	0, NULL, 'L'},
 		{"list-supported-wiki",	0, NULL, 'z'},
+		{"no-read-all",		0, NULL, 'A'},
+		{"noverify",		0, NULL, 'n'},
+		{"output",		1, NULL, 'o'},
 		{"programmer",		1, NULL, 'p'},
-		{"help",		0, NULL, 'h'},
+		{"read",		1, NULL, 'r'},
+		{"show-progress",	0, NULL, 's'},
+		{"verbose",		0, NULL, 'V'},
+		{"verify",		1, NULL, 'v'},
 		{"version",		0, NULL, 'R'},
-		{"output",		1, NULL, 'o'},
+		{"write",		1, NULL, 'w'},
 		{NULL,			0, NULL, 0},
 	};
 
@@ -206,6 +211,12 @@ 
 		case 'f':
 			force = 1;
 			break;
+		case 's':
+			show_progress = 1;
+			break;
+		case 'A':
+			read_all_first = 0;
+			break;
 		case 'l':
 			if (layoutfile) {
 				fprintf(stderr, "Error: --layout specified "
@@ -220,6 +231,7 @@ 
 				free(tempstr);
 				cli_classic_abort_usage();
 			}
+			included_args = 1;
 			break;
 		case 'L':
 			if (++operation_specified > 1) {
@@ -326,6 +338,11 @@ 
 		cli_classic_abort_usage();
 	}
 
+	if (!read_all_first && !included_args) {
+		fprintf(stderr, "Error: You must indicate at least one image when using --no-read-all.\n");
+		cli_classic_abort_usage();
+	}
+
 	if ((read_it | write_it | verify_it) && check_filename(filename, "image")) {
 		cli_classic_abort_usage();
 	}
@@ -542,7 +559,7 @@ 
 	 * Give the chip time to settle.
 	 */
 	programmer_delay(100000);
-	ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it);
+	ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it, read_all_first);
 
 	unmap_flash(fill_flash);
 out_shutdown:
Index: cli_output.c
===================================================================
--- cli_output.c	(revisión: 1955)
+++ cli_output.c	(copia de trabajo)
@@ -27,6 +27,8 @@ 
 
 int verbose_screen = MSG_INFO;
 int verbose_logfile = MSG_DEBUG2;
+/* Show progress during operations */
+int show_progress = 0;
 
 #ifndef STANDALONE
 static FILE *logfile = NULL;
Index: flash.h
===================================================================
--- flash.h	(revisión: 1955)
+++ flash.h	(copia de trabajo)
@@ -222,6 +222,7 @@ 
 	uintptr_t physical_registers;
 	chipaddr virtual_registers;
 	struct registered_master *mst;
+	void (*progress) (struct flashctx *flash, double done);
 };
 
 /* Timing used in probe routines. ZERO is -2 to differentiate between an unset
@@ -281,9 +282,11 @@ 
 void print_banner(void);
 void list_programmers_linebreak(int startcol, int cols, int paren);
 int selfcheck(void);
-int doit(struct flashctx *flash, int force, const char *filename, int read_it, int write_it, int erase_it, int verify_it);
+int doit(struct flashctx *flash, int force, const char *filename, int read_it, int write_it, int erase_it,
+	int verify_it, int read_all_first);
 int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename);
 int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *filename);
+int find_erase_function(struct flashctx *flash);
 
 /* Something happened that shouldn't happen, but we can go on. */
 #define ERROR_NONFATAL 0x100
@@ -305,6 +308,7 @@ 
 /* cli_output.c */
 extern int verbose_screen;
 extern int verbose_logfile;
+extern int show_progress;
 #ifndef STANDALONE
 int open_logfile(const char * const filename);
 int close_logfile(void);
@@ -349,7 +353,8 @@ 
 int process_include_args(void);
 int read_romlayout(const char *name);
 int normalize_romentries(const struct flashctx *flash);
-int build_new_image(struct flashctx *flash, bool oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents);
+int read_cur_content(struct flashctx *flash, uint8_t *oldcontents, const uint8_t *newcontents);
+void modify_new_image_from_old(struct flashctx *flash, const uint8_t *oldcontents, uint8_t *newcontents);
 void layout_cleanup(void);
 
 /* spi.c */
Index: flashrom.c
===================================================================
--- flashrom.c	(revisión: 1955)
+++ flashrom.c	(copia de trabajo)
@@ -454,6 +454,41 @@ 
 	return 0;
 }
 
+static const char *progress_message = NULL;
+
+static
+void do_show_progress(struct flashctx *flash, double done)
+{
+	if (show_progress && progress_message)
+		msg_cinfo("\r%s... %d %%", progress_message, (int)(done+0.5));
+}
+
+static
+void do_show_progress_done(void)
+{
+	msg_cinfo("\r%s... done. \n", progress_message);
+}
+
+static
+void do_show_progress_failed(void)
+{
+	msg_cinfo("\r%s... FAILED.\n", progress_message);
+}
+
+static
+void do_show_progress_verified(void)
+{
+	msg_cinfo("\r%s... VERIFIED.\n", progress_message);
+}
+
+static
+void set_progress_message(struct flashctx *flash, const char *msg)
+{
+	progress_message = msg;
+	msg_cinfo("%s... ", msg);
+	flash->progress = do_show_progress;
+}
+
 int programmer_init(enum programmer prog, const char *param)
 {
 	int ret;
@@ -677,9 +712,11 @@ 
 	for (i = 0; i < len; i++) {
 		if (wantbuf[i] != havebuf[i]) {
 			/* Only print the first failure. */
-			if (!failcount++)
-				msg_cerr("FAILED at 0x%08x! Expected=0x%02x, Found=0x%02x,",
+			if (!failcount++) {
+				do_show_progress_failed();
+				msg_cerr("FAILED at 0x%08x! Expected=0x%02x, Found=0x%02x, ",
 					 start + i, wantbuf[i], havebuf[i]);
+			}
 		}
 	}
 	if (failcount) {
@@ -1369,7 +1406,7 @@ 
 	unsigned char *buf = calloc(size, sizeof(char));
 	int ret = 0;
 
-	msg_cinfo("Reading flash... ");
+	set_progress_message(flash, "Reading flash");
 	if (!buf) {
 		msg_gerr("Memory allocation failed!\n");
 		msg_cinfo("FAILED.\n");
@@ -1389,7 +1426,10 @@ 
 	ret = write_buf_to_file(buf, size, filename);
 out_free:
 	free(buf);
-	msg_cinfo("%s.\n", ret ? "FAILED" : "done");
+	if (ret)
+		do_show_progress_failed();
+	else
+		do_show_progress_done();
 	return ret;
 }
 
@@ -1526,8 +1566,13 @@ 
 	int i, j;
 	unsigned int start = 0;
 	unsigned int len;
+	unsigned int total_blocks = 0;
+	unsigned int cur_block = 0;
 	struct block_eraser eraser = flash->chip->block_erasers[erasefunction];
 
+	/* determine the total number of iterations */
+	for (i = 0; i < NUM_ERASEREGIONS; i++)
+		total_blocks += eraser.eraseblocks[i].count;
 	for (i = 0; i < NUM_ERASEREGIONS; i++) {
 		/* count==0 for all automatically initialized array
 		 * members so the loop below won't be executed for them.
@@ -1537,16 +1582,17 @@ 
 			/* Print this for every block except the first one. */
 			if (i || j)
 				msg_cdbg(", ");
-			msg_cdbg("0x%06x-0x%06x", start,
-				     start + len - 1);
-			if (do_something(flash, start, len, param1, param2,
-					 eraser.block_erase)) {
+			msg_cdbg("0x%06x-0x%06x", start, start + len - 1);
+			cur_block++;
+			do_show_progress(flash, (double)cur_block/total_blocks*100.0);
+			if (do_something(flash, start, len, param1, param2, eraser.block_erase)) {
+				do_show_progress_failed();
 				return 1;
 			}
 			start += len;
 		}
 	}
-	msg_cdbg("\n");
+	do_show_progress_done();
 	return 0;
 }
 
@@ -1575,6 +1621,27 @@ 
 	return 0;
 }
 
+/* Look for the first valid erase function. Returns -1 if none.
+ */
+int find_erase_function(struct flashctx *flash)
+{
+	int k;
+
+	msg_cdbg("\nLooking for an erase function.\n");
+	for (k = 0; k < NUM_ERASEFUNCTIONS; k++) {
+		if (k != 0)
+			msg_cdbg("Looking for another erase function.\n");
+		msg_cdbg("Trying erase function %i... ", k);
+		if (!check_block_eraser(flash, k, 1)) {
+			msg_cdbg("OK\n");
+			return k;
+		}
+	msg_cdbg("INVALID\n");
+	}
+	msg_cdbg("No usable erase functions left.\n");
+	return -1;
+}
+
 int erase_and_write_flash(struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents)
 {
 	int k, ret = 1;
@@ -1582,7 +1649,10 @@ 
 	unsigned long size = flash->chip->total_size * 1024;
 	unsigned int usable_erasefunctions = count_usable_erasers(flash);
 
-	msg_cinfo("Erasing and writing flash chip... ");
+	set_progress_message(flash, "Erasing and writing flash chip");
+	/* We will read the memory while erasing & writing, so we will show the progress
+	 * and we don't want messages from the partial reads. */
+	flash->progress = NULL;
 	curcontents = malloc(size);
 	if (!curcontents) {
 		msg_gerr("Out of memory!\n");
@@ -1593,9 +1663,9 @@ 
 
 	for (k = 0; k < NUM_ERASEFUNCTIONS; k++) {
 		if (k != 0)
-			msg_cinfo("Looking for another erase function.\n");
+			msg_cinfo("\nLooking for another erase function.\n");
 		if (!usable_erasefunctions) {
-			msg_cinfo("No usable erase functions left.\n");
+			msg_cinfo("\nNo usable erase functions left.\n");
 			break;
 		}
 		msg_cdbg("Trying erase function %i... ", k);
@@ -1616,8 +1686,9 @@ 
 		/* Reading the whole chip may take a while, inform the user even
 		 * in non-verbose mode.
 		 */
-		msg_cinfo("Reading current flash chip contents... ");
+		set_progress_message(flash, "Reading current flash chip contents");
 		if (flash->chip->read(flash, curcontents, 0, size)) {
+			do_show_progress_failed();
 			/* Now we are truly screwed. Read failed as well. */
 			msg_cerr("Can't read anymore! Aborting.\n");
 			/* We have no idea about the flash chip contents, so
@@ -1625,7 +1696,7 @@ 
 			 */
 			break;
 		}
-		msg_cinfo("done. ");
+		do_show_progress_done();
 	}
 	/* Free the scratchpad. */
 	free(curcontents);
@@ -1634,7 +1705,7 @@ 
 		msg_cerr("FAILED!\n");
 	} else {
 		if (all_skipped)
-			msg_cinfo("\nWarning: Chip content is identical to the requested image.\n");
+			msg_cinfo("Warning: Chip content is identical to the requested image.\n");
 		msg_cinfo("Erase/write done.\n");
 	}
 	return ret;
@@ -1977,13 +2048,12 @@ 
  * Besides that, the function itself is a textbook example of abysmal code flow.
  */
 int doit(struct flashctx *flash, int force, const char *filename, int read_it,
-	 int write_it, int erase_it, int verify_it)
+	 int write_it, int erase_it, int verify_it, int read_all_first)
 {
 	uint8_t *oldcontents;
 	uint8_t *newcontents;
 	int ret = 0;
 	unsigned long size = flash->chip->total_size * 1024;
-	int read_all_first = 1; /* FIXME: Make this configurable. */
 
 	if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) {
 		msg_cerr("Aborting.\n");
@@ -2065,23 +2135,26 @@ 
 	 * preserved, but in that case we might perform unneeded erase which
 	 * takes time as well.
 	 */
+	set_progress_message(flash, "Reading old flash chip contents");
 	if (read_all_first) {
-		msg_cinfo("Reading old flash chip contents... ");
 		if (flash->chip->read(flash, oldcontents, 0, size)) {
 			ret = 1;
-			msg_cinfo("FAILED.\n");
+			do_show_progress_failed();
 			goto out;
 		}
+		/* We read all, we just need to filter out the user indicated images. */
+		modify_new_image_from_old(flash, oldcontents, newcontents);
+	} else {
+		/* Build a new image taking the given layout into account. */
+		if (read_cur_content(flash, oldcontents, newcontents)) {
+			do_show_progress_failed();
+			msg_gerr("Could not prepare the data to be written, aborting.\n");
+			ret = 1;
+			goto out;
+		}
 	}
-	msg_cinfo("done.\n");
+	do_show_progress_done();
 
-	/* Build a new image taking the given layout into account. */
-	if (build_new_image(flash, read_all_first, oldcontents, newcontents)) {
-		msg_gerr("Could not prepare the data to be written, aborting.\n");
-		ret = 1;
-		goto out;
-	}
-
 	// ////////////////////////////////////////////////////////////
 
 	if (write_it && erase_and_write_flash(flash, oldcontents, newcontents)) {
@@ -2088,9 +2161,9 @@ 
 		msg_cerr("Uh oh. Erase/write failed. ");
 		if (read_all_first) {
 			msg_cerr("Checking if anything has changed.\n");
-			msg_cinfo("Reading current flash chip contents... ");
+			set_progress_message(flash, "Reading old flash chip contents");
 			if (!flash->chip->read(flash, newcontents, 0, size)) {
-				msg_cinfo("done.\n");
+				do_show_progress_done();
 				if (!memcmp(oldcontents, newcontents, size)) {
 					nonfatal_help_message();
 					ret = 1;
@@ -2097,8 +2170,10 @@ 
 					goto out;
 				}
 				msg_cerr("Apparently at least some data has changed.\n");
-			} else
+			} else {
+				do_show_progress_failed();
 				msg_cerr("Can't even read anymore!\n");
+			}
 			emergency_help_message();
 			ret = 1;
 			goto out;
@@ -2111,12 +2186,20 @@ 
 
 	/* Verify only if we either did not try to write (verify operation) or actually changed something. */
 	if (verify_it && (!write_it || !all_skipped)) {
-		msg_cinfo("Verifying flash... ");
+		set_progress_message(flash, "Verifying flash");
 
 		if (write_it) {
 			/* Work around chips which need some time to calm down. */
 			programmer_delay(1000*1000);
-			ret = verify_range(flash, newcontents, 0, size);
+			if (read_all_first) {
+				/* Verify the whole memory. */
+				ret = verify_range(flash, newcontents, 0, size);
+			} else {
+				/* Read only the regions specified by the user. */
+				ret = read_cur_content(flash, oldcontents, newcontents);
+				if (!ret) /* Compare with what we wanted. */
+					ret = compare_range(newcontents, oldcontents, 0, size);
+			}
 			/* If we tried to write, and verification now fails, we
 			 * might have an emergency situation.
 			 */
@@ -2126,7 +2209,7 @@ 
 			ret = compare_range(newcontents, oldcontents, 0, size);
 		}
 		if (!ret)
-			msg_cinfo("VERIFIED.\n");
+			do_show_progress_verified();
 	}
 
 out:
@@ -2134,3 +2217,4 @@ 
 	free(newcontents);
 	return ret;
 }
+
Index: ft2232_spi.c
===================================================================
--- ft2232_spi.c	(revisión: 1955)
+++ ft2232_spi.c	(copia de trabajo)
@@ -102,6 +102,13 @@ 
 static uint8_t cs_bits = 0x08;
 static uint8_t pindir = 0x0b;
 static struct ftdi_context ftdic_context;
+/* SPI configuration memory for iCE40 FPGAs */
+static char ice40_mode = 0;
+/* iCE40 specific: verify that the FPGA asserted CDONE */
+static char check_cdone = 0;
+/* iCE40 memory commands */
+#define RELEASE_FROM_POWER_DOWN 0xAB
+#define DEEP_POWER_DOWN         0xB9
 
 static const char *get_ft2232_devicename(int ft2232_vid, int ft2232_type)
 {
@@ -168,6 +175,58 @@ 
 	.write_aai	= default_spi_write_aai,
 };
 
+/* Put the data bits in the shutdown state. */
+static int ice40_deinit(void *ftdic)
+{
+	unsigned char buf[4];
+	struct ftdi_context *f = (struct ftdi_context *)ftdic;
+
+	/* Put the memory to sleep, saves power */
+	msg_pdbg("Memory power down\n");
+	buf[0] = DEEP_POWER_DOWN;
+	if (ft2232_spi_send_command(NULL,1,0,buf,NULL))
+		return -1;
+
+	/* Disconnect the cable to avoid contention */
+	msg_pdbg("Set data bits, cable disable\n");
+	buf[0] = SET_BITS_LOW;
+	buf[1] = 0;
+	buf[2] = 0; /* All inputs => HiZ */
+	if (send_buf(f, buf, 3))
+		return -1;
+
+	/* Check if the CDONE line is asserted */
+	if (check_cdone) {
+		unsigned int count = 100;
+		unsigned char buf_read_cmd[2] = { GET_BITS_LOW, SEND_IMMEDIATE };
+		unsigned char buf_read_data;
+		msg_cinfo("Waiting for CDONE... ");
+		do {
+			msg_pdbg2("Reading I/O pins: ");
+			if (send_buf(f, buf_read_cmd, 2)) {
+				msg_perr("Error reading FTDI I/O status (send)\n");
+				return -1;
+			}
+			if (get_buf(f, &buf_read_data, 1)) {
+				msg_perr("Error reading FTDI I/O status (receive)\n");
+				return -1;
+			}
+			msg_pdbg2("0x%02X\n", buf_read_data);
+			if (buf_read_data & 0x40) /* IOL2 */
+				break;
+			usleep(10*1000);
+		} while (--count);
+		if (count)
+			msg_cinfo("done.\n");
+		else {
+			msg_cinfo("FAILED.\n");
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 /* Returns 0 upon success, a negative number upon errors. */
 int ft2232_spi_init(void)
 {
@@ -273,6 +332,32 @@ 
 		} else if (!strcasecmp(arg, "google-servo-v2-legacy")) {
 			ft2232_vid = GOOGLE_VID;
 			ft2232_type = GOOGLE_SERVO_V2_PID0;
+		} else if (!strcasecmp(arg, "ice40")) {
+			/* HW-USBN-2B or generic FTDI cable */
+			char *arg2;
+			ft2232_type = FTDI_FT2232H_PID;
+			channel_count = 2;
+			/* The IOL3 pin is used as system reset (needed to disable the FPGA)
+			 *     IOL0 pin is used instead of CS, called SS
+			 *     IOL2 pin is used to check CDONE
+			 *  value: 0x10  #RST=low, SS=high, DI=low, DO=low, SK=low
+			 *    dir: 0x93  #RST=output, SS=output, DI=input, DO=output, SK=output */
+			cs_bits = 0x10;
+			pindir = 0x93;
+			ice40_mode = 1;
+			if (register_shutdown(ice40_deinit,ftdic)) {
+				msg_perr("Error: Unable to register shutdown sequence.\n");
+				return -9;
+			}
+			arg2 = extract_programmer_param("cdone");
+			if (arg2) {
+				unsigned int temp = 0;
+				char *endptr;
+				temp = strtoul(arg2, &endptr, 0);
+				if (temp)
+					check_cdone = 1;
+				free(arg2);
+			}
 		} else {
 			msg_perr("Error: Invalid device type specified.\n");
 			free(arg);
@@ -420,6 +505,14 @@ 
 
 	register_spi_master(&spi_master_ft2232);
 
+	/* iCE40 mode: memory wake-up */
+	if (ice40_mode) {
+		msg_pdbg("Memory wake-up\n");
+		buf[0] = RELEASE_FROM_POWER_DOWN;
+		if (ft2232_spi_send_command(NULL,1,0,buf,NULL))
+			msg_perr("Unable to send wake-up command.\n");
+	}
+
 	return 0;
 
 ftdi_err:
Index: layout.c
===================================================================
--- layout.c	(revisión: 1955)
+++ layout.c	(copia de trabajo)
@@ -257,28 +257,12 @@ 
 	return ret;
 }
 
-static int copy_old_content(struct flashctx *flash, int oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents, unsigned int start, unsigned int size)
-{
-	if (!oldcontents_valid) {
-		/* oldcontents is a zero-filled buffer. By reading the current data into oldcontents here, we
-		 * avoid a rewrite of identical regions even if an initial full chip read didn't happen. */
-		msg_gdbg2("Read a chunk starting at 0x%06x (len=0x%06x).\n", start, size);
-		int ret = flash->chip->read(flash, oldcontents + start, start, size);
-		if (ret != 0) {
-			msg_gerr("Failed to read chunk 0x%06x-0x%06x.\n", start, start + size - 1);
-			return 1;
-		}
-	}
-	memcpy(newcontents + start, oldcontents + start, size);
-	return 0;
-}
-
 /**
  * Modify @newcontents so that it contains the data that should be on the chip eventually. In the case the user
- * wants to update only parts of it, copy the chunks to be preserved from @oldcontents to @newcontents. If
- * @oldcontents is not valid, we need to fetch the current data from the chip first.
+ * wants to update only parts of it, copy the chunks to be preserved from @oldcontents to @newcontents.
+ * This function is used only when @oldcontents contains the full memory information.
  */
-int build_new_image(struct flashctx *flash, bool oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents)
+void modify_new_image_from_old(struct flashctx *flash, const uint8_t *oldcontents, uint8_t *newcontents)
 {
 	unsigned int start = 0;
 	romentry_t *entry;
@@ -288,7 +272,7 @@ 
 	 * that the user wants to write the complete new image.
 	 */
 	if (num_include_args == 0)
-		return 0;
+		return;
 
 	/* Non-included romentries are ignored.
 	 * The union of all included romentries is used from the new image.
@@ -296,20 +280,116 @@ 
 	while (start < size) {
 		entry = get_next_included_romentry(start);
 		/* No more romentries for remaining region? */
-		if (!entry) {
-			copy_old_content(flash, oldcontents_valid, oldcontents, newcontents, start,
-					 size - start);
+		if (!entry) { /* Copy the old content to skip it */
+			memcpy(newcontents + start, oldcontents + start, size);
 			break;
 		}
 		/* For non-included region, copy from old content. */
 		if (entry->start > start)
-			copy_old_content(flash, oldcontents_valid, oldcontents, newcontents, start,
-					 entry->start - start);
-		/* Skip to location after current romentry. */
+			memcpy(newcontents + start, oldcontents + start, entry->start - start);
+		/* Skip to location after current romentry. The user wants to preserve it. */
 		start = entry->end + 1;
 		/* Catch overflow. */
 		if (!start)
 			break;
 	}
-	return 0;
+	return;
 }
+
+/**
+ * Look for the starting address of the erase region that contains @addr.
+ * The size of this region is returned in @sz
+ */
+int find_page_start(unsigned int addr, struct eraseblock *blocks, unsigned int *sz)
+{
+	unsigned int page_start, i, size, count, group_size;
+	/* Look for the beggining of the erase region containing start address */
+	page_start = 0;
+	for (i = 0; i < NUM_ERASEREGIONS; i++) {
+		size = blocks[i].size;
+		count = blocks[i].count;
+		group_size = size * count;
+		if (page_start + group_size > addr) {
+			/* The address is inside one of these erase regions */
+			*sz = size;
+			return page_start + ((addr - page_start) / size) * size;
+		} else {/* Not here */
+			page_start += group_size;
+		}
+	}
+	msg_gerr("ERROR: address out of memory!?\n");
+	exit(1);
+	return -1;
+}
+
+/**
+ * Read all the erase regions containing the @start-@end interval.
+ * Returns the last address fetched in @real_end_p
+ */
+int read_pages(struct flashctx *flash, uint8_t *oldcontents, unsigned int start, unsigned int end,
+		unsigned int *real_end_p)
+{
+	int k;
+	unsigned int real_start, real_end, size;
+	
+	k = find_erase_function(flash);
+	if (k == -1) {
+		msg_gerr("ERROR: No erase function available.\n");
+		return -1;
+	}
+	real_start = find_page_start(start, flash->chip->block_erasers[k].eraseblocks, &size);
+	msg_gdbg("Start of range 0x%X rounded to page boundary: 0x%X\n", start, real_start);
+	real_end = find_page_start(end, flash->chip->block_erasers[k].eraseblocks, &size);
+	real_end += size - 1;
+	msg_gdbg("End of range 0x%X rounded to page boundary: 0x%X\n", end, real_end);
+	*real_end_p = real_end;
+	return flash->chip->read(flash, oldcontents + real_start, real_start, real_end - real_start + 1);
+}
+
+/**
+ * Fill the @oldcontents buffer with data from the memory. The regions not included by the user are filled
+ * with data from @newcontents instead of the actual memory content. It forces to skip them.
+ * This function is used only when @oldcontents is empty.
+ * Is also mandatory that at least one region was specified by the user.
+ */
+int read_cur_content(struct flashctx *flash, uint8_t *oldcontents, const uint8_t *newcontents)
+{
+	unsigned int start = 0;
+	romentry_t *entry;
+	unsigned int size = flash->chip->total_size * 1024;
+	unsigned int region_end;
+	int ret = 0;
+
+	/* We will read from memory only the regions indicated by the user.
+	 * The rest will contain the same as the newcontents */
+	memcpy(oldcontents, newcontents, size);
+
+	/* Catch a missuse */
+	if (num_include_args == 0) {
+		msg_perr("Internal error! build_old_image() invoked, but no regions specified.\n");
+		return 1;
+	}
+
+	/* Non-included romentries are ignored.
+	 * The union of all included romentries is fetched from memory.
+	 */
+	while (start < size) {
+		entry = get_next_included_romentry(start);
+		/* No more romentries for remaining region? */
+		if (!entry)
+			break;
+		/* For included regions, read memory content content. */
+		msg_gdbg("Read a chunk 0x%06x-0x%06x.\n", entry->start, entry->end);
+		ret = read_pages(flash, oldcontents, entry->start, entry->end, &region_end);
+		if (ret != 0) {
+			msg_gerr("Failed to read chunk 0x%06x-0x%06x.\n", entry->start, entry->end);
+			break;
+		}
+		start = region_end + 1;
+		/* Catch overflow. */
+		if (!start)
+			break;
+	}
+	return ret;
+}
+
Index: spi25.c
===================================================================
--- spi25.c	(revisión: 1955)
+++ spi25.c	(copia de trabajo)
@@ -948,6 +948,7 @@ 
 	int rc = 0;
 	unsigned int i, j, starthere, lenhere, toread;
 	unsigned int page_size = flash->chip->page_size;
+	unsigned int first_page, last_page, total_pages;
 
 	/* Warning: This loop has a very unusual condition and body.
 	 * The loop needs to go through each page with at least one affected
@@ -958,7 +959,10 @@ 
 	 * (start + len - 1) / page_size. Since we want to include that last
 	 * page as well, the loop condition uses <=.
 	 */
-	for (i = start / page_size; i <= (start + len - 1) / page_size; i++) {
+	first_page = start / page_size;
+	last_page = (start + len - 1) / page_size;
+	total_pages = last_page - first_page + 1;
+	for (i = first_page; i <= last_page; i++) {
 		/* Byte position of the first byte in the range in this page. */
 		/* starthere is an offset to the base address of the chip. */
 		starthere = max(start, i * page_size);
@@ -970,6 +974,8 @@ 
 			if (rc)
 				break;
 		}
+		if (flash->progress)
+			flash->progress(flash,(double)(i-first_page+1)/total_pages*100.0);
 		if (rc)
 			break;
 	}