| Message ID | 1367847325-21463-7-git-send-email-dirk.eibach@gdsys.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfgang Denk |
| Headers | show |
Dear dirk.eibach@gdsys.cc, In message <1367847325-21463-7-git-send-email-dirk.eibach@gdsys.cc> you wrote: > From: Dirk Eibach <eibach@gdsys.de> > > Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc> > --- > Changes in v2: None > > board/gdsys/405ep/dlvision-10g.c | 20 ++++++++++++++------ > board/gdsys/405ep/io.c | 20 ++++++++++++++------ > board/gdsys/405ep/neo.c | 17 +++++++++++++---- > board/gdsys/405ex/405ex.c | 14 ++++++-------- > board/gdsys/405ex/io64.c | 38 ++++++++++++++++++++++---------------- > 5 files changed, 69 insertions(+), 40 deletions(-) > > diff --git a/board/gdsys/405ep/dlvision-10g.c b/board/gdsys/405ep/dlvision-10g.c > index 644493b..4d1a02e 100644 > --- a/board/gdsys/405ep/dlvision-10g.c > +++ b/board/gdsys/405ep/dlvision-10g.c > @@ -71,6 +71,16 @@ enum { > RAM_DDR2_64 = 2, > }; > > +void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) > +{ > + out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data); > +} > + > +u16 fpga_get_reg(unsigned int fpga, u16 reg) > +{ > + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); > +} > + We strictly discourage using a notation of base address plus offset, and require to use proper C structs instead, especially because this allows type checking by the compiler. You had the this, and now attempt to throw it away. This makes no sense. NAK. Best regards, Wolfgang Denk
Hi Wolfgang, > ... > > We strictly discourage using a notation of base address plus offset, > and require to use proper C structs instead, especially because this > allows type checking by the compiler. I am very well aware of that. > You had the this, and now attempt to throw it away. This makes no > sense. In fact it does. 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. Cheers Dirk
Dear Dirk Eibach, In message <e8fccb4d422d619744521268691e5a9b@gdsys.cc> you wrote: > > > You had the this, and now attempt to throw it away. This makes no > > sense. > > In fact it does. > 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. Your new code still boils down to using the same standard I/O accessors. So your FPGA registers must be mapped somehow to I/O memory. When you can do something like +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +} why would you not be able to continue using in_le16() directly? Sorry, I don't get it. Best regards, Wolfgang Denk
Hi Wolfgang, Am 06.05.2013 17:22, schrieb Wolfgang Denk: > Dear Dirk Eibach, > > In message <e8fccb4d422d619744521268691e5a9b@gdsys.cc> you wrote: >> >>> You had the this, and now attempt to throw it away. This makes no >>> sense. >> >> In fact it does. >> 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. > > Your new code still boils down to using the same standard I/O > accessors. > > So your FPGA registers must be mapped somehow to I/O memory. > > When you can do something like > > +u16 fpga_get_reg(unsigned int fpga, u16 reg) > +{ > + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / > sizeof(u16)); > +} > > why would you not be able to continue using in_le16() directly? > > Sorry, I don't get it. Read the source, Luke :) On our iocon platform you find: +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. And sorry for the messed up series. Sometimes rebase can make things worse :) Cheers Dirk
Dear Dirk Eibach, In message <449c45a6a9978c55e84d3fe7efe6f0ac@gdsys.cc> you wrote: > > Read the source, Luke :) It was a bit difficult to spot in this unordered mix of patches ;-) > +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? 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) 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. 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. Best regards, Wolfgang Denk
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
Dear Dirk Eibach, In message <cf2913653766edc89705f49e08758df7@gdsys.cc> you wrote: > > > 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." Yes, you wrote that. I did not understand it then, nor do I understand it now. What do you mean by "create FPGA instances at address NULL"? in any case, "getting the register offsets by casting pointers to u16" makes no sense, as this is exactly what we try to avoid. By referencing a C struct you do NOT use any offsets. You use references to struct elements; where needed, you let the compiler to the address calculations (and the type checking). No offsets. No casts. > > 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); Mo. It should look like that: int fpga_set_reg(u32 fpga, u16 *addr, u16 data) rc = fpga_set_reg(k, &ptr->reflection_low, REFLECTION_TESTPATTERN); if (rc) ... > 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, > }; Is this not redundant? If you have an array of pointers (addresses), then you can identify the FPGA by using the pointer, and don't need to pass both the pointer _and_ the index to the fpga_{g,s}et_reg() functions? > 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. Why would you need casts? The whole purpose of this is to _avoid_ casts, so that the compiler has a chance to make sure all accesses are performed using the correct alignment and size of the respective data types. > > 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 :) Eventually you can drop the index alltogether when you just pass a pointer? Best regards, Wolfgang Denk
Am 07.05.2013 13:36, schrieb Wolfgang Denk: > Dear Dirk Eibach, > > In message <cf2913653766edc89705f49e08758df7@gdsys.cc> you wrote: >> >>> 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." > > Yes, you wrote that. I did not understand it then, nor do I > understand it now. What do you mean by "create FPGA instances at > address NULL"? in any case, "getting the register offsets by casting > pointers to u16" makes no sense, as this is exactly what we try to > avoid. By referencing a C struct you do NOT use any offsets. You use > references to struct elements; where needed, you let the compiler to > the address calculations (and the type checking). > > No offsets. No casts. OK. Once more. 3 of our 4 FPGAs are *not* memory mapped. There is no base address. This is what I want to show by: >> 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, >> }; For accessing registers of those (not memory mapped) FPGAs I need an u16 register-index. That is what I want to show by: >> mclink_send(fpga - 1, (u16)addr, data); This is the cast I am talking about. This is the reason why I still need the fpga index parameter. Cheers Dirk
Dear Dirk Eibach, In message <ba0b32bf657f5f3ffb6056cc6bd0452e@gdsys.cc> you wrote: > > OK. Once more. 3 of our 4 FPGAs are *not* memory mapped. There is no > base address. This is what I want to show by: > >> 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, > >> }; > > For accessing registers of those (not memory mapped) FPGAs I need an > u16 register-index. That is what I want to show by: > >> mclink_send(fpga - 1, (u16)addr, data); Can you please provide a bit more context? What is "addr", and why does it need a cast? > This is the cast I am talking about. > > This is the reason why I still need the fpga index parameter. From these code snippets I can't see what you mean. Best regards, Wolfgang Denk
Hi Wolfgang,
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,
REFLECTION_TESTPATTERN);
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;
}
}
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);
}
The cast to u16 I am talking about happens, when mclink_send() is
called.
Cheers
Dirk
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
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@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
Dear Dirk, In message <913a7e8badaaed06605fdf68faacedc8@gdsys.cc> you wrote: > > thanks for investing so much time into this. It is really appreciated. Well, just rejecting a patch without being able to give recommendations _how_ to improve it is not exactly nice. Sometimes it happens (like due to lack of time and/or better knowledge), but I try to avoid 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. You don't get what I mean. There is no guarantee by the C standard that the "value" of NULL actually resolves to 0; it could be something else. So assuming it does is not only ugly, but non-conforming to the C standard, and thus error prone. > > What do you think? > > Very nice. > I think this looks supringsingly similar to my original implementation > in the patch series. There we have: Yes, except that now we still have 1) the full type checking when we access struct elements in the standard I/O accessors and 2) no need for casts when using offsets, and 3) no need to assume that NULL == 0. > Sorry, I'm a little confused here. Is your approach an improvement > anyway? Yes, definitely. At least I'm convinced of that. Best regards, Wolfgang Denk
Hello Wolfgang, sorry for the delay, I had to clean some other things on my ToDoList. I cleaned up my patchseries and wanted to start implementing your proposal. And ran into a problem. Remember, the basic reason for the the generic FPGA accessors was, that we have a certain amount of basically identical FPGAs which are only connected to the CPU in different ways. The drivers accessing those FPGAs should be agnostic of that and simply be able to access those FPGAs by index. > 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); This works nicely for our memory mapped FPGA. But how about the other FPGAs? I don't have a fpga_ptr for them. Setting it NULL would make FPGA_SET_REG dereference a NULL pointer. Certainly I might call fpga_set_reg(1, NULL, offsetof(struct ihs_fpga, mc_tx_address), addr); but that would not be generic any more. Do you have any idea how to solve this? Cheers Dirk
Hello Wolfgang, would you please have a look at this? I need some feedback to get this finally sorted. Cheers Dirk 2013/5/15 Dirk Eibach <dirk.eibach@gdsys.cc>: > Hello Wolfgang, > > sorry for the delay, I had to clean some other things on my ToDoList. > > I cleaned up my patchseries and wanted to start implementing your proposal. > And ran into a problem. Remember, the basic reason for the the generic FPGA > accessors was, that we have a certain amount of basically identical FPGAs > which are only connected to the CPU in different ways. The drivers accessing > those FPGAs should be agnostic of that and simply be able to access those > FPGAs by index. > > >> 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); > > > This works nicely for our memory mapped FPGA. But how about the other FPGAs? > I don't have a fpga_ptr for them. Setting it NULL would make FPGA_SET_REG > dereference a NULL pointer. > Certainly I might call > fpga_set_reg(1, > NULL, > offsetof(struct ihs_fpga, mc_tx_address), > addr); > but that would not be generic any more. > > Do you have any idea how to solve this? > > > Cheers > Dirk > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
Hello Wolfgang, would you please have a look at this? I need some feedback to get this finally sorted. Cheers Dirk 2013/5/15 Dirk Eibach <dirk.eibach@gdsys.cc>: > Hello Wolfgang, > > sorry for the delay, I had to clean some other things on my ToDoList. > > I cleaned up my patchseries and wanted to start implementing your proposal. > And ran into a problem. Remember, the basic reason for the the generic FPGA > accessors was, that we have a certain amount of basically identical FPGAs > which are only connected to the CPU in different ways. The drivers accessing > those FPGAs should be agnostic of that and simply be able to access those > FPGAs by index. > > >> 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); > > > This works nicely for our memory mapped FPGA. But how about the other FPGAs? > I don't have a fpga_ptr for them. Setting it NULL would make FPGA_SET_REG > dereference a NULL pointer. > Certainly I might call > fpga_set_reg(1, > NULL, > offsetof(struct ihs_fpga, mc_tx_address), > addr); > but that would not be generic any more. > > Do you have any idea how to solve this? > > > Cheers > Dirk > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
Dear Dirk, In message <0e4604dcfff89a1bd2b7f144a5b3ceae@gdsys.cc> you wrote: > > sorry for the delay, I had to clean some other things on my ToDoList. Ditto :-( > > 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); > > } ... > > #define FPGA_SET_REG(ix,fld,val) \ > > fpga_set_reg((ix), \ > > &fpga_ptr->fld, \ > > offsetof(struct ihs_fpga, fld), \ > > val) ... > > 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); > > This works nicely for our memory mapped FPGA. But how about the other > FPGAs? I don't have a fpga_ptr for them. Setting it NULL would make > FPGA_SET_REG dereference a NULL pointer. No, it does not. In your setup you have a single memory mapped FPGA, addressed by the globally visible pointer fpga_ptr. When accessing this memory mapped FPGA (index is zero), then this pointer gets used, which is OK. For the other devices (index is not zero), then the pointer is ignored, and only the offset is used. In case you had more memory mapped devices (not only index 0; say, something like index < N) you can still use this, and pass any (properly aligned) pointer for the non-memory mapped case. Note that the pointer will NOT be dereferenced - only the address of field "fld" will be calculated (and even this only in theory, because this code will not be executed for the non-memory mapped case). > Certainly I might call > fpga_set_reg(1, > NULL, > offsetof(struct ihs_fpga, mc_tx_address), > addr); > but that would not be generic any more. You don't call fpga_set_reg() any more, you just use the FPGA_SET_REG() macro described above. > Do you have any idea how to solve this? I think this needs no new solution, because it shoul just work :-) Best regards, Wolfgang Denk
diff --git a/board/gdsys/405ep/dlvision-10g.c b/board/gdsys/405ep/dlvision-10g.c index 644493b..4d1a02e 100644 --- a/board/gdsys/405ep/dlvision-10g.c +++ b/board/gdsys/405ep/dlvision-10g.c @@ -71,6 +71,16 @@ enum { RAM_DDR2_64 = 2, }; +void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data); +} + +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +} + int misc_init_r(void) { /* startup fans */ @@ -95,10 +105,9 @@ static unsigned int get_mc2_present(void) static void print_fpga_info(unsigned dev) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(dev); - u16 versions = in_le16(&fpga->versions); - u16 fpga_version = in_le16(&fpga->fpga_version); - u16 fpga_features = in_le16(&fpga->fpga_features); + u16 versions = fpga_get_reg(dev, REG(versions)); + u16 fpga_version = fpga_get_reg(dev, REG(fpga_version)); + u16 fpga_features = fpga_get_reg(dev, REG(fpga_features)); unsigned unit_type; unsigned hardware_version; unsigned feature_rs232; @@ -263,8 +272,7 @@ int checkboard(void) int last_stage_init(void) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); - u16 versions = in_le16(&fpga->versions); + u16 versions = fpga_get_reg(0, REG(versions)); print_fpga_info(0); if (get_mc2_present()) diff --git a/board/gdsys/405ep/io.c b/board/gdsys/405ep/io.c index 070dcbb..dbaf9d6 100644 --- a/board/gdsys/405ep/io.c +++ b/board/gdsys/405ep/io.c @@ -53,6 +53,16 @@ enum { HWVER_122 = 3, }; +void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data); +} + +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +} + int misc_init_r(void) { /* startup fans */ @@ -117,10 +127,9 @@ int checkboard(void) static void print_fpga_info(void) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); - u16 versions = in_le16(&fpga->versions); - u16 fpga_version = in_le16(&fpga->fpga_version); - u16 fpga_features = in_le16(&fpga->fpga_features); + u16 versions = fpga_get_reg(0, REG(versions)); + u16 fpga_version = fpga_get_reg(0, REG(fpga_version)); + u16 fpga_features = fpga_get_reg(0, REG(fpga_features)); unsigned unit_type; unsigned hardware_version; unsigned feature_channels; @@ -179,7 +188,6 @@ static void print_fpga_info(void) */ int last_stage_init(void) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); unsigned int k; print_fpga_info(); @@ -191,7 +199,7 @@ int last_stage_init(void) configure_gbit_phy(k); /* take fpga serdes blocks out of reset */ - out_le16(&fpga->quad_serdes_reset, 0); + fpga_set_reg(0, REG(quad_serdes_reset), 0); return 0; } diff --git a/board/gdsys/405ep/neo.c b/board/gdsys/405ep/neo.c index d336e55..f06a280 100644 --- a/board/gdsys/405ep/neo.c +++ b/board/gdsys/405ep/neo.c @@ -44,6 +44,16 @@ enum { HWVER_300 = 3, }; +void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data); +} + +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +} + int misc_init_r(void) { /* startup fans */ @@ -70,10 +80,9 @@ int checkboard(void) static void print_fpga_info(void) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); - u16 versions = in_le16(&fpga->versions); - u16 fpga_version = in_le16(&fpga->fpga_version); - u16 fpga_features = in_le16(&fpga->fpga_features); + u16 versions = fpga_get_reg(0, REG(versions)); + u16 fpga_version = fpga_get_reg(0, REG(fpga_version)); + u16 fpga_features = fpga_get_reg(0, REG(fpga_features)); int fpga_state = get_fpga_state(0); unsigned unit_type; unsigned hardware_version; diff --git a/board/gdsys/405ex/405ex.c b/board/gdsys/405ex/405ex.c index 32e24c0..b7e9802 100644 --- a/board/gdsys/405ex/405ex.c +++ b/board/gdsys/405ex/405ex.c @@ -220,23 +220,21 @@ int board_early_init_r(void) gd405ex_set_fpga_reset(0); for (k = 0; k < CONFIG_SYS_FPGA_COUNT; ++k) { - struct ihs_fpga *fpga = - (struct ihs_fpga *)CONFIG_SYS_FPGA_BASE(k); #ifdef CONFIG_SYS_FPGA_NO_RFL_HI - u16 *reflection_target = &fpga->reflection_low; + u16 reflection_target = REG(reflection_low); #else - u16 *reflection_target = &fpga->reflection_high; + u16 reflection_target = REG(reflection_high); #endif /* * wait for fpga out of reset */ ctr = 0; while (1) { - out_le16(&fpga->reflection_low, - REFLECTION_TESTPATTERN); + fpga_set_reg(k, REG(reflection_low), + REFLECTION_TESTPATTERN); - if (in_le16(reflection_target) == - REFLECTION_TESTPATTERN_INV) + if (fpga_get_reg(k, reflection_target) == + REFLECTION_TESTPATTERN_INV) break; udelay(100000); diff --git a/board/gdsys/405ex/io64.c b/board/gdsys/405ex/io64.c index 7d2899d..f0f4241 100644 --- a/board/gdsys/405ex/io64.c +++ b/board/gdsys/405ex/io64.c @@ -67,6 +67,16 @@ enum { HWVER_110 = 1, }; +void fpga_set_reg(unsigned int fpga, u16 reg, u16 data) +{ + out_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16), data); +} + +u16 fpga_get_reg(unsigned int fpga, u16 reg) +{ + return in_le16((u16 *)CONFIG_SYS_FPGA_BASE(fpga) + reg / sizeof(u16)); +} + static inline void blank_string(int size) { int i; @@ -100,10 +110,9 @@ int misc_init_r(void) static void print_fpga_info(unsigned dev) { - struct ihs_fpga *fpga = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(dev); - u16 versions = in_le16(&fpga->versions); - u16 fpga_version = in_le16(&fpga->fpga_version); - u16 fpga_features = in_le16(&fpga->fpga_features); + u16 versions = fpga_get_reg(dev, REG(versions)); + u16 fpga_version = fpga_get_reg(dev, REG(fpga_version)); + u16 fpga_features = fpga_get_reg(dev, REG(fpga_features)); int fpga_state = get_fpga_state(dev); unsigned unit_type; @@ -242,8 +251,6 @@ int last_stage_init(void) { unsigned int k; unsigned int fpga; - struct ihs_fpga *fpga0 = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(0); - struct ihs_fpga *fpga1 = (struct ihs_fpga *) CONFIG_SYS_FPGA_BASE(1); int failed = 0; char str_phys[] = "Setup PHYs -"; char str_serdes[] = "Start SERDES blocks"; @@ -281,17 +288,16 @@ int last_stage_init(void) /* take fpga serdes blocks out of reset */ puts(str_serdes); udelay(500000); - out_le16(&fpga0->quad_serdes_reset, 0); - out_le16(&fpga1->quad_serdes_reset, 0); + fpga_set_reg(0, REG(quad_serdes_reset), 0); + fpga_set_reg(1, REG(quad_serdes_reset), 0); blank_string(strlen(str_serdes)); /* take channels out of reset */ puts(str_channels); udelay(500000); for (fpga = 0; fpga < 2; ++fpga) { - u16 *ch0_config_int = &(fpga ? fpga1 : fpga0)->ch0_config_int; for (k = 0; k < 32; ++k) - out_le16(ch0_config_int + 4 * k, 0); + fpga_set_reg(fpga, REG(ch0_config_int) + 8 * k, 0); } blank_string(strlen(str_channels)); @@ -299,16 +305,16 @@ int last_stage_init(void) puts(str_locks); udelay(500000); for (fpga = 0; fpga < 2; ++fpga) { - u16 *ch0_status_int = &(fpga ? fpga1 : fpga0)->ch0_status_int; for (k = 0; k < 32; ++k) { - u16 status = in_le16(ch0_status_int + 4*k); + u16 status = + fpga_get_reg(fpga, REG(ch0_status_int) + 8 * k); if (!(status & (1 << 4))) { failed = 1; printf("fpga %d channel %d: no serdes lock\n", fpga, k); } /* reset events */ - out_le16(ch0_status_int + 4*k, status); + fpga_set_reg(fpga, REG(ch0_status_int) + 8 * k, 0); } } blank_string(strlen(str_locks)); @@ -316,14 +322,14 @@ int last_stage_init(void) /* verify hicb_status */ puts(str_hicb); for (fpga = 0; fpga < 2; ++fpga) { - u16 *ch0_hicb_status_int = &(fpga ? fpga1 : fpga0)->ch0_hicb_status_int; for (k = 0; k < 32; ++k) { - u16 status = in_le16(ch0_hicb_status_int + 4*k); + u16 status = + fpga_get_reg(fpga, REG(ch0_status_int) + 8 * k); if (status) printf("fpga %d hicb %d: hicb status %04x\n", fpga, k, status); /* reset events */ - out_le16(ch0_hicb_status_int + 4*k, status); + fpga_set_reg(fpga, REG(ch0_status_int) + 8 * k, 0); } } blank_string(strlen(str_hicb));