[U-Boot] [PATCH 3/9] apf9328: Add Armadeus Project board APF9328
Eric Jarrige
eric.jarrige at armadeus.org
Fri Aug 12 01:41:22 CEST 2011
Hi Stefano,
> 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 ?
The MACH_TYPE for this board is 906. This board runs linux and the integration in linux is on going.
>
>> +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.
Good point. I will fix it.
>
>> + * 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.
The idea is to keep the configuration of the CPU pins at one place and to keep the initialization of the ethernet controller in a separate function.
Here this principle of segregation is not obvious as there is only one GPIO pin to configure.
>
>> +
>> +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.
Well I choosed to keep it in place in empty state to be the template for the final user to add their own debug method if needed.
If it is an issue I would prefer to define the CONFIG_SHOW_BOOT_PROGRESS in the configuration file than removing this empty show_boot_progress as the main purpose of the board is education and trainings on embedded developments.
>
>
>> +#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..)
The APF9328 is a SOM therefore it possible to connect some other FPGA to the board. In such a situation the idea is to concatenate all the bitstream files in the firmware partition.
During the download process to the FPGAs the end of one FPGA download starts the download of the next one using the FPGA signal PRG_DONE.
So that it possible to have all firmware in only one partition and save memory space by using bitstream files.
To be honest I do not remember if someone use this feature on this board but that is .
>
>> +/*
>> + * 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 ?
I use this variable in the script "flash_firmware" but it is a good point as I should
use getenv() instead of the CONFIG_FIRMWARE_ADDR.
I will do it. Thx
>
> 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 ?
For the time being this switch applies to this board only. So I did not plan to document it in the README.
It is not clear for me if I should add a documentation in the README since this switch is only used in the APF9328 source files.
>
>> +/*
>> + * 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.
this file defines the public prototype of the function implemented in apf9328fpga.c and used in apf9328.c
I agree that it would not make sense to add a header file with prototypes never used anywhere else in the code.
>
>> +
>> +#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.
Good point. I will propose your recommendation as I am already pushing a patch for the DM9000
>
>
>> +#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,
I am fully in agrement with that. Thx
>
>> +/*
>> + * nitialize GPIO port B before download
>
> Initialize
Sure.
>
>> +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 ?
Well, What do you think to add the prototype directly in apf9328.c: extern int apf9328_init_fpga(void);
This will avoid to expose the fpga_xx_fn to apf9328.c that does not care how the firmware is moved from CPU to FPGA
and therefore remove the very small apf9328_fpga.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.
ok
>
>> +#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.
Sure.
>
> However, this code is quite the same I can find on other IMX boards. Can
> we factorize it and push it as common code ?
The other boards use hardcoded values that are not compatible with the apf9328 board.
We do not use the same components and so cannot reuse or merge their code.
A factorization would require deep changes in the config files of every board.
For this old CPU there is no chance to convince the maintainers of the other boards to rework their implementation and specifically the boot code in assembly.
On the other hand I am working on some other ARM SOC more recent, so It may worth the pain to invest some effrot of factorization on modern CPUs.
What is your opinion?
Thanks and best regards,
Eric
More information about the U-Boot
mailing list