[U-Boot] [PATCH-ARM] Add support for Embest SBC2440-II Board
kevin.morfitt at fearnside-systems.co.uk
kevin.morfitt at fearnside-systems.co.uk
Wed Jun 3 10:49:39 CEST 2009
Hi Jean-Christophe
Comments below...
On 03/06/2009 00:35, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 03:24 Sat 23 May , kevin.morfitt at 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 at 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 at 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 at sysgo.de>
>> + *
>> + * (C) Copyright 2002
>> + * David Mueller, ELSOFT AG, <d.mueller at elsoft.ch>
>> + *
>> + * (C) Copyright 2005
>> + * JinHua Luo, GuangDong Linux Center, <luo.jinhua at gd-linux.com>
>> + *
>> + * Modified for the Embest SBC2440-II by
>> + * (C) Copyright 2009
>> + * Kevin Morfitt, Fearnside Systems Ltd, <kevin.morfitt at 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 = 50MHz
>> + HCLK = 100MHz
>> + FCLK = 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.
>
> http://www.eset.com
>
>
>
>
>
--
Kevin Morfitt
Fearnside Systems Ltd
25 Holbeck Road
Nottingham
NG8 3PB
t: 0115 9136703
m: 07939 126277
e: kevin.morfitt at fearnside-systems.co.uk
__________ Information from ESET NOD32 Antivirus, version of virus signature database 4126 (20090603) __________
The message was checked by ESET NOD32 Antivirus.
http://www.eset.com
More information about the U-Boot
mailing list