Re: [coreboot] 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