[U-Boot] [RFC PATCH] arm, davinci: Remove board specific code from da850_lowlevel.c

[U-Boot] [RFC PATCH] arm, davinci: Remove board specific code from da850_lowlevel.c

[Christian Riesch]

Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
---

Hello Heiko,

On my board I cannot use your code in arch_cpu_init() in da850_lowlevel.c
since I have different versions of my board with different input
clock frequencies. Here u-boot should first determine the board
revision number and then configure the SoC accordingly. Therefore I would 
like to move all board-specific parts (and PLL and memory configuration is 
board-specific since it depends on the memory chips and oscillators 
deployed on the board) to board_early_init_f which is called right after 
arch_cpu_init() and keep only a few initializiation steps in arch_cpu_init().

This patch applies on top of

[U-Boot,v3,1/2] arm, davinci: Rename AM1808 lowlevel functions to DA850
http://patchwork.ozlabs.org/patch/124291/

[U-Boot,v3,2/2] arm, davinci: Remove the duplication of LPSC functions in da850_lowlevel.c
http://patchwork.ozlabs.org/patch/124290/

[U-Boot] arm, davinci: Fix setting of the SDRAM configuration register
http://patchwork.ozlabs.org/patch/124289/

Thank you for your comments!
Best regards, Christian

 arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c |   71 -----------------------
 1 files changed, 0 insertions(+), 71 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
index 6f72491..04dfa34 100644
--- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
+++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c
@@ -23,7 +23,6 @@
  */
 #include <common.h>
 #include <nand.h>
-#include <ns16550.h>
 #include <post.h>
 #include <asm/arch/da850_lowlevel.h>
 #include <asm/arch/hardware.h>
@@ -231,19 +230,6 @@ int da850_ddr_setup(unsigned int freq)
 	return 0;
 }
 
-void da850_pinmux_ctl(unsigned long offset, unsigned long mask,
-	unsigned long value)
-{
-	clrbits_le32(&davinci_syscfg_regs->pinmux[offset], mask);
-	setbits_le32(&davinci_syscfg_regs->pinmux[offset], (mask & value));
-}
-
-__attribute__((weak))
-void board_gpio_init(void)
-{
-	return;
-}
-
 #if defined(CONFIG_NAND_SPL)
 void nand_boot(void)
 {
@@ -260,11 +246,7 @@ void nand_boot(void)
 }
 #endif
 
-#if defined(CONFIG_NAND_SPL)
-void board_init_f(ulong bootflag)
-#else
 int arch_cpu_init(void)
-#endif
 {
 	/*
 	 * copied from arch/arm/cpu/arm926ejs/start.S
@@ -296,58 +278,5 @@ int arch_cpu_init(void)
 	dv_maskbits(&davinci_syscfg_regs->suspsrc,
 		((1 << 27) | (1 << 22) | (1 << 20) | (1 << 5) |	(1 << 16)));
 
-	/* Setup Pinmux */
-	da850_pinmux_ctl(0, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX0);
-	da850_pinmux_ctl(1, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX1);
-	da850_pinmux_ctl(2, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX2);
-	da850_pinmux_ctl(3, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX3);
-	da850_pinmux_ctl(4, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX4);
-	da850_pinmux_ctl(5, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX5);
-	da850_pinmux_ctl(6, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX6);
-	da850_pinmux_ctl(7, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX7);
-	da850_pinmux_ctl(8, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX8);
-	da850_pinmux_ctl(9, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX9);
-	da850_pinmux_ctl(10, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX10);
-	da850_pinmux_ctl(11, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX11);
-	da850_pinmux_ctl(12, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX12);
-	da850_pinmux_ctl(13, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX13);
-	da850_pinmux_ctl(14, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX14);
-	da850_pinmux_ctl(15, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX15);
-	da850_pinmux_ctl(16, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX16);
-	da850_pinmux_ctl(17, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX17);
-	da850_pinmux_ctl(18, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX18);
-	da850_pinmux_ctl(19, 0xFFFFFFFF, CONFIG_SYS_DA850_PINMUX19);
-
-	/* PLL setup */
-	da850_pll_init(davinci_pllc0_regs, CONFIG_SYS_DA850_PLL0_PLLM);
-	da850_pll_init(davinci_pllc1_regs, CONFIG_SYS_DA850_PLL1_PLLM);
-
-	/* GPIO setup */
-	board_gpio_init();
-
-	/* setup CSn config */
-	writel(CONFIG_SYS_DA850_CS2CFG, &davinci_emif_regs->ab1cr);
-	writel(CONFIG_SYS_DA850_CS3CFG, &davinci_emif_regs->ab2cr);
-
-	lpsc_on(DAVINCI_LPSC_UART2);
-	NS16550_init((NS16550_t)(CONFIG_SYS_NS16550_COM1),
-			CONFIG_SYS_NS16550_CLK / 16 / CONFIG_BAUDRATE);
-
-	/*
-	 * Fix Power and Emulation Management Register
-	 * see sprufw3a.pdf page 37 Table 24
-	 */
-	writel(readl((CONFIG_SYS_NS16550_COM1 + 0x30)) | 0x00006001,
-		(CONFIG_SYS_NS16550_COM1 + 0x30));
-#if defined(CONFIG_NAND_SPL)
-	puts("ddr init\n");
-	da850_ddr_setup(132);
-
-	puts("boot u-boot ...\n");
-
-	nand_boot();
-#else
-	da850_ddr_setup(132);
 	return 0;
-#endif
 }
-- 
1.7.0.4

[U-Boot] [RFC PATCH] arm, davinci: Remove board specific code from da850_lowlevel.c

[Heiko Schocher]

Hello Christian,

Christian Riesch wrote:
> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
> ---
> 
> Hello Heiko,
> 
> On my board I cannot use your code in arch_cpu_init() in da850_lowlevel.c
> since I have different versions of my board with different input
> clock frequencies. Here u-boot should first determine the board
> revision number and then configure the SoC accordingly. Therefore I would 
> like to move all board-specific parts (and PLL and memory configuration is 
> board-specific since it depends on the memory chips and oscillators 
> deployed on the board) to board_early_init_f which is called right after 
> arch_cpu_init() and keep only a few initializiation steps in arch_cpu_init().

Ok, if you have other needs on your board we must change something.
But I want to prevent, that all the code you remove in your patch is
moved to board code, so all boards must (copy?) it. Instead you should
move it to a weak function, which you can replace for your specific
needs. (I thought the code was common enough ...)

What do you think?

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

[U-Boot] [RFC PATCH] arm, davinci: Remove board specific code from da850_lowlevel.c

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4EBA11A9.6060905@denx.de> you wrote:
>
> > On my board I cannot use your code in arch_cpu_init() in da850_lowlevel.c
> > since I have different versions of my board with different input
> > clock frequencies. Here u-boot should first determine the board
> > revision number and then configure the SoC accordingly. Therefore I would 
> > like to move all board-specific parts (and PLL and memory configuration is 
> > board-specific since it depends on the memory chips and oscillators 
> > deployed on the board) to board_early_init_f which is called right after 
> > arch_cpu_init() and keep only a few initializiation steps in arch_cpu_init().
> 
> Ok, if you have other needs on your board we must change something.
> But I want to prevent, that all the code you remove in your patch is
> moved to board code, so all boards must (copy?) it. Instead you should
> move it to a weak function, which you can replace for your specific
> needs. (I thought the code was common enough ...)

I guess in most cases no different code will be needed, but just
different configuration parameters.  It should be possible to handle
these through settings in the board config header file.

In any case, a duplication of that coede shall be avoided.

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
The following statement is not true.  The previous statement is true.

[U-Boot] [RFC PATCH] arm, davinci: Remove board specific code from da850_lowlevel.c

[Christian Riesch]

Hello Heiko,
Thank you for your reply!

On Wed, Nov 9, 2011 at 6:37 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello Christian,
>
> Christian Riesch wrote:
>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>> ---
>>
>> Hello Heiko,
>>
>> On my board I cannot use your code in arch_cpu_init() in da850_lowlevel.c
>> since I have different versions of my board with different input
>> clock frequencies. Here u-boot should first determine the board
>> revision number and then configure the SoC accordingly. Therefore I would
>> like to move all board-specific parts (and PLL and memory configuration is
>> board-specific since it depends on the memory chips and oscillators
>> deployed on the board) to board_early_init_f which is called right after
>> arch_cpu_init() and keep only a few initializiation steps in arch_cpu_init().
>
> Ok, if you have other needs on your board we must change something.
> But I want to prevent, that all the code you remove in your patch is
> moved to board code, so all boards must (copy?) it. Instead you should
> move it to a weak function, which you can replace for your specific
> needs. (I thought the code was common enough ...)
>
> What do you think?

That's fine for me, I tried it now with

__attribute__((weak))  int arch_cpu_init(void)

and added my own arch_cpu_init() to my board specific file.

Of course I had to add all those CONFIG_SYS_DA850_PINMUXn with dummy
values to my board config file to make it build... I don't like those
defines because the values are difficult to read and we already have
code for pinmuxing in board/davinci/common/davinci_pinmux.c so it is
duplicate code. Couldn't we move this davinci_pinmux.c to
arch/arm/cpu/arm926ejs/davinci and use it instead? What do you think?

I'll submit a patch for adding the weak attribute.

Regards, Christian

[U-Boot] [RFC PATCH] arm, davinci: Remove board specific code from da850_lowlevel.c

[Heiko Schocher]

Hello Christian,

Christian Riesch wrote:
> Hello Heiko,
> Thank you for your reply!
> 
> On Wed, Nov 9, 2011 at 6:37 AM, Heiko Schocher <hs@denx.de> wrote:
>> Hello Christian,
>>
>> Christian Riesch wrote:
>>> Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
>>> ---
>>>
>>> Hello Heiko,
>>>
>>> On my board I cannot use your code in arch_cpu_init() in da850_lowlevel.c
>>> since I have different versions of my board with different input
>>> clock frequencies. Here u-boot should first determine the board
>>> revision number and then configure the SoC accordingly. Therefore I would
>>> like to move all board-specific parts (and PLL and memory configuration is
>>> board-specific since it depends on the memory chips and oscillators
>>> deployed on the board) to board_early_init_f which is called right after
>>> arch_cpu_init() and keep only a few initializiation steps in arch_cpu_init().
>> Ok, if you have other needs on your board we must change something.
>> But I want to prevent, that all the code you remove in your patch is
>> moved to board code, so all boards must (copy?) it. Instead you should
>> move it to a weak function, which you can replace for your specific
>> needs. (I thought the code was common enough ...)
>>
>> What do you think?
> 
> That's fine for me, I tried it now with
> 
> __attribute__((weak))  int arch_cpu_init(void)
> 
> and added my own arch_cpu_init() to my board specific file.
> 
> Of course I had to add all those CONFIG_SYS_DA850_PINMUXn with dummy
> values to my board config file to make it build... I don't like those
> defines because the values are difficult to read and we already have

Why? They are in sync with the doc ...

> code for pinmuxing in board/davinci/common/davinci_pinmux.c so it is
> duplicate code. Couldn't we move this davinci_pinmux.c to
> arch/arm/cpu/arm926ejs/davinci and use it instead? What do you think?

Hmm.. if you use the http://www-s.ti.com/sc/techlit/spraba2.zip
utility from ti, you exactly get the values for the defines ...
so I prefered to go this way, but have no real objections against
using the code from board/davinci/common/davinci_pinmux.c
(Of course, it should be moved to arch/arm/cpu/arm926ejs/davinci)

added Sandeep to cc. Sandeep, what do you think?

> I'll submit a patch for adding the weak attribute.

Thanks!

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

[U-Boot] [RFC PATCH] arm, davinci: Remove board specific code from da850_lowlevel.c

[Paulraj, Sandeep]

> 
> Why? They are in sync with the doc ...
> 
> > code for pinmuxing in board/davinci/common/davinci_pinmux.c so it is
> > duplicate code. Couldn't we move this davinci_pinmux.c to
> > arch/arm/cpu/arm926ejs/davinci and use it instead? What do you think?
> 
> Hmm.. if you use the http://www-s.ti.com/sc/techlit/spraba2.zip
> utility from ti, you exactly get the values for the defines ...
> so I prefered to go this way, but have no real objections against
> using the code from board/davinci/common/davinci_pinmux.c
> (Of course, it should be moved to arch/arm/cpu/arm926ejs/davinci)
> 
> added Sandeep to cc. Sandeep, what do you think?
That is fine
Should be moved to arch/arm/cpu/arm926ejs/davinci
> 
> > I'll submit a patch for adding the weak attribute.
> 
> Thanks!