[U-Boot] [PATCH v3 0/5] 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....
Signed-off-by: Ian Campbell ijc@hellion.org.uk Cc: Alexey Brodkin Alexey.Brodkin@synopsys.com --- drivers/net/designware.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index c45593b..78751b2 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -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;
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
Signed-off-by: Ian Campbell ijc@hellion.org.uk Reviewed-by: Alexey Brodkin abrodkin@synopsys.com Acked-by: Marek Vasut marex@denx.de --- 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(-)
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 78751b2..41ab3ac 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -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;
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.
Signed-off-by: Ian Campbell ijc@hellion.org.uk Cc: Alexey Brodkin abrodkin@synopsys.com --- 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(-)
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 41ab3ac..fa816bf 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -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);
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.
Signed-off-by: Ian Campbell ijc@hellion.org.uk Cc: Alexey Brodkin abrodkin@synopsys.com --- v3: Also align tx and rx bufs --- drivers/net/designware.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/designware.h b/drivers/net/designware.h index 382b0c7..eac5e72 100644 --- a/drivers/net/designware.h +++ b/drivers/net/designware.h @@ -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;
On Thu, 2014-05-08 at 22:26 +0100, Ian Campbell wrote:
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.
Looks like I managed to not actually commit the move of the field to the head of the struct! v3.1 follows....
Ian.
8<------------
From 2937ba01841887317f6792709ed57cb86b5fc0cd Mon Sep 17 00:00:00 2001
From: Ian Campbell ijc@hellion.org.uk Date: Thu, 1 May 2014 19:45:15 +0100 Subject: [PATCH] net/designware: reorder struct dw_eth_dev to pack more efficiently.
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.
Signed-off-by: Ian Campbell ijc@hellion.org.uk Cc: Alexey Brodkin abrodkin@synopsys.com --- v3.1: Actually move to the head of the struct, like the commit log said... v3: Also align tx and rx bufs --- drivers/net/designware.h | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/net/designware.h b/drivers/net/designware.h index 382b0c7..de2fdcb 100644 --- a/drivers/net/designware.h +++ b/drivers/net/designware.h @@ -215,15 +215,14 @@ struct dmamacdescr { #endif
struct dw_eth_dev { - 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] __aligned(ARCH_DMA_MINALIGN); + char rxbuffs[RX_TOTAL_BUFSIZE] __aligned(ARCH_DMA_MINALIGN);
- char txbuffs[TX_TOTAL_BUFSIZE]; - char rxbuffs[RX_TOTAL_BUFSIZE]; + u32 interface; + u32 tx_currdescnum; + u32 rx_currdescnum;
struct eth_mac_regs *mac_regs_p; struct eth_dma_regs *dma_regs_p;
On Wed, 14 May 2014 19:30:29 +0100 Ian Campbell ijc@hellion.org.uk wrote:
On Thu, 2014-05-08 at 22:26 +0100, Ian Campbell wrote:
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.
Looks like I managed to not actually commit the move of the field to the head of the struct! v3.1 follows....
Ian.
8<------------
From 2937ba01841887317f6792709ed57cb86b5fc0cd Mon Sep 17 00:00:00 2001 From: Ian Campbell ijc@hellion.org.uk Date: Thu, 1 May 2014 19:45:15 +0100 Subject: [PATCH] net/designware: reorder struct dw_eth_dev to pack more efficiently.
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.
Signed-off-by: Ian Campbell ijc@hellion.org.uk Cc: Alexey Brodkin abrodkin@synopsys.com
v3.1: Actually move to the head of the struct, like the commit log said... v3: Also align tx and rx bufs
drivers/net/designware.h | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/net/designware.h b/drivers/net/designware.h index 382b0c7..de2fdcb 100644 --- a/drivers/net/designware.h +++ b/drivers/net/designware.h @@ -215,15 +215,14 @@ struct dmamacdescr { #endif
struct dw_eth_dev {
- 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] __aligned(ARCH_DMA_MINALIGN);
- char rxbuffs[RX_TOTAL_BUFSIZE] __aligned(ARCH_DMA_MINALIGN);
- char txbuffs[TX_TOTAL_BUFSIZE];
- char rxbuffs[RX_TOTAL_BUFSIZE];
u32 interface;
u32 tx_currdescnum;
u32 rx_currdescnum;
struct eth_mac_regs *mac_regs_p; struct eth_dma_regs *dma_regs_p;
Thanks for your hard work addressing this nasty tftp breakage regression on the Cubietruck (and on the other ARM hardware, which happens to use the same designware driver). Now everything looks perfect. The whole v3 patch set with this v3.1 reorder fix is
Tested-by: Siarhei Siamashka siarhei.siamashka@gmail.com
and also if anybody cares
Reviewed-by: Siarhei Siamashka siarhei.siamashka@gmail.com
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.
Signed-off-by: Ian Campbell ijc@hellion.org.uk Cc: Alexey Brodkin abrodkin@synopsys.com --- 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(-)
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index fa816bf..7186e3b 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -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); diff --git a/drivers/net/designware.h b/drivers/net/designware.h index eac5e72..e5b5e20 100644 --- a/drivers/net/designware.h +++ b/drivers/net/designware.h @@ -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)
Hi Ian,
On Thu, 08 May 2014 22:26:06 +0100, Ian Campbell ijc@hellion.org.uk wrote:
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
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,
participants (3)
-
Albert ARIBAUD -
Ian Campbell -
Siarhei Siamashka