In message OF35EFB6C6.4311E888-ON88257386.007BCD10-88257386.007E0B81@selinc.com you wrote:
Not long ago Grant put in a fix to .../common/cmd_fpga.c that was the right thing to do but introduced another problem. What he did was essentially this:
#if CFG_FPGA_XILINX
#if (CONFIG_FPGA & CFG_FPGA_XILINX)It's the right thing to do because the first always evaluated to true and
Actually, my feeling is that both is wrong. It is IMHO not a good idea to use integer definitions and logic operations here.
The old form was definitely broken or at least redundant as CFG_FPGA_XILINX would always evaluate to a non-zero expression.
(in Grant's opinion and mine) the real intent is to check CONFIG_FPGA and see if it's a Xilinx part.
However, it introduces the following problem. CONFIG_FPGA is defined in the board specific config file and is usually going to be something like this:
#define CONFIG_FPGA CFG_XILINX_SPARTAN3
Following CFG_XILINX_SPARTAN3 to .../include/xilinx.h you find:
#define CFG_XILINX_SPARTAN3 CFG_FPGA_XILINX | CFG_SPARTAN3
Now as to the problem: .../common/cmd_fpga.c includes .../include/fpga.h but NOT .../include/xilinx.h. This makes sense because .../common/cmd_fpga.c is a common file not a Xilinx specific file. But, when the preprocessor hits Grant's fix it tries to substitute CFG_XILINX_SPARTAN3 for CONFIG_FPGA and can't because CFG_XILINX_SPARTAN3 is defined in .../include/xilinx.h. The simple solution is to include
Then this is a bug in your board config file by using a preprocessor variable (CFG_XILINX_SPARTAN3) that is nowhere defined. Seems you need to include xilinx.h in your board config file.
.../include/xilinx.h (which includes .../include/fpga.h) and life is happy. The reason I don't like doing that is because .../common/cmd_fpga.c is a common file and I believe it would be a "bad" thing to put Xilinx specific stuff into a common file.
Of course the flip side of this is that the function fpga_loadbitsream() in .../common/cmd_fpga.c is already Xilinx specific. IMHO, it shouldn't be there in the first place but I understand why it's there. It's doing some pre-processing on the Xilinx BIT file prior to handing it to fpga_load(), which is a common high-level function that then starts drilling down to determine Altera vs. Xilinx, serial vs. parallel loading, etc.
Since I've only been working with u-boot/open source for about 9 months, I'm not really sure what the design philosophies are and what the acceptable and/or "righ" way of fixing this would be. Any input or direction would be greatly appreciated. Thanks.
The simple rule is: whenever you use a variable / macro you must make sure it is defined. You use CFG_XILINX_SPARTAN3 in your board config, so you must make sure to include all needed header files there.
Best regards,
Wolfgang Denk