Skip to contentSkip to conversation

net: fec_mxc: allow use with cache enabled

PATCH Superseded Eric Nelson 3 participants 16 messages Mar 13, 2012 – Mar 14, 2012 1 message not in the archive Network drivers

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): http://lists.denx.de/pipermail/u-boot/2012-March/#119442

#1 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

ensure that transmit and receive buffers are cache-line aligned invalidate cache for each packet as received
update receive buffer descriptors one cache line at a time flush cache before transmitting

Original patch by Marek:

	http://lists.denx.de/pipermail/u-boot/2012-February/117695.html

---
V2 addresses some concerns from the ML:
- Use readl()/writel() instead of mapped data structure accesses
- Wrong comment style
- &rbd_base[0] == rbd_base
removed 'volatile' from fec_send().

V3 updates from ML (and Marek):
consolidated CONFIG_FEC_DATA_ALIGNMENT and CONFIG_FEC_DESC_ALIGNMENT added cache flushes after initialization of TBD/RBD

 drivers/net/fec_mxc.c |  265 +++++++++++++++++++++++++++++++++++--------------
 drivers/net/fec_mxc.h |   19 +----
 2 files changed, 189 insertions(+), 95 deletions(-)
drivers/net/fec_mxc.c +188 -77
index 1fdd071..94a3927 100644
@@ -50,6 +50,24 @@ DECLARE_GLOBAL_DATA_PTR;
 #define	CONFIG_FEC_MXC_SWAP_PACKET
 #endif
 
+#if ARCH_DMA_MINALIGN > CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_FEC_ALIGN ARCH_DMA_MINALIGN
+#else
+#define CONFIG_FEC_ALIGN CONFIG_SYS_CACHELINE_SIZE
+#endif
+
+#define RXDESC_PER_CACHELINE	(CONFIG_FEC_ALIGN/sizeof(struct fec_bd))
+
+/* Check various alignment issues at compile time */
+#if ((CONFIG_FEC_ALIGN < 16) || (CONFIG_FEC_ALIGN % 16 != 0))
+#error	"CONFIG_FEC_ALIGN must be multiple of 16!"
+#endif
+
+#if ((PKTALIGN < CONFIG_FEC_ALIGN) || \
+	(PKTALIGN % CONFIG_FEC_ALIGN != 0))
+#error	"PKTALIGN must be multiple of CONFIG_FEC_ALIGN!"
+#endif
+
 #undef DEBUG
 
 struct nbuf {
@@ -259,43 +277,52 @@ static int fec_tx_task_disable(struct fec_priv *fec)
  * Initialize receive task's buffer descriptors
  * @param[in] fec all we know about the device yet
  * @param[in] count receive buffer count to be allocated
- * @param[in] size size of each receive buffer
+ * @param[in] dsize desired size of each receive buffer
  * @return 0 on success
  *
  * For this task we need additional memory for the data buffers. And each
  * data buffer requires some alignment. Thy must be aligned to a specific
- * boundary each (DB_DATA_ALIGNMENT).
+ * boundary each.
  */
-static int fec_rbd_init(struct fec_priv *fec, int count, int size)
+static int fec_rbd_init(struct fec_priv *fec, int count, int dsize)
 {
-	int ix;
-	uint32_t p = 0;
-
-	/* reserve data memory and consider alignment */
-	if (fec->rdb_ptr == NULL)
-		fec->rdb_ptr = malloc(size * count + DB_DATA_ALIGNMENT);
-	p = (uint32_t)fec->rdb_ptr;
-	if (!p) {
-		puts("fec_mxc: not enough malloc memory\n");
-		return -ENOMEM;
-	}
-	memset((void *)p, 0, size * count + DB_DATA_ALIGNMENT);
-	p += DB_DATA_ALIGNMENT-1;
-	p &= ~(DB_DATA_ALIGNMENT-1);
-
-	for (ix = 0; ix < count; ix++) {
-		writel(p, &fec->rbd_base[ix].data_pointer);
-		p += size;
-		writew(FEC_RBD_EMPTY, &fec->rbd_base[ix].status);
-		writew(0, &fec->rbd_base[ix].data_length);
-	}
+	uint32_t size;
+	int i;
+
 	/*
-	 * mark the last RBD to close the ring
+	 * Allocate memory for the buffers. This allocation respects the
+	 * alignment
 	 */
-	writew(FEC_RBD_WRAP | FEC_RBD_EMPTY, &fec->rbd_base[ix - 1].status);
+	size = roundup(dsize, CONFIG_FEC_ALIGN);
+	for (i = 0; i < count; i++) {
+		uint32_t data_ptr = readl(&fec->rbd_base[i].data_pointer);
+		if (data_ptr == 0) {
+			uint8_t *data = memalign(CONFIG_FEC_ALIGN,
+						 size);
+			if (!data) {
+				printf("%s: error allocating rxbuf %d\n",
+				       __func__, i);
+				goto err;
+			}
+			writel((uint32_t)data, &fec->rbd_base[i].data_pointer);
+		} /* needs allocation */
+		writew(FEC_RBD_EMPTY, &fec->rbd_base[i].status);
+		writew(0, &fec->rbd_base[i].data_length);
+	}
+
+	/* Mark the last RBD to close the ring. */
+	writew(FEC_RBD_WRAP | FEC_RBD_EMPTY, &fec->rbd_base[i - 1].status);
 	fec->rbd_index = 0;
 
 	return 0;
+
+err:
+	for (; i >= 0; i--) {
+		uint32_t data_ptr = readl(&fec->rbd_base[i].data_pointer);
+		free((void *)data_ptr);
+	}
+
+	return -ENOMEM;
 }
 
 /**
@@ -312,9 +339,13 @@ static int fec_rbd_init(struct fec_priv *fec, int count, int size)
  */
 static void fec_tbd_init(struct fec_priv *fec)
 {
+	unsigned addr = (unsigned)fec->tbd_base;
+	unsigned size = roundup(2 * sizeof(struct fec_bd),
+				CONFIG_FEC_ALIGN);
 	writew(0x0000, &fec->tbd_base[0].status);
 	writew(FEC_TBD_WRAP, &fec->tbd_base[1].status);
 	fec->tbd_index = 0;
+	flush_dcache_range(addr, addr+size);
 }
 
 /**
@@ -324,16 +355,10 @@ static void fec_tbd_init(struct fec_priv *fec)
  */
 static void fec_rbd_clean(int last, struct fec_bd *pRbd)
 {
-	/*
-	 * Reset buffer descriptor as empty
-	 */
+	unsigned short flags = FEC_RBD_EMPTY;
 	if (last)
-		writew(FEC_RBD_WRAP | FEC_RBD_EMPTY, &pRbd->status);
-	else
-		writew(FEC_RBD_EMPTY, &pRbd->status);
-	/*
-	 * no data in it
-	 */
+		flags |= FEC_RBD_WRAP;
+	writew(flags, &pRbd->status);
 	writew(0, &pRbd->data_length);
 }
 
@@ -387,12 +412,25 @@ static int fec_open(struct eth_device *edev)
 {
 	struct fec_priv *fec = (struct fec_priv *)edev->priv;
 	int speed;
+	uint32_t addr, size;
+	int i;
 
 	debug("fec_open: fec_open(dev)\n");
 	/* full-duplex, heartbeat disabled */
 	writel(1 << 2, &fec->eth->x_cntrl);
 	fec->rbd_index = 0;
 
+	/* Invalidate all descriptors */
+	for (i = 0; i < FEC_RBD_NUM - 1; i++)
+		fec_rbd_clean(0, &fec->rbd_base[i]);
+	fec_rbd_clean(1, &fec->rbd_base[i]);
+
+	/* Flush the descriptors into RAM */
+	size = roundup(FEC_RBD_NUM * sizeof(struct fec_bd),
+			CONFIG_FEC_ALIGN);
+	addr = (uint32_t)fec->rbd_base;
+	flush_dcache_range(addr, addr + size);
+
 #ifdef FEC_QUIRK_ENET_MAC
 	/* Enable ENET HW endian SWAP */
 	writel(readl(&fec->eth->ecntrl) | FEC_ECNTRL_DBSWAP,
@@ -478,38 +516,55 @@ static int fec_open(struct eth_device *edev)
 
 static int fec_init(struct eth_device *dev, bd_t* bd)
 {
-	uint32_t base;
 	struct fec_priv *fec = (struct fec_priv *)dev->priv;
 	uint32_t mib_ptr = (uint32_t)&fec->eth->rmon_t_drop;
 	uint32_t rcntrl;
-	int i;
+	uint32_t size;
+	int i, ret;
 
 	/* Initialize MAC address */
 	fec_set_hwaddr(dev);
 
 	/*
-	 * reserve memory for both buffer descriptor chains at once
-	 * Datasheet forces the startaddress of each chain is 16 byte
-	 * aligned
+	 * Allocate transmit descriptors, there are two in total. This
+	 * allocation respects cache alignment.
 	 */
-	if (fec->base_ptr == NULL)
-		fec->base_ptr = malloc((2 + FEC_RBD_NUM) *
-				sizeof(struct fec_bd) + DB_ALIGNMENT);
-	base = (uint32_t)fec->base_ptr;
-	if (!base) {
-		puts("fec_mxc: not enough malloc memory\n");
-		return -ENOMEM;
+	if (!fec->tbd_base) {
+		size = roundup(2 * sizeof(struct fec_bd),
+				CONFIG_FEC_ALIGN);
+		fec->tbd_base = memalign(CONFIG_FEC_ALIGN, size);
+		if (!fec->tbd_base) {
+			ret = -ENOMEM;
+			goto err1;
+		}
+		memset(fec->tbd_base, 0, size);
+		fec_tbd_init(fec);
+		flush_dcache_range((unsigned)fec->tbd_base, size);
 	}
-	memset((void *)base, 0, (2 + FEC_RBD_NUM) *
-			sizeof(struct fec_bd) + DB_ALIGNMENT);
-	base += (DB_ALIGNMENT-1);
-	base &= ~(DB_ALIGNMENT-1);
-
-	fec->rbd_base = (struct fec_bd *)base;
 
-	base += FEC_RBD_NUM * sizeof(struct fec_bd);
-
-	fec->tbd_base = (struct fec_bd *)base;
+	/*
+	 * Allocate receive descriptors. This allocation respects cache
+	 * alignment.
+	 */
+	if (!fec->rbd_base) {
+		size = roundup(FEC_RBD_NUM * sizeof(struct fec_bd),
+				CONFIG_FEC_ALIGN);
+		fec->rbd_base = memalign(CONFIG_FEC_ALIGN, size);
+		if (!fec->rbd_base) {
+			ret = -ENOMEM;
+			goto err2;
+		}
+		memset(fec->rbd_base, 0, size);
+		/*
+		 * Initialize RxBD ring
+		 */
+		if (fec_rbd_init(fec, FEC_RBD_NUM, FEC_MAX_PKT_SIZE) < 0) {
+			ret = -ENOMEM;
+			goto err3;
+		}
+		flush_dcache_range((unsigned)fec->rbd_base,
+				   (unsigned)fec->rbd_base + size);
+	}
 
 	/*
 	 * Set interrupt mask register
@@ -566,23 +621,19 @@ static int fec_init(struct eth_device *dev, bd_t* bd)
 	writel((uint32_t)fec->tbd_base, &fec->eth->etdsr);
 	writel((uint32_t)fec->rbd_base, &fec->eth->erdsr);
 
-	/*
-	 * Initialize RxBD/TxBD rings
-	 */
-	if (fec_rbd_init(fec, FEC_RBD_NUM, FEC_MAX_PKT_SIZE) < 0) {
-		free(fec->base_ptr);
-		fec->base_ptr = NULL;
-		return -ENOMEM;
-	}
-	fec_tbd_init(fec);
-
-
 #ifndef CONFIG_PHYLIB
 	if (fec->xcv_type != SEVENWIRE)
 		miiphy_restart_aneg(dev);
 #endif
 	fec_open(dev);
 	return 0;
+
+err3:
+	free(fec->rbd_base);
+err2:
+	free(fec->tbd_base);
+err1:
+	return ret;
 }
 
 /**
@@ -631,9 +682,11 @@ static void fec_halt(struct eth_device *dev)
  * @param[in] length Data count in bytes
  * @return 0 on success
  */
-static int fec_send(struct eth_device *dev, volatile void* packet, int length)
+static int fec_send(struct eth_device *dev, void *packet, int length)
 {
 	unsigned int status;
+	uint32_t size;
+	uint32_t addr;
 
 	/*
 	 * This routine transmits one frame.  This routine only accepts
@@ -650,15 +703,21 @@ static int fec_send(struct eth_device *dev, volatile void* packet, int length)
 	}
 
 	/*
-	 * Setup the transmit buffer
-	 * Note: We are always using the first buffer for transmission,
-	 * the second will be empty and only used to stop the DMA engine
+	 * Setup the transmit buffer. We are always using the first buffer for
+	 * transmission, the second will be empty and only used to stop the DMA
+	 * engine. We also flush the packet to RAM here to avoid cache trouble.
 	 */
 #ifdef	CONFIG_FEC_MXC_SWAP_PACKET
 	swap_packet((uint32_t *)packet, length);
 #endif
+
+	addr = (uint32_t)packet;
+	size = roundup(length, CONFIG_FEC_ALIGN);
+	flush_dcache_range(addr, addr + size);
+
 	writew(length, &fec->tbd_base[fec->tbd_index].data_length);
-	writel((uint32_t)packet, &fec->tbd_base[fec->tbd_index].data_pointer);
+	writel(addr, &fec->tbd_base[fec->tbd_index].data_pointer);
+
 	/*
 	 * update BD's status now
 	 * This block:
@@ -672,16 +731,30 @@ static int fec_send(struct eth_device *dev, volatile void* packet, int length)
 	writew(status, &fec->tbd_base[fec->tbd_index].status);
 
 	/*
+	 * Flush data cache. This code flushes both TX descriptors to RAM.
+	 * After this code, the descriptors will be safely in RAM and we
+	 * can start DMA.
+	 */
+	size = roundup(2 * sizeof(struct fec_bd), CONFIG_FEC_ALIGN);
+	addr = (uint32_t)fec->tbd_base;
+	flush_dcache_range(addr, addr + size);
+
+	/*
 	 * Enable SmartDMA transmit task
 	 */
 	fec_tx_task_enable(fec);
 
 	/*
-	 * wait until frame is sent .
+	 * Wait until frame is sent. On each turn of the wait cycle, we must
+	 * invalidate data cache to see what's really in RAM. Also, we need
+	 * barrier here.
 	 */
+	invalidate_dcache_range(addr, addr + size);
 	while (readw(&fec->tbd_base[fec->tbd_index].status) & FEC_TBD_READY) {
 		udelay(1);
+		invalidate_dcache_range(addr, addr + size);
 	}
+
 	debug("fec_send: status 0x%x index %d\n",
 			readw(&fec->tbd_base[fec->tbd_index].status),
 			fec->tbd_index);
@@ -707,6 +780,8 @@ static int fec_recv(struct eth_device *dev)
 	int frame_length, len = 0;
 	struct nbuf *frame;
 	uint16_t bd_status;
+	uint32_t addr, size;
+	int i;
 	uchar buff[FEC_MAX_PKT_SIZE];
 
 	/*
@@ -737,8 +812,23 @@ static int fec_recv(struct eth_device *dev)
 	}
 
 	/*
-	 * ensure reading the right buffer status
+	 * Read the buffer status. Before the status can be read, the data cache
+	 * must be invalidated, because the data in RAM might have been changed
+	 * by DMA. The descriptors are properly aligned to cachelines so there's
+	 * no need to worry they'd overlap.
+	 *
+	 * WARNING: By invalidating the descriptor here, we also invalidate
+	 * the descriptors surrounding this one. Therefore we can NOT change the
+	 * contents of this descriptor nor the surrounding ones. The problem is
+	 * that in order to mark the descriptor as processed, we need to change
+	 * the descriptor. The solution is to mark the whole cache line when all
+	 * descriptors in the cache line are processed.
 	 */
+	addr = (uint32_t)rbd;
+	addr &= ~(CONFIG_FEC_ALIGN - 1);
+	size = roundup(sizeof(struct fec_bd), CONFIG_FEC_ALIGN);
+	invalidate_dcache_range(addr, addr + size);
+
 	bd_status = readw(&rbd->status);
 	debug("fec_recv: status 0x%x\n", bd_status);
 
@@ -751,6 +841,13 @@ static int fec_recv(struct eth_device *dev)
 			frame = (struct nbuf *)readl(&rbd->data_pointer);
 			frame_length = readw(&rbd->data_length) - 4;
 			/*
+			 * Invalidate data cache over the buffer
+			 */
+			addr = (uint32_t)frame;
+			size = roundup(frame_length, CONFIG_FEC_ALIGN);
+			invalidate_dcache_range(addr, addr + size);
+
+			/*
 			 *  Fill the buffer and pass it to upper layers
 			 */
 #ifdef	CONFIG_FEC_MXC_SWAP_PACKET
@@ -765,11 +862,25 @@ static int fec_recv(struct eth_device *dev)
 						(ulong)rbd->data_pointer,
 						bd_status);
 		}
+
 		/*
-		 * free the current buffer, restart the engine
-		 * and move forward to the next buffer
+		 * Free the current buffer, restart the engine and move forward
+		 * to the next buffer. Here we check if the whole cacheline of
+		 * descriptors was already processed and if so, we mark it free
+		 * as whole.
 		 */
-		fec_rbd_clean(fec->rbd_index == (FEC_RBD_NUM - 1) ? 1 : 0, rbd);
+		size = RXDESC_PER_CACHELINE - 1;
+		if ((fec->rbd_index & size) == size) {
+			i = fec->rbd_index - size;
+			addr = (uint32_t)&fec->rbd_base[i];
+			for (; i <= fec->rbd_index ; i++) {
+				fec_rbd_clean(i == (FEC_RBD_NUM - 1),
+					      &fec->rbd_base[i]);
+			}
+			flush_dcache_range(addr,
+				addr + CONFIG_FEC_ALIGN);
+		}
+
 		fec_rx_task_enable(fec);
 		fec->rbd_index = (fec->rbd_index + 1) % FEC_RBD_NUM;
 	}
drivers/net/fec_mxc.h +1 -18
index 2eb7803..852b2e0 100644
@@ -234,22 +234,6 @@ struct ethernet_regs {
 #endif
 
 /**
- * @brief Descriptor buffer alignment
- *
- * i.MX27 requires a 16 byte alignment (but for the first element only)
- */
-#define DB_ALIGNMENT		16
-
-/**
- * @brief Data buffer alignment
- *
- * i.MX27 requires a four byte alignment for transmit and 16 bits
- * alignment for receive so take 16
- * Note: Valid for member data_pointer in struct buffer_descriptor
- */
-#define DB_DATA_ALIGNMENT	16
-
-/**
  * @brief Receive & Transmit Buffer Descriptor definitions
  *
  * Note: The first BD must be aligned (see DB_ALIGNMENT)
@@ -282,8 +266,7 @@ struct fec_priv {
 	struct fec_bd *tbd_base;	/* TBD ring */
 	int tbd_index;			/* next transmit BD to write */
 	bd_t *bd;
-	void *rdb_ptr;
-	void *base_ptr;
+	uint8_t *tdb_ptr;
 	int dev_id;
 	int phy_id;
 	struct mii_dev *bus;
Signature · 1 line
1.7.9
#2 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 57

Quoted 8 lines · On Tuesday 13 March 2012 10:04:31 Eric Nelson wrote:
drivers/net/fec_mxc.c +5 -0
 
+#if ARCH_DMA_MINALIGN > CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_FEC_ALIGN ARCH_DMA_MINALIGN
+#else
+#define CONFIG_FEC_ALIGN CONFIG_SYS_CACHELINE_SIZE
+#endif

i don't think this is something you should be checking. if this is an actual problem (and i don't think it is), it's something we should handle in common code. if you need to dma from memory, then use ARCH_DMA_MINALIGN.

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 68

Quoted 2 lines
drivers/net/fec_mxc.c +2 -0
+#error	"CONFIG_FEC_ALIGN must be multiple of 16!"
+#error	"PKTALIGN must be multiple of CONFIG_FEC_ALIGN!"

please don't use tabs after #define/#error/etc... just one space
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120313/644a5ecb/attachment.pgp>

#3 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Thanks Mike,

Quoted 4 lines · Mike Frysinger
Quoted 3 lines · On Tuesday 13 March 2012 10:04:31 Eric Nelson wrote:
drivers/net/fec_mxc.c +0 -0
Quoted 10 lines

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 57

Quoted 5 lines
drivers/net/fec_mxc.c +5 -0
+#if ARCH_DMA_MINALIGN>  CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_FEC_ALIGN ARCH_DMA_MINALIGN
+#else
+#define CONFIG_FEC_ALIGN CONFIG_SYS_CACHELINE_SIZE
+#endif

i don't think this is something you should be checking. if this is an actual problem (and i don't think it is), it's something we should handle in common code. if you need to dma from memory, then use ARCH_DMA_MINALIGN.

Marek, you've mentioned some restrictions for other i.MX devices.

Are you aware of any problem collapsing this?

Note that other CPUs will need to have CONFIG_SYS_CACHELINE_SIZE to prevent the default of 64.

Quoted 4 lines

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 68

Quoted 2 lines
drivers/net/fec_mxc.c +2 -0
+#error	"CONFIG_FEC_ALIGN must be multiple of 16!"
+#error	"PKTALIGN must be multiple of CONFIG_FEC_ALIGN!"

please don't use tabs after #define/#error/etc... just one space

Ok. I'll address in V4.

#4 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Quoted 16 lines · On Tuesday 13 March 2012 14:42:29 Eric Nelson wrote:
Quoted 4 lines · Mike Frysinger
Quoted 3 lines · On Tuesday 13 March 2012 10:04:31 Eric Nelson wrote:
drivers/net/fec_mxc.c +0 -0
Quoted 10 lines

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 57

Quoted 5 lines
drivers/net/fec_mxc.c +5 -0
+#if ARCH_DMA_MINALIGN>  CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_FEC_ALIGN ARCH_DMA_MINALIGN
+#else
+#define CONFIG_FEC_ALIGN CONFIG_SYS_CACHELINE_SIZE
+#endif

i don't think this is something you should be checking. if this is an actual problem (and i don't think it is), it's something we should handle in common code. if you need to dma from memory, then use ARCH_DMA_MINALIGN.

Quoted 6 lines

Marek, you've mentioned some restrictions for other i.MX devices.

Are you aware of any problem collapsing this?

Note that other CPUs will need to have CONFIG_SYS_CACHELINE_SIZE to prevent the default of 64.

and those cores should be making sure that ARCH_DMA_MINALIGN is sufficient to handle those larger cache lines. this define provides two meanings: minimum alignment for the DMA itself, and for sanely flushing the cache on that dma buffer. so there should be no situation where ARCH_DMA_MINALIGN is smaller than the cacheline size.

the few boards i see defining CONFIG_SYS_CACHELINE_SIZE to 64 are all ARM based, and ARM sets ARCH_DMA_MINALIGN to CONFIG_SYS_CACHELINE_SIZE if it's defined
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120313/4b062dc8/attachment.pgp>

#5 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Dear Mike Frysinger,

Quoted 30 lines · On Tuesday 13 March 2012 14:42:29 Eric Nelson wrote:
Quoted 16 lines · Mike Frysinger
Quoted 14 lines · On Tuesday 13 March 2012 10:04:31 Eric Nelson wrote:

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 57

Quoted 8 lines
drivers/net/fec_mxc.c +5 -0
 
+#if ARCH_DMA_MINALIGN>  CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_FEC_ALIGN ARCH_DMA_MINALIGN
+#else
+#define CONFIG_FEC_ALIGN CONFIG_SYS_CACHELINE_SIZE
+#endif

i don't think this is something you should be checking. if this is an actual problem (and i don't think it is), it's something we should handle in common code. if you need to dma from memory, then use ARCH_DMA_MINALIGN.

Quoted 6 lines

Marek, you've mentioned some restrictions for other i.MX devices.

Are you aware of any problem collapsing this?

Note that other CPUs will need to have CONFIG_SYS_CACHELINE_SIZE to prevent the default of 64.

and those cores should be making sure that ARCH_DMA_MINALIGN is sufficient to handle those larger cache lines. this define provides two meanings:
minimum alignment for the DMA itself, and for sanely flushing the cache on that dma buffer. so there should be no situation where ARCH_DMA_MINALIGN is smaller than the cacheline size.

Quoted 4 lines

the few boards i see defining CONFIG_SYS_CACHELINE_SIZE to 64 are all ARM based, and ARM sets ARCH_DMA_MINALIGN to CONFIG_SYS_CACHELINE_SIZE if it's defined
-mike

Eric, can you check also PPC /wrt to this? This driver is also used on PPC.

Best regards,
Marek Vasut

#6 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Quoted 32 lines · Marek Vasut

Dear Mike Frysinger,

Quoted 30 lines · On Tuesday 13 March 2012 14:42:29 Eric Nelson wrote:
Quoted 16 lines · Mike Frysinger
Quoted 14 lines · On Tuesday 13 March 2012 10:04:31 Eric Nelson wrote:

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 57

Quoted 8 lines
drivers/net/fec_mxc.c +5 -0
 
+#if ARCH_DMA_MINALIGN>   CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_FEC_ALIGN ARCH_DMA_MINALIGN
+#else
+#define CONFIG_FEC_ALIGN CONFIG_SYS_CACHELINE_SIZE
+#endif

i don't think this is something you should be checking. if this is an actual problem (and i don't think it is), it's something we should handle in common code. if you need to dma from memory, then use ARCH_DMA_MINALIGN.

Quoted 6 lines

Marek, you've mentioned some restrictions for other i.MX devices.

Are you aware of any problem collapsing this?

Note that other CPUs will need to have CONFIG_SYS_CACHELINE_SIZE to prevent the default of 64.

and those cores should be making sure that ARCH_DMA_MINALIGN is sufficient to handle those larger cache lines. this define provides two meanings:
minimum alignment for the DMA itself, and for sanely flushing the cache on that dma buffer. so there should be no situation where ARCH_DMA_MINALIGN is smaller than the cacheline size.

Quoted 7 lines
Quoted 4 lines

the few boards i see defining CONFIG_SYS_CACHELINE_SIZE to 64 are all ARM based, and ARM sets ARCH_DMA_MINALIGN to CONFIG_SYS_CACHELINE_SIZE if it's defined
-mike

Eric, can you check also PPC /wrt to this? This driver is also used on PPC.

Hi Marek,

I'm not seeing any reference to FEC_MXC on PPC. All boards that use it appear to be i.MX:

~/u-boot-imx6$ grep -w CONFIG_FEC_MXC include/configs/*.h include/configs/flea3.h:#define CONFIG_FEC_MXC include/configs/imx27lite-common.h:#define CONFIG_FEC_MXC
include/configs/m28evk.h:#define CONFIG_FEC_MXC
include/configs/mx25pdk.h:#define CONFIG_FEC_MXC include/configs/mx28evk.h:#define CONFIG_FEC_MXC include/configs/mx35pdk.h:#define CONFIG_FEC_MXC include/configs/mx51evk.h:#define CONFIG_FEC_MXC include/configs/mx53evk.h:#define CONFIG_FEC_MXC include/configs/mx53loco.h:#define CONFIG_FEC_MXC include/configs/mx53smd.h:#define CONFIG_FEC_MXC
include/configs/mx6qarm2.h:#define CONFIG_FEC_MXC
include/configs/mx6qsabrelite.h:#define CONFIG_FEC_MXC
include/configs/tx25.h:#define CONFIG_FEC_MXC include/configs/vision2.h:#define CONFIG_FEC_MXC include/configs/zmx25.h:#define CONFIG_FEC_MXC

Most of the PPC devices seem to have values of 16 or 32 for ARCH_DMA_MINALIGN, but PPC64BRIDGE and E500MC would have a problem if their drivers don't implement a bounce buffer because PKTALIGN < ARCH_DMA_MINALIGN.

(see arch/powerpc/include/asm/cache.h)

This condition is properly tested for in fec_mxc.c.

Regards,

Eric

#7 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Quoted 8 lines · On Wednesday 14 March 2012 01:12:38 Eric Nelson wrote:

Most of the PPC devices seem to have values of 16 or 32 for ARCH_DMA_MINALIGN, but PPC64BRIDGE and E500MC would have a problem if their drivers don't implement a bounce buffer because PKTALIGN < ARCH_DMA_MINALIGN.

(see arch/powerpc/include/asm/cache.h)

This condition is properly tested for in fec_mxc.c.

so fix this in common code instead of hacking around it in individual drivers. seems to me that PKTALIGN should be defined to ARCH_DMA_MINALIGN and ultimately removed.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120314/3c90b478/attachment.pgp>

#8 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Quoted 6 lines · Mike Frysinger
Quoted 5 lines · On Wednesday 14 March 2012 01:12:38 Eric Nelson wrote:

Most of the PPC devices seem to have values of 16 or 32 for ARCH_DMA_MINALIGN, but PPC64BRIDGE and E500MC would have a problem if their drivers don't implement a bounce buffer because PKTALIGN< ARCH_DMA_MINALIGN.

Quoted 8 lines
Quoted 3 lines

(see arch/powerpc/include/asm/cache.h)

This condition is properly tested for in fec_mxc.c.

so fix this in common code instead of hacking around it in individual drivers. seems to me that PKTALIGN should be defined to ARCH_DMA_MINALIGN and ultimately removed.
-mike

Hi Mike,

I'm not in a position to test against MAKEALL, but it appears that all architectures have cache.h and define ARCH_DMA_MINALIGN, so it should be trivially easy to fix PKTALIGN to be at least ARCH_DMA_MINALIGN as shown below.

PKTSIZE_ALIGN seems safe for all architectures at 1536.

Note that this will reduce the value to 16 for some PPC devices, but I haven't found any place that this would break things.

Is this what you're after?

Are you in a position to run MAKEALL or can you advise about the requirements?

Please advise,

Eric

~/u-boot-imx6$ git diff

include/net.h +2 -1
index e4d42c2..ff428d0 100644
@@ -16,6 +16,7 @@
  #include <commproc.h>
  #endif /* CONFIG_8xx */
 
+#include <asm/cache.h>
  #include <asm/byteorder.h>     /* for nton* / ntoh* stuff */
 
 
@@ -31,7 +32,7 @@
  # define PKTBUFSRX     4
  #endif
 
-#define PKTALIGN       32
+#define PKTALIGN       ARCH_DMA_MINALIGN
 
  /* IPv4 addresses are always 32 bits in size */
  typedef u32            IPaddr_t;
#9 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Quoted 9 lines · On Wednesday 14 March 2012 15:12:10 Eric Nelson wrote:
Quoted 8 lines · Mike Frysinger
Quoted 7 lines · On Wednesday 14 March 2012 01:12:38 Eric Nelson wrote:

Most of the PPC devices seem to have values of 16 or 32 for ARCH_DMA_MINALIGN, but PPC64BRIDGE and E500MC would have a problem if their drivers don't implement a bounce buffer because PKTALIGN< ARCH_DMA_MINALIGN.

(see arch/powerpc/include/asm/cache.h)

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

This condition is properly tested for in fec_mxc.c.

so fix this in common code instead of hacking around it in individual drivers. seems to me that PKTALIGN should be defined to ARCH_DMA_MINALIGN and ultimately removed.

I'm not in a position to test against MAKEALL, but it appears that all architectures have cache.h and define ARCH_DMA_MINALIGN

ARCH_DMA_MINALIGN is required. if an arch/board omits it, they are broken and you need not worry about it. we already have common code requiring int.

include/net.h line 35

Quoted 5 lines
include/net.h +1 -1
 
-#define PKTALIGN       32
+#define PKTALIGN       ARCH_DMA_MINALIGN

looks fine to me
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120314/660cd39a/attachment.pgp>

#10 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Quoted 16 lines · Mike Frysinger
Quoted 15 lines · On Wednesday 14 March 2012 15:12:10 Eric Nelson wrote:
Quoted 13 lines · Mike Frysinger
Quoted 8 lines · On Wednesday 14 March 2012 01:12:38 Eric Nelson wrote:

Most of the PPC devices seem to have values of 16 or 32 for ARCH_DMA_MINALIGN, but PPC64BRIDGE and E500MC would have a problem if their drivers don't implement a bounce
buffer because PKTALIGN< ARCH_DMA_MINALIGN.

(see arch/powerpc/include/asm/cache.h)

This condition is properly tested for in fec_mxc.c.

so fix this in common code instead of hacking around it in individual drivers. seems to me that PKTALIGN should be defined to ARCH_DMA_MINALIGN and ultimately removed.

Quoted 6 lines
Quoted 2 lines

I'm not in a position to test against MAKEALL, but it appears that all architectures have cache.h and define ARCH_DMA_MINALIGN

ARCH_DMA_MINALIGN is required. if an arch/board omits it, they are broken and you need not worry about it. we already have common code requiring int.

Sounds good.

Quoted 8 lines

include/net.h line 35

Quoted 5 lines
include/net.h +1 -1
 
-#define PKTALIGN       32
+#define PKTALIGN       ARCH_DMA_MINALIGN

looks fine to me
-mike

You want I should send a formal patch?

Should I consider "looks fine" to be an ack?

If so, I'll also send an update (V5) to fec_mxc that removes the check on PKTALIGN.

Please advise,

Eric

#11 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Quoted 1 line · On Wednesday 14 March 2012 17:04:32 Eric Nelson wrote:

You want I should send a formal patch?

yes please

Quoted 1 line

Should I consider "looks fine" to be an ack?

i'll post an acked-by tag to that and then patchwork will do the right thing for people to track
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120314/d7ece295/attachment.pgp>

#12 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

In reply to Eric Nelson

Dear Eric Nelson,

Quoted 38 lines · Marek Vasut
Quoted 37 lines

Dear Mike Frysinger,

Quoted 34 lines · On Tuesday 13 March 2012 14:42:29 Eric Nelson wrote:
Quoted 16 lines · Mike Frysinger
Quoted 14 lines · On Tuesday 13 March 2012 10:04:31 Eric Nelson wrote:

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 57

Quoted 8 lines
drivers/net/fec_mxc.c +5 -0
 
+#if ARCH_DMA_MINALIGN>   CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_FEC_ALIGN ARCH_DMA_MINALIGN
+#else
+#define CONFIG_FEC_ALIGN CONFIG_SYS_CACHELINE_SIZE
+#endif

i don't think this is something you should be checking. if this is an actual problem (and i don't think it is), it's something we should handle in common code. if you need to dma from memory, then use ARCH_DMA_MINALIGN.

Quoted 6 lines

Marek, you've mentioned some restrictions for other i.MX devices.

Are you aware of any problem collapsing this?

Note that other CPUs will need to have CONFIG_SYS_CACHELINE_SIZE to prevent the default of 64.

and those cores should be making sure that ARCH_DMA_MINALIGN is sufficient to handle those larger cache lines. this define provides two meanings: minimum alignment for the DMA itself, and for sanely flushing the cache on that dma buffer. so there should be no situation where ARCH_DMA_MINALIGN is smaller than the cacheline size.

the few boards i see defining CONFIG_SYS_CACHELINE_SIZE to 64 are all ARM based, and ARM sets ARCH_DMA_MINALIGN to CONFIG_SYS_CACHELINE_SIZE if it's defined
-mike

Quoted 7 lines
Quoted 2 lines

Eric, can you check also PPC /wrt to this? This driver is also used on PPC.

Hi Marek,

I'm not seeing any reference to FEC_MXC on PPC. All boards that use it appear to be i.MX:

I see, my mistake. Though the PPC systems should be converted ;-)

Quoted 18 lines

~/u-boot-imx6$ grep -w CONFIG_FEC_MXC include/configs/*.h include/configs/flea3.h:#define CONFIG_FEC_MXC include/configs/imx27lite-common.h:#define CONFIG_FEC_MXC
include/configs/m28evk.h:#define CONFIG_FEC_MXC
include/configs/mx25pdk.h:#define CONFIG_FEC_MXC include/configs/mx28evk.h:#define CONFIG_FEC_MXC include/configs/mx35pdk.h:#define CONFIG_FEC_MXC include/configs/mx51evk.h:#define CONFIG_FEC_MXC include/configs/mx53evk.h:#define CONFIG_FEC_MXC include/configs/mx53loco.h:#define CONFIG_FEC_MXC include/configs/mx53smd.h:#define CONFIG_FEC_MXC
include/configs/mx6qarm2.h:#define CONFIG_FEC_MXC
include/configs/mx6qsabrelite.h:#define CONFIG_FEC_MXC
include/configs/tx25.h:#define CONFIG_FEC_MXC include/configs/vision2.h:#define CONFIG_FEC_MXC include/configs/zmx25.h:#define CONFIG_FEC_MXC

Quoted 4 lines

Most of the PPC devices seem to have values of 16 or 32 for ARCH_DMA_MINALIGN, but PPC64BRIDGE and E500MC would have a problem if their drivers don't implement a bounce buffer because PKTALIGN < ARCH_DMA_MINALIGN.

Awww, leave the check there then, those PPC systems should produce warning when they will be flipped so this is not forgotten.

Quoted 9 lines

(see arch/powerpc/include/asm/cache.h)

This condition is properly tested for in fec_mxc.c.

Regards,

Eric

Best regards,
Marek Vasut

#13 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

In reply to Eric Nelson

Dear Eric Nelson,

Quoted 18 lines

Thanks Mike,

Quoted 4 lines · Mike Frysinger
Quoted 3 lines · On Tuesday 13 March 2012 10:04:31 Eric Nelson wrote:
drivers/net/fec_mxc.c +0 -0
Quoted 10 lines

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 57

Quoted 5 lines
drivers/net/fec_mxc.c +5 -0
+#if ARCH_DMA_MINALIGN>  CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_FEC_ALIGN ARCH_DMA_MINALIGN
+#else
+#define CONFIG_FEC_ALIGN CONFIG_SYS_CACHELINE_SIZE
+#endif

i don't think this is something you should be checking. if this is an actual problem (and i don't think it is), it's something we should handle in common code. if you need to dma from memory, then use ARCH_DMA_MINALIGN.

Quoted 3 lines

Marek, you've mentioned some restrictions for other i.MX devices.

Are you aware of any problem collapsing this?

Well yes, there exists a CPU which has 32byte cacheline, but FEC such that needs DMA areas aligned to 16 bytes. That's why you need higher of those two to be safe.

Quoted 3 lines

Note that other CPUs will need to have CONFIG_SYS_CACHELINE_SIZE to prevent the default of 64.

Agreed

Quoted 7 lines
Quoted 5 lines

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 68

Quoted 3 lines
drivers/net/fec_mxc.c +2 -0
+#error	"CONFIG_FEC_ALIGN must be multiple of 16!"
+#error	"PKTALIGN must be multiple of CONFIG_FEC_ALIGN!"

please don't use tabs after #define/#error/etc... just one space

Ok. I'll address in V4.

This is actually my work, Mike, I just love it neatly aligned with tabs :-p

Best regards,
Marek Vasut

#14 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

In reply to Eric Nelson
Quoted 24 lines · Eric Nelson

ensure that transmit and receive buffers are cache-line aligned
invalidate cache for each packet as received
update receive buffer descriptors one cache line at a time
flush cache before transmitting

Original patch by Marek:

	http://lists.denx.de/pipermail/u-boot/2012-February/117695.html

---
V2 addresses some concerns from the ML:
- Use readl()/writel() instead of mapped data structure accesses
- Wrong comment style
-&rbd_base[0] == rbd_base
removed 'volatile' from fec_send().

<snip>

drivers/net/fec_mxc.c +0 -0
index 1fdd071..94a3927 100644

...

drivers/net/fec_mxc.c static void fec_halt(struct eth_device *dev) · line 687

Quoted 8 lines
drivers/net/fec_mxc.c +1 -1
@@ -631,9 +682,11 @@ static void fec_halt(struct eth_device *dev)
   * @param[in] length Data count in bytes
   * @return 0 on success
   */
-static int fec_send(struct eth_device *dev, volatile void* packet, int length)
+static int fec_send(struct eth_device *dev, void *packet, int length)
  {
  	unsigned int status;

I made this change to keep checkpatch happy (it doesn't like volatile), but the declaration of struct eth_device in include/net.h seems to want the volatile:

	int  (*send) (struct eth_device*, volatile void* packet, int length);

I'd rather have a checkpatch warning than a compiler warning, so I'll fix this in V4...

#15 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

Dear Eric Nelson,

Quoted 52 lines · Eric Nelson
Quoted 30 lines

ensure that transmit and receive buffers are cache-line aligned

invalidate cache for each packet as received

update receive buffer descriptors one cache line at a time

flush cache before transmitting

Original patch by Marek:

	http://lists.denx.de/pipermail/u-boot/2012-February/117695.html

---

V2 addresses some concerns from the ML:
- Use readl()/writel() instead of mapped data structure

accesses

- Wrong comment style
-&rbd_base[0] == rbd_base
removed 'volatile' from fec_send().

<snip>

drivers/net/fec_mxc.c +0 -0
index 1fdd071..94a3927 100644

...

Quoted 10 lines
drivers/net/fec_mxc.c +0 -1
@@ -631,9 +682,11 @@ static void fec_halt(struct eth_device *dev)
 
   * @param[in] length Data count in bytes
   * @return 0 on success
   */
 
-static int fec_send(struct eth_device *dev, volatile void* packet, int

length) +static int fec_send(struct eth_device *dev, void *packet, int length)

Quoted 3 lines
  {

unsigned int status;

I made this change to keep checkpatch happy (it doesn't like volatile), but the declaration of struct eth_device in include/net.h seems to want the volatile:

Quoted 4 lines
	int  (*send) (struct eth_device*, volatile void* packet, int length);

I'd rather have a checkpatch warning than a compiler warning, so I'll fix this in V4...

I believe the volatile is there for a reason (not a reason meaningful for this device though). I believe on some boards, this was used to avoid some cache trouble or such.

Best regards,
Marek Vasut

#16 lore raw

PATCH v3 1/1 net: fec_mxc: allow use with cache enabled

In reply to Eric Nelson

Dear Eric Nelson,

Quoted 32 lines

ensure that transmit and receive buffers are cache-line aligned invalidate cache for each packet as received
update receive buffer descriptors one cache line at a time flush cache before transmitting

Original patch by Marek:

	http://lists.denx.de/pipermail/u-boot/2012-February/117695.html

---
V2 addresses some concerns from the ML:
- Use readl()/writel() instead of mapped data structure accesses
- Wrong comment style
- &rbd_base[0] == rbd_base
removed 'volatile' from fec_send().

V3 updates from ML (and Marek):
consolidated CONFIG_FEC_DATA_ALIGNMENT and CONFIG_FEC_DESC_ALIGNMENT added cache flushes after initialization of TBD/RBD

 drivers/net/fec_mxc.c |  265
+++++++++++++++++++++++++++++++++++-------------- drivers/net/fec_mxc.h | 
 19 +----
 2 files changed, 189 insertions(+), 95 deletions(-)
drivers/net/fec_mxc.c +0 -0
index 1fdd071..94a3927 100644

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 57

Quoted 8 lines
drivers/net/fec_mxc.c +5 -0
@@ -50,6 +50,24 @@ DECLARE_GLOBAL_DATA_PTR;
 #define	CONFIG_FEC_MXC_SWAP_PACKET
 #endif
 
+#if ARCH_DMA_MINALIGN > CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_FEC_ALIGN ARCH_DMA_MINALIGN
+#else
+#define CONFIG_FEC_ALIGN CONFIG_SYS_CACHELINE_SIZE
+#endif

Check PPC and let's go with ARCH_DMA_MINALIGN

drivers/net/fec_mxc.c DECLARE_GLOBAL_DATA_PTR; · line 69

Quoted 12 lines
drivers/net/fec_mxc.c +12 -0
+
+#define RXDESC_PER_CACHELINE	(CONFIG_FEC_ALIGN/sizeof(struct fec_bd))
+
+/* Check various alignment issues at compile time */
+#if ((CONFIG_FEC_ALIGN < 16) || (CONFIG_FEC_ALIGN % 16 != 0))
+#error	"CONFIG_FEC_ALIGN must be multiple of 16!"
+#endif
+
+#if ((PKTALIGN < CONFIG_FEC_ALIGN) || \
+	(PKTALIGN % CONFIG_FEC_ALIGN != 0))
+#error	"PKTALIGN must be multiple of CONFIG_FEC_ALIGN!"
+#endif

We should keep these checks in case some obscure platform appears.

Otherwise, we're almost there Eric! Great work !!

Best regards,
Marek Vasut