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

[Dirk Eibach]

Hi Wolfgang,

>> +void fpga_set_reg(unsigned int fpga, u16 reg, u16 data)
>> +{
>> +	int res;
>> +	struct ihs_fpga *fpga_0 = (struct ihs_fpga 
>> *)CONFIG_SYS_FPGA_BASE(0);
>> +
>> +	switch (fpga) {
>> +	case 0:
>> +		out_le16((u16 *)fpga_0 + reg / sizeof(u16), data);
>> +		break;
>> +	default:
>> +		res = mclink_send(fpga - 1, reg, data);
>> +		if (res < 0)
>> +			printf("mclink_send reg %02x data %04x returned %d\n",
>> +			       reg, data, res);
>> +		break;
>> +	}
>> +}
>> 
>> So no memory mapping here. That's the reason for all this fuzz.
> 
> 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."


> Apparently all your FPGA accesses are for  u16  data only?  Then you
> 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);


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,
   };
and
   fpga_set_reg(k, system_fpgas[k].reflection_low, 
REFLECTION_TESTPATTERN);

In fpga_set_reg() it would look like
   mclink_send(fpga - 1, (u16)addr, data);

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.

> And voila, no need to move away from the C structs.
> 
> Note 1: You print an error message if mclink_send() returns an error;
> 	I think this error should propagate, i. e. this function
> 	should not return  void.

Yep. Happy were the days when accessing FPGA registers could not fail 
:(

> 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 :)

Cheers
Dirk