[PATCH] serprog SPI support

[Urja Rannikko]

Add SPI support to serprog protocol.

Signed-off-by: Urja Rannikko

[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

[Urja Rannikko]

> > Copyright (C) 2009 Urja Rannikko
> > Copyright (C) 2009,2011 Urja Rannikko
>
> space after ',' imho

ok
> > +/* 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.

Meh, I'll change this stuff (just got a better idea for this).
> > 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.

Opbuf is always used to implement the delay command, thus it is always needed.
> > 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)

This has propably creeped in accidentally, I can split it elsewhere,
but is that needed?
> > +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

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

ok
> > if ((sp_opbuf_usage) || (sp_max_write_n && sp_write_n_bytes))
> > sp_execute_opbuf();
>
> what's the purpose of this?

It will execute anything left behind in the opbuf (just delays if we
are in SPI mode) before doing the SPI operation.
> > memcpy(&(parmbuf[6]),writearr,writecnt);
>
> missing spaces after ','s (and i like "parmbuf+6" more).

ok ( I tend to have a very tight coding style and can't notice these
things at all.... :/ )

[Stefan Tauner]

hey urja!

coding style: the tabs are certainly to be used according to the
official coreboot rules, but the rest is just my personal view. i am
not sure how the other devs think about it.

opbuf/delay: what's the use of this in spi mode? i dont see why this is
needed at all.

patch split: dunno. it might be overlooked if the rest is not
committed. therefore i explicitly mentioned this in case someone with
commit rights reads the review ;)

[Urja Rannikko]

Hi,
> opbuf/delay: what's the use of this in spi mode? i dont see why this is
> needed at all.

Well, programmer_delay can be used anywhere in flashrom, and I dont
see a need to limit that.
I could make it so that these commands are optional if the programmer
only supports SPI, and if they are not supported serprog_delay would
call internal_delay ?

[Stefan Tauner]

On Mon, 16 May 2011 16:52:57 +0300
Urja Rannikko  wrote:
> > opbuf/delay: what's the use of this in spi mode? i dont see why this is
> > needed at all.
>
> Well, programmer_delay can be used anywhere in flashrom, and I dont
> see a need to limit that.
> I could make it so that these commands are optional if the programmer
> only supports SPI, and if they are not supported serprog_delay would
> call internal_delay ?

if the opbuf stuff is mandatory the firmware of the external programmers
would need to implement it although it is never used for spi
programming. i would check for S_CMD_O_DELAY support at the start of
serprog_delay. what to do if there is no support?
calling internal_delay is probably the best option, maybe also issuing
a warning or at least a debug message? just issuing a warning in
returning would be ok with me too. it should not happen in the current
code anyway. *shrug*

the other opbuf stuff should also be optional imho, but if this is done
we probably would like to change the functions using it because they
often bail out via exit if they read a NAK e.g. sp_flush_stream

maybe we can get a third opinion from someone else. i'll ask carldani
when i see him.

[Urja Rannikko]

Add SPI support to serprog protocol.

Signed-off-by: Urja Rannikko 
---
Changed stuff mostly according to review ... and an e-mail that
arrived while I was writing this.
Added the optimized SPI read function for serprog that I had already
done, -r time for 1Mbyte 18s -> 11s.
> the other opbuf stuff should also be optional imho, but if this is done
> we probably would like to change the functions using it because they
> often bail out via exit if they read a NAK e.g. sp_flush_stream

Now it is mandatory for non-SPI, but optional for SPi.
If the programmer is SPI-only we should not get any other than delay
calls that would generate opbuf-using commands,
will test for this later and report.

[Stefan Tauner]

On Mon, 16 May 2011 18:16:03 +0300
Urja Rannikko  wrote:
> Add SPI support to serprog protocol.
> Signed-off-by: Urja Rannikko
>
> Changed stuff mostly according to review ... and an e-mail that
> arrived while I was writing this.
> Added the optimized SPI read function for serprog that I had already
> done, -r time for 1Mbyte 18s -> 11s.
> > the other opbuf stuff should also be optional imho, but if this is done
> > we probably would like to change the functions using it because they
> > often bail out via exit if they read a NAK e.g. sp_flush_stream
>
> Now it is mandatory for non-SPI, but optional for SPi.
> If the programmer is SPI-only we should not get any other than delay
> calls that would generate opbuf-using commands,
> will test for this later and report.

hello urja,

i will port my own avr-based programmer to your serprog implementation
and review it on the way so that we can commit it soon. do you have
other pending, local changes for this that i should know about? have you
tested your stuff further?
thanks

[PATCH 0/3] serprog spi support + general cleanup

[Stefan Tauner]

i have rebased urja's patch to current HEAD and instead of sending a lengthy
"format this, format that" email i fixed the stuff that i thought needs fixing
myself. to ease the re-review for urja i am sending my fixup patch separately,
everyone else should probably combine 1/3 and 2/3.

urja was right about the delay function. i have also measured a similar
speedup with his read function. we should fix spi_read_chunked. everything
else slightly intersting is in the commit logs.

Stefan Tauner (3):
  Add SPI support to serprog protocol.
  fixup! Add SPI support to serprog protocol.
  serprog improvements

programmer.h         |    9 ++-
 serprog-protocol.txt |   12 ++-
 serprog.c            |  310 ++++++++++++++++++++++++++++++++++----------------
 3 files changed, 230 insertions(+), 101 deletions(-)