On Fri, Nov 11, 2011 at 9:03 AM, Mike Frysinger <vap...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
