Re: [PATCH 3/3] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller

[Florian Fainelli <f.fainelli@gmail.com>]

On 23/10/15 08:20, Måns Rullgård wrote:
> Florian Fainelli <f.fainelli@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.

But dev_kfree_skb_irq() works in interrupt context. Typically, what you
would do, depending on your number or RX/TX queue is reclaim transmitted
buffers in NAPI (softirq) context.

In your case, what you could do is use the RX or TX interrupt as an
indication to schedule your napi poll routine and you would cleanup TX
buffers first (cheap, and makes room for RX packets) and if there are RX
packets to process, process them. No need for the tasklet then.

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

That's fine as-is, I missed the goto end at the beginning.

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

Fair enough.

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

Might be best to put the interface in promiscuous mode until you have
proper multicast support. Since this is for a Set-Top box chip, having
proper multicast support still seems like something highly desirable.

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

I think that would be best, if you are currently using the Generic PHY
driver, consider writing a specific driver which would take care of
quirky behavior.
-- 
Florian