Re: [flashrom] [PATCH 3/3] Refine physical address mapping of flash chips.

[Carl-Daniel Hailfinger]

Am 08.08.2014 16:25 schrieb Stefan Tauner:
> Previously we did map the physical memory corresponding to all chips,
> even for SPI chips where it is completely unnecessary because we do
> not use it in any SPI-capable programmer.

Ahem. it87spi.c: it8716f_spi_chip_read() and it8716f_spi_page_program()
uses that.
> Also, the mapping was never undone before the program exited.
> Mapping of additional flash registers was done within probing methods
> instead of centrally.

This patch changes the behaviour: In current HEAD, the mapping of
registers only happens on successful probe. With this patch, it happens
unconditionally (depending only on whether the chip supports it, not on
whether the programmer supports it) before probe. If you really want to
change that, it should be mentioned in the changelog.
> Besides all of that, this patch also adds proper error handling for
> mapping-related operations.
>
> Signed-off-by: Stefan Tauner

Sorry, no ack yet.
> 82802ab.c  |  2 --
>  flash.h    |  1 -
>  flashrom.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++----------------
>  jedec.c    |  6 -----
>  4 files changed, 63 insertions(+), 30 deletions(-)
>
> diff --git a/82802ab.c b/82802ab.c
> index 70c6af0..5d015ba 100644
> --- a/82802ab.c
> +++ b/82802ab.c
> @@ -83,8 +83,6 @@ int probe_82802ab(struct flashctx *flash)
>    if (id1 != flash->chip->manufacture_id || id2 != flash->chip->model_id)
>    	return 0;
>
> if (flash->chip->feature_bits & FEATURE_REGISTERMAP)
> map_flash_registers(flash);
>
> return 1;
>
> }
> diff --git a/flash.h b/flash.h
> index 82a8b74..998696c 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -252,7 +252,6 @@ void tolower_string(char *str);
>  /* flashrom.c */
>  extern const char flashrom_version[];
>  extern const char *chip_to_probe;
> -void map_flash_registers(struct flashctx *flash);
>  int read_memmapped(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
>  int erase_flash(struct flashctx *flash);
>  int probe_flash(struct registered_master *mst, int startchip, struct flashctx *fill_flash, int force);
> diff --git a/flashrom.c b/flashrom.c
> index 0ff6b63..72bf157 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -528,12 +528,23 @@ void programmer_delay(unsigned int usecs)
>    	programmer_table[programmer].delay(usecs);
>  }
>
> -void map_flash_registers(struct flashctx *flash)
> +static int map_flash_registers(struct flashctx *flash)

Can you leave that one untouched, please? It's code I dare not change
because even I have no idea where (if at all) registers would be mapped
for chips on masters mapping the chip to nonstandard locations. If you
have access to Loongson and Elan SC520 datasheets, you're welcome to
check and fix this. Until then, the known broken and documented broken
code is IMHO better than speculative code that looks like it could work
but was never checked against datasheets or hardware.
> {
>    size_t size = flash->chip->total_size * 1024;
>    /* Flash registers live 4 MByte below the flash. */
> /* FIXME: This is incorrect for nonstandard flashbase. */
> flash->virtual_registers = (chipaddr)programmer_map_flash_region("flash chip registers", (0xFFFFFFFF - 0x400000 - size + 1), size);
> uintptr_t base = (flashbase != 0) ? flashbase : (0xffffffff - size + 1);
> if (base < 0x400000) {
> msg_perr("Base address is too low to map flash registers.");
> return 1;
> }
> base -= 0x400000;
> void *addr = programmer_map_flash_region("flash chip registers", base, size);
> if (addr == ERROR_PTR) {
> msg_perr("Could not map flash chip registers.\n");
> return 1;
> }
> flash->virtual_registers = (chipaddr)addr;
> return 0;
>
> }
>
> int read_memmapped(struct flashctx *flash, uint8_t *buf, unsigned int start,
> @@ -1052,12 +1063,49 @@ unsigned int count_max_decode_exceedings(struct flashctx *flash)
>    return limitexceeded;
>  }
>
> +void unmap_flash(struct flashctx *flash)
> +{
> if (flash->chip->bustype & (BUS_PARALLEL | BUS_LPC | BUS_FWH)) {
> if (flash->virtual_memory != 0) {
> 	programmer_unmap_flash_region((void *)flash->virtual_memory,
> 				      flash->chip->total_size * 1024);
> 	flash->virtual_memory = 0;
> }
> if (flash->chip->feature_bits & FEATURE_REGISTERMAP && flash->virtual_registers != 0) {
> 	programmer_unmap_flash_region((void *)flash->virtual_registers,
> 				      flash->chip->total_size * 1024);
> 	flash->virtual_registers = 0;
> }
> }
>
> +}
>
> +int map_flash(struct flashctx *flash)
> +{
> const unsigned long size = flash->chip->total_size;
> if (flash->chip->bustype & (BUS_PARALLEL | BUS_LPC | BUS_FWH)) {
> uintptr_t base = flashbase ? flashbase : (0xffffffff - size + 1);
> void *addr = programmer_map_flash_region("flash chip", base, size);
> if (addr == ERROR_PTR) {
> 	msg_perr("Could not map flash chip registers.\n");
> 	return 1;
> }
> flash->virtual_memory = (chipaddr)addr;
> if (flash->chip->feature_bits & FEATURE_REGISTERMAP) {
> 	if (map_flash_registers(flash) != 0) {
> 		unmap_flash(flash);
> 		return 1;
> 	}
> }
> }

AFAICS this will segfault (NULL pointer dereference) on chips of 4 MBit
and less on IT87 SPI.
> return 0;
>
> +}
>
> int probe_flash(struct registered_master *mst, int startchip, struct flashctx *flash, int force)
>  {
>    const struct flashchip *chip;
> unsigned long base = 0;
> char location[64];
> uint32_t size;
> enum chipbustype buses_common;
> char *tmp;
>
> @@ -1082,9 +1130,8 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
>    	memcpy(flash->chip, chip, sizeof(struct flashchip));
>    	flash->mst = mst;
> size = chip->total_size * 1024;
> base = flashbase ? flashbase : (0xffffffff - size + 1);
> flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size);
> if (map_flash(flash) != 0)
> 	return -1;
>
> /* We handle a forced match like a real match, we just avoid probing. Note that probe_flash()
> is only called with force=1 after normal probing failed.
>
> @@ -1133,8 +1180,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
>    		break;
>    	/* Not the first flash chip detected on this bus, and it's just a generic match. Ignore it. */
>  notfound:
> programmer_unmap_flash_region((void *)flash->virtual_memory, size);
> flash->virtual_memory = (chipaddr)NULL;
> unmap_flash(flash);
> free(flash->chip);
> flash->chip = NULL;
> }
>
> @@ -1142,16 +1188,9 @@ notfound:
>    if (!flash->chip)
>    	return -1;
>
> -#if CONFIG_INTERNAL == 1
> if (programmer_table[programmer].map_flash_region == physmap)
> snprintf(location, sizeof(location), "at physical address 0x%lx", base);
> else
>
> -#endif
> snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);

That info should not have disappeared, and we may soon have to extend it
for funny constellations like two chips in a programmer (EC flash and
chipset flash for laptops).
> tmp = flashbuses_to_text(flash->chip->bustype);
> msg_cinfo("%s %s flash chip "%s" (%d kB, %s) %s.\n", force ? "Assuming" : "Found",
>   flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp, location);
>
> msg_cinfo("%s %s flash chip "%s" (%d kB, %s).\n", force ? "Assuming" : "Found",
>   flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp);
>
> free(tmp);
>
> /* Flash registers will not be mapped if the chip was forced. Lock info
>
> @@ -1872,7 +1911,8 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>    	flash->chip->unlock(flash);
>
> if (read_it) {
> return read_flash_to_file(flash, filename);
> ret = read_flash_to_file(flash, filename);
> goto unmap;
>
> }
>
> oldcontents = malloc(size);
>
> @@ -1991,5 +2031,7 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>  out:
>    free(oldcontents);
>    free(newcontents);
> +unmap:
> unmap_flash(flash);
> return ret;
>
> }
> diff --git a/jedec.c b/jedec.c
> index 358b850..1345b89 100644
> --- a/jedec.c
> +++ b/jedec.c
> @@ -166,9 +166,6 @@ int probe_jedec_29gl(struct flashctx *flash)
>    if (man_id != chip->manufacture_id || dev_id != chip->model_id)
>    	return 0;
> if (chip->feature_bits & FEATURE_REGISTERMAP)
> map_flash_registers(flash);
> return 1;
>
> }
>
> @@ -287,9 +284,6 @@ static int probe_jedec_common(struct flashctx *flash, unsigned int mask)
>    if (largeid1 != chip->manufacture_id || largeid2 != chip->model_id)
>    	return 0;
> if (chip->feature_bits & FEATURE_REGISTERMAP)
> map_flash_registers(flash);
> return 1;
>
> }

Regards,
Carl-Daniel

[PATCH 3/3] Refine physical address mapping of flash chips.

[Stefan Tauner]

On Fri, 15 Aug 2014 01:09:44 +0200
Carl-Daniel Hailfinger  wrote:
> > -#if CONFIG_INTERNAL == 1
> > if (programmer_table[programmer].map_flash_region == physmap)
> > snprintf(location, sizeof(location), "at physical address 0x%lx", base);
> > else
> >
> > -#endif
> > snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);
>
> That info should not have disappeared, and we may soon have to extend it
> for funny constellations like two chips in a programmer (EC flash and
> chipset flash for laptops).

If this is an important detail we want to tell the user then we need to
define a proper interface... there is no reason at all to know the base
address in the probing function and that's why I have removed that bit
completely, and because I don't see how this is important at all at the
msg_pinfo level(!). I would be ok with printing the programmer (well,
the master would be better if that's already possible... do they
have names already? :) ... but the base address printing needs to die or
be moved elsewhere.

[PATCH 3/3] Refine physical address mapping of flash chips.

[Carl-Daniel Hailfinger]

Am 16.08.2014 01:43 schrieb Stefan Tauner:
> On Fri, 15 Aug 2014 01:09:44 +0200
> Carl-Daniel Hailfinger  wrote:
> > > -#if CONFIG_INTERNAL == 1
> > > if (programmer_table[programmer].map_flash_region == physmap)
> > > snprintf(location, sizeof(location), "at physical address 0x%lx", base);
> > > else
> > >
> > > -#endif
> > > snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);
> >
> > That info should not have disappeared, and we may soon have to extend it
> > for funny constellations like two chips in a programmer (EC flash and
> > chipset flash for laptops).
>
> If this is an important detail we want to tell the user then we need to
> define a proper interface... there is no reason at all to know the base
> address in the probing function and that's why I have removed that bit
> completely, and because I don't see how this is important at all at the
> msg_pinfo level(!).

What about dual flash chip setups in the FWH/LPC case? There we have
multiple flash chips at different addresses, e.g. a 2 MByte chip at
4G-4M and a 2 MByte chip at 4G-2M. Yes, such hardware exists. It's the
reason we have the feature to optionally set FWH IDSEL on ICH for
misconfigured boards of that type. Admittedly the LPC case is a bit less
weird and works by strapping LPC chips to the correct address.
> I would be ok with printing the programmer (well,
> the master would be better if that's already possible... do they
> have names already? :) ... but the base address printing needs to die or
> be moved elsewhere.

Indeed, we should print the master name, not the programmer name. I
thought I had a patch doing that, but digging through my archive it
seems I can't find that patch anymore and/or it was a figment of my
imagination.

Regards,
Carl-Daniel

[PATCH 3/3] Refine physical address mapping of flash chips.

[Stefan Tauner]

On Sun, 17 Aug 2014 01:09:00 +0200
Carl-Daniel Hailfinger  wrote:
> Am 16.08.2014 01:43 schrieb Stefan Tauner:
> > On Fri, 15 Aug 2014 01:09:44 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > > -#if CONFIG_INTERNAL == 1
> > > > if (programmer_table[programmer].map_flash_region == physmap)
> > > > snprintf(location, sizeof(location), "at physical address 0x%lx", base);
> > > > else
> > > >
> > > > -#endif
> > > > snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);
> > >
> > > That info should not have disappeared, and we may soon have to extend it
> > > for funny constellations like two chips in a programmer (EC flash and
> > > chipset flash for laptops).
> >
> > If this is an important detail we want to tell the user then we need to
> > define a proper interface... there is no reason at all to know the base
> > address in the probing function and that's why I have removed that bit
> > completely, and because I don't see how this is important at all at the
> > msg_pinfo level(!).
>
> What about dual flash chip setups in the FWH/LPC case? There we have
> multiple flash chips at different addresses, e.g. a 2 MByte chip at
> 4G-4M and a 2 MByte chip at 4G-2M. Yes, such hardware exists. It's the
> reason we have the feature to optionally set FWH IDSEL on ICH for
> misconfigured boards of that type. Admittedly the LPC case is a bit less
> weird and works by strapping LPC chips to the correct address.

Well OK, so we still want to print that but not necessarily at info
level. The problem is that the cli code does not know the base address,
not even the probing code knows it (and that's a good thing!). That's a
problem for the print though. The pseudo code of the old probe (with
this (refined) patch applied looks like this:

cli_classic.c:
loop over chips, call probe_flash with incremented index
...
if a single flash chip was detected then {
    if (probe only)
    	exit()
    map_flash()
    doit()
    unmap_flash()	
}

flashrom.c:
probe_flash:
loop over chips starting at the given index {
    map_flash()
    call chip probe function()
    unmap_flash()
}

Only map_flash() knows the base address but now we want to print it in
cli_classic.c.

We have at least these options:
 - print it all the time in map_flash() but on a high verbosity level
   like dbg2 or spew (BTW on spew it is already visible due to the
   output in programmer_map_flash_region())
 - refine map_flash and add a bool print parameter
 - add a get_map_base() function that can be called in map_flash() and
   gets exported to be callable in cli_classic etc. as well.
> > I would be ok with printing the programmer (well,
> > the master would be better if that's already possible... do they
> > have names already? :) ... but the base address printing needs to die or
> > be moved elsewhere.
>
> Indeed, we should print the master name, not the programmer name. I
> thought I had a patch doing that, but digging through my archive it
> seems I can't find that patch anymore and/or it was a figment of my
> imagination.

That can be tackled later though.

[PATCH] Refine physical address mapping of flash chips.

[Stefan Tauner]

- Create distinct functions for mapping and unmapping for flash chips.
 - Map only when needed: map before probing and unmap immediately
   after it. Map again when a single chip was probed successfully before
   taking any actual actions and clean up afterwards.
 - Map special function chip registers centrally together with flash space
   instead of within (some) probing methods after successful probes.
 - Save the used base addresses of the mappings in struct flashctx as well.
 - Do not try to (un)map the zero-sized chip definitions that are merely hacks.
   This also fixes the printing of wrong warnings for these chip definitions
   introduced in r1765.

Signed-off-by: Stefan Tauner 
---

After some more discussions on IRC I came up with this... It was
actually a nice exercise to get more familiar with the issue - too bad
it is so annoying ;)

82802ab.c     |  3 --
 cli_classic.c | 15 +++++++++-
 flash.h       | 17 +++++++----
 flashrom.c    | 93 ++++++++++++++++++++++++++++++++++++++++++++---------------
 jedec.c       |  6 ----
 5 files changed, 96 insertions(+), 38 deletions(-)

diff --git a/82802ab.c b/82802ab.c
index 70c6af0..1436f8a 100644
--- a/82802ab.c
+++ b/82802ab.c
@@ -83,9 +83,6 @@ int probe_82802ab(struct flashctx *flash)
    if (id1 != flash->chip->manufacture_id || id2 != flash->chip->model_id)
    	return 0;

-	if (flash->chip->feature_bits & FEATURE_REGISTERMAP)
-		map_flash_registers(flash);
-
    return 1;
 }

diff --git a/cli_classic.c b/cli_classic.c
index 945ad7b..a693596 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -482,7 +482,9 @@ int main(int argc, char *argv[])
    			goto out_shutdown;
    		}
    		msg_cinfo("Please note that forced reads most likely contain garbage.\n");
+			map_flash(&flashes[0]);
    		ret = read_flash_to_file(&flashes[0], filename);
+			unmap_flash(&flashes[0]);
    		free(flashes[0].chip);
    		goto out_shutdown;
    	}
@@ -516,9 +518,18 @@ int main(int argc, char *argv[])
    	goto out_shutdown;
    }

+	msg_pdbg2("Successfully mapped flash chip "%s" at physical address 0x%0*" PRIxPTR ".\n",
+		  fill_flash->chip->name, PRIxPTR_WIDTH, fill_flash->physical_memory);
+
+	/* Map the selected flash chip again. */
+	if (map_flash(fill_flash) != 0) {
+		ret = 1;
+		goto out_shutdown;
+	}
+
    if (!(read_it | write_it | verify_it | erase_it)) {
    	msg_ginfo("No operations were specified.\n");
-		goto out_shutdown;
+		goto out_unmap;
    }

/* Always verify write operations unless -n is used. */
@@ -532,6 +543,8 @@ int main(int argc, char *argv[])
    programmer_delay(100000);
    ret |= doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it);

+out_unmap:
+	unmap_flash(fill_flash);
 out_shutdown:
    programmer_shutdown();
 out:
diff --git a/flash.h b/flash.h
index 301eb7a..a5d3a55 100644
--- a/flash.h
+++ b/flash.h
@@ -48,9 +48,9 @@ typedef uintptr_t chipaddr;
 /* Types and macros regarding the maximum flash space size supported by generic code. */
 typedef uint32_t chipoff_t; /* Able to store any addressable offset within a supported flash memory. */
 typedef uint32_t chipsize_t; /* Able to store the number of bytes of any supported flash memory. */
-#define FL_MAX_CHIPADDR_BITS (24)
-#define FL_MAX_CHIPADDR ((chipoff_t)(1ULL<<FL_MAX_CHIPADDR_BITS)-1)
-#define PRIxCHIPADDR "06"PRIx32
+#define FL_MAX_CHIPOFF_BITS (24)
+#define FL_MAX_CHIPOFF ((chipoff_t)(1ULL<<FL_MAX_CHIPOFF_BITS)-1)
+#define PRIxCHIPOFF "06"PRIx32
 #define PRIuCHIPSIZE PRIu32

int register_shutdown(int (*function) (void *data), void *data);
@@ -209,8 +209,14 @@ struct flashchip {

struct flashctx {
    struct flashchip *chip;
+	/* FIXME: The memory mappings should be saved in a more structured way. */
+	/* The physical_* fields store the respective addresses in the physical address space of the CPU. */
+	uintptr_t physical_memory;
+	/* The virtual_* fields store where the respective physical address is mapped into flashrom's address
+	 * space. A value equivalent to (chipaddr)ERROR_PTR indicates an invalid mapping (or none at all). */
    chipaddr virtual_memory;
-	/* Some flash devices have an additional register space. */
+	/* Some flash devices have an additional register space; semantics are like above. */
+	uintptr_t physical_registers;
    chipaddr virtual_registers;
    struct registered_master *mst;
 };
@@ -252,7 +258,8 @@ void tolower_string(char *str);
 /* flashrom.c */
 extern const char flashrom_version[];
 extern const char *chip_to_probe;
-void map_flash_registers(struct flashctx *flash);
+int map_flash(struct flashctx *flash);
+void unmap_flash(struct flashctx *flash);
 int read_memmapped(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
 int erase_flash(struct flashctx *flash);
 int probe_flash(struct registered_master *mst, int startchip, struct flashctx *fill_flash, int force);
diff --git a/flashrom.c b/flashrom.c
index 93b292b..7471cac 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -479,6 +479,7 @@ void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t
 void programmer_unmap_flash_region(void *virt_addr, size_t len)
 {
    programmer_table[programmer].unmap_flash_region(virt_addr, len);
+	msg_gspew("%s: unmapped 0x%0*" PRIxPTR "\n", __func__, PRIxPTR_WIDTH, (uintptr_t)virt_addr);
 }

void chip_writeb(const struct flashctx *flash, uint8_t val, chipaddr addr)
@@ -528,14 +529,6 @@ void programmer_delay(unsigned int usecs)
    	programmer_table[programmer].delay(usecs);
 }

-void map_flash_registers(struct flashctx *flash)
-{
-	size_t size = flash->chip->total_size * 1024;
-	/* Flash registers live 4 MByte below the flash. */
-	/* FIXME: This is incorrect for nonstandard flashbase. */
-	flash->virtual_registers = (chipaddr)programmer_map_flash_region("flash chip registers", (0xFFFFFFFF - 0x400000 - size + 1), size);
-}
-
 int read_memmapped(struct flashctx *flash, uint8_t *buf, unsigned int start,
    	   int unsigned len)
 {
@@ -1052,12 +1045,64 @@ unsigned int count_max_decode_exceedings(const struct flashctx *flash)
    return limitexceeded;
 }

+void unmap_flash(struct flashctx *flash)
+{
+	if (flash->virtual_registers != (chipaddr)ERROR_PTR) {
+		programmer_unmap_flash_region((void *)flash->virtual_registers, flash->chip->total_size * 1024);
+		flash->physical_registers = 0;
+		flash->virtual_registers = (chipaddr)ERROR_PTR;
+	}
+
+	if (flash->virtual_memory != (chipaddr)ERROR_PTR) {
+		programmer_unmap_flash_region((void *)flash->virtual_memory, flash->chip->total_size * 1024);
+		flash->physical_memory = 0;
+		flash->virtual_memory = (chipaddr)ERROR_PTR;
+	}
+}
+
+int map_flash(struct flashctx *flash)
+{
+	/* Init pointers to the fail-safe state to distinguish them later from legit values. */
+	flash->virtual_memory = (chipaddr)ERROR_PTR;
+	flash->virtual_registers = (chipaddr)ERROR_PTR;
+
+	/* FIXME: This avoids mapping (and unmapping) of flash chip definitions with size 0.
+	 * These are used for various probing-related hacks that would not map successfully anyway and should be
+	 * removed ASAP. */
+	if (flash->chip->total_size == 0)
+		return 0;
+
+	const chipsize_t size = flash->chip->total_size * 1024;
+	uintptr_t base = flashbase ? flashbase : (0xffffffff - size + 1);
+	void *addr = programmer_map_flash_region(flash->chip->name, base, size);
+	if (addr == ERROR_PTR) {
+		msg_perr("Could not map flash chip %s at 0x%0*" PRIxPTR ".\n",
+			 flash->chip->name, PRIxPTR_WIDTH, base);
+		return 1;
+	}
+	flash->physical_memory = base;
+	flash->virtual_memory = (chipaddr)addr;
+
+	/* FIXME: Special function registers normally live 4 MByte below flash space, but it might be somewhere
+	 * completely different on some chips and programmers, or not mappable at all.
+	 * Ignore these problems for now and always report success. */
+	if (flash->chip->feature_bits & FEATURE_REGISTERMAP) {
+		base = 0xffffffff - size - 0x400000 + 1;
+		addr = programmer_map_flash_region("flash chip registers", base, size);
+		if (addr == ERROR_PTR) {
+			msg_pdbg2("Could not map flash chip registers %s at 0x%0*" PRIxPTR ".\n",
+				 flash->chip->name, PRIxPTR_WIDTH, base);
+			return 0;
+		}
+		flash->physical_registers = base;
+		flash->virtual_registers = (chipaddr)addr;
+	}
+	return 0;
+}
+
 int probe_flash(struct registered_master *mst, int startchip, struct flashctx *flash, int force)
 {
    const struct flashchip *chip;
-	unsigned long base = 0;
-	char location[64];
-	uint32_t size;
    enum chipbustype buses_common;
    char *tmp;

@@ -1082,9 +1127,8 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
    	memcpy(flash->chip, chip, sizeof(struct flashchip));
    	flash->mst = mst;

-		size = flash->chip->total_size * 1024;
-		base = flashbase ? flashbase : (0xffffffff - size + 1);
-		flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size);
+		if (map_flash(flash) != 0)
+			return -1;

/* We handle a forced match like a real match, we just avoid probing. Note that probe_flash()
    	 * is only called with force=1 after normal probing failed.
@@ -1133,8 +1177,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
    		break;
    	/* Not the first flash chip detected on this bus, and it's just a generic match. Ignore it. */
 notfound:
-		programmer_unmap_flash_region((void *)flash->virtual_memory, size);
-		flash->virtual_memory = (chipaddr)NULL;
+		unmap_flash(flash);
    	free(flash->chip);
    	flash->chip = NULL;
    }
@@ -1142,17 +1185,18 @@ notfound:
    if (!flash->chip)
    	return -1;

+
+	tmp = flashbuses_to_text(flash->chip->bustype);
+	msg_cinfo("%s %s flash chip "%s" (%d kB, %s) ", force ? "Assuming" : "Found",
+		  flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp);
+	free(tmp);
 #if CONFIG_INTERNAL == 1
    if (programmer_table[programmer].map_flash_region == physmap)
-		snprintf(location, sizeof(location), "at physical address 0x%lx", base);
+		msg_cinfo("mapped at physical address 0x%0*" PRIxPTR ".\n",
+			  PRIxPTR_WIDTH, flash->physical_memory);
    else
 #endif
-		snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);
-
-	tmp = flashbuses_to_text(flash->chip->bustype);
-	msg_cinfo("%s %s flash chip "%s" (%d kB, %s) %s.\n", force ? "Assuming" : "Found",
-		  flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp, location);
-	free(tmp);
+		msg_cinfo("on %s.\n", programmer_table[programmer].name);

/* Flash registers will not be mapped if the chip was forced. Lock info
     * may be stored in registers, so avoid lock info printing.
@@ -1161,6 +1205,9 @@ notfound:
    	if (flash->chip->printlock)
    		flash->chip->printlock(flash);

+	/* Get out of the way for later runs. */
+	unmap_flash(flash);
+
    /* Return position of matching chip. */
    return chip - flashchips;
 }
diff --git a/jedec.c b/jedec.c
index 358b850..1345b89 100644
--- a/jedec.c
+++ b/jedec.c
@@ -166,9 +166,6 @@ int probe_jedec_29gl(struct flashctx *flash)
    if (man_id != chip->manufacture_id || dev_id != chip->model_id)
    	return 0;

-	if (chip->feature_bits & FEATURE_REGISTERMAP)
-		map_flash_registers(flash);
-
    return 1;
 }

@@ -287,9 +284,6 @@ static int probe_jedec_common(struct flashctx *flash, unsigned int mask)
    if (largeid1 != chip->manufacture_id || largeid2 != chip->model_id)
    	return 0;

-	if (chip->feature_bits & FEATURE_REGISTERMAP)
-		map_flash_registers(flash);
-
    return 1;
 }