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.keis...@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 <lafo...@gnumonks.org>
Bad paste? I am not sure what Harald has to do with this code?
>> + * Copyright 2012 Tommi Keisala <tommi.keis...@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.keis...@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
_______________________________________________
devel mailing list
devel@lists.openmoko.org
https://lists.openmoko.org/mailman/listinfo/devel
