Create spi programer struct

[Jakob Bornecrantz]

Hi all

Here is a patch that replaces most of the switch cases in the spi code
with lookup on a struct instead. I have only done limited testing with
the external ft2232 programmer.

Cheers Jakob.

[Jakob Bornecrantz]

Hi all

Here is a updated version of the patch with some fixes spotted by
Card-Daniel and also rebased on top of latest flashrom trunk.

Cheers Jakob.

[Carl-Daniel Hailfinger]

On 11.07.2009 23:41, Jakob Bornecrantz wrote:
> Here is a updated version of the patch with some fixes spotted by
> Carl-Daniel and also rebased on top of latest flashrom trunk.

Thanks! Review follows.

A slightly more verbose changelog would be appreciated. The changelog of
your original patch fits the mandate.
A Signed-off-by statement was missing.
> diff --git a/flash.h b/flash.h
> index 8783032..e68b719 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -420,8 +420,24 @@ struct spi_command {
>    const unsigned char *writearr;
>    unsigned char *readarr;
>  };
> +struct spi_programmer {
> int (*command)(unsigned int writecnt, unsigned int readcnt,
>    const unsigned char *writearr, unsigned char *readarr);
> int (*multicommand)(struct spi_command *spicommands);
> /*
> * XXX Special work around commands.

Please use FIXME instead of XXX. A few editors have autohighlighting for
the former.
> *
> * These should be removed later on and instead we should filter the
> * command stream and apply the work arounds there.

Filtering the command stream is highly fragile. True, many functions
could be refactored, but I think the read and the write_256 function
will always be highly master specific (some masters don't even send SPI
commands for these actions). Your comment is completely valid for the
status register read, though, so maybe place it before read_status_register.
> */
> int (*read)(struct flashchip *flash, uint8_t *buf, int start, int len);
> int (*write_256)(struct flashchip *flash, uint8_t *buf);
> uint8_t (*read_status_register)(void);
>
> +};
> diff --git a/spi.c b/spi.c
> index c5ace17..a3b428b 100644
> --- a/spi.c
> +++ b/spi.c
> @@ -32,59 +32,91 @@ void *spibar = NULL;
>
> void spi_prettyprint_status_register(struct flashchip *flash);
>
> +const struct spi_programmer spi_programmer[] = {
> { /* SPI_CONTROLLER_NONE */
> .command = dummy_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = NULL,
> .write_256 = NULL,
> .read_status_register = NULL,
> },

All functions for CONTROLLER_NONE should be NULL. Dummy is a logging
programmer and you explicitly don't want to log accesses.
> { /* SPI_CONTROLLER_ICH7 */
> .command = ich_spi_send_command,
> .multicommand = ich_spi_send_multicommand,
> .read = ich_spi_read,
> .write_256 = ich_spi_write_256,
> .read_status_register = NULL,

I believe the read_status_register exception for SB600 is a bug in the
driver resulting from a bug in the hardware. It needs to die, but I'm
already tackling that.
Until then, could you set read_status_register to the correct function
instead of NULL for all others?
> },
> { /* SPI_CONTROLLER_ICH9 */
> .command = ich_spi_send_command,
> .multicommand = ich_spi_send_multicommand,
> .read = ich_spi_read,
> .write_256 = ich_spi_write_256,
> .read_status_register = NULL,
> },
> { /* SPI_CONTROLLER_IT87XX */
> .command = it8716f_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = it8716f_spi_chip_read,
> .write_256 = it8716f_spi_chip_write_256,
> .read_status_register = NULL,
> },
> { /* SPI_CONTROLLER_SB600 */
> .command = sb600_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = sb600_spi_read,
> .write_256 = sb600_spi_write_1,
> .read_status_register = sb600_read_status_register,
> },
> { /* SPI_CONTROLLER_VIA */
> .command = ich_spi_send_command,
> .multicommand = ich_spi_send_multicommand,
> .read = ich_spi_read,
> .write_256 = ich_spi_write_256,
> .read_status_register = NULL,
> },
> { /* SPI_CONTROLLER_WBSIO */
> .command = wbsio_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = wbsio_spi_read,
> .write_256 = wbsio_spi_write_1,
> .read_status_register = NULL,
> },
> { /* SPI_CONTROLLER_FT2232 */
> .command = ft2232_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = ft2232_spi_read,
> .write_256 = ft2232_spi_write_256,
> .read_status_register = NULL,
> },
> { /* SPI_CONTROLLER_DUMMY */
> .command = dummy_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = NULL,
> .write_256 = NULL,

Are read and write_256 really not implemented for the dummy flasher?
> .read_status_register = NULL,
> },
>
> +};
>
> int spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    	const unsigned char *writearr, unsigned char *readarr)
>  {
> -[...]

Please check for (!spi_programmer[spi_controller].command) and return 1
(or create a new define FUNCTION_NOT_IMPLEMENTED) otherwise.
> return spi_programmer[spi_controller].command(writecnt, readcnt,
> 				      writearr, readarr);
>
> }
>
> int spi_send_multicommand(struct spi_command *spicommands)
>  {
> -[...]

Same as above.
> return spi_programmer[spi_controller].multicommand(spicommands);
>
> }
>
> static int spi_rdid(unsigned char *readarr, int bytes)
> @@ -315,15 +347,12 @@ uint8_t spi_read_status_register(void)
>    unsigned char readarr[2]; /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */

I need to figure out why wbsio needs special treatment here.
> int ret;
> /* Read Status Register */
> if (spi_controller == SPI_CONTROLLER_SB600) {
> /* SB600 uses a different way to read status register. */
> return sb600_read_status_register();
> } else {
> ret = spi_send_command(sizeof(cmd), sizeof(readarr), cmd, readarr);
> if (ret)
> 	printf_debug("RDSR failed!\n");
> }
> if (spi_programmer[spi_controller].read_status_register)
> return spi_programmer[spi_controller].read_status_register();
> ret = spi_send_command(sizeof(cmd), sizeof(readarr), cmd, readarr);
> if (ret)
> printf_debug("RDSR failed!\n");

Maybe leave the function above unconverted. The individual drivers need
to be fixed.
> return readarr[0];
>  }
> @@ -810,26 +839,14 @@ int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len,
>
> int spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {
> switch (spi_controller) {
> case SPI_CONTROLLER_IT87XX:
> return it8716f_spi_chip_read(flash, buf, start, len);
> case SPI_CONTROLLER_SB600:
> return sb600_spi_read(flash, buf, start, len);
> case SPI_CONTROLLER_ICH7:
> case SPI_CONTROLLER_ICH9:
> case SPI_CONTROLLER_VIA:
> return ich_spi_read(flash, buf, start, len);
> case SPI_CONTROLLER_WBSIO:
> return wbsio_spi_read(flash, buf, start, len);
> case SPI_CONTROLLER_FT2232:
> return ft2232_spi_read(flash, buf, start, len);
> default:
> if (!spi_programmer[spi_controller].read) {
> printf_debug
>    ("%s called, but no SPI chipset/strapping detected\n",
>     __FUNCTION__);
> return 1;

Right. The "no strapping detected is also a possible error message for
the functions you converted above.
> }
> return 1;
> return spi_programmer[spi_controller].read(flash, buf, start, len);
>
> }
>
> /*
> @@ -922,3 +927,32 @@ int spi_aai_write(struct flashchip *flash, uint8_t *buf)
> +int default_spi_send_multicommand(struct spi_command *spicommands)
> +{
> int res = 0;
> while ((spicommands->writecnt || spicommands->readcnt) && !res) {
> res = spi_send_command(spicommands->writecnt, spicommands->readcnt,
> 		       spicommands->writearr, spicommands->readarr);
> }
> return res;
>
> +}

We could think about returning more info to the caller. After all, the
caller might be interested in the exact command which failed. No need to
address this now.

Regards,
Carl-Daniel

[PATCH] Create spi programmer struct

[Carl-Daniel Hailfinger]

Replace most of the switch cases in the spi code with lookup on a struct
instead. This brings the SPI code in line with the generic programmer
infrastructure.

This patch is a reworked version of a patch by Jakob Bornecrantz.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-spi_programmer_struct/flash.h
===================================================================
--- flashrom-spi_programmer_struct/flash.h	(Revision 656)
+++ flashrom-spi_programmer_struct/flash.h	(Arbeitskopie)
@@ -420,8 +420,18 @@
    const unsigned char *writearr;
    unsigned char *readarr;
 };
+struct spi_programmer {
+	int (*command)(unsigned int writecnt, unsigned int readcnt,
+		   const unsigned char *writearr, unsigned char *readarr);
+	int (*multicommand)(struct spi_command *spicommands);

+	/* Optimized functions for this programmer */
+	int (*read)(struct flashchip *flash, uint8_t *buf, int start, int len);
+	int (*write_256)(struct flashchip *flash, uint8_t *buf);
+};
+
 extern enum spi_controller spi_controller;
+extern const struct spi_programmer spi_programmer[];
 extern void *spibar;
 int probe_spi_rdid(struct flashchip *flash);
 int probe_spi_rdid4(struct flashchip *flash);
@@ -452,6 +462,9 @@
 int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize);
 int spi_aai_write(struct flashchip *flash, uint8_t *buf);
 uint32_t spi_get_valid_read_addr(void);
+int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
+			     const unsigned char *writearr, unsigned char *readarr);
+int default_spi_send_multicommand(struct spi_command *spicommands);

/* 82802ab.c */
 int probe_82802ab(struct flashchip *flash);
@@ -477,6 +490,7 @@
    	    const unsigned char *writearr, unsigned char *readarr);
 int ich_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len);
 int ich_spi_write_256(struct flashchip *flash, uint8_t * buf);
+int ich_spi_send_multicommand(struct spi_command *spicommands);

/* it87spi.c */
 extern char *it87opts;
Index: flashrom-spi_programmer_struct/spi.c
===================================================================
--- flashrom-spi_programmer_struct/spi.c	(Revision 656)
+++ flashrom-spi_programmer_struct/spi.c	(Arbeitskopie)
@@ -32,61 +32,125 @@

void spi_prettyprint_status_register(struct flashchip *flash);

+const struct spi_programmer spi_programmer[] = {
+	{ /* SPI_CONTROLLER_NONE */
+		.command = NULL,
+		.multicommand = NULL,
+		.read = NULL,
+		.write_256 = NULL,
+	},
+
+	{ /* SPI_CONTROLLER_ICH7 */
+		.command = ich_spi_send_command,
+		.multicommand = ich_spi_send_multicommand,
+		.read = ich_spi_read,
+		.write_256 = ich_spi_write_256,
+	},
+
+	{ /* SPI_CONTROLLER_ICH9 */
+		.command = ich_spi_send_command,
+		.multicommand = ich_spi_send_multicommand,
+		.read = ich_spi_read,
+		.write_256 = ich_spi_write_256,
+	},
+
+	{ /* SPI_CONTROLLER_IT87XX */
+		.command = it8716f_spi_send_command,
+		.multicommand = default_spi_send_multicommand,
+		.read = it8716f_spi_chip_read,
+		.write_256 = it8716f_spi_chip_write_256,
+	},
+
+	{ /* SPI_CONTROLLER_SB600 */
+		.command = sb600_spi_send_command,
+		.multicommand = default_spi_send_multicommand,
+		.read = sb600_spi_read,
+		.write_256 = sb600_spi_write_1,
+	},
+
+	{ /* SPI_CONTROLLER_VIA */
+		.command = ich_spi_send_command,
+		.multicommand = ich_spi_send_multicommand,
+		.read = ich_spi_read,
+		.write_256 = ich_spi_write_256,
+	},
+
+	{ /* SPI_CONTROLLER_WBSIO */
+		.command = wbsio_spi_send_command,
+		.multicommand = default_spi_send_multicommand,
+		.read = wbsio_spi_read,
+		.write_256 = wbsio_spi_write_1,
+	},
+
+	{ /* SPI_CONTROLLER_FT2232 */
+		.command = ft2232_spi_send_command,
+		.multicommand = default_spi_send_multicommand,
+		.read = ft2232_spi_read,
+		.write_256 = ft2232_spi_write_256,
+	},
+
+	{ /* SPI_CONTROLLER_DUMMY */
+		.command = dummy_spi_send_command,
+		.multicommand = default_spi_send_multicommand,
+		.read = NULL,
+		.write_256 = NULL,
+	},
+};
+
+
 int spi_send_command(unsigned int writecnt, unsigned int readcnt,
    	const unsigned char *writearr, unsigned char *readarr)
 {
-	switch (spi_controller) {
-	case SPI_CONTROLLER_IT87XX:
-		return it8716f_spi_send_command(writecnt, readcnt, writearr,
-					   readarr);
-	case SPI_CONTROLLER_ICH7:
-	case SPI_CONTROLLER_ICH9:
-	case SPI_CONTROLLER_VIA:
-		return ich_spi_send_command(writecnt, readcnt, writearr, readarr);
-	case SPI_CONTROLLER_SB600:
-		return sb600_spi_send_command(writecnt, readcnt, writearr, readarr);
-	case SPI_CONTROLLER_WBSIO:
-		return wbsio_spi_send_command(writecnt, readcnt, writearr, readarr);
-	case SPI_CONTROLLER_FT2232:
-		return ft2232_spi_send_command(writecnt, readcnt, writearr, readarr);
-	case SPI_CONTROLLER_DUMMY:
-		return dummy_spi_send_command(writecnt, readcnt, writearr, readarr);
-	default:
-		printf_debug
-		    ("%s called, but no SPI chipset/strapping detected\n",
-		     __FUNCTION__);
+	if (!spi_programmer[spi_controller].command) {
+		fprintf(stderr, "%s called, but SPI is unsupported on this "
+			"hardware. Please report a bug.\n", __func__);
+		return 1;
    }
-	return 1;
+
+	return spi_programmer[spi_controller].command(writecnt, readcnt,
+						      writearr, readarr);
 }

int spi_send_multicommand(struct spi_command *spicommands)
 {
-	int ret = 0;
-	while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
-		ret = spi_send_command(spicommands->writecnt, spicommands->readcnt,
-				       spicommands->writearr, spicommands->readarr);
-		/* This awful hack needs to be replaced with a multicommand
-		 * capable ICH/VIA SPI driver.
-		 */
-		if ((ret == SPI_INVALID_OPCODE) &&
-		    ((spicommands->writearr[0] == JEDEC_WREN) ||
-		     (spicommands->writearr[0] == JEDEC_EWSR))) {
-			switch (spi_controller) {
-			case SPI_CONTROLLER_ICH7:
-			case SPI_CONTROLLER_ICH9:
-			case SPI_CONTROLLER_VIA:
-				printf_debug(" due to SPI master limitation, ignoring"
-					     " and hoping it will be run as PREOP\n");
-				ret = 0;
-			default:
-				break;
-			}
-		}
-		spicommands++;
+	if (!spi_programmer[spi_controller].multicommand) {
+		fprintf(stderr, "%s called, but SPI is unsupported on this "
+			"hardware. Please report a bug.\n", __func__);
+		return 1;
    }
-	return ret;
+
+	return spi_programmer[spi_controller].multicommand(spicommands);
 }

+int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
+			     const unsigned char *writearr, unsigned char *readarr)
+{
+	struct spi_command cmd[] = {
+	{
+		.writecnt = writecnt,
+		.readcnt = readcnt,
+		.writearr = writearr,
+		.readarr = readarr,
+	}, {
+		.writecnt = 0,
+		.writearr = NULL,
+		.readcnt = 0,
+		.readarr = NULL,
+	}};
+
+	return spi_send_multicommand(cmd);
+}
+
+int default_spi_send_multicommand(struct spi_command *spicommands)
+{
+	int result = 0;
+	while ((spicommands->writecnt || spicommands->readcnt) && !result) {
+		result = spi_send_command(spicommands->writecnt, spicommands->readcnt,
+					  spicommands->writearr, spicommands->readarr);
+	}
+	return result;
+}
+
 static int spi_rdid(unsigned char *readarr, int bytes)
 {
    const unsigned char cmd[JEDEC_RDID_OUTSIZE] = { JEDEC_RDID };
@@ -298,18 +362,18 @@
 uint8_t spi_read_status_register(void)
 {
    const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { JEDEC_RDSR };
+	/* FIXME: No workarounds for driver/hardware bugs in generic code. */
    unsigned char readarr[2]; /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */
    int ret;

/* Read Status Register */
-	if (spi_controller == SPI_CONTROLLER_SB600) {
-		/* SB600 uses a different way to read status register. */
+	if (spi_controller == SPI_CONTROLLER_SB600) { /* FIXME */
+		/* Workaround for SB600 hardware bug. Can be killed later. */
    	return sb600_read_status_register();
-	} else {
-		ret = spi_send_command(sizeof(cmd), sizeof(readarr), cmd, readarr);
-		if (ret)
-			printf_debug("RDSR failed!\n");
    }
+	ret = spi_send_command(sizeof(cmd), sizeof(readarr), cmd, readarr);
+	if (ret)
+		printf_debug("RDSR failed!\n");

return readarr[0];
 }
@@ -875,26 +939,13 @@

int spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len)
 {
-	switch (spi_controller) {
-	case SPI_CONTROLLER_IT87XX:
-		return it8716f_spi_chip_read(flash, buf, start, len);
-	case SPI_CONTROLLER_SB600:
-		return sb600_spi_read(flash, buf, start, len);
-	case SPI_CONTROLLER_ICH7:
-	case SPI_CONTROLLER_ICH9:
-	case SPI_CONTROLLER_VIA:
-		return ich_spi_read(flash, buf, start, len);
-	case SPI_CONTROLLER_WBSIO:
-		return wbsio_spi_read(flash, buf, start, len);
-	case SPI_CONTROLLER_FT2232:
-		return ft2232_spi_read(flash, buf, start, len);
-	default:
-		printf_debug
-		    ("%s called, but no SPI chipset/strapping detected\n",
-		     __FUNCTION__);
+	if (!spi_programmer[spi_controller].read) {
+		fprintf(stderr, "%s called, but SPI read is unsupported on this"
+			" hardware. Please report a bug.\n", __func__);
+		return 1;
    }

-	return 1;
+	return spi_programmer[spi_controller].read(flash, buf, start, len);
 }

/*
@@ -924,26 +975,13 @@
  */
 int spi_chip_write_256(struct flashchip *flash, uint8_t *buf)
 {
-	switch (spi_controller) {
-	case SPI_CONTROLLER_IT87XX:
-		return it8716f_spi_chip_write_256(flash, buf);
-	case SPI_CONTROLLER_SB600:
-		return sb600_spi_write_1(flash, buf);
-	case SPI_CONTROLLER_ICH7:
-	case SPI_CONTROLLER_ICH9:
-	case SPI_CONTROLLER_VIA:
-		return ich_spi_write_256(flash, buf);
-	case SPI_CONTROLLER_WBSIO:
-		return wbsio_spi_write_1(flash, buf);
-	case SPI_CONTROLLER_FT2232:
-		return ft2232_spi_write_256(flash, buf);
-	default:
-		printf_debug
-		    ("%s called, but no SPI chipset/strapping detected\n",
-		     __FUNCTION__);
+	if (!spi_programmer[spi_controller].write_256) {
+		fprintf(stderr, "%s called, but SPI page write is unsupported "
+			" on this hardware. Please report a bug.\n", __func__);
+		return 1;
    }

-	return 1;
+	return spi_programmer[spi_controller].write_256(flash, buf);
 }

uint32_t spi_get_valid_read_addr(void)
Index: flashrom-spi_programmer_struct/ichspi.c
===================================================================
--- flashrom-spi_programmer_struct/ichspi.c	(Revision 656)
+++ flashrom-spi_programmer_struct/ichspi.c	(Arbeitskopie)
@@ -742,3 +742,23 @@

return result;
 }
+
+int ich_spi_send_multicommand(struct spi_command *spicommands)
+{
+	int ret = 0;
+	while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
+		ret = ich_spi_send_command(spicommands->writecnt, spicommands->readcnt,
+					   spicommands->writearr, spicommands->readarr);
+		/* This awful hack needs to be smarter.
+		 */
+		if ((ret == SPI_INVALID_OPCODE) &&
+		    ((spicommands->writearr[0] == JEDEC_WREN) ||
+		     (spicommands->writearr[0] == JEDEC_EWSR))) {
+			printf_debug(" due to SPI master limitation, ignoring"
+				     " and hoping it will be run as PREOP\n");
+			ret = 0;
+		}
+		spicommands++;
+	}
+	return ret;
+}

[PATCH] Create spi programmer struct

[Stefan Reinauer]

On 7/22/09 4:17 PM, Carl-Daniel Hailfinger wrote:
> Replace most of the switch cases in the spi code with lookup on a struct
> instead. This brings the SPI code in line with the generic programmer
> infrastructure.
>
> This patch is a reworked version of a patch by Jakob Bornecrantz.
>
> Signed-off-by: Carl-Daniel Hailfinger

I like this idea.

Acked-by: Stefan Reinauer
> Index: flashrom-spi_programmer_struct/flash.h
>
> --- flashrom-spi_programmer_struct/flash.h	(Revision 656)
> +++ flashrom-spi_programmer_struct/flash.h	(Arbeitskopie)
> @@ -420,8 +420,18 @@
>    const unsigned char *writearr;
>    unsigned char *readarr;
>  };
> +struct spi_programmer {
>
> int (*command)(unsigned int writecnt, unsigned int readcnt,
>    const unsigned char *writearr, unsigned char *readarr);
>
> int (*multicommand)(struct spi_command *spicommands);
>
> /* Optimized functions for this programmer */
>
> int (*read)(struct flashchip *flash, uint8_t *buf, int start, int len);
>
> int (*write_256)(struct flashchip *flash, uint8_t *buf);
>
> +};
>
> extern enum spi_controller spi_controller;
> +extern const struct spi_programmer spi_programmer[];
>  extern void *spibar;
>  int probe_spi_rdid(struct flashchip *flash);
>  int probe_spi_rdid4(struct flashchip *flash);
> @@ -452,6 +462,9 @@
>  int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize);
>  int spi_aai_write(struct flashchip *flash, uint8_t *buf);
>  uint32_t spi_get_valid_read_addr(void);
> +int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> 	     const unsigned char *writearr, unsigned char *readarr);
>
> +int default_spi_send_multicommand(struct spi_command *spicommands);
>
> /* 82802ab.c */
>  int probe_82802ab(struct flashchip *flash);
> @@ -477,6 +490,7 @@
>    	    const unsigned char *writearr, unsigned char *readarr);
>  int ich_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len);
>  int ich_spi_write_256(struct flashchip *flash, uint8_t * buf);
> +int ich_spi_send_multicommand(struct spi_command *spicommands);
>
> /* it87spi.c */
>  extern char *it87opts;
> Index: flashrom-spi_programmer_struct/spi.c
> ===================================================================
> --- flashrom-spi_programmer_struct/spi.c	(Revision 656)
> +++ flashrom-spi_programmer_struct/spi.c	(Arbeitskopie)
> @@ -32,61 +32,125 @@
>
> void spi_prettyprint_status_register(struct flashchip *flash);
>
> +const struct spi_programmer spi_programmer[] = {
> { /* SPI_CONTROLLER_NONE */
> .command = NULL,
> .multicommand = NULL,
> .read = NULL,
> .write_256 = NULL,
> },
> { /* SPI_CONTROLLER_ICH7 */
> .command = ich_spi_send_command,
> .multicommand = ich_spi_send_multicommand,
> .read = ich_spi_read,
> .write_256 = ich_spi_write_256,
> },
> { /* SPI_CONTROLLER_ICH9 */
> .command = ich_spi_send_command,
> .multicommand = ich_spi_send_multicommand,
> .read = ich_spi_read,
> .write_256 = ich_spi_write_256,
> },
> { /* SPI_CONTROLLER_IT87XX */
> .command = it8716f_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = it8716f_spi_chip_read,
> .write_256 = it8716f_spi_chip_write_256,
> },
> { /* SPI_CONTROLLER_SB600 */
> .command = sb600_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = sb600_spi_read,
> .write_256 = sb600_spi_write_1,
> },
> { /* SPI_CONTROLLER_VIA */
> .command = ich_spi_send_command,
> .multicommand = ich_spi_send_multicommand,
> .read = ich_spi_read,
> .write_256 = ich_spi_write_256,
> },
> { /* SPI_CONTROLLER_WBSIO */
> .command = wbsio_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = wbsio_spi_read,
> .write_256 = wbsio_spi_write_1,
> },
> { /* SPI_CONTROLLER_FT2232 */
> .command = ft2232_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = ft2232_spi_read,
> .write_256 = ft2232_spi_write_256,
> },
> { /* SPI_CONTROLLER_DUMMY */
> .command = dummy_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = NULL,
> .write_256 = NULL,
> },
>
> +};
>
> int spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    	const unsigned char *writearr, unsigned char *readarr)
>  {
> switch (spi_controller) {
> case SPI_CONTROLLER_IT87XX:
> return it8716f_spi_send_command(writecnt, readcnt, writearr,
> 			   readarr);
> case SPI_CONTROLLER_ICH7:
> case SPI_CONTROLLER_ICH9:
> case SPI_CONTROLLER_VIA:
> return ich_spi_send_command(writecnt, readcnt, writearr, readarr);
> case SPI_CONTROLLER_SB600:
> return sb600_spi_send_command(writecnt, readcnt, writearr, readarr);
> case SPI_CONTROLLER_WBSIO:
> return wbsio_spi_send_command(writecnt, readcnt, writearr, readarr);
> case SPI_CONTROLLER_FT2232:
> return ft2232_spi_send_command(writecnt, readcnt, writearr, readarr);
> case SPI_CONTROLLER_DUMMY:
> return dummy_spi_send_command(writecnt, readcnt, writearr, readarr);
> default:
> printf_debug
>     ("%s called, but no SPI chipset/strapping detected\n",
>      __FUNCTION__);
> if (!spi_programmer[spi_controller].command) {
> fprintf(stderr, "%s called, but SPI is unsupported on this "
> 	"hardware. Please report a bug.\n", __func__);
> return 1;
> }
> return 1;
> return spi_programmer[spi_controller].command(writecnt, readcnt,
> 				      writearr, readarr);
>
> }
>
> int spi_send_multicommand(struct spi_command *spicommands)
>  {
> int ret = 0;
> while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
> ret = spi_send_command(spicommands->writecnt, spicommands->readcnt,
> 		       spicommands->writearr, spicommands->readarr);
> /* This awful hack needs to be replaced with a multicommand
>  * capable ICH/VIA SPI driver.
>  */
> if ((ret == SPI_INVALID_OPCODE) &&
>     ((spicommands->writearr[0] == JEDEC_WREN) ||
>      (spicommands->writearr[0] == JEDEC_EWSR))) {
> 	switch (spi_controller) {
> 	case SPI_CONTROLLER_ICH7:
> 	case SPI_CONTROLLER_ICH9:
> 	case SPI_CONTROLLER_VIA:
> 		printf_debug(" due to SPI master limitation, ignoring"
> 			     " and hoping it will be run as PREOP\n");
> 		ret = 0;
> 	default:
> 		break;
> 	}
> }
> spicommands++;
> if (!spi_programmer[spi_controller].multicommand) {
> fprintf(stderr, "%s called, but SPI is unsupported on this "
> 	"hardware. Please report a bug.\n", __func__);
> return 1;
> }
> return ret;
> return spi_programmer[spi_controller].multicommand(spicommands);
>
> }
>
> +int default_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> 	     const unsigned char *writearr, unsigned char *readarr)
>
> +{
> struct spi_command cmd[] = {
> {
> .writecnt = writecnt,
> .readcnt = readcnt,
> .writearr = writearr,
> .readarr = readarr,
> }, {
> .writecnt = 0,
> .writearr = NULL,
> .readcnt = 0,
> .readarr = NULL,
> }};
> return spi_send_multicommand(cmd);
>
> +}
>
> +int default_spi_send_multicommand(struct spi_command *spicommands)
> +{
> int result = 0;
> while ((spicommands->writecnt || spicommands->readcnt) && !result) {
> result = spi_send_command(spicommands->writecnt, spicommands->readcnt,
> 			  spicommands->writearr, spicommands->readarr);
> }
> return result;
>
> +}
>
> static int spi_rdid(unsigned char *readarr, int bytes)
>  {
>    const unsigned char cmd[JEDEC_RDID_OUTSIZE] = { JEDEC_RDID };
> @@ -298,18 +362,18 @@
>  uint8_t spi_read_status_register(void)
>  {
>    const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { JEDEC_RDSR };
>
> /* FIXME: No workarounds for driver/hardware bugs in generic code. */
> unsigned char readarr[2]; /* JEDEC_RDSR_INSIZE=1 but wbsio needs 2 */
> int ret;
>
> /* Read Status Register */
> if (spi_controller == SPI_CONTROLLER_SB600) {
> /* SB600 uses a different way to read status register. */
> if (spi_controller == SPI_CONTROLLER_SB600) { /* FIXME */
> /* Workaround for SB600 hardware bug. Can be killed later. */
> return sb600_read_status_register();
> } else {
> ret = spi_send_command(sizeof(cmd), sizeof(readarr), cmd, readarr);
> if (ret)
> 	printf_debug("RDSR failed!\n");
> }
>
> ret = spi_send_command(sizeof(cmd), sizeof(readarr), cmd, readarr);
>
> if (ret)
> printf_debug("RDSR failed!\n");
>
> return readarr[0];
>
> }
> @@ -875,26 +939,13 @@
>
> int spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {
> switch (spi_controller) {
> case SPI_CONTROLLER_IT87XX:
> return it8716f_spi_chip_read(flash, buf, start, len);
> case SPI_CONTROLLER_SB600:
> return sb600_spi_read(flash, buf, start, len);
> case SPI_CONTROLLER_ICH7:
> case SPI_CONTROLLER_ICH9:
> case SPI_CONTROLLER_VIA:
> return ich_spi_read(flash, buf, start, len);
> case SPI_CONTROLLER_WBSIO:
> return wbsio_spi_read(flash, buf, start, len);
> case SPI_CONTROLLER_FT2232:
> return ft2232_spi_read(flash, buf, start, len);
> default:
> printf_debug
>     ("%s called, but no SPI chipset/strapping detected\n",
>      __FUNCTION__);
> if (!spi_programmer[spi_controller].read) {
> fprintf(stderr, "%s called, but SPI read is unsupported on this"
> 	" hardware. Please report a bug.\n", __func__);
> return 1;
> }
> return 1;
> return spi_programmer[spi_controller].read(flash, buf, start, len);
>
> }
>
> /*
> @@ -924,26 +975,13 @@
>   */
>  int spi_chip_write_256(struct flashchip *flash, uint8_t *buf)
>  {
> switch (spi_controller) {
> case SPI_CONTROLLER_IT87XX:
> return it8716f_spi_chip_write_256(flash, buf);
> case SPI_CONTROLLER_SB600:
> return sb600_spi_write_1(flash, buf);
> case SPI_CONTROLLER_ICH7:
> case SPI_CONTROLLER_ICH9:
> case SPI_CONTROLLER_VIA:
> return ich_spi_write_256(flash, buf);
> case SPI_CONTROLLER_WBSIO:
> return wbsio_spi_write_1(flash, buf);
> case SPI_CONTROLLER_FT2232:
> return ft2232_spi_write_256(flash, buf);
> default:
> printf_debug
>     ("%s called, but no SPI chipset/strapping detected\n",
>      __FUNCTION__);
> if (!spi_programmer[spi_controller].write_256) {
> fprintf(stderr, "%s called, but SPI page write is unsupported "
> 	" on this hardware. Please report a bug.\n", __func__);
> return 1;
> }
> return 1;
> return spi_programmer[spi_controller].write_256(flash, buf);
>
> }
>
> uint32_t spi_get_valid_read_addr(void)
> Index: flashrom-spi_programmer_struct/ichspi.c
> ===================================================================
> --- flashrom-spi_programmer_struct/ichspi.c	(Revision 656)
> +++ flashrom-spi_programmer_struct/ichspi.c	(Arbeitskopie)
> @@ -742,3 +742,23 @@
>
> return result;
>  }
>
> +int ich_spi_send_multicommand(struct spi_command *spicommands)
> +{
> int ret = 0;
> while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
> ret = ich_spi_send_command(spicommands->writecnt, spicommands->readcnt,
> 			   spicommands->writearr, spicommands->readarr);
> /* This awful hack needs to be smarter.
>  */
> if ((ret == SPI_INVALID_OPCODE) &&
>     ((spicommands->writearr[0] == JEDEC_WREN) ||
>      (spicommands->writearr[0] == JEDEC_EWSR))) {
> 	printf_debug(" due to SPI master limitation, ignoring"
> 		     " and hoping it will be run as PREOP\n");
> 	ret = 0;
> }
> spicommands++;
> }
> return ret;
>
> +}

[PATCH] Create spi programmer struct

[Stefan Reinauer]

On 7/22/09 4:17 PM, Carl-Daniel Hailfinger wrote:
> /* FIXME: No workarounds for driver/hardware bugs in generic code. */

Does this mean the patch creates a regression?

[PATCH] Create spi programmer struct

[Carl-Daniel Hailfinger]

On 22.07.2009 16:22, Stefan Reinauer wrote:
> On 7/22/09 4:17 PM, Carl-Daniel Hailfinger wrote:
> > /* FIXME: No workarounds for driver/hardware bugs in generic code. */
>
> Does this mean the patch creates a regression?

No, it just points out existing problems for which patches are pending
and serves as a reminder.

Regards,
Carl-Daniel

[PATCH] Create spi programmer struct

[Jakob Bornecrantz]

On Wed, Jul 22, 2009 at 4:17 PM, Carl-Daniel
Hailfinger wrote:
> Replace most of the switch cases in the spi code with lookup on a struct
> instead. This brings the SPI code in line with the generic programmer
> infrastructure.
>
> This patch is a reworked version of a patch by Jakob Bornecrantz.
>
> Signed-off-by: Carl-Daniel Hailfinger

Signed-off-by: Jakob Bornecrantz 
Acked-by: Jakob Bornecrantz

But it would be better if somebody who hasn't been involved also acks
the patch before it going in.

[SNIP]

Cheers Jakob.

[PATCH] Create spi programmer struct

[Carl-Daniel Hailfinger]

On 22.07.2009 16:21, Stefan Reinauer wrote:
> On 7/22/09 4:17 PM, Carl-Daniel Hailfinger wrote:
> > Replace most of the switch cases in the spi code with lookup on a struct
> > instead. This brings the SPI code in line with the generic programmer
> > infrastructure.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
>
> I like this idea.
>
> Acked-by: Stefan Reinauer

Thanks, r657.

Regards,
Carl-Daniel