Patchwork [U-Boot,5/7] dfu:cmd: Support for DFU u-boot command

login
register
mail settings
Submitter Łukasz Majewski
Date July 3, 2012, 9:38 a.m.
Message ID <1341308291-14663-6-git-send-email-l.majewski@samsung.com>
Download mbox | patch
Permalink /patch/168753/
State Changes Requested
Headers show

Comments

Łukasz Majewski - July 3, 2012, 9:38 a.m.
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>
---
 common/Makefile  |    1 +
 common/cmd_dfu.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 0 deletions(-)
 create mode 100644 common/cmd_dfu.c
Marek Vasut - July 3, 2012, 9:32 p.m.
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
Łukasz Majewski - July 4, 2012, 9:28 a.m.
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
Marek Vasut - July 4, 2012, 2:39 p.m.
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
Mike Frysinger - July 20, 2012, 4:20 a.m.
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
Mike Frysinger - July 20, 2012, 4:22 a.m.
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
Mike Frysinger - July 20, 2012, 4:23 a.m.
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
Marek Vasut - July 20, 2012, 11:33 a.m.
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
Marek Vasut - July 20, 2012, 11:35 a.m.
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
Mike Frysinger - July 20, 2012, 2:43 p.m.
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
Marek Vasut - July 20, 2012, 9:11 p.m.
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
Mike Frysinger - July 21, 2012, 5:20 p.m.
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
Marek Vasut - July 21, 2012, 5:21 p.m.
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
Łukasz Majewski - July 23, 2012, 4:01 p.m.
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
Mike Frysinger - July 24, 2012, 6 p.m.
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
Lukasz Majewski - July 24, 2012, 8:48 p.m.
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

Patch

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"
+);