lkml.org 
[lkml]   [2011]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] net: add Faraday FTMAC100 10/100 Ethernet driver
From
Date
On Thu, 2011-01-13 at 19:49 +0800, Po-Yu Chuang wrote:
> From: Po-Yu Chuang <ratbert@faraday-tech.com>
>
> 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.
[...]
> diff --git a/drivers/net/ftmac100.c b/drivers/net/ftmac100.c
> new file mode 100644
> index 0000000..97d1f8d
> --- /dev/null
> +++ b/drivers/net/ftmac100.c
[...]
> +#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.

[...]
> + 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.

[...]
> +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;
> +
> + 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.

[...]
> +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;
> +
> + 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.

[...]
> + netif_rx(skb);
> +#endif
> +
> + netdev->last_rx = jiffies;

Don't set last_rx; the networking core takes care of it now.

> + 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.

[...]
> +static int ftmac100_xmit(struct ftmac100 *priv, struct sk_buff *skb,
> + dma_addr_t map)
> +{
[...]
> + 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.

[...]
> +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.

[...]
> +/******************************************************************************
> + * 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)) {
> + /*
> + * 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.

[...]
> +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) {
> + 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.

[...]
> +/******************************************************************************
> + * struct platform_driver functions
> + *****************************************************************************/
> +static int ftmac100_remove(struct platform_device *pdev)
> +{
> + struct net_device *netdev;
> + struct ftmac100 *priv;
> +
> + 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.

--
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.



\
 
 \ /
  Last update: 2011-01-13 15:05    [W:0.229 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site