On 07.07.2010 16:31, Michael Karcher wrote:
Am Mittwoch, den 07.07.2010, 14:16 +0200 schrieb Carl-Daniel Hailfinger:
- /* Non-default port requested? */
- portpos = extract_param(&programmer_param, "it87spiport", ",:");
- if (portpos) {
char *endptr = NULL;if (strlen(portpos))forced_flashport = strtoul(portpos, &endptr, 0);/* Port 0, port >65535 and garbage strings are rejected. */if (!forced_flashport || (forced_flashport > 65535) ||(endptr && (*endptr != '\0'))) {I don't get the idea of the if in the beginning. The strtoul mangpage states: "If there were no digits at all, strtol() stores the original value of nptr in *endptr (and returns 0)." So you can just omit the "if (strlen(portpos))" test, because strtoul() returns zero in this case (and doesn't change forced_flashport). If
Changed. I wanted to avoid calling strtoul in a case where it doesn't make sense, but this is a one-time cost, so its impact may very well be negligible.
strtoul is executed unconditionally, you can also throw away the check for endptr not being NULL, as endptr is *always* set by strtoul.[1]
True.
Alignment verification would be nice here, too. The datasheet should state the required alignment.
There are actually more constraints. The port has significant bits 3..11, the rest is readonly and zero. I'm now checking for those constraints as well.
- if (forced_flashport) {
flashport = (uint16_t)forced_flashport;msg_pinfo("Forcing serial flash port 0x%04x\n",flashport);sio_write(port, 0x64, (flashport >> 8));sio_write(port, 0x65, (flashport & 0xff));- }
Can't you put this in an else branch of the if() shown above, or, in my oppinion even better: Invert the condition to something like
if(parameter makes sense) set flashport & setup hardware else print error message & exit(1)
Done.
The remaining stuff looks OK, but I didn't cross-check against the IT8705 datasheet. Do you want a cross-check too?
That would be awesome, but I can't demand that from you. Uwe indicated that he might do a datasheet cross-check as well, so please coordinate your efforts to make sure the work is not done twice. Thanks!
New version, should have all comments addressed.
Autodetect the ITE IT8705 Super I/O and enable flash writes if it performs LPC->Parallel translation. Remove board enables which triggered the IT8705 write enable manually. Change the IT87 SPI special case to cover IT87 LPC->SPI and LPC->Parallel translation.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Tested on Syntax SV266A. Acked-by: Paul Menzel paulepanter@users.sourceforge.net
Tested on Shuttle AK38N, all operations work fine.
Acked-by: Uwe Hermann uwe@hermann-uwe.de
Index: flashrom-it8705_autodetect/flash.h =================================================================== --- flashrom-it8705_autodetect/flash.h (Revision 1070) +++ flashrom-it8705_autodetect/flash.h (Arbeitskopie) @@ -350,6 +350,7 @@ /* board_enable.c */ void w836xx_ext_enter(uint16_t port); void w836xx_ext_leave(uint16_t port); +int it8705f_write_enable(uint8_t port); uint8_t sio_read(uint16_t port, uint8_t reg); void sio_write(uint16_t port, uint8_t reg, uint8_t data); void sio_mask(uint16_t port, uint8_t reg, uint8_t data, uint8_t mask); @@ -692,12 +693,11 @@ int ich_spi_send_multicommand(struct spi_command *cmds);
/* it87spi.c */ -extern uint16_t it8716f_flashport; void enter_conf_mode_ite(uint16_t port); void exit_conf_mode_ite(uint16_t port); struct superio probe_superio_ite(void); +int init_superio_ite(void); int it87spi_init(void); -int it87xx_probe_spi_flash(const char *name); int it8716f_spi_send_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); int it8716f_spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len); Index: flashrom-it8705_autodetect/it87spi.c =================================================================== --- flashrom-it8705_autodetect/it87spi.c (Revision 1070) +++ flashrom-it8705_autodetect/it87spi.c (Arbeitskopie) @@ -96,90 +96,104 @@ return ret; }
-static uint16_t find_ite_spi_flash_port(uint16_t port, uint16_t id) +static uint16_t it87spi_probe(uint16_t port) { uint8_t tmp = 0; char *portpos = NULL; uint16_t flashport = 0;
- switch (id) { - case 0x8716: - case 0x8718: - case 0x8720: - enter_conf_mode_ite(port); - /* NOLDN, reg 0x24, mask out lowest bit (suspend) */ - tmp = sio_read(port, 0x24) & 0xFE; - /* If IT87SPI was not explicitly selected, we want to check - * quickly if LPC->SPI translation is active. - */ - if ((programmer == PROGRAMMER_INTERNAL) && !(tmp & (0x0E))) { - msg_pdbg("No IT87* serial flash segment enabled.\n"); - exit_conf_mode_ite(port); - break; - } - msg_pdbg("Serial flash segment 0x%08x-0x%08x %sabled\n", - 0xFFFE0000, 0xFFFFFFFF, (tmp & 1 << 1) ? "en" : "dis"); - msg_pdbg("Serial flash segment 0x%08x-0x%08x %sabled\n", - 0x000E0000, 0x000FFFFF, (tmp & 1 << 1) ? "en" : "dis"); - msg_pdbg("Serial flash segment 0x%08x-0x%08x %sabled\n", - 0xFFEE0000, 0xFFEFFFFF, (tmp & 1 << 2) ? "en" : "dis"); - msg_pdbg("Serial flash segment 0x%08x-0x%08x %sabled\n", - 0xFFF80000, 0xFFFEFFFF, (tmp & 1 << 3) ? "en" : "dis"); - msg_pdbg("LPC write to serial flash %sabled\n", - (tmp & 1 << 4) ? "en" : "dis"); - /* The LPC->SPI force write enable below only makes sense for - * non-programmer mode. - */ - /* If any serial flash segment is enabled, enable writing. */ - if ((tmp & 0xe) && (!(tmp & 1 << 4))) { - msg_pdbg("Enabling LPC write to serial flash\n"); - tmp |= 1 << 4; - sio_write(port, 0x24, tmp); - } - msg_pdbg("Serial flash pin %i\n", (tmp & 1 << 5) ? 87 : 29); - /* LDN 0x7, reg 0x64/0x65 */ - sio_write(port, 0x07, 0x7); - flashport = sio_read(port, 0x64) << 8; - flashport |= sio_read(port, 0x65); - msg_pdbg("Serial flash port 0x%04x\n", flashport); - /* Non-default port requested? */ - portpos = extract_param(&programmer_param, "it87spiport", ",:"); - if (portpos && strlen(portpos)) { - flashport = strtol(portpos, (char **)NULL, 0); - msg_pinfo("Forcing serial flash port 0x%04x\n", - flashport); - sio_write(port, 0x64, (flashport >> 8)); - sio_write(port, 0x65, (flashport & 0xff)); - } else if (portpos) { - msg_perr("Error: it87spiport specified, but no port " - "given.\n"); + enter_conf_mode_ite(port); + /* NOLDN, reg 0x24, mask out lowest bit (suspend) */ + tmp = sio_read(port, 0x24) & 0xFE; + /* If IT87SPI was not explicitly selected, we want to check + * quickly if LPC->SPI translation is active. + */ + if ((programmer == PROGRAMMER_INTERNAL) && !(tmp & (0x0E))) { + msg_pdbg("No IT87* serial flash segment enabled.\n"); + exit_conf_mode_ite(port); + /* Nothing to do. */ + return 1; + } + msg_pdbg("Serial flash segment 0x%08x-0x%08x %sabled\n", + 0xFFFE0000, 0xFFFFFFFF, (tmp & 1 << 1) ? "en" : "dis"); + msg_pdbg("Serial flash segment 0x%08x-0x%08x %sabled\n", + 0x000E0000, 0x000FFFFF, (tmp & 1 << 1) ? "en" : "dis"); + msg_pdbg("Serial flash segment 0x%08x-0x%08x %sabled\n", + 0xFFEE0000, 0xFFEFFFFF, (tmp & 1 << 2) ? "en" : "dis"); + msg_pdbg("Serial flash segment 0x%08x-0x%08x %sabled\n", + 0xFFF80000, 0xFFFEFFFF, (tmp & 1 << 3) ? "en" : "dis"); + msg_pdbg("LPC write to serial flash %sabled\n", + (tmp & 1 << 4) ? "en" : "dis"); + /* The LPC->SPI force write enable below only makes sense for + * non-programmer mode. + */ + /* If any serial flash segment is enabled, enable writing. */ + if ((tmp & 0xe) && (!(tmp & 1 << 4))) { + msg_pdbg("Enabling LPC write to serial flash\n"); + tmp |= 1 << 4; + sio_write(port, 0x24, tmp); + } + msg_pdbg("Serial flash pin %i\n", (tmp & 1 << 5) ? 87 : 29); + /* LDN 0x7, reg 0x64/0x65 */ + sio_write(port, 0x07, 0x7); + flashport = sio_read(port, 0x64) << 8; + flashport |= sio_read(port, 0x65); + msg_pdbg("Serial flash port 0x%04x\n", flashport); + /* Non-default port requested? */ + portpos = extract_param(&programmer_param, "it87spiport", ",:"); + if (portpos) { + char *endptr = NULL; + unsigned long forced_flashport; + forced_flashport = strtoul(portpos, &endptr, 0); + /* Port 0, port >65535 and garbage strings are rejected. */ + if (!forced_flashport || (forced_flashport > 65535) || + (forced_flashport & 0xf007) || (*endptr != '\0')) { + msg_perr("Error: it87spiport specified, but no valid " + "port specified. Port must be a multiple of 8 " + "and lie between 8 and 4088.\n"); + forced_flashport = 0; free(portpos); /* FIXME: Return failure here once it87spi_common_init() * can handle the return value sanely. */ exit(1); + } else { + flashport = (uint16_t)forced_flashport; + msg_pinfo("Forcing serial flash port 0x%04x\n", + flashport); + sio_write(port, 0x64, (flashport >> 8)); + sio_write(port, 0x65, (flashport & 0xff)); } - free(portpos); - exit_conf_mode_ite(port); - break; - /* TODO: Handle more IT87xx if they support flash translation */ - default: - msg_pdbg("SuperI/O ID %04hx is not on the controller list.\n", id); } - return flashport; + free(portpos); + exit_conf_mode_ite(port); + it8716f_flashport = flashport; + if (buses_supported & CHIP_BUSTYPE_SPI) + msg_pdbg("Overriding chipset SPI with IT87 SPI.\n"); + spi_controller = SPI_CONTROLLER_IT87XX; + buses_supported |= CHIP_BUSTYPE_SPI; + return 0; }
-int it87spi_common_init(void) +int init_superio_ite(void) { if (superio.vendor != SUPERIO_VENDOR_ITE) return 1;
- it8716f_flashport = find_ite_spi_flash_port(superio.port, superio.model); - - if (it8716f_flashport) - spi_controller = SPI_CONTROLLER_IT87XX; - - return (!it8716f_flashport); + switch (superio.model) { + case 0x8705: + return it8705f_write_enable(superio.port); + break; + case 0x8716: + case 0x8718: + case 0x8720: + return it87spi_probe(superio.port); + break; + default: + msg_pdbg("Super I/O ID %04hx is not on the list of flash " + "capable controllers.\n", superio.model); + } + return 1; }
@@ -190,7 +204,7 @@ get_io_perms(); /* Probe for the Super I/O chip and fill global struct superio. */ probe_superio(); - ret = it87spi_common_init(); + ret = init_superio_ite(); if (!ret) { buses_supported = CHIP_BUSTYPE_SPI; } else { @@ -199,19 +213,6 @@ return ret; }
-int it87xx_probe_spi_flash(const char *name) -{ - int ret; - - ret = it87spi_common_init(); - if (!ret) { - if (buses_supported & CHIP_BUSTYPE_SPI) - msg_pdbg("Overriding chipset SPI with IT87 SPI.\n"); - buses_supported |= CHIP_BUSTYPE_SPI; - } - return ret; -} - /* * The IT8716F only supports commands with length 1,2,4,5 bytes including * command byte and can not read more than 3 bytes from the device. Index: flashrom-it8705_autodetect/internal.c =================================================================== --- flashrom-it8705_autodetect/internal.c (Revision 1070) +++ flashrom-it8705_autodetect/internal.c (Arbeitskopie) @@ -229,8 +229,10 @@ }
#if defined(__i386__) || defined(__x86_64__) - /* Probe for IT87* LPC->SPI translation unconditionally. */ - it87xx_probe_spi_flash(NULL); + /* Probe unconditionally for IT87* LPC->SPI translation and for + * IT87* Parallel write enable. + */ + init_superio_ite(); #endif
board_flash_enable(lb_vendor, lb_part); Index: flashrom-it8705_autodetect/board_enable.c =================================================================== --- flashrom-it8705_autodetect/board_enable.c (Revision 1070) +++ flashrom-it8705_autodetect/board_enable.c (Arbeitskopie) @@ -390,33 +390,94 @@ }
/** - * + * Suited for all boards with ITE IT8705F. + * The SIS950 Super I/O probably requires a similar flash write enable. */ -static int it8705f_write_enable(uint8_t port) +int it8705f_write_enable(uint8_t port) { + uint8_t tmp; + int ret = 0; + enter_conf_mode_ite(port); - sio_mask(port, 0x24, 0x04, 0x04); /* Flash ROM I/F Writes Enable */ + tmp = sio_read(port, 0x24); + /* Check if at least one flash segment is enabled. */ + if (tmp & 0xf0) { + /* The IT8705F will respond to LPC cycles and translate them. */ + buses_supported = CHIP_BUSTYPE_PARALLEL; + /* Flash ROM I/F Writes Enable */ + tmp |= 0x04; + msg_pdbg("Enabling IT8705F flash ROM interface write.\n"); + if (tmp & 0x02) { + /* The data sheet contradicts itself about max size. */ + max_rom_decode.parallel = 1024 * 1024; + msg_pinfo("IT8705F with very unusual settings. Please " + "send the output of "flashrom -V" to \n" + "flashrom@flashrom.org to help us finish " + "support for your Super I/O. Thanks.\n"); + ret = 1; + } else if (tmp & 0x08) { + max_rom_decode.parallel = 512 * 1024; + } else { + max_rom_decode.parallel = 256 * 1024; + } + /* Safety checks. The data sheet is unclear here: Segments 1+3 + * overlap, no segment seems to cover top - 1MB to top - 512kB. + * We assume that certain combinations make no sense. + */ + if (((tmp & 0x02) && !(tmp & 0x08)) || /* 1 MB en, 512 kB dis */ + (!(tmp & 0x10)) || /* 128 kB dis */ + (!(tmp & 0x40))) { /* 256/512 kB dis */ + msg_perr("Inconsistent IT8705F decode size!\n"); + ret = 1; + } + if (sio_read(port, 0x25) != 0) { + msg_perr("IT8705F flash data pins disabled!\n"); + ret = 1; + } + if (sio_read(port, 0x26) != 0) { + msg_perr("IT8705F flash address pins 0-7 disabled!\n"); + ret = 1; + } + if (sio_read(port, 0x27) != 0) { + msg_perr("IT8705F flash address pins 8-15 disabled!\n"); + ret = 1; + } + if ((sio_read(port, 0x29) & 0x10) != 0) { + msg_perr("IT8705F flash write enable pin disabled!\n"); + ret = 1; + } + if ((sio_read(port, 0x29) & 0x08) != 0) { + msg_perr("IT8705F flash chip select pin disabled!\n"); + ret = 1; + } + if ((sio_read(port, 0x29) & 0x04) != 0) { + msg_perr("IT8705F flash read strobe pin disabled!\n"); + ret = 1; + } + if ((sio_read(port, 0x29) & 0x03) != 0) { + msg_perr("IT8705F flash address pins 16-17 disabled!\n"); + /* Not really an error if you use flash chips smaller + * than 256 kByte, but + */ + ret = 1; + } + msg_pdbg("Maximum IT8705F parallel flash decode size is %u.\n", + max_rom_decode.parallel); + if (ret) { + msg_pinfo("Not enabling IT8705F flash write.\n"); + } else { + sio_write(port, 0x24, tmp); + } + } else { + msg_pdbg("No IT8705F flash segment enabled.\n"); + /* Not sure if this is an error or not. */ + ret = 0; + } exit_conf_mode_ite(port);
- return 0; + return ret; }
-/** - * Suited for: - * - AOpen vKM400Am-S: VIA KM400 + VT8237 + IT8705F. - * - Biostar P4M80-M4: VIA P4M800 + VT8237 + IT8705AF - * - Elitegroup K7S6A: SiS745 + ITE IT8705F - * - Elitegroup K7VTA3: VIA Apollo KT266/A/333 + VIA VT8235 + ITE IT8705F - * - GIGABYTE GA-7VT600: VIA KT600 + VT8237 + IT8705 - * - Shuttle AK38N: VIA KT333CF + VIA VT8235 + ITE IT8705F - * - * The SIS950 Super I/O probably requires the same flash write enable. - */ -static int it8705f_write_enable_2e(void) -{ - return it8705f_write_enable(0x2e); -} - static int pc87360_gpio_set(uint8_t gpio, int raise) { static const int bankbase[] = {0, 4, 8, 10, 12}; @@ -1590,7 +1651,6 @@ {0x105a, 0x0d30, 0x105a, 0x4d33, 0x8086, 0x1130, 0x8086, 0, NULL, NULL, NULL, "Acorp", "6A815EPD", 0, OK, board_acorp_6a815epd}, {0x1022, 0x746B, 0, 0, 0, 0, 0, 0, NULL, "AGAMI", "ARUMA", "agami", "Aruma", 0, OK, w83627hf_gpio24_raise_2e}, {0x1106, 0x3177, 0x17F2, 0x3177, 0x1106, 0x3148, 0x17F2, 0x3148, NULL, NULL, NULL, "Albatron", "PM266A Pro", 0, OK, w836xx_memw_enable_2e}, - {0x1106, 0x3205, 0x1106, 0x3205, 0x10EC, 0x8139, 0xA0A0, 0x0477, NULL, NULL, NULL, "AOpen", "vKM400Am-S", 0, OK, it8705f_write_enable_2e}, {0x1022, 0x2090, 0, 0, 0x1022, 0x2080, 0, 0, NULL, "artecgroup", "dbe61", "Artec Group", "DBE61", 0, OK, board_artecgroup_dbe6x}, {0x1022, 0x2090, 0, 0, 0x1022, 0x2080, 0, 0, NULL, "artecgroup", "dbe62", "Artec Group", "DBE62", 0, OK, board_artecgroup_dbe6x}, {0x8086, 0x24D4, 0x1849, 0x24D0, 0x8086, 0x24D5, 0x1849, 0x9739, NULL, NULL, NULL, "ASRock", "P4i65GV", 0, OK, intel_ich_gpio23_raise}, @@ -1611,14 +1671,11 @@ {0x8086, 0x2570, 0x1043, 0x80F2, 0x105A, 0x3373, 0x1043, 0x80F5, NULL, NULL, NULL, "ASUS", "P4P800-E Deluxe", 0, OK, intel_ich_gpio21_raise}, {0x10B9, 0x1541, 0, 0, 0x10B9, 0x1533, 0, 0, "^P5A$", "asus", "p5a", "ASUS", "P5A", 0, OK, board_asus_p5a}, {0x10DE, 0x0030, 0x1043, 0x818a, 0x8086, 0x100E, 0x1043, 0x80EE, NULL, NULL, NULL, "ASUS", "P5ND2-SLI Deluxe", 0, OK, nvidia_mcp_gpio10_raise}, - {0x1106, 0x3149, 0x1565, 0x3206, 0x1106, 0x3344, 0x1565, 0x1202, NULL, NULL, NULL, "Biostar", "P4M80-M4", 0, OK, it8705f_write_enable_2e}, {0x8086, 0x3590, 0x1028, 0x016c, 0x1000, 0x0030, 0x1028, 0x016c, NULL, NULL, NULL, "Dell", "PowerEdge 1850", 0, OK, intel_ich_gpio23_raise}, - {0x1039, 0x5513, 0x1019, 0x0A41, 0x1039, 0x0018, 0, 0, NULL, NULL, NULL, "Elitegroup", "K7S6A", 0, OK, it8705f_write_enable_2e}, - {0x1106, 0x3038, 0x1019, 0x0996, 0x1106, 0x3177, 0x1019, 0x0996, NULL, NULL, NULL, "Elitegroup", "K7VTA3", 256, OK, it8705f_write_enable_2e}, + {0x1106, 0x3038, 0x1019, 0x0996, 0x1106, 0x3177, 0x1019, 0x0996, NULL, NULL, NULL, "Elitegroup", "K7VTA3", 256, OK, NULL}, {0x1106, 0x3177, 0x1106, 0x3177, 0x1106, 0x3059, 0x1695, 0x3005, NULL, NULL, NULL, "EPoX", "EP-8K5A2", 0, OK, w836xx_memw_enable_2e}, {0x10EC, 0x8139, 0x1695, 0x9001, 0x11C1, 0x5811, 0x1695, 0x9015, NULL, NULL, NULL, "EPoX", "EP-8RDA3+", 0, OK, nvidia_mcp_gpio31_raise}, {0x8086, 0x7110, 0, 0, 0x8086, 0x7190, 0, 0, NULL, "epox", "ep-bx3", "EPoX", "EP-BX3", 0, OK, board_epox_ep_bx3}, - {0x1106, 0x3227, 0x1458, 0x5001, 0x10ec, 0x8139, 0x1458, 0xe000, NULL, NULL, NULL, "GIGABYTE", "GA-7VT600", 0, OK, it8705f_write_enable_2e}, {0x1106, 0x0686, 0x1106, 0x0686, 0x1106, 0x3058, 0x1458, 0xa000, NULL, NULL, NULL, "GIGABYTE", "GA-7ZM", 512, OK, NULL}, {0x10DE, 0x0050, 0x1458, 0x0C11, 0x10DE, 0x005e, 0x1458, 0x5000, NULL, NULL, NULL, "GIGABYTE", "GA-K8N-SLI", 0, OK, nvidia_mcp_gpio21_raise}, {0x1166, 0x0223, 0x103c, 0x320d, 0x14e4, 0x1678, 0x103c, 0x703e, NULL, "hp", "dl145_g3", "HP", "DL145 G3", 0, OK, board_hp_dl145_g3_enable}, @@ -1644,7 +1701,7 @@ {0x10DE, 0x0270, 0x1462, 0x7207, 0x10DE, 0x0264, 0x1462, 0x7207, NULL, NULL, NULL, "MSI", "MS-7207 (K8N GM2-L)", 0, NT, nvidia_mcp_gpio2_raise}, {0x1011, 0x0019, 0xaa55, 0xaa55, 0x8086, 0x7190, 0, 0, NULL, NULL, NULL, "Nokia", "IP530", 0, OK, fdc37b787_gpio50_raise_3f0}, {0x1106, 0x3099, 0, 0, 0x1106, 0x3074, 0, 0, NULL, "shuttle", "ak31", "Shuttle", "AK31", 0, OK, w836xx_memw_enable_2e}, - {0x1106, 0x3104, 0x1297, 0xa238, 0x1106, 0x3059, 0x1297, 0xc063, NULL, NULL, NULL, "Shuttle", "AK38N", 256, OK, it8705f_write_enable_2e}, + {0x1106, 0x3104, 0x1297, 0xa238, 0x1106, 0x3059, 0x1297, 0xc063, NULL, NULL, NULL, "Shuttle", "AK38N", 256, OK, NULL}, {0x10DE, 0x0050, 0x1297, 0x5036, 0x1412, 0x1724, 0x1297, 0x5036, NULL, NULL, NULL, "Shuttle", "FN25", 0, OK, board_shuttle_fn25}, {0x1106, 0x3038, 0x0925, 0x1234, 0x1106, 0x3058, 0x15DD, 0x7609, NULL, NULL, NULL, "Soyo", "SY-7VCA", 0, OK, via_apollo_gpo0_lower}, {0x1106, 0x3038, 0x0925, 0x1234, 0x1106, 0x0596, 0x1106, 0, NULL, NULL, NULL, "Tekram", "P6Pro-A5", 256, OK, NULL},