[U-Boot] [PATCH 1/4] net/miiphy/serial: drop duplicate "NAMESIZE" define
A few subsystems are using the same define "NAMESIZE". This has been working so far because they define it to the same number. However, I want to change the size of eth_device's NAMESIZE, so rather than tweak the define names, simply drop references to it. Almost no one does, and the handful that do can easily be changed to a sizeof().
Signed-off-by: Mike Frysinger vapier@gentoo.org --- board/Marvell/db64360/mv_eth.c | 2 +- board/Marvell/db64460/mv_eth.c | 2 +- board/esd/cpci750/mv_eth.c | 2 +- board/evb64260/eth.c | 2 +- board/prodrive/p3mx/mv_eth.c | 2 +- drivers/net/armada100_fec.c | 2 +- drivers/net/mvgbe.c | 2 +- drivers/qe/uec_phy.c | 2 +- include/miiphy.h | 2 +- include/net.h | 4 +--- include/serial.h | 5 ++--- net/eth.c | 2 +- 12 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/board/Marvell/db64360/mv_eth.c b/board/Marvell/db64360/mv_eth.c index 30304b0..82e08a1 100644 --- a/board/Marvell/db64360/mv_eth.c +++ b/board/Marvell/db64360/mv_eth.c @@ -221,7 +221,7 @@ void mv6436x_eth_initialize (bd_t * bis) return; }
- /* must be less than NAMESIZE (16) */ + /* must be less than sizeof(dev->name) */ sprintf (dev->name, "mv_enet%d", devnum);
#ifdef DEBUG diff --git a/board/Marvell/db64460/mv_eth.c b/board/Marvell/db64460/mv_eth.c index cd9d5a4..e20ca20 100644 --- a/board/Marvell/db64460/mv_eth.c +++ b/board/Marvell/db64460/mv_eth.c @@ -221,7 +221,7 @@ void mv6446x_eth_initialize (bd_t * bis) return; }
- /* must be less than NAMESIZE (16) */ + /* must be less than sizeof(dev->name) */ sprintf (dev->name, "mv_enet%d", devnum);
#ifdef DEBUG diff --git a/board/esd/cpci750/mv_eth.c b/board/esd/cpci750/mv_eth.c index 781ad23..06ae6ff 100644 --- a/board/esd/cpci750/mv_eth.c +++ b/board/esd/cpci750/mv_eth.c @@ -221,7 +221,7 @@ void mv6436x_eth_initialize (bd_t * bis) return; }
- /* must be less than NAMESIZE (16) */ + /* must be less than sizeof(dev->name) */ sprintf (dev->name, "mv_enet%d", devnum);
#ifdef DEBUG diff --git a/board/evb64260/eth.c b/board/evb64260/eth.c index 1492ffc..e96b0f4 100644 --- a/board/evb64260/eth.c +++ b/board/evb64260/eth.c @@ -685,7 +685,7 @@ gt6426x_eth_initialize(bd_t *bis) return; }
- /* must be less than NAMESIZE (16) */ + /* must be less than sizeof(dev->name) */ sprintf(dev->name, "gal_enet%d", devnum);
#ifdef DEBUG diff --git a/board/prodrive/p3mx/mv_eth.c b/board/prodrive/p3mx/mv_eth.c index 15b3bfc..9e3213b 100644 --- a/board/prodrive/p3mx/mv_eth.c +++ b/board/prodrive/p3mx/mv_eth.c @@ -274,7 +274,7 @@ void mv6446x_eth_initialize (bd_t * bis) return; }
- /* must be less than NAMESIZE (16) */ + /* must be less than sizeof(dev->name) */ sprintf (dev->name, "mv_enet%d", devnum);
#ifdef DEBUG diff --git a/drivers/net/armada100_fec.c b/drivers/net/armada100_fec.c index fbf9763..0a45f17 100644 --- a/drivers/net/armada100_fec.c +++ b/drivers/net/armada100_fec.c @@ -709,7 +709,7 @@ int armada100_fec_register(unsigned long base_addr) /* Assign ARMADA100 Fast Ethernet Controller Base Address */ darmdfec->regs = (void *)base_addr;
- /* must be less than NAMESIZE (16) */ + /* must be less than sizeof(dev->name) */ strcpy(dev->name, "armd-fec0");
dev->init = armdfec_init; diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c index fd13428..9ce5f6b 100644 --- a/drivers/net/mvgbe.c +++ b/drivers/net/mvgbe.c @@ -700,7 +700,7 @@ error1:
dev = &dmvgbe->dev;
- /* must be less than NAMESIZE (16) */ + /* must be less than sizeof(dev->name) */ sprintf(dev->name, "egiga%d", devnum);
/* Extract the MAC address from the environment */ diff --git a/drivers/qe/uec_phy.c b/drivers/qe/uec_phy.c index e26218b..ac580a0 100644 --- a/drivers/qe/uec_phy.c +++ b/drivers/qe/uec_phy.c @@ -85,7 +85,7 @@ #endif
struct fixed_phy_port { - char name[NAMESIZE]; /* ethernet port name */ + char name[16]; /* ethernet port name */ unsigned int speed; /* specified speed 10,100 or 1000 */ unsigned int duplex; /* specified duplex FULL or HALF */ }; diff --git a/include/miiphy.h b/include/miiphy.h index 7e70cf8..ca5040e 100644 --- a/include/miiphy.h +++ b/include/miiphy.h @@ -86,7 +86,7 @@ void mdio_list_devices(void); #define BB_MII_DEVNAME "bb_miiphy"
struct bb_miiphy_bus { - char name[NAMESIZE]; + char name[16]; int (*init)(struct bb_miiphy_bus *bus); int (*mdio_active)(struct bb_miiphy_bus *bus); int (*mdio_tristate)(struct bb_miiphy_bus *bus); diff --git a/include/net.h b/include/net.h index ad9afbf..b4acd8f 100644 --- a/include/net.h +++ b/include/net.h @@ -66,8 +66,6 @@ typedef void rxhand_icmp_f(unsigned type, unsigned code, unsigned dport, */ typedef void thand_f(void);
-#define NAMESIZE 16 - enum eth_state_t { ETH_STATE_INIT, ETH_STATE_PASSIVE, @@ -75,7 +73,7 @@ enum eth_state_t { };
struct eth_device { - char name[NAMESIZE]; + char name[16]; unsigned char enetaddr[6]; int iobase; int state; diff --git a/include/serial.h b/include/serial.h index 5926244..2b9e647 100644 --- a/include/serial.h +++ b/include/serial.h @@ -3,10 +3,9 @@
#include <post.h>
-#define NAMESIZE 16 - struct serial_device { - char name[NAMESIZE]; + /* enough bytes to match alignment of following func pointer */ + char name[16];
int (*init) (void); int (*uninit) (void); diff --git a/net/eth.c b/net/eth.c index 4280d6d..cdc3234 100644 --- a/net/eth.c +++ b/net/eth.c @@ -220,7 +220,7 @@ int eth_register(struct eth_device *dev) { struct eth_device *d;
- assert(strlen(dev->name) < NAMESIZE); + assert(strlen(dev->name) < sizeof(dev->name));
if (!eth_devices) { eth_current = eth_devices = dev;
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
Now device driver code can simply use "dev->enetaddr32" and "dev->enetaddr16[2]" to access the values without having to manually shift the bytes out of dev->enetaddr.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- include/net.h | 13 +++++++++++-- 1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/net.h b/include/net.h index b4acd8f..e8c12d9 100644 --- a/include/net.h +++ b/include/net.h @@ -73,8 +73,17 @@ enum eth_state_t { };
struct eth_device { - char name[16]; - unsigned char enetaddr[6]; + /* Keep enetaddr at start so it is guaranteed aligned */ + union { + u32 enetaddr32; + u16 enetaddr16[3]; + unsigned char enetaddr[6]; + }; + /* + * Note: name size is picked to fill the holes in memory after + * enetaddr, and to match up to alignment for following "int". + */ + char name[18]; int iobase; int state;
Hello Mike,
- union {
u32 enetaddr32;u16 enetaddr16[3];unsigned char enetaddr[6];- };
This will work only as long the endianess is matching.
Picking single chars from enetaddr[] and combine them to a u32 register will be more independent from endianess.
If this goes in, I would like to see at least a comment about the problem.
Best Regards, Thomas
Dear Mike Frysinger,
In message 1320970267-22297-2-git-send-email-vapier@gentoo.org you 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.
Best regards,
Wolfgang Denk
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. -mike
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
On Friday 11 November 2011 10:44:45 Andy Fleming wrote:
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.
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
Dear Mike Frysinger,
In message 201111111003.15436.vapier@gentoo.org you wrote:
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
No. Please drop this.
want. looking at the generated code shows really nice improvements: a single cpu load instead of 4 loads interspersed with bitshifts.
This is neither memory nor performance critical, but it is easy to misuse and thus dangerous.
Best regards,
Wolfgang Denk
Simplifies the code nicely.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- drivers/net/bfin_mac.c | 12 ++---------- 1 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c index dcc781a..ab2fe71 100644 --- a/drivers/net/bfin_mac.c +++ b/drivers/net/bfin_mac.c @@ -281,16 +281,8 @@ static int bfin_miiphy_init(struct eth_device *dev, int *opmode)
static int bfin_EMAC_setup_addr(struct eth_device *dev) { - bfin_write_EMAC_ADDRLO( - dev->enetaddr[0] | - dev->enetaddr[1] << 8 | - dev->enetaddr[2] << 16 | - dev->enetaddr[3] << 24 - ); - bfin_write_EMAC_ADDRHI( - dev->enetaddr[4] | - dev->enetaddr[5] << 8 - ); + bfin_write_EMAC_ADDRLO(dev->enetaddr32); + bfin_write_EMAC_ADDRHI(dev->enetaddr16[2]); return 0; }
The previous commit (79ad54400932d6484178a GCC4.6: Squash warnings in smsc95xx.c) broke loading of enetaddr into the hardware. It changed the semantics from reading 4 bytes to 1 byte.
Use the new enetaddr members instead to load the values out of memory. I don't have any hardware to test with, but it *looks* correct, and we know the current code is def broken ...
Signed-off-by: Mike Frysinger vapier@gentoo.org --- drivers/usb/eth/smsc95xx.c | 12 +++--------- 1 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c index 7ee4f87..f43b4b5 100644 --- a/drivers/usb/eth/smsc95xx.c +++ b/drivers/usb/eth/smsc95xx.c @@ -372,26 +372,20 @@ static int smsc95xx_init_mac_address(struct eth_device *eth, static int smsc95xx_write_hwaddr(struct eth_device *eth) { struct ueth_data *dev = (struct ueth_data *)eth->priv; - u32 addr_lo, addr_hi; int ret;
/* set hardware address */ debug("** %s()\n", __func__); - addr_lo = cpu_to_le32(*eth->enetaddr); - addr_hi = cpu_to_le16(*((u16 *)(eth->enetaddr + 4))); - ret = smsc95xx_write_reg(dev, ADDRL, addr_lo); + ret = smsc95xx_write_reg(dev, ADDRL, eth->enetaddr32); if (ret < 0) { debug("Failed to write ADDRL: %d\n", ret); return ret; }
- ret = smsc95xx_write_reg(dev, ADDRH, addr_hi); + ret = smsc95xx_write_reg(dev, ADDRH, eth->enetaddr16[2]); if (ret < 0) return ret; - debug("MAC %02x:%02x:%02x:%02x:%02x:%02x\n", - eth->enetaddr[0], eth->enetaddr[1], - eth->enetaddr[2], eth->enetaddr[3], - eth->enetaddr[4], eth->enetaddr[5]); + debug("MAC %pM\n", eth->enetaddr); dev->have_hwaddr = 1; return 0; }
On Thu, Nov 10, 2011 at 6:11 PM, Mike Frysinger vapier@gentoo.org wrote:
diff --git a/include/net.h b/include/net.h index ad9afbf..b4acd8f 100644 --- a/include/net.h +++ b/include/net.h @@ -66,8 +66,6 @@ typedef void rxhand_icmp_f(unsigned type, unsigned code, unsigned dport, */ typedef void thand_f(void);
-#define NAMESIZE 16
enum eth_state_t { ETH_STATE_INIT, ETH_STATE_PASSIVE, @@ -75,7 +73,7 @@ enum eth_state_t { };
struct eth_device {
- char name[NAMESIZE];
- char name[16];
I like all of the earlier NAMESIZE->sizeof changes, but I'm not as comfortable with changing the various name declarations. Seems like we might want named constants for them, just so any dependencies can be clearly-documented. For instance, in Linux the MDIO bus specifiers are designed to be bigger than the MDIO device name, and the size declarations reflect this.
Andy
On Friday 11 November 2011 17:07:46 Andy Fleming wrote:
On Thu, Nov 10, 2011 at 6:11 PM, Mike Frysinger vapier@gentoo.org wrote:
diff --git a/include/net.h b/include/net.h index ad9afbf..b4acd8f 100644 --- a/include/net.h +++ b/include/net.h @@ -66,8 +66,6 @@ typedef void rxhand_icmp_f(unsigned type, unsigned code, unsigned dport, */ typedef void thand_f(void);
-#define NAMESIZE 16
enum eth_state_t { ETH_STATE_INIT, ETH_STATE_PASSIVE, @@ -75,7 +73,7 @@ enum eth_state_t { };
struct eth_device {
char name[NAMESIZE];
char name[16];I like all of the earlier NAMESIZE->sizeof changes, but I'm not as comfortable with changing the various name declarations. Seems like we might want named constants for them, just so any dependencies can be clearly-documented. For instance, in Linux the MDIO bus specifiers are designed to be bigger than the MDIO device name, and the size declarations reflect this.
sounds like something that can easily be expressed with: BUILD_BUG_ON(sizeof(dev->name) < sizeof(mii->name)); -mike
Dear Mike Frysinger,
In message 1320970267-22297-1-git-send-email-vapier@gentoo.org you wrote:
A few subsystems are using the same define "NAMESIZE". This has been working so far because they define it to the same number. However, I want to change the size of eth_device's NAMESIZE, so rather than tweak the define names, simply drop references to it. Almost no one does, and the handful that do can easily be changed to a sizeof().
Signed-off-by: Mike Frysinger vapier@gentoo.org
board/Marvell/db64360/mv_eth.c | 2 +- board/Marvell/db64460/mv_eth.c | 2 +- board/esd/cpci750/mv_eth.c | 2 +- board/evb64260/eth.c | 2 +- board/prodrive/p3mx/mv_eth.c | 2 +- drivers/net/armada100_fec.c | 2 +- drivers/net/mvgbe.c | 2 +- drivers/qe/uec_phy.c | 2 +- include/miiphy.h | 2 +- include/net.h | 4 +--- include/serial.h | 5 ++--- net/eth.c | 2 +- 12 files changed, 13 insertions(+), 16 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
participants (4)
-
Andy Fleming -
Mike Frysinger -
thomas.langer＠lantiq.com -
Wolfgang Denk