[U-Boot] [PATCH v2 22/22] omap: spl: add more debug traces

[Aneesh V]

Dear Wolfgang,

I just realized that I had not responded to this message.

On Monday 16 May 2011 01:51 AM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<1305472900-4004-23-git-send-email-aneesh at ti.com>  you wrote:
>> In SPL console is enabled very early where as in U-Boot
>> it's not. So, SPL can have traces in early init code
>
> Console should _always_ be enabled as early as possible,

Unfortunately this is not the case with U-Boot today. Console
initialization happens in board_init_f(). Before board_init_f()
typically a lot of(in fact most of) low level initialization happens
through the lowlevel_init() function called from start.S and the
initial part of init_sequence()

>
>> while U-Boot can not have it in the same shared code.
>>
>> Adding a debug print macro that will be defined in SPL
>> but compiled out in U-Boot.
>
> Can we not rather change the code so both configurations behave the
> same?

In SPL I had more flexibility, so I do a simplified init of console
right at the beginning of lowlevel_init(), so I can use debug prints in
lowlevel_init(). Some part of our lowlevel_init() that is executed in
SPL path may also be executed by U-Boot if it runs from NOR flash, but
in this case console wouldn't be ready. That's why I made the macro.

>
>> --- a/arch/arm/cpu/armv7/omap4/clocks.c
>> +++ b/arch/arm/cpu/armv7/omap4/clocks.c
>> @@ -379,7 +379,7 @@ u32 omap4_ddr_clk(void)
>>
>>   	core_dpll_params = get_core_dpll_params();
>>
>> -	debug("sys_clk %d\n ", sys_clk_khz * 1000);
>> +	spl_debug("sys_clk %d\n ", sys_clk_khz * 1000);
>
> Do we really need a new macro name?  Can this not be the same debug()
> macro, just generating different code (if really needed) when building
> the SPL code?

No. That wouldn't serve the purpose. I need two macros to distinguish
between the two cases.
1. 'debug()' - can be used in all places at which console is guaranteed
to be initialized whether executed as part of U-Boot or SPL.
2. 'spl_debug()' to be used at places where console is initialized for
SPL but not for U-Boot (eg. lowlevel_init()) - so emit no code for
U-Boot.

>
>> @@ -1318,4 +1328,13 @@ void sdram_init(void)
>>
>>   	/* for the shadow registers to take effect */
>>   	freq_update_core();
>> +
>> +	/* Do some basic testing */
>> +	writel(0xDEADBEEF, CONFIG_SYS_SDRAM_BASE);
>> +	if (readl(CONFIG_SYS_SDRAM_BASE) == 0xDEADBEEF)
>> +		spl_debug("SDRAM Init success!\n");
>> +	else
>> +		printf("SDRAM Init failed!!\n");
>> +
>> +	spl_debug("<<sdram_init()\n");
>
> This is beyond the scope of "adding debug traces".  it must be split
> into separate patch.

will do.

>
> Also, please do not mess witrhout need with the RAM content - at the
> very least, restore the previous values.

Will do.

>
> But then - I wonder why this is needed at all. Are you not using
> get_ram_size()?  Maybe you should fix your code to using it!

It may make sense for us to do this as a memory test. For discovering
the amount of memory installed we are using a technique based on
reading of LPDDR2 mode registers.

The above was intended as a simple sanity testing.

>
>> diff --git a/arch/arm/include/asm/utils.h b/arch/arm/include/asm/utils.h
>> index d581539..3e847c1 100644
>> --- a/arch/arm/include/asm/utils.h
>> +++ b/arch/arm/include/asm/utils.h
>> @@ -25,6 +25,12 @@
>>   #ifndef	_UTILS_H_
>>   #define	_UTILS_H_
>>
>> +#if defined(DEBUG)&&  defined(CONFIG_PRELOADER)
>> +#define spl_debug(fmt, args...)	printf(fmt, ##args)
>> +#else
>> +#define spl_debug(fmt, args...)
>> +#endif
>
> NAK.  This is neither the right place for such a definition, nor do I
> want to see this addressed like that.
>
> I recommend to unify the code, so both SPL and non-SPL configurations
> can use teh same early console behaviour.

I always thought this is a serious issue with U-Boot. I gave it a quick
shot like below:

---
diff --git a/arch/arm/cpu/armv7/omap4/board.c 
b/arch/arm/cpu/armv7/omap4/board.c
index fcd29a7..56902e3 100644
--- a/arch/arm/cpu/armv7/omap4/board.c
+++ b/arch/arm/cpu/armv7/omap4/board.c
@@ -41,6 +41,7 @@ DECLARE_GLOBAL_DATA_PTR;
   */
  void s_init(void)
  {
+	printf("Hello World!!\n");
  	watchdog_init();
  }

diff --git a/arch/arm/cpu/armv7/omap4/lowlevel_init.S 
b/arch/arm/cpu/armv7/omap4/lowlevel_init.S
index 026dfa4..42264b2 100644
--- a/arch/arm/cpu/armv7/omap4/lowlevel_init.S
+++ b/arch/arm/cpu/armv7/omap4/lowlevel_init.S
@@ -31,17 +31,6 @@
  .globl lowlevel_init
  lowlevel_init:
  	/*
-	 * Setup a temporary stack
-	 */
-	ldr	sp, =LOW_LEVEL_SRAM_STACK
-
-	/*
-	 * Save the old lr(passed in ip) and the current lr to stack
-	 */
-	push	{ip, lr}
-
-	/*
  	 * go setup pll, mux, memory
  	 */
-	bl	s_init
-	pop	{ip, pc}
+	b	s_init
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index d91ae12..4a9251f 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -307,10 +307,11 @@ cpu_init_crit:
  	 * basic memory. Go here to bump up clock rate and handle
  	 * wake up conditions.
  	 */
-	mov	ip, lr			@ persevere link reg across call
+	ldr	sp, =CONFIG_SYS_INIT_SP_ADDR
+	push	{lr}
+	bl	early_console_init
  	bl	lowlevel_init		@ go setup pll,mux,memory
-	mov	lr, ip			@ restore link
-	mov	pc, lr			@ back to my caller
+	pop	{pc}
  /*
   *************************************************************************
   *
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index 1a784a1..0c696c2 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -248,9 +248,6 @@ init_fnc_t *init_sequence[] = {
  	get_clocks,
  #endif
  	env_init,		/* initialize environment */
-	init_baudrate,		/* initialze baudrate settings */
-	serial_init,		/* serial communications setup */
-	console_init_f,		/* stage 1 init of console */
  	display_banner,		/* say that we are here */
  #if defined(CONFIG_DISPLAY_CPUINFO)
  	print_cpuinfo,		/* display cpu info (and speed) */
@@ -268,6 +265,13 @@ init_fnc_t *init_sequence[] = {
  	NULL,
  };

+void early_console_init(void)
+{
+	init_baudrate();	/* initialze baudrate settings */
+	serial_init();		/* serial communications setup */
+	console_init_f();	/* stage 1 init of console */
+}
+
  void board_init_f (ulong bootflag)
  {
  	bd_t *bd;
---


But this didn't work. It crashes at the first printf(). The reason is
init_baudrate() needs global data and global data is initialized in
board_init_f(). Further, we can not move global data initialization
earlier because for global data we reuse low level stack used by
lowlevel_init().

	gd = (gd_t *) ((CONFIG_SYS_INIT_SP_ADDR) & ~0x07);

So, we have an egg-and-chicken problem unless we find different spaces
for low-level stack and global data(that should not be a problem for
our board, but I am not sure of other boards).

Also, perhaps this needs to be done for all CPUs/archs.

IMHO, this will need a major overhaul and is not in the scope of my
current activity. Please suggest how to go about this.

Do you think re-naming spl_debug to omap_spl_debug() and putting it in
an OMAP header will help?:-) I am not sure if everybody has the same
situation as we have(that is, U-Boot and SPL sharing low-level init
code and console being initialized in one case while not in the other)

best regards,
Aneesh