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

Thu Dec 15 09:47:05 CET 2011

Hi Peter,

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

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