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

[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

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?

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
It is much easier to suggest solutions when you know nothing