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

[Marek Vasut <marex@denx.de>]

Dear Michal Simek,

[...]

> +static inline int mdio_wait(struct eth_device *dev)
> +{
> +	struct zynq_gem_regs *regs = (struct zynq_gem_regs *)dev->iobase;
> +	u32 timeout = 200;
> +
> +	/* Wait till MDIO interface is ready to accept a new transaction. */
> +	while (timeout && !(readl(&regs->nwsr) & ZYNQ_GEM_NWSR_MDIOIDLE_MASK)) {

I'd say, rework this to

while (--timeout) {
	if (readl() & ... )
		break;
	WATCHDOG_RESET();
}

The WATCHDOG_RESET will give you the udelay and restart the WDT if you use any. 
Also, I think it's more readable when you omit the complex condition for the 
while cycle and split it a bit.

> +		timeout--;
> +		udelay(1);
> +	}
> +
> +	if (!timeout) {
> +		printf("%s: Timeout\n", __func__);
> +		return 1;
> +	}
> +
> +	return 0;
> +}

[...]

> +static int zynq_gem_init(struct eth_device *dev, bd_t * bis)
> +{
> +	int tmp;
> +	int i;
> +	struct zynq_gem_regs *regs = (struct zynq_gem_regs *)dev->iobase;
> +	struct zynq_gem_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);
> +
> +	/* Clear all counters */
> +	for (i = 0; i <= (sizeof(struct zynq_gem_regs) -
> +			offsetof(struct zynq_gem_regs, stat)) / 4; i++)

Add a const int variable and use it here so you don't have to break the for () .

> +		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 |= ZYNQ_GEM_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:  Unset to allow Copy all frames - MAC checking
> +	 *  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(0x000A0003, &regs->nwcfg);

Well you know ... magic numbers and defined bits ;-)

> +	/*
> +	 * 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 |= ZYNQ_GEM_NWCTRL_MDEN_MASK | ZYNQ_GEM_NWCTRL_RXEN_MASK |
> +	    ZYNQ_GEM_NWCTRL_TXEN_MASK;
> +	writel(tmp, &regs->nwctrl);

setbits_le32()

> +	/* 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;
> +}
> +
> +static int zynq_gem_send(struct eth_device *dev, void *ptr, int len)
> +{
> +	int status;
> +	u32 val;
> +	struct zynq_gem_priv *priv = dev->priv;
> +	struct zynq_gem_regs *regs = (struct zynq_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 | ZYNQ_GEM_TXBUF_LAST_MASK |
> +						ZYNQ_GEM_TXBUF_WRAP_MASK;
> +
> +	/* Start transmit */
> +	val = readl(&regs->nwctrl) | ZYNQ_GEM_NWCTRL_STARTTX_MASK;
> +	writel(val, &regs->nwctrl);

setbits_le32()

> +	/* Read the stat register to know if the packet has been transmitted */
> +	status = readl(&regs->txsr);
> +	if (status & (ZYNQ_GEM_TXSR_HRESPNOK_MASK | ZYNQ_GEM_TXSR_URUN_MASK |
> +						ZYNQ_GEM_TXSR_BUFEXH_MASK)) {

Add const int mask for the above.

> +		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 zynq_gem_recv(struct eth_device *dev)
> +{
> +	int frame_len;
> +	struct zynq_gem_priv *priv = dev->priv;
> +
> +	if (!(priv->rx_bd[priv->rxbd_current].addr & ZYNQ_GEM_RXBUF_NEW_MASK))
> +		return 0;
> +
> +	if (!(priv->rx_bd[priv->rxbd_current].status &
> +			(ZYNQ_GEM_RXBUF_SOF_MASK | ZYNQ_GEM_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 &
> +							ZYNQ_GEM_RXBUF_LEN_MASK;

Just introduce some variable for priv->rx_bd[priv->rxbd_current] so you don't 
have such long lines

> +	if (frame_len) {
> +		NetReceive((u8 *) (priv->rx_bd[priv->rxbd_current].addr &
> +					ZYNQ_GEM_RXBUF_ADD_MASK), frame_len);
> +
> +		if (priv->rx_bd[priv->rxbd_current].status &
> +							ZYNQ_GEM_RXBUF_SOF_MASK)
> +			priv->rx_first_buf = priv->rxbd_current;
> +		else {
> +			priv->rx_bd[priv->rxbd_current].addr &=
> +						~ZYNQ_GEM_RXBUF_NEW_MASK;
> +			priv->rx_bd[priv->rxbd_current].status = 0xF0000000;
> +		}
> +
> +		if (priv->rx_bd[priv->rxbd_current].status &
> +						ZYNQ_GEM_RXBUF_EOF_MASK) {
> +			priv->rx_bd[priv->rx_first_buf].addr &=
> +						~ZYNQ_GEM_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 zynq_gem_halt(struct eth_device *dev)
> +{
> +	return;

Does this need to be implemented at all ?

> +}
> +
> +static int zynq_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 zynq_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);
> +}
> +
> +int zynq_gem_initialize(bd_t *bis, int base_addr)
> +{
> +	struct eth_device *dev;
> +	struct zynq_gem_priv *priv;
> +
> +	dev = calloc(1, sizeof(*dev));
> +	if (dev == NULL)
> +		return -1;
> +
> +	dev->priv = calloc(1, sizeof(struct zynq_gem_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, "Gem.%x", base_addr);
> +
> +	dev->iobase = base_addr;
> +
> +	dev->init = zynq_gem_init;
> +	dev->halt = zynq_gem_halt;
> +	dev->send = zynq_gem_send;
> +	dev->recv = zynq_gem_recv;
> +	dev->write_hwaddr = zynq_gem_setup_mac;
> +
> +	eth_register(dev);
> +
> +	miiphy_register(dev->name, zynq_gem_miiphyread, zynq_gem_miiphy_write);
> +	priv->bus = miiphy_get_dev_by_name(dev->name);
> +
> +	return 1;
> +}
> diff --git a/include/netdev.h b/include/netdev.h
> index d1aaf0c..b8d303d 100644
> --- a/include/netdev.h
> +++ b/include/netdev.h
> @@ -104,7 +104,7 @@ int xilinx_emaclite_initialize(bd_t *bis, unsigned long
> base_addr, int txpp, int rxpp);
>  int xilinx_ll_temac_eth_init(bd_t *bis, unsigned long base_addr, int
> flags, unsigned long ctrl_addr);
> -
> +int zynq_gem_initialize(bd_t *bis, int base_addr);
>  /*
>   * As long as the Xilinx xps_ll_temac ethernet driver has not its own
> interface * exported by a public hader file, we need a global definition
> at this point.