From patchwork Mon Jun 13 13:59:30 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aneesh V X-Patchwork-Id: 100163 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 8CDEFB6FC4 for ; Tue, 14 Jun 2011 00:06:28 +1000 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 8D83228132; Mon, 13 Jun 2011 16:06:26 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qYhAmR5P40EO; Mon, 13 Jun 2011 16:06:26 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 924C02819C; Mon, 13 Jun 2011 16:06:23 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 4C6AD2819C for ; Mon, 13 Jun 2011 16:06:20 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id r0-MDEjPxHjb for ; Mon, 13 Jun 2011 16:06:18 +0200 (CEST) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from na3sys009aog108.obsmtp.com (na3sys009aog108.obsmtp.com [74.125.149.199]) by theia.denx.de (Postfix) with ESMTPS id 477CA28132 for ; Mon, 13 Jun 2011 16:06:15 +0200 (CEST) Received: from mail-yw0-f48.google.com ([209.85.213.48]) (using TLSv1) by na3sys009aob108.postini.com ([74.125.148.12]) with SMTP ID DSNKTfYZVTB+qRV8XKw6AeEqTYWIbio+MY3s@postini.com; Mon, 13 Jun 2011 07:06:17 PDT Received: by ywk9 with SMTP id 9so3048365ywk.35 for ; Mon, 13 Jun 2011 07:06:13 -0700 (PDT) Received: by 10.150.160.1 with SMTP id i1mr6783826ybe.234.1307973972882; Mon, 13 Jun 2011 07:06:12 -0700 (PDT) Received: from [172.24.137.55] (dragon.ti.com [192.94.94.33]) by mx.google.com with ESMTPS id d30sm1327481ybd.24.2011.06.13.07.06.10 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 13 Jun 2011 07:06:12 -0700 (PDT) Message-ID: <4DF617C2.6070100@ti.com> Date: Mon, 13 Jun 2011 19:29:30 +0530 From: Aneesh V User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.12) Gecko/20100915 Thunderbird/3.0.8 MIME-Version: 1.0 To: Wolfgang Denk References: <1298893591-17636-1-git-send-email-aneesh@ti.com> <1305472900-4004-23-git-send-email-aneesh@ti.com> <20110515202139.EAB481491B06@gemini.denx.de> In-Reply-To: <20110515202139.EAB481491B06@gemini.denx.de> Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH v2 22/22] omap: spl: add more debug traces X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.9 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: u-boot-bounces@lists.denx.de Errors-To: u-boot-bounces@lists.denx.de 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@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("< > 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: --- --- 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 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;