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