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);elsespace = 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;+}