Skip to contentSkip to conversation

RFC: Some improvements for the FPGA subsystem

U-Boot-Users Matthias Fuchs 5 participants 17 messages Nov 11, 2007 – Dec 13, 2007 Arm/Zynq architecture

lore
Share & tools
lore

Matthias Fuchs

author
#0 lore raw

Hi,

we had some discussions about the FPGA subsystem some days (and also a couple of months) before on this list. I have also some local improvements for the FPGA subsystem.

Before posting a couple of clean patches I dare to post a single patch just to get an ok for the different topics. When all points are clear, I will split my patch into one patch per per topic.

This is what I like to get into U-Boot:

1) Replace the CONFIG_FPGA bit mask by U-Boot like configuration options:

CONFIG_FPGA
- enable FPGA subsystem

CONFIG_FPGA_<vendor>
- enable support for specific chip vendors (ALTERA, XILINX)

CONFIG_FPGA_<family>
- enable support for FPGA family
(SPARTAN2, SPARTAN3, VIRTEX2, CYCLONE2, ACEX1K, ACEX)

This means when you have an Xilinx Spartan3 FPGA on your board you typically have these lines in your board configuration:

#define CONFIG_FPGA
#define CONFIG_FPGA_XILINX
#define CONFIG_FPGA_SPARTAN3

(TODO: add this to the README file)

2) Fix FPGA support for some boards that will get broken through the above change (GEN860T, ...)

3) Remove bit swapping in fpga_loadbitstream().

4) Use AND-operation for checking MSB of bytes instead of sign check for Spartan2/3 FPGAs in slave serial code.

5) Add post() and pre() callback for Spartan2/3 FPGAs inslave serial mode.

6) Add some new FPGA types.

Matthias

board/gen860t/fpga.c +1 -1
index 2ba7e0e..5997584 100644
@@ -34,7 +34,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#if (CONFIG_FPGA)
+#ifdef CONFIG_FPGA
 
 #if 0
 #define GEN860T_FPGA_DEBUG
board/gen860t/gen860t.c +1 -1
index d448f9f..70341f8 100644
@@ -254,7 +254,7 @@ int misc_init_r (void)
 	mii_init ();
 #endif
 
-#if (CONFIG_FPGA)
+#ifdef CONFIG_FPGA
 	gen860t_init_fpga ();
 #endif
 	return 0;
common/ACEX1K.c +2 -2
index 2a421e2..76dc166 100644
@@ -28,7 +28,7 @@
 #include <common.h>		/* core U-Boot definitions */
 #include <ACEX1K.h>		/* ACEX device family */
 
-#if (CONFIG_FPGA & (CFG_ALTERA | CFG_ACEX1K))
+#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_ALTERA) && defined(CONFIG_FPGA_ACEX1K)
 
 /* Define FPGA_DEBUG to get debug printf's */
 #ifdef	FPGA_DEBUG

@@ -363,4 +363,4 @@ static int ACEX1K_ps_reloc (Altera_desc * desc, ulong reloc_offset)
 
 }
 
-#endif /* (CONFIG_FPGA & (CFG_ALTERA | CFG_ACEX1K)) */
+#endif /* CONFIG_FPGA && CONFIG_FPGA_ALTERA && CONFIG_FPGA_ACEX1K */
common/altera.c +9 -9
index 06e8a95..eb874e4 100644
@@ -40,7 +40,7 @@
 #define PRINTF(fmt,args...)
 #endif
 
-#if (CONFIG_FPGA & CFG_FPGA_ALTERA)
+#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_ALTERA)
 
 /* Local Static Functions */
 static int altera_validate (Altera_desc * desc, char *fn);

@@ -56,11 +56,11 @@ int altera_load( Altera_desc *desc, void *buf, size_t bsize )
 		switch (desc->family) {
 		case Altera_ACEX1K:
 		case Altera_CYC2:
-#if (CONFIG_FPGA & CFG_ACEX1K)
+#ifdef CONFIG_FPGA_ACEX1K
 			PRINTF ("%s: Launching the ACEX1K Loader...\n",
 					__FUNCTION__);
 			ret_val = ACEX1K_load (desc, buf, bsize);
-#elif (CONFIG_FPGA & CFG_CYCLON2)
+#elif defined CONFIG_FPGA_CYCLON2
 			PRINTF ("%s: Launching the CYCLON II Loader...\n",
 					__FUNCTION__);
 			ret_val = CYC2_load (desc, buf, bsize);

@@ -88,7 +88,7 @@ int altera_dump( Altera_desc *desc, void *buf, size_t bsize )
 	} else {
 		switch (desc->family) {
 		case Altera_ACEX1K:
-#if (CONFIG_FPGA & CFG_ACEX)
+#ifdef CONFIG_FPGA_ACEX
 			PRINTF ("%s: Launching the ACEX1K Reader...\n",
 					__FUNCTION__);
 			ret_val = ACEX1K_dump (desc, buf, bsize);

@@ -156,9 +156,9 @@ int altera_info( Altera_desc *desc )
 			switch (desc->family) {
 			case Altera_ACEX1K:
 			case Altera_CYC2:
-#if (CONFIG_FPGA & CFG_ACEX1K)
+#ifdef CONFIG_FPGA_ACEX1K
 				ACEX1K_info (desc);
-#elif (CONFIG_FPGA & CFG_CYCLON2)
+#elif defined CONFIG_FPGA_CYCLON2
 				CYC2_info (desc);
 #else
 				/* just in case */

@@ -192,7 +192,7 @@ int altera_reloc( Altera_desc *desc, ulong reloc_offset)
 	} else {
 		switch (desc->family) {
 		case Altera_ACEX1K:
-#if (CONFIG_FPGA & CFG_ACEX1K)
+#ifdef CONFIG_FPGA_ACEX1K
 			ret_val = ACEX1K_reloc (desc, reloc_offset);
 #else
 			printf ("%s: No support for ACEX devices.\n",

@@ -200,7 +200,7 @@ int altera_reloc( Altera_desc *desc, ulong reloc_offset)
 #endif
 			break;
 		case Altera_CYC2:
-#if (CONFIG_FPGA & CFG_CYCLON2)
+#ifdef CONFIG_FPGA_CYCLON2
 			ret_val = CYC2_reloc (desc, reloc_offset);
 #else
 			printf ("%s: No support for CYCLON II devices.\n",

@@ -249,4 +249,4 @@ static int altera_validate (Altera_desc * desc, char *fn)
 
 /* ------------------------------------------------------------------------- */
 
-#endif /* CONFIG_FPGA & CFG_FPGA_ALTERA */
+#endif /* CONFIG_FPGA & CONFIG_FPGA_ALTERA */
common/cmd_fpga.c +3 -38

59 lines not on this page · raw message

common/cyclon2.c +2 -2
index dce13b5..06f5e8a 100644
@@ -27,7 +27,7 @@
 #include <altera.h>
 #include <ACEX1K.h>		/* ACEX device family */
 
-#if (CONFIG_FPGA & (CFG_ALTERA | CFG_CYCLON2))
+#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_ALTERA) && defined(CONFIG_FPGA_CYCLON2)
 
 /* Define FPGA_DEBUG to get debug printf's */
 #ifdef	FPGA_DEBUG

@@ -302,4 +302,4 @@ static int CYC2_ps_reloc (Altera_desc * desc, ulong reloc_offset)
 	return ret_val;
 }
 
-#endif /* (CONFIG_FPGA & (CFG_ALTERA | CFG_CYCLON2)) */
+#endif /* CONFIG_FPGA && CONFIG_FPGA_ALTERA && CONFIG_FPGA_CYCLON2 */
common/fpga.c +11 -19
index 2eff239..e4073be 100644
@@ -67,14 +67,11 @@ static int fpga_dev_info( int devnum );
 static void fpga_no_sup( char *fn, char *msg )
 {
 	if ( fn && msg ) {
-		printf( "%s: No support for %s.  CONFIG_FPGA defined as 0x%x.\n",
-				fn, msg, CONFIG_FPGA );
+		printf( "%s: No support for %s.\n", fn, msg);
 	} else if ( msg ) {
-		printf( "No support for %s. CONFIG_FPGA defined as 0x%x.\n",
-				msg, CONFIG_FPGA );
+		printf( "No support for %s.\n", msg);
 	} else {
-		printf( "No FPGA suport!  CONFIG_FPGA defined as 0x%x.\n",
-				CONFIG_FPGA );
+		printf( "No FPGA suport!\n");
 	}
 }
 

@@ -112,11 +109,6 @@ static __attribute__((__const__)) fpga_desc * __attribute__((__const__)) fpga_va
 		printf( "%s: Null buffer.\n", fn );
 		return (fpga_desc * const)NULL;
 	}
-	if ( !bsize ) {
-		printf( "%s: Null buffer size.\n", fn );
-		return (fpga_desc * const)NULL;
-	}
-
 	return desc;
 }
 

@@ -135,7 +127,7 @@ static int fpga_dev_info( int devnum )
 
 		switch ( desc->devtype ) {
 		case fpga_xilinx:
-#if CONFIG_FPGA & CFG_FPGA_XILINX
+#ifdef CONFIG_FPGA_XILINX
 			printf( "Xilinx Device\nDescriptor @ 0x%p\n", desc );
 			ret_val = xilinx_info( desc->devdesc );
 #else

@@ -143,7 +135,7 @@ static int fpga_dev_info( int devnum )
 #endif
 			break;
 		case fpga_altera:
-#if CONFIG_FPGA & CFG_FPGA_ALTERA
+#ifdef CONFIG_FPGA_ALTERA
 			printf( "Altera Device\nDescriptor @ 0x%p\n", desc );
 			ret_val = altera_info( desc->devdesc );
 #else

@@ -175,14 +167,14 @@ int fpga_reloc( fpga_type devtype, void *desc, ulong reloc_off )
 
 	switch ( devtype ) {
 	case fpga_xilinx:
-#if CONFIG_FPGA & CFG_FPGA_XILINX
+#ifdef CONFIG_FPGA_XILINX
 		ret_val = xilinx_reloc( desc, reloc_off );
 #else
 		fpga_no_sup( (char *)__FUNCTION__, "Xilinx devices" );
 #endif
 		break;
 	case fpga_altera:
-#if CONFIG_FPGA & CFG_FPGA_ALTERA
+#ifdef CONFIG_FPGA_ALTERA
 		ret_val = altera_reloc( desc, reloc_off );
 #else
 		fpga_no_sup( (char *)__FUNCTION__, "Altera devices" );

@@ -268,14 +260,14 @@ int fpga_load( int devnum, void *buf, size_t bsize )
 	if ( desc ) {
 		switch ( desc->devtype ) {
 		case fpga_xilinx:
-#if CONFIG_FPGA & CFG_FPGA_XILINX
+#ifdef CONFIG_FPGA_XILINX
 			ret_val = xilinx_load( desc->devdesc, buf, bsize );
 #else
 			fpga_no_sup( (char *)__FUNCTION__, "Xilinx devices" );
 #endif
 			break;
 		case fpga_altera:
-#if CONFIG_FPGA & CFG_FPGA_ALTERA
+#ifdef CONFIG_FPGA_ALTERA
 			ret_val = altera_load( desc->devdesc, buf, bsize );
 #else
 			fpga_no_sup( (char *)__FUNCTION__, "Altera devices" );

@@ -301,14 +293,14 @@ int fpga_dump( int devnum, void *buf, size_t bsize )
 	if ( desc ) {
 		switch ( desc->devtype ) {
 		case fpga_xilinx:
-#if CONFIG_FPGA & CFG_FPGA_XILINX
+#ifdef CONFIG_FPGA_XILINX
 			ret_val = xilinx_dump( desc->devdesc, buf, bsize );
 #else
 			fpga_no_sup( (char *)__FUNCTION__, "Xilinx devices" );
 #endif
 			break;
 		case fpga_altera:
-#if CONFIG_FPGA & CFG_FPGA_ALTERA
+#ifdef CONFIG_FPGA_ALTERA
 			ret_val = altera_dump( desc->devdesc, buf, bsize );
 #else
 			fpga_no_sup( (char *)__FUNCTION__, "Altera devices" );
common/spartan2.c +19 -5
index 0fb23b6..6d1ed96 100644
@@ -25,7 +25,7 @@
 #include <common.h>		/* core U-Boot definitions */
 #include <spartan2.h>		/* Spartan-II device family */
 
-#if (CONFIG_FPGA & (CFG_XILINX | CFG_SPARTAN2))
+#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_SPARTAN2)
 
 /* Define FPGA_DEBUG to get debug printf's */
 #ifdef	FPGA_DEBUG

@@ -441,7 +441,7 @@ static int Spartan2_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 	int ret_val = FPGA_FAIL;	/* assume the worst */
 	Xilinx_Spartan2_Slave_Serial_fns *fn = desc->iface_fns;
 	int i;
-	char  val;
+	unsigned char  val;
 
 	PRINTF ("%s: start with interface functions @ 0x%p\n",
 			__FUNCTION__, fn);

@@ -516,7 +516,7 @@ static int Spartan2_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 				(*fn->clk) (FALSE, TRUE, cookie);
 				CONFIG_FPGA_DELAY ();
 				/* Write data */
-				(*fn->wr) ((val < 0), TRUE, cookie);
+				(*fn->wr) ((val & 0x80), TRUE, cookie);
 				CONFIG_FPGA_DELAY ();
 				/* Assert the clock */
 				(*fn->clk) (TRUE, TRUE, cookie);

@@ -561,6 +561,13 @@ static int Spartan2_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 		}
 		putc ('\n');			/* terminate the dotted line */
 
+		/*
+		 * Run the post configuration function if there is one.
+		 */
+		if (*fn->post) {
+			(*fn->post) (cookie);
+		}
+
 #ifdef CFG_FPGA_PROG_FEEDBACK
 		if (ret_val == FPGA_SUCCESS) {
 			puts ("Done.\n");

@@ -615,8 +622,10 @@ static int Spartan2_ss_reloc (Xilinx_desc * desc, ulong reloc_offset)
 			PRINTF ("%s: Relocating descriptor at 0x%p\n", __FUNCTION__,
 					desc);
 
-			addr = (ulong) (fn->pre) + reloc_offset;
-			fn_r->pre = (Xilinx_pre_fn) addr;
+			if (fn->pre) {
+				addr = (ulong) (fn->pre) + reloc_offset;
+				fn_r->pre = (Xilinx_pre_fn) addr;
+			}
 
 			addr = (ulong) (fn->pgm) + reloc_offset;
 			fn_r->pgm = (Xilinx_pgm_fn) addr;

@@ -633,6 +642,11 @@ static int Spartan2_ss_reloc (Xilinx_desc * desc, ulong reloc_offset)
 			addr = (ulong) (fn->wr) + reloc_offset;
 			fn_r->wr = (Xilinx_wr_fn) addr;
 
+			if (fn->post) {
+				addr = (ulong) (fn->post) + reloc_offset;
+				fn_r->post = (Xilinx_post_fn) addr;
+			}
+
 			fn_r->relocated = TRUE;
 
 		} else {
common/spartan3.c +20 -5
index c0f2b05..4fe3e89 100644
@@ -30,7 +30,7 @@
 #include <common.h>		/* core U-Boot definitions */
 #include <spartan3.h>		/* Spartan-II device family */
 
-#if (CONFIG_FPGA & (CFG_XILINX | CFG_SPARTAN3))
+#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_SPARTAN3)
 
 /* Define FPGA_DEBUG to get debug printf's */
 #ifdef	FPGA_DEBUG

@@ -446,7 +446,7 @@ static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 	int ret_val = FPGA_FAIL;	/* assume the worst */
 	Xilinx_Spartan3_Slave_Serial_fns *fn = desc->iface_fns;
 	int i;
-	char  val;
+	unsigned char val;
 
 	PRINTF ("%s: start with interface functions @ 0x%p\n",
 			__FUNCTION__, fn);

@@ -514,6 +514,7 @@ static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 				puts ("** CRC error during FPGA load.\n");
 				return (FPGA_FAIL);
 			}
+
 			val = data [bytecount ++];
 			i = 8;
 			do {

@@ -521,7 +522,7 @@ static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 				(*fn->clk) (FALSE, TRUE, cookie);
 				CONFIG_FPGA_DELAY ();
 				/* Write data */
-				(*fn->wr) ((val < 0), TRUE, cookie);
+				(*fn->wr) ((val & 0x80), TRUE, cookie);
 				CONFIG_FPGA_DELAY ();
 				/* Assert the clock */
 				(*fn->clk) (TRUE, TRUE, cookie);

@@ -566,6 +567,13 @@ static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 		}
 		putc ('\n');			/* terminate the dotted line */
 
+		/*
+		 * Run the post configuration function if there is one.
+		 */
+		if (*fn->post) {
+			(*fn->post) (cookie);
+		}
+
 #ifdef CFG_FPGA_PROG_FEEDBACK
 		if (ret_val == FPGA_SUCCESS) {
 			puts ("Done.\n");

@@ -620,8 +628,10 @@ static int Spartan3_ss_reloc (Xilinx_desc * desc, ulong reloc_offset)
 			PRINTF ("%s: Relocating descriptor at 0x%p\n", __FUNCTION__,
 					desc);
 
-			addr = (ulong) (fn->pre) + reloc_offset;
-			fn_r->pre = (Xilinx_pre_fn) addr;
+			if (fn->pre) {
+				addr = (ulong) (fn->pre) + reloc_offset;
+				fn_r->pre = (Xilinx_pre_fn) addr;
+			}
 
 			addr = (ulong) (fn->pgm) + reloc_offset;
 			fn_r->pgm = (Xilinx_pgm_fn) addr;

@@ -638,6 +648,11 @@ static int Spartan3_ss_reloc (Xilinx_desc * desc, ulong reloc_offset)
 			addr = (ulong) (fn->wr) + reloc_offset;
 			fn_r->wr = (Xilinx_wr_fn) addr;
 
+			if (fn->post) {
+				addr = (ulong) (fn->post) + reloc_offset;
+				fn_r->post = (Xilinx_post_fn) addr;
+			}
+
 			fn_r->relocated = TRUE;
 
 		} else {
common/virtex2.c +1 -1
index b5dc366..1283ff6 100644
@@ -31,7 +31,7 @@
 #include <common.h>
 #include <virtex2.h>
 
-#if (CONFIG_FPGA & (CFG_XILINX | CFG_VIRTEX2))
+#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_VIRTEX2)
 
 #if 0
 #define FPGA_DEBUG
common/xilinx.c +14 -14
index e03e78c..2a7e9f4 100644
@@ -32,7 +32,7 @@
 #include <spartan2.h>
 #include <spartan3.h>
 
-#if (CONFIG_FPGA & CFG_FPGA_XILINX)
+#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX)
 
 #if 0
 #define FPGA_DEBUG

@@ -59,7 +59,7 @@ int xilinx_load (Xilinx_desc * desc, void *buf, size_t bsize)
 	} else
 		switch (desc->family) {
 		case Xilinx_Spartan2:
-#if (CONFIG_FPGA & CFG_SPARTAN2)
+#ifdef CONFIG_FPGA_SPARTAN2
 			PRINTF ("%s: Launching the Spartan-II Loader...\n",
 					__FUNCTION__);
 			ret_val = Spartan2_load (desc, buf, bsize);

@@ -69,7 +69,7 @@ int xilinx_load (Xilinx_desc * desc, void *buf, size_t bsize)
 #endif
 			break;
 		case Xilinx_Spartan3:
-#if (CONFIG_FPGA & CFG_SPARTAN3)
+#ifdef CONFIG_FPGA_SPARTAN3
 			PRINTF ("%s: Launching the Spartan-III Loader...\n",
 					__FUNCTION__);
 			ret_val = Spartan3_load (desc, buf, bsize);

@@ -79,7 +79,7 @@ int xilinx_load (Xilinx_desc * desc, void *buf, size_t bsize)
 #endif
 			break;
 		case Xilinx_Virtex2:
-#if (CONFIG_FPGA & CFG_VIRTEX2)
+#ifdef CONFIG_FPGA_VIRTEX2
 			PRINTF ("%s: Launching the Virtex-II Loader...\n",
 					__FUNCTION__);
 			ret_val = Virtex2_load (desc, buf, bsize);

@@ -106,7 +106,7 @@ int xilinx_dump (Xilinx_desc * desc, void *buf, size_t bsize)
 	} else
 		switch (desc->family) {
 		case Xilinx_Spartan2:
-#if (CONFIG_FPGA & CFG_SPARTAN2)
+#ifdef CONFIG_FPGA_SPARTAN2
 			PRINTF ("%s: Launching the Spartan-II Reader...\n",
 					__FUNCTION__);
 			ret_val = Spartan2_dump (desc, buf, bsize);

@@ -116,7 +116,7 @@ int xilinx_dump (Xilinx_desc * desc, void *buf, size_t bsize)
 #endif
 			break;
 		case Xilinx_Spartan3:
-#if (CONFIG_FPGA & CFG_SPARTAN3)
+#ifdef CONFIG_FPGA_SPARTAN3
 			PRINTF ("%s: Launching the Spartan-III Reader...\n",
 					__FUNCTION__);
 			ret_val = Spartan3_dump (desc, buf, bsize);

@@ -126,7 +126,7 @@ int xilinx_dump (Xilinx_desc * desc, void *buf, size_t bsize)
 #endif
 			break;
 		case Xilinx_Virtex2:
-#if (CONFIG_FPGA & CFG_VIRTEX2)
+#ifdef CONFIG_FPGA_VIRTEX2
 			PRINTF ("%s: Launching the Virtex-II Reader...\n",
 					__FUNCTION__);
 			ret_val = Virtex2_dump (desc, buf, bsize);

@@ -198,7 +198,7 @@ int xilinx_info (Xilinx_desc * desc)
 			printf ("Device Function Table @ 0x%p\n", desc->iface_fns);
 			switch (desc->family) {
 			case Xilinx_Spartan2:
-#if (CONFIG_FPGA & CFG_SPARTAN2)
+#ifdef CONFIG_FPGA_SPARTAN2
 				Spartan2_info (desc);
 #else
 				/* just in case */

@@ -207,7 +207,7 @@ int xilinx_info (Xilinx_desc * desc)
 #endif
 				break;
 			case Xilinx_Spartan3:
-#if (CONFIG_FPGA & CFG_SPARTAN3)
+#ifdef CONFIG_FPGA_SPARTAN3
 				Spartan3_info (desc);
 #else
 				/* just in case */

@@ -216,7 +216,7 @@ int xilinx_info (Xilinx_desc * desc)
 #endif
 				break;
 			case Xilinx_Virtex2:
-#if (CONFIG_FPGA & CFG_VIRTEX2)
+#ifdef CONFIG_FPGA_VIRTEX2
 				Virtex2_info (desc);
 #else
 				/* just in case */

@@ -249,7 +249,7 @@ int xilinx_reloc (Xilinx_desc * desc, ulong reloc_offset)
 	} else
 		switch (desc->family) {
 		case Xilinx_Spartan2:
-#if (CONFIG_FPGA & CFG_SPARTAN2)
+#ifdef CONFIG_FPGA_SPARTAN2
 			ret_val = Spartan2_reloc (desc, reloc_offset);
 #else
 			printf ("%s: No support for Spartan-II devices.\n",

@@ -257,7 +257,7 @@ int xilinx_reloc (Xilinx_desc * desc, ulong reloc_offset)
 #endif
 			break;
 		case Xilinx_Spartan3:
-#if (CONFIG_FPGA & CFG_SPARTAN3)
+#ifdef CONFIG_FPGA_SPARTAN3
 			ret_val = Spartan3_reloc (desc, reloc_offset);
 #else
 			printf ("%s: No support for Spartan-III devices.\n",

@@ -265,7 +265,7 @@ int xilinx_reloc (Xilinx_desc * desc, ulong reloc_offset)
 #endif
 			break;
 		case Xilinx_Virtex2:
-#if (CONFIG_FPGA & CFG_VIRTEX2)
+#ifdef CONFIG_FPGA_VIRTEX2
 			ret_val = Virtex2_reloc (desc, reloc_offset);
 #else
 			printf ("%s: No support for Virtex-II devices.\n",

@@ -308,4 +308,4 @@ static int xilinx_validate (Xilinx_desc * desc, char *fn)
 	return ret_val;
 }
 
-#endif							/* CONFIG_FPGA & CFG_FPGA_XILINX */
+#endif	/* CONFIG_FPGA && CONFIG_FPGA_XILINX */
include/configs/GEN860T.h +3 -1
index bfbf3a8..3eb3131 100644
@@ -273,7 +273,9 @@
  * Virtex2 FPGA configuration support
  */
 #define CONFIG_FPGA_COUNT		1
-#define CONFIG_FPGA				CFG_XILINX_VIRTEX2
+#define CONFIG_FPGA
+#define CONFIG_FPGA_XILINX
+#define CONFIG_FPGA_VIRTEX2
 #define CFG_FPGA_PROG_FEEDBACK
include/configs/M54455EVB.h +3 -1
index 6f4859c..487d2fc 100644
@@ -190,7 +190,9 @@
 
 /* FPGA - Spartan 2 */
 /* experiment
-#define CONFIG_FPGA		CFG_SPARTAN3
+#define CONFIG_FPGA
+#define CONFIG_FPGA_XILINX
+#define CONFIG_FPGA_SPARTAN3
 #define CONFIG_FPGA_COUNT	1
 #define CFG_FPGA_PROG_FEEDBACK
 #define CFG_FPGA_CHECK_CTRLC
include/configs/alpr.h +4 -1
index aff9823..bd32f6f 100644
@@ -296,7 +296,10 @@
 /*-----------------------------------------------------------------------
  * FPGA stuff
  *-----------------------------------------------------------------------*/
-#define CONFIG_FPGA             CFG_ALTERA_CYCLON2
+#define CONFIG_FPGA
+#define CONFIG_FPGA_ALTERA
+#define CONFIG_FPGA_CYCLON2
+
 #define CFG_FPGA_CHECK_CTRLC
 #define CFG_FPGA_PROG_FEEDBACK
 #define CONFIG_FPGA_COUNT       1		/* Ich habe 2 ... aber in
include/spartan2.h +5 -0
index d2e81e3..bd159e1 100644
@@ -58,6 +58,7 @@ typedef struct {
 	Xilinx_init_fn	init;
 	Xilinx_done_fn	done;
 	Xilinx_wr_fn	wr;
+	Xilinx_post_fn	post;
 	int           	relocated;
 } Xilinx_Spartan2_Slave_Serial_fns;
 

@@ -69,6 +70,7 @@ typedef struct {
 #define XILINX_XC2S50_SIZE  	559232/8
 #define XILINX_XC2S100_SIZE 	781248/8
 #define XILINX_XC2S150_SIZE 	1040128/8
+#define XILINX_XC2S200_SIZE 	1335872/8
 
 /* Spartan-IIE (1.8V) */
 #define XILINX_XC2S50E_SIZE     630048/8

@@ -95,6 +97,9 @@ typedef struct {
 #define XILINX_XC2S150_DESC(iface, fn_table, cookie) \
 { Xilinx_Spartan2, iface, XILINX_XC2S150_SIZE, fn_table, cookie }
 
+#define XILINX_XC2S200_DESC(iface, fn_table, cookie) \
+{ Xilinx_Spartan2, iface, XILINX_XC2S200_SIZE, fn_table, cookie }
+
 #define XILINX_XC2S50E_DESC(iface, fn_table, cookie) \
 { Xilinx_Spartan2, iface, XILINX_XC2S50E_SIZE, fn_table, cookie }
include/spartan3.h +10 -1
index 65a3f5a..c203eeb 100644
@@ -58,6 +58,7 @@ typedef struct {
 	Xilinx_init_fn	init;
 	Xilinx_done_fn	done;
 	Xilinx_wr_fn	wr;
+	Xilinx_post_fn	post;
 	int           	relocated;
 } Xilinx_Spartan3_Slave_Serial_fns;
 

@@ -80,9 +81,12 @@ typedef struct {
 #define	XILINX_XC3S1200E_SIZE	3841184/8
 #define	XILINX_XC3S1600E_SIZE	5969696/8
 
+/* Spartan-IIIE (1.2V) */
+#define XILINX_XC3S1200E_SIZE  	3841184/8
+
 /* Descriptor Macros
  *********************************************************************/
-/* Spartan-II devices */
+/* Spartan-III devices */
 #define XILINX_XC3S50_DESC(iface, fn_table, cookie) \
 { Xilinx_Spartan3, iface, XILINX_XC3S50_SIZE, fn_table, cookie }
 

@@ -124,4 +128,9 @@ typedef struct {
 #define XILINX_XC3S1600E_DESC(iface, fn_table, cookie) \
 { Xilinx_Spartan3, iface, XILINX_XC3S1600E_SIZE, fn_table, cookie }
 
+
+/* Spartan-IIIE devices */
+#define XILINX_XC3S1200E_DESC(iface, fn_table, cookie) \
+{ Xilinx_Spartan3, iface, XILINX_XC3S1200E_SIZE, fn_table, cookie }
+
 #endif /* _SPARTAN3_H_ */
include/xilinx.h +2 -2
index 3704e1d..95ebe3d 100644
@@ -31,11 +31,11 @@
  *********************************************************************/
 #define CFG_SPARTAN2 			CFG_FPGA_DEV( 0x1 )
 #define CFG_VIRTEX_E 			CFG_FPGA_DEV( 0x2 )
-#define CFG_VIRTEX2	 			CFG_FPGA_DEV( 0x4 )
+#define CFG_VIRTEX2			CFG_FPGA_DEV( 0x4 )
 #define CFG_SPARTAN3 			CFG_FPGA_DEV( 0x8 )
 #define CFG_XILINX_SPARTAN2 	(CFG_FPGA_XILINX | CFG_SPARTAN2)
 #define CFG_XILINX_VIRTEX_E 	(CFG_FPGA_XILINX | CFG_VIRTEX_E)
-#define CFG_XILINX_VIRTEX2	 	(CFG_FPGA_XILINX | CFG_VIRTEX2)
+#define CFG_XILINX_VIRTEX2	(CFG_FPGA_XILINX | CFG_VIRTEX2)
 #define CFG_XILINX_SPARTAN3 	(CFG_FPGA_XILINX | CFG_SPARTAN3)
 /* XXX - Add new models here */
#1 lore raw
Quoted 10 lines · Matthias Fuchs

Hi,

we had some discussions about the FPGA subsystem some days (and also a couple of months) before on this list. I have also some local improvements for the FPGA subsystem.

Before posting a couple of clean patches I dare to post a single patch just to get an ok for the different topics. When all points are clear, I will split my patch into one patch per per topic.

Quoted 6 lines

This is what I like to get into U-Boot:

1) Replace the CONFIG_FPGA bit mask by U-Boot like configuration options:

CONFIG_FPGA
- enable FPGA subsystem

This should be automatically selected by CONFIG_FPGA_<vendor>; for now it's probably okay to have it done manually in the board config file; but the move to Kconfig will allow for easy dependency checking.

Quoted 4 lines

CONFIG_FPGA_<vendor>
- enable support for specific chip vendors (ALTERA, XILINX)

Similarly, this should be automatically selected by CONFIG_FPGA_<family>

Quoted 19 lines

CONFIG_FPGA_<family>
- enable support for FPGA family
(SPARTAN2, SPARTAN3, VIRTEX2, CYCLONE2, ACEX1K, ACEX)

This means when you have an Xilinx Spartan3 FPGA on your board you typically have these lines in your board configuration:

#define CONFIG_FPGA
#define CONFIG_FPGA_XILINX
#define CONFIG_FPGA_SPARTAN3

(TODO: add this to the README file)

2) Fix FPGA support for some boards that will get broken through the above change (GEN860T, ...)

3) Remove bit swapping in fpga_loadbitstream().

Quoted 8 lines

4) Use AND-operation for checking MSB of bytes instead of sign check for Spartan2/3 FPGAs in slave serial code.

5) Add post() and pre() callback for Spartan2/3 FPGAs inslave serial mode.

6) Add some new FPGA types.

Matthias

Sounds good to me.

board/gen860t/fpga.c line 37

Quoted 12 lines
board/gen860t/fpga.c +1 -1
index 2ba7e0e..5997584 100644
@@ -34,7 +34,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#if (CONFIG_FPGA)
+#ifdef CONFIG_FPGA

#if defined(CONFIG_FPGA) is preferred.

Otherwise, this looks like the right direction to me.

Cheers,
g.

Signature · 4 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195
#2 lore raw
In reply to Matthias Fuchs

Hi,

Quoted 9 lines · Matthias Fuchs

Hi,

we had some discussions about the FPGA subsystem some days (and also a couple of months) before on this list. I have also some local improvements for the FPGA subsystem.

Before posting a couple of clean patches I dare to post a single patch just to get an ok for the different topics. When all points are clear, I will split my patch into one patch per per topic.

I have one wish on my list:

Would it be possible to have an optional "block write" function for the FPGA?

While I appreciate the current approach of single bit functions for the FPGA to be very convenient for board bring-up, it is somewhat slow on the larger FPGAs (with something like 1.5 MByte bitstream size).

An additional block write function that - if present - replaces the internal (generic) programming algorithm would give a clean way to use the existing FPGA infrastructure (commands, image handling, pre- and post-configuration) and switch to a fast load for production use.

Only then, you could take advantage of buses like SPI for FPGA load, too.

(I hacked something like this in my local u-boot, and the speed-up for FPGA loading was significant, at least 3 times. And I still did not implement the SPI path because I did not have the time yet...)

Best regards,
Wolfgang

author
#3 lore raw

Hi,

Quoted 1 line · w.wegner at astro-kom.de

I have one wish on my list:

... you haven't seen my wishlist :-)

Quoted 2 lines

Would it be possible to have an optional "block write" function for the FPGA?

yes.

Quoted 11 lines

While I appreciate the current approach of single bit functions for the FPGA to be very convenient for board bring-up, it is somewhat slow on the larger FPGAs (with something like 1.5 MByte bitstream size).

An additional block write function that - if present - replaces the internal (generic) programming algorithm would give a clean way to use the existing FPGA infrastructure (commands, image handling, pre- and post-configuration) and switch to a fast load for production use.

Only then, you could take advantage of buses like SPI for FPGA load, too.

I was in a similar situation some time before. Our PMC440 board (patches have been posted yesterday) uses a Spartan3E FPGA that is programmed in slave serial mode. I started with download time about 13 seconds (!!!). The main reason is the extensive use of callbacks by the FPGA subsystem. Then updated the boot code to use U-Boot's slave parallel implementation to boot the FPGA still in slave serial mode. My write-data-byte function just shifts out the byte bit by bit. This improved the download time to about 3 seconds. The last step was to enable the cache for the 440 CPU. This speeds things up to an acceptable level.

But I agree, a block write function would be cleaner.

Quoted 4 lines

(I hacked something like this in my local u-boot, and the speed-up for FPGA loading was significant, at least 3 times. And I still did not implement the SPI path because I did not have the time yet...)

If you already have something it might be a good idea to share your work with us. I think this is independent from what my patch does at the moment.

Matthias

#4 lore raw

Hi,

Quoted 2 lines · Matthias Fuchs
Quoted 1 line

I have one wish on my list:

... you haven't seen my wishlist :-)

:-))

Quoted 8 lines

I was in a similar situation some time before. Our PMC440 board (patches have been posted yesterday) uses a Spartan3E FPGA that is programmed in slave serial mode. I started with download time about 13 seconds (!!!). The main reason is the extensive use of callbacks by the FPGA subsystem. Then updated the boot code to use U-Boot's slave parallel implementation to boot the FPGA still in slave serial mode. My write-data-byte function just shifts out the byte bit by bit. This improved the download time to about 3 seconds. The last step was to enable the cache for the 440 CPU. This speeds things up to an acceptable level.

I currently have around 3 seconds for a partially filled XC3S4000 on my MCF5373L (240 MHz) with cache enabled. This is almost below my pain limit, which is why I did not yet take the effort to debug the SPI load any further.

Quoted 2 lines

If you already have something it might be a good idea to share your work with us. I think this is independent from what my patch does at the moment.

The usual problem: it is a bit difficult to extract only this as a clean patch...

I will see what I can do, but it can take some days.

Quoted 1 line

Matthias

Regards,
Wolfgang

#5 lore raw
In reply to Matthias Fuchs
Quoted 5 lines · Matthias Fuchs

Hi,

we had some discussions about the FPGA subsystem some days (and also a couple of months) before on this list. I have also some local improvements for the FPGA subsystem.

[snip]

Quoted 1 line

Matthias

Hi Matthias,

[massive snip]

Quoted 14 lines
common/spartan3.c +1 -1
index c0f2b05..4fe3e89 100644
@@ -30,7 +30,7 @@
 #include <common.h>		/* core U-Boot definitions */
 #include <spartan3.h>		/* Spartan-II device family */
 
-#if (CONFIG_FPGA & (CFG_XILINX | CFG_SPARTAN3))
+#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_SPARTAN3)
 
 /* Define FPGA_DEBUG to get debug printf's */
 #ifdef	FPGA_DEBUG

common/spartan3.c static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize) · line 449

Quoted 5 lines
common/spartan3.c +1 -1
@@ -446,7 +446,7 @@ static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 	int ret_val = FPGA_FAIL;	/* assume the worst */
 	Xilinx_Spartan3_Slave_Serial_fns *fn = desc->iface_fns;
 	int i;
-	char  val;
+	unsigned char val;

Trivia: This change should not be necessary since you fixed the conditional (below) to do a bit-wise & 0x80 rather than a signed compare that has portability and aesthetic problems.

Quoted 11 lines
common/spartan3.c +1 -0
 	PRINTF ("%s: start with interface functions @ 0x%p\n",
 			__FUNCTION__, fn);

@@ -514,6 +514,7 @@ static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 				puts ("** CRC error during FPGA load.\n");
 				return (FPGA_FAIL);
 			}
+
 			val = data [bytecount ++];
 			i = 8;
 			do {

common/spartan3.c static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize) · line 525

Quoted 5 lines
common/spartan3.c +1 -1
@@ -521,7 +522,7 @@ static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 				(*fn->clk) (FALSE, TRUE, cookie);
 				CONFIG_FPGA_DELAY ();
 				/* Write data */
-				(*fn->wr) ((val < 0), TRUE, cookie);
+				(*fn->wr) ((val & 0x80), TRUE, cookie);

Thank you, that makes my aesthetics radar much happier! :-) Now it should be immaterial whether val is signed or unsigned.

Painting the bike shed blue,
gvb

[snip]

Ref:
<http://www.freebsd.org/doc/en_US.ISO8859-1/books/faq/misc.html#BIKESHED-PAINTING>

author
#6 lore raw

Hi Jerry,

Quoted 6 lines · Jerry Van Baren

Review

Quoted 2 lines
(quoted diff) +1 -1
-	char  val;
+	unsigned char val;

Trivia: This change should not be necessary since you fixed the conditional (below) to do a bit-wise & 0x80 rather than a signed compare that has portability and aesthetic problems.

yes, of course.

Quoted 1 line
Quoted 1 line

val = data [bytecount ++];

But since the right value is an unsigned char, I like the left value to be of the same type.

Quoted 1 line

Painting the bike shed blue,

We should ask the others that are involved ins discussion if blue might bring disadvantages :-)

Matthias

#7 lore raw
Quoted 11 lines · Matthias Fuchs

Hi Jerry,

Quoted 5 lines · Jerry Van Baren

Review

Quoted 2 lines
(quoted diff) +1 -1
-	char  val;
+	unsigned char val;

Trivia: This change should not be necessary since you fixed the conditional (below) to do a bit-wise & 0x80 rather than a signed compare that has portability and aesthetic problems.

yes, of course.

Quoted 1 line
Quoted 1 line

val = data [bytecount ++];

Quoted 8 lines

But since the right value is an unsigned char, I like the left value to be of the same type.

Quoted 1 line

Painting the bike shed blue,

We should ask the others that are involved ins discussion if blue might bring disadvantages :-)

Matthias

Oops, yes, you are right. My jaundiced eye read *signed* char instead of unsigned.

Sorry for the noise,
gvb

(feeling blue...)

#8 lore raw
In reply to Matthias Fuchs

Hi,

sorry for the delay... I finally cleaned up my xilinx spartan3 code, and here is a proposal for an optional block write (originally called fast write).

The observation is that using callbacks for every single data/clock line change takes very long, doing this in a monolithic function is much faster; additionally, you could take advantage of some hardware (e.g. SPI, although I did not get this to work yet) to do the actual load.

Best regards,
Wolfgang

PS: sorry I can not provide a better patch at the moment, git-diff without any arguments prints out an empty diff for any file present in the tree, and I do not know what I did to cause this.

common/spartan3.c +36 -27
old mode 100644
new mode 100755
index f7c4f8c..a386006
@@ -40,7 +40,7 @@
 #endif
 
 #undef CFG_FPGA_CHECK_BUSY
-#undef CFG_FPGA_PROG_FEEDBACK
+// #undef CFG_FPGA_PROG_FEEDBACK
 
 /* Note: The assumption is that we cannot possibly run fast enough to
  * overrun the device (the Slave Parallel mode can free run at 50MHz).

@@ -505,35 +505,39 @@ static int Spartan3_ss_load (Xilinx_desc * desc, void *buf, size_t bsize)
 			}
 		} while ((*fn->init) (cookie));
 
-		/* Load the data */
-		while (bytecount < bsize) {
-
-			/* Xilinx detects an error if INIT goes low (active)
-			   while DONE is low (inactive) */
-			if ((*fn->done) (cookie) == 0 && (*fn->init) (cookie)) {
-				puts ("** CRC error during FPGA load.\n");
-				return (FPGA_FAIL);
-			}
-			val = data [bytecount ++];
-			i = 8;
-			do {
-				/* Deassert the clock */
-				(*fn->clk) (FALSE, TRUE, cookie);
-				CONFIG_FPGA_DELAY ();
-				/* Write data */
-				(*fn->wr) ((val & 0x80), TRUE, cookie);
-				CONFIG_FPGA_DELAY ();
-				/* Assert the clock */
-				(*fn->clk) (TRUE, TRUE, cookie);
-				CONFIG_FPGA_DELAY ();
-				val <<= 1;
-				i --;
-			} while (i > 0);
+		if(*fn->fwr)
+			(*fn->fwr) (data, bsize, TRUE, cookie);
+		else {
+			/* Load the data */
+			while (bytecount < bsize) {
+
+				/* Xilinx detects an error if INIT goes low (active)
+				   while DONE is low (inactive) */
+				if ((*fn->done) (cookie) == 0 && (*fn->init) (cookie)) {
+					puts ("** CRC error during FPGA load.\n");
+					return (FPGA_FAIL);
+				}
+				val = data [bytecount ++];
+				i = 8;
+				do {
+					/* Deassert the clock */
+					(*fn->clk) (FALSE, TRUE, cookie);
+					CONFIG_FPGA_DELAY ();
+					/* Write data */
+					(*fn->wr) ((val & 0x80), TRUE, cookie);
+					CONFIG_FPGA_DELAY ();
+					/* Assert the clock */
+					(*fn->clk) (TRUE, TRUE, cookie);
+					CONFIG_FPGA_DELAY ();
+					val <<= 1;
+					i --;
+				} while (i > 0);
 
 #ifdef CFG_FPGA_PROG_FEEDBACK
-			if (bytecount % (bsize / 40) == 0)
-				putc ('.');		/* let them know we are alive */
+				if (bytecount % (bsize / 40) == 0)
+					putc ('.');		/* let them know we are alive */
 #endif
+			}
 		}
 
 		CONFIG_FPGA_DELAY ();

@@ -638,6 +642,11 @@ static int Spartan3_ss_reloc (Xilinx_desc * desc, ulong reloc_offset)
 			addr = (ulong) (fn->wr) + reloc_offset;
 			fn_r->wr = (Xilinx_wr_fn) addr;
 
+			if(*fn->fwr) {
+				addr = (ulong) (fn->fwr) + reloc_offset;
+				fn_r->fwr = (Xilinx_fastwr_fn) addr;
+			}
+
 			fn_r->relocated = TRUE;
 
 		} else {
include/spartan3.h +2 -0
old mode 100644
new mode 100755
index 65a3f5a..11735e7
@@ -58,6 +58,8 @@ typedef struct {
 	Xilinx_init_fn	init;
 	Xilinx_done_fn	done;
 	Xilinx_wr_fn	wr;
+// for block write (fast_write):
+	Xilinx_fastwr_fn fwr;
 	int           	relocated;
 } Xilinx_Spartan3_Slave_Serial_fns;
author
#9 lore raw

Hi Wolfgang,

Quoted 7 lines · you

Hi,

The observation is that using callbacks for every single data/clock line change takes very long, doing this in a monolithic function is much faster; additionally, you could take advantage of some hardware (e.g. SPI, although I did not get this to work yet) to do the actual load.

The new fwr callback makes sense to me. I agree that using the callbacks for every signal change might be very slow. I used the the slave parallel code to load an fpga in slave serial mode some time before. This reduces the callbacks a lot and booting was about 4 times faster. I used the byte write callback to shift out 8 bits in this case.

I hope that my FPGA patches will get it into U-Boot when the next merge windows opens. Perhaps you can provide a patch on top of that afterwards.

Matthias

#10 lore raw

Hi Matthias,

Quoted 8 lines · Matthias Fuchs

The new fwr callback makes sense to me. I agree that using the callbacks for every signal change might be very slow. I used the the slave parallel code to load an fpga in slave serial mode some time before. This reduces the callbacks a lot and booting was about 4 times faster. I used the byte write callback to shift out 8 bits in this case.

I hope that my FPGA patches will get it into U-Boot when the next merge windows opens. Perhaps you can provide a patch on top of that afterwards.

I will try to, as meanwhile my U-Boot environment settled down a bit, this should not be too much work.

Would such a "selective patch" like the last one be OK, then I do not have to investigate why I get all the empty diff lines from the new tree...?

Quoted 1 line

Matthias

Regards,
Wolfgang

author
#11 lore raw

Hi Wolfgang,

Quoted 10 lines · w.wegner at astro-kom.de

Hi Matthias,

Quoted 6 lines · Matthias Fuchs

The new fwr callback makes sense to me. I agree that using the callbacks for every signal change might be very slow. I used the the slave parallel code to load an fpga in slave serial mode some time before. This reduces the callbacks a lot and booting was about 4 times faster. I used the byte write callback to shift out 8 bits in this case.

Quoted 8 lines
Quoted 2 lines

I hope that my FPGA patches will get it into U-Boot when the next merge windows opens. Perhaps you can provide a patch on top of that afterwards.

I will try to, as meanwhile my U-Boot environment settled down a bit, this should not be too much work.

Would such a "selective patch" like the last one be OK, then I do not have to investigate why I get all the empty diff lines from the new tree...?

I do not have to apply your patch. But I expect that wd will only except clean 'splipping' patches.

So it would be better to fix up your tree so that you can generate a patch as it is expected. If nothing helps, check out a clean U-boot tree, modify it and try it again. It should be easier to fix your toolchain (we the help of others) than to get unclean patches accepted.

Matthias

#12 lore raw
In reply to Matthias Fuchs

Matthias,

Matthias Fuchs <[email protected]> wrote on 11/11/2007 08:45:02 AM:

< snip >

Quoted 1 line

5) Add post() and pre() callback for Spartan2/3 FPGAs inslave serial

mode.

Only comment I have is to point out that one of the things we talked about last summer. The Spartan 3 code in particular (and I think all of the Xilinx/Altera code in general) makes the pre()/post() function calls optional. However, the relocation code doesn't check to see if the pre()/post() functions exist and therefore incorrectly "relocates" them even if they don't exist. This causes problems later because they now appear to exist since they no longer have a NULL address. One of the things we talked about was makeing the FPGA relocation code smart enough to detect NULL addresses for these functions and not relocate them.

Now it may not be an issue anymore. I made the changes to the code, but when stepping through 1.3.0-rc3 of the code, I noticed that gd->reloc_off = 0 which ment that the relocation_offset passed to the FPGA relocation code was zero and therefore nothing actually got relocated. So a NULL pointer is still NULL when zero is added to it. Now, I'm not sure why gd->reloc_off = 0, it may be my setup, it may be I'm broken, it may be that it's no longer used for some reason I don't know about. But it seems to me that the FPGA relocation code should still deal with non-existant pre()/post() functions, just in case gd->reloc_off is not zero in someone else's circumstances.

My 2 cents worth and thanks for making these changes. I'm looking forward to them all.

Bruce

#13 lore raw
Quoted 20 lines · Bruce_Leonard at selinc.com

Matthias,

Matthias Fuchs <[email protected]> wrote on 11/11/2007 08:45:02 AM:

< snip >

Quoted 1 line

5) Add post() and pre() callback for Spartan2/3 FPGAs inslave serial

mode.

Only comment I have is to point out that one of the things we talked about last summer. The Spartan 3 code in particular (and I think all of the Xilinx/Altera code in general) makes the pre()/post() function calls optional. However, the relocation code doesn't check to see if the pre()/post() functions exist and therefore incorrectly "relocates" them even if they don't exist. This causes problems later because they now appear to exist since they no longer have a NULL address. One of the things we talked about was makeing the FPGA relocation code smart enough to detect NULL addresses for these functions and not relocate them.

Quoted 10 lines

Now it may not be an issue anymore. I made the changes to the code, but when stepping through 1.3.0-rc3 of the code, I noticed that gd->reloc_off = 0 which ment that the relocation_offset passed to the FPGA relocation code was zero and therefore nothing actually got relocated. So a NULL pointer is still NULL when zero is added to it. Now, I'm not sure why gd->reloc_off = 0, it may be my setup, it may be I'm broken, it may be that it's no longer used for some reason I don't know about. But it seems to me that the FPGA relocation code should still deal with non-existant pre()/post() functions, just in case gd->reloc_off is not zero in someone else's circumstances.

Manual relocation shouldn't be necessary *at all*. The fact that we have it is due to the u-boot C environment being broken. I've fixed it for many of the powerpc ports, which is why reloc_off is now zero for those boards. Unfortunately, the fix only works with some versions of GCC, so the fix might need to be temporarily backed out. :-(

Regardless, fixing the FPGA manual reloc code is trivial and should definitely be done.

Cheers,
g.

Signature · 4 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195
author
#14 lore raw
In reply to Bruce_Leonard at selinc.com

Hi,

Quoted 10 lines · Bruce_Leonard at selinc.com

Matthias,

Matthias Fuchs <[email protected]> wrote on 11/11/2007 08:45:02 AM:

< snip >

Quoted 1 line

5) Add post() and pre() callback for Spartan2/3 FPGAs inslave serial

mode.

Quoted 9 lines

Only comment I have is to point out that one of the things we talked about last summer. The Spartan 3 code in particular (and I think all of the Xilinx/Altera code in general) makes the pre()/post() function calls optional. However, the relocation code doesn't check to see if the pre()/post() functions exist and therefore incorrectly "relocates" them even if they don't exist. This causes problems later because they now appear to exist since they no longer have a NULL address. One of the things we talked about was makeing the FPGA relocation code smart enough to detect NULL addresses for these functions and not relocate them.

I forgot to mention this in my 'RFC' email. But if you take a look at my patches, you will see that this has been implemented. I must say that I only implemented it for the pre and post functions. It may be helpful for others as well but for pre and post it is most obvious for me.

Perhaps you guys can give a little ack reply to my five FPGA patches. I did not see any no-go comment on any of them, uuh.

Matthias

#15 lore raw
Quoted 2 lines

Perhaps you guys can give a little ack reply to my five FPGA patches. I

did

Quoted 3 lines

not see any no-go comment on any of them, uuh.

Matthias

Sorry, in my haste and stupidity I failed to read your entire original post and see that you had patches :(.

ACK everything with the following question:

-                rc = fpga_load(dev, swapdata, swapsize);
-                free(swapdata);
+                rc = fpga_load(dev, dataptr, swapsize);
                 return rc;

I see you're using the size pulled from the BIT file rather than the size passed into the parameter (which IMOHO is the right way to do it), but you left the name of the variable as 'swapsize' which isn't really relevant anymore since there's no swapping going on. I don't care since I know what's going on, but it might be cleaner for future generations to rename it to something more descriptive of what it really is now. Also, on a 'I REALLY don't care' note, how much work do you think it would be to remove the requirement of having a size on the command line for this operation? Since the 'size' parameter is never used, I'd like to see it gone. If it's too much work, I can do it sometime when I'm bored ;). Just a thought.

Thanks for the work on this, saved me from having to embarrass myself by getting patch after patch rejected 'cause I is stupid.

Bruce

author
#16 lore raw

Hi Bruce,

Quoted 18 lines · Bruce_Leonard at selinc.com
Quoted 2 lines

Perhaps you guys can give a little ack reply to my five FPGA patches. I

did

Quoted 3 lines

not see any no-go comment on any of them, uuh.

Matthias

Sorry, in my haste and stupidity I failed to read your entire original post and see that you had patches :(.

ACK everything with the following question:

common/cmd_fpga.c +1 -2
-                rc = fpga_load(dev, swapdata, swapsize);
-                free(swapdata);
+                rc = fpga_load(dev, dataptr, swapsize);
                 return rc;
Quoted 4 lines

I see you're using the size pulled from the BIT file rather than the size passed into the parameter (which IMOHO is the right way to do it), but you left the name of the variable as 'swapsize' which isn't really relevant anymore since there's no swapping going on. I don't care since I know

I put this on my list.

Quoted 7 lines

what's going on, but it might be cleaner for future generations to rename it to something more descriptive of what it really is now. Also, on a 'I REALLY don't care' note, how much work do you think it would be to remove the requirement of having a size on the command line for this operation? Since the 'size' parameter is never used, I'd like to see it gone. If it's too much work, I can do it sometime when I'm bored ;). Just a thought.

You are right. You do not need to pass the size parameter to the 'fpga loadb' command at all. Only 'fpga load' still needs it.

Matthias