Il 06/04/2011 09:50, Wolfgang Denk ha scritto:
Dear Luca Ceresoli,
In message1301416116-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 BeagleBoardhttp://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