[U-Boot] [PATCH 5/9] apf9328: add default board configuration file
Eric Jarrige
eric.jarrige at armadeus.org
Mon Aug 15 22:25:16 CEST 2011
Hi Stefano,
On 11 août 2011, at 11:21, Stefano Babic wrote:
> On 08/10/2011 10:33 PM, Eric Jarrige wrote:
>> Signed-off-by: Eric Jarrige <eric.jarrige at armadeus.org>
>> ---
>> include/configs/apf9328.h | 1034 +++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 1034 insertions(+), 0 deletions(-)
>> create mode 100644 include/configs/apf9328.h
>
> Hi Eric,
>
>> +
>> +#ifndef __CONFIG_H
>> +#define __CONFIG_H
>> +
>> +#define CONFIG_VERSION_VARIABLE
>> +#define CONFIG_ENV_VERSION "4.0"
>> +#define CONFIG_IDENT_STRING " apf9328 patch 4.0"
>
> You add CONFIG_ without documenting them. If they are specific for your
> board, you should use another name, or please add documentation to make
> them available for other boards, too.
Good point. I propose to use a prefix like "APF_" for macros specifics to this board and I will update the documentation to make the generic config available for the other boards.
>
>> +
>> +#define CONFIG_ARM920T 1 /* this is an ARM920T CPU */
>
> It is enough to #define a switch, without setting the value. This should
> be only:
>
> #define CONFIG_ARM920T /* this is an ARM920T CPU */
>
> This must be fixed globally.
>
>> +#define CONFIG_IMX 1 /* in a Motorola MC9328MXL Chip */
>
> Ditto, and so on..
>
>> +/*
>> + * Definition of u-boot build in commands. Check out CONFIG_CMD_DFL if
>> + * neccessary in include/cmd_confdefs.h file. (Un)comment for getting
>> + * functionality or size of u-boot code.
>> + */
>> +
>> +#include <config_cmd_default.h>
>> +
>> +#define CONFIG_CMD_ASKENV /* ask for env variable */
>> +#define CONFIG_CMD_BSP /* Board Specific functions */
>> +#define CONFIG_CMD_CACHE /* icache, dcache */
>> +#define CONFIG_CMD_DATE /* support for RTC, date/time.. */
>> +#define CONFIG_CMD_DHCP /* DHCP Support */
>> +#define CONFIG_CMD_DIAG /* Diagnostics */
>> +#define CONFIG_CMD_EEPROM /* EEPROM read/write support */
>> +#define CONFIG_CMD_FLASH /* flinfo, erase, protect */
>> +#define CONFIG_CMD_I2C /* I2C serial bus support */
>> +#define CONFIG_CMD_IMLS /* List all found images */
>
> IMLS is already part of default commands
>
>> +#define CONFIG_CMD_IMMAP /* IMMR dump support */
>
> Is this not specific for PowerQuick processor ?
Thx, I ll remove it.
>
>> +#define CONFIG_EXTRA_ENV_SETTINGS \
>> + "env_version=" CONFIG_ENV_VERSION "\0" \
>> + "fileaddr=" MK_STR(CONFIG_SYS_LOAD_ADDR) "\0" \
>> + "filesize=" MK_STR(CONFIG_SYS_MONITOR_LEN) "\0" \
>
> filesize is dynamically computed, you should not add it
We need this variable already initialized at boot time to support the specific imx boot mode from serial port when the user lost the full content of the flash.
In such case (that is a stressing situation for the user) he can push U-Boot through the serial port and use directly the script "flash_uboot" to recover the original content of the flash. In such a use case this variable is not dynamically computed.
>
>> + "ntpserverip=217.147.208.1\0" \
>
> No fixed IP address, please, even if they are related to known NTP servers.
>
>> +#define CONFIG_MACH_TYPE MACH_TYPE_APF9328
>
> Why do you need it ? Is it not enough to use directly MACH_TYPE_APF9328 ?
The idea is to have most of the configuration parameters in the config.h files.
So that it become possible to factorize the initialization of the mach_type in a common place.
For the time being it is not needed it and I will remove it.
>
>> +#define CONFIG_ETHADDR
>
> Not required, right ?
Right.
>
>> +#define CONFIG_NETMASK 255.255.255.0
>> +#define CONFIG_IPADDR 192.168.000.10
>
> Please drop fix ip address. They should not be part of mainline,
Here, I have a problem as our documentation on the wiki armadeus.org is based on this default IP addresses.
We really need a set of default IP addresses for private network to simplify as much as possible the life of the armadeus project developers.
I never seen such a restriction in U-Boot doc and moreover there is a set CONFIG_XXIP documented in U-BOOT that confusing me,
Is there another solution for a complete usable default configuration?
>
>> +#define CONFIG_BOARD_NAME "apf9328"
>> +#define CONFIG_HOSTNAME "apf9328"
>> +#define CONFIG_GATEWAYIP 192.168.000.1
>> +#define CONFIG_SERVERIP 192.168.000.2
>
> Ditto
>
>> +/*
>> + * Malloc pool need to host env + 128 Kb reserve for other allocations.
>> + */
>> +#define CONFIG_SYS_MALLOC_LEN (CONFIG_ENV_SIZE + (128<<10))
>
> I Think checkpatch will complain about missing spaces before and after <<
checkpatch did not complain. << and >> do not require extra spaces.
>
>> +#define CONFIG_STACKSIZE (120<<10) /* stack size */
>> +
>> +#ifdef CONFIG_USE_IRQ
>> +#define CONFIG_STACKSIZE_IRQ (4<<10) /* IRQ stack */
>> +#define CONFIG_STACKSIZE_FIQ (4<<10) /* FIQ stack */
>> +#endif
>
> Do you need IRQ ? It is undefined, you can drop this part
We do not use IRQ. I will drop this part. Thx.
>
>
>> +
>> +/*
>> +* Clocks configuration
>> +*/
>> +/*
>> + * PLL configuration
>> + *
>> + * f_{dpll}=2*f{ref}*(MFI+MFN/(MFD+1))/(PD+1)
>> + * f_ref=16,777216MHz
>> + * 32768 Hz xtal
>> + * 0x07B32DA5: 192.0000173
>> + * 0x002a141f: 191,9944MHz
>> + * 0x040b2007: 144MHz
>> + * 0x0FB32DA5: 96.00000864 MHz
>> + * 0x042a141f: 96MHz
>> + * 0x0811140d: 64MHz
>> + * 0x040e200e: 150MHz
>> + * 0x00321431: 200MHz
>> + *
>> + *16 MHz xtal
>> + * 0x08001800: 64MHz mit 16er Quarz
>> + * 0x04001800: 96MHz mit 16er Quarz
>> + * 0x04002400: 144MHz mit 16er Quarz
>> + *
>> + * 31 |x x x x|x x x x|x x x x|x x x x|x x x x|x x x x|x x x x|x x x x| 0
>> + * |XXX|--PD---|-------MFD---------|XXX|--MFI--|-----MFN-----------|
>> + */
>> +
>> +#define CONFIG_SYS_OSC32 32768 /* 32768 or 32000 Hz crystal */
>> +#undef CONFIG_SYS_OSC16 /* there is no external 16MHz external clock */
>> +
>> +/* MPU CLOCK source before PLL (should be named CONFIG_SYS_SYS_CLK_FREQ) */
>> +#define CONFIG_SYS_CLK_FREQ (512*CONFIG_SYS_OSC32)
>> +#define CONFIG_SYS_MPCTL0_VAL 0x07B32DA5 /* 192.000017 MHz */
>> +#define CONFIG_SYS_MPCTL1_VAL 0
>> +
>> +/* system clock source before PLL (should be named CONFIG_SYS_SYSPLL_CLK_FREQ)*/
>> +#ifndef CONFIG_SYS_OSC16
>> +#define CONFIG_SYSPLL_CLK_FREQ (512*CONFIG_SYS_OSC32)
>> +#if (CONFIG_SYS_OSC32 == 32000)
>> +#define CONFIG_SYS_SPCTL0_VAL 0x043F1437 /* 96 MHz */
>> +#define CONFIG_SYS_SPCTL1_VAL 0
>> +#define CONFIG_SYS_FREQ 96 /* MHz */
>> +#else /* CONFIG_SYS_OSC32 == 32768 */
>> +#define CONFIG_SYS_SPCTL0_VAL 0x0FB32DA5 /* 96.000009 MHz */
>> +#define CONFIG_SYS_SPCTL1_VAL 0
>> +#define CONFIG_SYS_FREQ 96 /* MHz */
>> +#endif /* CONFIG_SYS_OSC32 */
>> +#else /* CONFIG_SYS_OSC16 in use */
>> +#define CONFIG_SYSPLL_CLK_FREQ CONFIG_SYS_OSC16
>> +#define CONFIG_SYS_SPCTL0_VAL 0x04001401 /* 96 MHz */
>> +#define CONFIG_SYS_SPCTL1_VAL 0x0C000040
>> +#define CONFIG_SYS_FREQ 96 /* MHz */
>> +#endif /* CONFIG_SYS_OSC16 */
>> +
>> +/* external bus frequency (have to be a CONFIG_SYS_FREQ ratio) */
>> +#define CONFIG_SYS_BUS_FREQ 96 /* 96|48... MHz (BCLOCK and HCLOCK) */
>> +#define CONFIG_USB_FREQ 48 /* 48 MHz */
>> +#define CONFIG_PERIF1_FREQ 16 /* 16 MHz UART, Timer PWM */
>> +#define CONFIG_PERIF2_FREQ 48 /* 48 MHz LCD SD SPI */
>> +#define CONFIG_PERIF3_FREQ 16 /* 16 MHz SSI */
>> +
>> +/*
>> + * SDRAM definition parameter
>> + */
>> +/* we have and support only 1 bank of SDRAM */
>> +#define CONFIG_NR_DRAM_BANKS 1
>> +
>> +#define CONFIG_SYS_SDRAM_MBYTE_SYZE 16
>> +
>> +/*
>> + * CONFIG_SYS_SDRAM_NUM_COL 8, 9, 10 or 11 column address bits
>> + * CONFIG_SYS_SDRAM_NUM_ROW 11, 12 or 13 row address bits
>> + * CONFIG_SYS_SDRAM_REFRESH 0=OFF 1=2048 2=4096 3=8192 refresh
>> + * CONFIG_SYS_SDRAM_CLOCK_CYCLE_CL_1 X ns clock cycle time when CL=1
>> + * CONFIG_SYS_SDRAM_ROW_PRECHARGE_DELAY X ns SRP
>> + * CONFIG_SYS_SDRAM_ROW_2_COL_DELAY X ns SRCD
>> + * CONFIG_SYS_SDRAM_ROW_CYCLE_DELAY X ns SRC
>> + * CONFIG_SYS_SDRAM_BURST_LENGTH 2^N BYTES (N=0..3)
>> + * CONFIG_SYS_SDRAM_SINGLE_ACCESS 1= single access; 0 = Burst mode
>> + */
>> +
>> +#if (CONFIG_SYS_SDRAM_MBYTE_SYZE == 8)
>
> Why do we need such a stuff when your configuration is fixed at compile
> time with CONFIG_SYS_SDRAM_MBYTE_SYZE 16 ?
16MiB is the regular configuration but there are many configurations of boards with different size of memory.
This set of parameters enables to support every boards at compilation by just changing the value of CONFIG_SYS_SDRAM_MBYTE_SYZE.
So that the binary generated is fully optimized for each board.
>
>> +/* lowlevel_linit copy U-Boot at CONFIG_SYS_INIT_SP_ADDR (here in RAM) */
>> +/* making u-boot runnable from flash and also RAM that is usefull to boot */
>> +/* from serial port and from flash with only one version of U-Boot */
>> +#ifndef CONFIG_SYS_TEXT_BASE
>> +#define CONFIG_SYS_TEXT_BASE 0x08000000
>> +#endif
>> +
>> +#define CONFIG_SYS_INIT_SP_ADDR (CONFIG_SYS_SDRAM_BASE \
>> + + CONFIG_SYS_SDRAM_1_SIZE - 0x0100000)
>
> You do not use U_Boot macro here (GENERATED_GBL_DATA_SIZE), as it is
> usual. Stack pointer is already in RAM before relocation. Is it correct ?
Correct. I do have to optimize the location of the initial Stack pointer for my board so I put the init SP at any safe place in RAM.
>
>> +/*#define CONFIG_MMC 1
>> +*/
>
> Dead code
>
>> +/*
>> + * External interfaces module
>> + *
>> + * CSxU_VAL:
>> + * 63| x |x|x x|x x x x|x x| x | x |x x x x|48
>> + * |DTACK_SEL|0|BCD| BCS |PSZ|PME|SYNC| DOL |
>> + *
>> + * 47| x x | x x x x x x | x | x x x x | x x x x |32
>> + * | CNC | WSC | 0 | WWS | EDC |
>> + *
>> + * CSxL_VAL:
>> + * 31| x x x x | x x x x | x x x x | x x x x |24
>> + * | OEA | OEN | WEA | WEN |
>> + * 23|x x x x| x | x x x | x x x x |x x| x | x | 0
>> + * | CSA |EBC| DSZ | 0|SP|0|WP|0 0|PA |CSEN |
>> + */
>> +
>> +/* CS0 configuration for flash memory Micron MT28F128J3-150 */
>> +#define CONFIG_SYS_CS0_CHIP_SELECT_ENABLE 1 /* 1 : enable CS0 */
>> +#define CONFIG_SYS_CS0_DATA_PORT_SIZE 0x5 /* 5=16 bits on D[15:0] */
>> + /* 3=8bits on D[7:0] 6=32 bits..*/
>> +#define CONFIG_SYS_CS0_SUPERVISOR_PROTECT 0 /* 1: user access prohibited*/
>> +#define CONFIG_SYS_CS0_WRITE_PROTECT 0 /* 1: write prohibited*/
>> +#define CONFIG_SYS_CS0_EB_SIGNAL_CONTROL_WRITE 1 /* 1 EB is as write signal */
>> +#define CONFIG_SYS_CS0_READ_CYC_LGTH 150 /* ns */
>> +#define CONFIG_SYS_CS0_OE_ASSERT_DLY 0 /* ns */
>> +#define CONFIG_SYS_CS0_OE_NEG_DLY 0 /* ns */
>> +
>> +#define CONFIG_SYS_CS0_CS_NEG_LGTH 0 /* ns CS HIGH to CS LOW : tCWH*/
>> +#define CONFIG_SYS_CS0_XTRA_DEAD_CYC 35 /* ns from CS HIGH to tristate*/
>> +
>> +#define CONFIG_SYS_CS0_WRITE_XTRA_LGTH 0 /* ns */
>> +#define CONFIG_SYS_CS0_EB_ASSERT_DLY 0 /* ns */
>> +#define CONFIG_SYS_CS0_EB_NEG_DLY 0 /* ns */
>> +#define CONFIG_SYS_CS0_CS_ASSERT_NEG_DLY 0 /* ns */
>> +
>
> All this stuff seems to me not general, that is they should not have a
> CONFIG_SYS_* name. Else they must be documented.
>
> Really I think that all this defines should be put in the code that use
> them, and not in the configuration file (if they are not spread with
> other boards).
I will rework all the CONFIG_SYS specific to this board.
>
>> +#define CONFIG_SYS_CSCR_VAL\
>> + (CSCR_MASK \
>> + |((((CONFIG_SYS_FREQ/CONFIG_USB_FREQ)-1)&0x07)<<26) \
>> + |((((CONFIG_SYS_FREQ/CONFIG_SYS_BUS_FREQ)-1)&0x0F)<<10))
>> +
>
> This macro (and the following ones) are related to the IMX processor and
> are not specific to the board. They should be available for all IMX
> boards, and the right place is arch/arm/include/asm/arch-imx/.
>
This macro and some others are specific to the iMX1/L cpu that is not any more recommended for new design by Freescale.
For a technical point of view I am fully is agreement with you but IMHO no maintainer will take the risk to rework deeply its implementation of U-boot for a CPU at the end of life. I apologize if I am wrong and will support the other mx1 board maintainers to improve the iMX1 boards.
For the next boards based on the iMX2 and 5 processors, we will put stuffs at the right place to make it available for the other boards.
Thanks a lot for the your time and all your feedbacks.
Best regards,
Eric
More information about the U-Boot
mailing list