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

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Tommi Keisala
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.

-Tommi


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel

0001-add-remove-and-check-TI-Stellaris-DFU-prefix.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [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 <[hidden email]>
>> 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 <[hidden email]>

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

>> + * Copyright 2012 Tommi Keisala <[hidden email]>
>> + *
>> + * 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 <[hidden email]>
>> + *
>> + * 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
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Tommi Keisala
On 07/04/2012 11:09 PM, Tormod Volden wrote:
>>> 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.

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.

>>> 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.

I guess I did not test this since it was not working.

> 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.
Added.
>>> + *
>>> + * (C) 2007-2008 by Harald Welte<[hidden email]>
> Bad paste? I am not sure what Harald has to do with this code?
Not a bad paste. First patch had a lot copy-paste code from Harald. Not
anymore so this is remove.
>
>> +
>> +/* dfu_prefix payload length excludes prefix and suffix */
>> +unsigned char dfu_prefix[] = {
> Maybe call this lmdfu_dfu_prefix[] or lmdfu_prefix[] for consistency?
Done.

>>> + }
>>> +
>>> + 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.

Whole function implementation is now guarded.

>
>> +
>> +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?
Good point. Don't see any reason to do this without -t or -s.
Implementation changed.

>
>> 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?
Fixed.
>
>> @@ -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?
Changed to lmdfu_flash_address to implicate it is address in target
flash memory.
> [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

Yes you can do pretty much the same with dfuwrap tool from StellarisWare.

Thank you for the comments. And here's new patch.

-Tommi


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel

0001-add-remove-and-check-TI-Stellaris-DFU-prefix.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tormod Volden
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 <[hidden email]>
> 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 <[hidden email]>
> + *
> + * 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 <[hidden email]>
> + *
> + * 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

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Andrew Leech
On 6/07/12 7:02 AM, Tormod Volden wrote:

> 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).
>
>
Another option would be to replace getopt for something else to parse
the arguments that can handle optional arguments. Ideally something
that's more posix/portable...?
I bring this up as getopt was one of the stumbling blocks I encountered
when compiling dfu-util with visual-c when I was having problems with
mingw and libusb. I had to find a port of getopt to include in the
project manually, as it's not covered by any standard windows libs.

To be honest though I don't know what would be a better args lib to use,
have to browse some other cross platform apps so see what they use.

Andrew

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Tommi Keisala-2
In reply to this post by Tormod Volden
On 07/06/2012 12:02 AM, Tormod Volden wrote:

> 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).

How about compination?

Add -T option to delete just Stellaris prefix. This option could be used
by itself or with combination with -D. If used it is without -D there
will be check if suffix exists and it does not remove prefix if suffix
exists.

Suffix CRC is included with prefix. That is why you can not just remove
prefix. You have to remove both or recalculate CRC for suffix.

This will also allow deleting prefix after suffix is already removed.
Prefix still needs to be added in combination with suffix.

Adding prefix would work with like now with -s addr.

I'll brew another patch with also other fixes suggested when I can
manage find time.

I agree you do not want change behavior of options once they have been
released.

-Tommi
> 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.
I agree you do not want change behavior of options once they have been
released.
>
> 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.
>

I check for the first two bytes on prefix if it exists since they are
only one fixed. I can later add check that size matches for the image.

I'll cook another patch with also other fixes suggested when I can
manage find time to code.

-Tommi



_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Tommi Keisala-2
In reply to this post by Tormod Volden

Here is another try with fixes Tormod earlier suggested plus a change
that make Stellaris prefix extension happen with different options
between adding (-s address) and deleting (-T).

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel

0001-add-remove-and-check-TI-Stellaris-DFU-prefix.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tommi Keisala

Hello,

I am back from vacation.
I have not seen any comments on this latest patch that's almost month old now.

-Tommi

On 07/10/2012 12:52 PM, Tommi Keisala wrote:

Here is another try with fixes Tormod earlier suggested plus a change that make Stellaris prefix extension happen with different options between adding (-s address) and deleting (-T).


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Tormod Volden
Hi Tommi,

Sorry for the long silence. Your latest patch looks good to me. I just
haven't had the time to do more about it, and I think the same goes
for Stefan. It is summer time and vacation time for many of us :)
Unless testing or further review detects any issues, you can count on
the patch being committed and included in version 0.7.

Best regards,
Tormod


On Mon, Aug 6, 2012 at 6:38 AM, Tommi Keisala <[hidden email]> wrote:

>
> Hello,
>
> I am back from vacation.
> I have not seen any comments on this latest patch that's almost month old
> now.
>
> -Tommi
>
>
> On 07/10/2012 12:52 PM, Tommi Keisala wrote:
>
>
> Here is another try with fixes Tormod earlier suggested plus a change that
> make Stellaris prefix extension happen with different options between adding
> (-s address) and deleting (-T).
>
>
> _______________________________________________
> devel mailing list
> [hidden email]
> https://lists.openmoko.org/mailman/listinfo/devel
>
>
>
> _______________________________________________
> devel mailing list
> [hidden email]
> https://lists.openmoko.org/mailman/listinfo/devel
>

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Tommi Keisala

No problem. I was just pinging to keep it alive.

Thanks,
-Tommi

On 08/06/2012 10:41 AM, Tormod Volden wrote:

> Hi Tommi,
>
> Sorry for the long silence. Your latest patch looks good to me. I just
> haven't had the time to do more about it, and I think the same goes
> for Stefan. It is summer time and vacation time for many of us :)
> Unless testing or further review detects any issues, you can count on
> the patch being committed and included in version 0.7.
>
> Best regards,
> Tormod
>
>
>


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Tormod Volden
In reply to this post by Tommi Keisala-2
On Tue, Jul 10, 2012 at 11:52 AM, Tommi Keisala wrote:
>
> Here is another try with fixes Tormod earlier suggested plus a change that
> make Stellaris prefix extension happen with different options between adding
> (-s address) and deleting (-T).
>

Hi Tommi,

While preparing to apply your patch, a few questions about the patch came up:

> @@ -67,6 +80,8 @@ static struct option opts[] = {
>        { "did", 1, 0, 'd' },
>        { "check", 1, 0, 'c' },
>        { "add", 1, 0, 'a' },
>+       { "stellaris-add", 1, 0, 's' },
>+       { "stellaris", 0, 0, 'T' },
> };
>
> static int check_suffix(struct dfu_file *file) {

About the long option name for -s: Do you agree that
"--stellaris-address" would be better? Since its argument is really an
address, and the user still need to use the --add option as well. So
the usage would be --add --stellaris-address XXXX image.file.

I can fix that up myself, no need for resending the patch.

+               case 's':
+                       lmdfu_mode = LMDFU_ADD;
+                       lmdfu_flash_address = strtoul(optarg, NULL, 0);
+                       if (strcmp(optarg, "default")) {
+                               lmdfu_flash_address = strtoul(optarg, &end, 0);
+                               if (!lmdfu_flash_address || (*end)) {

This looks a bit strange to me, you are reading out
lmdfu_flash_address twice. And does the "default" argument value,
leaving lmdfu_flash_address zero, make any sense?

Cheers,
Tormod

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Tommi Keisala-2
On 09/16/2012 01:51 PM, Tormod Volden wrote:
> About the long option name for -s: Do you agree that
> "--stellaris-address" would be better? Since its argument is really an
> address, and the user still need to use the --add option as well. So
> the usage would be --add --stellaris-address XXXX image.file.
>
> I can fix that up myself, no need for resending the patch.
I agree.

> +               case 's':
> +                       lmdfu_mode = LMDFU_ADD;
> +                       lmdfu_flash_address = strtoul(optarg, NULL, 0);
> +                       if (strcmp(optarg, "default")) {
> +                               lmdfu_flash_address = strtoul(optarg,&end, 0);
> +                               if (!lmdfu_flash_address || (*end)) {
>
> This looks a bit strange to me, you are reading out
> lmdfu_flash_address twice. And does the "default" argument value,
> leaving lmdfu_flash_address zero, make any sense?
>
> Cheers,
> Tormod

Good catch.
Maybe it would be good idea to force user give proper address rather
than defaulting to 0.
I can't recall the reasoning for reading lmdfu_flash_address twice and
now it seems quite silly.
Maybe I should rewrite this part to not accept "default" and still
checking if strtoul sets *end?

Something like this:
diff --git a/src/suffix.c b/src/suffix.c
index 4ebfee7..f66300e 100644
--- a/src/suffix.c
+++ b/src/suffix.c
@@ -192,14 +192,11 @@ int main(int argc, char **argv)
                         break;
                 case 's':
                         lmdfu_mode = LMDFU_ADD;
-                       lmdfu_flash_address = strtoul(optarg, NULL, 0);
-                       if (strcmp(optarg, "default")) {
-                               lmdfu_flash_address = strtoul(optarg,
&end, 0);
-                               if (!lmdfu_flash_address || (*end)) {
-                                       fprintf(stderr, "Error: Invalid
lmdfu "
-                                               "address: %s\n", optarg);
-                                       exit(2);
-                               }
+                       lmdfu_flash_address = strtoul(optarg, &end, 0);
+                       if (*end) {
+                               fprintf(stderr, "Error: Invalid lmdfu "
+                                       "address: %s\n", optarg);
+                               exit(2);
                         }
                         break;
                 case 'T':


-Tommi


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Tormod Volden
On Mon, Sep 17, 2012 at 7:27 AM, Tommi Keisala wrote:

> Good catch.
> Maybe it would be good idea to force user give proper address rather than
> defaulting to 0.
> I can't recall the reasoning for reading lmdfu_flash_address twice and now
> it seems quite silly.
> Maybe I should rewrite this part to not accept "default" and still checking
> if strtoul sets *end?
>
> Something like this:
> diff --git a/src/suffix.c b/src/suffix.c
> index 4ebfee7..f66300e 100644
> --- a/src/suffix.c
> +++ b/src/suffix.c
> @@ -192,14 +192,11 @@ int main(int argc, char **argv)
>                         break;
>                 case 's':
>
>                         lmdfu_mode = LMDFU_ADD;
> -                       lmdfu_flash_address = strtoul(optarg, NULL, 0);
> -                       if (strcmp(optarg, "default")) {
> -                               lmdfu_flash_address = strtoul(optarg, &end,
> 0);
> -                               if (!lmdfu_flash_address || (*end)) {
> -                                       fprintf(stderr, "Error: Invalid
> lmdfu "
> -                                               "address: %s\n", optarg);
> -                                       exit(2);
> -                               }
>
> +                       lmdfu_flash_address = strtoul(optarg, &end, 0);
> +                       if (*end) {
> +                               fprintf(stderr, "Error: Invalid lmdfu "
> +                                       "address: %s\n", optarg);
> +                               exit(2);
>                         }
>                         break;
>                 case 'T':

Yes, that looks good.

Tormod


>
>
> -Tommi
>

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|

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

Tommi Keisala

And here's new patch with the changes in case you did not yet integrate
this stuff.

-Tommi

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel

0001-add-remove-and-check-TI-Stellaris-DFU-prefix.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tormod Volden
On Mon, Sep 17, 2012 at 9:28 AM, Tommi Keisala wrote:
>
> And here's new patch with the changes in case you did not yet integrate this
> stuff.

Thanks a lot! Applied.

(I only massaged the help text a bit, to preserve the indentation of
the old options, these new ones are so long anyway it cannot be made
strictly consistent. And split it in two strings to please a -pedantic
warning.)

Cheers,
Tormod

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel