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

["Måns Rullgård" <mans@mansr.com>]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

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

>>>> +
>>>> +               rx_buf = &priv->rx_bufs[next];
>>>> +               rx = &priv->rx_descs[next];
>>>
>>>> +               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.

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

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