[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Joe Hershberger]

Because the code that handles bootdelay is compiled in conditionally
based on the default value, you are restricted in the default,
regardless of what you want the runtime options to be.

Change the source to always check if any default is given so that other
values can be selected and used at runtime.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---
 common/main.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/common/main.c b/common/main.c
index e2d2e09..0973c59 100644
--- a/common/main.c
+++ b/common/main.c
@@ -95,7 +95,7 @@ extern void mdm_init(void); /* defined in board.c */
  * Watch for 'delay' seconds for autoboot stop or autoboot delay string.
  * returns: 0 -  no key string, allow autoboot 1 - got key string, abort
  */
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
+#if defined(CONFIG_BOOTDELAY)
 # if defined(CONFIG_AUTOBOOT_KEYED)
 #ifndef CONFIG_MENU
 static inline
@@ -279,7 +279,7 @@ int abortboot(int bootdelay)
 	return abort;
 }
 # endif	/* CONFIG_AUTOBOOT_KEYED */
-#endif	/* CONFIG_BOOTDELAY >= 0  */
+#endif	/* CONFIG_BOOTDELAY */
 
 /*
  * Runs the given boot command securely.  Specifically:
@@ -295,8 +295,7 @@ int abortboot(int bootdelay)
  * printing the error message to console.
  */
 
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \
-	defined(CONFIG_OF_CONTROL)
+#if defined(CONFIG_BOOTDELAY) && defined(CONFIG_OF_CONTROL)
 static void secure_boot_cmd(char *cmd)
 {
 	cmd_tbl_t *cmdtp;
@@ -358,11 +357,10 @@ void main_loop (void)
 	int rc = 1;
 	int flag;
 #endif
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \
-		defined(CONFIG_OF_CONTROL)
+#if defined(CONFIG_BOOTDELAY) && defined(CONFIG_OF_CONTROL)
 	char *env;
 #endif
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
+#if defined(CONFIG_BOOTDELAY)
 	char *s;
 	int bootdelay;
 #endif
@@ -431,7 +429,7 @@ void main_loop (void)
 	update_tftp (0UL);
 #endif /* CONFIG_UPDATE_TFTP */
 
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
+#if defined(CONFIG_BOOTDELAY)
 	s = getenv ("bootdelay");
 	bootdelay = s ? (int)simple_strtol(s, NULL, 10) : CONFIG_BOOTDELAY;
 
-- 
1.7.11.5

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Wolfgang Denk]

Dear Joe Hershberger,

In message <1360355280-1197-1-git-send-email-joe.hershberger@ni.com> you wrote:
> Because the code that handles bootdelay is compiled in conditionally
> based on the default value, you are restricted in the default,
> regardless of what you want the runtime options to be.
> 
> Change the source to always check if any default is given so that other
> values can be selected and used at runtime.
> 
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>  common/main.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/common/main.c b/common/main.c
> index e2d2e09..0973c59 100644
> --- a/common/main.c
> +++ b/common/main.c
> @@ -95,7 +95,7 @@ extern void mdm_init(void); /* defined in board.c */
>   * Watch for 'delay' seconds for autoboot stop or autoboot delay string.
>   * returns: 0 -  no key string, allow autoboot 1 - got key string, abort
>   */
> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
> +#if defined(CONFIG_BOOTDELAY)

Careful!! This is probably changing behaviour of a number of boards
significantly.

we have to check if we really want this, and if yes, we have to
announce it and provide a grace period (eventually using
doc/feature-removal-schedule.txt ?)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
                  Nail here --X-- for new monitor.

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Otavio Salvador]

Hello Wolfgang,

On Sat, Feb 9, 2013 at 4:54 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Joe Hershberger,
>
> In message <1360355280-1197-1-git-send-email-joe.hershberger@ni.com> you wrote:
>> Because the code that handles bootdelay is compiled in conditionally
>> based on the default value, you are restricted in the default,
>> regardless of what you want the runtime options to be.
>>
>> Change the source to always check if any default is given so that other
>> values can be selected and used at runtime.
>>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> ---
>>  common/main.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/common/main.c b/common/main.c
>> index e2d2e09..0973c59 100644
>> --- a/common/main.c
>> +++ b/common/main.c
>> @@ -95,7 +95,7 @@ extern void mdm_init(void); /* defined in board.c */
>>   * Watch for 'delay' seconds for autoboot stop or autoboot delay string.
>>   * returns: 0 -  no key string, allow autoboot 1 - got key string, abort
>>   */
>> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
>> +#if defined(CONFIG_BOOTDELAY)
>
> Careful!! This is probably changing behaviour of a number of boards
> significantly.
>
> we have to check if we really want this, and if yes, we have to
> announce it and provide a grace period (eventually using
> doc/feature-removal-schedule.txt ?)

It seems the CONFIG_BOOTDELAY as < 0 is not very common:

~/hacking/u-boot% git grep CONFIG_BOOTDELAY | egrep 'BOOTDELAY\s* \-[0-9]'
include/configs/RPXsuper.h:#define CONFIG_BOOTDELAY        -1
include/configs/ep8260.h:#define CONFIG_BOOTDELAY        -1
include/configs/espt.h:#define CONFIG_BOOTDELAY        -1
include/configs/scb9328.h:#define CONFIG_BOOTDELAY   -1
include/configs/sh7763rdp.h:#define CONFIG_BOOTDELAY        -1

So maybe those could have CONFIG_BOOTDELAY undefined keeping them
working as before?


--
Otavio Salvador                             O.S. Systems
E-mail: otavio@ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Wolfgang Denk]

Dear Otavio Salvador,

In message <CAP9ODKqs5Fsw+NVQzNvm6+4VVK6jfC1vD6eZQfumdKvxzCqsjQ@mail.gmail.com> you wrote:
> 
> > Careful!! This is probably changing behaviour of a number of boards
> > significantly.
> >
> > we have to check if we really want this, and if yes, we have to
> > announce it and provide a grace period (eventually using
> > doc/feature-removal-schedule.txt ?)
> 
> It seems the CONFIG_BOOTDELAY as < 0 is not very common:

You do not know all the many out-of-tree ports...

[OK, we don't care much about these, true...]

> ~/hacking/u-boot% git grep CONFIG_BOOTDELAY | egrep 'BOOTDELAY\s* \-[0-9]'
> include/configs/RPXsuper.h:#define CONFIG_BOOTDELAY        -1
> include/configs/ep8260.h:#define CONFIG_BOOTDELAY        -1
> include/configs/espt.h:#define CONFIG_BOOTDELAY        -1
> include/configs/scb9328.h:#define CONFIG_BOOTDELAY   -1
> include/configs/sh7763rdp.h:#define CONFIG_BOOTDELAY        -1
> 
> So maybe those could have CONFIG_BOOTDELAY undefined keeping them
> working as before?

Yes, they could.  But it is a change of a long-standing behaviour, and
we try to avoid breaking existing boards.  At minimum this needs to be
announced, the respective board maintainers need to be modified, and
given sufficient time to react.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"UNIX was not designed to stop you from doing stupid things,  because
that would also stop you from doing clever things."       - Doug Gwyn

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Otavio Salvador]

On Sun, Feb 17, 2013 at 5:18 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Otavio Salvador,
>
> In message <CAP9ODKqs5Fsw+NVQzNvm6+4VVK6jfC1vD6eZQfumdKvxzCqsjQ@mail.gmail.com> you wrote:
>>
>> > Careful!! This is probably changing behaviour of a number of boards
>> > significantly.
>> >
>> > we have to check if we really want this, and if yes, we have to
>> > announce it and provide a grace period (eventually using
>> > doc/feature-removal-schedule.txt ?)
>>
>> It seems the CONFIG_BOOTDELAY as < 0 is not very common:
>
> You do not know all the many out-of-tree ports...
>
> [OK, we don't care much about these, true...]
>
>> ~/hacking/u-boot% git grep CONFIG_BOOTDELAY | egrep 'BOOTDELAY\s* \-[0-9]'
>> include/configs/RPXsuper.h:#define CONFIG_BOOTDELAY        -1
>> include/configs/ep8260.h:#define CONFIG_BOOTDELAY        -1
>> include/configs/espt.h:#define CONFIG_BOOTDELAY        -1
>> include/configs/scb9328.h:#define CONFIG_BOOTDELAY   -1
>> include/configs/sh7763rdp.h:#define CONFIG_BOOTDELAY        -1
>>
>> So maybe those could have CONFIG_BOOTDELAY undefined keeping them
>> working as before?
>
> Yes, they could.  But it is a change of a long-standing behaviour, and
> we try to avoid breaking existing boards.  At minimum this needs to be
> announced, the respective board maintainers need to be modified, and
> given sufficient time to react.

Yes; I agree but breaking out of tree boards ... well, it is the price
of not working upstream so I think it cannot be a blocker for U-Boot
improvements and cleanups.

-- 
Otavio Salvador                             O.S. Systems
E-mail: otavio at ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Wolfgang Denk]

Dear Otavio Salvador,

In message <CAP9ODKr3xaU_Vg7YgjYOsntD03YvLaygRdSZC-Y4jQgDi+D7sw@mail.gmail.com> you wrote:
>
> >> include/configs/RPXsuper.h:#define CONFIG_BOOTDELAY        -1
> >> include/configs/ep8260.h:#define CONFIG_BOOTDELAY        -1
> >> include/configs/espt.h:#define CONFIG_BOOTDELAY        -1
> >> include/configs/scb9328.h:#define CONFIG_BOOTDELAY   -1
> >> include/configs/sh7763rdp.h:#define CONFIG_BOOTDELAY        -1
> >>
> >> So maybe those could have CONFIG_BOOTDELAY undefined keeping them
> >> working as before?
> >
> > Yes, they could.  But it is a change of a long-standing behaviour, and
> > we try to avoid breaking existing boards.  At minimum this needs to be
> > announced, the respective board maintainers need to be modified, and
> > given sufficient time to react.
> 
> Yes; I agree but breaking out of tree boards ... well, it is the price
> of not working upstream so I think it cannot be a blocker for U-Boot
> improvements and cleanups.

You also change behaviour (and thus potentially break) at least 5
boards in mainline.  See also [1] about breaking user space in Linux
(which is pretty much equivalent to what would happen here).

[1] http://article.gmane.org/gmane.linux.kernel/1414106

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"'Tis true, 'tis pity, and pity 'tis 'tis true."
    - Poloniouius, in Willie the Shake's _Hamlet, Prince of Darkness_

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Otavio Salvador]

On Sun, Feb 17, 2013 at 6:02 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Otavio Salvador,
>
> In message <CAP9ODKr3xaU_Vg7YgjYOsntD03YvLaygRdSZC-Y4jQgDi+D7sw@mail.gmail.com> you wrote:
>>
>> >> include/configs/RPXsuper.h:#define CONFIG_BOOTDELAY        -1
>> >> include/configs/ep8260.h:#define CONFIG_BOOTDELAY        -1
>> >> include/configs/espt.h:#define CONFIG_BOOTDELAY        -1
>> >> include/configs/scb9328.h:#define CONFIG_BOOTDELAY   -1
>> >> include/configs/sh7763rdp.h:#define CONFIG_BOOTDELAY        -1
>> >>
>> >> So maybe those could have CONFIG_BOOTDELAY undefined keeping them
>> >> working as before?
>> >
>> > Yes, they could.  But it is a change of a long-standing behaviour, and
>> > we try to avoid breaking existing boards.  At minimum this needs to be
>> > announced, the respective board maintainers need to be modified, and
>> > given sufficient time to react.
>>
>> Yes; I agree but breaking out of tree boards ... well, it is the price
>> of not working upstream so I think it cannot be a blocker for U-Boot
>> improvements and cleanups.
>
> You also change behaviour (and thus potentially break) at least 5
> boards in mainline.  See also [1] about breaking user space in Linux
> (which is pretty much equivalent to what would happen here).
>
> [1] http://article.gmane.org/gmane.linux.kernel/1414106

I see it different; I understand we cannot break current boards so a
patch should also convert the boards to the new behavior.

I also don't see a compare it to user space in Linux fair, I think
it'd be more like a driver API change in Linux and Linux breaks API
for out of tree drivers very ofthen so I think same fits here.

-- 
Otavio Salvador                             O.S. Systems
E-mail: otavio at ossystems.com.br  http://www.ossystems.com.br
Mobile: +55 53 9981-7854              http://projetos.ossystems.com.br

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/09/2013 11:21 AM, Otavio Salvador wrote:
> Hello Wolfgang,
> 
> On Sat, Feb 9, 2013 at 4:54 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Joe Hershberger,
>> 
>> In message
>> <1360355280-1197-1-git-send-email-joe.hershberger@ni.com> you
>> wrote:
>>> Because the code that handles bootdelay is compiled in
>>> conditionally based on the default value, you are restricted in
>>> the default, regardless of what you want the runtime options to
>>> be.
>>> 
>>> Change the source to always check if any default is given so
>>> that other values can be selected and used at runtime.
>>> 
>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> --- 
>>> common/main.c | 14 ++++++-------- 1 file changed, 6
>>> insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/common/main.c b/common/main.c index
>>> e2d2e09..0973c59 100644 --- a/common/main.c +++
>>> b/common/main.c @@ -95,7 +95,7 @@ extern void mdm_init(void);
>>> /* defined in board.c */ * Watch for 'delay' seconds for
>>> autoboot stop or autoboot delay string. * returns: 0 -  no key
>>> string, allow autoboot 1 - got key string, abort */ -#if
>>> defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) +#if
>>> defined(CONFIG_BOOTDELAY)
>> 
>> Careful!! This is probably changing behaviour of a number of
>> boards significantly.
>> 
>> we have to check if we really want this, and if yes, we have to 
>> announce it and provide a grace period (eventually using 
>> doc/feature-removal-schedule.txt ?)
> 
> It seems the CONFIG_BOOTDELAY as < 0 is not very common:
> 
> ~/hacking/u-boot% git grep CONFIG_BOOTDELAY | egrep 'BOOTDELAY\s*
> \-[0-9]' include/configs/RPXsuper.h:#define CONFIG_BOOTDELAY
> -1 include/configs/ep8260.h:#define CONFIG_BOOTDELAY        -1 
> include/configs/espt.h:#define CONFIG_BOOTDELAY        -1 
> include/configs/scb9328.h:#define CONFIG_BOOTDELAY   -1 
> include/configs/sh7763rdp.h:#define CONFIG_BOOTDELAY        -1

I count 49 boards with git grep -E
'CONFIG_BOOTDELAY[[:blank:]]+-[0-9]' so it's not _that_ uncommon.

> So maybe those could have CONFIG_BOOTDELAY undefined keeping them 
> working as before?

The problem is that as I read the README, we document CONFIG_BOOTDELAY
as having a valid value range of from -2 to sane positive value.  So
yes, if we want to change this we need to (a) change the README too
and (b) give some sort of heads-up.

Off the top of my head, we could change to:
CONFIG_AUTOBOOT_DISABLED and CONFIG_FORCE_AUTOBOOT_NO_DELAY and update
doc/README.autoboot as well and add some sanity #warning checks for a
release or two in some common generated config related file or
something like that.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRImLoAAoJENk4IS6UOR1W78kP/1xHVrQ4pZbAV8WnBsxje+gn
qphEJU5Ud+gA5ThyEm/I1d1VSSvLgOWGqnarleOSXUmcWkBcbfetO69VVLnvlUI+
oVXEopn7plCO7iM2YAiOW8vx5fy97JqGxWn1/BJ64ySjup5GlEXd97Op9LQ+wjy7
xNgFQ1KN9wdoabQU2PUy7jeGXY8LTdFx4GsYSK/KRJWIvo3O57c2uOsE2BiA2Uxq
Ue780cVqqsoRmZxo2gooAUTupz3kPYmFthPn2qxs1y3nLn4GVrJM5YurOhH2weeA
sHOffHqlUn1kUxrhRnM6mpeu+JUeKFP8IoyOuhCYA7D28Bk1XtqkZai+yobOrf3U
cr4WUdhZEA9hFZAXnlrVe/FjVLe6pvQ9hPjUshPoCerahrcpUOwiegrw6IWg+vzB
DdK5hbPh09rTdpfnjWWRf0fSrqQqaXmNQALmRQOc4G1Vn8xKruC4vacetdNRfgCn
gFOSTpbDBG7oKFXHpCzliXEg8lFy+af+oMKTztR/UtPrBX5cfr6XF2SekNm9jus4
Njjq1ouYKXfOyeNIrg0prOi14ZMF3uQYb/eUsffzeujdmbhcgENopUrGLl9FvlxV
bsktw7oWhgirVZ5CsODk/3siDfJc1pi9yz3oMO61Qqtrrv3HLLjGIFEx4DjxNmkk
COfH7og2vh8xQavT8IXY
=oTlx
-----END PGP SIGNATURE-----

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Joe Hershberger]

Hi Tom,

On Mon, Feb 18, 2013 at 11:20 AM, Tom Rini <trini@ti.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 02/09/2013 11:21 AM, Otavio Salvador wrote:
>> Hello Wolfgang,
>>
>> On Sat, Feb 9, 2013 at 4:54 AM, Wolfgang Denk <wd@denx.de> wrote:
>>> Dear Joe Hershberger,
>>>
>>> In message
>>> <1360355280-1197-1-git-send-email-joe.hershberger@ni.com> you
>>> wrote:
>>>> Because the code that handles bootdelay is compiled in
>>>> conditionally based on the default value, you are restricted in
>>>> the default, regardless of what you want the runtime options to
>>>> be.
>>>>
>>>> Change the source to always check if any default is given so
>>>> that other values can be selected and used at runtime.
>>>>
>>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> ---
>>>> common/main.c | 14 ++++++-------- 1 file changed, 6
>>>> insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/common/main.c b/common/main.c index
>>>> e2d2e09..0973c59 100644 --- a/common/main.c +++
>>>> b/common/main.c @@ -95,7 +95,7 @@ extern void mdm_init(void);
>>>> /* defined in board.c */ * Watch for 'delay' seconds for
>>>> autoboot stop or autoboot delay string. * returns: 0 -  no key
>>>> string, allow autoboot 1 - got key string, abort */ -#if
>>>> defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) +#if
>>>> defined(CONFIG_BOOTDELAY)
>>>
>>> Careful!! This is probably changing behaviour of a number of
>>> boards significantly.
>>>
>>> we have to check if we really want this, and if yes, we have to
>>> announce it and provide a grace period (eventually using
>>> doc/feature-removal-schedule.txt ?)
>>
>> It seems the CONFIG_BOOTDELAY as < 0 is not very common:
>>
>> ~/hacking/u-boot% git grep CONFIG_BOOTDELAY | egrep 'BOOTDELAY\s*
>> \-[0-9]' include/configs/RPXsuper.h:#define CONFIG_BOOTDELAY
>> -1 include/configs/ep8260.h:#define CONFIG_BOOTDELAY        -1
>> include/configs/espt.h:#define CONFIG_BOOTDELAY        -1
>> include/configs/scb9328.h:#define CONFIG_BOOTDELAY   -1
>> include/configs/sh7763rdp.h:#define CONFIG_BOOTDELAY        -1
>
> I count 49 boards with git grep -E
> 'CONFIG_BOOTDELAY[[:blank:]]+-[0-9]' so it's not _that_ uncommon.
>
>> So maybe those could have CONFIG_BOOTDELAY undefined keeping them
>> working as before?
>
> The problem is that as I read the README, we document CONFIG_BOOTDELAY
> as having a valid value range of from -2 to sane positive value.  So
> yes, if we want to change this we need to (a) change the README too
> and (b) give some sort of heads-up.
>
> Off the top of my head, we could change to:
> CONFIG_AUTOBOOT_DISABLED and CONFIG_FORCE_AUTOBOOT_NO_DELAY and update
> doc/README.autoboot as well and add some sanity #warning checks for a
> release or two in some common generated config related file or
> something like that.

The change has no effect on the behavior, it simply changes what is
compiled in.  If the default value is not changed, then the behavior
is unchanged.  The only impact this may have on existing boards is to
make the code size slightly larger.  If that is an issue for some
reason, then CONFIG_BOOTDELAY can be undefined.

If we want to add other config options, that's fine, but that's more
involved that I was hoping for.

Thanks,
-Joe

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Tom Rini]

On Wed, Feb 27, 2013 at 02:11:31PM -0600, Joe Hershberger wrote:
> Hi Tom,
> 
> On Mon, Feb 18, 2013 at 11:20 AM, Tom Rini <trini@ti.com> wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > On 02/09/2013 11:21 AM, Otavio Salvador wrote:
> >> Hello Wolfgang,
> >>
> >> On Sat, Feb 9, 2013 at 4:54 AM, Wolfgang Denk <wd@denx.de> wrote:
> >>> Dear Joe Hershberger,
> >>>
> >>> In message
> >>> <1360355280-1197-1-git-send-email-joe.hershberger@ni.com> you
> >>> wrote:
> >>>> Because the code that handles bootdelay is compiled in
> >>>> conditionally based on the default value, you are restricted in
> >>>> the default, regardless of what you want the runtime options to
> >>>> be.
> >>>>
> >>>> Change the source to always check if any default is given so
> >>>> that other values can be selected and used at runtime.
> >>>>
> >>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> ---
> >>>> common/main.c | 14 ++++++-------- 1 file changed, 6
> >>>> insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/common/main.c b/common/main.c index
> >>>> e2d2e09..0973c59 100644 --- a/common/main.c +++
> >>>> b/common/main.c @@ -95,7 +95,7 @@ extern void mdm_init(void);
> >>>> /* defined in board.c */ * Watch for 'delay' seconds for
> >>>> autoboot stop or autoboot delay string. * returns: 0 -  no key
> >>>> string, allow autoboot 1 - got key string, abort */ -#if
> >>>> defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) +#if
> >>>> defined(CONFIG_BOOTDELAY)
> >>>
> >>> Careful!! This is probably changing behaviour of a number of
> >>> boards significantly.
> >>>
> >>> we have to check if we really want this, and if yes, we have to
> >>> announce it and provide a grace period (eventually using
> >>> doc/feature-removal-schedule.txt ?)
> >>
> >> It seems the CONFIG_BOOTDELAY as < 0 is not very common:
> >>
> >> ~/hacking/u-boot% git grep CONFIG_BOOTDELAY | egrep 'BOOTDELAY\s*
> >> \-[0-9]' include/configs/RPXsuper.h:#define CONFIG_BOOTDELAY
> >> -1 include/configs/ep8260.h:#define CONFIG_BOOTDELAY        -1
> >> include/configs/espt.h:#define CONFIG_BOOTDELAY        -1
> >> include/configs/scb9328.h:#define CONFIG_BOOTDELAY   -1
> >> include/configs/sh7763rdp.h:#define CONFIG_BOOTDELAY        -1
> >
> > I count 49 boards with git grep -E
> > 'CONFIG_BOOTDELAY[[:blank:]]+-[0-9]' so it's not _that_ uncommon.
> >
> >> So maybe those could have CONFIG_BOOTDELAY undefined keeping them
> >> working as before?
> >
> > The problem is that as I read the README, we document CONFIG_BOOTDELAY
> > as having a valid value range of from -2 to sane positive value.  So
> > yes, if we want to change this we need to (a) change the README too
> > and (b) give some sort of heads-up.
> >
> > Off the top of my head, we could change to:
> > CONFIG_AUTOBOOT_DISABLED and CONFIG_FORCE_AUTOBOOT_NO_DELAY and update
> > doc/README.autoboot as well and add some sanity #warning checks for a
> > release or two in some common generated config related file or
> > something like that.
> 
> The change has no effect on the behavior, it simply changes what is
> compiled in.  If the default value is not changed, then the behavior
> is unchanged.  The only impact this may have on existing boards is to
> make the code size slightly larger.  If that is an issue for some
> reason, then CONFIG_BOOTDELAY can be undefined.
> 
> If we want to add other config options, that's fine, but that's more
> involved that I was hoping for.

I think we do have a behavior change as previously negative bootdelay
would not result in bootdelay being saved to the default env and thus
not in the env.  Now, it's possible we can mostly retain compatibility
by making bootdelay part of the env if set, rather than if set >= 0.  Or
am I still missing it and really, there's no change in behavior?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130301/2fb32563/attachment.pgp>

[U-Boot] [PATCH] CONFIG_BOOTDELAY default should not affect runtime

[Joe Hershberger]

Hi Tom,

On Fri, Mar 1, 2013 at 2:28 PM, Tom Rini <trini@ti.com> wrote:
> On Wed, Feb 27, 2013 at 02:11:31PM -0600, Joe Hershberger wrote:
>> Hi Tom,
>>
>> On Mon, Feb 18, 2013 at 11:20 AM, Tom Rini <trini@ti.com> wrote:
>> > -----BEGIN PGP SIGNED MESSAGE-----
>> > Hash: SHA1
>> >
>> > On 02/09/2013 11:21 AM, Otavio Salvador wrote:
>> >> Hello Wolfgang,
>> >>
>> >> On Sat, Feb 9, 2013 at 4:54 AM, Wolfgang Denk <wd@denx.de> wrote:
>> >>> Dear Joe Hershberger,
>> >>>
>> >>> In message
>> >>> <1360355280-1197-1-git-send-email-joe.hershberger@ni.com> you
>> >>> wrote:
>> >>>> Because the code that handles bootdelay is compiled in
>> >>>> conditionally based on the default value, you are restricted in
>> >>>> the default, regardless of what you want the runtime options to
>> >>>> be.
>> >>>>
>> >>>> Change the source to always check if any default is given so
>> >>>> that other values can be selected and used at runtime.
>> >>>>
>> >>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> ---
>> >>>> common/main.c | 14 ++++++-------- 1 file changed, 6
>> >>>> insertions(+), 8 deletions(-)
>> >>>>
>> >>>> diff --git a/common/main.c b/common/main.c index
>> >>>> e2d2e09..0973c59 100644 --- a/common/main.c +++
>> >>>> b/common/main.c @@ -95,7 +95,7 @@ extern void mdm_init(void);
>> >>>> /* defined in board.c */ * Watch for 'delay' seconds for
>> >>>> autoboot stop or autoboot delay string. * returns: 0 -  no key
>> >>>> string, allow autoboot 1 - got key string, abort */ -#if
>> >>>> defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) +#if
>> >>>> defined(CONFIG_BOOTDELAY)
>> >>>
>> >>> Careful!! This is probably changing behaviour of a number of
>> >>> boards significantly.
>> >>>
>> >>> we have to check if we really want this, and if yes, we have to
>> >>> announce it and provide a grace period (eventually using
>> >>> doc/feature-removal-schedule.txt ?)
>> >>
>> >> It seems the CONFIG_BOOTDELAY as < 0 is not very common:
>> >>
>> >> ~/hacking/u-boot% git grep CONFIG_BOOTDELAY | egrep 'BOOTDELAY\s*
>> >> \-[0-9]' include/configs/RPXsuper.h:#define CONFIG_BOOTDELAY
>> >> -1 include/configs/ep8260.h:#define CONFIG_BOOTDELAY        -1
>> >> include/configs/espt.h:#define CONFIG_BOOTDELAY        -1
>> >> include/configs/scb9328.h:#define CONFIG_BOOTDELAY   -1
>> >> include/configs/sh7763rdp.h:#define CONFIG_BOOTDELAY        -1
>> >
>> > I count 49 boards with git grep -E
>> > 'CONFIG_BOOTDELAY[[:blank:]]+-[0-9]' so it's not _that_ uncommon.
>> >
>> >> So maybe those could have CONFIG_BOOTDELAY undefined keeping them
>> >> working as before?
>> >
>> > The problem is that as I read the README, we document CONFIG_BOOTDELAY
>> > as having a valid value range of from -2 to sane positive value.  So
>> > yes, if we want to change this we need to (a) change the README too
>> > and (b) give some sort of heads-up.
>> >
>> > Off the top of my head, we could change to:
>> > CONFIG_AUTOBOOT_DISABLED and CONFIG_FORCE_AUTOBOOT_NO_DELAY and update
>> > doc/README.autoboot as well and add some sanity #warning checks for a
>> > release or two in some common generated config related file or
>> > something like that.
>>
>> The change has no effect on the behavior, it simply changes what is
>> compiled in.  If the default value is not changed, then the behavior
>> is unchanged.  The only impact this may have on existing boards is to
>> make the code size slightly larger.  If that is an issue for some
>> reason, then CONFIG_BOOTDELAY can be undefined.
>>
>> If we want to add other config options, that's fine, but that's more
>> involved that I was hoping for.
>
> I think we do have a behavior change as previously negative bootdelay
> would not result in bootdelay being saved to the default env and thus
> not in the env.  Now, it's possible we can mostly retain compatibility
> by making bootdelay part of the env if set, rather than if set >= 0.  Or
> am I still missing it and really, there's no change in behavior?


The behavior change you are describing doesn't happen.  The condition
was not removed from include/env_default.h so the default env should
remain exactly the same as before.  The difference is that now the
code that checks for the var to exists will initially hit the default
case until "bootdelay" is added to the env.

Cheers,
-Joe