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