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....@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....@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;
> > +}
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
