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

Shawn Guo shawn.guo at linaro.org
Thu Jan 31 10:17:07 UTC 2019


Hi Igor,

On Tue, Jan 29, 2019 at 11:50:33PM +0200, Igor Opaniuk wrote:
> Hi Shawn,
> 
> Please see inline comments (mostly minor stuff).
> I'll also test the driver a bit later and leave my T-b.

Thanks.  I appreciate the effort of testing.

> 
> On Mon, 28 Jan 2019 at 11:15, Shawn Guo <shawn.guo at 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 at 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?

Suggestion taken.  I would move one step further to rename it
HIGMACV300_ETH to be more consistent with Ethernet driver naming
convention.

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

I would rather organize them in the order of offset, which should be the
same to what hardware manual does.

> Suggestion: move all of them to a separate header file.

I do not see much point of doing that, which only makes searching of the
definitions more difficult.

> 
> > +
> > +/* 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?

IMO, the current definition from vendor code is more intuitive to
reflect the fact, that the hardware descriptor consists of 8 32-bit
words.  That said, I would like to use the same type for all fields of
this structure.  I will change them all to use unsigned long though.

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

Hmm, do you see any problem without this attribute?  None of the
Ethernet driver in the tree has this, and I do not want higamc drive to
be special.

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

The implication here is that the buffer has to be in lower 4GB address
space, which is true for HiSilicon platform.

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

You are right.

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

Will do.

> 
> > +}
> > +
> > +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?

Okay, will do.

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

Okay.

Thanks for the review.

Shawn

> 
> > +}
> > +
> > +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
> >
> 
> 
> 
> -- 
> Regards,
> Igor Opaniuk


More information about the U-Boot mailing list