[flashrom] [PATCH] serprog SPI support

[Urja Rannikko]

>> - * 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