Hello all,
I've tracked down the problem with the DAC960 driver (drivers/block), a
module that handles the mylex range of raid cards.
All the structures dealing with the card sound like this one:
typedef union DAC960_LA_InboundDoorBellRegister
{
unsigned char All;
struct {
boolean HardwareMailboxNewCommand:1; /* Bit 0 */
boolean AcknowledgeHardwareMailboxStatus:1; /* Bit 1 */
boolean GenerateInterrupt:1; /* Bit 2 */
boolean ControllerReset:1; /* Bit 3 */
boolean MemoryMailboxNewCommand:1; /* Bit 4 */
unsigned char :3; /* Bits 5-7 */
} Write;
struct {
boolean HardwareMailboxEmpty:1; /* Bit 0 */
boolean InitializationNotInProgress:1; /* Bit 1 */
unsigned char :6; /* Bits 2-7 */
} Read;
}
DAC960_LA_InboundDoorBellRegister_T
And I guess on ppc we would need all the bits line reversed.
So, my question is how to handle this in the best approach, the goal
being a unified driver nt too difficult to maintain
I've attached a little sample written by hollis that reproduces the
problem. I've managed to get gcc choose the right structure according to
the endianness of the host with endian.h and a couple of #ifdef, but
this is still rather bad since we have to maintain two versions of the
structs.
Does antbody has a good idea on this or an example of driver we could
follow ?
Thanks,
GoM
From: Timothy A. Seufert <hidden> Date: 2001-06-23 18:19:01
At 11:29 AM +0200 6/23/01, Guillaume Laurès wrote:
And I guess on ppc we would need all the bits line reversed.
So, my question is how to handle this in the best approach, the goal
being a unified driver nt too difficult to maintain
Kill the struct definition, and replace it with a bunch of accessor
macros that mask-and-shift. When moving the register value to or
from the hardware, use le32_to_cpu() and cpu_to_le32() as appropriate.
Bitfields are not a good way to describe hardware registers.
Tim Seufert
** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2001-06-23 23:22:50
quoted
Bitfields are not a good way to describe hardware registers.
Why is that? Are there generally bitfield problems other than this one?
Neither structures nor bitfields are good for describing HW registers
because they are always implementation dependant, the actual layout
in memory of a structure of a bitfield depends completely on the
compiler, it's setting, the platform, whatever....
Ben.
** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/
Kill the struct definition, and replace it with a bunch of accessor
macros that mask-and-shift. When moving the register value to or
from the hardware, use le32_to_cpu() and cpu_to_le32() as appropriate.
Okay, so as this are my first steps in kernel programming, let's take an
example :-)
Somewhere at the beginning of the driver we have something like this:
while (DAC960_LA_InitializationInProgressP(BaseAddress))
{blabla}
DAC960_LA_InitializationInProgressP() is defined as follows in the .h:
static inline
boolean DAC960_BA_InitializationInProgressP(void *ControllerBaseAddress)
{
DAC960_BA_InboundDoorBellRegister_T InboundDoorBellRegister;
InboundDoorBellRegister.All =
readb(ControllerBaseAddress + DAC960_BA_InboundDoorBellRegisterOffset);
return !InboundDoorBellRegister.Read.InitializationNotInProgress;
}
and, for the record, the DAC960_BA_InboundDoorBellRegister_T is
something like this:
typedef union DAC960_BA_InboundDoorBellRegister
{
unsigned char All;
struct {
boolean HardwareMailboxNewCommand:1; /* Bit 0 */
boolean AcknowledgeHardwareMailboxStatus:1; /* Bit 1 */
boolean GenerateInterrupt:1; /* Bit 2 */
boolean ControllerReset:1; /* Bit 3 */
boolean MemoryMailboxNewCommand:1; /* Bit 4 */
unsigned char :3; /* Bits 5-7 */
} Write;
struct {
boolean HardwareMailboxEmpty:1; /* Bit 0 */
boolean InitializationNotInProgress:1; /* Bit 1 */
unsigned char :6; /* Bits 2-7 */
} Read;
}
DAC960_BA_InboundDoorBellRegister_T;
What would I do now is modify DAC960_LA_InitializationInProgressP() as
follows:
static inline
boolean DAC960_BA_InitializationInProgressP(void *ControllerBaseAddress)
{
unsigned long InboundDoorBellRegister =
le32_to_cpu(ControllerBaseAddress +
DAC960_BA_InboundDoorBellRegisterOffset);
return (boolean) !(InboundDoorBellRegister & 0x0002);
}
Is it correct ?
And where can I find the cpu_to_le32() and le32_to_cpu() declaration or
a guide on how to use them ?
Thanks
** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2001-06-24 14:50:42
Is it correct ?
And where can I find the cpu_to_le32() and le32_to_cpu() declaration or
a guide on how to use them ?
If you use readl/writel, you don't need to byteswap as it's done for
you by those functions. If you used readb/writeb, then you are reading
bytes, swapping has no meaning.
Also, don't hard code 0x0002, better #define a constant, it makes the
code more readable.
cpu_to_le32/le32_to_cpu() are necessary if you are either not using
readx/writex accessors, or when storing datas in memory that the
controller will read directly (bus master)
Ben.
** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/
Kill the struct definition, and replace it with a bunch of accessor
macros that mask-and-shift. When moving the register value to or
from the hardware, use le32_to_cpu() and cpu_to_le32() as appropriate.
Okay, so as this are my first steps in kernel programming, let's take an
example :-)
Somewhere at the beginning of the driver we have something like this:
while (DAC960_LA_InitializationInProgressP(BaseAddress))
{blabla}
DAC960_LA_InitializationInProgressP() is defined as follows in the .h:
static inline
boolean DAC960_BA_InitializationInProgressP(void *ControllerBaseAddress)
{
DAC960_BA_InboundDoorBellRegister_T InboundDoorBellRegister;
InboundDoorBellRegister.All =
readb(ControllerBaseAddress + DAC960_BA_InboundDoorBellRegisterOffset);
return !InboundDoorBellRegister.Read.InitializationNotInProgress;
}
and, for the record, the DAC960_BA_InboundDoorBellRegister_T is
something like this:
typedef union DAC960_BA_InboundDoorBellRegister
{
unsigned char All;
struct {
boolean HardwareMailboxNewCommand:1; /* Bit 0 */
boolean AcknowledgeHardwareMailboxStatus:1; /* Bit 1 */
boolean GenerateInterrupt:1; /* Bit 2 */
boolean ControllerReset:1; /* Bit 3 */
boolean MemoryMailboxNewCommand:1; /* Bit 4 */
unsigned char :3; /* Bits 5-7 */
} Write;
struct {
boolean HardwareMailboxEmpty:1; /* Bit 0 */
boolean InitializationNotInProgress:1; /* Bit 1 */
unsigned char :6; /* Bits 2-7 */
} Read;
}
DAC960_BA_InboundDoorBellRegister_T;
What would I do now is modify DAC960_LA_InitializationInProgressP() as
follows:
static inline
boolean DAC960_BA_InitializationInProgressP(void *ControllerBaseAddress)
{
unsigned long InboundDoorBellRegister =
le32_to_cpu(ControllerBaseAddress +
DAC960_BA_InboundDoorBellRegisterOffset);
return (boolean) !(InboundDoorBellRegister & 0x0002);
}
Is it correct ?
No, you must still use readb(), not a direct memory access.
Furthermore readw() and readl() do byteswapping theirselves on big-endian
architectures.
For this case le32_to_cpu() is wrong too, because it's meant for 32-bit
access while the original code used readb(), which does an 8 bit-access.
And 8-bit accesses don't have to be swapped.
Their is another issue with bitfields, though: their definition is reversed
for little and big endian machines. So you need two different struct
definitions, depending on the endianness.
And where can I find the cpu_to_le32() and le32_to_cpu() declaration or
a guide on how to use them ?
include/linux/byteorder/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/
It would also be nice if you would read the Linux kernel coding
standards
document. Upper/lower case and thirty character object names are
not appreciated.
-- Dan
** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/