[U-Boot-Users] Need the lists opinion on the "right" way to fix something (long)

[Wolfgang Denk]

In message <OF35EFB6C6.4311E888-ON88257386.007BCD10-88257386.007E0B81 at 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The human mind treats a new idea the way the body  treats  a  strange
protein - it rejects it.                                 - P. Medawar