Re: [flashrom] [PATCH 1/2] ichspi: remove dependency on spi_programmer from generate_opcodes and program_opcodes
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; + } oppos = find_opcode(curopcodes, opcode); msg_pdbg ("on-the-fly OPCODE (0x%02X) re-programmed, op-pos=%d\n", opcode, oppos); return oppos; @@ -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); } else { msg_pdbg("Programming OPCODES... "); curopcodes_done = &O_ST_M25P; - 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(); break; case 8: case 9: @@ -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; }
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, and - we really want to distinguish between chipset version-grained attributes which are not reflected by the registered programmer. Hence this patch introduces a new static variable which is set up early by the init functions and allows us to get rid of all "switch (spi_programmer->type)" in ichspi.c. We reuse the enum introduced for descriptor mode for the type of the new variable. Previously magic numbers were passed by chipset_enable wrappers. Now they use the enumeration items too. To get this working the enum definition had to be moved to programmer.h, which was fixed on the way by adding necessary includes. Another noteworthy detail: previously we have checked for a valid programmer/ich generation all over the place. I have removed those checks and added one single check in the init method. Calling any function of a programmer without executing the init method first, is undefined behavior. Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> --- On Wed, 02 Nov 2011 02:44:17 +0100 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
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.)
and even then they would have to be from different incompatible generations... i think we are quite safe ;)
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.
you may want to evaluate that "patch size" argument again... *sigh* i have added the first applicable chipset to each default case for documentation purposes. chipset_enable.c | 24 +++++++------- ich_descriptors.h | 15 +-------- ichspi.c | 90 ++++++++++++++++++++++------------------------------- programmer.h | 14 ++++++++ 4 files changed, 64 insertions(+), 79 deletions(-) diff --git a/chipset_enable.c b/chipset_enable.c index d2d81e0..6bde2d0 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -504,7 +504,7 @@ static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name, static const char *const straps_names_unknown[] = { "unknown", "unknown", "unknown", "unknown" }; switch (ich_generation) { - case 7: + case CHIPSET_ICH7: /* EP80579 may need further changes, but this is the least * intrusive way to get correct BOOT Strap printing without * changing the rest of its code path). */ @@ -513,13 +513,13 @@ static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name, else straps_names = straps_names_ich7_nm10; break; - case 8: - case 9: - case 10: + case CHIPSET_ICH8: + case CHIPSET_ICH9: + case CHIPSET_ICH10: straps_names = straps_names_ich8910; break; - case 11: - case 12: + case CHIPSET_5_SERIES_IBEX_PEAK: + case CHIPSET_6_SERIES_COUGAR_POINT: straps_names = straps_names_pch56; break; default: @@ -579,34 +579,34 @@ static int enable_flash_ich_dc_spi(struct pci_dev *dev, const char *name, static int enable_flash_ich7(struct pci_dev *dev, const char *name) { - return enable_flash_ich_dc_spi(dev, name, 7); + return enable_flash_ich_dc_spi(dev, name, CHIPSET_ICH7); } static int enable_flash_ich8(struct pci_dev *dev, const char *name) { - return enable_flash_ich_dc_spi(dev, name, 8); + return enable_flash_ich_dc_spi(dev, name, CHIPSET_ICH8); } static int enable_flash_ich9(struct pci_dev *dev, const char *name) { - return enable_flash_ich_dc_spi(dev, name, 9); + return enable_flash_ich_dc_spi(dev, name, CHIPSET_ICH9); } static int enable_flash_ich10(struct pci_dev *dev, const char *name) { - return enable_flash_ich_dc_spi(dev, name, 10); + return enable_flash_ich_dc_spi(dev, name, CHIPSET_ICH10); } /* Ibex Peak aka. 5 series & 3400 series */ static int enable_flash_pch5(struct pci_dev *dev, const char *name) { - return enable_flash_ich_dc_spi(dev, name, 11); + return enable_flash_ich_dc_spi(dev, name, CHIPSET_5_SERIES_IBEX_PEAK); } /* Cougar Point aka. 6 series & c200 series */ static int enable_flash_pch6(struct pci_dev *dev, const char *name) { - return enable_flash_ich_dc_spi(dev, name, 12); + return enable_flash_ich_dc_spi(dev, name, CHIPSET_6_SERIES_COUGAR_POINT); } static int via_no_byte_merge(struct pci_dev *dev, const char *name) diff --git a/ich_descriptors.h b/ich_descriptors.h index 4e2ad0a..714eda9 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -24,6 +24,7 @@ #define __ICH_DESCRIPTORS_H__ 1 #include <stdint.h> +#include "programmer.h" /* for enum ich_chipset */ /* FIXME: Replace with generic return codes */ #define ICH_RET_OK 0 @@ -63,20 +64,6 @@ #define ICH_FREG_BASE(flreg) (((flreg) << 12) & 0x01fff000) #define ICH_FREG_LIMIT(flreg) (((flreg) >> 4) & 0x01fff000) -/* Used to select the right descriptor printing function. - * Currently only ICH8 and Ibex Peak are supported. - */ -enum ich_chipset { - CHIPSET_ICH_UNKNOWN, - CHIPSET_ICH7 = 7, - CHIPSET_ICH8, - CHIPSET_ICH9, - CHIPSET_ICH10, - CHIPSET_5_SERIES_IBEX_PEAK, - CHIPSET_6_SERIES_COUGAR_POINT, - CHIPSET_7_SERIES_PANTHER_POINT -}; - void prettyprint_ich_reg_vscc(uint32_t reg_val, int verbosity); struct ich_desc_content { diff --git a/ichspi.c b/ichspi.c index afa420b..137f438 100644 --- a/ichspi.c +++ b/ichspi.c @@ -172,6 +172,7 @@ /* ICH SPI configuration lock-down. May be set during chipset enabling. */ static int ichspi_lock = 0; +static enum ich_chipset ich_generation = CHIPSET_ICH_UNKNOWN; uint32_t ichspi_bbar = 0; static void *ich_spibar = NULL; @@ -454,23 +455,20 @@ static int generate_opcodes(OPCODES * op) return -1; } - switch (spi_programmer->type) { - case SPI_CONTROLLER_ICH7: - case SPI_CONTROLLER_VIA: + switch (ich_generation) { + case CHIPSET_ICH7: preop = REGREAD16(ICH7_REG_PREOP); optype = REGREAD16(ICH7_REG_OPTYPE); opmenu[0] = REGREAD32(ICH7_REG_OPMENU); opmenu[1] = REGREAD32(ICH7_REG_OPMENU + 4); break; - case SPI_CONTROLLER_ICH9: + case CHIPSET_ICH8: + default: /* Future version might behave the same */ preop = REGREAD16(ICH9_REG_PREOP); optype = REGREAD16(ICH9_REG_OPTYPE); opmenu[0] = REGREAD32(ICH9_REG_OPMENU); opmenu[1] = REGREAD32(ICH9_REG_OPMENU + 4); break; - default: - msg_perr("%s: unsupported chipset\n", __func__); - return -1; } op->preop[0] = (uint8_t) preop; @@ -529,9 +527,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 CHIPSET_ICH7: /* Register undo only for enable_undo=1, i.e. first call. */ if (enable_undo) { rmmio_valw(ich_spibar + ICH7_REG_PREOP); @@ -544,7 +541,8 @@ 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 CHIPSET_ICH8: + 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 +555,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; @@ -569,16 +564,17 @@ static int program_opcodes(OPCODES *op, int enable_undo) * Try to set BBAR (BIOS Base Address Register), but read back the value in case * it didn't stick. */ -static void ich_set_bbar(int ich_generation, uint32_t min_addr) +static void ich_set_bbar(uint32_t min_addr) { int bbar_off; switch (ich_generation) { - case 7: + case CHIPSET_ICH7: bbar_off = 0x50; break; - case 8: + case CHIPSET_ICH8: msg_perr("BBAR offset is unknown on ICH8!\n"); return; + case CHIPSET_ICH9: default: /* Future version might behave the same */ bbar_off = ICH9_REG_BBAR; break; @@ -943,15 +939,12 @@ static int run_opcode(OPCODE op, uint32_t offset, return SPI_INVALID_LENGTH; } - switch (spi_programmer->type) { - case SPI_CONTROLLER_VIA: - case SPI_CONTROLLER_ICH7: + switch (ich_generation) { + case CHIPSET_ICH7: return ich7_run_opcode(op, offset, datalength, data, maxlength); - case SPI_CONTROLLER_ICH9: + case CHIPSET_ICH8: + default: /* Future version might behave the same */ return ich9_run_opcode(op, offset, datalength, data); - default: - /* If we ever get here, something really weird happened */ - return -1; } } @@ -1022,19 +1015,11 @@ static int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt, opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) { addr = (writearr[1] << 16) | (writearr[2] << 8) | (writearr[3] << 0); - switch (spi_programmer->type) { - case SPI_CONTROLLER_ICH7: - case SPI_CONTROLLER_VIA: - case SPI_CONTROLLER_ICH9: - if (addr < ichspi_bbar) { - msg_perr("%s: Address 0x%06x below allowed " - "range 0x%06x-0xffffff\n", __func__, - addr, ichspi_bbar); - return SPI_INVALID_ADDRESS; - } - break; - default: - break; + if (addr < ichspi_bbar) { + msg_perr("%s: Address 0x%06x below allowed " + "range 0x%06x-0xffffff\n", __func__, + addr, ichspi_bbar); + return SPI_INVALID_ADDRESS; } } @@ -1315,8 +1300,7 @@ static const struct spi_programmer spi_programmer_ich9 = { .write_256 = default_spi_write_256, }; -int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, - int ich_generation) +int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, int ich_gen) { int i; uint8_t old, new; @@ -1324,15 +1308,16 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, uint32_t tmp; int desc_valid = 0; + ich_generation = ich_gen; + switch (ich_generation) { - case 7: - spibar_offset = 0x3020; - break; - case 8: + case CHIPSET_ICH_UNKNOWN: + return -1; + case CHIPSET_ICH7: + case CHIPSET_ICH8: spibar_offset = 0x3020; break; - case 9: - case 10: + case CHIPSET_ICH9: default: /* Future version might behave the same */ spibar_offset = 0x3800; break; @@ -1345,7 +1330,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, ich_spibar = rcrb + spibar_offset; switch (ich_generation) { - case 7: + case CHIPSET_ICH7: msg_pdbg("0x00: 0x%04x (SPIS)\n", mmio_readw(ich_spibar + 0)); msg_pdbg("0x02: 0x%04x (SPIC)\n", @@ -1381,13 +1366,11 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n"); ichspi_lock = 1; } - ich_set_bbar(ich_generation, 0); + ich_set_bbar(0); register_spi_programmer(&spi_programmer_ich7); ich_init_opcodes(); break; - case 8: - case 9: - case 10: + case CHIPSET_ICH8: default: /* Future version might behave the same */ tmp2 = mmio_readw(ich_spibar + ICH9_REG_HSFS); msg_pdbg("0x04: 0x%04x (HSFS)\n", tmp2); @@ -1447,7 +1430,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, mmio_readl(ich_spibar + ICH9_REG_OPMENU)); msg_pdbg("0x9C: 0x%08x (OPMENU+4)\n", mmio_readl(ich_spibar + ICH9_REG_OPMENU + 4)); - if (ich_generation == 8) { + if (ich_generation == CHIPSET_ICH8) { tmp = mmio_readl(ich_spibar + ICH8_REG_VSCC); msg_pdbg("0xC1: 0x%08x (VSCC)\n", tmp); msg_pdbg("VSCC: "); @@ -1469,7 +1452,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, tmp = mmio_readl(ich_spibar + ICH9_REG_FPB); msg_pdbg("0xD0: 0x%08x (FPB)\n", tmp); - ich_set_bbar(ich_generation, 0); + ich_set_bbar(0); } msg_pdbg("\n"); @@ -1524,6 +1507,7 @@ int via_init_spi(struct pci_dev *dev) /* Not sure if it speaks all these bus protocols. */ buses_supported = BUS_LPC | BUS_FWH; + ich_generation = CHIPSET_ICH7; register_spi_programmer(&spi_programmer_via); msg_pdbg("0x00: 0x%04x (SPIS)\n", mmio_readw(ich_spibar + 0)); @@ -1556,7 +1540,7 @@ int via_init_spi(struct pci_dev *dev) ichspi_lock = 1; } - ich_set_bbar(7, 0); + ich_set_bbar(0); ich_init_opcodes(); return 0; diff --git a/programmer.h b/programmer.h index 8d72518..dc40ab8 100644 --- a/programmer.h +++ b/programmer.h @@ -24,6 +24,9 @@ #ifndef __PROGRAMMER_H__ #define __PROGRAMMER_H__ 1 +#include <stddef.h> +#include "flash.h" /* for chipaddr, pci_dev */ + enum programmer { #if CONFIG_INTERNAL == 1 PROGRAMMER_INTERNAL, @@ -577,6 +580,17 @@ void register_spi_programmer(const struct spi_programmer *programmer); /* ichspi.c */ #if CONFIG_INTERNAL == 1 +enum ich_chipset { + CHIPSET_ICH_UNKNOWN, + CHIPSET_ICH7 = 7, + CHIPSET_ICH8, + CHIPSET_ICH9, + CHIPSET_ICH10, + CHIPSET_5_SERIES_IBEX_PEAK, + CHIPSET_6_SERIES_COUGAR_POINT, + CHIPSET_7_SERIES_PANTHER_POINT +}; + extern uint32_t ichspi_bbar; int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb, int ich_generation); -- 1.7.1
participants (2)
-
Carl-Daniel Hailfinger -
Stefan Tauner