[U-Boot] [PATCH 3/9] apf9328: Add Armadeus Project board APF9328

[Stefano Babic <sbabic@denx.de>]

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

-- 
=====================================================================
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
=====================================================================