Dear Jean-Christophe PLAGNIOL-VILLARD,
Thank you for your detailed review
2009/6/24 Jean-Christophe PLAGNIOL-VILLARD <plagn...@jcrosoft.com>:
> please use git
I cannot access git in the company, but I will try that at home.
> please add a MAKEALL and MAINTAINER entry
ok, I will fix them
>> RCS file: /usr/local/cvsroot/ctd/u-boot-2009.06/Makefile,v
>> +a320_config : unconfig
>> + @$(MKCONFIG) $(@:_config=) arm arm920t a320 faraday
> 920T but you write 926ejs in the config.mk?
please see the explanation below
>> diff -N board/faraday/a320/board.c
>> +#define FLASH_BANK0_CONFIG (FTSMC_BANK_ENABLE | \
>> + FTSMC_BANK_BASE(PHYS_FLASH_1) | \
>> + FTSMC_BANK_SIZE_1M | \
>> + FTSMC_BANK_MBW_8)
>> +
>> +#define FLASH_BANK0_TIMING (FTSMC_TPR_RBE | \
>> + FTSMC_TPR_AST(3) | \
>> + FTSMC_TPR_CTW(3) | \
>> + FTSMC_TPR_ATI(0xf) | \
>> + FTSMC_TPR_AT2(3) | \
>> + FTSMC_TPR_WTC(3) | \
>> + FTSMC_TPR_AHT(3) | \
>> + FTSMC_TPR_TRNA(0xf))
>> +
>> +#define FLASH_BANK1_CONFIG (FTSMC_BANK_ENABLE | \
>> + FTSMC_BANK_BASE(PHYS_FLASH_2) | \
>> + FTSMC_BANK_SIZE_32M | \
>> + FTSMC_BANK_MBW_32 )
>> +
>> +#define FLASH_BANK1_TIMING (FTSMC_TPR_AST(3) | \
>> + FTSMC_TPR_CTW(3) | \
>> + FTSMC_TPR_ATI(0xf) | \
>> + FTSMC_TPR_AT2(3) | \
>> + FTSMC_TPR_WTC(3) | \
>> + FTSMC_TPR_AHT(3) | \
>> + FTSMC_TPR_TRNA(0xf))
> please move this to config header
ok, I will fix them
>> +
>> +extern int timer_init (void);
> no-need please remove
ok
>> + smc->bank[bank].cr = cpu_to_le32 (config);
>> + smc->bank[bank].tpr = cpu_to_le32 (timing);
> please use proper accessor everywhere
Should I use
writel(config, &smc->bank[bank].cr);
or
smc->bank[bank].cr = config;
>> +int board_init (void)
>> +{
>> + gd->bd->bi_arch_number = MACH_TYPE_FARADAY;
>> +
>> + ftsmc_init ();
>> + rtc_reset ();
> do you really need this so early the rtc and enable everytime?
sorry, I don't understand what do you mean.
>> + timer_init ();
> no-need pelase remove
ok
>> +int interrupt_init (void)
>> +{
>> + /* nothing happens here - we don't setup any IRQs */
>> + return (0);
>> +}
> no-need please remove
It looks like interrupt_init is an soc function, I will put it to the soc code.
>> diff -N board/faraday/a320/config.mk
>> +# Faraday A320 board with FA526/FA626TE/ARM926EJ-S cpus
> ??
>> Index: board/faraday/a320/ftahbc.h
>> Index: board/faraday/a320/ftpmu.h
>> Index: board/faraday/a320/ftsdmc.h
>> Index: board/faraday/a320/ftsmc.h
>> Index: board/faraday/a320/fttmr.h
> If I undersand correctly all those header are soc specific not board specific
> as the timer code
> so please move the header to
> include/asm-arm/arch-faraday or your soc familiy name
> and the code to
> cpu/armxxx/faraday/
We have an SOC called a320 (so does the evaluation board) which has an
armv4 cpu called fa526.
We can overdrive (with daughter board) it with a different cpu such as
arm926ej-s.
At first, I created cpu/fa526 and copy the cpu stuff from arm920t.
Then I found that it is not a good
idea to duplicate code, so I reused cpu/arm920t.
If I should create an SOC directory, where should I place it?
cpu/fa526/a320 ? but this duplicates cpu code
cpu/arm920t/a320 ? but it is not arm920t in fact
cpu/arm926ejs/a320? but it is not arm926ejs by default
>> diff -N board/faraday/a320/lowlevel_init.S
>> +/*
>> + * Memory Mapping
>> + */
>> +#define CONFIG_SYS_ROM_DEFAULT 0x00000000
>> +#define CONFIG_SYS_SDRAM_DEFAULT 0x10000000
>> +#define CONFIG_SYS_SDRAM_REMAPPED PHYS_SDRAM_1 /* remap location */
> I guess this board specific too
yes
>> +/*
>> + * numeric 7 segment display
>> + */
>> +.macro led, num, rtmp1, rtmp2
>> + mov \rtmp1, #\num
>> + ldr \rtmp2, =CONFIG_SYS_DEBUG_LED
>> + str \rtmp1, [\rtmp2]
>> +.endm
> please use write32
ok, I will fix this
>> + /*
>> + * copy U-Boot to RAM
>> + */
>> +copy_code:
>> + ldr r0, =CONFIG_SYS_ROM_DEFAULT /* r0 <- source address */
>> + ldr r1, =CONFIG_SYS_SDRAM_DEFAULT /* r1 <- target address */
>> +
>> + ldr r2, .LC5
>> + ldr r3, .LC6
>> + 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
> why???
> it's already done in the start.S
After power on, cpu starts from ROM (at 0x0) and this code copies u-boot to
SDRAM (at 0x10000000). Then the later "remap" routine modifies the setting
of the AHB controller. SDRAM base will become 0x0. Therefore I have to do
this copy to make "remap" work.
Other boot loader of our company behaves like this and our kernel and non-OS
programs depends on it (SDRAM base 0x0). Therefore, TEXT_BASE is
0x03f80000 rather than 0x13f80000.
>> +
>> + led 0x2, r0, r1
>> +
>> + bl remap
>> +1:
>> + led 0x3, r0, r1
>> +
>> + /* everything is fine now */
>> + mov lr, r11
>> + mov pc, lr
>> +
>> +.LC5:
>> + .word _start
>> +.LC6:
>> + .word __bss_start
>> +
>> +/*
>> + * memory initialization
>> + */
>> +init_sdmc:
>> + mov r9, lr
>> +
> please use write32 and co when it's possible
> include/asm-arm/macro.h
ok, I will fix them
>> + /* set SDRAM register */
>> + ldr r0, =CONFIG_SYS_SDMC_BASE
>> + ldr r1, tp0
>> + str r1, [r0, #FTSDMC_OFFSET_TP0]
>> + ldr r1, tp1
>> + str r1, [r0, #FTSDMC_OFFSET_TP1]
>> +/*
>> + * This code will remap the memory ROM and SDRAM
>> + * ROM will be placed on 0x80000000 SDRAM will jump to 0x0
>> + */
>> +remap:
>> + mov r9, lr
>> +
>> + ldr r0, =CONFIG_SYS_SDMC_BASE
>> +
>> + /* first adjust sdram */
>> + ldr r1, b0_bsr
>> + ldr r2, =FTSDMC_BANK_BASE(CONFIG_SYS_SDRAM_REMAPPED)
>> + orr r1, r1, r2
>> + str r1, [r0, #FTSDMC_OFFSET_BANK0_BSR] @ bank 0
>> +
>> + /* then remap */
>> + ldr r3, =CONFIG_SYS_AHBC_BASE
>> + ldr r4, [r3, #FTAHBC_OFFSET_ICR]
>> + orr r4, r4, #FTAHBC_ICR_REMAP @ Set REMAP bit
>> + str r4, [r3, #FTAHBC_OFFSET_ICR]
>> +
>> + mov lr, r9
>> + mov pc, lr
>> +
>> +/*
>> + * some parameters for the board
>> + */
>> +tp0: .word FTSDMC_TP0_TRAS(2) | \
>> + FTSDMC_TP0_TRP(1) | \
>> + FTSDMC_TP0_TRCD(1) | \
>> + FTSDMC_TP0_TRF(3) | \
>> + FTSDMC_TP0_TWR(1) | \
>> + FTSDMC_TP0_TCL(2)
>> +
>> +tp1: .word FTSDMC_TP1_INI_PREC(4) | \
>> + FTSDMC_TP1_INI_REFT(8) | \
>> + FTSDMC_TP1_REF_INTV(0x180)
>> +
>> +b0_bsr: .word FTSDMC_BANK_ENABLE | \
>> + FTSDMC_BANK_DDW_X16 | \
>> + FTSDMC_BANK_DSZ_256M | \
>> + FTSDMC_BANK_MBW_32 | \
>> + FTSDMC_BANK_SIZE_64M
> those config will need to the config head
ok, I will fix this
>> Index: board/faraday/a320/u-boot.lds
> no need please remove
>> Index: include/configs/a320.h
>> ===================================================================
>> RCS file: include/configs/a320.h
>> diff -N include/configs/a320.h
>> --- /dev/null 1 Jan 1970 00:00:00 -0000
>> +++ include/configs/a320.h 23 Jun 2009 06:25:54 -0000 1.3
>> @@ -0,0 +1,181 @@
>> +/*
>> + * (C) Copyright 2009 Faraday Technology
>> + * Po-Yu Chuang <ratb...@faraday-tech.com>
>> + *
>> + * Configuation settings for the Faraday A320 board.
>> + *
>> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
>> + */
>> +
>> +#ifndef __CONFIG_H
>> +#define __CONFIG_H
>> +
>> +/*-----------------------------------------------------------------------
>> + * CPU and Board Configuration Options
>> + */
>> +#define CONFIG_A320 /* in a Faraday A320 SoC/Board */
>> +
>> +#undef CONFIG_USE_IRQ /* we don't need IRQ/FIQ stuff */
>> +
>> +#undef CONFIG_SKIP_LOWLEVEL_INIT
>> +
>> +/*-----------------------------------------------------------------------
>> + * Timer
>> + */
>> +#define CONFIG_SYS_HZ 32768 /* timer ticks per second */
> 1000 mandatory
ok, I will fix it
>> +#define CONFIG_SYS_TIMERBASE 0x98400000
> is it board specific??
soc specific indeed
>> +
>> +/*-----------------------------------------------------------------------
>> + * RTC
>> + */
>> +#define CONFIG_RTC_FTRTC
>> +#define CONFIG_SYS_RTC_BASE 0x98600000
> ditto
ok
>> +#define CONFIG_SYS_MAC100_BASE 0x90900000
> ditto
ok
>> +
>> +#define CONFIG_BOOTDELAY 3
>> +#define CONFIG_ETHADDR c3:10:da:ce:3f:3e
>> +#define CONFIG_NETMASK 255.255.255.0
>> +#define CONFIG_IPADDR 192.168.68.55
>> +#define CONFIG_SERVERIP 192.168.68.63
> please remove the mac and ip config
ok, I will fix it
>> +
>> +/*-----------------------------------------------------------------------
>> + * Hardware register bases
>> + */
>> +#define CONFIG_SYS_AHBC_BASE 0x90100000 /* AHB Controller */
>> +#define CONFIG_SYS_SMC_BASE 0x90200000 /* Static Memory Controller */
>> +#define CONFIG_SYS_DEBUG_LED 0x902ffffc /* Debug LED */
>> +#define CONFIG_SYS_SDMC_BASE 0x90300000 /* SDRAM Controller */
> is it board specific??
You are right. soc specific
>
>> +
>> +#define CONFIG_SYS_FTPMU_BASE 0x98100000 /* Power Management
>> Unit */
> is it board specific??
ok
>> +/* no environments */
>> +#define CONFIG_ENV_IS_NOWHERE
> you do not plan to store the env in nor?
Yes, I do, but I will do this later after the current patches are accepted.
Best regards,
Po-Yu Chuang
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
