Re: [flashrom] [PATCH] Add Altera USB Blaster SPI programmer

[Stefan Tauner]

On Tue, 17 Jul 2012 15:39:31 +1000
James Laird  wrote:
> Adds support for the Altera USB Blaster programming dongle in SPI mode. It has only been tested against a cheap clone ($10, unclear origin), and so is marked as not tested.
> It should also support several variants which are present on Altera dev boards (PID 6002,6003...), although I believe these are hardwired to their targets.

iirc you said that those simulate spi chips. in that case it would make
sense to add them imho, even if there are probably not many use
cases... it is a trivial, inexpensive change that pays out even if a
single user needs this once imho :)
> Signed-off-by: James Laird 
> Index: Makefile
> ===================================================================
> --- Makefile	(revision 1548)
> +++ Makefile	(working copy)
> @@ -84,7 +84,7 @@
>  else
>  override CONFIG_SERPROG = no
>  endif
> -# Dediprog and FT2232 are not supported under DOS (missing USB support).
> +# Dediprog, FT2232 and USB Blaster are not supported under DOS (missing USB support).

the trademark is "USB-Blaster", please use that instead everywhere 
(including in the msg_perr invocations)
> ifeq ($(CONFIG_DEDIPROG), yes)
>  UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes
>  else
> @@ -95,7 +95,12 @@
>  else
>  override CONFIG_FT2232_SPI = no
>  endif
> +ifeq ($(CONFIG_USBBLASTER_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_USBBLASTER_SPI=yes
> +else
> +override CONFIG_USBBLASTER_SPI = no
>  endif
> +endif

the macro name is ok of course
> # FIXME: Should we check for Cygwin/MSVC as well?
>  ifeq ($(TARGET_OS), MinGW)
> @@ -196,7 +201,7 @@
>  else
>  override CONFIG_SERPROG = no
>  endif
> -# Dediprog and FT2232 are not supported with libpayload (missing libusb support)
> +# Dediprog, FT2232 and USB Blaster are not supported with libpayload (missing libusb support)
>  ifeq ($(CONFIG_DEDIPROG), yes)
>  UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes
>  else
> @@ -207,7 +212,12 @@
>  else
>  override CONFIG_FT2232_SPI = no
>  endif
> +ifeq ($(CONFIG_USBBLASTER_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_USBBLASTER_SPI=yes
> +else
> +override CONFIG_USBBLASTER_SPI = no
>  endif
> +endif
>
> ifneq ($(TARGET_OS), Linux)
>  ifeq ($(CONFIG_LINUX_SPI), yes)
> @@ -344,6 +354,9 @@
>  # Enable Linux spidev interface by default. We disable it on non-Linux targets.
>  CONFIG_LINUX_SPI ?= yes
>
> +# Always enable USB Blaster SPI for now.

we should get rid of this stupid duplicated all over again line (not
your fault of course :)
> +CONFIG_USBBLASTER_SPI ?= yes
>
> # Disable wiki printing by default. It is only useful if you have wiki access.
>  CONFIG_PRINT_WIKI ?= no
>
> @@ -430,14 +443,31 @@
>  NEED_PCI := yes
>  endif
>
> +# This is a totally ugly hack.
> +WANT_LIBFTDI := no
> +HAVE_LIBFTDI := $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && echo "yes")
>  ifeq ($(CONFIG_FT2232_SPI), yes)
> -FTDILIBS := $(shell pkg-config --libs libftdi 2>/dev/null || printf "%s" "-lftdi -lusb")
> -# This is a totally ugly hack.
> -FEATURE_CFLAGS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "-D'CONFIG_FT2232_SPI=1'")
> -FEATURE_LIBS += $(shell LC_ALL=C grep -q "FTDISUPPORT := yes" .features && printf "%s" "$(FTDILIBS)")
> -PROGRAMMER_OBJS += ft2232_spi.o
> WANT_LIBFTDI := yes
> ifeq ($(HAVE_LIBFTDI), yes)
> FEATURE_CFLAGS += -DCONFIG_FT2232_SPI=1
> PROGRAMMER_OBJS += ft2232_spi.o
> endif
>
> endif
> +ifeq ($(CONFIG_USBBLASTER_SPI), yes)
> WANT_LIBFTDI := yes
> ifeq ($(HAVE_LIBFTDI), yes)
> FEATURE_CFLAGS += -DCONFIG_USBBLASTER_SPI=1
> PROGRAMMER_OBJS += usbblaster_spi.o
> endif
>
> +endif
>
> +ifeq ($(WANT_LIBFTDI), yes)
> ifeq ($(WANT_LIBFTDI), yes)
> FTDILIBS := $(shell pkg-config --libs libftdi 2>/dev/null || printf "%s" "-lftdi -lusb")
> FEATURE_LIBS += $(FTDILIBS)
> endif
>
> +endif

i have not really looked at this hunk yet, but in general it looks ok,
the double "ifeq ($(WANT_LIBFTDI), yes)" is wtf though :)
> ifeq ($(CONFIG_DUMMY), yes)
>  FEATURE_CFLAGS += -D'CONFIG_DUMMY=1'
>  PROGRAMMER_OBJS += dummyflasher.o
> @@ -700,7 +730,7 @@
>
> features: compiler
>    @echo "FEATURES := yes" > .features.tmp
> -ifeq ($(CONFIG_FT2232_SPI), yes)
> +ifeq ($(WANT_LIBFTDI), yes)
>    @printf "Checking for FTDI support... "
>    @echo "$$FTDI_TEST" > .featuretest.c
>    @$(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest$(EXEC_SUFFIX) $(FTDILIBS) $(LIBS) >/dev/null 2>&1 &&	\
> Index: README
> ===================================================================
> --- README	(revision 1548)
> +++ README	(working copy)
> @@ -45,8 +45,8 @@
>  To build flashrom you need to install the following software:
> pciutils+libpci (if you want support for mainboard or PCI device flashing)
> libusb (if you want FT2232 or Dediprog support)
> libftdi (if you want FT2232 support)
> libusb (if you want FT2232, Dediprog or USB Blaster support)
> libftdi (if you want FT2232 or USB Blaster support)
>
> Linux et al:
> Index: usbblaster_spi.c
>
> --- usbblaster_spi.c	(revision 0)
> +++ usbblaster_spi.c	(working copy)
> @@ -0,0 +1,258 @@
> +/*
> This file is part of the flashrom project.
> Copyright (C) 2012 James Laird
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; version 2 of the License.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/*
> Device should be connected as per "active serial" mode:
>  +---------+------+-----------+
>  | SPI     | Pin  |  Altera   |
>  +---------+------+-----------+
>  | SCLK    | 1    | DCLK      |
>  | GND     | 2,10 | GND       |
>  | VCC     | 4    | VCC(TRGT) |
>  | MISO    | 7    | ASDO      |
>  | /CS     | 8    | nCS       |
>  | MOSI    | 9    | ASDI      |

hm... called "DATAOUT" in Table 2–2 in the "USB-Blaster Download Cable
User Guide".
some links to relevant documentation would be handy here too.
>  +---------+------+-----------+
> */
>
> +#if CONFIG_USBBLASTER_SPI == 1
>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <ctype.h>
> +#include "flash.h"
> +#include "programmer.h"
> +#include "spi.h"
> +#include <ftdi.h>
>
> +/* Please keep sorted by vendor ID, then device ID. */
>
> +#define ALTERA_VID		            0x09fb
> +#define ALTERA_USBBLASTER_PID       0x6001
>
> +const struct usbdev_status devs_usbblasterspi[] = {
> {ALTERA_VID, ALTERA_USBBLASTER_PID,     NT, "Altera", "USB Blaster"},

+ \n
> {},
>
> +};
>
> +static const struct spi_programmer spi_programmer_usbblaster;
>
> +static struct ftdi_context ftdic;
>
> +// command bytes
> +#define BIT_BYTE    (1<<7)  // byte mode (rather than bitbang)
> +#define BIT_READ    (1<<6)  // read request

in general we (seem to) try to avoid '//' although i am not really in
favor of this.
> +#define BIT_LED     (1<<5)
> +#define BIT_CS      (1<<3)
> +#define BIT_CLK     (1<<0)
> +#define BIT_TMS     (1<<1)
>
> +#define BITS_OTHER  0

what's that?
> +// The programmer shifts bits in the wrong order for SPI.
> +static unsigned char reverse[] = {
> 0x00, 0x80, 0x40, 0xc0, 0x20, 0xa0, 0x60, 0xe0,
> 0x10, 0x90, 0x50, 0xd0, 0x30, 0xb0, 0x70, 0xf0,
> 0x08, 0x88, 0x48, 0xc8, 0x28, 0xa8, 0x68, 0xe8,
> 0x18, 0x98, 0x58, 0xd8, 0x38, 0xb8, 0x78, 0xf8,
> 0x04, 0x84, 0x44, 0xc4, 0x24, 0xa4, 0x64, 0xe4,
> 0x14, 0x94, 0x54, 0xd4, 0x34, 0xb4, 0x74, 0xf4,
> 0x0c, 0x8c, 0x4c, 0xcc, 0x2c, 0xac, 0x6c, 0xec,
> 0x1c, 0x9c, 0x5c, 0xdc, 0x3c, 0xbc, 0x7c, 0xfc,
> 0x02, 0x82, 0x42, 0xc2, 0x22, 0xa2, 0x62, 0xe2,
> 0x12, 0x92, 0x52, 0xd2, 0x32, 0xb2, 0x72, 0xf2,
> 0x0a, 0x8a, 0x4a, 0xca, 0x2a, 0xaa, 0x6a, 0xea,
> 0x1a, 0x9a, 0x5a, 0xda, 0x3a, 0xba, 0x7a, 0xfa,
> 0x06, 0x86, 0x46, 0xc6, 0x26, 0xa6, 0x66, 0xe6,
> 0x16, 0x96, 0x56, 0xd6, 0x36, 0xb6, 0x76, 0xf6,
> 0x0e, 0x8e, 0x4e, 0xce, 0x2e, 0xae, 0x6e, 0xee,
> 0x1e, 0x9e, 0x5e, 0xde, 0x3e, 0xbe, 0x7e, 0xfe,
> 0x01, 0x81, 0x41, 0xc1, 0x21, 0xa1, 0x61, 0xe1,
> 0x11, 0x91, 0x51, 0xd1, 0x31, 0xb1, 0x71, 0xf1,
> 0x09, 0x89, 0x49, 0xc9, 0x29, 0xa9, 0x69, 0xe9,
> 0x19, 0x99, 0x59, 0xd9, 0x39, 0xb9, 0x79, 0xf9,
> 0x05, 0x85, 0x45, 0xc5, 0x25, 0xa5, 0x65, 0xe5,
> 0x15, 0x95, 0x55, 0xd5, 0x35, 0xb5, 0x75, 0xf5,
> 0x0d, 0x8d, 0x4d, 0xcd, 0x2d, 0xad, 0x6d, 0xed,
> 0x1d, 0x9d, 0x5d, 0xdd, 0x3d, 0xbd, 0x7d, 0xfd,
> 0x03, 0x83, 0x43, 0xc3, 0x23, 0xa3, 0x63, 0xe3,
> 0x13, 0x93, 0x53, 0xd3, 0x33, 0xb3, 0x73, 0xf3,
> 0x0b, 0x8b, 0x4b, 0xcb, 0x2b, 0xab, 0x6b, 0xeb,
> 0x1b, 0x9b, 0x5b, 0xdb, 0x3b, 0xbb, 0x7b, 0xfb,
> 0x07, 0x87, 0x47, 0xc7, 0x27, 0xa7, 0x67, 0xe7,
> 0x17, 0x97, 0x57, 0xd7, 0x37, 0xb7, 0x77, 0xf7,
> 0x0f, 0x8f, 0x4f, 0xcf, 0x2f, 0xaf, 0x6f, 0xef,
> 0x1f, 0x9f, 0x5f, 0xdf, 0x3f, 0xbf, 0x7f, 0xff
>
> +};

this has to die.
my favorite is
http://graphics.stanford.edu/~seander/bithacks.html#ReverseByteWith64Bits
but anything without a lookup table would be better than this.
> +/* Returns 0 upon success, a negative number upon errors. */
> +int usbblaster_spi_init(void)
> +{
> if (ftdi_init(&ftdic) < 0)
>    return -1;
> if (ftdi_usb_open(&ftdic, ALTERA_VID, ALTERA_USBBLASTER_PID) < 0) {
>    msg_perr("Failed to open USB Blaster: %s\n", ftdic.error_str);
>    return -1;
> }
> if (ftdi_usb_reset(&ftdic) < 0) {
>    msg_perr("Blaster reset failed\n");
>    return -1;
> }
> if (ftdi_set_latency_timer(&ftdic, 2) < 0) {
>    msg_perr("Blaster set latency timer failed\n");
>    return -1;
> }
> if (ftdi_write_data_set_chunksize(&ftdic, 4096) < 0 ||

why?
macro?
>    ftdi_read_data_set_chunksize(&ftdic, 64) < 0) {

why?
macro?
>    msg_perr("Blaster set chunk size failed\n");
>    return -1;
> }
> unsigned char buf[65];

why?
macro?
please do not use unsigned chars but uint8_t.
> memset(buf, 0, sizeof(buf));
> buf[sizeof(buf)-1] = BIT_LED | BITS_OTHER | BIT_CS;
> if (ftdi_write_data(&ftdic, buf, sizeof(buf)) < 0) {
>    msg_perr("Blaster reset write failed\n");
>    return -1;
> }
> if (ftdi_read_data(&ftdic, buf, sizeof(buf)) < 0) {
>    msg_perr("Blaster reset read failed\n");
>    return -1;
> }
> register_spi_programmer(&spi_programmer_usbblaster);
> return 0;
>
> +}
>
> +/* Returns 0 upon success, a negative number upon errors. */
> +static int usbblaster_spi_send_command(struct flashctx *flash,
> 		   unsigned int writecnt, unsigned int readcnt,
> 		   const unsigned char *writearr,
> 		   unsigned char *readarr)
>
> +{
> unsigned char cmd;
> unsigned char buf[64];

you know the drill ;)
> unsigned char tms_state = 0;
> int i;
> // assert /CS
> cmd = BIT_LED | BITS_OTHER;
> if (ftdi_write_data(&ftdic, &cmd, 1) < 0) {
>    msg_perr("Blaster write failed\n");
>    return -1;
> }
> while (writecnt) {
>    unsigned int n_write = writecnt;
>    if (n_write > 63)
>        n_write = 63;

looks slightly related to above which would mean there should be a
single constant ;)
>    msg_pspew("writing %d-byte packet\n", n_write);
>    cmd = BIT_BYTE | (unsigned char)n_write;
>    buf[0] = cmd;
>    for (i=0; i<n_write; i++) {
>        buf[i+1] = reverse[writearr[i]];
>    }
>    if (ftdi_write_data(&ftdic, buf, n_write+1) < 0) {
>        msg_perr("Blaster write failed\n");
>        return -1;
>    }
>    cmd = BIT_LED | BITS_OTHER | (tms_state ^= BIT_TMS);
>    if (ftdi_write_data(&ftdic, &cmd, 1) < 0) {
>        msg_perr("Blaster write failed\n");
>        return -1;
>    }
>    writearr += n_write;
>    writecnt -= n_write;
> }
> unsigned int n_readout = readcnt;

if your know the upper limit of variables like this, then please use an
appropriately dimensioned type of the uint*_t family.
> while (readcnt) {
>    unsigned int n_read = readcnt;
>    if (n_read > 63)
>        n_read = 63;
>    cmd = BIT_BYTE | BIT_READ | (unsigned char)n_read;
>    buf[0] = cmd;
>    if (ftdi_write_data(&ftdic, buf, n_read + 1) < 0) {
>        msg_perr("Blaster write failed\n");
>        return -1;
>    }
>    cmd = BIT_LED | BITS_OTHER | (tms_state ^= BIT_TMS);
>    if (ftdi_write_data(&ftdic, &cmd, 1) < 0) {
>        msg_perr("Blaster write failed\n");
>        return -1;
>    }
>    readcnt -= n_read;
>    msg_pspew("reading %d-byte packet\n", n_read);
> };
> unsigned int n_read = n_readout;
> while (n_read) {
>    int ret = ftdi_read_data(&ftdic, readarr, n_read);
>    if (ret < 0) {
>        msg_perr("Blaster read failed\n");
>        return -1;
>    }
>    for (i=0; i<ret; i++) {
>        readarr[i] = reverse[readarr[i]];
>    }
>    n_read -= ret;
>    readarr += ret;
> }
> cmd = BIT_LED | BIT_CS | BITS_OTHER;
> if (ftdi_write_data(&ftdic, &cmd, 1) < 0) {
>    msg_perr("Blaster write failed\n");
>    return -1;
> }
> cmd = BIT_CS | BITS_OTHER;
> if (ftdi_write_data(&ftdic, &cmd, 1) < 0) {
>    msg_perr("Blaster write failed\n");
>    return -1;
> }

thanks a lot for your effort! the only really mandatory change
requirements are the one in the makefile and the reverse array. the
others are partly personal opinions, but at least give them some
thought when refining the patch please.