[dfu-util] TI Stellaris prefix support for dfu-suffix

Tormod Volden lists.tormod at gmail.com
Thu Jul 5 23:02:33 CEST 2012


On Thu, Jul 5, 2012 at 7:40 AM, Tommi Keisala wrote:
>
> Yes you are correct. There is optional_argument option for getopt_long but
> that does not seem to work as I want it. And there seems to be a reason why
> it is implemented as it is. Anyhow I do not want to use it. So I added
> another option -t that requires no argument and is meant to use with
> deleting prefix (with -D). And option -s is for adding (-a).
>
> Suggestions are very welcome how to handle this better.

Hi,

I see some alternatives, not necessarily better:
1. Always require an address argument. Explain in the usage text that
a dummy address is needed when deleting or checking. This allows us to
do all Stellaris stuff without introducing more than one new option.
2. Replace "-D -t" with e.g. -T. So use -D for "normal" devices and -T
for Stellaris. Then we would need a third option for "-t -c" unless we
go back to always probing for this prefix. So we will still have to
(or three) new options, so I am not sure this is better than your
suggestion.
3. Let -D delete the suffix as before. Let -T only delete prefix. So
the most usual usage will practically be the same (-D -T vs -D -t),
but it is more clear what each option does, and it is more flexible.
Same issue for -c checking as in (2).

This is a lot of discussion for a for a few options, but it is
worthwhile to get this optimal before it is released to the users. It
is easy to add new options, but to change or remove them afterwards is
difficult and causes pain for users.

Additionally, it would be an idea to do some check that -T (or -D -t
or -D -s 0 ...) actually deletes something that looks like a Stellaris
prefix, and not just the first 8 bytes of a non-prefixed binary. This
can wait for another patch though.

Thanks for the new patch, just a few comments below.

> From 8b49b3441de91c580d7c0326d1d4973fd280aa54 Mon Sep 17 00:00:00 2001
> From: Tommi Keisala <tommi.keisala at ray.fi>
> Date: Thu, 5 Jul 2012 08:34:29 +0300
> Subject: [PATCH] add, remove and check TI Stellaris DFU prefix
>
> Patch adds functionality to add, remove and check TI Stellaris DFU prefix with
> dfu-suffix tool. TI Stellaris bootloader expects to have 8 byte prefix in the
> beginning of binary image. Patch adds option -s --stellaris to dfu-suffix to
> add that prefix (with -a).
>
> When adding (-a) prefix (and suffix) option -s requires address as an argument.
> To delete prefix use -D and -t option.
> Deleting prefix needs to happen at the same time when DFU suffix is removed.
>
> To add DFU suffix and prefix use:
> dfu-suffix -s 0x2000 -v 0x1cbe -p 0x00ff -d 0x0000 -a image.bin
>
> To remove DFU suffix and prefix use:
> dfu-suffix -t -D image.bin
>
> To check DFU suffix use:
> dfu-suffix -t -c image.bin
> ---
>  src/Makefile.am |    4 +-
>  src/lmdfu.c     |  185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/lmdfu.h     |   30 +++++++++
>  src/suffix.c    |   42 ++++++++++---
>  4 files changed, 253 insertions(+), 8 deletions(-)
>  create mode 100644 src/lmdfu.c
>  create mode 100644 src/lmdfu.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index df2ef36..99df307 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -19,4 +19,6 @@ dfu_util_SOURCES = main.c \
>
>  dfu_suffix_SOURCES = suffix.c \
>  		dfu_file.h \
> -		dfu_file.c
> +		dfu_file.c \
> +		lmdfu.c \
> +		lmdfu.h
> diff --git a/src/lmdfu.c b/src/lmdfu.c
> new file mode 100644
> index 0000000..0989056
> --- /dev/null
> +++ b/src/lmdfu.c
> @@ -0,0 +1,185 @@
> +/* This implements the TI Stellaris DFU
> + * as per the Application Update Using the USB Device Firmware Upgrade Class
> + * (Document AN012373)
> + * http://www.ti.com/general/docs/lit/getliterature.tsp?literatureNumber=spma003&fileType=pdf
> + *
> + * Copyright 2012 Tommi Keisala <tommi.keisala at ray.fi>
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include "portable.h"
> +#include "dfu.h"
> +#include "dfu_file.h"
> +#include "quirks.h"
> +
> +/* lmdfu_dfu_prefix payload length excludes prefix and suffix */
> +unsigned char lmdfu_dfu_prefix[] = {
> +	0x01,			/* STELLARIS_DFU_PROG */
> +	0x00,			/* Reserved */
> +	0x00,			/* LSB start address / 1024 */
> +	0x20,			/* MSB start address / 1024 */
> +	0x00,			/* LSB file payload length */
> +	0x00,			/* Byte 2 file payload length */
> +	0x00,			/* Byte 3 file payload length */
> +	0x00,			/* MSB file payload length */
> +};
> +
> +int lmdfu_add_prefix(struct dfu_file file, unsigned int address)
> +{
> +	int ret;
> +	uint16_t addr;
> +	uint32_t len;
> +
> +	unsigned char *data = NULL;
> +
> +	fseek(file.filep, 0, SEEK_END);
> +	len = ftell(file.filep);
> +	rewind(file.filep);
> +
> +	data = (unsigned char *)malloc(len);
> +	if (!data) {
> +		fprintf(stderr, "Unable to allocate buffer.\n");
> +		exit(1);
> +	}
> +
> +	ret = fread(data, 1, len, file.filep);
> +	if (ret < 0) {
> +		fprintf(stderr, "Could not read file\n");
> +		perror(file.name);
> +		free(data);
> +		return ret;
> +	} else if (ret < len) {
> +		fprintf(stderr, "Could not read whole file\n");
> +		free(data);
> +		return -EIO;
> +	}
> +
> +	/* fill Stellaris lmdfu_dfu_prefix with correct data */
> +	addr = address / 1024;
> +	lmdfu_dfu_prefix[2] = (unsigned char)addr & 0xff;
> +	lmdfu_dfu_prefix[3] = (unsigned char)addr >> 8;
> +	lmdfu_dfu_prefix[4] = (unsigned char)len & 0xff;
> +	lmdfu_dfu_prefix[5] = (unsigned char)(len >> 8) & 0xff;
> +	lmdfu_dfu_prefix[6] = (unsigned char)(len >> 16) & 0xff;
> +	lmdfu_dfu_prefix[7] = (unsigned char)(len) >> 24;
> +
> +	rewind(file.filep);
> +	ret = fwrite(lmdfu_dfu_prefix, 1, sizeof(lmdfu_dfu_prefix), file.filep);
> +	if (ret < 0) {
> +		fprintf(stderr, "Could not write TI Stellaris DFU prefix\n");
> +		perror(file.name);
> +	} else if (ret < sizeof(lmdfu_dfu_prefix)) {
> +		fprintf(stderr, "Could not write while file\n");
> +		ret = -EIO;
> +	}
> +
> +	ret = fwrite(data, 1, len, file.filep);
> +	if (ret < 0) {
> +		fprintf(stderr, "Could not write data after TI Stellaris DFU "
> +			"prefix\n");
> +		perror(file.name);
> +	} else if (ret < sizeof(lmdfu_dfu_prefix)) {
> +		fprintf(stderr, "Could not write whole file\n");
> +		ret = -EIO;
> +	}
> +
> +	rewind(file.filep);
> +	printf("TI Stellaris DFU prefix added.\n");
> +	return 0;
> +}
> +
> +int lmdfu_remove_prefix(struct dfu_file *file)
> +{
> +	long len;
> +	unsigned char *data;
> +	int ret;
> +
> +#ifdef HAVE_FTRUNCATE
> +	printf("Remove TI Stellaris prefix\n");
> +
> +	fseek(file->filep, 0, SEEK_END);
> +	len = ftell(file->filep);
> +	rewind(file->filep);
> +
> +	data = (unsigned char *)malloc(len);
> +	if (!data) {
> +		fprintf(stderr, "Unable to allocate buffer.\n");
> +		exit(1);
> +	}
> +
> +	ret = fread(data, 1, len, file->filep);
> +	if (ret < 0) {
> +		fprintf(stderr, "Could not read file\n");
> +		perror(file->name);
> +		free(data);
> +		return ret;
> +	} else if (ret < len) {
> +		fprintf(stderr, "Could not read whole file\n");
> +		free(data);
> +		return -EIO;
> +	}
> +
> +	ret = ftruncate(fileno(file->filep), 0);
> +	if (ret < 0) {
> +		fprintf(stderr, "Error truncating\n");
> +	}
> +	rewind(file->filep);
> +
> +	fwrite(data + sizeof(lmdfu_dfu_prefix), 1, len - sizeof(lmdfu_dfu_prefix),
> +	       file->filep);
> +
> +	printf("TI Stellaris prefix removed\n");
> +#else
> +	printf("Prefix removal not implemented on this platform\n");
> +#endif /* HAVE_FTRUNCATE */
> +
> +	return ret;
> +}
> +
> +int lmdfu_check_prefix(struct dfu_file *file)
> +{
> +	unsigned char *data;
> +	int ret;
> +
> +	data = malloc(sizeof(lmdfu_dfu_prefix));
> +
> +	ret = fread(data, 1, sizeof(lmdfu_dfu_prefix), file->filep);
> +	if (ret < sizeof(lmdfu_dfu_prefix)) {
> +		fprintf(stderr, "Could not read prefix\n");
> +		perror(file->name);
> +	}
> +
> +	if ((data[0] != 0x01) && (data[1] != 0x00)) {
> +		printf("Not valid TI Stellaris DFU prefix\n");
> +		ret = 0;
> +		goto out_rewind;
> +	} else {
> +		printf
> +		    ("Possible TI Stellaris DFU prefix with the following properties\n");
> +		printf("Address:        0x%08x\n",
> +		       1024 * (data[3] << 8 | data[2]));
> +		printf("Payload length: %d\n",
> +		       data[4] | data[5] << 8 | data[6] << 16 | data[7] << 14);
> +	}
> +
> +out_rewind:
> +	rewind(file->filep);
> +	return ret;
> +}
> diff --git a/src/lmdfu.h b/src/lmdfu.h
> new file mode 100644
> index 0000000..8af689f
> --- /dev/null
> +++ b/src/lmdfu.h
> @@ -0,0 +1,30 @@
> +
> +/* This implements the TI Stellaris DFU
> + * as per the Application Update Using the USB Device Firmware Upgrade Class
> + * (Document AN012373)
> + *
> + * Copyright 2012 Tommi Keisala <tommi.keisala at ray.fi>
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#ifndef LMDFU_H
> +#define LMDFU_H
> +
> +int lmdfu_add_prefix(struct dfu_file file, unsigned int address);
> +int lmdfu_remove_prefix(struct dfu_file *file);
> +int lmdfu_check_prefix(struct dfu_file *file);
> +
> +#endif /* LMDFU_H */
> diff --git a/src/suffix.c b/src/suffix.c
> index fe19194..30ca3f5 100644
> --- a/src/suffix.c
> +++ b/src/suffix.c
> @@ -20,8 +20,10 @@
>  #include <stdio.h>
>  #include <stdint.h>
>  #include <getopt.h>
> +#include <string.h>
>
>  #include "dfu_file.h"
> +#include "lmdfu.h"
>  #ifdef HAVE_CONFIG_H
>  #include "config.h"
>  #endif
> @@ -47,6 +49,8 @@ static void help(void)
>  		"  -d --did\tAdd device ID into DFU suffix in <file>\n"
>  		"  -c --check\tCheck DFU suffix of <file>\n"
>  		"  -a --add\tAdd DFU suffix to <file>\n"
> +		"  -s address\tAdd TI Stellaris prefix address to <file>\n"

s/prefix address/address prefix/?

> +		"  -t\t\tDelete TI Stellaris prefix from <file>\n"
>  		);
>  }
>
> @@ -67,6 +71,8 @@ static struct option opts[] = {
>  	{ "did", 1, 0, 'd' },
>  	{ "check", 1, 0, 'c' },
>  	{ "add", 1, 0, 'a' },
> +	{ "stellaris-add", 1, 0, 's' },
> +	{ "stellaris-del", 0, 0, 't' },

You should add the long options to the usage text as well.

>  };
>
>  static int check_suffix(struct dfu_file *file) {
> @@ -111,12 +117,6 @@ static void remove_suffix(struct dfu_file *file)
>  static void add_suffix(struct dfu_file *file, int pid, int vid, int did) {
>  	int ret;
>
> -	ret = check_suffix(file);
> -	if (ret > 0) {
> -		printf("Please remove existing DFU suffix before adding a new one.\n");
> -		exit(1);
> -	}
> -
>  	file->idProduct = pid;
>  	file->idVendor = vid;
>  	file->bcdDevice = did;
> @@ -134,6 +134,8 @@ int main(int argc, char **argv)
>  	struct dfu_file file;
>  	int pid, vid, did;
>  	enum mode mode = MODE_NONE;
> +	unsigned int lmdfu_flash_address=0;
> +	int lmdfu_prefix=0;
>
>  	print_version();
>
> @@ -142,7 +144,7 @@ int main(int argc, char **argv)
>
>  	while (1) {
>  		int c, option_index = 0;
> -		c = getopt_long(argc, argv, "hVD:p:v:d:c:a:", opts,
> +		c = getopt_long(argc, argv, "hVD:p:v:d:c:a:s:t", opts,
>  				&option_index);
>  		if (c == -1)
>  			break;
> @@ -176,6 +178,13 @@ int main(int argc, char **argv)
>  			file.name = optarg;
>  			mode = MODE_ADD;
>  			break;
> +		case 's':
> +			lmdfu_flash_address = strtoul(optarg, NULL, 16);

You have now changed the parsing to always treat the address as hex.
It used to be "lmdfu_address = strtoul(optarg, &end, 0);" so that hex
addresses would need 0x prefix. This should be mentioned in the usage
text. On the other side I prefer your older version here. I find it
handy to be able to give addresses in any base. One example is doing
shell calculations on the command line, e.g. dfu-suffix -s $(0x20000 +
 $myoffset) without having to wrap it in printf etc. We default to hex
for USB IDs but they are IDs and not numbers in mathematical sense.

> +			lmdfu_prefix = 1;
> +			break;
> +		case 't':
> +			lmdfu_prefix = 1;
> +			break;
>  		default:
>  			help();
>  			exit(2);
> @@ -198,14 +207,33 @@ int main(int argc, char **argv)
>
>  	switch(mode) {
>  	case MODE_ADD:
> +		if (check_suffix(&file)) {
> +			if(lmdfu_prefix) lmdfu_check_prefix(&file);
> +			printf("Please remove existing DFU suffix before adding a new one.\n");
> +			exit(1);
> +		}
> +		if(lmdfu_prefix) {
> +			if(lmdfu_check_prefix(&file)) {
> +				fprintf(stderr, "Adding new anyway\n");
> +			}
> +			if(lmdfu_flash_address)
> +				lmdfu_add_prefix(file, lmdfu_flash_address);

Is address 0 never applicable for these devices?

> +			else {
> +				fprintf(stderr, "Not valid address for TI Stellaris prefix\n");
> +				exit(2);
> +			}
> +		}
>  		add_suffix(&file, pid, vid, did);
>  		break;
>  	case MODE_CHECK:
>  		/* FIXME: could open read-only here */
>  		check_suffix(&file);
> +		if(lmdfu_prefix) lmdfu_check_prefix(&file);

I think consistent style would require a line break and indentation
here, like 4 lines below.

>  		break;
>  	case MODE_DEL:
>  		remove_suffix(&file);
> +		if(lmdfu_prefix)
> +			lmdfu_remove_prefix(&file);
>  		break;
>  	default:
>  		help();
> --
> 1.7.10
>

Cheers,
Tormod



More information about the devel mailing list