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

[Mike Frysinger]

On Friday 11 November 2011 10:44:45 Andy Fleming wrote:
> 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.

so you'd use:
	addrhi = cpu_to_le32(dev->enetaddr32);
sounds simple to me

> Plus, I have to say, accessing a variable via the second array entry
> (enetaddr16[2]) is a bit convoluted.

it is.  i couldn't quite figure out how to do an anonymous struct though so the 
eneaddr16 represented just the last set of 16bits.

> 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]));

casting like this is ugly, and enetaddr is uchar*, so there is no alignment 
guarantee in the type.  which is why i'm suggesting people do:
	addrhi = __get_unaligned_le32(&dev->enetaddr[0]);
	addrlo = __get_unaligned_le32(&dev->enetaddr[4]);

so this improves things for some arches while keeping the code safe for all.  
but if we have enetaddr32, it'd be even better.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111111/65aa47a4/attachment.pgp