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

[Wolfgang Denk]

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,

> 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?

> --- 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?

> @@ -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.

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

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!

> 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.

>  void board_init_f(ulong dummy)
>  {
> +	debug(">>board_init_f()\n");
>  	relocate_code(CONFIG_SYS_SPL_STACK, &gdata, CONFIG_SYS_SPL_TEXT_BASE);
> +	debug("<<board_init_f()\n");

This is overkill, isn't it?

> @@ -166,6 +172,7 @@ void board_init_r(gd_t *id, ulong dummy)
>  	switch (boot_device) {
>  	case BOOT_DEVICE_MMC1:
>  	case BOOT_DEVICE_MMC2:
> +		spl_debug("boot device - MMC2\n");
>  		mmc_load_uboot(boot_device - BOOT_DEVICE_MMC1);

This is wrong in the BOOT_DEVICE_MMC1 case.

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
"Marriage is like a cage; one sees the birds outside desperate to get
in, and those inside desperate to get out."               - Montaigne