Hi Jean-Christophe
Comments below...
On 03/06/2009 00:35, Jean-Christophe PLAGNIOL-VILLARD wrote:
On 03:24 Sat 23 May , kevin.morfitt@fearnside-systems.co.uk wrote:
Implementation based on the existing u-boot support for S3C2410-based boards. u-boot programmed into NOR flash.
Tested on an SBC2440-II Board using tftp to copy the files from a server and programming them into NAND flash.
MAKEALL used to build all LIST_ARM9 targets only - no other architectures built as the changes only affect ARM9-based boards.
Signed-off-by: Kevin Morfitt kevin.morfitt@fearnside-systems.co.uk
please limit your comments to 80 chars per line could you split this patch in multiple subset as adding the 2440 support, addind the nand drivers and then adding the board.
This will really simplify the review
MAKEALL | 1 +
please add MAINTAINERS entry
<snip>
diff --git a/MAKEALL b/MAKEALL index c98d03a..e7235e4 100755
- */
+#include <config.h> +#include <version.h>
+/*
- Taken from linux/arch/arm/boot/compressed/head-s3c2410.S
- Copyright (C) 2002 Samsung Electronics SW.LEE hitchcar@sec.samsung.com
- */
+#define BWSCON 0x48000000
+#define DW8 (0x0) +#define DW16 (0x1) +#define DW32 (0x2) +#define WAIT (0x1 << 2) +#define UBLB (0x1 << 3)
what is all theses macro? for what use?
They make setting the fields of the memory controller registers more readable. For example BSWCON is the Bus Width and Wait Control Register, DW32 sets a Data Width value bit-field in BWSCON to 8-bit etc. They're used later in the patch to configure the memory controller registers. The names are the same as those used in the S3C2440 data sheet.
+#define B1_BWSCON (DW16) +#define B2_BWSCON (DW16) +#define B3_BWSCON (DW16 + WAIT + UBLB) +#define B4_BWSCON (DW16) +#define B5_BWSCON (DW16) +#define B6_BWSCON (DW32) +#define B7_BWSCON (DW32)
diff --git a/board/embest/sbc2440ii/sbc2440ii.c b/board/embest/sbc2440ii/sbc2440ii.c new file mode 100644 index 0000000..e0599dd --- /dev/null +++ b/board/embest/sbc2440ii/sbc2440ii.c @@ -0,0 +1,127 @@ +/*
- (C) Copyright 2002
- Sysgo Real-Time Solutions, GmbH <www.elinos.com>
- Marius Groeger mgroeger@sysgo.de
- (C) Copyright 2002
- David Mueller, ELSOFT AG, d.mueller@elsoft.ch
- (C) Copyright 2005
- JinHua Luo, GuangDong Linux Center, luo.jinhua@gd-linux.com
- Modified for the Embest SBC2440-II by
- (C) Copyright 2009
- Kevin Morfitt, Fearnside Systems Ltd, kevin.morfitt@fearnside-systems.co.uk
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
- */
+#include <common.h> +#include <s3c2440.h>
+DECLARE_GLOBAL_DATA_PTR;
+/* Configure the PLLs for MPLL = 400MHz, UPLL = 48MHz
- The clock frequency ratios are set to 1:4:8 ie:
PCLK = 50MHzHCLK = 100MHzFCLK = 400MHz- */
+/* The MPLL values. */ +#define M_MDIV 0x5C +#define M_PDIV 1 +#define M_SDIV 1
+/* The UPLL values. */ +#define U_MDIV 0x38 +#define U_PDIV 2 +#define U_SDIV 2
this need to be in the config.h as CONFIS_SYS_xxxxx
+/*
- Miscellaneous platform dependent initialisations
- */
+static inline void pll_settle_delay(unsigned long loops) +{
- __asm__ volatile ("1:\n"
"subs %0, %1, #1\n""bne 1b" : "=r" (loops) : "0" (loops));+}
+int board_init(void) +{
- S3C24X0_CLOCK_POWER * const clk_power = S3C24X0_GetBase_CLOCK_POWER();
- S3C24X0_GPIO * const gpio = S3C24X0_GetBase_GPIO();
- /* to reduce PLL lock time, adjust the LOCKTIME register */
- clk_power->LOCKTIME = 0xFFFFFFFF;
- /* configure UPLL */
- clk_power->UPLLCON = ((U_MDIV << 12) + (U_PDIV << 4) + U_SDIV);
- /* some delay between UPLL and MPLL */
- pll_settle_delay(8000);
- /* configure MPLL */
- clk_power->MPLLCON = ((M_MDIV << 12) + (M_PDIV << 4) + M_SDIV);
- /* configure the GPIO */
- gpio->GPACON = 0x007FFFFF;
what means all these hardcoded values?
All they really do is set the GPIO's to default states and switch on an LED. I'll change it to use #defines to make them more readable.
- gpio->GPBCON = 0x00055555;
- gpio->GPBUP = 0x000007FF;
- gpio->GPBDAT = 0x000001C0; /* Switch on LED1. */
- gpio->GPCCON = 0xAAAAAAAA;
- gpio->GPCUP = 0x0000FFFF;
- gpio->GPDCON = 0xAAAAAAAA;
<snip> +
- /* arch number of SBC2440-II Board */
- gd->bd->bi_arch_number = MACH_TYPE_SBC2440II;
please use tab for indent
- /* adress of boot parameters */
- gd->bd->bi_boot_params = 0x30000100;
please use this style RAM_BASE + 0x100
- icache_enable();
- dcache_enable();
do you support mmu? if not enable the dcache will have no effect
No mmu - I'll remove these lines.
- return 0;
+}
+int dram_init(void) +{
- gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
- gd->bd->bi_dram[0].size = PHYS_SDRAM_1_SIZE;
- return 0;
+} diff --git a/board/embest/sbc2440ii/u-boot.lds b/board/embest/sbc2440ii/u-boot.lds
please remove no more need
new file mode 100644 index 0000000..3b79776
<snip>
--- a/cpu/arm920t/s3c24x0/speed.c +++ b/cpu/arm920t/s3c24x0/speed.c @@ -30,12 +30,15 @@ */
#include <common.h> -#if defined(CONFIG_S3C2400) || defined (CONFIG_S3C2410) || defined (CONFIG_TRAB) +#if defined(CONFIG_S3C2400) || defined(CONFIG_S3C2410) || \
- defined(CONFIG_S3C2440) || defined(CONFIG_TRAB)
#if defined(CONFIG_S3C2400) #include <s3c2400.h> #elif defined(CONFIG_S3C2410) #include <s3c2410.h> +#elif defined(CONFIG_S3C2440) +#include <s3c2440.h> #endif
#define MPLL 0 @@ -53,49 +56,81 @@
static ulong get_PLLCLK(int pllreg) {
- S3C24X0_CLOCK_POWER * const clk_power = S3C24X0_GetBase_CLOCK_POWER();
- ulong r, m, p, s;
- if (pllreg == MPLL)
- r = clk_power->MPLLCON;
- else if (pllreg == UPLL)
- r = clk_power->UPLLCON;
- else
- hang();
- m = ((r & 0xFF000) >> 12) + 8;
- p = ((r & 0x003F0) >> 4) + 2;
- s = r & 0x3;
- return((CONFIG_SYS_CLK_FREQ * m) / (p << s));
- S3C24X0_CLOCK_POWER * const clk_power = S3C24X0_GetBase_CLOCK_POWER();
please do not mix codind style fix and new code in the same patch
- ulong r, m, p, s;
- if (pllreg == MPLL)
r = clk_power->MPLLCON;- else if (pllreg == UPLL)
r = clk_power->UPLLCON;- else
hang();- m = ((r & 0xFF000) >> 12) + 8;
- p = ((r & 0x003F0) >> 4) + 2;
- s = r & 0x3;
+#ifdef CONFIG_S3C2440
- if (pllreg == MPLL)
return (2 * CONFIG_SYS_CLK_FREQ * m) / (p << s);- else
please add the #else here
return (CONFIG_SYS_CLK_FREQ * m) / (p << s);+#else
- return (CONFIG_SYS_CLK_FREQ * m) / (p << s);
+#endif }
/* return FCLK frequency */ ulong get_FCLK(void) {
- return(get_PLLCLK(MPLL));
- return get_PLLCLK(MPLL);
}
/* return HCLK frequency */
please send a new version before continuing the review as you mix codind style fix and new code in the same patch it's really hard to review
OK - I'll make the changes and split it into multiple patches as you suggested then re-send.
Regards
Best Regards, J.
__________ Information from ESET NOD32 Antivirus, version of virus signature database 4125 (20090603) __________
The message was checked by ESET NOD32 Antivirus.