Dear Luca Ceresoli,
In message <1301416116-5519-5-git-send-email-luca.ceres...@comelit.it> you
wrote:
> Board support for the DIG297 board manufactured by Comelit Group SpA.
> It is a custom board based on the BeagleBoard <http://beagleboard.org/> by
> Texas Instruments.
...
> +/* GPMC CS 5 connected to an SMSC LAN9220 ethernet controller */
> +#define NET_LAN9220_GPMC_CONFIG1 0x00001000
> +#define NET_LAN9220_GPMC_CONFIG2 0x00080701
> +#define NET_LAN9220_GPMC_CONFIG3 0x00020201
> +#define NET_LAN9220_GPMC_CONFIG4 0x08010701
> +#define NET_LAN9220_GPMC_CONFIG5 0x00061D1D
> +#define NET_LAN9220_GPMC_CONFIG6 0x9D030000
> +#define NET_LAN9220_GPMC_CONFIG7 0x00000f6c
See below for general comments on the network stuff. For this block:
would it not make sense to replace the magic numbers by sumbolic
constants and/or add some documentation what these settings are
supposed to do?
> + /* board id for Linux */
> + gd->bd->bi_arch_number = MACH_TYPE_OMAP3_CPS;
Is this the correct machine ID? "OMAP3_CPS" does not really relate to
the board name, "DIG297" ?
> + /* GPIO list
> + * - 159 OUT (GPIO5+31): reset for remote camera interface connector.
> + * - 19 OUT (GPIO1+19): integrated speaker amplifier (1=on, 0=shdn).
> + * - 20 OUT (GPIO1+20): handset amplifier (1=on, 0=shdn).
> + */
Incorrect multiline comment style, please fix globally.
> +#ifdef CONFIG_CMD_NET
> +/*
> + * Routine: setup_net_chip
> + * Description: Setting up the configuration GPMC registers specific to the
> + * Ethernet hardware.
> + */
> +static void setup_net_chip(void)
> +{
> +#ifdef CONFIG_SMC911X
> + struct ctrl *ctrl_base = (struct ctrl *)OMAP34XX_CTRL_BASE;
> +
> + /* Configure GPMC registers */
> + writel(NET_LAN9220_GPMC_CONFIG1, &gpmc_cfg->cs[5].config1);
> + writel(NET_LAN9220_GPMC_CONFIG2, &gpmc_cfg->cs[5].config2);
> + writel(NET_LAN9220_GPMC_CONFIG3, &gpmc_cfg->cs[5].config3);
> + writel(NET_LAN9220_GPMC_CONFIG4, &gpmc_cfg->cs[5].config4);
> + writel(NET_LAN9220_GPMC_CONFIG5, &gpmc_cfg->cs[5].config5);
> + writel(NET_LAN9220_GPMC_CONFIG6, &gpmc_cfg->cs[5].config6);
> + writel(NET_LAN9220_GPMC_CONFIG7, &gpmc_cfg->cs[5].config7);
This is pretty much unreadable.
> + /* Enable off mode for NWE in PADCONF_GPMC_NWE register */
> + writew(readw(&ctrl_base->gpmc_nwe) | 0x0E00, &ctrl_base->gpmc_nwe);
> + /* Enable off mode for NOE in PADCONF_GPMC_NADV_ALE register */
> + writew(readw(&ctrl_base->gpmc_noe) | 0x0E00, &ctrl_base->gpmc_noe);
> + /* Enable off mode for ALE in PADCONF_GPMC_NADV_ALE register */
> + writew(readw(&ctrl_base->gpmc_nadv_ale) | 0x0E00,
> + &ctrl_base->gpmc_nadv_ale);
> +
> + /* Make GPIO 12 as output pin and send a magic pulse through it */
> + if (!omap_request_gpio(NET_LAN9221_RESET_GPIO)) {
> + omap_set_gpio_direction(NET_LAN9221_RESET_GPIO, 0);
> + omap_set_gpio_dataout(NET_LAN9221_RESET_GPIO, 1);
> + udelay(1);
> + omap_set_gpio_dataout(NET_LAN9221_RESET_GPIO, 0);
> + udelay(31000); /* Should be >= 30ms according to datasheet */
> + omap_set_gpio_dataout(NET_LAN9221_RESET_GPIO, 1);
> + }
> +#endif /* CONFIG_SMC911X */
> +}
> +#endif /* CONFIG_CMD_NET */
This is board specific code - is there any chance you will add another
network controller? Or that you will undefine CONFIG_SMC911X and
still keep CONFIG_CMD_NET defined?
Please check these #ifdef's!
> +int board_eth_init(bd_t *bis)
> +{
> + int rc = 0;
> +#ifdef CONFIG_SMC911X
> + rc = smc911x_initialize(0, CONFIG_SMC911X_BASE);
> +#endif
> + return rc;
> +}
Also here.
...
> +/* MCSPI1: to TOUCH controller TSC2046 (ADS7846 compatible).*/\
> + MUX_VAL(CP(MCSPI1_CLK), (IEN | PTD | EN | M0)) /*McSPI1_CLK.
> + IEN needed fot the McSPI to "receive" the clock and be able to sample
> SOMI.
> + See http://e2e.ti.com/support/arm174_microprocessors/
> + omap_applications_processors/f/42/p/29444/102394.aspx#102394 */\
Incorrect multiline comment style. Please fix globally.
> + MUX_VAL(CP(MCSPI1_SIMO), (IDIS | PTD | EN | M0)) /*McSPI1_SIMO*/\
> + MUX_VAL(CP(MCSPI1_SOMI), (IEN | PTD | EN | M0)) /*McSPI1_SOMI*/\
> + MUX_VAL(CP(MCSPI1_CS0), (IDIS | PTU | EN | M0)) /*McSPI1_CS0*/\
> +/* MCSPI2: verso TFT controller HIMAX.*/\
...
> +#define CONFIG_SYS_PROMPT "CPS# "
This also does not appear to match your board name ?
> +#define CONFIG_SYS_FLASH_BASE boot_flash_base
...
> +#define CONFIG_SYS_ENV_SECT_SIZE boot_flash_sec
> +#define CONFIG_ENV_OFFSET boot_flash_off
...
> +#ifndef __ASSEMBLY__
> +extern unsigned int boot_flash_base;
> +extern unsigned int boot_flash_off;
> +extern unsigned int boot_flash_sec;
> +extern unsigned int boot_flash_type;
> +#endif
Can we please get rid of this?
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
Hi there! This is just a note from me, to you, to tell you, the per-
son reading this note, that I can't think up any more famous quotes,
jokes, nor bizarre stories, so you may as well go home.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
