Dear Dirk,
In message <58ff5023adcbf08481bc2707b0a70...@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: w...@denx.de
It is much easier to suggest solutions when you know nothing
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
