[U-Boot] [PATCH 3/9] apf9328: Add Armadeus Project board APF9328
Stefano Babic
sbabic at denx.de
Thu Aug 11 10:50:44 CEST 2011
On 08/10/2011 10:33 PM, Eric Jarrige wrote:
> Add Armadeus Project board APF9328
>
> Signed-off-by: Eric Jarrige <eric.jarrige at armadeus.org>
> Signed-off-by: Nicolas Colombain <nicolas.colombain at 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 at users.sourceforge.net>
> + * Eric Jarrige <eric.jarrige at 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 at 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
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================
More information about the U-Boot
mailing list