On 08/10/2011 10:33 PM, Eric Jarrige wrote:
Add Armadeus Project board APF9328
Signed-off-by: Eric Jarrige eric.jarrige@armadeus.org Signed-off-by: Nicolas Colombain nicolas.colombain@armadeus.com
Hi Eric,
diff --git a/board/armadeus/apf9328/apf9328.c b/board/armadeus/apf9328/apf9328.c new file mode 100644 index 0000000..2250221 --- /dev/null +++ b/board/armadeus/apf9328/apf9328.c @@ -0,0 +1,91 @@ +/*
- (C) Copyright 2005-2011
- Nicolas Colombin thom25@users.sourceforge.net
- Eric Jarrige eric.jarrige@armadeus.org
- Copyright (C) 2004 Sascha Hauer, Synertronixx GmbH
- 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 <asm/arch/imx-regs.h> +#include <flash.h> +#include <netdev.h> +#include "apf9328fpga.h"
+DECLARE_GLOBAL_DATA_PTR;
+int board_init(void) +{
- gd->bd->bi_arch_number = CONFIG_MACH_TYPE;
Is there no MACH_TYPE for this board ? It is uncommon for an ARM board. Should this board run Linux ?
+void dram_init_banksize(void) +{ +#if (CONFIG_NR_DRAM_BANKS > 0)
I think you can get rid of this #ifdef, if you have no RAM at all you cannot simply run u-boot.
- Miscellaneous intialization
- */
+int misc_init_r(void) +{
- char *s;
+#if (CONFIG_FPGA)
- apf9328_init_fpga();
+#endif
+#if (CONFIG_DRIVER_DM9000)
- imx_gpio_mode(GPIO_PORTB | GPIO_DR | GPIO_IN | 14);
Is there a reason to put this code here and not in board_eth_init ? It is related to Ethernet and I am supposing this setup should be done before dm9000_initialize.
+void show_boot_progress(int status) +{
- return;
+}
This function seems to me not very useful. Is it not better to drop it ? It is not strictly required. You set also #undef CONFIG_SHOW_BOOT_PROGRESS in the configuration file.
+#if (CONFIG_FPGA) +DECLARE_GLOBAL_DATA_PTR; +/* Note that these are pointers to code that is in Flash. They will be
- relocated at runtime.
- Spartan3 code is used to download our Spartan 3 :) code is compatible.
- Just take care about the file size
+*/ +Xilinx_Spartan3_Slave_Serial_fns fpga_fns = {
- fpga_pre_fn,
- fpga_pgm_fn,
- fpga_clk_fn,
- fpga_init_fn,
- fpga_done_fn,
- fpga_wr_fn,
+};
+Xilinx_desc fpga[CONFIG_FPGA_COUNT] = {
Do you have more as one FPGA on your board ? And if this is true, they share the same firmware ? (I see only one CONFIG_FIRMWARE_ADDR..)
+/*
- Initialize the fpga. Return 1 on success, 0 on failure.
- */
+int apf9328_init_fpga(void) +{
- char *autoload = getenv("firmware_autoload");
- int i, lout = 1;
- debug("%s:%d: Initialize FPGA interface (relocation offset= 0x%.8lx)\n",
__func__, __LINE__, gd->reloc_off);- fpga_init();
- for (i = 0; i < CONFIG_FPGA_COUNT; i++) {
debug("%s:%d: Adding fpga %d\n", __func__, __LINE__, i);fpga_add(fpga_xilinx, &fpga[i]);- }
- if ((autoload) && (0 == strcmp(autoload, "1"))) {
if (FPGA_SUCCESS != fpga_load(0, (void *)CONFIG_FIRMWARE_ADDR,
I am confused...you add in the configuration file a variable "firmware_addr=", and you set it as default to CONFIG_FIRMWARE_ADDR, but you do not use this variable at all. Do you not shoul get the address with getenv("firmware_addr") instead of the precompiled value ?
If you add some new CONFIG_ switches in U-Boot, you must document them in the Readme file. However, CONFIG_FIRMWARE_* do not need to be global, right ?
+/*
- Spartan 3 FPGA configuration support for the APF9328 daughter board
- */
+#include "fpga.h" +extern int apf9328_init_fpga(void); diff --git a/board/armadeus/apf9328/eeprom.c b/board/armadeus/apf9328/eeprom.c
It seems to much fo me to add a new file only for a single prototype. and it is used only in apf9328fpga.c, as I can see.
+#include <common.h> +#include <command.h> +#include <dm9000.h>
+static int do_read_dm9000_eeprom(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])+{
- unsigned int i;
- u8 data[2];
- for (i = 0; i < 0x40; i++) {
if (!(i % 0x10))printf("\n%08x:", i);dm9000_read_srom_word(i, data);printf(" %02x%02x", data[1], data[0]);- }
These functions can be factorized. I think the best place is inside the DM9000 driver itself, removing them from board code.
+#include <common.h>
+#if (CONFIG_FPGA)
#ifdef. We do not want to explicitely set the value for the CONFIG_ switches. It must be enough to use
#define CONFIG_FPGA
in your configuration file,
+/*
- nitialize GPIO port B before download
Initialize
+extern int fpga_pre_fn(int cookie); +extern int fpga_pgm_fn(int assert_pgm, int flush, int cookie); +extern int fpga_init_fn(int cookie); +extern int fpga_done_fn(int cookie); +extern int fpga_clk_fn(int assert_clk, int flush, int cookie); +extern int fpga_wr_fn(int assert_write, int flush, int cookie);
Maybe can you add here the prototy you set in apf9328.h ?
diff --git a/board/armadeus/apf9328/i2c.c b/board/armadeus/apf9328/i2c.c
This is a I2C driver, and must be stored into drivers/i2c, not in board directory. Please split your patch and push a separate patch only for the I2C code, sending it to the I2C maintainer, too.
+#include <common.h>
+#ifdef CONFIG_HARD_I2C
+#include <asm/io.h> +#include <asm/arch/imx-regs.h> +#include <i2c.h>
+/*-----------------------------------------------------------------------
- Definitions
- */
+#define I2C_ACK 0 /* level to ack a byte */ +#define I2C_NOACK 1 /* level to noack a byte */
+/*-----------------------------------------------------------------------
- Local functions
- */
+/*-----------------------------------------------------------------------
- START: High -> Low on SDA while SCL is High
- after check for a bus free
- */
+static void imxi2c_send_start(void) +{
- while (I2SR & I2SR_IBB)
udelay(1);
You are using the __REG macros, that are not allowed anymore in new u-boot code. Instead of that, please add C structure and use general accessors (writel, readl,...) to access to the registers. This comment apply to the whole code here.
diff --git a/board/armadeus/apf9328/lowlevel_init.S b/board/armadeus/apf9328/lowlevel_init.S new file mode 100644 index 0000000..e4c6157 --- /dev/null +++ b/board/armadeus/apf9328/lowlevel_init.S @@ -0,0 +1,469 @@ +/*
- (C) Copyright 2005-2011 ej Armadeus Project eric.jarrige@armadeus.org
- Copyright (C) 2004 Sascha Hauer, Synertronixx GmbH
- 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 <config.h> +#include <version.h> +#include <asm/arch/imx-regs.h>
+.globl lowlevel_init +lowlevel_init: +/* Change PERCLK1DIV to 14 ie 14+1 */
- ldr r0, =PCDR
- ldr r1, =CONFIG_SYS_PCDR_VAL
- str r1, [r0]
+/* set MCU PLL Control Register 0 */
- ldr r0, =MPCTL0
- ldr r1, =CONFIG_SYS_MPCTL0_VAL
- str r1, [r0]
+/* set MCU PLL Control Register 1 */
- ldr r0, =MPCTL1
- ldr r1, =CONFIG_SYS_MPCTL1_VAL
- str r1, [r0]
+/* set mpll restart bit */
- ldr r0, =CSCR
- ldr r1, [r0]
- orr r1,r1,#(1<<21)
- str r1, [r0]
- mov r2,#0x10
+1:
- mov r3,#0x2000
+2:
- subs r3,r3,#1
- bne 2b
- subs r2,r2,#1
- bne 1b
+/* set System PLL Control Register 0 */
- ldr r0, =SPCTL0
- ldr r1, =CONFIG_SYS_SPCTL0_VAL
- str r1, [r0]
+/* set System PLL Control Register 1 */
- ldr r0, =SPCTL1
- ldr r1, =CONFIG_SYS_SPCTL1_VAL
- str r1, [r0]
+/* set spll restart bit */
- ldr r0, =CSCR
- ldr r1, [r0]
- orr r1,r1,#(1<<22)
- str r1, [r0]
- mov r2,#0x10
+1:
- mov r3,#0x2000
+2:
- subs r3,r3,#1
- bne 2b
- subs r2,r2,#1
- bne 1b
- ldr r0, =CSCR
- ldr r1, =CONFIG_SYS_CSCR_VAL
- str r1, [r0]
- ldr r0, =GPCR
- ldr r1, =CONFIG_SYS_GPCR_VAL
- str r1, [r0]
+/* I have now read the ARM920 DataSheet back-to-Back, and have stumbled upon
- *this.....
- It would appear that from a Cold-Boot the ARM920T enters "FastBus" mode CP15
- register 1, this stops it using the output of the PLL and thus runs at the
- slow rate. Unless you place the Core into "Asynch" mode, the CPU will never
- use the value set in the CM_OSC registers...regardless of what you set it
- too! Thus, although i thought i was running at 140MHz, i'm actually running
- at 40!..
- Slapping this into my bootloader does the trick...
- MRC p15,0,r0,c1,c0,0 ; read core configuration register
- ORR r0,r0,#0xC0000000 ; set asynchronous clocks and not fastbus mode
- MCR p15,0,r0,c1,c0,0 ; write modified value to core configuration
- register
- */
- MRC p15,0,r0,c1,c0,0
- ORR r0,r0,#0xC0000000
- MCR p15,0,r0,c1,c0,0
+/* ldr r0, =GPR(0)
Do not add dead code, simply drop it.
However, this code is quite the same I can find on other IMX boards. Can we factorize it and push it as common code ?
Best regards, Stefano Babic