This is a workaround for a bug in SB600 and SB700. If we only send an opcode and no additional data/address, the SPI controller will read one byte too few from the chip. Basically, the last byte of the chip response is discarded and will not end up in the FIFO. It is unclear if the CS# line is set high too early as well. That hardware bug is undocumented as of now, but I'm working with AMD to add a detailed description of it to the errata.
Add loads of additional debugging to SB600/SB700 init.
Add explanatory comments for unintuitive code flow.
Thanks go to Uwe for testing quite a few iterations of the patch.
As a side benefit, we can now kill the SB600 flash chip status register special case, which was a somewhat misguided workaround for that hardware erratum.
Note for future added features in the SB600 SPI driver: It may be possible to read up to 15 bytes of command response with overlapping reads due to the ring buffer design of the FIFO if the command can be repeated without ill effects. Same for skipping up to 7 bytes between command and response.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-spi_sb600_offbyone_workaround/chipset_enable.c =================================================================== --- flashrom-spi_sb600_offbyone_workaround/chipset_enable.c (Revision 653) +++ flashrom-spi_sb600_offbyone_workaround/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_offbyone_workaround/sb600spi.c =================================================================== --- flashrom-spi_sb600_offbyone_workaround/sb600spi.c (Revision 653) +++ flashrom-spi_sb600_offbyone_workaround/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) { @@ -106,6 +106,7 @@ int count; /* First byte is cmd which can not being sent through FIFO. */ unsigned char cmd = *writearr++; + unsigned int readoffby1;
writecnt--;
@@ -124,8 +125,15 @@ return SPI_INVALID_LENGTH; }
+ /* This is a workaround for a bug in SB600 and SB700. If we only send + * an opcode and no additional data/address, the SPI controller will + * read one byte too few from the chip. Basically, the last byte of + * the chip response is discarded and will not end up in the FIFO. + * It is unclear if the CS# line is set high too early as well. + */ + readoffby1 = (writecnt) ? 0 : 1; + mmio_writeb((readcnt + readoffby1) << 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(); @@ -147,17 +155,24 @@
/* * After the command executed, we should find out the index of the - * received byte. Here we just reset the FIFO pointer, skip the - * writecnt, is there anyone who have anther method to replace it? + * received byte. Here we just reset the FIFO pointer and skip the + * writecnt. + * It would be possible to increase the FIFO pointer by one instead + * of reading and discarding one byte from the FIFO. + * The FIFO is implemented on top of an 8 byte ring buffer and the + * buffer is never cleared. For every byte that is shifted out after + * the opcode, the FIFO already stores the response from the chip. + * Usually, the chip will respond with 0x00 or 0xff. */ reset_internal_fifo_pointer();
+ /* Skip the bytes we sent. */ for (count = 0; count < writecnt; count++) { - cmd = mmio_readb(sb600_spibar + 0xC); /* Skip the byte we send. */ + cmd = mmio_readb(sb600_spibar + 0xC); 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);
This is a workaround for a bug in SB600 and SB700. If we only send an opcode and no additional data/address, the SPI controller will read one byte too few from the chip. Basically, the last byte of the chip response is discarded and will not end up in the FIFO. It is unclear if the CS# line is set high too early as well. That hardware bug is undocumented as of now, but I'm working with AMD to add a detailed description of it to the errata.
Add loads of additional debugging to SB600/SB700 init.
Add explanatory comments for unintuitive code flow.
Thanks go to Uwe for testing quite a few iterations of the patch.
Kill the SB600 flash chip status register special case, which was a somewhat misguided workaround for that hardware erratum.
Note for future added features in the SB600 SPI driver: It may be possible to read up to 15 bytes of command response with overlapping reads due to the ring buffer design of the FIFO if the command can be repeated without ill effects. Same for skipping up to 7 bytes between command and response.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-spi_sb600_offbyone_workaround/flash.h =================================================================== --- flashrom-spi_sb600_offbyone_workaround/flash.h (Revision 658) +++ flashrom-spi_sb600_offbyone_workaround/flash.h (Arbeitskopie) @@ -510,7 +510,6 @@ const unsigned char *writearr, unsigned char *readarr); int sb600_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len); int sb600_spi_write_1(struct flashchip *flash, uint8_t *buf); -uint8_t sb600_read_status_register(void); extern uint8_t *sb600_spibar;
/* jedec.c */ Index: flashrom-spi_sb600_offbyone_workaround/spi.c =================================================================== --- flashrom-spi_sb600_offbyone_workaround/spi.c (Revision 658) +++ flashrom-spi_sb600_offbyone_workaround/spi.c (Arbeitskopie) @@ -367,10 +367,6 @@ int ret;
/* Read Status Register */ - if (spi_controller == SPI_CONTROLLER_SB600) { /* FIXME */ - /* Workaround for SB600 hardware bug. Can be killed later. */ - return sb600_read_status_register(); - } ret = spi_send_command(sizeof(cmd), sizeof(readarr), cmd, readarr); if (ret) printf_debug("RDSR failed!\n"); Index: flashrom-spi_sb600_offbyone_workaround/chipset_enable.c =================================================================== --- flashrom-spi_sb600_offbyone_workaround/chipset_enable.c (Revision 658) +++ flashrom-spi_sb600_offbyone_workaround/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_offbyone_workaround/sb600spi.c =================================================================== --- flashrom-spi_sb600_offbyone_workaround/sb600spi.c (Revision 658) +++ flashrom-spi_sb600_offbyone_workaround/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) { @@ -46,16 +46,6 @@ return spi_read_chunked(flash, buf, start, len, 8); }
-uint8_t sb600_read_status_register(void) -{ - const unsigned char cmd[0x02] = { JEDEC_RDSR, 0x00 }; - unsigned char readarr[JEDEC_RDSR_INSIZE]; - - /* Read Status Register */ - spi_send_command(sizeof(cmd), sizeof(readarr), cmd, readarr); - return readarr[0]; -} - int sb600_spi_write_1(struct flashchip *flash, uint8_t *buf) { int rc = 0, i; @@ -106,6 +96,7 @@ int count; /* First byte is cmd which can not being sent through FIFO. */ unsigned char cmd = *writearr++; + unsigned int readoffby1;
writecnt--;
@@ -124,8 +115,15 @@ return SPI_INVALID_LENGTH; }
+ /* This is a workaround for a bug in SB600 and SB700. If we only send + * an opcode and no additional data/address, the SPI controller will + * read one byte too few from the chip. Basically, the last byte of + * the chip response is discarded and will not end up in the FIFO. + * It is unclear if the CS# line is set high too early as well. + */ + readoffby1 = (writecnt) ? 0 : 1; + mmio_writeb((readcnt + readoffby1) << 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(); @@ -147,17 +145,25 @@
/* * After the command executed, we should find out the index of the - * received byte. Here we just reset the FIFO pointer, skip the - * writecnt, is there anyone who have anther method to replace it? + * received byte. Here we just reset the FIFO pointer and skip the + * writecnt. + * It would be possible to increase the FIFO pointer by one instead + * of reading and discarding one byte from the FIFO. + * The FIFO is implemented on top of an 8 byte ring buffer and the + * buffer is never cleared. For every byte that is shifted out after + * the opcode, the FIFO already stores the response from the chip. + * Usually, the chip will respond with 0x00 or 0xff. */ reset_internal_fifo_pointer();
+ /* Skip the bytes we sent. */ for (count = 0; count < writecnt; count++) { - cmd = mmio_readb(sb600_spibar + 0xC); /* Skip the byte we send. */ + cmd = mmio_readb(sb600_spibar + 0xC); 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);
Carl-Daniel Hailfinger wrote:
This is a workaround for a bug in SB600 and SB700. If we only send an opcode and no additional data/address, the SPI controller will read one byte too few from the chip. Basically, the last byte of the chip response is discarded and will not end up in the FIFO. It is unclear if the CS# line is set high too early as well. That hardware bug is undocumented as of now, but I'm working with AMD to add a detailed description of it to the errata.
Add loads of additional debugging to SB600/SB700 init.
Add explanatory comments for unintuitive code flow.
Thanks go to Uwe for testing quite a few iterations of the patch.
Kill the SB600 flash chip status register special case, which was a somewhat misguided workaround for that hardware erratum.
Note for future added features in the SB600 SPI driver: It may be possible to read up to 15 bytes of command response with overlapping reads due to the ring buffer design of the FIFO if the command can be repeated without ill effects. Same for skipping up to 7 bytes between command and response.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Interesting.. good work!
Acked-by: Stefan Reinauer stepan@coresystems.de
On 23.07.2009 03:30, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
This is a workaround for a bug in SB600 and SB700. If we only send an opcode and no additional data/address, the SPI controller will read one byte too few from the chip. Basically, the last byte of the chip response is discarded and will not end up in the FIFO. It is unclear if the CS# line is set high too early as well. That hardware bug is undocumented as of now, but I'm working with AMD to add a detailed description of it to the errata.
Add loads of additional debugging to SB600/SB700 init.
Add explanatory comments for unintuitive code flow.
Thanks go to Uwe for testing quite a few iterations of the patch.
Kill the SB600 flash chip status register special case, which was a somewhat misguided workaround for that hardware erratum.
Note for future added features in the SB600 SPI driver: It may be possible to read up to 15 bytes of command response with overlapping reads due to the ring buffer design of the FIFO if the command can be repeated without ill effects. Same for skipping up to 7 bytes between command and response.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Interesting.. good work!
Thanks! Took me a few hours to figure out the trick. By the way, this also fixes all SPI ID strangeness we saw on SB600 in the past.
Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks, r661.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Stefan Reinauer