diff mbox

[U-Boot,v3,03/16] net: Add prototype for update_tftp, and use autoconf

Message ID 1361895069-7343-4-git-send-email-sjg@chromium.org
State Deferred
Delegated to: Tom Rini
Headers show

Commit Message

Simon Glass Feb. 26, 2013, 4:10 p.m. UTC 
This function should be declared in net.h. At the same time, let's use
autoconf instead of #ifdef for its inclusion.

Signed-off-by: Simon Glass <sjg@chromium.org>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3: None
Changes in v2: None

 common/cmd_fitupd.c |  3 +--
 common/main.c       |  9 ++-------
 common/update.c     | 24 ++++++++----------------
 include/net.h       |  3 +++
 4 files changed, 14 insertions(+), 25 deletions(-)

Comments

Tom Rini March 20, 2013, 7:40 p.m. UTC | #1 
On Tue, Feb 26, 2013 at 08:10:56AM -0800, Simon Glass wrote:
> This function should be declared in net.h. At the same time, let's use
> autoconf instead of #ifdef for its inclusion.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
[snip]
> @@ -266,12 +254,16 @@ int update_tftp(ulong addr)
>  	/* get load address of downloaded update file */
>  	if ((env_addr = getenv("loadaddr")) != NULL)
>  		addr = simple_strtoul(env_addr, NULL, 16);
> +	else if (autoconf_has_update_load_addr())
> +		addr = autoconf_update_load_addr();
>  	else
> -		addr = CONFIG_UPDATE_LOAD_ADDR;
> +		addr = 0x100000;
>  
> +	msec_max = autoconf_has_update_tftp_msec_max() ?
> +			autoconf_update_tftp_msec_max() : 100;
>  
> -	if (update_load(filename, CONFIG_UPDATE_TFTP_MSEC_MAX,
> -					CONFIG_UPDATE_TFTP_CNT_MAX, addr)) {
> +	if (update_load(filename, msec_max, autoconf_update_tftp_cnt_max(),
> +		addr)) {

This doesn't read nearly as clean to me as the old code.  Part of the
problem is that we really need a way to foce an CONFIG option to be set
to something and give a default (so, the Kconfig switch-over).  Now, in
cases like this it the compiler smart enough to say "oh, msec_max is a
constant, lets not waste the space on a variable" ?  Would const'ing
that help or confuse things would be a follow up question.
Simon Glass March 20, 2013, 7:57 p.m. UTC | #2 
Hi Tom,

On Wed, Mar 20, 2013 at 12:40 PM, Tom Rini <trini@ti.com> wrote:
> On Tue, Feb 26, 2013 at 08:10:56AM -0800, Simon Glass wrote:
>> This function should be declared in net.h. At the same time, let's use
>> autoconf instead of #ifdef for its inclusion.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> [snip]
>> @@ -266,12 +254,16 @@ int update_tftp(ulong addr)
>>       /* get load address of downloaded update file */
>>       if ((env_addr = getenv("loadaddr")) != NULL)
>>               addr = simple_strtoul(env_addr, NULL, 16);
>> +     else if (autoconf_has_update_load_addr())
>> +             addr = autoconf_update_load_addr();
>>       else
>> -             addr = CONFIG_UPDATE_LOAD_ADDR;
>> +             addr = 0x100000;
>>
>> +     msec_max = autoconf_has_update_tftp_msec_max() ?
>> +                     autoconf_update_tftp_msec_max() : 100;
>>
>> -     if (update_load(filename, CONFIG_UPDATE_TFTP_MSEC_MAX,
>> -                                     CONFIG_UPDATE_TFTP_CNT_MAX, addr)) {
>> +     if (update_load(filename, msec_max, autoconf_update_tftp_cnt_max(),
>> +             addr)) {
>
> This doesn't read nearly as clean to me as the old code.  Part of the
> problem is that we really need a way to foce an CONFIG option to be set
> to something and give a default (so, the Kconfig switch-over).  Now, in
> cases like this it the compiler smart enough to say "oh, msec_max is a
> constant, lets not waste the space on a variable" ?  Would const'ing
> that help or confuse things would be a follow up question.

Yes I believe the compiler does the right thing here. Even if there is
a 'variable' it would only be in a register.

const can be used, but I don't think it would do anything in this case.

In reading the old code, you need to look at the top of the file,
where some code was removed, and take that into account.

-/* set configuration defaults if needed */
-#ifndef CONFIG_UPDATE_LOAD_ADDR
-#define CONFIG_UPDATE_LOAD_ADDR        0x100000
-#endif
-
-#ifndef CONFIG_UPDATE_TFTP_MSEC_MAX
-#define CONFIG_UPDATE_TFTP_MSEC_MAX    100
-#endif
-
-#ifndef CONFIG_UPDATE_TFTP_CNT_MAX
-#define CONFIG_UPDATE_TFTP_CNT_MAX     0
-#endif
-

Regards,
Simon
Tom Rini March 20, 2013, 8:30 p.m. UTC | #3 
On Wed, Mar 20, 2013 at 12:57:37PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, Mar 20, 2013 at 12:40 PM, Tom Rini <trini@ti.com> wrote:
> > On Tue, Feb 26, 2013 at 08:10:56AM -0800, Simon Glass wrote:
> >> This function should be declared in net.h. At the same time, let's use
> >> autoconf instead of #ifdef for its inclusion.
> >>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> > [snip]
> >> @@ -266,12 +254,16 @@ int update_tftp(ulong addr)
> >>       /* get load address of downloaded update file */
> >>       if ((env_addr = getenv("loadaddr")) != NULL)
> >>               addr = simple_strtoul(env_addr, NULL, 16);
> >> +     else if (autoconf_has_update_load_addr())
> >> +             addr = autoconf_update_load_addr();
> >>       else
> >> -             addr = CONFIG_UPDATE_LOAD_ADDR;
> >> +             addr = 0x100000;

OK, this in particular would looke a little better as:
addr = autoconf_has_update_load_addr() ?
	     autoconf_update_load_addr() : 0x100000;
if ((env_addr = ...)

Set the addr to the default, then override.

> >>
> >> +     msec_max = autoconf_has_update_tftp_msec_max() ?
> >> +                     autoconf_update_tftp_msec_max() : 100;
> >>
> >> -     if (update_load(filename, CONFIG_UPDATE_TFTP_MSEC_MAX,
> >> -                                     CONFIG_UPDATE_TFTP_CNT_MAX, addr)) {
> >> +     if (update_load(filename, msec_max, autoconf_update_tftp_cnt_max(),
> >> +             addr)) {
> >
> > This doesn't read nearly as clean to me as the old code.  Part of the
> > problem is that we really need a way to foce an CONFIG option to be set
> > to something and give a default (so, the Kconfig switch-over).  Now, in
> > cases like this it the compiler smart enough to say "oh, msec_max is a
> > constant, lets not waste the space on a variable" ?  Would const'ing
> > that help or confuse things would be a follow up question.
> 
> Yes I believe the compiler does the right thing here. Even if there is
> a 'variable' it would only be in a register.
> 
> const can be used, but I don't think it would do anything in this case.

OK.

> In reading the old code, you need to look at the top of the file,
> where some code was removed, and take that into account.
> 
> -/* set configuration defaults if needed */
> -#ifndef CONFIG_UPDATE_LOAD_ADDR
> -#define CONFIG_UPDATE_LOAD_ADDR        0x100000
> -#endif
> -
> -#ifndef CONFIG_UPDATE_TFTP_MSEC_MAX
> -#define CONFIG_UPDATE_TFTP_MSEC_MAX    100
> -#endif
> -
> -#ifndef CONFIG_UPDATE_TFTP_CNT_MAX
> -#define CONFIG_UPDATE_TFTP_CNT_MAX     0
> -#endif

That's what I mean.  We have defaults that can be overriden and we use
the value simply.  This was a decent way of saying "we know most people
will use X as the value, but allow for override".  Now we have to do
if (autoconf_has_foo())
  x = autoconf_foo()
else
  x = default;

Or:
x = autoconf_has_foo() ? autoconf_foo() : default;
Which usually spills out into 2 lines and isn't my favorite construct
ever.  Lets see where I'm at after going over the whole series.
Simon Glass March 21, 2013, 1:47 a.m. UTC | #4 
Hi Tom,

On Wed, Mar 20, 2013 at 1:30 PM, Tom Rini <trini@ti.com> wrote:
> On Wed, Mar 20, 2013 at 12:57:37PM -0700, Simon Glass wrote:
>> Hi Tom,
>>
>> On Wed, Mar 20, 2013 at 12:40 PM, Tom Rini <trini@ti.com> wrote:
>> > On Tue, Feb 26, 2013 at 08:10:56AM -0800, Simon Glass wrote:
>> >> This function should be declared in net.h. At the same time, let's use
>> >> autoconf instead of #ifdef for its inclusion.
>> >>
>> >> Signed-off-by: Simon Glass <sjg@chromium.org>
>> >> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>> > [snip]
>> >> @@ -266,12 +254,16 @@ int update_tftp(ulong addr)
>> >>       /* get load address of downloaded update file */
>> >>       if ((env_addr = getenv("loadaddr")) != NULL)
>> >>               addr = simple_strtoul(env_addr, NULL, 16);
>> >> +     else if (autoconf_has_update_load_addr())
>> >> +             addr = autoconf_update_load_addr();
>> >>       else
>> >> -             addr = CONFIG_UPDATE_LOAD_ADDR;
>> >> +             addr = 0x100000;
>
> OK, this in particular would looke a little better as:
> addr = autoconf_has_update_load_addr() ?
>              autoconf_update_load_addr() : 0x100000;
> if ((env_addr = ...)

Yes, agreed.

>
> Set the addr to the default, then override.
>
>> >>
>> >> +     msec_max = autoconf_has_update_tftp_msec_max() ?
>> >> +                     autoconf_update_tftp_msec_max() : 100;
>> >>
>> >> -     if (update_load(filename, CONFIG_UPDATE_TFTP_MSEC_MAX,
>> >> -                                     CONFIG_UPDATE_TFTP_CNT_MAX, addr)) {
>> >> +     if (update_load(filename, msec_max, autoconf_update_tftp_cnt_max(),
>> >> +             addr)) {
>> >
>> > This doesn't read nearly as clean to me as the old code.  Part of the
>> > problem is that we really need a way to foce an CONFIG option to be set
>> > to something and give a default (so, the Kconfig switch-over).  Now, in
>> > cases like this it the compiler smart enough to say "oh, msec_max is a
>> > constant, lets not waste the space on a variable" ?  Would const'ing
>> > that help or confuse things would be a follow up question.
>>
>> Yes I believe the compiler does the right thing here. Even if there is
>> a 'variable' it would only be in a register.
>>
>> const can be used, but I don't think it would do anything in this case.
>
> OK.
>
>> In reading the old code, you need to look at the top of the file,
>> where some code was removed, and take that into account.
>>
>> -/* set configuration defaults if needed */
>> -#ifndef CONFIG_UPDATE_LOAD_ADDR
>> -#define CONFIG_UPDATE_LOAD_ADDR        0x100000
>> -#endif
>> -
>> -#ifndef CONFIG_UPDATE_TFTP_MSEC_MAX
>> -#define CONFIG_UPDATE_TFTP_MSEC_MAX    100
>> -#endif
>> -
>> -#ifndef CONFIG_UPDATE_TFTP_CNT_MAX
>> -#define CONFIG_UPDATE_TFTP_CNT_MAX     0
>> -#endif
>
> That's what I mean.  We have defaults that can be overriden and we use
> the value simply.  This was a decent way of saying "we know most people
> will use X as the value, but allow for override".  Now we have to do
> if (autoconf_has_foo())
>   x = autoconf_foo()
> else
>   x = default;
>
> Or:
> x = autoconf_has_foo() ? autoconf_foo() : default;
> Which usually spills out into 2 lines and isn't my favorite construct
> ever.  Lets see where I'm at after going over the whole series.

OK, in fact we need to set a way of doing this, and also whether in
some cases we are better off with #ifdefs. My main dislikes of #ifdef
were mentioned in the cover letter, but they do have their place.

Regards,
Simon
diff mbox

Patch

diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c
index 7a3789e..618ff7c 100644
--- a/common/cmd_fitupd.c
+++ b/common/cmd_fitupd.c
@@ -8,13 +8,12 @@ 
 
 #include <common.h>
 #include <command.h>
+#include <net.h>
 
 #if !defined(CONFIG_UPDATE_TFTP)
 #error "CONFIG_UPDATE_TFTP required"
 #endif
 
-extern int update_tftp(ulong addr);
-
 static int do_fitupd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	ulong addr = 0UL;
diff --git a/common/main.c b/common/main.c
index e2d2e09..2b8af2c 100644
--- a/common/main.c
+++ b/common/main.c
@@ -61,10 +61,6 @@  DECLARE_GLOBAL_DATA_PTR;
 void inline __show_boot_progress (int val) {}
 void show_boot_progress (int val) __attribute__((weak, alias("__show_boot_progress")));
 
-#if defined(CONFIG_UPDATE_TFTP)
-int update_tftp (ulong addr);
-#endif /* CONFIG_UPDATE_TFTP */
-
 #define MAX_DELAY_STOP_STR 32
 
 #undef DEBUG_PARSER
@@ -427,9 +423,8 @@  void main_loop (void)
 	}
 #endif /* CONFIG_PREBOOT */
 
-#if defined(CONFIG_UPDATE_TFTP)
-	update_tftp (0UL);
-#endif /* CONFIG_UPDATE_TFTP */
+	if (autoconf_update_tftp())
+		update_tftp(0UL);
 
 #if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
 	s = getenv ("bootdelay");
diff --git a/common/update.c b/common/update.c
index 94d6a82..9cd9ca2 100644
--- a/common/update.c
+++ b/common/update.c
@@ -43,19 +43,6 @@ 
 /* env variable holding the location of the update file */
 #define UPDATE_FILE_ENV		"updatefile"
 
-/* set configuration defaults if needed */
-#ifndef CONFIG_UPDATE_LOAD_ADDR
-#define CONFIG_UPDATE_LOAD_ADDR	0x100000
-#endif
-
-#ifndef CONFIG_UPDATE_TFTP_MSEC_MAX
-#define CONFIG_UPDATE_TFTP_MSEC_MAX	100
-#endif
-
-#ifndef CONFIG_UPDATE_TFTP_CNT_MAX
-#define CONFIG_UPDATE_TFTP_CNT_MAX	0
-#endif
-
 extern ulong TftpRRQTimeoutMSecs;
 extern int TftpRRQTimeoutCountMax;
 extern flash_info_t flash_info[];
@@ -244,6 +231,7 @@  int update_tftp(ulong addr)
 	char *filename, *env_addr;
 	int images_noffset, ndepth, noffset;
 	ulong update_addr, update_fladdr, update_size;
+	int msec_max;
 	void *fit;
 	int ret = 0;
 
@@ -266,12 +254,16 @@  int update_tftp(ulong addr)
 	/* get load address of downloaded update file */
 	if ((env_addr = getenv("loadaddr")) != NULL)
 		addr = simple_strtoul(env_addr, NULL, 16);
+	else if (autoconf_has_update_load_addr())
+		addr = autoconf_update_load_addr();
 	else
-		addr = CONFIG_UPDATE_LOAD_ADDR;
+		addr = 0x100000;
 
+	msec_max = autoconf_has_update_tftp_msec_max() ?
+			autoconf_update_tftp_msec_max() : 100;
 
-	if (update_load(filename, CONFIG_UPDATE_TFTP_MSEC_MAX,
-					CONFIG_UPDATE_TFTP_CNT_MAX, addr)) {
+	if (update_load(filename, msec_max, autoconf_update_tftp_cnt_max(),
+		addr)) {
 		printf("Can't load update file, aborting auto-update\n");
 		return 1;
 	}
diff --git a/include/net.h b/include/net.h
index 970d4d1..23fb947 100644
--- a/include/net.h
+++ b/include/net.h
@@ -695,6 +695,9 @@  extern void copy_filename(char *dst, const char *src, int size);
 /* get a random source port */
 extern unsigned int random_port(void);
 
+/* Update U-Boot over TFTP */
+extern int update_tftp(ulong addr);
+
 /**********************************************************************/
 
 #endif /* __NET_H__ */