On Sat, Oct 31, 2015 at 8:41 PM, Måns Rullgård <m...@mansr.com> wrote:
> Andy Shevchenko <andy.shevche...@gmail.com> writes:
>
>>> +static int nb8800_mdio_read(struct mii_bus *bus, int phy_id, int reg)
>>> +{
>>> + struct nb8800_priv *priv = bus->priv;
>>> + int val;
>>> +
>>> + if (!nb8800_mdio_wait(bus))
>>> + return -ETIMEDOUT;
>>> +
>>> + val = MIIAR_ADDR(phy_id) | MIIAR_REG(reg);
>>> +
>>> + nb8800_writel(priv, NB8800_MDIO_CMD, val);
>>> + udelay(10);
>>
>> Why 10? Perhaps add a comment line.
>
> Because it works. The documentation doesn't mention a delay, but it
> works unreliably without one.
/* Put delay here, otherwise it works unreliably */
>
>>> + 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?
>>> +static int nb8800_poll(struct napi_struct *napi, int budget)
>>> +{
>>> + struct net_device *dev = napi->dev;
>>> + struct nb8800_priv *priv = netdev_priv(dev);
>>> + struct nb8800_dma_desc *rx;
>>> + int work = 0;
>>> + int last = priv->rx_eoc;
>>> + int next;
>>> +
>>> + while (work < budget) {
>>> + struct rx_buf *rx_buf;
>>> + u32 report;
>>> + int len;
>>> +
>>> + next = (last + 1) & (RX_DESC_COUNT - 1);
>>
>> Maybe (last + 1) % RX_DESC_COUNT ? It will not prevent to use
>> non-power-of-two numbers.
>
> We don't want to be doing divisions anyway, but I can certainly change
> it to % if that's preferred.
I'm pretty sure the result for power-of-two numbers will be the
similar (right shift).
>
>>> +
>>> + 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?
>
>>> +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.
>>> + 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.
>>> +static int nb8800_probe(struct platform_device *pdev)
>>> +{
>>> + const struct of_device_id *match;
>>> + const struct nb8800_ops *ops = NULL;
>>> + struct nb8800_priv *priv;
>>> + struct resource *res;
>>> + struct net_device *dev;
>>> + struct mii_bus *bus;
>>> + const unsigned char *mac;
>>> + void __iomem *base;
>>> + int irq;
>>> + int ret;
>>> +
>>> + match = of_match_device(nb8800_dt_ids, &pdev->dev);
>>> + if (match)
>>> + ops = match->data;
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + if (!res) {
>>> + dev_err(&pdev->dev, "No MMIO base\n");
>>> + return -EINVAL;
>>> + }
>>
>> Move platform_get_resource() just before devm_ioremap_resource() and
>> remove redundant condition with the message.
>>
>>> + bus = devm_mdiobus_alloc(&pdev->dev);
>>> + if (!bus) {
>>> + ret = -ENOMEM;
>>> + goto err_disable_clk;
>>> + }
>>> +
>>> + bus->name = "nb8800-mii";
>>> + bus->read = nb8800_mdio_read;
>>> + bus->write = nb8800_mdio_write;
>>> + bus->parent = &pdev->dev;
>>> + snprintf(bus->id, MII_BUS_ID_SIZE, "%.*s-mii", MII_BUS_ID_SIZE - 5,
>>> + pdev->name);
>>
>> You are not using any IDs here, why not just to strscpy(bus->id,
>> bus->name, MII_BUS_ID_SIZE); for now?
>
> I was under the impression the ID should be unique, and there are two of
> these devices in the chip.
But pdev->name is somehow different in the *first* part? Otherwise it
is error prone.
Better to provide bus id derived either from some unique number (let's
say IRQ line), or explicitly from DT.
--
With Best Regards,
Andy Shevchenko
--
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
