Hi Peter, In addition to Tom's comments, several comments below:
On 12/15/11 00:47, Peter Barada wrote: > From: Peter Barada <pet...@logicpd.com> > > This patch adds basic support for OMAP35x/DM37x SOM LV/Torpedo > reference boards. It assumes U-boot is loaded to SDRAM with the > help of another small bootloader (x-load) running from SRAM. > > Signed-off-by: Peter Barada <peter.bar...@logicpd.com> > --- > board/logicpd/omap3som/Makefile | 48 +++ > board/logicpd/omap3som/config.mk | 33 ++ > board/logicpd/omap3som/omap3logic.c | 566 > +++++++++++++++++++++++++++++++++++ > board/logicpd/omap3som/omap3logic.h | 35 +++ > boards.cfg | 1 + > include/configs/omap3_logic.h | 356 ++++++++++++++++++++++ > 6 files changed, 1039 insertions(+), 0 deletions(-) > create mode 100644 board/logicpd/omap3som/Makefile > create mode 100644 board/logicpd/omap3som/config.mk > create mode 100644 board/logicpd/omap3som/omap3logic.c > create mode 100644 board/logicpd/omap3som/omap3logic.h > create mode 100644 include/configs/omap3_logic.h > > diff --git a/board/logicpd/omap3som/Makefile b/board/logicpd/omap3som/Makefile > new file mode 100644 > index 0000000..ef0409f > --- /dev/null > +++ b/board/logicpd/omap3som/Makefile > @@ -0,0 +1,48 @@ > +# > +# (C) Copyright 2000, 2001, 2002 > +# Wolfgang Denk, DENX Software Engineering, w...@denx.de. > +# > +# See file CREDITS for list of people who contributed to this > +# project. > +# > +# This program is free software; you can redistribute it and/or > +# modify it under the terms of the GNU General Public License as > +# published by the Free Software Foundation; either version 2 of > +# the License, or (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write to the Free Software > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, > +# MA 02111-1307 USA > +# > + > +include $(TOPDIR)/config.mk > + > +LIB = $(obj)lib$(BOARD).o > + > +COBJS-y := omap3logic.o > + > +COBJS := $(sort $(COBJS-y)) > +SRCS := $(COBJS:.o=.c) > +OBJS := $(addprefix $(obj),$(COBJS)) > + > +$(LIB): $(obj).depend $(OBJS) > + $(call cmd_link_o_target, $(OBJS)) > + > +clean: > + rm -f $(OBJS) > + > +distclean: clean > + rm -f $(LIB) core *.bak $(obj).depend clean and distclean are obsolete in this directory level, please remove. > + > +######################################################################### > + > +# defines $(obj).depend target > +include $(SRCTREE)/rules.mk > + > +sinclude $(obj).depend > diff --git a/board/logicpd/omap3som/config.mk > b/board/logicpd/omap3som/config.mk > new file mode 100644 > index 0000000..897b252 > --- /dev/null > +++ b/board/logicpd/omap3som/config.mk > @@ -0,0 +1,33 @@ > +# > +# (C) Copyright 2006 - 2008 > +# Texas Instruments, <www.ti.com> > +# > +# EVM uses OMAP3 (ARM-CortexA8) cpu > +# see http://www.ti.com/ for more information on Texas Instruments > +# > +# See file CREDITS for list of people who contributed to this > +# project. > +# > +# This program is free software; you can redistribute it and/or > +# modify it under the terms of the GNU General Public License as > +# published by the Free Software Foundation; either version 2 of > +# the License, or (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write to the Free Software > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, > +# MA 02111-1307 USA > +# > +# Physical Address: > +# 8000'0000 (bank0) > +# A000/0000 (bank1) > +# Linux-Kernel is expected to be at 8000'8000, entry 8000'8000 > +# (mem base + reserved) > + > +# For use with external or internal boots. > +CONFIG_SYS_TEXT_BASE = 0x80400000 As Tom already said, this should be in the board config file and this file should be removed completely. > diff --git a/board/logicpd/omap3som/omap3logic.c > b/board/logicpd/omap3som/omap3logic.c > new file mode 100644 > index 0000000..5c6e896 > --- /dev/null > +++ b/board/logicpd/omap3som/omap3logic.c > @@ -0,0 +1,566 @@ > +/* > + * (C) Copyright 2011 > + * Logic Product Development <www.logicpd.com> > + * > + * Author : > + * Peter Barada <peter.bar...@logicpd.com> > + * > + * Derived from Beagle Board and 3430 SDP code by > + * Richard Woodruff <r-woodru...@ti.com> > + * Syed Mohammed Khasim <kha...@ti.com> > + * > + * See file CREDITS for list of people who contributed to this > + * project. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > + * MA 02111-1307 USA I vote for removing the postal address, because it is subject to change and you will not follow it, but it is not a blocker. [...] > +/* > + * Routine: logic_identify > + * Description: Detect if we are running on a Logic or Torpedo. > + * This can be done by GPIO_189. If its low after driving it > high, > + * then its a SOM LV, else Torpedo. > + */ > +unsigned int logic_identify(void) > +{ > + unsigned int val = 0; > + u32 cpu_family = get_cpu_family(); You only use this once, so IMO can be inlined. > + int i; > + > + MUX_LOGIC_HSUSB0_DATA5_GPIO_MUX(); > + > + if (!gpio_request(189, "")) { This does not look good... can it be: if (gpio_request(...) == 0) and also please provide a label with a description instead of an empty one. > + > + gpio_direction_output(189, 0); > + gpio_set_value(189, 1); > + > + /* Let it soak for a bit */ > + for (i = 0; i < 0x100; ++i) > + asm("nop"); > + > + gpio_direction_input(189); > + val = gpio_get_value(189); > + gpio_free(189); > + > + printf("Board:"); > + if (cpu_family == CPU_OMAP36XX) { > + printf(" DM37xx"); > + if (val) { > + printf(" Torpedo\n"); > + val = MACH_TYPE_DM3730_TORPEDO; > + } else { > + printf(" SOM LV\n"); > + val = MACH_TYPE_DM3730_SOM_LV; > + } > + } else { > + printf(" OMAP35xx"); > + if (val) { > + printf(" Torpedo\n"); > + val = MACH_TYPE_OMAP3_TORPEDO; > + } else { > + printf(" SOM LV\n"); > + val = MACH_TYPE_OMAP3530_LV_SOM; > + } > + } > + } > + > + MUX_LOGIC_HSUSB0_DATA5_DATA5(); > + > + return val; > +} The whole function looks like checkboard(), isn't it? I think it should be then. [...] > +/* > + * Routine: board_init > + * Description: Early hardware init. > + */ > +int board_init(void) > +{ > + gpmc_init(); /* in SRAM or SDRAM, finish GPMC */ > + > + /* board id for Linux */ > + gd->bd->bi_arch_number = logic_identify(); This also can be moved to checkboard(). > + > + /* boot param addr */ > + gd->bd->bi_boot_params = (OMAP34XX_SDRC_CS0 + 0x100); > + > + > + return 0; > +} [...] > +/* > + * Routine: misc_init_r > + * Description: Init ethernet (done here so udelay works) > + */ > +int misc_init_r(void) > +{ > +#if defined(CONFIG_CMD_NET) > + setup_net_chip(); > +#endif Can this be done in board_eth_init()? So the net init code will be close to each other? > + > + dieid_num_r(); > + > + return 0; > +} > + > + > + Three empty lines? There should be only one line between functions. > +int board_eth_init(bd_t *bis) > +{ > + int rc = 0; > +#ifdef CONFIG_SMC911X > + rc = smc911x_initialize(0, CONFIG_SMC911X_BASE); > +#endif > + return rc; > +} board_eth_init() has a weak implementation, so I think it would be much nicer: #ifdef CONFIG_SMC911X int board_eth_init(bd_t *bis) { setup_net_chip(); return smc911x_initialize(0, CONFIG_SMC911X_BASE); } #endif > + > + > + Again three empty lines? > +/* > + * IEN - Input Enable > + * IDIS - Input Disable > + * PTD - Pull type Down > + * PTU - Pull type Up > + * DIS - Pull type selection is inactive > + * EN - Pull type selection is active > + * M0 - Mode 0 > + * The commented string gives the final mux configuration for that pin > + */ > + > +/* > + * Routine: set_muxconf_regs > + * Description: Setting up the configuration Mux registers specific to the > + * hardware. Many pins need to be moved from protect to primary > + * mode. > + */ > +void set_muxconf_regs(void) > +{ > + /*SDRC*/ Alignment... [...] > + /*GPMC*/ same here, comments must be aligned to code. [...] > + /*DSS*/ ditto [...] > + /*CAMERA*/ ditto [...] > + /*Audio Interface */ ditto > + MUX_VAL(CP(MCBSP2_FSX), (IEN | PTD | EN | M7)); /*safe mode */ > + MUX_VAL(CP(MCBSP2_CLKX), (IEN | PTD | EN | M7)); /*safe mode */ > + MUX_VAL(CP(MCBSP2_DR), (IEN | PTD | EN | M7)); /*safe mode */ > + MUX_VAL(CP(MCBSP2_DX), (IEN | PTD | EN | M7)); /*safe mode */ > + > + /*Expansion card */ ditto > + MUX_VAL(CP(MMC1_CLK), (IDIS | PTU | EN | M0)); > + MUX_VAL(CP(MMC1_CMD), (IEN | PTU | EN | M0)); > + MUX_VAL(CP(MMC1_DAT0), (IEN | PTU | EN | M0)); > + MUX_VAL(CP(MMC1_DAT1), (IEN | PTU | EN | M0)); > + MUX_VAL(CP(MMC1_DAT2), (IEN | PTU | EN | M0)); > + MUX_VAL(CP(MMC1_DAT3), (IEN | PTU | EN | M0)); > + MUX_VAL(CP(MMC1_DAT4), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC1_DAT5), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC1_DAT6), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC1_DAT7), (IEN | PTD | EN | M7)); /*safe mode*/ > + /*Wireless LAN */ ditto > + MUX_VAL(CP(MMC2_CLK), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC2_CMD), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC2_DAT0), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC2_DAT1), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC2_DAT2), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC2_DAT3), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC2_DAT4), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC2_DAT5), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC2_DAT6), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MMC2_DAT7), (IEN | PTD | EN | M7)); /*safe mode*/ > + /*Bluetooth*/ ditto > + MUX_VAL(CP(MCBSP3_DX), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP3_DR), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP3_CLKX), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP3_FSX), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(UART2_CTS), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(UART2_RTS), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(UART2_TX), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(UART2_RX), (IEN | PTD | EN | M7)); /*safe mode*/ > + /*Modem Interface */ ditto > + MUX_VAL(CP(UART1_TX), (IDIS | PTD | DIS | M0)); > + MUX_VAL(CP(UART1_RTS), (IDIS | PTD | DIS | M0)); > + MUX_VAL(CP(UART1_CTS), (IEN | PTU | DIS | M0)); > + MUX_VAL(CP(UART1_RX), (IEN | PTD | DIS | M0)); > + MUX_VAL(CP(MCBSP4_CLKX), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP4_DR), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP4_DX), (IDIS | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP4_FSX), (IDIS | PTD | EN | M7)); /*safe mode*/ > + > + MUX_VAL(CP(MCBSP1_CLKR), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP1_FSR), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP1_DX), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP1_DR), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP_CLKS), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP1_FSX), (IEN | PTD | EN | M7)); /*safe mode*/ > + MUX_VAL(CP(MCBSP1_CLKX), (IEN | PTD | EN | M7)); /*safe mode*/ > + /*Serial Interface*/ ditto [...] > + /*Control and debug */ ditto [...] > + /*Die to Die */ ditto [...] > +} I think this function is *much better* than the mux config done .h file. Good job! -- Regards, Igor. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
