Dear Dirk,
In message 58ff5023adcbf08481bc2707b0a70f50@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