net: add Faraday FTMAC100 10/100 Ethernet driver
Subsystems (2)
PATCH 1/1 net: add Faraday FTMAC100 10/100 Ethernet driver
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC including Faraday A320 and Andes AG101.
--- drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1341 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 +++++++ 4 files changed, 1531 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
drivers/net/Kconfig +9 -0
@@ -2014,6 +2014,15 @@ config BCM63XX_ENET This driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX). +config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 Ethernet controller from + Faraday. It is used on Faraday A320, Andes AG101, AG101P + and some other ARM/NDS32 SoC's. + source "drivers/net/fs_enet/Kconfig" source "drivers/net/octeon/Kconfig"
drivers/net/Makefile +1 -0
@@ -147,6 +147,7 @@ obj-$(CONFIG_FORCEDETH) += forcedeth.o obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o obj-$(CONFIG_AX88796) += ax88796.o obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o +obj-$(CONFIG_FTMAC100) += ftmac100.o obj-$(CONFIG_TSI108_ETH) += tsi108_eth.o obj-$(CONFIG_MV643XX_ETH) += mv643xx_eth.o
drivers/net/ftmac100.c new file +1341 -0
1,341 lines not on this page · raw message
drivers/net/ftmac100.h new file +180 -0
180 lines not on this page · raw message
Signature · 1 line
1.6.3.3
Quoted 5 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC including Faraday A320 and Andes AG101.
[...]
Quoted 5 lines
drivers/net/ftmac100.c new file +0
-0
[...]
drivers/net/ftmac100.c line 36
Quoted 3 lines
drivers/net/ftmac100.c new file +3
-0+#include "ftmac100.h" + +#define USE_NAPI
All new network drivers should use NAPI only, so please remove the definition of USE_NAPI and change the conditional code to use NAPI always.
[...]
drivers/net/ftmac100.c line 78
Quoted 4 lines
drivers/net/ftmac100.c new file +4
-0+struct napi_struct napi; +#endif + +struct net_device_stats stats;
There is a net_device_stats structure in the net_device structure; you should normally use that instead of adding another one.
[...]
Quoted 15 lines
drivers/net/ftmac100.c new file +15
-0+static int ftmac100_reset (struct ftmac100 *priv) +{ +struct device *dev = &priv->netdev->dev; +unsigned long flags; +int i; + +/* NOTE: reset clears all registers */ + +spin_lock_irqsave (&priv->hw_lock, flags); +iowrite32 (FTMAC100_MACCR_SW_RST , priv->base +FTMAC100_OFFSET_MACCR ); +spin_unlock_irqrestore (&priv->hw_lock, flags); + +for (i =0 ; i <5 ; i++) { +int maccr; +
drivers/net/ftmac100.c line 168
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+spin_lock_irqsave (&priv->hw_lock, flags); + maccr =ioread32 (priv->base +FTMAC100_OFFSET_MACCR ); +spin_unlock_irqrestore (&priv->hw_lock, flags); +if (!(maccr &FTMAC100_MACCR_SW_RST )) { +/* +* FTMAC100_MACCR_SW_RST cleared does not indicate +* that hardware reset completed (what the f*ck). +* We still need to wait for a while. +*/ +usleep_range (500 ,1000 );
Sleeping here means this must be running in process context. But you used spin_lock_irqsave() above which implies you're not sure what the context is. One of these must be wrong.
I wonder whether hw_lock is even needed; you seem to acquire and release it around each PIO (read or write). This should be unnecessary since each PIO is atomic.
[...]
Quoted 11 lines
drivers/net/ftmac100.c new file +11
-0+static int ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ +struct net_device *netdev = priv->netdev; +struct device *dev = &netdev->dev; +unsigned long flags; +struct ftmac100_rxdes *rxdes; +struct sk_buff *skb; +int length; +int copied =0 ; +int done =0 ; +
drivers/net/ftmac100.c line 456
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+spin_lock_irqsave (&priv->rx_lock, flags); + rxdes =ftmac100_rx_locate_first_segment (priv); +spin_unlock_irqrestore (&priv->rx_lock, flags); +if (!rxdes) +return 0 ; + +if (unlikely (ftmac100_rx_packet_error (priv, rxdes))) { +spin_lock_irqsave (&priv->rx_lock, flags); +ftmac100_rx_drop_packet (priv); +spin_unlock_irqrestore (&priv->rx_lock, flags);
I think you can also get rid of rx_lock; it's only used in the RX data path which is already serialised by NAPI.
[...]
drivers/net/ftmac100.c line 519
Quoted 4 lines
drivers/net/ftmac100.c new file +4
-0+netif_rx (skb); +#endif + + netdev->last_rx = jiffies;
Don't set last_rx; the networking core takes care of it now.
drivers/net/ftmac100.c line 522
Quoted 2 lines
drivers/net/ftmac100.c new file +2
-0+ priv->stats.rx_packets++; + priv->stats.rx_bytes += skb->len;
This should be done earlier, so that these stats include packets that are dropped for any reason.
[...]
Quoted 3 lines
drivers/net/ftmac100.c new file +3
-0+static int ftmac100_xmit (struct ftmac100 *priv,struct sk_buff *skb, +dma_addr_t map) +{
[...]
drivers/net/ftmac100.c line 739
Quoted 3 lines
drivers/net/ftmac100.c new file +3
-0+ftmac100_txdma_start_polling (priv); +spin_unlock_irqrestore (&priv->hw_lock, flags); + netdev->trans_start = jiffies;
Don't set trans_start; the networking core takes care of it now.
[...]
drivers/net/ftmac100.c line 788
Quoted 7 lines
drivers/net/ftmac100.c new file +7
-0+static int ftmac100_alloc_buffers (struct ftmac100 *priv) +{ +int i; + + priv->descs =dma_alloc_coherent (priv->dev, +sizeof (struct ftmac100_descs ), &priv->descs_dma_addr, +GFP_KERNEL |GFP_DMA );
On x86, GFP_DMA means the memory must be within the ISA DMA range (< 16 MB). I don't know quite what it means on ARM but it may not be what you want.
[...]
Quoted 48 lines
drivers/net/ftmac100.c new file +48
-0+/****************************************************************************** +* interrupt handler +*****************************************************************************/ +static irqreturn_t ftmac100_interrupt (int irq,void *dev_id) +{ +struct net_device *netdev = dev_id; +struct device *dev = &netdev->dev; +struct ftmac100 *priv =netdev_priv (netdev); +unsigned long flags; +unsigned int status; +unsigned int imr; + +spin_lock_irqsave (&priv->hw_lock, flags); + status =ioread32 (priv->base +FTMAC100_OFFSET_ISR ); + imr =ioread32 (priv->base +FTMAC100_OFFSET_IMR ); +spin_unlock_irqrestore (&priv->hw_lock, flags); + + status &= imr; +if (status & (FTMAC100_INT_RPKT_FINISH |FTMAC100_INT_NORXBUF )) { +/* +* FTMAC100_INT_RPKT_FINISH: +* RX DMA has received packets into RX buffer successfully +* +* FTMAC100_INT_NORXBUF: +* RX buffer unavailable +*/ +#ifdef USE_NAPI +/* Disable interrupts for polling */ +ftmac100_disable_rxint (priv); + +napi_schedule (&priv->napi); +#else +int rx =0 ; + +while (ftmac100_rx_packet (priv, &rx)) + ; +#endif + } + +if (status &FTMAC100_INT_NORXBUF ) { +/* RX buffer unavailable */ +if (printk_ratelimit ()) +dev_info (dev,"INT_NORXBUF\n" ); + + priv->stats.rx_over_errors++; + } + +if (status & (FTMAC100_INT_XPKT_OK |FTMAC100_INT_XPKT_LOST )) {
drivers/net/ftmac100.c line 1009
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+/* +* FTMAC100_INT_XPKT_OK: +* packet transmitted to ethernet successfully +* +* FTMAC100_INT_XPKT_LOST: +* packet transmitted to ethernet lost due to late +* collision or excessive collision +*/ +ftmac100_tx_complete (priv); + }
TX completions should also be handled through NAPI if possible.
[...]
Quoted 15 lines
drivers/net/ftmac100.c new file +15
-0+static int ftmac100_open (struct net_device *netdev) +{ +struct device *dev = &netdev->dev; +struct ftmac100 *priv =netdev_priv (netdev); +int err; + + err =ftmac100_alloc_buffers (priv); +if (err) { +dev_err (dev,"failed to allocate buffers\n" ); + goto err_alloc; + } + + err =request_irq (priv->irq, ftmac100_interrupt,0 , netdev->name, + netdev); +if (err) {
drivers/net/ftmac100.c line 1089
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+dev_err (dev,"failed to request irq %d\n" , priv->irq); + goto err_irq; + } + + priv->rx_pointer =0 ; + priv->tx_clean_pointer =0 ; + priv->tx_pointer =0 ; +spin_lock_init (&priv->hw_lock); +spin_lock_init (&priv->rx_lock); +spin_lock_init (&priv->tx_lock);
These locks should be initialised in your probe function.
[...]
Quoted 7 lines
drivers/net/ftmac100.c new file +7
-0+/****************************************************************************** +* struct platform_driver functions +*****************************************************************************/ +static int ftmac100_remove (struct platform_device *pdev) +{ +struct net_device *netdev; +struct ftmac100 *priv;
drivers/net/ftmac100.c line 1202
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ + netdev =platform_get_drvdata (pdev); +if (netdev ==NULL ) +return 0 ; + +platform_set_drvdata (pdev,NULL ); + + priv =netdev_priv (netdev); + +unregister_netdev (netdev);
[...]
There should be a netif_napi_del() before this.
A general comment: please use netdev_err(), netdev_info() etc. for logging. This ensures that both the platform device address and the network device name appears in the log messages.
Ben.
Signature · 3 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.
Dear Ben,
Quoted 6 lines · Ben Hutchings
Quoted 5 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC including Faraday A320 and Andes AG101.
Quoted 6 lines
[...]
Quoted 1 line
+#define
USE_NAPIAll new network drivers should use NAPI only, so please remove the definition of
USE_NAPIand change the conditional code to use NAPI always.
OK, fixed.
Quoted 5 lines
[...]
Quoted 1 line
+ struct
net_device_statsstats;There is a
net_device_statsstructure in thenet_devicestructure; you should normally use that instead of adding another one.
OK, fixed.
Quoted 31 lines
[...]
Quoted 15 lines
drivers/net/ftmac100.c new file +15
-0+static int ftmac100_reset (struct ftmac100 *priv) +{ + struct device *dev = &priv->netdev->dev; + unsigned long flags; + int i; + + /* NOTE: reset clears all registers */ + + spin_lock_irqsave(&priv->hw_lock, flags); + iowrite32(FTMAC100_MACCR_SW_RST, priv->base + FTMAC100_OFFSET_MACCR); + spin_unlock_irqrestore(&priv->hw_lock, flags); + + for (i = 0; i < 5; i++) { + int maccr; +drivers/net/ftmac100.c line 168
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ spin_lock_irqsave(&priv->hw_lock, flags); + maccr = ioread32(priv->base + FTMAC100_OFFSET_MACCR); + spin_unlock_irqrestore(&priv->hw_lock, flags); + if (!(maccr & FTMAC100_MACCR_SW_RST)) { + /* + * FTMAC100_MACCR_SW_RST cleared does not indicate + * that hardware reset completed (what the f*ck). + * We still need to wait for a while. + */ + usleep_range(500, 1000);Sleeping here means this must be running in process context. But you used
spin_lock_irqsave()above which implies you're not sure what the context is. One of these must be wrong.
Quoted 3 lines
I wonder whether
hw_lockis even needed; you seem to acquire and release it around each PIO (read or write). This should be unnecessary since each PIO is atomic.
OK, fixed.
Quoted 2 lines
I think you can also get rid of
rx_lock; it's only used in the RX data path which is already serialised by NAPI.
OK, fixed.
Quoted 4 lines
[...]
Quoted 1 line
+
netdev->last_rx= jiffies;Don't set
last_rx; the networking core takes care of it now.
OK, fixed.
Quoted 5 lines
Quoted 2 lines
+ priv->stats.rx_packets++;
+priv->stats.rx_bytes+=skb->len;This should be done earlier, so that these stats include packets that are dropped for any reason.
OK, fixed.
Quoted 4 lines
[...]
Quoted 1 line
+
netdev->trans_start= jiffies;Don't set
trans_start; the networking core takes care of it now.
OK, fixed.
Quoted 8 lines
[...]
Quoted 3 lines
+
priv->descs= dma_alloc_coherent(priv->dev,
+sizeof(struct ftmac100_descs),&priv->descs_dma_addr,
+GFP_KERNEL|GFP_DMA);On x86,
GFP_DMAmeans the memory must be within the ISA DMA range (< 16 MB). I don't know quite what it means on ARM but it may not be what you want.
On ARM, all 4G address space can be used by DMA (at least for all the hardwares I have ever used). All memory pages are in DMA zone AFAIK. I put GFP_DMA here just to be safe if there were any constraints.
By checking drivers in drivers/net/arm/, ep93xx_eth.c also uses this flag, so I guess this is acceptable?
Quoted 13 lines
[...]
Quoted 11 lines
+ if (status & (
FTMAC100_INT_XPKT_OK|FTMAC100_INT_XPKT_LOST)) {
+ /*
+ *FTMAC100_INT_XPKT_OK:
+ * packet transmitted to ethernet successfully
+ *
+ *FTMAC100_INT_XPKT_LOST:
+ * packet transmitted to ethernet lost due to late
+ * collision or excessive collision
+ */
+ftmac100_tx_complete(priv);
+ }
Quoted 1 line
TX completions should also be handled through NAPI if possible.
OK, I'll study how to do that.
Quoted 9 lines
[...]
Quoted 6 lines
+
priv->rx_pointer= 0;
+priv->tx_clean_pointer= 0;
+priv->tx_pointer= 0;
+spin_lock_init(&priv->hw_lock);
+spin_lock_init(&priv->rx_lock);
+spin_lock_init(&priv->tx_lock);These locks should be initialised in your probe function.
OK, fixed.
Quoted 4 lines
[...]
Quoted 1 line
+
unregister_netdev(netdev);There should be a
netif_napi_del()before this.
OK, fixed.
Quoted 3 lines
A general comment: please use
netdev_err(),netdev_info()etc. for logging. This ensures that both the platform device address and the network device name appears in the log messages.
OK, fixed.
Thanks a lot for your detailed review. I'll submit a new version ASAP.
Thanks,
Po-Yu Chuang
Quoted 7 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC including Faraday A320 and Andes AG101.
Signed-off-by Po-Yu Chuang <[email protected]>
Quoted 8 lines
--- drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1341 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 +++++++ 4 files changed, 1531 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
Hi
1) please use netdev_alloc_skb_ip_align() instead of dev_alloc_skb() + skb_reserve(skb, NET_IP_ALIGN);
2) Dont include a "struct net_device_stats stats" in struct ftmac100, you can use the one included in struct net_device
(You can then remove ftmac100_get_stats())
3) Dont "netdev->last_rx = jiffies;" : This is not driver job. Ditto for trans_start
4) You must comment why wmb() is needed in ftmac100_xmit()
5) Why isnt NAPI used too for TX completions ?
BTW, I am not sure we want a define. All new drivers must use NAPI.
6) Use is_valid_ether_addr() instead of is_zero_ether_addr() in ftmac100_probe()
7) "static struct ethtool_ops ftmac100_ethtool_ops" should be const :
"static const struct ethtool_ops ftmac100_ethtool_ops"
Ditto for :
"static struct net_device_ops ftmac100_netdev_ops"
8) Why an interrupt handler (ftmac100_interrupt()) needs to block interrupts with spin_lock_irqsave(&priv->hw_lock, flags); ?
9) Instead of dev_info(&netdev->dev ...) , please consider netdev_info()
Quoted 19 lines · On Thu, 13 Jan 2011 15:22:39 +0100 Eric Dumazet <[email protected]> wrote:
Quoted 7 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC including Faraday A320 and Andes AG101.
Signed-off-by Po-Yu Chuang <[email protected]>Quoted 8 lines
--- drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1341++++++++++++++++++++++++++++++++++++++++++++++++
drivers/net/ftmac100.h| 180 +++++++ 4 files changed, 1531 insertions(+), 0 deletions(-) create mode 100644drivers/net/ftmac100.ccreate mode 100644drivers/net/ftmac100.hHi
[...]
Quoted 5 lines
9) Instead of
dev_info(&netdev->dev ...), please considernetdev_info()
No one else mentioned it, so I'll add:
Don't explicitly inline functions unless they're in a header, or you have a really good reason (and that reason should probably be described in a comment). Otherwise, just leave off the 'inline' keyword; the compiler is smart enough to decide whether a function should be inlined or not.
Dear Andres,
Quoted 11 lines · Andres Salomon
Quoted 1 line · On Thu, 13 Jan 2011 15:22:39 +0100 Eric Dumazet <[email protected]> wrote:
Le jeudi 13 janvier 2011 à 19:49 +0800, Po-Yu Chuang a écrit :
No one else mentioned it, so I'll add:
Don't explicitly inline functions unless they're in a header, or you have a really good reason (and that reason should probably be described in a comment). Otherwise, just leave off the 'inline' keyword; the compiler is smart enough to decide whether a function should be inlined or not.
OK, fixed.
Thanks,
Po-Yu Chuang
Dear Eric,
Quoted 7 lines · Eric Dumazet
Quoted 6 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC including Faraday A320 and Andes AG101.
Quoted 5 lines
Quoted 1 line
Signed-off-by Po-Yu Chuang <[email protected]>Hi
1) please use
netdev_alloc_skb_ip_align()instead ofdev_alloc_skb()+skb_reserve(skb, NET_IP_ALIGN);
OK, fixed.
Quoted 3 lines
2) Dont include a "struct
net_device_statsstats" in struct ftmac100, you can use the one included in structnet_device
(You can then removeftmac100_get_stats())
OK, fixed.
Quoted 2 lines
3) Dont "
netdev->last_rx= jiffies;" : This is not driver job. Ditto fortrans_start
OK, fixed.
Quoted 1 line
4) You must comment why
wmb()is needed inftmac100_xmit()
OK, comment added.
Quoted 2 lines
5) Why isnt NAPI used too for TX completions ?
BTW, I am not sure we want a define. All new drivers must use NAPI.
I'll study how to do that
Quoted 2 lines
6) Use
is_valid_ether_addr()instead ofis_zero_ether_addr()inftmac100_probe()
OK, fixed.
Quoted 4 lines
7) "static struct
ethtool_opsftmac100_ethtool_ops" should be const :
"static const structethtool_opsftmac100_ethtool_ops" Ditto for :
"static structnet_device_opsftmac100_netdev_ops"
OK, both fixed.
Quoted 2 lines
8) Why an interrupt handler (
ftmac100_interrupt()) needs to block interrupts withspin_lock_irqsave(&priv->hw_lock, flags); ?
Fixed. Not a problem anymore since hw_lock is removed now.
Quoted 1 line
9) Instead of
dev_info(&netdev->dev ...), please considernetdev_info()
OK, fixed.
Thanks a lot for your detailed review. I'll submit a new version ASAP.
Thanks,
Po-Yu Chuang
Quoted 5 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC including Faraday A320 and Andes AG101.
A couple of trivial comments not already mentioned by others.
drivers/net/Kconfig
Quoted 1 line
drivers/net/Kconfig +0
-0
[]
drivers/net/Kconfig config BCM63XX_ENET · line 2025
Quoted 13 lines
drivers/net/Kconfig +9
-0@@ -2014,6 +2014,15 @@ config BCM63XX_ENET This driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX). +config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 Ethernet controller from + Faraday. It is used on Faraday A320, Andes AG101, AG101P + and some other ARM/NDS32 SoC's. +
ARM specific net drivers are for now in drivers/net/arm/ Perhaps it's better to locate these files there?
drivers/net/ftmac100.c
Quoted 1 line
drivers/net/ftmac100.c +0
-0
[]
drivers/net/ftmac100.c line 374
Quoted 9 lines
drivers/net/ftmac100.c +9
-0+static int ftmac100_rx_packet_error (struct ftmac100 *priv, +struct ftmac100_rxdes *rxdes) +{ +struct device *dev = &priv->netdev->dev; +int error =0 ; + +if (unlikely (ftmac100_rxdes_rx_error (rxdes))) { +if (printk_ratelimit ()) +dev_info (dev,"rx err\n" );
There are many printk_ratelimit() tests.
It's better to use net_ratelimit() or a local ratelimit_state so there's less possible suppression of other subsystem messages.
Dear Joe,
Quoted 15 lines · Joe Perches
Quoted 5 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC including Faraday A320 and Andes AG101.
A couple of trivial comments not already mentioned by others.
drivers/net/Kconfig
Quoted 1 line
drivers/net/Kconfig +0
-0[]
Quoted 4 lines
drivers/net/Kconfig +0
-0@@ -2014,6 +2014,15 @@ config BCM63XX_ENETThis driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX).
Quoted 12 lines
drivers/net/Kconfig config BCM63XX_ENET · line 2025
Quoted 9 lines
drivers/net/Kconfig +9
-0+config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 Ethernet controller from + Faraday. It is used on Faraday A320, Andes AG101, AG101P + and some other ARM/NDS32 SoC's. +ARM specific net drivers are for now in
drivers/net/arm/Perhaps it's better to locate these files there?
This controller is used by not only ARM-base platforms, but also NDS32-base ones.
NDS32 is an ISA designed by Andes tech. Although it is not yet in the mainline, they plan to push their stuff to mainline and are working on that.
So... I don't know if it is better to put my driver to driver/net/arm/.
Should I leave my driver at driver/net/ or put it in drvier/net/arm/ and let Andes tech guys
move it out of driver/net/arm/ when they use it later?
Quoted 9 lines
Quoted 3 lines
+ if (unlikely(ftmac100_rxdes_rx_error(rxdes))) {
+ if (printk_ratelimit())
+dev_info(dev, "rx err\n");There are many
printk_ratelimit()tests.It's better to use
net_ratelimit()or a localratelimit_stateso there's less possible suppression of other subsystem messages.
OK, fixed.
Thanks a lot for your absolutely non-trivial review. :-) I'll submit a new version ASAP.
Thanks,
Po-Yu Chuang
A message that is not in the archive (sent off-list or not yet received): 1294959948.4114.189.camel@Joe-Laptop
Dear Joe,
Quoted 11 lines · Joe Perches
Quoted 1 line · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
[]
Quoted 7 lines
+ if (
is_zero_ether_addr(netdev->dev_addr)) {
+random_ether_addr(netdev->dev_addr);
+ dev_info(&netdev->dev, "generated random MAC address "
+ "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n",
+ netdev->dev_addr[0], netdev->dev_addr[1],
+ netdev->dev_addr[2], netdev->dev_addr[3],
+ netdev->dev_addr[4], netdev->dev_addr[5]);
Quoted 7 lines
Sorry, one other thing.
There are kernel specific printf extensions for pointer types. (look atlib/vsprintf.c) There's one for mac addresses: "%pM". This could be done as:netdev_info(netdev, "Generated random MAC address: %pM\n",
netdev->dev_addr);
Wow, this is pretty beautiful. Thanks.
best regards,
Po-Yu Chuang
PATCH v2 1/1 net: add Faraday FTMAC100 10/100 Ethernet driver
In reply to Po-Yu ChuangFrom: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
---
v2:
always use NAPI
do not use our own net_device_stats structure don't set trans_start and last_rx
stats.rx_packets and stats.rx_bytes include dropped packets add missed netif_napi_del()
initialize spinlocks in probe function
remove rx_lock and hw_lock
use netdev_[err/info/dbg] instead of dev_* ones use netdev_alloc_skb_ip_align()
remove ftmac100_get_stats()
use is_valid_ether_addr() instead of is_zero_ether_addr() add const to ftmac100_ethtool_ops and ftmac100_netdev_ops use net_ratelimit() instead of printk_ratelimit() no explicit inline
use %pM to print MAC address
add comment before wmb
use napi poll() to handle all interrupts
As I said in the previous mail, this controller is used not only on ARM, so I still leave it in drivers/net/. If there is any suggestion, please let me know. Thanks
drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1223 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 +++++++ 4 files changed, 1413 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
drivers/net/Kconfig +9 -0
@@ -2014,6 +2014,15 @@ config BCM63XX_ENET This driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX). +config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 Ethernet controller from + Faraday. It is used on Faraday A320, Andes AG101, AG101P + and some other ARM/NDS32 SoC's. + source "drivers/net/fs_enet/Kconfig" source "drivers/net/octeon/Kconfig"
drivers/net/Makefile +1 -0
@@ -147,6 +147,7 @@ obj-$(CONFIG_FORCEDETH) += forcedeth.o obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o obj-$(CONFIG_AX88796) += ax88796.o obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o +obj-$(CONFIG_FTMAC100) += ftmac100.o obj-$(CONFIG_TSI108_ETH) += tsi108_eth.o obj-$(CONFIG_MV643XX_ETH) += mv643xx_eth.o
drivers/net/ftmac100.c new file +1223 -0
1,223 lines not on this page · raw message
drivers/net/ftmac100.h new file +180 -0
180 lines not on this page · raw message
Signature · 1 line
1.6.3.3
Quoted 4 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
Hi again.
Quoted 12 lines
Signed-off-by Po-Yu Chuang <[email protected]>---
v2:
always use NAPI
do not use our ownnet_device_statsstructure don't settrans_startandlast_rx
stats.rx_packets and stats.rx_bytes include dropped packets add missednetif_napi_del()
initialize spinlocks in probe function
removerx_lockandhw_lock
use netdev_[err/info/dbg] instead of dev_* ones usenetdev_alloc_skb_ip_align()
Quoted 8 lines
remove
ftmac100_get_stats()
useis_valid_ether_addr()instead ofis_zero_ether_addr()add const toftmac100_ethtool_opsandftmac100_netdev_opsusenet_ratelimit()instead ofprintk_ratelimit()no explicit inline
use %pM to print MAC address
add comment before wmb
use napipoll()to handle all interrupts
This looks very clean, thanks for doing the rework.
Now the the really trivial...
Quoted 1 line
+ * priveate data
private
drivers/net/ftmac100.c line 85
Quoted 8 lines
drivers/net/ftmac100.c new file +8
-0+static void ftmac100_enable_all_int (struct ftmac100 *priv) +{ +unsigned int imr; + + imr =FTMAC100_INT_RPKT_FINISH |FTMAC100_INT_NORXBUF + |FTMAC100_INT_XPKT_OK |FTMAC100_INT_XPKT_LOST + |FTMAC100_INT_RPKT_LOST |FTMAC100_INT_AHB_ERR + |FTMAC100_INT_PHYSTS_CHG ;
This could be a #define.
drivers/net/ftmac100.c line 176
Quoted 8 lines
drivers/net/ftmac100.c new file +8
-0+ maccr =FTMAC100_MACCR_XMT_EN | +FTMAC100_MACCR_RCV_EN | +FTMAC100_MACCR_XDMA_EN | +FTMAC100_MACCR_RDMA_EN | +FTMAC100_MACCR_CRC_APD | +FTMAC100_MACCR_FULLDUP | +FTMAC100_MACCR_RX_RUNT | +FTMAC100_MACCR_RX_BROADPKT ;
Here too.
drivers/net/ftmac100.c line 316
Quoted 2 lines
drivers/net/ftmac100.c new file +2
-0+static int ftmac100_rx_packet_error (struct ftmac100 *priv, +struct ftmac100_rxdes *rxdes)
[]
drivers/net/ftmac100.c line 345
Quoted 9 lines
drivers/net/ftmac100.c new file +9
-0+if (unlikely (ftmac100_rxdes_frame_too_long (rxdes))) { +if (net_ratelimit ()) +netdev_info (netdev,"rx frame too long\n" ); + + netdev->stats.rx_length_errors++; + error =1 ; + } + +if (unlikely (ftmac100_rxdes_runt (rxdes))) {
else if ?
drivers/net/ftmac100.c line 392
Quoted 8 lines
drivers/net/ftmac100.c new file +8
-0+static int ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ +struct net_device *netdev = priv->netdev; +struct ftmac100_rxdes *rxdes; +struct sk_buff *skb; +int length; +int copied =0 ; +int done =0 ;
You could use bool/true/false here for copied and done and all the other uses of an int for a logical bool.
drivers/net/ftmac100.c line 476
Quoted 5 lines
drivers/net/ftmac100.c new file +5
-0+static void ftmac100_txdes_set_dma_own (struct ftmac100_txdes *txdes) +{ +/* +* Make sure dma own bit will not be set before any other +* descriptor fiels.
field/fields
Quoted 12 lines
drivers/net/ftmac100.c new file +12
-0+static int ftmac100_mdio_read (struct net_device *netdev,int phy_id,int reg) +{ +struct ftmac100 *priv =netdev_priv (netdev); +int phycr; +int i; + + phycr =FTMAC100_PHYCR_PHYAD (phy_id) | +FTMAC100_PHYCR_REGAD (reg) | +FTMAC100_PHYCR_MIIRD ; + +iowrite32 (phycr, priv->base +FTMAC100_OFFSET_PHYCR ); +for (i =0 ; i <10 ; i++) {
drivers/net/ftmac100.c line 788
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ phycr =ioread32 (priv->base +FTMAC100_OFFSET_PHYCR ); + +if ((phycr &FTMAC100_PHYCR_MIIRD ) ==0 ) +return phycr &FTMAC100_PHYCR_MIIRDATA ; + +usleep_range (100 ,1000 ); + } + +netdev_err (netdev,"mdio read timed out\n" ); +return 0xffff ;
0xffff is a rather odd return, perhaps a #define?
drivers/net/ftmac100.c line 1209
Quoted 6 lines
drivers/net/ftmac100.c new file +6
-0+/****************************************************************************** +* initialization / finalization +*****************************************************************************/ +static int __init ftmac100_init (void ) +{ +printk (KERN_INFO "Loading " DRV_NAME ": version " DRV_VERSION " ...\n" );
You could use
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before any #include and
pr_info("Loading version " DRV_VERSION " ...\n");One last comment on split long line indentation style and long function declarations.
There's no required style so you can use what you are most comfortable doing.
Most of drivers/net uses an alignment to open parenthesis using maximal tabs and minimal necessary spaces instead of an extra tabstop.
Like:
static int some_long_function(type var1, type var2...
type varN)
and
some_long_function(var1, var2, ... varN);
not
static int some_long_function(type var1, type var2...
type varN)
and
some_long_function(var1, var2, ... varN);
Dear Joe,
Quoted 5 lines · Joe Perches
Quoted 1 line · Po-Yu Chuang
+ * priveate data
private
Fixed.
Quoted 10 lines
drivers/net/ftmac100.c line 85
Quoted 8 lines
drivers/net/ftmac100.c new file +8
-0+static void ftmac100_enable_all_int (struct ftmac100 *priv) +{ + unsigned int imr; + + imr = FTMAC100_INT_RPKT_FINISH | FTMAC100_INT_NORXBUF + | FTMAC100_INT_XPKT_OK | FTMAC100_INT_XPKT_LOST + | FTMAC100_INT_RPKT_LOST | FTMAC100_INT_AHB_ERR + | FTMAC100_INT_PHYSTS_CHG;This could be a #define.
OK, done.
Quoted 10 lines
Quoted 8 lines
+ maccr =
FTMAC100_MACCR_XMT_EN|
+FTMAC100_MACCR_RCV_EN|+ FTMAC100_MACCR_XDMA_EN | + FTMAC100_MACCR_RDMA_EN | + FTMAC100_MACCR_CRC_APD | + FTMAC100_MACCR_FULLDUP | + FTMAC100_MACCR_RX_RUNT |+
FTMAC100_MACCR_RX_BROADPKT;Here too.
OK, done.
Quoted 14 lines
Quoted 2 lines
+static int ftmac100_rx_packet_error(struct ftmac100 *priv,
+ structftmac100_rxdes*rxdes)[]
drivers/net/ftmac100.c line 345
Quoted 9 lines
+ if (unlikely(ftmac100_rxdes_frame_too_long(rxdes))) {
+ if (net_ratelimit())
+netdev_info(netdev, "rx frame too long\n");drivers/net/ftmac100.c new file +6
-0+ + netdev->stats.rx_length_errors++; + error = 1; + } + + if (unlikely(ftmac100_rxdes_runt(rxdes))) {else if ?
OK, fixed.
Quoted 11 lines
drivers/net/ftmac100.c line 392
Quoted 8 lines
drivers/net/ftmac100.c new file +8
-0+static int ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ + struct net_device *netdev = priv->netdev; + struct ftmac100_rxdes *rxdes; + struct sk_buff *skb; + int length; + int copied = 0; + int done = 0;You could use bool/true/false here for copied and done and all the other uses of an int for a logical bool.
OK, fixed.
Quoted 7 lines
drivers/net/ftmac100.c line 476
Quoted 5 lines
drivers/net/ftmac100.c new file +5
-0+static void ftmac100_txdes_set_dma_own (struct ftmac100_txdes *txdes) +{ + /* +* Make sure dma own bit will not be set before any other +* descriptor fiels. field/fields
Fixed.
Quoted 16 lines
Quoted 16 lines
drivers/net/ftmac100.c new file +16
-0+static int ftmac100_mdio_read (struct net_device *netdev,int phy_id,int reg) +{ + struct ftmac100 *priv = netdev_priv(netdev); + int phycr; + int i; + + phycr = FTMAC100_PHYCR_PHYAD(phy_id) | + FTMAC100_PHYCR_REGAD(reg) | + FTMAC100_PHYCR_MIIRD; + + iowrite32(phycr, priv->base + FTMAC100_OFFSET_PHYCR); + for (i = 0; i < 10; i++) { + phycr = ioread32(priv->base + FTMAC100_OFFSET_PHYCR); + + if ((phycr & FTMAC100_PHYCR_MIIRD) == 0) + return phycr & FTMAC100_PHYCR_MIIRDATA;
Quoted 8 lines
drivers/net/ftmac100.c line 788
Quoted 6 lines
drivers/net/ftmac100.c new file +6
-0+ + usleep_range(100, 1000); + } + + netdev_err(netdev, "mdio read timed out\n"); + return 0xffff;
0xffffis a rather odd return, perhaps a #define?
After a little digging in drivers/net/mii.c, it seems that mii lib does not check return value if it is error. So I guess I should return 0 if error.
Quoted 11 lines
drivers/net/ftmac100.c line 1209
Quoted 6 lines
drivers/net/ftmac100.c new file +6
-0+/****************************************************************************** +* initialization / finalization +*****************************************************************************/ +static int __init ftmac100_init (void ) +{ + printk(KERN_INFO "Loading " DRV_NAME ": version " DRV_VERSION " ...\n");You could use
#definepr_fmt(fmt)KBUILD_MODNAME": " fmt before any #include and
pr_info("Loading version "DRV_VERSION" ...\n");
OK
Quoted 18 lines
One last comment on split long line indentation style and long function declarations.
There's no required style so you can use what you are most comfortable doing.
Most of
drivers/netuses an alignment to open parenthesis using maximal tabs and minimal necessary spaces instead of an extra tabstop.Like:
static int some_long_function(type var1, type var2... type varN)
and
some_long_function(var1, var2, ... varN);
Quoted 6 lines
not
static int some_long_function(type var1, type var2... type varN)
and
some_long_function(var1, var2, ... varN);
Well, TBH, I don't like this style because if I changed the function name, the indentation might need to be adjusted.
Even worse, I got an infeasible case :-(
static struct ftmac100_rxdes *ftmac100_rx_locate_first_segment(
struct ftmac100 *priv)
I know my function names are quite long, but I like them to be descriptive. Do you really insist on it?
Thanks,
Po-Yu Chuang
On Wed, 2011-01-19 at 17:40 +0800, Po-Yu Chuang wrote:
[...]
Quoted 10 lines
Well, TBH, I don't like this style because if I changed the function name, the indentation might need to be adjusted.
Even worse, I got an infeasible case :-(
static struct
ftmac100_rxdes*ftmac100_rx_locate_first_segment(
struct ftmac100 *priv)I know my function names are quite long, but I like them to be descriptive. Do you really insist on it?
You can break after the return type:
static struct ftmac100_rxdes * ftmac100_rx_locate_first_segment(struct ftmac100 *priv)
Ben.
Signature · 3 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.
Quoted 3 lines · Po-Yu Chuang
Quoted 2 lines · Joe Perches
on split long line indentation style
and long function declarations.
[]
Quoted 3 lines
Quoted 3 lines
Most of
drivers/netuses an alignment to open parenthesis using maximal tabs and minimal necessary spaces instead of an extra tabstop.
[]
Quoted 2 lines
Well, TBH, I don't like this style because if I changed the function name, the indentation might need to be adjusted.
No worries. That could happen using either style.
There's no required style so you can use what you are most comfortable doing. It's not a big deal at all.
Quoted 7 lines
Even worse, I got an infeasible case :-(
static struct
ftmac100_rxdes*ftmac100_rx_locate_first_segment(
struct ftmac100 *priv)I know my function names are quite long, but I like them to be descriptive. Do you really insist on it?
Here's a common alternative style for this case:
static struct ftmac100_rxdes * ftmac100_rx_locate_first_segment(struct ftmac100 *priv)
Dear Joe,
Quoted 14 lines · Joe Perches
Quoted 2 lines · Po-Yu Chuang
Well, TBH, I don't like this style because if I changed the function name, the indentation might need to be adjusted.
No worries. That could happen using either style.
There's no required style so you can use what you are most comfortable doing. It's not a big deal at all.
Quoted 5 lines
Even worse, I got an infeasible case :-(
static struct
ftmac100_rxdes*ftmac100_rx_locate_first_segment(
struct ftmac100 *priv)
Quoted 7 lines
Quoted 2 lines
I know my function names are quite long, but I like them to be descriptive. Do you really insist on it?
Here's a common alternative style for this case:
static struct
ftmac100_rxdes*ftmac100_rx_locate_first_segment(struct ftmac100 *priv)
OK, I see.
One more question: how to deal with this? Add a local variable for the 2nd argument?
ftmac100_set_receive_ring_base(priv, priv->descs_dma_addr + offsetof(struct ftmac100_descs, rxdes));
best regards,
Po-Yu Chuang
Quoted 5 lines · Po-Yu Chuang
One more question: how to deal with this? Add a local variable for the 2nd argument?
ftmac100_set_receive_ring_base(priv, priv->descs_dma_addr + offsetof(struct ftmac100_descs, rxdes));
That's one way, another is:
ftmac100_set_receive_ring_base(priv,
priv->descs_dma_addr +
offsetof(struct ftmac100_descs, rxdes));
Another would be to change set_receive_ring_base to just pass priv and determine the dma address there:
static void ftmac100_set_receive_ring_base(struct ftmac100 *priv)
{
dma_addr_t addr;
addr = priv->descs_dma_addr + offsetof(struct ftmac100_descs, rxdes);
iowrite32(addr, priv->base + FTMAC100_OFFSET_RXR_BADR);
}
...
ftmac100_set_receive_ring_base(priv);Another is not to be overly adherent to 80 columns.
Pick one that suits you.
You chose to use a lot of single use, single line functions with descriptive names that use iowrite32 or return some flag. I probably would have just used iowrite32 or tested the flag directly, but that's your choice and it's perfectly fine.
There are a lot of coding choices that are readable and good. There isn't and shouldn't be some mandate for some specific code appearance before inclusion or acceptance.
checkpatch is just a style guide. Ignore it and ignore me when you feel it's appropriate. I won't mind.
cheers, Joe
Quoted 23 lines · Po-Yu Chuang
drivers/net/ftmac100.c new file +23
-0+static int ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ +struct net_device *netdev = priv->netdev; +struct ftmac100_rxdes *rxdes; +struct sk_buff *skb; +int length; +int copied =0 ; +int done =0 ; + + rxdes =ftmac100_rx_locate_first_segment (priv); +if (!rxdes) +return 0 ; + + length =ftmac100_rxdes_frame_length (rxdes); + + netdev->stats.rx_packets++; + netdev->stats.rx_bytes += length; + +if (unlikely (ftmac100_rx_packet_error (priv, rxdes))) { +ftmac100_rx_drop_packet (priv); +return 1 ; + } +
drivers/net/ftmac100.c line 417
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+/* start processing */ + skb =netdev_alloc_skb_ip_align (netdev, length); +if (unlikely (!skb)) { +if (net_ratelimit ()) +netdev_err (netdev,"rx skb alloc failed\n" ); + +ftmac100_rx_drop_packet (priv); +return 1 ; + } +
Please dont increase rx_packets/rx_bytes before the netdev_alloc_skb_ip_align().
In case of mem allocation failure, it would be better not pretending we handled a packet.
drivers/net/r8169.c for example does the rx_packets/rx_bytes only if packet is delivered to upper stack.
Quoted 40 lines · Eric Dumazet
Quoted 23 lines · Po-Yu Chuang
drivers/net/ftmac100.c new file +23
-0+static int ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ +struct net_device *netdev = priv->netdev; +struct ftmac100_rxdes *rxdes; +struct sk_buff *skb; +int length; +int copied =0 ; +int done =0 ; + + rxdes =ftmac100_rx_locate_first_segment (priv); +if (!rxdes) +return 0 ; + + length =ftmac100_rxdes_frame_length (rxdes); + + netdev->stats.rx_packets++; + netdev->stats.rx_bytes += length; + +if (unlikely (ftmac100_rx_packet_error (priv, rxdes))) { +ftmac100_rx_drop_packet (priv); +return 1 ; + } +drivers/net/ftmac100.c line 417
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+/* start processing */ + skb =netdev_alloc_skb_ip_align (netdev, length); +if (unlikely (!skb)) { +if (net_ratelimit ()) +netdev_err (netdev,"rx skb alloc failed\n" ); + +ftmac100_rx_drop_packet (priv); +return 1 ; + } +Please dont increase rx_packets/rx_bytes before the
netdev_alloc_skb_ip_align().
Quoted 5 lines
In case of mem allocation failure, it would be better not pretending we handled a packet.
drivers/net/r8169.cfor example does the rx_packets/rx_bytes only if packet is delivered to upper stack.
That's news to me. I specifically advised Po-Yu Chuang to increment these earlier because my understanding is that all packets/bytes should be counted. And drivers which use hardware MAC stats will generally do that, so I really don't think it makes sense to make other drivers different deliberately.
Ben.
Signature · 3 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.
Quoted 44 lines · Ben Hutchings
Quoted 43 lines · Eric Dumazet
Quoted 23 lines · Po-Yu Chuang
drivers/net/ftmac100.c new file +23
-0+static int ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ +struct net_device *netdev = priv->netdev; +struct ftmac100_rxdes *rxdes; +struct sk_buff *skb; +int length; +int copied =0 ; +int done =0 ; + + rxdes =ftmac100_rx_locate_first_segment (priv); +if (!rxdes) +return 0 ; + + length =ftmac100_rxdes_frame_length (rxdes); + + netdev->stats.rx_packets++; + netdev->stats.rx_bytes += length; + +if (unlikely (ftmac100_rx_packet_error (priv, rxdes))) { +ftmac100_rx_drop_packet (priv); +return 1 ; + } +drivers/net/ftmac100.c line 417
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+/* start processing */ + skb =netdev_alloc_skb_ip_align (netdev, length); +if (unlikely (!skb)) { +if (net_ratelimit ()) +netdev_err (netdev,"rx skb alloc failed\n" ); + +ftmac100_rx_drop_packet (priv); +return 1 ; + } +Please dont increase rx_packets/rx_bytes before the
netdev_alloc_skb_ip_align().In case of mem allocation failure, it would be better not pretending we handled a packet.
Quoted 9 lines
Quoted 2 lines
drivers/net/r8169.cfor example does the rx_packets/rx_bytes only if packet is delivered to upper stack.That's news to me. I specifically advised Po-Yu Chuang to increment these earlier because my understanding is that all packets/bytes should be counted. And drivers which use hardware MAC stats will generally do that, so I really don't think it makes sense to make other drivers different deliberately.
I see, but when one frame is dropped because of RX ring buffer under/overflow we dont account for the lost packet/bytes.
Thats probably not very important, but would be good if all drivers behave the same.
drivers/net/ftmac100.c line 647
Quoted 10 lines · Po-Yu Chuang
drivers/net/ftmac100.c new file +10
-0+ +spin_lock_irqsave (&priv->tx_lock, flags); +ftmac100_txdes_set_skb (txdes, skb); +ftmac100_txdes_set_dma_addr (txdes, map); + +ftmac100_txdes_set_first_segment (txdes); +ftmac100_txdes_set_last_segment (txdes); +ftmac100_txdes_set_txint (txdes); +ftmac100_txdes_set_buffer_size (txdes, len); +
I wonder if its not too expensive to read/modify/write txdes->txdes1
Maybe you should use a temporary u32 var and perform one final write on txdes->txdes1 (with the set_dma_own)
drivers/net/ftmac100.c line 657
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ priv->tx_pending++; +if (priv->tx_pending ==TX_QUEUE_ENTRIES ) { +if (net_ratelimit ()) +netdev_info (netdev,"tx queue full\n" ); + +netif_stop_queue (netdev); + } + +/* start transmit */ +ftmac100_txdes_set_dma_own (txdes);
txdes->txdes1 = txdes1;
BTW, shouldnt you use cpu_to_be32() or cpu_to_le32(), if this driver is multi platform ?
Dear Eric,
Quoted 2 lines · Eric Dumazet
BTW, shouldnt you use
cpu_to_be32()orcpu_to_le32(), if this driver is multi platform ?
This reminds me another thing. Should I use u32 instead of unsigned int for all hardware related variables (registers, descriptors) ?
Not quite sure about these cross-platform issues.
best regards,
Po-Yu Chuang
Dear Eric,
Quoted 10 lines · Eric Dumazet
Quoted 8 lines · Po-Yu Chuang
+
spin_lock_irqsave(&priv->tx_lock, flags);
+ftmac100_txdes_set_skb(txdes, skb);
+ftmac100_txdes_set_dma_addr(txdes, map);
+
+ftmac100_txdes_set_first_segment(txdes);
+ftmac100_txdes_set_last_segment(txdes);
+ftmac100_txdes_set_txint(txdes);
+ftmac100_txdes_set_buffer_size(txdes, len);
Quoted 4 lines
I wonder if its not too expensive to read/modify/write
txdes->txdes1Maybe you should use a temporary u32 var and perform one final write on
txdes->txdes1(with theset_dma_own)
That's OK, the compiler combines these bits updates together and write once since txdes is not volatile. (I checked the object file)
It's not expensive.
BTW, DMA_OWN bit is at txdes0
Quoted 13 lines
drivers/net/ftmac100.c line 657
Quoted 10 lines
+ priv->tx_pending++;
+ if (priv->tx_pending==TX_QUEUE_ENTRIES) {
+ if (net_ratelimit())
+netdev_info(netdev, "tx queue full\n");drivers/net/ftmac100.c new file +6
-0+ + netif_stop_queue(netdev); + } + + /* start transmit */ + ftmac100_txdes_set_dma_own(txdes);txdes->txdes1 = txdes1;
Quoted 2 lines
BTW, shouldnt you use
cpu_to_be32()orcpu_to_le32(), if this driver is multi platform ?
OK, but I am not sure if I use them the right way. Please check my v3 patch later.
Thanks,
Po-Yu Chuang
PATCH v3 1/1 net: add Faraday FTMAC100 10/100 Ethernet driver
In reply to Po-Yu ChuangFrom: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
---
v2:
always use NAPI
do not use our own net_device_stats structure don't set trans_start and last_rx
stats.rx_packets and stats.rx_bytes include dropped packets add missed netif_napi_del()
initialize spinlocks in probe function
remove rx_lock and hw_lock
use netdev_[err/info/dbg] instead of dev_* ones use netdev_alloc_skb_ip_align()
remove ftmac100_get_stats()
use is_valid_ether_addr() instead of is_zero_ether_addr() add const to ftmac100_ethtool_ops and ftmac100_netdev_ops use net_ratelimit() instead of printk_ratelimit() no explicit inline
use %pM to print MAC address
add comment before wmb
use napi poll() to handle all interrupts
v3:
undo "stats.rx_packets and stats.rx_bytes include dropped packets" ftmac100_mdio_read() returns 0 if error
fix comment typos
use pr_fmt and pr_info
define INT_MASK_ALL_ENABLED
define MACCR_ENABLE_ALL
do not count length error many times
use bool/true/false
use cpu_to_le32/le32_to_cpu to access descriptors indent style fix
drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1243 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 +++++++ 4 files changed, 1433 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
drivers/net/Kconfig +9 -0
@@ -2014,6 +2014,15 @@ config BCM63XX_ENET This driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX). +config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 Ethernet controller from + Faraday. It is used on Faraday A320, Andes AG101, AG101P + and some other ARM/NDS32 SoC's. + source "drivers/net/fs_enet/Kconfig" source "drivers/net/octeon/Kconfig"
drivers/net/Makefile +1 -0
@@ -147,6 +147,7 @@ obj-$(CONFIG_FORCEDETH) += forcedeth.o obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o obj-$(CONFIG_AX88796) += ax88796.o obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o +obj-$(CONFIG_FTMAC100) += ftmac100.o obj-$(CONFIG_TSI108_ETH) += tsi108_eth.o obj-$(CONFIG_MV643XX_ETH) += mv643xx_eth.o
drivers/net/ftmac100.c new file +1243 -0
1,243 lines not on this page · raw message
drivers/net/ftmac100.h new file +180 -0
180 lines not on this page · raw message
Signature · 1 line
1.6.3.3
drivers/net/ftmac100.c line 460
Quoted 11 lines · Po-Yu Chuang
drivers/net/ftmac100.c new file +11
-0+/* push packet to protocol stack */ +netif_receive_skb (skb); + + netdev->stats.rx_packets++; + netdev->stats.rx_bytes += skb->len; + + (*processed)++; + +return true; +} +
Hmm, after call to netif_receive_skb(skb), you are not allowed to access skb anymore (maybe it was freed)
netdev->stats.rx_packets++;netdev->stats.rx_bytes += skb->len;netif_receive_skb(skb);
Dear Eric,
Quoted 17 lines · Eric Dumazet
drivers/net/ftmac100.c line 460
Quoted 11 lines · Po-Yu Chuang
+ /* push packet to protocol stack */
+netif_receive_skb(skb);drivers/net/ftmac100.c new file +9
-0+ + netdev->stats.rx_packets++; + netdev->stats.rx_bytes += skb->len; + + (*processed)++; + + return true; +} +Hmm, after call to
netif_receive_skb(skb), you are not allowed to access skb anymore (maybe it was freed)
Quoted 3 lines
netdev->stats.rx_packets++;
netdev->stats.rx_bytes+=skb->len;netif_receive_skb(skb);
Wow, you are totally right.
Thanks,
Po-Yu Chuang
Quoted 44 lines · Po-Yu Chuang
drivers/net/ftmac100.c new file +44
-0+static bool ftmac100_tx_complete_packet (struct ftmac100 *priv) +{ +struct net_device *netdev = priv->netdev; +struct ftmac100_txdes *txdes; +struct sk_buff *skb; +dma_addr_t map; + +if (priv->tx_pending ==0 ) +return false; + + txdes =ftmac100_current_clean_txdes (priv); + +if (ftmac100_txdes_owned_by_dma (txdes)) +return false; + + skb =ftmac100_txdes_get_skb (txdes); + map =ftmac100_txdes_get_dma_addr (txdes); + +if (unlikely (ftmac100_txdes_excessive_collision (txdes) || +ftmac100_txdes_late_collision (txdes))) { +/* +* packet transmitted to ethernet lost due to late collision +* or excessive collision +*/ + netdev->stats.tx_aborted_errors++; + }else { + netdev->stats.tx_packets++; + netdev->stats.tx_bytes += skb->len; + } + +dma_unmap_single (priv->dev, map,skb_headlen (skb),DMA_TO_DEVICE ); + +dev_kfree_skb_irq (skb); + +ftmac100_txdes_reset (txdes); + +ftmac100_tx_clean_pointer_advance (priv); + + priv->tx_pending--; +netif_wake_queue (netdev); + +return true; +} +
drivers/net/ftmac100.c line 648
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+static void ftmac100_tx_complete (struct ftmac100 *priv) +{ +unsigned long flags; + +spin_lock_irqsave (&priv->tx_lock, flags); +while (ftmac100_tx_complete_packet (priv)) + ; +spin_unlock_irqrestore (&priv->tx_lock, flags); +} +
I dont understand why you still block hard IRQS, after full NAPI conversion.
Now you run from NAPI, and softirq handler, are you sure you still need to block hard IRQ and tx_lock ?
It seems to me ftmac100_xmit() could only block softirqs (but they already are blocked by caller), so you could use spin_lock() from ftmac100_xmit()
Dear Eric,
Quoted 60 lines · Eric Dumazet
Quoted 44 lines · Po-Yu Chuang
drivers/net/ftmac100.c new file +44
-0+static bool ftmac100_tx_complete_packet (struct ftmac100 *priv) +{ + struct net_device *netdev = priv->netdev; + struct ftmac100_txdes *txdes; + struct sk_buff *skb; + dma_addr_t map; + + if (priv->tx_pending == 0) + return false; + + txdes = ftmac100_current_clean_txdes(priv); + + if (ftmac100_txdes_owned_by_dma(txdes)) + return false; + + skb = ftmac100_txdes_get_skb(txdes); + map = ftmac100_txdes_get_dma_addr(txdes); + + if (unlikely(ftmac100_txdes_excessive_collision(txdes) || + ftmac100_txdes_late_collision(txdes))) { + /* + * packet transmitted to ethernet lost due to late collision + * or excessive collision + */ + netdev->stats.tx_aborted_errors++; + } else { + netdev->stats.tx_packets++; + netdev->stats.tx_bytes += skb->len; + } + + dma_unmap_single(priv->dev, map, skb_headlen(skb), DMA_TO_DEVICE); + + dev_kfree_skb_irq(skb); + + ftmac100_txdes_reset(txdes); + + ftmac100_tx_clean_pointer_advance(priv); + + priv->tx_pending--; + netif_wake_queue(netdev); + + return true; +} +drivers/net/ftmac100.c line 648
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+static void ftmac100_tx_complete (struct ftmac100 *priv) +{ + unsigned long flags; + + spin_lock_irqsave(&priv->tx_lock, flags); + while (ftmac100_tx_complete_packet(priv)) + ; + spin_unlock_irqrestore(&priv->tx_lock, flags); +} +I dont understand why you still block hard IRQS, after full NAPI conversion.
Quoted 6 lines
Now you run from NAPI, and softirq handler, are you sure you still need to block hard IRQ and
tx_lock?It seems to me
ftmac100_xmit()could only block softirqs (but they already are blocked by caller), so you could usespin_lock()fromftmac100_xmit()
I was not quite clear about when to use what kinds of locking. After your explanation, now I understand.
I will submit v4 tomorrow.
really appreciate,
Po-Yu Chuang
Quoted 1 line · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
[]
drivers/net/ftmac100.c line 1167
Quoted 3 lines
drivers/net/ftmac100.c new file +3
-0+/* map io memory */ + priv->res =request_mem_region (res->start, res->end - res->start, +dev_name (&pdev->dev));
Off by one?
priv->res = request_mem_region(res->start, resource_size(res), dev_name(&pdev->dev));
Dear Joe,
Quoted 11 lines · Joe Perches
Quoted 1 line · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
[]
Quoted 3 lines
+ /* map io memory */
+priv->res= request_mem_region(res->start,res->end-res->start,
+ dev_name(&pdev->dev));Off by one?
priv->res =
request_mem_region(res->start, resource_size(res), dev_name(&pdev->dev));
Fixed.
best regards,
Po-Yu Chuang
Quoted 1 line · Po-Yu Chuang
drivers/net/ftmac100.c | 1243 ++++++++++++++++++++++++++++++++++++++++++++++++
[]
Quoted 19 lines
drivers/net/ftmac100.c new file +19
-0+/****************************************************************************** +* struct napi_struct functions +*****************************************************************************/ +static int ftmac100_poll (struct napi_struct *napi,int budget) +{ + struct ftmac100 *priv = container_of(napi, struct ftmac100, napi); +struct net_device *netdev = priv->netdev; +unsigned int status; +bool completed = true; +int rx =0 ; + + status =ioread32 (priv->base +FTMAC100_OFFSET_ISR ); + +if (status & (FTMAC100_INT_RPKT_FINISH |FTMAC100_INT_NORXBUF )) { +/* +* FTMAC100_INT_RPKT_FINISH: +* RX DMA has received packets into RX buffer successfully +* +* FTMAC100_INT_NORXBUF:
drivers/net/ftmac100.c line 930
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+* RX buffer unavailable +*/ +bool retry; + +do { + retry =ftmac100_rx_packet (priv, &rx); + }while (retry && rx < budget); + +if (retry && rx == budget) + completed = false;
Is it useful to retry the NORXBUF case?
drivers/net/ftmac100.c line 939
Quoted 9 lines
drivers/net/ftmac100.c new file +9
-0+ } + +if (status &FTMAC100_INT_NORXBUF ) { +/* RX buffer unavailable */ +if (net_ratelimit ()) +netdev_info (netdev,"INT_NORXBUF\n" ); + + netdev->stats.rx_over_errors++; + }
Perhaps this "if (status & FTMAC100_INT_NORXBUF)" block should be moved into the test block above it before the retry?
Quoted 35 lines
drivers/net/ftmac100.c new file +35
-0+ +if (status & (FTMAC100_INT_XPKT_OK |FTMAC100_INT_XPKT_LOST )) { +/* +* FTMAC100_INT_XPKT_OK: +* packet transmitted to ethernet successfully +* +* FTMAC100_INT_XPKT_LOST: +* packet transmitted to ethernet lost due to late +* collision or excessive collision +*/ +ftmac100_tx_complete (priv); + } + +if (status &FTMAC100_INT_RPKT_LOST ) { +/* received packet lost due to RX FIFO full */ +if (net_ratelimit ()) +netdev_info (netdev,"INT_RPKT_LOST\n" ); + + netdev->stats.rx_fifo_errors++; + } + +if (status &FTMAC100_INT_AHB_ERR ) { +/* AHB error */ +if (net_ratelimit ()) +netdev_info (netdev,"INT_AHB_ERR\n" ); + +/* do nothing */ + } + +if (status &FTMAC100_INT_PHYSTS_CHG ) { +/* PHY link status change */ +if (net_ratelimit ()) +netdev_info (netdev,"INT_PHYSTS_CHG\n" ); + +mii_check_link (&priv->mii);
drivers/net/ftmac100.c line 984
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ } + +if (completed) { +/* stop polling */ +napi_complete (napi); +ftmac100_enable_all_int (priv); + } + +return rx; +}
It's possible to miss multiple states because of the ratelimit.
If multiple ISR status bits are possible, it might be better to combine all netdev_info uses into a single call.
Something like:
if ((status & (FTMAC100_INT_NORXBUF | FTMAC100_INT_RPKT_LOST | FTMAC100_INT_AHB_ERR | FTMAC100_INT_PHYSTS_CHG)) && net_ratelimit()) netdev_info(netdev, "ISR status: %x%s%s%s%s\n", status & FTMAC100_INT_NORXBUF ? ": INT_NORXBUF" : "", status & FTMAC100_INT_RPKT_LOST ? ": INT_RPKT_LOST" : "", status & FTMAC100_INT_AHB_ERR ? ": INT_AHB_ERR" : "", status & FTMAC100_INT_PHYSTS_CHG ? " : INT_PHYSTS_CHG" : "");
Dear Joe,
Quoted 28 lines · Joe Perches
Quoted 1 line · Po-Yu Chuang
drivers/net/ftmac100.c | 1243 ++++++++++++++++++++++++++++++++++++++++++++++++
[]
Quoted 23 lines
drivers/net/ftmac100.c new file +23
-0+/****************************************************************************** +* struct napi_struct functions +*****************************************************************************/ +static int ftmac100_poll (struct napi_struct *napi,int budget) +{ + struct ftmac100 *priv = container_of(napi, struct ftmac100, napi); + struct net_device *netdev = priv->netdev; + unsigned int status; + bool completed = true; + int rx = 0; + + status = ioread32(priv->base + FTMAC100_OFFSET_ISR); + + if (status & (FTMAC100_INT_RPKT_FINISH | FTMAC100_INT_NORXBUF)) { + /* + * FTMAC100_INT_RPKT_FINISH: + * RX DMA has received packets into RX buffer successfully + * + * FTMAC100_INT_NORXBUF: + * RX buffer unavailable + */ + bool retry; +
Quoted 8 lines
Quoted 6 lines
+ do {
+ retry =ftmac100_rx_packet(priv, &rx);
+ } while (retry && rx < budget);
+
+ if (retry && rx == budget)
+ completed = false;Is it useful to retry the NORXBUF case?
The idea is that if I miss packet finished interrupts (then rx buffers used up), I should retrieve the received packets ASAP to free buffers to HW. Maybe this is really unnecessary.
I am not quite sure, but I'll do your advice now.
Quoted 4 lines
Quoted 4 lines
+ }
+
+ if (status &FTMAC100_INT_NORXBUF) {
+ /* RX buffer unavailable */
Quoted 8 lines
Quoted 5 lines
+ if (
net_ratelimit())
+netdev_info(netdev, "INT_NORXBUF\n"); +
+ netdev->stats.rx_over_errors++;
+ }Perhaps this "if (status &
FTMAC100_INT_NORXBUF)" block should be moved into the test block above it before the retry?
Since status is not changed in the function, it does not matter where the test is.
But I agree that it is better to handle error cases earlier.
Quoted 7 lines
It's possible to miss multiple states because of the ratelimit.
If multiple ISR status bits are possible, it might be better to combine all
netdev_infouses into a single call.Something like:
Quoted 8 lines
if ((status & (
FTMAC100_INT_NORXBUF|FTMAC100_INT_RPKT_LOST|FTMAC100_INT_AHB_ERR|FTMAC100_INT_PHYSTS_CHG)) &&net_ratelimit())
netdev_info(netdev, "ISR status: %x%s%s%s%s\n",
status &FTMAC100_INT_NORXBUF? ":INT_NORXBUF" : "", status &FTMAC100_INT_RPKT_LOST? ":INT_RPKT_LOST" : "", status &FTMAC100_INT_AHB_ERR? ":INT_AHB_ERR" : "", status &FTMAC100_INT_PHYSTS_CHG? " :INT_PHYSTS_CHG" : "");
Agree. Thanks.
best regards,
Po-Yu Chuang
A message that is not in the archive (sent off-list or not yet received): 1295592411.6795.10.camel@Joe-Laptop
Dear Joe,
Quoted 9 lines · Joe Perches
Quoted 5 lines · Po-Yu Chuang
Quoted 1 line
Is it useful to retry the NORXBUF case?
The idea is that if I miss packet finished interrupts (then rx buffers used up), I should retrieve the received packets ASAP to free buffers to HW. Maybe this is really unnecessary.
I am not quite sure, but I'll do your advice now.I wasn't giving advice, just asking a question.
Your concept makes sense to me.
I see. So I will leave it as is.
Quoted 31 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
+ if (status &
FTMAC100_INT_NORXBUF) {
+ /* RX buffer unavailable */
+ if (net_ratelimit())
+netdev_info(netdev, "INT_NORXBUF\n"); +
+ netdev->stats.rx_over_errors++;
+ }Quoted 6 lines
Quoted 2 lines
Perhaps this "if (status &
FTMAC100_INT_NORXBUF)" block should be moved into the test block above it before the retry?Since status is not changed in the function, it does not matter where the test is.
But I agree that it is better to handle error cases earlier.This wasn't so much a handle error case early, but a suggestion that
if (status & (foo | bar)) {
...
}
if (status & bar) {
...
}
should be
if (status & (foo | bar)) {
...
if (status & bar) {
...
}
}
Quoted 2 lines
so that when the first test fails, a known
subset of the first test isn't tested again.
Understand. Thanks.
best regards,
Po-Yu Chuang
2011/1/20 Po-Yu Chuang <[email protected]>:
[...]
drivers/net/ftmac100.c line 201
Quoted 13 lines
drivers/net/ftmac100.c new file +13
-0+/****************************************************************************** +* internal functions (receive descriptor) +*****************************************************************************/ +static bool ftmac100_rxdes_first_segment (struct ftmac100_rxdes *rxdes) +{ + return le32_to_cpu(rxdes->rxdes0) & FTMAC100_RXDES0_FRS; +} + +static bool ftmac100_rxdes_last_segment (struct ftmac100_rxdes *rxdes) +{ + return le32_to_cpu(rxdes->rxdes0) & FTMAC100_RXDES0_LRS; +} +
[...]
You can change these and similar functions to use:
rxdes->rxdes0 & cpu_to_le32(FTMAC100_RXDES0_LRS)
This will be subject to constant folding by compiler and produce better code for big-endian arches.
Best Regards,
Michał Mirosław
Dear Michał ,
2011/1/21 Michał Mirosław <[email protected]>:
Quoted 17 lines
2011/1/20 Po-Yu Chuang <[email protected]>:
[...]Quoted 13 lines
drivers/net/ftmac100.c new file +13
-0+/****************************************************************************** +* internal functions (receive descriptor) +*****************************************************************************/ +static bool ftmac100_rxdes_first_segment (struct ftmac100_rxdes *rxdes) +{ + return le32_to_cpu(rxdes->rxdes0) & FTMAC100_RXDES0_FRS; +} + +static bool ftmac100_rxdes_last_segment (struct ftmac100_rxdes *rxdes) +{ + return le32_to_cpu(rxdes->rxdes0) & FTMAC100_RXDES0_LRS; +} +[...]
Quoted 6 lines
You can change these and similar functions to use:
rxdes->rxdes0&cpu_to_le32(FTMAC100_RXDES0_LRS)This will be subject to constant folding by compiler and produce better code for big-endian arches.
Fixed. It looks much better now.
Thanks a lot.
best regards,
Po-Yu Chuang
PATCH v4 1/1 net: add Faraday FTMAC100 10/100 Ethernet driver
In reply to Po-Yu ChuangFrom: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
---
v2:
always use NAPI
do not use our own net_device_stats structure don't set trans_start and last_rx
stats.rx_packets and stats.rx_bytes include dropped packets add missed netif_napi_del()
initialize spinlocks in probe function
remove rx_lock and hw_lock
use netdev_[err/info/dbg] instead of dev_* ones use netdev_alloc_skb_ip_align()
remove ftmac100_get_stats()
use is_valid_ether_addr() instead of is_zero_ether_addr() add const to ftmac100_ethtool_ops and ftmac100_netdev_ops use net_ratelimit() instead of printk_ratelimit() no explicit inline
use %pM to print MAC address
add comment before wmb
use napi poll() to handle all interrupts
v3:
undo "stats.rx_packets and stats.rx_bytes include dropped packets" ftmac100_mdio_read() returns 0 if error
fix comment typos
use pr_fmt and pr_info
define INT_MASK_ALL_ENABLED
define MACCR_ENABLE_ALL
do not count length error many times
use bool/true/false
use cpu_to_le32/le32_to_cpu to access descriptors indent style fix
v4:
should not access skb after netif_receive_skb() use resource_size()
better way to use cpu_to_le32/le32_to_cpu
use spin_lock() for tx_lock
combine all netdev_info() together in ftmac100_poll()
drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1207 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 +++++++ 4 files changed, 1397 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
drivers/net/Kconfig +9 -0
@@ -2014,6 +2014,15 @@ config BCM63XX_ENET This driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX). +config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 Ethernet controller from + Faraday. It is used on Faraday A320, Andes AG101, AG101P + and some other ARM/NDS32 SoC's. + source "drivers/net/fs_enet/Kconfig" source "drivers/net/octeon/Kconfig"
drivers/net/Makefile +1 -0
@@ -147,6 +147,7 @@ obj-$(CONFIG_FORCEDETH) += forcedeth.o obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o obj-$(CONFIG_AX88796) += ax88796.o obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o +obj-$(CONFIG_FTMAC100) += ftmac100.o obj-$(CONFIG_TSI108_ETH) += tsi108_eth.o obj-$(CONFIG_MV643XX_ETH) += mv643xx_eth.o
drivers/net/ftmac100.c new file +1207 -0
1,207 lines not on this page · raw message
drivers/net/ftmac100.h new file +180 -0
180 lines not on this page · raw message
Signature · 1 line
1.6.3.3
Quoted 12 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
Signed-off-by Po-Yu Chuang <[email protected]>drivers/net/ftmac100.c new file +3
-0+ +static bool ftmac100_tx_complete_packet (struct ftmac100 *priv) +{
...
Quoted 5 lines
drivers/net/ftmac100.c new file +5
-0+ +dma_unmap_single (priv->dev, map,skb_headlen (skb),DMA_TO_DEVICE ); + +dev_kfree_skb_irq (skb); +
drivers/net/ftmac100.c line 612
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ftmac100_txdes_reset (txdes); + +ftmac100_tx_clean_pointer_advance (priv); + + priv->tx_pending--; +netif_wake_queue (netdev); + +return true; +} +
Thanks to NAPI, you can free skb directly, not queuing it via NET_TX_SOFTIRQ softirq, using dev_kfree_skb() instead of dev_kfree_skb_irq()
Dear Eric,
Quoted 13 lines · Eric Dumazet
Le vendredi 21 janvier 2011 à 15:55 +0800, Po-Yu Chuang a écrit :
...Quoted 11 lines
drivers/net/ftmac100.c new file +11
-0+ + dma_unmap_single(priv->dev, map, skb_headlen(skb), DMA_TO_DEVICE); + + dev_kfree_skb_irq(skb); + + ftmac100_txdes_reset(txdes); + + ftmac100_tx_clean_pointer_advance(priv); + + priv->tx_pending--; + netif_wake_queue(netdev);
Quoted 8 lines
drivers/net/ftmac100.c line 400
Quoted 4 lines
drivers/net/ftmac100.c new file +4
-0+ + return true; +} +Thanks to NAPI, you can free skb directly, not queuing it via
NET_TX_SOFTIRQsoftirq, usingdev_kfree_skb()instead ofdev_kfree_skb_irq()
Fixed. Thanks a lot.
best regards,
Po-Yu Chuang
2011/1/21 Po-Yu Chuang <[email protected]>:
Quoted 7 lines
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
Signed-off-by Po-Yu Chuang <[email protected]>
[...]
drivers/net/ftmac100.c line 464
Quoted 8 lines
drivers/net/ftmac100.c new file +8
-0+static void ftmac100_txdes_reset (struct ftmac100_txdes *txdes) +{ + /* clear all except end of ring bit */ + txdes->txdes0 = 0; + txdes->txdes1 &= FTMAC100_TXDES1_EDOTR; + txdes->txdes2 = 0; + txdes->txdes3 = 0; +}
This also probably needs cpu_to_le32().
[...]
Quoted 17 lines
drivers/net/ftmac100.c new file +17
-0+static void ftmac100_free_buffers (struct ftmac100 *priv) +{ + int i; + + for (i = 0; i < RX_QUEUE_ENTRIES; i += 2) { + struct ftmac100_rxdes *rxdes = &priv->descs->rxdes[i]; + dma_addr_t d = ftmac100_rxdes_get_dma_addr(rxdes); + void *page = ftmac100_rxdes_get_va(rxdes); + + if (d) + dma_unmap_single(priv->dev, d, PAGE_SIZE, + DMA_FROM_DEVICE); + + if (page != NULL) + free_page((unsigned long)page); + } +
[...]
Quoted 38 lines
drivers/net/ftmac100.c new file +38
-0+static int ftmac100_alloc_buffers (struct ftmac100 *priv) +{ + int i; + + priv->descs = dma_alloc_coherent(priv->dev, + sizeof(struct ftmac100_descs), + &priv->descs_dma_addr, + GFP_KERNEL | GFP_DMA); + if (priv->descs == NULL) + return -ENOMEM; + + memset(priv->descs, 0, sizeof(struct ftmac100_descs)); + + /* initialize RX ring */ + + ftmac100_rxdes_set_end_of_ring(&priv->descs->rxdes[RX_QUEUE_ENTRIES - 1]); + + for (i = 0; i < RX_QUEUE_ENTRIES; i += 2) { + struct ftmac100_rxdes *rxdes = &priv->descs->rxdes[i]; + void *page; + dma_addr_t d; + + page = (void *)__get_free_page(GFP_KERNEL | GFP_DMA); + if (page == NULL) + goto err; + + d = dma_map_single(priv->dev, page, PAGE_SIZE, DMA_FROM_DEVICE); + if (unlikely(dma_mapping_error(priv->dev, d))) { + free_page((unsigned long)page); + goto err; + } + + /* + * The hardware enforces a sub-2K maximum packet size, so we + * put two buffers on every hardware page. + */ + ftmac100_rxdes_set_va(rxdes, page); + ftmac100_rxdes_set_va(rxdes + 1, page + PAGE_SIZE / 2);
drivers/net/ftmac100.c line 744
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ + ftmac100_rxdes_set_dma_addr(rxdes, d); + ftmac100_rxdes_set_dma_addr(rxdes + 1, d + PAGE_SIZE / 2); + + ftmac100_rxdes_set_buffer_size(rxdes, RX_BUF_SIZE); + ftmac100_rxdes_set_buffer_size(rxdes + 1, RX_BUF_SIZE); + + ftmac100_rxdes_set_dma_own(rxdes); + ftmac100_rxdes_set_dma_own(rxdes + 1); + }
[...]
Did you test this? This looks like it will result in double free after packet RX, as you are giving the same page (referenced once) to two distinct RX descriptors, that may be assigned different packets.
Since your not implementing any RX offloads, you might just allocate fresh skb's with alloc_skb() and store skb pointer in rxdes3. Since hardware doesn't touch it, you can skip cpu_to_le32()/le32_to_cpu() there (leave a comment, though).
Unless this needs to work for ISA devices, you should drop GFP_DMA allocation flag.
Best Regards,
Michał Mirosław
Dear Michał,
2011/1/21 Michał Mirosław <[email protected]>:
Quoted 9 lines
2011/1/21 Po-Yu Chuang <[email protected]>:
Quoted 7 lines
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
Signed-off-by Po-Yu Chuang <[email protected]>[...]
Quoted 10 lines
drivers/net/ftmac100.c line 464
Quoted 8 lines
drivers/net/ftmac100.c new file +8
-0+static void ftmac100_txdes_reset (struct ftmac100_txdes *txdes) +{ + /* clear all except end of ring bit */ + txdes->txdes0 = 0; + txdes->txdes1 &= FTMAC100_TXDES1_EDOTR; + txdes->txdes2 = 0; + txdes->txdes3 = 0; +}This also probably needs
cpu_to_le32().
Ah, I missed that. Fixed.
Quoted 70 lines
[...]
Quoted 17 lines
drivers/net/ftmac100.c new file +17
-0+static void ftmac100_free_buffers (struct ftmac100 *priv) +{ + int i; + + for (i = 0; i < RX_QUEUE_ENTRIES; i += 2) { + struct ftmac100_rxdes *rxdes = &priv->descs->rxdes[i]; + dma_addr_t d = ftmac100_rxdes_get_dma_addr(rxdes); + void *page = ftmac100_rxdes_get_va(rxdes); + + if (d) + dma_unmap_single(priv->dev, d, PAGE_SIZE, + DMA_FROM_DEVICE); + + if (page != NULL) + free_page((unsigned long)page); + } +[...]
Quoted 48 lines
drivers/net/ftmac100.c new file +48
-0+static int ftmac100_alloc_buffers (struct ftmac100 *priv) +{ + int i; + + priv->descs = dma_alloc_coherent(priv->dev, + sizeof(struct ftmac100_descs), + &priv->descs_dma_addr, + GFP_KERNEL | GFP_DMA); + if (priv->descs == NULL) + return -ENOMEM; + + memset(priv->descs, 0, sizeof(struct ftmac100_descs)); + + /* initialize RX ring */ + + ftmac100_rxdes_set_end_of_ring(&priv->descs->rxdes[RX_QUEUE_ENTRIES - 1]); + + for (i = 0; i < RX_QUEUE_ENTRIES; i += 2) { + struct ftmac100_rxdes *rxdes = &priv->descs->rxdes[i]; + void *page; + dma_addr_t d; + + page = (void *)__get_free_page(GFP_KERNEL | GFP_DMA); + if (page == NULL) + goto err; + + d = dma_map_single(priv->dev, page, PAGE_SIZE, DMA_FROM_DEVICE); + if (unlikely(dma_mapping_error(priv->dev, d))) { + free_page((unsigned long)page); + goto err; + } + + /* + * The hardware enforces a sub-2K maximum packet size, so we + * put two buffers on every hardware page. + */ + ftmac100_rxdes_set_va(rxdes, page); + ftmac100_rxdes_set_va(rxdes + 1, page + PAGE_SIZE / 2); + + ftmac100_rxdes_set_dma_addr(rxdes, d); + ftmac100_rxdes_set_dma_addr(rxdes + 1, d + PAGE_SIZE / 2); + + ftmac100_rxdes_set_buffer_size(rxdes, RX_BUF_SIZE); + ftmac100_rxdes_set_buffer_size(rxdes + 1, RX_BUF_SIZE); + + ftmac100_rxdes_set_dma_own(rxdes); + ftmac100_rxdes_set_dma_own(rxdes + 1); + }[...]
Quoted 3 lines
Did you test this? This looks like it will result in double free after packet RX, as you are giving the same page (referenced once) to two distinct RX descriptors, that may be assigned different packets.
Yes, this is tested.
Quoted 2 lines
Since your not implementing any RX offloads, you might just allocate fresh skb's with
alloc_skb()and store skb pointer in rxdes3. Since
rxdes3 does not store virtual address of an skb.
It stores the address of the buffer allocated while open() and freed only when stop().
The data in that buffer will be memcpy()ed to an skb allocated in ftmac100_rx_packet().
No double free happens.
Quoted 2 lines
hardware doesn't touch it, you can skip cpu_to_le32()/le32_to_cpu() there (leave a comment, though).
Agree. Thanks.
Quoted 2 lines
Unless this needs to work for ISA devices, you should drop
GFP_DMAallocation flag.
Ben mentioned about this in the previous mail. I thought that it is OK to keep GFP_DMA
on ARM platform, but since you point out this flag is for ISA, I will drop it.
best regards,
Po-Yu Chuang
W dniu 24 stycznia 2011 09:26 użytkownik Po-Yu Chuang
Quoted 80 lines · <[email protected]> napisał:
2011/1/21 Michał Mirosław <[email protected]>:
Quoted 70 lines
2011/1/21 Po-Yu Chuang <[email protected]>:
Quoted 17 lines
drivers/net/ftmac100.c new file +17
-0+static void ftmac100_free_buffers (struct ftmac100 *priv) +{ + int i; + + for (i = 0; i < RX_QUEUE_ENTRIES; i += 2) { + struct ftmac100_rxdes *rxdes = &priv->descs->rxdes[i]; + dma_addr_t d = ftmac100_rxdes_get_dma_addr(rxdes); + void *page = ftmac100_rxdes_get_va(rxdes); + + if (d) + dma_unmap_single(priv->dev, d, PAGE_SIZE, + DMA_FROM_DEVICE); + + if (page != NULL) + free_page((unsigned long)page); + } +[...]
Quoted 48 lines
drivers/net/ftmac100.c new file +48
-0+static int ftmac100_alloc_buffers (struct ftmac100 *priv) +{ + int i; + + priv->descs = dma_alloc_coherent(priv->dev, + sizeof(struct ftmac100_descs), + &priv->descs_dma_addr, + GFP_KERNEL | GFP_DMA); + if (priv->descs == NULL) + return -ENOMEM; + + memset(priv->descs, 0, sizeof(struct ftmac100_descs)); + + /* initialize RX ring */ + + ftmac100_rxdes_set_end_of_ring(&priv->descs->rxdes[RX_QUEUE_ENTRIES - 1]); + + for (i = 0; i < RX_QUEUE_ENTRIES; i += 2) { + struct ftmac100_rxdes *rxdes = &priv->descs->rxdes[i]; + void *page; + dma_addr_t d; + + page = (void *)__get_free_page(GFP_KERNEL | GFP_DMA); + if (page == NULL) + goto err; + + d = dma_map_single(priv->dev, page, PAGE_SIZE, DMA_FROM_DEVICE); + if (unlikely(dma_mapping_error(priv->dev, d))) { + free_page((unsigned long)page); + goto err; + } + + /* + * The hardware enforces a sub-2K maximum packet size, so we + * put two buffers on every hardware page. + */ + ftmac100_rxdes_set_va(rxdes, page); + ftmac100_rxdes_set_va(rxdes + 1, page + PAGE_SIZE / 2); + + ftmac100_rxdes_set_dma_addr(rxdes, d); + ftmac100_rxdes_set_dma_addr(rxdes + 1, d + PAGE_SIZE / 2); + + ftmac100_rxdes_set_buffer_size(rxdes, RX_BUF_SIZE); + ftmac100_rxdes_set_buffer_size(rxdes + 1, RX_BUF_SIZE); + + ftmac100_rxdes_set_dma_own(rxdes); + ftmac100_rxdes_set_dma_own(rxdes + 1); + }[...]
Quoted 3 lines
Did you test this? This looks like it will result in double free after packet RX, as you are giving the same page (referenced once) to two distinct RX descriptors, that may be assigned different packets.
Yes, this is tested.
Quoted 2 lines
Since your not implementing any RX offloads, you might just allocate fresh skb's with
alloc_skb()and store skb pointer in rxdes3. Since
Quoted 6 lines
rxdes3 does not store virtual address of an skb.
It stores the address of the buffer allocated whileopen()and freed only whenstop().
The data in that buffer will be memcpy()ed to an skb allocated inftmac100_rx_packet().
No double free happens.
Ah, I blindly assumed that you're just appending the buffers to the skb (using skb_fill_page_desc() and friends). Since you have to mark descriptors for the device anyway, it might be faster to allocate new skbs and map those as rx buffers (changing the descriptor's buffer address after every RX) instead of keeping static buffer and copying every time. (For small packets it wastes lot of memory, though - so the right choice depends on the expected workload.)
Best Regards,
Michał Mirosław
Dear Michał,
2011/1/25 Michał Mirosław <[email protected]>:
Quoted 19 lines
W dniu 24 stycznia 2011 09:26 użytkownik Po-Yu Chuang
Quoted 10 lines · <[email protected]> napisał:
2011/1/21 Michał Mirosław <[email protected]>:
Quoted 3 lines
Did you test this? This looks like it will result in double free after packet RX, as you are giving the same page (referenced once) to two distinct RX descriptors, that may be assigned different packets.
Yes, this is tested.
Quoted 2 lines
Since your not implementing any RX offloads, you might just allocate fresh skb's with
alloc_skb()and store skb pointer in rxdes3. SinceQuoted 6 lines
rxdes3 does not store virtual address of an skb.
It stores the address of the buffer allocated whileopen()and freed only whenstop().
The data in that buffer will be memcpy()ed to an skb allocated inftmac100_rx_packet().
No double free happens.
Quoted 7 lines
Ah, I blindly assumed that you're just appending the buffers to the skb (using
skb_fill_page_desc()and friends). Since you have to mark descriptors for the device anyway, it might be faster to allocate new skbs and map those as rx buffers (changing the descriptor's buffer address after every RX) instead of keeping static buffer and copying every time. (For small packets it wastes lot of memory, though - so the right choice depends on the expected workload.)
The reason I chose to use memcpy rx buffer to skb is that I didn't know how to deal with multi-segment packets if I preallocated skb for each rx descriptor. This function seems to be what I need.
Let me rework this. Thanks.
best regards,
Po-Yu Chuang
Dear Michał
2011/1/25 Po-Yu Chuang <[email protected]>:
Quoted 10 lines
2011/1/25 Michał Mirosław <[email protected]>:
Quoted 8 lines
Ah, I blindly assumed that you're just appending the buffers to the skb (using
skb_fill_page_desc()and friends). Since you have to mark descriptors for the device anyway, it might be faster to allocate new skbs and map those as rx buffers (changing the descriptor's buffer address after every RX) instead of keeping static buffer and copying every time. (For small packets it wastes lot of memory, though - so the right choice depends on the expected workload.)
Quoted 4 lines
The reason I chose to use memcpy rx buffer to skb is that I didn't know how to deal with multi-segment packets if I preallocated skb for each rx descriptor. This function seems to be what I need.
Let me rework this. Thanks.
After looking at some drivers using skb_fill_page_desc(), I am still confused.
It seems that this function is mainly for jumbo packet and the beginning of data of received packet should still be in skb->data.
Scheme 1)
If I simply allocate a page for each rx ring entry, I still need to allocate an skb and copy at least packet header in first page to skb->data. Then add the page of rest of payload to skb by skb_fill_page_desc().
Scheme 2)
If I simply allocate an skb for each rx ring entry, zero copy can be easily achieved if the packet contains only one segment (one rx ring entry). However, if the received packet has more than one segment, I guess I might need to allocate a bigger skb and copy data of all segments (skb->data) to the new skb?
The problem is, in most cases, the received packets are one-segment only. (TBH, I don't know when will multi-segment packets appear.)
If all packets are one-segment, I can just allocate an skb for each rx ring enry and achieve zero-copy. However, since there might be multi-segment packets, I need to deal with them.
How can I do?
best regards,
Po-Yu Chuang
David Miller
maintainerFrom: Po-Yu Chuang <[email protected]>
Date: Tue, 1 Feb 2011 11:56:16 +0800
Quoted 3 lines
If I simply allocate a page for each rx ring entry, I still need to allocate an skb and copy at least packet header in first page to
skb->data. Then add the page of rest of payload to skb byskb_fill_page_desc().
You should attach the pages, the use __pskb_pull_tail() to bring in the headers to the linear skb->data area.
See drivers/net/niu.c:niu_process_rx_pkt().
Hi David,
Quoted 11 lines · David Miller
From: Po-Yu Chuang <[email protected]>
Date: Tue, 1 Feb 2011 11:56:16 +0800Quoted 3 lines
If I simply allocate a page for each rx ring entry, I still need to allocate an skb and copy at least packet header in first page to
skb->data. Then add the page of rest of payload to skb byskb_fill_page_desc().You should attach the pages, the use
__pskb_pull_tail()to bring in the headers to the linearskb->dataarea.See drivers/net/niu.c:niu_process_rx_pkt().
I tried two ways to implement zero-copy.
One is to preallocate skb big enough for any rx packet and use the skb as rx buffer.
The other is use page as rx buffer, use skb_fill_page_desc() to add a data page to
skb and then pull only header to skb by __pskb_pull_tail() as you suggested.
Two implementations are slower than the original memcpy version. (benchmarked with iperf)
I guess the problem is because a HW restriction that the rx buffer must be 64 bits aligned. Since I cannot make rx buffer starts at offset 2 bytes, the IP header, TCP header and data are not 4 bytes aligned. The performance drops drastically.
Therefore, I will submit later a v6 which is still using memcpy().
best regards,
Po-Yu Chuang
David Miller
maintainerFrom: Po-Yu Chuang <[email protected]>
Date: Thu, 24 Feb 2011 15:27:55 +0800
Quoted 4 lines
I guess the problem is because a HW restriction that the rx buffer must be 64 bits aligned. Since I cannot make rx buffer starts at offset 2 bytes, the IP header, TCP header and data are not 4 bytes aligned. The performance drops drastically.
I cannot believe that after 20 years of commodity ethernet networking chips were first designed, people are still designing hardware that doesn't do this right.
Just emit garbage bytes into the sub-word alignment padding if the chip wants to word align it's DMA writes.
Even the 15 year old Dec Tulip chips do this properly.
Hi David,
Quoted 11 lines · David Miller
From: Po-Yu Chuang <[email protected]>
Date: Thu, 24 Feb 2011 15:27:55 +0800Quoted 4 lines
I guess the problem is because a HW restriction that the rx buffer must be 64 bits aligned. Since I cannot make rx buffer starts at offset 2 bytes, the IP header, TCP header and data are not 4 bytes aligned. The performance drops drastically.
I cannot believe that after 20 years of commodity ethernet networking chips were first designed, people are still designing hardware that doesn't do this right.
Ha ha...
Well, this restriction was removed in the later IPs of our company. :-p
Quoted 3 lines
Just emit garbage bytes into the sub-word alignment padding if the chip wants to word align it's DMA writes.
Not sure what do you mean. The problem is that HW does not accept a base address of RX buffer which is not 8 bytes aligned.
Quoted 1 line
Even the 15 year old Dec Tulip chips do this properly.
best regards,
Po-Yu Chuang
Quoted 19 lines · Po-Yu Chuang
Hi David,
Quoted 11 lines · David Miller
From: Po-Yu Chuang <[email protected]>
Date: Thu, 24 Feb 2011 15:27:55 +0800Quoted 4 lines
I guess the problem is because a HW restriction that the rx buffer must be 64 bits aligned. Since I cannot make rx buffer starts at offset 2 bytes, the IP header, TCP header and data are not 4 bytes aligned. The performance drops drastically.
I cannot believe that after 20 years of commodity ethernet networking chips were first designed, people are still designing hardware that doesn't do this right.
Ha ha...
Well, this restriction was removed in the later IPs of our company. :-pQuoted 1 line
Quoted 5 lines
Quoted 2 lines
Just emit garbage bytes into the sub-word alignment padding if the chip wants to word align it's DMA writes.
Not sure what do you mean. The problem is that HW does not accept a base address of RX buffer which is not 8 bytes aligned.
I still dont understand the problem, maybe you should post your work as RFC
Why not using for the first part the skb buffer (eventually copy the first 128 bytes to get aligned IP/TCP header), and add frags for following parts ?
PATCH 1/1 net: add Faraday FTMAC100 10/100 Ethernet driver
From: Po-Yu Chuang <[email protected]>
*** NOTE ***
This patch is only for reference. Its performance is worse than v5. Please do not apply this.
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
---
v2:
always use NAPI
do not use our own net_device_stats structure don't set trans_start and last_rx
stats.rx_packets and stats.rx_bytes include dropped packets add missed netif_napi_del()
initialize spinlocks in probe function
remove rx_lock and hw_lock
use netdev_[err/info/dbg] instead of dev_* ones use netdev_alloc_skb_ip_align()
remove ftmac100_get_stats()
use is_valid_ether_addr() instead of is_zero_ether_addr() add const to ftmac100_ethtool_ops and ftmac100_netdev_ops use net_ratelimit() instead of printk_ratelimit() no explicit inline
use %pM to print MAC address
add comment before wmb
use napi poll() to handle all interrupts
v3:
undo "stats.rx_packets and stats.rx_bytes include dropped packets" ftmac100_mdio_read() returns 0 if error
fix comment typos
use pr_fmt and pr_info
define INT_MASK_ALL_ENABLED
define MACCR_ENABLE_ALL
do not count length error many times
use bool/true/false
use cpu_to_le32/le32_to_cpu to access descriptors indent style fix
v4:
should not access skb after netif_receive_skb() use resource_size()
better way to use cpu_to_le32/le32_to_cpu
use spin_lock() for tx_lock
combine all netdev_info() together in ftmac100_poll()
v5:
use dev_kfree_skb() in ftmac100_tx_complete_packet() cpu_to_le32/le32_to_cpu usage fix
drop GFP_DMA
ref0:
cpu_to_le32/le32_to_cpu usage fix
remove "tx queue full" message
reduce critical section protected by tx_lock
add check of MAX_PKT_SIZE and RX_BUF_SIZE
add __exit to ftmac100_remove()
zero copy - use skb_fill_page_desc() and __pskb_pull_tail().
drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1196 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 ++++++++ 4 files changed, 1386 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
drivers/net/Kconfig +9 -0
@@ -2014,6 +2014,15 @@ config BCM63XX_ENET This driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX). +config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 10/100 Ethernet controller + from Faraday. It is used on Faraday A320, Andes AG101 and some + other ARM/NDS32 SoC's. + source "drivers/net/fs_enet/Kconfig" source "drivers/net/octeon/Kconfig"
drivers/net/Makefile +1 -0
@@ -147,6 +147,7 @@ obj-$(CONFIG_FORCEDETH) += forcedeth.o obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o obj-$(CONFIG_AX88796) += ax88796.o obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o +obj-$(CONFIG_FTMAC100) += ftmac100.o obj-$(CONFIG_TSI108_ETH) += tsi108_eth.o obj-$(CONFIG_MV643XX_ETH) += mv643xx_eth.o
drivers/net/ftmac100.c new file +1196 -0
1,196 lines not on this page · raw message
drivers/net/ftmac100.h new file +180 -0
180 lines not on this page · raw message
Signature · 1 line
1.6.3.3
Quoted 22 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
drivers/net/ftmac100.c new file +18
-0+ +static bool ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ +struct net_device *netdev = priv->netdev; +struct ftmac100_rxdes *rxdes; +struct sk_buff *skb; +struct page *page; +dma_addr_t map; +int length; + + rxdes =ftmac100_rx_locate_first_segment (priv); +if (!rxdes) +return false; + +if (unlikely (ftmac100_rx_packet_error (priv, rxdes))) { +ftmac100_rx_drop_packet (priv); +return true; + }
drivers/net/ftmac100.c line 421
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ +/* +* It is impossible to get multi-segment packets +* because we always provide big enough receive buffers. +*/ +if (unlikely (!ftmac100_rxdes_last_segment (rxdes))) +BUG (); + +/* start processing */ + skb =netdev_alloc_skb_ip_align (netdev,ETH_HLEN );
Oh I see... You should allocate a bigger head (say... 128 bytes)
And copy in it up to 128 bytes of first part... this to avoid upper stack to reallocate skb head (because IP/TCP processing need to get their headers in skb head)
Quoted 38 lines
drivers/net/ftmac100.c new file +38
-0+if (unlikely (!skb)) { +if (net_ratelimit ()) +netdev_err (netdev,"rx skb alloc failed\n" ); + +ftmac100_rx_drop_packet (priv); +return true; + } + +if (unlikely (ftmac100_rxdes_multicast (rxdes))) + netdev->stats.multicast++; + + map =ftmac100_rxdes_get_dma_addr (rxdes); +dma_unmap_page (priv->dev, map,RX_BUF_SIZE ,DMA_FROM_DEVICE ); + + length =ftmac100_rxdes_frame_length (rxdes); + page =ftmac100_rxdes_get_page (rxdes); +skb_fill_page_desc (skb,0 , page,0 , length); + skb->len += length; + skb->data_len += length; + skb->truesize += length; +__pskb_pull_tail (skb,ETH_HLEN ); + +ftmac100_alloc_rx_page (priv, rxdes); + +ftmac100_rx_pointer_advance (priv); + + skb->protocol =eth_type_trans (skb, netdev); + + netdev->stats.rx_packets++; + netdev->stats.rx_bytes += skb->len; + +/* push packet to protocol stack */ +netif_receive_skb (skb); + + (*processed)++; +return true; +} +
Quoted 36 lines · Eric Dumazet
Quoted 22 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
drivers/net/ftmac100.c new file +18
-0+ +static bool ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ +struct net_device *netdev = priv->netdev; +struct ftmac100_rxdes *rxdes; +struct sk_buff *skb; +struct page *page; +dma_addr_t map; +int length; + + rxdes =ftmac100_rx_locate_first_segment (priv); +if (!rxdes) +return false; + +if (unlikely (ftmac100_rx_packet_error (priv, rxdes))) { +ftmac100_rx_drop_packet (priv); +return true; + }drivers/net/ftmac100.c line 421
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ +/* +* It is impossible to get multi-segment packets +* because we always provide big enough receive buffers. +*/ +if (unlikely (!ftmac100_rxdes_last_segment (rxdes))) +BUG (); + +/* start processing */ + skb =netdev_alloc_skb_ip_align (netdev,ETH_HLEN );Oh I see... You should allocate a bigger head (say... 128 bytes)
Quoted 3 lines
And copy in it up to 128 bytes of first part... this to avoid upper stack to reallocate skb head (because IP/TCP processing need to get their headers in skb head)
Take a look at drivers/net/niu.c :
#define RX_SKB_ALLOC_SIZE 128 + NET_IP_ALIGN
static int niu_process_rx_pkt(...)
{
...
skb = netdev_alloc_skb(np->dev, RX_SKB_ALLOC_SIZE);
...
while (1) {
...
niu_rx_skb_append(skb, page, off, append_size);
}
}Hi Eric,
Quoted 42 lines · Eric Dumazet
Quoted 33 lines · Eric Dumazet
Quoted 19 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
drivers/net/ftmac100.c new file +18
-0+ +static bool ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ + struct net_device *netdev = priv->netdev; + struct ftmac100_rxdes *rxdes; + struct sk_buff *skb; + struct page *page; + dma_addr_t map; + int length; + + rxdes = ftmac100_rx_locate_first_segment(priv); + if (!rxdes) + return false; + + if (unlikely(ftmac100_rx_packet_error(priv, rxdes))) { + ftmac100_rx_drop_packet(priv); + return true; + }drivers/net/ftmac100.c line 421
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ + /* + * It is impossible to get multi-segment packets + * because we always provide big enough receive buffers. + */ + if (unlikely(!ftmac100_rxdes_last_segment(rxdes))) + BUG(); + + /* start processing */ + skb = netdev_alloc_skb_ip_align(netdev, ETH_HLEN);Oh I see... You should allocate a bigger head (say... 128 bytes)
Quoted 3 lines
And copy in it up to 128 bytes of first part... this to avoid upper stack to reallocate skb head (because IP/TCP processing need to get their headers in skb head)
Take a look at
drivers/net/niu.c:#define
RX_SKB_ALLOC_SIZE128 +NET_IP_ALIGN
Quoted 10 lines
static int
niu_process_rx_pkt(...)
{
...
skb =netdev_alloc_skb(np->dev, RX_SKB_ALLOC_SIZE);
...
while (1) {
...
niu_rx_skb_append(skb, page, off,append_size); }
}
Oh I got it.
I will try this and redo the benchmarking.
Thanks,
Po-Yu Chuang
Hi Eric,
Quoted 54 lines · Po-Yu Chuang
Quoted 42 lines · Eric Dumazet
Quoted 33 lines · Eric Dumazet
Quoted 19 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
drivers/net/ftmac100.c new file +18
-0+ +static bool ftmac100_rx_packet (struct ftmac100 *priv,int *processed) +{ + struct net_device *netdev = priv->netdev; + struct ftmac100_rxdes *rxdes; + struct sk_buff *skb; + struct page *page; + dma_addr_t map; + int length; + + rxdes = ftmac100_rx_locate_first_segment(priv); + if (!rxdes) + return false; + + if (unlikely(ftmac100_rx_packet_error(priv, rxdes))) { + ftmac100_rx_drop_packet(priv); + return true; + }drivers/net/ftmac100.c line 421
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ + /* + * It is impossible to get multi-segment packets + * because we always provide big enough receive buffers. + */ + if (unlikely(!ftmac100_rxdes_last_segment(rxdes))) + BUG(); + + /* start processing */ + skb = netdev_alloc_skb_ip_align(netdev, ETH_HLEN);Oh I see... You should allocate a bigger head (say... 128 bytes)
Quoted 3 lines
And copy in it up to 128 bytes of first part... this to avoid upper stack to reallocate skb head (because IP/TCP processing need to get their headers in skb head)
Take a look at
drivers/net/niu.c:#define
RX_SKB_ALLOC_SIZE128 +NET_IP_ALIGNQuoted 10 lines
static int
niu_process_rx_pkt(...)
{
...
skb =netdev_alloc_skb(np->dev, RX_SKB_ALLOC_SIZE);
...
while (1) {
...
niu_rx_skb_append(skb, page, off,append_size); }
}
Quoted 3 lines
Oh I got it.
I will try this and redo the benchmarking.
It's a little faster than v5 now. Thanks.
I will submit the current version later.
One more question just curious, why 128 bytes?
best regards,
Po-Yu Chuang
Quoted 4 lines · Po-Yu Chuang
It's a little faster than v5 now. Thanks.
I will submit the current version later.One more question just curious, why 128 bytes?
Probably its best for NIU hardware specs
You could try 64, as it should be enough for most IP/TCP/UDP processing.
David Miller
maintainerFrom: Eric Dumazet <[email protected]>
Date: Fri, 25 Feb 2011 11:52:07 +0100
Quoted 10 lines · Po-Yu Chuang
Quoted 5 lines
It's a little faster than v5 now. Thanks.
I will submit the current version later.One more question just curious, why 128 bytes?
Probably its best for NIU hardware specs
You could try 64, as it should be enough for most IP/TCP/UDP processing.
IPV6.
Quoted 8 lines · David Miller
From: Eric Dumazet <[email protected]>
Date: Fri, 25 Feb 2011 11:52:07 +0100Quoted 5 lines · Po-Yu Chuang
Quoted 4 lines
It's a little faster than v5 now. Thanks.
I will submit the current version later.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
One more question just curious, why 128 bytes?
Probably its best for NIU hardware specs
You could try 64, as it should be enough for most IP/TCP/UDP processing.
IPV6.
drivers/net/benet/be.h:70:#define BE_HDR_LEN 64
Maybe we should have a comment somewhere.
CC Ajit Khaparde <[email protected]>
Quoted 15 lines · Eric Dumazet
Quoted 14 lines · David Miller
From: Eric Dumazet <[email protected]>
Date: Fri, 25 Feb 2011 11:52:07 +0100Quoted 10 lines · Po-Yu Chuang
Quoted 5 lines
It's a little faster than v5 now. Thanks.
I will submit the current version later.One more question just curious, why 128 bytes?
Probably its best for NIU hardware specs
You could try 64, as it should be enough for most IP/TCP/UDP processing.
Quoted 8 lines
Quoted 1 line
IPV6.
drivers/net/benet/be.h:70:#define
BE_HDR_LEN64Maybe we should have a comment somewhere.
CC Ajit Khaparde <[email protected]>
A compromise would be to use 128 for the allocation, but only copy 64 bytes.
Hi Eric,
Quoted 20 lines · Eric Dumazet
Quoted 19 lines · Eric Dumazet
Quoted 8 lines · David Miller
From: Eric Dumazet <[email protected]>
Date: Fri, 25 Feb 2011 11:52:07 +0100Quoted 5 lines · Po-Yu Chuang
Quoted 4 lines
It's a little faster than v5 now. Thanks.
I will submit the current version later.Quoted 7 lines
Quoted 5 lines
Quoted 1 line
One more question just curious, why 128 bytes?
Probably its best for NIU hardware specs
You could try 64, as it should be enough for most IP/TCP/UDP processing.
IPV6.
drivers/net/benet/be.h:70:#define
BE_HDR_LEN64
Quoted 8 lines
Quoted 4 lines
Maybe we should have a comment somewhere.
CC Ajit Khaparde <[email protected]>
A compromise would be to use 128 for the allocation, but only copy 64 bytes.
I will use this way and submit v7 later.
Thanks,
Po-Yu Chuang
Quoted 10 lines · Po-Yu Chuang
Hi Eric,
Quoted 3 lines · Eric Dumazet
A compromise would be to use 128 for the allocation, but only copy 64 bytes.
I will use this way and submit v7 later.
BTW, even with IPV6, the first 64 bytes contain "RPS" header
We changed NET_SKB_PAD a while back and added this comment in include/linux/skbuff.h
NET_IP_ALIGN(2) + ethernet_header(14) + IP_header(20/40) + ports(8)
PATCH v6 1/1 net: add Faraday FTMAC100 10/100 Ethernet driver
In reply to Po-Yu ChuangFrom: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
---
v2:
always use NAPI
do not use our own net_device_stats structure don't set trans_start and last_rx
stats.rx_packets and stats.rx_bytes include dropped packets add missed netif_napi_del()
initialize spinlocks in probe function
remove rx_lock and hw_lock
use netdev_[err/info/dbg] instead of dev_* ones use netdev_alloc_skb_ip_align()
remove ftmac100_get_stats()
use is_valid_ether_addr() instead of is_zero_ether_addr() add const to ftmac100_ethtool_ops and ftmac100_netdev_ops use net_ratelimit() instead of printk_ratelimit() no explicit inline
use %pM to print MAC address
add comment before wmb
use napi poll() to handle all interrupts
v3:
undo "stats.rx_packets and stats.rx_bytes include dropped packets" ftmac100_mdio_read() returns 0 if error
fix comment typos
use pr_fmt and pr_info
define INT_MASK_ALL_ENABLED
define MACCR_ENABLE_ALL
do not count length error many times
use bool/true/false
use cpu_to_le32/le32_to_cpu to access descriptors indent style fix
v4:
should not access skb after netif_receive_skb() use resource_size()
better way to use cpu_to_le32/le32_to_cpu
use spin_lock() for tx_lock
combine all netdev_info() together in ftmac100_poll()
v5:
use dev_kfree_skb() in ftmac100_tx_complete_packet() cpu_to_le32/le32_to_cpu usage fix
drop GFP_DMA
v6:
cpu_to_le32/le32_to_cpu usage fix
remove "tx queue full" message
reduce critical section protected by tx_lock
add check of MAX_PKT_SIZE and RX_BUF_SIZE
add __exit to ftmac100_remove()
simplify ftmac100_rx_packet()
zero copy - use skb_fill_page_desc() and __pskb_pull_tail(). pull more data to skb head to include tcp/ip header
drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1198 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 ++++++++ 4 files changed, 1388 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
drivers/net/Kconfig +9 -0
@@ -2014,6 +2014,15 @@ config BCM63XX_ENET This driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX). +config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 10/100 Ethernet controller + from Faraday. It is used on Faraday A320, Andes AG101 and some + other ARM/NDS32 SoC's. + source "drivers/net/fs_enet/Kconfig" source "drivers/net/octeon/Kconfig"
drivers/net/Makefile +1 -0
@@ -147,6 +147,7 @@ obj-$(CONFIG_FORCEDETH) += forcedeth.o obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o obj-$(CONFIG_AX88796) += ax88796.o obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o +obj-$(CONFIG_FTMAC100) += ftmac100.o obj-$(CONFIG_TSI108_ETH) += tsi108_eth.o obj-$(CONFIG_MV643XX_ETH) += mv643xx_eth.o
drivers/net/ftmac100.c new file +1198 -0
1,198 lines not on this page · raw message
drivers/net/ftmac100.h new file +180 -0
180 lines not on this page · raw message
Signature · 1 line
1.6.3.3
Quoted 7 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
Signed-off-by Po-Yu Chuang <[email protected]>
It seems fine to me, but I have somes questions
1) On V5, the receive function ftmac100_rx_packet() was able to process several segments per skb. On V6 you process one frag only.
Isnt this NIC able to handle large MTU (say... 9000) ?
2) ftmac100_alloc_rx_page() is called and allocate a full page for a rxdes.
128*4K -> 512 Kbytes of memory for RX ring
In V5, you were using half pages only, so 256 kbytes of memory.
If you look at other drivers (NIU, BENET), they are able to use exactly 128*2 kbytes (for a rxring of 128 slots, and 2Kbytes per slot)
Hi Eric,
Quoted 7 lines · Eric Dumazet
Quoted 6 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
Quoted 8 lines
Quoted 1 line
Signed-off-by Po-Yu Chuang <[email protected]>It seems fine to me, but I have somes questions
1) On V5, the receive function
ftmac100_rx_packet()was able to process several segments per skb. On V6 you process one frag only.Isnt this NIC able to handle large MTU (say... 9000) ?
No, it isn't. Its max supported packet size is 0x7ff (2047 bytes).
I wasn't sure when will get multi-segment packet. After confirmed with HW designer two weeks ago, he told me that only if the rx buffer described by the first descriptor is not big enough, another rx buffer will be used.
That means, since every rx buffer is large enough, it is impossible to get a multi-segment packets.
So I simplified ftmac100_rx_packet().
Quoted 10 lines
2)
ftmac100_alloc_rx_page()is called and allocate a full page for a rxdes.128*4K -> 512 Kbytes of memory for RX ring
In V5, you were using half pages only, so 256 kbytes of memory.
If you look at other drivers (NIU, BENET), they are able to use exactly 128*2 kbytes (for a rxring of 128 slots, and 2Kbytes per slot)
Both NIU and BENET refill rx ring after all packets receive work done, but I allocate rx buffer right after a packet is received. I think it is difficult
to use a page for two rx buffers in my driver now. Can we just keep it the way it is? I can study how to achieve that after current driver is accepted.
best regards,
Po-Yu Chuang
Quoted 10 lines · Po-Yu Chuang
Hi Eric,
On Fri, Feb 25, 2011 at 7:40 PM, Eric Dumazet <[email protected]> wrote:
Both NIU and BENET refill rx ring after all packets receive work done, but I allocate rx buffer right after a packet is received. I think it is difficult
to use a page for two rx buffers in my driver now. Can we just keep it the way it is? I can study how to achieve that after current driver is accepted.
No problem, you are the author, I am only a reviewer and ask questions after all ;)
Thanks !
Quoted 4 lines · Po-Yu Chuang
That means, since every rx buffer is large enough, it is impossible to get a multi-segment packets.
So I simplified
ftmac100_rx_packet().
I am only wondering then if not using fragments would be faster then (eventually doing copybreaks for small frames like tg3)
But if you plan to get new hw soon, you have the infrastructure...
Hi Eric,
Quoted 8 lines · Eric Dumazet
Quoted 4 lines · Po-Yu Chuang
That means, since every rx buffer is large enough, it is impossible to get a multi-segment packets.
So I simplified
ftmac100_rx_packet().Signed-off-by Eric Dumazet <[email protected]>
Ah, I just want to submit a v7.
Could you resign again later, please? :-)
Quoted 2 lines
I am only wondering then if not using fragments would be faster then (eventually doing copybreaks for small frames like tg3)
Although not many circumstances are tested.
iperf shows that it is a little faster to use fragments than memcpy, so...
Quoted 1 line
But if you plan to get new hw soon, you have the infrastructure...
best regards,
Po-Yu Chuang
Quoted 2 lines · Po-Yu Chuang
Ah, I just want to submit a v7.
Could you resign again later, please? :-)
Sure
Quoted 7 lines · Po-Yu Chuang
Quoted 2 lines · Eric Dumazet
I am only wondering then if not using fragments would be faster then (eventually doing copybreaks for small frames like tg3)
Although not many circumstances are tested.
iperf shows that it is a little faster to use fragments than memcpy, so...
This might be a side effect of skb->truesize being smaller with fragments than "regular packets" and socket backlog congestion.
If a full page was really accounted for in skb->truesize, instead of used length, performance might be the same or lower :(
- skb->truesize += length;
+ skb->truesize += PAGE_SIZE;
This has nothing to do with your driver, but a core implementation detail.
PATCH 1/1 net: add Faraday FTMAC100 10/100 Ethernet driver
In reply to Po-Yu ChuangFrom: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
---
v2:
always use NAPI
do not use our own net_device_stats structure don't set trans_start and last_rx
stats.rx_packets and stats.rx_bytes include dropped packets add missed netif_napi_del()
initialize spinlocks in probe function
remove rx_lock and hw_lock
use netdev_[err/info/dbg] instead of dev_* ones use netdev_alloc_skb_ip_align()
remove ftmac100_get_stats()
use is_valid_ether_addr() instead of is_zero_ether_addr() add const to ftmac100_ethtool_ops and ftmac100_netdev_ops use net_ratelimit() instead of printk_ratelimit() no explicit inline
use %pM to print MAC address
add comment before wmb
use napi poll() to handle all interrupts
v3:
undo "stats.rx_packets and stats.rx_bytes include dropped packets" ftmac100_mdio_read() returns 0 if error
fix comment typos
use pr_fmt and pr_info
define INT_MASK_ALL_ENABLED
define MACCR_ENABLE_ALL
do not count length error many times
use bool/true/false
use cpu_to_le32/le32_to_cpu to access descriptors indent style fix
v4:
should not access skb after netif_receive_skb() use resource_size()
better way to use cpu_to_le32/le32_to_cpu
use spin_lock() for tx_lock
combine all netdev_info() together in ftmac100_poll()
v5:
use dev_kfree_skb() in ftmac100_tx_complete_packet() cpu_to_le32/le32_to_cpu usage fix
drop GFP_DMA
v6:
cpu_to_le32/le32_to_cpu usage fix
remove "tx queue full" message
reduce critical section protected by tx_lock
add check of MAX_PKT_SIZE and RX_BUF_SIZE
add __exit to ftmac100_remove()
simplify ftmac100_rx_packet()
zero copy - use skb_fill_page_desc() and __pskb_pull_tail(). pull more data to skb head to include tcp/ip header
v7:
allocate 128 bytes skb and pull 64 bytes only
drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1196 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 ++++++++ 4 files changed, 1386 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
drivers/net/Kconfig +9 -0
@@ -2014,6 +2014,15 @@ config BCM63XX_ENET This driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX). +config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 10/100 Ethernet controller + from Faraday. It is used on Faraday A320, Andes AG101 and some + other ARM/NDS32 SoC's. + source "drivers/net/fs_enet/Kconfig" source "drivers/net/octeon/Kconfig"
drivers/net/Makefile +1 -0
@@ -147,6 +147,7 @@ obj-$(CONFIG_FORCEDETH) += forcedeth.o obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o obj-$(CONFIG_AX88796) += ax88796.o obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o +obj-$(CONFIG_FTMAC100) += ftmac100.o obj-$(CONFIG_TSI108_ETH) += tsi108_eth.o obj-$(CONFIG_MV643XX_ETH) += mv643xx_eth.o
drivers/net/ftmac100.c new file +1196 -0
1,196 lines not on this page · raw message
drivers/net/ftmac100.h new file +180 -0
180 lines not on this page · raw message
Signature · 1 line
1.6.3.3
Quoted 64 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
Signed-off-by Po-Yu Chuang <[email protected]>---
v2:
always use NAPI
do not use our ownnet_device_statsstructure don't settrans_startandlast_rx
stats.rx_packets and stats.rx_bytes include dropped packets add missednetif_napi_del()
initialize spinlocks in probe function
removerx_lockandhw_lock
use netdev_[err/info/dbg] instead of dev_* ones usenetdev_alloc_skb_ip_align()
removeftmac100_get_stats()
useis_valid_ether_addr()instead ofis_zero_ether_addr()add const toftmac100_ethtool_opsandftmac100_netdev_opsusenet_ratelimit()instead ofprintk_ratelimit()no explicit inline
use %pM to print MAC address
add comment before wmb
use napipoll()to handle all interruptsv3:
undo "stats.rx_packets and stats.rx_bytes include dropped packets"ftmac100_mdio_read()returns 0 if error
fix comment typos
usepr_fmtandpr_info
defineINT_MASK_ALL_ENABLED
defineMACCR_ENABLE_ALL
do not count length error many times
use bool/true/false
use cpu_to_le32/le32_to_cpu to access descriptors indent style fixv4:
should not access skb afternetif_receive_skb()useresource_size()
better way to use cpu_to_le32/le32_to_cpu
usespin_lock()fortx_lock
combine allnetdev_info()together inftmac100_poll()v5:
usedev_kfree_skb()inftmac100_tx_complete_packet()cpu_to_le32/le32_to_cpu usage fix
dropGFP_DMAv6:
cpu_to_le32/le32_to_cpu usage fix
remove "tx queue full" message
reduce critical section protected bytx_lock
add check ofMAX_PKT_SIZEandRX_BUF_SIZE
add__exittoftmac100_remove()
simplifyftmac100_rx_packet()
zero copy - useskb_fill_page_desc()and__pskb_pull_tail(). pull more data to skb head to include tcp/ip headerv7:
allocate 128 bytes skb and pull 64 bytes only
Quoted 7 lines
drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1196 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 ++++++++ 4 files changed, 1386 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
From: Eric Dumazet <[email protected]>
Date: Tue, 01 Mar 2011 08:27:00 +0100
Quoted 8 lines · Po-Yu Chuang
Quoted 7 lines
From: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
Signed-off-by Po-Yu Chuang <[email protected]>
...
Quoted 2 lines
Signed-off-by Eric Dumazet <[email protected]>
Applied to net-next-2.6, thanks.
David Miller
maintainerFrom: Po-Yu Chuang <[email protected]>
Date: Thu, 24 Feb 2011 16:07:48 +0800
Quoted 6 lines · David Miller
Quoted 2 lines
Just emit garbage bytes into the sub-word alignment padding if the chip wants to word align it's DMA writes.
Not sure what do you mean. The problem is that HW does not accept a base address of RX buffer which is not 8 bytes aligned.
I am saying this is what hardware should do if it has such a restriction.
PATCH v5 1/1 net: add Faraday FTMAC100 10/100 Ethernet driver
In reply to Po-Yu ChuangFrom: Po-Yu Chuang <[email protected]>
FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and MII. This driver has been working on some ARM/NDS32 SoC's including Faraday A320 and Andes AG101.
---
v2:
always use NAPI
do not use our own net_device_stats structure don't set trans_start and last_rx
stats.rx_packets and stats.rx_bytes include dropped packets add missed netif_napi_del()
initialize spinlocks in probe function
remove rx_lock and hw_lock
use netdev_[err/info/dbg] instead of dev_* ones use netdev_alloc_skb_ip_align()
remove ftmac100_get_stats()
use is_valid_ether_addr() instead of is_zero_ether_addr() add const to ftmac100_ethtool_ops and ftmac100_netdev_ops use net_ratelimit() instead of printk_ratelimit() no explicit inline
use %pM to print MAC address
add comment before wmb
use napi poll() to handle all interrupts
v3:
undo "stats.rx_packets and stats.rx_bytes include dropped packets" ftmac100_mdio_read() returns 0 if error
fix comment typos
use pr_fmt and pr_info
define INT_MASK_ALL_ENABLED
define MACCR_ENABLE_ALL
do not count length error many times
use bool/true/false
use cpu_to_le32/le32_to_cpu to access descriptors indent style fix
v4:
should not access skb after netif_receive_skb() use resource_size()
better way to use cpu_to_le32/le32_to_cpu
use spin_lock() for tx_lock
combine all netdev_info() together in ftmac100_poll()
v5:
use dev_kfree_skb() in ftmac100_tx_complete_packet() cpu_to_le32/le32_to_cpu usage fix
drop GFP_DMA
drivers/net/Kconfig | 9 + drivers/net/Makefile | 1 + drivers/net/ftmac100.c | 1212 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/ftmac100.h | 180 +++++++ 4 files changed, 1402 insertions(+), 0 deletions(-) create mode 100644 drivers/net/ftmac100.c create mode 100644 drivers/net/ftmac100.h
drivers/net/Kconfig +9 -0
@@ -2014,6 +2014,15 @@ config BCM63XX_ENET This driver supports the ethernet MACs in the Broadcom 63xx MIPS chipset family (BCM63XX). +config FTMAC100 + tristate "Faraday FTMAC100 10/100 Ethernet support" + depends on ARM + select MII + help + This driver supports the FTMAC100 Ethernet controller from + Faraday. It is used on Faraday A320, Andes AG101, AG101P + and some other ARM/NDS32 SoC's. + source "drivers/net/fs_enet/Kconfig" source "drivers/net/octeon/Kconfig"
drivers/net/Makefile +1 -0
@@ -147,6 +147,7 @@ obj-$(CONFIG_FORCEDETH) += forcedeth.o obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o obj-$(CONFIG_AX88796) += ax88796.o obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o +obj-$(CONFIG_FTMAC100) += ftmac100.o obj-$(CONFIG_TSI108_ETH) += tsi108_eth.o obj-$(CONFIG_MV643XX_ETH) += mv643xx_eth.o
drivers/net/ftmac100.c new file +1212 -0
1,212 lines not on this page · raw message
drivers/net/ftmac100.h new file +180 -0
180 lines not on this page · raw message
Signature · 1 line
1.6.3.3
Quoted 18 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
drivers/net/ftmac100.c new file +15
-0+static int ftmac100_xmit (struct ftmac100 *priv,struct sk_buff *skb, +dma_addr_t map) +{ +struct net_device *netdev = priv->netdev; +struct ftmac100_txdes *txdes; +unsigned int len = (skb->len <ETH_ZLEN ) ?ETH_ZLEN : skb->len; + + txdes =ftmac100_current_txdes (priv); +ftmac100_tx_pointer_advance (priv); + +/* setup TX descriptor */ + +spin_lock (&priv->tx_lock); +ftmac100_txdes_set_skb (txdes, skb); +ftmac100_txdes_set_dma_addr (txdes, map);
drivers/net/ftmac100.c line 651
Quoted 10 lines
drivers/net/ftmac100.c new file +10
-0+ +ftmac100_txdes_set_first_segment (txdes); +ftmac100_txdes_set_last_segment (txdes); +ftmac100_txdes_set_txint (txdes); +ftmac100_txdes_set_buffer_size (txdes, len); + + priv->tx_pending++; +if (priv->tx_pending ==TX_QUEUE_ENTRIES ) { +if (net_ratelimit ()) +netdev_info (netdev,"tx queue full\n" );
Hmm, I guess you didnt tested your driver with a pktgen flood ;)
This 'netdev_info(netdev, "tx queue full\n");' is not necessary, since its a pretty normal condition for a driver (to fill its TX ring buffer)
Quoted 12 lines
drivers/net/ftmac100.c new file +12
-0+ +netif_stop_queue (netdev); + } + +/* start transmit */ +ftmac100_txdes_set_dma_own (txdes); +spin_unlock (&priv->tx_lock); + +ftmac100_txdma_start_polling (priv); + +return NETDEV_TX_OK ; +}
Dear Eric,
Quoted 8 lines · Eric Dumazet
Quoted 6 lines · Po-Yu Chuang
From: Po-Yu Chuang <[email protected]>
+
+ priv->tx_pending++;
+ if (priv->tx_pending==TX_QUEUE_ENTRIES) {
+ if (net_ratelimit())
+netdev_info(netdev, "tx queue full\n");
Quoted 4 lines
Hmm, I guess you didnt tested your driver with a pktgen flood ;)
This '
netdev_info(netdev, "tx queue full\n");' is not necessary, since its a pretty normal condition for a driver (to fill its TX ring buffer)
Errr... No, I didn't test it with pktgen. I tested it with normal NFS usages. I'll try it.
best regards,
Po-Yu Chuang