[flashrom] [PATCH] ICH SPI fixes/cleanup
David Hendricks
dhendrix at google.com
Thu Feb 11 05:31:02 CET 2010
On Thu, Dec 17, 2009 at 2:15 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:
> This megapatch rewrites substantial parts of ICH SPI to actually do what
> the SPI layer wants instead of its own weird idea about commands
> (running unrequested commands, running modified commands).
> Besides that, there is a fair share of cleanups as well.
>
> - Add JEDEC_EWSR (Enable Write Status Register) to default commands.
> - Mark a no longer used opcode/preopcode table as unused.
> - Declare all commands as non-atomic/standalone by default. The ICH SPI
> driver has no business executing commands (preopcodes) automatically if
> they were not requested.
> - Automatically adjust preopcode/opcode pairings (like WREN+ERASE) based
> on what the SPI layer requested. The ICH SPI driver has no business
> executing altered opcode pairs as it sees fit.
> - Fix incomplete initialization in the case of a locked down chipset.
> Leaving the first 4 opcodes with uninitialized pairings had
> unpredictable results.
> - switch() exists for a reason. Nested if() checking on the same
> variable is an interesting style.
> - Actually check if the requested readcnt/writecnt for a command is
> supported by the hardware instead of delivering corrupt/incomplete
> commands and data.
> - If a command has unsupported readlen/writelen, complain loudly to the
> user.
> - Use find_opcode instead of open-coding the same stuff in a dozen
> variations.
> - Introduce infrastructure for updating the command set of unlocked
> chipsets on the fly.
>
> Untested. This will expose any bugs in the SPI layer (I know of one bug
> regarding write enables there), but the behaviour will now be consistent
> across all SPI drivers.
>
> Read/write/erase logs in full verbosity are appreciated.
>
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>
> Index: flashrom-ichspi_cleanup_checklen_lockedfix/ichspi.c
> ===================================================================
> --- flashrom-ichspi_cleanup_checklen_lockedfix/ichspi.c (Revision 807)
> +++ flashrom-ichspi_cleanup_checklen_lockedfix/ichspi.c (Arbeitskopie)
> @@ -5,6 +5,7 @@
> * Copyright (C) 2008 Claus Gindhart <claus.gindhart at kontron.com>
> * Copyright (C) 2008 Dominik Geyer <dominik.geyer at kontron.com>
> * Copyright (C) 2008 coresystems GmbH <info at coresystems.de>
> + * Copyright (C) 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
> @@ -105,7 +106,7 @@
> uint8_t atomic; //Use preop: (0: none, 1: preop0, 2: preop1
> } OPCODE;
>
> -/* Opcode definition:
> +/* Suggested opcode definition:
> * Preop 1: Write Enable
> * Preop 2: Write Status register enable
> *
> @@ -115,7 +116,7 @@
> * OP 3: Read Status register
> * OP 4: Read ID
> * OP 5: Write Status register
> - * OP 6: chip private (read JDEC id)
> + * OP 6: chip private (read JEDEC id)
> * OP 7: Chip erase
> */
> typedef struct _OPCODES {
> @@ -156,6 +157,7 @@
> uint8_t opcode;
> };
>
> +/* List of opcodes which need preopcodes and matching preopcodes. Unused.
> */
> struct preop_opcode_pair pops[] = {
> {JEDEC_WREN, JEDEC_BYTE_PROGRAM},
> {JEDEC_WREN, JEDEC_SE}, /* sector erase */
> @@ -163,24 +165,30 @@
> {JEDEC_WREN, JEDEC_BE_D8}, /* block erase */
> {JEDEC_WREN, JEDEC_CE_60}, /* chip erase */
> {JEDEC_WREN, JEDEC_CE_C7}, /* chip erase */
> + /* FIXME: WRSR requires either EWSR or WREN depending on chip
> type. */
> + {JEDEC_WREN, JEDEC_WRSR},
> {JEDEC_EWSR, JEDEC_WRSR},
> {0,}
> };
>
> +/* Reasonable default configuration. Needs ad-hoc modifications if we
> + * encounter unlisted opcodes. Fun.
> + */
> OPCODES O_ST_M25P = {
> {
> JEDEC_WREN,
> - 0},
> + JEDEC_EWSR,
> + },
> {
> - {JEDEC_BYTE_PROGRAM, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 1}, //
> Write Byte
> + {JEDEC_BYTE_PROGRAM, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0}, //
> Write Byte
> {JEDEC_READ, SPI_OPCODE_TYPE_READ_WITH_ADDRESS, 0}, // Read Data
> - {JEDEC_BE_D8, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 1}, // Erase
> Sector
> + {JEDEC_BE_D8, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0}, // Erase
> Sector
> {JEDEC_RDSR, SPI_OPCODE_TYPE_READ_NO_ADDRESS, 0}, // Read
> Device Status Reg
> {JEDEC_REMS, SPI_OPCODE_TYPE_READ_WITH_ADDRESS, 0}, // Read
> Electronic Manufacturer Signature
> - {JEDEC_WRSR, SPI_OPCODE_TYPE_WRITE_NO_ADDRESS, 1}, // Write
> Status Register
> + {JEDEC_WRSR, SPI_OPCODE_TYPE_WRITE_NO_ADDRESS, 0}, // Write
> Status Register
> {JEDEC_RDID, SPI_OPCODE_TYPE_READ_NO_ADDRESS, 0}, // Read JDEC
> ID
> - {JEDEC_CE_C7, SPI_OPCODE_TYPE_WRITE_NO_ADDRESS, 1}, // Bulk
> erase
> - }
> + {JEDEC_CE_C7, SPI_OPCODE_TYPE_WRITE_NO_ADDRESS, 0}, // Bulk
> erase
> + }
> };
>
> OPCODES O_EXISTING = {};
> @@ -209,9 +217,10 @@
> return -1;
> }
>
> +/* Create a struct OPCODES based on what we find in the locked down
> chipset. */
> static int generate_opcodes(OPCODES * op)
> {
> - int a, b, i;
> + int a;
> uint16_t preop, optype;
> uint32_t opmenu[2];
>
> @@ -257,17 +266,10 @@
> opmenu[1] >>= 8;
> }
>
> - /* atomic (link opcode with required pre-op) */
> - for (a = 4; a < 8; a++)
> + /* No preopcodes used by default. */
> + for (a = 0; a < 8; a++)
> op->opcode[a].atomic = 0;
>
> - for (i = 0; pops[i].opcode; i++) {
> - a = find_opcode(op, pops[i].opcode);
> - b = find_preop(op, pops[i].preop);
> - if ((a != -1) && (b != -1))
> - op->opcode[a].atomic = (uint8_t) ++b;
> - }
> -
> return 0;
> }
>
> @@ -431,14 +433,15 @@
> temp16 |= ((uint16_t) (opcode_index & 0x07)) << 4;
>
> /* Handle Atomic */
> - if (op.atomic != 0) {
> - /* Select atomic command */
> + switch (op.atomic) {
> + case 2:
> + /* Select second preop. */
> + temp16 |= SPIC_SPOP;
> + /* And fall through. */
> + case 1:
> + /* Atomic command (preop+op) */
> temp16 |= SPIC_ACS;
> - /* Select prefix opcode */
> - if ((op.atomic - 1) == 1) {
> - /*Select prefix opcode 2 */
> - temp16 |= SPIC_SPOP;
> - }
> + break;
> }
>
> /* Start */
> @@ -548,14 +551,15 @@
> temp32 |= ((uint32_t) (opcode_index & 0x07)) << (8 + 4);
>
> /* Handle Atomic */
> - if (op.atomic != 0) {
> - /* Select atomic command */
> + switch (op.atomic) {
> + case 2:
> + /* Select second preop. */
> + temp32 |= SSFC_SPOP;
> + /* And fall through. */
> + case 1:
> + /* Atomic command (preop+op) */
> temp32 |= SSFC_ACS;
> - /* Selct prefix opcode */
> - if ((op.atomic - 1) == 1) {
> - /*Select prefix opcode 2 */
> - temp32 |= SSFC_SPOP;
> - }
> + break;
> }
>
> /* Start */
> @@ -598,16 +602,28 @@
> {
> switch (spi_controller) {
> case SPI_CONTROLLER_VIA:
> - if (datalength > 16)
> + if (datalength > 16) {
> + fprintf(stderr, "%s: Internal command size error
> for "
> + "opcode 0x%02x, got datalength=%i, want
> <=16\n",
> + __func__, op.opcode, datalength);
> return SPI_INVALID_LENGTH;
> + }
> return ich7_run_opcode(op, offset, datalength, data, 16);
> case SPI_CONTROLLER_ICH7:
> - if (datalength > 64)
> + if (datalength > 64) {
> + fprintf(stderr, "%s: Internal command size error
> for "
> + "opcode 0x%02x, got datalength=%i, want
> <=16\n",
> + __func__, op.opcode, datalength);
> return SPI_INVALID_LENGTH;
> + }
> return ich7_run_opcode(op, offset, datalength, data, 64);
> case SPI_CONTROLLER_ICH9:
> - if (datalength > 64)
> + if (datalength > 64) {
> + fprintf(stderr, "%s: Internal command size error
> for "
> + "opcode 0x%02x, got datalength=%i, want
> <=16\n",
> + __func__, op.opcode, datalength);
> return SPI_INVALID_LENGTH;
> + }
> return ich9_run_opcode(op, offset, datalength, data);
> default:
> printf_debug("%s: unsupported chipset\n", __func__);
> @@ -686,7 +702,6 @@
> int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> const unsigned char *writearr, unsigned char *readarr)
> {
> - int a;
> int result;
> int opcode_index = -1;
> const unsigned char cmd = *writearr;
> @@ -696,21 +711,54 @@
> int count;
>
> /* find cmd in opcodes-table */
> - for (a = 0; a < 8; a++) {
> - if ((curopcodes->opcode[a]).opcode == cmd) {
> - opcode_index = a;
> - break;
> - }
> - }
> -
> - /* unknown / not programmed command */
> + opcode_index = find_opcode(curopcodes, cmd);
> if (opcode_index == -1) {
> + /* FIXME: Reprogram opcodes if possible. Autodetect type of
> + * opcode by checking readcnt/writecnt.
> + */
> printf_debug("Invalid OPCODE 0x%02x\n", cmd);
> return SPI_INVALID_OPCODE;
> }
>
> opcode = &(curopcodes->opcode[opcode_index]);
>
> + /* The following valid writecnt/readcnt combinations exist:
> + * writecnt = 4, readcnt >= 0
> + * writecnt = 1, readcnt >= 0
> + * writecnt >= 4, readcnt = 0
> + * writecnt >= 1, readcnt = 0
> + * writecnt >= 1 is guaranteed for all commands.
> + */
> + if ((opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS) &&
> + (writecnt != 4)) {
> + fprintf(stderr, "%s: Internal command size error for opcode
> "
> + "0x%02x, got writecnt=%i, want =4\n", __func__,
> cmd,
> + writecnt);
> + return SPI_INVALID_LENGTH;
> + }
> + if ((opcode->spi_type == SPI_OPCODE_TYPE_READ_NO_ADDRESS) &&
> + (writecnt != 1)) {
> + fprintf(stderr, "%s: Internal command size error for opcode
> "
> + "0x%02x, got writecnt=%i, want =1\n", __func__,
> cmd,
> + writecnt);
> + return SPI_INVALID_LENGTH;
> + }
> + if ((opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) &&
> + (writecnt < 4)) {
> + fprintf(stderr, "%s: Internal command size error for opcode
> "
> + "0x%02x, got writecnt=%i, want >=4\n", __func__,
> cmd,
> + writecnt);
> + return SPI_INVALID_LENGTH;
> + }
> + if (((opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) ||
> + (opcode->spi_type == SPI_OPCODE_TYPE_WRITE_NO_ADDRESS)) &&
> + (readcnt)) {
> + fprintf(stderr, "%s: Internal command size error for opcode
> "
> + "0x%02x, got readcnt=%i, want =0\n", __func__, cmd,
> + readcnt);
> + return SPI_INVALID_LENGTH;
> + }
> +
> /* if opcode-type requires an address */
> if (opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS ||
> opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) {
> @@ -741,23 +789,69 @@
> int ich_spi_send_multicommand(struct spi_command *cmds)
> {
> int ret = 0;
> + int i;
> int oppos, preoppos;
> for (; (cmds->writecnt || cmds->readcnt) && !ret; cmds++) {
> - /* Is the next command valid or a terminator? */
> if ((cmds + 1)->writecnt || (cmds + 1)->readcnt) {
> + /* Next command is valid. */
> preoppos = find_preop(curopcodes,
> cmds->writearr[0]);
> oppos = find_opcode(curopcodes, (cmds +
> 1)->writearr[0]);
> - /* Is the opcode of the current command listed in
> the
> - * ICH struct OPCODES as associated preopcode for
> the
> - * opcode of the next command?
> + if ((oppos == -1) && (preoppos != -1)) {
> + /* Current command is listed as preopcode
> in
> + * ICH struct OPCODES, but next command is
> not
> + * listed as opcode in that struct.
> + * Check for command sanity, then
> + * try to reprogram the ICH opcode list.
> + */
> + if (find_preop(curopcodes,
> + (cmds + 1)->writearr[0]) !=
> -1) {
> + fprintf(stderr, "%s: Two subsequent
> "
> + "preopcodes 0x%02x and
> 0x%02x, "
> + "ignoring the first.\n",
> + __func__,
> cmds->writearr[0],
> + (cmds + 1)->writearr[0]);
> + continue;
> + }
> + /* If the chipset is locked down, we'll
> fail
> + * during execution of the next command
> anyway.
> + * No need to bother with fixups.
> + */
> + if (!ichspi_lock) {
> + printf_debug("%s: FIXME: Add
> on-the-fly"
> + " reprogramming of the
> "
> + "chipset opcode
> list.\n",
> + __func__);
> + /* FIXME: Reprogram opcode menu.
> + * Find a less-useful opcode,
> replace it
> + * with the wanted opcode, detect
> optype
> + * and reprogram the opcode menu.
> + * Update oppos so the next
> if-statement
> + * can do something useful.
> + */
> +
> //curopcodes.opcode[lessusefulindex] = (cmds + 1)->writearr[0]);
> + //update_optypes(curopcodes);
> + //program_opcodes(curopcodes);
> + //oppos = find_opcode(curopcodes,
> (cmds + 1)->writearr[0]);
> + continue;
> + }
> + }
> + if ((oppos != -1) && (preoppos != -1)) {
> + /* Current command is listed as preopcode
> in
> + * ICH struct OPCODES and next command is
> listed
> + * as opcode in that struct. Match them up.
> + */
> + curopcodes->opcode[oppos].atomic = preoppos
> + 1;
> + continue;
> + }
> + /* If none of the above if-statements about oppos
> or
> + * preoppos matched, this is a normal opcode.
> */
> - if ((oppos != -1) && (preoppos != -1) &&
> - ((curopcodes->opcode[oppos].atomic - 1) ==
> preoppos))
> - continue;
> - }
> -
> + }
> ret = ich_spi_send_command(cmds->writecnt, cmds->readcnt,
> cmds->writearr, cmds->readarr);
> + /* Reset the type of all opcodes to non-atomic. */
> + for (i = 0; i < 8; i++)
> + curopcodes->opcode[i].atomic = 0;
> }
> return ret;
> }
>
>
> --
> Developer quote of the month:
> "We are juggling too many chainsaws and flaming arrows and tigers."
>
>
> _______________________________________________
> flashrom mailing list
> flashrom at flashrom.org
> http://www.flashrom.org/mailman/listinfo/flashrom
>
I tested this out on an NM10-based reference board, which is based off the
ICH7, so at least we can say it was tested :-)
The patch also fixes a bad assumption about preopcodes (Carl-Daniel's third
comment), so now it works with my SST25VF016B SPI flash chip.
I'm not a committer, but for what it's worth:
Acked-by: David Hendricks <dhendrix at google.com>
--
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20100210/883a6995/attachment.html>
More information about the flashrom
mailing list