| Message ID | 1341308291-14663-6-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Lukasz Majewski, > Support for u-boot's command line command "dfu <interface> <dev> [list]". > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Marek Vasut <marex@denx.de> > --- [...] > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > +{ > + char *str_env = NULL, *env_bkp = NULL; > + static char *s = "dfu"; > + int ret = 0; > + > + if (argc < 3) > + return CMD_RET_USAGE; > + > + str_env = getenv("dfu_alt_info"); > + if (str_env == NULL) { > + printf("%s: \"dfu_alt_info\" env variable not defined!\n", > + __func__); I was always curious if it's not possible to do something like puts(__func__ "rest of string"); Maybe it'd help the overhead a bit? Certainly, it's beyond the scope of this patchset, I'm just curious :) > + return CMD_RET_FAILURE; > + } [...] Best regards, Marek Vasut
Hi Marek, > Dear Lukasz Majewski, > > > Support for u-boot's command line command "dfu <interface> <dev> > > [list]". > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > Cc: Marek Vasut <marex@denx.de> > > --- > > [...] > > > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const > > argv[]) +{ > > + char *str_env = NULL, *env_bkp = NULL; > > + static char *s = "dfu"; > > + int ret = 0; > > + > > + if (argc < 3) > > + return CMD_RET_USAGE; > > + > > + str_env = getenv("dfu_alt_info"); > > + if (str_env == NULL) { > > + printf("%s: \"dfu_alt_info\" env variable not > > defined!\n", > > + __func__); > > I was always curious if it's not possible to do something like > > puts(__func__ "rest of string"); > > Maybe it'd help the overhead a bit? Certainly, it's beyond the scope > of this patchset, I'm just curious :) It is a good idea, since many error/info messages are supposed to produce following output: "dfu_write: Not enough space!" Putting there the __func__ name would improve structure and speed up finding right place. > > > + return CMD_RET_FAILURE; > > + } > > [...] > > Best regards, > Marek Vasut
Dear Lukasz Majewski, > Hi Marek, > > > Dear Lukasz Majewski, > > > > > Support for u-boot's command line command "dfu <interface> <dev> > > > [list]". > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > > Cc: Marek Vasut <marex@denx.de> > > > --- > > > > [...] > > > > > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const > > > argv[]) +{ > > > + char *str_env = NULL, *env_bkp = NULL; > > > + static char *s = "dfu"; > > > + int ret = 0; > > > + > > > + if (argc < 3) > > > + return CMD_RET_USAGE; > > > + > > > + str_env = getenv("dfu_alt_info"); > > > + if (str_env == NULL) { > > > + printf("%s: \"dfu_alt_info\" env variable not > > > defined!\n", > > > + __func__); > > > > I was always curious if it's not possible to do something like > > > > puts(__func__ "rest of string"); > > > > Maybe it'd help the overhead a bit? Certainly, it's beyond the scope > > of this patchset, I'm just curious :) > > It is a good idea, since many error/info messages are supposed to > produce following output: > > "dfu_write: Not enough space!" > > Putting there the __func__ name would improve structure and speed up > finding right place. And if you want to use even __LINE__, look up __stringify patch in the ML archives ;-) > > > + return CMD_RET_FAILURE; > > > + } > > > > [...] > > > > Best regards, > > Marek Vasut Best regards, Marek Vasut
On Tuesday 03 July 2012 05:38:09 Lukasz Majewski wrote: > --- /dev/null > +++ b/common/cmd_dfu.c > > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) static > +{ > + char *str_env = NULL, *env_bkp = NULL; no need to assign NULL here. str_env should be const. > + static char *s = "dfu"; no need to declare this static > + int ret = 0; no need to init to 0 > + env_bkp = strdup(str_env); > + ret = dfu_config_entities(env_bkp, argv[1], > + (int)simple_strtoul(argv[2], NULL, 10)); > + if (ret) > + return CMD_RET_FAILURE; > + > + if (strcmp(argv[3], "list") == 0) { > + dfu_show_entities(); > + dfu_free_entities(); > + free(env_bkp); > + return CMD_RET_SUCCESS; for these last three statements, you could just do "goto done" and put a done label below ... > +exit: > + g_dnl_cleanup(); done: > + dfu_free_entities(); > + free(env_bkp); > + > + return CMD_RET_SUCCESS; -mike
On Tuesday 03 July 2012 17:32:54 Marek Vasut wrote: > > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > +{ > > + char *str_env = NULL, *env_bkp = NULL; > > + static char *s = "dfu"; > > + int ret = 0; > > + > > + if (argc < 3) > > + return CMD_RET_USAGE; > > + > > + str_env = getenv("dfu_alt_info"); > > + if (str_env == NULL) { > > + printf("%s: \"dfu_alt_info\" env variable not defined!\n", > > + __func__); > > I was always curious if it's not possible to do something like > > puts(__func__ "rest of string"); > > Maybe it'd help the overhead a bit? Certainly, it's beyond the scope of > this patchset, I'm just curious :) not anymore -- gcc disabled support for that sometime ago. and it's good they did as it causes more bloat than good. as soon as you do more than one statement, you get duplication. the string table will have: "some_func: rest of string" "some_func: boo" "some_func: another message" which takes up more space than: "some_func" "%s: rest of string" "%s: boo" "%s: another message" -mike
On Wednesday 04 July 2012 10:39:20 Marek Vasut wrote: > > Putting there the __func__ name would improve structure and speed up > > finding right place. > > And if you want to use even __LINE__, look up __stringify patch in the ML > archives ;-) ugh, no, let's not use __LINE__ anywhere other than debug(). it has no business in code we ship as it is pointless bloated noise. -mike
Dear Mike Frysinger, > On Wednesday 04 July 2012 10:39:20 Marek Vasut wrote: > > > Putting there the __func__ name would improve structure and speed up > > > finding right place. > > > > And if you want to use even __LINE__, look up __stringify patch in the ML > > archives ;-) > > ugh, no, let's not use __LINE__ anywhere other than debug(). it has no > business in code we ship as it is pointless bloated noise. Helps find out the problematic place in code ... > -mike Best regards, Marek Vasut
Dear Mike Frysinger, > On Tuesday 03 July 2012 17:32:54 Marek Vasut wrote: > > > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > > +{ > > > + char *str_env = NULL, *env_bkp = NULL; > > > + static char *s = "dfu"; > > > + int ret = 0; > > > + > > > + if (argc < 3) > > > + return CMD_RET_USAGE; > > > + > > > + str_env = getenv("dfu_alt_info"); > > > + if (str_env == NULL) { > > > + printf("%s: \"dfu_alt_info\" env variable not defined!\n", > > > + __func__); > > > > I was always curious if it's not possible to do something like > > > > puts(__func__ "rest of string"); > > > > Maybe it'd help the overhead a bit? Certainly, it's beyond the scope of > > this patchset, I'm just curious :) > > not anymore -- gcc disabled support for that sometime ago. and it's good > they did as it causes more bloat than good. as soon as you do more than > one statement, you get duplication. the string table will have: > "some_func: rest of string" > "some_func: boo" > "some_func: another message" > which takes up more space than: > "some_func" > "%s: rest of string" > "%s: boo" > "%s: another message" Good knowing this. I'd expect gcc would build some trie in there to optimize the size. > -mike Best regards, Marek Vasut
On Friday 20 July 2012 07:33:49 Marek Vasut wrote: > Dear Mike Frysinger, > > On Wednesday 04 July 2012 10:39:20 Marek Vasut wrote: > > > > Putting there the __func__ name would improve structure and speed up > > > > finding right place. > > > > > > And if you want to use even __LINE__, look up __stringify patch in the > > > ML archives ;-) > > > > ugh, no, let's not use __LINE__ anywhere other than debug(). it has no > > business in code we ship as it is pointless bloated noise. > > Helps find out the problematic place in code ... except the code changes thus invalidating the line numbers, and how often are you putting the same string in multiple places that you can't easily coordinate where it came from ? if people are using the same exact string in multiple places, that sounds like a different problem. -mike
Dear Mike Frysinger, > On Friday 20 July 2012 07:33:49 Marek Vasut wrote: > > Dear Mike Frysinger, > > > > > On Wednesday 04 July 2012 10:39:20 Marek Vasut wrote: > > > > > Putting there the __func__ name would improve structure and speed > > > > > up finding right place. > > > > > > > > And if you want to use even __LINE__, look up __stringify patch in > > > > the ML archives ;-) > > > > > > ugh, no, let's not use __LINE__ anywhere other than debug(). it has no > > > business in code we ship as it is pointless bloated noise. > > > > Helps find out the problematic place in code ... > > except the code changes thus invalidating the line numbers, and how often > are you putting the same string in multiple places that you can't easily > coordinate where it came from ? if people are using the same exact string > in multiple places, that sounds like a different problem. You can always replace the function names with macros, which expand in place. And then simply add __func__ __LINE__ __FILE__ etc. > -mike Best regards, Marek Vasut
On Friday 20 July 2012 17:11:33 Marek Vasut wrote: > Dear Mike Frysinger, > > On Friday 20 July 2012 07:33:49 Marek Vasut wrote: > > > Dear Mike Frysinger, > > > > On Wednesday 04 July 2012 10:39:20 Marek Vasut wrote: > > > > > > Putting there the __func__ name would improve structure and speed > > > > > > up finding right place. > > > > > > > > > > And if you want to use even __LINE__, look up __stringify patch in > > > > > the ML archives ;-) > > > > > > > > ugh, no, let's not use __LINE__ anywhere other than debug(). it has > > > > no business in code we ship as it is pointless bloated noise. > > > > > > Helps find out the problematic place in code ... > > > > except the code changes thus invalidating the line numbers, and how often > > are you putting the same string in multiple places that you can't easily > > coordinate where it came from ? if people are using the same exact > > string in multiple places, that sounds like a different problem. > > You can always replace the function names with macros, which expand in > place. And then simply add __func__ __LINE__ __FILE__ etc. if you wanted to add it while debugging, that's fine, but the point is that this doesn't belong in normal runtime images. it's even trivial to define such a macro: #ifdef DEBUG # define printf(fmt, args...) printf("%s:%s:%i: " fmt, __FILE__, __LINE__, __func__, ## args) #endif (will obviously need a little more work to handle non-const fmt strings, but you get the idea). -mike
Dear Mike Frysinger, > On Friday 20 July 2012 17:11:33 Marek Vasut wrote: > > Dear Mike Frysinger, > > > > > On Friday 20 July 2012 07:33:49 Marek Vasut wrote: > > > > Dear Mike Frysinger, > > > > > > > > > On Wednesday 04 July 2012 10:39:20 Marek Vasut wrote: > > > > > > > Putting there the __func__ name would improve structure and > > > > > > > speed up finding right place. > > > > > > > > > > > > And if you want to use even __LINE__, look up __stringify patch > > > > > > in the ML archives ;-) > > > > > > > > > > ugh, no, let's not use __LINE__ anywhere other than debug(). it > > > > > has no business in code we ship as it is pointless bloated noise. > > > > > > > > Helps find out the problematic place in code ... > > > > > > except the code changes thus invalidating the line numbers, and how > > > often are you putting the same string in multiple places that you > > > can't easily coordinate where it came from ? if people are using the > > > same exact string in multiple places, that sounds like a different > > > problem. > > > > You can always replace the function names with macros, which expand in > > place. And then simply add __func__ __LINE__ __FILE__ etc. > > if you wanted to add it while debugging, that's fine, but the point is that > this doesn't belong in normal runtime images. Well doh ... > it's even trivial to define > such a macro: > #ifdef DEBUG > # define printf(fmt, args...) printf("%s:%s:%i: " fmt, __FILE__, __LINE__, > __func__, ## args) > #endif Uh, now I'm not sure what you mean by this stuff above. > (will obviously need a little more work to handle non-const fmt strings, > but you get the idea). > -mike Best regards, Marek Vasut
Dear Mike, > On Tuesday 03 July 2012 05:38:09 Lukasz Majewski wrote: > > --- /dev/null > > +++ b/common/cmd_dfu.c > > > > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const > > argv[]) > > static It can be static (static int do_dfu). On the other hand the U_BOOT_CMD macro defines: int (*cmd)(struct cmd_tbl_s *, int, int, char * const []); at struct cmd_tbl_s, which is int. > > > +{ > > + char *str_env = NULL, *env_bkp = NULL; > > no need to assign NULL here. str_env should be const. Yes, correct. > > > + static char *s = "dfu"; > > no need to declare this static Why not? The s ptr is further passed to g_dnl_init(s), so my intend was to declare string in the data segment of this translation unit. In this way the data under this ptr will not vanish. > > > + int ret = 0; > > no need to init to 0 Ok. > > > + env_bkp = strdup(str_env); > > + ret = dfu_config_entities(env_bkp, argv[1], > > + (int)simple_strtoul(argv[2], NULL, > > 10)); > > + if (ret) > > + return CMD_RET_FAILURE; > > + > > + if (strcmp(argv[3], "list") == 0) { > > + dfu_show_entities(); > > + dfu_free_entities(); > > + free(env_bkp); > > + return CMD_RET_SUCCESS; > > for these last three statements, you could just do "goto done" and > put a done label below ... Yes, you are correct. Thanks for tip. > > > +exit: > > + g_dnl_cleanup(); > > done: > > > + dfu_free_entities(); > > + free(env_bkp); > > + > > + return CMD_RET_SUCCESS; > > -mike
On Monday 23 July 2012 12:01:04 Lukasz Majewski wrote: > Dear Mike, > > On Tuesday 03 July 2012 05:38:09 Lukasz Majewski wrote: > > > --- /dev/null > > > +++ b/common/cmd_dfu.c > > > > > > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const > > > argv[]) > > > > static > > It can be static (static int do_dfu). On the other hand the U_BOOT_CMD > macro defines: > > int (*cmd)(struct cmd_tbl_s *, int, int, char * const []); at > struct cmd_tbl_s, which is int. i don't understand what you're trying to say > > > + static char *s = "dfu"; > > > > no need to declare this static > > Why not? The s ptr is further passed to g_dnl_init(s), so my intend was > to declare string in the data segment of this translation unit. In this > way the data under this ptr will not vanish. except your g_dnl_init already copies the incoming string to local storage, so where it is stored doesn't matter. further, the "dfu" is going to be in .rodata (thus never going away), it is only the "s" pointer which will be stored in .data, not the thing it points to. had you done: char s[] = { 'd', 'f', 'u', '\0', }; then you'd get 4 bytes on the stack -mike
Hi Mike, > On Monday 23 July 2012 12:01:04 Lukasz Majewski wrote: > > Dear Mike, > > > On Tuesday 03 July 2012 05:38:09 Lukasz Majewski wrote: > > > > --- /dev/null > > > > +++ b/common/cmd_dfu.c > > > > > > > > +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const > > > > argv[]) > > > > > > static > > > > It can be static (static int do_dfu). On the other hand the > > U_BOOT_CMD macro defines: > > > > int (*cmd)(struct cmd_tbl_s *, int, int, char * const []); at > > struct cmd_tbl_s, which is int. > > i don't understand what you're trying to say > > > > > + static char *s = "dfu"; > > > > > > no need to declare this static > > > > Why not? The s ptr is further passed to g_dnl_init(s), so my intend > > was to declare string in the data segment of this translation unit. > > In this way the data under this ptr will not vanish. > > except your g_dnl_init already copies the incoming string to local > storage, so where it is stored doesn't matter. further, the "dfu" is > going to be in .rodata (thus never going away), it is only the "s" > pointer which will be stored in .data, not the thing it points to. > > had you done: > char s[] = { 'd', 'f', 'u', '\0', }; > then you'd get 4 bytes on the stack Thanks for clarification Regards, Lukasz Majewski
diff --git a/common/Makefile b/common/Makefile index 6e23baa..de43ead 100644 --- a/common/Makefile +++ b/common/Makefile @@ -186,6 +186,7 @@ COBJS-$(CONFIG_MENU) += menu.o COBJS-$(CONFIG_MODEM_SUPPORT) += modem.o COBJS-$(CONFIG_UPDATE_TFTP) += update.o COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o +COBJS-$(CONFIG_CMD_DFU) += cmd_dfu.o endif ifdef CONFIG_SPL_BUILD diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c new file mode 100644 index 0000000..ceb0f54 --- /dev/null +++ b/common/cmd_dfu.c @@ -0,0 +1,81 @@ +/* + * cmd_dfu.c -- dfu command + * + * Copyright (C) 2012 Samsung Electronics + * authors: Andrzej Pietrasiewicz <andrzej.p@samsung.com> + * Lukasz Majewski <l.majewski@samsung.com> + * + * 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 <common.h> +#include <command.h> +#include <malloc.h> +#include <dfu.h> +#include <asm/errno.h> +#include <g_dnl.h> + +int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + char *str_env = NULL, *env_bkp = NULL; + static char *s = "dfu"; + int ret = 0; + + if (argc < 3) + return CMD_RET_USAGE; + + str_env = getenv("dfu_alt_info"); + if (str_env == NULL) { + printf("%s: \"dfu_alt_info\" env variable not defined!\n", + __func__); + return CMD_RET_FAILURE; + } + + env_bkp = strdup(str_env); + ret = dfu_config_entities(env_bkp, argv[1], + (int)simple_strtoul(argv[2], NULL, 10)); + if (ret) + return CMD_RET_FAILURE; + + if (strcmp(argv[3], "list") == 0) { + dfu_show_entities(); + dfu_free_entities(); + free(env_bkp); + return CMD_RET_SUCCESS; + } + + board_usb_init(); + g_dnl_init(s); + while (1) { + if (ctrlc()) + goto exit; + + usb_gadget_handle_interrupts(); + } +exit: + g_dnl_cleanup(); + dfu_free_entities(); + free(env_bkp); + + return CMD_RET_SUCCESS; +} + +U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu, + "Device Firmware Upgrade", + "<interface> <dev> [list]\n" + " - device firmware upgrade on a device <dev>\n" + " attached to interface <interface>\n" + " [list] - list available alt settings" +);