[U-Boot] [PATCH v3 4/4] ARMV7: OMAP3: Add support for Comelit DIG297 board

[Wolfgang Denk]

Dear Luca Ceresoli,

In message <1301416116-5519-5-git-send-email-luca.ceresoli at 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: wd at 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.