[M] Change in flashrom[master]: drivers: Move (un)map_flash_region to par/spi/opaque_master
Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/67695 )
Change subject: drivers: Move (un)map_flash_region to par/spi/opaque_master ......................................................................
drivers: Move (un)map_flash_region to par/spi/opaque_master
Move (un)map_flash_region function pointers from programmer_entry to par_master, spi_master, and opaque_master. This enables programmers to specify a different mapper per bus, which is needed for the internal programmer. Mapping is closely tied to the way the memory is accessed using the other functions in the bus master structs.
Validate that FWH/LPC programmers provide specialized mapping in register_par_master(); this is needed for chips with FEATURE_REGISTERMAP, which only exist on FWH or LPC buses.
programmer.c: Update comment in fallback_map(), NULL return is the desired behavior.
Test: Read firmware on SB600 Promontory mainboard (requires physmap) Test: Read firmware externally with ft2232_spi Test: Read firmware on ICH hwseq, verify physmap still occurs
Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b Signed-off-by: Jonathon Hall jonathon.hall@puri.sm Co-Authored-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/67695 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M atavia.c M dummyflasher.c M flashrom.c M ichspi.c M include/flash.h M include/programmer.h M internal.c M it87spi.c M parallel.c M programmer.c M sb600spi.c M serprog.c M wbsio_spi.c 13 files changed, 150 insertions(+), 41 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/atavia.c b/atavia.c index 66bca65..5cbe058 100644 --- a/atavia.c +++ b/atavia.c @@ -133,6 +133,7 @@ }
static const struct par_master lpc_master_atavia = { + .map_flash_region = atavia_map, .chip_readb = atavia_chip_readb, .chip_readw = fallback_chip_readw, .chip_readl = fallback_chip_readl, @@ -189,5 +190,4 @@ .type = PCI, .devs.dev = ata_via, .init = atavia_init, - .map_flash_region = atavia_map, }; diff --git a/dummyflasher.c b/dummyflasher.c index b2a41c6..ad734f0 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -920,6 +920,8 @@ }
static const struct spi_master spi_master_dummyflasher = { + .map_flash_region = dummy_map, + .unmap_flash_region = dummy_unmap, .features = SPI_MASTER_4BA, .max_data_read = MAX_DATA_READ_UNLIMITED, .max_data_write = MAX_DATA_UNSPECIFIED, @@ -932,6 +934,8 @@ };
static const struct par_master par_master_dummyflasher = { + .map_flash_region = dummy_map, + .unmap_flash_region = dummy_unmap, .chip_readb = dummy_chip_readb, .chip_readw = dummy_chip_readw, .chip_readl = dummy_chip_readl, @@ -943,6 +947,8 @@ };
static const struct opaque_master opaque_master_dummyflasher = { + .map_flash_region = dummy_map, + .unmap_flash_region = dummy_unmap, .probe = probe_variable_size, .read = dummy_opaque_read, .write = dummy_opaque_write, @@ -1424,6 +1430,4 @@ /* FIXME */ .devs.note = "Dummy device, does nothing and logs all accesses\n", .init = dummy_init, - .map_flash_region = dummy_map, - .unmap_flash_region = dummy_unmap, }; diff --git a/flashrom.c b/flashrom.c index f74b79a..90899f0 100644 --- a/flashrom.c +++ b/flashrom.c @@ -203,27 +203,62 @@ return ret; }
-void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len) +void *master_map_flash_region(const struct registered_master *mst, + const char *descr, uintptr_t phys_addr, + size_t len) { + /* Check the bus master for a specialized map_flash_region; default to + * fallback if it does not specialize it + */ + void *(*map_flash_region) (const char *descr, uintptr_t phys_addr, size_t len) = NULL; + if (mst->buses_supported & BUS_PROG) + map_flash_region = mst->opaque.map_flash_region; + else if (mst->buses_supported & BUS_SPI) + map_flash_region = mst->spi.map_flash_region; + else if (mst->buses_supported & BUS_NONSPI) + map_flash_region = mst->par.map_flash_region; + void *ret; - if (programmer->map_flash_region) - ret = programmer->map_flash_region(descr, phys_addr, len); + if (map_flash_region) + ret = map_flash_region(descr, phys_addr, len); else ret = fallback_map(descr, phys_addr, len); msg_gspew("%s: mapping %s from 0x%0*" PRIxPTR " to 0x%0*" PRIxPTR "\n", - __func__, descr, PRIxPTR_WIDTH, phys_addr, PRIxPTR_WIDTH, (uintptr_t) ret); + __func__, descr, PRIxPTR_WIDTH, phys_addr, PRIxPTR_WIDTH, (uintptr_t) ret); return ret; }
-void programmer_unmap_flash_region(void *virt_addr, size_t len) +void master_unmap_flash_region(const struct registered_master *mst, + void *virt_addr, size_t len) { - if (programmer->unmap_flash_region) - programmer->unmap_flash_region(virt_addr, len); + void (*unmap_flash_region) (void *virt_addr, size_t len) = NULL; + if (mst->buses_supported & BUS_PROG) + unmap_flash_region = mst->opaque.unmap_flash_region; + else if (mst->buses_supported & BUS_SPI) + unmap_flash_region = mst->spi.unmap_flash_region; + else if (mst->buses_supported & BUS_NONSPI) + unmap_flash_region = mst->par.unmap_flash_region; + + if (unmap_flash_region) + unmap_flash_region(virt_addr, len); else fallback_unmap(virt_addr, len); msg_gspew("%s: unmapped 0x%0*" PRIxPTR "\n", __func__, PRIxPTR_WIDTH, (uintptr_t)virt_addr); }
+static bool master_uses_physmap(const struct registered_master *mst) +{ +#if CONFIG_INTERNAL == 1 + if (mst->buses_supported & BUS_PROG) + return mst->opaque.map_flash_region == physmap; + else if (mst->buses_supported & BUS_SPI) + return mst->spi.map_flash_region == physmap; + else if (mst->buses_supported & BUS_NONSPI) + return mst->par.map_flash_region == physmap; +#endif + return false; +} + void programmer_delay(unsigned int usecs) { if (usecs > 0) { @@ -647,13 +682,13 @@ 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); + master_unmap_flash_region(flash->mst, (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); + master_unmap_flash_region(flash->mst, (void *)flash->virtual_memory, flash->chip->total_size * 1024); flash->physical_memory = 0; flash->virtual_memory = (chipaddr)ERROR_PTR; } @@ -673,7 +708,7 @@
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); + void *addr = master_map_flash_region(flash->mst, 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); @@ -687,7 +722,7 @@ * 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); + addr = master_map_flash_region(flash->mst, "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); @@ -831,12 +866,10 @@ 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->map_flash_region == physmap) + if (master_uses_physmap(mst)) msg_cinfo("mapped at physical address 0x%0*" PRIxPTR ".\n", PRIxPTR_WIDTH, flash->physical_memory); else -#endif msg_cinfo("on %s.\n", programmer->name);
/* Flash registers may more likely not be mapped if the chip was forced. diff --git a/ichspi.c b/ichspi.c index fa15a95..7e7967e 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1773,6 +1773,8 @@ .max_data_write = 64, .command = ich_spi_send_command, .multicommand = ich_spi_send_multicommand, + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .read = default_spi_read, .write_256 = default_spi_write_256, .write_aai = default_spi_write_aai, @@ -1783,6 +1785,8 @@ .max_data_write = 64, .command = ich_spi_send_command, .multicommand = ich_spi_send_multicommand, + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .read = default_spi_read, .write_256 = default_spi_write_256, .write_aai = default_spi_write_aai, @@ -1790,6 +1794,8 @@ };
static const struct opaque_master opaque_master_ich_hwseq = { + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .max_data_read = 64, .max_data_write = 64, .probe = ich_hwseq_probe, @@ -2199,6 +2205,8 @@ .max_data_write = 16, .command = ich_spi_send_command, .multicommand = ich_spi_send_multicommand, + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .read = default_spi_read, .write_256 = default_spi_write_256, .write_aai = default_spi_write_aai, diff --git a/include/flash.h b/include/flash.h index 79aaa64..c696f64 100644 --- a/include/flash.h +++ b/include/flash.h @@ -55,8 +55,11 @@ #define PRIxPTR_WIDTH ((int)(sizeof(uintptr_t)*2))
int register_shutdown(int (*function) (void *data), void *data); -void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len); -void programmer_unmap_flash_region(void *virt_addr, size_t len); +struct registered_master; +void *master_map_flash_region(const struct registered_master *mast, + const char *descr, uintptr_t phys_addr, size_t len); +void master_unmap_flash_region(const struct registered_master *mast, + void *virt_addr, size_t len); void programmer_delay(unsigned int usecs);
#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) diff --git a/include/programmer.h b/include/programmer.h index 17abfd2..4b5bf92 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -50,9 +50,6 @@
int (*init) (const struct programmer_cfg *cfg);
- void *(*map_flash_region) (const char *descr, uintptr_t phys_addr, size_t len); - void (*unmap_flash_region) (void *virt_addr, size_t len); - void (*delay) (unsigned int usecs); };
@@ -309,6 +306,8 @@ int (*multicommand)(const struct flashctx *flash, struct spi_command *cmds);
/* Optimized functions for this master */ + void *(*map_flash_region) (const char *descr, uintptr_t phys_addr, size_t len); + void (*unmap_flash_region) (void *virt_addr, size_t len); int (*read)(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*write_256)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int (*write_aai)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); @@ -399,6 +398,8 @@
/* opaque.c */ struct opaque_master { + void *(*map_flash_region) (const char *descr, uintptr_t phys_addr, size_t len); + void (*unmap_flash_region) (void *virt_addr, size_t len); int max_data_read; int max_data_write; /* Specific functions for this master */ @@ -424,6 +425,8 @@
/* parallel.c */ struct par_master { + void *(*map_flash_region) (const char *descr, uintptr_t phys_addr, size_t len); + void (*unmap_flash_region) (void *virt_addr, size_t len); void (*chip_writeb) (const struct flashctx *flash, uint8_t val, chipaddr addr); void (*chip_writew) (const struct flashctx *flash, uint16_t val, chipaddr addr); void (*chip_writel) (const struct flashctx *flash, uint32_t val, chipaddr addr); diff --git a/internal.c b/internal.c index 2558cde..8c834e2 100644 --- a/internal.c +++ b/internal.c @@ -107,6 +107,8 @@ }
static const struct par_master par_master_internal = { + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .chip_readb = internal_chip_readb, .chip_readw = internal_chip_readw, .chip_readl = internal_chip_readl, @@ -347,6 +349,4 @@ .type = OTHER, .devs.note = NULL, .init = internal_init, - .map_flash_region = physmap, - .unmap_flash_region = physunmap, }; diff --git a/it87spi.c b/it87spi.c index 85da150..5acaea6 100644 --- a/it87spi.c +++ b/it87spi.c @@ -315,6 +315,8 @@ .max_data_write = MAX_DATA_UNSPECIFIED, .command = it8716f_spi_send_command, .multicommand = default_spi_send_multicommand, + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .read = it8716f_spi_chip_read, .write_256 = it8716f_spi_chip_write_256, .write_aai = spi_chip_write_1, diff --git a/parallel.c b/parallel.c index 9001fe8..e635088 100644 --- a/parallel.c +++ b/parallel.c @@ -76,6 +76,17 @@ } }
+ /* Bus masters supporting FWH/LPC cannot use fallback_map(), distinct + * mappings are needed to support chips with FEATURE_REGISTERMAP + */ + if ((buses & (BUS_FWH | BUS_LPC)) && !mst->map_flash_region) { + msg_perr("%s called with incomplete master definition. " + "FWH/LPC masters must provide memory mappings. " + "Please report a bug at flashrom@flashrom.org\n", + __func__); + return ERROR_FLASHROM_BUG; + } + if (!mst->chip_writeb || !mst->chip_writew || !mst->chip_writel || !mst->chip_writen || !mst->chip_readb || !mst->chip_readw || !mst->chip_readl || !mst->chip_readn) { diff --git a/programmer.c b/programmer.c index 2b960e6..939d8c2 100644 --- a/programmer.c +++ b/programmer.c @@ -20,7 +20,12 @@ /* Fallback map() for programmers which don't need special handling */ void *fallback_map(const char *descr, uintptr_t phys_addr, size_t len) { - /* FIXME: Should return phys_addr. */ + /* A result of NULL causes mapped addresses to be chip physical + * addresses, assuming only a single region is mapped (the entire flash + * space). Chips with a second region (like a register map) require a + * real memory mapping to distinguish the different ranges. Those chips + * are FWH/LPC, so the bus master provides a real mapping. + */ return NULL; }
diff --git a/sb600spi.c b/sb600spi.c index 8b02b51..6020145 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -600,6 +600,8 @@ .max_data_write = FIFO_SIZE_OLD - 3, .command = sb600_spi_send_command, .multicommand = default_spi_send_multicommand, + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .read = default_spi_read, .write_256 = default_spi_write_256, .write_aai = default_spi_write_aai, @@ -612,6 +614,8 @@ .max_data_write = FIFO_SIZE_YANGTZE - 3, .command = spi100_spi_send_command, .multicommand = default_spi_send_multicommand, + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .read = default_spi_read, .write_256 = default_spi_write_256, .write_aai = default_spi_write_aai, @@ -624,6 +628,8 @@ .max_data_write = FIFO_SIZE_YANGTZE - 3, .command = spi100_spi_send_command, .multicommand = default_spi_send_multicommand, + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .read = promontory_read_memmapped, .write_256 = default_spi_write_256, .write_aai = default_spi_write_aai, diff --git a/serprog.c b/serprog.c index 12d15d7..a3a3db3 100644 --- a/serprog.c +++ b/serprog.c @@ -438,7 +438,23 @@ return 0; }
+static void *serprog_map(const char *descr, uintptr_t phys_addr, size_t len) +{ + /* Serprog transmits 24 bits only and assumes the underlying implementation handles any remaining bits + * correctly (usually setting them to one either in software (for FWH/LPC) or relying on the fact that + * the hardware observes a subset of the address bits only). Combined with the standard mapping of + * flashrom this creates a 16 MB-wide window just below the 4 GB boundary where serprog can operate (as + * needed for non-SPI chips). Below we make sure that the requested range is within this window. */ + if ((phys_addr & 0xFF000000) == 0xFF000000) { + return (void*)phys_addr; + } + msg_pwarn(MSGHEADER "requested mapping %s is incompatible: 0x%zx bytes at 0x%0*" PRIxPTR ".\n", + descr, len, PRIxPTR_WIDTH, phys_addr); + return NULL; +} + static struct spi_master spi_master_serprog = { + .map_flash_region = serprog_map, .features = SPI_MASTER_4BA, .max_data_read = MAX_DATA_READ_UNLIMITED, .max_data_write = MAX_DATA_WRITE_UNLIMITED, @@ -553,6 +569,7 @@ }
static const struct par_master par_master_serprog = { + .map_flash_region = serprog_map, .chip_readb = serprog_chip_readb, .chip_readw = fallback_chip_readw, .chip_readl = fallback_chip_readl, @@ -945,27 +962,11 @@ sp_prev_was_write = 0; }
-static void *serprog_map(const char *descr, uintptr_t phys_addr, size_t len) -{ - /* Serprog transmits 24 bits only and assumes the underlying implementation handles any remaining bits - * correctly (usually setting them to one either in software (for FWH/LPC) or relying on the fact that - * the hardware observes a subset of the address bits only). Combined with the standard mapping of - * flashrom this creates a 16 MB-wide window just below the 4 GB boundary where serprog can operate (as - * needed for non-SPI chips). Below we make sure that the requested range is within this window. */ - if ((phys_addr & 0xFF000000) == 0xFF000000) { - return (void*)phys_addr; - } - msg_pwarn(MSGHEADER "requested mapping %s is incompatible: 0x%zx bytes at 0x%0*" PRIxPTR ".\n", - descr, len, PRIxPTR_WIDTH, phys_addr); - return NULL; -} - const struct programmer_entry programmer_serprog = { .name = "serprog", .type = OTHER, /* FIXME */ .devs.note = "All programmer devices speaking the serprog protocol\n", .init = serprog_init, - .map_flash_region = serprog_map, .delay = serprog_delay, }; diff --git a/wbsio_spi.c b/wbsio_spi.c index 95ca1e9..661ecbb 100644 --- a/wbsio_spi.c +++ b/wbsio_spi.c @@ -187,6 +187,8 @@ .max_data_write = MAX_DATA_UNSPECIFIED, .command = wbsio_spi_send_command, .multicommand = default_spi_send_multicommand, + .map_flash_region = physmap, + .unmap_flash_region = physunmap, .read = wbsio_spi_read, .write_256 = spi_chip_write_1, .write_aai = spi_chip_write_1,
-
Angel Pons (Code Review)