diff mbox

[U-Boot] net: axi_ethernet: Add driver to u-boot

Message ID 1314705919-4237-2-git-send-email-monstr@monstr.eu
State Superseded
Headers show

Commit Message

Michal Simek Aug. 30, 2011, 12:05 p.m. UTC
Add the first axi_ethernet driver for little-endian Microblaze.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---
 drivers/net/Makefile          |    1 +
 drivers/net/xilinx_axi_emac.c |  622 +++++++++++++++++++++++++++++++++++++++++
 include/netdev.h              |    1 +
 3 files changed, 624 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/xilinx_axi_emac.c

Comments

Marek Vasut Aug. 31, 2011, 12:19 p.m. UTC | #1
On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
> Add the first axi_ethernet driver for little-endian Microblaze.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>
> ---
>  drivers/net/Makefile          |    1 +
>  drivers/net/xilinx_axi_emac.c |  622
> +++++++++++++++++++++++++++++++++++++++++ include/netdev.h              | 
>   1 +
>  3 files changed, 624 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/xilinx_axi_emac.c
> 
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 4541eaf..ae9d4cb 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o
>  COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
>  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_LL_TEMAC) += xilinx_ll_temac.o
> 
> diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c
> new file mode 100644
> index 0000000..ce79b80
> --- /dev/null
> +++ b/drivers/net/xilinx_axi_emac.c
> @@ -0,0 +1,622 @@
> +/*
> + * Copyright (C) 2011 Michal Simek <monstr@monstr.eu>
> + * Copyright (C) 2011 PetaLogix
> + * Copyright (C) 2010 Xilinx, Inc. All rights reserved.
> + *
> + * 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 <config.h>
> +#include <common.h>
> +#include <net.h>
> +#include <malloc.h>
> +#include <asm/io.h>
> +#include <phy.h>
> +#include <miiphy.h>
> +
> +/* Axi Ethernet registers offset */
> +#define XAE_IS_OFFSET		0x0000000C /* Interrupt status */
> +#define XAE_IE_OFFSET		0x00000014 /* Interrupt enable */
> +#define XAE_RCW1_OFFSET		0x00000404 /* Rx Configuration Word 1 */
> +#define XAE_TC_OFFSET		0x00000408 /* Tx Configuration */
> +#define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
> +#define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
> +#define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
> +#define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data */
> +#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read Data */

Please use struct xae_regs {...} as the rest of the u-boot.

> +
> +/* Link setup */
> +#define XAE_EMMC_LINKSPEED_MASK	0xC0000000 /* Link speed */
> +#define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 Mbit */
> +#define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for 100 Mbit */
> +#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask for 1000 Mbit
> */ +

Use (1 << n) ?

> +/* Interrupt Status/Enable/Mask Registers bit definitions */
> +#define XAE_INT_RXRJECT_MASK	0x00000008 /* Rx frame rejected */
> +#define XAE_INT_MGTRDY_MASK	0x00000080 /* MGT clock Lock */
> +

[...]

> +#define DMAALIGN	128
> +
> +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;

Don't use cammelcase, all lowcase please. Also, can't you allocate this with 
memalign and hide it in axidma_priv or something ?
> +
> +/* reflect dma offsets */
> +struct axidma_reg {
> +	u32 control; /* DMACR */
> +	u32 status; /* DMASR */
> +	u32 current; /* CURDESC */
> +	u32 reserved;
> +	u32 tail; /* TAILDESC */
> +};
> +
> +/* Private driver structures */
> +struct axidma_priv {
> +	struct axidma_reg *dmatx;
> +	struct axidma_reg *dmarx;
> +	int phyaddr;
> +
> +	struct phy_device *phydev;
> +	struct mii_dev *bus;
> +};
> +
> +/* BD descriptors */
> +struct axidma_bd {
> +	u32 next;	/* Next descriptor pointer */
> +	u32 reserved1;
> +	u32 phys;	/* Buffer address */
> +	u32 reserved2;
> +	u32 reserved3;
> +	u32 reserved4;
> +	u32 cntrl;	/* Control */
> +	u32 status;	/* Status */
> +	u32 app0;
> +	u32 app1;	/* TX start << 16 | insert */
> +	u32 app2;	/* TX csum seed */
> +	u32 app3;
> +	u32 app4;
> +	u32 sw_id_offset;
> +	u32 reserved5;
> +	u32 reserved6;
> +};
> +
> +/* Static BDs - driver uses only one BD */
> +static struct axidma_bd tx_bd __attribute((aligned(DMAALIGN)));
> +static struct axidma_bd rx_bd __attribute((aligned(DMAALIGN)));
> +
> +static inline void aximac_out32(u32 addr, u32 offset, u32 val)
> +{
> +	out_be32((u32 *)(addr + offset), val);

Please fix these casts ... though I don't think you even need these functions.

> +}
> +
> +static inline u32 aximac_in32(u32 addr, u32 offset)
> +{
> +	return in_be32((u32 *)(addr + offset));
> +}
> +
> +
> +/* 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
> +
> +static u16 phyread(struct eth_device *dev, u32 phyaddress, u32
> registernum) +{
> +	u32 mdioctrlreg = 0;
> +
> +	/* Wait till MDIO interface is ready to accept a new transaction. */
> +	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
> +						& XAE_MDIO_MCR_READY_MASK))
> +		;

No endless loops please.

> +
> +	mdioctrlreg = ((phyaddress << XAE_MDIO_MCR_PHYAD_SHIFT) &
> +			XAE_MDIO_MCR_PHYAD_MASK) |
> +			((registernum << XAE_MDIO_MCR_REGAD_SHIFT)
> +			& XAE_MDIO_MCR_REGAD_MASK) |
> +			XAE_MDIO_MCR_INITIATE_MASK |
> +			XAE_MDIO_MCR_OP_READ_MASK;
> +
> +	aximac_out32(dev->iobase, XAE_MDIO_MCR_OFFSET, mdioctrlreg);
> +
> +	/* Wait till MDIO transaction is completed. */
> +	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
> +						& XAE_MDIO_MCR_READY_MASK))
> +		;
> +
> +	/* Read data */
> +	return (u16) aximac_in32(dev->iobase, XAE_MDIO_MRD_OFFSET);

Is the cast needed ?

> +}
> +
> +static void phywrite(struct eth_device *dev, u32 phyaddress, u32
> registernum, +								u32 data)
> +{
> +	u32 mdioctrlreg = 0;
> +
> +	/* Wait till MDIO interface is ready to accept a new transaction. */
> +	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
> +						& XAE_MDIO_MCR_READY_MASK))
> +		;

No endless loops.

> +
> +	mdioctrlreg = ((phyaddress << XAE_MDIO_MCR_PHYAD_SHIFT) &
> +			XAE_MDIO_MCR_PHYAD_MASK) |
> +			((registernum << XAE_MDIO_MCR_REGAD_SHIFT)
> +			& XAE_MDIO_MCR_REGAD_MASK) |
> +			XAE_MDIO_MCR_INITIATE_MASK |
> +			XAE_MDIO_MCR_OP_WRITE_MASK;
> +
> +	/* Write data */
> +	aximac_out32(dev->iobase, XAE_MDIO_MWD_OFFSET, data);
> +
> +	aximac_out32(dev->iobase, XAE_MDIO_MCR_OFFSET, mdioctrlreg);
> +
> +	/* Wait till MDIO transaction is completed. */
> +	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
> +						& XAE_MDIO_MCR_READY_MASK))
> +		;
> +}
> +
> +
> +/* setting axi emac and phy to proper setting */
> +static int setup_phy(struct eth_device *dev)
> +{
> +#ifdef CONFIG_PHYLIB
> +	int i;
> +	unsigned int speed;
> +	u16 phyreg;
> +	u32 emmc_reg;
> +	struct axidma_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;

cammelcase ?

> +
> +	if (priv->phyaddr == -1) {
> +		/* detect the PHY address */
> +		for (i = 31; i >= 0; i--) {
> +			phyreg = phyread(dev, i, PHY_DETECT_REG);
> +
> +			if ((phyreg != 0xFFFF) &&
> +			((phyreg & PHY_DETECT_MASK) == PHY_DETECT_MASK)) {
> +				/* Found a valid PHY address */
> +				priv->phyaddr = i;
> +				debug("Found valid phy address, %d\n", phyreg);
> +				break;
> +			}
> +		}
> +	}
> +
> +	/* 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);
> +
> +	switch(phydev->speed) {
> +	case 1000:
> +		speed = XAE_EMMC_LINKSPD_1000;
> +		break;
> +	case 100:
> +		speed = XAE_EMMC_LINKSPD_100;
> +		break;
> +	case 10:
> +		speed = XAE_EMMC_LINKSPD_10;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	/* Setup the emac for the phy speed */
> +	emmc_reg = aximac_in32(dev->iobase, XAE_EMMC_OFFSET);
> +	emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
> +	emmc_reg |= speed;
> +
> +	/* Write new speed setting out to Axi Ethernet */
> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);

Use clrsetbits() here.

> +
> +	/*
> +	* Setting the operating speed of the MAC needs a delay. There
> +	* doesn't seem to be register to poll, so please consider this
> +	* during your application design.
> +	*/
> +	udelay(1);
> +
> +	return 1;
> +#else
> +	puts("Enable PHYLIB support!\n");

Compile time warning at the top of the file please.

> +	return 0;
> +#endif
> +}
> +
> +/* STOP DMA transfers */
> +static void axiemac_halt(struct eth_device *dev)
> +{
> +	struct axidma_priv *priv = dev->priv;
> +
> +	/* Stop the hardware */
> +	priv->dmatx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;
> +	priv->dmarx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;
> +
> +	debug("axiemac halted\n");
> +}
> +
> +static void axi_ethernet_init(struct eth_device *dev)
> +{
> +	/*
> +	 * Check the status of the MgtRdy bit in the interrupt status
> +	 * registers. This must be done to allow the MGT clock to become stable
> +	 * for the Sgmii and 1000BaseX PHY interfaces. No other register reads
> +	 * will be valid until this bit is valid.
> +	 * The bit is always a 1 for all other PHY interfaces.
> +	 */
> +	u32 timeout = 200;
> +	while (timeout && (!(aximac_in32(dev->iobase, XAE_IS_OFFSET) &
> +							XAE_INT_MGTRDY_MASK)))
> +		timeout--;
> +
> +	if (!timeout)
> +		printf("%s: Timeout\n", __func__);

I think you don't want to continue in case of timeout ?

> +
> +	/* Stop the device and reset HW */
> +	/* Disable interrupts */
> +	aximac_out32(dev->iobase, XAE_IE_OFFSET, 0);
> +
> +	/* Disable the receiver */
> +	aximac_out32(dev->iobase, XAE_RCW1_OFFSET,
> +		aximac_in32(dev->iobase, XAE_RCW1_OFFSET) & ~XAE_RCW1_RX_MASK);
> +
> +	/*
> +	 * Stopping the receiver in mid-packet causes a dropped packet
> +	 * indication from HW. Clear it.
> +	 */
> +	/* set the interrupt status register to clear the interrupt */
> +	aximac_out32(dev->iobase, XAE_IS_OFFSET, XAE_INT_RXRJECT_MASK);
> +
> +	/* Setup HW */
> +	/* Set default MDIO divisor */
> +	aximac_out32(dev->iobase, XAE_MDIO_MC_OFFSET,
> +			(u32) XAE_MDIO_DIV_DFT | XAE_MDIO_MC_MDIOEN_MASK);
> +
> +	debug("XAxiEthernet InitHw: done\n");

Unify the debuging message please.

> +}
> +
> +static void setup_mac(struct eth_device *dev)
> +{
> +	/* Set the MAC address */
> +	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
> +		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
> +	aximac_out32(dev->iobase, XAE_UAW0_OFFSET, val);
> +
> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
> +	val |= aximac_in32(dev->iobase, XAE_UAW1_OFFSET) &
> +						~XAE_UAW1_UNICASTADDR_MASK;
> +	aximac_out32(dev->iobase, XAE_UAW1_OFFSET, val);

clrsetbits()

> +}
> +
> +/* Reset DMA engine */
> +static void axi_dma_init(struct eth_device *dev)
> +{
> +	struct axidma_priv *priv = dev->priv;
> +
> +	/* Reset the engine so the hardware starts from a known state */
> +	priv->dmatx->control = XAXIDMA_CR_RESET_MASK;
> +	priv->dmarx->control = XAXIDMA_CR_RESET_MASK;
> +
> +	/* At the initialization time, hardware should finish reset quickly */
> +	u32 timeout = 500;
> +	while (timeout--) {
> +		/* Check transmit/receive channel */
> +		/* Reset is done when the reset bit is low */
> +		if (!(priv->dmatx->control | priv->dmarx->control)
> +						& XAXIDMA_CR_RESET_MASK)
> +			break;
> +		timeout -= 1;

You're decrementing it twice in here.

> +	}
> +
> +	if (!timeout)
> +		printf("%s: Timeout\n", __func__);
> +}
> +
> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
> +{
> +	struct axidma_priv *priv = dev->priv;
> +	debug("axi emac init started\n");
> +
> +	/*
> +	 * Initialize AXIDMA engine. AXIDMA engine must be initialized before
> +	 * AxiEthernet. During AXIDMA engine initialization, AXIDMA hardware is
> +	 * reset, and since AXIDMA reset line is connected to AxiEthernet, this
> +	 * would ensure a reset of AxiEthernet.
> +	 */
> +	axi_dma_init(dev);
> +
> +	/* Initialize AxiEthernet hardware. */
> +	axi_ethernet_init(dev);
> +	setup_mac(dev);
> +
> +	/* Start the hardware */
> +	priv->dmarx->control |= XAXIDMA_CR_RUNSTOP_MASK;
> +	/* Start DMA RX channel. Now it's ready to receive data.*/
> +	priv->dmarx->current = (u32)&rx_bd;
> +
> +	/* Disable all RX interrupts before RxBD space setup */
> +	priv->dmarx->control &= ~(XAXIDMA_IRQ_ALL_MASK & XAXIDMA_IRQ_ALL_MASK);

I don't think I understand the above code.

> +
> +	/* Setup the BD. */
> +	memset((void *) &rx_bd, 0, sizeof(rx_bd));
> +	rx_bd.next = (u32)&rx_bd;
> +	rx_bd.phys = (u32)&RxFrame;
> +	rx_bd.cntrl = sizeof(RxFrame);

please get rid of the cammelcase.

> +	/* Flush the last BD so DMA core could see the updates */
> +	flush_cache((u32)&rx_bd, sizeof(rx_bd));
> +
> +	/* it is necessary to flush RxFrame because if you don't do it
> +	 * then cache can contain uninitialized data */
> +	flush_cache((u32)&RxFrame, sizeof(RxFrame));
> +
> +	/* Rx BD is ready - start */
> +	priv->dmarx->tail = (u32)&rx_bd;
> +
> +	/* enable TX */
> +	aximac_out32(dev->iobase, XAE_TC_OFFSET, XAE_TC_TX_MASK);
> +	/* enable RX */
> +	aximac_out32(dev->iobase, XAE_RCW1_OFFSET, XAE_RCW1_RX_MASK);
> +

[...]

> +static int axiemac_recv(struct eth_device *dev)
> +{
> +	u32 length;
> +	struct axidma_priv *priv = dev->priv;
> +
> +	/* wait for an incoming packet */
> +	if (!IsRxReady(dev))
> +		return 0;
> +
> +	debug("axi emac, rx data ready\n");
> +
> +	/* Disable IRQ for a moment till packet is handled */
> +	priv->dmarx->control &= ~(XAXIDMA_IRQ_ALL_MASK & XAXIDMA_IRQ_ALL_MASK);
> +
> +	length = rx_bd.app4 & 0x0000FFFF;

Get rid of the strange constants please.

> +#ifdef DEBUG
> +	print_buffer(&RxFrame, &RxFrame[0], 1, length, 16);
> +#endif
> +	/* pass the received frame up for processing */
> +	if (length)
> +		NetReceive(RxFrame, length);
> +
> +#ifdef DEBUG
> +	/* It is useful to clear buffer to be sure that it is consistent */
> +	memset(RxFrame, 0, sizeof(RxFrame));
> +#endif
> +	/* Setup RxBD */
[...]

Cheers!
Michal Simek Aug. 31, 2011, 2:46 p.m. UTC | #2
Marek Vasut wrote:
> On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
>> Add the first axi_ethernet driver for little-endian Microblaze.
>>
>> Signed-off-by: Michal Simek <monstr@monstr.eu>
>> ---
>>  drivers/net/Makefile          |    1 +
>>  drivers/net/xilinx_axi_emac.c |  622
>> +++++++++++++++++++++++++++++++++++++++++ include/netdev.h              | 
>>   1 +
>>  3 files changed, 624 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/net/xilinx_axi_emac.c
>>
>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> index 4541eaf..ae9d4cb 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o
>>  COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
>>  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_LL_TEMAC) += xilinx_ll_temac.o
>>
>> diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c
>> new file mode 100644
>> index 0000000..ce79b80
>> --- /dev/null
>> +++ b/drivers/net/xilinx_axi_emac.c
>> @@ -0,0 +1,622 @@
>> +/*
>> + * Copyright (C) 2011 Michal Simek <monstr@monstr.eu>
>> + * Copyright (C) 2011 PetaLogix
>> + * Copyright (C) 2010 Xilinx, Inc. All rights reserved.
>> + *
>> + * 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 <config.h>
>> +#include <common.h>
>> +#include <net.h>
>> +#include <malloc.h>
>> +#include <asm/io.h>
>> +#include <phy.h>
>> +#include <miiphy.h>
>> +
>> +/* Axi Ethernet registers offset */
>> +#define XAE_IS_OFFSET		0x0000000C /* Interrupt status */
>> +#define XAE_IE_OFFSET		0x00000014 /* Interrupt enable */
>> +#define XAE_RCW1_OFFSET		0x00000404 /* Rx Configuration Word 1 */
>> +#define XAE_TC_OFFSET		0x00000408 /* Tx Configuration */
>> +#define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
>> +#define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
>> +#define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
>> +#define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data */
>> +#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read Data */
> 
> Please use struct xae_regs {...} as the rest of the u-boot.

struct is not useful here because it will be big with a lot of reserved values.

> 
>> +
>> +/* Link setup */
>> +#define XAE_EMMC_LINKSPEED_MASK	0xC0000000 /* Link speed */
>> +#define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 Mbit */
>> +#define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for 100 Mbit */
>> +#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask for 1000 Mbit
>> */ +
> 
> Use (1 << n) ?

just another solution - I prefer to use 32bit value - easier when you decode it
by md.

> 
>> +/* Interrupt Status/Enable/Mask Registers bit definitions */
>> +#define XAE_INT_RXRJECT_MASK	0x00000008 /* Rx frame rejected */
>> +#define XAE_INT_MGTRDY_MASK	0x00000080 /* MGT clock Lock */
>> +
> 
> [...]

same here..

> 
>> +#define DMAALIGN	128
>> +
>> +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
> 
> Don't use cammelcase, all lowcase please.

Agree - will be done in v2.

  Also, can't you allocate this with
> memalign and hide it in axidma_priv or something ?

There are two things in one.
1. Adding it to axidma_priv means that I will need to dynamically allocate big private structure
which is bad thing to do for several eth devices. This is FPGA you can create a lot of MACs that's
why I think it is better not to do so.
2. Current style is sharing this rxframe buffer among all controllers because only one MAC works.
Others are stopped which means that no packet come to them.

BTW: Looking for that memalign function - thanks.



>> +
>> +/* reflect dma offsets */
>> +struct axidma_reg {
>> +	u32 control; /* DMACR */
>> +	u32 status; /* DMASR */
>> +	u32 current; /* CURDESC */
>> +	u32 reserved;
>> +	u32 tail; /* TAILDESC */
>> +};
>> +
>> +/* Private driver structures */
>> +struct axidma_priv {
>> +	struct axidma_reg *dmatx;
>> +	struct axidma_reg *dmarx;
>> +	int phyaddr;
>> +
>> +	struct phy_device *phydev;
>> +	struct mii_dev *bus;
>> +};
>> +
>> +/* BD descriptors */
>> +struct axidma_bd {
>> +	u32 next;	/* Next descriptor pointer */
>> +	u32 reserved1;
>> +	u32 phys;	/* Buffer address */
>> +	u32 reserved2;
>> +	u32 reserved3;
>> +	u32 reserved4;
>> +	u32 cntrl;	/* Control */
>> +	u32 status;	/* Status */
>> +	u32 app0;
>> +	u32 app1;	/* TX start << 16 | insert */
>> +	u32 app2;	/* TX csum seed */
>> +	u32 app3;
>> +	u32 app4;
>> +	u32 sw_id_offset;
>> +	u32 reserved5;
>> +	u32 reserved6;
>> +};
>> +
>> +/* Static BDs - driver uses only one BD */
>> +static struct axidma_bd tx_bd __attribute((aligned(DMAALIGN)));
>> +static struct axidma_bd rx_bd __attribute((aligned(DMAALIGN)));
>> +
>> +static inline void aximac_out32(u32 addr, u32 offset, u32 val)
>> +{
>> +	out_be32((u32 *)(addr + offset), val);
> 
> Please fix these casts ... though I don't think you even need these functions.

Cast is necessary. I use that helper just because of recast.
If you see solution which will be elegant, I am open to use it.

> 
>> +}
>> +
>> +static inline u32 aximac_in32(u32 addr, u32 offset)
>> +{
>> +	return in_be32((u32 *)(addr + offset));
>> +}
>> +
>> +
>> +/* 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
>> +
>> +static u16 phyread(struct eth_device *dev, u32 phyaddress, u32
>> registernum) +{
>> +	u32 mdioctrlreg = 0;
>> +
>> +	/* Wait till MDIO interface is ready to accept a new transaction. */
>> +	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
>> +						& XAE_MDIO_MCR_READY_MASK))
>> +		;
> 
> No endless loops please.

Agree will fix it.

> 
>> +
>> +	mdioctrlreg = ((phyaddress << XAE_MDIO_MCR_PHYAD_SHIFT) &
>> +			XAE_MDIO_MCR_PHYAD_MASK) |
>> +			((registernum << XAE_MDIO_MCR_REGAD_SHIFT)
>> +			& XAE_MDIO_MCR_REGAD_MASK) |
>> +			XAE_MDIO_MCR_INITIATE_MASK |
>> +			XAE_MDIO_MCR_OP_READ_MASK;
>> +
>> +	aximac_out32(dev->iobase, XAE_MDIO_MCR_OFFSET, mdioctrlreg);
>> +
>> +	/* Wait till MDIO transaction is completed. */
>> +	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
>> +						& XAE_MDIO_MCR_READY_MASK))
>> +		;
>> +
>> +	/* Read data */
>> +	return (u16) aximac_in32(dev->iobase, XAE_MDIO_MRD_OFFSET);
> 
> Is the cast needed ?

not - thanks.

> 
>> +}
>> +
>> +static void phywrite(struct eth_device *dev, u32 phyaddress, u32
>> registernum, +								u32 data)
>> +{
>> +	u32 mdioctrlreg = 0;
>> +
>> +	/* Wait till MDIO interface is ready to accept a new transaction. */
>> +	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
>> +						& XAE_MDIO_MCR_READY_MASK))
>> +		;
> 
> No endless loops.

will be fixed.

> 
>> +
>> +	mdioctrlreg = ((phyaddress << XAE_MDIO_MCR_PHYAD_SHIFT) &
>> +			XAE_MDIO_MCR_PHYAD_MASK) |
>> +			((registernum << XAE_MDIO_MCR_REGAD_SHIFT)
>> +			& XAE_MDIO_MCR_REGAD_MASK) |
>> +			XAE_MDIO_MCR_INITIATE_MASK |
>> +			XAE_MDIO_MCR_OP_WRITE_MASK;
>> +
>> +	/* Write data */
>> +	aximac_out32(dev->iobase, XAE_MDIO_MWD_OFFSET, data);
>> +
>> +	aximac_out32(dev->iobase, XAE_MDIO_MCR_OFFSET, mdioctrlreg);
>> +
>> +	/* Wait till MDIO transaction is completed. */
>> +	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
>> +						& XAE_MDIO_MCR_READY_MASK))
>> +		;
>> +}
>> +
>> +
>> +/* setting axi emac and phy to proper setting */
>> +static int setup_phy(struct eth_device *dev)
>> +{
>> +#ifdef CONFIG_PHYLIB
>> +	int i;
>> +	unsigned int speed;
>> +	u16 phyreg;
>> +	u32 emmc_reg;
>> +	struct axidma_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;
> 
> cammelcase ?

Just using standard values.
include/phy.h
include/linux/ethtool.h

> 
>> +
>> +	if (priv->phyaddr == -1) {
>> +		/* detect the PHY address */
>> +		for (i = 31; i >= 0; i--) {
>> +			phyreg = phyread(dev, i, PHY_DETECT_REG);
>> +
>> +			if ((phyreg != 0xFFFF) &&
>> +			((phyreg & PHY_DETECT_MASK) == PHY_DETECT_MASK)) {
>> +				/* Found a valid PHY address */
>> +				priv->phyaddr = i;
>> +				debug("Found valid phy address, %d\n", phyreg);
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* 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);
>> +
>> +	switch(phydev->speed) {
>> +	case 1000:
>> +		speed = XAE_EMMC_LINKSPD_1000;
>> +		break;
>> +	case 100:
>> +		speed = XAE_EMMC_LINKSPD_100;
>> +		break;
>> +	case 10:
>> +		speed = XAE_EMMC_LINKSPD_10;
>> +		break;
>> +	default:
>> +		return 0;
>> +	}
>> +
>> +	/* Setup the emac for the phy speed */
>> +	emmc_reg = aximac_in32(dev->iobase, XAE_EMMC_OFFSET);
>> +	emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
>> +	emmc_reg |= speed;
>> +
>> +	/* Write new speed setting out to Axi Ethernet */
>> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
> 
> Use clrsetbits() here.

Not defined for Microblaze - just for ARM/PPC.
Not going to use it.

> 
>> +
>> +	/*
>> +	* Setting the operating speed of the MAC needs a delay. There
>> +	* doesn't seem to be register to poll, so please consider this
>> +	* during your application design.
>> +	*/
>> +	udelay(1);
>> +
>> +	return 1;
>> +#else
>> +	puts("Enable PHYLIB support!\n");
> 
> Compile time warning at the top of the file please.

Fixed - the same for ll_temac.

> 
>> +	return 0;
>> +#endif
>> +}
>> +
>> +/* STOP DMA transfers */
>> +static void axiemac_halt(struct eth_device *dev)
>> +{
>> +	struct axidma_priv *priv = dev->priv;
>> +
>> +	/* Stop the hardware */
>> +	priv->dmatx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;
>> +	priv->dmarx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;
>> +
>> +	debug("axiemac halted\n");
>> +}
>> +
>> +static void axi_ethernet_init(struct eth_device *dev)
>> +{
>> +	/*
>> +	 * Check the status of the MgtRdy bit in the interrupt status
>> +	 * registers. This must be done to allow the MGT clock to become stable
>> +	 * for the Sgmii and 1000BaseX PHY interfaces. No other register reads
>> +	 * will be valid until this bit is valid.
>> +	 * The bit is always a 1 for all other PHY interfaces.
>> +	 */
>> +	u32 timeout = 200;
>> +	while (timeout && (!(aximac_in32(dev->iobase, XAE_IS_OFFSET) &
>> +							XAE_INT_MGTRDY_MASK)))
>> +		timeout--;
>> +
>> +	if (!timeout)
>> +		printf("%s: Timeout\n", __func__);
> 
> I think you don't want to continue in case of timeout ?

fixed.

> 
>> +
>> +	/* Stop the device and reset HW */
>> +	/* Disable interrupts */
>> +	aximac_out32(dev->iobase, XAE_IE_OFFSET, 0);
>> +
>> +	/* Disable the receiver */
>> +	aximac_out32(dev->iobase, XAE_RCW1_OFFSET,
>> +		aximac_in32(dev->iobase, XAE_RCW1_OFFSET) & ~XAE_RCW1_RX_MASK);
>> +
>> +	/*
>> +	 * Stopping the receiver in mid-packet causes a dropped packet
>> +	 * indication from HW. Clear it.
>> +	 */
>> +	/* set the interrupt status register to clear the interrupt */
>> +	aximac_out32(dev->iobase, XAE_IS_OFFSET, XAE_INT_RXRJECT_MASK);
>> +
>> +	/* Setup HW */
>> +	/* Set default MDIO divisor */
>> +	aximac_out32(dev->iobase, XAE_MDIO_MC_OFFSET,
>> +			(u32) XAE_MDIO_DIV_DFT | XAE_MDIO_MC_MDIOEN_MASK);
>> +
>> +	debug("XAxiEthernet InitHw: done\n");
> 
> Unify the debuging message please.

good point.

> 
>> +}
>> +
>> +static void setup_mac(struct eth_device *dev)
>> +{
>> +	/* Set the MAC address */
>> +	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
>> +		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
>> +	aximac_out32(dev->iobase, XAE_UAW0_OFFSET, val);
>> +
>> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
>> +	val |= aximac_in32(dev->iobase, XAE_UAW1_OFFSET) &
>> +						~XAE_UAW1_UNICASTADDR_MASK;
>> +	aximac_out32(dev->iobase, XAE_UAW1_OFFSET, val);
> 
> clrsetbits()

same as above.

> 
>> +}
>> +
>> +/* Reset DMA engine */
>> +static void axi_dma_init(struct eth_device *dev)
>> +{
>> +	struct axidma_priv *priv = dev->priv;
>> +
>> +	/* Reset the engine so the hardware starts from a known state */
>> +	priv->dmatx->control = XAXIDMA_CR_RESET_MASK;
>> +	priv->dmarx->control = XAXIDMA_CR_RESET_MASK;
>> +
>> +	/* At the initialization time, hardware should finish reset quickly */
>> +	u32 timeout = 500;
>> +	while (timeout--) {
>> +		/* Check transmit/receive channel */
>> +		/* Reset is done when the reset bit is low */
>> +		if (!(priv->dmatx->control | priv->dmarx->control)
>> +						& XAXIDMA_CR_RESET_MASK)
>> +			break;
>> +		timeout -= 1;
> 
> You're decrementing it twice in here.

thanks.

> 
>> +	}
>> +
>> +	if (!timeout)
>> +		printf("%s: Timeout\n", __func__);
>> +}
>> +
>> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
>> +{
>> +	struct axidma_priv *priv = dev->priv;
>> +	debug("axi emac init started\n");
>> +
>> +	/*
>> +	 * Initialize AXIDMA engine. AXIDMA engine must be initialized before
>> +	 * AxiEthernet. During AXIDMA engine initialization, AXIDMA hardware is
>> +	 * reset, and since AXIDMA reset line is connected to AxiEthernet, this
>> +	 * would ensure a reset of AxiEthernet.
>> +	 */
>> +	axi_dma_init(dev);
>> +
>> +	/* Initialize AxiEthernet hardware. */
>> +	axi_ethernet_init(dev);
>> +	setup_mac(dev);
>> +
>> +	/* Start the hardware */
>> +	priv->dmarx->control |= XAXIDMA_CR_RUNSTOP_MASK;
>> +	/* Start DMA RX channel. Now it's ready to receive data.*/
>> +	priv->dmarx->current = (u32)&rx_bd;
>> +
>> +	/* Disable all RX interrupts before RxBD space setup */
>> +	priv->dmarx->control &= ~(XAXIDMA_IRQ_ALL_MASK & XAXIDMA_IRQ_ALL_MASK);
> 
> I don't think I understand the above code.

There is one more XAXIDMA_IRQ_ALL_MASK. There is longer history above it.
Fixed.

> 
>> +
>> +	/* Setup the BD. */
>> +	memset((void *) &rx_bd, 0, sizeof(rx_bd));
>> +	rx_bd.next = (u32)&rx_bd;
>> +	rx_bd.phys = (u32)&RxFrame;
>> +	rx_bd.cntrl = sizeof(RxFrame);
> 
> please get rid of the cammelcase.

done

> 
>> +	/* Flush the last BD so DMA core could see the updates */
>> +	flush_cache((u32)&rx_bd, sizeof(rx_bd));
>> +
>> +	/* it is necessary to flush RxFrame because if you don't do it
>> +	 * then cache can contain uninitialized data */
>> +	flush_cache((u32)&RxFrame, sizeof(RxFrame));
>> +
>> +	/* Rx BD is ready - start */
>> +	priv->dmarx->tail = (u32)&rx_bd;
>> +
>> +	/* enable TX */
>> +	aximac_out32(dev->iobase, XAE_TC_OFFSET, XAE_TC_TX_MASK);
>> +	/* enable RX */
>> +	aximac_out32(dev->iobase, XAE_RCW1_OFFSET, XAE_RCW1_RX_MASK);
>> +
> 
> [...]

above.

> 
>> +static int axiemac_recv(struct eth_device *dev)
>> +{
>> +	u32 length;
>> +	struct axidma_priv *priv = dev->priv;
>> +
>> +	/* wait for an incoming packet */
>> +	if (!IsRxReady(dev))
>> +		return 0;
>> +
>> +	debug("axi emac, rx data ready\n");
>> +
>> +	/* Disable IRQ for a moment till packet is handled */
>> +	priv->dmarx->control &= ~(XAXIDMA_IRQ_ALL_MASK & XAXIDMA_IRQ_ALL_MASK);
>> +
>> +	length = rx_bd.app4 & 0x0000FFFF;
> 
> Get rid of the strange constants please.

done.

> 
>> +#ifdef DEBUG
>> +	print_buffer(&RxFrame, &RxFrame[0], 1, length, 16);
>> +#endif
>> +	/* pass the received frame up for processing */
>> +	if (length)
>> +		NetReceive(RxFrame, length);
>> +
>> +#ifdef DEBUG
>> +	/* It is useful to clear buffer to be sure that it is consistent */
>> +	memset(RxFrame, 0, sizeof(RxFrame));
>> +#endif
>> +	/* Setup RxBD */
> [...]

Thanks for your comments,
Michal
Marek Vasut Aug. 31, 2011, 3:19 p.m. UTC | #3
On Wednesday, August 31, 2011 04:46:22 PM Michal Simek wrote:
> Marek Vasut wrote:
> > On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
> >> Add the first axi_ethernet driver for little-endian Microblaze.
> >> 
> >> Signed-off-by: Michal Simek <monstr@monstr.eu>
> >> ---
> >> 
> >>  drivers/net/Makefile          |    1 +
> >>  drivers/net/xilinx_axi_emac.c |  622
> >> 
> >> +++++++++++++++++++++++++++++++++++++++++ include/netdev.h             
> >> |
> >> 
> >>   1 +
> >>  
> >>  3 files changed, 624 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/net/xilinx_axi_emac.c
> >> 
> >> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> >> index 4541eaf..ae9d4cb 100644
> >> --- a/drivers/net/Makefile
> >> +++ b/drivers/net/Makefile
> >> @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o
> >> 
> >>  COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
> >>  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_LL_TEMAC) += xilinx_ll_temac.o
> >> 
> >> diff --git a/drivers/net/xilinx_axi_emac.c
> >> b/drivers/net/xilinx_axi_emac.c new file mode 100644
> >> index 0000000..ce79b80
> >> --- /dev/null
> >> +++ b/drivers/net/xilinx_axi_emac.c
> >> @@ -0,0 +1,622 @@
> >> +/*
> >> + * Copyright (C) 2011 Michal Simek <monstr@monstr.eu>
> >> + * Copyright (C) 2011 PetaLogix
> >> + * Copyright (C) 2010 Xilinx, Inc. All rights reserved.
> >> + *
> >> + * 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 <config.h>
> >> +#include <common.h>
> >> +#include <net.h>
> >> +#include <malloc.h>
> >> +#include <asm/io.h>
> >> +#include <phy.h>
> >> +#include <miiphy.h>
> >> +
> >> +/* Axi Ethernet registers offset */
> >> +#define XAE_IS_OFFSET		0x0000000C /* Interrupt status */
> >> +#define XAE_IE_OFFSET		0x00000014 /* Interrupt enable */
> >> +#define XAE_RCW1_OFFSET		0x00000404 /* Rx Configuration Word 1 */
> >> +#define XAE_TC_OFFSET		0x00000408 /* Tx Configuration */
> >> +#define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
> >> +#define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
> >> +#define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
> >> +#define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data */
> >> +#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read Data */
> > 
> > Please use struct xae_regs {...} as the rest of the u-boot.
> 
> struct is not useful here because it will be big with a lot of reserved
> values.

I see like 10 registers here, you can use "uint32_t reserved[n];"

> 
> >> +
> >> +/* Link setup */
> >> +#define XAE_EMMC_LINKSPEED_MASK	0xC0000000 /* Link speed */
> >> +#define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 Mbit
> >> */ +#define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for 100
> >> Mbit */ +#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask
> >> for 1000 Mbit */ +
> > 
> > Use (1 << n) ?
> 
> just another solution - I prefer to use 32bit value - easier when you
> decode it by md.

It's hard to read, really.

> 
> >> +/* Interrupt Status/Enable/Mask Registers bit definitions */
> >> +#define XAE_INT_RXRJECT_MASK	0x00000008 /* Rx frame rejected */
> >> +#define XAE_INT_MGTRDY_MASK	0x00000080 /* MGT clock Lock */
> >> +
> > 
> > [...]
> 
> same here..
> 
> >> +#define DMAALIGN	128
> >> +
> >> +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
> > 
> > Don't use cammelcase, all lowcase please.
> 
> Agree - will be done in v2.
> 
>   Also, can't you allocate this with
> 
> > memalign and hide it in axidma_priv or something ?
> 
> There are two things in one.
> 1. Adding it to axidma_priv means that I will need to dynamically allocate
> big private structure which is bad thing to do for several eth devices.
> This is FPGA you can create a lot of MACs that's why I think it is better
> not to do so.
> 2. Current style is sharing this rxframe buffer among all controllers
> because only one MAC works. Others are stopped which means that no packet
> come to them.

Ok, I don't think I understand pt. 1. -- you need to keep the state for each of 
the ethernet devices on the FPGA separate, don't you. As for pt. 2. -- 
"currently", so there's possibility, in future this won't hold?

> 
> BTW: Looking for that memalign function - thanks.
> 
> >> +
> >> +/* reflect dma offsets */
> >> +struct axidma_reg {
> >> +	u32 control; /* DMACR */
> >> +	u32 status; /* DMASR */
> >> +	u32 current; /* CURDESC */
> >> +	u32 reserved;
> >> +	u32 tail; /* TAILDESC */
> >> +};
> >> +
> >> +/* Private driver structures */
> >> +struct axidma_priv {
> >> +	struct axidma_reg *dmatx;
> >> +	struct axidma_reg *dmarx;
> >> +	int phyaddr;
> >> +
> >> +	struct phy_device *phydev;
> >> +	struct mii_dev *bus;
> >> +};
> >> +
> >> +/* BD descriptors */
> >> +struct axidma_bd {
> >> +	u32 next;	/* Next descriptor pointer */
> >> +	u32 reserved1;
> >> +	u32 phys;	/* Buffer address */
> >> +	u32 reserved2;
> >> +	u32 reserved3;
> >> +	u32 reserved4;
> >> +	u32 cntrl;	/* Control */
> >> +	u32 status;	/* Status */
> >> +	u32 app0;
> >> +	u32 app1;	/* TX start << 16 | insert */
> >> +	u32 app2;	/* TX csum seed */
> >> +	u32 app3;
> >> +	u32 app4;
> >> +	u32 sw_id_offset;
> >> +	u32 reserved5;
> >> +	u32 reserved6;
> >> +};
> >> +
> >> +/* Static BDs - driver uses only one BD */
> >> +static struct axidma_bd tx_bd __attribute((aligned(DMAALIGN)));
> >> +static struct axidma_bd rx_bd __attribute((aligned(DMAALIGN)));
> >> +
> >> +static inline void aximac_out32(u32 addr, u32 offset, u32 val)
> >> +{
> >> +	out_be32((u32 *)(addr + offset), val);
> > 
> > Please fix these casts ... though I don't think you even need these
> > functions.
> 
> Cast is necessary. I use that helper just because of recast.
> If you see solution which will be elegant, I am open to use it.

See above -- use the structure and then just use &axiregs->reg.

> 
> >> +}
> >> +
> >> +static inline u32 aximac_in32(u32 addr, u32 offset)
> >> +{
> >> +	return in_be32((u32 *)(addr + offset));
> >> +}
> >> +
> >> +
> >> +/* Use MII register 1 (MII status register) to detect PHY */
> >> +#define PHY_DETECT_REG  1

[...]

> >> +	/* Write new speed setting out to Axi Ethernet */
> >> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
> > 
> > Use clrsetbits() here.
> 
> Not defined for Microblaze - just for ARM/PPC.
> Not going to use it.

Please fix then. You're the microblaze maintainer, right?

[...]

Cheers!
Mike Frysinger Aug. 31, 2011, 7:24 p.m. UTC | #4
On Tuesday, August 30, 2011 08:05:19 Michal Simek wrote:
> +static void setup_mac(struct eth_device *dev)
> +{
> +	/* Set the MAC address */
> +	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
> +		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
> +	aximac_out32(dev->iobase, XAE_UAW0_OFFSET, val);
> +
> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
> +	val |= aximac_in32(dev->iobase, XAE_UAW1_OFFSET) &
> +						~XAE_UAW1_UNICASTADDR_MASK;
> +	aximac_out32(dev->iobase, XAE_UAW1_OFFSET, val);
> +}
> +
> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
> +{
> +	setup_mac(dev);

pretty sure this should be dev->write_hwaddr

> +int xilinx_axiemac_initialize(bd_t *bis, unsigned long base_addr, int
> dma_addr)

you got base_addr right, but forgot to change dma_addr to unsigned long too ;)

otherwise it seems that Marek covered much of what i would have suggested
-mike
Wolfgang Denk Aug. 31, 2011, 8:13 p.m. UTC | #5
Dear Michal Simek,

In message <4E5E493E.3070104@monstr.eu> you wrote:
>
> >> +/* Axi Ethernet registers offset */
> >> +#define XAE_IS_OFFSET		0x0000000C /* Interrupt status */
> >> +#define XAE_IE_OFFSET		0x00000014 /* Interrupt enable */
> >> +#define XAE_RCW1_OFFSET		0x00000404 /* Rx Configuration Word 1 */
> >> +#define XAE_TC_OFFSET		0x00000408 /* Tx Configuration */
> >> +#define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
> >> +#define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
> >> +#define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
> >> +#define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data */
> >> +#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read Data */
> > 
> > Please use struct xae_regs {...} as the rest of the u-boot.
> 
> struct is not useful here because it will be big with a lot of reserved values.

Code based on base address + offset notation will not be accepted.
Please do use a struct as requested.

Best regards,

Wolfgang Denk
Michal Simek Sept. 1, 2011, 6:52 a.m. UTC | #6
Mike Frysinger wrote:
> On Tuesday, August 30, 2011 08:05:19 Michal Simek wrote:
>> +static void setup_mac(struct eth_device *dev)
>> +{
>> +	/* Set the MAC address */
>> +	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
>> +		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
>> +	aximac_out32(dev->iobase, XAE_UAW0_OFFSET, val);
>> +
>> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
>> +	val |= aximac_in32(dev->iobase, XAE_UAW1_OFFSET) &
>> +						~XAE_UAW1_UNICASTADDR_MASK;
>> +	aximac_out32(dev->iobase, XAE_UAW1_OFFSET, val);
>> +}
>> +
>> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
>> +{
>> +	setup_mac(dev);
> 
> pretty sure this should be dev->write_hwaddr

I expected it. The point is where dev->write_hwaddr is called.

I mean sequence is:
dev->write_hwaddr
u-boot network command
dev->init

If init reset device include setup mac then this sequence can't work.
But not case for my MAC - I can use it.

> 
>> +int xilinx_axiemac_initialize(bd_t *bis, unsigned long base_addr, int
>> dma_addr)
> 
> you got base_addr right, but forgot to change dma_addr to unsigned long too ;)

Fixed.

Michal
Michal Simek Sept. 1, 2011, 7:17 a.m. UTC | #7
Marek Vasut wrote:
> On Wednesday, August 31, 2011 04:46:22 PM Michal Simek wrote:
>> Marek Vasut wrote:
>>> On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
>>>> Add the first axi_ethernet driver for little-endian Microblaze.
>>>>
>>>> Signed-off-by: Michal Simek <monstr@monstr.eu>
>>>> ---
>>>>
>>>>  drivers/net/Makefile          |    1 +
>>>>  drivers/net/xilinx_axi_emac.c |  622
>>>>
>>>> +++++++++++++++++++++++++++++++++++++++++ include/netdev.h             
>>>> |
>>>>
>>>>   1 +
>>>>  
>>>>  3 files changed, 624 insertions(+), 0 deletions(-)
>>>>  create mode 100644 drivers/net/xilinx_axi_emac.c
>>>>
>>>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>>>> index 4541eaf..ae9d4cb 100644
>>>> --- a/drivers/net/Makefile
>>>> +++ b/drivers/net/Makefile
>>>> @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o
>>>>
>>>>  COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
>>>>  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_LL_TEMAC) += xilinx_ll_temac.o
>>>>
>>>> diff --git a/drivers/net/xilinx_axi_emac.c
>>>> b/drivers/net/xilinx_axi_emac.c new file mode 100644
>>>> index 0000000..ce79b80
>>>> --- /dev/null
>>>> +++ b/drivers/net/xilinx_axi_emac.c
>>>> @@ -0,0 +1,622 @@
>>>> +/*
>>>> + * Copyright (C) 2011 Michal Simek <monstr@monstr.eu>
>>>> + * Copyright (C) 2011 PetaLogix
>>>> + * Copyright (C) 2010 Xilinx, Inc. All rights reserved.
>>>> + *
>>>> + * 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 <config.h>
>>>> +#include <common.h>
>>>> +#include <net.h>
>>>> +#include <malloc.h>
>>>> +#include <asm/io.h>
>>>> +#include <phy.h>
>>>> +#include <miiphy.h>
>>>> +
>>>> +/* Axi Ethernet registers offset */
>>>> +#define XAE_IS_OFFSET		0x0000000C /* Interrupt status */
>>>> +#define XAE_IE_OFFSET		0x00000014 /* Interrupt enable */
>>>> +#define XAE_RCW1_OFFSET		0x00000404 /* Rx Configuration Word 1 */
>>>> +#define XAE_TC_OFFSET		0x00000408 /* Tx Configuration */
>>>> +#define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
>>>> +#define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
>>>> +#define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
>>>> +#define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data */
>>>> +#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read Data */
>>> Please use struct xae_regs {...} as the rest of the u-boot.
>> struct is not useful here because it will be big with a lot of reserved
>> values.
> 
> I see like 10 registers here, you can use "uint32_t reserved[n];"

+ UAW0 and UAW1 with 0x700 offset.

struct axi_ethernet {
	u32 reserved[3];
	u32 is; /* 0xC: Interrupt status */
	u32 reserved2;
	u32 ie; /* 0x14: Interrupt enable */
	u32 reserved3[251];
	u32 rcw1; /* 0x404: Rx Configuration Word 1 */
	u32 tc; /* 0x408: Tx Configuration */
	u32 reserved4;
	u32 emmc; /* 0x410: EMAC mode configuration */
	u32 reserved5[59];
	u32 mdio_mc; /* 0x500: MII Management Config */
	u32 mdio_mcr; /* 0x504: MII Management Control */
	u32 mdio_mwd; /* 0x508: MII Management Write Data */
	u32 mdio_mrd; /* 0x50C: MII Management Read Data */
	u32 reserved6[124];
	u32 uaw0; /* 0x700: Unicast address word 0 */
	u32 uaw1; /* 0x704: Unicast address word 1 */
};

Are you really sure that this is nice?


> 
>>>> +
>>>> +/* Link setup */
>>>> +#define XAE_EMMC_LINKSPEED_MASK	0xC0000000 /* Link speed */
>>>> +#define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 Mbit
>>>> */ +#define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for 100
>>>> Mbit */ +#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask
>>>> for 1000 Mbit */ +
>>> Use (1 << n) ?
>> just another solution - I prefer to use 32bit value - easier when you
>> decode it by md.
> 
> It's hard to read, really.

At offset 40b40410. It is really hard to decode it if values are (1 << n).
With defined macro you directly see that this is on 100Mbit/s LAN.

U-Boot-mONStR> md 40b40400
40b40400: ddccbbaa 6800ffee 4a000000 60000000    .......h...J...`
40b40410: 40000000 00000000 000ce000 000ce000    ...@............
40b40420: 000ce000 000ce000 000ce000 000ce000    ................
40b40430: 000ce000 000ce000 000ce000 000ce000    ................

I am fine to use 1 << n solution but definitely in our repo I will use in way I like.

> 
>>>> +/* Interrupt Status/Enable/Mask Registers bit definitions */
>>>> +#define XAE_INT_RXRJECT_MASK	0x00000008 /* Rx frame rejected */
>>>> +#define XAE_INT_MGTRDY_MASK	0x00000080 /* MGT clock Lock */
>>>> +
>>> [...]
>> same here..
>>
>>>> +#define DMAALIGN	128
>>>> +
>>>> +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
>>> Don't use cammelcase, all lowcase please.
>> Agree - will be done in v2.
>>
>>   Also, can't you allocate this with
>>
>>> memalign and hide it in axidma_priv or something ?
>> There are two things in one.
>> 1. Adding it to axidma_priv means that I will need to dynamically allocate
>> big private structure which is bad thing to do for several eth devices.
>> This is FPGA you can create a lot of MACs that's why I think it is better
>> not to do so.
>> 2. Current style is sharing this rxframe buffer among all controllers
>> because only one MAC works. Others are stopped which means that no packet
>> come to them.
> 
> Ok, I don't think I understand pt. 1. -- you need to keep the state for each of 
> the ethernet devices on the FPGA separate, don't you.

Just the whole private structure with addresses, + phy.


  As for pt. 2. --
> "currently", so there's possibility, in future this won't hold?

BTW: I am also sharing rx/tx buffer descriptors for dma.

When do you expect that u-boot will be able to use several MACs in one time?

> 
>> BTW: Looking for that memalign function - thanks.
>>
>>>> +
>>>> +/* reflect dma offsets */
>>>> +struct axidma_reg {
>>>> +	u32 control; /* DMACR */
>>>> +	u32 status; /* DMASR */
>>>> +	u32 current; /* CURDESC */
>>>> +	u32 reserved;
>>>> +	u32 tail; /* TAILDESC */
>>>> +};
>>>> +
>>>> +/* Private driver structures */
>>>> +struct axidma_priv {
>>>> +	struct axidma_reg *dmatx;
>>>> +	struct axidma_reg *dmarx;
>>>> +	int phyaddr;
>>>> +
>>>> +	struct phy_device *phydev;
>>>> +	struct mii_dev *bus;
>>>> +};
>>>> +
>>>> +/* BD descriptors */
>>>> +struct axidma_bd {
>>>> +	u32 next;	/* Next descriptor pointer */
>>>> +	u32 reserved1;
>>>> +	u32 phys;	/* Buffer address */
>>>> +	u32 reserved2;
>>>> +	u32 reserved3;
>>>> +	u32 reserved4;
>>>> +	u32 cntrl;	/* Control */
>>>> +	u32 status;	/* Status */
>>>> +	u32 app0;
>>>> +	u32 app1;	/* TX start << 16 | insert */
>>>> +	u32 app2;	/* TX csum seed */
>>>> +	u32 app3;
>>>> +	u32 app4;
>>>> +	u32 sw_id_offset;
>>>> +	u32 reserved5;
>>>> +	u32 reserved6;
>>>> +};
>>>> +
>>>> +/* Static BDs - driver uses only one BD */
>>>> +static struct axidma_bd tx_bd __attribute((aligned(DMAALIGN)));
>>>> +static struct axidma_bd rx_bd __attribute((aligned(DMAALIGN)));
>>>> +
>>>> +static inline void aximac_out32(u32 addr, u32 offset, u32 val)
>>>> +{
>>>> +	out_be32((u32 *)(addr + offset), val);
>>> Please fix these casts ... though I don't think you even need these
>>> functions.
>> Cast is necessary. I use that helper just because of recast.
>> If you see solution which will be elegant, I am open to use it.
> 
> See above -- use the structure and then just use &axiregs->reg.
> 
>>>> +}
>>>> +
>>>> +static inline u32 aximac_in32(u32 addr, u32 offset)
>>>> +{
>>>> +	return in_be32((u32 *)(addr + offset));
>>>> +}
>>>> +
>>>> +
>>>> +/* Use MII register 1 (MII status register) to detect PHY */
>>>> +#define PHY_DETECT_REG  1
> 
> [...]
> 
>>>> +	/* Write new speed setting out to Axi Ethernet */
>>>> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
>>> Use clrsetbits() here.
>> Not defined for Microblaze - just for ARM/PPC.
>> Not going to use it.
> 
> Please fix then. You're the microblaze maintainer, right?

Custodian. But I won't do that.
If you think that all archs should have it then move it to generic location
which clean code duplication and I will include it.

Thanks,
Michal
Marek Vasut Sept. 1, 2011, 8:18 a.m. UTC | #8
On Thursday, September 01, 2011 09:17:35 AM Michal Simek wrote:
> Marek Vasut wrote:
> > On Wednesday, August 31, 2011 04:46:22 PM Michal Simek wrote:
> >> Marek Vasut wrote:
> >>> On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
> >>>> Add the first axi_ethernet driver for little-endian Microblaze.
> >>>> 
> >>>> Signed-off-by: Michal Simek <monstr@monstr.eu>
> >>>> ---
> >>>> 

[...]

> >>>> +
> >>>> +/* Axi Ethernet registers offset */
> >>>> +#define XAE_IS_OFFSET		0x0000000C /* Interrupt status */
> >>>> +#define XAE_IE_OFFSET		0x00000014 /* Interrupt enable */
> >>>> +#define XAE_RCW1_OFFSET		0x00000404 /* Rx Configuration Word 1 */
> >>>> +#define XAE_TC_OFFSET		0x00000408 /* Tx Configuration */
> >>>> +#define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
> >>>> +#define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
> >>>> +#define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
> >>>> +#define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data
> >>>> */ +#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read
> >>>> Data */
> >>> 
> >>> Please use struct xae_regs {...} as the rest of the u-boot.
> >> 
> >> struct is not useful here because it will be big with a lot of reserved
> >> values.
> > 
> > I see like 10 registers here, you can use "uint32_t reserved[n];"
> 
> + UAW0 and UAW1 with 0x700 offset.
> 
> struct axi_ethernet {
> 	u32 reserved[3];
> 	u32 is; /* 0xC: Interrupt status */
> 	u32 reserved2;
> 	u32 ie; /* 0x14: Interrupt enable */
> 	u32 reserved3[251];
> 	u32 rcw1; /* 0x404: Rx Configuration Word 1 */
> 	u32 tc; /* 0x408: Tx Configuration */
> 	u32 reserved4;
> 	u32 emmc; /* 0x410: EMAC mode configuration */
> 	u32 reserved5[59];
> 	u32 mdio_mc; /* 0x500: MII Management Config */
> 	u32 mdio_mcr; /* 0x504: MII Management Control */
> 	u32 mdio_mwd; /* 0x508: MII Management Write Data */
> 	u32 mdio_mrd; /* 0x50C: MII Management Read Data */
> 	u32 reserved6[124];
> 	u32 uaw0; /* 0x700: Unicast address word 0 */
> 	u32 uaw1; /* 0x704: Unicast address word 1 */
> };
> 
> Are you really sure that this is nice?
> 

Yep, definitelly great.

> >>>> +
> >>>> +/* Link setup */
> >>>> +#define XAE_EMMC_LINKSPEED_MASK	0xC0000000 /* Link speed */
> >>>> +#define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 
Mbit
> >>>> */ +#define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for 100
> >>>> Mbit */ +#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask
> >>>> for 1000 Mbit */ +
> >>> 
> >>> Use (1 << n) ?
> >> 
> >> just another solution - I prefer to use 32bit value - easier when you
> >> decode it by md.
> > 
> > It's hard to read, really.
> 
> At offset 40b40410. It is really hard to decode it if values are (1 << n).
> With defined macro you directly see that this is on 100Mbit/s LAN.
> 
> U-Boot-mONStR> md 40b40400
> 40b40400: ddccbbaa 6800ffee 4a000000 60000000    .......h...J...`
> 40b40410: 40000000 00000000 000ce000 000ce000    ...@............
> 40b40420: 000ce000 000ce000 000ce000 000ce000    ................
> 40b40430: 000ce000 000ce000 000ce000 000ce000    ................
> 
> I am fine to use 1 << n solution but definitely in our repo I will use in
> way I like.

Well I see it both ways ... 0x40000000 == 1 << 30 ... it's the same thing. On 
the other note, it's hard to count the zeroes in there AND you can mistake 0 and 
8 in a huge series of those.

Also, you can have whatever you want in your repo if you seriously care to 
invest the energy into maintaining it just because you need to be stubborn. But 
it'd really be great if you invested that energy in a more productive manner ;-)

> 
> >>>> +/* Interrupt Status/Enable/Mask Registers bit definitions */
> >>>> +#define XAE_INT_RXRJECT_MASK	0x00000008 /* Rx frame rejected */
> >>>> +#define XAE_INT_MGTRDY_MASK	0x00000080 /* MGT clock Lock */
> >>>> +
> >>> 
> >>> [...]
> >> 
> >> same here..

Yep, 1 << 3 and 1 << 7.

> >> 
> >>>> +#define DMAALIGN	128
> >>>> +
> >>>> +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
> >>> 
> >>> Don't use cammelcase, all lowcase please.
> >> 
> >> Agree - will be done in v2.
> >> 
> >>   Also, can't you allocate this with
> >>> 
> >>> memalign and hide it in axidma_priv or something ?
> >> 
> >> There are two things in one.
> >> 1. Adding it to axidma_priv means that I will need to dynamically
> >> allocate big private structure which is bad thing to do for several eth
> >> devices. This is FPGA you can create a lot of MACs that's why I think
> >> it is better not to do so.
> >> 2. Current style is sharing this rxframe buffer among all controllers
> >> because only one MAC works. Others are stopped which means that no
> >> packet come to them.
> > 
> > Ok, I don't think I understand pt. 1. -- you need to keep the state for
> > each of the ethernet devices on the FPGA separate, don't you.
> 
> Just the whole private structure with addresses, + phy.
> 

Ok, now after reading your explanation for pt.2 below, I'm starting to follow.

> 
>   As for pt. 2. --
> 
> > "currently", so there's possibility, in future this won't hold?
> 
> BTW: I am also sharing rx/tx buffer descriptors for dma.
> 
> When do you expect that u-boot will be able to use several MACs in one
> time?

It's not a matter of when, but -- write a correct code, it's much less burden to 
fix it later.

[...]
> > 
> >>>> +	/* Write new speed setting out to Axi Ethernet */
> >>>> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
> >>> 
> >>> Use clrsetbits() here.
> >> 
> >> Not defined for Microblaze - just for ARM/PPC.
> >> Not going to use it.
> > 
> > Please fix then. You're the microblaze maintainer, right?
> 
> Custodian.

Oh come on ...

> But I won't do that.

I think you should.

> If you think that all archs should have it then move it to generic location
> which clean code duplication and I will include it.

That's not the point, it's platform specific.

> 
> Thanks,
> Michal

Cheers
Michal Simek Sept. 1, 2011, 8:55 a.m. UTC | #9
>>>>>> +
>>>>>> +/* Axi Ethernet registers offset */
>>>>>> +#define XAE_IS_OFFSET		0x0000000C /* Interrupt status */
>>>>>> +#define XAE_IE_OFFSET		0x00000014 /* Interrupt enable */
>>>>>> +#define XAE_RCW1_OFFSET		0x00000404 /* Rx Configuration Word 1 */
>>>>>> +#define XAE_TC_OFFSET		0x00000408 /* Tx Configuration */
>>>>>> +#define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
>>>>>> +#define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
>>>>>> +#define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
>>>>>> +#define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data
>>>>>> */ +#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read
>>>>>> Data */
>>>>> Please use struct xae_regs {...} as the rest of the u-boot.
>>>> struct is not useful here because it will be big with a lot of reserved
>>>> values.
>>> I see like 10 registers here, you can use "uint32_t reserved[n];"
>> + UAW0 and UAW1 with 0x700 offset.
>>
>> struct axi_ethernet {
>> 	u32 reserved[3];
>> 	u32 is; /* 0xC: Interrupt status */
>> 	u32 reserved2;
>> 	u32 ie; /* 0x14: Interrupt enable */
>> 	u32 reserved3[251];
>> 	u32 rcw1; /* 0x404: Rx Configuration Word 1 */
>> 	u32 tc; /* 0x408: Tx Configuration */
>> 	u32 reserved4;
>> 	u32 emmc; /* 0x410: EMAC mode configuration */
>> 	u32 reserved5[59];
>> 	u32 mdio_mc; /* 0x500: MII Management Config */
>> 	u32 mdio_mcr; /* 0x504: MII Management Control */
>> 	u32 mdio_mwd; /* 0x508: MII Management Write Data */
>> 	u32 mdio_mrd; /* 0x50C: MII Management Read Data */
>> 	u32 reserved6[124];
>> 	u32 uaw0; /* 0x700: Unicast address word 0 */
>> 	u32 uaw1; /* 0x704: Unicast address word 1 */
>> };
>>
>> Are you really sure that this is nice?
>>
> 
> Yep, definitelly great.

ok.

> 
>>>>>> +
>>>>>> +/* Link setup */
>>>>>> +#define XAE_EMMC_LINKSPEED_MASK	0xC0000000 /* Link speed */
>>>>>> +#define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 
> Mbit
>>>>>> */ +#define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for 100
>>>>>> Mbit */ +#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask
>>>>>> for 1000 Mbit */ +
>>>>> Use (1 << n) ?
>>>> just another solution - I prefer to use 32bit value - easier when you
>>>> decode it by md.
>>> It's hard to read, really.
>> At offset 40b40410. It is really hard to decode it if values are (1 << n).
>> With defined macro you directly see that this is on 100Mbit/s LAN.
>>
>> U-Boot-mONStR> md 40b40400
>> 40b40400: ddccbbaa 6800ffee 4a000000 60000000    .......h...J...`
>> 40b40410: 40000000 00000000 000ce000 000ce000    ...@............
>> 40b40420: 000ce000 000ce000 000ce000 000ce000    ................
>> 40b40430: 000ce000 000ce000 000ce000 000ce000    ................
>>
>> I am fine to use 1 << n solution but definitely in our repo I will use in
>> way I like.
> 
> Well I see it both ways ... 0x40000000 == 1 << 30 ... it's the same thing. On 
> the other note, it's hard to count the zeroes in there AND you can mistake 0 and 
> 8 in a huge series of those.
> 
> Also, you can have whatever you want in your repo if you seriously care to 
> invest the energy into maintaining it just because you need to be stubborn. But 
> it'd really be great if you invested that energy in a more productive manner ;-)

There are two points of view. And both have con & pro.
I don't want to argue. Net custodian should decided if is OK or not.

Look at tsec.h and probably others.


> 
>>>>>> +/* Interrupt Status/Enable/Mask Registers bit definitions */
>>>>>> +#define XAE_INT_RXRJECT_MASK	0x00000008 /* Rx frame rejected */
>>>>>> +#define XAE_INT_MGTRDY_MASK	0x00000080 /* MGT clock Lock */
>>>>>> +
>>>>> [...]
>>>> same here..
> 
> Yep, 1 << 3 and 1 << 7.
> 
>>>>>> +#define DMAALIGN	128
>>>>>> +
>>>>>> +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
>>>>> Don't use cammelcase, all lowcase please.
>>>> Agree - will be done in v2.
>>>>
>>>>   Also, can't you allocate this with
>>>>> memalign and hide it in axidma_priv or something ?
>>>> There are two things in one.
>>>> 1. Adding it to axidma_priv means that I will need to dynamically
>>>> allocate big private structure which is bad thing to do for several eth
>>>> devices. This is FPGA you can create a lot of MACs that's why I think
>>>> it is better not to do so.
>>>> 2. Current style is sharing this rxframe buffer among all controllers
>>>> because only one MAC works. Others are stopped which means that no
>>>> packet come to them.
>>> Ok, I don't think I understand pt. 1. -- you need to keep the state for
>>> each of the ethernet devices on the FPGA separate, don't you.
>> Just the whole private structure with addresses, + phy.
>>
> 
> Ok, now after reading your explanation for pt.2 below, I'm starting to follow.
> 
>>   As for pt. 2. --
>>
>>> "currently", so there's possibility, in future this won't hold?
>> BTW: I am also sharing rx/tx buffer descriptors for dma.
>>
>> When do you expect that u-boot will be able to use several MACs in one
>> time?
> 
> It's not a matter of when, but -- write a correct code, it's much less burden to 
> fix it later.

Agree in general.
It is always question of when. You can always do it in better way. The question is if
someone will pay you for doing it in better way. If this feature is not important
for us, make no sense to invest our time/money to it.


>>>>>> +	/* Write new speed setting out to Axi Ethernet */
>>>>>> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
>>>>> Use clrsetbits() here.
>>>> Not defined for Microblaze - just for ARM/PPC.
>>>> Not going to use it.
>>> Please fix then. You're the microblaze maintainer, right?
>> Custodian.
> 
> Oh come on ...
> 
>> But I won't do that.
> 
> I think you should.
> 
>> If you think that all archs should have it then move it to generic location
>> which clean code duplication and I will include it.
> 
> That's not the point, it's platform specific.

Just compare ppc/arm implementation and they are the same. It is even pure generic code.
Adding it to microblaze is just code duplication which is also not good way to go.
Then in future someone will move it to generic location.
There were a lot of examples in linux kernel and includes.

+ network driver should be platform independent which is exactly how it is written.
Writing in the pure C should be fine.

Thanks,
Michal
Marek Vasut Sept. 1, 2011, 10:07 a.m. UTC | #10
On Thursday, September 01, 2011 10:55:31 AM Michal Simek wrote:

[...]

> >> I am fine to use 1 << n solution but definitely in our repo I will use
> >> in way I like.
> > 
> > Well I see it both ways ... 0x40000000 == 1 << 30 ... it's the same
> > thing. On the other note, it's hard to count the zeroes in there AND you
> > can mistake 0 and 8 in a huge series of those.
> > 
> > Also, you can have whatever you want in your repo if you seriously care
> > to invest the energy into maintaining it just because you need to be
> > stubborn. But it'd really be great if you invested that energy in a more
> > productive manner ;-)
> 
> There are two points of view. And both have con & pro.
> I don't want to argue. Net custodian should decided if is OK or not.
> 
> Look at tsec.h and probably others.

That something's in mainline doesn't mean it's obviously correct !

[...]

> >>> 
> >>> "currently", so there's possibility, in future this won't hold?
> >> 
> >> BTW: I am also sharing rx/tx buffer descriptors for dma.
> >> 
> >> When do you expect that u-boot will be able to use several MACs in one
> >> time?
> > 
> > It's not a matter of when, but -- write a correct code, it's much less
> > burden to fix it later.
> 
> Agree in general.
> It is always question of when. You can always do it in better way. The
> question is if someone will pay you for doing it in better way.

And if the code isn't accepted, they won't pay you ;-)

> If this
> feature is not important for us, make no sense to invest our time/money to
> it.

I guess having the code mainline is important ?

> 
> >>>>>> +	/* Write new speed setting out to Axi Ethernet */
> >>>>>> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
> >>>>> 
> >>>>> Use clrsetbits() here.
> >>>> 
> >>>> Not defined for Microblaze - just for ARM/PPC.
> >>>> Not going to use it.
> >>> 
> >>> Please fix then. You're the microblaze maintainer, right?
> >> 
> >> Custodian.
> > 
> > Oh come on ...
> > 
> >> But I won't do that.
> > 
> > I think you should.
> > 
> >> If you think that all archs should have it then move it to generic
> >> location which clean code duplication and I will include it.
> > 
> > That's not the point, it's platform specific.
> 
> Just compare ppc/arm implementation and they are the same. It is even pure
> generic code. Adding it to microblaze is just code duplication which is
> also not good way to go. Then in future someone will move it to generic
> location.
> There were a lot of examples in linux kernel and includes.

Ok, if it's a generic code, please submit a patch putting it into a generic 
place and the fix this driver to use it.

> 
> + network driver should be platform independent which is exactly how it is
> written. Writing in the pure C should be fine.

Well there's the issue with register access and endianity. Even Linux has 
trouble with that.

Cheers

> 
> Thanks,
> Michal
Michal Simek Sept. 1, 2011, 11:04 a.m. UTC | #11
Marek Vasut wrote:
> On Thursday, September 01, 2011 10:55:31 AM Michal Simek wrote:
> 
> [...]
> 
>>>> I am fine to use 1 << n solution but definitely in our repo I will use
>>>> in way I like.
>>> Well I see it both ways ... 0x40000000 == 1 << 30 ... it's the same
>>> thing. On the other note, it's hard to count the zeroes in there AND you
>>> can mistake 0 and 8 in a huge series of those.
>>>
>>> Also, you can have whatever you want in your repo if you seriously care
>>> to invest the energy into maintaining it just because you need to be
>>> stubborn. But it'd really be great if you invested that energy in a more
>>> productive manner ;-)
>> There are two points of view. And both have con & pro.
>> I don't want to argue. Net custodian should decided if is OK or not.
>>
>> Look at tsec.h and probably others.
> 
> That something's in mainline doesn't mean it's obviously correct !
> 

Make no sense to argue with you. Two equal solutions.


>>>>> "currently", so there's possibility, in future this won't hold?
>>>> BTW: I am also sharing rx/tx buffer descriptors for dma.
>>>>
>>>> When do you expect that u-boot will be able to use several MACs in one
>>>> time?
>>> It's not a matter of when, but -- write a correct code, it's much less
>>> burden to fix it later.
>> Agree in general.
>> It is always question of when. You can always do it in better way. The
>> question is if someone will pay you for doing it in better way.
> 
> And if the code isn't accepted, they won't pay you ;-)

I don't need to add these drivers to mainline. For me is much easier to keep
it just for me and don't care about others.

> 
>> If this
>> feature is not important for us, make no sense to invest our time/money to
>> it.
> 
> I guess having the code mainline is important ?

Partially yes. It depends how often is API changed. If API change is so often
it is better to be in mainline because others fixed your driver too. If not,
you can easily fix your driver yourself.
If you are out of mainline, you don't need to spend your time to keep it alive
on the latest version. Most of our customers don't need u-boot for final products too.


>>>>>>>> +	/* Write new speed setting out to Axi Ethernet */
>>>>>>>> +	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
>>>>>>> Use clrsetbits() here.
>>>>>> Not defined for Microblaze - just for ARM/PPC.
>>>>>> Not going to use it.
>>>>> Please fix then. You're the microblaze maintainer, right?
>>>> Custodian.
>>> Oh come on ...
>>>
>>>> But I won't do that.
>>> I think you should.
>>>
>>>> If you think that all archs should have it then move it to generic
>>>> location which clean code duplication and I will include it.
>>> That's not the point, it's platform specific.
>> Just compare ppc/arm implementation and they are the same. It is even pure
>> generic code. Adding it to microblaze is just code duplication which is
>> also not good way to go. Then in future someone will move it to generic
>> location.
>> There were a lot of examples in linux kernel and includes.
> 
> Ok, if it's a generic code, please submit a patch putting it into a generic 
> place and the fix this driver to use it.

It it the same point to you.

Michal
Wolfgang Denk Sept. 1, 2011, 12:47 p.m. UTC | #12
Dear Michal Simek,

In message <4E5F4883.3080409@monstr.eu> you wrote:
>
> > Well I see it both ways ... 0x40000000 == 1 << 30 ... it's the same thing. On 
> > the other note, it's hard to count the zeroes in there AND you can mistake 0 and 
> > 8 in a huge series of those.
> > 
> > Also, you can have whatever you want in your repo if you seriously care to 
> > invest the energy into maintaining it just because you need to be stubborn. But 
> > it'd really be great if you invested that energy in a more productive manner ;-)
> 
> There are two points of view. And both have con & pro.
> I don't want to argue. Net custodian should decided if is OK or not.

I can parse 0x00200000 much faster than '1 << 21' so my personal
preference for such masks is the raw hex numbers.

But this is just that: a personal preference.  I see no problem with
using either one or the other form, and I have no really strong
preferences.  In any case this should not be a reason for serious
fights - it's a matter of taste issue, not more.

Best regards,

Wolfgang Denk
diff mbox

Patch

diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 4541eaf..ae9d4cb 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -83,6 +83,7 @@  COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o
 COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
 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_LL_TEMAC) += xilinx_ll_temac.o
 
diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c
new file mode 100644
index 0000000..ce79b80
--- /dev/null
+++ b/drivers/net/xilinx_axi_emac.c
@@ -0,0 +1,622 @@ 
+/*
+ * Copyright (C) 2011 Michal Simek <monstr@monstr.eu>
+ * Copyright (C) 2011 PetaLogix
+ * Copyright (C) 2010 Xilinx, Inc. All rights reserved.
+ *
+ * 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 <config.h>
+#include <common.h>
+#include <net.h>
+#include <malloc.h>
+#include <asm/io.h>
+#include <phy.h>
+#include <miiphy.h>
+
+/* Axi Ethernet registers offset */
+#define XAE_IS_OFFSET		0x0000000C /* Interrupt status */
+#define XAE_IE_OFFSET		0x00000014 /* Interrupt enable */
+#define XAE_RCW1_OFFSET		0x00000404 /* Rx Configuration Word 1 */
+#define XAE_TC_OFFSET		0x00000408 /* Tx Configuration */
+#define XAE_EMMC_OFFSET		0x00000410 /* EMAC mode configuration */
+#define XAE_MDIO_MC_OFFSET	0x00000500 /* MII Management Config */
+#define XAE_MDIO_MCR_OFFSET	0x00000504 /* MII Management Control */
+#define XAE_MDIO_MWD_OFFSET	0x00000508 /* MII Management Write Data */
+#define XAE_MDIO_MRD_OFFSET	0x0000050C /* MII Management Read Data */
+
+/* Link setup */
+#define XAE_EMMC_LINKSPEED_MASK	0xC0000000 /* Link speed */
+#define XAE_EMMC_LINKSPD_10	0x00000000 /* Link Speed mask for 10 Mbit */
+#define XAE_EMMC_LINKSPD_100	0x40000000 /* Link Speed mask for 100 Mbit */
+#define XAE_EMMC_LINKSPD_1000	0x80000000 /* Link Speed mask for 1000 Mbit */
+
+/* Interrupt Status/Enable/Mask Registers bit definitions */
+#define XAE_INT_RXRJECT_MASK	0x00000008 /* Rx frame rejected */
+#define XAE_INT_MGTRDY_MASK	0x00000080 /* MGT clock Lock */
+
+/* Receive Configuration Word 1 (RCW1) Register bit definitions */
+#define XAE_RCW1_RX_MASK	0x10000000 /* Receiver enable */
+
+/* Transmitter Configuration (TC) Register bit definitions */
+#define XAE_TC_TX_MASK		0x10000000 /* Transmitter enable */
+
+/*
+ * Station address bits [47:32]
+ * Station address bits [31:0] are stored in register UAW0
+ */
+#define XAE_UAW0_OFFSET			0x00000700 /* Unicast address word 0 */
+#define XAE_UAW1_OFFSET			0x00000704 /* Unicast address word 1 */
+#define XAE_UAW1_UNICASTADDR_MASK	0x0000FFFF
+
+/* MDIO Management Configuration (MC) Register bit definitions */
+#define XAE_MDIO_MC_MDIOEN_MASK		0x00000040 /* MII management enable*/
+
+/* MDIO Management Control Register (MCR) Register bit definitions */
+#define XAE_MDIO_MCR_PHYAD_MASK		0x1F000000 /* Phy Address Mask */
+#define XAE_MDIO_MCR_PHYAD_SHIFT	24	   /* Phy Address Shift */
+#define XAE_MDIO_MCR_REGAD_MASK		0x001F0000 /* Reg Address Mask */
+#define XAE_MDIO_MCR_REGAD_SHIFT	16	   /* Reg Address Shift */
+#define XAE_MDIO_MCR_OP_READ_MASK	0x00008000 /* Op Code Read Mask */
+#define XAE_MDIO_MCR_OP_WRITE_MASK	0x00004000 /* Op Code Write Mask */
+#define XAE_MDIO_MCR_INITIATE_MASK	0x00000800 /* Ready Mask */
+#define XAE_MDIO_MCR_READY_MASK		0x00000080 /* Ready Mask */
+
+#define XAE_MDIO_DIV_DFT	29	/* Default MDIO clock divisor */
+
+/* DMA macros */
+/* Bitmasks of XAXIDMA_CR_OFFSET register */
+#define XAXIDMA_CR_RUNSTOP_MASK	0x00000001 /* Start/stop DMA channel */
+#define XAXIDMA_CR_RESET_MASK	0x00000004 /* Reset DMA engine */
+
+/* Bitmasks of XAXIDMA_SR_OFFSET register */
+#define XAXIDMA_HALTED_MASK	0x00000001  /* DMA channel halted */
+
+/* Bitmask for interrupts */
+#define XAXIDMA_IRQ_IOC_MASK	0x00001000 /* Completion intr */
+#define XAXIDMA_IRQ_DELAY_MASK	0x00002000 /* Delay interrupt */
+#define XAXIDMA_IRQ_ALL_MASK	0x00007000 /* All interrupts */
+
+/* Bitmasks of XAXIDMA_BD_CTRL_OFFSET register */
+#define XAXIDMA_BD_CTRL_TXSOF_MASK	0x08000000 /* First tx packet */
+#define XAXIDMA_BD_CTRL_TXEOF_MASK	0x04000000 /* Last tx packet */
+
+#define DMAALIGN	128
+
+static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
+
+/* reflect dma offsets */
+struct axidma_reg {
+	u32 control; /* DMACR */
+	u32 status; /* DMASR */
+	u32 current; /* CURDESC */
+	u32 reserved;
+	u32 tail; /* TAILDESC */
+};
+
+/* Private driver structures */
+struct axidma_priv {
+	struct axidma_reg *dmatx;
+	struct axidma_reg *dmarx;
+	int phyaddr;
+
+	struct phy_device *phydev;
+	struct mii_dev *bus;
+};
+
+/* BD descriptors */
+struct axidma_bd {
+	u32 next;	/* Next descriptor pointer */
+	u32 reserved1;
+	u32 phys;	/* Buffer address */
+	u32 reserved2;
+	u32 reserved3;
+	u32 reserved4;
+	u32 cntrl;	/* Control */
+	u32 status;	/* Status */
+	u32 app0;
+	u32 app1;	/* TX start << 16 | insert */
+	u32 app2;	/* TX csum seed */
+	u32 app3;
+	u32 app4;
+	u32 sw_id_offset;
+	u32 reserved5;
+	u32 reserved6;
+};
+
+/* Static BDs - driver uses only one BD */
+static struct axidma_bd tx_bd __attribute((aligned(DMAALIGN)));
+static struct axidma_bd rx_bd __attribute((aligned(DMAALIGN)));
+
+static inline void aximac_out32(u32 addr, u32 offset, u32 val)
+{
+	out_be32((u32 *)(addr + offset), val);
+}
+
+static inline u32 aximac_in32(u32 addr, u32 offset)
+{
+	return in_be32((u32 *)(addr + offset));
+}
+
+
+/* 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
+
+static u16 phyread(struct eth_device *dev, u32 phyaddress, u32 registernum)
+{
+	u32 mdioctrlreg = 0;
+
+	/* Wait till MDIO interface is ready to accept a new transaction. */
+	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
+						& XAE_MDIO_MCR_READY_MASK))
+		;
+
+	mdioctrlreg = ((phyaddress << XAE_MDIO_MCR_PHYAD_SHIFT) &
+			XAE_MDIO_MCR_PHYAD_MASK) |
+			((registernum << XAE_MDIO_MCR_REGAD_SHIFT)
+			& XAE_MDIO_MCR_REGAD_MASK) |
+			XAE_MDIO_MCR_INITIATE_MASK |
+			XAE_MDIO_MCR_OP_READ_MASK;
+
+	aximac_out32(dev->iobase, XAE_MDIO_MCR_OFFSET, mdioctrlreg);
+
+	/* Wait till MDIO transaction is completed. */
+	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
+						& XAE_MDIO_MCR_READY_MASK))
+		;
+
+	/* Read data */
+	return (u16) aximac_in32(dev->iobase, XAE_MDIO_MRD_OFFSET);
+}
+
+static void phywrite(struct eth_device *dev, u32 phyaddress, u32 registernum,
+								u32 data)
+{
+	u32 mdioctrlreg = 0;
+
+	/* Wait till MDIO interface is ready to accept a new transaction. */
+	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
+						& XAE_MDIO_MCR_READY_MASK))
+		;
+
+	mdioctrlreg = ((phyaddress << XAE_MDIO_MCR_PHYAD_SHIFT) &
+			XAE_MDIO_MCR_PHYAD_MASK) |
+			((registernum << XAE_MDIO_MCR_REGAD_SHIFT)
+			& XAE_MDIO_MCR_REGAD_MASK) |
+			XAE_MDIO_MCR_INITIATE_MASK |
+			XAE_MDIO_MCR_OP_WRITE_MASK;
+
+	/* Write data */
+	aximac_out32(dev->iobase, XAE_MDIO_MWD_OFFSET, data);
+
+	aximac_out32(dev->iobase, XAE_MDIO_MCR_OFFSET, mdioctrlreg);
+
+	/* Wait till MDIO transaction is completed. */
+	while (!(aximac_in32(dev->iobase, XAE_MDIO_MCR_OFFSET)
+						& XAE_MDIO_MCR_READY_MASK))
+		;
+}
+
+
+/* setting axi emac and phy to proper setting */
+static int setup_phy(struct eth_device *dev)
+{
+#ifdef CONFIG_PHYLIB
+	int i;
+	unsigned int speed;
+	u16 phyreg;
+	u32 emmc_reg;
+	struct axidma_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->phyaddr == -1) {
+		/* detect the PHY address */
+		for (i = 31; i >= 0; i--) {
+			phyreg = phyread(dev, i, PHY_DETECT_REG);
+	
+			if ((phyreg != 0xFFFF) &&
+			((phyreg & PHY_DETECT_MASK) == PHY_DETECT_MASK)) {
+				/* Found a valid PHY address */
+				priv->phyaddr = i;
+				debug("Found valid phy address, %d\n", phyreg);
+				break;
+			}
+		}
+	}
+
+	/* 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);
+
+	switch(phydev->speed) {
+	case 1000:
+		speed = XAE_EMMC_LINKSPD_1000;
+		break;
+	case 100:
+		speed = XAE_EMMC_LINKSPD_100;
+		break;
+	case 10:
+		speed = XAE_EMMC_LINKSPD_10;
+		break;
+	default:
+		return 0;
+	}
+
+	/* Setup the emac for the phy speed */
+	emmc_reg = aximac_in32(dev->iobase, XAE_EMMC_OFFSET);
+	emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
+	emmc_reg |= speed;
+
+	/* Write new speed setting out to Axi Ethernet */
+	aximac_out32(dev->iobase, XAE_EMMC_OFFSET, emmc_reg);
+
+	/*
+	* Setting the operating speed of the MAC needs a delay. There
+	* doesn't seem to be register to poll, so please consider this
+	* during your application design.
+	*/
+	udelay(1);
+
+	return 1;
+#else
+	puts("Enable PHYLIB support!\n");
+	return 0;
+#endif
+}
+
+/* STOP DMA transfers */
+static void axiemac_halt(struct eth_device *dev)
+{
+	struct axidma_priv *priv = dev->priv;
+
+	/* Stop the hardware */
+	priv->dmatx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;
+	priv->dmarx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;
+
+	debug("axiemac halted\n");
+}
+
+static void axi_ethernet_init(struct eth_device *dev)
+{
+	/*
+	 * Check the status of the MgtRdy bit in the interrupt status
+	 * registers. This must be done to allow the MGT clock to become stable
+	 * for the Sgmii and 1000BaseX PHY interfaces. No other register reads
+	 * will be valid until this bit is valid.
+	 * The bit is always a 1 for all other PHY interfaces.
+	 */
+	u32 timeout = 200;
+	while (timeout && (!(aximac_in32(dev->iobase, XAE_IS_OFFSET) &
+							XAE_INT_MGTRDY_MASK)))
+		timeout--;
+
+	if (!timeout)
+		printf("%s: Timeout\n", __func__);
+
+	/* Stop the device and reset HW */
+	/* Disable interrupts */
+	aximac_out32(dev->iobase, XAE_IE_OFFSET, 0);
+
+	/* Disable the receiver */
+	aximac_out32(dev->iobase, XAE_RCW1_OFFSET,
+		aximac_in32(dev->iobase, XAE_RCW1_OFFSET) & ~XAE_RCW1_RX_MASK);
+
+	/*
+	 * Stopping the receiver in mid-packet causes a dropped packet
+	 * indication from HW. Clear it.
+	 */
+	/* set the interrupt status register to clear the interrupt */
+	aximac_out32(dev->iobase, XAE_IS_OFFSET, XAE_INT_RXRJECT_MASK);
+
+	/* Setup HW */
+	/* Set default MDIO divisor */
+	aximac_out32(dev->iobase, XAE_MDIO_MC_OFFSET,
+			(u32) XAE_MDIO_DIV_DFT | XAE_MDIO_MC_MDIOEN_MASK);
+
+	debug("XAxiEthernet InitHw: done\n");
+}
+
+static void setup_mac(struct eth_device *dev)
+{
+	/* Set the MAC address */
+	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
+		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
+	aximac_out32(dev->iobase, XAE_UAW0_OFFSET, val);
+
+	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
+	val |= aximac_in32(dev->iobase, XAE_UAW1_OFFSET) &
+						~XAE_UAW1_UNICASTADDR_MASK;
+	aximac_out32(dev->iobase, XAE_UAW1_OFFSET, val);
+}
+
+/* Reset DMA engine */
+static void axi_dma_init(struct eth_device *dev)
+{
+	struct axidma_priv *priv = dev->priv;
+
+	/* Reset the engine so the hardware starts from a known state */
+	priv->dmatx->control = XAXIDMA_CR_RESET_MASK;
+	priv->dmarx->control = XAXIDMA_CR_RESET_MASK;
+
+	/* At the initialization time, hardware should finish reset quickly */
+	u32 timeout = 500;
+	while (timeout--) {
+		/* Check transmit/receive channel */
+		/* Reset is done when the reset bit is low */
+		if (!(priv->dmatx->control | priv->dmarx->control)
+						& XAXIDMA_CR_RESET_MASK)
+			break;
+		timeout -= 1;
+	}
+
+	if (!timeout)
+		printf("%s: Timeout\n", __func__);
+}
+
+static int axiemac_init(struct eth_device *dev, bd_t * bis)
+{
+	struct axidma_priv *priv = dev->priv;
+	debug("axi emac init started\n");
+
+	/*
+	 * Initialize AXIDMA engine. AXIDMA engine must be initialized before
+	 * AxiEthernet. During AXIDMA engine initialization, AXIDMA hardware is
+	 * reset, and since AXIDMA reset line is connected to AxiEthernet, this
+	 * would ensure a reset of AxiEthernet.
+	 */
+	axi_dma_init(dev);
+
+	/* Initialize AxiEthernet hardware. */
+	axi_ethernet_init(dev);
+	setup_mac(dev);
+
+	/* Start the hardware */
+	priv->dmarx->control |= XAXIDMA_CR_RUNSTOP_MASK;
+	/* Start DMA RX channel. Now it's ready to receive data.*/
+	priv->dmarx->current = (u32)&rx_bd;
+
+	/* Disable all RX interrupts before RxBD space setup */
+	priv->dmarx->control &= ~(XAXIDMA_IRQ_ALL_MASK & XAXIDMA_IRQ_ALL_MASK);
+
+	/* Setup the BD. */
+	memset((void *) &rx_bd, 0, sizeof(rx_bd));
+	rx_bd.next = (u32)&rx_bd;
+	rx_bd.phys = (u32)&RxFrame;
+	rx_bd.cntrl = sizeof(RxFrame);
+	/* Flush the last BD so DMA core could see the updates */
+	flush_cache((u32)&rx_bd, sizeof(rx_bd));
+
+	/* it is necessary to flush RxFrame because if you don't do it
+	 * then cache can contain uninitialized data */
+	flush_cache((u32)&RxFrame, sizeof(RxFrame));
+
+	/* Rx BD is ready - start */
+	priv->dmarx->tail = (u32)&rx_bd;
+
+	/* enable TX */
+	aximac_out32(dev->iobase, XAE_TC_OFFSET, XAE_TC_TX_MASK);
+	/* enable RX */
+	aximac_out32(dev->iobase, XAE_RCW1_OFFSET, XAE_RCW1_RX_MASK);
+
+	/* PHY setup */
+	if (!setup_phy(dev)) {
+		axiemac_halt(dev);
+		return -1;
+	}
+
+	debug("axi emac init complete\n");
+	return 0;
+}
+
+static int axiemac_send(struct eth_device *dev, volatile void *ptr, int len)
+{
+	struct axidma_priv *priv = dev->priv;
+
+	if (len > PKTSIZE_ALIGN)
+		len = PKTSIZE_ALIGN;
+
+	/* Flush packet to main memory to be trasfered by DMA */
+	flush_cache((u32)ptr, len);
+
+	/* Setup Tx BD */
+	memset((void *) &tx_bd, 0, sizeof(tx_bd));
+	/* At the end of the ring, link the last BD back to the top */
+	tx_bd.next = (u32)&tx_bd;
+	tx_bd.phys = (u32)ptr;
+	/* save len */
+	tx_bd.cntrl = len | XAXIDMA_BD_CTRL_TXSOF_MASK |
+						XAXIDMA_BD_CTRL_TXEOF_MASK;
+
+	/* Flush the last BD so DMA core could see the updates */
+	flush_cache((u32)&tx_bd, sizeof(tx_bd));
+
+	if (priv->dmatx->status & XAXIDMA_HALTED_MASK) {
+		priv->dmatx->current = (u32)&tx_bd;
+		/* Start the hardware */
+		priv->dmatx->control |= XAXIDMA_CR_RUNSTOP_MASK;
+	}
+
+	/* start transfer */
+	priv->dmatx->tail = (u32)&tx_bd;
+
+	/* Wait for transmission to complete */
+	debug("axi emac, waiting for tx to be done\n");
+	while (!priv->dmatx->status &
+				(XAXIDMA_IRQ_DELAY_MASK | XAXIDMA_IRQ_IOC_MASK))
+		;
+
+	debug("axi emac send complete\n");
+	return 0;
+}
+
+static int IsRxReady(struct eth_device *dev)
+{
+	u32 status;
+	struct axidma_priv *priv = dev->priv;
+
+	/* Read pending interrupts */
+	status = priv->dmarx->status;
+
+	/* Acknowledge pending interrupts */
+	priv->dmarx->status &= XAXIDMA_IRQ_ALL_MASK;
+
+	/*
+	 * If Reception done interrupt is asserted, call RX call back function
+	 * to handle the processed BDs and then raise the according flag.
+	 */
+	if ((status & (XAXIDMA_IRQ_DELAY_MASK | XAXIDMA_IRQ_IOC_MASK)))
+		return 1;
+
+	return 0;
+}
+
+static int axiemac_recv(struct eth_device *dev)
+{
+	u32 length;
+	struct axidma_priv *priv = dev->priv;
+
+	/* wait for an incoming packet */
+	if (!IsRxReady(dev))
+		return 0;
+
+	debug("axi emac, rx data ready\n");
+
+	/* Disable IRQ for a moment till packet is handled */
+	priv->dmarx->control &= ~(XAXIDMA_IRQ_ALL_MASK & XAXIDMA_IRQ_ALL_MASK);
+
+	length = rx_bd.app4 & 0x0000FFFF;
+#ifdef DEBUG
+	print_buffer(&RxFrame, &RxFrame[0], 1, length, 16);
+#endif
+	/* pass the received frame up for processing */
+	if (length)
+		NetReceive(RxFrame, length);
+
+#ifdef DEBUG
+	/* It is useful to clear buffer to be sure that it is consistent */
+	memset(RxFrame, 0, sizeof(RxFrame));
+#endif
+	/* Setup RxBD */
+	/* Clear the whole buffer and setup it again - all flags are cleared */
+	memset((void *) &rx_bd, 0, sizeof(rx_bd));
+	rx_bd.next = (u32)&rx_bd;
+	rx_bd.phys = (u32)&RxFrame;
+	rx_bd.cntrl = sizeof(RxFrame);
+
+	/* write bd to HW */
+	flush_cache((u32)&rx_bd, sizeof(rx_bd));
+
+	/* it is necessary to flush RxFrame because if you don't do it
+	 * then cache will contain previous packet */
+	flush_cache((u32)&RxFrame, sizeof(RxFrame));
+
+	/* Rx BD is ready - start again */
+	priv->dmarx->tail = (u32)&rx_bd;
+
+	debug("axi emac rx complete, framelength = %d\n", length);
+
+	return length;
+}
+
+static int axiemac_miiphy_read(const char *devname, uchar addr,
+							uchar reg, ushort *val)
+{
+	struct eth_device *dev = eth_get_dev();
+	*val = phyread(dev, addr, reg);
+	debug("%s 0x%x, 0x%x, 0x%x\n", __func__, addr, reg, *val);
+	return 0;
+}
+
+static int axiemac_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);
+	phywrite(dev, addr, reg, val);
+	return 0;
+}
+
+static int axiemac_bus_reset(struct mii_dev *bus)
+{
+	debug("Just bus reset\n");
+	return 0;
+}
+
+int xilinx_axiemac_initialize(bd_t *bis, unsigned long base_addr, int dma_addr)
+{
+	struct eth_device *dev;
+	struct axidma_priv *priv;
+
+	dev = calloc(1, sizeof(struct eth_device));
+	if (dev == NULL)
+		return -1;
+
+	dev->priv = calloc(1, sizeof(struct axidma_priv));
+	if (dev->priv == NULL) {
+		free(dev);
+		return -1;
+	}
+	priv = dev->priv;
+
+	sprintf(dev->name, "aximac.%lx", base_addr);
+
+	dev->iobase = base_addr;
+	priv->dmatx = (struct axidma_reg *)dma_addr;
+	/* rx channel offset */
+	priv->dmarx = (struct axidma_reg *)(dma_addr + 0x30);
+	dev->init = axiemac_init;
+	dev->halt = axiemac_halt;
+	dev->send = axiemac_send;
+	dev->recv = axiemac_recv;
+
+#ifdef CONFIG_PHY_ADDR
+	priv->phyaddr = CONFIG_PHY_ADDR;
+#else
+	priv->phyaddr = -1;
+#endif
+
+	eth_register(dev);
+
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB)
+	miiphy_register(dev->name, axiemac_miiphy_read, axiemac_miiphy_write);
+	priv->bus = miiphy_get_dev_by_name(dev->name);
+	priv->bus->reset = axiemac_bus_reset;
+#endif
+	return 1;
+}
diff --git a/include/netdev.h b/include/netdev.h
index cebae3b..a791ece 100644
--- a/include/netdev.h
+++ b/include/netdev.h
@@ -90,6 +90,7 @@  int smc91111_initialize(u8 dev_num, int base_addr);
 int tsi108_eth_initialize(bd_t *bis);
 int uec_standard_init(bd_t *bis);
 int uli526x_initialize(bd_t *bis);
+int xilinx_axiemac_initialize(bd_t *bis, unsigned long base_addr, int dma_addr);
 int xilinx_emaclite_initialize(bd_t *bis, unsigned long base_addr,
 							int txpp, int rxpp);
 int xilinx_ll_temac_initialize(bd_t *bis, unsigned long base_addr,