[U-Boot-Users] Need the lists opinion on the "right" way to fix something (long)
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 (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 .../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.
Bruce
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
On 11/1/07, Wolfgang Denk wd@denx.de wrote:
In message OF35EFB6C6.4311E888-ON88257386.007BCD10-88257386.007E0B81@selinc.com you wrote:
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.
Also, the whole CFG_FPGA bitmask is a bad approach and should be removed. Instead the config should be based on whether or not the CFG_XILINX_SPARTAN et al are defined or not. CONFIG_CMD used to be done with a bit field too, but it was removed due to it being difficult to extend.
Cheers, g.
Hi,
On Friday 02 November 2007 00:17, Grant Likely wrote:
On 11/1/07, Wolfgang Denk wd@denx.de wrote:
In message OF35EFB6C6.4311E888-ON88257386.007BCD10-88257386.007E0B81@selinc.com you wrote:
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.
Also, the whole CFG_FPGA bitmask is a bad approach and should be removed. Instead the config should be based on whether or not the CFG_XILINX_SPARTAN et al are defined or not. CONFIG_CMD used to be done with a bit field too, but it was removed due to it being difficult to extend.
Ack. That's what I also throught about.
Matthias
Wolfgang,
wd@denx.de wrote on 11/01/2007 04:14:19 PM:
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.
Well, I thought of that and that was the first thing I tried. However, when I include xilinx.h in the board config, envcrc.c fails to build with the following message:
In file included from /home/builder/temp/u-boot/include/fpga.h:25 from /home/builder/temp/u-boot/include/xilinx.h:25 from /home/builder/temp/u-boot/include/configs/sel3530.h:46 from /home/builder/temp/u-boot/include/config.h from envcrc.c:32: /usr/include/linux/types.h:133: error: syntax error before '__le16' /usr/include/linux/types.h:134: error: syntax error before '__be16' /usr/include/linux/types.h:135: error: syntax error before '__le32' /usr/include/linux/types.h:136: error: syntax error before '__be32' /usr/include/linux/types.h:140: error: syntax error before '__le64' /usr/include/linux/types.h:141: error: syntax error before '__be64'
I gather from your comments that this is not expected and I should be able to include files in my config file? I ask because all the other config files I've looked at don't have includes in them.
Bruce
participants (4)
-
Bruce_Leonard＠selinc.com -
Grant Likely -
Matthias Fuchs -
Wolfgang Denk