On Fri, Nov 11, 2011 at 9:03 AM, Mike Frysinger vapier@gentoo.org wrote:
On Friday 11 November 2011 06:55:45 Wolfgang Denk wrote:
Mike Frysinger wrote:
The current eth_device leaves a 2 byte hole after "enetaddr" and before "iobase". Since the enetaddr member has to be 6 bytes, we might as well fill that 2 byte hole with something useful.
Further, most device drivers want to load enetaddr from memory into the hardware as 1 32bit value and 1 16bit value.
So re-arrange the structure slightly, and add an anonymous union to make
eth_device even better: - expand the name field to fill the 2 byte hole - make sure enetaddr is aligned, and provides 32bit/16bit members
I'm OK with expanding the name[] field, but as Thomas pointed out, providing "convenient" u32 / u16 variables for the MAC address is actually a faux ami that misleads people into using these variables without thinking about endianess and such.
Please omit this part.
there's always the endian issue. i'd wager that the vast majority of the time, the endian of the target hardware is the same as the core. so omitting this for odd devices or device driver writers who aren't careful is being too pessimistic imo. i can add qualifiers to the name like "enetaddr_cpu32" if you want. looking at the generated code shows really nice improvements: a single cpu load instead of 4 loads interspersed with bitshifts.
All of the Freescale ethernet devices have their MAC address register in "little-endian" mode. I have no idea why. But this means that the change would still require shifts, but not it would also require masking.
Plus, I have to say, accessing a variable via the second array entry (enetaddr16[2]) is a bit convoluted. If you want your driver to pull in the address in two loads, and you want C code which indicates that level of explicit awareness of the layout of a structure, then you might as well:
addrhi = *((u32 *)(dev->enetaddr)); addrlo = *((u16 *)(&dev->enetaddr[4]));
But I'm pretty sure the TSEC, UCC, and FM drivers will have to continue doing byte-by-byte stuff.
Andy