From patchwork Wed Nov 9 12:18:09 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Heiko Schocher X-Patchwork-Id: 124538 X-Patchwork-Delegate: trini@ti.com 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 7F372B6F98 for ; Wed, 9 Nov 2011 23:18:29 +1100 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 39DA42989E; Wed, 9 Nov 2011 13:18:26 +0100 (CET) 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 NuiWb2njQrFp; Wed, 9 Nov 2011 13:18:25 +0100 (CET) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id DC4602978E; Wed, 9 Nov 2011 13:18:24 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 4D2772978E for ; Wed, 9 Nov 2011 13:18:23 +0100 (CET) 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 cZrH0CTiLnKy for ; Wed, 9 Nov 2011 13:18:22 +0100 (CET) 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 a.relay.invitel.net (a.relay.invitel.net [62.77.203.3]) by theia.denx.de (Postfix) with ESMTP id 5BAF62975A for ; Wed, 9 Nov 2011 13:18:20 +0100 (CET) Received: from mail.invitel.hu (mail.invitel.hu [213.163.59.4]) by a.relay.invitel.net (Invitel Core SMTP Transmitter) with ESMTP id 3504F1560FF; Wed, 9 Nov 2011 13:18:20 +0100 (CET) Received: from [192.168.1.6] ([87.97.113.160]) by mail.invitel.hu (Invitel Messaging Server) with ESMTPA id <0LUE00FRE7IBM080@invitel.hu>; Wed, 09 Nov 2011 13:18:12 +0100 (CET) Date: Wed, 09 Nov 2011 13:18:09 +0100 From: Heiko Schocher In-reply-to: To: Christian Riesch Message-id: <4EBA6F81.2060400@denx.de> Organization: DENX Software Engineering MIME-version: 1.0 References: <1320830586-19124-1-git-send-email-christian.riesch@omicron.at> <20111109101217.2EAC613BE0E2@gemini.denx.de> <20111109104424.8DA7E13BE08A@gemini.denx.de> User-Agent: Thunderbird 2.0.0.6 (X11/20070801) Cc: Albert Aribaud , u-boot@lists.denx.de, Heiko Schocher Subject: Re: [U-Boot] [PATCH] arm, davinci: make arch_cpu_init() in da850_lowlevel.c weak X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.9 Precedence: list Reply-To: hs@denx.de 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 Hello Christian, Christian Riesch wrote: > Hello Wolfgang, > > On Wed, Nov 9, 2011 at 11:44 AM, Wolfgang Denk wrote: >> In message you wrote: >>>> All this needs a _thorough_ cleanup before I'm willing to accept this >>>> for mainline. >>> This is already in mainline, see >> Indeed. What a pity. >> >> Anyway. We should clean it up first, before attempting any other >> changes. >> >> >> I don't understand yet what your exact requirements are - can you >> confirm that my assumption is correct that you can do without the >> "weak" if the hardwired constants in this fle get replaced by symbolic >> names that can be set from the board config file? > > I'll comment on the code that is currently in u-boot-arm:arch > /arm/cpu/arm926ejs/davinci/da850_lowlevel.c > > 263 #if defined(CONFIG_NAND_SPL) > > I guess this will become obsolete soon, in the new SPL framework this > should be done in another way, right? Yes. I prefer actually to remove the old NAND_SPL style complete in this cleanup step, because nobody is using it yet. And if someone use this code for SPL, it must be adapted. > 264 void board_init_f(ulong bootflag) > 265 #else > 266 int arch_cpu_init(void) > 267 #endif > 268 { > 269 /* > 270 * copied from arch/arm/cpu/arm926ejs/start.S > 271 * > 272 * flush v4 I/D caches > 273 */ > 274 asm("mov r0, #0"); > 275 asm("mcr p15, 0, r0, c7, c7, 0"); /* flush > v3/v4 cache */ > 276 asm("mcr p15, 0, r0, c8, c7, 0"); /* flush v4 TLB */ > 277 > 278 /* > 279 * disable MMU stuff and caches > 280 */ > 281 asm("mrc p15, 0, r0, c1, c0, 0"); > 282 /* clear bits 13, 9:8 (--V- --RS) */ > 283 asm("bic r0, r0, #0x00002300"); > 284 /* clear bits 7, 2:0 (B--- -CAM) */ > 285 asm("bic r0, r0, #0x00000087"); > 286 /* set bit 2 (A) Align */ > 287 asm("orr r0, r0, #0x00000002"); > 288 /* set bit 12 (I) I-Cache */ > 289 asm("orr r0, r0, #0x00001000"); > 290 asm("mcr p15, 0, r0, c1, c0, 0"); > 291 > > Heiko, why do we need this? I noticed, that u-boot takes longer to > start when I remove this code. We need it, because we have defined CONFIG_SKIP_LOWLEVEL_INIT (at least for the enbw_cmc board), and this is a copy from arch/arm/cpu/arm926ejs/start.S ... I tend to change arch/arm/cpu/arm926ejs/start.S, so we always execute this code from arch/arm/cpu/arm926ejs/start.S and don't need it here anymore: Albert, what do you think? Do you see some problems against this? > > 292 /* Unlock kick registers */ > 293 writel(0x83e70b13, &davinci_syscfg_regs->kick0); > 294 writel(0x95a4f1e0, &davinci_syscfg_regs->kick1); > 295 > > hawkboard has two defines HAWKBOARD_KICK0_UNLOCK and > HAWKBOARD_KICK1_UNLOCK for these magic numbers. Maybe we should rename > them because they are not HAWKBOARD specific and use them? > see board/davinci/da8xxevm/hawkboard.c added this defines to arch/arm/include/asm/arch-davinci/hardware.h > 296 dv_maskbits(&davinci_syscfg_regs->suspsrc, > 297 ((1 << 27) | (1 << 22) | (1 << 20) | (1 << 5) | > (1 << 16))); > 298 > > This is done in a nicer way in board/davinci/da8xxevm/da850evm.c > I wonder if these settings work for all boards or if any boards wound > need different settings here. Changed this. You need now a CONFIG_SYS_DA850_SYSCFG_SUSPSRC in your board config. > > 299 /* Setup Pinmux */ > 300 da850_pinmux_ctl(0, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX0); > 301 da850_pinmux_ctl(1, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX1); > 302 da850_pinmux_ctl(2, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX2); > 303 da850_pinmux_ctl(3, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX3); > 304 da850_pinmux_ctl(4, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX4); > 305 da850_pinmux_ctl(5, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX5); > 306 da850_pinmux_ctl(6, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX6); > 307 da850_pinmux_ctl(7, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX7); > 308 da850_pinmux_ctl(8, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX8); > 309 da850_pinmux_ctl(9, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX9); > 310 da850_pinmux_ctl(10, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX10); > 311 da850_pinmux_ctl(11, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX11); > 312 da850_pinmux_ctl(12, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX12); > 313 da850_pinmux_ctl(13, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX13); > 314 da850_pinmux_ctl(14, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX14); > 315 da850_pinmux_ctl(15, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX15); > 316 da850_pinmux_ctl(16, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX16); > 317 da850_pinmux_ctl(17, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX17); > 318 da850_pinmux_ctl(18, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX18); > 319 da850_pinmux_ctl(19, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX19); > 320 > > I could do with this code and setting my custom PINMUX constants. > However, the da850evm uses a different way of configuring pinmux, so > we have a duplication of code here. I'd prefer the da850evm way > because the code is still readable when you don't use the TI tool > mentioned by Heiko in [1] (I was not aware of this tool, thanks for > the hint, Heiko!). I prefer it this way, but as I said, if we come to the result to use it like the da850evm way, it is ok with me. > 321 /* PLL setup */ > 322 da850_pll_init(davinci_pllc0_regs, CONFIG_SYS_DA850_PLL0_PLLM); > 323 da850_pll_init(davinci_pllc1_regs, CONFIG_SYS_DA850_PLL1_PLLM); > > On my board I need to determine the hardware revision first and then > set the values of CONFIG_SYS_DA850_PLL0_PLLM and > CONFIG_SYS_DA850_PLL1_PLLM accordingly. But maybe I could uses > something like > > #define CONFIG_SYS_DA850_PLL1_PLLM board_get_pllm1() > > and write a function that reads the board revision... Maybe this is a way to prevent the weak function, yes. > 324 > 325 /* GPIO setup */ > 326 board_gpio_init(); > > Why is this done here? For SPL? Will this change when the code moves > to the new framework? I don't need GPIOs here. If you don't need this code, do nothing. The weak function for this is a dummy function. Maybe boards will use gpios early, as I use this on the enbw_cmc board. > 328 /* setup CSn config */ > 329 writel(CONFIG_SYS_DA850_CS2CFG, &davinci_emif_regs->ab1cr); > 330 writel(CONFIG_SYS_DA850_CS3CFG, &davinci_emif_regs->ab2cr); > 331 > > Ok for NAND, but how about a board that uses SPI flash? It should be > possible to chose whether to initialize CSn. We could check if CONFIG_SYS_DA850_CS2CFG/CONFIG_SYS_DA850_CS3CFG is defined, and if not, dont init the register. > 332 lpsc_on(DAVINCI_LPSC_UART2); > 333 NS16550_init((NS16550_t)(CONFIG_SYS_NS16550_COM1), > 334 CONFIG_SYS_NS16550_CLK / 16 / CONFIG_BAUDRATE); > 335 > 336 /* > 337 * Fix Power and Emulation Management Register > 338 * see sprufw3a.pdf page 37 Table 24 > 339 */ > 340 writel(readl((CONFIG_SYS_NS16550_COM1 + 0x30)) | 0x00006001, > 341 (CONFIG_SYS_NS16550_COM1 + 0x30)); > > I guess this is only needed for debugging the SPL? This must be done before using the uart as a console, and I think this should be done in common code as early as possible. > 342 #if defined(CONFIG_NAND_SPL) > 343 puts("ddr init\n"); > 344 da850_ddr_setup(132); > 345 > 346 puts("boot u-boot ...\n"); > 347 > 348 nand_boot(); > 349 #else > 350 da850_ddr_setup(132); > > Ok for my board. But how about boards that use SDRAM on EMIFA instead > (if there are any)? As no boards use this code except the new enbw_cmc port, there are no such boards ... if somebody want to use this feature, it must be adapted here, yes. bye, Heiko diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S index 339c5ed..73ceb30 100644 --- a/arch/arm/cpu/arm926ejs/start.S +++ b/arch/arm/cpu/arm926ejs/start.S @@ -353,7 +353,6 @@ _dynsym_start_ofs: * ************************************************************************* */ -#ifndef CONFIG_SKIP_LOWLEVEL_INIT cpu_init_crit: /* * flush v4 I/D caches @@ -372,14 +371,15 @@ cpu_init_crit: orr r0, r0, #0x00001000 /* set bit 12 (I) I-Cache */ mcr p15, 0, r0, c1, c0, 0 +#ifndef CONFIG_SKIP_LOWLEVEL_INIT /* * Go setup Memory and board specific bits prior to relocation. */ mov ip, lr /* perserve link reg across call */ bl lowlevel_init /* go setup pll,mux,memory */ mov lr, ip /* restore link */ - mov pc, lr /* back to my caller */ #endif /* CONFIG_SKIP_LOWLEVEL_INIT */ + mov pc, lr /* back to my caller */ #ifndef CONFIG_SPL_BUILD /*