Re: [U-Boot-Users] Request: s3c24xx getting its own start.S file ?

I meanwhile realized that the entire makefile system doesn't really cope with the fact. So what I'm left with is something along the lines of the attached patch, where the cpu/arm920t/start.S #includes a cpu/arm920t/s3c24x0/start.S file.
It's not really nice, but otherwise I assure you anyone touching the arm920t start.S file again will find itself in #ifdef/endif hell, once all my s3c24xx related patches would be merged...
Index: u-boot/cpu/arm920t/s3c24x0/start.S =================================================================== --- /dev/null +++ u-boot/cpu/arm920t/s3c24x0/start.S @@ -0,0 +1,155 @@ +/* + * armboot - Startup Code for S3C24xx CPU-cores + * + * + */ + + +/* the actual start code */ + +start_code: + /* + * set the cpu to SVC32 mode + */ + mrs r0,cpsr + bic r0,r0,#0x1f + orr r0,r0,#0xd3 + msr cpsr,r0 + + bl coloured_LED_init + bl red_LED_on + + /* turn off the watchdog */ + +# if defined(CONFIG_S3C2400) +# define pWTCON 0x15300000 +# define INTMSK 0x14400008 /* Interupt-Controller base addresses */ +# define CLKDIVN 0x14800014 /* clock divisor register */ +#else +# define pWTCON 0x53000000 +# define INTMSK 0x4A000008 /* Interupt-Controller base addresses */ +# define INTSUBMSK 0x4A00001C +# define CLKDIVN 0x4C000014 /* clock divisor register */ +# endif + + ldr r0, =pWTCON + mov r1, #0x0 + str r1, [r0] + + /* + * mask all IRQs by setting all bits in the INTMR - default + */ + mov r1, #0xffffffff + ldr r0, =INTMSK + str r1, [r0] +# if defined(CONFIG_S3C2410) + ldr r1, =0x3ff + ldr r0, =INTSUBMSK + str r1, [r0] +# endif + + /* FCLK:HCLK:PCLK = 1:2:4 */ + /* default FCLK is 120 MHz ! */ + ldr r0, =CLKDIVN + mov r1, #3 + str r1, [r0] + + /* + * we do sys-critical inits only at reboot, + * not when booting from ram! + */ +#ifndef CONFIG_SKIP_LOWLEVEL_INIT + bl cpu_init_crit +#endif + + +#ifndef CONFIG_SKIP_RELOCATE_UBOOT +relocate: /* relocate U-Boot to RAM */ + adr r0, _start /* r0 <- current position of code */ + ldr r1, _TEXT_BASE /* test if we run from flash or RAM */ + cmp r0, r1 /* don't reloc during debug */ + beq stack_setup + + ldr r2, _armboot_start + ldr r3, _bss_start + sub r2, r3, r2 /* r2 <- size of armboot */ + add r2, r0, r2 /* r2 <- source end address */ + +copy_loop: + ldmia r0!, {r3-r10} /* copy from source address [r0] */ + stmia r1!, {r3-r10} /* copy to target address [r1] */ + cmp r0, r2 /* until source end addreee [r2] */ + ble copy_loop +#endif /* CONFIG_SKIP_RELOCATE_UBOOT */ + + /* Set up the stack */ +stack_setup: + ldr r0, _TEXT_BASE /* upper 128 KiB: relocated uboot */ + sub r0, r0, #CFG_MALLOC_LEN /* malloc area */ + sub r0, r0, #CFG_GBL_DATA_SIZE /* bdinfo */ +#ifdef CONFIG_USE_IRQ + sub r0, r0, #(CONFIG_STACKSIZE_IRQ+CONFIG_STACKSIZE_FIQ) +#endif + sub sp, r0, #12 /* leave 3 words for abort-stack */ + +clear_bss: + ldr r0, _bss_start /* find start of bss segment */ + ldr r1, _bss_end /* stop here */ + mov r2, #0x00000000 /* clear */ + +clbss_l:str r2, [r0] /* clear loop... */ + add r0, r0, #4 + cmp r0, r1 + ble clbss_l + + ldr pc, _start_armboot + +_start_armboot: .word start_armboot + + +/* + ************************************************************************* + * + * CPU_init_critical registers + * + * setup important registers + * setup memory timing + * + ************************************************************************* + */ + + +#ifndef CONFIG_SKIP_LOWLEVEL_INIT +cpu_init_crit: + /* + * flush v4 I/D caches + */ + mov r0, #0 + mcr p15, 0, r0, c7, c7, 0 /* flush v3/v4 cache */ + mcr p15, 0, r0, c8, c7, 0 /* flush v4 TLB */ + + /* + * disable MMU stuff and caches + */ + mrc p15, 0, r0, c1, c0, 0 + bic r0, r0, #0x00002300 @ clear bits 13, 9:8 (--V- --RS) + bic r0, r0, #0x00000087 @ clear bits 7, 2:0 (B--- -CAM) + orr r0, r0, #0x00000002 @ set bit 2 (A) Align + orr r0, r0, #0x00001000 @ set bit 12 (I) I-Cache + mcr p15, 0, r0, c1, c0, 0 + + /* + * before relocating, we have to setup RAM timing + * because memory timing is board-dependend, you will + * find a lowlevel_init.S in your board directory. + */ + mov ip, lr + + bl lowlevel_init + + mov lr, ip + mov pc, lr +#endif /* CONFIG_SKIP_LOWLEVEL_INIT */ + + + Index: u-boot/cpu/arm920t/start.S =================================================================== --- u-boot.orig/cpu/arm920t/start.S +++ u-boot/cpu/arm920t/start.S @@ -103,6 +103,10 @@ #endif
+#if defined(CONFIG_S3C24xx) +#include "s3c24x0/start.S" +#else + /* * the actual start code */ @@ -133,43 +137,6 @@ bne copyex #endif
-#if defined(CONFIG_S3C2400) || defined(CONFIG_S3C2410) - /* turn off the watchdog */ - -# if defined(CONFIG_S3C2400) -# define pWTCON 0x15300000 -# define INTMSK 0x14400008 /* Interupt-Controller base addresses */ -# define CLKDIVN 0x14800014 /* clock divisor register */ -#else -# define pWTCON 0x53000000 -# define INTMSK 0x4A000008 /* Interupt-Controller base addresses */ -# define INTSUBMSK 0x4A00001C -# define CLKDIVN 0x4C000014 /* clock divisor register */ -# endif - - ldr r0, =pWTCON - mov r1, #0x0 - str r1, [r0] - - /* - * mask all IRQs by setting all bits in the INTMR - default - */ - mov r1, #0xffffffff - ldr r0, =INTMSK - str r1, [r0] -# if defined(CONFIG_S3C2410) - ldr r1, =0x3ff - ldr r0, =INTSUBMSK - str r1, [r0] -# endif - - /* FCLK:HCLK:PCLK = 1:2:4 */ - /* default FCLK is 120 MHz ! */ - ldr r0, =CLKDIVN - mov r1, #3 - str r1, [r0] -#endif /* CONFIG_S3C2400 || CONFIG_S3C2410 */ - /* * we do sys-critical inits only at reboot, * not when booting from ram! @@ -270,6 +237,8 @@ mov pc, lr #endif /* CONFIG_SKIP_LOWLEVEL_INIT */
+#endif /* S3C24xx */ + /* ************************************************************************* * Index: u-boot/include/configs/VCMA9.h =================================================================== --- u-boot.orig/include/configs/VCMA9.h +++ u-boot/include/configs/VCMA9.h @@ -35,6 +35,7 @@ */ #define CONFIG_ARM920T 1 /* This is an ARM920T Core */ #define CONFIG_S3C2410 1 /* in a SAMSUNG S3C2410 SoC */ +#define CONFIG_S3C24xx 1 /* in a SAMSUNG S3C24xx family */ #define CONFIG_VCMA9 1 /* on a MPL VCMA9 Board */ #define LITTLEENDIAN 1 /* used by usb_ohci.c */
Index: u-boot/include/configs/sbc2410x.h =================================================================== --- u-boot.orig/include/configs/sbc2410x.h +++ u-boot/include/configs/sbc2410x.h @@ -45,6 +45,7 @@ */ #define CONFIG_ARM920T 1 /* This is an ARM920T Core */ #define CONFIG_S3C2410 1 /* in a SAMSUNG S3C2410 SoC */ +#define CONFIG_S3C24xx 1 /* in a SAMSUNG S3C24xx family */ #define CONFIG_SBC2410X 1 /* on a friendly-arm SBC-2410X Board */
/* input clock of PLL */ Index: u-boot/include/configs/smdk2400.h =================================================================== --- u-boot.orig/include/configs/smdk2400.h +++ u-boot/include/configs/smdk2400.h @@ -36,6 +36,7 @@ */ #define CONFIG_ARM920T 1 /* This is an ARM920T core */ #define CONFIG_S3C2400 1 /* in a SAMSUNG S3C2400 SoC */ +#define CONFIG_S3C24xx 1 /* in a SAMSUNG S3C24xx family */ #define CONFIG_SMDK2400 1 /* on an SAMSUNG SMDK2400 Board */
/* input clock of PLL */ Index: u-boot/include/configs/smdk2410.h =================================================================== --- u-boot.orig/include/configs/smdk2410.h +++ u-boot/include/configs/smdk2410.h @@ -35,6 +35,7 @@ */ #define CONFIG_ARM920T 1 /* This is an ARM920T Core */ #define CONFIG_S3C2410 1 /* in a SAMSUNG S3C2410 SoC */ +#define CONFIG_S3C24xx 1 /* in a SAMSUNG S3C24xx family */ #define CONFIG_SMDK2410 1 /* on a SAMSUNG SMDK2410 Board */
/* input clock of PLL */

In message 20080706164700.GE20299@prithivi.gnumonks.org you wrote:
I meanwhile realized that the entire makefile system doesn't really cope with the fact. So what I'm left with is something along the lines of
Please explain what's the problem...
the attached patch, where the cpu/arm920t/start.S #includes a cpu/arm920t/s3c24x0/start.S file.
It's not really nice, but otherwise I assure you anyone touching the arm920t start.S file again will find itself in #ifdef/endif hell, once all my s3c24xx related patches would be merged...
I really dislike the code duplication.
+/*
- armboot - Startup Code for S3C24xx CPU-cores
You're posting this on the wrong mailing list, then. Did you mean armboot-users@lists.sourceforge.net ? ;-)
...
+#ifndef CONFIG_SKIP_RELOCATE_UBOOT +relocate: /* relocate U-Boot to RAM */
- adr r0, _start /* r0 <- current position of code */
- ldr r1, _TEXT_BASE /* test if we run from flash or RAM */
- cmp r0, r1 /* don't reloc during debug */
- beq stack_setup
- ldr r2, _armboot_start
armboot?
- ldr pc, _start_armboot
+_start_armboot: .word start_armboot
?
--- u-boot.orig/cpu/arm920t/start.S +++ u-boot/cpu/arm920t/start.S @@ -103,6 +103,10 @@ #endif
+#if defined(CONFIG_S3C24xx) +#include "s3c24x0/start.S" +#else
Why cannot we handle this on a Makefile level?
--- u-boot.orig/include/configs/VCMA9.h +++ u-boot/include/configs/VCMA9.h @@ -35,6 +35,7 @@ */ #define CONFIG_ARM920T 1 /* This is an ARM920T Core */ #define CONFIG_S3C2410 1 /* in a SAMSUNG S3C2410 SoC */ +#define CONFIG_S3C24xx 1 /* in a SAMSUNG S3C24xx family */
Is this really needed? Should not CONFIG_S3C2410 automatically include setting CONFIG_S3C24xx ?
Best regards,
Wolfgang Denk

On Sun, Jul 06, 2008 at 08:44:52PM +0200, Wolfgang Denk wrote:
In message 20080706164700.GE20299@prithivi.gnumonks.org you wrote:
I meanwhile realized that the entire makefile system doesn't really cope with the fact. So what I'm left with is something along the lines of
Please explain what's the problem...
the problem is tha the main makefile always wants to build cpu/arm920t/start.o, and I don't see an easy way how it could be modified to build cpu/arm920t/s3c24x0/start.o instead. At least until now I don't see an infrastructure for this.
the attached patch, where the cpu/arm920t/start.S #includes a cpu/arm920t/s3c24x0/start.S file.
It's not really nice, but otherwise I assure you anyone touching the arm920t start.S file again will find itself in #ifdef/endif hell, once all my s3c24xx related patches would be merged...
I really dislike the code duplication.
same here. but I'd rather duplicate some 50-100 lines than have 300 lines of completely unreadable #ifdef hell.
- armboot - Startup Code for S3C24xx CPU-cores
You're posting this on the wrong mailing list, then. Did you mean armboot-users@lists.sourceforge.net ? ;-)
please look into your own u-boot source, the entire arm start.S file looks like this, and I just copy+pasted that 'standard'
--- u-boot.orig/include/configs/VCMA9.h +++ u-boot/include/configs/VCMA9.h @@ -35,6 +35,7 @@ */ #define CONFIG_ARM920T 1 /* This is an ARM920T Core */ #define CONFIG_S3C2410 1 /* in a SAMSUNG S3C2410 SoC */ +#define CONFIG_S3C24xx 1 /* in a SAMSUNG S3C24xx family */
Is this really needed? Should not CONFIG_S3C2410 automatically include setting CONFIG_S3C24xx ?
this would have been my preferred choice. But only config.h is included by all the various header files. and config.h itself only includes board/foobar/config.h, i.e. there is no generic header file which gets preprocessed after the board-level config is included and which is still included from config.h. Should I invent one? I'm usually careful with doing things differently than it is already established in the u-boot project. And I've seen PPC examples just doing it like I did it now (defining the family directly in the board-level config). I agree, it's ugly and error prone...

Dear Jean-Christophe,
In message 20080707072909.GA4412@prithivi.gnumonks.org Harald Welte wrote:
the problem is tha the main makefile always wants to build cpu/arm920t/start.o, and I don't see an easy way how it could be modified to build cpu/arm920t/s3c24x0/start.o instead. At least until now I don't see an infrastructure for this.
the attached patch, where the cpu/arm920t/start.S #includes a cpu/arm920t/s3c24x0/start.S file.
It's not really nice, but otherwise I assure you anyone touching the arm920t start.S file again will find itself in #ifdef/endif hell, once all my s3c24xx related patches would be merged...
I really dislike the code duplication.
same here. but I'd rather duplicate some 50-100 lines than have 300 lines of completely unreadable #ifdef hell.
...
This falls mostly into your (i. e. ARM custodian) bailiwick. Could you please make sure that we don't lose this thread? Thanks.
Best regards,
Wolfgang Denk

Hi Jean-Christophe,
On Mon, Aug 18, 2008 at 11:03:32PM +0200, Wolfgang Denk wrote:
In message 20080707072909.GA4412@prithivi.gnumonks.org Harald Welte wrote:
the problem is tha the main makefile always wants to build cpu/arm920t/start.o, and I don't see an easy way how it could be modified to build cpu/arm920t/s3c24x0/start.o instead. At least until now I don't see an infrastructure for this.
the attached patch, where the cpu/arm920t/start.S #includes a cpu/arm920t/s3c24x0/start.S file.
It's not really nice, but otherwise I assure you anyone touching the arm920t start.S file again will find itself in #ifdef/endif hell, once all my s3c24xx related patches would be merged...
I really dislike the code duplication.
same here. but I'd rather duplicate some 50-100 lines than have 300 lines of completely unreadable #ifdef hell.
...
This falls mostly into your (i. e. ARM custodian) bailiwick. Could you please make sure that we don't lose this thread? Thanks.
JC: can you please comment on this?
I'd really like to 'fork' the arm920t/start.S in order to have a s3c24xx specific part. The many various boot related #ifdefs make the s3c24xx specific bit hard enough to read, not even talking about other arm920t based systems.
I have an entire pile of patches waiting for a decision on this. I've converted my s3c24xx related (and openmoko related) patches to work on top of a s3c24xx separate start.S now and I think the code looks much clearer (please see the original thread leading to this mail)
Also: Would you want to get my s3c24xx related work based on your ARM custodian tree, or based on the main u-boot tree?

On 11:31 Fri 22 Aug , Harald Welte wrote:
Hi Jean-Christophe,
On Mon, Aug 18, 2008 at 11:03:32PM +0200, Wolfgang Denk wrote:
In message 20080707072909.GA4412@prithivi.gnumonks.org Harald Welte wrote:
the problem is tha the main makefile always wants to build cpu/arm920t/start.o, and I don't see an easy way how it could be modified to build cpu/arm920t/s3c24x0/start.o instead. At least until now I don't see an infrastructure for this.
the attached patch, where the cpu/arm920t/start.S #includes a cpu/arm920t/s3c24x0/start.S file.
It's not really nice, but otherwise I assure you anyone touching the arm920t start.S file again will find itself in #ifdef/endif hell, once all my s3c24xx related patches would be merged...
I really dislike the code duplication.
same here. but I'd rather duplicate some 50-100 lines than have 300 lines of completely unreadable #ifdef hell.
...
This falls mostly into your (i. e. ARM custodian) bailiwick. Could you please make sure that we don't lose this thread? Thanks.
JC: can you please comment on this?
I'd really like to 'fork' the arm920t/start.S in order to have a s3c24xx specific part. The many various boot related #ifdefs make the s3c24xx specific bit hard enough to read, not even talking about other arm920t based systems.
I'm not against it, but need to do it in a way that can be done for other arch
so If you already have an idea how to architecture it go a head.
Also: Would you want to get my s3c24xx related work based on your ARM custodian tree, or based on the main u-boot tree?
based on arm tree or main I'll do the merge if needed
Best Regards, J.

On Sun, Jul 06, 2008 at 08:44:52PM +0200, Wolfgang Denk wrote:
the attached patch, where the cpu/arm920t/start.S #includes a cpu/arm920t/s3c24x0/start.S file.
It's not really nice, but otherwise I assure you anyone touching the arm920t start.S file again will find itself in #ifdef/endif hell, once all my s3c24xx related patches would be merged...
I really dislike the code duplication.
I am sympathetic to the cause. For reference, I'm attaching the s3c24xx specific start.S file of my internal tree _before_ s3c24a0, s3c2460, s3c2412 and s3c2443 suppor is merged. So this is for three CPU's, not yet for seven. And it has already way too many ifdefs for my taste. Also, the number of lines of code that I duplicated is probably something like 25-40 assembly instructions. We're not talking about massive amounts here.
I think the problem is a quite generic one. The same one exists for other ARM-core based SoCs, since multiple vendors use the same core, and every vendor (samsung, ti, ...) may have an entire product line each with their specific low-level initialization bits.
I'm having the same problem looking at the arm1136 based s3c6400 code... It is quite different but still somehow related to the cpu/arm1136/start.S code.
The only alternative to either having #ifdef hell or code duplication is the extensive use of macros. Either have one common start.S and put macro placeholders throughout the code, have the actual bits implemented per-cpu. Or have one start.S for each cpu and macros for the shared generic stuff.
Oh, and did I mention that samsung also shares code between s3c24xx and s3c64xx? This means that e.g. the boot-from-nand part of the s3c2443 and the s3c6400 might be identical, so we'd end up having code in cpu/arm920t/s3c24xx/ including/linking to code in cpu/arm1136/s3c64xx...
I'll probably end up with something like cpu/arm920t/s3c24xx 2400, 2410, 2412, 24a0, 2460, 2440, 2442, 2443 cpu/s3c/ generic stuff that is shared cpu/arm1136/s3c64xx 6400, later 6410
participants (3)
-
Harald Welte
-
Jean-Christophe PLAGNIOL-VILLARD
-
Wolfgang Denk