Florian Fainelli <f.faine...@gmail.com> writes:
> On 22/10/15 07:02, Mans Rullgard wrote:
>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>> It is an almost complete rewrite of a driver originally found in
>> a Sigma Designs 2.6.22 tree.
>
> Some reviews here and there, mostly related to how you use the PHY library.
Thanks.
>> + nb8800_tx_dma_start(dev, next);
>> +
>> + if (!skb->xmit_more && !timer_pending(&priv->tx_reclaim_timer))
>> + mod_timer(&priv->tx_reclaim_timer, jiffies + HZ / 20);
>
> You do not have a TX completion interrupt? Using a timer to reclaim TX
> buffers is really not great for latency.
There is an interrupt, but dev_kfree_skb() can't be called from
interrupt context, and running a tasklet for each packet has too much
overhead. Someone suggested I use this approach. If there's a better
way, I'm all ears.
>> +
>> + if (atomic_read(&priv->tx_free) <= NB8800_DESC_LOW)
>> + netif_stop_queue(dev);
>> +
>> + return NETDEV_TX_OK;
>> +}
>> +
>> +static void nb8800_tx_reclaim(unsigned long data)
>> +{
>> + struct net_device *dev = (struct net_device *)data;
>> + struct nb8800_priv *priv = netdev_priv(dev);
>> + int packets = 0, bytes = 0;
>> + int reclaimed = 0;
>> + int dirty, limit;
>> +
>> + dirty = xchg(&priv->tx_dirty, -1);
>> + if (dirty < 0)
>> + return;
>> +
>> + limit = priv->tx_reclaim_limit;
>> + if (dirty == limit)
>> + goto end;
>> +
>> + while (dirty != limit) {
>> + struct nb8800_dma_desc *tx = &priv->tx_descs[dirty];
>> + struct tx_buf *tx_buf = &priv->tx_bufs[dirty];
>> + struct sk_buff *skb = tx_buf->skb;
>> + struct tx_skb_data *skb_data = (struct tx_skb_data *)skb->cb;
>> + int frags = tx_buf->frags;
>> +
>> + packets++;
>> + bytes += skb->len;
>> +
>> + dma_unmap_single(&dev->dev, skb_data->dma_addr,
>> + skb_data->dma_len, DMA_TO_DEVICE);
>> + dev_kfree_skb(skb);
>> +
>> + tx->report = 0;
>> + tx_buf->skb = NULL;
>> + tx_buf->frags = 0;
>> +
>> + dirty = (dirty + frags) & (TX_DESC_COUNT - 1);
>> + reclaimed += frags;
>> + }
>> +
>> + priv->stats.tx_packets += packets;
>> + priv->stats.tx_bytes += bytes;
>> +
>> + smp_mb__before_atomic();
>> + atomic_add(reclaimed, &priv->tx_free);
>> +
>> + netif_wake_queue(dev);
>
> You can only wake up your queue if you have successfully reclaimed
> transmitted buffers, otherwise this is giving false information to the
> stack that there is room to push more packets.
The code is correct, if a bit unclear. I'll clean it up so it's obvious.
>> +static void nb8800_mac_config(struct net_device *dev)
>> +{
>> + struct nb8800_priv *priv = netdev_priv(dev);
>> +
>> + if (priv->duplex)
>> + nb8800_clear_bits(b, priv, NB8800_MAC_MODE, HALF_DUPLEX);
>> + else
>> + nb8800_set_bits(b, priv, NB8800_MAC_MODE, HALF_DUPLEX);
>> +
>> + if (priv->speed == SPEED_1000) {
>> + nb8800_set_bits(b, priv, NB8800_MAC_MODE,
>> + RGMII_MODE | GMAC_MODE);
>> + nb8800_writeb(priv, NB8800_IC_THRESHOLD, 3);
>
> What is the IC_THRESHOLD value for? Is this some form of interrupt
> coalescing? If so, there is a proper ethtool interface to configure that.
It has something to do with some clocks, and this value is quite
possibly wrong; it's what the original driver did. I'll do some tests.
>> + nb8800_writeb(priv, NB8800_SLOT_TIME, 255);
>> + } else {
>> + nb8800_clear_bits(b, priv, NB8800_MAC_MODE,
>> + RGMII_MODE | GMAC_MODE);
>> + nb8800_writeb(priv, NB8800_IC_THRESHOLD, 1);
>> + nb8800_writeb(priv, NB8800_SLOT_TIME, 127);
>
> What about if you link at 10Mbits/sec, would the slot time value still
> make sense here?
The documentation says the exact meaning on this register is different
between gigabit and 10/100, so using the same value for 10 and 100 makes
sense. Again, the values used here are from the original driver.
>> +static void nb8800_set_rx_mode(struct net_device *dev)
>> +{
>> + struct nb8800_priv *priv = netdev_priv(dev);
>> + struct netdev_hw_addr *ha;
>> + int af_en;
>> +
>> + if ((dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) ||
>> + netdev_mc_count(dev) > 64)
>
> 64, that's pretty generous for a perfect match filter, nice.
That's bogus; I forgot to delete it. The hardware uses a 64-entry hash
table, and whoever wrote the old driver apparently didn't understand how
it works.
>> +static void nb8800_tangox_init(struct net_device *dev)
>> +{
>> + struct nb8800_priv *priv = netdev_priv(dev);
>> + u32 val;
>> +
>> + val = nb8800_readb(priv, NB8800_TANGOX_PAD_MODE) & 0x78;
>> + if (priv->phydev->supported & PHY_1000BT_FEATURES)
>> + val |= 1;
>> + nb8800_writeb(priv, NB8800_TANGOX_PAD_MODE, val);
>
> Is this possibly a RGMII delay setting? If so, you need to look at
> phydev->interface, not whether Gigabit is supported or not.
This does something to the configuration of the external pins, the
documentation is vague about what.
>> + phydev = phy_find_first(bus);
>> + if (!phydev || phy_read(phydev, MII_BMSR) <= 0) {
>
> What is this additional MII_MBSR read used for?
On one of my boards, phylib misdetects a phy on the second ethernet port
even though there is none. Perhaps I should revisit that problem and
look for a better solution.
>> +static int nb8800_remove(struct platform_device *pdev)
>> +{
>> + struct net_device *ndev = platform_get_drvdata(pdev);
>> + struct nb8800_priv *priv = netdev_priv(ndev);
>> +
>> + unregister_netdev(ndev);
>> + free_irq(ndev->irq, ndev);
>> +
>> + phy_detach(priv->phydev);
>> + mdiobus_unregister(priv->mii_bus);
>> +
>> + clk_disable_unprepare(priv->clk);
>> +
>> + nb8800_dma_free(ndev);
>> + free_netdev(ndev);
>
> Should not there be a tangox specific callback here to de-initialize the HW?
There's nothing specific to do for this hardware. It's easy enough to
add a callback should any future hardware require it.
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
