[U-Boot] [PATCH 1/1] ARMV7: Add support For Logic OMAP35x/DM37x modules

Igor Grinberg
2011-12-15T08:47:05Z

Hi Peter,

In addition to Tom's comments, several comments below:

On 12/15/11 00:47, Peter Barada wrote:
> From: Peter Barada <peterb@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.barada@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, wd at 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.barada@logicpd.com>
> + *
> + * Derived from Beagle Board and 3430 SDP code by
> + * Richard Woodruff <r-woodruff2@ti.com>
> + * Syed Mohammed Khasim <khasim@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.