Patchwork [U-Boot,V3,3/4] ARM: Warn when the machine ID isn't set.

login
register
mail settings
Submitter Christopher Harvey
Date July 14, 2011, 6:02 p.m.
Message ID <20110714180240.GA21529@harvey-pc.matrox.com>
Download mbox | patch
Permalink /patch/104704/
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Comments

Christopher Harvey - July 14, 2011, 6:02 p.m.
Linux cannot boot without it.

Signed-off-by: Christopher Harvey <charvey@matrox.com>
---

V2:
Used a #define instead of a constant.
Used a printf instead of a debug.

---

V3:
Moved gd->bd->bi_arch_number = BI_ARCH_NUMBER_INVALID; before the 
  init_sequence loop, so it doesn't overwrite existing values.
Removed unneeded braces. 

Reminder, in V2 of this series there are 3 uncommited patches that
  have no comments.

 arch/arm/include/asm/u-boot.h |    2 ++
 arch/arm/lib/board.c          |    2 ++
 arch/arm/lib/bootm.c          |    5 +++++
 3 files changed, 9 insertions(+), 0 deletions(-)
Igor Grinberg - July 14, 2011, 7:02 p.m.
Hi Christopher,

On 07/14/11 21:02, Christopher Harvey wrote:
> Linux cannot boot without it.
>
> Signed-off-by: Christopher Harvey <charvey@matrox.com>
> ---
>
> V2:
> Used a #define instead of a constant.
> Used a printf instead of a debug.
>
> ---
>
> V3:
> Moved gd->bd->bi_arch_number = BI_ARCH_NUMBER_INVALID; before the 
>   init_sequence loop, so it doesn't overwrite existing values.
> Removed unneeded braces. 

This one looks fine, except that you keep ignoring my question...
Please, see below

> Reminder, in V2 of this series there are 3 uncommited patches that
>   have no comments.
>
>  arch/arm/include/asm/u-boot.h |    2 ++
>  arch/arm/lib/board.c          |    2 ++
>  arch/arm/lib/bootm.c          |    5 +++++
>  3 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/include/asm/u-boot.h b/arch/arm/include/asm/u-boot.h
> index ed33327..81735de 100644
> --- a/arch/arm/include/asm/u-boot.h
> +++ b/arch/arm/include/asm/u-boot.h
> @@ -48,4 +48,6 @@ typedef struct bd_info {
>      }			bi_dram[CONFIG_NR_DRAM_BANKS];
>  } bd_t;
>  
> +#define BI_ARCH_NUMBER_INVALID  0xffffffff
> +
>  #endif	/* _U_BOOT_H_ */
> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index fc52a26..1635aa1 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -281,6 +281,8 @@ void board_init_f (ulong bootflag)
>  
>  	gd->mon_len = _bss_end_ofs;
>  
> +	gd->bd->bi_arch_number = BI_ARCH_NUMBER_INVALID;
> +
>  	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
>  		if ((*init_fnc_ptr)() != 0) {
>  			hang ();
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 802e833..8e75b5a 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -113,6 +113,11 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
>  		printf ("Using machid 0x%x from environment\n", machid);
>  	}
>  
> +#ifdef DEBUG
> +	if (machid == BI_ARCH_NUMBER_INVALID)
> +	        printf("Warning: machid not set.\n");
> +#endif
> +

Is it essential to enclose that check in #ifdef DEBUG?
IMHO, it can be useful also with no DEBUG defined,
so I'd add it without the #ifdef DEBUG.

Also, in the printf line, you are mixing tabs with spaces
(sorry for not noticing this in previous versions...).
Stefano Babic - July 15, 2011, 12:44 p.m.
On 07/14/2011 09:02 PM, Igor Grinberg wrote:

>> +#ifdef DEBUG
>> +	if (machid == BI_ARCH_NUMBER_INVALID)
>> +	        printf("Warning: machid not set.\n");
>> +#endif
>> +
> 
> Is it essential to enclose that check in #ifdef DEBUG?
> IMHO, it can be useful also with no DEBUG defined,
> so I'd add it without the #ifdef DEBUG.
> 
> Also, in the printf line, you are mixing tabs with spaces
> (sorry for not noticing this in previous versions...).

...and if you want to print something only for debug purposes, the best
way is to substitute printf() with debug() and get rid of #ifdef.

+	if (machid == BI_ARCH_NUMBER_INVALID)
+	        debug("Warning: machid not set.\n");

Best regards,
Stefano Babic
Igor Grinberg - July 17, 2011, 6:53 a.m.
On 07/15/11 15:44, Stefano Babic wrote:
> On 07/14/2011 09:02 PM, Igor Grinberg wrote:
>
>>> +#ifdef DEBUG
>>> +	if (machid == BI_ARCH_NUMBER_INVALID)
>>> +	        printf("Warning: machid not set.\n");
>>> +#endif
>>> +
>> Is it essential to enclose that check in #ifdef DEBUG?
>> IMHO, it can be useful also with no DEBUG defined,
>> so I'd add it without the #ifdef DEBUG.
>>
>> Also, in the printf line, you are mixing tabs with spaces
>> (sorry for not noticing this in previous versions...).
> ...and if you want to print something only for debug purposes, the best
> way is to substitute printf() with debug() and get rid of #ifdef.
>
> +	if (machid == BI_ARCH_NUMBER_INVALID)
> +	        debug("Warning: machid not set.\n");

That is understood completely and that is not what I'm asking...
I think that this warning should be printed not just for debug purposes...
So, I'd prefer:

+	if (machid == BI_ARCH_NUMBER_INVALID)
+		printf("Warning: machid not set.\n");

with no #ifdefs.
So, I'm asking is it essential to make it only for debug purposes?
Are there any cases when this code will harm if no #define DEBUG is specified?
Stefano Babic - July 17, 2011, 8:26 a.m.
Am 17/07/2011 08:53, schrieb Igor Grinberg:
>>> Also, in the printf line, you are mixing tabs with spaces
>>> (sorry for not noticing this in previous versions...).
>> ...and if you want to print something only for debug purposes, the best
>> way is to substitute printf() with debug() and get rid of #ifdef.
>>
>> +	if (machid == BI_ARCH_NUMBER_INVALID)
>> +	        debug("Warning: machid not set.\n");
> 
> That is understood completely and that is not what I'm asking...
> I think that this warning should be printed not just for debug purposes...
> So, I'd prefer:
> 
> +	if (machid == BI_ARCH_NUMBER_INVALID)
> +		printf("Warning: machid not set.\n");
> 
> with no #ifdefs.

Agree. And because the goal of thi patch is to warn when the mach-id is
not set, I am expecting to see this warning on the console without
recompiling the code.

> So, I'm asking is it essential to make it only for debug purposes?

IMHO, I think no.

> Are there any cases when this code will harm if no #define DEBUG is specified?

Agree with you, I do not see any reason to output the warning only if
DEBUG is set

Best regards,
Stefano Babic
Albert ARIBAUD - July 17, 2011, 9 a.m.
Hi all,

Le 17/07/2011 10:26, stefano babic a écrit :
> Am 17/07/2011 08:53, schrieb Igor Grinberg:
>>>> Also, in the printf line, you are mixing tabs with spaces
>>>> (sorry for not noticing this in previous versions...).
>>> ...and if you want to print something only for debug purposes, the best
>>> way is to substitute printf() with debug() and get rid of #ifdef.
>>>
>>> +	if (machid == BI_ARCH_NUMBER_INVALID)
>>> +	        debug("Warning: machid not set.\n");
>>
>> That is understood completely and that is not what I'm asking...
>> I think that this warning should be printed not just for debug purposes...
>> So, I'd prefer:
>>
>> +	if (machid == BI_ARCH_NUMBER_INVALID)
>> +		printf("Warning: machid not set.\n");
>>
>> with no #ifdefs.
>
> Agree. And because the goal of thi patch is to warn when the mach-id is
> not set, I am expecting to see this warning on the console without
> recompiling the code.
>
>> So, I'm asking is it essential to make it only for debug purposes?
>
> IMHO, I think no.
>
>> Are there any cases when this code will harm if no #define DEBUG is specified?
>
> Agree with you, I do not see any reason to output the warning only if
> DEBUG is set

Agreed as well, only more so: I see reason for this warning *only* if it 
is emitted in non-DEBUG builds. Such a warning is emitted to warn 
against possible future complications; if it is only emitted in DEBUG 
builds, then when the complication actually happens, that is, in a 
production build, the developer is deprived of an important clue 
regarding the cause.

> Best regards,
> Stefano Babic

Amicalement,

Patch

diff --git a/arch/arm/include/asm/u-boot.h b/arch/arm/include/asm/u-boot.h
index ed33327..81735de 100644
--- a/arch/arm/include/asm/u-boot.h
+++ b/arch/arm/include/asm/u-boot.h
@@ -48,4 +48,6 @@  typedef struct bd_info {
     }			bi_dram[CONFIG_NR_DRAM_BANKS];
 } bd_t;
 
+#define BI_ARCH_NUMBER_INVALID  0xffffffff
+
 #endif	/* _U_BOOT_H_ */
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index fc52a26..1635aa1 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -281,6 +281,8 @@  void board_init_f (ulong bootflag)
 
 	gd->mon_len = _bss_end_ofs;
 
+	gd->bd->bi_arch_number = BI_ARCH_NUMBER_INVALID;
+
 	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
 		if ((*init_fnc_ptr)() != 0) {
 			hang ();
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 802e833..8e75b5a 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -113,6 +113,11 @@  int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
 		printf ("Using machid 0x%x from environment\n", machid);
 	}
 
+#ifdef DEBUG
+	if (machid == BI_ARCH_NUMBER_INVALID)
+	        printf("Warning: machid not set.\n");
+#endif
+
 	show_boot_progress (15);
 
 #ifdef CONFIG_OF_LIBFDT