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-ane...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
