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

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@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@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 */