Re: [flashrom] Add PICkit2 as an SPI programmer

[Stefan Tauner]

On Sun, 24 Mar 2013 11:10:19 -0400
Justin Chevrier  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:
> 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?
> 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.
> 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;
>
> […]

Add PICkit2 as an SPI programmer

[Stefan Tauner]

On Mon, 25 Mar 2013 01:59:09 +0100
Stefan Tauner  wrote:
> On Sun, 24 Mar 2013 11:10:19 -0400
> Justin Chevrier  wrote:
> > 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.

Hello Justin,

I was wondering if we should still expect a reworked version of your
patch... I guess motivation has faded? :)

Add PICkit2 as an SPI programmer

[Justin Chevrier]

On Thu, May 1, 2014 at 1:03 PM, Stefan Tauner
 wrote:
> On Mon, 25 Mar 2013 01:59:09 +0100
> Stefan Tauner  wrote:
> > On Sun, 24 Mar 2013 11:10:19 -0400
> > Justin Chevrier  wrote:
> > > 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.
>
> Hello Justin,
>
> I was wondering if we should still expect a reworked version of your
> patch... I guess motivation has faded? :)

Hi Stefan,

Very sorry for not replying to your email (thanks for the review by the way).

I do have a newer patch that fixes most if not all of the points you
made, but I've been slacking in going over it one last time and
getting it sent in. I had someone contact me directly around December
and they successfully used this updated patch on their hardware.

I'll see if I can get the patch brushed up and sent in shortly.

Thanks for the reminder (ugh, can't believe it's been over one year).

Justin