[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