[U-Boot] [PATCH 2/3] net: add higmacv300 Ethernet driver for HiSilicon platform

[Shawn Guo <shawn.guo@linaro.org>]

Hi Joe,

Thanks for spending time to review the patch.

On Mon, Feb 04, 2019 at 11:48:40PM +0000, Joe Hershberger wrote:
> On Mon, Jan 28, 2019 at 3:16 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > It adds the driver for HIGMACV300 Ethernet controller found on HiSilicon
> > SoCs like Hi3798CV200.  It's based on a downstream U-Boot driver, but
> > quite a lot of code gets rewritten and cleaned up to adopt driver model
> > and PHY API.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/net/Kconfig      |   9 +
> >  drivers/net/Makefile     |   1 +
> >  drivers/net/higmacv300.c | 592 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 602 insertions(+)
> >  create mode 100644 drivers/net/higmacv300.c

<snip>

> > +#define MDIO_WRITE                     1
> > +#define MDIO_READ                      2
> > +#define MDIO_COMMAND(rw, addr, reg)    (BIT_MDIO_BUSY | \
> > +                                       (((rw) & 0x3) << 16) | \
> > +                                       (((addr) & 0x1f) << 8) | \
> > +                                       ((reg) & 0x1f))
> > +#define MDIO_WRITE_CMD(addr, reg)      MDIO_COMMAND(MDIO_WRITE, addr, reg)
> > +#define MDIO_READ_CMD(addr, reg)       MDIO_COMMAND(MDIO_READ, addr, reg)
> 
> This is a strange construct... can you just inline this into the
> actual functions? I don't see the value in these macros that hide
> what's happening.

Yes, I can eliminate the macros with inline code.  That's actually what
kernel driver does.

> 
> > +
> > +enum higmac_queue {
> > +       RX_FQ,
> > +       RX_BQ,
> > +       TX_BQ,
> > +       TX_RQ,
> > +};

<snip>

> > +static int higmac_free_pkt(struct udevice *dev, uchar *packet, int length)
> > +{
> > +       if (packet)
> > +               free(packet);
> 
> Why free them and reallocate them? Won't that just fragment memory?

Practically it will not fragment the memory, as all the buffers get
a fixed size and allocation/free always happens as a pair for given
network transaction.  But I still think it's a good suggestion to get
all buffers allocated at initialization time, so that we do not need to
allocate/free buffers repeatedly.

> Also, you should be somehow telling the MAC that the buffer /
> descriptor is no longer in use here.

Sensible suggestion.

> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int higmac_recv(struct udevice *dev, int flags, uchar **packetp)
> > +{
> > +       struct higmac_priv *priv = dev_get_priv(dev);
> > +       struct higmac_desc *fqd = priv->rxfq;
> > +       struct higmac_desc *bqd = priv->rxbq;
> > +       int fqw_pos, fqr_pos, bqw_pos, bqr_pos;
> > +       int timeout = 100000;
> > +       unsigned long addr;
> > +       int len = 0;
> > +       int space;
> > +       int i;
> > +
> > +       fqw_pos = DESC_CNT(readl(priv->base + RX_FQ_WR_ADDR));
> > +       fqr_pos = DESC_CNT(readl(priv->base + RX_FQ_RD_ADDR));
> > +
> > +       if (fqw_pos >= fqr_pos)
> > +               space = RX_DESC_NUM - (fqw_pos - fqr_pos);
> > +       else
> > +               space = fqr_pos - fqw_pos;
> > +
> > +       /* Leave one free to distinguish full filled from empty buffer */
> > +       for (i = 0; i < space - 1; i++) {
> > +               void *buf = memalign(ARCH_DMA_MINALIGN, MAC_MAX_FRAME_SIZE);
> > +
> > +               if (!buf)
> > +                       break;
> > +
> > +               fqd = priv->rxfq + fqw_pos;
> > +               fqd->buf_addr = (unsigned long)buf;
> > +               invalidate_dcache_range(fqd->buf_addr,
> > +                                       fqd->buf_addr + MAC_MAX_FRAME_SIZE);
> > +
> > +               fqd->descvid = DESC_VLD_FREE;
> > +               fqd->buf_len = MAC_MAX_FRAME_SIZE - 1;
> > +               flush_desc(fqd);
> > +
> > +               if (++fqw_pos >= RX_DESC_NUM)
> > +                       fqw_pos = 0;
> > +
> > +               writel(DESC_BYTE(fqw_pos), priv->base + RX_FQ_WR_ADDR);
> > +       }
> > +
> > +       bqr_pos = DESC_CNT(readl(priv->base + RX_BQ_RD_ADDR));
> > +       bqd += bqr_pos;
> > +       /* BQ is only ever written by GMAC */
> > +       invalidate_desc(bqd);
> > +
> > +       do {
> > +               bqw_pos = DESC_CNT(readl(priv->base + RX_BQ_WR_ADDR));
> > +               udelay(1);
> > +       } while (--timeout && bqw_pos == bqr_pos);
> > +
> > +       if (!timeout)
> > +               return -ETIMEDOUT;
> > +
> > +       if (++bqr_pos >= RX_DESC_NUM)
> > +               bqr_pos = 0;
> > +
> > +       len = bqd->data_len;
> > +
> > +       /* CPU should not have touched this buffer since we added it to FQ */
> > +       addr = bqd->buf_addr;
> > +       invalidate_dcache_range(addr, addr + len);
> > +       *packetp = (void *)addr;
> > +
> > +       writel(DESC_BYTE(bqr_pos), priv->base + RX_BQ_RD_ADDR);
> 
> Does this belong in free_pkt()?

Yeah, I guess that's what you mean by telling MAC the descriptors are
not longer used above.

> > +
> > +       return len;
> > +}

<snip>

> > +static int higmac_start(struct udevice *dev)
> > +{
> > +       struct higmac_priv *priv = dev_get_priv(dev);
> > +       struct phy_device *phydev = priv->phydev;
> > +       int ret;
> > +
> > +       ret = phy_startup(phydev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (!phydev->link) {
> > +               debug("%s: link down\n", phydev->dev->name);
> > +               return -ENODEV;
> > +       }
> > +
> > +       ret = higmac_adjust_link(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Enable port */
> > +       writel(0xf, priv->base + DESC_WR_RD_ENA);
> 
> What is 0xf? No magic numbers please.

Okay, will define it.

> 
> > +       writel(BIT_TX_EN | BIT_RX_EN, priv->base + PORT_EN);
> > +
> > +       return 0;
> > +}
> > +
> > +static void higmac_stop(struct udevice *dev)
> > +{
> > +       struct higmac_priv *priv = dev_get_priv(dev);
> > +
> > +       /* Disable port */
> > +       writel(0, priv->base + PORT_EN);
> > +       writel(0, priv->base + DESC_WR_RD_ENA);
> > +}
> > +
> > +static const struct eth_ops higmac_ops = {
> > +       .start          = higmac_start,
> > +       .send           = higmac_send,
> > +       .recv           = higmac_recv,
> > +       .free_pkt       = higmac_free_pkt,
> > +       .stop           = higmac_stop,
> > +       .write_hwaddr   = higmac_write_hwaddr,
> > +};
> > +
> > +static int higmac_wait_mdio_ready(struct higmac_priv *priv)
> > +{
> > +       int timeout = 1000;
> > +
> > +       while (--timeout) {
> > +               /* Wait until busy bit is cleared */
> > +               if ((readl(priv->base + MDIO_SINGLE_CMD) & BIT_MDIO_BUSY) == 0)
> > +                       break;
> > +               udelay(10);
> > +       }
> 
> It would be good if you retrofit to use wait_bit or its macros
> throughout this driver instead of hand-writing it.

I didn't know that before.  Thanks for the pointer.  Will do.

> 
> > +
> > +       return timeout ? 0 : -ETIMEDOUT;
> > +}

<snip>

> > +static int higmac_probe(struct udevice *dev)
> > +{
> > +       struct higmac_priv *priv = dev_get_priv(dev);
> > +       struct phy_device *phydev;
> > +       struct mii_dev *bus;
> > +       int ret;
> > +
> > +       ret = higmac_hw_init(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       bus = mdio_alloc();
> > +       if (!bus)
> > +               return -ENOMEM;
> > +
> > +       bus->read = higmac_mdio_read;
> > +       bus->write = higmac_mdio_write;
> > +       bus->priv = priv;
> > +       priv->bus = bus;
> > +
> > +       ret = mdio_register_seq(bus, dev->seq);
> > +       if (ret)
> > +               return ret;
> > +
> > +       phydev = phy_connect(bus, priv->phyaddr, dev, priv->phyintf);
> > +       if (!phydev)
> > +               return -ENODEV;
> > +
> > +       phydev->supported &= PHY_GBIT_FEATURES;
> 
> I would expect either phydev->supported &= ~PHY_GBIT_FEATURES; or
> phydev->supported |= PHY_GBIT_FEATURES;

To be honest, this code was copied from other drivers.  It's all over
the drivers under driver/net.  The phydev->supported gets initialized as
phydev->drv->features, and we want to make sure feature bits are set on
both sides, no?

Shawn

> 
> > +       phydev->advertising = phydev->supported;
> > +       priv->phydev = phydev;
> > +
> > +       ret = phy_config(phydev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}