On Sat, Oct 31, 2015 at 8:41 PM, Måns Rullgård [off-list ref] wrote:

Andy Shevchenko [off-list ref] writes:

quoted

quoted

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

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?

quoted

quoted

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

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?

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.

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.

quoted

quoted

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

quoted

+       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