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:

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

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

>> +
>> +       return val & 0xffff;
>> +}

[...]

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

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

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

>> +static void nb8800_set_rx_mode(struct net_device *dev)
>> +{
>> +       struct nb8800_priv *priv = netdev_priv(dev);
>> +       struct netdev_hw_addr *ha;
>> +       bool af_en;
>> +       int i;
>> +
>> +       if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI))
>> +               af_en = false;
>> +       else
>> +               af_en = true;
>> +
>> +       nb8800_mac_af(dev, af_en);
>> +
>> +       if (!af_en)
>> +               return;
>
> Would it be
>
> if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
>   nb8800_mac_af(dev, false);
>   return;
> }
>
> nb8800_mac_af(dev, true);
>
> ?

Looks equivalent.  Maybe that's clearer.

>> +static int nb8800_hw_init(struct net_device *dev)
>> +{
>> +       struct nb8800_priv *priv = netdev_priv(dev);
>
>> +       unsigned int val = 0;
>
> Useless assignment.

Indeed.  Why didn't gcc warn about that?

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

>> +
>> +       /* configure TX DMA Channels */
>> +       val = nb8800_readl(priv, NB8800_TXC_CR);
>> +       val &= TCR_LE;
>> +       val |= TCR_DM | TCR_RS | TCR_TFI(1) | TCR_BTS(2);
>> +       nb8800_writel(priv, NB8800_TXC_CR, val);
>> +
>> +       /* TX Interrupt Time Register */
>> +       nb8800_writel(priv, NB8800_TX_ITR, 1);
>> +
>> +       /* configure RX DMA Channels */
>> +       val = nb8800_readl(priv, NB8800_RXC_CR);
>> +       val &= RCR_LE;
>> +       val |= RCR_DM | RCR_RS | RCR_RFI(7) | RCR_BTS(2) | RCR_FL;
>> +       nb8800_writel(priv, NB8800_RXC_CR, val);
>> +
>> +       /* RX Interrupt Time Register */
>> +       nb8800_writel(priv, NB8800_RX_ITR, 1);
>> +
>> +       val = TX_RETRY_EN | TX_PAD_EN | TX_APPEND_FCS;
>> +       nb8800_writeb(priv, NB8800_TX_CTL1, val);
>> +
>> +       /* collision retry count */
>> +       nb8800_writeb(priv, NB8800_TX_CTL2, 5);
>> +
>> +       val = RX_PAD_STRIP | RX_PAUSE_EN | RX_AF_EN | RX_RUNT;
>> +       nb8800_writeb(priv, NB8800_RX_CTL, val);
>> +
>> +       nb8800_mc_init(dev, 0);
>> +
>> +       nb8800_writeb(priv, NB8800_TX_BUFSIZE, 0xff);
>> +
>> +       return 0;
>> +}
>> +
>> +static void nb8800_tangox_init(struct net_device *dev)
>> +{
>> +       struct nb8800_priv *priv = netdev_priv(dev);
>> +       u32 val;
>> +
>> +       val = nb8800_readb(priv, NB8800_TANGOX_PAD_MODE) & 0x78;
>
> Magic 0x78.

That one I can clarify.  Will do.

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

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