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

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

[Michal Simek]

Add the first axi_ethernet driver for little-endian Microblaze.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---
 .../xilinx/microblaze-generic/microblaze-generic.c |    4 +
 drivers/net/Makefile                               |    1 +
 drivers/net/xilinx_axi_emac.c                      |  529 ++++++++++++++++++++
 include/configs/microblaze-generic.h               |    3 +
 4 files changed, 537 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/xilinx_axi_emac.c

diff --git a/board/xilinx/microblaze-generic/microblaze-generic.c b/board/xilinx/microblaze-generic/microblaze-generic.c
index 0102c9e..b38cf3d 100644
--- a/board/xilinx/microblaze-generic/microblaze-generic.c
+++ b/board/xilinx/microblaze-generic/microblaze-generic.c
@@ -73,6 +73,10 @@ int board_eth_init(bd_t *bis)
 {
 	int ret = 0;
 
+#ifdef CONFIG_XILINX_AXIEMAC
+	ret |= xilinx_axiemac_initialize(bis, XILINX_AXIEMAC_BASEADDR,
+						XILINX_AXIDMA_BASEADDR);
+#endif
 #ifdef CONFIG_XILINX_EMACLITE
 	ret |= xilinx_emaclite_initialize(bis, XILINX_EMACLITE_BASEADDR);
 #endif
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index d720e27..4c77062 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.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..75ebac7
--- /dev/null
+++ b/drivers/net/xilinx_axi_emac.c
@@ -0,0 +1,529 @@
+/*
+ * 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>
+
+/* 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_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 ENET_MAX_MTU PKTSIZE_ALIGN
+#define DMAALIGN	128
+
+static u8 RxFrame[ENET_MAX_MTU] __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;
+};
+
+/* BD descriptors */
+typedef 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;
+} axidma_bd __attribute((aligned(DMAALIGN)));
+
+/* Static BDs - driver uses only one BD */
+static axidma_bd tx_bd;
+static axidma_bd rx_bd;
+
+/* 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 phy_read(struct eth_device *dev, u32 PhyAddress, u32 RegisterNum)
+{
+	u32 MdioCtrlReg = 0;
+
+	/* Wait till MDIO interface is ready to accept a new transaction. */
+	while (!(in_be32(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;
+
+	out_be32(dev->iobase + XAE_MDIO_MCR_OFFSET, MdioCtrlReg);
+
+	/* Wait till MDIO transaction is completed. */
+	while (!(in_be32(dev->iobase + XAE_MDIO_MCR_OFFSET)
+						& XAE_MDIO_MCR_READY_MASK))
+		;
+
+	/* Read data */
+	return (u16) in_be32(dev->iobase + XAE_MDIO_MRD_OFFSET);
+}
+
+/* setting axi emac and phy to proper setting */
+static void setup_phy(struct eth_device *dev)
+{
+	int i;
+	unsigned retries = 100;
+	u16 PhyReg;
+	u16 PhyReg2;
+	int PhyAddr = -1;
+	u32 emmc_reg;
+
+	debug("detecting phy address\n");
+
+	/* detect the PHY address */
+	for (PhyAddr = 31; PhyAddr >= 0; PhyAddr--) {
+		PhyReg = phy_read(dev, PhyAddr, PHY_DETECT_REG);
+
+		if ((PhyReg != 0xFFFF) &&
+		   ((PhyReg & PHY_DETECT_MASK) == PHY_DETECT_MASK)) {
+			/* Found a valid PHY address */
+
+			debug("Found valid phy address, %d\n", PhyReg);
+			break;
+		}
+	}
+
+	debug("waiting for the phy to be up\n");
+
+	/* wait for link up and autonegotiation completed */
+	while (retries-- > 0) {
+		if ((PhyReg & 0x24) == 0x24)
+			break;
+	}
+
+	/* get PHY id */
+	PhyReg = phy_read(dev, PhyAddr, 2);
+	PhyReg2 = phy_read(dev, PhyAddr, 2);
+	i = (PhyReg << 16) | PhyReg2;
+	debug("LL_TEMAC: Phy ID 0x%x\n", i);
+
+	/* Marwell 88e1111 id - ml50x, 0x1410141 id - sp605 */
+	if ((i == 0x1410cc2) || (i == 0x1410141)) {
+		debug("Marvell PHY recognized\n");
+
+		/* Setup the emac for the phy speed */
+		emmc_reg = in_be32(dev->iobase + XAE_EMMC_OFFSET);
+		emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
+
+		PhyReg = phy_read(dev, PhyAddr, 17);
+
+		if ((PhyReg & 0x8000) == 0x8000) {
+			emmc_reg |= XAE_EMMC_LINKSPD_1000;
+			printf("1000BASE-T\n");
+		} else if ((PhyReg & 0x4000) == 0x4000) {
+			printf("100BASE-T\n");
+			emmc_reg |= XAE_EMMC_LINKSPD_100;
+		} else {
+			printf("10BASE-T\n");
+			emmc_reg |= XAE_EMMC_LINKSPD_10;
+		}
+
+		/* Write new speed setting out to Axi Ethernet */
+		out_be32(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);
+	}
+}
+
+/* STOP DMA transfers */
+static void axiemac_halt(struct eth_device *dev)
+{
+	struct axidma_priv *dma = dev->priv;
+
+	if (dev->state) {
+		/* Stop the hardware */
+		dma->dmatx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;
+		dma->dmarx->control &= ~XAXIDMA_CR_RUNSTOP_MASK;
+		dev->state = 0;
+	}
+	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 &&
+		(!(in_be32(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 */
+	out_be32(dev->iobase + XAE_IE_OFFSET, 0);
+
+	/* Disable the receiver */
+	out_be32(dev->iobase + XAE_RCW1_OFFSET,
+		in_be32(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 */
+	out_be32(dev->iobase + XAE_IS_OFFSET, XAE_INT_RXRJECT_MASK);
+
+	/* Setup HW */
+	/* Set default MDIO divisor */
+	out_be32(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]));
+	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
+
+	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
+	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
+						& ~XAE_UAW1_UNICASTADDR_MASK;
+	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
+}
+
+/* Reset DMA engine */
+static void axi_dma_init(struct eth_device *dev)
+{
+	struct axidma_priv *dma = dev->priv;
+
+	/* Reset the engine so the hardware starts from a known state */
+	dma->dmatx->control = XAXIDMA_CR_RESET_MASK;
+	dma->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 (!(dma->dmatx->control | dma->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 *dma = 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 */
+	dma->dmarx->control |= XAXIDMA_CR_RUNSTOP_MASK;
+	/* Start DMA RX channel. Now it's ready to receive data.*/
+	dma->dmarx->current = (u32)&rx_bd;
+
+	/* Disable all RX interrupts before RxBD space setup */
+	dma->dmarx->control &= ~(XAXIDMA_IRQ_ALL_MASK & XAXIDMA_IRQ_ALL_MASK);
+
+	/* Setup the BD. */
+	memset((void *) &rx_bd, 0, sizeof(axidma_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(axidma_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 */
+	dma->dmarx->tail = (u32)&rx_bd;
+
+	/* enable TX */
+	out_be32(dev->iobase + XAE_TC_OFFSET, XAE_TC_TX_MASK);
+	/* enable RX */
+	out_be32(dev->iobase + XAE_RCW1_OFFSET, XAE_RCW1_RX_MASK);
+
+	/* PHY setup */
+	setup_phy(dev);
+	dev->state = 1;
+	debug("axi emac init complete\n");
+	return 1;
+}
+
+static int axiemac_send(struct eth_device *dev, volatile void *ptr, int len)
+{
+	struct axidma_priv *dma = dev->priv;
+
+	if (len > ENET_MAX_MTU)
+		len = ENET_MAX_MTU;
+
+	/* Flush packet to main memory to be trasfered by DMA */
+	flush_cache(ptr, len);
+
+	/* Setup Tx BD */
+	memset((void *) &tx_bd, 0, sizeof(axidma_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(axidma_bd));
+
+	if (dma->dmatx->status & XAXIDMA_HALTED_MASK) {
+		dma->dmatx->current = (u32)&tx_bd;
+		/* Start the hardware */
+		dma->dmatx->control |= XAXIDMA_CR_RUNSTOP_MASK;
+	}
+
+	/* start transfer */
+	dma->dmatx->tail = (u32)&tx_bd;
+
+	/* Wait for transmission to complete */
+	debug("axi emac, waiting for tx to be done\n");
+	while (!dma->dmatx->status &
+				(XAXIDMA_IRQ_DELAY_MASK | XAXIDMA_IRQ_IOC_MASK))
+		;
+
+	debug("axi emac send complete\n");
+	return 1;
+}
+
+static int IsRxReady(struct eth_device *dev)
+{
+	u32 status;
+	struct axidma_priv *dma = dev->priv;
+
+	/* Read pending interrupts */
+	status = dma->dmarx->status;
+
+	/* Acknowledge pending interrupts */
+	dma->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 *dma = 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 */
+	dma->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(axidma_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(axidma_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 */
+	dma->dmarx->tail = (u32)&rx_bd;
+
+	debug("axi emac rx complete, framelength = %d\n", length);
+
+	return length;
+}
+
+int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
+{
+	struct eth_device *dev;
+	struct axidma_priv *dma;
+
+	dev = malloc(sizeof(*dev));
+	if (dev == NULL)
+		hang();
+
+	memset(dev, 0, sizeof(*dev));
+	sprintf(dev->name, "Xilinx_AxiEmac");
+
+	dev->iobase = base_addr;
+	dma = dev->priv;
+	dma->dmatx = dma_addr;
+	dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */
+	dev->init = axiemac_init;
+	dev->halt = axiemac_halt;
+	dev->send = axiemac_send;
+	dev->recv = axiemac_recv;
+
+	eth_register(dev);
+
+	return 0;
+}
diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h
index ce9d426..efa0c73 100644
--- a/include/configs/microblaze-generic.h
+++ b/include/configs/microblaze-generic.h
@@ -78,6 +78,9 @@
 #elif XILINX_LLTEMAC_BASEADDR
 # define CONFIG_XILINX_LL_TEMAC		1
 # define CONFIG_SYS_ENET
+#elif defined(XILINX_AXIEMAC_BASEADDR)
+# define CONFIG_XILINX_AXIEMAC		1
+# define CONFIG_SYS_ENET
 #endif
 
 #undef ET_DEBUG
-- 
1.5.5.6

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

[Mike Frysinger]

On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
> --- /dev/null
> +++ b/drivers/net/xilinx_axi_emac.c
> +static void axi_ethernet_init(struct eth_device *dev)
> +{
> ...
> +	u32 timeout = 200;
> +	while (timeout &&
> +		(!(in_be32(dev->iobase + XAE_IS_OFFSET) & XAE_INT_MGTRDY_MASK)))
> +		timeout--;
> ...
> +static void axi_dma_init(struct eth_device *dev)
> +{
> ...
> +	/* 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 (!(dma->dmatx->control | dma->dmarx->control)
> +						& XAXIDMA_CR_RESET_MASK)
> +			break;
> +		timeout -= 1;
> +	}

shouldnt this be using a timer rather than some constant that will change the 
actual timeout length based on the speed of the core ?

> +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]));
> +	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
> +
> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
> +	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
> +						& ~XAE_UAW1_UNICASTADDR_MASK;
> +	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
> +}
> ...
> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
> +{
> ...
> +	setup_mac(dev);

this should be moved to eth_device.write_hwaddr in the initialize function.  
then the common layers will call setup_mac for you as needed.

> +	/* Setup the BD. */
> +	memset((void *) &rx_bd, 0, sizeof(axidma_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(axidma_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 */
> +	dma->dmarx->tail = (u32)&rx_bd;

hmm, shouldnt this be done only in the recv func ?

> +	return 1;

a bunch of these funcs return 1 when i'm pretty sure they should be 0

> +int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
> +{
> +	struct eth_device *dev;
> +	struct axidma_priv *dma;
> +
> +	dev = malloc(sizeof(*dev));
> +	if (dev == NULL)
> +		hang();
> +
> +	memset(dev, 0, sizeof(*dev));
> +	sprintf(dev->name, "Xilinx_AxiEmac");
> +
> +	dev->iobase = base_addr;
> +	dma = dev->priv;
> +	dma->dmatx = dma_addr;
> +	dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */

hmm, this is weird.  you just memset(dev) to 0, and then used dev->priv 
without assigning it storage.  so you're scribbling on top of address 0 with 
your dma struct here arent you ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110228/b68f2e0f/attachment.pgp 

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

[Michal Simek]

Mike Frysinger wrote:
> On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
>> --- /dev/null
>> +++ b/drivers/net/xilinx_axi_emac.c
>> +static void axi_ethernet_init(struct eth_device *dev)
>> +{
>> ...
>> +	u32 timeout = 200;
>> +	while (timeout &&
>> +		(!(in_be32(dev->iobase + XAE_IS_OFFSET) & XAE_INT_MGTRDY_MASK)))
>> +		timeout--;
>> ...
>> +static void axi_dma_init(struct eth_device *dev)
>> +{
>> ...
>> +	/* 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 (!(dma->dmatx->control | dma->dmarx->control)
>> +						& XAXIDMA_CR_RESET_MASK)
>> +			break;
>> +		timeout -= 1;
>> +	}
> 
> shouldnt this be using a timer rather than some constant that will change the 
> actual timeout length based on the speed of the core ?

it could/should be.

> 
>> +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]));
>> +	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
>> +
>> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
>> +	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
>> +						& ~XAE_UAW1_UNICASTADDR_MASK;
>> +	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
>> +}
>> ...
>> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
>> +{
>> ...
>> +	setup_mac(dev);
> 
> this should be moved to eth_device.write_hwaddr in the initialize function.  
> then the common layers will call setup_mac for you as needed.

ok. I haven't know about this possibility. Will fix it.

> 
>> +	/* Setup the BD. */
>> +	memset((void *) &rx_bd, 0, sizeof(axidma_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(axidma_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 */
>> +	dma->dmarx->tail = (u32)&rx_bd;
> 
> hmm, shouldnt this be done only in the recv func ?

For transmit I decided to setup BD directly in function axiemac_send.
For recv func BD must be setup before RX is enable that's why I have done it.
I haven't tried to setup BD and enable RX in axiemac_recv function.
Anyway axiemac_recv is called after axiemac_send and if I setup BD in that it will take some time to 
get that packet that's why I think it is better to setup BD and enable RX in init function.

> 
>> +	return 1;
> 
> a bunch of these funcs return 1 when i'm pretty sure they should be 0

init function is called from eth.c:eth_init(). From the code below you see that return can be >=0.


		if (eth_current->init(eth_current,bis) >= 0) {
			eth_current->state = ETH_STATE_ACTIVE;

			return 0;
		}


> 
>> +int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
>> +{
>> +	struct eth_device *dev;
>> +	struct axidma_priv *dma;
>> +
>> +	dev = malloc(sizeof(*dev));
>> +	if (dev == NULL)
>> +		hang();
>> +
>> +	memset(dev, 0, sizeof(*dev));
>> +	sprintf(dev->name, "Xilinx_AxiEmac");
>> +
>> +	dev->iobase = base_addr;
>> +	dma = dev->priv;
>> +	dma->dmatx = dma_addr;
>> +	dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */
> 
> hmm, this is weird.  you just memset(dev) to 0, and then used dev->priv 
> without assigning it storage.  so you're scribbling on top of address 0 with 
> your dma struct here arent you ?

dev contains:
iobase - axi emac baseaddr
init, halt, send, recv pointers to functions
and pointer priv
+ others

I need to clear it that's why memset.

dma controller is special IP that's why I use priv for storing pointer to axidma_priv structure 
which contains two pointers to dma for master to slave and slave to master directions (rx and tx if 
you like). As I wrote dma controller is different IP that's why there are different addresses.
axi_dma has offset between channels which is 0x30.
I used structures for hw access.

Thanks for your comments,
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

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

[Mike Frysinger]

On Monday, February 28, 2011 13:50:55 Michal Simek wrote:
> Mike Frysinger wrote:
> > On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
> >> +	return 1;
> > 
> > a bunch of these funcs return 1 when i'm pretty sure they should be 0
> 
> init function is called from eth.c:eth_init(). From the code below you see
> that return can be >=0.

funny enough, that func in your patch is returning 0 when it should be 1.  it 
doesnt explain why your recv/send are returning 1 when it should be 0.

the init func is a special beast -- it returns the # of devices registered, 
not "1 is success".

> >> +int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
> >> +{
> >> +	struct eth_device *dev;
> >> +	struct axidma_priv *dma;
> >> +
> >> +	dev = malloc(sizeof(*dev));
> >> +	if (dev == NULL)
> >> +		hang();
> >> +
> >> +	memset(dev, 0, sizeof(*dev));
> >> +	sprintf(dev->name, "Xilinx_AxiEmac");
> >> +
> >> +	dev->iobase = base_addr;
> >> +	dma = dev->priv;
> >> +	dma->dmatx = dma_addr;
> >> +	dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */
> > 
> > hmm, this is weird.  you just memset(dev) to 0, and then used dev->priv
> > without assigning it storage.  so you're scribbling on top of address 0
> > with your dma struct here arent you ?
> 
> dev contains:
> iobase - axi emac baseaddr
> init, halt, send, recv pointers to functions
> and pointer priv
> + others
> 
> I need to clear it that's why memset.
> 
> dma controller is special IP that's why I use priv for storing pointer to
> axidma_priv structure which contains two pointers to dma for master to
> slave and slave to master directions (rx and tx if you like). As I wrote
> dma controller is different IP that's why there are different addresses.
> axi_dma has offset between channels which is 0x30.
> I used structures for hw access.

ok, but i think you missed my point.  let me use this example:
dev->priv = 0;	/* the memset */
dma = dev->priv;
dma->dmatx = dma_addr;

you're doing a NULL pointer deref here.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110228/d2db302c/attachment.pgp 

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

[Michal Simek]

Mike Frysinger wrote:
> On Monday, February 28, 2011 13:50:55 Michal Simek wrote:
>> Mike Frysinger wrote:
>>> On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
>>>> +	return 1;
>>> a bunch of these funcs return 1 when i'm pretty sure they should be 0
>> init function is called from eth.c:eth_init(). From the code below you see
>> that return can be >=0.
> 
> funny enough, that func in your patch is returning 0 when it should be 1.  it 
> doesnt explain why your recv/send are returning 1 when it should be 0.

for recv:
Doesn't matter what you return because there is no return value checking at all.
$ grep -rn "eth_rx" net/
net/eth.c:398:int eth_rx(void)
net/eth.c:433:          eth_rx();
net/net.c:480:          eth_rx();

+ I think that doesn't matter what you return.
For recv I am returning packet length as for example uli526x, ep93xx_eth and I believe other do.

The truth is that in README.drivers.eth is suggested to return 0 for recv.


for sending - yes, there should be 0 but why this even work.

net/net.c:252:  (void) eth_send (NetTxPacket, (pkt - NetTxPacket) + ARP_HDR_SIZE);
net/net.c:641:  (void) eth_send(pkt, len);
net/net.c:686:  (void) eth_send(NetTxPacket, (pkt - NetTxPacket) + IP_HDR_SIZE + len);
net/net.c:970:  (void) eth_send(NetTxPacket, (uchar *)s - NetTxPacket);
net/net.c:1452:                 (void) eth_send((uchar *)et, (pkt - (uchar *)et) + ARP_HDR_SIZE);
net/net.c:1484:                         (void) eth_send(NetArpWaitTxPacket, NetArpWaitTxPacketSize);
net/net.c:1618:                         (void) eth_send((uchar *)et, ETHER_HDR_SIZE + len);
+
api/api_net.c:100:      return eth_send(buf, len);

Only api_net checks return values.

If both functions should return 0 then any code should check it and all others drivers should be fixed.

> 
> the init func is a special beast -- it returns the # of devices registered, 
> not "1 is success".

Where is it written?
ep93xx_eth.c returns also 1.
Anyway if is number of registered devices, "1" should means one registered device.
If zero means one registered device then please point me to that documentation.

Does any code even work with return value?

I expect that if I use bad return values that network driver shouldn't work but it works.

> 
>>>> +int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
>>>> +{
>>>> +	struct eth_device *dev;
>>>> +	struct axidma_priv *dma;
>>>> +
>>>> +	dev = malloc(sizeof(*dev));
>>>> +	if (dev == NULL)
>>>> +		hang();
>>>> +
>>>> +	memset(dev, 0, sizeof(*dev));
>>>> +	sprintf(dev->name, "Xilinx_AxiEmac");
>>>> +
>>>> +	dev->iobase = base_addr;
>>>> +	dma = dev->priv;
>>>> +	dma->dmatx = dma_addr;
>>>> +	dma->dmarx = (u32)dma->dmatx + 0x30; /* rx channel offset */
>>> hmm, this is weird.  you just memset(dev) to 0, and then used dev->priv
>>> without assigning it storage.  so you're scribbling on top of address 0
>>> with your dma struct here arent you ?
>> dev contains:
>> iobase - axi emac baseaddr
>> init, halt, send, recv pointers to functions
>> and pointer priv
>> + others
>>
>> I need to clear it that's why memset.
>>
>> dma controller is special IP that's why I use priv for storing pointer to
>> axidma_priv structure which contains two pointers to dma for master to
>> slave and slave to master directions (rx and tx if you like). As I wrote
>> dma controller is different IP that's why there are different addresses.
>> axi_dma has offset between channels which is 0x30.
>> I used structures for hw access.
> 
> ok, but i think you missed my point.  let me use this example:
> dev->priv = 0;	/* the memset */
> dma = dev->priv;
> dma->dmatx = dma_addr;
> 
> you're doing a NULL pointer deref here.

you are right. I have to fix it.

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

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

[Mike Frysinger]

On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:
> If both functions should return 0 then any code should check it and all
> others drivers should be fixed.

i agree, but that doesnt mean new code should knowingly be left broken

> > the init func is a special beast -- it returns the # of devices
> > registered, not "1 is success".
> 
> Where is it written?

atm, only the mailing list.  i have a local patch that i havent gotten around 
to pushing up yet for the README.drivers.eth.

> ep93xx_eth.c returns also 1.
> Anyway if is number of registered devices, "1" should means one registered
> device. If zero means one registered device then please point me to that
> documentation.

the change hasnt been ported to all drivers yet.  but new drivers should be 
doing it as i described.

also, you should change the "hang()" to "return 0" in the init func.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110301/f910b693/attachment.pgp 

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

[Michal Simek]

Mike Frysinger wrote:
> On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:
>> If both functions should return 0 then any code should check it and all
>> others drivers should be fixed.
> 
> i agree, but that doesnt mean new code should knowingly be left broken

I agree that make no sense do not fix it right now.

> 
>>> the init func is a special beast -- it returns the # of devices
>>> registered, not "1 is success".
>> Where is it written?
> 
> atm, only the mailing list.  i have a local patch that i havent gotten around 
> to pushing up yet for the README.drivers.eth.
> 
>> ep93xx_eth.c returns also 1.
>> Anyway if is number of registered devices, "1" should means one registered
>> device. If zero means one registered device then please point me to that
>> documentation.
> 
> the change hasnt been ported to all drivers yet.  but new drivers should be 
> doing it as i described.

How does it look like phy lib u-boot support?

> 
> also, you should change the "hang()" to "return 0" in the init func.

Are you sure return 0 which should mean success. Anything different from 0 seems to me relevant.

you too.
int bfin_EMAC_initialize(bd_t *bis)
{
	struct eth_device *dev;
	dev = malloc(sizeof(*dev));
	if (dev == NULL)
		hang();


I maintain emaclite driver and none tell me this that's why the process is so slow.
I believe if you release that documentation, which you are talking about, then others will 
clean/test their drivers.

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

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

[Mike Frysinger]

On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote:
> Mike Frysinger wrote:
> > On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:
> >> If both functions should return 0 then any code should check it and all
> >> others drivers should be fixed.
> > 
> > i agree, but that doesnt mean new code should knowingly be left broken
> 
> I agree that make no sense do not fix it right now.

err, your new driver should return the correct values.  what higher levels do 
or do not check does not matter, and whether other drivers do it correctly 
does not matter.

> >> ep93xx_eth.c returns also 1.
> >> Anyway if is number of registered devices, "1" should means one
> >> registered device. If zero means one registered device then please
> >> point me to that documentation.
> > 
> > the change hasnt been ported to all drivers yet.  but new drivers should
> > be doing it as i described.
> 
> How does it look like phy lib u-boot support?

i dont know what you mean ... how is phylib relevant to this ?  or are you 
just asking in general ?

> > also, you should change the "hang()" to "return 0" in the init func.
> 
> Are you sure return 0 which should mean success. Anything different from 0
> seems to me relevant.

as i said, the initialize function is not returning "success" or "failure".  
it is returning "# of devices registered".  if you cannot register any, you 
should return 0.  having the boot process fail because of network issues 
doesnt make much sense when u-boot can do quite a lot without the network.  
including updating itself via other means.

> I maintain emaclite driver and none tell me this that's why the process is
> so slow. I believe if you release that documentation, which you are
> talking about, then others will clean/test their drivers.

the behavior i describe isnt a decision i made.  it was made by the previous 
net maintainer and agreed upon by others in the discussion.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110301/5c643037/attachment.pgp 

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

[Michal Simek]

Mike Frysinger wrote:
> On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote:
>> Mike Frysinger wrote:
>>> On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:
>>>> If both functions should return 0 then any code should check it and all
>>>> others drivers should be fixed.
>>> i agree, but that doesnt mean new code should knowingly be left broken
>> I agree that make no sense do not fix it right now.
> 
> err, your new driver should return the correct values.  what higher levels do 
> or do not check does not matter, and whether other drivers do it correctly 
> does not matter.

Comment below.

> 
>>>> ep93xx_eth.c returns also 1.
>>>> Anyway if is number of registered devices, "1" should means one
>>>> registered device. If zero means one registered device then please
>>>> point me to that documentation.
>>> the change hasnt been ported to all drivers yet.  but new drivers should
>>> be doing it as i described.
>> How does it look like phy lib u-boot support?
> 
> i dont know what you mean ... how is phylib relevant to this ?  or are you 
> just asking in general ?

Ben wanted to create general phy lib support and remove all phy specific things from net drivers.
I hope you see that connection because there is also phy part.
If phy lib support is in progress (which probably not) then I would change the driver to support it.

> 
>>> also, you should change the "hang()" to "return 0" in the init func.
>> Are you sure return 0 which should mean success. Anything different from 0
>> seems to me relevant.
> 
> as i said, the initialize function is not returning "success" or "failure".  
> it is returning "# of devices registered".  if you cannot register any, you 
> should return 0.  having the boot process fail because of network issues 
> doesnt make much sense when u-boot can do quite a lot without the network.  
> including updating itself via other means.

Interesting.
1. you talked about hang() in initialize function(not dev->init) and for me xilinx_axiemac_initialize
Initialize function is called from  board_eth_init which is called from eth_initialize(eth.c)

There is this part of code
	if (board_eth_init != __def_eth_init) {
		if (board_eth_init(bis) < 0)
			printf("Board Net Initialization Failed\n");

If initialization failed the return value is < 0.
That's why hang() should be changed to return -1 and doesn't matter how many device there are.

If you write:
 >>> also, you should change the "hang()" to "return 0" in the init func.
(hang is only in xilinx_axiemac_initialize) and should be changed which I agree.

If you propose any change which I should do, I expect that if you are focus on blackfin that you 
have done that changes in all blackfin eth drivers. For example in bfin_mac.c where hang is also used.


> 
>> I maintain emaclite driver and none tell me this that's why the process is
>> so slow. I believe if you release that documentation, which you are
>> talking about, then others will clean/test their drivers.
> 
> the behavior i describe isnt a decision i made.  it was made by the previous 
> net maintainer and agreed upon by others in the discussion.

Ok. If that decision was made than I expect that should be written somewhere in doc.
I know it is boring to write any documentation but I expect that if any decision was made then is 
common that general code will be changed. The changes can be from top to down. If general eth code 
checks return values then driver which returns wrong return codes won't work.

I am not arguing that my driver shouldn't return correct values!!! That's not my point.

If you start arguing that my driver has some faults, I am open to fix it and I really appreciate 
your comments.

I think it is good time to summarize all that changes we discuss: (Please correct it if you think 
that it is wrong). (Our communication is getting messy a little bit)

1. hang() -> return -1

2. driver initialize function (setup dev functions, driver name, priv, etc)
return -1 - if initialize failed
return 0 - on success

3. dev->init
return -1 - if init failed
return 0 - on success
(here you are saying should be return # of devices)

4. dev->recv
return -1 - failed
return 0 - packet not received
return >0 - success - packet lenght

5. dev->send
return -1 - failed
return 0 - success

6. dev->write_hwaddr
return -1 - failed
return 0 - success

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

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

[Mike Frysinger]

On Tuesday, March 01, 2011 04:29:21 Michal Simek wrote:
> Mike Frysinger wrote:
> > On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote:
> >> How does it look like phy lib u-boot support?
> > 
> > i dont know what you mean ... how is phylib relevant to this ?  or are
> > you just asking in general ?
> 
> Ben wanted to create general phy lib support and remove all phy specific
> things from net drivers. I hope you see that connection because there is
> also phy part.
> If phy lib support is in progress (which probably not) then I would change
> the driver to support it.

yes, but that isnt relevant to any of the feedback ive given for this patch

> >>> also, you should change the "hang()" to "return 0" in the init func.
> >> 
> >> Are you sure return 0 which should mean success. Anything different from
> >> 0 seems to me relevant.
> > 
> > as i said, the initialize function is not returning "success" or
> > "failure". it is returning "# of devices registered".  if you cannot
> > register any, you should return 0.  having the boot process fail because
> > of network issues doesnt make much sense when u-boot can do quite a lot
> > without the network. including updating itself via other means.
> 
> Interesting.
> 1. you talked about hang() in initialize function(not dev->init) and for me
> xilinx_axiemac_initialize Initialize function is called from 
> board_eth_init which is called from eth_initialize(eth.c)
> 
> There is this part of code
> 	if (board_eth_init != __def_eth_init) {
> 		if (board_eth_init(bis) < 0)
> 			printf("Board Net Initialization Failed\n");
> 
> If initialization failed the return value is < 0.
> That's why hang() should be changed to return -1 and doesn't matter how
> many device there are.

this detail isnt currently ironed out, so if you wanted to change the "hang()" 
into "return -1" or "return 0", that is fine.

> If you write:
>  >>> also, you should change the "hang()" to "return 0" in the init func.
> 
> (hang is only in xilinx_axiemac_initialize) and should be changed which I
> agree.
> 
> If you propose any change which I should do, I expect that if you are focus
> on blackfin that you have done that changes in all blackfin eth drivers.
> For example in bfin_mac.c where hang is also used.

incorrect code in other drivers (including bfin_mac) is not justification for 
adding incorrect code to new drivers.  bfin_mac.c's call to hang() is wrong 
too in the current code base.

> >> I maintain emaclite driver and none tell me this that's why the process
> >> is so slow. I believe if you release that documentation, which you are
> >> talking about, then others will clean/test their drivers.
> > 
> > the behavior i describe isnt a decision i made.  it was made by the
> > previous net maintainer and agreed upon by others in the discussion.
> 
> Ok. If that decision was made than I expect that should be written
> somewhere in doc. I know it is boring to write any documentation but I
> expect that if any decision was made then is common that general code will
> be changed.

obviously that is true, but docs only get written/updated when someone is so 
inclined to do the work.  the existing doc only exists because i felt like 
writing at the time.  ive never been the net maintainer and thus "obligated" 
to write net documentation.  i just got tired of people doing it wrong and no 
one knowing what should be going on.

> 1. hang() -> return -1

ok

> 2. driver initialize function (setup dev functions, driver name, priv, etc)
> return -1 - if initialize failed
> return 0 - on success

no, "return 1" when the device has successfully registered

> 3. dev->init
> return -1 - if init failed
> return 0 - on success

ok

> (here you are saying should be return # of devices)

no, i think you confused "initialize" with "init" in my feedback

> 4. dev->recv
> return -1 - failed
> return 0 - packet not received
> return >0 - success - packet lenght

"return 0" is still "success" in the sense that there is nothing to do, but 
that nuance doesnt matter

> 5. dev->send
> return -1 - failed
> return 0 - success
> 
> 6. dev->write_hwaddr
> return -1 - failed
> return 0 - success

ok
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110303/0f1f9735/attachment.pgp 

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

[Michal Simek]

Mike Frysinger wrote:
>>>>> also, you should change the "hang()" to "return 0" in the init func.
>>>> Are you sure return 0 which should mean success. Anything different from
>>>> 0 seems to me relevant.
>>> as i said, the initialize function is not returning "success" or
>>> "failure". it is returning "# of devices registered".  if you cannot
>>> register any, you should return 0.  having the boot process fail because
>>> of network issues doesnt make much sense when u-boot can do quite a lot
>>> without the network. including updating itself via other means.
>> Interesting.
>> 1. you talked about hang() in initialize function(not dev->init) and for me
>> xilinx_axiemac_initialize Initialize function is called from 
>> board_eth_init which is called from eth_initialize(eth.c)
>>
>> There is this part of code
>> 	if (board_eth_init != __def_eth_init) {
>> 		if (board_eth_init(bis) < 0)
>> 			printf("Board Net Initialization Failed\n");
>>
>> If initialization failed the return value is < 0.
>> That's why hang() should be changed to return -1 and doesn't matter how
>> many device there are.
> 
> this detail isnt currently ironed out, so if you wanted to change the "hang()" 
> into "return -1" or "return 0", that is fine.

ok.

> 
>> If you write:
>>  >>> also, you should change the "hang()" to "return 0" in the init func.
>>
>> (hang is only in xilinx_axiemac_initialize) and should be changed which I
>> agree.
>>
>> If you propose any change which I should do, I expect that if you are focus
>> on blackfin that you have done that changes in all blackfin eth drivers.
>> For example in bfin_mac.c where hang is also used.
> 
> incorrect code in other drivers (including bfin_mac) is not justification for 
> adding incorrect code to new drivers.  bfin_mac.c's call to hang() is wrong 
> too in the current code base.

I am not arguing that I shouldn't fix this driver. I was/am still open to any suggestions which help 
me to improve microblaze code/drivers.
I am just saying that if you know that there is something wrong (even in blackfin part) and it is 
easy to fix it, then it will be good to fix it.

> 
>>>> I maintain emaclite driver and none tell me this that's why the process
>>>> is so slow. I believe if you release that documentation, which you are
>>>> talking about, then others will clean/test their drivers.
>>> the behavior i describe isnt a decision i made.  it was made by the
>>> previous net maintainer and agreed upon by others in the discussion.
>> Ok. If that decision was made than I expect that should be written
>> somewhere in doc. I know it is boring to write any documentation but I
>> expect that if any decision was made then is common that general code will
>> be changed.
> 
> obviously that is true, but docs only get written/updated when someone is so 
> inclined to do the work.  the existing doc only exists because i felt like 
> writing at the time.  ive never been the net maintainer and thus "obligated" 
> to write net documentation.  i just got tired of people doing it wrong and no 
> one knowing what should be going on.
> 
>> 1. hang() -> return -1
> 
> ok
> 
>> 2. driver initialize function (setup dev functions, driver name, priv, etc)
>> return -1 - if initialize failed
>> return 0 - on success
> 
> no, "return 1" when the device has successfully registered
> 
>> 3. dev->init
>> return -1 - if init failed
>> return 0 - on success
> 
> ok
> 
>> (here you are saying should be return # of devices)
> 
> no, i think you confused "initialize" with "init" in my feedback

ok. From my point of view is better do initialize only for one device
and not to registered all device in the driver. IMHO this functionality should be done by
board specific net function in board_eth_init.
Not sure if this covers all cases which can happen. But anyway if the driver initialize several 
drivers with the same type then you are losing flexibility.
I see at least one reason why will be better not to do it:
Initialize only one device which can speedup bootup time and maybe security reasons.

Number of registered device can be counted through eth_register function or similar
If you and others like to return number of devices I will return it that's no problem.
But I don't want to loose flexibility which is the reason why FPGAs are great.

To finish this discuss - here is what you think that it is correct.
ad 2)
return -1 - if initialize failed
return 0 - never return
return >0 - # of devices

Is it ok for you?

> 
>> 4. dev->recv
>> return -1 - failed
>> return 0 - packet not received
>> return >0 - success - packet lenght
> 
> "return 0" is still "success" in the sense that there is nothing to do, but 
> that nuance doesnt matter

agree.

> 
>> 5. dev->send
>> return -1 - failed
>> return 0 - success
>>
>> 6. dev->write_hwaddr
>> return -1 - failed
>> return 0 - success
> 
> ok

ok. great.
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

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

[Mike Frysinger]

On Friday, March 04, 2011 05:09:53 Michal Simek wrote:
> Mike Frysinger wrote:
> >> 3. dev->init
> >> return -1 - if init failed
> >> return 0 - on success
> > 
> > ok
> > 
> >> (here you are saying should be return # of devices)
> > 
> > no, i think you confused "initialize" with "init" in my feedback
> 
> ok. From my point of view is better do initialize only for one device
> and not to registered all device in the driver. IMHO this functionality
> should be done by board specific net function in board_eth_init.
> Not sure if this covers all cases which can happen. But anyway if the
> driver initialize several drivers with the same type then you are losing
> flexibility.

no, that isnt what i meant.  the initialize function is allowed to return more 
than one because a single part might support more than one interface.  think 
of MAC switches or multiple PHYs hooked up to a single MAC.  in neither of 
these cases should the board_eth_init func need to call a driver's initialize 
function multiple times imo.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110309/48425236/attachment.pgp 

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

[Mike Frysinger]

On Friday, March 04, 2011 05:09:53 Michal Simek wrote:
> To finish this discuss - here is what you think that it is correct.
> ad 2)
> return -1 - if initialize failed
> return 0 - never return
> return >0 - # of devices

oh, and to clarify on the "return 0", i think it's conceivable that if a 
device might not exist, the initialize function may return 0.  whether any 
driver currently utilizes this is a different issue.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110309/6dde67b9/attachment.pgp 

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

[Michal Simek]

Mike Frysinger wrote:
> On Monday, February 28, 2011 13:50:55 Michal Simek wrote:
>> Mike Frysinger wrote:
>>> On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
>>>> +	return 1;
>>> a bunch of these funcs return 1 when i'm pretty sure they should be 0
>> init function is called from eth.c:eth_init(). From the code below you see
>> that return can be >=0.
> 
> funny enough, that func in your patch is returning 0 when it should be 1.  it 
> doesnt explain why your recv/send are returning 1 when it should be 0.

BTW: blackin emac recv functions return 0, -1 and length values.
Is it correct?

static int bfin_EMAC_recv(struct eth_device *dev)
{
	int length = 0;

	for (;;) {
		if ((rxbuf[rxIdx]->StatusWord & RX_COMP) == 0) {
			length = -1;
			break;
		}
...
	return length;
}

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

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

[Mike Frysinger]

On Tuesday, March 01, 2011 03:37:30 Michal Simek wrote:
> Mike Frysinger wrote:
> > On Monday, February 28, 2011 13:50:55 Michal Simek wrote:
> >> Mike Frysinger wrote:
> >>> On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
> >>>> +	return 1;
> >>> 
> >>> a bunch of these funcs return 1 when i'm pretty sure they should be 0
> >> 
> >> init function is called from eth.c:eth_init(). From the code below you
> >> see that return can be >=0.
> > 
> > funny enough, that func in your patch is returning 0 when it should be 1.
> >  it doesnt explain why your recv/send are returning 1 when it should be
> > 0.
> 
> BTW: blackin emac recv functions return 0, -1 and length values.
> Is it correct?

looking at net/eth.c, -1 is an error return value.  add in current docs which 
says 0 is a success return value.  that leaves all other values as currently 
undocumented.  if a driver survey suggested that returning the length was 
useful info, then perhaps the documentation could be updated.  but as you 
noted, that return value isnt currently checked.

so for now, you could change the recv func to return the length rather than 0 
and i wouldnt complain, but always returning 1 is wrong.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110301/64a56b5f/attachment.pgp 

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

[Michal Simek]

>> +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]));
>> +	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
>> +
>> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
>> +	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
>> +						& ~XAE_UAW1_UNICASTADDR_MASK;
>> +	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
>> +}
>> ...
>> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
>> +{
>> ...
>> +	setup_mac(dev);
> 
> this should be moved to eth_device.write_hwaddr in the initialize function.  
> then the common layers will call setup_mac for you as needed.

I am not going to use write_hwaddr function because axi emac needs some initialization before you 
can write a mac addr that's why I will keep it in the same location as is.

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

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

[Mike Frysinger]

On Tuesday, March 01, 2011 03:19:12 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]));
> >> +	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
> >> +
> >> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
> >> +	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
> >> +						& ~XAE_UAW1_UNICASTADDR_MASK;
> >> +	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
> >> +}
> >> ...
> >> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
> >> +{
> >> ...
> >> +	setup_mac(dev);
> > 
> > this should be moved to eth_device.write_hwaddr in the initialize
> > function. then the common layers will call setup_mac for you as needed.
> 
> I am not going to use write_hwaddr function because axi emac needs some
> initialization before you can write a mac addr that's why I will keep it
> in the same location as is.

please add explicit comments to the setup_mac() func then so someone else 
doesnt go changing things without taking this into consideration first
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110301/55de810b/attachment.pgp 

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

[Michal Simek]

Mike Frysinger wrote:
> On Tuesday, March 01, 2011 03:19:12 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]));
>>>> +	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
>>>> +
>>>> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
>>>> +	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
>>>> +						& ~XAE_UAW1_UNICASTADDR_MASK;
>>>> +	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
>>>> +}
>>>> ...
>>>> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
>>>> +{
>>>> ...
>>>> +	setup_mac(dev);
>>> this should be moved to eth_device.write_hwaddr in the initialize
>>> function. then the common layers will call setup_mac for you as needed.
>> I am not going to use write_hwaddr function because axi emac needs some
>> initialization before you can write a mac addr that's why I will keep it
>> in the same location as is.
> 
> please add explicit comments to the setup_mac() func then so someone else 
> doesnt go changing things without taking this into consideration first

No problem to write it.

I have experience with several network IPs but I think less amount than you.
But anyway I think it is common that device must be in proper state to be able to setup mac for most 
of them.
 From my simple test I see that dev->write_hwaddr is called before dev->init which looks weird.

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

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

[Mike Frysinger]

On Tuesday, March 01, 2011 03:46:16 Michal Simek wrote:
> Mike Frysinger wrote:
> > On Tuesday, March 01, 2011 03:19:12 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]));
> >>>> +	out_be32(dev->iobase + XAE_UAW0_OFFSET, val);
> >>>> +
> >>>> +	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
> >>>> +	val |= in_be32(dev->iobase + XAE_UAW1_OFFSET)
> >>>> +						& ~XAE_UAW1_UNICASTADDR_MASK;
> >>>> +	out_be32(dev->iobase + XAE_UAW1_OFFSET, val);
> >>>> +}
> >>>> ...
> >>>> +static int axiemac_init(struct eth_device *dev, bd_t * bis)
> >>>> +{
> >>>> ...
> >>>> +	setup_mac(dev);
> >>> 
> >>> this should be moved to eth_device.write_hwaddr in the initialize
> >>> function. then the common layers will call setup_mac for you as needed.
> >> 
> >> I am not going to use write_hwaddr function because axi emac needs some
> >> initialization before you can write a mac addr that's why I will keep it
> >> in the same location as is.
> > 
> > please add explicit comments to the setup_mac() func then so someone else
> > doesnt go changing things without taking this into consideration first
> 
> No problem to write it.
> 
> I have experience with several network IPs but I think less amount than
> you.

i think you unduly credit me with more experience than i actually have

> But anyway I think it is common that device must be in proper state
> to be able to setup mac for most of them.
>  From my simple test I see that dev->write_hwaddr is called before
> dev->init which looks weird.

i dont think this is weird ... usually the MAC address is simply memory mapped 
storage, so the state of the EMAC controller itself doesnt matter.  you can 
peek/poke the contents of that MAC address storage at any time.

but i dont know what weirdness your controller has in place to make this not 
work too.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110301/8c3eae25/attachment.pgp 

[U-Boot] [PATCH v2] net: ll_temac: Add LL TEMAC driver to u-boot

[Michal Simek]

LL Temac driver can be used by microblaze, xilinx ppc405/440
in sdma and fifo mode. DCR or XPS bus can be used.

The driver uses and requires PHYLIB.

Signed-off-by: Michal Simek <monstr@monstr.eu>

---
v2: Remove helper function for access to temac
    Remove SDMA/FIFO/DCR macros and configure it in board
    Setup mac by write_hwaddr
---
 drivers/net/Makefile          |    1 +
 drivers/net/xilinx_ll_temac.c |  665 +++++++++++++++++++++++++++++++++++++++++
 include/netdev.h              |    2 +
 3 files changed, 668 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/xilinx_ll_temac.c

diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 819b197..4541eaf 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -84,6 +84,7 @@ COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
 COBJS-$(CONFIG_ULI526X) += uli526x.o
 COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
 COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
+COBJS-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o
 
 COBJS	:= $(sort $(COBJS-y))
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/net/xilinx_ll_temac.c b/drivers/net/xilinx_ll_temac.c
new file mode 100644
index 0000000..878cdf2
--- /dev/null
+++ b/drivers/net/xilinx_ll_temac.c
@@ -0,0 +1,665 @@
+/*
+ * Xilinx xps_ll_temac ethernet driver for u-boot
+ *
+ * Copyright (C) 2008 - 2011 Michal Simek <monstr@monstr.eu>
+ * Copyright (C) 2008 - 2011 PetaLogix
+ *
+ * Based on Yoshio Kashiwagi kashiwagi at co-nss.co.jp driver
+ * Copyright (C) 2008 Nissin Systems Co.,Ltd.
+ * March 2008 created
+ *
+ * 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.
+ */
+
+#include <config.h>
+#include <common.h>
+#include <net.h>
+#include <malloc.h>
+#include <asm/processor.h>
+#include <asm/io.h>
+#include <phy.h>
+#include <miiphy.h>
+
+#undef ETH_HALTING
+
+#define XTE_EMMC_LINKSPEED_MASK	0xC0000000 /* Link speed */
+/* XTE_EMCFG_LINKSPD_MASK */
+#define XTE_EMMC_LINKSPD_10	0x00000000 /* for 10 Mbit */
+#define XTE_EMMC_LINKSPD_100	0x40000000 /* for 100 Mbit */
+#define XTE_EMMC_LINKSPD_1000	0x80000000 /* forr 1000 Mbit */
+
+#define XTE_RSE_MIIM_RR_MASK	0x0002
+#define XTE_RSE_MIIM_WR_MASK	0x0004
+#define XTE_RSE_CFG_RR_MASK	0x0020
+#define XTE_RSE_CFG_WR_MASK	0x0040
+
+/* XPS_LL_TEMAC indirect registers offset definition */
+#define RCW1	0x240
+#define TC	0x280
+#define EMMC	0x300
+#define MC	0x340
+#define UAW0	0x380
+#define UAW1	0x384
+#define AFM	0x390
+#define MIIMWD	0x3b0
+#define MIIMAI	0x3b4
+
+#define CNTLREG_WRITE_ENABLE_MASK	0x8000
+
+#define MDIO_ENABLE_MASK	0x40
+#define MDIO_CLOCK_DIV_100MHz	0x28
+
+/* XPS_LL_TEMAC SDMA registers definition */
+# define TX_CURDESC_PTR		0x03
+# define TX_TAILDESC_PTR	0x04
+# define TX_CHNL_CTRL		0x05
+# define TX_IRQ_REG		0x06
+# define TX_CHNL_STS		0x07
+# define RX_NXTDESC_PTR		0x08
+# define RX_CURDESC_PTR		0x0b
+# define RX_TAILDESC_PTR	0x0c
+# define RX_CHNL_CTRL		0x0d
+# define RX_IRQ_REG		0x0e
+# define RX_CHNL_STS		0x0f
+
+# define DMA_CONTROL_REG	0x10
+
+/* CDMAC descriptor status bit definitions */
+# define BDSTAT_STOP_ON_END_MASK	0x20
+# define BDSTAT_COMPLETED_MASK		0x10
+# define BDSTAT_SOP_MASK		0x08
+# define BDSTAT_EOP_MASK		0x04
+
+# define CHNL_STS_ERROR_MASK		0x80
+
+/* All interrupt enable bits */
+#define XLLDMA_CR_IRQ_ALL_EN_MASK	0x00000087
+/* All interrupt bits */
+#define XLLDMA_IRQ_ALL_MASK		0x0000001F
+/* Disable error when 2 or 4 bit coalesce counter overflows */
+#define XLLDMA_DMACR_RX_OVERFLOW_ERR_DIS_MASK	0x00000010
+/* Disable error when 2 or 4 bit coalesce counter overflows */
+#define XLLDMA_DMACR_TX_OVERFLOW_ERR_DIS_MASK	0x00000008
+/* Enable use of tail pointer register */
+#define XLLDMA_DMACR_TAIL_PTR_EN_MASK	0x00000004
+
+#define SDMA_BIT	1
+#define DCR_BIT		2
+
+#define DMAALIGN	32
+
+/* SDMA Buffer Descriptor */
+struct cdmac_bd_t {
+	struct cdmac_bd_t *next_p;
+	unsigned char *phys_buf_p;
+	unsigned long buf_len;
+	unsigned char stat;
+	unsigned char app1_1;
+	unsigned short app1_2;
+	unsigned long app2;
+	unsigned long app3;
+	unsigned long app4;
+	unsigned long app5;
+};
+
+static struct cdmac_bd_t tx_bd __attribute((aligned(DMAALIGN)));
+static struct cdmac_bd_t rx_bd __attribute((aligned(DMAALIGN)));
+
+struct ll_fifo_s {
+	int isr; /* Interrupt Status Register 0x0 */
+	int ier; /* Interrupt Enable Register 0x4 */
+	int tdfr; /* Transmit data FIFO reset 0x8 */
+	int tdfv; /* Transmit data FIFO Vacancy 0xC */
+	int tdfd; /* Transmit data FIFO 32bit wide data write port 0x10 */
+	int tlf; /* Write Transmit Length FIFO 0x14 */
+	int rdfr; /* Read Receive data FIFO reset 0x18 */
+	int rdfo; /* Receive data FIFO Occupancy 0x1C */
+	int rdfd; /* Read Receive data FIFO 32bit wide data read port 0x20 */
+	int rlf; /* Read Receive Length FIFO 0x24 */
+	int llr; /* Read LocalLink reset 0x28 */
+};
+
+static unsigned char tx_buffer[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN)));
+static unsigned char rx_buffer[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN)));
+
+struct temac_reg {
+	unsigned long msw; /* Hard TEMAC MSW Data Register */
+	unsigned long lsw; /* Hard TEMAC LSW Data Register */
+	unsigned long ctl; /* Hard TEMAC Control Register */
+	unsigned long rdy; /* Hard TEMAC Ready Status */
+};
+
+struct ll_priv {
+	unsigned int ctrl;
+	struct temac_reg *regs;
+	unsigned int mode;
+	int phyaddr;
+
+	struct phy_device *phydev;
+	struct mii_dev *bus;
+};
+
+static void mtdcr_local(u32 reg, u32 val)
+{
+#if defined(CONFIG_PPC)
+	mtdcr(0x00, reg);
+	mtdcr(0x01, val);
+#endif
+}
+
+static u32 mfdcr_local(u32 reg)
+{
+	u32 val = 0;
+#if defined(CONFIG_PPC)
+	mtdcr(0x00, reg);
+	val = mfdcr(0x01);
+#endif
+	return val;
+}
+
+static void sdma_out_be32(struct ll_priv *priv, u32 offset, u32 val)
+{
+	if (priv->mode & DCR_BIT)
+		mtdcr_local(priv->ctrl + offset, val);
+	else
+		out_be32((u32 *)(priv->ctrl + offset * 4), val);
+}
+
+static u32 sdma_in_be32(struct ll_priv *priv, u32 offset)
+{
+	if (priv->mode & DCR_BIT)
+		return mfdcr_local(priv->ctrl + offset);
+
+	return in_be32((u32 *)(priv->ctrl + offset * 4));
+}
+
+#if defined(DEBUG) || defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+/* undirect hostif write to ll_temac */
+static void xps_ll_temac_hostif_set(struct eth_device *dev, int emac,
+			int phy_addr, int reg_addr, int phy_data)
+{
+	struct ll_priv *priv = dev->priv;
+	volatile struct temac_reg *regs = priv->regs;
+
+	regs->lsw = phy_data;
+	regs->ctl = CNTLREG_WRITE_ENABLE_MASK | MIIMWD;
+	regs->lsw = (phy_addr << 5) | reg_addr;
+	regs->ctl = CNTLREG_WRITE_ENABLE_MASK | MIIMAI | (emac << 10);
+	while (!(regs->rdy & XTE_RSE_MIIM_WR_MASK))
+		;
+}
+#endif
+
+/* undirect hostif read from ll_temac */
+static unsigned int xps_ll_temac_hostif_get(struct eth_device *dev,
+			int emac, int phy_addr, int reg_addr)
+{
+	struct ll_priv *priv = dev->priv;
+	volatile struct temac_reg *regs = priv->regs;
+
+	regs->lsw = (phy_addr << 5) | reg_addr;
+	regs->ctl = MIIMAI | (emac << 10);
+	while (!(regs->rdy & XTE_RSE_MIIM_RR_MASK))
+		;
+	return regs->lsw;
+}
+
+/* undirect write to ll_temac */
+static void xps_ll_temac_indirect_set(struct eth_device *dev,
+				int emac, int reg_offset, int reg_data)
+{
+	struct ll_priv *priv = dev->priv;
+	volatile struct temac_reg *regs = priv->regs;
+
+	regs->lsw = reg_data;
+	regs->ctl = CNTLREG_WRITE_ENABLE_MASK | (emac << 10) | reg_offset;
+	while (!(regs->rdy & XTE_RSE_CFG_WR_MASK))
+		;
+}
+
+/* undirect read from ll_temac */
+static int xps_ll_temac_indirect_get(struct eth_device *dev,
+			int emac, int reg_offset)
+{
+	struct ll_priv *priv = dev->priv;
+	volatile struct temac_reg *regs = priv->regs;
+
+	regs->ctl = (emac << 10) | reg_offset;
+	while (!(regs->rdy & XTE_RSE_CFG_RR_MASK))
+		;
+	return regs->lsw;
+}
+
+#ifdef DEBUG
+/* read from phy */
+static void read_phy_reg(struct eth_device *dev, int phy_addr)
+{
+	int j, result;
+	debug("phy%d ", phy_addr);
+	for (j = 0; j < 32; j++) {
+		result = xps_ll_temac_hostif_get(dev, 0, phy_addr, j);
+		debug("%d: 0x%x ", j, result);
+	}
+	debug("\n");
+}
+#endif
+
+/* setting ll_temac and phy to proper setting */
+static int xps_ll_temac_phy_ctrl(struct eth_device *dev)
+{
+#ifdef CONFIG_PHYLIB
+	int i;
+	unsigned int temp, speed;
+	struct ll_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) {
+		for (i = 31; i >= 0; i--) {
+			temp = xps_ll_temac_hostif_get(dev, 0, i, 1);
+			if ((temp & 0x0ffff) != 0x0ffff) {
+				debug("phy %x result %x\n", i, temp);
+				priv->phyaddr = i;
+				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 = XTE_EMMC_LINKSPD_1000;
+		break;
+	case 100:
+		speed = XTE_EMMC_LINKSPD_100;
+		break;
+	case 10:
+		speed = XTE_EMMC_LINKSPD_10;
+		break;
+	default:
+		return 0;
+	}
+	temp = xps_ll_temac_indirect_get(dev, 0, EMMC) &
+						(~XTE_EMMC_LINKSPEED_MASK);
+	temp |= speed;
+	xps_ll_temac_indirect_set(dev, 0, EMMC, temp);
+
+	return 1;
+#else
+	puts("Enable PHYLIB support!\n");
+	return 0;
+#endif
+}
+
+static inline int xps_ll_temac_dma_error(struct eth_device *dev)
+{
+	int err;
+	struct ll_priv *priv = dev->priv;
+
+	/* Check for TX and RX channel errrors.  */
+	err = sdma_in_be32(priv, TX_CHNL_STS) & CHNL_STS_ERROR_MASK;
+	err |= sdma_in_be32(priv, RX_CHNL_STS) & CHNL_STS_ERROR_MASK;
+	return err;
+}
+
+static void xps_ll_temac_reset_dma(struct eth_device *dev)
+{
+	u32 r;
+	struct ll_priv *priv = dev->priv;
+
+	/* Soft reset the DMA.  */
+	sdma_out_be32(priv, DMA_CONTROL_REG, 0x00000001);
+	while (sdma_in_be32(priv, DMA_CONTROL_REG) & 1)
+		;
+
+	/* Now clear the interrupts.  */
+	r = sdma_in_be32(priv, TX_CHNL_CTRL);
+	r &= ~XLLDMA_CR_IRQ_ALL_EN_MASK;
+	sdma_out_be32(priv, TX_CHNL_CTRL, r);
+
+	r = sdma_in_be32(priv, RX_CHNL_CTRL);
+	r &= ~XLLDMA_CR_IRQ_ALL_EN_MASK;
+	sdma_out_be32(priv, RX_CHNL_CTRL, r);
+
+	/* Now ACK pending IRQs.  */
+	sdma_out_be32(priv, TX_IRQ_REG, XLLDMA_IRQ_ALL_MASK);
+	sdma_out_be32(priv, RX_IRQ_REG, XLLDMA_IRQ_ALL_MASK);
+
+	/* Set tail-ptr mode, disable errors for both channels.  */
+	sdma_out_be32(priv, DMA_CONTROL_REG,
+			XLLDMA_DMACR_TAIL_PTR_EN_MASK |
+			XLLDMA_DMACR_RX_OVERFLOW_ERR_DIS_MASK |
+			XLLDMA_DMACR_TX_OVERFLOW_ERR_DIS_MASK);
+}
+
+/* bd init */
+static void xps_ll_temac_bd_init(struct eth_device *dev)
+{
+	struct ll_priv *priv = dev->priv;
+	memset((void *)&tx_bd, 0, sizeof(tx_bd));
+	memset((void *)&rx_bd, 0, sizeof(rx_bd));
+
+	rx_bd.phys_buf_p = &rx_buffer[0];
+
+	rx_bd.next_p = &rx_bd;
+	rx_bd.buf_len = PKTSIZE_ALIGN;
+	flush_cache((u32)&rx_bd, sizeof(tx_bd));
+	flush_cache((u32)rx_bd.phys_buf_p, PKTSIZE_ALIGN);
+
+	sdma_out_be32(priv, RX_CURDESC_PTR, (u32)&rx_bd);
+	sdma_out_be32(priv, RX_TAILDESC_PTR, (u32)&rx_bd);
+	sdma_out_be32(priv, RX_NXTDESC_PTR, (u32)&rx_bd); /* setup first fd */
+
+	tx_bd.phys_buf_p = &tx_buffer[0];
+	tx_bd.next_p = &tx_bd;
+
+	flush_cache((u32)&tx_bd, sizeof(tx_bd));
+	sdma_out_be32(priv, TX_CURDESC_PTR, (u32)&tx_bd);
+}
+
+static int ll_temac_send_sdma(struct eth_device *dev,
+				volatile void *buffer, int length)
+{
+	struct ll_priv *priv = dev->priv;
+
+	if (xps_ll_temac_dma_error(dev)) {
+		xps_ll_temac_reset_dma(dev);
+		xps_ll_temac_bd_init(dev);
+	}
+
+	memcpy(tx_buffer, (void *)buffer, length);
+	flush_cache((u32)tx_buffer, length);
+
+	tx_bd.stat = BDSTAT_SOP_MASK | BDSTAT_EOP_MASK |
+			BDSTAT_STOP_ON_END_MASK;
+	tx_bd.buf_len = length;
+	flush_cache((u32)&tx_bd, sizeof(tx_bd));
+
+	sdma_out_be32(priv, TX_CURDESC_PTR, (u32)&tx_bd);
+	sdma_out_be32(priv, TX_TAILDESC_PTR, (u32)&tx_bd); /* DMA start */
+
+	do {
+		flush_cache((u32)&tx_bd, sizeof(tx_bd));
+	} while (!(((volatile int)tx_bd.stat) & BDSTAT_COMPLETED_MASK));
+
+	return 0;
+}
+
+static int ll_temac_recv_sdma(struct eth_device *dev)
+{
+	int length;
+	struct ll_priv *priv = dev->priv;
+
+	if (xps_ll_temac_dma_error(dev)) {
+		xps_ll_temac_reset_dma(dev);
+		xps_ll_temac_bd_init(dev);
+	}
+
+	flush_cache((u32)&rx_bd, sizeof(rx_bd));
+
+	if (!(rx_bd.stat & BDSTAT_COMPLETED_MASK))
+		return 0;
+
+	/* Read out the packet info and start the DMA
+	   onto the second buffer to enable the ethernet rx
+	   path to run in parallel with sw processing
+	   packets.  */
+	length = rx_bd.app5 & 0x3FFF;
+	if (length > 0)
+		NetReceive(rx_bd.phys_buf_p, length);
+
+	/* flip the buffer and re-enable the DMA.  */
+	flush_cache((u32)rx_bd.phys_buf_p, length);
+
+	rx_bd.buf_len = PKTSIZE_ALIGN;
+	rx_bd.stat = 0;
+	rx_bd.app5 = 0;
+
+	flush_cache((u32)&rx_bd, sizeof(rx_bd));
+	sdma_out_be32(priv, RX_TAILDESC_PTR, (u32)&rx_bd);
+
+	return length;
+}
+
+#ifdef DEBUG
+static void debugll(struct eth_device *dev, int count)
+{
+	struct ll_priv *priv = dev->priv;
+	struct ll_fifo_s *ll_fifo = (void *)priv->ctrl;
+	printf("%d fifo isr 0x%08x, fifo_ier 0x%08x, fifo_rdfr 0x%08x, "
+		"fifo_rdfo 0x%08x fifo_rlr 0x%08x\n", count, ll_fifo->isr,
+		ll_fifo->ier, ll_fifo->rdfr, ll_fifo->rdfo, ll_fifo->rlf);
+}
+#endif
+
+static int ll_temac_send_fifo(struct eth_device *dev,
+					volatile void *buffer, int length)
+{
+	struct ll_priv *priv = dev->priv;
+	struct ll_fifo_s *ll_fifo = (void *)priv->ctrl;
+	u32 *buf = (u32 *)buffer;
+	u32 len, i, val;
+
+	len = (length / 4) + 1;
+
+	for (i = 0; i < len; i++) {
+		val = *buf++;
+		ll_fifo->tdfd = val;
+	}
+
+	ll_fifo->tlf = length;
+
+	return 0;
+}
+
+static int ll_temac_recv_fifo(struct eth_device *dev)
+{
+	struct ll_priv *priv = dev->priv;
+	struct ll_fifo_s *ll_fifo = (void *)priv->ctrl;
+	u32 len = 0;
+	u32 len2, i, val;
+	u32 *buf = (u32 *)&rx_buffer;
+
+	if (ll_fifo->isr & 0x04000000) {
+		ll_fifo->isr = 0xffffffff; /* reset isr */
+
+		/* while (ll_fifo->isr); */
+		len = ll_fifo->rlf & 0x7FF;
+		len2 = (len / 4) + 1;
+
+		for (i = 0; i < len2; i++) {
+			val = ll_fifo->rdfd;
+			*buf++ = val ;
+		}
+#ifdef DEBUG
+		debugll(dev, 1);
+#endif
+		NetReceive((uchar *)&rx_buffer, len);
+	}
+	return len;
+}
+
+/* setup mac addr */
+static int ll_temac_addr_setup(struct eth_device *dev)
+{
+	int val;
+
+	/* set up unicast MAC address filter */
+	val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
+		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
+	xps_ll_temac_indirect_set(dev, 0, UAW0, val);
+	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
+	xps_ll_temac_indirect_set(dev, 0, UAW1, val);
+
+	return 0;
+}
+
+static int xps_ll_temac_init(struct eth_device *dev, bd_t *bis)
+{
+	struct ll_priv *priv = dev->priv;
+	struct ll_fifo_s *ll_fifo = (void *)priv->ctrl;
+
+	if (priv->mode & SDMA_BIT) {
+		xps_ll_temac_reset_dma(dev);
+		xps_ll_temac_bd_init(dev);
+	} else {
+		ll_fifo->tdfr = 0x000000a5; /* set fifo length */
+		ll_fifo->rdfr = 0x000000a5;
+		/* ll_fifo->isr = 0x0; */
+		/* ll_fifo->ier = 0x0; */
+	}
+
+	xps_ll_temac_indirect_set(dev, 0, MC,
+				MDIO_ENABLE_MASK | MDIO_CLOCK_DIV_100MHz);
+
+	/* Promiscuous mode disable */
+	xps_ll_temac_indirect_set(dev, 0, AFM, 0x00000000);
+	/* Enable Receiver */
+	xps_ll_temac_indirect_set(dev, 0, RCW1, 0x10000000);
+	/* Enable Transmitter */
+	xps_ll_temac_indirect_set(dev, 0, TC, 0x10000000);
+	return 0;
+}
+
+/* halt device */
+static void ll_temac_halt(struct eth_device *dev)
+{
+#ifdef ETH_HALTING
+	struct ll_priv *priv = dev->priv;
+	/* Disable Receiver */
+	xps_ll_temac_indirect_set(dev, 0, RCW1, 0x00000000);
+	/* Disable Transmitter */
+	xps_ll_temac_indirect_set(dev, 0, TC, 0x00000000);
+
+	if (priv->mode & SDMA_BIT) {
+		sdma_out_be32(priv->ctrl, DMA_CONTROL_REG, 0x00000001);
+		while (sdma_in_be32(priv->ctrl, DMA_CONTROL_REG) & 1)
+			;
+	}
+	/* reset fifos */
+#endif
+}
+
+static int ll_temac_init(struct eth_device *dev, bd_t *bis)
+{
+	struct ll_priv *priv = dev->priv;
+#if DEBUG
+	int i;
+#endif
+	xps_ll_temac_init(dev, bis);
+
+	printf("%s: Xilinx XPS LocalLink Tri-Mode Ether MAC #%d at 0x%08X.\n",
+		dev->name, 0, dev->iobase);
+
+#if DEBUG
+	for (i = 0; i < 32; i++)
+		read_phy_reg(dev, i);
+#endif
+
+	if (!xps_ll_temac_phy_ctrl(dev)) {
+		ll_temac_halt(dev);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int ll_temac_miiphy_read(const char *devname, uchar addr,
+							uchar reg, ushort *val)
+{
+	struct eth_device *dev = eth_get_dev();
+
+	*val = xps_ll_temac_hostif_get(dev, 0, addr, reg); /* emac = 0 */
+
+	debug("%s 0x%x, 0x%x, 0x%x\n", __func__, addr, reg, *val);
+	return 0;
+}
+
+static int ll_temac_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);
+
+	xps_ll_temac_hostif_set(dev, 0, addr, reg, val);
+
+	return 0;
+}
+
+static int ll_temac_bus_reset(struct mii_dev *bus)
+{
+	debug("Just bus reset\n");
+	return 0;
+}
+
+/* mode bits: 0bit - fifo(0)/sdma(1):SDMA_BIT, 1bit - no dcr(0)/dcr(1):DCR_BIT
+ * ctrl - control address for file/sdma */
+int xilinx_ll_temac_initialize(bd_t *bis, unsigned long base_addr,
+							int mode, int ctrl)
+{
+	struct eth_device *dev;
+	struct ll_priv *priv;
+
+	dev = calloc(1, sizeof(*dev));
+	if (dev == NULL)
+		return -1;
+
+	dev->priv = calloc(1, sizeof(struct ll_priv));
+	if (dev->priv == NULL) {
+		free(dev);
+		return -1;
+	}
+
+	priv = dev->priv;
+
+	sprintf(dev->name, "Xlltem.%lx", base_addr);
+
+	dev->iobase = base_addr;
+	priv->regs = (void *) (base_addr + 0x20);
+	priv->ctrl = ctrl;
+	priv->mode = mode;
+
+#ifdef CONFIG_PHY_ADDR
+	priv->phyaddr = CONFIG_PHY_ADDR;
+#else
+	priv->phyaddr = -1;
+#endif
+
+	dev->init = ll_temac_init;
+	dev->halt = ll_temac_halt;
+	dev->write_hwaddr = ll_temac_addr_setup;
+
+	if (priv->mode & SDMA_BIT) {
+		dev->send = ll_temac_send_sdma;
+		dev->recv = ll_temac_recv_sdma;
+	} else {
+		dev->send = ll_temac_send_fifo;
+		dev->recv = ll_temac_recv_fifo;
+	}
+
+	eth_register(dev);
+
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB)
+	miiphy_register(dev->name, ll_temac_miiphy_read, ll_temac_miiphy_write);
+	priv->bus = miiphy_get_dev_by_name(dev->name);
+	priv->bus->reset = ll_temac_bus_reset;
+#endif
+	return 1;
+}
diff --git a/include/netdev.h b/include/netdev.h
index 26ce13d..cebae3b 100644
--- a/include/netdev.h
+++ b/include/netdev.h
@@ -92,6 +92,8 @@ int uec_standard_init(bd_t *bis);
 int uli526x_initialize(bd_t *bis);
 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,
+							int mode, int ctrl);
 int sh_eth_initialize(bd_t *bis);
 int dm9000_initialize(bd_t *bis);
 int fecmxc_initialize(bd_t *bis);
-- 
1.5.5.6

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

[Michal Simek]

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 */
+
+/* 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,
-- 
1.5.5.6

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

[Marek Vasut]

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!

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

[Michal Simek]

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

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

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

[Marek Vasut]

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!

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

[Michal Simek]

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    ... at ............
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


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

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

[Marek Vasut]

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    ... at ............
> 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

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

[Michal Simek]

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

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

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

[Marek Vasut]

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

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

[Michal Simek]

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

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
A witty saying proves nothing, but saying  something  pointless  gets
people's attention.

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If programming was easy, they wouldn't need something as  complicated
as a human being to do it, now would they?
                       - L. Wall & R. L. Schwartz, _Programming Perl_

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

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110831/9c258c03/attachment.pgp 

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

[Michal Simek]

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