Patchworkβ ICH SPI fixes/cleanup

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2009-12-17 22:15:37
Message ID <4B2AAD89.1000502@gmx.net>
Download mbox | patch
Permalink /patch/663/
State Accepted
Commit r894
Headers show

Comments

Carl-Daniel Hailfinger - 2009-12-17 22:15:37
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@gmx.net>
David Hendricks - 2010-02-11 04:31:02
On Thu, Dec 17, 2009 at 2:15 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006@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@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@kontron.com>
>  * Copyright (C) 2008 Dominik Geyer <dominik.geyer@kontron.com>
>  * Copyright (C) 2008 coresystems GmbH <info@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@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@google.com>
Carl-Daniel Hailfinger - 2010-02-11 11:29:56
On 11.02.2010 05:31, David Hendricks wrote:
> On Thu, Dec 17, 2009 at 2:15 PM, Carl-Daniel Hailfinger <
> c-d.hailfinger.devel.2006@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.
>>
>>     
> 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@google.com>
>   

Thanks for the review.
Committed in r894.

Regards,
Carl-Daniel

Patch

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@kontron.com>
  * Copyright (C) 2008 Dominik Geyer <dominik.geyer@kontron.com>
  * Copyright (C) 2008 coresystems GmbH <info@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;
 }