Patchwork [U-Boot,V2,2/4] Don't compile in large memory test function by default.

login
register
mail settings
Submitter Christopher Harvey
Date July 7, 2011, 2:31 p.m.
Message ID <20110707143109.GC5438@harvey-pc.matrox.com>
Download mbox | patch
Permalink /patch/103673/
State Accepted
Delegated to: Albert ARIBAUD
Headers show

Comments

Christopher Harvey - July 7, 2011, 2:31 p.m.
Signed-off-by: Christopher Harvey <charvey@matrox.com>
---

V2:
I didn't receive comments on this one.  By not compiling this by
default it removes a couple of #defines people need to think
about. Also, I'm guessing the memory test isn't used very much, so
this makes u-boot a bit smaller.

 common/cmd_mem.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Albert ARIBAUD - Aug. 4, 2011, 6:31 a.m.
Hi Christopher,

Le 07/07/2011 16:31, Christopher Harvey a écrit :
> Signed-off-by: Christopher Harvey<charvey@matrox.com>
> ---
>
> V2:
> I didn't receive comments on this one.  By not compiling this by
> default it removes a couple of #defines people need to think
> about. Also, I'm guessing the memory test isn't used very much, so
> this makes u-boot a bit smaller.
>
>   common/cmd_mem.c |    5 +++++
>   1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/common/cmd_mem.c b/common/cmd_mem.c
> index a5576aa..833af66 100644
> --- a/common/cmd_mem.c
> +++ b/common/cmd_mem.c
> @@ -610,6 +610,8 @@ int do_mem_loopw (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   }
>   #endif /* CONFIG_LOOPW */
>
> +#ifdef CONFIG_CMD_MTEST
> +
>   /*
>    * Perform a memory test. A more complete alternative test can be
>    * configured using CONFIG_SYS_ALT_MEMTEST. The complete test loops until
> @@ -965,6 +967,7 @@ int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   	return 0;	/* not reached */
>   }
>
> +#endif /* CONFIG_CMD_MTEST */
>
>   /* Modify memory.
>    *
> @@ -1245,11 +1248,13 @@ U_BOOT_CMD(
>   );
>   #endif /* CONFIG_LOOPW */
>
> +#ifdef CONFIG_CMD_MTEST
>   U_BOOT_CMD(
>   	mtest,	5,	1,	do_mem_mtest,
>   	"simple RAM read/write test",
>   	"[start [end [pattern [iterations]]]]"
>   );
> +#endif /* CONFIG_CMD_MTEST */
>
>   #ifdef CONFIG_MX_CYCLIC
>   U_BOOT_CMD(

Applied to u-boot-arm/master, thanks!

Amicalement,
Wolfgang Denk - Aug. 4, 2011, 10:14 a.m.
Dear Albert ARIBAUD,

In message <4E3A3CB8.2030703@aribaud.net> you wrote:
> 
> > I didn't receive comments on this one.  By not compiling this by
> > default it removes a couple of #defines people need to think
> > about. Also, I'm guessing the memory test isn't used very much, so
> > this makes u-boot a bit smaller.
...
> >   common/cmd_mem.c |    5 +++++
> >   1 files changed, 5 insertions(+), 0 deletions(-)
...
> Applied to u-boot-arm/master, thanks!

Argh..

This is NOT an ARM specific patch.

Even worse, it changes global behaviour for most baords.  This cannot
be dones easily.

Sorry, but NAK.

Best regards,

Wolfgang Denk
Wolfgang Denk - Aug. 4, 2011, 10:20 a.m.
Dear Christopher Harvey,

In message <20110707143109.GC5438@harvey-pc.matrox.com> you wrote:
>
> I didn't receive comments on this one.  By not compiling this by
> default it removes a couple of #defines people need to think
> about. Also, I'm guessing the memory test isn't used very much, so
> this makes u-boot a bit smaller.
> 
>  common/cmd_mem.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/common/cmd_mem.c b/common/cmd_mem.c
> index a5576aa..833af66 100644
> --- a/common/cmd_mem.c
> +++ b/common/cmd_mem.c
> @@ -610,6 +610,8 @@ int do_mem_loopw (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  }
>  #endif /* CONFIG_LOOPW */
>  
> +#ifdef CONFIG_CMD_MTEST

Formal complaint:  ifh you introduce new CONFIG_ options, you MUST
document these in the README.


What is worse is that your commit changes global behaviour of U-Boot
for all architectures, nearly all boards.

Even if you guess that some feature might not be used we cannot simply
remove it lightly.

If you want to make this configurable, then as an opt-out, but not
mandatory for everybody.



Best regards,

Wolfgang Denk
Albert ARIBAUD - Aug. 4, 2011, 10:54 a.m.
On 04/08/2011 12:14, Wolfgang Denk wrote:
> Dear Albert ARIBAUD,
>
> In message<4E3A3CB8.2030703@aribaud.net>  you wrote:
>>
>>> I didn't receive comments on this one.  By not compiling this by
>>> default it removes a couple of #defines people need to think
>>> about. Also, I'm guessing the memory test isn't used very much, so
>>> this makes u-boot a bit smaller.
> ...
>>>    common/cmd_mem.c |    5 +++++
>>>    1 files changed, 5 insertions(+), 0 deletions(-)
> ...
>> Applied to u-boot-arm/master, thanks!
>
> Argh..
>
> This is NOT an ARM specific patch.
>
> Even worse, it changes global behaviour for most baords.  This cannot
> be dones easily.
>
> Sorry, but NAK.

Quite understandable, as I hurriedly applied the entire four-patch set 
when I should only have pulled the last two, ARM-related, patches.

> Best regards,
>
> Wolfgang Denk

Amicalement,

Patch

diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index a5576aa..833af66 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -610,6 +610,8 @@  int do_mem_loopw (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 }
 #endif /* CONFIG_LOOPW */
 
+#ifdef CONFIG_CMD_MTEST
+
 /*
  * Perform a memory test. A more complete alternative test can be
  * configured using CONFIG_SYS_ALT_MEMTEST. The complete test loops until
@@ -965,6 +967,7 @@  int do_mem_mtest (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	return 0;	/* not reached */
 }
 
+#endif /* CONFIG_CMD_MTEST */
 
 /* Modify memory.
  *
@@ -1245,11 +1248,13 @@  U_BOOT_CMD(
 );
 #endif /* CONFIG_LOOPW */
 
+#ifdef CONFIG_CMD_MTEST
 U_BOOT_CMD(
 	mtest,	5,	1,	do_mem_mtest,
 	"simple RAM read/write test",
 	"[start [end [pattern [iterations]]]]"
 );
+#endif /* CONFIG_CMD_MTEST */
 
 #ifdef CONFIG_MX_CYCLIC
 U_BOOT_CMD(