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

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

[Christian Riesch]

This patch allows replacing arch_cpu_init() if a custom initialization
is required.

Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
Cc: Sandeep Paulraj <s-paulraj@ti.com>
Cc: Heiko Schocher <hs@denx.de>
---
 arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
index 327ff97..fe142dc 100644
--- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
+++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
@@ -263,6 +263,7 @@ void nand_boot(void)
 #if defined(CONFIG_NAND_SPL)
 void board_init_f(ulong bootflag)
 #else
+__attribute__ ((weak))
 int arch_cpu_init(void)
 #endif
 {
-- 
1.7.0.4

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

[Wolfgang Denk]

Dear Christian Riesch,

In message <1320830586-19124-1-git-send-email-christian.riesch@omicron.at> you wrote:
> This patch allows replacing arch_cpu_init() if a custom initialization
> is required.
> 
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> Cc: Sandeep Paulraj <s-paulraj@ti.com>
> Cc: Heiko Schocher <hs@denx.de>
> ---
>  arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
> index 327ff97..fe142dc 100644
> --- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
> +++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
> @@ -263,6 +263,7 @@ void nand_boot(void)
>  #if defined(CONFIG_NAND_SPL)
>  void board_init_f(ulong bootflag)
>  #else
> +__attribute__ ((weak))
>  int arch_cpu_init(void)
>  #endif

Stop please.

This whole code is crap and needs to be changed.


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");

What is #0x00002300? #0x00000087? #0x00000002? #0x00001000?

293         writel(0x83e70b13, &davinci_syscfg_regs->kick0);
294         writel(0x95a4f1e0, &davinci_syscfg_regs->kick1);

What is 0x83e70b13? 0x95a4f1e0?

296         dv_maskbits(&davinci_syscfg_regs->suspsrc,
297                 ((1 << 27) | (1 << 22) | (1 << 20) | (1 << 5) | (1 << 16)));

What is ((1 << 27) | (1 << 22) | (1 << 20) | (1 << 5) | (1 << 16))) ?

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));

What is CONFIG_SYS_NS16550_COM1 + 0x30 ???? don't we have proper
NS16550 register definitions?  And what is 0x00006001 ??


All this needs a _thorough_ cleanup before I'm willing to accept this
for mainline.


Heiko, Christian: please negotiate who performs which part of the
cleanup. But I expect that with proper symbolic names instead of the
hardwired constants the need for a wek function will go away.

Albert, please make sure to block this current code. I do not want to
see this in mainline as is.


Thanks.

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@denx.de
Another megabytes the dust.

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

[Christian Riesch]

Hi Wolfgang,

On Wed, Nov 9, 2011 at 11:12 AM, Wolfgang Denk <wd@denx.de> wrote:
> In message <1320830586-19124-1-git-send-email-christian.riesch@omicron.at> you wrote:
>> This patch allows replacing arch_cpu_init() if a custom initialization
>> is required.
>>
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> Cc: Sandeep Paulraj <s-paulraj@ti.com>
>> Cc: Heiko Schocher <hs@denx.de>
>> ---
>> ?arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c | ? ?1 +
>> ?1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> index 327ff97..fe142dc 100644
>> --- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> +++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> @@ -263,6 +263,7 @@ void nand_boot(void)
>> ?#if defined(CONFIG_NAND_SPL)
>> ?void board_init_f(ulong bootflag)
>> ?#else
>> +__attribute__ ((weak))
>> ?int arch_cpu_init(void)
>> ?#endif
>
> Stop please.
>
> This whole code is crap and needs to be changed.
>
>
> 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");
>
> What is #0x00002300? #0x00000087? #0x00000002? #0x00001000?
>
> 293 ? ? ? ? writel(0x83e70b13, &davinci_syscfg_regs->kick0);
> 294 ? ? ? ? writel(0x95a4f1e0, &davinci_syscfg_regs->kick1);
>
> What is 0x83e70b13? 0x95a4f1e0?
>
> 296 ? ? ? ? dv_maskbits(&davinci_syscfg_regs->suspsrc,
> 297 ? ? ? ? ? ? ? ? ((1 << 27) | (1 << 22) | (1 << 20) | (1 << 5) | (1 << 16)));
>
> What is ((1 << 27) | (1 << 22) | (1 << 20) | (1 << 5) | (1 << 16))) ?
>
> 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));
>
> What is CONFIG_SYS_NS16550_COM1 + 0x30 ???? don't we have proper
> NS16550 register definitions? ?And what is 0x00006001 ??
>
>
> All this needs a _thorough_ cleanup before I'm willing to accept this
> for mainline.

This is already in mainline, see

commit 310ae55efe14aa7923b16c718cbdb22ec364b18b
Author: Heiko Schocher <hs@denx.de>
Date:   Wed Sep 14 19:59:38 2011 +0000

    arm, davinci, am1808: add lowlevel functions for booting from NOR

Regards, Christian

>
>
> Heiko, Christian: please negotiate who performs which part of the
> cleanup. But I expect that with proper symbolic names instead of the
> hardwired constants the need for a wek function will go away.
>
> Albert, please make sure to block this current code. I do not want to
> see this in mainline as is.
>
>
> Thanks.
>
> 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
> Another megabytes the dust.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

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

[Wolfgang Denk]

Dear Christian Riesch,

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?

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
Humanity has the  stars  in  its  future,  and  that  future  is  too
important  to be lost under the burden of juvenile folly and ignorant
superstition.                                          - Isaac Asimov

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

[Heiko Schocher]

Hello Wolfgang, Christian,

Wolfgang Denk wrote:
> Dear Christian Riesch,
> 
> In message <1320830586-19124-1-git-send-email-christian.riesch@omicron.at> you wrote:
>> This patch allows replacing arch_cpu_init() if a custom initialization
>> is required.
>>
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> Cc: Sandeep Paulraj <s-paulraj@ti.com>
>> Cc: Heiko Schocher <hs@denx.de>
>> ---
>>  arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> index 327ff97..fe142dc 100644
>> --- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> +++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
>> @@ -263,6 +263,7 @@ void nand_boot(void)
>>  #if defined(CONFIG_NAND_SPL)
>>  void board_init_f(ulong bootflag)
>>  #else
>> +__attribute__ ((weak))
>>  int arch_cpu_init(void)
>>  #endif
> 
> Stop please.
> 
> This whole code is crap and needs to be changed.
[...]
> Heiko, Christian: please negotiate who performs which part of the
> cleanup. But I expect that with proper symbolic names instead of the
> hardwired constants the need for a wek function will go away.

I rework this.

> Albert, please make sure to block this current code. I do not want to
> see this in mainline as is.

Is it OK, if I send a cleanup patch against current u-boot-arm.git?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

[Christian Riesch]

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?

 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.

 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

 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.

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

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

 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.

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

 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?

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

 351         return 0;
 352 #endif
 353 }

Regards, Christian

[1] http://lists.denx.de/pipermail/u-boot/2011-November/109104.html

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

[Christian Riesch]

Hello Heiko,

On Wed, Nov 9, 2011 at 11:44 AM, Heiko Schocher <hs@denx.de> wrote:
>> Heiko, Christian: please negotiate who performs which part of the
>> cleanup. But I expect that with proper symbolic names instead of the
>> hardwired constants the need for a wek function will go away.
>
> I rework this.

I'd like to help but since we have different custom boards it's a bit
hard to test. Therefore I'd like to get this lowlevel initialization
working on the da850evm board. Since it boots from SPI I guess I'll
need an SPL for SPI flash, right? Could we try to clean it up in a way
so it nicely integrates with your SPL code in
arch/arm/cpu/arm926ejs/davinci/spl.c?

Regards, Christian

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

[Heiko Schocher]

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

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

[Heiko Schocher]

Hello Christian,

Christian Riesch wrote:
> Hello Heiko,
> 
> On Wed, Nov 9, 2011 at 11:44 AM, Heiko Schocher <hs@denx.de> wrote:
>>> Heiko, Christian: please negotiate who performs which part of the
>>> cleanup. But I expect that with proper symbolic names instead of the
>>> hardwired constants the need for a wek function will go away.
>> I rework this.
> 
> I'd like to help but since we have different custom boards it's a bit
> hard to test. Therefore I'd like to get this lowlevel initialization

Do you plan to post your patches for your custom board to the ML?

> working on the da850evm board. Since it boots from SPI I guess I'll
> need an SPL for SPI flash, right? Could we try to clean it up in a way

Yes.

> so it nicely integrates with your SPL code in
> arch/arm/cpu/arm926ejs/davinci/spl.c?

I soon post the cleanup for the da850_lowlevel.c file, so you can
do this based on that patch. You only have to adapt board_init_f() in
arch/arm/cpu/arm926ejs/davinci/spl.c. There for the da850 case,
arch_cpu_init() or for the spl case better called da850_lowlevel_init()
from arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c must be called.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

[Christian Riesch]

Hello Heiko,

On Wed, Nov 9, 2011 at 1:27 PM, Heiko Schocher <hs@denx.de> wrote:
> Christian Riesch wrote:
>> On Wed, Nov 9, 2011 at 11:44 AM, Heiko Schocher <hs@denx.de> wrote:
>>>> Heiko, Christian: please negotiate who performs which part of the
>>>> cleanup. But I expect that with proper symbolic names instead of the
>>>> hardwired constants the need for a wek function will go away.
>>> I rework this.
>>
>> I'd like to help but since we have different custom boards it's a bit
>> hard to test. Therefore I'd like to get this lowlevel initialization
>
> Do you plan to post your patches for your custom board to the ML?

I'm not sure yet if it makes sense... Of course we will publish the
code to comply with GPL once the product development is finished. But
I would rather provide the features that I develop for my custom board
for the da850evm, so everyone with an evaluation board can test them
and adapt them for their own boards.

>
>> working on the da850evm board. Since it boots from SPI I guess I'll
>> need an SPL for SPI flash, right? Could we try to clean it up in a way
>
> Yes.
>
>> so it nicely integrates with your SPL code in
>> arch/arm/cpu/arm926ejs/davinci/spl.c?
>
> I soon post the cleanup for the da850_lowlevel.c file, so you can
> do this based on that patch. You only have to adapt board_init_f() in
> arch/arm/cpu/arm926ejs/davinci/spl.c. There for the da850 case,
> arch_cpu_init() or for the spl case better called da850_lowlevel_init()
> from arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c must be called.

Great! I'll try to do that.
Thanks for your comments!
Christian

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

[Christian Riesch]

Hello Heiko,

On Wed, Nov 9, 2011 at 1:18 PM, Heiko Schocher <hs@denx.de> wrote:
> Christian Riesch wrote:
>> ?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.

Yes, but it will also be done later in serial_init() called from
board_init_f(), right? So this is only a hack to allow debug messages
at this stage before it is done properly in serial_init(). Am I
missing something here?
Regards, Christian