More debugging for SB600/SB700 SPI. Some of this should be conditional on SB700. Anyway, it should be harmless on SB600.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-spi_sb600_sb700_debug/chipset_enable.c =================================================================== --- flashrom-spi_sb600_sb700_debug/chipset_enable.c (Revision 645) +++ flashrom-spi_sb600_sb700_debug/chipset_enable.c (Arbeitskopie) @@ -692,7 +692,9 @@
/* Read SPI_BaseAddr */ tmp = pci_read_long(dev, 0xa0); - tmp &= 0xfffffff0; /* remove low 4 bits (reserved) */ + printf_debug("AltSpiCSEnable=%i, SpiRomEnable=%i, AbortEnable=%i\n", + tmp & 0x1, (tmp & 0x2) >> 1, (tmp & 0x4) >> 2); + tmp &= 0xffffffe0; /* remove bits 4-0 (reserved) */ printf_debug("SPI base address is at 0x%x\n", tmp);
/* If the BAR has address 0, it is unlikely SPI is used. */ @@ -704,6 +706,13 @@ /* The low bits of the SPI base address are used as offset into the mapped page */ sb600_spibar += tmp & 0xfff;
+ tmp = (pci_read_byte(dev, 0xba) & 0x4) >> 2; + printf_debug("PrefetchEnSPIFromIMC=%i, ", tmp); + + tmp = pci_read_byte(dev, 0xbb); + printf_debug("PrefetchEnSPIFromHost=%i, SpiOpEnInLpcMode=%i\n", + tmp & 0x1, (tmp & 0x20) >> 5); + /* Look for the SMBus device. */ smbus_dev = pci_dev_find(0x1002, 0x4385);
Index: flashrom-spi_sb600_sb700_debug/sb600spi.c =================================================================== --- flashrom-spi_sb600_sb700_debug/sb600spi.c (Revision 645) +++ flashrom-spi_sb600_sb700_debug/sb600spi.c (Arbeitskopie) @@ -117,13 +117,13 @@
if (readcnt > 8) { printf("%s, SB600 SPI controller can not receive %d bytes, " - "which is limited with 8 bytes\n", __func__, readcnt); + "it is limited to 8 bytes\n", __func__, readcnt); return 1; }
if (writecnt > 8) { - printf("%s, SB600 SPI controller can not sent %d bytes, " - "which is limited with 8 bytes\n", __func__, writecnt); + printf("%s, SB600 SPI controller can not send %d bytes, " + "it is limited to 8 bytes\n", __func__, writecnt); return 1; }
@@ -136,6 +136,7 @@ /* Send the write byte to FIFO. */ for (count = 0; count < writecnt; count++, writearr++) { printf_debug(" [%x]", *writearr); + /* FIXME: Can we rely on auto-posting these writes or do we have to issue a read afterwards? */ mmio_writeb(*writearr, sb600_spibar + 0xC); } printf_debug("\n"); @@ -153,19 +154,28 @@ * received byte. Here we just reset the FIFO pointer, skip the * writecnt, is there anyone who have anther method to replace it? */ + printf_debug("The FIFO pointer before reset is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); reset_internal_fifo_pointer();
for (count = 0; count < writecnt; count++) { - cmd = mmio_readb(sb600_spibar + 0xC); /* Skip the byte we send. */ + cmd = mmio_readb(sb600_spibar + 0xC); /* Skip the bytes we send. */ printf_debug("[ %2x]", cmd); }
- printf_debug("The FIFO pointer 6 is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); + printf_debug("The FIFO pointer after skipping is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); for (count = 0; count < readcnt; count++, readarr++) { *readarr = mmio_readb(sb600_spibar + 0xC); printf_debug("[%02x]", *readarr); } printf_debug("\n");
+ printf_debug("The FIFO pointer after reading is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); + printf_debug("Dumping everything that's left in the FIFO\n"); + for (count = 0; count < 8; count++) { + cmd = mmio_readb(sb600_spibar + 0xC); + printf_debug("[ %2x]", cmd); + } + printf_debug("The FIFO pointer after dumping is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); + return 0; }
On 11.07.2009 15:29, Carl-Daniel Hailfinger wrote:
More debugging for SB600/SB700 SPI. Some of this should be conditional on SB700. Anyway, it should be harmless on SB600.
Even more debugging and some scrubbing. I want to find out exactly how that bug manifests itself. It's entirely possible that this patch does corrupt input/output or yield undefined behaviour, so please don't try writing or erasing. ID and most probably read should be safe.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-spi_sb600_sb700_debug/chipset_enable.c =================================================================== --- flashrom-spi_sb600_sb700_debug/chipset_enable.c (Revision 653) +++ flashrom-spi_sb600_sb700_debug/chipset_enable.c (Arbeitskopie) @@ -692,7 +692,9 @@
/* Read SPI_BaseAddr */ tmp = pci_read_long(dev, 0xa0); - tmp &= 0xfffffff0; /* remove low 4 bits (reserved) */ + printf_debug("AltSpiCSEnable=%i, SpiRomEnable=%i, AbortEnable=%i\n", + tmp & 0x1, (tmp & 0x2) >> 1, (tmp & 0x4) >> 2); + tmp &= 0xffffffe0; /* remove bits 4-0 (reserved) */ printf_debug("SPI base address is at 0x%x\n", tmp);
/* If the BAR has address 0, it is unlikely SPI is used. */ @@ -704,6 +706,13 @@ /* The low bits of the SPI base address are used as offset into the mapped page */ sb600_spibar += tmp & 0xfff;
+ tmp = (pci_read_byte(dev, 0xba) & 0x4) >> 2; + printf_debug("PrefetchEnSPIFromIMC=%i, ", tmp); + + tmp = pci_read_byte(dev, 0xbb); + printf_debug("PrefetchEnSPIFromHost=%i, SpiOpEnInLpcMode=%i\n", + tmp & 0x1, (tmp & 0x20) >> 5); + /* Look for the SMBus device. */ smbus_dev = pci_dev_find(0x1002, 0x4385);
Index: flashrom-spi_sb600_sb700_debug/sb600spi.c =================================================================== --- flashrom-spi_sb600_sb700_debug/sb600spi.c (Revision 653) +++ flashrom-spi_sb600_sb700_debug/sb600spi.c (Arbeitskopie) @@ -104,6 +104,7 @@ const unsigned char *writearr, unsigned char *readarr) { int count; + int tmp; /* First byte is cmd which can not being sent through FIFO. */ unsigned char cmd = *writearr++;
@@ -124,15 +125,24 @@ return SPI_INVALID_LENGTH; }
+ tmp = mmio_readb(sb600_spibar + 1); + printf_debug("%s, old readcnt=%i, old writecnt=%i\n", + __func__, tmp >> 4, tmp & 0xf); + mmio_writeb(readcnt << 4 | (writecnt), sb600_spibar + 1); mmio_writeb(cmd, sb600_spibar + 0); - mmio_writeb(readcnt << 4 | (writecnt), sb600_spibar + 1);
/* Before we use the FIFO, reset it first. */ reset_internal_fifo_pointer();
+ /* Scrub the FIFO by writing a stream of 0xab. */ + for (count = 0; count < 8; count++) + mmio_writeb(0xab, sb600_spibar + 0xC); + reset_internal_fifo_pointer(); + /* Send the write byte to FIFO. */ for (count = 0; count < writecnt; count++, writearr++) { printf_debug(" [%x]", *writearr); + /* FIXME: Can we rely on auto-posting these writes or do we have to issue a read afterwards? */ mmio_writeb(*writearr, sb600_spibar + 0xC); } printf_debug("\n"); @@ -150,19 +160,28 @@ * received byte. Here we just reset the FIFO pointer, skip the * writecnt, is there anyone who have anther method to replace it? */ + printf_debug("The FIFO pointer before reset is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); reset_internal_fifo_pointer();
for (count = 0; count < writecnt; count++) { - cmd = mmio_readb(sb600_spibar + 0xC); /* Skip the byte we send. */ + cmd = mmio_readb(sb600_spibar + 0xC); /* Skip the bytes we send. */ printf_debug("[ %2x]", cmd); }
- printf_debug("The FIFO pointer 6 is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); + printf_debug("The FIFO pointer after skipping is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); for (count = 0; count < readcnt; count++, readarr++) { *readarr = mmio_readb(sb600_spibar + 0xC); printf_debug("[%02x]", *readarr); } printf_debug("\n");
+ printf_debug("The FIFO pointer after reading is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); + printf_debug("Dumping everything that's left in the FIFO\n"); + for (count = 0; count < 8; count++) { + cmd = mmio_readb(sb600_spibar + 0xC); + printf_debug("[ %2x]", cmd); + } + printf_debug("The FIFO pointer after dumping is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); + return 0; }
On 14.07.2009 12:44, Carl-Daniel Hailfinger wrote:
On 11.07.2009 15:29, Carl-Daniel Hailfinger wrote:
More debugging for SB600/SB700 SPI. Some of this should be conditional on SB700. Anyway, it should be harmless on SB600.
Even more debugging and some scrubbing. I want to find out exactly how that bug manifests itself. It's entirely possible that this patch does corrupt input/output or yield undefined behaviour, so please don't try writing or erasing. ID and most probably read should be safe.
Thanks to Uwe's testing, I deduced the following behaviour: SB600 stores one read byte too few in the FIFO if we just write an opcode and nothing else. The result is corruption for some operations. It also explains why SB600 has a special function for reading the SPI flash status register. That special function is a hack to work around this bug.
This patch should fix the bug for all commands we're using, but I want to fix this for all cases, not just a subset. Uwe, can you please reply to this patch with two runs of "flashrom -V", once with the patch and once with the patch and //#define SB600_READCNT_MAX uncommented.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-spi_sb600_sb700_debug/chipset_enable.c =================================================================== --- flashrom-spi_sb600_sb700_debug/chipset_enable.c (Revision 653) +++ flashrom-spi_sb600_sb700_debug/chipset_enable.c (Arbeitskopie) @@ -692,25 +692,49 @@
/* Read SPI_BaseAddr */ tmp = pci_read_long(dev, 0xa0); - tmp &= 0xfffffff0; /* remove low 4 bits (reserved) */ + tmp &= 0xffffffe0; /* remove bits 4-0 (reserved) */ printf_debug("SPI base address is at 0x%x\n", tmp);
/* If the BAR has address 0, it is unlikely SPI is used. */ if (!tmp) has_spi = 0;
- /* Physical memory can only be mapped at page (4k) boundaries */ - sb600_spibar = physmap("SB600 SPI registers", tmp & 0xfffff000, 0x1000); - /* The low bits of the SPI base address are used as offset into the mapped page */ - sb600_spibar += tmp & 0xfff; + if (has_spi) { + /* Physical memory has to be mapped at page (4k) boundaries. */ + sb600_spibar = physmap("SB600 SPI registers", tmp & 0xfffff000, + 0x1000); + /* The low bits of the SPI base address are used as offset into + * the mapped page. + */ + sb600_spibar += tmp & 0xfff;
+ tmp = pci_read_long(dev, 0xa0); + printf_debug("AltSpiCSEnable=%i, SpiRomEnable=%i, " + "AbortEnable=%i\n", tmp & 0x1, (tmp & 0x2) >> 1, + (tmp & 0x4) >> 2); + tmp = (pci_read_byte(dev, 0xba) & 0x4) >> 2; + printf_debug("PrefetchEnSPIFromIMC=%i, ", tmp); + + tmp = pci_read_byte(dev, 0xbb); + printf_debug("PrefetchEnSPIFromHost=%i, SpiOpEnInLpcMode=%i\n", + tmp & 0x1, (tmp & 0x20) >> 5); + tmp = mmio_readl(sb600_spibar); + printf_debug("SpiArbEnable=%i, SpiAccessMacRomEn=%i, " + "SpiHostAccessRomEn=%i, ArbWaitCount=%i, " + "SpiBridgeDisable=%i, DropOneClkOnRd=%i\n", + (tmp >> 19) & 0x1, (tmp >> 22) & 0x1, + (tmp >> 23) & 0x1, (tmp >> 24) & 0x7, + (tmp >> 27) & 0x1, (tmp >> 28) & 0x1); + } + /* Look for the SMBus device. */ smbus_dev = pci_dev_find(0x1002, 0x4385);
- if (!smbus_dev) { + if (has_spi && !smbus_dev) { fprintf(stderr, "ERROR: SMBus device not found. Not enabling SPI.\n"); has_spi = 0; - } else { + } + if (has_spi) { /* Note about the bit tests below: If a bit is zero, the GPIO is SPI. */ /* GPIO11/SPI_DO and GPIO12/SPI_DI status */ reg = pci_read_byte(smbus_dev, 0xAB); Index: flashrom-spi_sb600_sb700_debug/sb600spi.c =================================================================== --- flashrom-spi_sb600_sb700_debug/sb600spi.c (Revision 653) +++ flashrom-spi_sb600_sb700_debug/sb600spi.c (Arbeitskopie) @@ -38,7 +38,7 @@ *}; */
-uint8_t *sb600_spibar; +uint8_t *sb600_spibar = NULL;
int sb600_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len) { @@ -104,8 +104,10 @@ const unsigned char *writearr, unsigned char *readarr) { int count; + int tmp; /* First byte is cmd which can not being sent through FIFO. */ unsigned char cmd = *writearr++; + unsigned int readcnt_helper;
writecnt--;
@@ -124,15 +126,37 @@ return SPI_INVALID_LENGTH; }
+ tmp = mmio_readb(sb600_spibar + 1); + printf_debug("%s, old readcnt=%i, old writecnt=%i\n", + __func__, tmp >> 4, tmp & 0xf); + readcnt_helper = readcnt; + if (!writecnt) { + printf_debug("%s, writecnt=0 ", __func__); +//#define SB600_READCNT_MAX +#ifdef SB600_READCNT_MAX + printf_debug("workaround: increasing readcnt by 2, "); + readcnt_helper += 2; + printf_debug("readcnt is now %i", readcnt_helper); +#else + printf_debug("workaround: increasing readcnt to 8\n"); + readcnt_helper = 8; +#endif + } + mmio_writeb(readcnt_helper << 4 | (writecnt), sb600_spibar + 1); mmio_writeb(cmd, sb600_spibar + 0); - mmio_writeb(readcnt << 4 | (writecnt), sb600_spibar + 1);
/* Before we use the FIFO, reset it first. */ reset_internal_fifo_pointer();
+ /* Scrub the FIFO by writing a stream of 0xab. */ + for (count = 0; count < 8; count++) + mmio_writeb(0xab, sb600_spibar + 0xC); + reset_internal_fifo_pointer(); + /* Send the write byte to FIFO. */ for (count = 0; count < writecnt; count++, writearr++) { printf_debug(" [%x]", *writearr); + /* FIXME: Can we rely on auto-posting these writes or do we have to issue a read afterwards? */ mmio_writeb(*writearr, sb600_spibar + 0xC); } printf_debug("\n"); @@ -150,19 +174,28 @@ * received byte. Here we just reset the FIFO pointer, skip the * writecnt, is there anyone who have anther method to replace it? */ + printf_debug("The FIFO pointer before reset is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); reset_internal_fifo_pointer();
for (count = 0; count < writecnt; count++) { - cmd = mmio_readb(sb600_spibar + 0xC); /* Skip the byte we send. */ + cmd = mmio_readb(sb600_spibar + 0xC); /* Skip the bytes we send. */ printf_debug("[ %2x]", cmd); }
- printf_debug("The FIFO pointer 6 is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); + printf_debug("The FIFO pointer after skipping is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); for (count = 0; count < readcnt; count++, readarr++) { *readarr = mmio_readb(sb600_spibar + 0xC); printf_debug("[%02x]", *readarr); } printf_debug("\n");
+ printf_debug("The FIFO pointer after reading is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); + printf_debug("Dumping everything that's left in the FIFO\n"); + for (count = 0; count < 8; count++) { + cmd = mmio_readb(sb600_spibar + 0xC); + printf_debug("[ %2x]", cmd); + } + printf_debug("The FIFO pointer after dumping is %d.\n", mmio_readb(sb600_spibar + 0xd) & 0x07); + return 0; }
On Thu, Jul 16, 2009 at 05:54:43PM +0200, Carl-Daniel Hailfinger wrote:
Uwe, can you please reply to this patch with two runs of "flashrom -V", once with the patch and once with the patch and //#define SB600_READCNT_MAX uncommented.
Attached.
- 1.txt: trunk - 2.txt: trunk + patch - 3.txt: trunk + patch + uncommented #define SB600_READCNT_MAX
Uwe.
-
Carl-Daniel Hailfinger -
Uwe Hermann