This depends on [PATCH] Add struct flashctx * everywhere
All programmer types (Parallel, SPI, Opaque) now register themselves into a generic programmer list and probing is now programmer-centric instead of chip-centric. Registering multiple SPI/... masters at the same time is now possible without any problems. Handling multiple flash chips is still unchanged, but now we have the infrastructure to deal with "dual BIOS" and "one flash behind southbridge and one flash behind EC" sanely.
A nice side effect is that this patch kills quite a few global variables and improves the situation for libflashrom.
Hint for developers: struct {spi,par,opaque}_programmer now have a void *data pointer to store any additional programmer-specific data, e.g. hardware configuratoin info.
I'd appreciate tests for the following programmer classes: - mainboard native SPI (ICH SPI, SB600 SPI, VIA SPI) - mainboard native LPC/FWH/Parallel - mainboard native bitbanged SPI (MCP SPI) - mainboard translated Parallel (SuperI/O acts as LPC->Parallel translator) - mainboard translated SPI (SuperI/O acts as LPC->SPI translator) - serprog - external SPI (Bus Pirate/Dediprog/FT2232) - external bitbanged SPI (nicintel_spi, ogp_spi) - external LPC/Parallel (satasii, atahpt, nic3com, nicintel, ...)
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
--- flashrom-register_all_programmers_register_generic/flash.h (Arbeitskopie) +++ flashrom-register_all_programmers_register_generic/flash.h (Arbeitskopie) @@ -171,6 +171,7 @@ chipaddr virtual_memory; /* Some flash devices have an additional register space. */ chipaddr virtual_registers; + struct registered_programmer *pgm; };
#define TEST_UNTESTED 0 @@ -224,14 +225,13 @@ write_gran_1byte, write_gran_256bytes, }; -extern enum chipbustype buses_supported; extern int verbose; extern const char flashrom_version[]; extern 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(int startchip, struct flashctx *fill_flash, int force); +int probe_flash(struct registered_programmer *pgm, int startchip, struct flashctx *fill_flash, int force); int read_flash_to_file(struct flashctx *flash, const char *filename); int min(int a, int b); int max(int a, int b); @@ -300,2 +300,3 @@
+enum chipbustype get_buses_supported(void); #endif /* !__FLASH_H__ */ --- flashrom-register_all_programmers_register_generic/bitbang_spi.c (Arbeitskopie) +++ flashrom-register_all_programmers_register_generic/bitbang_spi.c (Arbeitskopie) @@ -25,46 +25,43 @@ #include "programmer.h" #include "spi.h"
-/* Length of half a clock period in usecs. */ -static int bitbang_spi_half_period; - -static const struct bitbang_spi_master *bitbang_spi_master = NULL; - /* Note that CS# is active low, so val=0 means the chip is active. */ -static void bitbang_spi_set_cs(int val) +static void bitbang_spi_set_cs(const const struct bitbang_spi_master *master, int val) { - bitbang_spi_master->set_cs(val); + master->set_cs(val); }
-static void bitbang_spi_set_sck(int val) +static void bitbang_spi_set_sck(const const struct bitbang_spi_master *master, int val) { - bitbang_spi_master->set_sck(val); + master->set_sck(val); }
-static void bitbang_spi_set_mosi(int val) +static void bitbang_spi_set_mosi(const const struct bitbang_spi_master *master, int val) { - bitbang_spi_master->set_mosi(val); + master->set_mosi(val); }
-static int bitbang_spi_get_miso(void) +static int bitbang_spi_get_miso(const const struct bitbang_spi_master *master) { - return bitbang_spi_master->get_miso(); + return master->get_miso(); }
-static void bitbang_spi_request_bus(void) +static void bitbang_spi_request_bus(const const struct bitbang_spi_master *master) { - if (bitbang_spi_master->request_bus) - bitbang_spi_master->request_bus(); + if (master->request_bus) + master->request_bus(); }
-static void bitbang_spi_release_bus(void) +static void bitbang_spi_release_bus(const const struct bitbang_spi_master *master) { - if (bitbang_spi_master->release_bus) - bitbang_spi_master->release_bus(); + if (master->release_bus) + master->release_bus(); }
-static int bitbang_spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, - const unsigned char *writearr, unsigned char *readarr); +static int bitbang_spi_send_command(struct flashctx *flash, + unsigned int writecnt, unsigned int readcnt, + const unsigned char *writearr, + unsigned char *readarr);
static const struct spi_programmer spi_programmer_bitbang = { .type = SPI_CONTROLLER_BITBANG, @@ -76,8 +73,9 @@ .write_256 = default_spi_write_256, };
-int bitbang_spi_init(const struct bitbang_spi_master *master, int halfperiod) +int bitbang_spi_init(const struct bitbang_spi_master *master) { + struct spi_programmer pgm = spi_programmer_bitbang; /* BITBANG_SPI_INVALID is 0, so if someone forgot to initialize ->type, * we catch it here. Same goes for missing initialization of bitbanging * functions. @@ -88,80 +86,76 @@ "Please report a bug at flashrom@flashrom.org\n"); return 1; } - if (bitbang_spi_master) { - msg_perr("SPI bitbang master already initialized!\n" - "Please report a bug at flashrom@flashrom.org\n"); - return 1; - }
- bitbang_spi_master = master; - bitbang_spi_half_period = halfperiod; + pgm.data = master; + register_spi_programmer(&pgm);
- register_spi_programmer(&spi_programmer_bitbang); - - /* FIXME: Run bitbang_spi_request_bus here or in programmer init? */ - bitbang_spi_set_cs(1); - bitbang_spi_set_sck(0); - bitbang_spi_set_mosi(0); + /* Only mess with the bus if we're sure nobody else uses it. */ + bitbang_spi_request_bus(master); + bitbang_spi_set_cs(master, 1); + bitbang_spi_set_sck(master, 0); + bitbang_spi_set_mosi(master, 0); + /* FIXME: Release SPI bus here and request it again for each command or + * don't release it now and only release it on programmer shutdown? + */ + bitbang_spi_release_bus(master); return 0; }
int bitbang_spi_shutdown(const struct bitbang_spi_master *master) { - if (!bitbang_spi_master) { + if (!master) { msg_perr("Shutting down an uninitialized SPI bitbang master!\n" "Please report a bug at flashrom@flashrom.org\n"); return 1; } - if (master != bitbang_spi_master) { - msg_perr("Shutting down a mismatched SPI bitbang master!\n" - "Please report a bug at flashrom@flashrom.org\n"); - return 1; - }
/* FIXME: Run bitbang_spi_release_bus here or per command? */ - bitbang_spi_master = NULL; return 0; }
-static uint8_t bitbang_spi_readwrite_byte(uint8_t val) +static uint8_t bitbang_spi_rw_byte(const struct bitbang_spi_master *master, + uint8_t val) { uint8_t ret = 0; int i;
for (i = 7; i >= 0; i--) { - bitbang_spi_set_mosi((val >> i) & 1); - programmer_delay(bitbang_spi_half_period); - bitbang_spi_set_sck(1); + bitbang_spi_set_mosi(master, (val >> i) & 1); + programmer_delay(master->half_period); + bitbang_spi_set_sck(master, 1); ret <<= 1; - ret |= bitbang_spi_get_miso(); - programmer_delay(bitbang_spi_half_period); - bitbang_spi_set_sck(0); + ret |= bitbang_spi_get_miso(master); + programmer_delay(master->half_period); + bitbang_spi_set_sck(master, 0); } return ret; }
-static int bitbang_spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, - const unsigned char *writearr, unsigned char *readarr) +static int bitbang_spi_send_command(struct flashctx *flash, + unsigned int writecnt, unsigned int readcnt, + const unsigned char *writearr, + unsigned char *readarr) { int i; + const struct bitbang_spi_master *master = flash->pgm->spi.data;
/* FIXME: Run bitbang_spi_request_bus here or in programmer init? * Requesting and releasing the SPI bus is handled in here to allow the * programmer to use its own SPI engine for native accesses. */ - bitbang_spi_request_bus(); - bitbang_spi_set_cs(0); + bitbang_spi_request_bus(master); + bitbang_spi_set_cs(master, 0); for (i = 0; i < writecnt; i++) - bitbang_spi_readwrite_byte(writearr[i]); + bitbang_spi_rw_byte(master, writearr[i]); for (i = 0; i < readcnt; i++) - readarr[i] = bitbang_spi_readwrite_byte(0); + readarr[i] = bitbang_spi_rw_byte(master, 0);
- programmer_delay(bitbang_spi_half_period); - bitbang_spi_set_cs(1); - programmer_delay(bitbang_spi_half_period); + programmer_delay(master->half_period); + bitbang_spi_set_cs(master, 1); + programmer_delay(master->half_period); /* FIXME: Run bitbang_spi_release_bus here or in programmer init? */ - bitbang_spi_release_bus(); + bitbang_spi_release_bus(master);
return 0; } --- flashrom-register_all_programmers_register_generic/ichspi.c (Arbeitskopie) +++ flashrom-register_all_programmers_register_generic/ichspi.c (Arbeitskopie) @@ -635,7 +635,7 @@
/* Read len bytes from the fdata/spid register into the data array. * - * Note that using len > spi_programmer->max_data_read will return garbage or + * Note that using len > flash->pgm->spi.max_data_read will return garbage or * may even crash. */ static void ich_read_data(uint8_t *data, int len, int reg0_off) @@ -653,7 +653,7 @@
/* Fill len bytes from the data array into the fdata/spid registers. * - * Note that using len > spi_programmer->max_data_write will trash the registers + * Note that using len > flash->pgm->spi.max_data_write will trash the registers * following the data registers. */ static void ich_fill_data(const uint8_t *data, int len, int reg0_off) @@ -960,9 +960,9 @@ uint8_t datalength, uint8_t * data) { /* max_data_read == max_data_write for all Intel/VIA SPI masters */ - uint8_t maxlength = spi_programmer->max_data_read; + uint8_t maxlength = flash->pgm->spi.max_data_read;
- if (spi_programmer->type == SPI_CONTROLLER_NONE) { + if (ich_generation == CHIPSET_ICH_UNKNOWN) { msg_perr("%s: unsupported chipset\n", __func__); return -1; } @@ -1296,7 +1296,7 @@ REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS));
while (len > 0) { - block_len = min(len, opaque_programmer->max_data_read); + block_len = min(len, flash->pgm->opaque.max_data_read); ich_hwseq_set_addr(addr); hsfc = REGREAD16(ICH9_REG_HSFC); hsfc &= ~HSFC_FCYCLE; /* set read operation */ @@ -1335,7 +1335,7 @@
while (len > 0) { ich_hwseq_set_addr(addr); - block_len = min(len, opaque_programmer->max_data_write); + block_len = min(len, flash->pgm->opaque.max_data_write); ich_fill_data(buf, block_len, ICH9_REG_FDATA0); hsfc = REGREAD16(ICH9_REG_HSFC); hsfc &= ~HSFC_FCYCLE; /* clear operation */ --- flashrom-register_all_programmers_register_generic/spi25.c (Arbeitskopie) +++ flashrom-register_all_programmers_register_generic/spi25.c (Arbeitskopie) @@ -177,7 +177,7 @@ /* Some SPI controllers do not support commands with writecnt=1 and * readcnt=4. */ - switch (spi_programmer->type) { + switch (flash->pgm->spi.type) { #if CONFIG_INTERNAL == 1 #if defined(__i386__) || defined(__x86_64__) case SPI_CONTROLLER_IT87XX: @@ -1103,7 +1103,7 @@ .readarr = NULL, }};
- switch (spi_programmer->type) { + switch (flash->pgm->spi.type) { #if CONFIG_INTERNAL == 1 #if defined(__i386__) || defined(__x86_64__) case SPI_CONTROLLER_IT87XX: --- flashrom-register_all_programmers_register_generic/spi.c (Arbeitskopie) +++ flashrom-register_all_programmers_register_generic/spi.c (Arbeitskopie) @@ -30,41 +30,30 @@ #include "programmer.h" #include "spi.h"
-const struct spi_programmer spi_programmer_none = { - .type = SPI_CONTROLLER_NONE, - .max_data_read = MAX_DATA_UNSPECIFIED, - .max_data_write = MAX_DATA_UNSPECIFIED, - .command = NULL, - .multicommand = NULL, - .read = NULL, - .write_256 = NULL, -}; - -const struct spi_programmer *spi_programmer = &spi_programmer_none; - int spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) { - if (!spi_programmer->command) { + if (!flash->pgm->spi.command) { msg_perr("%s called, but SPI is unsupported on this " "hardware. Please report a bug at " "flashrom@flashrom.org\n", __func__); return 1; }
- return spi_programmer->command(flash, writecnt, readcnt, writearr, readarr); + return flash->pgm->spi.command(flash, writecnt, readcnt, + writearr, readarr); }
int spi_send_multicommand(struct flashctx *flash, struct spi_command *cmds) { - if (!spi_programmer->multicommand) { + if (!flash->pgm->spi.multicommand) { msg_perr("%s called, but SPI is unsupported on this " "hardware. Please report a bug at " "flashrom@flashrom.org\n", __func__); return 1; }
- return spi_programmer->multicommand(flash, cmds); + return flash->pgm->spi.multicommand(flash, cmds); }
int default_spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, @@ -98,7 +87,7 @@
int default_spi_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - unsigned int max_data = spi_programmer->max_data_read; + unsigned int max_data = flash->pgm->spi.max_data_read; if (max_data == MAX_DATA_UNSPECIFIED) { msg_perr("%s called, but SPI read chunk size not defined " "on this hardware. Please report a bug at " @@ -110,7 +99,7 @@
int default_spi_write_256(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - unsigned int max_data = spi_programmer->max_data_write; + unsigned int max_data = flash->pgm->spi.max_data_write; if (max_data == MAX_DATA_UNSPECIFIED) { msg_perr("%s called, but SPI write chunk size not defined " "on this hardware. Please report a bug at " @@ -123,7 +112,7 @@ int spi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { unsigned int addrbase = 0; - if (!spi_programmer->read) { + if (!flash->pgm->spi.read) { msg_perr("%s called, but SPI read is unsupported on this " "hardware. Please report a bug at " "flashrom@flashrom.org\n", __func__); @@ -149,7 +138,7 @@ "access window.\n"); msg_perr("Read will probably return garbage.\n"); } - return spi_programmer->read(flash, buf, addrbase + start, len); + return flash->pgm->spi.read(flash, buf, addrbase + start, len); }
/* @@ -161,14 +150,14 @@ /* real chunksize is up to 256, logical chunksize is 256 */ int spi_chip_write_256(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - if (!spi_programmer->write_256) { + if (!flash->pgm->spi.write_256) { msg_perr("%s called, but SPI page write is unsupported on this " "hardware. Please report a bug at " "flashrom@flashrom.org\n", __func__); return 1; }
- return spi_programmer->write_256(flash, buf, start, len); + return flash->pgm->spi.write_256(flash, buf, start, len); }
/* @@ -178,7 +167,7 @@ */ uint32_t spi_get_valid_read_addr(struct flashctx *flash) { - switch (spi_programmer->type) { + switch (flash->pgm->spi.type) { #if CONFIG_INTERNAL == 1 #if defined(__i386__) || defined(__x86_64__) case SPI_CONTROLLER_ICH7: @@ -195,4 +184,7 @@ { - spi_programmer = pgm; - buses_supported |= BUS_SPI; + struct registered_programmer rpgm; + + rpgm.buses_supported = BUS_SPI; + rpgm.spi = *pgm; + register_programmer(&rpgm); } --- flashrom-register_all_programmers_register_generic/programmer.c (Arbeitskopie) +++ flashrom-register_all_programmers_register_generic/programmer.c (Arbeitskopie) @@ -21,19 +21,6 @@ #include "flash.h" #include "programmer.h"
-static const struct par_programmer par_programmer_none = { - .chip_readb = noop_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, - .chip_writeb = noop_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, -}; - -const struct par_programmer *par_programmer = &par_programmer_none; - /* No-op shutdown() for programmers which don't need special handling */ int noop_shutdown(void) { @@ -115,4 +102,37 @@ { - par_programmer = pgm; - buses_supported |= buses; + struct registered_programmer rpgm; + + rpgm.buses_supported = buses; + rpgm.par = *pgm; + register_programmer(&rpgm); +} + +/* The limit of 4 is totally arbitrary. */ +#define PROGRAMMERS_MAX 4 +struct registered_programmer registered_programmers[PROGRAMMERS_MAX]; +int registered_programmer_count = 0; + +/* This function copies the struct registered_programmer parameter. */ +int register_programmer(struct registered_programmer *pgm) +{ + if (registered_programmer_count >= PROGRAMMERS_MAX) { + msg_perr("Tried to register more than %i programmer " + "interfaces.\n", PROGRAMMERS_MAX); + return 1; + } + registered_programmers[registered_programmer_count] = *pgm; + registered_programmer_count++; + + return 0; +} + +enum chipbustype get_buses_supported(void) +{ + int i; + enum chipbustype ret = BUS_NONE; + + for (i = 0; i < registered_programmer_count; i++) + ret |= registered_programmers[i].buses_supported; + + return ret; } --- flashrom-register_all_programmers_register_generic/flashrom.c (Arbeitskopie) +++ flashrom-register_all_programmers_register_generic/flashrom.c (Arbeitskopie) @@ -46,9 +46,6 @@
static char *programmer_param = NULL;
-/* Supported buses for the current programmer. */ -enum chipbustype buses_supported; - /* * Programmers supporting multiple buses can have differing size limits on * each bus. Store the limits for each bus in a common struct. @@ -314,7 +311,6 @@ .fwh = 0xffffffff, .spi = 0xffffffff, }; - buses_supported = BUS_NONE; /* Default to top aligned flash at 4 GB. */ flashbase = 0; /* Registering shutdown functions is now allowed. */ @@ -361,42 +357,42 @@
void chip_writeb(const struct flashctx *flash, uint8_t val, chipaddr addr) { - par_programmer->chip_writeb(flash, val, addr); + flash->pgm->par.chip_writeb(flash, val, addr); }
void chip_writew(const struct flashctx *flash, uint16_t val, chipaddr addr) { - par_programmer->chip_writew(flash, val, addr); + flash->pgm->par.chip_writew(flash, val, addr); }
void chip_writel(const struct flashctx *flash, uint32_t val, chipaddr addr) { - par_programmer->chip_writel(flash, val, addr); + flash->pgm->par.chip_writel(flash, val, addr); }
void chip_writen(const struct flashctx *flash, uint8_t *buf, chipaddr addr, size_t len) { - par_programmer->chip_writen(flash, buf, addr, len); + flash->pgm->par.chip_writen(flash, buf, addr, len); }
uint8_t chip_readb(const struct flashctx *flash, const chipaddr addr) { - return par_programmer->chip_readb(flash, addr); + return flash->pgm->par.chip_readb(flash, addr); }
uint16_t chip_readw(const struct flashctx *flash, const chipaddr addr) { - return par_programmer->chip_readw(flash, addr); + return flash->pgm->par.chip_readw(flash, addr); }
uint32_t chip_readl(const struct flashctx *flash, const chipaddr addr) { - return par_programmer->chip_readl(flash, addr); + return flash->pgm->par.chip_readl(flash, addr); }
void chip_readn(const struct flashctx *flash, uint8_t *buf, chipaddr addr, size_t len) { - par_programmer->chip_readn(flash, buf, addr, len); + flash->pgm->par.chip_readn(flash, buf, addr, len); }
void programmer_delay(int usecs) @@ -938,7 +934,8 @@ return 1; }
-int probe_flash(int startchip, struct flashctx *fill_flash, int force) +int probe_flash(struct registered_programmer *pgm, int startchip, + struct flashctx *fill_flash, int force) { const struct flashchip *flash; unsigned long base = 0; @@ -950,11 +947,12 @@ for (flash = flashchips + startchip; flash && flash->name; flash++) { if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0) continue; - buses_common = buses_supported & flash->bustype; + buses_common = pgm->buses_supported & flash->bustype; if (!buses_common) { +#if 0 // Does not really make sense anymore if we use a programmer-centric walk. msg_gspew("Probing for %s %s, %d kB: skipped. ", flash->vendor, flash->name, flash->total_size); - tmp = flashbuses_to_text(buses_supported); + tmp = flashbuses_to_text(get_buses_supported()); msg_gspew("Host bus type %s ", tmp); free(tmp); tmp = flashbuses_to_text(flash->bustype); @@ -962,6 +960,7 @@ tmp); free(tmp); msg_gspew("\n"); +#endif continue; } msg_gdbg("Probing for %s %s, %d kB: ", @@ -977,6 +976,7 @@
/* Start filling in the dynamic data. */ memcpy(fill_flash, flash, sizeof(struct flashchip)); + fill_flash->pgm = pgm;
base = flashbase ? flashbase : (0xffffffff - size + 1); fill_flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size); --- flashrom-register_all_programmers_register_generic/programmer.h (Arbeitskopie) +++ flashrom-register_all_programmers_register_generic/programmer.h (Arbeitskopie) @@ -133,6 +133,8 @@ int (*get_miso) (void); void (*request_bus) (void); void (*release_bus) (void); + /* Length of half a clock period in usecs. */ + unsigned int half_period; };
#if CONFIG_INTERNAL == 1 @@ -208,6 +210,7 @@
#if NEED_PCI == 1 /* pcidev.c */ +// FIXME: These need to be local, not global extern uint32_t io_base_addr; extern struct pci_access *pacc; extern struct pci_dev *pcidev_dev; @@ -427,7 +430,7 @@ #endif
/* bitbang_spi.c */ -int bitbang_spi_init(const struct bitbang_spi_master *master, int halfperiod); +int bitbang_spi_init(const struct bitbang_spi_master *master); int bitbang_spi_shutdown(const struct bitbang_spi_master *master);
/* buspirate_spi.c */ @@ -452,6 +455,7 @@ uint32_t fwh; uint32_t spi; }; +// FIXME: These need to be local, not global extern struct decode_sizes max_rom_decode; extern int programmer_may_write; extern unsigned long flashbase; @@ -498,7 +502,6 @@ SPI_CONTROLLER_SERPROG, #endif }; -extern const int spi_programmer_count;
#define MAX_DATA_UNSPECIFIED 0 #define MAX_DATA_READ_UNLIMITED 64 * 1024 @@ -514,9 +517,9 @@ /* Optimized functions for this programmer */ int (*read)(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*write_256)(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); + void *data; };
-extern const struct spi_programmer *spi_programmer; int default_spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int default_spi_send_multicommand(struct flashctx *flash, struct spi_command *cmds); @@ -570,8 +573,8 @@ int (*read) (struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*write) (struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*erase) (struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen); + void *data; }; -extern const struct opaque_programmer *opaque_programmer; void register_opaque_programmer(const struct opaque_programmer *pgm);
/* programmer.c */ @@ -595,9 +598,20 @@ uint16_t (*chip_readw) (const struct flashctx *flash, const chipaddr addr); uint32_t (*chip_readl) (const struct flashctx *flash, const chipaddr addr); void (*chip_readn) (const struct flashctx *flash, uint8_t *buf, const chipaddr addr, size_t len); + void *data; }; -extern const struct par_programmer *par_programmer; void register_par_programmer(const struct par_programmer *pgm, const enum chipbustype buses); +struct registered_programmer { + enum chipbustype buses_supported; + union { + struct par_programmer par; + struct spi_programmer spi; + struct opaque_programmer opaque; + }; +}; +extern struct registered_programmer registered_programmers[]; +extern int registered_programmer_count; +int register_programmer(struct registered_programmer *pgm);
/* serprog.c */ #if CONFIG_SERPROG == 1 --- flashrom-register_all_programmers_register_generic/ogp_spi.c (Revision 1473) +++ flashrom-register_all_programmers_register_generic/ogp_spi.c (Arbeitskopie) @@ -91,6 +91,8 @@ .get_miso = ogp_bitbang_get_miso, .request_bus = ogp_request_spibus, .release_bus = ogp_release_spibus, + /* no delay for now. */ + .half_period = 0, };
static int ogp_spi_shutdown(void *data) @@ -136,8 +138,7 @@ if (register_shutdown(ogp_spi_shutdown, NULL)) return 1;
- /* no delay for now. */ - if (bitbang_spi_init(&bitbang_spi_master_ogp, 0)) + if (bitbang_spi_init(&bitbang_spi_master_ogp)) return 1;
return 0; --- flashrom-register_all_programmers_register_generic/cli_classic.c (Revision 1473) +++ flashrom-register_all_programmers_register_generic/cli_classic.c (Arbeitskopie) @@ -172,7 +172,7 @@ struct flashctx flashes[3]; struct flashctx *fill_flash; const char *name; - int namelen, opt, i; + int namelen, opt, i, j; int startchip = 0, chipcount = 0, option_index = 0, force = 0; #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; @@ -444,17 +444,21 @@ ret = 1; goto out_shutdown; } - tempstr = flashbuses_to_text(buses_supported); + tempstr = flashbuses_to_text(get_buses_supported()); msg_pdbg("This programmer supports the following protocols: %s.\n", tempstr); free(tempstr);
- for (i = 0; i < ARRAY_SIZE(flashes); i++) { - startchip = probe_flash(startchip, &flashes[i], 0); - if (startchip == -1) - break; - chipcount++; - startchip++; + for (j = 0; j < registered_programmer_count; j++) { + startchip = 0; + for (i = 0; i < ARRAY_SIZE(flashes); i++) { + startchip = probe_flash(®istered_programmers[j], + startchip, &flashes[i], 0); + if (startchip == -1) + break; + chipcount++; + startchip++; + } }
if (chipcount > 1) { @@ -472,6 +476,7 @@ printf("Note: flashrom can never write if the flash " "chip isn't found automatically.\n"); } +#if 0 // FIXME: What happens for a forced chip read if multiple compatible programmers are registered? if (force && read_it && chip_to_probe) { printf("Force read (-f -r -c) requested, pretending " "the chip is there:\n"); @@ -486,6 +491,7 @@ "contain garbage.\n"); return read_flash_to_file(&flashes[0], filename); } +#endif ret = 1; goto out_shutdown; } else if (!chip_to_probe) { @@ -502,7 +508,7 @@ check_chip_supported(fill_flash);
size = fill_flash->total_size * 1024; - if (check_max_decode((buses_supported & fill_flash->bustype), size) && + if (check_max_decode((get_buses_supported() & fill_flash->bustype), size) && (!force)) { fprintf(stderr, "Chip is too big for this programmer " "(-V gives details). Use --force to override.\n"); --- flashrom-register_all_programmers_register_generic/nicintel_spi.c (Revision 1473) +++ flashrom-register_all_programmers_register_generic/nicintel_spi.c (Arbeitskopie) @@ -137,6 +137,8 @@ .get_miso = nicintel_bitbang_get_miso, .request_bus = nicintel_request_spibus, .release_bus = nicintel_release_spibus, + /* 1 usec halfperiod delay for now. */ + .half_period = 1, };
static int nicintel_spi_shutdown(void *data) @@ -181,8 +183,7 @@ if (register_shutdown(nicintel_spi_shutdown, NULL)) return 1;
- /* 1 usec halfperiod delay for now. */ - if (bitbang_spi_init(&bitbang_spi_master_nicintel, 1)) + if (bitbang_spi_init(&bitbang_spi_master_nicintel)) return 1;
return 0; --- flashrom-register_all_programmers_register_generic/opaque.c (Revision 1473) +++ flashrom-register_all_programmers_register_generic/opaque.c (Arbeitskopie) @@ -30,70 +30,62 @@ #include "chipdrivers.h" #include "programmer.h"
-const struct opaque_programmer opaque_programmer_none = { - .max_data_read = MAX_DATA_UNSPECIFIED, - .max_data_write = MAX_DATA_UNSPECIFIED, - .probe = NULL, - .read = NULL, - .write = NULL, - .erase = NULL, -}; - -const struct opaque_programmer *opaque_programmer = &opaque_programmer_none; - int probe_opaque(struct flashctx *flash) { - if (!opaque_programmer->probe) { + if (!flash->pgm->opaque.probe) { msg_perr("%s called before register_opaque_programmer. " "Please report a bug at flashrom@flashrom.org\n", __func__); return 0; }
- return opaque_programmer->probe(flash); + return flash->pgm->opaque.probe(flash); }
int read_opaque(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - if (!opaque_programmer->read) { + if (!flash->pgm->opaque.read) { msg_perr("%s called before register_opaque_programmer. " "Please report a bug at flashrom@flashrom.org\n", __func__); return 1; } - return opaque_programmer->read(flash, buf, start, len); + return flash->pgm->opaque.read(flash, buf, start, len); }
int write_opaque(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - if (!opaque_programmer->write) { + if (!flash->pgm->opaque.write) { msg_perr("%s called before register_opaque_programmer. " "Please report a bug at flashrom@flashrom.org\n", __func__); return 1; } - return opaque_programmer->write(flash, buf, start, len); + return flash->pgm->opaque.write(flash, buf, start, len); }
int erase_opaque(struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen) { - if (!opaque_programmer->erase) { + if (!flash->pgm->opaque.erase) { msg_perr("%s called before register_opaque_programmer. " "Please report a bug at flashrom@flashrom.org\n", __func__); return 1; } - return opaque_programmer->erase(flash, blockaddr, blocklen); + return flash->pgm->opaque.erase(flash, blockaddr, blocklen); }
void register_opaque_programmer(const struct opaque_programmer *pgm) { + struct registered_programmer rpgm; + if (!pgm->probe || !pgm->read || !pgm->write || !pgm->erase) { msg_perr("%s called with one of probe/read/write/erase being " "NULL. Please report a bug at flashrom@flashrom.org\n", __func__); return; } - opaque_programmer = pgm; - buses_supported |= BUS_PROG; + rpgm.buses_supported = BUS_PROG; + rpgm.opaque = *pgm; + register_programmer(&rpgm); } --- flashrom-register_all_programmers_register_generic/rayer_spi.c (Revision 1473) +++ flashrom-register_all_programmers_register_generic/rayer_spi.c (Arbeitskopie) @@ -92,6 +92,8 @@ .set_sck = rayer_bitbang_set_sck, .set_mosi = rayer_bitbang_set_mosi, .get_miso = rayer_bitbang_get_miso, + /* Zero halfperiod delay. */ + .half_period = 0, };
int rayer_spi_init(void) @@ -171,8 +173,7 @@ /* Get the initial value before writing to any line. */ lpt_outbyte = INB(lpt_iobase);
- /* Zero halfperiod delay. */ - if (bitbang_spi_init(&bitbang_spi_master_rayer, 0)) + if (bitbang_spi_init(&bitbang_spi_master_rayer)) return 1;
return 0; --- flashrom-register_all_programmers_register_generic/mcp6x_spi.c (Revision 1473) +++ flashrom-register_all_programmers_register_generic/mcp6x_spi.c (Arbeitskopie) @@ -98,6 +98,8 @@ .get_miso = mcp6x_bitbang_get_miso, .request_bus = mcp6x_request_spibus, .release_bus = mcp6x_release_spibus, + /* Zero halfperiod delay. */ + .half_period = 0, };
int mcp6x_spi_init(int want_spi) @@ -159,8 +161,7 @@ (status >> MCP6X_SPI_GRANT) & 0x1); mcp_gpiostate = status & 0xff;
- /* Zero halfperiod delay. */ - if (bitbang_spi_init(&bitbang_spi_master_mcp6x, 0)) { + if (bitbang_spi_init(&bitbang_spi_master_mcp6x)) { /* This should never happen. */ msg_perr("MCP6X bitbang SPI master init failed!\n"); return 1;
Am Samstag, den 17.12.2011, 15:23 +0100 schrieb Carl-Daniel Hailfinger:
This depends on [PATCH] Add struct flashctx * everywhere
But not on the revision you posted on December 16, 14:41 CET.
A patch that is rebased to that revision and fixes a compiler warning (which gets fatal due to -Werror) is attached.
- mainboard native SPI (ICH SPI, SB600 SPI, VIA SPI)
ICH7 SPI works for reading. Currently I don't have no hardware at hand to test other stuff.
int spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) {
- if (!spi_programmer->command) {
- if (!flash->pgm->spi.command) { msg_perr("%s called, but SPI is unsupported on this " "hardware. Please report a bug at " "flashrom@flashrom.org\n", __func__); return 1; }
I suggest to drop the NULL pointer check here, and just do it once in register_spi_programmer (refusing to call register_programmer if any function pointer is NULL) Same for the other functions in here.
-static const struct par_programmer par_programmer_none = {
.chip_readb = noop_chip_readb,.chip_readw = fallback_chip_readw,.chip_readl = fallback_chip_readl,.chip_readn = fallback_chip_readn,.chip_writeb = noop_chip_writeb,.chip_writew = fallback_chip_writew,.chip_writel = fallback_chip_writel,.chip_writen = fallback_chip_writen,-};
I suggest to drop noop_chip_readb/noop_chip_writeb at the same time as we drop par_programmer_none, as these functions are no longer used.
+enum chipbustype get_buses_supported(void) +{
- int i;
- enum chipbustype ret = BUS_NONE;
- for (i = 0; i < registered_programmer_count; i++)
ret |= registered_programmers[i].buses_supported;- return ret;
}
Looking at this function, just keep in mind it returns a list of busses supported by all the programmers found.
+#if 0 // Does not really make sense anymore if we use a programmer-centric walk. msg_gspew("Probing for %s %s, %d kB: skipped. ", flash->vendor, flash->name, flash->total_size);
tmp = flashbuses_to_text(buses_supported);
tmp = flashbuses_to_text(get_buses_supported()); msg_gspew("Host bus type %s ", tmp);
If we reinstate this code (i.e. decide to not #if 0 it), don't use get_buses_supported(), but pgm->buses_supported. Also don't call it "Host bus type", but "programmer bus type"
--- flashrom-register_all_programmers_register_generic/ogp_spi.c (Revision 1473) +++ flashrom-register_all_programmers_register_generic/ogp_spi.c (Arbeitskopie) @@ -91,6 +91,8 @@ .get_miso = ogp_bitbang_get_miso, .request_bus = ogp_request_spibus, .release_bus = ogp_release_spibus,
- /* no delay for now. */
- .half_period = 0,
Is this comment really useful? Opposed to where it was used before, in this place, the name of the setting "half_period" is explicitly mentioned.
- tempstr = flashbuses_to_text(get_buses_supported()); msg_pdbg("This programmer supports the following protocols: %s.\n", tempstr);
It's definitely not "this programmer", as should be obvious if you "kept in mind" as I told you that get_buses_supportet returns the union of all programmers. So either move this into the loop over all the programmers and print programmer specific bus types, or call it something like "The programmers in this systems provide the following protocols: %s.\n"
- /* 1 usec halfperiod delay for now. */
- .half_period = 1,
Same comment as above. If you want to have the unit visible, rename the variable to half_period_usecs, instead of adding comments of questionable information content.
No ack yet because of the non-appliacation of your patch and the use of get_buses_supported where a specific programmer bus list would make more sense. The "superflous comment" issue, the "NULL pointer on each spi call" issue, and the "orphaned noop" issue are not something that blocks acking the patch.
Regards, Michael Karcher
Hi Michael,
thanks a lot for your in-depth review!
Am 18.12.2011 14:22 schrieb Michael Karcher:
Am Samstag, den 17.12.2011, 15:23 +0100 schrieb Carl-Daniel Hailfinger:
This depends on [PATCH] Add struct flashctx * everywhere
But not on the revision you posted on December 16, 14:41 CET.
Indeed, I posted the flashctx version which didn't obey the 80 column limit, but my local tree (against which the registration diff was run) already had that fixed. Thanks for noticing and for wrangling the patch into a state where it applied.
A patch that is rebased to that revision and fixes a compiler warning (which gets fatal due to -Werror) is attached.
Used those changes as basis for my reworked patch. Did you introduce any functional changes except the compiler warning fix?
- mainboard native SPI (ICH SPI, SB600 SPI, VIA SPI)
ICH7 SPI works for reading. Currently I don't have no hardware at hand to test other stuff.
Well, so at least that works. That's encouraging.
int spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) {
- if (!spi_programmer->command) {
- if (!flash->pgm->spi.command) { msg_perr("%s called, but SPI is unsupported on this " "hardware. Please report a bug at " "flashrom@flashrom.org\n", __func__); return 1; }
I suggest to drop the NULL pointer check here, and just do it once in register_spi_programmer (refusing to call register_programmer if any function pointer is NULL) Same for the other functions in here.
Good point, done.
I don't see a good way to move the max_data_read check in default_spi_read to an init function, though, because you can never be sure at init time if default_spi_read will be called.
-static const struct par_programmer par_programmer_none = {
.chip_readb = noop_chip_readb,.chip_readw = fallback_chip_readw,.chip_readl = fallback_chip_readl,.chip_readn = fallback_chip_readn,.chip_writeb = noop_chip_writeb,.chip_writew = fallback_chip_writew,.chip_writel = fallback_chip_writel,.chip_writen = fallback_chip_writen,-};
I suggest to drop noop_chip_readb/noop_chip_writeb at the same time as we drop par_programmer_none, as these functions are no longer used.
noop_chip_readb dropped. noop_chip_writeb may be useful if we merge a programmer driver which does not support write. What do you think?
+enum chipbustype get_buses_supported(void) +{
- int i;
- enum chipbustype ret = BUS_NONE;
- for (i = 0; i < registered_programmer_count; i++)
ret |= registered_programmers[i].buses_supported;- return ret;
}
Looking at this function, just keep in mind it returns a list of busses supported by all the programmers found.
Indeed.
+#if 0 // Does not really make sense anymore if we use a programmer-centric walk. msg_gspew("Probing for %s %s, %d kB: skipped. ", flash->vendor, flash->name, flash->total_size);
tmp = flashbuses_to_text(buses_supported);
tmp = flashbuses_to_text(get_buses_supported()); msg_gspew("Host bus type %s ", tmp);If we reinstate this code (i.e. decide to not #if 0 it), don't use get_buses_supported(), but pgm->buses_supported. Also don't call it "Host bus type", but "programmer bus type"
Killed.
--- flashrom-register_all_programmers_register_generic/ogp_spi.c (Revision 1473) +++ flashrom-register_all_programmers_register_generic/ogp_spi.c (Arbeitskopie) @@ -91,6 +91,8 @@ .get_miso = ogp_bitbang_get_miso, .request_bus = ogp_request_spibus, .release_bus = ogp_release_spibus,
- /* no delay for now. */
- .half_period = 0,
Is this comment really useful? Opposed to where it was used before, in this place, the name of the setting "half_period" is explicitly mentioned.
Killed everywhere.
- tempstr = flashbuses_to_text(get_buses_supported()); msg_pdbg("This programmer supports the following protocols: %s.\n", tempstr);
It's definitely not "this programmer", as should be obvious if you "kept in mind" as I told you that get_buses_supportet returns the union of all programmers.
I beg to differ. This is a message to the end user, and the end user does not think of a mainboard as multiple programmers. The usage of the word "programmer" in our code is inconsistent: Sometimes it refers to a whole device with multiple masters/controllers, sometimes it only refers to a single master (usually in the register_something context). We could rename all register_foo_programmer to register_foo_master to get more clarity in the code. What do you think?
So either move this into the loop over all the programmers and print programmer specific bus types, or call it something like "The programmers in this systems provide the following protocols: %s.\n"
See above.
- /* 1 usec halfperiod delay for now. */
- .half_period = 1,
Same comment as above. If you want to have the unit visible, rename the variable to half_period_usecs, instead of adding comments of questionable information content.
Indeed. probe_timing in struct flashchip doesn't have _usecs in the name either, so I just decided to drop it.
New patch against svn HEAD.
Notes: ichspi.c: Use ich_generation instead of spi_programmer->type (or flash->pgm->spi.type) in run_opcode to determine if the programmer was initialized correctly. This change is debatable because it uses a local static variable instead of the info available in flash->pgm, but OTOH now the internal usage is consistent. Another question is whether we should really check for correct initialization at this point. There are two init functions (for ICH and VIA), both are bug-free in that regard and it's not clear whether guarding against future bugs in those (or and additional) init functions is worth the additional code. generic observations: The programmer registration functions now return error/success instead of void, but their callers don't care. Fix now or later?
TODO? - max_decode is a programmer property, add it to the register_*_programmer parameters - programmer_may_write is a programmer property, add it to the register_*_programmer parameters
All programmer types (Parallel, SPI, Opaque) now register themselves into a generic programmer list and probing is now programmer-centric instead of chip-centric. Registering multiple SPI/... masters at the same time is now possible without any problems. Handling multiple flash chips is still unchanged, but now we have the infrastructure to deal with "dual BIOS" and "one flash behind southbridge and one flash behind EC" sanely.
A nice side effect is that this patch kills quite a few global variables and improves the situation for libflashrom.
Hint for developers: struct {spi,par,opaque}_programmer now have a void *data pointer to store any additional programmer-specific data, e.g. hardware configuration info.
I'd appreciate tests for the following programmer classes: - mainboard native SPI (ICH SPI, SB600 SPI, VIA SPI) - mainboard native LPC/FWH/Parallel - mainboard native bitbanged SPI (MCP SPI) - mainboard translated Parallel (SuperI/O acts as LPC->Parallel translator) - mainboard translated SPI (SuperI/O acts as LPC->SPI translator) - serprog - external SPI (Bus Pirate/Dediprog/FT2232) - external bitbanged SPI (nicintel_spi, ogp_spi) - external LPC/Parallel (satasii, atahpt, nic3com, nicintel, ...)
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-register_all_programmers_register_generic/ogp_spi.c =================================================================== --- flashrom-register_all_programmers_register_generic/ogp_spi.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/ogp_spi.c (Arbeitskopie) @@ -91,6 +91,7 @@ .get_miso = ogp_bitbang_get_miso, .request_bus = ogp_request_spibus, .release_bus = ogp_release_spibus, + .half_period = 0, };
static int ogp_spi_shutdown(void *data) @@ -136,8 +137,7 @@ if (register_shutdown(ogp_spi_shutdown, NULL)) return 1;
- /* no delay for now. */ - if (bitbang_spi_init(&bitbang_spi_master_ogp, 0)) + if (bitbang_spi_init(&bitbang_spi_master_ogp)) return 1;
return 0; Index: flashrom-register_all_programmers_register_generic/flash.h =================================================================== --- flashrom-register_all_programmers_register_generic/flash.h (Revision 1474) +++ flashrom-register_all_programmers_register_generic/flash.h (Arbeitskopie) @@ -171,6 +171,7 @@ chipaddr virtual_memory; /* Some flash devices have an additional register space. */ chipaddr virtual_registers; + struct registered_programmer *pgm; };
#define TEST_UNTESTED 0 @@ -224,14 +225,13 @@ write_gran_1byte, write_gran_256bytes, }; -extern enum chipbustype buses_supported; extern int verbose; extern const char flashrom_version[]; extern 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(int startchip, struct flashctx *fill_flash, int force); +int probe_flash(struct registered_programmer *pgm, int startchip, struct flashctx *fill_flash, int force); int read_flash_to_file(struct flashctx *flash, const char *filename); int min(int a, int b); int max(int a, int b); @@ -256,6 +256,13 @@
/* Something happened that shouldn't happen, we'll abort. */ #define ERROR_FATAL -0xee +#define ERROR_FLASHROM_BUG -200 +/* We reached one of the hardcoded limits of flashrom. This can be fixed by + * increasing the limit of a compile-time allocation or by switching to dynamic + * allocation. + * Note: If this warning is triggered, check first for runaway registrations. + */ +#define ERROR_FLASHROM_LIMIT -201
/* cli_output.c */ /* Let gcc and clang check for correct printf-style format strings. */ @@ -297,4 +304,5 @@ int spi_send_multicommand(struct flashctx *flash, struct spi_command *cmds); uint32_t spi_get_valid_read_addr(struct flashctx *flash);
+enum chipbustype get_buses_supported(void); #endif /* !__FLASH_H__ */ Index: flashrom-register_all_programmers_register_generic/bitbang_spi.c =================================================================== --- flashrom-register_all_programmers_register_generic/bitbang_spi.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/bitbang_spi.c (Arbeitskopie) @@ -25,42 +25,37 @@ #include "programmer.h" #include "spi.h"
-/* Length of half a clock period in usecs. */ -static int bitbang_spi_half_period; - -static const struct bitbang_spi_master *bitbang_spi_master = NULL; - /* Note that CS# is active low, so val=0 means the chip is active. */ -static void bitbang_spi_set_cs(int val) +static void bitbang_spi_set_cs(const const struct bitbang_spi_master *master, int val) { - bitbang_spi_master->set_cs(val); + master->set_cs(val); }
-static void bitbang_spi_set_sck(int val) +static void bitbang_spi_set_sck(const const struct bitbang_spi_master *master, int val) { - bitbang_spi_master->set_sck(val); + master->set_sck(val); }
-static void bitbang_spi_set_mosi(int val) +static void bitbang_spi_set_mosi(const const struct bitbang_spi_master *master, int val) { - bitbang_spi_master->set_mosi(val); + master->set_mosi(val); }
-static int bitbang_spi_get_miso(void) +static int bitbang_spi_get_miso(const const struct bitbang_spi_master *master) { - return bitbang_spi_master->get_miso(); + return master->get_miso(); }
-static void bitbang_spi_request_bus(void) +static void bitbang_spi_request_bus(const const struct bitbang_spi_master *master) { - if (bitbang_spi_master->request_bus) - bitbang_spi_master->request_bus(); + if (master->request_bus) + master->request_bus(); }
-static void bitbang_spi_release_bus(void) +static void bitbang_spi_release_bus(const const struct bitbang_spi_master *master) { - if (bitbang_spi_master->release_bus) - bitbang_spi_master->release_bus(); + if (master->release_bus) + master->release_bus(); }
static int bitbang_spi_send_command(struct flashctx *flash, @@ -78,67 +73,63 @@ .write_256 = default_spi_write_256, };
-int bitbang_spi_init(const struct bitbang_spi_master *master, int halfperiod) +int bitbang_spi_init(const struct bitbang_spi_master *master) { + struct spi_programmer pgm = spi_programmer_bitbang; /* BITBANG_SPI_INVALID is 0, so if someone forgot to initialize ->type, * we catch it here. Same goes for missing initialization of bitbanging * functions. */ if (!master || master->type == BITBANG_SPI_INVALID || !master->set_cs || - !master->set_sck || !master->set_mosi || !master->get_miso) { + !master->set_sck || !master->set_mosi || !master->get_miso || + (master->request_bus && !master->release_bus) || + (!master->request_bus && master->release_bus)) { msg_perr("Incomplete SPI bitbang master setting!\n" "Please report a bug at flashrom@flashrom.org\n"); - return 1; + return ERROR_FLASHROM_BUG; } - if (bitbang_spi_master) { - msg_perr("SPI bitbang master already initialized!\n" - "Please report a bug at flashrom@flashrom.org\n"); - return 1; - }
- bitbang_spi_master = master; - bitbang_spi_half_period = halfperiod; + pgm.data = master; + register_spi_programmer(&pgm);
- register_spi_programmer(&spi_programmer_bitbang); - - /* FIXME: Run bitbang_spi_request_bus here or in programmer init? */ - bitbang_spi_set_cs(1); - bitbang_spi_set_sck(0); - bitbang_spi_set_mosi(0); + /* Only mess with the bus if we're sure nobody else uses it. */ + bitbang_spi_request_bus(master); + bitbang_spi_set_cs(master, 1); + bitbang_spi_set_sck(master, 0); + bitbang_spi_set_mosi(master, 0); + /* FIXME: Release SPI bus here and request it again for each command or + * don't release it now and only release it on programmer shutdown? + */ + bitbang_spi_release_bus(master); return 0; }
int bitbang_spi_shutdown(const struct bitbang_spi_master *master) { - if (!bitbang_spi_master) { + if (!master) { msg_perr("Shutting down an uninitialized SPI bitbang master!\n" "Please report a bug at flashrom@flashrom.org\n"); return 1; } - if (master != bitbang_spi_master) { - msg_perr("Shutting down a mismatched SPI bitbang master!\n" - "Please report a bug at flashrom@flashrom.org\n"); - return 1; - }
/* FIXME: Run bitbang_spi_release_bus here or per command? */ - bitbang_spi_master = NULL; return 0; }
-static uint8_t bitbang_spi_readwrite_byte(uint8_t val) +static uint8_t bitbang_spi_rw_byte(const struct bitbang_spi_master *master, + uint8_t val) { uint8_t ret = 0; int i;
for (i = 7; i >= 0; i--) { - bitbang_spi_set_mosi((val >> i) & 1); - programmer_delay(bitbang_spi_half_period); - bitbang_spi_set_sck(1); + bitbang_spi_set_mosi(master, (val >> i) & 1); + programmer_delay(master->half_period); + bitbang_spi_set_sck(master, 1); ret <<= 1; - ret |= bitbang_spi_get_miso(); - programmer_delay(bitbang_spi_half_period); - bitbang_spi_set_sck(0); + ret |= bitbang_spi_get_miso(master); + programmer_delay(master->half_period); + bitbang_spi_set_sck(master, 0); } return ret; } @@ -149,23 +140,24 @@ unsigned char *readarr) { int i; + const struct bitbang_spi_master *master = flash->pgm->spi.data;
/* FIXME: Run bitbang_spi_request_bus here or in programmer init? * Requesting and releasing the SPI bus is handled in here to allow the * programmer to use its own SPI engine for native accesses. */ - bitbang_spi_request_bus(); - bitbang_spi_set_cs(0); + bitbang_spi_request_bus(master); + bitbang_spi_set_cs(master, 0); for (i = 0; i < writecnt; i++) - bitbang_spi_readwrite_byte(writearr[i]); + bitbang_spi_rw_byte(master, writearr[i]); for (i = 0; i < readcnt; i++) - readarr[i] = bitbang_spi_readwrite_byte(0); + readarr[i] = bitbang_spi_rw_byte(master, 0);
- programmer_delay(bitbang_spi_half_period); - bitbang_spi_set_cs(1); - programmer_delay(bitbang_spi_half_period); + programmer_delay(master->half_period); + bitbang_spi_set_cs(master, 1); + programmer_delay(master->half_period); /* FIXME: Run bitbang_spi_release_bus here or in programmer init? */ - bitbang_spi_release_bus(); + bitbang_spi_release_bus(master);
return 0; } Index: flashrom-register_all_programmers_register_generic/cli_classic.c =================================================================== --- flashrom-register_all_programmers_register_generic/cli_classic.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/cli_classic.c (Arbeitskopie) @@ -172,7 +172,7 @@ struct flashctx flashes[3]; struct flashctx *fill_flash; const char *name; - int namelen, opt, i; + int namelen, opt, i, j; int startchip = 0, chipcount = 0, option_index = 0, force = 0; #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; @@ -444,17 +444,21 @@ ret = 1; goto out_shutdown; } - tempstr = flashbuses_to_text(buses_supported); + tempstr = flashbuses_to_text(get_buses_supported()); msg_pdbg("This programmer supports the following protocols: %s.\n", tempstr); free(tempstr);
- for (i = 0; i < ARRAY_SIZE(flashes); i++) { - startchip = probe_flash(startchip, &flashes[i], 0); - if (startchip == -1) - break; - chipcount++; - startchip++; + for (j = 0; j < registered_programmer_count; j++) { + startchip = 0; + for (i = 0; i < ARRAY_SIZE(flashes); i++) { + startchip = probe_flash(®istered_programmers[j], + startchip, &flashes[i], 0); + if (startchip == -1) + break; + chipcount++; + startchip++; + } }
if (chipcount > 1) { @@ -472,6 +476,7 @@ printf("Note: flashrom can never write if the flash " "chip isn't found automatically.\n"); } +#if 0 // FIXME: What happens for a forced chip read if multiple compatible programmers are registered? if (force && read_it && chip_to_probe) { printf("Force read (-f -r -c) requested, pretending " "the chip is there:\n"); @@ -486,6 +491,7 @@ "contain garbage.\n"); return read_flash_to_file(&flashes[0], filename); } +#endif ret = 1; goto out_shutdown; } else if (!chip_to_probe) { @@ -502,7 +508,7 @@ check_chip_supported(fill_flash);
size = fill_flash->total_size * 1024; - if (check_max_decode((buses_supported & fill_flash->bustype), size) && + if (check_max_decode(fill_flash->pgm->buses_supported & fill_flash->bustype, size) && (!force)) { fprintf(stderr, "Chip is too big for this programmer " "(-V gives details). Use --force to override.\n"); Index: flashrom-register_all_programmers_register_generic/ichspi.c =================================================================== --- flashrom-register_all_programmers_register_generic/ichspi.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/ichspi.c (Arbeitskopie) @@ -635,7 +635,7 @@
/* Read len bytes from the fdata/spid register into the data array. * - * Note that using len > spi_programmer->max_data_read will return garbage or + * Note that using len > flash->pgm->spi.max_data_read will return garbage or * may even crash. */ static void ich_read_data(uint8_t *data, int len, int reg0_off) @@ -653,7 +653,7 @@
/* Fill len bytes from the data array into the fdata/spid registers. * - * Note that using len > spi_programmer->max_data_write will trash the registers + * Note that using len > flash->pgm->spi.max_data_write will trash the registers * following the data registers. */ static void ich_fill_data(const uint8_t *data, int len, int reg0_off) @@ -960,9 +960,9 @@ uint8_t datalength, uint8_t * data) { /* max_data_read == max_data_write for all Intel/VIA SPI masters */ - uint8_t maxlength = spi_programmer->max_data_read; + uint8_t maxlength = flash->pgm->spi.max_data_read;
- if (spi_programmer->type == SPI_CONTROLLER_NONE) { + if (ich_generation == CHIPSET_ICH_UNKNOWN) { msg_perr("%s: unsupported chipset\n", __func__); return -1; } @@ -1297,7 +1297,7 @@ REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS));
while (len > 0) { - block_len = min(len, opaque_programmer->max_data_read); + block_len = min(len, flash->pgm->opaque.max_data_read); ich_hwseq_set_addr(addr); hsfc = REGREAD16(ICH9_REG_HSFC); hsfc &= ~HSFC_FCYCLE; /* set read operation */ @@ -1336,7 +1336,7 @@
while (len > 0) { ich_hwseq_set_addr(addr); - block_len = min(len, opaque_programmer->max_data_write); + block_len = min(len, flash->pgm->opaque.max_data_write); ich_fill_data(buf, block_len, ICH9_REG_FDATA0); hsfc = REGREAD16(ICH9_REG_HSFC); hsfc &= ~HSFC_FCYCLE; /* clear operation */ Index: flashrom-register_all_programmers_register_generic/nicintel_spi.c =================================================================== --- flashrom-register_all_programmers_register_generic/nicintel_spi.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/nicintel_spi.c (Arbeitskopie) @@ -137,6 +137,7 @@ .get_miso = nicintel_bitbang_get_miso, .request_bus = nicintel_request_spibus, .release_bus = nicintel_release_spibus, + .half_period = 1, };
static int nicintel_spi_shutdown(void *data) @@ -181,8 +182,7 @@ if (register_shutdown(nicintel_spi_shutdown, NULL)) return 1;
- /* 1 usec halfperiod delay for now. */ - if (bitbang_spi_init(&bitbang_spi_master_nicintel, 1)) + if (bitbang_spi_init(&bitbang_spi_master_nicintel)) return 1;
return 0; Index: flashrom-register_all_programmers_register_generic/opaque.c =================================================================== --- flashrom-register_all_programmers_register_generic/opaque.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/opaque.c (Arbeitskopie) @@ -30,70 +30,37 @@ #include "chipdrivers.h" #include "programmer.h"
-const struct opaque_programmer opaque_programmer_none = { - .max_data_read = MAX_DATA_UNSPECIFIED, - .max_data_write = MAX_DATA_UNSPECIFIED, - .probe = NULL, - .read = NULL, - .write = NULL, - .erase = NULL, -}; - -const struct opaque_programmer *opaque_programmer = &opaque_programmer_none; - int probe_opaque(struct flashctx *flash) { - if (!opaque_programmer->probe) { - msg_perr("%s called before register_opaque_programmer. " - "Please report a bug at flashrom@flashrom.org\n", - __func__); - return 0; - } - - return opaque_programmer->probe(flash); + return flash->pgm->opaque.probe(flash); }
int read_opaque(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - if (!opaque_programmer->read) { - msg_perr("%s called before register_opaque_programmer. " - "Please report a bug at flashrom@flashrom.org\n", - __func__); - return 1; - } - return opaque_programmer->read(flash, buf, start, len); + return flash->pgm->opaque.read(flash, buf, start, len); }
int write_opaque(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - if (!opaque_programmer->write) { - msg_perr("%s called before register_opaque_programmer. " - "Please report a bug at flashrom@flashrom.org\n", - __func__); - return 1; - } - return opaque_programmer->write(flash, buf, start, len); + return flash->pgm->opaque.write(flash, buf, start, len); }
int erase_opaque(struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen) { - if (!opaque_programmer->erase) { - msg_perr("%s called before register_opaque_programmer. " - "Please report a bug at flashrom@flashrom.org\n", - __func__); - return 1; - } - return opaque_programmer->erase(flash, blockaddr, blocklen); + return flash->pgm->opaque.erase(flash, blockaddr, blocklen); }
-void register_opaque_programmer(const struct opaque_programmer *pgm) +int register_opaque_programmer(const struct opaque_programmer *pgm) { + struct registered_programmer rpgm; + if (!pgm->probe || !pgm->read || !pgm->write || !pgm->erase) { msg_perr("%s called with one of probe/read/write/erase being " "NULL. Please report a bug at flashrom@flashrom.org\n", __func__); - return; + return ERROR_FLASHROM_BUG; } - opaque_programmer = pgm; - buses_supported |= BUS_PROG; + rpgm.buses_supported = BUS_PROG; + rpgm.opaque = *pgm; + return register_programmer(&rpgm); } Index: flashrom-register_all_programmers_register_generic/rayer_spi.c =================================================================== --- flashrom-register_all_programmers_register_generic/rayer_spi.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/rayer_spi.c (Arbeitskopie) @@ -92,6 +92,7 @@ .set_sck = rayer_bitbang_set_sck, .set_mosi = rayer_bitbang_set_mosi, .get_miso = rayer_bitbang_get_miso, + .half_period = 0, };
int rayer_spi_init(void) @@ -171,8 +172,7 @@ /* Get the initial value before writing to any line. */ lpt_outbyte = INB(lpt_iobase);
- /* Zero halfperiod delay. */ - if (bitbang_spi_init(&bitbang_spi_master_rayer, 0)) + if (bitbang_spi_init(&bitbang_spi_master_rayer)) return 1;
return 0; Index: flashrom-register_all_programmers_register_generic/spi25.c =================================================================== --- flashrom-register_all_programmers_register_generic/spi25.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/spi25.c (Arbeitskopie) @@ -179,7 +179,7 @@ /* Some SPI controllers do not support commands with writecnt=1 and * readcnt=4. */ - switch (spi_programmer->type) { + switch (flash->pgm->spi.type) { #if CONFIG_INTERNAL == 1 #if defined(__i386__) || defined(__x86_64__) case SPI_CONTROLLER_IT87XX: @@ -1120,7 +1120,7 @@ .readarr = NULL, }};
- switch (spi_programmer->type) { + switch (flash->pgm->spi.type) { #if CONFIG_INTERNAL == 1 #if defined(__i386__) || defined(__x86_64__) case SPI_CONTROLLER_IT87XX: Index: flashrom-register_all_programmers_register_generic/spi.c =================================================================== --- flashrom-register_all_programmers_register_generic/spi.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/spi.c (Arbeitskopie) @@ -1,7 +1,7 @@ /* * This file is part of the flashrom project. * - * Copyright (C) 2007, 2008, 2009 Carl-Daniel Hailfinger + * Copyright (C) 2007, 2008, 2009, 2010, 2011 Carl-Daniel Hailfinger * Copyright (C) 2008 coresystems GmbH * * This program is free software; you can redistribute it and/or modify @@ -30,43 +30,17 @@ #include "programmer.h" #include "spi.h"
-const struct spi_programmer spi_programmer_none = { - .type = SPI_CONTROLLER_NONE, - .max_data_read = MAX_DATA_UNSPECIFIED, - .max_data_write = MAX_DATA_UNSPECIFIED, - .command = NULL, - .multicommand = NULL, - .read = NULL, - .write_256 = NULL, -}; - -const struct spi_programmer *spi_programmer = &spi_programmer_none; - int spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) { - if (!spi_programmer->command) { - msg_perr("%s called, but SPI is unsupported on this " - "hardware. Please report a bug at " - "flashrom@flashrom.org\n", __func__); - return 1; - } - - return spi_programmer->command(flash, writecnt, readcnt, writearr, + return flash->pgm->spi.command(flash, writecnt, readcnt, writearr, readarr); }
int spi_send_multicommand(struct flashctx *flash, struct spi_command *cmds) { - if (!spi_programmer->multicommand) { - msg_perr("%s called, but SPI is unsupported on this " - "hardware. Please report a bug at " - "flashrom@flashrom.org\n", __func__); - return 1; - } - - return spi_programmer->multicommand(flash, cmds); + return flash->pgm->spi.multicommand(flash, cmds); }
int default_spi_send_command(struct flashctx *flash, unsigned int writecnt, @@ -104,7 +78,7 @@ int default_spi_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - unsigned int max_data = spi_programmer->max_data_read; + unsigned int max_data = flash->pgm->spi.max_data_read; if (max_data == MAX_DATA_UNSPECIFIED) { msg_perr("%s called, but SPI read chunk size not defined " "on this hardware. Please report a bug at " @@ -117,7 +91,7 @@ int default_spi_write_256(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - unsigned int max_data = spi_programmer->max_data_write; + unsigned int max_data = flash->pgm->spi.max_data_write; if (max_data == MAX_DATA_UNSPECIFIED) { msg_perr("%s called, but SPI write chunk size not defined " "on this hardware. Please report a bug at " @@ -131,12 +105,6 @@ unsigned int len) { unsigned int addrbase = 0; - if (!spi_programmer->read) { - msg_perr("%s called, but SPI read is unsupported on this " - "hardware. Please report a bug at " - "flashrom@flashrom.org\n", __func__); - return 1; - }
/* Check if the chip fits between lowest valid and highest possible * address. Highest possible address with the current SPI implementation @@ -157,7 +125,7 @@ "access window.\n"); msg_perr("Read will probably return garbage.\n"); } - return spi_programmer->read(flash, buf, addrbase + start, len); + return flash->pgm->spi.read(flash, buf, addrbase + start, len); }
/* @@ -170,14 +138,7 @@ int spi_chip_write_256(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - if (!spi_programmer->write_256) { - msg_perr("%s called, but SPI page write is unsupported on this " - "hardware. Please report a bug at " - "flashrom@flashrom.org\n", __func__); - return 1; - } - - return spi_programmer->write_256(flash, buf, start, len); + return flash->pgm->spi.write_256(flash, buf, start, len); }
/* @@ -187,7 +148,7 @@ */ uint32_t spi_get_valid_read_addr(struct flashctx *flash) { - switch (spi_programmer->type) { + switch (flash->pgm->spi.type) { #if CONFIG_INTERNAL == 1 #if defined(__i386__) || defined(__x86_64__) case SPI_CONTROLLER_ICH7: @@ -200,8 +161,22 @@ } }
-void register_spi_programmer(const struct spi_programmer *pgm) +int register_spi_programmer(const struct spi_programmer *pgm) { - spi_programmer = pgm; - buses_supported |= BUS_SPI; + struct registered_programmer rpgm; + + if (!pgm->write_256 || !pgm->read || !pgm->command || + !pgm->multicommand || + ((pgm->command == default_spi_send_command) && + (pgm->multicommand == default_spi_send_multicommand))) { + msg_perr("%s called with inconsistent settings. " + "Please report a bug at flashrom@flashrom.org\n", + __func__); + return ERROR_FLASHROM_BUG; + } + + + rpgm.buses_supported = BUS_SPI; + rpgm.spi = *pgm; + return register_programmer(&rpgm); } Index: flashrom-register_all_programmers_register_generic/mcp6x_spi.c =================================================================== --- flashrom-register_all_programmers_register_generic/mcp6x_spi.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/mcp6x_spi.c (Arbeitskopie) @@ -98,6 +98,7 @@ .get_miso = mcp6x_bitbang_get_miso, .request_bus = mcp6x_request_spibus, .release_bus = mcp6x_release_spibus, + .half_period = 0, };
int mcp6x_spi_init(int want_spi) @@ -159,8 +160,7 @@ (status >> MCP6X_SPI_GRANT) & 0x1); mcp_gpiostate = status & 0xff;
- /* Zero halfperiod delay. */ - if (bitbang_spi_init(&bitbang_spi_master_mcp6x, 0)) { + if (bitbang_spi_init(&bitbang_spi_master_mcp6x)) { /* This should never happen. */ msg_perr("MCP6X bitbang SPI master init failed!\n"); return 1; Index: flashrom-register_all_programmers_register_generic/programmer.c =================================================================== --- flashrom-register_all_programmers_register_generic/programmer.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/programmer.c (Arbeitskopie) @@ -21,19 +21,6 @@ #include "flash.h" #include "programmer.h"
-static const struct par_programmer par_programmer_none = { - .chip_readb = noop_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, - .chip_writeb = noop_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, -}; - -const struct par_programmer *par_programmer = &par_programmer_none; - /* No-op shutdown() for programmers which don't need special handling */ int noop_shutdown(void) { @@ -52,13 +39,7 @@ { }
-/* No-op chip_writeb() for drivers not supporting addr/data pair accesses */ -uint8_t noop_chip_readb(const struct flashctx *flash, const chipaddr addr) -{ - return 0xff; -} - -/* No-op chip_writeb() for drivers not supporting addr/data pair accesses */ +/* No-op chip_writeb() for parallel style drivers not supporting writes */ void noop_chip_writeb(const struct flashctx *flash, uint8_t val, chipaddr addr) { } @@ -115,8 +96,49 @@ return; }
-void register_par_programmer(const struct par_programmer *pgm, const enum chipbustype buses) +int register_par_programmer(const struct par_programmer *pgm, + const enum chipbustype buses) { - par_programmer = pgm; - buses_supported |= buses; + struct registered_programmer rpgm; + if (!chip_writeb || !chip_writew || !chip_writel || !chip_writen || + !chip_readb || !chip_readw || !chip_readl || !chip_readn) { + msg_perr("%s called with one of probe/read/write/erase being " + "NULL. Please report a bug at flashrom@flashrom.org\n", + __func__); + return ERROR_FLASHROM_BUG; + } + + rpgm.buses_supported = buses; + rpgm.par = *pgm; + return register_programmer(&rpgm); } + +/* The limit of 4 is totally arbitrary. */ +#define PROGRAMMERS_MAX 4 +struct registered_programmer registered_programmers[PROGRAMMERS_MAX]; +int registered_programmer_count = 0; + +/* This function copies the struct registered_programmer parameter. */ +int register_programmer(struct registered_programmer *pgm) +{ + if (registered_programmer_count >= PROGRAMMERS_MAX) { + msg_perr("Tried to register more than %i programmer " + "interfaces.\n", PROGRAMMERS_MAX); + return ERROR_FLASHROM_LIMIT; + } + registered_programmers[registered_programmer_count] = *pgm; + registered_programmer_count++; + + return 0; +} + +enum chipbustype get_buses_supported(void) +{ + int i; + enum chipbustype ret = BUS_NONE; + + for (i = 0; i < registered_programmer_count; i++) + ret |= registered_programmers[i].buses_supported; + + return ret; +} Index: flashrom-register_all_programmers_register_generic/flashrom.c =================================================================== --- flashrom-register_all_programmers_register_generic/flashrom.c (Revision 1474) +++ flashrom-register_all_programmers_register_generic/flashrom.c (Arbeitskopie) @@ -46,9 +46,6 @@
static char *programmer_param = NULL;
-/* Supported buses for the current programmer. */ -enum chipbustype buses_supported; - /* * Programmers supporting multiple buses can have differing size limits on * each bus. Store the limits for each bus in a common struct. @@ -314,7 +311,6 @@ .fwh = 0xffffffff, .spi = 0xffffffff, }; - buses_supported = BUS_NONE; /* Default to top aligned flash at 4 GB. */ flashbase = 0; /* Registering shutdown functions is now allowed. */ @@ -361,44 +357,44 @@
void chip_writeb(const struct flashctx *flash, uint8_t val, chipaddr addr) { - par_programmer->chip_writeb(flash, val, addr); + flash->pgm->par.chip_writeb(flash, val, addr); }
void chip_writew(const struct flashctx *flash, uint16_t val, chipaddr addr) { - par_programmer->chip_writew(flash, val, addr); + flash->pgm->par.chip_writew(flash, val, addr); }
void chip_writel(const struct flashctx *flash, uint32_t val, chipaddr addr) { - par_programmer->chip_writel(flash, val, addr); + flash->pgm->par.chip_writel(flash, val, addr); }
void chip_writen(const struct flashctx *flash, uint8_t *buf, chipaddr addr, size_t len) { - par_programmer->chip_writen(flash, buf, addr, len); + flash->pgm->par.chip_writen(flash, buf, addr, len); }
uint8_t chip_readb(const struct flashctx *flash, const chipaddr addr) { - return par_programmer->chip_readb(flash, addr); + return flash->pgm->par.chip_readb(flash, addr); }
uint16_t chip_readw(const struct flashctx *flash, const chipaddr addr) { - return par_programmer->chip_readw(flash, addr); + return flash->pgm->par.chip_readw(flash, addr); }
uint32_t chip_readl(const struct flashctx *flash, const chipaddr addr) { - return par_programmer->chip_readl(flash, addr); + return flash->pgm->par.chip_readl(flash, addr); }
void chip_readn(const struct flashctx *flash, uint8_t *buf, chipaddr addr, size_t len) { - par_programmer->chip_readn(flash, buf, addr, len); + flash->pgm->par.chip_readn(flash, buf, addr, len); }
void programmer_delay(int usecs) @@ -942,7 +938,8 @@ return 1; }
-int probe_flash(int startchip, struct flashctx *fill_flash, int force) +int probe_flash(struct registered_programmer *pgm, int startchip, + struct flashctx *fill_flash, int force) { const struct flashchip *flash; unsigned long base = 0; @@ -954,20 +951,9 @@ for (flash = flashchips + startchip; flash && flash->name; flash++) { if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0) continue; - buses_common = buses_supported & flash->bustype; - if (!buses_common) { - msg_gspew("Probing for %s %s, %d kB: skipped. ", - flash->vendor, flash->name, flash->total_size); - tmp = flashbuses_to_text(buses_supported); - msg_gspew("Host bus type %s ", tmp); - free(tmp); - tmp = flashbuses_to_text(flash->bustype); - msg_gspew("and chip bus type %s are incompatible.", - tmp); - free(tmp); - msg_gspew("\n"); + buses_common = pgm->buses_supported & flash->bustype; + if (!buses_common) continue; - } msg_gdbg("Probing for %s %s, %d kB: ", flash->vendor, flash->name, flash->total_size); if (!flash->probe && !force) { @@ -981,6 +967,7 @@
/* Start filling in the dynamic data. */ memcpy(fill_flash, flash, sizeof(struct flashchip)); + fill_flash->pgm = pgm;
base = flashbase ? flashbase : (0xffffffff - size + 1); fill_flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size); Index: flashrom-register_all_programmers_register_generic/programmer.h =================================================================== --- flashrom-register_all_programmers_register_generic/programmer.h (Revision 1474) +++ flashrom-register_all_programmers_register_generic/programmer.h (Arbeitskopie) @@ -133,6 +133,8 @@ int (*get_miso) (void); void (*request_bus) (void); void (*release_bus) (void); + /* Length of half a clock period in usecs. */ + unsigned int half_period; };
#if CONFIG_INTERNAL == 1 @@ -208,6 +210,7 @@
#if NEED_PCI == 1 /* pcidev.c */ +// FIXME: These need to be local, not global extern uint32_t io_base_addr; extern struct pci_access *pacc; extern struct pci_dev *pcidev_dev; @@ -427,7 +430,7 @@ #endif
/* bitbang_spi.c */ -int bitbang_spi_init(const struct bitbang_spi_master *master, int halfperiod); +int bitbang_spi_init(const struct bitbang_spi_master *master); int bitbang_spi_shutdown(const struct bitbang_spi_master *master);
/* buspirate_spi.c */ @@ -452,6 +455,7 @@ uint32_t fwh; uint32_t spi; }; +// FIXME: These need to be local, not global extern struct decode_sizes max_rom_decode; extern int programmer_may_write; extern unsigned long flashbase; @@ -498,7 +502,6 @@ SPI_CONTROLLER_SERPROG, #endif }; -extern const int spi_programmer_count;
#define MAX_DATA_UNSPECIFIED 0 #define MAX_DATA_READ_UNLIMITED 64 * 1024 @@ -514,15 +517,15 @@ /* Optimized functions for this programmer */ int (*read)(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*write_256)(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); + const void *data; };
-extern const struct spi_programmer *spi_programmer; int default_spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int default_spi_send_multicommand(struct flashctx *flash, struct spi_command *cmds); int default_spi_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int default_spi_write_256(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); -void register_spi_programmer(const struct spi_programmer *programmer); +int register_spi_programmer(const struct spi_programmer *programmer);
/* ichspi.c */ #if CONFIG_INTERNAL == 1 @@ -570,15 +573,14 @@ int (*read) (struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*write) (struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*erase) (struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen); + const void *data; }; -extern const struct opaque_programmer *opaque_programmer; -void register_opaque_programmer(const struct opaque_programmer *pgm); +int register_opaque_programmer(const struct opaque_programmer *pgm);
/* programmer.c */ int noop_shutdown(void); void *fallback_map(const char *descr, unsigned long phys_addr, size_t len); void fallback_unmap(void *virt_addr, size_t len); -uint8_t noop_chip_readb(const struct flashctx *flash, const chipaddr addr); void noop_chip_writeb(const struct flashctx *flash, uint8_t val, chipaddr addr); void fallback_chip_writew(const struct flashctx *flash, uint16_t val, chipaddr addr); void fallback_chip_writel(const struct flashctx *flash, uint32_t val, chipaddr addr); @@ -595,9 +597,20 @@ uint16_t (*chip_readw) (const struct flashctx *flash, const chipaddr addr); uint32_t (*chip_readl) (const struct flashctx *flash, const chipaddr addr); void (*chip_readn) (const struct flashctx *flash, uint8_t *buf, const chipaddr addr, size_t len); + const void *data; }; -extern const struct par_programmer *par_programmer; -void register_par_programmer(const struct par_programmer *pgm, const enum chipbustype buses); +int register_par_programmer(const struct par_programmer *pgm, const enum chipbustype buses); +struct registered_programmer { + enum chipbustype buses_supported; + union { + struct par_programmer par; + struct spi_programmer spi; + struct opaque_programmer opaque; + }; +}; +extern struct registered_programmer registered_programmers[]; +extern int registered_programmer_count; +int register_programmer(struct registered_programmer *pgm);
/* serprog.c */ #if CONFIG_SERPROG == 1
Am Montag, den 19.12.2011, 02:38 +0100 schrieb Carl-Daniel Hailfinger:
A patch that is rebased to that revision and fixes a compiler warning (which gets fatal due to -Werror) is attached.
Used those changes as basis for my reworked patch. Did you introduce any functional changes except the compiler warning fix?
If I remember correctly, apart from that warning fix I just shuffled whitespace around to fix the 80-column-wrap-mismatches.
I don't see a good way to move the max_data_read check in default_spi_read to an init function, though, because you can never be sure at init time if default_spi_read will be called.
Currently, default_spi_read is only called directly from the spi programmer structure, but we can't be sure a programmer decides to use the default implementation only sometimes, so agreed, we have to keep the checks.
I suggest to drop noop_chip_readb/noop_chip_writeb at the same time as we drop par_programmer_none, as these functions are no longer used.
noop_chip_readb dropped. noop_chip_writeb may be useful if we merge a programmer driver which does not support write. What do you think?
If we find a way to revive the "blind read via force", good idea. On the other hand, I think a opaque mmap dumper would be more apt for the blind read. It should just get the chip size and the optionally start or end address of the mapping as parameter.
msg_pdbg("This programmer supports the following protocols: %s.\n", tempstr);
It's definitely not "this programmer", as should be obvious if you "kept in mind" as I told you that get_buses_supportet returns the union of all programmers.
I beg to differ. This is a message to the end user, and the end user does not think of a mainboard as multiple programmers.
As on IRC: "The following protocols are supported".
ichspi.c: Use ich_generation instead of spi_programmer->type (or flash->pgm->spi.type) in run_opcode to determine if the programmer was initialized correctly. This change is debatable because it uses a local static variable instead of the info available in flash->pgm, but OTOH now the internal usage is consistent.
consistency first - we can eliminate the local variable later anyway (and we are going to, I expect)
Another question is whether we should really check for correct initialization at this point.
I guess the checking makes no sense anymore, as there is no way to enter this function without going through the init function.
The programmer registration functions now return error/success instead of void, but their callers don't care. Fix now or later?
Fix later. We already inform the user if we are losing programmers, that's enough for now.
TODO?
- max_decode is a programmer property, add it to the
register_*_programmer parameters
- programmer_may_write is a programmer property, add it to the
register_*_programmer parameters
Yes, both of them should be added to the programmer structure. I am unsure whether we want rarely used options (like may-not-write) as parameters though. Having the size there is good - it makes the programmer think about the maximum supported size.
All programmer types (Parallel, SPI, Opaque) now register themselves into a generic programmer list and probing is now programmer-centric instead of chip-centric.
TODO: split the flash chip list into SPI and non-SPI lists. No need to iterate over the other type and skip those chips because of bus mismatches. Do it later.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
int bitbang_spi_shutdown(const struct bitbang_spi_master *master) {
- if (!bitbang_spi_master) {
- if (!master) { msg_perr("Shutting down an uninitialized SPI bitbang master!\n" "Please report a bug at flashrom@flashrom.org\n"); return 1;
As I just found out, bitbang_spi_shutdown is never called at all, but if it will be called, it will be a callback registered in bitbang_spi_init which already checked master for nullness. Also the function could be made static in that case.
msg_pdbg("This programmer supports the following protocols: %s.\n", tempstr);
Message change: see intro.
if (!pgm->probe || !pgm->read || !pgm->write || !pgm->erase) { msg_perr("%s called with one of probe/read/write/erase being " "NULL. Please report a bug at flashrom@flashrom.org\n", __func__);
return;
return ERROR_FLASHROM_BUG;
Later, we could add a feature that allows read-only "programmers" that have write and erase NULL, setting programmer_may_write to FALSE.
-void register_spi_programmer(const struct spi_programmer *pgm) +int register_spi_programmer(const struct spi_programmer *pgm) {
- spi_programmer = pgm;
- buses_supported |= BUS_SPI;
- struct registered_programmer rpgm;
- if (!pgm->write_256 || !pgm->read || !pgm->command ||
!pgm->multicommand ||((pgm->command == default_spi_send_command) &&(pgm->multicommand == default_spi_send_multicommand))) {msg_perr("%s called with inconsistent settings. "
s/inconsistent settings/incomplete programmer definition/
- if (!chip_writeb || !chip_writew || !chip_writel || !chip_writen ||
!chip_readb || !chip_readw || !chip_readl || !chip_readn) {msg_perr("%s called with one of probe/read/write/erase being "
dito.
Regards, Michael Karcher
Am 19.12.2011 22:57 schrieb Michael Karcher:
Am Montag, den 19.12.2011, 02:38 +0100 schrieb Carl-Daniel Hailfinger:
I suggest to drop noop_chip_readb/noop_chip_writeb at the same time as we drop par_programmer_none, as these functions are no longer used.
noop_chip_readb dropped. noop_chip_writeb may be useful if we merge a programmer driver which does not support write. What do you think?
If we find a way to revive the "blind read via force", good idea. On the other hand, I think a opaque mmap dumper would be more apt for the blind read. It should just get the chip size and the optionally start or end address of the mapping as parameter.
I'll leave the #if 0 forced read code in cli_classic.c until we have a blind read solution again.
msg_pdbg("This programmer supports the following protocols: %s.\n", tempstr);
It's definitely not "this programmer", as should be obvious if you "kept in mind" as I told you that get_buses_supportet returns the union of all programmers.
I beg to differ. This is a message to the end user, and the end user does not think of a mainboard as multiple programmers.
As on IRC: "The following protocols are supported".
Done.
ichspi.c: Use ich_generation instead of spi_programmer->type (or flash->pgm->spi.type) in run_opcode to determine if the programmer was initialized correctly. This change is debatable because it uses a local static variable instead of the info available in flash->pgm, but OTOH now the internal usage is consistent.
consistency first - we can eliminate the local variable later anyway (and we are going to, I expect)
OK.
Another question is whether we should really check for correct initialization at this point.
I guess the checking makes no sense anymore, as there is no way to enter this function without going through the init function.
Hm yes, it would only serve as a check against buggy init functions. Looking at one of the recent ICH init additions in r1430 where new code was committed because it worked, but it was not really integrated well with existing ICH init code.
The programmer registration functions now return error/success instead of void, but their callers don't care. Fix now or later?
Fix later. We already inform the user if we are losing programmers, that's enough for now.
OK.
TODO?
- max_decode is a programmer property, add it to the
register_*_programmer parameters
- programmer_may_write is a programmer property, add it to the
register_*_programmer parameters
Yes, both of them should be added to the programmer structure. I am unsure whether we want rarely used options (like may-not-write) as parameters though. Having the size there is good - it makes the programmer think about the maximum supported size.
Postponed.
All programmer types (Parallel, SPI, Opaque) now register themselves into a generic programmer list and probing is now programmer-centric instead of chip-centric.
TODO: split the flash chip list into SPI and non-SPI lists. No need to iterate over the other type and skip those chips because of bus mismatches. Do it later.
Postponed, but given that flash chips may support multiple buses (some weird currently unsupported models even speak SPI and parallel), the reordering is not something I want to do.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks a lot for the review, committed in r1475 with the changes mentioned during review.
int bitbang_spi_shutdown(const struct bitbang_spi_master *master) {
- if (!bitbang_spi_master) {
- if (!master) { msg_perr("Shutting down an uninitialized SPI bitbang master!\n" "Please report a bug at flashrom@flashrom.org\n"); return 1;
As I just found out, bitbang_spi_shutdown is never called at all, but if it will be called, it will be a callback registered in bitbang_spi_init which already checked master for nullness. Also the function could be made static in that case.
Unfortunately, register_shutdown() takes a non-const void *parameter, and that causes warnings if it is called from bitbang_spi_init with const struct bitbang_spi_master *. Made static inside #if 0 for now until register_shutdown is changed.
msg_pdbg("This programmer supports the following protocols: %s.\n", tempstr);
Message change: see intro.
if (!pgm->probe || !pgm->read || !pgm->write || !pgm->erase) { msg_perr("%s called with one of probe/read/write/erase being " "NULL. Please report a bug at flashrom@flashrom.org\n", __func__);
return;
return ERROR_FLASHROM_BUG;Later, we could add a feature that allows read-only "programmers" that have write and erase NULL, setting programmer_may_write to FALSE.
Or we check for programmer_may_write==FALSE and then skip the erase/write member check.
-void register_spi_programmer(const struct spi_programmer *pgm) +int register_spi_programmer(const struct spi_programmer *pgm) {
- spi_programmer = pgm;
- buses_supported |= BUS_SPI;
- struct registered_programmer rpgm;
- if (!pgm->write_256 || !pgm->read || !pgm->command ||
!pgm->multicommand ||((pgm->command == default_spi_send_command) &&(pgm->multicommand == default_spi_send_multicommand))) {msg_perr("%s called with inconsistent settings. "s/inconsistent settings/incomplete programmer definition/
- if (!chip_writeb || !chip_writew || !chip_writel || !chip_writen ||
!chip_readb || !chip_readw || !chip_readl || !chip_readn) {msg_perr("%s called with one of probe/read/write/erase being "dito.
Done.
-
Carl-Daniel Hailfinger -
Michael Karcher