[flashrom] [PATCH] serprog SPI support
Urja Rannikko
urjaman at gmail.com
Mon May 16 15:30:59 CEST 2011
>> - * Copyright (C) 2009 Urja Rannikko <urjaman at gmail.com>
>> + * Copyright (C) 2009,2011 Urja Rannikko <urjaman at gmail.com>
> 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.... :/ )
--
Urja Rannikko
More information about the flashrom
mailing list