views:

424

answers:

7

I am trying to perform a less-than-32bit read over the PCI bus to a VME-bridge chip (Tundra Universe II), which will then go onto the VME bus and picked up by the target.

The target VME application only accepts D32 (a data width read of 32bits) and will ignore anything else.

If I use bit field structure mapped over a VME window (nmap'd into main memory) I CAN read bit fields >24 bits, but anything less fails. ie :-

struct works {
    unsigned int a:24;
};

struct fails {
    unsigned int a:1;
    unsigned int b:1;
    unsigned int c:1;
};

struct main {
    works work;
    fails fail;
}
volatile *reg = function_that_creates_and_maps_the_vme_windows_returns_address()

This shows that the struct works is read as a 32bit, but a read via fails struct of a for eg reg->fail.a is getting factored down to a X bit read. (where X might be 16 or 8?)

So the questions are :
a) Where is this scaled down? Compiler? OS? or the Tundra chip?
b) What is the actual size of the read operation performed?

I basiclly want to rule out everything but the chip. Documentation on that is on the web, but if it can be proved that the data width requested over the PCI bus is 32bits then the problem can be blamed on the Tundra chip!

edit:-
Concrete example, code was:-

struct SVersion
{
    unsigned title         : 8;
    unsigned pecversion    : 8;
    unsigned majorversion  : 8;
    unsigned minorversion  : 8;
} Version;

So now I have changed it to this :-

union UPECVersion
{
    struct SVersion
    {
        unsigned title         : 8;
        unsigned pecversion    : 8;
        unsigned majorversion  : 8;
        unsigned minorversion  : 8;
    } Version;
    unsigned int dummy;
};

And the base main struct :-

typedef struct SEPUMap
{
    ...
    ...
    UPECVersion PECVersion;

};

So I still have to change all my baseline code

// perform dummy 32bit read
pEpuMap->PECVersion.dummy;

// get the bits out
x = pEpuMap->PECVersion.Version.minorversion;

And how do I know if the second read wont actually do a real read again, as my original code did? (Instead of using the already read bits via the union!)

+2  A: 

I am wondering about the value of sizeof(struct fails). Is it 1? In this case, if you perform the read by dereferencing a pointer to a struct fails, it looks correct to issue a D8 read on the VME bus.

You can try to add a field unsigned int unused:29; to your struct fails.

Didier Trosset
Thanks, and I think your correct, but the reads are members of the "fails" struct, eg "reg->fail.a = true;" and I cannot change that code base.I have actually packed the struct, but for some reason didnt post that!
IanVaughan
+1  A: 

Ian - if you want to be sure as to the size of things you're reading/writing I'd suggest not using structs like this to do it - it's possible the sizeof of the fails struct is just 1 byte - the compiler is free to decide what it should be based on optimizations etc- I'd suggest reading/writing explicitly using int's or generally the things you need to assure the sizes of and then doing something else like converting to a union/struct where you don't have those limitations.

RnR
Thanks, and yes I would of loved the original programmer to of done that, or (as you may be suggesting) it should of been abstracted away from the interface via accessors (read/writes)!But I have this code base with lots of "reg->fail.a = true;" etc and I have to try and get this working, hopefully without any changes to the calling code!Also WRT the "sizeof(fails)", I have actually packed it, of which I forgot to show/post!
IanVaughan
RnR
Oh yes, I do think it would be easier/better ..., so that is what I have now just started!
IanVaughan
+6  A: 

Your compiler is adjusting the size of your struct to a multiple of its memory alignment setting. Almost all modern compilers do this. On some processors, variables and instructions have to begin on memory addresses that are multiples of some memory alignment value (often 32-bits or 64-bits, but the alignment depends on the processor architecture). Most modern processors don't require memory alignment anymore - but almost all of them see substantial performance benefit from it. So the compilers align your data for you for the performance boost.

However, in many cases (such as yours) this isn't the behavior you want. The size of your structure, for various reasons, can turn out to be extremely important. In those cases, there are various ways around the problem.

One option is to force the compiler to use different alignment settings. The options for doing this vary from compiler to compiler, so you'll have to check your documentation. It's usually a #pragma of some sort. On some compilers (the Microsoft compilers, for instance) it's possible to change the memory alignment for only a very small section of code. For example (in VC++):

#pragma pack(push)      // save the current alignment
#pragma pack(1)         // set the alignment to one byte
// Define variables that are alignment sensitive
#pragma pack(pop)       // restore the alignment

Another option is to define your variables in other ways. Intrinsic types are not resized based on alignment, so instead of your 24-bit bitfield, another approach is to define your variable as an array of bytes.

Finally, you can just let the compilers make the structs whatever size they want and manually record the size that you need to read/write. As long as you're not concatenating structures together, this should work fine. Remember, however, that the compiler is giving you padded structs under the hood, so if you make a larger struct that includes, say, a works and a fails struct, there will be padded bits in between them that could cause you problems.

On most compilers, it's going to be darn near impossible to create a data type smaller than 8 bits. Most architectures just don't think that way. This shouldn't be a huge problem because most hardware devices that use datatypes of smaller than 8-bits end up arranging their packets in such a way that they still come in 8-bit multiples, so you can do the bit manipulations to extract or encode the values on the data stream as it leaves or comes in.

For all of the reasons listed above, a lot of code that works with hardware devices like this work with raw byte arrays and just encode the data within the arrays. Despite losing a lot of the conveniences of modern language constructs, it ends up just being easier.

Russell Newquist
thank you for a well informed post, but I dont think it has anything todo with alignement!If I request a read 8 bits, I want it do read the full 32bit word, and then just give me the 8 bits I asked for.This works for 24 bits, as somewhere this is getting translatted into a 32bit read, and then giving me back the 24 bits I wanted.But this doesnt work for 8 bits for eg, as "it" knows it can do a smaller read, but my hardware doesn't support this, so I want "it" to always do 32bit reads!But I dont know where/who "it" is!
IanVaughan
+1  A: 

As an example, the Linux kernel has inline functions that explicitly handle memory-mapped IO reads and writes. In newer kernels it's a big macro wrapper that boils down to an inline assembly movl instruction, but it older kernels it was defined like this:

#define readl(addr) (*(volatile unsigned int *) (addr))
#define writel(b,addr) ((*(volatile unsigned int *) (addr)) = (b))
Eric Seppanen
Thanks, but with this I would have to rewrite all the code to use these accessors, and it is rewrite I was asking for help to avoid!I have very many in-line code statements as above "reg->fail.a = true;" etc. Using the macro is no different than how it should of been done in the first place, by abstracting away and using accessors! (but not my code!)
IanVaughan
I think is actually gone to be the solution! Im working on using this...
IanVaughan
IanVaughan
I think one reason for using inline functions like this, even though it forces a little bit of casting ugliness, is for style reasons. You will never, ever confuse a read or write to a device register with a read or write to memory. I like to be able to see where in my code device registers are touched, especially when I end up dealing with badly designed hardware that does silly things like clearing a register on read. Plus, I would probably wake up at night wondering if I remembered to declare all my pointers 'volatile'.
Eric Seppanen
hear hear! good points.
IanVaughan
+1  A: 
Thomas Matthews
Many thanks, and very interesting. I have always assumed that if I started defining bit fields within a struct, the compiler would not pad/pack/align, as I have stated 'I know what im doing!'.Also very interesting re using #defines, although personally I find them very unreadable/attractive, but what do I want here (working code)!My main problem is that the codebase has already been written, and it does read/writes of "reg->fail.a" in-line! As I stated, the >=:24bit reads work, so I was hoping to get the <24bit reads working by a compiler option!
IanVaughan
+1  A: 

It is the compiler that decides what size read to issue. To force a 32 bit read, you could use a union:

union dev_word {
    struct dev_reg {
        unsigned int a:1;
        unsigned int b:1;
        unsigned int c:1;
    } fail;
    uint32_t dummy;
};

volatile union dev_word *vme_map_window();

If reading the union through a volatile-qualified pointer isn't enough to force a read of the whole union (I would think it would be - but that could be compiler-dependent), then you could use a function to provide the required indirection:

volatile union dev_word *real_reg; /* Initialised with vme_map_window() */

union dev_word * const *reg_func(void)
{
    static union dev_word local_copy;
    static union dev_word * const static_ptr = &local_copy;

    local_copy = *real_reg;
    return &static_ptr;
}

#define reg (*reg_func())

...then (for compatibility with the existing code) your accesses are done as:

reg->fail.a
caf
Excellent idea, I like the sound of this, but my problem is I have lots of in-line code statements as "reg->fail.a = true;" etc, and Im not sure how adding the union would effect the size of the read done via my original call, unless I first did the read of the "dummy"?
IanVaughan
I would think that on most compilers, reading through a volatile-qualified pointer to union would force it to read the entire size of the union - but if that's not the case on your particular compiler, see my updated answer for a possible solution.
caf
"reading through a volatile-qualified pointer to union" yes I see how reading the "dummy" would work, but as stated re not wishing to change all my original code.You then give a seemly nice alternative, of which I don't really understand! (many thanks) (I will try tho)But as I have 29 bit-field structs, grouped in my "main" struct, I would have to create all of these reg_func for each struct type!
IanVaughan
Well, what I meant is that the compiler *ought* to issue a read for the entire size of the union, even if you just read one bitfield from it, because of the volatile qualifier. Also, rather than writing one function for each struct type, you could probably have a single generic function and use pointer-casting (or more unions!) within the macros to get at the data via the right struct type.
caf
A: 

I believe the only solution is to
1) edit/create my main struct as all 32bit ints (unsigned longs)
2) keep my original bit-field structs
3) each access I require,
3.1) I have to read the struct member as a 32bit word, and cast it into the bit-field struct,
3.2) read the bit-field element I require. (and for writes, set this bit-field, and write the word back!)

(1) Which is a same, because then I lose the intrinsic types that each member of the "main/SEPUMap" struct are.

End solution :-
Instead of :-

printf("FirmwareVersionMinor: 0x%x\n", pEpuMap->PECVersion);

This :-

SPECVersion ver = *(SPECVersion*)&pEpuMap->PECVersion;

printf("FirmwareVersionMinor: 0x%x\n", ver.minorversion);

Only problem I have is writting! (Writes are now Read/Modify/Writes!)

// Read - Get current
_HVPSUControl temp = *(_HVPSUControl*)&pEpuMap->HVPSUControl;

// Modify - set to new value
temp.OperationalRequestPort = true;

// Write
volatile unsigned int *addr = reinterpret_cast<volatile unsigned int*>(&pEpuMap->HVPSUControl);

*addr = *reinterpret_cast<volatile unsigned int*>(&temp);

Just have to tidy that code up into a method!

#define writel(addr, data) ( *(volatile unsigned long*)(&addr) = (*(volatile unsigned long*)(&data)) )
IanVaughan
All I really need now is some funky method code that I can call with any struct member and some data, and it will work out the base address of the 32bit read, and then do that read/modify/write. "writecool(pEpuMap->Struct1.BitField, true);" !
IanVaughan