[flashrom] Add PICkit2 as an SPI programmer

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Mon Mar 25 01:59:09 CET 2013


On Sun, 24 Mar 2013 11:10:19 -0400
Justin Chevrier <jchevrier at gmail.com> wrote:

> From the 'You can never have enough programmers' file, comes support
> for another programmer type.

Supporting the internal one becomes more painful every day, so
expanding the support for external programmers is very welcomed, thanks!

> Specifically it's the PICkit2. This patch was inspired by the code in
> AVRDude (open source Atmel AVR
> programmer) to support the PICkit2 written by Doug Brown [1]. The
> Dediprog code in Flashrom was used
> as the template for this code with some reference to the ft2232 code
> as well. I left Carl-Daniel in
> the copyright assignment as I thought it made the most sense as I
> basically copied and pasted then
> modified the code as needed. Hopefully that was the right thing to do.

Sure, the copyright does not vanish when you copy some code to a new
file, so all previous copyright notices should be copied over too
(if one thinks they are useful at all... :)

> A few notes:
> 
> 1) The code makes no provision for externally powered targets (where
> VDD from the PICkit can be connected to
> the device and an external power source and it will detect external
> power and not apply its own).
> I'm assuming that code needs to be written for this to work and that
> the PICkit itself doesn't make it happen
> automagically. Not sure on that one

I have *no* idea about that programmer or the protocol... but maybe
that's what the equivalent of voltage_selector = 0 is for?
> 
> 2) I've tested with PICkit2 firmware version 2.32. I suspect that
> there are earlier versions of the
> firmware that are not capable of talking SPI, but I'm not sure. Should
> we include a warning if we detect
> that a user is running on older firmware and ask users to submit if
> their firmware version works
> for them?

Maybe... other programmers use their devices struct for such things. I
don't think dediprog is a good example in this regard.

> 3) I've only tested with the one easily accessible SPI chip I have on
> hand (GigaDevice GD25Q80)
> 
> Connections are as follows:
> 
> PICkit2 Pin 1 - VPP/MCLR -> CS
> PICkit2 Pin 2 - VDD -> Vcc
> PICkit2 Pin 3 - GND -> Vss
> PICkit2 Pin 4 - PGD -> SO
> PICkit2 Pin 5 - PDC -> SCLK
> PICkit2 Pin 6 - AUX -> SI

Creating a wiki page would be welcomed. If you want an account to do
that, just ask.

> Feedback welcomed! Hopefully this is useful to someone.
> 
> Thanks for the great tools as always guys.

Thanks for your patch!
The following is not a full review, but I mention a few things I
spotted while skimming through. Feel free to ignore them (for now :) if
you don't agree.

> Index: Makefile
> ===================================================================
> --- Makefile	(revision 1657)
> +++ Makefile	(working copy)
> @@ -117,7 +117,7 @@
>  else
>  override CONFIG_PONY_SPI = no
>  endif
> -# Dediprog and FT2232 are not supported under DOS (missing USB support).
> +# Dediprog, FT2232 and PICkit2 are not supported under DOS (missing USB support).
>  ifeq ($(CONFIG_DEDIPROG), yes)
>  UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes
>  else
> @@ -128,7 +128,12 @@
>  else
>  override CONFIG_FT2232_SPI = no
>  endif
> +ifeq ($(CONFIG_PICKIT2_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_PICKIT2_SPI=yes
> +else
> +override CONFIG_PICKIT2_SPI = no
>  endif
> +endif
>  
>  # FIXME: Should we check for Cygwin/MSVC as well?
>  ifeq ($(TARGET_OS), MinGW)
> @@ -229,7 +234,7 @@
>  else
>  override CONFIG_SERPROG = no
>  endif
> -# Dediprog and FT2232 are not supported with libpayload (missing libusb support)
> +# Dediprog, FT2232 and PICkit2 are not supported with libpayload (missing libusb support)
>  ifeq ($(CONFIG_DEDIPROG), yes)
>  UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes
>  else
> @@ -240,7 +245,12 @@
>  else
>  override CONFIG_FT2232_SPI = no
>  endif
> +ifeq ($(CONFIG_PICKIT2_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_PICKIT2_SPI=yes
> +else
> +override CONFIG_PICKIT2_SPI = no
>  endif
> +endif
>  
>  ifneq ($(TARGET_OS), Linux)
>  ifeq ($(CONFIG_LINUX_SPI), yes)
> @@ -382,6 +392,9 @@
>  # Enable Linux spidev interface by default. We disable it on non-Linux targets.
>  CONFIG_LINUX_SPI ?= yes
>  
> +# Always enable PICkit2 SPI dongles for now.
> +CONFIG_PICKIT2_SPI ?= yes
> +
>  # Disable wiki printing by default. It is only useful if you have wiki access.
>  CONFIG_PRINT_WIKI ?= no
>  
> @@ -549,6 +562,12 @@
>  PROGRAMMER_OBJS += linux_spi.o
>  endif
>  
> +ifeq ($(CONFIG_PICKIT2_SPI), yes)
> +FEATURE_CFLAGS += -D'CONFIG_PICKIT2_SPI=1'
> +PROGRAMMER_OBJS += pickit2_spi.o
> +NEED_USB := yes
> +endif
> +
>  ifeq ($(NEED_SERIAL), yes)
>  LIB_OBJS += serial.o
>  endif
> Index: flashrom.8
> ===================================================================
> --- flashrom.8	(revision 1657)
> +++ flashrom.8	(working copy)
> @@ -220,6 +220,8 @@
>  .sp
>  .BR "* linux_spi" " (for SPI flash ROMs accessible via /dev/spidevX.Y on Linux)"
>  .sp
> +.BR "* pickit2_spi" " (for SPI flash ROMs accessible via Microchip PICkit2 on Linux)"

Linux only? I guess that's a copy & paste error and it will work whereever flashrom and libusb work.

> +.sp
>  Some programmers have optional or mandatory parameters which are described
>  in detail in the
>  .B PROGRAMMER SPECIFIC INFO

TODO: explanation of programmer parameters in an detailed section.

> Index: flashrom.c
> ===================================================================
> --- flashrom.c	(revision 1657)
> +++ flashrom.c	(working copy)
> @@ -308,6 +308,19 @@
>  	},
>  #endif
>  
> +#if CONFIG_PICKIT2_SPI == 1
> +	{
> +		.name			= "pickit2_spi",
> +		.type			= OTHER,
> +					/* FIXME */
> +		.devs.note		= "Microchip PICkit2\n",
> +		.init			= pickit2_spi_init,
> +		.map_flash_region	= fallback_map,
> +		.unmap_flash_region	= fallback_unmap,
> +		.delay			= internal_delay,
> +	},
> +#endif
> +
>  	{0}, /* This entry corresponds to PROGRAMMER_INVALID. */
>  };
>  
> Index: pickit2_spi.c
> ===================================================================
> --- pickit2_spi.c	(revision 0)
> +++ pickit2_spi.c	(working copy)
> @@ -0,0 +1,519 @@
> +/*
> + * This file is part of the flashrom project.
> + *
> + * Copyright (C) 2010 Carl-Daniel Hailfinger
> + * Copyright (C) 2013 Justin Chevrier
> + *
> + * 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
> + */
> +

Links to or citations of relevant documentation should go here (or the
to be written manpage section, if useful to users too), at least a
reference to avrdude should be given IMHO, but feel free to be as
verbose as the commit log.

> +#include <stdio.h>
> +#include <string.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <usb.h>
> +#include "flash.h"
> +#include "chipdrivers.h"
> +#include "programmer.h"
> +#include "spi.h"
> +
> +static usb_dev_handle *pickit2_handle;
> +
> +#define DEFAULT_TIMEOUT		10000

which unit? when is it used? etc.

> +
> +#define CMD_LENGTH			64
> +#define ENDPOINT_OUT			0x01
> +#define ENDPOINT_IN			0x81
> +
> +#define PICKIT2_VID			0x04D8
> +#define PICKIT2_PID			0x0033
> +
> +#define CMD_GET_VERSION			0x76
> +#define CMD_SET_VDD			0xA0
> +#define CMD_SET_VPP			0xA1
> +#define CMD_READ_VDD_VPP		0xA3
> +#define CMD_EXEC_SCRIPT			0xA6
> +#define CMD_CLR_DLOAD_BUFF		0xA7
> +#define CMD_DOWNLOAD_DATA		0xA8
> +#define CMD_CLR_ULOAD_BUFF		0xA9
> +#define CMD_UPLOAD_DATA			0xAA
> +#define CMD_END_OF_BUFFER		0xAD
> +
> +#define SCR_SPI_READ_BUF		0xC5
> +#define SCR_SPI_WRITE_BUF		0xC6
> +#define SCR_SET_AUX			0xCF
> +#define SCR_LOOP			0xE9
> +#define SCR_SET_ICSP_CLK_PERIOD		0xEA
> +#define SCR_SET_PINS			0xF3
> +#define SCR_BUSY_LED_OFF		0xF4
> +#define SCR_BUSY_LED_ON			0xF5
> +#define SCR_MCLR_GND_OFF		0xF6
> +#define SCR_MCLR_GND_ON			0xF7
> +#define SCR_VPP_PWM_OFF			0xF8
> +#define SCR_VPP_PWM_ON			0xF9
> +#define SCR_VPP_OFF			0xFA
> +#define SCR_VPP_ON			0xFB
> +#define SCR_VDD_OFF			0xFE
> +#define SCR_VDD_ON			0xFF
> +
> +/* Might be useful for other USB devices as well. static for now. */

We need to sort stuff like this out, but that's not your problem.

> +/* device parameter allows user to specify one device of multiple installed */
> +static struct usb_device *get_device_by_vid_pid(uint16_t vid, uint16_t pid, unsigned int device)
> +{
> +	struct usb_bus *bus;
> +	struct usb_device *dev;
> +
> +	for (bus = usb_get_busses(); bus; bus = bus->next)
> +		for (dev = bus->devices; dev; dev = dev->next)
> +			if ((dev->descriptor.idVendor == vid) &&
> +			    (dev->descriptor.idProduct == pid)) {
> +				if (device == 0)
> +					return dev;
> +				device--;
> +			}
> +
> +	return NULL;
> +}
> +
> +static int pickit2_get_firmware_version(void)
> +{
> +	int ret;
> +	unsigned char command[CMD_LENGTH] = {CMD_GET_VERSION, CMD_END_OF_BUFFER};
> +
> +	ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> +	ret = usb_interrupt_read(pickit2_handle, ENDPOINT_IN, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> +	
> +	msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]);
> +	if (ret != CMD_LENGTH) {
> +		msg_perr("Command Get Firmware Version failed (%s)!\n",
> +			 usb_strerror());
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +

> +static int pickit2_set_spi_voltage(int millivolt)
> +{
> +	int ret;
> +	uint16_t voltage_selector;
> […]
> +		voltage_selector = 1.8;

that would be in the best case some "creative" usage of floating point constants. ;)

> […]
> +
> +	unsigned char command[CMD_LENGTH] = {

I personally favor uint8_t instead of unsigned chars...

> +		CMD_SET_VDD,
> +		(uint8_t)(voltage_selector * 2048+672),

... especially if you cast to them anyway :)

> +		(uint8_t)((voltage_selector * 2048+672)/256),

We normally add spaces around all binary ops.

> +		(uint8_t)(voltage_selector * 36),
> +		CMD_SET_VPP,
> +		0x40,
> +		(uint8_t)(voltage_selector * 18.61),

I am not sure I want to understand that protocol better, if this is correct. :)

> +		(uint8_t)(voltage_selector * 13),
> +		CMD_END_OF_BUFFER
> +	};
> +
> +	ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> +	if (ret != CMD_LENGTH) {
> +		msg_perr("Command Set Voltage failed (%s)!\n",
> +			 usb_strerror());
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +
> +struct pickit2_spispeeds {
> +	const char *const name;
> +	const int speed;
> +};
> +
> +static const struct pickit2_spispeeds spispeeds[] = {
> +	{ "1M",		0x1 },
> +	{ "500k",	0x2 },
> +	{ "333k",	0x3 },
> +	{ "250k",	0x4 },
> +	{ NULL,		0x0 },
> +};
> +
> +static int pickit2_set_spi_speed(unsigned int spispeed_idx)
> +{
> +	int ret;
> +
> +	msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed_idx].name);
> +
> +	unsigned char command[CMD_LENGTH] = {
> +		CMD_EXEC_SCRIPT,
> +		2,
> +		SCR_SET_ICSP_CLK_PERIOD,
> +		spispeed_idx,
> +		CMD_END_OF_BUFFER
> +	};
> +
> +	ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> +	if (ret != CMD_LENGTH) {
> +		msg_perr("Command Set SPI Speed failed (%s)!\n",
> +			 usb_strerror());
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pickit2_spi_send_command(struct flashctx *flash,
> +				     unsigned int writecnt,
> +				     unsigned int readcnt,
> +				     const unsigned char *writearr,
> +				     unsigned char *readarr)
> +{
> +	int ret,i;
> +
> +	unsigned char buf[CMD_LENGTH] = {CMD_DOWNLOAD_DATA, writecnt};
> +
> +	/* Maximum number of bytes per transaction (including command overhead) is 64. Lets play it safe
> +	 * and always assume the worst case scenario of 20 bytes command overhead.
> +	 */
> +	msg_pspew("%s, writecnt=%i, readcnt=%i\n", __func__, writecnt, readcnt);
> +	if (writecnt + readcnt + 20 > CMD_LENGTH) {
> +		msg_perr("\nTotal packetsize (%i) is greater than 64 supported, aborting.\n", writecnt + readcnt + 20);
> +		return 1;
> +	}
> +
> +	for (i = 2; i < writecnt + 2; i++)
> +	{

The curly brace should be at the end of the "for line" according to our style.

> +		buf[i] = writearr[i - 2];
> +	}
> +
> +	buf[i++] = CMD_CLR_ULOAD_BUFF;
> +	buf[i++] = CMD_EXEC_SCRIPT;
> +
> +	/* Determine script length based on number of bytes to be read or written */
> +	if (writecnt == 1 && readcnt == 1)
> +		buf[i++] = 7;
> +	else if (writecnt == 1 || readcnt == 1)
> +		buf[i++] = 10;
> +	else
> +		buf[i++] = 13;
> +		
> +	/* Assert CS# */
> +	buf[i++] = SCR_VPP_OFF;
> +	buf[i++] = SCR_MCLR_GND_ON;
> +
> +	buf[i++] = SCR_SPI_WRITE_BUF;
> +
> +	if (writecnt > 1) {
> +		buf[i++] = SCR_LOOP;
> +		buf[i++] = 1; /* Loop back one instruction */
> +		buf[i++] = writecnt - 1; /* Number of times to loop */
> +	}
> +
> +	if (readcnt)
> +		buf[i++] = SCR_SPI_READ_BUF;
> +
> +	if (readcnt > 1) {
> +		buf[i++] = SCR_LOOP;
> +		buf[i++] = 1; /* Loop back one instruction */
> +		buf[i++] = readcnt - 1; /* Number of times to loop */
> +	}
> +
> +	/* De-assert CS# */
> +	buf[i++] = SCR_MCLR_GND_OFF;
> +	buf[i++] = SCR_VPP_PWM_ON;
> +	buf[i++] = SCR_VPP_ON;
> +
> +	buf[i++] = CMD_UPLOAD_DATA;
> +	buf[i++] = CMD_END_OF_BUFFER;
> +
> +	ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)buf, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> +	if (ret != CMD_LENGTH) {
> +		msg_perr("Send SPI failed, expected %i, got %i %s!\n",
> +			writecnt, ret, usb_strerror());
> +			return 1;
> +	}
> +
> +	if (readcnt) {
> +		memset(readarr, 0, readcnt);
> +		memset(buf, 0, sizeof(buf));

Is this really necessary?

> +
> +		ret = usb_interrupt_read(pickit2_handle, ENDPOINT_IN, (char *)buf, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> +		if (ret != CMD_LENGTH) {
> +			msg_perr("Receive SPI failed, expected %i, got %i %s!\n",
> +				readcnt, ret, usb_strerror());
> +			return 1;
> +		}
> +
> +		/* First byte indicates number of bytes transferred from upload buffer */
> +		if (buf[0] != readcnt) {
> +			msg_perr("Unexpected number of bytes transferred, expected %i, got %i!\n",
> +				 readcnt, ret);
> +			return 1;
> +		}
> +		
> +		/* Actual data starts at byte number two */
> +		memcpy(readarr, &buf[1], readcnt);
> +	}
> +
> +	return 0;
> +}
> +

This is a candidate for a ultity "class" too, please add the "might be useful" comment here too:
/* Might be useful for other USB devices as well. static for now. */

And maybe add a reference to dediprog (and likewise for other copied
functions) so that we know immediately that they have been copied.

> +static int parse_voltage(char *voltage)
> +{
> +	char *tmp = NULL;
> +	int i;
> +	int millivolt = 0, fraction = 0;
> +
> +	if (!voltage || !strlen(voltage)) {
> +		msg_perr("Empty voltage= specified.\n");
> +		return -1;
> +	}
> +	millivolt = (int)strtol(voltage, &tmp, 0);
> +	voltage = tmp;
> +	/* Handle "," and "." as decimal point. Everything after it is assumed
> +	 * to be in decimal notation.
> +	 */
> +	if ((*voltage == '.') || (*voltage == ',')) {
> +		voltage++;
> +		for (i = 0; i < 3; i++) {
> +			fraction *= 10;
> +			/* Don't advance if the current character is invalid,
> +			 * but continue multiplying.
> +			 */
> +			if ((*voltage < '0') || (*voltage > '9'))
> +				continue;
> +			fraction += *voltage - '0';
> +			voltage++;
> +		}
> +		/* Throw away remaining digits. */
> +		voltage += strspn(voltage, "0123456789");
> +	}
> +	/* The remaining string must be empty or "mV" or "V". */
> +	tolower_string(voltage);
> +
> +	/* No unit or "V". */
> +	if ((*voltage == '\0') || !strncmp(voltage, "v", 1)) {
> +		millivolt *= 1000;
> +		millivolt += fraction;
> +	} else if (!strncmp(voltage, "mv", 2) ||
> +		   !strncmp(voltage, "milliv", 6)) {
> +		/* No adjustment. fraction is discarded. */
> +	} else {
> +		/* Garbage at the end of the string. */
> +		msg_perr("Garbage voltage= specified.\n");
> +		return -1;
> +	}
> +	return millivolt;
> +}
> +

I think this function can be inlined without disadvantages. The comment
is clear enough.

OTOH it might be worth it if you can generify a pickit2_teardown or
similar (the code in the shutdown function).

> +static int pickit2_setup(void)
> +{
> +	int ret;
> +	/* Configure pins for correct directions and logic levels,
> +	 * turn Vdd on, turn busy LED on and clear buffers
> +	 */
> +	unsigned char buf[CMD_LENGTH] = {
> +		CMD_EXEC_SCRIPT,
> +		10,			/* Script length */
> +		SCR_SET_PINS,
> +		/* Bit-0=0(PDC Out), Bit-1=1(PGD In), Bit-2=0(PDC LL), Bit-3=0(PGD LL) */
> +		2,
> +		SCR_SET_AUX,
> +		/* Bit-0=0(Aux Out), Bit-1=0(Aux LL) */
> +		0,
> +		SCR_VDD_ON,
> +		SCR_MCLR_GND_OFF,	/* Let CS# float */
> +		SCR_VPP_PWM_ON,
> +		SCR_VPP_ON,		/* Pull CS# high */
> +		SCR_BUSY_LED_ON,
> +		CMD_CLR_DLOAD_BUFF,
> +		CMD_CLR_ULOAD_BUFF,
> +		CMD_END_OF_BUFFER
> +	};
> +
> +	ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)buf, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> +	if (ret != CMD_LENGTH) {
> +		msg_perr("Command Setup failed (%s)!\n",
> +			  usb_strerror());
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +[…]
> +
> +static int pickit2_shutdown(void *data)
> +{
> +	int ret;
> +	msg_pspew("%s\n", __func__);
> +
> +	/* Set all pins to float and turn voltages off */
> +	unsigned char command[CMD_LENGTH] = {
> +		CMD_EXEC_SCRIPT,
> +		8,
> +		SCR_SET_PINS,
> +		/* Bit-0=1(PDC In), Bit-1=1(PGD In), Bit-2=0(PDC LL), Bit-3=0(PGD LL) */
> +		3,
> +		SCR_SET_AUX,
> +		/* Bit-0=1(Aux In), Bit-1=0(Aux LL) */
> +		1,
> +		SCR_MCLR_GND_OFF,
> +		SCR_VPP_OFF,
> +		SCR_VDD_OFF,
> +		SCR_BUSY_LED_OFF,
> +		CMD_END_OF_BUFFER
> +	};
> +
> +	ret = usb_interrupt_write(pickit2_handle, ENDPOINT_OUT, (char *)command, CMD_LENGTH, DEFAULT_TIMEOUT);
> +
> +	if (ret != CMD_LENGTH) {
> +		msg_perr("Command Shutdown failed (%s)!\n",
> +			 usb_strerror());
> +		return 1;
> +	}
> +
> +	if (usb_release_interface(pickit2_handle, 0)) {
> +		msg_perr("Could not release USB interface!\n");
> +		return 1;
> +	}
> +	if (usb_close(pickit2_handle)) {
> +		msg_perr("Could not close USB device!\n");
> +		return 1;
> +	}
> +	return 0;
> […]
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner




More information about the flashrom mailing list