flashrom: Winbond W83627DHG SPI, RDID 90, SST AAI & Intel D201GLY

[Peter Stuge]

Patch attached.

The wbsio driver has been tested by rphlx to work correctly for probe
and read on the Intel D201GLY board, but erase and write was not
tested since he had no easy recovery strategy in case of failure. If
anyone can test erase, write or both on a board with W83627DHG or
compatible (please add ids to wbsio_get_spibase()) I would be most
grateful. Comments, as always, appreciated.

//Peter

flashrom: Winbond W83627DHG SPI, RDID 90, SST AAI & Intel D201GLY

[Carl-Daniel Hailfinger]

On 07.07.2008 09:18, Peter Stuge wrote:
> Patch attached.
>
> The wbsio driver has been tested by rphlx to work correctly for probe
> and read on the Intel D201GLY board, but erase and write was not
> tested since he had no easy recovery strategy in case of failure. If
> anyone can test erase, write or both on a board with W83627DHG or
> compatible (please add ids to wbsio_get_spibase()) I would be most
> grateful. Comments, as always, appreciated.

Nice patch. Review follows:
> flashrom: Winbond SPI driver, RDID 90 probe, SST25VF040B.RDID90 chip, SST AAI write and Intel D201GLY board enable
>
> The Winbond superio SPI driver supports the maximum possible flash chip size
> of 8Mbit. Unfortunately Winbond left out the 4 top address bits in the
> programming interface, so there is no way to reach data below 4GB-1Mbyte.
> Chip erase still works.

I have not yet reviewed the Winbond driver itself, only the hooks for it
in generic code.
> Winbond superio SPI masters only support certain command types, so a new probe
> is needed at least for the SST25VF040B.

It should be possible to use Winbond command mode 3 (for partial match)
and command mode b (for cycling match) to emulate RDID.
If that doesn't work, we need a set of 3 IDs per SPI chip: RDID, REMS, RES.
> Add prettyprinting for the SST25VF040B status register.
>
> SST AAI is Auto Address Increment writing, a streamed write to the flash chip
> where the first write command sets a starting address, and following commands
> simply append data. Unfortunately not supported by Winbond SPI masters.

I believe AAI may be slower than per-page programming due to bigger
overhead.
Programming in AAI needs
1(command)+3(dummyaddr)+2(data)+(1(command)+2(data))*127(aaiwrites) =
387 cycles for 256 bytes
Programming in 16 byte chunks needs
(1(command)+3(address)+16(data))*16(chunks) = 320 cycles for 256 bytes
Programming in 64 byte chunks needs
(1(command)+3(address)+64(data))*4(chunks) = 272 cycles for 256 bytes
Programming in 256 byte chunks needs 1(command)+3(address)+256(data) =
260 cycles for 256 bytes
> Add an autodetected board enable for the mini-ITX Intel Desktop Board D201GLY.
> The PCI ids are 0:2.5 SiS IDE and 1:0.0 SiS VGA with Intel subsystem ids.
>
> Signed-off-by: Peter Stuge

Can you split this patch into multiple ones?
- Winbond SPI master support
- SST status prettyprinting support
- AAI support (which is chip-specific)
- D201GLY board enable
> Index: flash.h
>
> --- flash.h	(revision 3418)
> +++ flash.h	(working copy)
> @@ -378,7 +378,8 @@
>    BUS_TYPE_ICH7_SPI,
>    BUS_TYPE_ICH9_SPI,
>    BUS_TYPE_IT87XX_SPI,
> BUS_TYPE_VIA_SPI
> BUS_TYPE_VIA_SPI,
> BUS_TYPE_WBSIO_SPI
>
> } flashbus_t;
>
> extern flashbus_t flashbus;
> @@ -411,6 +412,7 @@
>  extern char *lb_part, *lb_vendor;
>
> /* spi.c */
> +int probe_spi_rdid_90(struct flashchip *flash);

That's actually the REMS (read electronci manufacturer signature)
command, not a RDID variant. Please call it probe_spi_rems.
> int probe_spi_rdid(struct flashchip *flash);
>  int probe_spi_rdid4(struct flashchip *flash);
>  int probe_spi_res(struct flashchip *flash);
> @@ -424,6 +426,7 @@
>  void spi_disable_blockprotect(void);
>  void spi_byte_program(int address, uint8_t byte);
>  void spi_nbyte_read(int address, uint8_t *bytes, int len);
> +int spi_aai_write(struct flashchip *flash, uint8_t *buf);
>
> /* 82802ab.c */
>  int probe_82802ab(struct flashchip *flash);
> @@ -435,6 +438,12 @@
>  int erase_29f040b(struct flashchip *flash);
>  int write_29f040b(struct flashchip *flash, uint8_t *buf);
>
> +/* board_enable.c */
> +void w836xx_ext_enter(uint16_t port);
> +void w836xx_ext_leave(uint16_t port);
> +unsigned char wbsio_read(uint16_t index, uint8_t reg);
> +void wbsio_write(uint16_t index, uint8_t reg, uint8_t data);
>
> /* ichspi.c */
>  int ich_spi_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr);
>  int ich_spi_read(struct flashchip *flash, uint8_t * buf);
> @@ -528,6 +537,12 @@
>  /* w49f002u.c */
>  int write_49f002(struct flashchip *flash, uint8_t *buf);
>
> +/* wbsio_spi.c */
> +int wbsio_check_for_spi(const char *name);
> +int wbsio_spi_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr);
> +int wbsio_spi_read(struct flashchip *flash, uint8_t *buf);
> +int wbsio_spi_write(struct flashchip *flash, uint8_t *buf);
>
> /* stm50flw0x0x.c */
>  int probe_stm50flw0x0x(struct flashchip *flash);
>  int erase_stm50flw0x0x(struct flashchip *flash);
> Index: flashchips.c
> ===================================================================
> --- flashchips.c	(revision 3418)
> +++ flashchips.c	(working copy)
> @@ -69,6 +69,7 @@
>    {"Spansion",	"S25FL016A",		SPANSION_ID,	SPANSION_S25FL016A,	2048,	256,		TEST_OK_PREW,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
>    {"SST",		"SST25VF016B",		SST_ID,		SST_25VF016B,		2048,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
>    {"SST",		"SST25VF040B",		SST_ID,		SST_25VF040B,		512,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
> {"SST",		"SST25VF040B.RDID90",	0xbf,		0x8d,			512,	64 * 1024,	TEST_OK_PR,	probe_spi_rdid_90,	spi_chip_erase_c7,		spi_aai_write,		spi_chip_read},

probe_spi_rems instead of probe_spi_rdid_90, but to be honest, having a
SPI controller which does not support RDID is a real PITA and needs a
structure redesign.
> {"SST",		"SST28SF040A",		SST_ID,		SST_28SF040,		512,	256,		TEST_UNTESTED,	probe_28sf040,		erase_28sf040,			write_28sf040},
>    {"SST",		"SST29EE010",		SST_ID,		SST_29EE010,		128,	128,		TEST_OK_PREW,	probe_jedec,		erase_chip_jedec,		write_jedec},
>    {"SST",		"SST29LE010",		SST_ID,		SST_29LE010,		128,	128,		TEST_UNTESTED,	probe_jedec,		erase_chip_jedec, 		write_jedec},
> Index: spi.c
> ===================================================================
> --- spi.c	(revision 3418)
> +++ spi.c	(working copy)
> @@ -41,6 +41,8 @@
>    case BUS_TYPE_ICH9_SPI:
>    case BUS_TYPE_VIA_SPI:
>                 return ich_spi_command(writecnt, readcnt, writearr, readarr);
> case BUS_TYPE_WBSIO_SPI:
> return wbsio_spi_command(writecnt, readcnt, writearr, readarr);
> default:
> printf_debug("%s called, but no SPI chipset/strapping detected\n", __FUNCTION__);
> }
>
> @@ -130,6 +132,25 @@
>    return 0;
>  }
>
> +int probe_spi_rdid_90(struct flashchip *flash) {

probe_spi_rems
> unsigned char r[2];

please call this readarr for consistency
> const unsigned char cmd[4] = {0x90, 0, 0, 0};

const unsigned char cmd[JEDEC_REMS_OUTSIZE] = {JEDEC_REMS, 0, 0, 0};
> if (spi_command(sizeof(cmd), 2, cmd, r))

if (spi_command(JEDEC_REMS_OUTSIZE, JEDEC_REMS_INSIZE, cmd, r))
> return 0;
> if (!oddparity(r[0]))
> printf_debug("0x90 RDID byte 0 parity violation.\n");

"REMS..."
> printf_debug("%s: id1 0x%02x, id2 0x%02x\n", __func__, r[0], r[1]);
> if (r[0] != flash->manufacture_id || r[1] != flash->model_id)
> return 0;
> spi_prettyprint_status_register(flash);
> return 1;
>
> +}
>
> int probe_spi_rdid(struct flashchip *flash) {
>    return probe_spi_rdid_generic(flash, 3);
>  }
> @@ -142,6 +163,7 @@
>    case BUS_TYPE_ICH7_SPI:
>    case BUS_TYPE_ICH9_SPI:
>    case BUS_TYPE_VIA_SPI:
> case BUS_TYPE_WBSIO_SPI:
> return probe_spi_rdid_generic(flash, 4);
> default:
> printf_debug("4b ID not supported on this SPI controller\n");
>
> @@ -182,7 +204,7 @@
>  uint8_t spi_read_status_register()
>  {
>    const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = {JEDEC_RDSR};
> unsigned char readarr[JEDEC_RDSR_INSIZE];
> unsigned char readarr[2]; /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */

Simply change JEDEC_RDSR_INSIZE to 2.
> /* Read Status Register */
>    spi_command(sizeof(cmd), sizeof(readarr), cmd, readarr);
> @@ -243,6 +265,24 @@
>    	bpt[(status & 0x1c) >> 2]);
>  }
>
> +void spi_prettyprint_status_register_sst25vf040b(uint8_t status)
> +{
> const char *bpt[] = {
> "none",
> "0x70000-0x7ffff",
> "0x60000-0x7ffff",
> "0x40000-0x7ffff",
> "all blocks", "all blocks", "all blocks", "all blocks"
> };
> printf_debug("Chip status register: Block Protect Write Disable "
> "(BPL) is %sset\n", (status & (1 << 7)) ? "" : "not ");
> printf_debug("Chip status register: Auto Address Increment Programming "
> "(AAI) is %sset\n", (status & (1 << 6)) ? "" : "not ");
> spi_prettyprint_status_register_common(status);
> printf_debug("Resulting block protection : %s\n",
> bpt[(status & 0x3c) >> 2]);
>
> +}
>
> void spi_prettyprint_status_register(struct flashchip *flash)
>  {
>    uint8_t status;
> @@ -260,8 +300,15 @@
>    		spi_prettyprint_status_register_st_m25p(status);
>    	break;
>    case SST_ID:
> if (flash->model_id == SST_25VF016B)
> switch (flash->model_id) {
> case SST_25VF016B:
> spi_prettyprint_status_register_sst25vf016(status);
> 	break;
> case 0x8d:
> case SST_25VF040B:

These case statements are very inconsistent. Please use #defines for all
of them.
> 	spi_prettyprint_status_register_sst25vf040b(status);
> 	break;
> }
> break;
> }
>
> }
> @@ -384,6 +431,8 @@
>    case BUS_TYPE_ICH9_SPI:
>    case BUS_TYPE_VIA_SPI:
>    	return ich_spi_read(flash, buf);
> case BUS_TYPE_WBSIO_SPI:
> return wbsio_spi_read(flash, buf);
> default:
> printf_debug("%s called, but no SPI chipset/strapping detected\n", __FUNCTION__);
> }
>
> @@ -400,6 +449,8 @@
>    case BUS_TYPE_ICH9_SPI:
>    case BUS_TYPE_VIA_SPI:
>    	return ich_spi_write(flash, buf);
> case BUS_TYPE_WBSIO_SPI:
> return wbsio_spi_write(flash, buf);
> default:
> printf_debug("%s called, but no SPI chipset/strapping detected\n", __FUNCTION__);
> }
>
> @@ -407,3 +458,28 @@
>    return 1;
>  }
>
> +int spi_aai_write(struct flashchip *flash, uint8_t *buf) {
> uint32_t pos = 2, size = flash->total_size * 1024;
> unsigned char w[6] = {0xad, 0, 0, 0, buf[0], buf[1]};

Symbolic constants/#define please.
> switch (flashbus) {
> case BUS_TYPE_WBSIO_SPI:

This switch/case statement needs to be a bit more complicated: For IT87
SPI, something similar to AAI is possible if the number of bytes is 256
or less and if the addresses are below 512k.
> 	fprintf(stderr, "%s: impossible through Winbond SPI masters, degrading to byte program\n", __func__);
> 	return spi_chip_write(flash, buf);
> default:
> 	break;
> }
> spi_chip_erase_c7(flash);

Please use the corresponding pointer in struct flashchip.
> spi_write_enable();
> spi_command(6, 0, w, NULL);

Symbolic constants/#define please.
> while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> myusec_delay(5); /* SST25VF040B Tbp is max 10us */
> while (pos < size) {
> w[1] = buf[pos++];
> w[2] = buf[pos++];
> spi_command(3, 0, w, NULL);
> while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> 	myusec_delay(5); /* SST25VF040B Tbp is max 10us */
> }
> spi_write_disable();
> return 0;
>
> +}

The function definitely needs more comments. The inner workings are not
obvious at first glance which suggests some rather clever programming.
> Index: Makefile
>
> --- Makefile	(revision 3418)
> +++ Makefile	(working copy)
> @@ -29,7 +29,7 @@
>    w49f002u.o 82802ab.o pm49fl00x.o sst49lf040.o \
>    sst49lfxxxc.o sst_fwhub.o layout.o cbtable.o flashchips.o \
>    flashrom.o w39v080fa.o sharplhf00l04.o w29ee011.o spi.o it87spi.o \
> ichspi.o
> ichspi.o wbsio_spi.o
>
> all: pciutils dep $(PROGRAM)
> Index: wbsio_spi.c
>
> --- wbsio_spi.c	(revision 0)
> +++ wbsio_spi.c	(revision 0)
> @@ -0,0 +1,195 @@
> +/*
> This file is part of the flashrom project.
> Copyright (C) 2008 Peter Stuge
> 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
> the Free Software Foundation; version 2 of the License.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <stdio.h>
> +#include <pci/pci.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include "flash.h"
> +#include "spi.h"
>
> +#define WBSIO_PORT1	0x2e
> +#define WBSIO_PORT2	0x4e
>
> +static uint16_t wbsio_spibase = 0;
>
> +static uint16_t wbsio_get_spibase(uint16_t port) {
> uint8_t id;
> uint16_t flashport = 0;
> w836xx_ext_enter(port);
> id = wbsio_read(port, 0x20);
> if(id != 0xa0) {
> fprintf(stderr, "W83627 not found at 0x%x, id=0x%02x want=0xa0.\n", port, id);
> goto done;
> }
> if (0 == (wbsio_read(port, 0x24) & 2)) {
> fprintf(stderr, "W83627 found at 0x%x, but SPI pins are not enabled. (CR[0x24] bit 1=0)\n", port);
> goto done;
> }
> wbsio_write(port, 0x07, 0x06);
> if (0 == (wbsio_read(port, 0x30) & 1)) {
> fprintf(stderr, "W83627 found at 0x%x, but SPI is not enabled. (LDN6[0x30] bit 0=0)\n", port);
> goto done;
> }
> flashport = (wbsio_read(port, 0x62) << 8) | wbsio_read(port, 0x63);
>
> +done:
> w836xx_ext_leave(port);
> return flashport;
>
> +}
>
> +int wbsio_check_for_spi(const char *name) {
> if (0 == (wbsio_spibase = wbsio_get_spibase(WBSIO_PORT1)))
> if (0 == (wbsio_spibase = wbsio_get_spibase(WBSIO_PORT2)))
> 	return 1;
> printf_debug("wbsio_spibase = 0x%x\n", wbsio_spibase);
> flashbus = BUS_TYPE_WBSIO_SPI;
> return 0;
>
> +}
>
> +/* W83627DHG has 11 command modes:
> 1=1 command only
> 2=1 command+1 data write
> 3=1 command+2 data read
> 4=1 command+3 address
> 5=1 command+3 address+1 data write
> 6=1 command+3 address+4 data write
> 7=1 command+3 address+1 dummy address inserted by wbsio+4 data read
> 8=1 command+3 address+1 data read
> 9=1 command+3 address+2 data read
> a=1 command+3 address+3 data read
> b=1 command+3 address+4 data read
> */
>
> +int wbsio_spi_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) {
> int i;
> uint8_t mode = 0;
> printf_debug("%s:", __func__);
> if (1 == writecnt && 0 == readcnt) {
> mode = 0x10;
> } else if (2 == writecnt && 0 == readcnt) {
> OUTB(writearr[1], wbsio_spibase + 4);
> printf_debug(" data=0x%02x", writearr[1]);
> mode = 0x20;
> } else if (1 == writecnt && 2 == readcnt) {
> mode = 0x30;
> } else if (4 == writecnt && 0 == readcnt) {
> printf_debug(" addr=0x%02x", (writearr[1] & 0x0f));
> for (i = 2; i < writecnt; i++) {
> 	OUTB(writearr[i], wbsio_spibase + i);
> 	printf_debug("%02x", writearr[i]);
> }
> mode = 0x40 | (writearr[1] & 0x0f);
> } else if (5 == writecnt && 0 == readcnt) {
> printf_debug(" addr=0x%02x", (writearr[1] & 0x0f));
> for (i = 2; i < 4; i++) {
> 	OUTB(writearr[i], wbsio_spibase + i);
> 	printf_debug("%02x", writearr[i]);
> }
> OUTB(writearr[i], wbsio_spibase + i);
> printf_debug(" data=0x%02x", writearr[i]);
> mode = 0x50 | (writearr[1] & 0x0f);
> } else if (8 == writecnt && 0 == readcnt) {
> printf_debug(" addr=0x%02x", (writearr[1] & 0x0f));
> for (i = 2; i < 4; i++) {
> 	OUTB(writearr[i], wbsio_spibase + i);
> 	printf_debug("%02x", writearr[i]);
> }
> printf_debug(" data=0x");
> for (; i < writecnt; i++) {
> 	OUTB(writearr[i], wbsio_spibase + i);
> 	printf_debug("%02x", writearr[i]);
> }
> mode = 0x60 | (writearr[1] & 0x0f);
> } else if (5 == writecnt && 4 == readcnt) {
> /* XXX: TODO not supported by flashrom infrastructure!
>  * This mode, 7, discards the fifth byte in writecnt,
>  * but since we can not express that in flashrom, fail
>  * the operation for now.
>  */
> ;
> } else if (4 == writecnt && readcnt >= 1 && readcnt <= 4) {
> printf_debug(" addr=0x%02x", (writearr[1] & 0x0f));
> for (i = 2; i < writecnt; i++) {
> 	OUTB(writearr[i], wbsio_spibase + i);
> 	printf_debug("%02x", writearr[i]);
> }
> mode = ((7 + readcnt) << 4) | (writearr[1] & 0x0f);
> }
> printf_debug(" cmd=%02x mode=%02x\n", writearr[0], mode);
> if (!mode) {
> fprintf(stderr, "%s: unsupported command type wr=%d rd=%d\n",
> 	__func__, writecnt, readcnt);
> return 1;
> }
> OUTB(writearr[0], wbsio_spibase);
> OUTB(mode, wbsio_spibase + 1);
> myusec_delay(10);
> if (!readcnt)
> return 0;
> printf_debug("%s: returning data =", __func__);
> for (i = 0; i < readcnt; i++) {
> readarr[i] = INB(wbsio_spibase + 4 + i);
> printf_debug(" 0x%02x", readarr[i]);
> }
> printf_debug("\n");
> return 0;
>
> +}
>
> +int wbsio_spi_read(struct flashchip *flash, uint8_t *buf) {
> int size = flash->total_size * 1024;
> if (flash->total_size > 1024) {
> fprintf(stderr, "%s: Winbond saved on 4 register bits, max chip size is 512 KB. :\\\n", __func__);
> return 1;
> }
> memcpy(buf, (const char *)flash->virtual_memory, size);
> return 0;
>
> +}
>
> +int wbsio_spi_write(struct flashchip *flash, uint8_t *buf) {
> int pos, size = flash->total_size * 1024;
> if (flash->total_size > 1024) {
> fprintf(stderr, "%s: Winbond saved on 4 register bits, max chip size is 512 KB. :\\\n", __func__);
> return 1;
> }
> flash->erase(flash);
> spi_write_enable();
> for (pos = 0; pos < size; pos++) {
> spi_byte_program(pos, buf[pos]);
> while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> 	myusec_delay(10);
> }
> spi_write_disable();
> return 0;
>
> +}
> Index: board_enable.c
> ===================================================================
> --- board_enable.c	(revision 3418)
> +++ board_enable.c	(working copy)
> @@ -35,32 +35,32 @@
> Helper functions for many Winbond Super I/Os of the W836xx range.
>
> */
>  /* Enter extended functions */
> -static void w836xx_ext_enter(uint16_t port)
> +void w836xx_ext_enter(uint16_t port)
>  {
>    OUTB(0x87, port);
>    OUTB(0x87, port);
>  }
>
> /* Leave extended functions */
> -static void w836xx_ext_leave(uint16_t port)
> +void w836xx_ext_leave(uint16_t port)
>  {
>    OUTB(0xAA, port);
>  }
>
> /* General functions for reading/writing Winbond Super I/Os. */
> -static unsigned char wbsio_read(uint16_t index, uint8_t reg)
> +unsigned char wbsio_read(uint16_t index, uint8_t reg)
>  {
>    OUTB(reg, index);
>    return INB(index + 1);
>  }
>
> -static void wbsio_write(uint16_t index, uint8_t reg, uint8_t data)
> +void wbsio_write(uint16_t index, uint8_t reg, uint8_t data)
>  {
>    OUTB(reg, index);
>    OUTB(data, index + 1);
>  }
>
> -static void wbsio_mask(uint16_t index, uint8_t reg, uint8_t data, uint8_t mask)
> +void wbsio_mask(uint16_t index, uint8_t reg, uint8_t data, uint8_t mask)
>  {
>    uint8_t tmp;
>
> @@ -617,6 +617,8 @@
>     NULL, NULL, "BioStar P4M80-M4", board_biostar_p4m80_m4},
>    {0x1106, 0x3227, 0x1458, 0x5001, 0x10ec, 0x8139, 0x1458, 0xe000,
>     NULL, NULL, "GIGABYTE GA-7VT600", board_biostar_p4m80_m4},
> {0x1039, 0x5513, 0x8086, 0xd61f, 0x1039, 0x6330, 0x8086, 0xd61f,
> NULL, NULL, "Intel Desktop Board D201GLY", wbsio_check_for_spi},
> {0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL}	/* Keep this */
>
> };

Regards,
Carl-Daniel