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