From: Shawn Guo <shawn.guo@linaro.org>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 2/3] net: add higmacv300 Ethernet driver for HiSilicon platform
Date: Thu, 31 Jan 2019 18:17:07 +0800 [thread overview]
Message-ID: <20190131101703.GA3789@dragon> (raw)
In-Reply-To: <CAModR+WqbuOrzmhEnWeJTd+ixvfKk273RRwo-rXXxQ1udRRgJA@mail.gmail.com>
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@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?
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
next prev parent reply other threads:[~2019-01-31 10:17 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-28 9:13 [U-Boot] [PATCH 0/3] Add Ethernet support for Poplar board Shawn Guo
2019-01-28 9:13 ` [U-Boot] [PATCH 1/3] reset: add reset driver for HiSilicon platform Shawn Guo
2019-01-29 21:57 ` Igor Opaniuk
2019-01-28 9:13 ` [U-Boot] [PATCH 2/3] net: add higmacv300 Ethernet " Shawn Guo
2019-01-29 21:50 ` Igor Opaniuk
2019-01-31 10:17 ` Shawn Guo [this message]
2019-02-04 23:53 ` Joe Hershberger
2019-02-13 11:46 ` Shawn Guo
2019-02-09 0:13 ` [U-Boot] [U-Boot, " Tom Rini
2019-02-13 7:21 ` Shawn Guo
2019-01-28 9:13 ` [U-Boot] [PATCH 3/3] poplar: enable Ethernet driver support Shawn Guo
2019-01-29 21:52 ` Igor Opaniuk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190131101703.GA3789@dragon \
--to=shawn.guo@linaro.org \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.