[U-Boot-Users] RFC: Some improvements for the FPGA subsystem
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
diff --git a/board/gen860t/fpga.c b/board/gen860t/fpga.c index 2ba7e0e..5997584 100644 --- a/board/gen860t/fpga.c +++ b/board/gen860t/fpga.c @@ -34,7 +34,7 @@
DECLARE_GLOBAL_DATA_PTR;
-#if (CONFIG_FPGA) +#ifdef CONFIG_FPGA
#if 0 #define GEN860T_FPGA_DEBUG diff --git a/board/gen860t/gen860t.c b/board/gen860t/gen860t.c index d448f9f..70341f8 100644 --- a/board/gen860t/gen860t.c +++ b/board/gen860t/gen860t.c @@ -254,7 +254,7 @@ int misc_init_r (void) mii_init (); #endif
-#if (CONFIG_FPGA) +#ifdef CONFIG_FPGA gen860t_init_fpga (); #endif return 0; diff --git a/common/ACEX1K.c b/common/ACEX1K.c index 2a421e2..76dc166 100644 --- a/common/ACEX1K.c +++ b/common/ACEX1K.c @@ -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 */ diff --git a/common/altera.c b/common/altera.c index 06e8a95..eb874e4 100644 --- a/common/altera.c +++ b/common/altera.c @@ -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 */ diff --git a/common/cmd_fpga.c b/common/cmd_fpga.c index cce23ad..3813140 100644 --- a/common/cmd_fpga.c +++ b/common/cmd_fpga.c @@ -60,14 +60,11 @@ static int fpga_get_op (char *opstr); /* Convert bitstream data and load into the fpga */ int fpga_loadbitstream(unsigned long dev, char* fpgadata, size_t size) { -#if (CONFIG_FPGA & CFG_FPGA_XILINX) +#ifdef CONFIG_FPGA_XILINX unsigned int length; - unsigned char* swapdata; unsigned int swapsize; char buffer[80]; - unsigned char *ptr; unsigned char *dataptr; - unsigned char data; unsigned int i; int rc;
@@ -142,42 +139,10 @@ int fpga_loadbitstream(unsigned long dev, char* fpgadata, size_t size) ((unsigned int) *(dataptr+1) <<16) + ((unsigned int) *(dataptr+2) <<8 ) + ((unsigned int) *(dataptr+3) ) ; - dataptr+=4; + dataptr += 4; printf(" bytes in bitstream = %d\n", swapsize);
- /* check consistency of length obtained */ - if (swapsize >= size) { - printf("%s: Could not find right length of data in bitstream\n", - __FUNCTION__); - return FPGA_FAIL; - } - - /* allocate memory */ - swapdata = (unsigned char *)malloc(swapsize); - if (swapdata == NULL) { - printf("%s: Could not allocate %d bytes memory !\n", - __FUNCTION__, swapsize); - return FPGA_FAIL; - } - - /* read data into memory and swap bits */ - ptr = swapdata; - for (i = 0; i < swapsize; i++) { - data = 0x00; - data |= (*dataptr & 0x01) << 7; - data |= (*dataptr & 0x02) << 5; - data |= (*dataptr & 0x04) << 3; - data |= (*dataptr & 0x08) << 1; - data |= (*dataptr & 0x10) >> 1; - data |= (*dataptr & 0x20) >> 3; - data |= (*dataptr & 0x40) >> 5; - data |= (*dataptr & 0x80) >> 7; - *ptr++ = data; - dataptr++; - } - - rc = fpga_load(dev, swapdata, swapsize); - free(swapdata); + rc = fpga_load(dev, dataptr, swapsize); return rc; #else printf("Bitstream support only for Xilinx devices\n"); diff --git a/common/cyclon2.c b/common/cyclon2.c index dce13b5..06f5e8a 100644 --- a/common/cyclon2.c +++ b/common/cyclon2.c @@ -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 */ diff --git a/common/fpga.c b/common/fpga.c index 2eff239..e4073be 100644 --- a/common/fpga.c +++ b/common/fpga.c @@ -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" ); diff --git a/common/spartan2.c b/common/spartan2.c index 0fb23b6..6d1ed96 100644 --- a/common/spartan2.c +++ b/common/spartan2.c @@ -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 { diff --git a/common/spartan3.c b/common/spartan3.c index c0f2b05..4fe3e89 100644 --- a/common/spartan3.c +++ b/common/spartan3.c @@ -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 { diff --git a/common/virtex2.c b/common/virtex2.c index b5dc366..1283ff6 100644 --- a/common/virtex2.c +++ b/common/virtex2.c @@ -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 diff --git a/common/xilinx.c b/common/xilinx.c index e03e78c..2a7e9f4 100644 --- a/common/xilinx.c +++ b/common/xilinx.c @@ -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 */ diff --git a/include/configs/GEN860T.h b/include/configs/GEN860T.h index bfbf3a8..3eb3131 100644 --- a/include/configs/GEN860T.h +++ b/include/configs/GEN860T.h @@ -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
diff --git a/include/configs/M54455EVB.h b/include/configs/M54455EVB.h index 6f4859c..487d2fc 100644 --- a/include/configs/M54455EVB.h +++ b/include/configs/M54455EVB.h @@ -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 diff --git a/include/configs/alpr.h b/include/configs/alpr.h index aff9823..bd32f6f 100644 --- a/include/configs/alpr.h +++ b/include/configs/alpr.h @@ -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 diff --git a/include/spartan2.h b/include/spartan2.h index d2e81e3..bd159e1 100644 --- a/include/spartan2.h +++ b/include/spartan2.h @@ -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 }
diff --git a/include/spartan3.h b/include/spartan3.h index 65a3f5a..c203eeb 100644 --- a/include/spartan3.h +++ b/include/spartan3.h @@ -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_ */ diff --git a/include/xilinx.h b/include/xilinx.h index 3704e1d..95ebe3d 100644 --- a/include/xilinx.h +++ b/include/xilinx.h @@ -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 */
On 11/11/07, Matthias Fuchs matthias.fuchs@esd-electronics.com wrote:
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:
- 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.
CONFIG_FPGA_<vendor> - enable support for specific chip vendors (ALTERA, XILINX)
Similarly, this should be automatically selected by CONFIG_FPGA_<family>
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)
- Fix FPGA support for some boards that will get broken through the
above change (GEN860T, ...)
Remove bit swapping in fpga_loadbitstream().
Use AND-operation for checking MSB of bytes instead of sign check for
Spartan2/3 FPGAs in slave serial code.
Add post() and pre() callback for Spartan2/3 FPGAs inslave serial mode.
Add some new FPGA types.
Matthias
Sounds good to me.
diff --git a/board/gen860t/fpga.c b/board/gen860t/fpga.c index 2ba7e0e..5997584 100644 --- a/board/gen860t/fpga.c +++ b/board/gen860t/fpga.c @@ -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.
Hi,
On 11 Nov 2007 at 17:45, Matthias Fuchs wrote:
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
Hi,
On Monday 12 November 2007 10:15, w.wegner@astro-kom.de wrote:
I have one wish on my list:
... you haven't seen my wishlist :-)
Would it be possible to have an optional "block write" function for the FPGA?
yes.
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.
(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
Hi,
On 12 Nov 2007 at 10:45, Matthias Fuchs wrote:
I have one wish on my list:
... you haven't seen my wishlist :-)
:-))
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.
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.
Matthias
Regards, Wolfgang
Matthias Fuchs wrote:
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]
Matthias
Hi Matthias,
[massive snip]
diff --git a/common/spartan3.c b/common/spartan3.c index c0f2b05..4fe3e89 100644 --- a/common/spartan3.c +++ b/common/spartan3.c @@ -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;
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.
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);
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
Hi Jerry,
On Monday 12 November 2007 14:24, Jerry Van Baren wrote:
- 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.
val = data [bytecount ++];
But since the right value is an unsigned char, I like the left value to be of the same type.
Painting the bike shed blue,
We should ask the others that are involved ins discussion if blue might bring disadvantages :-)
Matthias
Matthias Fuchs wrote:
Hi Jerry,
On Monday 12 November 2007 14:24, Jerry Van Baren wrote:
- 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.
val = data [bytecount ++];But since the right value is an unsigned char, I like the left value to be of the same type.
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...)
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.
diff --git a/common/spartan3.c b/common/spartan3.c old mode 100644 new mode 100755 index f7c4f8c..a386006 --- a/common/spartan3.c +++ b/common/spartan3.c @@ -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 { diff --git a/include/spartan3.h b/include/spartan3.h old mode 100644 new mode 100755 index 65a3f5a..11735e7 --- a/include/spartan3.h +++ b/include/spartan3.h @@ -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;
Hi Wolfgang,
On Monday 10 December 2007 13:04, you wrote:
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
Hi Matthias,
On 11 Dec 2007 at 18:00, Matthias Fuchs wrote:
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...?
Matthias
Regards, Wolfgang
Hi Wolfgang,
On Wednesday 12 December 2007 11:44, w.wegner@astro-kom.de wrote:
Hi Matthias,
On 11 Dec 2007 at 18:00, Matthias Fuchs wrote:
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...?
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
Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com wrote on 11/11/2007 08:45:02 AM:
< snip >
- 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
On 11/12/07, Bruce_Leonard@selinc.com Bruce_Leonard@selinc.com wrote:
Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com wrote on 11/11/2007 08:45:02 AM:
< snip >
- 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.
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.
Hi,
On Monday 12 November 2007 23:55, Bruce_Leonard@selinc.com wrote:
Matthias,
Matthias Fuchs matthias.fuchs@esd-electronics.com wrote on 11/11/2007 08:45:02 AM:
< snip >
- 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.
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
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
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
Hi Bruce,
On Tuesday 13 November 2007 19:15, Bruce_Leonard@selinc.com wrote:
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
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
I put this on my list.
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
participants (5)
-
Bruce_Leonard＠selinc.com -
Grant Likely -
Jerry Van Baren -
Matthias Fuchs -
w.wegner＠astro-kom.de