[U-Boot] [PATCH 4/4] ARM: omap3: Added Teejet mt_ventoux

[Igor Grinberg <grinberg@compulab.co.il>]

Hi Stefano,

Isn't Ilya is author of these?
I would expect you (at least) to put him in Cc, no?

On 12/28/11 18:47, Stefano Babic wrote:
> The mt_ventoux board is a custom board using
> the Technexion TAM3517 module.
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---
>  MAINTAINERS                          |    1 +
>  board/teejet/mt_ventoux/Makefile     |   44 ++++
>  board/teejet/mt_ventoux/mt_ventoux.c |  235 +++++++++++++++++++
>  board/teejet/mt_ventoux/mt_ventoux.h |  429 ++++++++++++++++++++++++++++++++++
>  boards.cfg                           |    1 +
>  include/configs/mt_ventoux.h         |   61 +++++
>  6 files changed, 771 insertions(+), 0 deletions(-)
>  create mode 100644 board/teejet/mt_ventoux/Makefile
>  create mode 100644 board/teejet/mt_ventoux/mt_ventoux.c
>  create mode 100644 board/teejet/mt_ventoux/mt_ventoux.h
>  create mode 100644 include/configs/mt_ventoux.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4e3246d..d680eaf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -558,6 +558,7 @@ Stefano Babic <sbabic@denx.de>
>  
>  	ea20		davinci
>  	flea3		i.MX35
> +	mt_ventoux	omap3
>  	mx35pdk		i.MX35
>  	mx51evk		i.MX51
>  	polaris		xscale/pxa
> diff --git a/board/teejet/mt_ventoux/Makefile b/board/teejet/mt_ventoux/Makefile
> new file mode 100644
> index 0000000..ce20ec7
> --- /dev/null
> +++ b/board/teejet/mt_ventoux/Makefile
> @@ -0,0 +1,44 @@
> +#
> +# Copyright (C) 2011 Ilya Yanok, Emcraft Systems
> +#
> +# Based on ti/evm/Makefile
> +#
> +# 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.
> +#
> +
> +include $(TOPDIR)/config.mk
> +
> +LIB	= $(obj)lib$(BOARD).o
> +
> +COBJS	:= $(BOARD).o
> +
> +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

Please, remove these...

> +
> +#########################################################################
> +
> +# defines $(obj).depend target
> +include $(SRCTREE)/rules.mk
> +
> +sinclude $(obj).depend
> diff --git a/board/teejet/mt_ventoux/mt_ventoux.c b/board/teejet/mt_ventoux/mt_ventoux.c
> new file mode 100644
> index 0000000..dd8e244
> --- /dev/null
> +++ b/board/teejet/mt_ventoux/mt_ventoux.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright (C) 2011
> + * Stefano Babic, DENX Software Engineering, sbabic at denx.de.
> + *
> + * Copyright (C) 2009 TechNexion Ltd.
> + *
> + * 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.
> + */
> +
> +#include <common.h>
> +#include <netdev.h>
> +#include <fpga.h>
> +#include <asm/io.h>
> +#include <asm/arch/mem.h>
> +#include <asm/arch/mux.h>
> +#include <asm/arch/sys_proto.h>
> +#include <asm/omap_gpio.h>
> +#include <asm/arch/mmc_host_def.h>
> +#include <i2c.h>
> +#include <spartan3.h>
> +#include <asm/gpio.h>
> +#include "mt_ventoux.h"
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#if defined(CONFIG_FPGA)

Probably #ifdef would be better or
see the comment in the end of this section...

> +
> +#define FPGA_RESET	62
> +#define FPGA_PROG	116
> +#define FPGA_CCLK	117
> +#define FPGA_DIN	118
> +#define FPGA_INIT	119
> +#define FPGA_DONE	154
> +
> +/* Timing definitions for FPGA */
> +static const u32 gpmc_fpga[] = {
> +	FPGA_GPMC_CONFIG1,
> +	FPGA_GPMC_CONFIG2,
> +	FPGA_GPMC_CONFIG3,
> +	FPGA_GPMC_CONFIG4,
> +	FPGA_GPMC_CONFIG5,
> +	FPGA_GPMC_CONFIG6,
> +};
> +
> +static void fpga_reset(int nassert)
> +{
> +	if (nassert)
> +		gpio_set_value(FPGA_RESET, 0);
> +	else
> +		gpio_set_value(FPGA_RESET, 1);
> +}

Isn't this just:
gpio_set_value(FPGA_RESET, !nassert);

Also, this function is called once, may be just inline it instead?

> +
> +int fpga_pgm_fn(int nassert, int nflush, int cookie)
> +{
> +	debug("%s:%d: FPGA PROGRAM ", __func__, __LINE__);
> +
> +	if (nassert) {
> +		gpio_set_value(FPGA_PROG, 0);
> +		debug("asserted\n");
> +	} else {
> +		gpio_set_value(FPGA_PROG, 1);
> +		debug("deasserted\n");
> +	}
> +
> +	return nassert;
> +}

This function look very similar to the fpga_reset(),
may they can be consolidated in some way?

> +
> +int fpga_init_fn(int cookie)
> +{
> +	u32 val = gpio_get_value(FPGA_INIT);
> +
> +	return (val == 0);
> +}

Isn't this just:
return !gpio_get_value(FPGA_INIT);
?

> +
> +int fpga_done_fn(int cookie)
> +{
> +	u32 val = gpio_get_value(FPGA_DONE);
> +
> +	if (val)
> +		return 1;
> +
> +	return 0;
> +}

return gpio_get_value(FPGA_DONE);
?

> +
> +int fpga_pre_config_fn(int cookie)
> +{
> +

needless empty line?

> +	debug("%s:%d: FPGA pre-configuration\n", __func__, __LINE__);
> +
> +	/* Setting GPIOs for programming Mode */
> +	gpio_request(FPGA_RESET, "FPGA_RESET");
> +	gpio_direction_output(FPGA_RESET, 1);
> +	gpio_request(FPGA_PROG, "FPGA_PROG");
> +	gpio_direction_output(FPGA_PROG, 1);
> +	gpio_request(FPGA_CCLK, "FPGA_CCLK");
> +	gpio_direction_output(FPGA_CCLK, 1);
> +	gpio_request(FPGA_DIN, "FPGA_DIN");
> +	gpio_direction_output(FPGA_DIN, 0);
> +	gpio_request(FPGA_INIT, "FPGA_INIT");
> +	gpio_direction_input(FPGA_INIT);
> +	gpio_request(FPGA_DONE, "FPGA_DONE");
> +	gpio_direction_input(FPGA_DONE);
> +
> +	/* Be sure that signal are deasserted */
> +	gpio_set_value(FPGA_RESET, 1);
> +	gpio_set_value(FPGA_PROG, 1);
> +
> +	return 0;
> +}
> +
> +int fpga_post_config_fn(int cookie)
> +{
> +

same here

> +	debug("%s:%d: FPGA post-configuration\n", __func__, __LINE__);
> +
> +	fpga_reset(TRUE);
> +	udelay(100);
> +	fpga_reset(FALSE);
> +
> +	return 0;
> +}
> +
> +/* Write program to the FPGA */
> +int fpga_wr_fn(int nassert_write, int flush, int cookie)
> +{
> +	if (nassert_write)
> +		gpio_set_value(FPGA_DIN, 1);
> +	else
> +		gpio_set_value(FPGA_DIN, 0);
> +
> +	return nassert_write;
> +}

gpio_set_value(FPGA_DIN, nassert_write);
return nassert_write;
?

And also below?

> +
> +int fpga_clk_fn(int assert_clk, int flush, int cookie)
> +{
> +	if (assert_clk)
> +		gpio_set_value(FPGA_CCLK, 1);
> +	else
> +		gpio_set_value(FPGA_CCLK, 0);
> +
> +	return assert_clk;
> +}
> +
> +Xilinx_Spartan3_Slave_Serial_fns mt_ventoux_fpga_fns = {
> +	fpga_pre_config_fn,
> +	fpga_pgm_fn,
> +	fpga_clk_fn,
> +	fpga_init_fn,
> +	fpga_done_fn,
> +	fpga_wr_fn,
> +	fpga_post_config_fn,
> +};
> +
> +Xilinx_desc fpga = XILINX_XC6SLX4_DESC(slave_serial,
> +			(void *)&mt_ventoux_fpga_fns, 0);
> +
> +/* Initialize the FPGA */
> +static void mt_ventoux_init_fpga(void)
> +{
> +	fpga_pre_config_fn(0);
> +
> +	/* Setting CS1 for FPGA access */
> +	enable_gpmc_cs_config(gpmc_fpga, &gpmc_cfg->cs[1],
> +		FPGA_BASE_ADDR, GPMC_SIZE_128M);
> +
> +	fpga_init();
> +	fpga_add(fpga_xilinx, &fpga);
> +}
> +#endif

I would also recommend to put all this fpga handling code in
a separate file and tweak the Makefile to compile it out if
!CONFIG_FPGA instead of the #ifdefs here.
This can also spare you the need for #ifdef around
mt_ventoux_init_fpga(); below (if you stub it in a .h file).

> +
> +/*
> + * Routine: board_init
> + * Description: Early hardware init.
> + */
> +int board_init(void)
> +{
> +	gpmc_init(); /* in SRAM or SDRAM, finish GPMC */
> +
> +	/* boot param addr */
> +	gd->bd->bi_boot_params = (OMAP34XX_SDRC_CS0 + 0x100);
> +
> +#if defined(CONFIG_FPGA)
> +	mt_ventoux_init_fpga();
> +#endif
> +
> +	return 0;
> +}

[...]

> diff --git a/include/configs/mt_ventoux.h b/include/configs/mt_ventoux.h
> new file mode 100644
> index 0000000..f588558
> --- /dev/null
> +++ b/include/configs/mt_ventoux.h
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright (C) 2011
> + * Stefano Babic, DENX Software Engineering, sbabic at denx.de.
> + *
> + * Copyright (C) 2009 TechNexion Ltd.
> + *
> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#ifndef __CONFIG_H
> +#define __CONFIG_H
> +
> +#include "tam3517-common.h"
> +
> +#define MACH_TYPE_AM3517_MT_VENTOUX	3832
> +#define CONFIG_MACH_TYPE	MACH_TYPE_AM3517_MT_VENTOUX
> +
> +#define	CONFIG_CMD_FPGA

Can it be space instead of tab after define, like all others?

> +
> +#define CONFIG_BOOTDELAY	10
> +#define CONFIG_BOOTFILE		"uImage"
> +#define CONFIG_AUTO_COMPLETE
> +
> +#define CONFIG_HOSTNAME mt_ventoux
> +
> +/*
> + * Miscellaneous configurable options
> + */
> +#define V_PROMPT			"mt_ventoux => "
> +#define CONFIG_SYS_PROMPT		V_PROMPT
> +
> +/*
> + * FPGA
> + */
> +#ifdef	CONFIG_CMD_FPGA
> +#define	CONFIG_FPGA
> +#define	CONFIG_FPGA_XILINX
> +#define	CONFIG_FPGA_SPARTAN3
> +#define	CONFIG_SYS_FPGA_PROG_FEEDBACK
> +#define	CONFIG_SYS_FPGA_WAIT	10000
> +#define	CONFIG_MAX_FPGA_DEVICES	1

Can it be space after the define, like all others?

> +#define CONFIG_FPGA_DELAY() udelay(1)
> +#define CONFIG_SYS_FPGA_PROG_FEEDBACK
> +#endif
> +
> +#define	CONFIG_EXTRA_ENV_SETTINGS	CONFIG_TAM3517_SETTINGS \

same here

> +	"bootcmd=run net_nfs\0"
> +
> +#endif /* __CONFIG_H */

-- 
Regards,
Igor.