Some people have reported that running flashrom on Ibex Peak (after patching in the PCI ID 0x8086:0x3b02) will not only hang flashrom during the first RES/REMS command issued, it may even cause the machine to lock up or issue a hard reset. We suspect this is because RES/REMS are commands with an address, whereas RDID does not take an address and works fine.
Our current ICH SPI code doesn't really follow the datasheets and uses lots of hope instead of error checking in the wrong places.
This patch will fail early and fail often if something is odd. As such, merge is not recommended, but running it should definitely generating interesting results on all Intel chipsets with SPI, and especially on Ibex Peak. It probably won't fix the hang, but it should be pretty good at spewing an error before the machine dies.
Untested. Highly likely to explode on VIA chipsets which use the same interface but have docs only under NDA.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-ich_spi_fcerr_fcdone_paranoia/chipset_enable.c =================================================================== --- flashrom-ich_spi_fcerr_fcdone_paranoia/chipset_enable.c (Revision 1052) +++ flashrom-ich_spi_fcerr_fcdone_paranoia/chipset_enable.c (Arbeitskopie) @@ -579,8 +579,20 @@ mmio_readl(spibar + 0x80)); msg_pdbg("0x84: 0x%08x (PR4)\n", mmio_readl(spibar + 0x84)); - msg_pdbg("0x90: 0x%08x (SSFS, SSFC)\n", - mmio_readl(spibar + 0x90)); + + tmp = mmio_readl(spibar + 0x90); + msg_pdbg("0x90: 0x%02x (SSFS)\n", tmp & 0xff); + msg_pdbg("AEL %i, ", (tmp >> 4) & 1); + msg_pdbg("FCERR %i, ", (tmp >> 3) & 1); + msg_pdbg("FDONE %i, ", (tmp >> 2) & 1); + msg_pdbg("SCIP %i\n", (tmp >> 0) & 1); + if (tmp & (1 << 3)) { + msg_pdbg("Clearing SSFS.FCERR\n"); + mmio_writeb(1 << 3, spibar + 0x90); + } + tmp >>= 8; + msg_pdbg("0x91: 0x%06x (SSFC)\n", tmp); + msg_pdbg("0x94: 0x%04x (PREOP)\n", mmio_readw(spibar + 0x94)); msg_pdbg("0x96: 0x%04x (OPTYPE)\n", Index: flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c =================================================================== --- flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c (Revision 1052) +++ flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c (Arbeitskopie) @@ -413,6 +413,12 @@ write_cmd = 1; }
+ /* FIXME: This should be a time-limited while loop. */ + if (REGREAD16(ICH7_REG_SPIS) & SPIS_SCIP) { + msg_perr("Error: Totally unhandled SCIP condition!\n"); + return 1; + } + /* Programm Offset in Flash into FADDR */ REGWRITE32(ICH7_REG_SPIA, (offset & 0x00FFFFFF)); /* SPI addresses are 24 BIT only */
@@ -486,18 +492,22 @@ /* write it */ REGWRITE16(ICH7_REG_SPIC, temp16);
- /* wait for cycle complete */ - timeout = 100 * 1000 * 60; // 60s is a looong timeout. - while (((REGREAD16(ICH7_REG_SPIS) & SPIS_CDS) == 0) && --timeout) { + /* Wait for Cycle Done Status or Flash Cycle Error. */ + timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */ + while (((REGREAD16(ICH7_REG_SPIS) & (SPIS_CDS | SPIS_FCERR)) == 0) && + --timeout) { programmer_delay(10); } if (!timeout) { msg_perr("timeout\n"); + return 1; }
/* FIXME: make sure we do not needlessly cause transaction errors. */ - if ((REGREAD16(ICH7_REG_SPIS) & SPIS_FCERR) != 0) { - msg_pdbg("Transaction error!\n"); + if (REGREAD16(ICH7_REG_SPIS) & SPIS_FCERR) { + msg_perr("Transaction error for opcode 0x%02x!\n", + op.opcode); + REGWRITE16(ICH7_REG_SPIS, SPIS_FCERR); return 1; }
@@ -532,6 +542,12 @@ write_cmd = 1; }
+ /* FIXME: This should be a time-limited while loop. */ + if (REGREAD16(ICH9_REG_SSFS) & SSFS_SCIP) { + msg_perr("Error: Totally unhandled SCIP condition!\n"); + return 1; + } + /* Programm Offset in Flash into FADDR */ REGWRITE32(ICH9_REG_FADDR, (offset & 0x00FFFFFF)); /* SPI addresses are 24 BIT only */
@@ -605,18 +621,23 @@ /* write it */ REGWRITE32(ICH9_REG_SSFS, temp32);
- /*wait for cycle complete */ - timeout = 100 * 1000 * 60; // 60s is a looong timeout. - while (((REGREAD32(ICH9_REG_SSFS) & SSFS_CDS) == 0) && --timeout) { + /* Wait for Cycle Done Status or Flash Cycle Error. */ + timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */ + while (((REGREAD32(ICH9_REG_SSFS) & (SSFS_CDS | SSFS_FCERR)) == 0) && + --timeout) { programmer_delay(10); } if (!timeout) { msg_perr("timeout\n"); + return 1; }
/* FIXME make sure we do not needlessly cause transaction errors. */ - if ((REGREAD32(ICH9_REG_SSFS) & SSFS_FCERR) != 0) { - msg_pdbg("Transaction error!\n"); + if (REGREAD32(ICH9_REG_SSFS) & SSFS_FCERR) { + msg_perr("Transaction error for opcode 0x%02x!\n", + op.opcode); + //FIXME: Really REGWRITE8? + REGWRITE8(ICH9_REG_SSFS, SSFS_FCERR); return 1; }
Untested. Highly likely to explode on VIA chipsets which use the same interface but have docs only under NDA.
No true. Check the VX800 datasheet, page 445 (in xpdf).
http://linux.via.com.tw/support/beginDownload.action?eleid=161&fid=241
I can test it this evening, Differences between ICH7 and VIA are described in source code:
/* VIA SPI is compatible with ICH7, but maxdata to transfer is 16 bytes.
DATA byte count on ICH7 is 8:13, on VIA 8:11
bit 12 is port select CS0 CS1 bit 13 is FAST READ enable bit 7 is used with fast read and one shot controls CS de-assert? */
Thanks, Rudolf
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
It works on VT8237S, or at least it works while trying -v -w -r
Thanks, Rudolf
On 23.06.2010 21:58, Rudolf Marek wrote:
It works on VT8237S, or at least it works while trying -v -w -r
Great. Updated version follows.
Our current ICH SPI code doesn't really follow the datasheets and uses lots of hope instead of error checking in the wrong places.
This patch will fail early and fail often if something is odd. As such, merge is not recommended, but running it should definitely generating interesting results on all Intel chipsets with SPI. It probably won't fix the hang, but it should be pretty good at spewing an error before the machine dies.
Tested with VT8237S SPI.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c =================================================================== --- flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c (Revision 1217) +++ flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c (Arbeitskopie) @@ -485,6 +485,12 @@ write_cmd = 1; }
+ /* FIXME: This should be a time-limited while loop. */ + if (REGREAD16(ICH7_REG_SPIS) & SPIS_SCIP) { + msg_perr("Error: Totally unhandled SCIP condition!\n"); + return 1; + } + /* Programm Offset in Flash into FADDR */ REGWRITE32(ICH7_REG_SPIA, (offset & 0x00FFFFFF)); /* SPI addresses are 24 BIT only */
@@ -558,18 +564,23 @@ /* write it */ REGWRITE16(ICH7_REG_SPIC, temp16);
- /* wait for cycle complete */ - timeout = 100 * 1000 * 60; // 60s is a looong timeout. - while (((REGREAD16(ICH7_REG_SPIS) & SPIS_CDS) == 0) && --timeout) { + /* Wait for Cycle Done Status or Flash Cycle Error. */ + timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */ + while (((REGREAD16(ICH7_REG_SPIS) & (SPIS_CDS | SPIS_FCERR)) == 0) && + --timeout) { programmer_delay(10); } if (!timeout) { - msg_perr("timeout\n"); + msg_perr("timeout, ICH7_REG_SPIS=0x%04x\n", + REGREAD16(ICH7_REG_SPIS)); + return 1; }
/* FIXME: make sure we do not needlessly cause transaction errors. */ - if ((REGREAD16(ICH7_REG_SPIS) & SPIS_FCERR) != 0) { - msg_pdbg("Transaction error!\n"); + if (REGREAD16(ICH7_REG_SPIS) & SPIS_FCERR) { + msg_perr("Transaction error for opcode 0x%02x!\n", + op.opcode); + REGWRITE16(ICH7_REG_SPIS, SPIS_FCERR); return 1; }
@@ -604,6 +615,12 @@ write_cmd = 1; }
+ /* FIXME: This should be a time-limited while loop. */ + if (REGREAD16(ICH9_REG_SSFS) & SSFS_SCIP) { + msg_perr("Error: Totally unhandled SCIP condition!\n"); + return 1; + } + /* Programm Offset in Flash into FADDR */ REGWRITE32(ICH9_REG_FADDR, (offset & 0x00FFFFFF)); /* SPI addresses are 24 BIT only */
@@ -679,18 +696,24 @@ /* write it */ REGWRITE32(ICH9_REG_SSFS, temp32);
- /*wait for cycle complete */ - timeout = 100 * 1000 * 60; // 60s is a looong timeout. - while (((REGREAD32(ICH9_REG_SSFS) & SSFS_CDS) == 0) && --timeout) { + /* Wait for Cycle Done Status or Flash Cycle Error. */ + timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */ + while (((REGREAD32(ICH9_REG_SSFS) & (SSFS_CDS | SSFS_FCERR)) == 0) && + --timeout) { programmer_delay(10); } if (!timeout) { - msg_perr("timeout\n"); + msg_perr("timeout, ICH9_REG_SSFS=0x%08x\n", + REGREAD32(ICH9_REG_SSFS)); + return 1; }
/* FIXME make sure we do not needlessly cause transaction errors. */ - if ((REGREAD32(ICH9_REG_SSFS) & SSFS_FCERR) != 0) { - msg_pdbg("Transaction error!\n"); + if (REGREAD32(ICH9_REG_SSFS) & SSFS_FCERR) { + msg_perr("Transaction error for opcode 0x%02x!\n", + op.opcode); + //FIXME: Really REGWRITE8? + REGWRITE8(ICH9_REG_SSFS, SSFS_FCERR); return 1; }
@@ -1069,8 +1092,20 @@ mmio_readl(ich_spibar + 0x80)); msg_pdbg("0x84: 0x%08x (PR4)\n", mmio_readl(ich_spibar + 0x84)); - msg_pdbg("0x90: 0x%08x (SSFS, SSFC)\n", - mmio_readl(ich_spibar + 0x90)); + + tmp = mmio_readl(ich_spibar + 0x90); + msg_pdbg("0x90: 0x%02x (SSFS)\n", tmp & 0xff); + msg_pdbg("AEL %i, ", (tmp >> 4) & 1); + msg_pdbg("FCERR %i, ", (tmp >> 3) & 1); + msg_pdbg("FDONE %i, ", (tmp >> 2) & 1); + msg_pdbg("SCIP %i\n", (tmp >> 0) & 1); + if (tmp & (1 << 3)) { + msg_pdbg("Clearing SSFS.FCERR\n"); + mmio_writeb(1 << 3, ich_spibar + 0x90); + } + tmp >>= 8; + msg_pdbg("0x91: 0x%06x (SSFC)\n", tmp); + msg_pdbg("0x94: 0x%04x (PREOP)\n", mmio_readw(ich_spibar + 0x94)); msg_pdbg("0x96: 0x%04x (OPTYPE)\n",
Hi
I guess you want to test me on VIA right? I hope I can find some time later but I dont think I can manage this today.
Thanks, Rudolf
On 02.11.2010 23:14, Rudolf Marek wrote:
Hi
I guess you want to test me on VIA right? I hope I can find some time later but I dont think I can manage this today.
I need to reread the SPI chapter in the VIA datasheet anyway before I ask for a merge.
Regards, Carl-Daniel
Hi
It works here no errors reported. Is there anything else I can try?
I tried writing to empry flashchip -Vw
Thanks, Rudolf
New version.
Proper error handling for ICH/VIA SPI: Use 16-bit values for bit masks in 16-bit registers. Check for SPI Cycle In Progress and wait up to 60 ms. Do not touch reserved bits. Reduce SPI cycle timeout from 60 s to 60 ms. Clear transaction errors caused by our own SPI accesses. Add better debugging in case the hardware misbehaves.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c =================================================================== --- flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c (Revision 1236) +++ flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c (Arbeitskopie) @@ -51,6 +51,7 @@ #define SSFS_CDS 0x00000004 #define SSFS_FCERR 0x00000008 #define SSFS_AEL 0x00000010 +#define SSFS_RESERVED_MASK 0x000000e2
#define ICH9_REG_SSFC 0x91 /* 24 Bits */ #define SSFC_SCGO 0x00000200 @@ -63,6 +64,7 @@ #define SSFC_SCF 0x01000000 #define SSFC_SCF_20MHZ 0x00000000 #define SSFC_SCF_33MHZ 0x01000000 +#define SSFC_RESERVED_MASK 0xf8008100
#define ICH9_REG_PREOP 0x94 /* 16 Bits */ #define ICH9_REG_OPTYPE 0x96 /* 16 Bits */ @@ -76,9 +78,11 @@
// ICH7 registers #define ICH7_REG_SPIS 0x00 /* 16 Bits */ -#define SPIS_SCIP 0x00000001 -#define SPIS_CDS 0x00000004 -#define SPIS_FCERR 0x00000008 +#define SPIS_SCIP 0x0001 +#define SPIS_GRANT 0x0002 +#define SPIS_CDS 0x0004 +#define SPIS_FCERR 0x0008 +#define SPIS_RESERVED_MASK 0x7ff0
/* VIA SPI is compatible with ICH7, but maxdata to transfer is 16 bytes. @@ -146,6 +150,11 @@ return mmio_readw(ich_spibar + X); }
+static uint16_t REGREAD8(int X) +{ + return mmio_readb(ich_spibar + X); +} + #define REGWRITE32(X,Y) mmio_writel(Y, ich_spibar+X) #define REGWRITE16(X,Y) mmio_writew(Y, ich_spibar+X) #define REGWRITE8(X,Y) mmio_writeb(Y, ich_spibar+X) @@ -497,6 +506,15 @@ write_cmd = 1; }
+ timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */ + while ((REGREAD16(ICH7_REG_SPIS) & SPIS_SCIP) && --timeout) { + programmer_delay(10); + } + if (!timeout) { + msg_perr("Error: SCIP never cleared!\n"); + return 1; + } + /* Programm Offset in Flash into FADDR */ REGWRITE32(ICH7_REG_SPIA, (offset & 0x00FFFFFF)); /* SPI addresses are 24 BIT only */
@@ -523,7 +541,9 @@ }
/* Assemble SPIS */ - temp16 = 0; + temp16 = REGREAD16(ICH7_REG_SPIS); + /* keep reserved bits */ + temp16 &= SPIS_RESERVED_MASK; /* clear error status registers */ temp16 |= (SPIS_CDS + SPIS_FCERR); REGWRITE16(ICH7_REG_SPIS, temp16); @@ -570,18 +590,26 @@ /* write it */ REGWRITE16(ICH7_REG_SPIC, temp16);
- /* wait for cycle complete */ - timeout = 100 * 1000 * 60; // 60s is a looong timeout. - while (((REGREAD16(ICH7_REG_SPIS) & SPIS_CDS) == 0) && --timeout) { + /* Wait for Cycle Done Status or Flash Cycle Error. */ + timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */ + while (((REGREAD16(ICH7_REG_SPIS) & (SPIS_CDS | SPIS_FCERR)) == 0) && + --timeout) { programmer_delay(10); } if (!timeout) { - msg_perr("timeout\n"); + msg_perr("timeout, ICH7_REG_SPIS=0x%04x\n", + REGREAD16(ICH7_REG_SPIS)); + return 1; }
/* FIXME: make sure we do not needlessly cause transaction errors. */ - if ((REGREAD16(ICH7_REG_SPIS) & SPIS_FCERR) != 0) { - msg_pdbg("Transaction error!\n"); + temp16 = REGREAD16(ICH7_REG_SPIS); + if (temp16 & SPIS_FCERR) { + msg_perr("Transaction error for opcode 0x%02x!\n", + op.opcode); + /* keep reserved bits */ + temp16 &= SPIS_RESERVED_MASK; + REGWRITE16(ICH7_REG_SPIS, temp16 | SPIS_FCERR); return 1; }
@@ -616,6 +644,15 @@ write_cmd = 1; }
+ timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */ + while ((REGREAD8(ICH9_REG_SSFS) & SSFS_SCIP) && --timeout) { + programmer_delay(10); + } + if (!timeout) { + msg_perr("Error: SCIP never cleared!\n"); + return 1; + } + /* Programm Offset in Flash into FADDR */ REGWRITE32(ICH9_REG_FADDR, (offset & 0x00FFFFFF)); /* SPI addresses are 24 BIT only */
@@ -641,12 +678,13 @@ }
/* Assemble SSFS + SSFC */ - /* keep reserved bits (23-19,7,0) */ temp32 = REGREAD32(ICH9_REG_SSFS); - temp32 &= 0xF8008100; - + /* keep reserved bits */ + temp32 &= SSFS_RESERVED_MASK | SSFC_RESERVED_MASK; /* clear error status registers */ temp32 |= (SSFS_CDS + SSFS_FCERR); + REGWRITE32(ICH9_REG_SSFS, temp32); + /* Use 20 MHz */ temp32 |= SSFC_SCF_20MHZ;
@@ -691,18 +729,27 @@ /* write it */ REGWRITE32(ICH9_REG_SSFS, temp32);
- /*wait for cycle complete */ - timeout = 100 * 1000 * 60; // 60s is a looong timeout. - while (((REGREAD32(ICH9_REG_SSFS) & SSFS_CDS) == 0) && --timeout) { + /* Wait for Cycle Done Status or Flash Cycle Error. */ + timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */ + while (((REGREAD32(ICH9_REG_SSFS) & (SSFS_CDS | SSFS_FCERR)) == 0) && + --timeout) { programmer_delay(10); } if (!timeout) { - msg_perr("timeout\n"); + msg_perr("timeout, ICH9_REG_SSFS=0x%08x\n", + REGREAD32(ICH9_REG_SSFS)); + return 1; }
/* FIXME make sure we do not needlessly cause transaction errors. */ - if ((REGREAD32(ICH9_REG_SSFS) & SSFS_FCERR) != 0) { - msg_pdbg("Transaction error!\n"); + temp32 = REGREAD32(ICH9_REG_SSFS); + if (temp32 & SSFS_FCERR) { + msg_perr("Transaction error for opcode 0x%02x!\n", + op.opcode); + /* keep reserved bits */ + temp32 &= SSFS_RESERVED_MASK | SSFC_RESERVED_MASK; + /* Clear the transaction error. */ + REGWRITE32(ICH9_REG_SSFS, temp32 | SSFS_FCERR); return 1; }
@@ -1042,7 +1089,6 @@ msg_pdbg("0x%02x: 0x%08x (PBR%d)\n", offs, mmio_readl(ich_spibar + offs), i); } - msg_pdbg("\n"); if (mmio_readw(ich_spibar) & (1 << 15)) { msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n"); ichspi_lock = 1; @@ -1082,8 +1128,20 @@ mmio_readl(ich_spibar + 0x80)); msg_pdbg("0x84: 0x%08x (PR4)\n", mmio_readl(ich_spibar + 0x84)); - msg_pdbg("0x90: 0x%08x (SSFS, SSFC)\n", - mmio_readl(ich_spibar + 0x90)); + + tmp = mmio_readl(ich_spibar + 0x90); + msg_pdbg("0x90: 0x%02x (SSFS)\n", tmp & 0xff); + msg_pdbg("AEL %i, ", (tmp >> 4) & 1); + msg_pdbg("FCERR %i, ", (tmp >> 3) & 1); + msg_pdbg("FDONE %i, ", (tmp >> 2) & 1); + msg_pdbg("SCIP %i\n", (tmp >> 0) & 1); + if (tmp & (1 << 3)) { + msg_pdbg("Clearing SSFS.FCERR\n"); + mmio_writeb(1 << 3, ich_spibar + 0x90); + } + tmp >>= 8; + msg_pdbg("0x91: 0x%06x (SSFC)\n", tmp); + msg_pdbg("0x94: 0x%04x (PREOP)\n", mmio_readw(ich_spibar + 0x94)); msg_pdbg("0x96: 0x%04x (OPTYPE)\n",
* Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [101125 00:45]:
New version.
Proper error handling for ICH/VIA SPI: Use 16-bit values for bit masks in 16-bit registers. Check for SPI Cycle In Progress and wait up to 60 ms. Do not touch reserved bits. Reduce SPI cycle timeout from 60 s to 60 ms. Clear transaction errors caused by our own SPI accesses. Add better debugging in case the hardware misbehaves.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Stefan Reinauer stefan.reinauer@coreboot.org
Index: flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c
--- flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c (Revision 1236) +++ flashrom-ich_spi_fcerr_fcdone_paranoia/ichspi.c (Arbeitskopie) @@ -51,6 +51,7 @@ #define SSFS_CDS 0x00000004 #define SSFS_FCERR 0x00000008 #define SSFS_AEL 0x00000010 +#define SSFS_RESERVED_MASK 0x000000e2
#define ICH9_REG_SSFC 0x91 /* 24 Bits */ #define SSFC_SCGO 0x00000200 @@ -63,6 +64,7 @@ #define SSFC_SCF 0x01000000 #define SSFC_SCF_20MHZ 0x00000000 #define SSFC_SCF_33MHZ 0x01000000 +#define SSFC_RESERVED_MASK 0xf8008100
#define ICH9_REG_PREOP 0x94 /* 16 Bits */ #define ICH9_REG_OPTYPE 0x96 /* 16 Bits */ @@ -76,9 +78,11 @@
// ICH7 registers #define ICH7_REG_SPIS 0x00 /* 16 Bits */ -#define SPIS_SCIP 0x00000001 -#define SPIS_CDS 0x00000004 -#define SPIS_FCERR 0x00000008 +#define SPIS_SCIP 0x0001 +#define SPIS_GRANT 0x0002 +#define SPIS_CDS 0x0004 +#define SPIS_FCERR 0x0008 +#define SPIS_RESERVED_MASK 0x7ff0
/* VIA SPI is compatible with ICH7, but maxdata to transfer is 16 bytes. @@ -146,6 +150,11 @@ return mmio_readw(ich_spibar + X); }
+static uint16_t REGREAD8(int X) +{
- return mmio_readb(ich_spibar + X);
+}
#define REGWRITE32(X,Y) mmio_writel(Y, ich_spibar+X) #define REGWRITE16(X,Y) mmio_writew(Y, ich_spibar+X) #define REGWRITE8(X,Y) mmio_writeb(Y, ich_spibar+X) @@ -497,6 +506,15 @@ write_cmd = 1; }
- timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */
- while ((REGREAD16(ICH7_REG_SPIS) & SPIS_SCIP) && --timeout) {
programmer_delay(10);- }
- if (!timeout) {
msg_perr("Error: SCIP never cleared!\n");return 1;- }
- /* Programm Offset in Flash into FADDR */ REGWRITE32(ICH7_REG_SPIA, (offset & 0x00FFFFFF)); /* SPI addresses are 24 BIT only */
@@ -523,7 +541,9 @@ }
/* Assemble SPIS */
- temp16 = 0;
- temp16 = REGREAD16(ICH7_REG_SPIS);
- /* keep reserved bits */
- temp16 &= SPIS_RESERVED_MASK; /* clear error status registers */ temp16 |= (SPIS_CDS + SPIS_FCERR); REGWRITE16(ICH7_REG_SPIS, temp16);
@@ -570,18 +590,26 @@ /* write it */ REGWRITE16(ICH7_REG_SPIC, temp16);
- /* wait for cycle complete */
- timeout = 100 * 1000 * 60; // 60s is a looong timeout.
- while (((REGREAD16(ICH7_REG_SPIS) & SPIS_CDS) == 0) && --timeout) {
- /* Wait for Cycle Done Status or Flash Cycle Error. */
- timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */
- while (((REGREAD16(ICH7_REG_SPIS) & (SPIS_CDS | SPIS_FCERR)) == 0) &&
--timeout) {
msg_perr("timeout\n");
msg_perr("timeout, ICH7_REG_SPIS=0x%04x\n",REGREAD16(ICH7_REG_SPIS));return 1;}
/* FIXME: make sure we do not needlessly cause transaction errors. */
- if ((REGREAD16(ICH7_REG_SPIS) & SPIS_FCERR) != 0) {
msg_pdbg("Transaction error!\n");
- temp16 = REGREAD16(ICH7_REG_SPIS);
- if (temp16 & SPIS_FCERR) {
msg_perr("Transaction error for opcode 0x%02x!\n",op.opcode);/* keep reserved bits */temp16 &= SPIS_RESERVED_MASK;REGWRITE16(ICH7_REG_SPIS, temp16 | SPIS_FCERR);@@ -616,6 +644,15 @@ write_cmd = 1; }
- timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */
- while ((REGREAD8(ICH9_REG_SSFS) & SSFS_SCIP) && --timeout) {
programmer_delay(10);- }
- if (!timeout) {
msg_perr("Error: SCIP never cleared!\n");return 1;- }
- /* Programm Offset in Flash into FADDR */ REGWRITE32(ICH9_REG_FADDR, (offset & 0x00FFFFFF)); /* SPI addresses are 24 BIT only */
@@ -641,12 +678,13 @@ }
/* Assemble SSFS + SSFC */
- /* keep reserved bits (23-19,7,0) */ temp32 = REGREAD32(ICH9_REG_SSFS);
- temp32 &= 0xF8008100;
- /* keep reserved bits */
- temp32 &= SSFS_RESERVED_MASK | SSFC_RESERVED_MASK; /* clear error status registers */ temp32 |= (SSFS_CDS + SSFS_FCERR);
- REGWRITE32(ICH9_REG_SSFS, temp32);
- /* Use 20 MHz */ temp32 |= SSFC_SCF_20MHZ;
@@ -691,18 +729,27 @@ /* write it */ REGWRITE32(ICH9_REG_SSFS, temp32);
- /*wait for cycle complete */
- timeout = 100 * 1000 * 60; // 60s is a looong timeout.
- while (((REGREAD32(ICH9_REG_SSFS) & SSFS_CDS) == 0) && --timeout) {
- /* Wait for Cycle Done Status or Flash Cycle Error. */
- timeout = 100 * 60; /* 60 ms are 9.6 million cycles at 16 MHz. */
- while (((REGREAD32(ICH9_REG_SSFS) & (SSFS_CDS | SSFS_FCERR)) == 0) &&
--timeout) {
msg_perr("timeout\n");
msg_perr("timeout, ICH9_REG_SSFS=0x%08x\n",REGREAD32(ICH9_REG_SSFS));return 1;}
/* FIXME make sure we do not needlessly cause transaction errors. */
- if ((REGREAD32(ICH9_REG_SSFS) & SSFS_FCERR) != 0) {
msg_pdbg("Transaction error!\n");
- temp32 = REGREAD32(ICH9_REG_SSFS);
- if (temp32 & SSFS_FCERR) {
msg_perr("Transaction error for opcode 0x%02x!\n",op.opcode);/* keep reserved bits */temp32 &= SSFS_RESERVED_MASK | SSFC_RESERVED_MASK;/* Clear the transaction error. */REGWRITE32(ICH9_REG_SSFS, temp32 | SSFS_FCERR);@@ -1042,7 +1089,6 @@ msg_pdbg("0x%02x: 0x%08x (PBR%d)\n", offs, mmio_readl(ich_spibar + offs), i); }
msg_pdbg("\n");@@ -1082,8 +1128,20 @@ mmio_readl(ich_spibar + 0x80)); msg_pdbg("0x84: 0x%08x (PR4)\n", mmio_readl(ich_spibar + 0x84));
msg_pdbg("0x90: 0x%08x (SSFS, SSFC)\n",mmio_readl(ich_spibar + 0x90));
tmp = mmio_readl(ich_spibar + 0x90);msg_pdbg("0x90: 0x%02x (SSFS)\n", tmp & 0xff);msg_pdbg("AEL %i, ", (tmp >> 4) & 1);msg_pdbg("FCERR %i, ", (tmp >> 3) & 1);msg_pdbg("FDONE %i, ", (tmp >> 2) & 1);msg_pdbg("SCIP %i\n", (tmp >> 0) & 1);if (tmp & (1 << 3)) {msg_pdbg("Clearing SSFS.FCERR\n");mmio_writeb(1 << 3, ich_spibar + 0x90);}tmp >>= 8;msg_pdbg("0x91: 0x%06x (SSFC)\n", tmp);- msg_pdbg("0x94: 0x%04x (PREOP)\n", mmio_readw(ich_spibar + 0x94)); msg_pdbg("0x96: 0x%04x (OPTYPE)\n",
flashrom mailing list flashrom@flashrom.org http://www.flashrom.org/mailman/listinfo/flashrom
Auf 16.03.2011 23:31, Stefan Reinauer schrieb:
Acked-by: Stefan Reinauerstefan.reinauer@coreboot.org
Thanks, committed in r1281.
Regards, Carl-Daniel
Am Mittwoch, den 16.03.2011, 23:31 +0100 schrieb Stefan Reinauer:
Reduce SPI cycle timeout from 60 s to 60 ms.
Turned out to be a bad idea in the current code base. We use the preop mechanism if it seems to fit, which means we use the ACS (atomic cycle sequence) bit in the control register for that.
According to the ICH10 datasheet, the following operations are done atomically: - sending the preop - sending the main opcode - polling the status register until WIP (bit 0) is low (!!!)
During all these operations, the SCIP (SPI cycle in progress) bit is set.
So if enable write/sector erase is sent as "atomic sequence", the cycle is only over after the erase completed, which can easily exceed 60ms. See for example this report where exactly that happened:
http://paste.flashrom.org/view.php?id=513
We need to revert the timeout to 60 seconds at least if we use the atomic sequence feature of the chipset. Or we need to stop using atomic sequences.
Regards, Michael Karcher
El 23/04/11 14:37, Michael Karcher escribió:
http://paste.flashrom.org/view.php?id=513
We need to revert the timeout to 60 seconds at least if we use the atomic sequence feature of the chipset. Or we need to stop using atomic sequences.
Worked just fine raising all timeouts in ichspi.c from (100 * 60) to (100 * 1000 * 60) , flashes,verifies and system reboots with new BIOS correctly after that.
HTH.
Hi Michael,
Am 23.04.2011 19:37 schrieb Michael Karcher:
Am Mittwoch, den 16.03.2011, 23:31 +0100 schrieb Stefan Reinauer:
Reduce SPI cycle timeout from 60 s to 60 ms.
Turned out to be a bad idea in the current code base. We use the preop mechanism if it seems to fit, which means we use the ACS (atomic cycle sequence) bit in the control register for that.
According to the ICH10 datasheet, the following operations are done atomically:
- sending the preop
- sending the main opcode
- polling the status register until WIP (bit 0) is low (!!!)
During all these operations, the SCIP (SPI cycle in progress) bit is set.
So if enable write/sector erase is sent as "atomic sequence", the cycle is only over after the erase completed, which can easily exceed 60ms. See for example this report where exactly that happened:
Thank you for the excellent analysis.
We need to revert the timeout to 60 seconds at least if we use the atomic sequence feature of the chipset. Or we need to stop using atomic sequences.
Atomic sequences are needed for locked down hardware (no opmenu reprograming), so we either use two different code paths for SPI command execution on locked down and unlocked hardware, or we indeed increase the timeouts again. What do you think about an exponential timeout increase starting at 1 us, doubling the per-loop timeout each loop with a cap at 1 s per loop? And should we introduce programmer_soft_delay (i.e. sleep instead of busywait) while we're at it to allow for CPU load reduction?
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Cristian Rodríguez -
Michael Karcher -
Rudolf Marek -
Stefan Reinauer