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 <[email protected]> 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 <[email protected]> > + * Copyright (C) 2009,2011 Urja Rannikko <[email protected]> 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 -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
