Hello Igor, Igor Grinberg wrote: > Hi Heiko, > > On 11/28/11 12:44, Heiko Schocher wrote: >> - booting from NOR Flash with direct boot method >> - POST support >> - LOGBUF support >> >> Signed-off-by: Heiko Schocher <h...@denx.de> >> Cc: Paulraj Sandeep <s-paul...@ti.com> >> Cc: Albert ARIBAUD <albert.u.b...@aribaud.net> >> Cc: Igor Grinberg <grinb...@compulab.co.il> >> Cc: Christian Riesch <christian.rie...@omicron.at> >> *** [...] >> diff --git a/board/enbw/enbw_cmc/Makefile b/board/enbw/enbw_cmc/Makefile >> new file mode 100644 >> index 0000000..bdba069 >> --- /dev/null >> +++ b/board/enbw/enbw_cmc/Makefile >> @@ -0,0 +1,51 @@ [...] >> +clean: >> + rm -f $(SOBJS) $(OBJS) >> + >> +distclean: clean >> + rm -f $(LIB) core *.bak *~ .depend > > clean and distclean on that directory level should be gone.
fixed, thanks! >> + >> +######################################################################### >> +# This is for $(obj).depend target >> +include $(SRCTREE)/rules.mk >> + >> +sinclude $(obj).depend >> + >> +######################################################################### >> diff --git a/board/enbw/enbw_cmc/enbw_cmc.c b/board/enbw/enbw_cmc/enbw_cmc.c >> new file mode 100644 >> index 0000000..6333b3a >> --- /dev/null >> +++ b/board/enbw/enbw_cmc/enbw_cmc.c >> @@ -0,0 +1,652 @@ >> +/* >> + * (C) Copyright 2011 >> + * Heiko Schocher, DENX Software Engineering, h...@denx.de. [...] >> + >> +#include <common.h> >> +#include <command.h> >> +#include <environment.h> >> +#include <hwconfig.h> >> +#include <i2c.h> >> +#include <malloc.h> >> +#include <miiphy.h> >> +#include <mmc.h> >> +#include <net.h> >> +#include <netdev.h> >> +#include <asm/arch/hardware.h> >> +#include <asm/arch/emif_defs.h> >> +#include <asm/arch/emac_defs.h> >> +#include <asm/gpio.h> >> +#include <asm/arch/gpio.h> >> +#include <asm/arch/davinci_misc.h> >> +#include <asm/arch/sdmmc_defs.h> >> +#include <asm/arch/timer_defs.h> >> +#include <asm/arch/pinmux_defs.h> >> +#include <asm/io.h> >> +#include <asm/arch/da850_lowlevel.h> > > Can the above includes be made like: > all #include <*.h> together > all #include <asm/*.h> together > all #include <asm/arch/*.h> together > ? Done. > >> + >> +DECLARE_GLOBAL_DATA_PTR; >> + [...] >> + >> +const int pinmuxes_size = ARRAY_SIZE(pinmuxes); > > Where do you use this one? and why not just inline it? This is introduced through patch (not in mainline yet): [U-Boot,v3,07/15] arm, davinci: Remove duplication of pinmux configuration code http://patchwork.ozlabs.org/patch/127680/ >> + >> +struct gpio_config { >> + char name[GPIO_NAME_SIZE]; >> + unsigned char bank; >> + unsigned char gpio; >> + unsigned char out; >> + unsigned char value; >> +}; >> + >> +static const struct gpio_config enbw_gpio_config[] = { >> + { "RS485 enable", 8, 11, 1, 0 }, >> + { "RS485 iso", 8, 10, 1, 0 }, >> + { "W2HUT RS485 Rx ena", 8, 9, 1, 0 }, >> + { "W2HUT RS485 iso", 8, 8, 1, 0 }, >> + { "LAN reset", 7, 15, 1, 1 }, >> + { "ena 11V PLC", 7, 14, 1, 0 }, >> + { "ena 1.5V PLC", 7, 13, 1, 0 }, >> + { "disable VBUS", 7, 12, 1, 1 }, >> + { "PLC reset", 6, 13, 1, 1 }, >> + { "LCM RS", 6, 12, 1, 0 }, >> + { "LCM R/W", 6, 11, 1, 0 }, >> + { "PLC pairing", 6, 10, 1, 0 }, >> + { "PLC MDIO CLK", 6, 9, 1, 0 }, >> + { "HK218", 6, 8, 1, 0 }, >> + { "HK218 Rx", 6, 1, 1, 1 }, >> + { "TPM reset", 6, 0, 1, 1 }, >> + { "LCM E", 2, 2, 1, 1 }, >> + { "PV-IF RxD ena", 0, 15, 1, 0 }, >> + { "LED1", 1, 15, 1, 1 }, >> + { "LED2", 0, 1, 1, 1 }, >> + { "LED3", 0, 2, 1, 1 }, >> + { "LED4", 0, 3, 1, 1 }, >> + { "LED5", 0, 4, 1, 1 }, >> + { "LED6", 0, 5, 1, 0 }, >> + { "LED7", 0, 6, 1, 0 }, >> + { "LED8", 0, 14, 1, 0 }, >> + { "USER1", 0, 12, 0, 0 }, >> + { "USER2", 0, 13, 0, 0 }, >> +}; > > This will look much better if all values will be aligned - > making columns. Done. > Also, IMO, this is usable platform wide. Hmm.. this is board specific gpio pin setup ... >> + >> +#ifndef CONFIG_USE_IRQ >> +void irq_init(void) >> +{ >> + /* >> + * Mask all IRQs by clearing the global enable and setting >> + * the enable clear for all the 90 interrupts. >> + */ >> + >> + writel(0, &davinci_aintc_regs->ger); >> + >> + writel(0, &davinci_aintc_regs->hier); >> + >> + writel(0xffffffff, &davinci_aintc_regs->ecr1); >> + writel(0xffffffff, &davinci_aintc_regs->ecr2); >> + writel(0xffffffff, &davinci_aintc_regs->ecr3); >> +} >> +#endif >> + >> +void davinci_emac_mii_mode_sel(int mode_sel) >> +{ >> + int val; >> + >> + val = readl(&davinci_syscfg_regs->cfgchip3); >> + if (mode_sel == 0) >> + val &= ~(1 << 8); >> + else >> + val |= (1 << 8); >> + writel(val, &davinci_syscfg_regs->cfgchip3); >> +} > > This one also can be used platform wide. Ok, done. Move for this board/davinci/common/misc.c to arch/arm/cpu/arm926ejs/davinci/misc.c in a seperate patch. [...] >> +int board_init(void) >> +{ >> + int i, ret; >> + >> +#ifndef CONFIG_USE_IRQ >> + irq_init(); >> +#endif >> + /* address of boot parameters, not used as booting with DTT */ >> + gd->bd->bi_boot_params = 0; >> + >> + for (i = 0; i < ARRAY_SIZE(enbw_gpio_config); i++) { >> + int gpio = enbw_gpio_config[i].bank * 16 + >> + enbw_gpio_config[i].gpio; >> + >> + ret = gpio_request(gpio, enbw_gpio_config[i].name); >> + if (ret) >> + printf("%s: Could not get %s gpio\n", __func__, >> + enbw_gpio_config[i].name); >> + else > > instead of having that else and adding another level of indentation below > you can just add continue; Done. >> + if (enbw_gpio_config[i].out) >> + gpio_direction_output(gpio, >> + enbw_gpio_config[i].value); >> + else >> + gpio_direction_input(gpio); >> + } >> + >> + >> + /* setup the SUSPSRC for ARM to control emulation suspend */ >> + clrbits_le32(&davinci_syscfg_regs->suspsrc, >> + (DAVINCI_SYSCFG_SUSPSRC_EMAC | DAVINCI_SYSCFG_SUSPSRC_I2C | >> + DAVINCI_SYSCFG_SUSPSRC_SPI1 | DAVINCI_SYSCFG_SUSPSRC_TIMER0 | >> + DAVINCI_SYSCFG_SUSPSRC_UART2)); >> + >> +#ifdef CONFIG_DRIVER_TI_EMAC >> + davinci_emac_mii_mode_sel(0); >> +#endif /* CONFIG_DRIVER_TI_EMAC */ > > Can't the above be called from board_eth_init()? > Or is it too late? Moved. [...] >> +U_BOOT_CMD(led, 3, 1, do_led, >> + "switch on/off board led", >> + "[name] [on/off]" >> +); > > Why can't you use the common/cmd_led.c? We should add the possibilty to define static const led_tbl_t led_commands board specific ... the more or less fix table is not fitting my board needs. [...] >> +void board_gpio_init(void) >> +{ >> + struct davinci_gpio *gpio = davinci_gpio_bank01; >> + int i; >> + >> + /* >> + * Power on required peripherals >> + * ARM does not have access by default to PSC0 and PSC1 >> + * assuming here that the DSP bootloader has set the IOPU >> + * such that PSC access is available to ARM >> + */ >> + for (i = 0; i < ARRAY_SIZE(lpsc); i++) >> + lpsc_on(lpsc[i].lpsc_no); >> + >> + /* >> + * set LED (gpio Interface not usable here) >> + * set LED pins to output and state 0 >> + */ >> + clrbits_le32(&gpio->dir, 0x8000407e); >> + clrbits_le32(&gpio->out_data, 0x8000407e); >> + /* set LED 1 - 5 to state on */ >> + setbits_le32(&gpio->out_data, 0x8000001e); >> + >> + return; > > no need for this return. removed. >> +} > > This also looks like can be done platform wide and > not for the specific board. Yep, calling here da8xx_configure_lpsc_items(). [...] >> +int board_mmc_init(bd_t *bis) >> +{ >> + int err; >> + >> + mmc_sd1.input_clk = clk_get(DAVINCI_MMC_CLKID); >> + /* Add slot-0 to mmc subsystem */ >> + err = davinci_mmc_init(bis, &mmc_sd1); >> + >> + return err; > > You can just return davinci_mmc_init(bis, &mmc_sd1); > and remove the err variable. Done. > Also, is clk_get() never fails? No. [...] >> diff --git a/include/configs/enbw_cmc.h b/include/configs/enbw_cmc.h >> new file mode 100644 >> index 0000000..bd00f39 >> --- /dev/null >> +++ b/include/configs/enbw_cmc.h [...] >> +#define CONFIG_ARM926EJS /* arm926ejs CPU core */ >> +#define CONFIG_SOC_DA8XX /* TI DA8xx SoC */ >> +#define CONFIG_SOC_DA850 /* TI DA850 SoC */ >> +#define CONFIG_SYS_CLK_FREQ clk_get(DAVINCI_ARM_CLKID) >> +#define CONFIG_SYS_OSCIN_FREQ 24000000 >> +#define CONFIG_SYS_TIMERBASE DAVINCI_TIMER0_BASE >> +#define CONFIG_SYS_HZ_CLOCK clk_get(DAVINCI_AUXCLK_CLKID) > > This and the one above it using clk_get() does not look good. > Is this how all davinci boards define it? Yes. >> +/* >> + * Flash & Environment >> + */ >> +#ifdef CONFIG_USE_NAND >> +#define CONFIG_NAND_DAVINCI >> +#define CONFIG_SYS_NAND_USE_FLASH_BBT >> +#define CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST >> +#define CONFIG_SYS_NAND_PAGE_2K > > Alignment? fixed. [...] Thanks for your comments! bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot