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

[Dirk Eibach]

Hi Wolfgang,

thanks for investing so much time into this. It is really appreciated.

Am 08.05.2013 12:13, schrieb Wolfgang Denk:
> Dear Dirk,
> 
> In message <58ff5023adcbf08481bc2707b0a70f50 at gdsys.cc> you wrote:
>> 
>> in my example we have four FPGAs. One is memory mapped while the 
>> other
>> three are not.
>> They all have the same register interface which is mapped by
>>    struct ihs_fpga {
>> 	u16 reflection_low;
>> 	u16 versions;
>> 	...
>> 	u16 mc_tx_address;
>> 	u16 mc_tx_data;
>> 	u16 mc_tx_cmd;
>> 	...
>>    };
>> 
>> To have instances of those FPGA structs, we might create an array 
>> like
>> this:
>>    struct ihs_fpga system_fpgas[] = {
>> 	(struct ihs_fpga *)CONFIG_SYS_FPGA_BASE,
>> 	(struct ihs_fpga *)NULL,
>> 	(struct ihs_fpga *)NULL,
>> 	(struct ihs_fpga *)NULL,
>>    };
>> The first element ist our memory mapped FPGA with base address
>> CONFIG_SYS_FPGA_BASE. For the other three I use NULL as base address
>> because I don't have any better idea what else to choose.
>> 
>> To probe those FPGAs we might have to do something like:
>>    for (k=0; k < 4; ++k)
>> 	fpga_set_reg(k, &system_fpgas[k].reflection_low,
> 
> I think using index notation everywhere makes the code hard to read.
> Use a pointer, for example like that:
> 
> 	struct ihs_fpga *fpgap = system_fpgas[k];
> 
> 	fpga_set_reg(k, &fpgap->reflection_low, ...
> 
>> The implementation of fpga_set_reg() might look like:
>> 
>> void fpga_set_reg(unsigned int fpga, u16 *reg, u16 data)
>> {
>> 	switch (fpga) {
>> 	case 0:
>> 		out_le16(reg, data);
>> 		break;
>> 	default:
>> 		mclink_send(fpga - 1, (u16)reg, data);
>> 		break;
>> 	}
>> }
> 
> The problem here comes from your decision to use the same interface
> for two different, incompatible things.  There are two ugly parts in
> this code:  first is the need for a cast, second (and even worse) is
> the use of a pointer value of 0 to get to the offset of a field.
> Actually this is not even standard conformng, as you initialize the
> pointer as NULL, and there is no real guarantee that
> 
> 	NULL == (struct ihs_fpga *)0

You are right. This is *exactly* what I meant when I wrote: "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." on monday.


> It appears you need both the element address and the offset in your
> fpga_set_reg() code, so you should pass this information, like that
> [note that we no longer need an array of addresses]:
> 
> struct ihs_fpga fpga_ptr = (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE;
> ...
> 
> void fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data)
> {
> 	if (fpga)
> 		return mclink_send(fpga - 1, regoff, data);
> 
> 	return out_le16(reg, data);
> }
> 
>> where mclink_send() is the routine to access the non memory mapped
>> FPGAs through the memory mapped FPGA:
>>    int mclink_send(u8 slave, u16 addr, u16 data)
>>    {
>> 	...
>> 	fpga_set_reg(0, &system_fpgas[0].mc_tx_address, addr);
>> 	fpga_set_reg(0, &system_fpgas[0].mc_tx_data, data);
>> 	fpga_set_reg(0, &system_fpgas[0].mc_tx_cmd, (slave & 0x03) << 14);
>>    }
> 
> And this becomes:
> 
> 	fpga_set_reg(0,
> 		     &fpga_ptr->mc_tx_address,
> 		     offsetof(struct ihs_fpga, mc_tx_address),
> 		     addr);
> 
> etc. Yes, this is long and ugly to write, so you may want to define a
> helper macro like:
> 
> #define FPGA_SET_REG(ix,fld,val)	\
> 	fpga_set_reg((ix),		\
> 	&fpga_ptr->fld,			\
> 	offsetof(struct ihs_fpga, fld),	\
> 	val)
> 
> so this turns into a plain and simple
> 
> 	FPGA_SET_REG(0, mc_tx_address, addr);
> 	FPGA_SET_REG(0, mc_tx_data, data);
> 	FPGA_SET_REG(0, mc_tx_cmd, (slave & 0x03) << 14);
> 
> 
> What do you think?

Very nice.
I think this looks supringsingly similar to my original implementation 
in the patch series. There we have:
   #define REG(reg) offsetof(struct ihs_fpga, reg)
and
   fpga_set_reg(k, REG(reflection_low), REFLECTION_TESTPATTERN);
and
   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;
	}
   }

Sorry, I'm a little confused here. Is your approach an improvement 
anyway?

Cheers
Dirk