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

[Michal Simek <monstr@monstr.eu>]

On 09/13/2012 11:28 AM, Marek Vasut wrote:
> 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.

make sense

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

if you like.

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


:-) Or. let me create macros.


>
>> +	/*
>> +	 * 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);

the same here.

>> +
>> +	/*
>> +	 * 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()

ok.

>
>> +	/* 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()

ok

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

or macro should be fine.


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

no problem.

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

probably not.
Let me do that changes and I will send updated version.

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