Re: [flashrom] Create spi programer struct

[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