[U-Boot] [PATCH] arm, davinci: make arch_cpu_init() in da850_lowlevel.c weak

[Heiko Schocher <hs@denx.de>]

Hello Christian,

Christian Riesch wrote:
> Hello Wolfgang,
> 
> On Wed, Nov 9, 2011 at 11:44 AM, Wolfgang Denk <wd@denx.de> wrote:
>> In message <CABkLObpuzibrEef+8EKKqTgyYcyTeC2qMx2UM1GqkzX--K3jLQ@mail.gmail.com> 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:

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
 /*

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany