[U-Boot] [PATCH 2/4] net: tweak eth_device layout to simplify enetaddr use

[Andy Fleming]

On Fri, Nov 11, 2011 at 9:03 AM, Mike Frysinger <vapier at 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