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

[Tormod Volden]

On Tue, Jul 3, 2012 at 6:56 AM, Tommi Keisala wrote:
> Hi,
>
> After discussion over the weekend I brewed another patch that obsoletes the
> old one.
> So here's a new patch that adds TI Stellaris bootloader DFU prefix support
> for dfu-suffix tool.
> After binary image is prefixed (and suffixed) it can be flashed with stock
> dfu-util.
>
> So comments and suggestions are welcome.

Great! I like this approach better. Thanks a lot for the rework. The
patch looks very clean and nice, just a few comments, nitpicks and
bikeshedding from me:

>> From 99e1f89efe677897ade62d70a3f05e7c26c9f125 Mon Sep 17 00:00:00 2001
>> From: Tommi Keisala <tommi.keisala at ray.fi>
>> Date: Tue, 3 Jul 2012 07:47:09 +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
>> generate that prefix.
>>
>> When adding prefix (and suffix) option -s requires address as and argument.

s/and/an

>> Address argument is not required when dfu-suffix is used to delete or check
>> prefix. Deleting prefix needs to happen at the same time when DFU suffix is
>> removed.

I don't think that options can have optional argument when using
getopt(). I think the -s option will swallow whatever word comes next
whether it starts with a hyphen or not.

>>
>> 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 -s -D image.bin

Did you test this? So I would think -D will be interpreted as a
Stellaris address.

>>
>> To check DFU suffix use:
>> dfu-suffix -c image.bin
>> ---
>>  src/Makefile.am |    4 +-
>>  src/lmdfu.c     |  181 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  src/lmdfu.h     |   30 +++++++++
>>  src/suffix.c    |   35 ++++++++---
>>  4 files changed, 242 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..d7a5bb7
>> --- /dev/null
>> +++ b/src/lmdfu.c
>> @@ -0,0 +1,181 @@
>> +/* This implements the TI Stellaris DFU
>> + * as per the Application Update Using the USB Device Firmware Upgrade Class
>> + * (Document AN012373)

Maybe add the document link since it is difficult to find.

>> + *
>> + * (C) 2007-2008 by Harald Welte <laforge at gnumonks.org>

Bad paste? I am not sure what Harald has to do with this code?

>> + * 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"
>> +
>> +/* dfu_prefix payload length excludes prefix and suffix */
>> +unsigned char dfu_prefix[] = {

Maybe call this lmdfu_dfu_prefix[] or lmdfu_prefix[] for consistency?

>> + 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 dfu_prefix with correct data */
>> + addr = address / 1024;
>> + dfu_prefix[2] = (unsigned char)addr & 0xff;
>> + dfu_prefix[3] = (unsigned char)addr >> 8;
>> + dfu_prefix[4] = (unsigned char)len & 0xff;
>> + dfu_prefix[5] = (unsigned char)(len >> 8) & 0xff;
>> + dfu_prefix[6] = (unsigned char)(len >> 16) & 0xff;
>> + dfu_prefix[7] = (unsigned char)(len) >> 24;
>> +
>> + rewind(file.filep);
>> + ret = fwrite(dfu_prefix, 1, sizeof(dfu_prefix), file.filep);
>> + if (ret < 0) {
>> + fprintf(stderr, "Could not write TI Stellaris DFU prefix\n");
>> + perror(file.name);
>> + } else if (ret < sizeof(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(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;
>> +
>> + 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);

We guard this with #ifdef HAVE_FTRUNCATE in src/suffix.c, just in case
someone manage to compile it in some exotic environment.

>> + if (ret < 0) {
>> + fprintf(stderr, "Error truncating\n");
>> + }
>> + rewind(file->filep);
>> +
>> + fwrite(data + sizeof(dfu_prefix), 1, len - sizeof(dfu_prefix),
>> +       file->filep);
>> +
>> + printf("TI Stellaris prefix removed\n");
>> +
>> + return ret;
>> +}
>> +
>> +int lmdfu_check_prefix(struct dfu_file *file)
>> +{
>> + unsigned char *data;
>> + int ret;
>> +
>> + data = malloc(sizeof(dfu_prefix));
>> +
>> + ret = fread(data, 1, sizeof(dfu_prefix), file->filep);
>> + if (ret < sizeof(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");

This will be printed out for all "normal" files, since this function
is unconditionally called from check_suffix, right? Would it be
possible to reserve this to the case -s is used? Or are there
advantages enough to always report on a missing TI prefix?

>> + 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..7b3f843 100644
>> --- a/src/suffix.c
>> +++ b/src/suffix.c
>> @@ -22,6 +22,7 @@
>>  #include <getopt.h>
>>
>>  #include "dfu_file.h"
>> +#include "lmdfu.h"
>>  #ifdef HAVE_CONFIG_H
>>  #include "config.h"
>>  #endif
>> @@ -47,6 +48,7 @@ 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 --stellaris Add TI Stellaris prefic to <file>\n"

s/prefic/prefix

You do not mention the address argument here?

>>   );
>>  }
>>
>> @@ -67,6 +69,7 @@ static struct option opts[] = {
>>   { "did", 1, 0, 'd' },
>>   { "check", 1, 0, 'c' },
>>   { "add", 1, 0, 'a' },
>> + { "stellaris", 1, 0, 's' },
>>  };
>>
>>  static int check_suffix(struct dfu_file *file) {
>> @@ -81,6 +84,7 @@ static int check_suffix(struct dfu_file *file) {
>>   printf("BCD DFU:\t0x%04X\n", file->bcdDFU);
>>   printf("Length:\t\t%i\n", file->suffixlen);
>>   printf("CRC:\t\t0x%08X\n", file->dwCRC);
>> + lmdfu_check_prefix(file);
>>   }
>>
>>   return ret;
>> @@ -111,12 +115,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 +132,8 @@ int main(int argc, char **argv)
>>   struct dfu_file file;
>>   int pid, vid, did;
>>   enum mode mode = MODE_NONE;
>> + unsigned int prefix_address=0;

Maybe you should call this lmdfu_prefix_address? Or even lmdfu_start_address?

>> + int lmdfu_prefix=0;
>>
>>   print_version();
>>
>> @@ -142,7 +142,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:", opts,
>>   &option_index);
>>   if (c == -1)
>>   break;
>> @@ -176,6 +176,10 @@ int main(int argc, char **argv)
>>   file.name = optarg;
>>   mode = MODE_ADD;
>>   break;
>> + case 's':
>> + prefix_address = strtoul(optarg, NULL, 16);
>> + lmdfu_prefix = 1;
>> + break;
>>   default:
>>   help();
>>   exit(2);
>> @@ -198,6 +202,21 @@ int main(int argc, char **argv)
>>
>>   switch(mode) {
>>   case MODE_ADD:
>> + if (check_suffix(&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(prefix_address)
>> + lmdfu_add_prefix(file, prefix_address);
>> + else {
>> + fprintf(stderr, "Not valid address for TI Stellaris prefix\n");
>> + exit(2);
>> + }
>> + }
>>   add_suffix(&file, pid, vid, did);
>>   break;
>>   case MODE_CHECK:
>> @@ -206,6 +225,8 @@ int main(int argc, char **argv)
>>   break;
>>   case MODE_DEL:
>>   remove_suffix(&file);
>> + if(lmdfu_prefix)
>> + lmdfu_remove_prefix(&file);
>>   break;
>>   default:
>>   help();
>> --
>> 1.7.10

[sorry, gmail ate all the tabs when I quoted the patch]

After taking a look at the TI application note, I see TI offer a
dfu-wrap command line utility. Is your dfu-suffix version pretty much
equivalent to this? Then you might want to mention it somewhere to
help people googling for alternatives. Oh, I just did ;) We can also
add that to the dfu-util web page.

Cheers,
Tormod