Il 06/04/2011 09:50, Wolfgang Denk ha scritto: > 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? 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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
