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

[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

[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 v2] net: ll_temac: Add LL TEMAC driver to u-boot

[Mike Frysinger]

On Tuesday, August 30, 2011 08:05:18 Michal Simek wrote:
> +	volatile struct temac_reg *regs = priv->regs;
> +
> +	regs->lsw = (phy_addr << 5) | reg_addr;
> +	regs->ctl = MIIMAI | (emac << 10);

you should still be going through read/write i/o helper funcs rather than 
relying on the volatile markings

the rest looks sane
-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/8aa274b5/attachment.pgp 

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

[Marek Vasut]

On Wednesday, August 31, 2011 09:26:27 PM Mike Frysinger wrote:
> On Tuesday, August 30, 2011 08:05:18 Michal Simek wrote:
> > +	volatile struct temac_reg *regs = priv->regs;
> > +
> > +	regs->lsw = (phy_addr << 5) | reg_addr;
> > +	regs->ctl = MIIMAI | (emac << 10);
> 
> you should still be going through read/write i/o helper funcs rather than
> relying on the volatile markings

Oh, I didn't comment on the TEMAC. I'll have to go through it.

Cheers

> 
> the rest looks sane
> -mike

[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 v2] net: ll_temac: Add LL TEMAC driver to u-boot

[Marek Vasut]

On Tuesday, August 30, 2011 02:05:18 PM Michal Simek wrote:
> 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 */
> +

Hi,

ok, this is the usual stuff -- use (1 << n) and struct temac_regs {...};

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

maybe app[4] ?

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

What are these magic values with no description ?

> +}
> +
> +static u32 mfdcr_local(u32 reg)
> +{
> +	u32 val = 0;
> +#if defined(CONFIG_PPC)
> +	mtdcr(0x00, reg);
> +	val = mfdcr(0x01);
> +#endif

dtto

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

If priv->ctrl was well defined structure, you won't need this cast, offset etc. 
Besides I see you have it defined as unsigned int ... make it u32 if you want to 
define registers that way. Still, struct is way to go.

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

DITTO, besides, in the previous code, you only do out_be32() in else branch, 
here you do it unconditionally. Maybe you can make them look similar.

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

writel() or out_be32()

> +	while (!(regs->rdy & XTE_RSE_MIIM_WR_MASK))
> +		;

No endless loops

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

writel() or out_be32(), please fix globally.

> +	while (!(regs->rdy & XTE_RSE_MIIM_RR_MASK))
> +		;

no endless loops, please fix globally

> +	return regs->lsw;

return readl() I guess ... 

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

No volatiles please.

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

Do you need those parenthesis ? Maybe you can divide this into ...

temp = xps_ll...();
temp &= ~XTE...;
temp |= speed;
xps_ll...();

Might be a bit more readable.

> +	temp |= speed;
> +	xps_ll_temac_indirect_set(dev, 0, EMMC, temp);
> +
> +	return 1;
> +#else
> +	puts("Enable PHYLIB support!\n");
> +	return 0;
> +#endif

Please move this #ifdef CONFIG_PHYLIB at the begining of the file and emit a 
compile-time warning.

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

Ok, maybe #define SDMA_RESET 0x1 won't hurt here.

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

Do you need the cast?

> +
> +	rx_bd.phys_buf_p = &rx_buffer[0];

rx_buffer would be sufficient here.

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

Whoa ... volatile int is really dangerous. And please add timeout.
> +
> +	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;

0x3fff should be explained here.

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

Do you need the len at all ?

for (i = 0; i < length; i +=4 )
	writel(*buf++, ll_fifo->tdfd);

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

Another magic value, please fix globally.

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

Why do you have this dead code here?

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

Magic values.

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

Magic.

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

Dead comment?

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

Here you'd just load the struct :)

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

The driver has some way to go, but it's mostly coding style. One or two more 
rounds and it's ready.

Cheers!

[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

[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 v2] net: ll_temac: Add LL TEMAC driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
> On Tuesday, August 30, 2011 08:05:18 Michal Simek wrote:
>> +	volatile struct temac_reg *regs = priv->regs;
>> +
>> +	regs->lsw = (phy_addr << 5) | reg_addr;
>> +	regs->ctl = MIIMAI | (emac << 10);
> 
> you should still be going through read/write i/o helper funcs rather than 
> relying on the volatile markings

Fixed.

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 v2] net: ll_temac: Add LL TEMAC driver to u-boot

[Michal Simek]

Marek Vasut wrote:
> On Tuesday, August 30, 2011 02:05:18 PM Michal Simek wrote:
>> 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 */
>> +
> 
> Hi,
> 
> ok, this is the usual stuff -- use (1 << n) and struct temac_regs {...};

keep it till this is solved for axi enet.

> 
>> +#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;
> 
> maybe app[4] ?

Just follow temac documentation.

> 
>> +};
>> +
>> +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
> 
> What are these magic values with no description ?

DCR number. Fix that.

> 
>> +}
>> +
>> +static u32 mfdcr_local(u32 reg)
>> +{
>> +	u32 val = 0;
>> +#if defined(CONFIG_PPC)
>> +	mtdcr(0x00, reg);
>> +	val = mfdcr(0x01);
>> +#endif
> 
> dtto

too.

> 
>> +	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);
> 
> If priv->ctrl was well defined structure, you won't need this cast, offset etc. 
> Besides I see you have it defined as unsigned int ... make it u32 if you want to 
> define registers that way. Still, struct is way to go.

Not sure if struct is way to go because offset for DCR and PLB is the same but register length
is different that's why I am using these helpers. It is different case than was for axi emac where
is no DCR access.

> 
>> +}
>> +
>> +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));
> 
> DITTO, besides, in the previous code, you only do out_be32() in else branch, 
> here you do it unconditionally. Maybe you can make them look similar.

Because if DCR_BIT is set it ends on return. Above I should use
one more line to return.

> 
>> +}
>> +
>> +#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);
> 
> writel() or out_be32()

In Mike's comment.

> 
>> +	while (!(regs->rdy & XTE_RSE_MIIM_WR_MASK))
>> +		;
> 
> No endless loops

Fixed.

> 
>> +}
>> +#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);
> 
> writel() or out_be32(), please fix globally.

as above

> 
>> +	while (!(regs->rdy & XTE_RSE_MIIM_RR_MASK))
>> +		;
> 
> no endless loops, please fix globally

as above

> 
>> +	return regs->lsw;
> 
> return readl() I guess ... 

as above
> 
>> +}
>> +
>> +/* 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;
> 
> No volatiles please.

as above

> 
>> +
>> +	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);
> 
> Do you need those parenthesis ? Maybe you can divide this into ...
> 
> temp = xps_ll...();
> temp &= ~XTE...;
> temp |= speed;
> xps_ll...();
> 
> Might be a bit more readable.

fixed.

> 
>> +	temp |= speed;
>> +	xps_ll_temac_indirect_set(dev, 0, EMMC, temp);
>> +
>> +	return 1;
>> +#else
>> +	puts("Enable PHYLIB support!\n");
>> +	return 0;
>> +#endif
> 
> Please move this #ifdef CONFIG_PHYLIB at the begining of the file and emit a 
> compile-time warning.

Reported in axi thread.

> 
>> +}
>> +
>> +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)
>> +		;
> 
> Ok, maybe #define SDMA_RESET 0x1 won't hurt here.

Done.

> 
>> +
>> +	/* 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));
> 
> Do you need the cast?

no, fixed.

> 
>> +
>> +	rx_bd.phys_buf_p = &rx_buffer[0];
> 
> rx_buffer would be sufficient here.

fix for tx_buffer too.

> 
>> +
>> +	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));
> 
> Whoa ... volatile int is really dangerous. And please add timeout.
>> +
>> +	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;
> 
> 0x3fff should be explained here.

Add comment to that line.

> 
>> +	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;
>> +
> 
> Do you need the len at all ?
> 
> for (i = 0; i < length; i +=4 )
> 	writel(*buf++, ll_fifo->tdf
d);

fixed.

> 
>> +	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) {
> 
> Another magic value, please fix globally.

use comments.

> 
>> +		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; */
> 
> Why do you have this dead code here?

Fixed.

> 
>> +	}
>> +
>> +	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);
> 
> Magic values.

Extend comment.


> 
>> +	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 */
> 
> Magic.
> 
>> +	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 */
> 
> Dead comment?

More question if make sense to do it - probably not - I removed it.

> 
>> +#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);
> 
> Here you'd just load the struct :)

It is changed.

> 
>> +	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);
> 
> The driver has some way to go, but it's mostly coding style. One or two more 
> rounds and it's ready.

I will look at fifo mode and will send v3.

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

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