[U-Boot] [PATCH 0/9] net: tsec: Driver portability fixes and cleanup
Though the first 3 patches fix tsec's driver mcast() instance primarily, the fix from net.h was propagated to other drivers too, as appropriate. Notably, these fixes also end up in removal of some unwanted global static vars from the tsec driver. The rest of the patches are driver portability fixes, so that the tsec driver may work on little endian architectures as well. The issues uncovered by the sparse tool where also addressed in the process. All checkpatch issues were addressed, including those on existing code. Only a couple of CamelCase warnings remain, coming from the net stack code (see NetReceive(), NetRxPackets[]).
Claudiu Manoil (9): net: Fix mcast function pointer prototype net: tsec: Fix and cleanup tsec_mcast_addr() net: tsec: Fix priv pointer in tsec_mcast_addr() net: tsec: Cleanup tsec regs init and fix __iomem warns net: fsl_mdio: Fix warnings for __iomem pointers net: tsec: Fix CamelCase issues around BD code net: tsec: Use portable types and accessors for BDs net: tsec: Use portable regs type (uint->u32) net: tsec: Fix mac addr setup portability, cleanup
drivers/net/fsl_mdio.c | 17 ++- drivers/net/rtl8139.c | 2 +- drivers/net/tsec.c | 200 ++++++++++++++-------------- include/fsl_mdio.h | 8 +- include/net.h | 2 +- include/tsec.h | 345 +++++++++++++++++++++++++------------------------ 6 files changed, 296 insertions(+), 278 deletions(-)
This fixes the following compiler warnings when activating CONFIG_MCAST_TFTP:
tsec.c: In function 'tsec_mcast_addr': tsec.c:130:2: warning: passing argument 2 of 'ether_crc' makes pointer from integer without a cast [enabled by default] In file included from /work/u-boot-net/include/common.h:874:0, from tsec.c:15: /work/u-boot-net/include/net.h:189:5: note: expected 'const unsigned char *' but argument is of type 'u8' tsec.c: In function 'tsec_initialize': tsec.c:646:13: warning: assignment from incompatible pointer type [enabled by default] eth.c: In function 'eth_mcast_join': eth.c:358:2: warning: passing argument 2 of 'eth_current->mcast' makes integer from pointer without a cast [enabled by default] eth.c:358:2: note: expected 'u32' but argument is of type 'u8 *'
In the eth_mcast_join() implementation, eth_current->mcast() takes a u8 pointer to the multicast mac address and not a ip address value as implied by its prototype.
Fix parameter type mismatch for tsec_macst_addr() (tsec.c): ether_crc() takes a u8 pointer not a u8 value. mcast() is given a u8 pointer to the multicats mac address. Update parameter type for the rest of mcast() instances.
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- drivers/net/rtl8139.c | 2 +- drivers/net/tsec.c | 4 ++-- include/net.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/rtl8139.c b/drivers/net/rtl8139.c index 4186699..208ce5c 100644 --- a/drivers/net/rtl8139.c +++ b/drivers/net/rtl8139.c @@ -188,7 +188,7 @@ static int rtl_transmit(struct eth_device *dev, void *packet, int length); static int rtl_poll(struct eth_device *dev); static void rtl_disable(struct eth_device *dev); #ifdef CONFIG_MCAST_TFTP/* This driver already accepts all b/mcast */ -static int rtl_bcast_addr (struct eth_device *dev, u8 bcast_mac, u8 set) +static int rtl_bcast_addr(struct eth_device *dev, const u8 *bcast_mac, u8 set) { return (0); } diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index f5e314b..3428dd0 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -120,14 +120,14 @@ static void tsec_configure_serdes(struct tsec_private *priv) * for PowerPC (tm) is usually the case) in the tregister holds * the entry. */ static int -tsec_mcast_addr (struct eth_device *dev, u8 mcast_mac, u8 set) +tsec_mcast_addr(struct eth_device *dev, const u8 *mcast_mac, u8 set) { struct tsec_private *priv = privlist[1]; volatile tsec_t *regs = priv->regs; volatile u32 *reg_array, value; u8 result, whichbit, whichreg;
- result = (u8)((ether_crc(MAC_ADDR_LEN,mcast_mac) >> 24) & 0xff); + result = (u8)((ether_crc(MAC_ADDR_LEN, mcast_mac) >> 24) & 0xff); whichbit = result & 0x1f; /* the 5 LSB = which bit to set */ whichreg = result >> 5; /* the 3 MSB = which reg to set it in */ value = (1 << (31-whichbit)); diff --git a/include/net.h b/include/net.h index 5aedc17..0802fad 100644 --- a/include/net.h +++ b/include/net.h @@ -89,7 +89,7 @@ struct eth_device { int (*recv) (struct eth_device *); void (*halt) (struct eth_device *); #ifdef CONFIG_MCAST_TFTP - int (*mcast) (struct eth_device *, u32 ip, u8 set); + int (*mcast) (struct eth_device *, const u8 *enetaddr, u8 set); #endif int (*write_hwaddr) (struct eth_device *); struct eth_device *next;
There are several implementation issues for tsec_mcast_addr() addressed by this patch: * unmanaged, not portable r/w access to registers; fixed with setbits_be32()/ clrbits_be32() * use of volatile pointers * unnecessary forced cast to u8 for the ether_crc() result * removed redundant parens * corrected some comment slips
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- drivers/net/tsec.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 3428dd0..9ffc801 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -113,32 +113,31 @@ static void tsec_configure_serdes(struct tsec_private *priv) * result. * 2) Use the 8 most significant bits as a hash into a 256-entry * table. The table is controlled through 8 32-bit registers: - * gaddr0-7. gaddr0's MSB is entry 0, and gaddr7's LSB is - * gaddr7. This means that the 3 most significant bits in the + * gaddr0-7. gaddr0's MSB is entry 0, and gaddr7's LSB is entry + * 255. This means that the 3 most significant bits in the * hash index which gaddr register to use, and the 5 other bits * indicate which bit (assuming an IBM numbering scheme, which - * for PowerPC (tm) is usually the case) in the tregister holds + * for PowerPC (tm) is usually the case) in the register holds * the entry. */ static int tsec_mcast_addr(struct eth_device *dev, const u8 *mcast_mac, u8 set) { struct tsec_private *priv = privlist[1]; - volatile tsec_t *regs = priv->regs; - volatile u32 *reg_array, value; - u8 result, whichbit, whichreg; + struct tsec __iomem *regs = priv->regs; + u32 result, value; + u8 whichbit, whichreg;
- result = (u8)((ether_crc(MAC_ADDR_LEN, mcast_mac) >> 24) & 0xff); - whichbit = result & 0x1f; /* the 5 LSB = which bit to set */ - whichreg = result >> 5; /* the 3 MSB = which reg to set it in */ - value = (1 << (31-whichbit)); + result = ether_crc(MAC_ADDR_LEN, mcast_mac); + whichbit = (result >> 24) & 0x1f; /* the 5 LSB = which bit to set */ + whichreg = result >> 29; /* the 3 MSB = which reg to set it in */
- reg_array = &(regs->hash.gaddr0); + value = 1 << (31-whichbit); + + if (set) + setbits_be32(®s->hash.gaddr0 + whichreg, value); + else + clrbits_be32(®s->hash.gaddr0 + whichreg, value);
- if (set) { - reg_array[whichreg] |= value; - } else { - reg_array[whichreg] &= ~value; - } return 0; } #endif /* Multicast TFTP ? */
Access to privlist[1] (hardcoded referece to the 2nd tsec's priv area) is neither correct nor does it make sense in the current context. Each tsec dev has access to its own priv instance only, and hence to its own set of group address registers (GADDR) to filter multicast addresses.
This fix leads to removal of the unused (faulty) privlist[] and related global static vars. Note that mcast() can be called only after eth_device allocation and init, and hence after priv area allocation, so dev->priv is correctly initialized upon mcast() call.
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- drivers/net/tsec.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 9ffc801..9371ffa 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -33,11 +33,6 @@ typedef volatile struct rtxbd { rxbd8_t rxbd[PKTBUFSRX]; } RTXBD;
-#define MAXCONTROLLERS (8) - -static struct tsec_private *privlist[MAXCONTROLLERS]; -static int num_tsecs = 0; - #ifdef __GNUC__ static RTXBD rtx __attribute__ ((aligned(8))); #else @@ -122,7 +117,7 @@ static void tsec_configure_serdes(struct tsec_private *priv) static int tsec_mcast_addr(struct eth_device *dev, const u8 *mcast_mac, u8 set) { - struct tsec_private *priv = privlist[1]; + struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs; u32 result, value; u8 whichbit, whichreg; @@ -625,7 +620,6 @@ static int tsec_initialize(bd_t *bis, struct tsec_info_struct *tsec_info) if (NULL == priv) return 0;
- privlist[num_tsecs++] = priv; priv->regs = tsec_info->regs; priv->phyregs_sgmii = tsec_info->miiregs_sgmii;
Remove tsec_t typedef. Define macros as getters of tsec and mdio register memory regions, for consistent initialization of various 'regs' fields and also to manage overly long initialization lines. Use the __iomem address space marker to address sparse warnings in tsec.c where IO accessors are used, like:
tsec.c:394:19: warning: incorrect type in argument 1 (different address spaces) tsec.c:394:19: expected unsigned int volatile [noderef] asn:2*addr tsec.c:394:19: got unsigned int *<noident> [...]
Add the __iomem address space marker for the tsec pointers to struct tsec_mii_mng memory mapped register regions. This solves the sparse warnings for mixig normal pointers with __iomem pointers for tsec.
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- drivers/net/tsec.c | 26 +++++++++++++------------- include/tsec.h | 39 +++++++++++++++++++++++++-------------- 2 files changed, 38 insertions(+), 27 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 9371ffa..adb6c12 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -5,7 +5,7 @@ * terms of the GNU Public License, Version 2, incorporated * herein by reference. * - * Copyright 2004-2011 Freescale Semiconductor, Inc. + * Copyright 2004-2011, 2013 Freescale Semiconductor, Inc. * (C) Copyright 2003, Motorola, Inc. * author Andy Fleming * @@ -52,7 +52,7 @@ static struct tsec_info_struct tsec_info[] = { #endif #ifdef CONFIG_MPC85XX_FEC { - .regs = (tsec_t *)(TSEC_BASE_ADDR + 0x2000), + .regs = TSEC_GET_REGS(2, 0x2000), .devname = CONFIG_MPC85XX_FEC_NAME, .phyaddr = FEC_PHY_ADDR, .flags = FEC_FLAGS, @@ -141,7 +141,7 @@ tsec_mcast_addr(struct eth_device *dev, const u8 *mcast_mac, u8 set) * those we don't care about (unless zero is bad, in which case, * choose a more appropriate value) */ -static void init_registers(tsec_t *regs) +static void init_registers(struct tsec __iomem *regs) { /* Clear IEVENT */ out_be32(®s->ievent, IEVENT_INIT_CLEAR); @@ -188,7 +188,7 @@ static void init_registers(tsec_t *regs) */ static void adjust_link(struct tsec_private *priv, struct phy_device *phydev) { - tsec_t *regs = priv->regs; + struct tsec __iomem *regs = priv->regs; u32 ecntrl, maccfg2;
if (!phydev->link) { @@ -242,7 +242,7 @@ static void adjust_link(struct tsec_private *priv, struct phy_device *phydev) void redundant_init(struct eth_device *dev) { struct tsec_private *priv = dev->priv; - tsec_t *regs = priv->regs; + struct tsec __iomem *regs = priv->regs; uint t, count = 0; int fail = 1; static const u8 pkt[] = { @@ -321,7 +321,7 @@ static void startup_tsec(struct eth_device *dev) { int i; struct tsec_private *priv = (struct tsec_private *)dev->priv; - tsec_t *regs = priv->regs; + struct tsec __iomem *regs = priv->regs;
/* reset the indices to zero */ rxIdx = 0; @@ -375,7 +375,7 @@ static int tsec_send(struct eth_device *dev, void *packet, int length) int i; int result = 0; struct tsec_private *priv = (struct tsec_private *)dev->priv; - tsec_t *regs = priv->regs; + struct tsec __iomem *regs = priv->regs;
/* Find an empty buffer descriptor */ for (i = 0; rtx.txbd[txIdx].status & TXBD_READY; i++) { @@ -411,7 +411,7 @@ static int tsec_recv(struct eth_device *dev) { int length; struct tsec_private *priv = (struct tsec_private *)dev->priv; - tsec_t *regs = priv->regs; + struct tsec __iomem *regs = priv->regs;
while (!(rtx.rxbd[rxIdx].status & RXBD_EMPTY)) {
@@ -447,7 +447,7 @@ static int tsec_recv(struct eth_device *dev) static void tsec_halt(struct eth_device *dev) { struct tsec_private *priv = (struct tsec_private *)dev->priv; - tsec_t *regs = priv->regs; + struct tsec __iomem *regs = priv->regs;
clrbits_be32(®s->dmactrl, DMACTRL_GRS | DMACTRL_GTS); setbits_be32(®s->dmactrl, DMACTRL_GRS | DMACTRL_GTS); @@ -473,7 +473,7 @@ static int tsec_init(struct eth_device *dev, bd_t * bd) char tmpbuf[MAC_ADDR_LEN]; int i; struct tsec_private *priv = (struct tsec_private *)dev->priv; - tsec_t *regs = priv->regs; + struct tsec __iomem *regs = priv->regs; int ret;
/* Make sure the controller is stopped */ @@ -521,7 +521,7 @@ static int tsec_init(struct eth_device *dev, bd_t * bd)
static phy_interface_t tsec_get_interface(struct tsec_private *priv) { - tsec_t *regs = priv->regs; + struct tsec __iomem *regs = priv->regs; u32 ecntrl;
ecntrl = in_be32(®s->ecntrl); @@ -570,7 +570,7 @@ static int init_phy(struct eth_device *dev) { struct tsec_private *priv = (struct tsec_private *)dev->priv; struct phy_device *phydev; - tsec_t *regs = priv->regs; + struct tsec __iomem *regs = priv->regs; u32 supported = (SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full | SUPPORTED_100baseT_Half | @@ -677,7 +677,7 @@ int tsec_standard_init(bd_t *bis) { struct fsl_pq_mdio_info info;
- info.regs = (struct tsec_mii_mng *)CONFIG_SYS_MDIO_BASE_ADDR; + info.regs = TSEC_GET_MDIO_REGS_BASE(1); info.name = DEFAULT_MII_NAME;
fsl_pq_mdio_init(bis, &info); diff --git a/include/tsec.h b/include/tsec.h index f0f3d4d..c30aafb 100644 --- a/include/tsec.h +++ b/include/tsec.h @@ -7,7 +7,7 @@ * terms of the GNU Public License, Version 2, incorporated * herein by reference. * - * Copyright 2004, 2007, 2009, 2011 Freescale Semiconductor, Inc. + * Copyright 2004, 2007, 2009, 2011, 2013 Freescale Semiconductor, Inc. * (C) Copyright 2003, Motorola, Inc. * maintained by Xianghua Xiao (x.xiao@motorola.com) * author Andy Fleming @@ -27,13 +27,26 @@
#define CONFIG_SYS_MDIO_BASE_ADDR (MDIO_BASE_ADDR + 0x520)
+#define TSEC_GET_REGS(num, offset) \ + (struct tsec __iomem *)\ + (TSEC_BASE_ADDR + (((num) - 1) * (offset))) + +#define TSEC_GET_REGS_BASE(num) \ + TSEC_GET_REGS((num), TSEC_SIZE) + +#define TSEC_GET_MDIO_REGS(num, offset) \ + (struct tsec_mii_mng __iomem *)\ + (CONFIG_SYS_MDIO_BASE_ADDR + ((num) - 1) * (offset)) + +#define TSEC_GET_MDIO_REGS_BASE(num) \ + TSEC_GET_MDIO_REGS((num), TSEC_MDIO_OFFSET) + #define DEFAULT_MII_NAME "FSL_MDIO"
#define STD_TSEC_INFO(num) \ { \ - .regs = (tsec_t *)(TSEC_BASE_ADDR + ((num - 1) * TSEC_SIZE)), \ - .miiregs_sgmii = (struct tsec_mii_mng *)(CONFIG_SYS_MDIO_BASE_ADDR \ - + (num - 1) * TSEC_MDIO_OFFSET), \ + .regs = TSEC_GET_REGS_BASE(num), \ + .miiregs_sgmii = TSEC_GET_MDIO_REGS_BASE(num), \ .devname = CONFIG_TSEC##num##_NAME, \ .phyaddr = TSEC##num##_PHY_ADDR, \ .flags = TSEC##num##_FLAGS, \ @@ -42,9 +55,8 @@
#define SET_STD_TSEC_INFO(x, num) \ { \ - x.regs = (tsec_t *)(TSEC_BASE_ADDR + ((num - 1) * TSEC_SIZE)); \ - x.miiregs_sgmii = (struct tsec_mii_mng *)(CONFIG_SYS_MDIO_BASE_ADDR \ - + (num - 1) * TSEC_MDIO_OFFSET); \ + x.regs = TSEC_GET_REGS_BASE(num); \ + x.miiregs_sgmii = TSEC_GET_MDIO_REGS_BASE(num); \ x.devname = CONFIG_TSEC##num##_NAME; \ x.phyaddr = TSEC##num##_PHY_ADDR; \ x.flags = TSEC##num##_FLAGS;\ @@ -281,8 +293,7 @@ typedef struct tsec_hash_regs uint res2[24]; } tsec_hash_t;
-typedef struct tsec -{ +struct tsec { /* General Control and Status Registers (0x2_n000) */ uint res000[4];
@@ -374,7 +385,7 @@ typedef struct tsec
/* TSEC Future Expansion Space (0x2_nc00-0x2_nffc) */ uint resc00[256]; -} tsec_t; +};
#define TSEC_GIGABIT (1 << 0)
@@ -383,8 +394,8 @@ typedef struct tsec #define TSEC_SGMII (1 << 2) /* MAC-PHY interface uses SGMII */
struct tsec_private { - tsec_t *regs; - struct tsec_mii_mng *phyregs_sgmii; + struct tsec __iomem *regs; + struct tsec_mii_mng __iomem *phyregs_sgmii; struct phy_device *phydev; phy_interface_t interface; struct mii_dev *bus; @@ -394,8 +405,8 @@ struct tsec_private { };
struct tsec_info_struct { - tsec_t *regs; - struct tsec_mii_mng *miiregs_sgmii; + struct tsec __iomem *regs; + struct tsec_mii_mng __iomem *miiregs_sgmii; char *devname; char *mii_devname; phy_interface_t interface;
Add the __iomem address space marker for the tsec pointers to struct tsec_mii_mng memory mapped register regions. This solves the sparse warnings for mixig normal pointers with __iomem pointers for tsec. E.g.:
fsl_mdio.c:34:19: warning: incorrect type in argument 1 (different address spaces) fsl_mdio.c:34:19: expected unsigned int volatile [noderef] asn:2*addr fsl_mdio.c:34:19: got unsigned int *<noident> [...]
tsec.c:91:35: warning: incorrect type in argument 1 (different address spaces) tsec.c:91:35: expected struct tsec_mii_mng *phyregs tsec.c:91:35: got struct tsec_mii_mng [noderef] asn:2*phyregs_sgmii [...]
tsec.c:680:19: warning: incorrect type in assignment (different address spaces) tsec.c:680:19: expected struct tsec_mii_mng *regs tsec.c:680:19: got struct tsec_mii_mng [noderef] asn:2*<noident> [...]
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- drivers/net/fsl_mdio.c | 17 ++++++++++------- include/fsl_mdio.h | 8 ++++---- 2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/net/fsl_mdio.c b/drivers/net/fsl_mdio.c index ce36bd7..1d88e65 100644 --- a/drivers/net/fsl_mdio.c +++ b/drivers/net/fsl_mdio.c @@ -1,5 +1,5 @@ /* - * Copyright 2009-2010 Freescale Semiconductor, Inc. + * Copyright 2009-2010, 2013 Freescale Semiconductor, Inc. * Jun-jie Zhang b18070@freescale.com * Mingkai Hu Mingkai.hu@freescale.com * @@ -13,7 +13,7 @@ #include <asm/errno.h> #include <asm/fsl_enet.h>
-void tsec_local_mdio_write(struct tsec_mii_mng *phyregs, int port_addr, +void tsec_local_mdio_write(struct tsec_mii_mng __iomem *phyregs, int port_addr, int dev_addr, int regnum, int value) { int timeout = 1000000; @@ -26,7 +26,7 @@ void tsec_local_mdio_write(struct tsec_mii_mng *phyregs, int port_addr, ; }
-int tsec_local_mdio_read(struct tsec_mii_mng *phyregs, int port_addr, +int tsec_local_mdio_read(struct tsec_mii_mng __iomem *phyregs, int port_addr, int dev_addr, int regnum) { int value; @@ -57,7 +57,8 @@ int tsec_local_mdio_read(struct tsec_mii_mng *phyregs, int port_addr,
static int fsl_pq_mdio_reset(struct mii_dev *bus) { - struct tsec_mii_mng *regs = bus->priv; + struct tsec_mii_mng __iomem *regs = + (struct tsec_mii_mng __iomem *)bus->priv;
/* Reset MII (due to new addresses) */ out_be32(®s->miimcfg, MIIMCFG_RESET_MGMT); @@ -72,7 +73,8 @@ static int fsl_pq_mdio_reset(struct mii_dev *bus)
int tsec_phy_read(struct mii_dev *bus, int addr, int dev_addr, int regnum) { - struct tsec_mii_mng *phyregs = bus->priv; + struct tsec_mii_mng __iomem *phyregs = + (struct tsec_mii_mng __iomem *)bus->priv;
return tsec_local_mdio_read(phyregs, addr, dev_addr, regnum); } @@ -80,7 +82,8 @@ int tsec_phy_read(struct mii_dev *bus, int addr, int dev_addr, int regnum) int tsec_phy_write(struct mii_dev *bus, int addr, int dev_addr, int regnum, u16 value) { - struct tsec_mii_mng *phyregs = bus->priv; + struct tsec_mii_mng __iomem *phyregs = + (struct tsec_mii_mng __iomem *)bus->priv;
tsec_local_mdio_write(phyregs, addr, dev_addr, regnum, value);
@@ -101,7 +104,7 @@ int fsl_pq_mdio_init(bd_t *bis, struct fsl_pq_mdio_info *info) bus->reset = fsl_pq_mdio_reset; sprintf(bus->name, info->name);
- bus->priv = info->regs; + bus->priv = (void *)info->regs;
return mdio_register(bus); } diff --git a/include/fsl_mdio.h b/include/fsl_mdio.h index 9c0b762..b58713d 100644 --- a/include/fsl_mdio.h +++ b/include/fsl_mdio.h @@ -1,5 +1,5 @@ /* - * Copyright 2009-2012 Freescale Semiconductor, Inc. + * Copyright 2009-2012, 2013 Freescale Semiconductor, Inc. * Jun-jie Zhang b18070@freescale.com * Mingkai Hu Mingkai.hu@freescale.com * @@ -31,9 +31,9 @@ #define MIIMIND_BUSY 0x00000001 #define MIIMIND_NOTVALID 0x00000004
-void tsec_local_mdio_write(struct tsec_mii_mng *phyregs, int port_addr, +void tsec_local_mdio_write(struct tsec_mii_mng __iomem *phyregs, int port_addr, int dev_addr, int reg, int value); -int tsec_local_mdio_read(struct tsec_mii_mng *phyregs, int port_addr, +int tsec_local_mdio_read(struct tsec_mii_mng __iomem *phyregs, int port_addr, int dev_addr, int regnum); int tsec_phy_read(struct mii_dev *bus, int addr, int dev_addr, int regnum); int tsec_phy_write(struct mii_dev *bus, int addr, int dev_addr, int regnum, @@ -44,7 +44,7 @@ int memac_mdio_read(struct mii_dev *bus, int port_addr, int dev_addr, int regnum);
struct fsl_pq_mdio_info { - struct tsec_mii_mng *regs; + struct tsec_mii_mng __iomem *regs; char *name; }; int fsl_pq_mdio_init(bd_t *bis, struct fsl_pq_mdio_info *info);
Fix bufPtr and the rxIdx/ txIdx occurrences to solve the related checkpatch warnings for the coming patches.
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- drivers/net/tsec.c | 60 +++++++++++++++++++++++++++--------------------------- include/tsec.h | 4 ++-- 2 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index adb6c12..289229a 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -25,8 +25,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define TX_BUF_CNT 2
-static uint rxIdx; /* index of the current RX buffer */ -static uint txIdx; /* index of the current TX buffer */ +static uint rx_idx; /* index of the current RX buffer */ +static uint tx_idx; /* index of the current TX buffer */
typedef volatile struct rtxbd { txbd8_t txbd[TX_BUF_CNT]; @@ -278,20 +278,20 @@ void redundant_init(struct eth_device *dev) tsec_send(dev, (void *)pkt, sizeof(pkt));
/* Wait for buffer to be received */ - for (t = 0; rtx.rxbd[rxIdx].status & RXBD_EMPTY; t++) { + for (t = 0; rtx.rxbd[rx_idx].status & RXBD_EMPTY; t++) { if (t >= 10 * TOUT_LOOP) { printf("%s: tsec: rx error\n", dev->name); break; } }
- if (!memcmp(pkt, (void *)NetRxPackets[rxIdx], sizeof(pkt))) + if (!memcmp(pkt, (void *)NetRxPackets[rx_idx], sizeof(pkt))) fail = 0;
- rtx.rxbd[rxIdx].length = 0; - rtx.rxbd[rxIdx].status = - RXBD_EMPTY | (((rxIdx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0); - rxIdx = (rxIdx + 1) % PKTBUFSRX; + rtx.rxbd[rx_idx].length = 0; + rtx.rxbd[rx_idx].status = + RXBD_EMPTY | (((rx_idx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0); + rx_idx = (rx_idx + 1) % PKTBUFSRX;
if (in_be32(®s->ievent) & IEVENT_BSY) { out_be32(®s->ievent, IEVENT_BSY); @@ -324,21 +324,21 @@ static void startup_tsec(struct eth_device *dev) struct tsec __iomem *regs = priv->regs;
/* reset the indices to zero */ - rxIdx = 0; - txIdx = 0; + rx_idx = 0; + tx_idx = 0; #ifdef CONFIG_SYS_FSL_ERRATUM_NMG_ETSEC129 uint svr; #endif
/* Point to the buffer descriptors */ - out_be32(®s->tbase, (unsigned int)(&rtx.txbd[txIdx])); - out_be32(®s->rbase, (unsigned int)(&rtx.rxbd[rxIdx])); + out_be32(®s->tbase, (unsigned int)(&rtx.txbd[tx_idx])); + out_be32(®s->rbase, (unsigned int)(&rtx.rxbd[rx_idx]));
/* Initialize the Rx Buffer descriptors */ for (i = 0; i < PKTBUFSRX; i++) { rtx.rxbd[i].status = RXBD_EMPTY; rtx.rxbd[i].length = 0; - rtx.rxbd[i].bufPtr = (uint) NetRxPackets[i]; + rtx.rxbd[i].bufptr = (uint) NetRxPackets[i]; } rtx.rxbd[PKTBUFSRX - 1].status |= RXBD_WRAP;
@@ -346,7 +346,7 @@ static void startup_tsec(struct eth_device *dev) for (i = 0; i < TX_BUF_CNT; i++) { rtx.txbd[i].status = 0; rtx.txbd[i].length = 0; - rtx.txbd[i].bufPtr = 0; + rtx.txbd[i].bufptr = 0; } rtx.txbd[TX_BUF_CNT - 1].status |= TXBD_WRAP;
@@ -378,31 +378,31 @@ static int tsec_send(struct eth_device *dev, void *packet, int length) struct tsec __iomem *regs = priv->regs;
/* Find an empty buffer descriptor */ - for (i = 0; rtx.txbd[txIdx].status & TXBD_READY; i++) { + for (i = 0; rtx.txbd[tx_idx].status & TXBD_READY; i++) { if (i >= TOUT_LOOP) { debug("%s: tsec: tx buffers full\n", dev->name); return result; } }
- rtx.txbd[txIdx].bufPtr = (uint) packet; - rtx.txbd[txIdx].length = length; - rtx.txbd[txIdx].status |= + rtx.txbd[tx_idx].bufptr = (uint) packet; + rtx.txbd[tx_idx].length = length; + rtx.txbd[tx_idx].status |= (TXBD_READY | TXBD_LAST | TXBD_CRC | TXBD_INTERRUPT);
/* Tell the DMA to go */ out_be32(®s->tstat, TSTAT_CLEAR_THALT);
/* Wait for buffer to be transmitted */ - for (i = 0; rtx.txbd[txIdx].status & TXBD_READY; i++) { + for (i = 0; rtx.txbd[tx_idx].status & TXBD_READY; i++) { if (i >= TOUT_LOOP) { debug("%s: tsec: tx error\n", dev->name); return result; } }
- txIdx = (txIdx + 1) % TX_BUF_CNT; - result = rtx.txbd[txIdx].status & TXBD_STATS; + tx_idx = (tx_idx + 1) % TX_BUF_CNT; + result = rtx.txbd[tx_idx].status & TXBD_STATS;
return result; } @@ -413,25 +413,25 @@ static int tsec_recv(struct eth_device *dev) struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs;
- while (!(rtx.rxbd[rxIdx].status & RXBD_EMPTY)) { + while (!(rtx.rxbd[rx_idx].status & RXBD_EMPTY)) {
- length = rtx.rxbd[rxIdx].length; + length = rtx.rxbd[rx_idx].length;
/* Send the packet up if there were no errors */ - if (!(rtx.rxbd[rxIdx].status & RXBD_STATS)) { - NetReceive(NetRxPackets[rxIdx], length - 4); + if (!(rtx.rxbd[rx_idx].status & RXBD_STATS)) { + NetReceive(NetRxPackets[rx_idx], length - 4); } else { printf("Got error %x\n", - (rtx.rxbd[rxIdx].status & RXBD_STATS)); + (rtx.rxbd[rx_idx].status & RXBD_STATS)); }
- rtx.rxbd[rxIdx].length = 0; + rtx.rxbd[rx_idx].length = 0;
/* Set the wrap bit if this is the last element in the list */ - rtx.rxbd[rxIdx].status = - RXBD_EMPTY | (((rxIdx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0); + rtx.rxbd[rx_idx].status = + RXBD_EMPTY | (((rx_idx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0);
- rxIdx = (rxIdx + 1) % PKTBUFSRX; + rx_idx = (rx_idx + 1) % PKTBUFSRX; }
if (in_be32(®s->ievent) & IEVENT_BSY) { diff --git a/include/tsec.h b/include/tsec.h index c30aafb..6bc43ef 100644 --- a/include/tsec.h +++ b/include/tsec.h @@ -202,14 +202,14 @@ typedef struct txbd8 { ushort status; /* Status Fields */ ushort length; /* Buffer length */ - uint bufPtr; /* Buffer Pointer */ + uint bufptr; /* Buffer Pointer */ } txbd8_t;
typedef struct rxbd8 { ushort status; /* Status Fields */ ushort length; /* Buffer Length */ - uint bufPtr; /* Buffer Pointer */ + uint bufptr; /* Buffer Pointer */ } rxbd8_t;
typedef struct rmon_mib
Currently, the buffer descriptor (BD) fields cannot be correctly accessed by a little endian processor. This patch fixes the issue by making the access of BDs to be portable among different cpu architectures.
Use portable data types for the Rx/Tx buffer descriptor fields. Add macro accessors to insure that the big endian buffer descriptors are correctly accessed by the little endian cpus too. Sparse tool was also used to verify the correctness of these conversion macros.
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- drivers/net/tsec.c | 89 ++++++++++++++++++++++++++++++++---------------------- include/tsec.h | 22 ++++++-------- 2 files changed, 63 insertions(+), 48 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 289229a..cf99546 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -29,12 +29,20 @@ static uint rx_idx; /* index of the current RX buffer */ static uint tx_idx; /* index of the current TX buffer */
typedef volatile struct rtxbd { - txbd8_t txbd[TX_BUF_CNT]; - rxbd8_t rxbd[PKTBUFSRX]; + struct txbd8 txbd[TX_BUF_CNT]; + struct rxbd8 rxbd[PKTBUFSRX]; } RTXBD;
#ifdef __GNUC__ -static RTXBD rtx __attribute__ ((aligned(8))); +static RTXBD rtx __aligned(8); +#define RXBD(i) rtx.rxbd[i] +#define TXBD(i) rtx.txbd[i] +#define GET_BD_STAT(T, i) be16_to_cpu((__force __be16)T##BD(i).status) +#define SET_BD_STAT(T, i, v) T##BD(i).status = (__force __u16)cpu_to_be16(v) +#define GET_BD_BLEN(T, i) be16_to_cpu((__force __be16)T##BD(i).length) +#define SET_BD_BLEN(T, i, v) T##BD(i).length = (__force __u16)cpu_to_be16(v) +#define GET_BD_BPTR(T, i) be32_to_cpu((__force __be32)T##BD(i).bufptr) +#define SET_BD_BPTR(T, i, v) T##BD(i).bufptr = (__force __u32)cpu_to_be32(v) #else #error "rtx must be 64-bit aligned" #endif @@ -275,10 +283,11 @@ void redundant_init(struct eth_device *dev) clrbits_be32(®s->dmactrl, DMACTRL_GRS | DMACTRL_GTS);
do { + uint16_t status; tsec_send(dev, (void *)pkt, sizeof(pkt));
/* Wait for buffer to be received */ - for (t = 0; rtx.rxbd[rx_idx].status & RXBD_EMPTY; t++) { + for (t = 0; GET_BD_STAT(RX, rx_idx) & RXBD_EMPTY; t++) { if (t >= 10 * TOUT_LOOP) { printf("%s: tsec: rx error\n", dev->name); break; @@ -288,9 +297,11 @@ void redundant_init(struct eth_device *dev) if (!memcmp(pkt, (void *)NetRxPackets[rx_idx], sizeof(pkt))) fail = 0;
- rtx.rxbd[rx_idx].length = 0; - rtx.rxbd[rx_idx].status = - RXBD_EMPTY | (((rx_idx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0); + SET_BD_BLEN(RX, rx_idx, 0); + status = RXBD_EMPTY; + if ((rx_idx + 1) == PKTBUFSRX) + status |= RXBD_WRAP; + SET_BD_STAT(RX, rx_idx, status); rx_idx = (rx_idx + 1) % PKTBUFSRX;
if (in_be32(®s->ievent) & IEVENT_BSY) { @@ -319,9 +330,10 @@ void redundant_init(struct eth_device *dev) */ static void startup_tsec(struct eth_device *dev) { - int i; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs; + uint16_t status; + int i;
/* reset the indices to zero */ rx_idx = 0; @@ -331,24 +343,26 @@ static void startup_tsec(struct eth_device *dev) #endif
/* Point to the buffer descriptors */ - out_be32(®s->tbase, (unsigned int)(&rtx.txbd[tx_idx])); - out_be32(®s->rbase, (unsigned int)(&rtx.rxbd[rx_idx])); + out_be32(®s->tbase, (u32)&TXBD(0)); + out_be32(®s->rbase, (u32)&RXBD(0));
/* Initialize the Rx Buffer descriptors */ for (i = 0; i < PKTBUFSRX; i++) { - rtx.rxbd[i].status = RXBD_EMPTY; - rtx.rxbd[i].length = 0; - rtx.rxbd[i].bufptr = (uint) NetRxPackets[i]; + SET_BD_STAT(RX, i, RXBD_EMPTY); + SET_BD_BLEN(RX, i, 0); + SET_BD_BPTR(RX, i, (u32)NetRxPackets[i]); } - rtx.rxbd[PKTBUFSRX - 1].status |= RXBD_WRAP; + status = GET_BD_STAT(RX, PKTBUFSRX - 1); + SET_BD_STAT(RX, PKTBUFSRX - 1, status | RXBD_WRAP);
/* Initialize the TX Buffer Descriptors */ for (i = 0; i < TX_BUF_CNT; i++) { - rtx.txbd[i].status = 0; - rtx.txbd[i].length = 0; - rtx.txbd[i].bufptr = 0; + SET_BD_STAT(TX, i, 0); + SET_BD_BLEN(TX, i, 0); + SET_BD_BPTR(TX, i, 0); } - rtx.txbd[TX_BUF_CNT - 1].status |= TXBD_WRAP; + status = GET_BD_STAT(TX, TX_BUF_CNT - 1); + SET_BD_STAT(TX, TX_BUF_CNT - 1, status | TXBD_WRAP);
#ifdef CONFIG_SYS_FSL_ERRATUM_NMG_ETSEC129 svr = get_svr(); @@ -372,29 +386,31 @@ static void startup_tsec(struct eth_device *dev) */ static int tsec_send(struct eth_device *dev, void *packet, int length) { - int i; - int result = 0; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs; + uint16_t status; + int result = 0; + int i;
/* Find an empty buffer descriptor */ - for (i = 0; rtx.txbd[tx_idx].status & TXBD_READY; i++) { + for (i = 0; GET_BD_STAT(TX, tx_idx) & TXBD_READY; i++) { if (i >= TOUT_LOOP) { debug("%s: tsec: tx buffers full\n", dev->name); return result; } }
- rtx.txbd[tx_idx].bufptr = (uint) packet; - rtx.txbd[tx_idx].length = length; - rtx.txbd[tx_idx].status |= - (TXBD_READY | TXBD_LAST | TXBD_CRC | TXBD_INTERRUPT); + SET_BD_BPTR(TX, tx_idx, (u32)packet); + SET_BD_BLEN(TX, tx_idx, length); + status = GET_BD_STAT(TX, tx_idx); + SET_BD_STAT(TX, tx_idx, status | + (TXBD_READY | TXBD_LAST | TXBD_CRC | TXBD_INTERRUPT));
/* Tell the DMA to go */ out_be32(®s->tstat, TSTAT_CLEAR_THALT);
/* Wait for buffer to be transmitted */ - for (i = 0; rtx.txbd[tx_idx].status & TXBD_READY; i++) { + for (i = 0; GET_BD_STAT(TX, tx_idx) & TXBD_READY; i++) { if (i >= TOUT_LOOP) { debug("%s: tsec: tx error\n", dev->name); return result; @@ -402,34 +418,35 @@ static int tsec_send(struct eth_device *dev, void *packet, int length) }
tx_idx = (tx_idx + 1) % TX_BUF_CNT; - result = rtx.txbd[tx_idx].status & TXBD_STATS; + result = GET_BD_STAT(TX, tx_idx) & TXBD_STATS;
return result; }
static int tsec_recv(struct eth_device *dev) { - int length; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs;
- while (!(rtx.rxbd[rx_idx].status & RXBD_EMPTY)) { - - length = rtx.rxbd[rx_idx].length; + while (!(GET_BD_STAT(RX, rx_idx) & RXBD_EMPTY)) { + int length = GET_BD_BLEN(RX, rx_idx); + uint16_t status;
/* Send the packet up if there were no errors */ - if (!(rtx.rxbd[rx_idx].status & RXBD_STATS)) { + if (!(GET_BD_STAT(RX, rx_idx) & RXBD_STATS)) { NetReceive(NetRxPackets[rx_idx], length - 4); } else { printf("Got error %x\n", - (rtx.rxbd[rx_idx].status & RXBD_STATS)); + (GET_BD_STAT(RX, rx_idx) & RXBD_STATS)); }
- rtx.rxbd[rx_idx].length = 0; + SET_BD_BLEN(RX, rx_idx, 0);
+ status = RXBD_EMPTY; /* Set the wrap bit if this is the last element in the list */ - rtx.rxbd[rx_idx].status = - RXBD_EMPTY | (((rx_idx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0); + if ((rx_idx + 1) == PKTBUFSRX) + status |= RXBD_WRAP; + SET_BD_STAT(RX, rx_idx, status);
rx_idx = (rx_idx + 1) % PKTBUFSRX; } diff --git a/include/tsec.h b/include/tsec.h index 6bc43ef..95054be 100644 --- a/include/tsec.h +++ b/include/tsec.h @@ -198,19 +198,17 @@ #define RXBD_TRUNCATED 0x0001 #define RXBD_STATS 0x003f
-typedef struct txbd8 -{ - ushort status; /* Status Fields */ - ushort length; /* Buffer length */ - uint bufptr; /* Buffer Pointer */ -} txbd8_t; +struct txbd8 { + uint16_t status; /* Status Fields */ + uint16_t length; /* Buffer length */ + uint32_t bufptr; /* Buffer Pointer */ +};
-typedef struct rxbd8 -{ - ushort status; /* Status Fields */ - ushort length; /* Buffer Length */ - uint bufptr; /* Buffer Pointer */ -} rxbd8_t; +struct rxbd8 { + uint16_t status; /* Status Fields */ + uint16_t length; /* Buffer Length */ + uint32_t bufptr; /* Buffer Pointer */ +};
typedef struct rmon_mib {
On Mon, 2013-09-30 at 12:44 +0300, Claudiu Manoil wrote:
+static RTXBD rtx __aligned(8); +#define RXBD(i) rtx.rxbd[i] +#define TXBD(i) rtx.txbd[i] +#define GET_BD_STAT(T, i) be16_to_cpu((__force __be16)T##BD(i).status) +#define SET_BD_STAT(T, i, v) T##BD(i).status = (__force __u16)cpu_to_be16(v) +#define GET_BD_BLEN(T, i) be16_to_cpu((__force __be16)T##BD(i).length) +#define SET_BD_BLEN(T, i, v) T##BD(i).length = (__force __u16)cpu_to_be16(v) +#define GET_BD_BPTR(T, i) be32_to_cpu((__force __be32)T##BD(i).bufptr) +#define SET_BD_BPTR(T, i, v) T##BD(i).bufptr = (__force __u32)cpu_to_be32(v)
Why the forcing? Can't you declare the data with __be16/__be32?
#else #error "rtx must be 64-bit aligned" #endif @@ -275,10 +283,11 @@ void redundant_init(struct eth_device *dev) clrbits_be32(®s->dmactrl, DMACTRL_GRS | DMACTRL_GTS);
do {
uint16_t status;tsec_send(dev, (void *)pkt, sizeof(pkt));
/* Wait for buffer to be received */
for (t = 0; rtx.rxbd[rx_idx].status & RXBD_EMPTY; t++) {
for (t = 0; GET_BD_STAT(RX, rx_idx) & RXBD_EMPTY; t++) {
What's wrong with:
for (t = 0; in_be16(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
Or if you don't want to use I/O accessors on the DMA descriptors (Is synchronization ensured some other way? We had problems with this in the Linux driver before...):
for (t = 0; be16_to_cpup(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
-Scott
On 10/1/2013 2:22 AM, Scott Wood wrote:
On Mon, 2013-09-30 at 12:44 +0300, Claudiu Manoil wrote:
+static RTXBD rtx __aligned(8); +#define RXBD(i) rtx.rxbd[i] +#define TXBD(i) rtx.txbd[i] +#define GET_BD_STAT(T, i) be16_to_cpu((__force __be16)T##BD(i).status) +#define SET_BD_STAT(T, i, v) T##BD(i).status = (__force __u16)cpu_to_be16(v) +#define GET_BD_BLEN(T, i) be16_to_cpu((__force __be16)T##BD(i).length) +#define SET_BD_BLEN(T, i, v) T##BD(i).length = (__force __u16)cpu_to_be16(v) +#define GET_BD_BPTR(T, i) be32_to_cpu((__force __be32)T##BD(i).bufptr) +#define SET_BD_BPTR(T, i, v) T##BD(i).bufptr = (__force __u32)cpu_to_be32(v)
Why the forcing? Can't you declare the data with __be16/__be32?
The __force annotation is obviously needed to suppress the sparse warnings due to BD data declaration type not being __bexx, but the generic uintxx_t, as you noticed as well. Now, why I didn't use __bexx instead? The main reason would be maintainability: the DMA descriptors may not be in big endian format exclusively. The eTSEC hw may actually have an option to interpret the DMA descriptors in little endian format. If we decide to use that option for future platforms, then the __bexx type wouldn't be correct anymore. Besides, I noticed that most drivers prefer to use the generic type uintxx_t for buffer descriptors, instead of the annotated __bexx/__lexx types.
#else #error "rtx must be 64-bit aligned" #endif @@ -275,10 +283,11 @@ void redundant_init(struct eth_device *dev) clrbits_be32(®s->dmactrl, DMACTRL_GRS | DMACTRL_GTS);
do {
uint16_t status;tsec_send(dev, (void *)pkt, sizeof(pkt));
/* Wait for buffer to be received */
for (t = 0; rtx.rxbd[rx_idx].status & RXBD_EMPTY; t++) {
for (t = 0; GET_BD_STAT(RX, rx_idx) & RXBD_EMPTY; t++) {What's wrong with:
for (t = 0; in_be16(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
Or if you don't want to use I/O accessors on the DMA descriptors (Is synchronization ensured some other way? We had problems with this in the Linux driver before...):
Synchronization here is is insured by declaring the RTXBD structure type as "volatile" (see RTXBD declaration, a couple of lines above). The existing code has been working this way for quite a while on the mpc85xx platforms, so I thought it would be better not to change this approach. Using i/o accessors for the Buffer Descriptors would be a significant change, and I don't see how to argue such a change: why would the I/O accessors be better than the current approach - i.e. declaring the DMA descriptors as volatile? Is there something wrong with about volatile usage in this case? (afaik, this is a case were the volatile declaration is permitted)
Also, there doesn't seem to be other net drivers using I/O accessors for the BD rings.
for (t = 0; be16_to_cpup(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
I opted for using macros instead due to code maintainability, and to avoid overly long lines (80) and to better control these changes. For instance, if etsec were to access it's BDs in little endian format in the future, then it would be enough to update the implementation of the macro accessors without touching the whole code.
-Scott
Thanks for reviewing this.
Regards, Claudiu
On Tue, 2013-10-01 at 14:38 +0300, Claudiu Manoil wrote:
On 10/1/2013 2:22 AM, Scott Wood wrote:
On Mon, 2013-09-30 at 12:44 +0300, Claudiu Manoil wrote:
+static RTXBD rtx __aligned(8); +#define RXBD(i) rtx.rxbd[i] +#define TXBD(i) rtx.txbd[i] +#define GET_BD_STAT(T, i) be16_to_cpu((__force __be16)T##BD(i).status) +#define SET_BD_STAT(T, i, v) T##BD(i).status = (__force __u16)cpu_to_be16(v) +#define GET_BD_BLEN(T, i) be16_to_cpu((__force __be16)T##BD(i).length) +#define SET_BD_BLEN(T, i, v) T##BD(i).length = (__force __u16)cpu_to_be16(v) +#define GET_BD_BPTR(T, i) be32_to_cpu((__force __be32)T##BD(i).bufptr) +#define SET_BD_BPTR(T, i, v) T##BD(i).bufptr = (__force __u32)cpu_to_be32(v)
Why the forcing? Can't you declare the data with __be16/__be32?
The __force annotation is obviously needed to suppress the sparse warnings due to BD data declaration type not being __bexx, but the generic uintxx_t, as you noticed as well. Now, why I didn't use __bexx instead? The main reason would be maintainability: the DMA descriptors may not be in big endian format exclusively. The eTSEC hw may actually have an option to interpret the DMA descriptors in little endian format.
May have or does have?
If it does have such a feature, do you plan to use it? Usually I have not seen such features used for (e.g.) little-endian PCI devices on big endian hosts.
What's wrong with:
for (t = 0; in_be16(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
Or if you don't want to use I/O accessors on the DMA descriptors (Is synchronization ensured some other way? We had problems with this in the Linux driver before...):
Synchronization here is is insured by declaring the RTXBD structure type as "volatile" (see RTXBD declaration, a couple of lines above).
That does not achieve hardware synchronization, and even the effects on the compiler are questionable due to volatile's vague semantics.
The existing code has been working this way for quite a while on the mpc85xx platforms,
It was "working" for a while in Linux as well, until we encountered a workload where it didn't (though granted, there was no volatile in that case). See Linux commit 3b6330ce2a3e1f152f79a6203f73d23356e243a7
FWIW, I see some other places in U-Boot's TSEC driver that use out_be32() on the descriptors (e.g. startup_tsec after "Point to the buffer descriptors").
so I thought it would be better not to change this approach. Using i/o accessors for the Buffer Descriptors would be a significant change, and I don't see how to argue such a change: why would the I/O accessors be better than the current approach - i.e. declaring the DMA descriptors as volatile? Is there something wrong with about volatile usage in this case? (afaik, this is a case were the volatile declaration is permitted)
Also, there doesn't seem to be other net drivers using I/O accessors for the BD rings.
I picked some random examples, and the first driver in Linux in which I could quickly find the BD rings uses I/O accessors (drivers/net/ethernet/realtek/8319too.c). I then checked its U-Boot eqivalent (drivers/net/rtl8139.c) and it also uses I/O accessors for the descriptors.
for (t = 0; be16_to_cpup(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
I opted for using macros instead due to code maintainability,
Obfuscatory macros do not help.
and to avoid overly long lines (80)
You could factor out an rxbd_empty() function, or factor that loop out to be its own function, or have a local variable point to rtx.rxbd[rx_idx]...
and to better control these changes. For instance, if etsec were to access it's BDs in little endian format in the future,
Either don't do that (preferred option), or at that point add tsec16_to_cpup() and friends.
-Scott
On 10/1/2013 9:50 PM, Scott Wood wrote:
On Tue, 2013-10-01 at 14:38 +0300, Claudiu Manoil wrote:
On 10/1/2013 2:22 AM, Scott Wood wrote:
On Mon, 2013-09-30 at 12:44 +0300, Claudiu Manoil wrote:
+static RTXBD rtx __aligned(8); +#define RXBD(i) rtx.rxbd[i] +#define TXBD(i) rtx.txbd[i] +#define GET_BD_STAT(T, i) be16_to_cpu((__force __be16)T##BD(i).status) +#define SET_BD_STAT(T, i, v) T##BD(i).status = (__force __u16)cpu_to_be16(v) +#define GET_BD_BLEN(T, i) be16_to_cpu((__force __be16)T##BD(i).length) +#define SET_BD_BLEN(T, i, v) T##BD(i).length = (__force __u16)cpu_to_be16(v) +#define GET_BD_BPTR(T, i) be32_to_cpu((__force __be32)T##BD(i).bufptr) +#define SET_BD_BPTR(T, i, v) T##BD(i).bufptr = (__force __u32)cpu_to_be32(v)
Why the forcing? Can't you declare the data with __be16/__be32?
The __force annotation is obviously needed to suppress the sparse warnings due to BD data declaration type not being __bexx, but the generic uintxx_t, as you noticed as well. Now, why I didn't use __bexx instead? The main reason would be maintainability: the DMA descriptors may not be in big endian format exclusively. The eTSEC hw may actually have an option to interpret the DMA descriptors in little endian format.
May have or does have?
If it does have such a feature, do you plan to use it? Usually I have not seen such features used for (e.g.) little-endian PCI devices on big endian hosts.
I has that option, but I don't really plan to use it, clearly not for big endian hosts. The "may have" was for future little endian hosts. But I think this option is not really needed by the uboot driver so doing the byte swapping in software should be ok (i.e. performance wise).
What's wrong with:
for (t = 0; in_be16(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
Or if you don't want to use I/O accessors on the DMA descriptors (Is synchronization ensured some other way? We had problems with this in the Linux driver before...):
Synchronization here is is insured by declaring the RTXBD structure type as "volatile" (see RTXBD declaration, a couple of lines above).
That does not achieve hardware synchronization, and even the effects on the compiler are questionable due to volatile's vague semantics.
The existing code has been working this way for quite a while on the mpc85xx platforms,
It was "working" for a while in Linux as well, until we encountered a workload where it didn't (though granted, there was no volatile in that case). See Linux commit 3b6330ce2a3e1f152f79a6203f73d23356e243a7
Good point, I guess it would be safer too use some memory barriers around accesses to BD fields in the uboot driver too. However some portable barriers would be needed, eieio() doesn't have an equivalent for ARM.
FWIW, I see some other places in U-Boot's TSEC driver that use out_be32() on the descriptors (e.g. startup_tsec after "Point to the buffer descriptors").
That's only to program the tbase and rbase registers with the physical address of the Tx/Rx BD rings' base.
so I thought it would be better not to change this approach. Using i/o accessors for the Buffer Descriptors would be a significant change, and I don't see how to argue such a change: why would the I/O accessors be better than the current approach - i.e. declaring the DMA descriptors as volatile? Is there something wrong with about volatile usage in this case? (afaik, this is a case were the volatile declaration is permitted)
Also, there doesn't seem to be other net drivers using I/O accessors for the BD rings.
I picked some random examples, and the first driver in Linux in which I could quickly find the BD rings uses I/O accessors (drivers/net/ethernet/realtek/8319too.c). I then checked its U-Boot eqivalent (drivers/net/rtl8139.c) and it also uses I/O accessors for the descriptors.
I actually meant accessing buffer descriptor fields (like status, length, dma address). As you can see in this example from linux 8319too.c's Rx routine, there's no I/O accessor involved:
/* read size+status of next frame from DMA ring buffer */ rx_status = le32_to_cpu (*(__le32 *) (rx_ring + ring_offset));
However the driver does use a rmb() just before this line.
for (t = 0; be16_to_cpup(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
I opted for using macros instead due to code maintainability,
Obfuscatory macros do not help.
and to avoid overly long lines (80)
You could factor out an rxbd_empty() function, or factor that loop out to be its own function, or have a local variable point to rtx.rxbd[rx_idx]...
and to better control these changes. For instance, if etsec were to access it's BDs in little endian format in the future,
Either don't do that (preferred option), or at that point add tsec16_to_cpup() and friends.
I'll have a try with the portable I/O accessors (in_be, out_be) to see how it works that way.
Thanks, Claudiu
On Wed, 2013-10-02 at 17:16 +0300, Claudiu Manoil wrote:
On 10/1/2013 9:50 PM, Scott Wood wrote:
On Tue, 2013-10-01 at 14:38 +0300, Claudiu Manoil wrote:
On 10/1/2013 2:22 AM, Scott Wood wrote:
On Mon, 2013-09-30 at 12:44 +0300, Claudiu Manoil wrote:
+static RTXBD rtx __aligned(8); +#define RXBD(i) rtx.rxbd[i] +#define TXBD(i) rtx.txbd[i] +#define GET_BD_STAT(T, i) be16_to_cpu((__force __be16)T##BD(i).status) +#define SET_BD_STAT(T, i, v) T##BD(i).status = (__force __u16)cpu_to_be16(v) +#define GET_BD_BLEN(T, i) be16_to_cpu((__force __be16)T##BD(i).length) +#define SET_BD_BLEN(T, i, v) T##BD(i).length = (__force __u16)cpu_to_be16(v) +#define GET_BD_BPTR(T, i) be32_to_cpu((__force __be32)T##BD(i).bufptr) +#define SET_BD_BPTR(T, i, v) T##BD(i).bufptr = (__force __u32)cpu_to_be32(v)
Why the forcing? Can't you declare the data with __be16/__be32?
The __force annotation is obviously needed to suppress the sparse warnings due to BD data declaration type not being __bexx, but the generic uintxx_t, as you noticed as well. Now, why I didn't use __bexx instead? The main reason would be maintainability: the DMA descriptors may not be in big endian format exclusively. The eTSEC hw may actually have an option to interpret the DMA descriptors in little endian format.
May have or does have?
If it does have such a feature, do you plan to use it? Usually I have not seen such features used for (e.g.) little-endian PCI devices on big endian hosts.
I has that option, but I don't really plan to use it, clearly not for big endian hosts. The "may have" was for future little endian hosts. But I think this option is not really needed by the uboot driver so doing the byte swapping in software should be ok (i.e. performance wise).
If we don't plan to use it even on future little endian hosts, then it's no big deal to use __beNN.
What's wrong with:
for (t = 0; in_be16(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
Or if you don't want to use I/O accessors on the DMA descriptors (Is synchronization ensured some other way? We had problems with this in the Linux driver before...):
Synchronization here is is insured by declaring the RTXBD structure type as "volatile" (see RTXBD declaration, a couple of lines above).
That does not achieve hardware synchronization, and even the effects on the compiler are questionable due to volatile's vague semantics.
The existing code has been working this way for quite a while on the mpc85xx platforms,
It was "working" for a while in Linux as well, until we encountered a workload where it didn't (though granted, there was no volatile in that case). See Linux commit 3b6330ce2a3e1f152f79a6203f73d23356e243a7
Good point, I guess it would be safer too use some memory barriers around accesses to BD fields in the uboot driver too. However some portable barriers would be needed, eieio() doesn't have an equivalent for ARM.
FWIW, I see some other places in U-Boot's TSEC driver that use out_be32() on the descriptors (e.g. startup_tsec after "Point to the buffer descriptors").
That's only to program the tbase and rbase registers with the physical address of the Tx/Rx BD rings' base.
Oops. :-P
Also, there doesn't seem to be other net drivers using I/O accessors for the BD rings.
I picked some random examples, and the first driver in Linux in which I could quickly find the BD rings uses I/O accessors (drivers/net/ethernet/realtek/8319too.c). I then checked its U-Boot eqivalent (drivers/net/rtl8139.c) and it also uses I/O accessors for the descriptors.
I actually meant accessing buffer descriptor fields (like status, length, dma address). As you can see in this example from linux 8319too.c's Rx routine, there's no I/O accessor involved:
/* read size+status of next frame from DMA ring buffer */ rx_status = le32_to_cpu (*(__le32 *) (rx_ring + ring_offset));
However the driver does use a rmb() just before this line.
Yeah, it doesn't help when both types of accesses show up when searching for the ring, and accessors exist with both possible argument orderings. Especially when a driver has custom accessors.
It's OK to use explicit synchronization rather than I/O accessors, if you're careful to ensure that it's correct.
It looks like drivers/net/fec_mxc.c in U-Boot is an example that uses I/O accessors on ring data, e.g.:
writew(length, &fec->tbd_base[fec->tbd_index].data_length);
-Scott
Currently, the buffer descriptor (BD) fields cannot be correctly accessed by a little endian processor. This patch fixes the issue by making the access of BDs to be portable among different cpu architectures.
Use portable data types for the Rx/Tx buffer descriptor fields. Use portable I/O accessors to insure that the big endian BDs are correctly accessed by little endian cpus too, and to insure proper sync with the H/W. Removed the redundant RTXBD "volatile" type, as proper synchronization around BD data accesses is provided by the I/O accessors now. The "sparse" tool was also used to verify the correctness of these changes.
Cc: Scott Wood scottwood@freescale.com
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- v2: * used portable I/O accessors (in_be/out_be) * replaced macro usage * retested on p1020 (ping, tftp)
drivers/net/tsec.c | 108 ++++++++++++++++++++++++++++++++--------------------- include/tsec.h | 22 +++++------ 2 files changed, 76 insertions(+), 54 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 289229a..b48e595 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -28,13 +28,30 @@ DECLARE_GLOBAL_DATA_PTR; static uint rx_idx; /* index of the current RX buffer */ static uint tx_idx; /* index of the current TX buffer */
-typedef volatile struct rtxbd { - txbd8_t txbd[TX_BUF_CNT]; - rxbd8_t rxbd[PKTBUFSRX]; -} RTXBD; - #ifdef __GNUC__ -static RTXBD rtx __attribute__ ((aligned(8))); +static struct txbd8 txbd[TX_BUF_CNT] __aligned(8); +static struct rxbd8 rxbd[PKTBUFSRX] __aligned(8); + +static inline u16 read_txbd_stat(uint idx) +{ + return in_be16((u16 __iomem *)&txbd[idx].status); +} + +static inline void write_txbd_stat(uint idx, u16 status) +{ + out_be16((u16 __iomem *)&txbd[idx].status, status); +} + +static inline u16 read_rxbd_stat(uint idx) +{ + return in_be16((u16 __iomem *)&rxbd[idx].status); +} + +static inline void write_rxbd_stat(uint idx, u16 status) +{ + out_be16((u16 __iomem *)&rxbd[idx].status, status); +} + #else #error "rtx must be 64-bit aligned" #endif @@ -275,10 +292,11 @@ void redundant_init(struct eth_device *dev) clrbits_be32(®s->dmactrl, DMACTRL_GRS | DMACTRL_GTS);
do { + uint16_t status; tsec_send(dev, (void *)pkt, sizeof(pkt));
/* Wait for buffer to be received */ - for (t = 0; rtx.rxbd[rx_idx].status & RXBD_EMPTY; t++) { + for (t = 0; read_rxbd_stat(rx_idx) & RXBD_EMPTY; t++) { if (t >= 10 * TOUT_LOOP) { printf("%s: tsec: rx error\n", dev->name); break; @@ -288,9 +306,11 @@ void redundant_init(struct eth_device *dev) if (!memcmp(pkt, (void *)NetRxPackets[rx_idx], sizeof(pkt))) fail = 0;
- rtx.rxbd[rx_idx].length = 0; - rtx.rxbd[rx_idx].status = - RXBD_EMPTY | (((rx_idx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0); + out_be16((u16 __iomem *)&rxbd[rx_idx].length, 0); + status = RXBD_EMPTY; + if ((rx_idx + 1) == PKTBUFSRX) + status |= RXBD_WRAP; + write_rxbd_stat(rx_idx, status); rx_idx = (rx_idx + 1) % PKTBUFSRX;
if (in_be32(®s->ievent) & IEVENT_BSY) { @@ -319,9 +339,10 @@ void redundant_init(struct eth_device *dev) */ static void startup_tsec(struct eth_device *dev) { - int i; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs; + uint16_t status; + int i;
/* reset the indices to zero */ rx_idx = 0; @@ -331,24 +352,26 @@ static void startup_tsec(struct eth_device *dev) #endif
/* Point to the buffer descriptors */ - out_be32(®s->tbase, (unsigned int)(&rtx.txbd[tx_idx])); - out_be32(®s->rbase, (unsigned int)(&rtx.rxbd[rx_idx])); + out_be32(®s->tbase, (u32)&txbd[0]); + out_be32(®s->rbase, (u32)&rxbd[0]);
/* Initialize the Rx Buffer descriptors */ for (i = 0; i < PKTBUFSRX; i++) { - rtx.rxbd[i].status = RXBD_EMPTY; - rtx.rxbd[i].length = 0; - rtx.rxbd[i].bufptr = (uint) NetRxPackets[i]; + write_rxbd_stat(i, RXBD_EMPTY); + out_be16((u16 __iomem *)&rxbd[i].length, 0); + out_be32((u32 __iomem *)&rxbd[i].bufptr, (u32)NetRxPackets[i]); } - rtx.rxbd[PKTBUFSRX - 1].status |= RXBD_WRAP; + status = read_rxbd_stat(PKTBUFSRX - 1); + write_rxbd_stat(PKTBUFSRX - 1, status | RXBD_WRAP);
/* Initialize the TX Buffer Descriptors */ for (i = 0; i < TX_BUF_CNT; i++) { - rtx.txbd[i].status = 0; - rtx.txbd[i].length = 0; - rtx.txbd[i].bufptr = 0; + write_txbd_stat(i, 0); + out_be16((u16 __iomem *)&txbd[i].length, 0); + out_be32((u32 __iomem *)&txbd[i].bufptr, 0); } - rtx.txbd[TX_BUF_CNT - 1].status |= TXBD_WRAP; + status = read_txbd_stat(TX_BUF_CNT - 1); + write_txbd_stat(TX_BUF_CNT - 1, status | TXBD_WRAP);
#ifdef CONFIG_SYS_FSL_ERRATUM_NMG_ETSEC129 svr = get_svr(); @@ -372,29 +395,31 @@ static void startup_tsec(struct eth_device *dev) */ static int tsec_send(struct eth_device *dev, void *packet, int length) { - int i; - int result = 0; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs; + uint16_t status; + int result = 0; + int i;
/* Find an empty buffer descriptor */ - for (i = 0; rtx.txbd[tx_idx].status & TXBD_READY; i++) { + for (i = 0; read_txbd_stat(tx_idx) & TXBD_READY; i++) { if (i >= TOUT_LOOP) { debug("%s: tsec: tx buffers full\n", dev->name); return result; } }
- rtx.txbd[tx_idx].bufptr = (uint) packet; - rtx.txbd[tx_idx].length = length; - rtx.txbd[tx_idx].status |= - (TXBD_READY | TXBD_LAST | TXBD_CRC | TXBD_INTERRUPT); + out_be32((u32 __iomem *)&txbd[tx_idx].bufptr, (u32)packet); + out_be16((u16 __iomem *)&txbd[tx_idx].length, length); + status = read_txbd_stat(tx_idx); + write_txbd_stat(tx_idx, status | + (TXBD_READY | TXBD_LAST | TXBD_CRC | TXBD_INTERRUPT));
/* Tell the DMA to go */ out_be32(®s->tstat, TSTAT_CLEAR_THALT);
/* Wait for buffer to be transmitted */ - for (i = 0; rtx.txbd[tx_idx].status & TXBD_READY; i++) { + for (i = 0; read_txbd_stat(tx_idx) & TXBD_READY; i++) { if (i >= TOUT_LOOP) { debug("%s: tsec: tx error\n", dev->name); return result; @@ -402,34 +427,33 @@ static int tsec_send(struct eth_device *dev, void *packet, int length) }
tx_idx = (tx_idx + 1) % TX_BUF_CNT; - result = rtx.txbd[tx_idx].status & TXBD_STATS; + result = read_txbd_stat(tx_idx) & TXBD_STATS;
return result; }
static int tsec_recv(struct eth_device *dev) { - int length; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs;
- while (!(rtx.rxbd[rx_idx].status & RXBD_EMPTY)) { - - length = rtx.rxbd[rx_idx].length; + while (!(read_rxbd_stat(rx_idx) & RXBD_EMPTY)) { + int length = in_be16((u16 __iomem *)&rxbd[rx_idx].length); + uint16_t status = read_rxbd_stat(rx_idx);
/* Send the packet up if there were no errors */ - if (!(rtx.rxbd[rx_idx].status & RXBD_STATS)) { + if (!(status & RXBD_STATS)) NetReceive(NetRxPackets[rx_idx], length - 4); - } else { - printf("Got error %x\n", - (rtx.rxbd[rx_idx].status & RXBD_STATS)); - } + else + printf("Got error %x\n", (status & RXBD_STATS));
- rtx.rxbd[rx_idx].length = 0; + out_be16((u16 __iomem *)&rxbd[rx_idx].length, 0);
+ status = RXBD_EMPTY; /* Set the wrap bit if this is the last element in the list */ - rtx.rxbd[rx_idx].status = - RXBD_EMPTY | (((rx_idx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0); + if ((rx_idx + 1) == PKTBUFSRX) + status |= RXBD_WRAP; + write_rxbd_stat(rx_idx, status);
rx_idx = (rx_idx + 1) % PKTBUFSRX; } diff --git a/include/tsec.h b/include/tsec.h index 6bc43ef..95054be 100644 --- a/include/tsec.h +++ b/include/tsec.h @@ -198,19 +198,17 @@ #define RXBD_TRUNCATED 0x0001 #define RXBD_STATS 0x003f
-typedef struct txbd8 -{ - ushort status; /* Status Fields */ - ushort length; /* Buffer length */ - uint bufptr; /* Buffer Pointer */ -} txbd8_t; +struct txbd8 { + uint16_t status; /* Status Fields */ + uint16_t length; /* Buffer length */ + uint32_t bufptr; /* Buffer Pointer */ +};
-typedef struct rxbd8 -{ - ushort status; /* Status Fields */ - ushort length; /* Buffer Length */ - uint bufptr; /* Buffer Pointer */ -} rxbd8_t; +struct rxbd8 { + uint16_t status; /* Status Fields */ + uint16_t length; /* Buffer Length */ + uint32_t bufptr; /* Buffer Pointer */ +};
typedef struct rmon_mib {
On Thu, 2013-10-03 at 14:48 +0300, Claudiu Manoil wrote:
+static inline u16 read_txbd_stat(uint idx) +{
- return in_be16((u16 __iomem *)&txbd[idx].status);
+}
+static inline void write_txbd_stat(uint idx, u16 status) +{
- out_be16((u16 __iomem *)&txbd[idx].status, status);
+}
+static inline u16 read_rxbd_stat(uint idx) +{
- return in_be16((u16 __iomem *)&rxbd[idx].status);
+}
+static inline void write_rxbd_stat(uint idx, u16 status) +{
- out_be16((u16 __iomem *)&rxbd[idx].status, status);
+}
Do you need __force on these to make sparse happy?
I'd rather see these declared as __iomem than use casts (at which point, you probably don't need per-field accessor functions).
-Scott
On 10/3/2013 9:37 PM, Scott Wood wrote:
On Thu, 2013-10-03 at 14:48 +0300, Claudiu Manoil wrote:
+static inline u16 read_txbd_stat(uint idx) +{
- return in_be16((u16 __iomem *)&txbd[idx].status);
+}
+static inline void write_txbd_stat(uint idx, u16 status) +{
- out_be16((u16 __iomem *)&txbd[idx].status, status);
+}
+static inline u16 read_rxbd_stat(uint idx) +{
- return in_be16((u16 __iomem *)&rxbd[idx].status);
+}
+static inline void write_rxbd_stat(uint idx, u16 status) +{
- out_be16((u16 __iomem *)&rxbd[idx].status, status);
+}
Do you need __force on these to make sparse happy?
No, we don't need __force in this case, in_be/out_be are less restrictive and take plain unsigned pointers (not __beNN pointers). On the other hand, they require the __iomem address space marker, to make sparse happy.
I'd rather see these declared as __iomem than use casts (at which point, you probably don't need per-field accessor functions).
Me too, but I wasn't sure how to do that. I thought __iomem works with pointer declarations only. But it turns out it works this way too:
-static struct txbd8 txbd[TX_BUF_CNT] __aligned(8); -static struct rxbd8 rxbd[PKTBUFSRX] __aligned(8); [...] +static struct txbd8 __iomem txbd[TX_BUF_CNT] __aligned(8); +static struct rxbd8 __iomem rxbd[PKTBUFSRX] __aligned(8);
[...] - for (i = 0; read_txbd_stat(tx_idx) & TXBD_READY; i++) { + for (i = 0; in_be16(&txbd[tx_idx].status) & TXBD_READY; i++) { [...]
And sparse doesn't complain about it. In this case I'll drop the read_txbd_stat() and friends. Is this acceptable?
Thanks. Claudiu
On Fri, 2013-10-04 at 11:27 +0300, Claudiu Manoil wrote:
On 10/3/2013 9:37 PM, Scott Wood wrote:
On Thu, 2013-10-03 at 14:48 +0300, Claudiu Manoil wrote:
+static inline u16 read_txbd_stat(uint idx) +{
- return in_be16((u16 __iomem *)&txbd[idx].status);
+}
+static inline void write_txbd_stat(uint idx, u16 status) +{
- out_be16((u16 __iomem *)&txbd[idx].status, status);
+}
+static inline u16 read_rxbd_stat(uint idx) +{
- return in_be16((u16 __iomem *)&rxbd[idx].status);
+}
+static inline void write_rxbd_stat(uint idx, u16 status) +{
- out_be16((u16 __iomem *)&rxbd[idx].status, status);
+}
Do you need __force on these to make sparse happy?
No, we don't need __force in this case, in_be/out_be are less restrictive and take plain unsigned pointers (not __beNN pointers). On the other hand, they require the __iomem address space marker, to make sparse happy.
I thought you'd need __force to convert a non-iomem pointer to an __iomem pointer.
I'd rather see these declared as __iomem than use casts (at which point, you probably don't need per-field accessor functions).
Me too, but I wasn't sure how to do that. I thought __iomem works with pointer declarations only. But it turns out it works this way too:
Even if that were the case, you could put it on the pointers, which is how it's usually used.
-static struct txbd8 txbd[TX_BUF_CNT] __aligned(8); -static struct rxbd8 rxbd[PKTBUFSRX] __aligned(8); [...] +static struct txbd8 __iomem txbd[TX_BUF_CNT] __aligned(8); +static struct rxbd8 __iomem rxbd[PKTBUFSRX] __aligned(8);
[...]
for (i = 0; read_txbd_stat(tx_idx) & TXBD_READY; i++) {
for (i = 0; in_be16(&txbd[tx_idx].status) & TXBD_READY; i++) {[...]
And sparse doesn't complain about it. In this case I'll drop the read_txbd_stat() and friends. Is this acceptable?
Yes.
-Scott
Currently, the buffer descriptor (BD) fields cannot be correctly accessed by a little endian processor. This patch fixes the issue by making the access of BDs to be portable among different cpu architectures.
Use portable data types for the Rx/Tx buffer descriptor fields. Use portable I/O accessors to insure that the big endian BDs are correctly accessed by little endian cpus too, and to insure proper sync with the H/W. Removed the redundant RTXBD "volatile" type, as proper synchronization around BD data accesses is provided by the I/O accessors now. The "sparse" tool was also used to verify the correctness of these changes.
Cc: Scott Wood scottwood@freescale.com
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- v2: * used portable I/O accessors (in_be/out_be) * replaced macro usage * retested on p1020 (ping, tftp) v3: * txbd, rxbd declared as __iomem to avoid casting
drivers/net/tsec.c | 88 ++++++++++++++++++++++++++++-------------------------- include/tsec.h | 22 +++++++------- 2 files changed, 56 insertions(+), 54 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 289229a..e354fad 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -28,13 +28,10 @@ DECLARE_GLOBAL_DATA_PTR; static uint rx_idx; /* index of the current RX buffer */ static uint tx_idx; /* index of the current TX buffer */
-typedef volatile struct rtxbd { - txbd8_t txbd[TX_BUF_CNT]; - rxbd8_t rxbd[PKTBUFSRX]; -} RTXBD; - #ifdef __GNUC__ -static RTXBD rtx __attribute__ ((aligned(8))); +static struct txbd8 __iomem txbd[TX_BUF_CNT] __aligned(8); +static struct rxbd8 __iomem rxbd[PKTBUFSRX] __aligned(8); + #else #error "rtx must be 64-bit aligned" #endif @@ -275,10 +272,11 @@ void redundant_init(struct eth_device *dev) clrbits_be32(®s->dmactrl, DMACTRL_GRS | DMACTRL_GTS);
do { + uint16_t status; tsec_send(dev, (void *)pkt, sizeof(pkt));
/* Wait for buffer to be received */ - for (t = 0; rtx.rxbd[rx_idx].status & RXBD_EMPTY; t++) { + for (t = 0; in_be16(&rxbd[rx_idx].status) & RXBD_EMPTY; t++) { if (t >= 10 * TOUT_LOOP) { printf("%s: tsec: rx error\n", dev->name); break; @@ -288,9 +286,11 @@ void redundant_init(struct eth_device *dev) if (!memcmp(pkt, (void *)NetRxPackets[rx_idx], sizeof(pkt))) fail = 0;
- rtx.rxbd[rx_idx].length = 0; - rtx.rxbd[rx_idx].status = - RXBD_EMPTY | (((rx_idx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0); + out_be16(&rxbd[rx_idx].length, 0); + status = RXBD_EMPTY; + if ((rx_idx + 1) == PKTBUFSRX) + status |= RXBD_WRAP; + out_be16(&rxbd[rx_idx].status, status); rx_idx = (rx_idx + 1) % PKTBUFSRX;
if (in_be32(®s->ievent) & IEVENT_BSY) { @@ -319,9 +319,10 @@ void redundant_init(struct eth_device *dev) */ static void startup_tsec(struct eth_device *dev) { - int i; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs; + uint16_t status; + int i;
/* reset the indices to zero */ rx_idx = 0; @@ -331,24 +332,26 @@ static void startup_tsec(struct eth_device *dev) #endif
/* Point to the buffer descriptors */ - out_be32(®s->tbase, (unsigned int)(&rtx.txbd[tx_idx])); - out_be32(®s->rbase, (unsigned int)(&rtx.rxbd[rx_idx])); + out_be32(®s->tbase, (u32)&txbd[0]); + out_be32(®s->rbase, (u32)&rxbd[0]);
/* Initialize the Rx Buffer descriptors */ for (i = 0; i < PKTBUFSRX; i++) { - rtx.rxbd[i].status = RXBD_EMPTY; - rtx.rxbd[i].length = 0; - rtx.rxbd[i].bufptr = (uint) NetRxPackets[i]; + out_be16(&rxbd[i].status, RXBD_EMPTY); + out_be16(&rxbd[i].length, 0); + out_be32(&rxbd[i].bufptr, (u32)NetRxPackets[i]); } - rtx.rxbd[PKTBUFSRX - 1].status |= RXBD_WRAP; + status = in_be16(&rxbd[PKTBUFSRX - 1].status); + out_be16(&rxbd[PKTBUFSRX - 1].status, status | RXBD_WRAP);
/* Initialize the TX Buffer Descriptors */ for (i = 0; i < TX_BUF_CNT; i++) { - rtx.txbd[i].status = 0; - rtx.txbd[i].length = 0; - rtx.txbd[i].bufptr = 0; + out_be16(&txbd[i].status, 0); + out_be16(&txbd[i].length, 0); + out_be32(&txbd[i].bufptr, 0); } - rtx.txbd[TX_BUF_CNT - 1].status |= TXBD_WRAP; + status = in_be16(&txbd[TX_BUF_CNT - 1].status); + out_be16(&txbd[TX_BUF_CNT - 1].status, status | TXBD_WRAP);
#ifdef CONFIG_SYS_FSL_ERRATUM_NMG_ETSEC129 svr = get_svr(); @@ -372,29 +375,31 @@ static void startup_tsec(struct eth_device *dev) */ static int tsec_send(struct eth_device *dev, void *packet, int length) { - int i; - int result = 0; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs; + uint16_t status; + int result = 0; + int i;
/* Find an empty buffer descriptor */ - for (i = 0; rtx.txbd[tx_idx].status & TXBD_READY; i++) { + for (i = 0; in_be16(&txbd[tx_idx].status) & TXBD_READY; i++) { if (i >= TOUT_LOOP) { debug("%s: tsec: tx buffers full\n", dev->name); return result; } }
- rtx.txbd[tx_idx].bufptr = (uint) packet; - rtx.txbd[tx_idx].length = length; - rtx.txbd[tx_idx].status |= - (TXBD_READY | TXBD_LAST | TXBD_CRC | TXBD_INTERRUPT); + out_be32(&txbd[tx_idx].bufptr, (u32)packet); + out_be16(&txbd[tx_idx].length, length); + status = in_be16(&txbd[tx_idx].status); + out_be16(&txbd[tx_idx].status, status | + (TXBD_READY | TXBD_LAST | TXBD_CRC | TXBD_INTERRUPT));
/* Tell the DMA to go */ out_be32(®s->tstat, TSTAT_CLEAR_THALT);
/* Wait for buffer to be transmitted */ - for (i = 0; rtx.txbd[tx_idx].status & TXBD_READY; i++) { + for (i = 0; in_be16(&txbd[tx_idx].status) & TXBD_READY; i++) { if (i >= TOUT_LOOP) { debug("%s: tsec: tx error\n", dev->name); return result; @@ -402,34 +407,33 @@ static int tsec_send(struct eth_device *dev, void *packet, int length) }
tx_idx = (tx_idx + 1) % TX_BUF_CNT; - result = rtx.txbd[tx_idx].status & TXBD_STATS; + result = in_be16(&txbd[tx_idx].status) & TXBD_STATS;
return result; }
static int tsec_recv(struct eth_device *dev) { - int length; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs;
- while (!(rtx.rxbd[rx_idx].status & RXBD_EMPTY)) { - - length = rtx.rxbd[rx_idx].length; + while (!(in_be16(&rxbd[rx_idx].status) & RXBD_EMPTY)) { + int length = in_be16(&rxbd[rx_idx].length); + uint16_t status = in_be16(&rxbd[rx_idx].status);
/* Send the packet up if there were no errors */ - if (!(rtx.rxbd[rx_idx].status & RXBD_STATS)) { + if (!(status & RXBD_STATS)) NetReceive(NetRxPackets[rx_idx], length - 4); - } else { - printf("Got error %x\n", - (rtx.rxbd[rx_idx].status & RXBD_STATS)); - } + else + printf("Got error %x\n", (status & RXBD_STATS));
- rtx.rxbd[rx_idx].length = 0; + out_be16(&rxbd[rx_idx].length, 0);
+ status = RXBD_EMPTY; /* Set the wrap bit if this is the last element in the list */ - rtx.rxbd[rx_idx].status = - RXBD_EMPTY | (((rx_idx + 1) == PKTBUFSRX) ? RXBD_WRAP : 0); + if ((rx_idx + 1) == PKTBUFSRX) + status |= RXBD_WRAP; + out_be16(&rxbd[rx_idx].status, status);
rx_idx = (rx_idx + 1) % PKTBUFSRX; } diff --git a/include/tsec.h b/include/tsec.h index 6bc43ef..95054be 100644 --- a/include/tsec.h +++ b/include/tsec.h @@ -198,19 +198,17 @@ #define RXBD_TRUNCATED 0x0001 #define RXBD_STATS 0x003f
-typedef struct txbd8 -{ - ushort status; /* Status Fields */ - ushort length; /* Buffer length */ - uint bufptr; /* Buffer Pointer */ -} txbd8_t; +struct txbd8 { + uint16_t status; /* Status Fields */ + uint16_t length; /* Buffer length */ + uint32_t bufptr; /* Buffer Pointer */ +};
-typedef struct rxbd8 -{ - ushort status; /* Status Fields */ - ushort length; /* Buffer Length */ - uint bufptr; /* Buffer Pointer */ -} rxbd8_t; +struct rxbd8 { + uint16_t status; /* Status Fields */ + uint16_t length; /* Buffer Length */ + uint32_t bufptr; /* Buffer Pointer */ +};
typedef struct rmon_mib {
On 10/4/2013 6:50 PM, Scott Wood wrote:
On Fri, 2013-10-04 at 11:27 +0300, Claudiu Manoil wrote:
On 10/3/2013 9:37 PM, Scott Wood wrote:
On Thu, 2013-10-03 at 14:48 +0300, Claudiu Manoil wrote:
+static inline u16 read_txbd_stat(uint idx) +{
- return in_be16((u16 __iomem *)&txbd[idx].status);
+}
+static inline void write_txbd_stat(uint idx, u16 status) +{
- out_be16((u16 __iomem *)&txbd[idx].status, status);
+}
+static inline u16 read_rxbd_stat(uint idx) +{
- return in_be16((u16 __iomem *)&rxbd[idx].status);
+}
+static inline void write_rxbd_stat(uint idx, u16 status) +{
- out_be16((u16 __iomem *)&rxbd[idx].status, status);
+}
Do you need __force on these to make sparse happy?
No, we don't need __force in this case, in_be/out_be are less restrictive and take plain unsigned pointers (not __beNN pointers). On the other hand, they require the __iomem address space marker, to make sparse happy.
I thought you'd need __force to convert a non-iomem pointer to an __iomem pointer.
I'd rather see these declared as __iomem than use casts (at which point, you probably don't need per-field accessor functions).
Me too, but I wasn't sure how to do that. I thought __iomem works with pointer declarations only. But it turns out it works this way too:
Even if that were the case, you could put it on the pointers, which is how it's usually used.
-static struct txbd8 txbd[TX_BUF_CNT] __aligned(8); -static struct rxbd8 rxbd[PKTBUFSRX] __aligned(8); [...] +static struct txbd8 __iomem txbd[TX_BUF_CNT] __aligned(8); +static struct rxbd8 __iomem rxbd[PKTBUFSRX] __aligned(8);
[...]
for (i = 0; read_txbd_stat(tx_idx) & TXBD_READY; i++) {
for (i = 0; in_be16(&txbd[tx_idx].status) & TXBD_READY; i++) {[...]
And sparse doesn't complain about it. In this case I'll drop the read_txbd_stat() and friends. Is this acceptable?
Yes.
[v3] declaring the BDs as __iomem to avoid casting submitted: http://patchwork.ozlabs.org/patch/280664/
Thanks. Claudiu
On Fri, Oct 4, 2013 at 11:25 AM, Claudiu Manoil claudiu.manoil@freescale.com wrote:
[v3] declaring the BDs as __iomem to avoid casting submitted: http://patchwork.ozlabs.org/patch/280664/
+ out_be32(®s->tbase, (u32)&txbd[0]); + out_be32(®s->rbase, (u32)&rxbd[0]);
&rxbd[0] is a virtual address.
Doesn't rbase require a physical address? You're assuming that virt == phys.
On Sat, Oct 5, 2013 at 9:31 AM, Timur Tabi timur@tabi.org wrote:
- out_be32(®s->tbase, (u32)&txbd[0]);
- out_be32(®s->rbase, (u32)&rxbd[0]);
&rxbd[0] is a virtual address.
Doesn't rbase require a physical address? You're assuming that virt == phys.
Also:
- out_be32(®s->tbase, (unsigned int)(&rtx.txbd[tx_idx])); - out_be32(®s->rbase, (unsigned int)(&rtx.rxbd[rx_idx])); + out_be32(®s->tbase, (u32)&txbd[0]); + out_be32(®s->rbase, (u32)&rxbd[0]);
Are you assuming that rx_idx will always be zero in this case?
On 10/5/2013 5:49 PM, Timur Tabi wrote:
On Sat, Oct 5, 2013 at 9:31 AM, Timur Tabi timur@tabi.org wrote:
- out_be32(®s->tbase, (u32)&txbd[0]);
- out_be32(®s->rbase, (u32)&rxbd[0]);
&rxbd[0] is a virtual address.
Doesn't rbase require a physical address? You're assuming that virt == phys.
Also:
- out_be32(®s->tbase, (unsigned int)(&rtx.txbd[tx_idx]));
- out_be32(®s->rbase, (unsigned int)(&rtx.rxbd[rx_idx]));
- out_be32(®s->tbase, (u32)&txbd[0]);
- out_be32(®s->rbase, (u32)&rxbd[0]);
Are you assuming that rx_idx will always be zero in this case?
Hi,
This is just initialization code, rx_idx and tx_idx are set to 0 just 4-5 irrelevant lines above (unfortunately format-patch doesn't catch that in the patch's context).
Thanks, Claudiu
On 10/5/2013 5:31 PM, Timur Tabi wrote:
On Fri, Oct 4, 2013 at 11:25 AM, Claudiu Manoil claudiu.manoil@freescale.com wrote:
[v3] declaring the BDs as __iomem to avoid casting submitted: http://patchwork.ozlabs.org/patch/280664/
- out_be32(®s->tbase, (u32)&txbd[0]);
- out_be32(®s->rbase, (u32)&rxbd[0]);
&rxbd[0] is a virtual address.
Doesn't rbase require a physical address? You're assuming that virt == phys.
These SoCs don't feature IOMMU so it cannot be a virtual address. I think you're suggesting that virt_to_phys() should be used to fix that, right? However, virt_to_phys() is equivalent to that simple cast in most cases as there's no CONFIG_PHYS_64BIT for the platforms with eTSEC. I'm actually not sure if there's a platform with eTSEC for which that cast wouldn't be enough.
If so, it should be a separate patch as this fix would apply to existing (old) code and is out of the scope of this patch about portable accessors.
Thanks. Claudiu
Claudiu Manoil wrote:
I think you're suggesting that virt_to_phys() should be used to fix that, right?
Yes.
However, virt_to_phys() is equivalent to that simple cast in most cases as there's no CONFIG_PHYS_64BIT for the platforms with eTSEC. I'm actually not sure if there's a platform with eTSEC for which that cast wouldn't be enough.
Freescale might create a eTSEC-compatible 10G NIC one day that does run on e6500 cores.
If so, it should be a separate patch as this fix would apply to existing (old) code and is out of the scope of this patch about portable accessors.
Fair enough.
On Mon, 2013-10-07 at 13:16 +0300, Claudiu Manoil wrote:
On 10/5/2013 5:31 PM, Timur Tabi wrote:
On Fri, Oct 4, 2013 at 11:25 AM, Claudiu Manoil claudiu.manoil@freescale.com wrote:
[v3] declaring the BDs as __iomem to avoid casting submitted: http://patchwork.ozlabs.org/patch/280664/
- out_be32(®s->tbase, (u32)&txbd[0]);
- out_be32(®s->rbase, (u32)&rxbd[0]);
&rxbd[0] is a virtual address.
Doesn't rbase require a physical address? You're assuming that virt == phys.
These SoCs don't feature IOMMU so it cannot be a virtual address. I think you're suggesting that virt_to_phys() should be used to fix that, right? However, virt_to_phys() is equivalent to that simple cast in most cases as there's no CONFIG_PHYS_64BIT for the platforms with eTSEC. I'm actually not sure if there's a platform with eTSEC for which that cast wouldn't be enough.
There is CONFIG_PHYS_64BIT for most eTSEC SoCs, but since this is a RAM address rather than MMIO (and U-Boot only uses a static identity mapping of the low 2G of RAM), it doesn't matter, though ideally virt_to_phys should still be used.
-Scott
On 10/3/2013 1:15 AM, Scott Wood wrote: [snip]
Yeah, it doesn't help when both types of accesses show up when searching for the ring, and accessors exist with both possible argument orderings. Especially when a driver has custom accessors.
It's OK to use explicit synchronization rather than I/O accessors, if you're careful to ensure that it's correct.
It looks like drivers/net/fec_mxc.c in U-Boot is an example that uses I/O accessors on ring data, e.g.:
writew(length, &fec->tbd_base[fec->tbd_index].data_length);
Hi Scott,
I sent a v2 for this patch (the next 2 patches in the series are not affected by this one): http://patchwork.ozlabs.org/patch/280285/
In this version I dropped the macro usage and used I/O accessors for the ring data, as discussed. This approach does not require the explicit __beNN type for the BD fields, it also removes the need to declare the BD structures as "volatile" and it's safer because in_be()/ out_be() enforce HW sync.
Thanks, Claudiu
On Mon, Sep 30, 2013 at 4:44 AM, Claudiu Manoil claudiu.manoil@freescale.com wrote:
+#define GET_BD_STAT(T, i) be16_to_cpu((__force __be16)T##BD(i).status) +#define SET_BD_STAT(T, i, v) T##BD(i).status = (__force __u16)cpu_to_be16(v) +#define GET_BD_BLEN(T, i) be16_to_cpu((__force __be16)T##BD(i).length) +#define SET_BD_BLEN(T, i, v) T##BD(i).length = (__force __u16)cpu_to_be16(v) +#define GET_BD_BPTR(T, i) be32_to_cpu((__force __be32)T##BD(i).bufptr) +#define SET_BD_BPTR(T, i, v) T##BD(i).bufptr = (__force __u32)cpu_to_be32(v)
This is pretty ugly. There's got to be a better way to handle this. Are you going to be doing stuff like this for every driver for bi-endian hardware?
Some time ago I suggest that we re-purpose iowrite() and ioread() to be native-endian, and not just little endian. I think something like that would make more sense than hacky macros like this.
On 10/4/2013 6:12 AM, Timur Tabi wrote:
On Mon, Sep 30, 2013 at 4:44 AM, Claudiu Manoil claudiu.manoil@freescale.com wrote:
+#define GET_BD_STAT(T, i) be16_to_cpu((__force __be16)T##BD(i).status) +#define SET_BD_STAT(T, i, v) T##BD(i).status = (__force __u16)cpu_to_be16(v) +#define GET_BD_BLEN(T, i) be16_to_cpu((__force __be16)T##BD(i).length) +#define SET_BD_BLEN(T, i, v) T##BD(i).length = (__force __u16)cpu_to_be16(v) +#define GET_BD_BPTR(T, i) be32_to_cpu((__force __be32)T##BD(i).bufptr) +#define SET_BD_BPTR(T, i, v) T##BD(i).bufptr = (__force __u32)cpu_to_be32(v)
This is pretty ugly. There's got to be a better way to handle this. Are you going to be doing stuff like this for every driver for bi-endian hardware?
Some time ago I suggest that we re-purpose iowrite() and ioread() to be native-endian, and not just little endian. I think something like that would make more sense than hacky macros like this.
Hi Timur,
We dropped these macros in favor of the in_be/out_be() I/O accessors (see http://patchwork.ozlabs.org/patch/280285/).
Regards, Claudiu
Use cross arch portable u32 instead of uint for the tsec registers. Remove the typedefs for the register struct definitions in the process. Fix long lines.
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- drivers/net/tsec.c | 2 +- include/tsec.h | 278 ++++++++++++++++++++++++++--------------------------- 2 files changed, 139 insertions(+), 141 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index cf99546..5dfdc94 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -177,7 +177,7 @@ static void init_registers(struct tsec __iomem *regs) out_be32(®s->rctrl, 0x00000000);
/* Init RMON mib registers */ - memset((void *)&(regs->rmon), 0, sizeof(rmon_mib_t)); + memset((void *)®s->rmon, 0, sizeof(regs->rmon));
out_be32(®s->rmon.cam1, 0xffffffff); out_be32(®s->rmon.cam2, 0xffffffff); diff --git a/include/tsec.h b/include/tsec.h index 95054be..1046426 100644 --- a/include/tsec.h +++ b/include/tsec.h @@ -210,179 +210,177 @@ struct rxbd8 { uint32_t bufptr; /* Buffer Pointer */ };
-typedef struct rmon_mib -{ +struct tsec_rmon_mib { /* Transmit and Receive Counters */ - uint tr64; /* Transmit and Receive 64-byte Frame Counter */ - uint tr127; /* Transmit and Receive 65-127 byte Frame Counter */ - uint tr255; /* Transmit and Receive 128-255 byte Frame Counter */ - uint tr511; /* Transmit and Receive 256-511 byte Frame Counter */ - uint tr1k; /* Transmit and Receive 512-1023 byte Frame Counter */ - uint trmax; /* Transmit and Receive 1024-1518 byte Frame Counter */ - uint trmgv; /* Transmit and Receive 1519-1522 byte Good VLAN Frame */ + u32 tr64; /* Tx/Rx 64-byte Frame Counter */ + u32 tr127; /* Tx/Rx 65-127 byte Frame Counter */ + u32 tr255; /* Tx/Rx 128-255 byte Frame Counter */ + u32 tr511; /* Tx/Rx 256-511 byte Frame Counter */ + u32 tr1k; /* Tx/Rx 512-1023 byte Frame Counter */ + u32 trmax; /* Tx/Rx 1024-1518 byte Frame Counter */ + u32 trmgv; /* Tx/Rx 1519-1522 byte Good VLAN Frame */ /* Receive Counters */ - uint rbyt; /* Receive Byte Counter */ - uint rpkt; /* Receive Packet Counter */ - uint rfcs; /* Receive FCS Error Counter */ - uint rmca; /* Receive Multicast Packet (Counter) */ - uint rbca; /* Receive Broadcast Packet */ - uint rxcf; /* Receive Control Frame Packet */ - uint rxpf; /* Receive Pause Frame Packet */ - uint rxuo; /* Receive Unknown OP Code */ - uint raln; /* Receive Alignment Error */ - uint rflr; /* Receive Frame Length Error */ - uint rcde; /* Receive Code Error */ - uint rcse; /* Receive Carrier Sense Error */ - uint rund; /* Receive Undersize Packet */ - uint rovr; /* Receive Oversize Packet */ - uint rfrg; /* Receive Fragments */ - uint rjbr; /* Receive Jabber */ - uint rdrp; /* Receive Drop */ + u32 rbyt; /* Receive Byte Counter */ + u32 rpkt; /* Receive Packet Counter */ + u32 rfcs; /* Receive FCS Error Counter */ + u32 rmca; /* Receive Multicast Packet (Counter) */ + u32 rbca; /* Receive Broadcast Packet */ + u32 rxcf; /* Receive Control Frame Packet */ + u32 rxpf; /* Receive Pause Frame Packet */ + u32 rxuo; /* Receive Unknown OP Code */ + u32 raln; /* Receive Alignment Error */ + u32 rflr; /* Receive Frame Length Error */ + u32 rcde; /* Receive Code Error */ + u32 rcse; /* Receive Carrier Sense Error */ + u32 rund; /* Receive Undersize Packet */ + u32 rovr; /* Receive Oversize Packet */ + u32 rfrg; /* Receive Fragments */ + u32 rjbr; /* Receive Jabber */ + u32 rdrp; /* Receive Drop */ /* Transmit Counters */ - uint tbyt; /* Transmit Byte Counter */ - uint tpkt; /* Transmit Packet */ - uint tmca; /* Transmit Multicast Packet */ - uint tbca; /* Transmit Broadcast Packet */ - uint txpf; /* Transmit Pause Control Frame */ - uint tdfr; /* Transmit Deferral Packet */ - uint tedf; /* Transmit Excessive Deferral Packet */ - uint tscl; /* Transmit Single Collision Packet */ + u32 tbyt; /* Transmit Byte Counter */ + u32 tpkt; /* Transmit Packet */ + u32 tmca; /* Transmit Multicast Packet */ + u32 tbca; /* Transmit Broadcast Packet */ + u32 txpf; /* Transmit Pause Control Frame */ + u32 tdfr; /* Transmit Deferral Packet */ + u32 tedf; /* Transmit Excessive Deferral Packet */ + u32 tscl; /* Transmit Single Collision Packet */ /* (0x2_n700) */ - uint tmcl; /* Transmit Multiple Collision Packet */ - uint tlcl; /* Transmit Late Collision Packet */ - uint txcl; /* Transmit Excessive Collision Packet */ - uint tncl; /* Transmit Total Collision */ - - uint res2; - - uint tdrp; /* Transmit Drop Frame */ - uint tjbr; /* Transmit Jabber Frame */ - uint tfcs; /* Transmit FCS Error */ - uint txcf; /* Transmit Control Frame */ - uint tovr; /* Transmit Oversize Frame */ - uint tund; /* Transmit Undersize Frame */ - uint tfrg; /* Transmit Fragments Frame */ + u32 tmcl; /* Transmit Multiple Collision Packet */ + u32 tlcl; /* Transmit Late Collision Packet */ + u32 txcl; /* Transmit Excessive Collision Packet */ + u32 tncl; /* Transmit Total Collision */ + + u32 res2; + + u32 tdrp; /* Transmit Drop Frame */ + u32 tjbr; /* Transmit Jabber Frame */ + u32 tfcs; /* Transmit FCS Error */ + u32 txcf; /* Transmit Control Frame */ + u32 tovr; /* Transmit Oversize Frame */ + u32 tund; /* Transmit Undersize Frame */ + u32 tfrg; /* Transmit Fragments Frame */ /* General Registers */ - uint car1; /* Carry Register One */ - uint car2; /* Carry Register Two */ - uint cam1; /* Carry Register One Mask */ - uint cam2; /* Carry Register Two Mask */ -} rmon_mib_t; - -typedef struct tsec_hash_regs -{ - uint iaddr0; /* Individual Address Register 0 */ - uint iaddr1; /* Individual Address Register 1 */ - uint iaddr2; /* Individual Address Register 2 */ - uint iaddr3; /* Individual Address Register 3 */ - uint iaddr4; /* Individual Address Register 4 */ - uint iaddr5; /* Individual Address Register 5 */ - uint iaddr6; /* Individual Address Register 6 */ - uint iaddr7; /* Individual Address Register 7 */ - uint res1[24]; - uint gaddr0; /* Group Address Register 0 */ - uint gaddr1; /* Group Address Register 1 */ - uint gaddr2; /* Group Address Register 2 */ - uint gaddr3; /* Group Address Register 3 */ - uint gaddr4; /* Group Address Register 4 */ - uint gaddr5; /* Group Address Register 5 */ - uint gaddr6; /* Group Address Register 6 */ - uint gaddr7; /* Group Address Register 7 */ - uint res2[24]; -} tsec_hash_t; + u32 car1; /* Carry Register One */ + u32 car2; /* Carry Register Two */ + u32 cam1; /* Carry Register One Mask */ + u32 cam2; /* Carry Register Two Mask */ +}; + +struct tsec_hash_regs { + u32 iaddr0; /* Individual Address Register 0 */ + u32 iaddr1; /* Individual Address Register 1 */ + u32 iaddr2; /* Individual Address Register 2 */ + u32 iaddr3; /* Individual Address Register 3 */ + u32 iaddr4; /* Individual Address Register 4 */ + u32 iaddr5; /* Individual Address Register 5 */ + u32 iaddr6; /* Individual Address Register 6 */ + u32 iaddr7; /* Individual Address Register 7 */ + u32 res1[24]; + u32 gaddr0; /* Group Address Register 0 */ + u32 gaddr1; /* Group Address Register 1 */ + u32 gaddr2; /* Group Address Register 2 */ + u32 gaddr3; /* Group Address Register 3 */ + u32 gaddr4; /* Group Address Register 4 */ + u32 gaddr5; /* Group Address Register 5 */ + u32 gaddr6; /* Group Address Register 6 */ + u32 gaddr7; /* Group Address Register 7 */ + u32 res2[24]; +};
struct tsec { /* General Control and Status Registers (0x2_n000) */ - uint res000[4]; + u32 res000[4];
- uint ievent; /* Interrupt Event */ - uint imask; /* Interrupt Mask */ - uint edis; /* Error Disabled */ - uint res01c; - uint ecntrl; /* Ethernet Control */ - uint minflr; /* Minimum Frame Length */ - uint ptv; /* Pause Time Value */ - uint dmactrl; /* DMA Control */ - uint tbipa; /* TBI PHY Address */ + u32 ievent; /* Interrupt Event */ + u32 imask; /* Interrupt Mask */ + u32 edis; /* Error Disabled */ + u32 res01c; + u32 ecntrl; /* Ethernet Control */ + u32 minflr; /* Minimum Frame Length */ + u32 ptv; /* Pause Time Value */ + u32 dmactrl; /* DMA Control */ + u32 tbipa; /* TBI PHY Address */
- uint res034[3]; - uint res040[48]; + u32 res034[3]; + u32 res040[48];
/* Transmit Control and Status Registers (0x2_n100) */ - uint tctrl; /* Transmit Control */ - uint tstat; /* Transmit Status */ - uint res108; - uint tbdlen; /* Tx BD Data Length */ - uint res110[5]; - uint ctbptr; /* Current TxBD Pointer */ - uint res128[23]; - uint tbptr; /* TxBD Pointer */ - uint res188[30]; + u32 tctrl; /* Transmit Control */ + u32 tstat; /* Transmit Status */ + u32 res108; + u32 tbdlen; /* Tx BD Data Length */ + u32 res110[5]; + u32 ctbptr; /* Current TxBD Pointer */ + u32 res128[23]; + u32 tbptr; /* TxBD Pointer */ + u32 res188[30]; /* (0x2_n200) */ - uint res200; - uint tbase; /* TxBD Base Address */ - uint res208[42]; - uint ostbd; /* Out of Sequence TxBD */ - uint ostbdp; /* Out of Sequence Tx Data Buffer Pointer */ - uint res2b8[18]; + u32 res200; + u32 tbase; /* TxBD Base Address */ + u32 res208[42]; + u32 ostbd; /* Out of Sequence TxBD */ + u32 ostbdp; /* Out of Sequence Tx Data Buffer Pointer */ + u32 res2b8[18];
/* Receive Control and Status Registers (0x2_n300) */ - uint rctrl; /* Receive Control */ - uint rstat; /* Receive Status */ - uint res308; - uint rbdlen; /* RxBD Data Length */ - uint res310[4]; - uint res320; - uint crbptr; /* Current Receive Buffer Pointer */ - uint res328[6]; - uint mrblr; /* Maximum Receive Buffer Length */ - uint res344[16]; - uint rbptr; /* RxBD Pointer */ - uint res388[30]; + u32 rctrl; /* Receive Control */ + u32 rstat; /* Receive Status */ + u32 res308; + u32 rbdlen; /* RxBD Data Length */ + u32 res310[4]; + u32 res320; + u32 crbptr; /* Current Receive Buffer Pointer */ + u32 res328[6]; + u32 mrblr; /* Maximum Receive Buffer Length */ + u32 res344[16]; + u32 rbptr; /* RxBD Pointer */ + u32 res388[30]; /* (0x2_n400) */ - uint res400; - uint rbase; /* RxBD Base Address */ - uint res408[62]; + u32 res400; + u32 rbase; /* RxBD Base Address */ + u32 res408[62];
/* MAC Registers (0x2_n500) */ - uint maccfg1; /* MAC Configuration #1 */ - uint maccfg2; /* MAC Configuration #2 */ - uint ipgifg; /* Inter Packet Gap/Inter Frame Gap */ - uint hafdup; /* Half-duplex */ - uint maxfrm; /* Maximum Frame */ - uint res514; - uint res518; + u32 maccfg1; /* MAC Configuration #1 */ + u32 maccfg2; /* MAC Configuration #2 */ + u32 ipgifg; /* Inter Packet Gap/Inter Frame Gap */ + u32 hafdup; /* Half-duplex */ + u32 maxfrm; /* Maximum Frame */ + u32 res514; + u32 res518;
- uint res51c; + u32 res51c;
- uint resmdio[6]; + u32 resmdio[6];
- uint res538; + u32 res538;
- uint ifstat; /* Interface Status */ - uint macstnaddr1; /* Station Address, part 1 */ - uint macstnaddr2; /* Station Address, part 2 */ - uint res548[46]; + u32 ifstat; /* Interface Status */ + u32 macstnaddr1; /* Station Address, part 1 */ + u32 macstnaddr2; /* Station Address, part 2 */ + u32 res548[46];
/* (0x2_n600) */ - uint res600[32]; + u32 res600[32];
/* RMON MIB Registers (0x2_n680-0x2_n73c) */ - rmon_mib_t rmon; - uint res740[48]; + struct tsec_rmon_mib rmon; + u32 res740[48];
/* Hash Function Registers (0x2_n800) */ - tsec_hash_t hash; + struct tsec_hash_regs hash;
- uint res900[128]; + u32 res900[128];
/* Pattern Registers (0x2_nb00) */ - uint resb00[62]; - uint attr; /* Default Attribute Register */ - uint attreli; /* Default Attribute Extract Length and Index */ + u32 resb00[62]; + u32 attr; /* Default Attribute Register */ + u32 attreli; /* Default Attribute Extract Length and Index */
/* TSEC Future Expansion Space (0x2_nc00-0x2_nffc) */ - uint resc00[256]; + u32 resc00[256]; };
#define TSEC_GIGABIT (1 << 0)
Fix the 32-bit memory access that is not "endianess safe", i.e. not giving the desired byte layout for LE cpus: tempval = *((uint *) (tmpbuf + 4)), where 'char tmpbuf[]'.
Free the stack from rendundant local vars: tmpbuf[] and i.
Use a portable type (u32) for the 32bit tsec register value holder: tempval.
Signed-off-by: Claudiu Manoil claudiu.manoil@freescale.com --- drivers/net/tsec.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 5dfdc94..44cead9 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -486,11 +486,9 @@ static void tsec_halt(struct eth_device *dev) */ static int tsec_init(struct eth_device *dev, bd_t * bd) { - uint tempval; - char tmpbuf[MAC_ADDR_LEN]; - int i; struct tsec_private *priv = (struct tsec_private *)dev->priv; struct tsec __iomem *regs = priv->regs; + u32 tempval; int ret;
/* Make sure the controller is stopped */ @@ -503,16 +501,16 @@ static int tsec_init(struct eth_device *dev, bd_t * bd) out_be32(®s->ecntrl, ECNTRL_INIT_SETTINGS);
/* Copy the station address into the address registers. - * Backwards, because little endian MACS are dumb */ - for (i = 0; i < MAC_ADDR_LEN; i++) - tmpbuf[MAC_ADDR_LEN - 1 - i] = dev->enetaddr[i]; - - tempval = (tmpbuf[0] << 24) | (tmpbuf[1] << 16) | (tmpbuf[2] << 8) | - tmpbuf[3]; + * For a station address of 0x12345678ABCD in transmission + * order (BE), MACnADDR1 is set to 0xCDAB7856 and + * MACnADDR2 is set to 0x34120000. + */ + tempval = (dev->enetaddr[5] << 24) | (dev->enetaddr[4] << 16) | + (dev->enetaddr[3] << 8) | dev->enetaddr[2];
out_be32(®s->macstnaddr1, tempval);
- tempval = *((uint *) (tmpbuf + 4)); + tempval = (dev->enetaddr[1] << 24) | (dev->enetaddr[0] << 16);
out_be32(®s->macstnaddr2, tempval);
participants (3)
-
Claudiu Manoil -
Scott Wood -
Timur Tabi