Am 26.10.2011 15:35 schrieb Stefan Tauner:
This allows them to be called before programmer registration.
i think it might be a good idea to get rid of the whole switch(spi_programmer->type) pattern and create a file-scope static ich_generation variable instead of using the type member and the ich_generation parameters everywhere.
Absolutely agreed. The only possible generic problem with that approach would be the registration of multiple ICH-style SPI programmers, but unless we see boards with two active ICH-style southbridges I think we can assume the static variable handles it well. (Note that boards with multiple MCP55 southbridges exist, but only one southbridge has an attached flash chip.)
There might be an issue for those who want to use flashrom as a standalone tool (e.g. on top of libpayload) where heap allocations for static variables are unwanted, but that's a huge can of worms and I'd rather ignore that issue until either static variables work well in that environment or until someone hacks a way around static variables being a problem there.
the type member is enough most of the time to derive the wanted information, but
- not always (e.g. ich_set_bbar)
- only available after registration, which we want to delay till the end of init.
- we really want to distinguish between chipset version-grained attributes which are not reflected by the registered programmer.
Indeed. So if you could rework the patch to use your static variable suggestion, it would reduce patch size and make the code more readable.
Regards, Carl-Daniel
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
ichspi.c | 44 +++++++++++++++++++++++++++----------------- 1 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/ichspi.c b/ichspi.c index afa420b..bc03554 100644 --- a/ichspi.c +++ b/ichspi.c @@ -225,8 +225,8 @@ static uint16_t REGREAD8(int X) /* Common SPI functions */ static int find_opcode(OPCODES *op, uint8_t opcode); static int find_preop(OPCODES *op, uint8_t preop); -static int generate_opcodes(OPCODES * op); -static int program_opcodes(OPCODES *op, int enable_undo); +static int generate_opcodes(int ich_generation, OPCODES * op); +static int program_opcodes(int ich_generation, OPCODES *op, int enable_undo); static int run_opcode(OPCODE op, uint32_t offset, uint8_t datalength, uint8_t * data);
@@ -410,7 +410,18 @@ static int reprogram_opcode_on_the_fly(uint8_t opcode, unsigned int writecnt, un int oppos=2; // use original JEDEC_BE_D8 offset curopcodes->opcode[oppos].opcode = opcode; curopcodes->opcode[oppos].spi_type = spi_type;
program_opcodes(curopcodes, 0);
switch (spi_programmer->type) {case SPI_CONTROLLER_ICH7:case SPI_CONTROLLER_VIA:program_opcodes(7, curopcodes, 0);break;case SPI_CONTROLLER_ICH9:program_opcodes(9, curopcodes, 0);break;default:msg_perr("%s: unsupported chipset\n", __func__);return -1;}@@ -443,7 +454,7 @@ static int find_preop(OPCODES *op, uint8_t preop) }
/* Create a struct OPCODES based on what we find in the locked down chipset. */ -static int generate_opcodes(OPCODES * op) +static int generate_opcodes(int ich_generation, OPCODES * op) { int a; uint16_t preop, optype; @@ -498,7 +509,7 @@ static int generate_opcodes(OPCODES * op) return 0; }
-static int program_opcodes(OPCODES *op, int enable_undo) +static int program_opcodes(int ich_generation, OPCODES *op, int enable_undo) { uint8_t a; uint16_t preop, optype; @@ -529,9 +540,8 @@ static int program_opcodes(OPCODES *op, int enable_undo) }
msg_pdbg("\n%s: preop=%04x optype=%04x opmenu=%08x%08x\n", __func__, preop, optype, opmenu[0], opmenu[1]);
- switch (spi_programmer->type) {
- case SPI_CONTROLLER_ICH7:
- case SPI_CONTROLLER_VIA:
- switch (ich_generation) {
- case 7: /* Register undo only for enable_undo=1, i.e. first call. */ if (enable_undo) { rmmio_valw(ich_spibar + ICH7_REG_PREOP);
@@ -544,7 +554,10 @@ static int program_opcodes(OPCODES *op, int enable_undo) mmio_writel(opmenu[0], ich_spibar + ICH7_REG_OPMENU); mmio_writel(opmenu[1], ich_spibar + ICH7_REG_OPMENU + 4); break;
- case SPI_CONTROLLER_ICH9:
- case 8:
- case 9:
- case 10:
- default: /* Future version might behave the same */ /* Register undo only for enable_undo=1, i.e. first call. */ if (enable_undo) { rmmio_valw(ich_spibar + ICH9_REG_PREOP);
@@ -557,9 +570,6 @@ static int program_opcodes(OPCODES *op, int enable_undo) mmio_writel(opmenu[0], ich_spibar + ICH9_REG_OPMENU); mmio_writel(opmenu[1], ich_spibar + ICH9_REG_OPMENU + 4); break;
default:
msg_perr("%s: unsupported chipset\n", __func__);return -1;}
return 0;
@@ -652,7 +662,7 @@ static void ich_fill_data(const uint8_t *data, int len, int reg0_off)
- It should be called before ICH sends any spi command.
*/ -static int ich_init_opcodes(void) +static int ich_init_opcodes(int ich_generation) { int rc = 0; OPCODES *curopcodes_done; @@ -663,11 +673,11 @@ static int ich_init_opcodes(void) if (ichspi_lock) { msg_pdbg("Reading OPCODES... "); curopcodes_done = &O_EXISTING;
rc = generate_opcodes(curopcodes_done);
rc = generate_opcodes(ich_generation, curopcodes_done);
rc = program_opcodes(curopcodes_done, 1);
rc = program_opcodes(ich_generation, curopcodes_done, 1);}
if (rc) {
@@ -1343,6 +1353,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
/* Assign Virtual Address */ ich_spibar = rcrb + spibar_offset;
ich_init_opcodes(ich_generation);
switch (ich_generation) { case 7:
@@ -1383,7 +1394,6 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, } ich_set_bbar(ich_generation, 0); register_spi_programmer(&spi_programmer_ich7);
ich_init_opcodes();@@ -1557,7 +1567,7 @@ int via_init_spi(struct pci_dev *dev) }
ich_set_bbar(7, 0);
- ich_init_opcodes();
ich_init_opcodes(7);
return 0;
}