Re: [flashrom] [PATCH] serprog SPI support

[Stefan Tauner]

hey there and thanks for your patch!

all in all the patch looks ok. the only really problem i see is the
opbuf stuff (see details below).

for the record: we now have 2,5 implementations of this laying around
(this, Juhana Helovuo's http://www.coreboot.org/InSystemFlasher and my
derivative of that). mine includes a command line argument to set the
spi frequency which is mapped to another opcode; the rest is almost the
same.

On Sun, 15 May 2011 06:58:19 +0300
Urja Rannikko  wrote:
> Index: serprog-protocol.txt
>
> --- serprog-protocol.txt	(revision 1299)
> +++ serprog-protocol.txt	(working copy)
> @@ -31,6 +31,8 @@
>  0x10	Sync NOP			none				NAK + ACK (for synchronization)
>  0x11	Query maximum read-n length	none				ACK + 24-bit length (0==2^24) / NAK
>  0x12	Set used bustype		8-bit flags (as with 0x05)	ACK / NAK
> +0x13	Perform SPI operation		24-bit slen + 24-bit rlen	ACK + rlen bytes of data / NAK
> 			 + slen bytes of data
>
> 0x??	unimplemented command - invalid.
>
> @@ -50,7 +52,7 @@
>    	it should return a big bogus value - eg 0xFFFF.
>    0x05 (Q_BUSTYPE):
>    	The bit's are defined as follows:
> bit 0: PARALLEL, bit 1: LPC, bit 2: FWH, bit 3: SPI (if ever supported).
> bit 0: PARALLEL, bit 1: LPC, bit 2: FWH, bit 3: SPI.
> 0x06 (Q_CHIPSIZE):
> Only applicable to parallel programmers.
> An LPC/FWH/SPI-programmer can report this as not supported in the command bitmap.
>
> @@ -66,6 +68,10 @@
>    	Set's the used bustype if the programmer can support more than one flash protocol.
>    	Sending a byte with more than 1 bit set will make the programmer decide among them
>    	on it's own. Bit values as with Q_BUSTYPE.
> 0x13 (O_SPIOP):
> Maximum slen is Q_WRNMAXLEN result after Q_BUSTYPE returns
> only SPI or S_BUSTYPE == SPI is used. Same for rlen and Q_RDNMAXLEN.
> This operation is immediate, meaning it doesnt use the operation buffer.
> About mandatory commands:
> The only truly mandatory commands for any device are 0x00, 0x01, 0x02 and 0x10,
> but one can't really do anything with these commands.
>
> @@ -99,3 +105,4 @@
>  #define S_CMD_SYNCNOP		0x10		/* Special no-operation that returns NAK+ACK	*/
>  #define S_CMD_Q_RDNMAXLEN	0x11		/* Query read-n maximum length			*/
>  #define S_CMD_S_BUSTYPE		0x12		/* Set used bustype(s).				*/
> +#define S_CMD_O_SPIOP		0x13		/* Perform SPI operation.			*/
> Index: serprog.c
> ===================================================================
> --- serprog.c	(revision 1299)
> +++ serprog.c	(working copy)
> @@ -1,7 +1,7 @@
>  /*
> This file is part of the flashrom project.
> Copyright (C) 2009 Urja Rannikko
> Copyright (C) 2009,2011 Urja Rannikko

space after ',' imho
> Copyright (C) 2009 Carl-Daniel Hailfinger
> This program is free software; you can redistribute it and/or modify
>
> @@ -60,6 +60,7 @@
>  #define S_CMD_SYNCNOP		0x10	/* Special no-operation that returns NAK+ACK    */
>  #define S_CMD_Q_RDNMAXLEN	0x11	/* Query read-n maximum length			*/
>  #define S_CMD_S_BUSTYPE		0x12	/* Set used bustype(s).				*/
> +#define S_CMD_O_SPIOP		0x13	/* Perform SPI operation.			*/
>
> static uint16_t sp_device_serbuf_size = 16;
>  static uint16_t sp_device_opbuf_size = 300;
> @@ -295,6 +296,19 @@
>    return 0;
>  }
>
> +static const struct spi_programmer spi_programmer_serprog = {
> .type = SPI_CONTROLLER_SERPROG,
> .max_data_read = MAX_DATA_READ_UNLIMITED,
> .max_data_write = MAX_DATA_WRITE_UNLIMITED,
> .command = serprog_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = default_spi_read,
> .write_256 = default_spi_write_256,
>
> +};
>
> +/* TODO: Support SPI max read & write data length reporting by the programmer,
> currently we cannot do that because we dont have access to curent flashchip data. */

those fields are not to indicate limitations of the flash chip but
limits of the programmers themselves. since we just relay any spi
streams sent, we probably dont need a buffer on the microcontroller or
whatever is behind. i would just drop the comment.
> int serprog_init(void)
>  {
>    uint16_t iface;
> @@ -318,7 +332,7 @@
>    		msg_perr("Error: No baudrate specified.\n"
>    			 "Use flashrom -p serprog:dev=/dev/device:baud\n");
>    		free(device);
> 	return 1;		
> 	return 1;
> }
> if (strlen(device)) {
> 	sp_fd = sp_openserport(device, atoi(baudport));
>
> @@ -351,7 +365,7 @@
>    		msg_perr("Error: No port specified.\n"
>    			 "Use flashrom -p serprog:ip=ipaddr:port\n");
>    		free(device);
> 	return 1;		
> 	return 1;
> }
> if (strlen(device)) {
> 	sp_fd = sp_opensocket(device, atoi(baudport));
>
> @@ -400,37 +414,58 @@
>
> sp_check_avail_automatic = 1;
> if (sp_docommand(S_CMD_Q_BUSTYPE, 0, NULL, 1, &c)) {
> msg_perr("Warning: NAK to query supported buses\n");
> c = CHIP_BUSTYPE_NONSPI;	/* A reasonable default for now. */
> }
> buses_supported = c;
> /* Check for the minimum operational set of commands */
> if (sp_check_commandavail(S_CMD_R_BYTE) == 0) {
> msg_perr("Error: Single byte read not supported\n");
> exit(1);
> }
> /* This could be translated to single byte reads (if missing),	*
> * but now we dont support that.				*/
> if (sp_check_commandavail(S_CMD_R_NBYTES) == 0) {
> msg_perr("Error: Read n bytes not supported\n");
> exit(1);
> }
> /* In the future one could switch to read-only mode if these	*
> are not available.						*/
> if (sp_check_commandavail(S_CMD_O_INIT) == 0) {
>   msg_perr("Error: Initialize operation buffer not supported\n");
>   exit(1);
>  }
> if (sp_check_commandavail(S_CMD_O_WRITEB) == 0) {
> msg_perr("Error: Write to opbuf: write byte not supported\n");
> exit(1);
> }
>
> if (sp_check_commandavail(S_CMD_O_DELAY) == 0) {
>   msg_perr("Error: Write to opbuf: delay not supported\n");
>   exit(1);
>  }
>
> if (sp_check_commandavail(S_CMD_O_EXEC) == 0) {
>   msg_perr(
>   	"Error: Execute operation buffer not supported\n");
>   exit(1);
>  }
>
> if (buses_supported & CHIP_BUSTYPE_SPI) {
> if (sp_check_commandavail(S_CMD_O_SPIOP) == 0) {
> 	msg_perr("Error: SPI operation not supported while the SPI bustype is\n");
> 	exit(1);
> }
> register_spi_programmer(&spi_programmer_serprog);
>
> }
>
> if (buses_supported & CHIP_BUSTYPE_NONSPI) {
> if (sp_check_commandavail(S_CMD_R_BYTE) == 0) {
> 	msg_perr("Error: Single byte read not supported\n");
> 	exit(1);
> }
> /* This could be translated to single byte reads (if missing),	*
> * but now we dont support that.				*/
> if (sp_check_commandavail(S_CMD_R_NBYTES) == 0) {
> 	msg_perr("Error: Read n bytes not supported\n");
> 	exit(1);
> }
> if (sp_check_commandavail(S_CMD_O_WRITEB) == 0) {
> 	msg_perr("Error: Write to opbuf: write byte not supported\n");
> 	exit(1);
> }
>
> }
>
> if (sp_docommand(S_CMD_Q_PGMNAME, 0, NULL, 16, pgmname)) {
>   msg_perr("Warning: NAK to query programmer name\n");
>   strcpy((char *)pgmname, "(unknown)");
>
> @@ -450,12 +485,6 @@
>    msg_pdbg(MSGHEADER "operation buffer size %d\n",
>    	     sp_device_opbuf_size);
> if (sp_docommand(S_CMD_Q_BUSTYPE, 0, NULL, 1, &c)) {
> msg_perr("Warning: NAK to query supported buses\n");
> c = CHIP_BUSTYPE_NONSPI;	/* A reasonable default for now. */
> }
> buses_supported = c;

you make the read and write commands optional in spi mode and keep them
mandatory in non-spi mode, but the opbuf stuff remains mandatory in all
modes. why? imho they should also be moved into the "if
(buses_supported & CHIP_BUSTYPE_NONSPI)" branch.
> if (sp_docommand(S_CMD_O_INIT, 0, NULL, 0, NULL)) {
>    	msg_perr("Error: NAK to initialize operation buffer\n");
>    	exit(1);
> @@ -468,6 +497,7 @@
>    	sp_max_write_n = ((unsigned int)(rbuf[0]) << 0);
>    	sp_max_write_n |= ((unsigned int)(rbuf[1]) << 8);
>    	sp_max_write_n |= ((unsigned int)(rbuf[2]) << 16);
> if (!sp_max_write_n) sp_max_write_n = (1<<24);

i detest single line ifs and fors, but besides that this fixes an
unrelated bug (not sure if that's good or bad)
> msg_pdbg(MSGHEADER "Maximum write-n length %d\n",
> 	     sp_max_write_n);
> sp_write_n_buf = malloc(sp_max_write_n);
>
> @@ -477,7 +507,7 @@
>    	}
>    	sp_write_n_bytes = 0;
>    }
> if ((sp_check_commandavail(S_CMD_Q_RDNMAXLEN))
>   &&((sp_docommand(S_CMD_Q_RDNMAXLEN,0,NULL, 3, rbuf) == 0))) {
>   sp_max_read_n = ((unsigned int)(rbuf[0]) << 0);
>
> @@ -680,3 +710,25 @@
>    sp_opbuf_usage += 5;
>    sp_prev_was_write = 0;
>  }
>
> +int serprog_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>                    const unsigned char *writearr, unsigned char *readarr)
>
> +{
>    unsigned char *parmbuf;

spaces instead of tabs
> int ret;
>    msg_pspew("%s, writecnt=%i, readcnt=%i\n", __func__, writecnt, readcnt);

spaces instead of tabs
> if ((sp_opbuf_usage) || (sp_max_write_n && sp_write_n_bytes))
> sp_execute_opbuf();

what's the purpose of this?
> parmbuf = malloc(writecnt+6);
> if (!parmbuf) sp_die("Error: cannot malloc SPI send param buffer");

i detest single line ifs
> parmbuf[0] = (writecnt >> 0) & 0xFF;
> parmbuf[1] = (writecnt >> 8) & 0xFF;
> parmbuf[2] = (writecnt >> 16) & 0xFF;
> parmbuf[3] = (readcnt >> 0) & 0xFF;
> parmbuf[4] = (readcnt >> 8) & 0xFF;
> parmbuf[5] = (readcnt >> 16) & 0xFF;
> memcpy(&(parmbuf[6]),writearr,writecnt);

missing spaces after ','s (and i like "parmbuf+6" more).
> ret = sp_docommand(S_CMD_O_SPIOP, writecnt+6, parmbuf, readcnt, readarr);
> free(parmbuf);
> return ret;
>
> +}
> Index: programmer.h
> ===================================================================
> --- programmer.h	(revision 1299)
> +++ programmer.h	(working copy)
> @@ -560,6 +560,9 @@
>  #if CONFIG_OGP_SPI == 1 || CONFIG_NICINTEL_SPI == 1 || CONFIG_RAYER_SPI == 1 || (CONFIG_INTERNAL == 1 && (defined(__i386__) || defined(__x86_64__)))
>    SPI_CONTROLLER_BITBANG,
>  #endif
> +#if CONFIG_SERPROG == 1
> SPI_CONTROLLER_SERPROG,
>
> +#endif
>  };
>  extern const int spi_programmer_count;
>
> @@ -622,6 +625,8 @@
>  uint8_t serprog_chip_readb(const chipaddr addr);
>  void serprog_chip_readn(uint8_t *buf, const chipaddr addr, size_t len);
>  void serprog_delay(int delay);
> +int serprog_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> 	const unsigned char *writearr, unsigned char *readarr);
>
> /* serial.c */
>  #if _WIN32