[U-Boot] [PATCH 0/9] net: tsec: Driver portability fixes and cleanup

[U-Boot] [PATCH 0/9] net: tsec: Driver portability fixes and cleanup

[Claudiu Manoil]

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(-)

-- 
1.7.11.7

[U-Boot] [PATCH 1/9] net: Fix mcast function pointer prototype

[Claudiu Manoil]

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;
-- 
1.7.11.7

[U-Boot] [PATCH 2/9] net: tsec: Fix and cleanup tsec_mcast_addr()

[Claudiu Manoil]

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(&regs->hash.gaddr0 + whichreg, value);
+	else
+		clrbits_be32(&regs->hash.gaddr0 + whichreg, value);
 
-	if (set) {
-		reg_array[whichreg] |= value;
-	} else {
-		reg_array[whichreg] &= ~value;
-	}
 	return 0;
 }
 #endif /* Multicast TFTP ? */
-- 
1.7.11.7

[U-Boot] [PATCH 3/9] net: tsec: Fix priv pointer in tsec_mcast_addr()

[Claudiu Manoil]

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;
 
-- 
1.7.11.7

[U-Boot] [PATCH 4/9] net: tsec: Cleanup tsec regs init and fix __iomem warns

[Claudiu Manoil]

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(&regs->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(&regs->dmactrl, DMACTRL_GRS | DMACTRL_GTS);
 	setbits_be32(&regs->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(&regs->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 at 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;
-- 
1.7.11.7

[U-Boot] [PATCH 5/9] net: fsl_mdio: Fix warnings for __iomem pointers

[Claudiu Manoil]

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(&regs->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);
-- 
1.7.11.7

[U-Boot] [PATCH 6/9] net: tsec: Fix CamelCase issues around BD code

[Claudiu Manoil]

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(&regs->ievent) & IEVENT_BSY) {
 			out_be32(&regs->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(&regs->tbase, (unsigned int)(&rtx.txbd[txIdx]));
-	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rxIdx]));
+	out_be32(&regs->tbase, (unsigned int)(&rtx.txbd[tx_idx]));
+	out_be32(&regs->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(&regs->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(&regs->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
-- 
1.7.11.7

[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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(&regs->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(&regs->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(&regs->tbase, (unsigned int)(&rtx.txbd[tx_idx]));
-	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rx_idx]));
+	out_be32(&regs->tbase, (u32)&TXBD(0));
+	out_be32(&regs->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(&regs->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
 {
-- 
1.7.11.7

[U-Boot] [PATCH 8/9] net: tsec: Use portable regs type (uint->u32)

[Claudiu Manoil]

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(&regs->rctrl, 0x00000000);
 
 	/* Init RMON mib registers */
-	memset((void *)&(regs->rmon), 0, sizeof(rmon_mib_t));
+	memset((void *)&regs->rmon, 0, sizeof(regs->rmon));
 
 	out_be32(&regs->rmon.cam1, 0xffffffff);
 	out_be32(&regs->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)
-- 
1.7.11.7

[U-Boot] [PATCH 9/9] net: tsec: Fix mac addr setup portability, cleanup

[Claudiu Manoil]

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(&regs->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(&regs->macstnaddr1, tempval);
 
-	tempval = *((uint *) (tmpbuf + 4));
+	tempval = (dev->enetaddr[1] << 24) | (dev->enetaddr[0] << 16);
 
 	out_be32(&regs->macstnaddr2, tempval);
 
-- 
1.7.11.7

[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Scott Wood]

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(&regs->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

[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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(&regs->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

[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Scott Wood]

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

[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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

[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Scott Wood]

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

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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(&regs->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(&regs->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(&regs->tbase, (unsigned int)(&rtx.txbd[tx_idx]));
-	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rx_idx]));
+	out_be32(&regs->tbase, (u32)&txbd[0]);
+	out_be32(&regs->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(&regs->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
 {
-- 
1.7.11.7

[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Scott Wood]

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

[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Timur Tabi]

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.

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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

[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Scott Wood]

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

[U-Boot] [PATCH 7/9][v3] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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(&regs->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(&regs->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(&regs->tbase, (unsigned int)(&rtx.txbd[tx_idx]));
-	out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rx_idx]));
+	out_be32(&regs->tbase, (u32)&txbd[0]);
+	out_be32(&regs->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(&regs->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
 {
-- 
1.7.11.7

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Timur Tabi]

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(&regs->tbase, (u32)&txbd[0]);
+ out_be32(&regs->rbase, (u32)&rxbd[0]);

&rxbd[0] is a virtual address.

Doesn't rbase require a physical address?  You're assuming that virt == phys.

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Timur Tabi]

On Sat, Oct 5, 2013 at 9:31 AM, Timur Tabi <timur@tabi.org> wrote:
>
> + out_be32(&regs->tbase, (u32)&txbd[0]);
> + out_be32(&regs->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(&regs->tbase, (unsigned int)(&rtx.txbd[tx_idx]));
- out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rx_idx]));
+ out_be32(&regs->tbase, (u32)&txbd[0]);
+ out_be32(&regs->rbase, (u32)&rxbd[0]);

Are you assuming that rx_idx will always be zero in this case?

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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(&regs->tbase, (u32)&txbd[0]);
>> + out_be32(&regs->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(&regs->tbase, (unsigned int)(&rtx.txbd[tx_idx]));
> - out_be32(&regs->rbase, (unsigned int)(&rtx.rxbd[rx_idx]));
> + out_be32(&regs->tbase, (u32)&txbd[0]);
> + out_be32(&regs->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

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

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(&regs->tbase, (u32)&txbd[0]);
> + out_be32(&regs->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

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Timur Tabi]

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.

[U-Boot] [PATCH 7/9][v2] net: tsec: Use portable types and accessors for BDs

[Scott Wood]

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(&regs->tbase, (u32)&txbd[0]);
> > + out_be32(&regs->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