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

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

[Bruce_Leonard at selinc.com]

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

[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@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

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

[Grant Likely]

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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

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

[Matthias Fuchs]

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

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

[Bruce_Leonard at selinc.com]

Wolfgang,

wd at 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