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

Wolfgang Denk
2011-05-15T20:21:39Z

Dear Aneesh V,

In message <1305472900-4004-23-git-send-email-aneesh@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@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