[flashrom] [PATCH] serprog SPI support
Stefan Tauner
stefan.tauner at student.tuwien.ac.at
Sun May 15 21:50:49 CEST 2011
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 <urjaman at gmail.com> 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 <urjaman at gmail.com>
> + * Copyright (C) 2009,2011 Urja Rannikko <urjaman at gmail.com>
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
More information about the flashrom
mailing list