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