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

Hi Shawn,

Please see inline comments (mostly minor stuff). I'll also test the driver a bit later and leave my T-b.

On Mon, 28 Jan 2019 at 11:15, 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

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 39ce4e8a1fc6..e07a382c28c7 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -530,4 +530,13 @@ config MEDIATEK_ETH This Driver support MediaTek Ethernet GMAC Say Y to enable support for the MediaTek Ethernet GMAC.

+config NET_HIGMACV300

•   bool "HiSilicon Gigabit Ethernet Controller"
  

•   depends on DM_ETH
  

•   select DM_RESET
  

•   select PHYLIB
  

•   help
  

•     This driver supports HIGMACV300 Ethernet controller found on
  

•     HiSilicon SoCs.
  

endif # NETDEVICES diff --git a/drivers/net/Makefile b/drivers/net/Makefile index e38c16464478..0951cb17e6ba 100644 --- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -76,3 +76,4 @@ obj-$(CONFIG_SNI_AVE) += sni_ave.o obj-y += ti/ obj-$(CONFIG_MEDIATEK_ETH) += mtk_eth.o obj-$(CONFIG_MSCC_OCELOT_SWITCH) += ocelot_switch.o +obj-$(CONFIG_NET_HIGMACV300) += higmacv300.o

suggestion: CONFIG_HIGMACV300_NET, similar to CONFIG_MEDIATEK_ETH above?

diff --git a/drivers/net/higmacv300.c b/drivers/net/higmacv300.c new file mode 100644 index 000000000000..6cac5cec73df --- /dev/null +++ b/drivers/net/higmacv300.c @@ -0,0 +1,592 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2019, Linaro Limited
• */

+#include <asm/io.h> +#include <common.h> +#include <console.h> +#include <linux/bug.h> +#include <linux/mii.h> +#include <miiphy.h> +#include <net.h> +#include <reset.h>

+#define STATION_ADDR_LOW 0x0000 +#define STATION_ADDR_HIGH 0x0004 +#define MAC_DUPLEX_HALF_CTRL 0x0008 +#define PORT_MODE 0x0040 +#define PORT_EN 0x0044 +#define BIT_TX_EN BIT(2) +#define BIT_RX_EN BIT(1) +#define MODE_CHANGE_EN 0x01b4 +#define BIT_MODE_CHANGE_EN BIT(0) +#define MDIO_SINGLE_CMD 0x03c0 +#define BIT_MDIO_BUSY BIT(20) +#define MDIO_SINGLE_DATA 0x03c4 +#define MDIO_RDATA_STATUS 0x03d0 +#define BIT_MDIO_RDATA_INVALID BIT(0) +#define RX_FQ_START_ADDR 0x0500 +#define RX_FQ_DEPTH 0x0504 +#define RX_FQ_WR_ADDR 0x0508 +#define RX_FQ_RD_ADDR 0x050c +#define RX_FQ_REG_EN 0x0518 +#define RX_BQ_START_ADDR 0x0520 +#define RX_BQ_DEPTH 0x0524 +#define RX_BQ_WR_ADDR 0x0528 +#define RX_BQ_RD_ADDR 0x052c +#define RX_BQ_REG_EN 0x0538 +#define TX_BQ_START_ADDR 0x0580 +#define TX_BQ_DEPTH 0x0584 +#define TX_BQ_WR_ADDR 0x0588 +#define TX_BQ_RD_ADDR 0x058c +#define TX_BQ_REG_EN 0x0598 +#define TX_RQ_START_ADDR 0x05a0 +#define TX_RQ_DEPTH 0x05a4 +#define TX_RQ_WR_ADDR 0x05a8 +#define TX_RQ_RD_ADDR 0x05ac +#define TX_RQ_REG_EN 0x05b8 +#define BIT_START_ADDR_EN BIT(2) +#define BIT_DEPTH_EN BIT(1) +#define DESC_WR_RD_ENA 0x05cc

Please group above defines into categories. Suggestion: move all of them to a separate header file.

+/* MACIF_CTRL */ +#define RGMII_SPEED_1000 0x2c +#define RGMII_SPEED_100 0x2f +#define RGMII_SPEED_10 0x2d +#define MII_SPEED_100 0x0f +#define MII_SPEED_10 0x0d +#define GMAC_SPEED_1000 0x05 +#define GMAC_SPEED_100 0x01 +#define GMAC_SPEED_10 0x00 +#define GMAC_FULL_DUPLEX BIT(4)

+#define RX_DESC_NUM 64 +#define TX_DESC_NUM 2 +#define DESC_SIZE 32 +#define DESC_WORD_SHIFT 3 +#define DESC_BYTE_SHIFT 5 +#define DESC_CNT(n) ((n) >> DESC_BYTE_SHIFT) +#define DESC_BYTE(n) ((n) << DESC_BYTE_SHIFT) +#define DESC_VLD_FREE 0 +#define DESC_VLD_BUSY 1

+#define MAC_MAX_FRAME_SIZE 1600

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

+enum higmac_queue {

•   RX_FQ,
  

•   RX_BQ,
  

•   TX_BQ,
  

•   TX_RQ,
  

+};

+struct higmac_desc {

•   unsigned int buf_addr;
  

uintptr_t?

•   unsigned int buf_len:11;
  

•   unsigned int reserve0:5;
  

•   unsigned int data_len:11;
  

•   unsigned int reserve1:2;
  

•   unsigned int fl:2;
  

•   unsigned int descvid:1;
  

•   unsigned int reserve2[6];
  

+};

+struct higmac_priv {

•   void __iomem *base;
  

•   void __iomem *macif_ctrl;
  

•   struct reset_ctl rst_phy;
  

•   struct higmac_desc *rxfq;
  

•   struct higmac_desc *rxbq;
  

•   struct higmac_desc *txbq;
  

•   struct higmac_desc *txrq;
  

•   struct mii_dev *bus;
  

•   struct phy_device *phydev;
  

•   int phyintf;
  

•   int phyaddr;
  

+};

+#define flush_desc(d) flush_cache((unsigned long)(d), sizeof(*(d))) +#define invalidate_desc(d) \

•   invalidate_dcache_range((unsigned long)(d), \
  

•                           (unsigned long)(d) + sizeof(*(d)))
  

+static int higmac_write_hwaddr(struct udevice *dev) +{

•   struct eth_pdata *pdata = dev_get_platdata(dev);
  

•   struct higmac_priv *priv = dev_get_priv(dev);
  

•   unsigned char *mac = pdata->enetaddr;
  

•   u32 val;
  

•   val = mac[1] | (mac[0] << 8);
  

•   writel(val, priv->base + STATION_ADDR_HIGH);
  

•   val = mac[5] | (mac[4] << 8) | (mac[3] << 16) | (mac[2] << 24);
  

•   writel(val, priv->base + STATION_ADDR_LOW);
  

•   return 0;
  

+}

+static int higmac_free_pkt(struct udevice *dev, uchar *packet, int length)

__always_unused for length and dev

+{

•   if (packet)
  

•           free(packet);
  

•   return 0;
  

+}

+static int higmac_recv(struct udevice *dev, int flags, uchar **packetp)

__always_unused for flags

+{

•   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;
  

buf_addr is unsigned int, possible value overflow.

•           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;
  

Not sure why we need to use one more var (addr) here, why not to keep using bqd->buf_addr ?

•   invalidate_dcache_range(addr, addr + len);
  

•   *packetp = (void *)addr;
  

•   writel(DESC_BYTE(bqr_pos), priv->base + RX_BQ_RD_ADDR);
  

•   return len;
  

+}

+static int higmac_send(struct udevice *dev, void *packet, int length) +{

•   struct higmac_priv *priv = dev_get_priv(dev);
  

•   struct higmac_desc *bqd = priv->txbq;
  

•   int bqw_pos, rqw_pos, rqr_pos;
  

•   int timeout = 1000;
  

•   flush_cache((unsigned long)packet, length);
  

•   bqw_pos = DESC_CNT(readl(priv->base + TX_BQ_WR_ADDR));
  

•   bqd += bqw_pos;
  

•   bqd->buf_addr = (unsigned long)packet;
  

•   bqd->descvid = DESC_VLD_BUSY;
  

•   bqd->data_len = length;
  

•   flush_desc(bqd);
  

•   if (++bqw_pos >= TX_DESC_NUM)
  

•           bqw_pos = 0;
  

•   writel(DESC_BYTE(bqw_pos), priv->base + TX_BQ_WR_ADDR);
  

•   rqr_pos = DESC_CNT(readl(priv->base + TX_RQ_RD_ADDR));
  

•   if (++rqr_pos >= TX_DESC_NUM)
  

•           rqr_pos = 0;
  

•   do {
  

•           rqw_pos = DESC_CNT(readl(priv->base + TX_RQ_WR_ADDR));
  

•           udelay(1);
  

•   } while (--timeout && rqr_pos != rqw_pos);
  

•   if (!timeout)
  

•           return -ETIMEDOUT;
  

•   writel(DESC_BYTE(rqr_pos), priv->base + TX_RQ_RD_ADDR);
  

•   return 0;
  

+}

+static int higmac_adjust_link(struct higmac_priv *priv) +{

•   struct phy_device *phydev = priv->phydev;
  

•   int interface = priv->phyintf;
  

•   u32 val;
  

•   switch (interface) {
  

•   case PHY_INTERFACE_MODE_RGMII:
  

•           if (phydev->speed == SPEED_1000)
  

•                   val = RGMII_SPEED_1000;
  

•           else if (phydev->speed == SPEED_100)
  

•                   val = RGMII_SPEED_100;
  

•           else
  

•                   val = RGMII_SPEED_10;
  

•           break;
  

•   case PHY_INTERFACE_MODE_MII:
  

•           if (phydev->speed == SPEED_100)
  

•                   val = MII_SPEED_100;
  

•           else
  

•                   val = MII_SPEED_10;
  

•           break;
  

•   default:
  

•           debug("unsupported mode: %d\n", interface);
  

•           return -EINVAL;
  

•   }
  

•   if (phydev->duplex)
  

•           val |= GMAC_FULL_DUPLEX;
  

•   writel(val, priv->macif_ctrl);
  

•   if (phydev->speed == SPEED_1000)
  

•           val = GMAC_SPEED_1000;
  

•   else if (phydev->speed == SPEED_100)
  

•           val = GMAC_SPEED_100;
  

•   else
  

•           val = GMAC_SPEED_10;
  

•   writel(BIT_MODE_CHANGE_EN, priv->base + MODE_CHANGE_EN);
  

•   writel(val, priv->base + PORT_MODE);
  

•   writel(0, priv->base + MODE_CHANGE_EN);
  

•   writel(phydev->duplex, priv->base + MAC_DUPLEX_HALF_CTRL);
  

•   return 0;
  

+}

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

•   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);
  

•   }
  

•   return timeout ? 0 : -ETIMEDOUT;
  

+}

+static int higmac_mdio_read(struct mii_dev *bus, int addr, int devad, int reg) +{

•   struct higmac_priv *priv = bus->priv;
  

•   int ret;
  

•   ret = higmac_wait_mdio_ready(priv);
  

•   if (ret)
  

•           return ret;
  

•   writel(MDIO_READ_CMD(addr, reg), priv->base + MDIO_SINGLE_CMD);
  

•   ret = higmac_wait_mdio_ready(priv);
  

•   if (ret)
  

•           return ret;
  

•   if (readl(priv->base + MDIO_RDATA_STATUS) & BIT_MDIO_RDATA_INVALID)
  

•           return -EINVAL;
  

•   return readl(priv->base + MDIO_SINGLE_DATA) >> 16;
  

+}

+static int higmac_mdio_write(struct mii_dev *bus, int addr, int devad,

•                        int reg, u16 value)
  

+{

•   struct higmac_priv *priv = bus->priv;
  

•   int ret;
  

•   ret = higmac_wait_mdio_ready(priv);
  

•   if (ret)
  

•           return ret;
  

•   writel(value, priv->base + MDIO_SINGLE_DATA);
  

•   writel(MDIO_WRITE_CMD(addr, reg), priv->base + MDIO_SINGLE_CMD);
  

•   return 0;
  

+}

+static int higmac_init_hw_queue(struct higmac_priv *priv,

•                           enum higmac_queue queue)
  

+{

•   struct higmac_desc *desc, **pdesc;
  

•   u32 regaddr, regen, regdep;
  

•   int depth;
  

•   int len;
  

•   switch (queue) {
  

•   case RX_FQ:
  

•           regaddr = RX_FQ_START_ADDR;
  

•           regen = RX_FQ_REG_EN;
  

•           regdep = RX_FQ_DEPTH;
  

•           depth = RX_DESC_NUM;
  

•           pdesc = &priv->rxfq;
  

•           break;
  

•   case RX_BQ:
  

•           regaddr = RX_BQ_START_ADDR;
  

•           regen = RX_BQ_REG_EN;
  

•           regdep = RX_BQ_DEPTH;
  

•           depth = RX_DESC_NUM;
  

•           pdesc = &priv->rxbq;
  

•           break;
  

•   case TX_BQ:
  

•           regaddr = TX_BQ_START_ADDR;
  

•           regen = TX_BQ_REG_EN;
  

•           regdep = TX_BQ_DEPTH;
  

•           depth = TX_DESC_NUM;
  

•           pdesc = &priv->txbq;
  

•           break;
  

•   case TX_RQ:
  

•           regaddr = TX_RQ_START_ADDR;
  

•           regen = TX_RQ_REG_EN;
  

•           regdep = TX_RQ_DEPTH;
  

•           depth = TX_DESC_NUM;
  

•           pdesc = &priv->txrq;
  

•           break;
  

•   }
  

•   /* Enable depth */
  

•   writel(BIT_DEPTH_EN, priv->base + regen);
  

•   writel(depth << DESC_WORD_SHIFT, priv->base + regdep);
  

•   writel(0, priv->base + regen);
  

•   len = depth * sizeof(*desc);
  

•   desc = memalign(ARCH_DMA_MINALIGN, len);
  

•   if (!desc)
  

•           return -ENOMEM;
  

•   memset(desc, 0, len);
  

•   flush_cache((unsigned long)desc, len);
  

•   *pdesc = desc;
  

•   /* Enable start address */
  

•   writel(BIT_START_ADDR_EN, priv->base + regen);
  

•   writel((unsigned long)desc, priv->base + regaddr);
  

•   writel(0, priv->base + regen);
  

•   return 0;
  

+}

+static int higmac_hw_init(struct higmac_priv *priv) +{

•   int ret;
  

•   /* Initialize hardware queues */
  

•   ret = higmac_init_hw_queue(priv, RX_FQ);
  

•   if (ret)
  

•           return ret;
  

•   ret = higmac_init_hw_queue(priv, RX_BQ);
  

•   if (ret)
  

•           goto free_rx_fq;
  

•   ret = higmac_init_hw_queue(priv, TX_BQ);
  

•   if (ret)
  

•           goto free_rx_bq;
  

•   ret = higmac_init_hw_queue(priv, TX_RQ);
  

•   if (ret)
  

•           goto free_tx_bq;
  

•   /* Reset phy */
  

•   reset_assert(&priv->rst_phy);
  

•   mdelay(10);
  

•   reset_deassert(&priv->rst_phy);
  

•   mdelay(30);
  

•   reset_assert(&priv->rst_phy);
  

•   mdelay(30);
  

•   return 0;
  

+free_tx_bq:

•   free(priv->txbq);
  

+free_rx_bq:

•   free(priv->rxbq);
  

+free_rx_fq:

•   free(priv->rxfq);
  

•   return ret;
  

+}

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

•   phydev->advertising = phydev->supported;
  

•   priv->phydev = phydev;
  

•   ret = phy_config(phydev);
  

•   if (ret)
  

•           return ret;
  

•   return 0;
  

why not simply return ret? and remove the check above?

+}

+static int higmac_remove(struct udevice *dev) +{

•   struct higmac_priv *priv = dev_get_priv(dev);
  

•   mdio_unregister(priv->bus);
  

•   mdio_free(priv->bus);
  

•   return 0;
  

+}

+static int higmac_ofdata_to_platdata(struct udevice *dev) +{

•   struct higmac_priv *priv = dev_get_priv(dev);
  

•   ofnode phy_node;
  

•   const char *phy_mode;
  

•   int phyintf;
  

initialize to PHY_INTERFACE_MODE_NONE here?

•   int ret;
  

•   priv->base = dev_remap_addr_index(dev, 0);
  

•   priv->macif_ctrl = dev_remap_addr_index(dev, 1);
  

•   phyintf = PHY_INTERFACE_MODE_NONE;
  

•   phy_mode = dev_read_string(dev, "phy-mode");
  

•   if (phy_mode)
  

•           phyintf = phy_get_interface_by_name(phy_mode);
  

•   if (phyintf == PHY_INTERFACE_MODE_NONE)
  

•           return -ENODEV;
  

•   priv->phyintf = phyintf;
  

•   phy_node = dev_read_subnode(dev, "phy");
  

•   if (!ofnode_valid(phy_node)) {
  

•           debug("failed to find phy node\n");
  

•           return -ENODEV;
  

•   }
  

•   priv->phyaddr = ofnode_read_u32_default(phy_node, "reg", 0);
  

•   ret = reset_get_by_name(dev, "phy", &priv->rst_phy);
  

•   if (ret)
  

•           return ret;
  

•   return 0;
  

same here with return ret.

+}

+static const struct udevice_id higmac_ids[] = {

•   { .compatible = "hisilicon,hi3798cv200-gmac" },
  

•   { }
  

+};

+U_BOOT_DRIVER(eth_higmac) = {

•   .name   = "eth_higmac",
  

•   .id     = UCLASS_ETH,
  

•   .of_match = higmac_ids,
  

•   .ofdata_to_platdata = higmac_ofdata_to_platdata,
  

•   .probe  = higmac_probe,
  

•   .remove = higmac_remove,
  

•   .ops    = &higmac_ops,
  

•   .priv_auto_alloc_size = sizeof(struct higmac_priv),
  

•   .platdata_auto_alloc_size = sizeof(struct eth_pdata),
  

+};

2.18.0