Andy Shevchenko <andy.shevche...@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
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
