Index: Makefile

25 Mar 2013

On Sun, 24 Mar 2013 11:10:19 -0400
Justin Chevrier jchevrier@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:

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;

[…]
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner


    

Re: [flashrom] Add PICkit2 as an SPI programmer

Index: Makefile

Index: flashrom.c

Index: pickit2_spi.c