All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
@ 2011-09-01 11:39 Michal Simek
  2011-09-01 11:39 ` [U-Boot] [PATCH v2] net: axi_ethernet: Add " Michal Simek
  2011-09-01 13:06 ` [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC " Wolfgang Denk
  0 siblings, 2 replies; 17+ messages in thread
From: Michal Simek @ 2011-09-01 11:39 UTC (permalink / raw)
  To: u-boot

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

v3: Use helper functions for fifo mode
    Use helper functions for indirect accesses
    Code cleanup
    Add comments for MAGIC values
    Simplify code in fifo mode
---
 drivers/net/Makefile          |    1 +
 drivers/net/xilinx_ll_temac.c |  666 +++++++++++++++++++++++++++++++++++++++++
 include/netdev.h              |    2 +
 3 files changed, 669 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..1639363
--- /dev/null
+++ b/drivers/net/xilinx_ll_temac.c
@@ -0,0 +1,666 @@
+/*
+ * 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
+
+#if !defined(CONFIG_PHYLIB)
+# error LL_TEMAC requires PHYLIB
+#endif
+
+#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
+
+/* DMA control bit */
+#define DMA_CONTROL_RESET	0x1
+
+/* 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 LL_FIFO_ISR_RC_COMPLETE	0x04000000
+
+#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 {
+	u32 isr; /* Interrupt Status Register 0x0 */
+	u32 ier; /* Interrupt Enable Register 0x4 */
+	u32 tdfr; /* Transmit data FIFO reset 0x8 */
+	u32 tdfv; /* Transmit data FIFO Vacancy 0xC */
+	u32 tdfd; /* Transmit data FIFO 32bit wide data write port 0x10 */
+	u32 tlf; /* Write Transmit Length FIFO 0x14 */
+	u32 rdfr; /* Read Receive data FIFO reset 0x18 */
+	u32 rdfo; /* Receive data FIFO Occupancy 0x1C */
+	u32 rdfd; /* Read Receive data FIFO 32bit wide data read port 0x20 */
+	u32 rlf; /* Read Receive Length FIFO 0x24 */
+	u32 llr; /* Read LocalLink reset 0x28 */
+};
+
+static u8 tx_buffer[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN)));
+static u8 rx_buffer[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN)));
+
+struct temac_reg {
+	u32 reserved[8];
+	u32 msw; /* Hard TEMAC MSW Data Register */
+	u32 lsw; /* Hard TEMAC LSW Data Register */
+	u32 ctl; /* Hard TEMAC Control Register */
+	u32 rdy; /* Hard TEMAC Ready Status */
+};
+
+struct ll_priv {
+	u32 ctrl;
+	u32 mode;
+	int phyaddr;
+
+	struct phy_device *phydev;
+	struct mii_dev *bus;
+};
+
+#define XILINX_INDIRECT_DCR_ADDRESS_REG	0
+#define XILINX_INDIRECT_DCR_ACCESS_REG	1
+
+static void mtdcr_local(u32 reg, u32 val)
+{
+#if defined(CONFIG_PPC)
+	mtdcr(XILINX_INDIRECT_DCR_ADDRESS_REG, reg);
+	mtdcr(XILINX_INDIRECT_DCR_ACCESS_REG, val);
+#endif
+}
+
+static u32 mfdcr_local(u32 reg)
+{
+	u32 val = 0;
+#if defined(CONFIG_PPC)
+	mtdcr(XILINX_INDIRECT_DCR_ADDRESS_REG, reg);
+	val = mfdcr(XILINX_INDIRECT_DCR_ACCESS_REG);
+#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));
+}
+
+static void xps_ll_temac_check_status(struct temac_reg *regs, int mask)
+{
+	u32 timeout = 2000;
+	while (timeout && (!(in_be32(&regs->rdy) & mask)))
+		timeout--;
+
+	if (!timeout)
+		printf("%s: Timeout\n", __func__);
+}
+
+#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 temac_reg *regs = (struct temac_reg *)dev->iobase;
+
+	out_be32(&regs->lsw, phy_data);
+	out_be32(&regs->ctl, CNTLREG_WRITE_ENABLE_MASK | MIIMWD);
+	out_be32(&regs->lsw, (phy_addr << 5) | reg_addr);
+	out_be32(&regs->ctl, CNTLREG_WRITE_ENABLE_MASK | MIIMAI | (emac << 10));
+	xps_ll_temac_check_status(regs, 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 temac_reg *regs = (struct temac_reg *)dev->iobase;
+
+	out_be32(&regs->lsw, (phy_addr << 5) | reg_addr);
+	out_be32(&regs->ctl, MIIMAI | (emac << 10));
+	xps_ll_temac_check_status(regs, XTE_RSE_MIIM_RR_MASK);
+	return in_be32(&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 temac_reg *regs = (struct temac_reg *)dev->iobase;
+
+	out_be32(&regs->lsw, reg_data);
+	out_be32(&regs->ctl,
+			CNTLREG_WRITE_ENABLE_MASK | (emac << 10) | reg_offset);
+	xps_ll_temac_check_status(regs, 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 temac_reg *regs = (struct temac_reg *)dev->iobase;
+
+	out_be32(&regs->ctl, (emac << 10) | reg_offset);
+	xps_ll_temac_check_status(regs, XTE_RSE_CFG_RR_MASK);
+	return in_be32(&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)
+{
+	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);
+	temp &= ~XTE_EMMC_LINKSPEED_MASK;
+	temp |= speed;
+	xps_ll_temac_indirect_set(dev, 0, EMMC, temp);
+
+	return 1;
+}
+
+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, DMA_CONTROL_RESET);
+	while (sdma_in_be32(priv, DMA_CONTROL_REG) & DMA_CONTROL_RESET)
+		;
+
+	/* 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(&tx_bd, 0, sizeof(tx_bd));
+	memset(&rx_bd, 0, sizeof(rx_bd));
+
+	rx_bd.phys_buf_p = rx_buffer;
+
+	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;
+	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 (!(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; /* max length mask */
+	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,
+		in_be32(&ll_fifo->isr), in_be32(&ll_fifo->ier),
+		in_be32(&ll_fifo->rdfr), in_be32(&ll_fifo->rdfo),
+		in_be32(&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 i;
+
+	for (i = 0; i < length; i += 4)
+		out_be32(&ll_fifo->tdfd, *buf++);
+
+	out_be32(&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 i, len = 0;
+	u32 *buf = (u32 *)&rx_buffer;
+
+	if (in_be32(&ll_fifo->isr) & LL_FIFO_ISR_RC_COMPLETE) {
+		out_be32(&ll_fifo->isr, 0xffffffff); /* reset isr */
+
+		/* while (ll_fifo->isr); */
+		len = in_be32(&ll_fifo->rlf) & 0x7FF;
+
+		for (i = 0; i < len; i += 4)
+			*buf++ = in_be32(&ll_fifo->rdfd);
+
+#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 {
+		out_be32(&ll_fifo->tdfr, 0x000000a5); /* Fifo reset key */
+		out_be32(&ll_fifo->rdfr, 0x000000a5); /* Fifo reset key */
+		out_be32(&ll_fifo->isr, 0xFFFFFFFF); /* Reset status register */
+		out_be32(&ll_fifo->ier, 0); /* Disable all IRQs */
+	}
+
+	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, 0);
+	/* Enable Receiver - RX bit */
+	xps_ll_temac_indirect_set(dev, 0, RCW1, 0x10000000);
+	/* Enable Transmitter - TX bit */
+	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, 0);
+	/* Disable Transmitter */
+	xps_ll_temac_indirect_set(dev, 0, TC, 0);
+
+	if (priv->mode & SDMA_BIT) {
+		sdma_out_be32(priv->ctrl, DMA_CONTROL_REG, DMA_CONTROL_RESET);
+		while (sdma_in_be32(priv->ctrl, DMA_CONTROL_REG)
+							& DMA_CONTROL_RESET)
+			;
+	}
+#endif
+}
+
+static int ll_temac_init(struct eth_device *dev, bd_t *bis)
+{
+#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, unsigned long 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->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)
+	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..e976659 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, unsigned long ctrl);
 int sh_eth_initialize(bd_t *bis);
 int dm9000_initialize(bd_t *bis);
 int fecmxc_initialize(bd_t *bis);
-- 
1.5.5.6

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v2] net: axi_ethernet: Add driver to u-boot
  2011-09-01 11:39 [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot Michal Simek
@ 2011-09-01 11:39 ` Michal Simek
  2011-09-01 13:17   ` Wolfgang Denk
  2011-09-01 13:06 ` [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC " Wolfgang Denk
  1 sibling, 1 reply; 17+ messages in thread
From: Michal Simek @ 2011-09-01 11:39 UTC (permalink / raw)
  To: u-boot

Add axi_ethernet driver for little-endian Microblaze.

RX/TX BDs and rxframe buffer are shared among all axi_ethernet MACs.
Only one MAC can work in one time.

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

---

v2: Fix cammelcase weirdness
    Create mdio_wait function with timeouts
    Synchronize debug messages
    Remove base address + offset notation -> use struct instead
---
 drivers/net/Makefile          |    1 +
 drivers/net/xilinx_axi_emac.c |  629 +++++++++++++++++++++++++++++++++++++++++
 include/netdev.h              |    2 +
 3 files changed, 632 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..1c90b0e
--- /dev/null
+++ b/drivers/net/xilinx_axi_emac.c
@@ -0,0 +1,629 @@
+/*
+ * 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>
+
+#if !defined(CONFIG_PHYLIB)
+# error AXI_ETHERNET requires PHYLIB
+#endif
+
+/* 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 */
+
+#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)));
+
+struct axi_regs {
+	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 */
+};
+
+/* 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 inline int mdio_wait(struct eth_device *dev)
+{
+	struct axi_regs *regs = (struct axi_regs *)dev->iobase;
+
+	u32 timeout = 200;
+	/* Wait till MDIO interface is ready to accept a new transaction. */
+	while (timeout && (!(in_be32(&regs->mdio_mcr)
+						& XAE_MDIO_MCR_READY_MASK)))
+		timeout--;
+
+	if (!timeout) {
+		printf("%s: Timeout\n", __func__);
+		return 1;
+	}
+	return 0;
+}
+
+static u16 phyread(struct eth_device *dev, u32 phyaddress, u32 registernum)
+{
+	struct axi_regs *regs = (struct axi_regs *)dev->iobase;
+	u32 mdioctrlreg = 0;
+
+	if (mdio_wait(dev))
+		return 0;
+
+	mdioctrlreg = ((phyaddress << XAE_MDIO_MCR_PHYAD_SHIFT) &
+			XAE_MDIO_MCR_PHYAD_MASK) |
+			((registernum << XAE_MDIO_MCR_REGAD_SHIFT)
+			& XAE_MDIO_MCR_REGAD_MASK) |
+			XAE_MDIO_MCR_INITIATE_MASK |
+			XAE_MDIO_MCR_OP_READ_MASK;
+
+	out_be32(&regs->mdio_mcr, mdioctrlreg);
+
+	if (mdio_wait(dev))
+		return 0;
+
+	/* Read data */
+	return in_be32(&regs->mdio_mrd);
+}
+
+static void phywrite(struct eth_device *dev, u32 phyaddress, u32 registernum,
+								u32 data)
+{
+	struct axi_regs *regs = (struct axi_regs *)dev->iobase;
+	u32 mdioctrlreg = 0;
+
+	if (mdio_wait(dev))
+		return;
+
+	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 */
+	out_be32(&regs->mdio_mwd, data);
+
+	out_be32(&regs->mdio_mcr, mdioctrlreg);
+
+	mdio_wait(dev);
+}
+
+
+/* setting axi emac and phy to proper setting */
+static int setup_phy(struct eth_device *dev)
+{
+	int i;
+	unsigned int speed;
+	u16 phyreg;
+	u32 emmc_reg;
+	struct axidma_priv *priv = dev->priv;
+	struct axi_regs *regs = (struct axi_regs *)dev->iobase;
+	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("axiemac: 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 = in_be32(&regs->emmc);
+	emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK;
+	emmc_reg |= speed;
+
+	/* Write new speed setting out to Axi Ethernet */
+	out_be32(&regs->emmc, 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;
+}
+
+/* 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 int axi_ethernet_init(struct eth_device *dev)
+{
+	struct axi_regs *regs = (struct axi_regs *)dev->iobase;
+	/*
+	 * Check the status of the MgtRdy bit in the interrupt status
+	 * registers. This must be done to allow the MGT clock to become stable
+	 * for the Sgmii and 1000BaseX PHY interfaces. No other register reads
+	 * will be valid until this bit is valid.
+	 * The bit is always a 1 for all other PHY interfaces.
+	 */
+	u32 timeout = 200;
+	while (timeout && (!(in_be32(&regs->is) & XAE_INT_MGTRDY_MASK)))
+		timeout--;
+
+	if (!timeout) {
+		printf("%s: Timeout\n", __func__);
+		return 1;
+	}
+
+	/* Stop the device and reset HW */
+	/* Disable interrupts */
+	out_be32(&regs->ie, 0);
+
+	/* Disable the receiver */
+	out_be32(&regs->rcw1, in_be32(&regs->rcw1) & ~XAE_RCW1_RX_MASK);
+
+	/*
+	 * Stopping the receiver in mid-packet causes a dropped packet
+	 * indication from HW. Clear it.
+	 */
+	/* set the interrupt status register to clear the interrupt */
+	out_be32(&regs->is, XAE_INT_RXRJECT_MASK);
+
+	/* Setup HW */
+	/* Set default MDIO divisor */
+	out_be32(&regs->mdio_mc, XAE_MDIO_DIV_DFT | XAE_MDIO_MC_MDIOEN_MASK);
+
+	debug("axiemac: InitHw done\n");
+	return 0;
+}
+
+static int axiemac_setup_mac(struct eth_device *dev)
+{
+	struct axi_regs *regs = (struct axi_regs *)dev->iobase;
+
+	/* Set the MAC address */
+	int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
+		(dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
+	out_be32(&regs->uaw0, val);
+
+	val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
+	val |= in_be32(&regs->uaw1) & ~XAE_UAW1_UNICASTADDR_MASK;
+	out_be32(&regs->uaw1, val);
+	return 0;
+}
+
+/* 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;
+	}
+
+	if (!timeout)
+		printf("%s: Timeout\n", __func__);
+}
+
+static int axiemac_init(struct eth_device *dev, bd_t * bis)
+{
+	struct axidma_priv *priv = dev->priv;
+	struct axi_regs *regs = (struct axi_regs *)dev->iobase;
+	debug("axiemac: 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. */
+	if (axi_ethernet_init(dev))
+		return -1;
+
+	/* 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;
+
+	/* Setup the BD. */
+	memset(&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 */
+	out_be32(&regs->tc, XAE_TC_TX_MASK);
+	/* enable RX */
+	out_be32(&regs->rcw1, XAE_RCW1_RX_MASK);
+
+	/* PHY setup */
+	if (!setup_phy(dev)) {
+		axiemac_halt(dev);
+		return -1;
+	}
+
+	debug("axiemac: 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(&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("axiemac: Waiting for tx to be done\n");
+	while (!priv->dmatx->status &
+				(XAXIDMA_IRQ_DELAY_MASK | XAXIDMA_IRQ_IOC_MASK))
+		;
+
+	debug("axiemac: Sending 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("axiemac: RX data ready\n");
+
+	/* Disable IRQ for a moment till packet is handled */
+	priv->dmarx->control &= ~XAXIDMA_IRQ_ALL_MASK;
+
+	length = rx_bd.app4 & 0xFFFF; /* max length mask */
+#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(&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("axiemac: RX completed, 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("axiemac: Read MII 0x%x, 0x%x, 0x%x\n", 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("axiemac: Write MII 0x%x, 0x%x, 0x%x\n", addr, reg, val);
+	phywrite(dev, addr, reg, val);
+	return 0;
+}
+
+static int axiemac_bus_reset(struct mii_dev *bus)
+{
+	debug("axiemac: Bus reset\n");
+	return 0;
+}
+
+int xilinx_axiemac_initialize(bd_t *bis, unsigned long base_addr,
+							unsigned long 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;
+	dev->write_hwaddr = axiemac_setup_mac;
+
+#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 e976659..3125b4a 100644
--- a/include/netdev.h
+++ b/include/netdev.h
@@ -90,6 +90,8 @@ 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,
+							unsigned long 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-01 11:39 [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot Michal Simek
  2011-09-01 11:39 ` [U-Boot] [PATCH v2] net: axi_ethernet: Add " Michal Simek
@ 2011-09-01 13:06 ` Wolfgang Denk
  2011-09-01 13:17   ` Michal Simek
  1 sibling, 1 reply; 17+ messages in thread
From: Wolfgang Denk @ 2011-09-01 13:06 UTC (permalink / raw)
  To: u-boot

Dear Michal Simek,

In message <1314877154-14536-1-git-send-email-monstr@monstr.eu> you 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.


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

Can we please get rid of these functions?  As mentioned many, many
times before, we discourage all use of "base address plus offset" to
access any device registers etc.

These functions here re-introduce such accesses, and this is something
I will not accept.


> +	/* Soft reset the DMA.  */
> +	sdma_out_be32(priv, DMA_CONTROL_REG, DMA_CONTROL_RESET);
> +	while (sdma_in_be32(priv, DMA_CONTROL_REG) & DMA_CONTROL_RESET)
> +		;

I think this has been pointed out before: we do not accept such
potentially endless loops. Please fix this  (and integrate theother
previously made review remarks).  Thanks.

> +	do {
> +		flush_cache((u32)&tx_bd, sizeof(tx_bd));
> +	} while (!(tx_bd.stat & BDSTAT_COMPLETED_MASK));

Ditto. Please fix globally.

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

Incorrect multiline comment style; please fix globally.

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
The project was large enough and management communication poor enough
to prompt many members of the team to see themselves  as  contestants
making  brownie  points,  rather  than as builders making programming
products. Each suboptimized  his  piece  to  meet  his  targets;  few
stopped to think about the total effect on the customer.
                              - Fred Brooks, "The Mythical Man Month"

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-01 13:06 ` [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC " Wolfgang Denk
@ 2011-09-01 13:17   ` Michal Simek
  2011-09-01 14:09     ` Wolfgang Denk
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2011-09-01 13:17 UTC (permalink / raw)
  To: u-boot

Wolfgang Denk wrote:
> Dear Michal Simek,
> 
> In message <1314877154-14536-1-git-send-email-monstr@monstr.eu> you 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.
> 
> 
>> +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));
>> +}
> 
> Can we please get rid of these functions?  As mentioned many, many
> times before, we discourage all use of "base address plus offset" to
> access any device registers etc.
> 
> These functions here re-introduce such accesses, and this is something
> I will not accept.

Ok. How to do it?

For bus access it is necessary to use 4B offsets for DCR just 1B
and one system can contains two MACs where the first use 4B offset and the second
1B.


>> +	/* Soft reset the DMA.  */
>> +	sdma_out_be32(priv, DMA_CONTROL_REG, DMA_CONTROL_RESET);
>> +	while (sdma_in_be32(priv, DMA_CONTROL_REG) & DMA_CONTROL_RESET)
>> +		;
> 
> I think this has been pointed out before: we do not accept such
> potentially endless loops. Please fix this  (and integrate theother
> previously made review remarks).  Thanks.
> 
>> +	do {
>> +		flush_cache((u32)&tx_bd, sizeof(tx_bd));
>> +	} while (!(tx_bd.stat & BDSTAT_COMPLETED_MASK));
> 
> Ditto. Please fix globally.

I forget. Sorry. Will fix it.

> 
>> +	/* 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.  */
> 
> Incorrect multiline comment style; please fix globally.

Fixed and checked.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v2] net: axi_ethernet: Add driver to u-boot
  2011-09-01 11:39 ` [U-Boot] [PATCH v2] net: axi_ethernet: Add " Michal Simek
@ 2011-09-01 13:17   ` Wolfgang Denk
  2011-09-02  9:02     ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Wolfgang Denk @ 2011-09-01 13:17 UTC (permalink / raw)
  To: u-boot

Dear Michal Simek,

In message <1314877154-14536-2-git-send-email-monstr@monstr.eu> you wrote:
> Add axi_ethernet driver for little-endian Microblaze.
> 
> RX/TX BDs and rxframe buffer are shared among all axi_ethernet MACs.
> Only one MAC can work in one time.
> 
> Signed-off-by: Michal Simek <monstr@monstr.eu>

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

Incorrect multiline comment style. Please fix globally.


> +	u32 timeout = 200;
> +	/* Wait till MDIO interface is ready to accept a new transaction. */
> +	while (timeout && (!(in_be32(&regs->mdio_mcr)
> +						& XAE_MDIO_MCR_READY_MASK)))
> +		timeout--;

Multiline statement - braches needed.

Also, add some udelay() or similar to have a defined length of the
timeout.  Please fix globally.


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

Please ALWAYS use I/O accessors to access device registers.  Please
fix globally.

> +static int IsRxReady(struct eth_device *dev)

As requested before:  Please get rid of these CamelCaps identifiers!!!


> +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("axiemac: Read MII 0x%x, 0x%x, 0x%x\n", addr, reg, *val);
> +	return 0;

Please separate declarations and code by a single blank line.  Please
fix globally.


Why does this function return int when you uncondsitionally always
return 0 only?

> +}
> +
> +static int axiemac_miiphy_write(const char *devname, uchar addr,
> +							uchar reg, ushort val)
> +{
> +	struct eth_device *dev = eth_get_dev();
> +	debug("axiemac: Write MII 0x%x, 0x%x, 0x%x\n", addr, reg, val);
> +	phywrite(dev, addr, reg, val);
> +	return 0;
> +}

Ditto.

> +static int axiemac_bus_reset(struct mii_dev *bus)
> +{
> +	debug("axiemac: Bus reset\n");
> +	return 0;
> +}

Ditto.


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
"Marriage is like a cage; one sees the birds outside desperate to get
in, and those inside desperate to get out."               - Montaigne

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-01 13:17   ` Michal Simek
@ 2011-09-01 14:09     ` Wolfgang Denk
  2011-09-02  6:39       ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Wolfgang Denk @ 2011-09-01 14:09 UTC (permalink / raw)
  To: u-boot

Dear Michal Simek,

In message <4E5F85DA.4080403@monstr.eu> you wrote:
>
> >> +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));
> >> +}
> > 
> > Can we please get rid of these functions?  As mentioned many, many
> > times before, we discourage all use of "base address plus offset" to
> > access any device registers etc.
> > 
> > These functions here re-introduce such accesses, and this is something
> > I will not accept.
> 
> Ok. How to do it?
> 
> For bus access it is necessary to use 4B offsets for DCR just 1B
> and one system can contains two MACs where the first use 4B offset and the second
> 1B.

I don't think your description here matches the code above.  With "1
byte offsets" you would be doing all unaligned bus accesses.

I think you should use an array of structs and an index to select the
right one.

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
"A dirty mind is a joy forever."                       - Randy Kunkee

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-01 14:09     ` Wolfgang Denk
@ 2011-09-02  6:39       ` Michal Simek
  2011-09-02  8:19         ` Wolfgang Denk
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2011-09-02  6:39 UTC (permalink / raw)
  To: u-boot

Dear Wolfgang Denk,

Wolfgang Denk wrote:
> Dear Michal Simek,
> 
> In message <4E5F85DA.4080403@monstr.eu> you wrote:
>>>> +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));
>>>> +}
>>> Can we please get rid of these functions?  As mentioned many, many
>>> times before, we discourage all use of "base address plus offset" to
>>> access any device registers etc.
>>>
>>> These functions here re-introduce such accesses, and this is something
>>> I will not accept.
>> Ok. How to do it?
>>
>> For bus access it is necessary to use 4B offsets for DCR just 1B
>> and one system can contains two MACs where the first use 4B offset and the second
>> 1B.
> 
> I don't think your description here matches the code above.  With "1
> byte offsets" you would be doing all unaligned bus accesses.


Let me show it on example.
on MB or PPC system with bus:
DMA is in memory controller on 32bit address + sdma DMA port offset.

DMA : Sdma offset(www.xilinx.com/support/documentation/ip_documentation/mpmc.pdf page 54 and 55)
0   : 0x0
1   : 0x80
2   : 0x100
3   : 0x180
...
7   : 0x380

Let me assume that MPMC is at 0x12340000
The first reg for DMA2 accessed through bus is at 0x12340100 offset the second at 0x12340104, the third 0x12340108 etc.

On PPC system with DCR is special connection between memory controller through DCR bus. Handling is done
with mfdcr_local and mtdcr_local functions.

DMA : Sdma address ranges (www.xilinx.com/support/documentation/user_guides/ug200.pdf page 261 and 299)
0   : 0x80-0x90
1   : 0x98-0xA8
2   : 0xB0-0xC0
3   : 0xC8-0xD8

The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-02  6:39       ` Michal Simek
@ 2011-09-02  8:19         ` Wolfgang Denk
  2011-09-02  8:40           ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Wolfgang Denk @ 2011-09-02  8:19 UTC (permalink / raw)
  To: u-boot

Dear Michal Simek,

In message <4E607A0B.3040608@monstr.eu> you wrote:
>
> >>>> +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);
> >>>> +}
...
> On PPC system with DCR is special connection between memory controller through DCR bus. Handling is done
> with mfdcr_local and mtdcr_local functions.
> 
> DMA : Sdma address ranges (www.xilinx.com/support/documentation/user_guides/ug200.pdf page 261 and 299)
> 0   : 0x80-0x90
> 1   : 0x98-0xA8
> 2   : 0xB0-0xC0
> 3   : 0xC8-0xD8
> 
> The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..

This is indeed a good example, as it shows how terribly broken your
code is.

See function sdma_out_be32() above.  It is suppose to write a 32 bit
value ("u32 val") as a 32 bit entity in big endian mode ("_be32") to
some device register - but the register addresses are (1) not aligned
to 32 bit boundaries and (2) not even 32 bits apart.

Oh!  You are NOT writing 32 bit data? You are writing only 8 bit?
But the function claims to be writing 32 bit!!!

And your function mtdcr_local() is actually a NOOP for anything but
PPC?  Without even a warning?


This code is obfuscated in multiple levels.

I will not accept that.

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
What is tolerance? -- it is the consequence of humanity. We  are  all
formed  of frailty and error; let us pardon reciprocally each other's
folly -- that is the first law of nature.                  - Voltaire

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-02  8:19         ` Wolfgang Denk
@ 2011-09-02  8:40           ` Michal Simek
  2011-09-02  9:24             ` Wolfgang Denk
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2011-09-02  8:40 UTC (permalink / raw)
  To: u-boot

Wolfgang Denk wrote:
> Dear Michal Simek,
> 
> In message <4E607A0B.3040608@monstr.eu> you wrote:
>>>>>> +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);
>>>>>> +}
> ...
>> On PPC system with DCR is special connection between memory controller through DCR bus. Handling is done
>> with mfdcr_local and mtdcr_local functions.
>>
>> DMA : Sdma address ranges (www.xilinx.com/support/documentation/user_guides/ug200.pdf page 261 and 299)
>> 0   : 0x80-0x90
>> 1   : 0x98-0xA8
>> 2   : 0xB0-0xC0
>> 3   : 0xC8-0xD8
>>
>> The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..
> 
> This is indeed a good example, as it shows how terribly broken your
> code is.
> 
> See function sdma_out_be32() above.  It is suppose to write a 32 bit
> value ("u32 val") as a 32 bit entity in big endian mode ("_be32") to
> some device register - but the register addresses are (1) not aligned
> to 32 bit boundaries and (2) not even 32 bits apart.

I think you misunderstand what there is written.
For DCR is register address with offset + 0x1 but still you are writing there 32bit values. It is not direct access.
and it is not any unaligned access at all. But bus access it will be unaligned access but not for DCR.

DCR is defined just for PPC right now because none wanted to do it for Microblaze.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v2] net: axi_ethernet: Add driver to u-boot
  2011-09-01 13:17   ` Wolfgang Denk
@ 2011-09-02  9:02     ` Michal Simek
  0 siblings, 0 replies; 17+ messages in thread
From: Michal Simek @ 2011-09-02  9:02 UTC (permalink / raw)
  To: u-boot

Dear Wolfgang Denk,

Wolfgang Denk wrote:
> Dear Michal Simek,
> 
> In message <1314877154-14536-2-git-send-email-monstr@monstr.eu> you wrote:
>> Add axi_ethernet driver for little-endian Microblaze.
>>
>> RX/TX BDs and rxframe buffer are shared among all axi_ethernet MACs.
>> Only one MAC can work in one time.
>>
>> Signed-off-by: Michal Simek <monstr@monstr.eu>
> 
>> +/* 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
>> + */
> 
> Incorrect multiline comment style. Please fix globally.

Fixed.

> 
> 
>> +	u32 timeout = 200;
>> +	/* Wait till MDIO interface is ready to accept a new transaction. */
>> +	while (timeout && (!(in_be32(&regs->mdio_mcr)
>> +						& XAE_MDIO_MCR_READY_MASK)))
>> +		timeout--;
> 
> Multiline statement - braches needed.
> 
> Also, add some udelay() or similar to have a defined length of the
> timeout.  Please fix globally.

Fixed.

> 
> 
>> +/* 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;
> 
> Please ALWAYS use I/O accessors to access device registers.  Please
> fix globally.

Done

> 
>> +static int IsRxReady(struct eth_device *dev)
> 
> As requested before:  Please get rid of these CamelCaps identifiers!!!

Fixed this.

> 
> 
>> +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("axiemac: Read MII 0x%x, 0x%x, 0x%x\n", addr, reg, *val);
>> +	return 0;
> 
> Please separate declarations and code by a single blank line.  Please
> fix globally.

Done

> 
> 
> Why does this function return int when you uncondsitionally always
> return 0 only?

I have fixed phywrite/phyread functions.

> 
>> +}
>> +
>> +static int axiemac_miiphy_write(const char *devname, uchar addr,
>> +							uchar reg, ushort val)
>> +{
>> +	struct eth_device *dev = eth_get_dev();
>> +	debug("axiemac: Write MII 0x%x, 0x%x, 0x%x\n", addr, reg, val);
>> +	phywrite(dev, addr, reg, val);
>> +	return 0;
>> +}
> 
> Ditto.
> 
>> +static int axiemac_bus_reset(struct mii_dev *bus)
>> +{
>> +	debug("axiemac: Bus reset\n");
>> +	return 0;
>> +}
> 
> Ditto.

Because of miiphyutil code requires to return 0 and I am not aware about
doing anything useful for bus_reset.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-02  8:40           ` Michal Simek
@ 2011-09-02  9:24             ` Wolfgang Denk
  2011-09-02 10:38               ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Wolfgang Denk @ 2011-09-02  9:24 UTC (permalink / raw)
  To: u-boot

Dear Michal Simek,

In message <4E609682.8030701@monstr.eu> you wrote:
>
> >>>>>> +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);
> >>>>>> +}
> > ...
...
> >> The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..
> > 
> > This is indeed a good example, as it shows how terribly broken your
> > code is.
> > 
> > See function sdma_out_be32() above.  It is suppose to write a 32 bit
> > value ("u32 val") as a 32 bit entity in big endian mode ("_be32") to
> > some device register - but the register addresses are (1) not aligned
> > to 32 bit boundaries and (2) not even 32 bits apart.
> 
> I think you misunderstand what there is written.

Maybe.  Maybe even I want to misunderstand it.  The problem is that
the code does not prevent such misunderstanding.

There are many shortcomings of that code.

> DCR is defined just for PPC right now because none wanted to do it for Microblaze.

Actually even this is incorrect - AFAIK Device Control Registers (DCR)
exist not on all PPC systems, but only on 4xx (and even there only on
some models).  So your code works on a few systems, silently does not
do anything on others, and crashes on yet others with an illegal
instruction.

How do you call code that exposes such behaviour?

I don't want to have this in mainline.

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
Don't hit a man when he's down - kick him; it's easier.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-02  9:24             ` Wolfgang Denk
@ 2011-09-02 10:38               ` Michal Simek
  2011-09-02 12:53                 ` Wolfgang Denk
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2011-09-02 10:38 UTC (permalink / raw)
  To: u-boot

Dear Wolfgang Denk,

Wolfgang Denk wrote:
> Dear Michal Simek,
> 
> In message <4E609682.8030701@monstr.eu> you wrote:
>>>>>>>> +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);
>>>>>>>> +}
>>> ...
> ...
>>>> The first reg for DMA2 accessed trough DCR is at 0xB0, the second at 0xB1, etc..
>>> This is indeed a good example, as it shows how terribly broken your
>>> code is.
>>>
>>> See function sdma_out_be32() above.  It is suppose to write a 32 bit
>>> value ("u32 val") as a 32 bit entity in big endian mode ("_be32") to
>>> some device register - but the register addresses are (1) not aligned
>>> to 32 bit boundaries and (2) not even 32 bits apart.
>> I think you misunderstand what there is written.
> 
> Maybe.  Maybe even I want to misunderstand it.  The problem is that
> the code does not prevent such misunderstanding.
> 
> There are many shortcomings of that code.

I think that this is the reason why we have review process, don't we?

Especially this driver is 2-3 years old and it is used by many our customers.
It is only my effort to clear xilinx drivers/platforms.

As I see there is still ugly board/xilinx/common folder and ancient enet driver and i2c
driver.

> 
>> DCR is defined just for PPC right now because none wanted to do it for Microblaze.
> 
> Actually even this is incorrect - AFAIK Device Control Registers (DCR)
> exist not on all PPC systems, but only on 4xx (and even there only on
> some models).  So your code works on a few systems, silently does not
> do anything on others, and crashes on yet others with an illegal
> instruction.

That driver is not definitely for all ppc systems. That IP is available just for
Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405).
That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation.
Which means that none can add this IP to any other PPC system out of Xilinx FPGA.

> 
> How do you call code that exposes such behaviour?

If I look at drivers/net folder there are a lot of examples like that.
None expect that altera_tse/bfin_mac/tsec will be possible to use with all systems.
Maybe you expect that they can be use on the same architecture but this is Xilinx FPGA.
It is up to you how you want to compose your system.

IRC tsec is used just for Freescale PPC and this ll_temac driver is just used for xilinx microblaze
and xilinx ppc.

Sorry I can't see any problem to have driver for specific platform or even to have one driver
which supports two completely different platform.

I saw that there are some drivers in arch/<cpu>/ folders but this is not that case because
can be possible to use it for microblaze and xilinx ppc.

This ll_temac driver is just another net IP like emaclite is. Emaclite driver is possible to use
on Microblaze and xilinx ppc systems and in near future with arm on Xilinx zynq platform.


> I don't want to have this in mainline.

If this is your decision, I won't send any updated version.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-02 10:38               ` Michal Simek
@ 2011-09-02 12:53                 ` Wolfgang Denk
  2011-09-02 13:29                   ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Wolfgang Denk @ 2011-09-02 12:53 UTC (permalink / raw)
  To: u-boot

Dear Michal Simek,

In message <4E60B220.6010309@monstr.eu> you wrote:
> 
> As I see there is still ugly board/xilinx/common folder and ancient enet driver and i2c
> driver.

Indeed, and improvementrs are more than welcome.

> > Actually even this is incorrect - AFAIK Device Control Registers (DCR)
> > exist not on all PPC systems, but only on 4xx (and even there only on
> > some models).  So your code works on a few systems, silently does not
> > do anything on others, and crashes on yet others with an illegal
> > instruction.
> 
> That driver is not definitely for all ppc systems. That IP is available just for
> Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405).
> That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation.
> Which means that none can add this IP to any other PPC system out of Xilinx FPGA.

So why not use something like CONFIG_440 in this test, and add an
#error for anything else?

Do we actually need this m{f,t}dcr_local() then?

> Sorry I can't see any problem to have driver for specific platform or even to have one driver
> which supports two completely different platform.

My issue is that this code silently breaks or crashes when certain
(undocumented) conditions are not met.  Preventing this seems not to
bee too difficult: add a comment, make it depend on the right CONFIG_
settings, and bail out with a clear error message when conditions are
not met.

As for the other part of the problem - you try to mix two different
cases: one where you refer to an index, and one where you refer to an
address.  This obviously doesn't mix well.  If there is no better way
of doing this, I'd still prefer deriving the index from the offset in
a struct than deriving the address from an offset - the intention is
to enable the compiler to perform type checking, which is impossible
with a typeless base+offset address.

> > I don't want to have this in mainline.
> 
> If this is your decision, I won't send any updated version.

Attempted extortion?

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
"Wagner's music is better than it sounds."               - Mark Twain

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-02 12:53                 ` Wolfgang Denk
@ 2011-09-02 13:29                   ` Michal Simek
  2011-09-02 14:08                     ` Wolfgang Denk
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2011-09-02 13:29 UTC (permalink / raw)
  To: u-boot

Dear Wolfgang Denk,

> In message <4E60B220.6010309@monstr.eu> you wrote:
>> As I see there is still ugly board/xilinx/common folder and ancient enet driver and i2c
>> driver.
> 
> Indeed, and improvementrs are more than welcome.
> 
>>> Actually even this is incorrect - AFAIK Device Control Registers (DCR)
>>> exist not on all PPC systems, but only on 4xx (and even there only on
>>> some models).  So your code works on a few systems, silently does not
>>> do anything on others, and crashes on yet others with an illegal
>>> instruction.
>> That driver is not definitely for all ppc systems. That IP is available just for
>> Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405).
>> That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation.
>> Which means that none can add this IP to any other PPC system out of Xilinx FPGA.
> 
> So why not use something like CONFIG_440 in this test, and add an
> #error for anything else?
> 
> Do we actually need this m{f,t}dcr_local() then?

DCR handling is specific for Xilinx ppc440 which means that not all PPC440 can use it.
As you see m{f,t}dcr_local setup handling for it that's why it is neeeded.


> 
>> Sorry I can't see any problem to have driver for specific platform or even to have one driver
>> which supports two completely different platform.
> 
> My issue is that this code silently breaks or crashes when certain
> (undocumented) conditions are not met.  Preventing this seems not to
> bee too difficult: add a comment, make it depend on the right CONFIG_
> settings, and bail out with a clear error message when conditions are
> not met.

Driver is protected by CONFIG_XILINX_LL_TEMAC option which means that
any platform is not silently breaks. You can use it with Microblaze and PPC
and configuration is done (xparameters.h and config.mk files) by u-boot BSP
in connection to Xilinx EDK which check if DCR can be used or not.

> 
> As for the other part of the problem - you try to mix two different
> cases: one where you refer to an index, and one where you refer to an
> address. 

In technical sense it is still address not index. It is different addressing mode.

I have done it because it is much better than a lot of ifdefs. It is more than that
because ppc has dcr up to 4 DMAs but memory controller supports up to 8 DMAs
that's why I think that it is better to support both modes and decide by configuration.

  This obviously doesn't mix well.  If there is no better way
> of doing this, I'd still prefer deriving the index from the offset in
> a struct than deriving the address from an offset - the intention is
> to enable the compiler to perform type checking, which is impossible
> with a typeless base+offset address.

I understand the reasons for that but I can't see any elegant way how to do so.

>>> I don't want to have this in mainline.
>> If this is your decision, I won't send any updated version.
> 
> Attempted extortion?

My god why do you think that it is extortion?
If you don't want to add it to mainline because you think that this driver
is bad/broken/anything, I can do nothing with it and make no sense for me to invest my time to it.
And I am not going to disturb others with it.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-02 13:29                   ` Michal Simek
@ 2011-09-02 14:08                     ` Wolfgang Denk
  2011-09-05  7:15                       ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Wolfgang Denk @ 2011-09-02 14:08 UTC (permalink / raw)
  To: u-boot

Dear Michal Simek,

In message <4E60DA47.4070301@monstr.eu> you wrote:
>
> >> That driver is not definitely for all ppc systems. That IP is available just for
> >> Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405).
> >> That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation.
> >> Which means that none can add this IP to any other PPC system out of Xilinx FPGA.
> > 
> > So why not use something like CONFIG_440 in this test, and add an
> > #error for anything else?

You did not answer this - why not using CONFIG_440 instead of
CONFIG_PPC ?

> > Do we actually need this m{f,t}dcr_local() then?
> 
> DCR handling is specific for Xilinx ppc440 which means that not all PPC440 can use it.
> As you see m{f,t}dcr_local setup handling for it that's why it is neeeded.

Then maybe you should chose a better name for it, say m?dcr_xilinx()
or so.

> > My issue is that this code silently breaks or crashes when certain
> > (undocumented) conditions are not met.  Preventing this seems not to
> > bee too difficult: add a comment, make it depend on the right CONFIG_
> > settings, and bail out with a clear error message when conditions are
> > not met.
> 
> Driver is protected by CONFIG_XILINX_LL_TEMAC option which means that
> any platform is not silently breaks. You can use it with Microblaze and PPC
> and configuration is done (xparameters.h and config.mk files) by u-boot BSP
> in connection to Xilinx EDK which check if DCR can be used or not.

I can only offer you a solution that seems acceptable to me.

> > As for the other part of the problem - you try to mix two different
> > cases: one where you refer to an index, and one where you refer to an
> > address. 
> 
> In technical sense it is still address not index. It is different addressing mode.

The Processor Core User's Manual says for example:

	The DCR number (DCRN) is specified by the Device Control
	Register Immediate Prefix Register (DCRIPR)
	and the DCRF field of the mfdcr instruction.

To me, "number" translates much better into index than into address.

Also, the DCR number are incremented by 1 - if these were addresses in
the common sense, they could only point to 8 bit entities - but the
registers are actually 32 bit wide.


But again, I can only show you what I think could be an acceptable
approach.  If you don't like it, please feel free to develop a better
one.

In any case, I will not accept the current (V3) code.

>   This obviously doesn't mix well.  If there is no better way
> > of doing this, I'd still prefer deriving the index from the offset in
> > a struct than deriving the address from an offset - the intention is
> > to enable the compiler to perform type checking, which is impossible
> > with a typeless base+offset address.
> 
> I understand the reasons for that but I can't see any elegant way how to do so.

Well, I just proposed one way - not elegant indeed, but I'd be willing
to swallow that.

> If you don't want to add it to mainline because you think that this driver
> is bad/broken/anything, I can do nothing with it and make no sense for me to invest my time to it.

I only complain about a few details of the implementation, and I even
lend you a helping hand by offering specific solutions.  Feel free to
take or to refuse it.  I got better things to do myself, either.


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
The ultimate barrier is one's viewpoint.
                        - Terry Pratchett, _The Dark Side of the Sun_

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-02 14:08                     ` Wolfgang Denk
@ 2011-09-05  7:15                       ` Michal Simek
  2011-09-05  8:00                         ` Wolfgang Denk
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2011-09-05  7:15 UTC (permalink / raw)
  To: u-boot

Dear Wolfgang Denk,

> In message <4E60DA47.4070301@monstr.eu> you wrote:
>>>> That driver is not definitely for all ppc systems. That IP is available just for
>>>> Xilinx FPGA where can be possible to use it with Microblaze and xilinx ppc440 (maybe ppc405).
>>>> That DCR handling, which is implemented in this driver, fits to xilinx ppc440 implementation.
>>>> Which means that none can add this IP to any other PPC system out of Xilinx FPGA.
>>> So why not use something like CONFIG_440 in this test, and add an
>>> #error for anything else?
> 
> You did not answer this - why not using CONFIG_440 instead of
> CONFIG_PPC ?

Probably the best is CONFIG_XILINX_440.

> 
>>> Do we actually need this m{f,t}dcr_local() then?
>> DCR handling is specific for Xilinx ppc440 which means that not all PPC440 can use it.
>> As you see m{f,t}dcr_local setup handling for it that's why it is neeeded.
> 
> Then maybe you should chose a better name for it, say m?dcr_xilinx()
> or so.

np.

> 
>>> My issue is that this code silently breaks or crashes when certain
>>> (undocumented) conditions are not met.  Preventing this seems not to
>>> bee too difficult: add a comment, make it depend on the right CONFIG_
>>> settings, and bail out with a clear error message when conditions are
>>> not met.
>> Driver is protected by CONFIG_XILINX_LL_TEMAC option which means that
>> any platform is not silently breaks. You can use it with Microblaze and PPC
>> and configuration is done (xparameters.h and config.mk files) by u-boot BSP
>> in connection to Xilinx EDK which check if DCR can be used or not.
> 
> I can only offer you a solution that seems acceptable to me.

seems?


>>> As for the other part of the problem - you try to mix two different
>>> cases: one where you refer to an index, and one where you refer to an
>>> address. 
>> In technical sense it is still address not index. It is different addressing mode.
> 
> The Processor Core User's Manual says for example:
> 
> 	The DCR number (DCRN) is specified by the Device Control
> 	Register Immediate Prefix Register (DCRIPR)
> 	and the DCRF field of the mfdcr instruction.
> 
> To me, "number" translates much better into index than into address.

DCRN is number but for my case I use 0 for register "address" and 1 for value.
To be accurate - 0 is indirect dcr address reg, and 1 is indirect dcr access reg.
(Have changed that magic value to macros according technical documentation).


> Also, the DCR number are incremented by 1 - if these were addresses in
> the common sense, they could only point to 8 bit entities - but the
> registers are actually 32 bit wide.

And agree that DCR number is incremented by 1.

On www.xilinx.com/support/documentation/user_guides/ug200.pdf page 261
is written that it is address that's why I use address.

> But again, I can only show you what I think could be an acceptable
> approach.  If you don't like it, please feel free to develop a better
> one.
> 
> In any case, I will not accept the current (V3) code.

sure.

> 
>>   This obviously doesn't mix well.  If there is no better way
>>> of doing this, I'd still prefer deriving the index from the offset in
>>> a struct than deriving the address from an offset - the intention is
>>> to enable the compiler to perform type checking, which is impossible
>>> with a typeless base+offset address.
>> I understand the reasons for that but I can't see any elegant way how to do so.
> 
> Well, I just proposed one way - not elegant indeed, but I'd be willing
> to swallow that.

You mean that array of structs, right?

> 
>> If you don't want to add it to mainline because you think that this driver
>> is bad/broken/anything, I can do nothing with it and make no sense for me to invest my time to it.
> 
> I only complain about a few details of the implementation, and I even
> lend you a helping hand by offering specific solutions.  Feel free to
> take or to refuse it.  I got better things to do myself, either.

Sure.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot
  2011-09-05  7:15                       ` Michal Simek
@ 2011-09-05  8:00                         ` Wolfgang Denk
  0 siblings, 0 replies; 17+ messages in thread
From: Wolfgang Denk @ 2011-09-05  8:00 UTC (permalink / raw)
  To: u-boot

Dear Michal Simek,

In message <4E647722.9080602@monstr.eu> you wrote:
> 
> > I can only offer you a solution that seems acceptable to me.
> 
> seems?

Well, yes.  As mentioned, I'm not happy with the propsal myself, but
as long as I cannot come up with any better one it would be unjust to
demand it from you.

> >>   This obviously doesn't mix well.  If there is no better way
> >>> of doing this, I'd still prefer deriving the index from the offset in
> >>> a struct than deriving the address from an offset - the intention is
> >>> to enable the compiler to perform type checking, which is impossible
> >>> with a typeless base+offset address.
> >> I understand the reasons for that but I can't see any elegant way how to do so.
> > 
> > Well, I just proposed one way - not elegant indeed, but I'd be willing
> > to swallow that.
> 
> You mean that array of structs, right?

No. I mean the text above: I'd rather derive the index from the
offset in a struct instead of deriving the address from an offset
(but I have to admit that I don't see an elegant way to implement this
yet).


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
Why can you only have two doors on a chicken coop? If it had four  it
would be a chicken sedan.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2011-09-05  8:00 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-01 11:39 [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot Michal Simek
2011-09-01 11:39 ` [U-Boot] [PATCH v2] net: axi_ethernet: Add " Michal Simek
2011-09-01 13:17   ` Wolfgang Denk
2011-09-02  9:02     ` Michal Simek
2011-09-01 13:06 ` [U-Boot] [PATCH v3] net: ll_temac: Add LL TEMAC " Wolfgang Denk
2011-09-01 13:17   ` Michal Simek
2011-09-01 14:09     ` Wolfgang Denk
2011-09-02  6:39       ` Michal Simek
2011-09-02  8:19         ` Wolfgang Denk
2011-09-02  8:40           ` Michal Simek
2011-09-02  9:24             ` Wolfgang Denk
2011-09-02 10:38               ` Michal Simek
2011-09-02 12:53                 ` Wolfgang Denk
2011-09-02 13:29                   ` Michal Simek
2011-09-02 14:08                     ` Wolfgang Denk
2011-09-05  7:15                       ` Michal Simek
2011-09-05  8:00                         ` Wolfgang Denk

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.