Dear Jens Scharsig,
In message <4ae6b186.9030...@bus-elektronik.de> you wrote:
> This patch adds a new ARM AT91RM9200 board, named EB+CPUx9K2.
>
> * support for EB+CPUx9K2 board by BuS Elektronik GmbH & Co. KG
> * select via make EB_CPUx9K2_config
>
> Signed-off-by: Jens Scharsig <e...@bus-elektronik.de>
> ---
> This patch needs the [PATCH] AT91RM9200 BGA port D defines
> (http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/50667)
Please use shorter lines.
...
> diff --git a/board/BuS/EB+CPUx9K2/cpux9k2.c b/board/BuS/EB+CPUx9K2/cpux9k2.c
> new file mode 100644
> index 0000000..6c29d91
> --- /dev/null
> +++ b/board/BuS/EB+CPUx9K2/cpux9k2.c
...
> +int board_init(void)
> +{
> + /* Enable Ctrlc */
> + console_init_f();
> +
> + /* Correct IRDA resistor problem / Set PA23_TXD in Output */
> + ((AT91PS_PIO) AT91C_BASE_PIOA)->PIO_OER = AT91C_PA23_TXD2;
Please use I/O accessors to access device registers. Please fix
globally.
> +int misc_init_r(void)
> +{
...
> + if (tm) {
> + sprintf(str, "%02x:%02x:%02x:%02x:%02x:%02x",
> + mac[0], mac[1], mac[2],
> + mac[3], mac[4], mac[5]);
> + setenv("ethaddr", str);
Please don't reinvent the wheel. Use eth_setenv_enetaddr().
> +/*
> + * DRAM initialisations
> + */
> +
> +int dram_init(void)
> +{
> + gd->bd->bi_dram[0].start = PHYS_SDRAM;
> + gd->bd->bi_dram[0].size = PHYS_SDRAM_SIZE;
> + return 0;
Please consider using get_ram_size() for auto-sizing the memor and
perfoming at least a basic RAM test.
> +unsigned int lxt972_IsPhyConnected(AT91PS_EMAC p_mac);
> +UCHAR lxt972_GetLinkSpeed(AT91PS_EMAC p_mac);
> +UCHAR lxt972_InitPhy(AT91PS_EMAC p_mac);
> +UCHAR lxt972_AutoNegotiate(AT91PS_EMAC p_mac, int *status);
Please don't use non-stanrard data types like UCHAR.
> +void cpux9k2_nand_hw_init(void)
> +{
> + /* Setup Smart Media, fitst enable the address range of CS3 */
> + *AT91C_EBI_CSA |= AT91C_EBI_CS3A_SMC_SmartMedia;
> + /* set the bus interface characteristics based on
> + tDS Data Set up Time 30 - ns
> + tDH Data Hold Time 20 - ns
> + tALS ALE Set up Time 20 - ns
> + 16ns at 60 MHz ~= 3 */
> + /*memory mapping structures */
> +#define SM_ID_RWH (5 << 28)
> +#define SM_RWH (1 << 28)
> +#define SM_RWS (0 << 24)
> +#define SM_TDF (1 << 8)
> +#define SM_NWS (3)
Please don't add #defines right in the middle of the code. Move to
header file, or at least to head of file.
And see comment above - use I/O accessors here and everywhere.
> +#if defined(CONFIG_VIDEO)
> +/*
> + ****h* EB+CPUx9K2/drv_video_init
> + * FUNCTION
> + ***
> + */
Incorrect multiline comment style.
> +int drv_video_init(void)
> +{
> + char *s;
> + unsigned long splash;
> +
> + printf("Init Video as ");
> + s = getenv("displaywidth");
> + if (s != NULL)
> + display_width = simple_strtoul(s, NULL, 10);
> + else
> + display_width = 256;
> + s = getenv("displayheight");
> + if (s != NULL)
> + display_height = simple_strtoul(s, NULL, 10);
> + else
> + display_height = 256;
> + printf("%ld x %ld pixel matrix\n", display_width, display_height);
> +
> + #define SM2_RWH (0x7 << 28)
> + #define SM2_RWS (0x7 << 24)
> + #define SM2_TDF (15 << 8)
> + #define SM2_NWS (0x7F)
Please don't add #defines right in the middle of the code. Move to
header file, or at least to head of file.
> diff --git a/board/BuS/EB+CPUx9K2/u-boot.lds b/board/BuS/EB+CPUx9K2/u-boot.lds
> new file mode 100644
> index 0000000..f4fbf96
> --- /dev/null
> +++ b/board/BuS/EB+CPUx9K2/u-boot.lds
Do you really need a private linker script? Doesn't look so...
> diff --git a/include/configs/EB_CPUx9K2.h b/include/configs/EB_CPUx9K2.h
> new file mode 100644
> index 0000000..9ff743d
> --- /dev/null
> +++ b/include/configs/EB_CPUx9K2.h
...
> +
> +/*--------------------------------------------------------------------------*/
> +
> +#undef DEBUG
> +
> +/*--------------------------------------------------------------------------*/
Get rid of all this stuff. Don't undef what is not defined in the
beginning, and don't meddle with command line options.
> +#include <config_cmd_default.h>
> +
> +#define CONFIG_CMD_DHCP
> +#define CONFIG_CMD_PING
> +#define CONFIG_CMD_NAND
> +#define CONFIG_CMD_BMP
> +#define CONFIG_CMD_I2C
> +#define CONFIG_I2C_CMD_TREE
> +#define CONFIG_I2C_CMD_NO_FLAT
> +#define CONFIG_CMD_DATE
> +#define CONFIG_CMD_JFFS2
You may want to keep such lists sorted. And please use a consistent
style - either always follow #define with a TAB, or (probably better)
always with a space.
> +#define CONFIG_BAUDRATE 115200
> +#define CONFIG_AT91RM9200_USART
> +#define CONFIG_DBGU /* define DBGU as console */
> +#undef CONFIG_USART0
> +#undef CONFIG_USART1
> +
> +#undef CONFIG_HWFLOW
> +#undef CONFIG_MODEM_SUPPORT
Don;t undefine what is not defined.
> +/*
> + * network
> + */
> +
> +#if 0
> +#define CONFIG_NET_MULTI 1
> +#define CONFIG_DRIVER_AT91EMAC 1
> +#define CONFIG_DRIVER_AT91EMAC_QUIET 1
> +#define CONFIG_SYS_RX_ETH_BUFFER 8
> +#undef CONFIG_RMII
> +#define CONFIG_MII 1
> +#define CONFIG_CMD_MII 1
Don't add dead code.
> +#define CONFIG_SYS_FLASH_ERASE_TOUT (6*CONFIG_SYS_HZ)
> +#define CONFIG_SYS_FLASH_WRITE_TOUT (2*CONFIG_SYS_HZ)
This looks wrong to me. A timeout is a time, but CONFIG_SYS_HZ is a
frequency, i. e. the inverse of a time. These don't mix.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
This all sounds complicated, but it mostly does excatly what you ex-
pect. It's just difficult for us to explain what you expect...
- L. Wall & R. L. Schwartz, _Programming Perl_
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
