Index: flashrom-ichspi_cleanup_checklen_lockedfix/ichspi.c

17 Dec 2009

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."



    

[PATCH] ICH SPI fixes/cleanup

Index: flashrom-ichspi_cleanup_checklen_lockedfix/ichspi.c