net/designware: fixes for data cache, phylib and burst size

Pulling together various fixes for designware into a single series. I'm calling this v3, although some of these never had a v2.

Most of these have been floating around for a few weeks, the cache line issues in particular mean that the driver is unlikely to work on any ARM system. I've been testing these on top of my sunxi mainlining series.

I've also thrown them onto gitorious. Pull-request details are below.

Thanks,
Ian.

The following changes since commit dda0dbfc69f3d560c87f5be85f127ed862ea6721:

Prepare v2014.04 (2014-04-14 15:19:24 -0400)

are available in the git repository at:

git://gitorious.org/ijc/u-boot.git designware

for you to fetch changes up to 5e3e5c37bc7a867ca42fa43f69ad764c8942b631:

net/designware: Make DMA burst length configurable and reduce by default (2014-05-08 22:16:33 +0100)

----------------------------------------------------------------
Ian Campbell (5):
net/designware: call phy_connect_dev() to properly setup phylib device net/designware: ensure device private data is DMA aligned. net/designware: ensure cache invalidations are aligned to ARCH_DMA_MINALIGN net/designware: reorder struct dw_eth_dev to pack more efficiently. net/designware: Make DMA burst length configurable and reduce by default

 drivers/net/designware.c | 25 ++++++++++++++++++-------
 drivers/net/designware.h | 22 +++++++++++-----------
 2 files changed, 29 insertions(+), 18 deletions(-)

This sets up the linkage from the phydev back to the ethernet device. This symptom of not doing this which I noticed was:
<NULL> Waiting for PHY auto negotiation to complete....
rather than:
dwmac.1c50000 Waiting for PHY auto negotiation to complete....

---
 drivers/net/designware.c | 2 ++
 1 file changed, 2 insertions(+)

index c45593b..78751b2 100644
@@ -390,6 +390,8 @@ static int dw_phy_init(struct eth_device *dev)
 	if (!phydev)
 		return -1;
 
+	phy_connect_dev(phydev, dev);
+
 	phydev->supported &= PHY_GBIT_FEATURES;
 	phydev->advertising = phydev->supported;
 

1.9.0

struct dw_eth_dev contains fields which are accessed via DMA, so make sure it is aligned to a dma boundary. Without this I see:

    ERROR: v7_dcache_inval_range - start address is not aligned - 0x7fb677e0

---
v2: Sign of with my own mail not my work mail. I made these changes on my own time, but a .gitconfig vcsh mismerge caused "git commit -s" to use the wrong thing. (I wondered why everyone was CCing me at work...)

---
 drivers/net/designware.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 78751b2..41ab3ac 100644
@@ -414,7 +414,8 @@ int designware_initialize(ulong base_addr, u32 interface)
 	 * Since the priv structure contains the descriptors which need a strict
 	 * buswidth alignment, memalign is used to allocate memory
 	 */
-	priv = (struct dw_eth_dev *) memalign(16, sizeof(struct dw_eth_dev));
+	priv = (struct dw_eth_dev *) memalign(ARCH_DMA_MINALIGN,
+					      sizeof(struct dw_eth_dev));
 	if (!priv) {
 		free(dev);
 		return -ENOMEM;

1.9.0

This is required at least on ARM.

When sending instead of simply invalidating the entire descriptor, flush as little as possible while still respecting ARCH_DMA_MINALIGN, as requested by Alexey.

---
v2: - collapsed "net/designware: align cache invalidation on rx" and
"net/designware: invalidate entire descriptor in dw_eth_send" into one.
- roundup sizeof(txrx_status) to ARCH_DMA_MINALIGN instead of just invalidating the entire descriptor.

---
 drivers/net/designware.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

index 41ab3ac..fa816bf 100644
@@ -280,10 +280,18 @@ static int dw_eth_send(struct eth_device *dev, void *packet, int length)
 	u32 desc_num = priv->tx_currdescnum;
 	struct dmamacdescr *desc_p = &priv->tx_mac_descrtable[desc_num];
 
-	/* Invalidate only "status" field for the following check */
-	invalidate_dcache_range((unsigned long)&desc_p->txrx_status,
-				(unsigned long)&desc_p->txrx_status +
-				sizeof(desc_p->txrx_status));
+	/*
+	 * Strictly we only need to invalidate the "txrx_status" field
+	 * for the following check, but on some platforms we cannot
+	 * invalidate only 4 bytes, so roundup to
+	 * ARCH_DMA_MINALIGN. This is safe because the individual
+	 * descriptors in the array are each aligned to
+	 * ARCH_DMA_MINALIGN.
+	 */
+	invalidate_dcache_range(
+		(unsigned long)desc_p,
+		(unsigned long)desc_p +
+		roundup(sizeof(desc_p->txrx_status), ARCH_DMA_MINALIGN));
 
 	/* Check if the descriptor is owned by CPU */
 	if (desc_p->txrx_status & DESC_TXSTS_OWNBYDMA) {

@@ -351,7 +359,7 @@ static int dw_eth_recv(struct eth_device *dev)
 		/* Invalidate received data */
 		invalidate_dcache_range((unsigned long)desc_p->dmamac_addr,
 					(unsigned long)desc_p->dmamac_addr +
-					length);
+					roundup(length, ARCH_DMA_MINALIGN));
 
 		NetReceive(desc_p->dmamac_addr, length);
 

1.9.0

The {tx,rx}_mac_descrtable fields are aligned to ARCH_DMA_MINALIGN, which could be 256 or even larger. That means there is a potentially huge hole in the struct before those fields, so move them to the front where they are better packed.

Moving them to the front also helps ensure that so long as dw_eth_dev is properly aligned (which it is since "net/designware: ensure device private data is DMA aligned.") the {tx,rx}_mac_descrtable will be too, or at least avoids having to worry too much about compiler specifics.

The {r,t}xbuffs fields also need to be aligned. Previously this was done implicitly because they immediately followed the descriptor tables. Make this explicit and also move to the head of the struct.

---
v3: Also align tx and rx bufs

---
 drivers/net/designware.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 382b0c7..eac5e72 100644
@@ -215,15 +215,15 @@ struct dmamacdescr {
 #endif
 
 struct dw_eth_dev {
+	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
+	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
+
 	u32 interface;
 	u32 tx_currdescnum;
 	u32 rx_currdescnum;
 
-	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
-	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
-
-	char txbuffs[TX_TOTAL_BUFSIZE];
-	char rxbuffs[RX_TOTAL_BUFSIZE];
+	char txbuffs[TX_TOTAL_BUFSIZE] __aligned(ARCH_DMA_MINALIGN);
+	char rxbuffs[RX_TOTAL_BUFSIZE] __aligned(ARCH_DMA_MINALIGN);
 
 	struct eth_mac_regs *mac_regs_p;
 	struct eth_dma_regs *dma_regs_p;

1.9.0

Looks like I managed to not actually commit the move of the field to the head of the struct! v3.1 follows....

Ian.

8<------------

The correct value for this setting can vary across SoCs and boards, so make it configurable.

Also reduce the default value to 8, which is the same default as used in the Linux driver.

---
v3: Pulled into this series instead of sunxi series.

---
 drivers/net/designware.c |  2 +-
 drivers/net/designware.h | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

index fa816bf..7186e3b 100644
@@ -249,7 +249,7 @@ static int dw_eth_init(struct eth_device *dev, bd_t *bis)
 	rx_descs_init(dev);
 	tx_descs_init(dev);
 
-	writel(FIXEDBURST | PRIORXTX_41 | BURST_16, &dma_p->busmode);
+	writel(FIXEDBURST | PRIORXTX_41 | DMA_PBL, &dma_p->busmode);
 
 	writel(readl(&dma_p->opmode) | FLUSHTXFIFO | STOREFORWARD,
 	       &dma_p->opmode);

index eac5e72..e5b5e20 100644
@@ -77,18 +77,18 @@ struct eth_dma_regs {
 
 #define DW_DMA_BASE_OFFSET	(0x1000)
 
+/* Default DMA Burst length */
+#ifndef CONFIG_DW_GMAC_DEFAULT_DMA_PBL
+#define CONFIG_DW_GMAC_DEFAULT_DMA_PBL 8
+#endif
+
 /* Bus mode register definitions */
 #define FIXEDBURST		(1 << 16)
 #define PRIORXTX_41		(3 << 14)
 #define PRIORXTX_31		(2 << 14)
 #define PRIORXTX_21		(1 << 14)
 #define PRIORXTX_11		(0 << 14)
-#define BURST_1			(1 << 8)
-#define BURST_2			(2 << 8)
-#define BURST_4			(4 << 8)
-#define BURST_8			(8 << 8)
-#define BURST_16		(16 << 8)
-#define BURST_32		(32 << 8)
+#define DMA_PBL			(CONFIG_DW_GMAC_DEFAULT_DMA_PBL<<8)
 #define RXHIGHPRIO		(1 << 1)
 #define DMAMAC_SRST		(1 << 0)
 

1.9.0

Hi Ian,

1/5 was already applied by Tom (but still assigned to me on Patchwork).

2/5 to 5/5 (with v3.1 of 4/5) applied to u-boot-arm/master, thanks!

Amicalement,

Albert.