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

[Luca Ceresoli <luca.ceresoli@comelit.it>]

Il 06/04/2011 09:50, Wolfgang Denk ha scritto:

> Dear Luca Ceresoli,
>
> In message<1301416116-5519-5-git-send-email-luca.ceresoli@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?
I'm going to define the bit values for the GPMC_CONFIGn registers.
Is arch/arm/include/asm/arch-omap3/omap_gpmc.h the correct place?


>> +	/* 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" ?
As per our previous discussion (see the bottom of
http://lists.denx.de/pipermail/u-boot/2011-March/088964.html), I renamed the
board everywhere except for the Machine ID in the ARM registry, and
consequently mach-types.h.
As you suggested, I plan to ask for a rename in the registry just after this
patch series is integrated in U-Boot, to avoid confusion. The rename in
mach-types.h and in my code would follow.


>> +	/* 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.
Do you mean like this?

-       /* GPIO list
+       /*
+        * GPIO list
          * - 159 OUT (GPIO5+31): reset for remote camera interface connector.
          * - 19  OUT (GPIO1+19): integrated speaker amplifier (1=on, 0=shdn).


>> +#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.
Fixed using enable_gpmc_cs_config().

>> +	/* 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!

Removed all #ifdef CONFIG_SMC911X. The board is never expected to exist
without Ethernet.


>> +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.

Ditto.

> ...
>> +/* 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.
>> +    Seehttp://e2e.ti.com/support/arm174_microprocessors/
>> +      omap_applications_processors/f/42/p/29444/102394.aspx#102394 */\
> Incorrect multiline comment style.  Please fix globally.

Fixed.

>> +	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.*/\

Ouch.

>> +#define CONFIG_SYS_PROMPT		"CPS# "
> This also does not appear to match your board name ?

Historical (ee the aforementioned discussion).
I'll rename this one right now, as it doesn't have to wait for the
rename in the registry of course.

>> +#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?

Hopefully. This mostly depends on how able I will be in understanding
the meaning of this stuff and how it should be done instead. I might
need support.

> Best regards,
>
> Wolfgang Denk
>