[PATCH 1/2] Error out when chipset/board doesn't decode full ROM
Add infrastructure to check and report to the user the maximum supported decode size for chipsets and tested mainboards. The rationale is to warn users when they, for example, try to flash a 512KB parallel flash chip but their chipset only supports 256KB, or they try to flash 512KB and the chipset _does_ theoretically support 512KB but their special board doesn't wire all address lines and thus supports only 256 KB ROM chips at maximum. This has cost Uwe hours of debugging on some board already, until he figured out what was going on. We should try warn our users where possible about this. The chipset and the chip may have more than one bus in common (e.g. SB600 and Pm49* can both speak LPC+FWH) and on SB600/SB7x0/SB8x0 there are different limits for LPC and FWH. The only way to tell the user about the exact circumstances is to spew error messages per bus. The code will issue a warning during probe (which does fail for some chips if the size is too big) and abort before the first real read/write/erase action. That way, a user can find out why probe might not have worked, and will be stopped before he/she gets incorrect results. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-rom_decode_check_infrastructure/flash.h =================================================================== --- flashrom-rom_decode_check_infrastructure/flash.h (Revision 753) +++ flashrom-rom_decode_check_infrastructure/flash.h (Arbeitskopie) @@ -352,9 +352,17 @@ void sio_mask(uint16_t port, uint8_t reg, uint8_t data, uint8_t mask); int board_flash_enable(const char *vendor, const char *part); +struct decode_sizes { + uint32_t parallel; + uint32_t lpc; + uint32_t fwh; + uint32_t spi; +}; + /* chipset_enable.c */ extern enum chipbustype buses_supported; int chipset_flash_enable(void); +extern struct decode_sizes max_rom_decode; extern unsigned long flashbase; Index: flashrom-rom_decode_check_infrastructure/chipset_enable.c =================================================================== --- flashrom-rom_decode_check_infrastructure/chipset_enable.c (Revision 753) +++ flashrom-rom_decode_check_infrastructure/chipset_enable.c (Arbeitskopie) @@ -42,6 +42,17 @@ enum chipbustype buses_supported = CHIP_BUSTYPE_NONSPI; +/** + * Programmers supporting multiple buses can have differing size limits on + * each bus. Store the limits for each bus in a common struct. + */ +struct decode_sizes max_rom_decode = { + .parallel = 0xffffffff, + .lpc = 0xffffffff, + .fwh = 0xffffffff, + .spi = 0xffffffff +}; + extern int ichspi_lock; static int enable_flash_ali_m1533(struct pci_dev *dev, const char *name) Index: flashrom-rom_decode_check_infrastructure/flashrom.c =================================================================== --- flashrom-rom_decode_check_infrastructure/flashrom.c (Revision 753) +++ flashrom-rom_decode_check_infrastructure/flashrom.c (Arbeitskopie) @@ -299,6 +299,15 @@ return (a > b) ? a : b; } +int bitcount(unsigned long a) +{ + int i = 0; + for (; a != 0; a >>= 1) + if (a & 1) + i++; + return i; +} + char *strcat_realloc(char *dest, const char *src) { dest = realloc(dest, strlen(dest) + strlen(src) + 1); @@ -398,10 +407,63 @@ return ret; } +int check_max_decode(enum chipbustype buses, unsigned int size) +{ + int limitexceeded = 0; + if ((buses & CHIP_BUSTYPE_PARALLEL) && + (max_rom_decode.parallel < size)) { + limitexceeded++; + printf_debug("Chip size %u is bigger than supported " + "size %u of chipset/board/programmer " + "for Parallel interface, " + "probe/read/erase/write may fail. ", size, + max_rom_decode.parallel); + } + if ((buses & CHIP_BUSTYPE_LPC) && + (max_rom_decode.lpc < size)) { + limitexceeded++; + printf_debug("Chip size %u is bigger than supported " + "size %u of chipset/board/programmer " + "for LPC interface, " + "probe/read/erase/write may fail. ", size, + max_rom_decode.lpc); + } + if ((buses & CHIP_BUSTYPE_FWH) && + (max_rom_decode.fwh < size)) { + limitexceeded++; + printf_debug("Chip size %u is bigger than supported " + "size %u of chipset/board/programmer " + "for FWH interface, " + "probe/read/erase/write may fail. ", size, + max_rom_decode.fwh); + } + if ((buses & CHIP_BUSTYPE_SPI) && + (max_rom_decode.spi < size)) { + limitexceeded++; + printf_debug("Chip size %u is bigger than supported " + "size %u of chipset/board/programmer " + "for SPI interface, " + "probe/read/erase/write may fail. ", size, + max_rom_decode.spi); + } + if (!limitexceeded) + return 0; + /* Sometimes chip and programmer have more than one bus in common, + * and the limit is not exceeded on all buses. Tell the user. + */ + if (bitcount(buses) > limitexceeded) + printf_debug("There is at least one common chip/programmer " + "interface which can support a chip of this size. " + "You can try --force at your own risk.\n"); + return 1; +} + struct flashchip *probe_flash(struct flashchip *first_flash, int force) { struct flashchip *flash; - unsigned long base = 0, size; + unsigned long base = 0; + unsigned int size; + enum chipbustype buses_common; char *tmp; for (flash = first_flash; flash && flash->name; flash++) { @@ -413,7 +475,8 @@ printf_debug("failed! flashrom has no probe function for this flash chip.\n"); continue; } - if (!(buses_supported & flash->bustype)) { + buses_common = buses_supported & flash->bustype; + if (!buses_common) { tmp = flashbuses_to_text(buses_supported); printf_debug("skipped. Host bus type %s ", tmp); free(tmp); @@ -424,6 +487,7 @@ } size = flash->total_size * 1024; + check_max_decode(buses_common, size); base = flashbase ? flashbase : (0xffffffff - size + 1); flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size); @@ -975,6 +1039,14 @@ verify_it = 1; size = flash->total_size * 1024; + if (check_max_decode((buses_supported & flash->bustype), size) && + (!force)) { + fprintf(stderr, "This chip is too big for this programmer. " + "Details are given in verbose mode. You can override " + "this check with --force."); + programmer_shutdown(); + return 1; + } buf = (uint8_t *) calloc(size, sizeof(char)); if (erase_it) { -- Developer quote of the week: "We are juggling too many chainsaws and flaming arrows and tigers."
Use the maximum decode size infrastructure. - Detect max FWH size for Intel 631xESB/632xESB/3100/ICH6/ICH7/ICH8/ICH9/ICH10. - Move IDSEL override before decode size checking for the chipsets listed above or flashrom will complain based on old values. - Adjust supported flash buses for the chipsets listed above (none of them supports LPC or Parallel). - Detect max parallel size for AMD/National Semiconductor CS5530. - Adjust supported flash buses for CS5530. - Set board-specific max decode size for Elitegroup K7VTA3. - Set board-specific max decode size for Shuttle AK38N. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> diff -u flashrom-rom_decode_check/chipset_enable.c flashrom-rom_decode_check/chipset_enable.c --- flashrom-rom_decode_check/chipset_enable.c (Revision 754) +++ flashrom-rom_decode_check/chipset_enable.c (Arbeitskopie) @@ -4,6 +4,7 @@ * Copyright (C) 2000 Silicon Integrated System Corporation * Copyright (C) 2005-2009 coresystems GmbH * Copyright (C) 2006 Uwe Hermann <uwe@hermann-uwe.de> + * Copyright (C) 2007,2008,2009 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -215,48 +216,92 @@ uint32_t fwh_conf; int i; char *idsel = NULL; + int tmp; + int max_decode_fwh_idsel = 0; + int max_decode_fwh_decode = 0; + int contiguous = 1; - /* Ignore all legacy ranges below 1 MB. */ + if (programmer_param) + idsel = strstr(programmer_param, "fwh_idsel="); + + if (idsel) { + idsel += strlen("fwh_idsel="); + fwh_conf = (uint32_t)strtoul(idsel, NULL, 0); + + /* FIXME: Need to undo this on shutdown. */ + printf("\nSetting IDSEL=0x%x for top 16 MB", fwh_conf); + pci_write_long(dev, 0xd0, fwh_conf); + pci_write_word(dev, 0xd4, fwh_conf); + /* FIXME: Decode settings are not changed. */ + } + + /* Ignore all legacy ranges below 1 MB. + * We currently only support flashing the chip which responds to + * IDSEL=0. To support IDSEL!=0, flashbase and decode size calculations + * have to be adjusted. + */ /* FWH_SEL1 */ fwh_conf = pci_read_long(dev, 0xd0); - for (i = 7; i >= 0; i--) + for (i = 7; i >= 0; i--) { + tmp = (fwh_conf >> (i * 4)) & 0xf; printf_debug("\n0x%08x/0x%08x FWH IDSEL: 0x%x", (0x1ff8 + i) * 0x80000, (0x1ff0 + i) * 0x80000, - (fwh_conf >> (i * 4)) & 0xf); + tmp); + if ((tmp == 0) && contiguous) { + max_decode_fwh_idsel = (8 - i) * 0x80000; + } else { + contiguous = 0; + } + } /* FWH_SEL2 */ fwh_conf = pci_read_word(dev, 0xd4); - for (i = 3; i >= 0; i--) + for (i = 3; i >= 0; i--) { + tmp = (fwh_conf >> (i * 4)) & 0xf; printf_debug("\n0x%08x/0x%08x FWH IDSEL: 0x%x", (0xff4 + i) * 0x100000, (0xff0 + i) * 0x100000, - (fwh_conf >> (i * 4)) & 0xf); + tmp); + if ((tmp == 0) && contiguous) { + max_decode_fwh_idsel = (8 - i) * 0x100000; + } else { + contiguous = 0; + } + } + contiguous = 1; /* FWH_DEC_EN1 */ fwh_conf = pci_read_word(dev, 0xd8); - for (i = 7; i >= 0; i--) + for (i = 7; i >= 0; i--) { + tmp = (fwh_conf >> (i + 0x8)) & 0x1; printf_debug("\n0x%08x/0x%08x FWH decode %sabled", (0x1ff8 + i) * 0x80000, (0x1ff0 + i) * 0x80000, - (fwh_conf >> (i + 0x8)) & 0x1 ? "en" : "dis"); - for (i = 3; i >= 0; i--) + tmp ? "en" : "dis"); + if ((tmp == 0) && contiguous) { + max_decode_fwh_decode = (8 - i) * 0x80000; + } else { + contiguous = 0; + } + } + for (i = 3; i >= 0; i--) { + tmp = (fwh_conf >> i) & 0x1; printf_debug("\n0x%08x/0x%08x FWH decode %sabled", (0xff4 + i) * 0x100000, (0xff0 + i) * 0x100000, - (fwh_conf >> i) & 0x1 ? "en" : "dis"); - - if (programmer_param) - idsel = strstr(programmer_param, "fwh_idsel="); - - if (idsel) { - idsel += strlen("fwh_idsel="); - fwh_conf = (uint32_t)strtoul(idsel, NULL, 0); - - /* FIXME: Need to undo this on shutdown. */ - printf("\nSetting IDSEL=0x%x for top 16 MB", fwh_conf); - pci_write_long(dev, 0xd0, fwh_conf); - pci_write_word(dev, 0xd4, fwh_conf); + tmp ? "en" : "dis"); + if ((tmp == 0) && contiguous) { + max_decode_fwh_decode = (8 - i) * 0x100000; + } else { + contiguous = 0; + } } + max_rom_decode.fwh = min(max_decode_fwh_idsel, max_decode_fwh_decode); + printf_debug("\nMaximum FWH chip size: 0x%x bytes", max_rom_decode.fwh); + /* If we're called by enable_flash_ich_dc_spi, it will override + * buses_supported anyway. + */ + buses_supported = CHIP_BUSTYPE_FWH; return enable_flash_ich(dev, name, 0xdc); } @@ -269,6 +314,7 @@ { uint32_t mmio_base; + /* Do we really need no write enable? */ mmio_base = (pci_read_long(dev, 0xbc)) << 8; printf_debug("MMIO base at = 0x%x\n", mmio_base); spibar = physmap("VT8237S MMIO registers", mmio_base, 0x70); @@ -323,8 +369,7 @@ */ if (ich_generation == 7 && bbs == ICH_STRAP_LPC) { - /* Not sure if it speaks LPC as well. */ - buses_supported = CHIP_BUSTYPE_LPC | CHIP_BUSTYPE_FWH; + buses_supported = CHIP_BUSTYPE_FWH; /* No further SPI initialization required */ return ret; } @@ -336,16 +381,14 @@ spibar_offset = 0x3020; break; case 8: - /* Not sure if it speaks LPC as well. */ - buses_supported = CHIP_BUSTYPE_LPC | CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI; + buses_supported = CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI; spi_controller = SPI_CONTROLLER_ICH9; spibar_offset = 0x3020; break; case 9: case 10: default: /* Future version might behave the same */ - /* Not sure if it speaks LPC as well. */ - buses_supported = CHIP_BUSTYPE_LPC | CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI; + buses_supported = CHIP_BUSTYPE_FWH | CHIP_BUSTYPE_SPI; spi_controller = SPI_CONTROLLER_ICH9; spibar_offset = 0x3800; break; @@ -526,14 +569,22 @@ #define DECODE_CONTROL_REG2 0x5b /* F0 index 0x5b */ #define ROM_AT_LOGIC_CONTROL_REG 0x52 /* F0 index 0x52 */ +#define CS5530_RESET_CONTROL_REG 0x44 /* F0 index 0x44 */ +#define CS5530_USB_SHADOW_REG 0x43 /* F0 index 0x43 */ #define LOWER_ROM_ADDRESS_RANGE (1 << 0) #define ROM_WRITE_ENABLE (1 << 1) #define UPPER_ROM_ADDRESS_RANGE (1 << 2) #define BIOS_ROM_POSITIVE_DECODE (1 << 5) +#define CS5530_ISA_MASTER (1 << 7) +#define CS5530_ENABLE_SA2320 (1 << 2) +#define CS5530_ENABLE_SA20 (1 << 6) + buses_supported = CHIP_BUSTYPE_PARALLEL; /* Decode 0x000E0000-0x000FFFFF (128 KB), not just 64 KB, and * decode 0xFF000000-0xFFFFFFFF (16 MB), not just 256 KB. + * FIXME: Should we really touch the low mapping below 1 MB? Flashrom + * ignores that region completely. * Make the configured ROM areas writable. */ reg8 = pci_read_byte(dev, ROM_AT_LOGIC_CONTROL_REG); @@ -547,6 +598,24 @@ reg8 |= BIOS_ROM_POSITIVE_DECODE; pci_write_byte(dev, DECODE_CONTROL_REG2, reg8); + reg8 = pci_read_byte(dev, CS5530_RESET_CONTROL_REG); + if (reg8 & CS5530_ISA_MASTER) { + /* We have A0-A23 available. */ + max_rom_decode.parallel = 16 * 1024 * 1024; + } else { + reg8 = pci_read_byte(dev, CS5530_USB_SHADOW_REG); + if (reg8 & CS5530_ENABLE_SA2320) { + /* We have A0-19, A20-A23 available. */ + max_rom_decode.parallel = 16 * 1024 * 1024; + } else if (reg8 & CS5530_ENABLE_SA20) { + /* We have A0-19, A20 available. */ + max_rom_decode.parallel = 2 * 1024 * 1024; + } else { + /* A20 and above are not active. */ + max_rom_decode.parallel = 1024 * 1024; + } + } + return 0; } diff -u flashrom-rom_decode_check/board_enable.c flashrom-rom_decode_check/board_enable.c --- flashrom-rom_decode_check/board_enable.c (Revision 754) +++ flashrom-rom_decode_check/board_enable.c (Arbeitskopie) @@ -804,16 +804,24 @@ } /** - * Suited for: - * - Shuttle AK38N: VIA KT333CF + VIA VT8235 + ITE IT8705F - * - Elitegroup K7VTA3: VIA Apollo KT266/A/333 + VIA VT8235 + ITE IT8705F + * Suited for: Elitegroup K7VTA3: VIA Apollo KT266/A/333 + VIA VT8235 + ITE IT8705F */ -static int it8705f_write_enable_2e(const char *name) +static int elitegroup_k7vta3(const char *name) { + max_rom_decode.parallel = 256 * 1024; return it8705f_write_enable(0x2e, name); } /** + * Suited for: Shuttle AK38N: VIA KT333CF + VIA VT8235 + ITE IT8705F + */ +static int shuttle_ak38n(const char *name) +{ + max_rom_decode.parallel = 256 * 1024; + return it8705f_write_enable(0x2e, name); +} + +/** * Find the runtime registers of an SMSC Super I/O, after verifying its * chip ID. * @@ -1079,7 +1087,7 @@ {0x10DE, 0x0030, 0x1043, 0x818a, 0x8086, 0x100E, 0x1043, 0x80EE, NULL, NULL, "ASUS", "P5ND2-SLI Deluxe", board_asus_p5nd2_sli}, {0x1106, 0x3149, 0x1565, 0x3206, 0x1106, 0x3344, 0x1565, 0x1202, NULL, NULL, "Biostar", "P4M80-M4", it8705_rom_write_enable}, {0x8086, 0x3590, 0x1028, 0x016c, 0x1000, 0x0030, 0x1028, 0x016c, NULL, NULL, "Dell", "PowerEdge 1850", ich5_gpio23_raise}, - {0x1106, 0x3038, 0x1019, 0x0996, 0x1106, 0x3177, 0x1019, 0x0996, NULL, NULL, "Elitegroup", "K7VTA3", it8705f_write_enable_2e}, + {0x1106, 0x3038, 0x1019, 0x0996, 0x1106, 0x3177, 0x1019, 0x0996, NULL, NULL, "Elitegroup", "K7VTA3", elitegroup_k7vta3}, {0x1106, 0x3177, 0x1106, 0x3177, 0x1106, 0x3059, 0x1695, 0x3005, NULL, NULL, "EPoX", "EP-8K5A2", board_epox_ep_8k5a2}, {0x10EC, 0x8139, 0x1695, 0x9001, 0x11C1, 0x5811, 0x1695, 0x9015, NULL, NULL, "EPoX", "EP-8RDA3+", board_epox_ep_8rda3plus}, {0x8086, 0x7110, 0, 0, 0x8086, 0x7190, 0, 0, "epox", "ep-bx3", "EPoX", "EP-BX3", board_epox_ep_bx3}, @@ -1105,7 +1113,7 @@ {0x1106, 0x0571, 0x1462, 0x7120, 0, 0, 0, 0, "msi", "kt4v", "MSI", "MS-6712 (KT4V)", board_msi_kt4v}, {0x8086, 0x2658, 0x1462, 0x7046, 0x1106, 0x3044, 0x1462, 0x046d, NULL, NULL, "MSI", "MS-7046", ich6_gpio19_raise}, {0x10de, 0x005e, 0, 0, 0, 0, 0, 0, "msi", "k8n-neo3", "MSI", "MS-7135 (K8N Neo3)", w83627thf_gpio4_4_raise_4e}, - {0x1106, 0x3104, 0x1297, 0xa238, 0x1106, 0x3059, 0x1297, 0xc063, NULL, NULL, "Shuttle", "AK38N", it8705f_write_enable_2e}, + {0x1106, 0x3104, 0x1297, 0xa238, 0x1106, 0x3059, 0x1297, 0xc063, NULL, NULL, "Shuttle", "AK38N", shuttle_ak38n}, {0x10DE, 0x0050, 0x1297, 0x5036, 0x1412, 0x1724, 0x1297, 0x5036, NULL, NULL, "Shuttle", "FN25", board_shuttle_fn25}, {0x1106, 0x3038, 0x0925, 0x1234, 0x1106, 0x3058, 0x15DD, 0x7609, NULL, NULL, "Soyo", "SY-7VCA", board_soyo_sy_7vca}, {0x8086, 0x1076, 0x8086, 0x1176, 0x1106, 0x3059, 0x10f1, 0x2498, NULL, NULL, "Tyan", "S2498 (Tomcat K7M)", board_asus_a7v8x_mx}, -- Developer quote of the week: "We are juggling too many chainsaws and flaming arrows and tigers."
On Fri, Oct 30, 2009 at 05:24:40AM +0100, Carl-Daniel Hailfinger wrote:
Use the maximum decode size infrastructure. - Detect max FWH size for Intel 631xESB/632xESB/3100/ICH6/ICH7/ICH8/ICH9/ICH10. - Move IDSEL override before decode size checking for the chipsets listed above or flashrom will complain based on old values. - Adjust supported flash buses for the chipsets listed above (none of them supports LPC or Parallel). - Detect max parallel size for AMD/National Semiconductor CS5530.
Btw, this also applies CS5530A (which is mostly compatible to the CS5530). I checked the CS5530A datasheet, your code works for both.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Acked-by: Uwe Hermann <uwe@hermann-uwe.de>
@@ -215,48 +216,92 @@ uint32_t fwh_conf; int i; char *idsel = NULL; + int tmp; + int max_decode_fwh_idsel = 0; + int max_decode_fwh_decode = 0; + int contiguous = 1;
- /* Ignore all legacy ranges below 1 MB. */ + if (programmer_param) + idsel = strstr(programmer_param, "fwh_idsel=");
+ + if (idsel) { + idsel += strlen("fwh_idsel="); + fwh_conf = (uint32_t)strtoul(idsel, NULL, 0);
We should probably check the return value of strtoul() and handle errors (this can be an extra follow-up patch).
@@ -547,6 +598,24 @@ reg8 |= BIOS_ROM_POSITIVE_DECODE; pci_write_byte(dev, DECODE_CONTROL_REG2, reg8);
+ reg8 = pci_read_byte(dev, CS5530_RESET_CONTROL_REG); + if (reg8 & CS5530_ISA_MASTER) { + /* We have A0-A23 available. */ + max_rom_decode.parallel = 16 * 1024 * 1024; + } else { + reg8 = pci_read_byte(dev, CS5530_USB_SHADOW_REG); + if (reg8 & CS5530_ENABLE_SA2320) { + /* We have A0-19, A20-A23 available. */ + max_rom_decode.parallel = 16 * 1024 * 1024; + } else if (reg8 & CS5530_ENABLE_SA20) { + /* We have A0-19, A20 available. */ + max_rom_decode.parallel = 2 * 1024 * 1024; + } else { + /* A20 and above are not active. */ + max_rom_decode.parallel = 1024 * 1024;
I think we should add a print after each of the cases, so that the user can see how much this board can decode. IMHO we should just _always_ print that info for all chipsets/boards (if known), at least in -V. In the CS5530 case it might also be interesting _why_ 16MB can be decoded (as there are two cases which result in 16MB), so a print would be nice. This is material for an extra patch, though. On my test-board (GX1) the value was 16MB due to CS5530_ENABLE_SA2320, IIRC.
@@ -1079,7 +1087,7 @@ {0x10DE, 0x0030, 0x1043, 0x818a, 0x8086, 0x100E, 0x1043, 0x80EE, NULL, NULL, "ASUS", "P5ND2-SLI Deluxe", board_asus_p5nd2_sli}, {0x1106, 0x3149, 0x1565, 0x3206, 0x1106, 0x3344, 0x1565, 0x1202, NULL, NULL, "Biostar", "P4M80-M4", it8705_rom_write_enable}, {0x8086, 0x3590, 0x1028, 0x016c, 0x1000, 0x0030, 0x1028, 0x016c, NULL, NULL, "Dell", "PowerEdge 1850", ich5_gpio23_raise}, - {0x1106, 0x3038, 0x1019, 0x0996, 0x1106, 0x3177, 0x1019, 0x0996, NULL, NULL, "Elitegroup", "K7VTA3", it8705f_write_enable_2e}, + {0x1106, 0x3038, 0x1019, 0x0996, 0x1106, 0x3177, 0x1019, 0x0996, NULL, NULL, "Elitegroup", "K7VTA3", elitegroup_k7vta3}, {0x1106, 0x3177, 0x1106, 0x3177, 0x1106, 0x3059, 0x1695, 0x3005, NULL, NULL, "EPoX", "EP-8K5A2", board_epox_ep_8k5a2}, {0x10EC, 0x8139, 0x1695, 0x9001, 0x11C1, 0x5811, 0x1695, 0x9015, NULL, NULL, "EPoX", "EP-8RDA3+", board_epox_ep_8rda3plus}, {0x8086, 0x7110, 0, 0, 0x8086, 0x7190, 0, 0, "epox", "ep-bx3", "EPoX", "EP-BX3", board_epox_ep_bx3}, @@ -1105,7 +1113,7 @@ {0x1106, 0x0571, 0x1462, 0x7120, 0, 0, 0, 0, "msi", "kt4v", "MSI", "MS-6712 (KT4V)", board_msi_kt4v}, {0x8086, 0x2658, 0x1462, 0x7046, 0x1106, 0x3044, 0x1462, 0x046d, NULL, NULL, "MSI", "MS-7046", ich6_gpio19_raise}, {0x10de, 0x005e, 0, 0, 0, 0, 0, 0, "msi", "k8n-neo3", "MSI", "MS-7135 (K8N Neo3)", w83627thf_gpio4_4_raise_4e}, - {0x1106, 0x3104, 0x1297, 0xa238, 0x1106, 0x3059, 0x1297, 0xc063, NULL, NULL, "Shuttle", "AK38N", it8705f_write_enable_2e}, + {0x1106, 0x3104, 0x1297, 0xa238, 0x1106, 0x3059, 0x1297, 0xc063, NULL, NULL, "Shuttle", "AK38N", shuttle_ak38n}, {0x10DE, 0x0050, 0x1297, 0x5036, 0x1412, 0x1724, 0x1297, 0x5036, NULL, NULL, "Shuttle", "FN25", board_shuttle_fn25}, {0x1106, 0x3038, 0x0925, 0x1234, 0x1106, 0x3058, 0x15DD, 0x7609, NULL, NULL, "Soyo", "SY-7VCA", board_soyo_sy_7vca}, {0x8086, 0x1076, 0x8086, 0x1176, 0x1106, 0x3059, 0x10f1, 0x2498, NULL, NULL, "Tyan", "S2498 (Tomcat K7M)", board_asus_a7v8x_mx},
These two won't apply anymore, but it's easily fixable. Uwe. -- http://www.hermann-uwe.de | http://www.randomprojects.org http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
On 17.12.2009 10:52, Uwe Hermann wrote:
On Fri, Oct 30, 2009 at 05:24:40AM +0100, Carl-Daniel Hailfinger wrote:
Use the maximum decode size infrastructure. - Detect max FWH size for Intel 631xESB/632xESB/3100/ICH6/ICH7/ICH8/ICH9/ICH10. - Move IDSEL override before decode size checking for the chipsets listed above or flashrom will complain based on old values. - Adjust supported flash buses for the chipsets listed above (none of them supports LPC or Parallel). - Detect max parallel size for AMD/National Semiconductor CS5530.
Btw, this also applies CS5530A (which is mostly compatible to the CS5530). I checked the CS5530A datasheet, your code works for both.
Thanks, I updated the changlog.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Acked-by: Uwe Hermann <uwe@hermann-uwe.de>
Committed in r806.
+ + if (idsel) { + idsel += strlen("fwh_idsel="); + fwh_conf = (uint32_t)strtoul(idsel, NULL, 0);
We should probably check the return value of strtoul() and handle errors
Yes.
(this can be an extra follow-up patch).
I think we should add a print after each of the cases, so that the user can see how much this board can decode. IMHO we should just _always_ print that info for all chipsets/boards (if known), at least in -V. In the CS5530 case it might also be interesting _why_ 16MB can be decoded (as there are two cases which result in 16MB), so a print would be nice.
Agreed. I'd like to have the decode printing (at least for the end result) in generic code, though. We already perform decodesize vs. chipsize checking
This is material for an extra patch, though.
On my test-board (GX1) the value was 16MB due to CS5530_ENABLE_SA2320, IIRC.
Thanks for testing. Regards, Carl-Daniel -- Developer quote of the month: "We are juggling too many chainsaws and flaming arrows and tigers."
On Fri, Oct 30, 2009 at 01:52:28AM +0100, Carl-Daniel Hailfinger wrote:
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
The slightly updated version attached to this mail is Acked-by: Uwe Hermann <uwe@hermann-uwe.de> I tested this on an FWH board, one parallel board, and one nic3com with parallel flash. Changes in the updated patch: - Running "flashrom" without options also shows the error if the chip is too big (not just -r, -E, or -w). - Print ROM sizes in KB (not bytes) to be more user-friendly and better readable. - Make the error that gets printed without -V fit into one line: "Chip is too big for this programmer (-V gives details). Use --force to override." Also, add missing \n to that string. Uwe. -- http://www.hermann-uwe.de | http://www.randomprojects.org http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
On 31.10.2009 01:01, Uwe Hermann wrote:
On Fri, Oct 30, 2009 at 01:52:28AM +0100, Carl-Daniel Hailfinger wrote:
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
The slightly updated version attached to this mail is
Acked-by: Uwe Hermann <uwe@hermann-uwe.de>
Thanks!
I tested this on an FWH board, one parallel board, and one nic3com with parallel flash.
Changes in the updated patch:
- Running "flashrom" without options also shows the error if the chip is too big (not just -r, -E, or -w).
- Print ROM sizes in KB (not bytes) to be more user-friendly and better readable.
- Make the error that gets printed without -V fit into one line: "Chip is too big for this programmer (-V gives details). Use --force to override." Also, add missing \n to that string.
I agree with the changes. On top of your changes, I fixed one memory leak and changed kB vs. KB (for consistency), patch is below for reference. Add infrastructure to check and report to the user the maximum supported decode size for chipsets and tested mainboards. The rationale is to warn users when they, for example, try to flash a 512KB parallel flash chip but their chipset only supports 256KB, or they try to flash 512KB and the chipset _does_ theoretically support 512KB but their special board doesn't wire all address lines and thus supports only 256 KB ROM chips at maximum. This has cost Uwe hours of debugging on some board already, until he figured out what was going on. We should try warn our users where possible about this. The chipset and the chip may have more than one bus in common (e.g. SB600 and Pm49* can both speak LPC+FWH) and on SB600/SB7x0/SB8x0 there are different limits for LPC and FWH. The only way to tell the user about the exact circumstances is to spew error messages per bus. The code will issue a warning during probe (which does fail for some chips if the size is too big) and abort before the first real read/write/erase action. If no action is specified, the warning is printed anyway. That way, a user can find out why probe might not have worked, and will be stopped before he/she gets incorrect results. Add a bitcount function to the infrastructure. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Acked-by: Uwe Hermann <uwe@hermann-uwe.de> Index: flashrom-rom_decode_check_infrastructure/flash.h =================================================================== --- flashrom-rom_decode_check_infrastructure/flash.h (Revision 753) +++ flashrom-rom_decode_check_infrastructure/flash.h (Arbeitskopie) @@ -352,9 +352,17 @@ void sio_mask(uint16_t port, uint8_t reg, uint8_t data, uint8_t mask); int board_flash_enable(const char *vendor, const char *part); +struct decode_sizes { + uint32_t parallel; + uint32_t lpc; + uint32_t fwh; + uint32_t spi; +}; + /* chipset_enable.c */ extern enum chipbustype buses_supported; int chipset_flash_enable(void); +extern struct decode_sizes max_rom_decode; extern unsigned long flashbase; Index: flashrom-rom_decode_check_infrastructure/chipset_enable.c =================================================================== --- flashrom-rom_decode_check_infrastructure/chipset_enable.c (Revision 753) +++ flashrom-rom_decode_check_infrastructure/chipset_enable.c (Arbeitskopie) @@ -42,6 +42,17 @@ enum chipbustype buses_supported = CHIP_BUSTYPE_NONSPI; +/** + * Programmers supporting multiple buses can have differing size limits on + * each bus. Store the limits for each bus in a common struct. + */ +struct decode_sizes max_rom_decode = { + .parallel = 0xffffffff, + .lpc = 0xffffffff, + .fwh = 0xffffffff, + .spi = 0xffffffff +}; + extern int ichspi_lock; static int enable_flash_ali_m1533(struct pci_dev *dev, const char *name) Index: flashrom-rom_decode_check_infrastructure/flashrom.c =================================================================== --- flashrom-rom_decode_check_infrastructure/flashrom.c (Revision 753) +++ flashrom-rom_decode_check_infrastructure/flashrom.c (Arbeitskopie) @@ -299,6 +299,15 @@ return (a > b) ? a : b; } +int bitcount(unsigned long a) +{ + int i = 0; + for (; a != 0; a >>= 1) + if (a & 1) + i++; + return i; +} + char *strcat_realloc(char *dest, const char *src) { dest = realloc(dest, strlen(dest) + strlen(src) + 1); @@ -398,10 +407,60 @@ return ret; } +int check_max_decode(enum chipbustype buses, uint32_t size) +{ + int limitexceeded = 0; + if ((buses & CHIP_BUSTYPE_PARALLEL) && + (max_rom_decode.parallel < size)) { + limitexceeded++; + printf_debug("Chip size %u kB is bigger than supported " + "size %u kB of chipset/board/programmer " + "for %s interface, " + "probe/read/erase/write may fail. ", size / 1024, + max_rom_decode.parallel / 1024, "Parallel"); + } + if ((buses & CHIP_BUSTYPE_LPC) && (max_rom_decode.lpc < size)) { + limitexceeded++; + printf_debug("Chip size %u kB is bigger than supported " + "size %u kB of chipset/board/programmer " + "for %s interface, " + "probe/read/erase/write may fail. ", size / 1024, + max_rom_decode.lpc / 1024, "LPC"); + } + if ((buses & CHIP_BUSTYPE_FWH) && (max_rom_decode.fwh < size)) { + limitexceeded++; + printf_debug("Chip size %u kB is bigger than supported " + "size %u kB of chipset/board/programmer " + "for %s interface, " + "probe/read/erase/write may fail. ", size / 1024, + max_rom_decode.fwh / 1024, "FWH"); + } + if ((buses & CHIP_BUSTYPE_SPI) && (max_rom_decode.spi < size)) { + limitexceeded++; + printf_debug("Chip size %u kB is bigger than supported " + "size %u kB of chipset/board/programmer " + "for %s interface, " + "probe/read/erase/write may fail. ", size / 1024, + max_rom_decode.spi / 1024, "SPI"); + } + if (!limitexceeded) + return 0; + /* Sometimes chip and programmer have more than one bus in common, + * and the limit is not exceeded on all buses. Tell the user. + */ + if (bitcount(buses) > limitexceeded) + printf_debug("There is at least one common chip/programmer " + "interface which can support a chip of this size. " + "You can try --force at your own risk.\n"); + return 1; +} + struct flashchip *probe_flash(struct flashchip *first_flash, int force) { struct flashchip *flash; - unsigned long base = 0, size; + unsigned long base = 0; + uint32_t size; + enum chipbustype buses_common; char *tmp; for (flash = first_flash; flash && flash->name; flash++) { @@ -413,7 +472,8 @@ printf_debug("failed! flashrom has no probe function for this flash chip.\n"); continue; } - if (!(buses_supported & flash->bustype)) { + buses_common = buses_supported & flash->bustype; + if (!buses_common) { tmp = flashbuses_to_text(buses_supported); printf_debug("skipped. Host bus type %s ", tmp); free(tmp); @@ -424,6 +484,7 @@ } size = flash->total_size * 1024; + check_max_decode(buses_common, size); base = flashbase ? flashbase : (0xffffffff - size + 1); flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size); @@ -956,6 +1017,15 @@ "mainboard you tested. Thanks for your help!\n===\n"); } + size = flash->total_size * 1024; + if (check_max_decode((buses_supported & flash->bustype), size) && + (!force)) { + fprintf(stderr, "Chip is too big for this programmer " + "(-V gives details). Use --force to override.\n"); + programmer_shutdown(); + return 1; + } + if (!(read_it | write_it | verify_it | erase_it)) { printf("No operations were specified.\n"); // FIXME: flash writes stay enabled! @@ -974,7 +1044,6 @@ if (write_it && !dont_verify_it) verify_it = 1; - size = flash->total_size * 1024; buf = (uint8_t *) calloc(size, sizeof(char)); if (erase_it) { -- Developer quote of the week: "We are juggling too many chainsaws and flaming arrows and tigers."
On 31.10.2009 02:52, Carl-Daniel Hailfinger wrote:
I agree with the changes. On top of your changes, I fixed one memory leak and changed kB vs. KB (for consistency), patch is below for reference.
Add infrastructure to check and report to the user the maximum supported decode size for chipsets and tested mainboards.
And committed in r755. Regards, Carl-Daniel -- Developer quote of the week: "We are juggling too many chainsaws and flaming arrows and tigers."
participants (2)
-
Carl-Daniel Hailfinger -
Uwe Hermann