[U-Boot] [PATCH 2/4] net: Add driver for Zynq Gem IP

Michal Simek
2012-08-14T17:04:09Z

Hi Joe,

On 08/14/2012 04:59 PM, Joe Hershberger wrote:
> Hi Michal,
>
> On Tue, Aug 14, 2012 at 6:42 AM, Michal Simek <monstr@monstr.eu> wrote:
>> Device driver for Zynq Gem IP.
>>
>> Signed-off-by: Michal Simek <monstr@monstr.eu>
>> CC: Joe Hershberger <joe.hershberger@gmail.com>
>> ---
>> drivers/net/Makefile | 1 +
>> drivers/net/xilinx_gem.c | 514 ++++++++++++++++++++++++++++++++++++++++++++++
>> include/netdev.h | 1 +
>> 3 files changed, 516 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/net/xilinx_gem.c
>
> Call this zynq_gem.c My understanding is this is only available in
> the hard silicon for Zynq.

That's correct understanding.

>
>>
>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> index 430f90c..e510ffa 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -77,6 +77,7 @@ COBJS-$(CONFIG_ULI526X) += uli526x.o
>> COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
>> COBJS-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o
>> COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
>> +COBJS-$(CONFIG_XILINX_GEM) += xilinx_gem.o
>> COBJS-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o xilinx_ll_temac_mdio.o \
>> xilinx_ll_temac_fifo.o xilinx_ll_temac_sdma.o
>>
>> diff --git a/drivers/net/xilinx_gem.c b/drivers/net/xilinx_gem.c
>> new file mode 100644
>> index 0000000..7c8c14d
>> --- /dev/null
>> +++ b/drivers/net/xilinx_gem.c
>> @@ -0,0 +1,514 @@
>> +/*
>> + * (C) Copyright 2011 Michal Simek
>> + *
>> + * Michal SIMEK <monstr@monstr.eu>
>> + *
>> + * Based on Xilinx gmac driver:
>> + * (C) Copyright 2011 Xilinx
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#include <common.h>
>> +#include <net.h>
>> +#include <config.h>
>> +#include <malloc.h>
>> +#include <asm/io.h>
>> +#include <phy.h>
>> +#include <miiphy.h>
>> +
>> +#if !defined(CONFIG_PHYLIB)
>> +# error XILINX_GEM_ETHERNET requires PHYLIB
>> +#endif
>> +
>> +/* Bit/mask specification */
>> +#define XEMACPSS_PHYMNTNC_OP_MASK 0x40020000 /* operation mask bits */
>> +#define XEMACPSS_PHYMNTNC_OP_R_MASK 0x20000000 /* read operation */
>> +#define XEMACPSS_PHYMNTNC_OP_W_MASK 0x10000000 /* write operation */
>> +#define XEMACPSS_PHYMNTNC_PHYAD_SHIFT_MASK 23 /* Shift bits for PHYAD */
>> +#define XEMACPSS_PHYMNTNC_PHREG_SHIFT_MASK 18 /* Shift bits for PHREG */
>
> Replace all "XEMACPSS" with "ZYNQ_GEM"

ok

>
>> +
>> +#define XEMACPSS_RXBUF_EOF_MASK 0x00008000 /* End of frame. */
>> +#define XEMACPSS_RXBUF_SOF_MASK 0x00004000 /* Start of frame. */
>> +#define XEMACPSS_RXBUF_LEN_MASK 0x00003FFF /* Mask for length field */
>> +
>> +#define XEMACPSS_RXBUF_WRAP_MASK 0x00000002 /* Wrap bit, last BD */
>> +#define XEMACPSS_RXBUF_NEW_MASK 0x00000001 /* Used bit.. */
>> +#define XEMACPSS_RXBUF_ADD_MASK 0xFFFFFFFC /* Mask for address */
>> +
>> +/* Wrap bit, last descriptor */
>> +#define XEMACPSS_TXBUF_WRAP_MASK 0x40000000
>> +#define XEMACPSS_TXBUF_LAST_MASK 0x00008000 /* Last buffer */
>> +
>> +#define XEMACPSS_TXSR_HRESPNOK_MASK 0x00000100 /* Transmit hresp not OK */
>> +#define XEMACPSS_TXSR_URUN_MASK 0x00000040 /* Transmit underrun */
>> +/* Transmit buffs exhausted mid frame */
>> +#define XEMACPSS_TXSR_BUFEXH_MASK 0x00000010
>> +
>> +#define XEMACPSS_NWCTRL_TXEN_MASK 0x00000008 /* Enable transmit */
>> +#define XEMACPSS_NWCTRL_RXEN_MASK 0x00000004 /* Enable receive */
>> +#define XEMACPSS_NWCTRL_MDEN_MASK 0x00000010 /* Enable MDIO port */
>> +#define XEMACPSS_NWCTRL_STARTTX_MASK 0x00000200 /* Start tx (tx_go) */
>> +
>> +#define XEMACPSS_NWSR_MDIOIDLE_MASK 0x00000004 /* PHY management idle */
>> +
>> +/* Use MII register 1 (MII status register) to detect PHY */
>> +#define PHY_DETECT_REG 1
>> +
>> +/* Mask used to verify certain PHY features (or register contents)
>> + * in the register above:
>> + * 0x1000: 10Mbps full duplex support
>> + * 0x0800: 10Mbps half duplex support
>> + * 0x0008: Auto-negotiation support
>> + */
>> +#define PHY_DETECT_MASK 0x1808
>> +
>> +/* Device registers */
>> +struct gem_regs {
>> + u32 nwctrl; /* Network Control reg */
>> + u32 nwcfg; /* Network Config reg */
>> + u32 nwsr; /* Network Status reg */
>> + u32 reserved1;
>> + u32 dmacr; /* DMA Control reg */
>> + u32 txsr; /* TX Status reg */
>> + u32 rxqbase; /* RX Q Base address reg */
>> + u32 txqbase; /* TX Q Base address reg */
>> + u32 rxsr; /* RX Status reg */
>> + u32 reserved2[2];
>> + u32 idr; /* Interrupt Disable reg */
>> + u32 reserved3;
>> + u32 phymntnc; /* Phy Maintaince reg */
>> + u32 reserved4[18];
>> + u32 hashl; /* Hash Low address reg */
>> + u32 hashh; /* Hash High address reg */
>> +#define LADDR_LOW 0
>> +#define LADDR_HIGH 1
>> + u32 laddr[4][LADDR_HIGH + 1]; /* Specific1 addr low/high reg */
>> + u32 match[4]; /* Type ID1 Match reg */
>> + u32 reserved6[18];
>> + u32 stat[44]; /* Octects transmitted Low reg - stat start */
>> +};
>> +
>> +/* BD descriptors */
>> +struct emac_bd {
>> + u32 addr; /* Next descriptor pointer */
>> + u32 status;
>> +};
>> +
>> +#define RX_BUF 3
>> +
>> +/* Initialized, rxbd_current, rx_first_buf must be 0 after init */
>> +struct gemac_priv {
>> + struct emac_bd tx_bd;
>> + struct emac_bd rx_bd[RX_BUF];
>> + u32 initialized;
>> + char rxbuffers[RX_BUF * PKTSIZE_ALIGN];
>> + u32 rxbd_current;
>> + u32 rx_first_buf;
>> + int phyaddr;
>> + struct phy_device *phydev;
>> + struct mii_dev *bus;
>> +};
>> +
>> +static inline int mdio_wait(struct eth_device *dev)
>> +{
>> + struct gem_regs *regs = (struct gem_regs *)dev->iobase;
>> + u32 timeout = 200;
>> +
>> + /* Wait till MDIO interface is ready to accept a new transaction. */
>> + while (timeout && !(readl(&regs->nwsr) & XEMACPSS_NWSR_MDIOIDLE_MASK)) {
>> + timeout--;
>> + udelay(1);
>> + }
>> +
>> + if (!timeout) {
>> + printf("%s: Timeout\n", __func__);
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static u32 phy_setup_op(struct eth_device *dev, u32 phy_addr, u32 regnum,
>> + u32 op, u16 *data)
>> +{
>> + u32 mgtcr;
>> + struct gem_regs *regs = (struct gem_regs *)dev->iobase;
>> +
>> + if (mdio_wait(dev))
>> + return 1;
>> +
>> + /* Construct mgtcr mask for the operation */
>> + mgtcr = XEMACPSS_PHYMNTNC_OP_MASK | op |
>> + (phy_addr << XEMACPSS_PHYMNTNC_PHYAD_SHIFT_MASK) |
>> + (regnum << XEMACPSS_PHYMNTNC_PHREG_SHIFT_MASK) | *data;
>> +
>> + /* Write mgtcr and wait for completion */
>> + writel(mgtcr, &regs->phymntnc);
>> +
>> + if (mdio_wait(dev))
>> + return 1;
>> +
>> + if (op == XEMACPSS_PHYMNTNC_OP_R_MASK)
>> + *data = readl(&regs->phymntnc);
>> +
>> + return 0;
>> +}
>> +
>> +static u32 phyread(struct eth_device *dev, u32 phy_addr, u32 regnum, u16 *val)
>> +{
>> + return phy_setup_op(dev, phy_addr, regnum,
>> + XEMACPSS_PHYMNTNC_OP_R_MASK, val);
>> +}
>> +
>> +static u32 phywrite(struct eth_device *dev, u32 phy_addr, u32 regnum, u16 data)
>> +{
>> + return phy_setup_op(dev, phy_addr, regnum,
>> + XEMACPSS_PHYMNTNC_OP_W_MASK, &data);
>> +}
>> +
>> +static void phy_detection(struct eth_device *dev)
>> +{
>> + int i, ret;
>> + u16 phyreg;
>> + struct gemac_priv *priv = dev->priv;
>> +
>> + if (priv->phyaddr != -1) {
>> + ret = phyread(dev, priv->phyaddr, PHY_DETECT_REG, &phyreg);
>> + if (!ret && (phyreg != 0xFFFF) &&
>> + ((phyreg & PHY_DETECT_MASK) == PHY_DETECT_MASK)) {
>> + /* Found a valid PHY address */
>> + debug("Default phy address %d is valid\n",
>> + priv->phyaddr);
>> + return;
>> + } else {
>> + debug("PHY address is not setup correctly %d\n",
>> + priv->phyaddr);
>> + priv->phyaddr = -1;
>> + }
>> + }
>> +
>> + debug("detecting phy address\n");
>> + if (priv->phyaddr == -1) {
>> + /* detect the PHY address */
>> + for (i = 31; i >= 0; i--) {
>> + ret = phyread(dev, i, PHY_DETECT_REG, &phyreg);
>> + if (!ret && (phyreg != 0xFFFF) &&
>> + ((phyreg & PHY_DETECT_MASK) == PHY_DETECT_MASK)) {
>> + /* Found a valid PHY address */
>> + priv->phyaddr = i;
>> + debug("Found valid phy address, %d\n", i);
>> + return;
>> + }
>> + }
>> + }
>> + printf("PHY is not detected\n");
>> +}
>
> Phy detection should be optional (behind a config token). It also
> sounds like something that has nothing to do with this driver. If you
> want to add it, it should probably be in phylib and work with any
> driver.

No problem to remove it. Will look at phylib if provides feature like this.
It means detect phy or check that the actual phy address is correct.

>
>> +
>> +/* Address bits[47:32] bit[31:0] are in BOTTOM */
>> +#define XEMACPSS_LADDR_MACH_MASK 0x0000FFFF
>> +
>> +static void setmacaddress(struct eth_device *dev, void *addressptr, u8 index)
>> +{
>> + u32 macaddr;
>> + struct gem_regs *regs = (struct gem_regs *)dev->iobase;
>> + u8 *aptr = (u8 *) addressptr;
>> +
>> + /* Set the MAC bits [31:0] in BOT */
>> + macaddr = aptr[0];
>> + macaddr |= aptr[1] << 8;
>> + macaddr |= aptr[2] << 16;
>> + macaddr |= aptr[3] << 24;
>> + writel(macaddr, &regs->laddr[index][LADDR_LOW]);
>> +
>> + /* There are reserved bits in TOP so don't affect them */
>> + macaddr = readl(&regs->laddr[index][LADDR_HIGH]);
>> +
>> + macaddr &= ~XEMACPSS_LADDR_MACH_MASK;
>> +
>> + /* Set MAC bits [47:32] in TOP */
>> + macaddr |= aptr[4];
>> + macaddr |= aptr[5] << 8;
>> +
>> + writel(macaddr, &regs->laddr[index][LADDR_HIGH]);
>> +
>> + /* Set the ID bits in MATCHx register - settypeidcheck */
>> + writel(0, &regs->match[index]);
>> +}
>> +
>> +static int gem_init(struct eth_device *dev, bd_t * bis)
>> +{
>> + int tmp;
>> + int i;
>> + struct gem_regs *regs = (struct gem_regs *)dev->iobase;
>> + char emacpss_zero_mac[6] = { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 };
>> + struct gemac_priv *priv = dev->priv;
>> + struct phy_device *phydev;
>> + u32 supported = SUPPORTED_10baseT_Half |
>> + SUPPORTED_10baseT_Full |
>> + SUPPORTED_100baseT_Half |
>> + SUPPORTED_100baseT_Full |
>> + SUPPORTED_1000baseT_Half |
>> + SUPPORTED_1000baseT_Full;
>> +
>> + if (priv->initialized)
>> + return 0;
>> +
>> + /* Disable all interrupts */
>> + writel(0xFFFFFFFF, &regs->idr);
>> +
>> + /* Disable the receiver & transmitter */
>> + writel(0, &regs->nwctrl);
>> + writel(0, &regs->txsr);
>> + writel(0, &regs->rxsr);
>> + writel(0, &regs->phymntnc);
>> +
>> + /* Clear the Hash registers for the mac address pointed by AddressPtr */
>> + writel(0x0, &regs->hashl);
>> + /* Write bits [63:32] in TOP */
>> + writel(0x0, &regs->hashh);
>> +
>> + for (i = 0; i < 4; i++)
>> + setmacaddress(dev, emacpss_zero_mac, i);
>> +
>> + /* Clear all counters */
>> + for (i = 0; i <= (sizeof(struct gem_regs) -
>> + offsetof(struct gem_regs, stat)) / 4; i++)
>> + readl(&regs->stat[i]);
>> +
>> + /* Setup RxBD space */
>> + memset(&(priv->rx_bd), 0, sizeof(priv->rx_bd));
>> + /* Create the RxBD ring */
>> + memset(&(priv->rxbuffers), 0, sizeof(priv->rxbuffers));
>> +
>> + for (i = 0; i < RX_BUF; i++) {
>> + priv->rx_bd[i].status = 0xF0000000;
>> + priv->rx_bd[i].addr = (u32)((char *) &(priv->rxbuffers) +
>> + (i * PKTSIZE_ALIGN));
>> + }
>> + /* WRAP bit to last BD */
>> + priv->rx_bd[--i].addr |= XEMACPSS_RXBUF_WRAP_MASK;
>> + /* Write RxBDs to IP */
>> + writel((u32) &(priv->rx_bd), &regs->rxqbase);
>> +
>> +
>> + /* MAC Setup */
>> + /*
>> + * Following is the setup for Network Configuration register.
>> + * Bit 0: Set for 100 Mbps operation.
>> + * Bit 1: Set for Full Duplex mode.
>> + * Bit 4: Set to allow Copy all frames.
>> + * Bit 17: Set for FCS removal.
>> + * Bits 20-18: Set with value binary 010 to divide pclk by 32
>> + * (pclk up to 80 MHz)
>> + */
>> + writel(0x000A0013, &regs->nwcfg);
>> +
>> + /*
>> + * Following is the setup for DMA Configuration register.
>> + * Bits 4-0: To set AHB fixed burst length for DMA data operations ->
>> + * Set with binary 00100 to use INCR4 AHB bursts.
>> + * Bits 9-8: Receiver packet buffer memory size ->
>> + * Set with binary 11 to Use full configured addressable space (8 Kb)
>> + * Bit 10 : Transmitter packet buffer memory size ->
>> + * Set with binary 1 to Use full configured addressable space (4 Kb)
>> + * Bits 23-16 : DMA receive buffer size in AHB system memory ->
>> + * Set with binary 00011000 to use 1536 byte(1*max length frame/buffer)
>> + */
>> + writel(0x00180704, &regs->dmacr);
>> +
>> + /*
>> + * Following is the setup for Network Control register.
>> + * Bit 2: Set to enable Receive operation.
>> + * Bit 3: Set to enable Transmitt operation.
>> + * Bit 4: Set to enable MDIO operation.
>> + */
>> + tmp = readl(&regs->nwctrl);
>> + /* MDIO, Rx and Tx enable */
>> + tmp |= XEMACPSS_NWCTRL_MDEN_MASK | XEMACPSS_NWCTRL_RXEN_MASK |
>> + XEMACPSS_NWCTRL_TXEN_MASK;
>> + writel(tmp, &regs->nwctrl);
>> +
>> + phy_detection(dev);
>> +
>> + /* interface - look at tsec */
>> + phydev = phy_connect(priv->bus, priv->phyaddr, dev, 0);
>> +
>> + phydev->supported &= supported;
>> + phydev->advertising = phydev->supported;
>> + priv->phydev = phydev;
>> + phy_config(phydev);
>> + phy_startup(phydev);
>> +
>> + priv->initialized = 1;
>> + return 0;
>
> How do you set the clock rate for the GEM based on the phy's link
> speed? You specify that you support 10 Mb/s, 100 Mb/s, and 1000Mb/s.
> They need the GEM running at 2.5 MHz, 25 MHz, and 125 MHz
> respectively.

I think you are talking about MDCCLKDIV value. Current setting is 010 which is
pclk up to 80MHz. My zc702 works only at 100MHz that's why I can't test 1000Mb/s.

If doesn't work at 1000Mb/s, we can setup different value.

On two boards which we have tested there were no problem with this driver.

>
>> +}
>> +
>> +static int gem_send(struct eth_device *dev, void *ptr, int len)
>> +{
>> + int status;
>> + u32 val;
>> + struct gemac_priv *priv = dev->priv;
>> + struct gem_regs *regs = (struct gem_regs *)dev->iobase;
>> +
>> + if (!priv->initialized) {
>> + puts("Error GMAC not initialized");
>> + return -1;
>> + }
>> +
>> + /* setup BD */
>> + writel((u32)&(priv->tx_bd), &regs->txqbase);
>> +
>> + /* Setup Tx BD */
>> + memset((void *) &(priv->tx_bd), 0, sizeof(struct emac_bd));
>> +
>> + priv->tx_bd.addr = (u32)ptr;
>> + priv->tx_bd.status = len | XEMACPSS_TXBUF_LAST_MASK |
>> + XEMACPSS_TXBUF_WRAP_MASK;
>> +
>> + /* Start transmit */
>> + val = readl(&regs->nwctrl) | XEMACPSS_NWCTRL_STARTTX_MASK;
>> + writel(val, &regs->nwctrl);
>> +
>> + /* Read the stat register to know if the packet has been transmitted */
>> + status = readl(&regs->txsr);
>> + if (status & (XEMACPSS_TXSR_HRESPNOK_MASK | XEMACPSS_TXSR_URUN_MASK |
>> + XEMACPSS_TXSR_BUFEXH_MASK)) {
>> + printf("Something has gone wrong here!? Status is 0x%x.\n",
>> + status);
>> + }
>> +
>> + /* Clear Tx status register before leaving . */
>> + writel(status, &regs->txsr);
>> + return 0;
>> +}
>> +
>> +/* Do not check frame_recd flag in rx_status register 0x20 - just poll BD */
>> +static int gem_recv(struct eth_device *dev)
>> +{
>> + int frame_len;
>> + struct gemac_priv *priv = dev->priv;
>> +
>> + if (!(priv->rx_bd[priv->rxbd_current].addr & XEMACPSS_RXBUF_NEW_MASK))
>> + return 0;
>> +
>> + if (!(priv->rx_bd[priv->rxbd_current].status &
>> + (XEMACPSS_RXBUF_SOF_MASK | XEMACPSS_RXBUF_EOF_MASK))) {
>> + printf("GEM: SOF or EOF not set for last buffer received!\n");
>> + return 0;
>> + }
>> +
>> + frame_len = priv->rx_bd[priv->rxbd_current].status &
>> + XEMACPSS_RXBUF_LEN_MASK;
>> + if (frame_len) {
>> + NetReceive((u8 *) (priv->rx_bd[priv->rxbd_current].addr &
>> + XEMACPSS_RXBUF_ADD_MASK), frame_len);
>> +
>> + if (priv->rx_bd[priv->rxbd_current].status &
>> + XEMACPSS_RXBUF_SOF_MASK)
>> + priv->rx_first_buf = priv->rxbd_current;
>> + else {
>> + priv->rx_bd[priv->rxbd_current].addr &=
>> + ~XEMACPSS_RXBUF_NEW_MASK;
>> + priv->rx_bd[priv->rxbd_current].status = 0xF0000000;
>> + }
>> +
>> + if (priv->rx_bd[priv->rxbd_current].status &
>> + XEMACPSS_RXBUF_EOF_MASK) {
>> + priv->rx_bd[priv->rx_first_buf].addr &=
>> + ~XEMACPSS_RXBUF_NEW_MASK;
>> + priv->rx_bd[priv->rx_first_buf].status = 0xF0000000;
>> + }
>> +
>> + if ((++priv->rxbd_current) >= RX_BUF)
>> + priv->rxbd_current = 0;
>> +
>> + return frame_len;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void gem_halt(struct eth_device *dev)
>> +{
>> + return;
>> +}
>> +
>> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB)
>
> At the top of this file you error if CONFIG_PHYLIB is not defined.
> That means that in no useful case will this not be defined, so you can
> remove this #if entirely.

ok. I have one more patch which covers cases where PHYLIB is not enabled
but yes it can be removed from this driver.

>
>> +static int gem_miiphyread(const char *devname, uchar addr,
>> + uchar reg, ushort *val)
>> +{
>> + struct eth_device *dev = eth_get_dev();
>> + int ret;
>> +
>> + ret = phyread(dev, addr, reg, val);
>> + debug("%s 0x%x, 0x%x, 0x%x\n", __func__, addr, reg, *val);
>> + return ret;
>> +}
>> +
>> +static int gem_miiphy_write(const char *devname, uchar addr,
>> + uchar reg, ushort val)
>> +{
>> + struct eth_device *dev = eth_get_dev();
>> +
>> + debug("%s 0x%x, 0x%x, 0x%x\n", __func__, addr, reg, val);
>> + return phywrite(dev, addr, reg, val);
>> +}
>> +#endif
>> +
>> +int xilinx_gem_initialize(bd_t *bis, int base_addr)
>> +{
>> + struct eth_device *dev;
>> + struct gemac_priv *priv;
>> +
>> + dev = calloc(1, sizeof(*dev));
>> + if (dev == NULL)
>> + return -1;
>> +
>> + dev->priv = calloc(1, sizeof(struct gemac_priv));
>> + if (dev->priv == NULL) {
>> + free(dev);
>> + return -1;
>> + }
>> + priv = dev->priv;
>> +
>> +#ifdef CONFIG_PHY_ADDR
>> + priv->phyaddr = CONFIG_PHY_ADDR;
>> +#else
>> + priv->phyaddr = -1;
>> +#endif
>> +
>> + sprintf(dev->name, "XGem.%x", base_addr);
>
> Replace "XGem" with "Zynq GEM". Also, the base address is not nice to
> look at here. You should know about the 2 base addresses in hardware
> (0xE000B000 and 0xE000C000) and should pass in the instance instead of
> the base address. Then you can use the instance here and not force
> each board to specify the base address.

This is the same case as was for serial driver. If we connect Microblaze to it
then Microblaze could use it and addresses can changed. That's why
using name.<addr> is 100% description which IP u-boot uses.

About the name - there is 16 chars for it where 9 chars is used for baseaddr + dot
and the rest can be used for driver identification.

I don't care if is XGem or Zynq, etc.

>
>> +
>> + dev->iobase = base_addr;
>> +
>> + dev->init = gem_init;
>> + dev->halt = gem_halt;
>> + dev->send = gem_send;
>> + dev->recv = gem_recv;
>
> You should have dev->write_hwaddr = setmacaddress; here. Naturally
> you should make the signature match and rename the function.

will look.

>
>> +
>> + eth_register(dev);
>> +
>> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB)
>
> At the top of this file you error if CONFIG_PHYLIB is not defined.
> That means that in no useful case will this not be defined, so you can
> remove this #if entirely.

The same reason as above.

Thanks,
Michal

--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian