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

Andy Shevchenko [off-list ref] writes:

quoted

quoted

quoted

+       nb8800_writel(priv, NB8800_MDIO_CMD, val | MDIO_CMD_GO);
+
+       if (!nb8800_mdio_wait(bus))
+               return -ETIMEDOUT;
+
+       val = nb8800_readl(priv, NB8800_MDIO_STS);
+       if (val & MDIO_STS_ERR)
+               return 0xffff;

Can we return an error here?

That breaks the bus scanning in phylib.

You mean this is non-fatal error?

It's what you get if nothing responded to the address.

quoted

quoted

quoted

+
+               rx_buf = &priv->rx_bufs[next];
+               rx = &priv->rx_descs[next];

quoted

+               report = rx->report;

Maybe you can use rx->report directly below.

It's in uncached memory, so didn't want to have gcc accidentally doing
more reads than necessary.

How it would not be possible without ACCESS_ONCE() or similar?

True, it probably doesn't make a difference.  Sometimes copying a value
to a local variable prevents re-reads due to potential pointer aliasing,
but I don't think that's an issue here.

quoted

quoted

quoted

+static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+       struct nb8800_priv *priv = netdev_priv(dev);
+       struct tx_skb_data *skb_data;
+       struct tx_buf *tx_buf;
+       dma_addr_t dma_addr;
+       unsigned int dma_len;
+       int cpsz, next;
+       int frags;
+
+       if (atomic_read(&priv->tx_free) <= NB8800_DESC_LOW) {
+               netif_stop_queue(dev);
+               return NETDEV_TX_BUSY;
+       }
+
+       cpsz = (8 - (uintptr_t)skb->data) & 7;

So, cast to uintptr_t looks strange in this driver, since used only
twice in such expression, why not to use plain unsigned int * ?

Because it's not the same thing.  uintptr_t is an integer type the same
size as a pointer.  I need to check if the data pointer is 8-byte
aligned as required by the DMA.

Yes, I understand why you're doing this. But since you use lowest
bits, I think result will be the same even without casting.

You can't do that kind of arithmetic on pointer values.  They have to be
cast to an integer type first.

quoted

quoted

quoted

+       nb8800_writeb(priv, NB8800_RANDOM_SEED, 0x08);
+
+       /* TX single deferral params */
+       nb8800_writeb(priv, NB8800_TX_SDP, 0xc);
+
+       /* Threshold for partial full */
+       nb8800_writeb(priv, NB8800_PF_THRESHOLD, 0xff);
+
+       /* Pause Quanta */
+       nb8800_writeb(priv, NB8800_PQ1, 0xff);
+       nb8800_writeb(priv, NB8800_PQ2, 0xff);

Lot of magic numbers above and below.

Those are from the original driver.  Some of them disagree with the
documentation, and the "correct" values don't work.

It would be nice to somehow describe them if possible.

I'll see what I can do.

-- 
Måns Rullgård
mans@mansr.com