On 29/12/2011 09:42, Igor Grinberg wrote:
Hi Stefano,
Hi Igor,
Isn't Ilya is author of these? I would expect you (at least) to put him in Cc, no?
No, he's not, but these patches rely on Ilya's patches for AM3517. It is sure a good idea to add him in CC
+distclean: clean
- rm -f $(LIB) core *.bak $(obj).depend
Please, remove these...
Sure, bad cut&paste..
+#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...
ok
+#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);
Yes, and it is a cleaner way - got it, I will fix it.
Also, this function is called once, may be just inline it instead?
Agree.
+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?
I check it
+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);
Right - fixed in V2
+int fpga_pre_config_fn(int cookie) +{
needless empty line?
Thanks, I drop it
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).
After checking the hardware again there is no reason to compile without FPGA support. The board relies on the FPGA itself and U-Boot should never compiled without a way to load the fpga. For this reason I can drop completely any #ifdef CONFIG_FPGA.
+#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?
Sure, I check it in the whole file.
Thanks, Stefano Babic