[U-Boot] [PATCH v2 06/10] powerpc/ppc4xx: Use generic FPGA accessors on all gdsys 405ep/ex boards

[Wolfgang Denk]

Dear Dirk Eibach,

In message <cf2913653766edc89705f49e08758df7 at gdsys.cc> you wrote:
> 
> > OK - but I don't see why mclink_send() could not be used in the same
> > way, i. e. taking a struct member as argument?
> 
> Certainly it *can* be used using *pointers* to struct members as 
> argument.
> 
> What I stated yesterday was:
> "We have FPGAs that are memory mapped and others that are not. They 
> must be accessed by the same drivers. So the alternative would be to 
> create FPGA instances at address NULL and getting the register offesets 
> by casting pointers to u16. Not very nice either."

Yes, you wrote that.  I did not understand it then, nor do I
understand it now.  What do you mean by "create FPGA instances at
address NULL"?  in any case, "getting the register offsets by casting
pointers to u16" makes no sense, as this is exactly what we try to
avoid.  By referencing a C struct you do NOT use any offsets.  You use
references to struct elements; where needed, you let the compiler to
the address calculations (and the type checking).

No offsets. No casts.

> > could rewrite fpga_set_reg() similar to this:
> > 
> > 	int fpga_set_reg(u32 fpga, u16 *addr, u16 data)
> 
> With
>    struct ihs_fpga {
> 	u16 reflection_low;	/* 0x0000 */
> 	u16 versions;		/* 0x0002 */
> 	...
>    };
> and
>    void fpga_set_reg(unsigned int fpga, u16 reg, u16 data)
> the call looks like
>    fpga_set_reg(k, REG(reflection_low), REFLECTION_TESTPATTERN);

Mo. It should look like that:

	int fpga_set_reg(u32 fpga, u16 *addr, u16 data)

	rc = fpga_set_reg(k, &ptr->reflection_low, REFLECTION_TESTPATTERN);
	if (rc)
		...
> To use
>    int fpga_set_reg(u32 fpga, u16 *addr, u16 data)
> we need something like
>    struct ihs_fpga system_fpgas[] = {
> 	(struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(0),
> 	(struct ihs_fpga *)NULL,
> 	(struct ihs_fpga *)NULL,
> 	(struct ihs_fpga *)NULL,
>    };

Is this not redundant?  If you have an array of pointers (addresses),
then you can identify the FPGA by using the pointer, and don't need
to pass both the pointer _and_ the index to the fpga_{g,s}et_reg()
functions?

> I personally dislike all this NULL-pointer passing and casting. But 
> YMMV. If that's the u-boot-way I will be happy to change it.

Why would you need casts?  The whole purpose of this is to _avoid_
casts, so that the compiler has a chance to make sure all accesses are
performed using the correct alignment and size of the respective data
types.

> > Note 2: I changed the type of the first arg into "u32" so it is
> >         consistent with the other args. Mixing native types (unsigned
> >         int) here and defined types (as u16 instead of unsigned
> >         short) looks weird to me.
> 
> I wanted to express the 16 bit io-implementation of our address/data 
> busses, while the fpga index is simply a number. Maybe that is 
> information overkill :)

Eventually you can drop the index alltogether when you just pass a
pointer?

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
Time is a drug. Too much of it kills you.
                                      - Terry Pratchett, _Small Gods_