All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
@ 2011-08-24 13:07 Ajay Bhargav
  2011-08-24 15:42 ` Mike Frysinger
  2011-08-24 16:36 ` Marek Vasut
  0 siblings, 2 replies; 14+ messages in thread
From: Ajay Bhargav @ 2011-08-24 13:07 UTC (permalink / raw)
  To: u-boot

This patch adds support for Fast Ethernet Controller driver for
Armada100 series.

Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
---
Changes for v2:
	- removed random MAC generation
	- driver init function changed to register as per new naming convention
	- code cleanup (Thanks to Wolfgang, Marek & Mike for tips)

 arch/arm/include/asm/arch-armada100/armada100.h |    1 +
 drivers/net/Makefile                            |    1 +
 drivers/net/armada100_fec.c                     |  761 +++++++++++++++++++++++
 drivers/net/armada100_fec.h                     |  232 +++++++
 include/netdev.h                                |    1 +
 5 files changed, 996 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/armada100_fec.c
 create mode 100644 drivers/net/armada100_fec.h

diff --git a/arch/arm/include/asm/arch-armada100/armada100.h b/arch/arm/include/asm/arch-armada100/armada100.h
index d5d125a..3d567eb 100644
--- a/arch/arm/include/asm/arch-armada100/armada100.h
+++ b/arch/arm/include/asm/arch-armada100/armada100.h
@@ -59,6 +59,7 @@
 #define ARMD1_MPMU_BASE		0xD4050000
 #define ARMD1_APMU_BASE		0xD4282800
 #define ARMD1_CPU_BASE		0xD4282C00
+#define ARMD1_FEC_BASE		0xC0800000
 
 /*
  * Main Power Management (MPMU) Registers
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 819b197..34b4322 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -28,6 +28,7 @@ LIB	:= $(obj)libnet.o
 COBJS-$(CONFIG_DRIVER_3C589) += 3c589.o
 COBJS-$(CONFIG_PPC4xx_EMAC) += 4xx_enet.o
 COBJS-$(CONFIG_ALTERA_TSE) += altera_tse.o
+COBJS-$(CONFIG_ARMADA100_FEC) += armada100_fec.o
 COBJS-$(CONFIG_DRIVER_AT91EMAC) += at91_emac.o
 COBJS-$(CONFIG_DRIVER_AX88180) += ax88180.o
 COBJS-$(CONFIG_BCM570x) += bcm570x.o
diff --git a/drivers/net/armada100_fec.c b/drivers/net/armada100_fec.c
new file mode 100644
index 0000000..e36dca6
--- /dev/null
+++ b/drivers/net/armada100_fec.c
@@ -0,0 +1,761 @@
+/*
+ * (C) Copyright 2011
+ * eInfochips Ltd. <www.einfochips.com>
+ * Written-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
+ *
+ * (C) Copyright 2010
+ * Marvell Semiconductor <www.marvell.com>
+ * Contributor: Mahavir Jain <mjain@marvell.com>
+ *
+ * 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., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301 USA
+ */
+
+#include <common.h>
+#include <net.h>
+#include <malloc.h>
+#include <miiphy.h>
+#include <netdev.h>
+#include <asm/types.h>
+#include <asm/byteorder.h>
+#include <linux/err.h>
+#include <linux/mii.h>
+#include <asm/io.h>
+#include <asm/arch/armada100.h>
+#include "armada100_fec.h"
+
+#define  PHY_ADR_REQ     0xFF	/* Magic number to read/write PHY address */
+
+#ifdef DEBUG
+static int eth_dump_regs(struct eth_device *dev)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+	unsigned int i = 0;
+
+	printf("\noffset: phy_adr, value: 0x%x\n", readl(&regs->phyadr));
+	printf("offset: smi, value: 0x%x\n", readl(&regs->smi));
+	for (i = 0x400; i <= 0x4e4; i += 4)
+		printf("offset: 0x%x, value: 0x%x\n",
+			i, readl(ARMD1_FEC_BASE + i));
+	return 0;
+}
+#endif
+
+static int armdfec_phy_timeout(u32 reg, u32 flag, int cond)
+{
+	u32 timeout = PHY_WAIT_ITERATIONS;
+	while (--timeout) {
+		if (cond && (readl(reg) & flag))
+			break;
+		else if (!cond && !(readl(reg) & flag))
+			break;
+		udelay(PHY_WAIT_MICRO_SECONDS);
+	}
+	return !timeout;
+}
+
+static int smi_reg_read(const char *devname, u8 phy_addr, u8 phy_reg,
+			u16 *value)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+	u32 val, reg_data;
+
+	if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {
+		reg_data = readl(&regs->phyadr);
+		*value = (u16) (reg_data & 0x1f);
+		return 0;
+	}
+
+	/* check parameters */
+	if (phy_addr > PHY_MASK) {
+		printf("Err..(%s) Invalid phy address: 0x%X\n",
+				__func__, phy_addr);
+		return -EINVAL;
+	}
+	if (phy_reg > PHY_MASK) {
+		printf("Err..(%s) Invalid register offset: 0x%X\n",
+				__func__, phy_reg);
+		return -EINVAL;
+	}
+
+	/* wait for the SMI register to become available */
+	if (armdfec_phy_timeout((u32)&regs->smi, SMI_BUSY, FALSE)) {
+		printf("Error (%s) PHY busy timeout\n",	__func__);
+		return -1;
+	}
+
+	writel(phy_addr << 16 | phy_reg << 21 | SMI_OP_R, &regs->smi);
+
+	/* now wait for the data to be valid */
+	if (armdfec_phy_timeout((u32)&regs->smi, SMI_R_VALID, TRUE)) {
+		val = readl(&regs->smi);
+		printf("Err (%s) PHY Read timeout, val=0x%x\n", __func__, val);
+		return -1;
+	}
+	val = readl(&regs->smi);
+	*value = val & 0xffff;
+
+	return 0;
+}
+
+static int smi_reg_write(const char *devname,
+	 u8 phy_addr, u8 phy_reg, u16 value)
+{
+	struct eth_device *dev = eth_get_dev_by_name(devname);
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+
+	if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {
+		clrsetbits_le32(&regs->phyadr, 0x1f, value & 0x1f);
+		return 0;
+	}
+
+	/* check parameters */
+	if (phy_addr > PHY_MASK) {
+		printf("Err..(%s) Invalid phy address\n", __func__);
+		return -EINVAL;
+	}
+	if (phy_reg > PHY_MASK) {
+		printf("Err..(%s) Invalid register offset\n", __func__);
+		return -EINVAL;
+	}
+
+	/* wait for the SMI register to become available */
+	if (armdfec_phy_timeout((u32)&regs->smi, SMI_BUSY, FALSE)) {
+		printf("Error (%s) PHY busy timeout\n",	__func__);
+		return -1;
+	}
+
+	writel(phy_addr << 16 | phy_reg << 21 | SMI_OP_W | (value & 0xffff),
+			&regs->smi);
+	return 0;
+}
+
+/*
+ * Abort any transmit and receive operations and put DMA
+ * in idle state. AT and AR bits are cleared upon entering
+ * in IDLE state. So poll those bits to verify operation.
+ */
+static void abortdma(struct eth_device *dev)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+	int delay;
+	int maxretries = 40;
+
+	do {
+		writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
+		udelay(100);
+
+		delay = 10;
+		while ((readl(&regs->sdma_cmd) &
+			(SDMA_CMD_AR | SDMA_CMD_AT))
+			&& delay-- > 0) {
+			udelay(10);
+		}
+	} while (maxretries-- > 0 && delay <= 0);
+
+	if (maxretries <= 0)
+		printf("%s : DMA Stuck\n", __func__);
+}
+
+static inline u32 nibble_swapping_32_bit(u32 x)
+{
+	return (((x) & 0xf0f0f0f0) >> 4) | (((x) & 0x0f0f0f0f) << 4);
+}
+
+static inline u32 nibble_swapping_16_bit(u32 x)
+{
+	return (((x) & 0x0000f0f0) >> 4) | (((x) & 0x00000f0f) << 4);
+}
+
+static inline u32 flip_4_bits(u32 x)
+{
+	return (((x) & 0x01) << 3) | (((x) & 0x002) << 1)
+		| (((x) & 0x04) >> 1) | (((x) & 0x008) >> 3);
+}
+
+/*
+ * This function will calculate the hash function of the address.
+ * depends on the hash mode and hash size.
+ * Inputs
+ * mach             - the 2 most significant bytes of the MAC address.
+ * macl             - the 4 least significant bytes of the MAC address.
+ * Outputs
+ * return the calculated entry.
+ */
+static u32 hash_function(u32 mach, u32 macl)
+{
+	u32 hashresult;
+	u32 addrh;
+	u32 addrl;
+	u32 addr0;
+	u32 addr1;
+	u32 addr2;
+	u32 addr3;
+	u32 addrhswapped;
+	u32 addrlswapped;
+
+	addrh = nibble_swapping_16_bit(mach);
+	addrl = nibble_swapping_32_bit(macl);
+
+	addrhswapped = flip_4_bits(addrh & 0xf)
+		+ ((flip_4_bits((addrh >> 4) & 0xf)) << 4)
+		+ ((flip_4_bits((addrh >> 8) & 0xf)) << 8)
+		+ ((flip_4_bits((addrh >> 12) & 0xf)) << 12);
+
+	addrlswapped = flip_4_bits(addrl & 0xf)
+		+ ((flip_4_bits((addrl >> 4) & 0xf)) << 4)
+		+ ((flip_4_bits((addrl >> 8) & 0xf)) << 8)
+		+ ((flip_4_bits((addrl >> 12) & 0xf)) << 12)
+		+ ((flip_4_bits((addrl >> 16) & 0xf)) << 16)
+		+ ((flip_4_bits((addrl >> 20) & 0xf)) << 20)
+		+ ((flip_4_bits((addrl >> 24) & 0xf)) << 24)
+		+ ((flip_4_bits((addrl >> 28) & 0xf)) << 28);
+
+	addrh = addrhswapped;
+	addrl = addrlswapped;
+
+	addr0 = (addrl >> 2) & 0x03f;
+	addr1 = (addrl & 0x003) | (((addrl >> 8) & 0x7f) << 2);
+	addr2 = (addrl >> 15) & 0x1ff;
+	addr3 = ((addrl >> 24) & 0x0ff) | ((addrh & 1) << 8);
+
+	hashresult = (addr0 << 9) | (addr1 ^ addr2 ^ addr3);
+	hashresult = hashresult & 0x07ff;
+	return hashresult;
+}
+
+/*
+ * This function will add an entry to the address table.
+ * depends on the hash mode and hash size that was initialized.
+ * Inputs
+ * mach - the 2 most significant bytes of the MAC address.
+ * macl - the 4 least significant bytes of the MAC address.
+ * skip - if 1, skip this address.
+ * rd   - the RD field in the address table.
+ * Outputs
+ * address table entry is added.
+ * 0 if success.
+ * -ENOSPC if table full
+ */
+static int add_del_hash_entry(struct armdfec_device *darmdfec, u32 mach,
+			      u32 macl, u32 rd, u32 skip, int del)
+{
+	struct addr_table_entry_t *entry, *start;
+	u32 newhi;
+	u32 newlo;
+	u32 i;
+	u8 *last;
+
+	newlo = (((mach >> 4) & 0xf) << 15)
+		| (((mach >> 0) & 0xf) << 11)
+		| (((mach >> 12) & 0xf) << 7)
+		| (((mach >> 8) & 0xf) << 3)
+		| (((macl >> 20) & 0x1) << 31)
+		| (((macl >> 16) & 0xf) << 27)
+		| (((macl >> 28) & 0xf) << 23)
+		| (((macl >> 24) & 0xf) << 19)
+		| (skip << HTESKIP) | (rd << HTERDBIT)
+		| HTEVALID;
+
+	newhi = (((macl >> 4) & 0xf) << 15)
+		| (((macl >> 0) & 0xf) << 11)
+		| (((macl >> 12) & 0xf) << 7)
+		| (((macl >> 8) & 0xf) << 3)
+		| (((macl >> 21) & 0x7) << 0);
+
+	/*
+	 * Pick the appropriate table, start scanning for free/reusable
+	 * entries@the index obtained by hashing the specified MAC address
+	 */
+	start = (struct addr_table_entry_t *) (darmdfec->htpr);
+	entry = start + hash_function(mach, macl);
+	for (i = 0; i < HOP_NUMBER; i++) {
+		if (!(entry->lo & HTEVALID)) {
+			break;
+		} else {
+			/* if same address put in same position */
+			if (((entry->lo & 0xfffffff8) ==
+			     (newlo & 0xfffffff8))
+			    && (entry->hi == newhi)) {
+				break;
+			}
+		}
+		if (entry == start + 0x7ff)
+			entry = start;
+		else
+			entry++;
+	}
+
+	if (((entry->lo & 0xfffffff8) != (newlo & 0xfffffff8)) &&
+		(entry->hi != newhi) && del)
+		return 0;
+
+	if (i == HOP_NUMBER) {
+		if (!del) {
+			printf("%s: table section is full\n", __FILE__);
+			return -ENOSPC;
+		} else {
+			return 0;
+		}
+	}
+
+	/*
+	 * Update the selected entry
+	 */
+	if (del) {
+		entry->hi = 0;
+		entry->lo = 0;
+	} else {
+		entry->hi = newhi;
+		entry->lo = newlo;
+	}
+
+	last = (u8 *) entry;
+	last = last + sizeof(*entry);
+
+	return 0;
+}
+
+/*
+ *  Create an addressTable entry from MAC address info
+ *  found in the specifed net_device struct
+ *
+ *  Input : pointer to ethernet interface network device structure
+ *  Output : N/A
+ */
+static void update_hash_table_mac_address(struct armdfec_device *darmdfec,
+					  u8 *oaddr, u8 *addr)
+{
+	u32 mach;
+	u32 macl;
+
+	/* Delete old entry */
+	if (oaddr) {
+		mach = (oaddr[0] << 8) | oaddr[1];
+		macl = (oaddr[2] << 24) | (oaddr[3] << 16) |
+			(oaddr[4] << 8) | oaddr[5];
+		add_del_hash_entry(darmdfec, mach, macl, 1, 0, HASH_DELETE);
+	}
+
+	/* Add new entry */
+	mach = (addr[0] << 8) | addr[1];
+	macl = (addr[2] << 24) | (addr[3] << 16) | (addr[4] << 8) | addr[5];
+	add_del_hash_entry(darmdfec, mach, macl, 1, 0, HASH_ADD);
+}
+
+/* Address Table Initialization */
+static void init_hashtable(struct eth_device *dev)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+	memset(darmdfec->htpr, 0, HASH_ADDR_TABLE_SIZE);
+	writel((u32) darmdfec->htpr, &regs->htpr);
+}
+
+static int setportconfigext(struct eth_device *dev, int mtu)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+	int mtusize;
+
+	/* 64 should work but does not -- dhcp packets NEVER get transmitted. */
+	if ((mtu > MAX_PKT_SIZE) || (mtu < 64))
+		return -EINVAL;
+
+	/* add source/dest mac addr (12) + pid (2) + crc (4) */
+	mtu += ETH_EXTRA_HEADER;
+	if (mtu <= 1518)
+		mtusize = PCXR_MFL_1518;
+	else if (mtu <= 1536)
+		mtusize = PCXR_MFL_1536;
+	else if (mtu <= 2048)
+		mtusize = PCXR_MFL_2048;
+	else
+		mtusize = PCXR_MFL_64K;
+
+	/* Extended Port Configuration */
+	writel(PCXR_2BSM |	/* Two byte suffix aligns IP hdr */
+		PCXR_DSCP_EN |	/* Enable DSCP in IP */
+		mtusize | PCXR_FLP |	/* do not force link pass */
+		PCXR_TX_HIGH_PRI,	/* Transmit - high priority queue */
+		&regs->pconf_ext);
+
+	/* subtract source/dest mac addr (12) + pid (2) + crc (4) */
+	mtu -= ETH_EXTRA_HEADER;
+	return 0;
+}
+
+/*
+ * This detects PHY chip from address 0-31 by reading PHY status
+ * registers. PHY chip can be connected at any of this address.
+ */
+static int ethernet_phy_detect(struct eth_device *dev)
+{
+	u32 val;
+	u16 tmp, mii_status;
+	u8 addr;
+
+	for (addr = 0; addr < 32; addr++) {
+		if (miiphy_read(dev->name, addr, MII_BMSR, &mii_status)	!= 0)
+			/* try next phy */
+			continue;
+
+		/* invalid MII status. More validation required here... */
+		if (mii_status == 0 || mii_status == 0xffff)
+			/* try next phy */
+			continue;
+
+		if (miiphy_read(dev->name, addr, MII_PHYSID1, &tmp) != 0)
+			/* try next phy */
+			continue;
+
+		val = tmp << 16;
+		if (miiphy_read(dev->name, addr, MII_PHYSID2, &tmp) != 0)
+			/* try next phy */
+			continue;
+
+		val |= tmp;
+
+		if ((val & 0xfffffff0) != 0)
+			return addr;
+	}
+	return -1;
+}
+
+static void armdfec_init_rx_desc_ring(struct armdfec_device *darmdfec)
+{
+	struct rx_desc *p_rx_desc;
+	int i;
+
+	/* initialize the Rx descriptors ring */
+	p_rx_desc = darmdfec->p_rxdesc;
+	for (i = 0; i < RINGSZ; i++) {
+		p_rx_desc->cmd_sts = BUF_OWNED_BY_DMA | RX_EN_INT;
+		p_rx_desc->buf_size = PKTSIZE_ALIGN;
+		p_rx_desc->byte_cnt = 0;
+		p_rx_desc->buf_ptr = darmdfec->p_rxbuf + i * PKTSIZE_ALIGN;
+		if (i == (RINGSZ - 1))
+			p_rx_desc->nxtdesc_p = darmdfec->p_rxdesc;
+		else {
+			p_rx_desc->nxtdesc_p = (struct rx_desc *)
+			    ((u32) p_rx_desc + ARMDFEC_RXQ_DESC_ALIGNED_SIZE);
+			p_rx_desc = p_rx_desc->nxtdesc_p;
+		}
+	}
+	darmdfec->p_rxdesc_curr = darmdfec->p_rxdesc;
+}
+
+static int armdfec_init(struct eth_device *dev, bd_t *bd)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+
+	armdfec_init_rx_desc_ring(darmdfec);
+
+	/* Disable interrupts */
+	writel(0, &regs->im);
+	writel(0, &regs->ic);
+	/* Write to ICR to clear interrupts. */
+	writel(0, &regs->iwc);
+
+	/*
+	 * Abort any transmit and receive operations and put DMA
+	 * in idle state.
+	 */
+	abortdma(dev);
+
+	/* Initialize address hash table */
+	init_hashtable(dev);
+
+	/* SDMA configuration */
+	writel(SDCR_BSZ8 |	/* Burst size = 32 bytes */
+		SDCR_RIFB |	/* Rx interrupt on frame */
+		SDCR_BLMT |	/* Little endian transmit */
+		SDCR_BLMR |	/* Little endian receive */
+		SDCR_RC_MAX_RETRANS,	/* Max retransmit count */
+		&regs->sdma_conf);
+	/* Port Configuration */
+	writel(PCR_HS, &regs->pconf);	/* Hash size is 1/2kb */
+	setportconfigext(dev, 1500);
+
+	update_hash_table_mac_address(darmdfec, dev->enetaddr, dev->enetaddr);
+
+	/* Update TX and RX queue descriptor register */
+	writel((u32) darmdfec->p_txdesc, &regs->txcdp[TXQ]);
+	writel((u32) darmdfec->p_rxdesc, &regs->rxfdp[RXQ]);
+	writel((u32) darmdfec->p_rxdesc_curr, &regs->rxcdp[RXQ]);
+
+	/* Enable Interrupts */
+	writel(ALL_INTS, &regs->im);
+
+	/* Enable Ethernet Port */
+	setbits_le32(&regs->pconf, PCR_EN);
+
+	/* Enable RX DMA engine */
+	setbits_le32(&regs->sdma_cmd, SDMA_CMD_ERD);
+
+#ifdef DEBUG
+	eth_dump_regs(dev);
+#endif
+
+#if (defined(CONFIG_MII) || defined(CONFIG_CMD_MII)) \
+			&& defined(CONFIG_SYS_FAULT_ECHO_LINK_DOWN)
+	/* Wait up to 5s for the link status */
+	for (i = 0; i < 5; i++) {
+		u16 phy_adr;
+
+		miiphy_read(dev->name, 0xFF, 0xFF, &phy_adr);
+		/* Return if we get link up */
+		if (miiphy_link(dev->name, phy_adr))
+			return 0;
+		udelay(1000000);
+	}
+
+	printf("No link on %s\n", dev->name);
+	return -1;
+#endif
+	return 0;
+}
+
+static void armdfec_halt(struct eth_device *dev)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+
+	/* Stop RX DMA */
+	clrbits_le32(&regs->sdma_cmd, SDMA_CMD_ERD);
+
+	/*
+	 * Abort any transmit and receive operations and put DMA
+	 * in idle state.
+	 */
+	abortdma(dev);
+
+	/* Disable interrupts */
+	writel(0, &regs->im);
+	writel(0, &regs->ic);
+	writel(0, &regs->iwc);
+
+	/* Disable Port */
+	clrbits_le32(&regs->pconf, PCR_EN);
+}
+
+static int armdfec_send(struct eth_device *dev, volatile void *dataptr,
+		    int datasize)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+	struct tx_desc *p_txdesc = darmdfec->p_txdesc;
+	void *p = (void *) dataptr;
+	int retry = PHY_WAIT_ITERATIONS * PHY_WAIT_MICRO_SECONDS;
+	u32 cmd_sts;
+
+	/* Copy buffer if it's misaligned */
+	if ((u32) dataptr & 0x07) {
+		if (datasize > PKTSIZE_ALIGN) {
+			printf("Non-aligned data too large (%d)\n", datasize);
+			return -1;
+		}
+		memcpy(darmdfec->p_aligned_txbuf, p, datasize);
+		p = darmdfec->p_aligned_txbuf;
+	}
+
+	p_txdesc->cmd_sts = TX_ZERO_PADDING | TX_GEN_CRC;
+	p_txdesc->cmd_sts |= TX_FIRST_DESC | TX_LAST_DESC;
+	p_txdesc->cmd_sts |= BUF_OWNED_BY_DMA;
+	p_txdesc->cmd_sts |= TX_EN_INT;
+	p_txdesc->buf_ptr = (u8 *)p;
+	p_txdesc->byte_cnt = datasize;
+
+	/* Apply send command using high priority TX queue */
+	writel((u32) p_txdesc, &regs->txcdp[TXQ]);
+	writel(SDMA_CMD_TXDL | SDMA_CMD_TXDH | SDMA_CMD_ERD, &regs->sdma_cmd);
+
+	/*
+	 * wait for packet xmit completion
+	 */
+	cmd_sts = readl(&p_txdesc->cmd_sts);
+	while (cmd_sts & BUF_OWNED_BY_DMA) {
+		/* return fail if error is detected */
+		if ((cmd_sts & (TX_ERROR | TX_LAST_DESC)) ==
+			(TX_ERROR | TX_LAST_DESC)) {
+			printf("Err..(%s) in xmit packet\n", __func__);
+			return -1;
+		}
+		cmd_sts = readl(&p_txdesc->cmd_sts);
+		if (!(retry--)) {
+			printf("%s: xmit packet timeout!\n", __func__);
+			return -1;
+		}
+	};
+
+	return 0;
+}
+
+static int armdfec_recv(struct eth_device *dev)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct rx_desc *p_rxdesc_curr = darmdfec->p_rxdesc_curr;
+	u32 cmd_sts;
+	u32 timeout = 0;
+
+	/* wait untill rx packet available or timeout */
+	do {
+		if (timeout < PHY_WAIT_ITERATIONS * PHY_WAIT_MICRO_SECONDS)
+			timeout++;
+		else {
+			debug("%s time out...\n", __func__);
+			return -1;
+		}
+	} while (readl(&p_rxdesc_curr->cmd_sts) & BUF_OWNED_BY_DMA);
+
+	if (p_rxdesc_curr->byte_cnt != 0) {
+		debug
+			("%s: Received %d byte Packet @ 0x%x (cmd_sts= %08x)\n",
+			__func__, (u32) p_rxdesc_curr->byte_cnt,
+			(u32) p_rxdesc_curr->buf_ptr,
+			(u32) p_rxdesc_curr->cmd_sts);
+	}
+
+	/*
+	 * In case received a packet without first/last bits on
+	 * OR the error summary bit is on,
+	 * the packets needs to be dropeed.
+	 */
+	cmd_sts = readl(&p_rxdesc_curr->cmd_sts);
+
+	if ((cmd_sts & (RX_FIRST_DESC | RX_LAST_DESC))
+		!= (RX_FIRST_DESC | RX_LAST_DESC)) {
+
+		printf("Err..(%s) Dropping packet spread on"
+			" multiple descriptors\n", __func__);
+	} else if (cmd_sts & RX_ERROR) {
+		printf("Err..(%s) Dropping packet with errors\n",
+			__func__);
+	} else {
+		/* !!! call higher layer processing */
+		debug("%s: Sending Received packet to"
+			" upper layer (NetReceive)\n", __func__);
+
+		/*
+		 * let the upper layer handle the packet, subtract offset
+		 * as two dummy bytes are added in received buffer see
+		 * PORT_CONFIG_EXT register bit TWO_Byte_Stuff_Mode bit.
+		 */
+		NetReceive((p_rxdesc_curr->buf_ptr + RX_BUF_OFFSET),
+			   (int) (p_rxdesc_curr->byte_cnt - RX_BUF_OFFSET));
+	}
+	/*
+	 * free these descriptors and point next in the ring
+	 */
+	p_rxdesc_curr->cmd_sts = BUF_OWNED_BY_DMA | RX_EN_INT;
+	p_rxdesc_curr->buf_size = PKTSIZE_ALIGN;
+	p_rxdesc_curr->byte_cnt = 0;
+
+	writel((u32) p_rxdesc_curr->nxtdesc_p, &darmdfec->p_rxdesc_curr);
+
+	return 0;
+}
+
+int armada100_fec_register()
+{
+	struct armdfec_device *darmdfec;
+	struct eth_device *dev;
+	int phy_adr;
+
+	darmdfec = malloc(sizeof(struct armdfec_device));
+	if (!darmdfec)
+		goto error;
+
+	memset(darmdfec, 0, sizeof(struct armdfec_device));
+
+	darmdfec->htpr = memalign(8, HASH_ADDR_TABLE_SIZE);
+	if (!darmdfec->htpr)
+		goto error;
+
+	darmdfec->p_rxdesc = (struct rx_desc *) memalign(PKTALIGN,
+			ARMDFEC_RXQ_DESC_ALIGNED_SIZE * RINGSZ + 1);
+
+	if (!darmdfec->p_rxdesc)
+		goto error;
+
+	darmdfec->p_rxbuf = (u8 *) memalign(PKTALIGN,
+			RINGSZ * PKTSIZE_ALIGN + 1);
+	if (!darmdfec->p_rxbuf)
+		goto error;
+
+	darmdfec->p_aligned_txbuf = memalign(8, PKTSIZE_ALIGN);
+	if (!darmdfec->p_aligned_txbuf)
+		goto error;
+
+	darmdfec->p_txdesc = (struct tx_desc *)
+		memalign(PKTALIGN, sizeof(struct tx_desc) + 1);
+	if (!darmdfec->p_txdesc)
+		goto error;
+
+	dev = &darmdfec->dev;
+	/* Assign ARMADA100 Fast Ethernet Controller Base Address */
+	darmdfec->regs = (void *) ARMD1_FEC_BASE;
+
+	/* must be less than NAMESIZE (16) */
+	strcpy(dev->name, "armd-fec0");
+
+	/* Read mac from env if available */
+	eth_getenv_enetaddr("ethaddr", dev->enetaddr);
+
+	dev->init = armdfec_init;
+	dev->halt = armdfec_halt;
+	dev->send = armdfec_send;
+	dev->recv = armdfec_recv;
+
+	eth_register(dev);
+
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+	miiphy_register(dev->name, smi_reg_read, smi_reg_write);
+
+#if defined(CONFIG_PHY_BASE_ADR)
+	miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ,
+		     (u16) CONFIG_PHY_BASE_ADR);
+#else
+	/* Search phy address from range 0-31 */
+	phy_adr = ethernet_phy_detect(dev);
+	if (phy_adr < 0) {
+		printf("Error: PHY not detected at address range 0-31\n");
+		return -1;
+	} else {
+		debug("PHY detected@addr %d\n", phy_adr);
+		miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ, (u16)phy_adr);
+	}
+#endif
+#endif
+	return 0;
+
+error:
+	free(darmdfec->p_aligned_txbuf);
+	free(darmdfec->p_rxbuf);
+	free(darmdfec->p_rxdesc);
+	free(darmdfec->htpr);
+	free(darmdfec);
+	printf("Err.. %s Failed to allocate memory\n", __func__);
+	return -1;
+}
diff --git a/drivers/net/armada100_fec.h b/drivers/net/armada100_fec.h
new file mode 100644
index 0000000..e2df4fc
--- /dev/null
+++ b/drivers/net/armada100_fec.h
@@ -0,0 +1,232 @@
+/*
+ * (C) Copyright 2011
+ * eInfochips Ltd. <www.einfochips.com>
+ * Written-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
+ *
+ * (C) Copyright 2010
+ * Marvell Semiconductor <www.marvell.com>
+ * Contributor: Mahavir Jain <mjain@marvell.com>
+ *
+ * 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., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301 USA
+ */
+
+#ifndef __ARMADA100_FEC_H__
+#define __ARMADA100_FEC_H__
+
+#ifndef TRUE
+#define TRUE 1
+#endif
+#ifndef FALSE
+#define FALSE 0
+#endif
+
+#define PORT_NUM		0x0
+
+/* RX & TX descriptor command */
+#define BUF_OWNED_BY_DMA        (1<<31)
+
+/* RX descriptor status */
+#define RX_EN_INT               (1<<23)
+#define RX_FIRST_DESC           (1<<17)
+#define RX_LAST_DESC            (1<<16)
+#define RX_ERROR                (1<<15)
+
+/* TX descriptor command */
+#define TX_EN_INT               (1<<23)
+#define TX_GEN_CRC              (1<<22)
+#define TX_ZERO_PADDING         (1<<18)
+#define TX_FIRST_DESC           (1<<17)
+#define TX_LAST_DESC            (1<<16)
+#define TX_ERROR                (1<<15)
+
+/* smi register */
+#define SMI_BUSY                (1<<28)	/* 0 - Write, 1 - Read  */
+#define SMI_R_VALID             (1<<27)	/* 0 - Write, 1 - Read  */
+#define SMI_OP_W                (0<<26)	/* Write operation      */
+#define SMI_OP_R                (1<<26)	/* Read operation */
+
+#define HASH_ADD                0
+#define HASH_DELETE             1
+#define HASH_ADDR_TABLE_SIZE    0x4000	/* 16K (1/2K address - PCR_HS == 1) */
+#define HOP_NUMBER              12
+
+#define PHY_WAIT_ITERATIONS     1000	/* 1000 iterations * 10uS = 10mS max */
+#define PHY_WAIT_MICRO_SECONDS  10
+
+#define ETH_HW_IP_ALIGN         2	/* hw aligns IP header */
+#define ETH_EXTRA_HEADER        (6+6+2+4)
+					/* dest+src addr+protocol id+crc */
+#define MAX_PKT_SIZE            1536
+
+
+/* Bit definitions of the SDMA Config Reg */
+#define SDCR_BSZ_OFF            12
+#define SDCR_BSZ8               (3<<SDCR_BSZ_OFF)
+#define SDCR_BSZ4               (2<<SDCR_BSZ_OFF)
+#define SDCR_BSZ2               (1<<SDCR_BSZ_OFF)
+#define SDCR_BSZ1               (0<<SDCR_BSZ_OFF)
+#define SDCR_BLMR               (1<<6)
+#define SDCR_BLMT               (1<<7)
+#define SDCR_RIFB               (1<<9)
+#define SDCR_RC_OFF             2
+#define SDCR_RC_MAX_RETRANS     (0xf << SDCR_RC_OFF)
+
+/* SDMA_CMD */
+#define SDMA_CMD_AT             (1<<31)
+#define SDMA_CMD_TXDL           (1<<24)
+#define SDMA_CMD_TXDH           (1<<23)
+#define SDMA_CMD_AR             (1<<15)
+#define SDMA_CMD_ERD            (1<<7)
+
+
+/* Bit definitions of the Port Config Reg */
+#define PCR_HS                  (1<<12)
+#define PCR_EN                  (1<<7)
+#define PCR_PM                  (1<<0)
+
+/* Bit definitions of the Port Config Extend Reg */
+#define PCXR_2BSM               (1<<28)
+#define PCXR_DSCP_EN            (1<<21)
+#define PCXR_MFL_1518           (0<<14)
+#define PCXR_MFL_1536           (1<<14)
+#define PCXR_MFL_2048           (2<<14)
+#define PCXR_MFL_64K            (3<<14)
+#define PCXR_FLP                (1<<11)
+#define PCXR_PRIO_TX_OFF        3
+#define PCXR_TX_HIGH_PRI        (7<<PCXR_PRIO_TX_OFF)
+
+/*
+ *  * Bit definitions of the Interrupt Cause Reg
+ *   * and Interrupt MASK Reg is the same
+ *    */
+#define ICR_RXBUF               (1<<0)
+#define ICR_TXBUF_H             (1<<2)
+#define ICR_TXBUF_L             (1<<3)
+#define ICR_TXEND_H             (1<<6)
+#define ICR_TXEND_L             (1<<7)
+#define ICR_RXERR               (1<<8)
+#define ICR_TXERR_H             (1<<10)
+#define ICR_TXERR_L             (1<<11)
+#define ICR_TX_UDR              (1<<13)
+#define ICR_MII_CH              (1<<28)
+
+#define ALL_INTS (ICR_TXBUF_H  | ICR_TXBUF_L  | ICR_TX_UDR |\
+				ICR_TXERR_H  | ICR_TXERR_L |\
+				ICR_TXEND_H  | ICR_TXEND_L |\
+				ICR_RXBUF | ICR_RXERR  | ICR_MII_CH)
+
+#define PHY_MASK               0x0000001f
+
+#define to_darmdfec(_kd) container_of(_kd, struct armdfec_device, dev)
+/* Size of a Tx/Rx descriptor used in chain list data structure */
+#define ARMDFEC_RXQ_DESC_ALIGNED_SIZE \
+	(((sizeof(struct rx_desc) / PKTALIGN) + 1) * PKTALIGN)
+
+#define RX_BUF_OFFSET		0x2
+#define RXQ			0x0	/* RX Queue 0 */
+#define TXQ			0x1	/* TX Queue 1 */
+
+struct addr_table_entry_t {
+	u32 lo;
+	u32 hi;
+};
+
+/* Bit fields of a Hash Table Entry */
+enum hash_table_entry {
+	HTEVALID = 1,
+	HTESKIP = 2,
+	HTERD = 4,
+	HTERDBIT = 2
+};
+
+struct tx_desc {
+	u32 cmd_sts;		/* Command/status field */
+	u16 reserved;
+	u16 byte_cnt;		/* buffer byte count */
+	u8 *buf_ptr;		/* pointer to buffer for this descriptor */
+	struct tx_desc *nextdesc_p;	/* Pointer to next descriptor */
+};
+
+struct rx_desc {
+	u32 cmd_sts;		/* Descriptor command status */
+	u16 byte_cnt;		/* Descriptor buffer byte count */
+	u16 buf_size;		/* Buffer size */
+	u8 *buf_ptr;		/* Descriptor buffer pointer */
+	struct rx_desc *nxtdesc_p;	/* Next descriptor pointer */
+};
+
+/*
+ * Armada100 Fast Ethernet controller Registers
+ * Refer Datasheet Appendix A.22
+ */
+struct armdfec_reg {
+	u32 phyadr;			/* PHY Address */
+	u32 pad1[3];
+	u32 smi;			/* SMI */
+	u32 pad2[0xFB];
+	u32 pconf;			/* Port configuration */
+	u32 pad3;
+	u32 pconf_ext;			/* Port configuration extend */
+	u32 pad4;
+	u32 pcmd;			/* Port Command */
+	u32 pad5;
+	u32 pstatus;			/* Port Status */
+	u32 pad6;
+	u32 spar;			/* Serial Parameters */
+	u32 pad7;
+	u32 htpr;			/* Hash table pointer */
+	u32 pad8;
+	u32 fcsal;			/* Flow control source address low */
+	u32 pad9;
+	u32 fcsah;			/* Flow control source address high */
+	u32 pad10;
+	u32 sdma_conf;			/* SDMA configuration */
+	u32 pad11;
+	u32 sdma_cmd;			/* SDMA command */
+	u32 pad12;
+	u32 ic;				/* Interrupt cause */
+	u32 iwc;			/* Interrupt write to clear */
+	u32 im;				/* Interrupt mask */
+	u32 pad13;
+	u32 *eth_idscpp[4];		/* Eth0 IP Differentiated Services Code
+					   Point to Priority 0 Low */
+	u32 eth_vlan_p;			/* Eth0 VLAN Priority Tag to Priority */
+	u32 pad14[3];
+	struct rx_desc *rxfdp[4];	/* Ethernet First Rx Descriptor
+					   Pointer */
+	u32 pad15[4];
+	struct rx_desc *rxcdp[4];	/* Ethernet Current Rx Descriptor
+					   Pointer */
+	u32 pad16[0x0C];
+	struct tx_desc *txcdp[2];	/* Ethernet Current Tx Descriptor
+					   Pointer */
+};
+
+struct armdfec_device {
+	struct eth_device dev;
+	struct armdfec_reg *regs;
+	struct tx_desc *p_txdesc;
+	struct rx_desc *p_rxdesc;
+	struct rx_desc *p_rxdesc_curr;
+	u8 *p_rxbuf;
+	u8 *p_aligned_txbuf;
+	u8 *htpr;		/* hash pointer */
+};
+
+#endif /* __ARMADA100_FEC_H__ */
diff --git a/include/netdev.h b/include/netdev.h
index 6f0a971..d12f179 100644
--- a/include/netdev.h
+++ b/include/netdev.h
@@ -94,6 +94,7 @@ int xilinx_emaclite_initialize (bd_t *bis, int base_addr);
 int sh_eth_initialize(bd_t *bis);
 int dm9000_initialize(bd_t *bis);
 int fecmxc_initialize(bd_t *bis);
+int armada100_fec_register(void);
 
 /* Boards with PCI network controllers can call this from their board_eth_init()
  * function to initialize whatever's on board.
-- 
1.7.0.4

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-24 13:07 [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100 Ajay Bhargav
@ 2011-08-24 15:42 ` Mike Frysinger
  2011-08-24 16:38   ` Marek Vasut
  2011-08-25  5:10   ` Ajay Bhargav
  2011-08-24 16:36 ` Marek Vasut
  1 sibling, 2 replies; 14+ messages in thread
From: Mike Frysinger @ 2011-08-24 15:42 UTC (permalink / raw)
  To: u-boot

On Wednesday, August 24, 2011 09:07:18 Ajay Bhargav wrote:
> +       darmdfec->p_rxdesc = (struct rx_desc *) memalign(PKTALIGN,
> +                       ARMDFEC_RXQ_DESC_ALIGNED_SIZE * RINGSZ + 1);

memalign() returns a void*, so you shouldnt need to cast its return value (you 
do this a couple of times)

> +	/* Read mac from env if available */
> +	eth_getenv_enetaddr("ethaddr", dev->enetaddr);

you shouldnt need to do this.  the higher layers will take care of this for 
you when you set write_hwaddr

also, it seems like some of my previous feedback wasnt addressed ?

> +     while (cmd_sts & BUF_OWNED_BY_DMA) {
> ...
> +     };

no semi-colon needed

> +int armada100_fec_initialize()
> +{
> ...
> +     darmdfec->regs = (void *) ARMD1_FEC_BASE;

make the reg base a parameter to armada100_fec_initialize()

> +#if defined(CONFIG_PHY_BASE_ADR)
> +     miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ,
> +                  (u16) CONFIG_PHY_BASE_ADR);
> +#else
> +     /* Search phy address from range 0-31 */
> +     phy_adr = ethernet_phy_detect(dev);
> +     if (phy_adr < 0) {
> +             printf("Error: PHY not detected at address range 0-31\n");
> +             return -1;
> +     } else {
> +             debug("PHY detected at addr %d\n", phy_adr);
> +             miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ,
> +                          (u16) phy_adr);
> +     }
> +#endif

this should be done in the armdfec_init() func, not the initialize func
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110824/ad840bed/attachment.pgp 

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-24 13:07 [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100 Ajay Bhargav
  2011-08-24 15:42 ` Mike Frysinger
@ 2011-08-24 16:36 ` Marek Vasut
  1 sibling, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2011-08-24 16:36 UTC (permalink / raw)
  To: u-boot

On Wednesday, August 24, 2011 03:07:18 PM Ajay Bhargav wrote:
> This patch adds support for Fast Ethernet Controller driver for
> Armada100 series.
> 
> Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> ---

[...]

> diff --git a/drivers/net/armada100_fec.c b/drivers/net/armada100_fec.c
> new file mode 100644
> index 0000000..e36dca6
> --- /dev/null
> +++ b/drivers/net/armada100_fec.c
> @@ -0,0 +1,761 @@
> +/*
> + * (C) Copyright 2011
> + * eInfochips Ltd. <www.einfochips.com>
> + * Written-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> + *
> + * (C) Copyright 2010
> + * Marvell Semiconductor <www.marvell.com>
> + * Contributor: Mahavir Jain <mjain@marvell.com>
> + *
> + * 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., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301 USA
> + */
> +
> +#include <common.h>
> +#include <net.h>
> +#include <malloc.h>
> +#include <miiphy.h>
> +#include <netdev.h>
> +#include <asm/types.h>
> +#include <asm/byteorder.h>
> +#include <linux/err.h>
> +#include <linux/mii.h>
> +#include <asm/io.h>
> +#include <asm/arch/armada100.h>
> +#include "armada100_fec.h"
> +
> +#define  PHY_ADR_REQ     0xFF	/* Magic number to read/write PHY address */
> +
> +#ifdef DEBUG
> +static int eth_dump_regs(struct eth_device *dev)
> +{
> +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> +	struct armdfec_reg *regs = darmdfec->regs;
> +	unsigned int i = 0;
> +
> +	printf("\noffset: phy_adr, value: 0x%x\n", readl(&regs->phyadr));
> +	printf("offset: smi, value: 0x%x\n", readl(&regs->smi));
> +	for (i = 0x400; i <= 0x4e4; i += 4)
> +		printf("offset: 0x%x, value: 0x%x\n",
> +			i, readl(ARMD1_FEC_BASE + i));
> +	return 0;
> +}
> +#endif
> +
> +static int armdfec_phy_timeout(u32 reg, u32 flag, int cond)
> +{
> +	u32 timeout = PHY_WAIT_ITERATIONS;
> +	while (--timeout) {
> +		if (cond && (readl(reg) & flag))
> +			break;
> +		else if (!cond && !(readl(reg) & flag))

You can read the register into some temporary variable so you don't need the 
readl() at two places.

> +			break;
> +		udelay(PHY_WAIT_MICRO_SECONDS);
> +	}
> +	return !timeout;
> +}
> +
> +static int smi_reg_read(const char *devname, u8 phy_addr, u8 phy_reg,
> +			u16 *value)
> +{
> +	struct eth_device *dev = eth_get_dev_by_name(devname);
> +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> +	struct armdfec_reg *regs = darmdfec->regs;
> +	u32 val, reg_data;
> +
> +	if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {
> +		reg_data = readl(&regs->phyadr);
> +		*value = (u16) (reg_data & 0x1f);
> +		return 0;
> +	}
> +
> +	/* check parameters */
> +	if (phy_addr > PHY_MASK) {
> +		printf("Err..(%s) Invalid phy address: 0x%X\n",
> +				__func__, phy_addr);
> +		return -EINVAL;
> +	}
> +	if (phy_reg > PHY_MASK) {
> +		printf("Err..(%s) Invalid register offset: 0x%X\n",
> +				__func__, phy_reg);
> +		return -EINVAL;
> +	}
> +
> +	/* wait for the SMI register to become available */
> +	if (armdfec_phy_timeout((u32)&regs->smi, SMI_BUSY, FALSE)) {

Adjust the function so you don't need the cast.

> +		printf("Error (%s) PHY busy timeout\n",	__func__);
> +		return -1;
> +	}
> +
> +	writel(phy_addr << 16 | phy_reg << 21 | SMI_OP_R, &regs->smi);

Parentheses missing maybe ?

> +
> +	/* now wait for the data to be valid */
> +	if (armdfec_phy_timeout((u32)&regs->smi, SMI_R_VALID, TRUE)) {
> +		val = readl(&regs->smi);
> +		printf("Err (%s) PHY Read timeout, val=0x%x\n", __func__, val);
> +		return -1;
> +	}
> +	val = readl(&regs->smi);
> +	*value = val & 0xffff;
> +
> +	return 0;
> +}
> +
> +static int smi_reg_write(const char *devname,
> +	 u8 phy_addr, u8 phy_reg, u16 value)
> +{
> +	struct eth_device *dev = eth_get_dev_by_name(devname);
> +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> +	struct armdfec_reg *regs = darmdfec->regs;
> +
> +	if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {
> +		clrsetbits_le32(&regs->phyadr, 0x1f, value & 0x1f);
> +		return 0;
> +	}
> +
> +	/* check parameters */
> +	if (phy_addr > PHY_MASK) {
> +		printf("Err..(%s) Invalid phy address\n", __func__);
> +		return -EINVAL;
> +	}
> +	if (phy_reg > PHY_MASK) {
> +		printf("Err..(%s) Invalid register offset\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	/* wait for the SMI register to become available */
> +	if (armdfec_phy_timeout((u32)&regs->smi, SMI_BUSY, FALSE)) {
> +		printf("Error (%s) PHY busy timeout\n",	__func__);
> +		return -1;
> +	}
> +
> +	writel(phy_addr << 16 | phy_reg << 21 | SMI_OP_W | (value & 0xffff),
> +			&regs->smi);
> +	return 0;
> +}
> +
> +/*
> + * Abort any transmit and receive operations and put DMA
> + * in idle state. AT and AR bits are cleared upon entering
> + * in IDLE state. So poll those bits to verify operation.
> + */
> +static void abortdma(struct eth_device *dev)
> +{
> +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> +	struct armdfec_reg *regs = darmdfec->regs;
> +	int delay;
> +	int maxretries = 40;
> +
> +	do {
> +		writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
> +		udelay(100);
> +
> +		delay = 10;
> +		while ((readl(&regs->sdma_cmd) &
> +			(SDMA_CMD_AR | SDMA_CMD_AT))
> +			&& delay-- > 0) {
> +			udelay(10);
> +		}
> +	} while (maxretries-- > 0 && delay <= 0);

Didn't I comment on this one in V1?

> +
> +	if (maxretries <= 0)
> +		printf("%s : DMA Stuck\n", __func__);
> +}

[...]

> +int armada100_fec_register()
> +{
> +	struct armdfec_device *darmdfec;
> +	struct eth_device *dev;
> +	int phy_adr;
> +
> +	darmdfec = malloc(sizeof(struct armdfec_device));
> +	if (!darmdfec)
> +		goto error;
> +
> +	memset(darmdfec, 0, sizeof(struct armdfec_device));
> +
> +	darmdfec->htpr = memalign(8, HASH_ADDR_TABLE_SIZE);
> +	if (!darmdfec->htpr)
> +		goto error;
> +
> +	darmdfec->p_rxdesc = (struct rx_desc *) memalign(PKTALIGN,
> +			ARMDFEC_RXQ_DESC_ALIGNED_SIZE * RINGSZ + 1);
> +
> +	if (!darmdfec->p_rxdesc)
> +		goto error;
> +
> +	darmdfec->p_rxbuf = (u8 *) memalign(PKTALIGN,
> +			RINGSZ * PKTSIZE_ALIGN + 1);

Is the cast needed ? You should review your casts, most of them can be solved by 
using proper type!

> +	if (!darmdfec->p_rxbuf)
> +		goto error;
> +
> +	darmdfec->p_aligned_txbuf = memalign(8, PKTSIZE_ALIGN);
> +	if (!darmdfec->p_aligned_txbuf)
> +		goto error;
> +
> +	darmdfec->p_txdesc = (struct tx_desc *)
> +		memalign(PKTALIGN, sizeof(struct tx_desc) + 1);
> +	if (!darmdfec->p_txdesc)
> +		goto error;
> +
> +	dev = &darmdfec->dev;
> +	/* Assign ARMADA100 Fast Ethernet Controller Base Address */
> +	darmdfec->regs = (void *) ARMD1_FEC_BASE;

Cheers!

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-24 15:42 ` Mike Frysinger
@ 2011-08-24 16:38   ` Marek Vasut
  2011-08-25  5:24     ` Ajay Bhargav
  2011-08-25  5:10   ` Ajay Bhargav
  1 sibling, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2011-08-24 16:38 UTC (permalink / raw)
  To: u-boot

On Wednesday, August 24, 2011 05:42:06 PM Mike Frysinger wrote:
> On Wednesday, August 24, 2011 09:07:18 Ajay Bhargav wrote:
> > +       darmdfec->p_rxdesc = (struct rx_desc *) memalign(PKTALIGN,
> > +                       ARMDFEC_RXQ_DESC_ALIGNED_SIZE * RINGSZ + 1);
> 
> memalign() returns a void*, so you shouldnt need to cast its return value
> (you do this a couple of times)
> 
> > +	/* Read mac from env if available */
> > +	eth_getenv_enetaddr("ethaddr", dev->enetaddr);
> 
> you shouldnt need to do this.  the higher layers will take care of this for
> you when you set write_hwaddr
> 
> also, it seems like some of my previous feedback wasnt addressed ?

I have exactly the same feeling :-(

Ajay, please go through the feedback, if you don't understand something, just 
ask instead of hoping we won't notice ... we will, we see everything ;-)

Cheers

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-24 15:42 ` Mike Frysinger
  2011-08-24 16:38   ` Marek Vasut
@ 2011-08-25  5:10   ` Ajay Bhargav
  2011-08-25 14:13     ` Mike Frysinger
  1 sibling, 1 reply; 14+ messages in thread
From: Ajay Bhargav @ 2011-08-25  5:10 UTC (permalink / raw)
  To: u-boot


----- "Mike Frysinger" <vapier@gentoo.org> wrote:

> On Wednesday, August 24, 2011 09:07:18 Ajay Bhargav wrote:
> > +       darmdfec->p_rxdesc = (struct rx_desc *) memalign(PKTALIGN,
> > +                       ARMDFEC_RXQ_DESC_ALIGNED_SIZE * RINGSZ +
> 1);
> 
> memalign() returns a void*, so you shouldnt need to cast its return
> value (you 
> do this a couple of times)
> 
> > +	/* Read mac from env if available */
> > +	eth_getenv_enetaddr("ethaddr", dev->enetaddr);
> 
> you shouldnt need to do this.  the higher layers will take care of
> this for 
> you when you set write_hwaddr
> 

I do not have a hardware storage for MAC on my controller. write_hwaddr
is not needed for me.

> also, it seems like some of my previous feedback wasnt addressed ?
> 

I might have missed some points. My apologies.

> > +     while (cmd_sts & BUF_OWNED_BY_DMA) {
> > ...
> > +     };
> 
> no semi-colon needed
> 
> > +int armada100_fec_initialize()
> > +{
> > ...
> > +     darmdfec->regs = (void *) ARMD1_FEC_BASE;
> 
> make the reg base a parameter to armada100_fec_initialize()
> 

This driver is for Armada100 series and base address is same for
the whole series, so i did not feel passing it as a parameter. Can
you please tell me if there is any specific reason for the same?

> > +#if defined(CONFIG_PHY_BASE_ADR)
> > +     miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ,
> > +                  (u16) CONFIG_PHY_BASE_ADR);
> > +#else
> > +     /* Search phy address from range 0-31 */
> > +     phy_adr = ethernet_phy_detect(dev);
> > +     if (phy_adr < 0) {
> > +             printf("Error: PHY not detected at address range
> 0-31\n");
> > +             return -1;
> > +     } else {
> > +             debug("PHY detected at addr %d\n", phy_adr);
> > +             miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ,
> > +                          (u16) phy_adr);
> > +     }
> > +#endif
> 
> this should be done in the armdfec_init() func, not the initialize
> func
> -mike

Okay.. will move it there.

Thanks & Regards,
Ajay Bhargav

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-24 16:38   ` Marek Vasut
@ 2011-08-25  5:24     ` Ajay Bhargav
  2011-08-25 12:14       ` Marek Vasut
  0 siblings, 1 reply; 14+ messages in thread
From: Ajay Bhargav @ 2011-08-25  5:24 UTC (permalink / raw)
  To: u-boot


----- "Marek Vasut" <marek.vasut@gmail.com> wrote:

> On Wednesday, August 24, 2011 05:42:06 PM Mike Frysinger wrote:
> > On Wednesday, August 24, 2011 09:07:18 Ajay Bhargav wrote:
> > > +       darmdfec->p_rxdesc = (struct rx_desc *)
> memalign(PKTALIGN,
> > > +                       ARMDFEC_RXQ_DESC_ALIGNED_SIZE * RINGSZ +
> 1);
> > 
> > memalign() returns a void*, so you shouldnt need to cast its return
> value
> > (you do this a couple of times)
> > 
> > > +	/* Read mac from env if available */
> > > +	eth_getenv_enetaddr("ethaddr", dev->enetaddr);
> > 
> > you shouldnt need to do this.  the higher layers will take care of
> this for
> > you when you set write_hwaddr
> > 
> > also, it seems like some of my previous feedback wasnt addressed ?
> 
> I have exactly the same feeling :-(
> 
> Ajay, please go through the feedback, if you don't understand
> something, just 
> ask instead of hoping we won't notice ... we will, we see everything
> ;-)
> 
> Cheers
> 
I did go through your feedbacks.. My apologies if I missed. Marek, Its good
for me that you guys are monitoring :) it will surely help me to write a better
code :)

Thanks,
Ajay Bhargav

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-25  5:24     ` Ajay Bhargav
@ 2011-08-25 12:14       ` Marek Vasut
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2011-08-25 12:14 UTC (permalink / raw)
  To: u-boot

On Thursday, August 25, 2011 07:24:22 AM Ajay Bhargav wrote:
> ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > On Wednesday, August 24, 2011 05:42:06 PM Mike Frysinger wrote:
> > > On Wednesday, August 24, 2011 09:07:18 Ajay Bhargav wrote:
> > > > +       darmdfec->p_rxdesc = (struct rx_desc *)
> > 
> > memalign(PKTALIGN,
> > 
> > > > +                       ARMDFEC_RXQ_DESC_ALIGNED_SIZE * RINGSZ +
> > 
> > 1);
> > 
> > > memalign() returns a void*, so you shouldnt need to cast its return
> > 
> > value
> > 
> > > (you do this a couple of times)
> > > 
> > > > +	/* Read mac from env if available */
> > > > +	eth_getenv_enetaddr("ethaddr", dev->enetaddr);
> > > 
> > > you shouldnt need to do this.  the higher layers will take care of
> > 
> > this for
> > 
> > > you when you set write_hwaddr
> > > 
> > > also, it seems like some of my previous feedback wasnt addressed ?
> > 
> > I have exactly the same feeling :-(
> > 
> > Ajay, please go through the feedback, if you don't understand
> > something, just
> > ask instead of hoping we won't notice ... we will, we see everything
> > ;-)
> > 
> > Cheers
> 
> I did go through your feedbacks.. My apologies if I missed. Marek, Its good
> for me that you guys are monitoring :) it will surely help me to write a
> better code :)

It's good thing you don't whine and run away ... some people do. Btw. your code 
is quite good, it just need the finishing touch :)

Cheers
> 
> Thanks,
> Ajay Bhargav

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-25  5:10   ` Ajay Bhargav
@ 2011-08-25 14:13     ` Mike Frysinger
  0 siblings, 0 replies; 14+ messages in thread
From: Mike Frysinger @ 2011-08-25 14:13 UTC (permalink / raw)
  To: u-boot

On Thursday, August 25, 2011 01:10:30 Ajay Bhargav wrote:
> ----- "Mike Frysinger" <vapier@gentoo.org> wrote:
> > On Wednesday, August 24, 2011 09:07:18 Ajay Bhargav wrote:
> > > +	/* Read mac from env if available */
> > > +	eth_getenv_enetaddr("ethaddr", dev->enetaddr);
> > 
> > you shouldnt need to do this.  the higher layers will take care of
> > this for
> > you when you set write_hwaddr
> 
> I do not have a hardware storage for MAC on my controller. write_hwaddr
> is not needed for me.

ok, but you should not be touching dev->enetaddr in your registration 
function.  the main net/eth.c:eth_initialize() takes care of this for you.

> > > +int armada100_fec_initialize()
> > > +{
> > > ...
> > > +     darmdfec->regs = (void *) ARMD1_FEC_BASE;
> > 
> > make the reg base a parameter to armada100_fec_initialize()
> 
> This driver is for Armada100 series and base address is same for
> the whole series, so i did not feel passing it as a parameter. Can
> you please tell me if there is any specific reason for the same?

drivers should be written for the IP they control, not for specific SoCs or 
boards.  and what people often start off with "this SoC only has one MAC so 
screw multi-instance" quite frequently turns into "this next SoC supports 
multiple MACs!".

i'm not familiar with the Armada100, or the MAC IP that is in that SoC, but 
this story repeats itself constantly in the SoC world because people focus on 
the one specific SoC they have in their hand and not the bigger picture.  
simply witness the ARM hell that Linux is currently in and is being cleaned up 
through the Linaro organization.
-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/20110825/1bb6d465/attachment.pgp 

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
       [not found] <1826454834.6634.1314336622428.JavaMail.root@ahm.einfochips.com>
@ 2011-08-26  5:33 ` Ajay Bhargav
  0 siblings, 0 replies; 14+ messages in thread
From: Ajay Bhargav @ 2011-08-26  5:33 UTC (permalink / raw)
  To: u-boot


----- "Mike Frysinger" <vapier@gentoo.org> wrote:

> On Thursday, August 25, 2011 01:10:30 Ajay Bhargav wrote:
> > ----- "Mike Frysinger" <vapier@gentoo.org> wrote:
> > > On Wednesday, August 24, 2011 09:07:18 Ajay Bhargav wrote:
> > > > +	/* Read mac from env if available */
> > > > +	eth_getenv_enetaddr("ethaddr", dev->enetaddr);
> > > 
> > > you shouldnt need to do this.  the higher layers will take care
> of
> > > this for
> > > you when you set write_hwaddr
> > 
> > I do not have a hardware storage for MAC on my controller.
> write_hwaddr
> > is not needed for me.
> 
> ok, but you should not be touching dev->enetaddr in your registration
> 
> function.  the main net/eth.c:eth_initialize() takes care of this for
> you.
> 
> > > > +int armada100_fec_initialize()
> > > > +{
> > > > ...
> > > > +     darmdfec->regs = (void *) ARMD1_FEC_BASE;
> > > 
> > > make the reg base a parameter to armada100_fec_initialize()
> > 
> > This driver is for Armada100 series and base address is same for
> > the whole series, so i did not feel passing it as a parameter. Can
> > you please tell me if there is any specific reason for the same?
> 
> drivers should be written for the IP they control, not for specific
> SoCs or 
> boards.  and what people often start off with "this SoC only has one
> MAC so 
> screw multi-instance" quite frequently turns into "this next SoC
> supports 
> multiple MACs!".
> 
> i'm not familiar with the Armada100, or the MAC IP that is in that
> SoC, but 
> this story repeats itself constantly in the SoC world because people
> focus on 
> the one specific SoC they have in their hand and not the bigger
> picture.  
> simply witness the ARM hell that Linux is currently in and is being
> cleaned up 
> through the Linaro organization.
> -mike

Hi Mike,

I got your point.. Thanks for clearing this. I will make sure that this
kind of problem does not happen in my case :)

Regards,
Ajay

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-25 11:07 ` Ajay Bhargav
@ 2011-08-25 12:19   ` Marek Vasut
  2011-08-25 12:12     ` Ajay Bhargav
  0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2011-08-25 12:19 UTC (permalink / raw)
  To: u-boot

On Thursday, August 25, 2011 01:07:32 PM Ajay Bhargav wrote:
> ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> 
> [...]
> 
> > > +static void abortdma(struct eth_device *dev)
> > > +{
> > > +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> > > +	struct armdfec_reg *regs = darmdfec->regs;
> > > +	int delay;
> > > +	int maxretries = 40;
> > > +
> > > +	do {
> > > +		writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
> > > +		udelay(100);
> > > +
> > > +		delay = 10;
> > > +		while ((readl(&regs->sdma_cmd) &
> > > +			(SDMA_CMD_AR | SDMA_CMD_AT))
> > > +			&& delay-- > 0) {
> > > +			udelay(10);
> > > +		}
> > > +	} while (maxretries-- > 0 && delay <= 0);
> > 
> > Didn't I comment on this one in V1?
> 
> I modified it as follows... Is it more readable now? :)
> 
> while (maxretries--) {
> 	writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
> 	udelay(100);
> 
> 	delay = 10;
> 	while ((readl(&regs->sdma_cmd) & (SDMA_CMD_AR | SDMA_CMD_AT))
> 			&& delay--)
> 		udelay(10);
> 	if(delay)
> 		break;

 	delay = 10;
 	while (--delay) {
		tmp = readl(&regs->sdma_cmd);
		if (!(tmp & (SDMA_CMD_AR | SDMA_CMD_AT))
			break;
 		udelay(10);
	}
 	if (delay)
 		break;

It makes the code horizontally shorter. What do you think? Btw there's a rule in 
U-Boot that multi-line statements must have braces.

> }
> 
> Regards,
> Ajay Bhargav

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-25  5:21 ` Ajay Bhargav
@ 2011-08-25 12:15   ` Marek Vasut
  0 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2011-08-25 12:15 UTC (permalink / raw)
  To: u-boot

On Thursday, August 25, 2011 07:21:25 AM Ajay Bhargav wrote:
> ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > On Wednesday, August 24, 2011 03:07:18 PM Ajay Bhargav wrote:
> > > This patch adds support for Fast Ethernet Controller driver for
> > > Armada100 series.
> > > 
> > > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > > ---
> > 
> > [...]
> 
> [...]
> 
> > > +
> > > +static int armdfec_phy_timeout(u32 reg, u32 flag, int cond)
> > > +{
> > > +	u32 timeout = PHY_WAIT_ITERATIONS;
> > > +	while (--timeout) {
> > > +		if (cond && (readl(reg) & flag))
> > > +			break;
> > > +		else if (!cond && !(readl(reg) & flag))
> > 
> > You can read the register into some temporary variable so you don't
> > need the
> > readl() at two places.
> 
> readl will be called only once... do i really need a temp var?
> 

I think it'll help the readability a bit.

Cheers

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
  2011-08-25 12:19   ` Marek Vasut
@ 2011-08-25 12:12     ` Ajay Bhargav
  0 siblings, 0 replies; 14+ messages in thread
From: Ajay Bhargav @ 2011-08-25 12:12 UTC (permalink / raw)
  To: u-boot


----- "Marek Vasut" <marek.vasut@gmail.com> wrote:

> On Thursday, August 25, 2011 01:07:32 PM Ajay Bhargav wrote:
> > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > 
> > [...]
> > 
> > > > +static void abortdma(struct eth_device *dev)
> > > > +{
> > > > +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> > > > +	struct armdfec_reg *regs = darmdfec->regs;
> > > > +	int delay;
> > > > +	int maxretries = 40;
> > > > +
> > > > +	do {
> > > > +		writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
> > > > +		udelay(100);
> > > > +
> > > > +		delay = 10;
> > > > +		while ((readl(&regs->sdma_cmd) &
> > > > +			(SDMA_CMD_AR | SDMA_CMD_AT))
> > > > +			&& delay-- > 0) {
> > > > +			udelay(10);
> > > > +		}
> > > > +	} while (maxretries-- > 0 && delay <= 0);
> > > 
> > > Didn't I comment on this one in V1?
> > 
> > I modified it as follows... Is it more readable now? :)
> > 
> > while (maxretries--) {
> > 	writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
> > 	udelay(100);
> > 
> > 	delay = 10;
> > 	while ((readl(&regs->sdma_cmd) & (SDMA_CMD_AR | SDMA_CMD_AT))
> > 			&& delay--)
> > 		udelay(10);
> > 	if(delay)
> > 		break;
> 
>  	delay = 10;
>  	while (--delay) {
> 		tmp = readl(&regs->sdma_cmd);
> 		if (!(tmp & (SDMA_CMD_AR | SDMA_CMD_AT))
> 			break;
>  		udelay(10);
> 	}
>  	if (delay)
>  		break;
> 
> It makes the code horizontally shorter. What do you think? Btw there's
> a rule in 
> U-Boot that multi-line statements must have braces.
> 

Thanks Marek, Yes it looks much better :) see the plus point on working with experts rather
running away? :D

Regards,
Ajay Bhargav

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
       [not found] <583672539.2717.1314270415380.JavaMail.root@ahm.einfochips.com>
@ 2011-08-25 11:07 ` Ajay Bhargav
  2011-08-25 12:19   ` Marek Vasut
  0 siblings, 1 reply; 14+ messages in thread
From: Ajay Bhargav @ 2011-08-25 11:07 UTC (permalink / raw)
  To: u-boot


----- "Marek Vasut" <marek.vasut@gmail.com> wrote:

[...]
> > +static void abortdma(struct eth_device *dev)
> > +{
> > +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> > +	struct armdfec_reg *regs = darmdfec->regs;
> > +	int delay;
> > +	int maxretries = 40;
> > +
> > +	do {
> > +		writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
> > +		udelay(100);
> > +
> > +		delay = 10;
> > +		while ((readl(&regs->sdma_cmd) &
> > +			(SDMA_CMD_AR | SDMA_CMD_AT))
> > +			&& delay-- > 0) {
> > +			udelay(10);
> > +		}
> > +	} while (maxretries-- > 0 && delay <= 0);
> 
> Didn't I comment on this one in V1?
> 

I modified it as follows... Is it more readable now? :)

while (maxretries--) {
	writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
	udelay(100);

	delay = 10;
	while ((readl(&regs->sdma_cmd) & (SDMA_CMD_AR | SDMA_CMD_AT))
			&& delay--)
		udelay(10);
	if(delay)
		break;
}

Regards,
Ajay Bhargav

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

* [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100
       [not found] <569776325.496.1314249408394.JavaMail.root@ahm.einfochips.com>
@ 2011-08-25  5:21 ` Ajay Bhargav
  2011-08-25 12:15   ` Marek Vasut
  0 siblings, 1 reply; 14+ messages in thread
From: Ajay Bhargav @ 2011-08-25  5:21 UTC (permalink / raw)
  To: u-boot


----- "Marek Vasut" <marek.vasut@gmail.com> wrote:

> On Wednesday, August 24, 2011 03:07:18 PM Ajay Bhargav wrote:
> > This patch adds support for Fast Ethernet Controller driver for
> > Armada100 series.
> > 
> > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > ---
> 
> [...]
> 
[...]
> > +
> > +static int armdfec_phy_timeout(u32 reg, u32 flag, int cond)
> > +{
> > +	u32 timeout = PHY_WAIT_ITERATIONS;
> > +	while (--timeout) {
> > +		if (cond && (readl(reg) & flag))
> > +			break;
> > +		else if (!cond && !(readl(reg) & flag))
> 
> You can read the register into some temporary variable so you don't
> need the 
> readl() at two places.
> 

readl will be called only once... do i really need a temp var?

> > +			break;
> > +		udelay(PHY_WAIT_MICRO_SECONDS);
> > +	}
> > +	return !timeout;
> > +}
> > +
> > +static int smi_reg_read(const char *devname, u8 phy_addr, u8
> phy_reg,
> > +			u16 *value)
> > +{
> > +	struct eth_device *dev = eth_get_dev_by_name(devname);
> > +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> > +	struct armdfec_reg *regs = darmdfec->regs;
> > +	u32 val, reg_data;
> > +
> > +	if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {
> > +		reg_data = readl(&regs->phyadr);
> > +		*value = (u16) (reg_data & 0x1f);
> > +		return 0;
> > +	}
> > +
> > +	/* check parameters */
> > +	if (phy_addr > PHY_MASK) {
> > +		printf("Err..(%s) Invalid phy address: 0x%X\n",
> > +				__func__, phy_addr);
> > +		return -EINVAL;
> > +	}
> > +	if (phy_reg > PHY_MASK) {
> > +		printf("Err..(%s) Invalid register offset: 0x%X\n",
> > +				__func__, phy_reg);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* wait for the SMI register to become available */
> > +	if (armdfec_phy_timeout((u32)&regs->smi, SMI_BUSY, FALSE)) {
> 
> Adjust the function so you don't need the cast.
> 
okay...

> > +		printf("Error (%s) PHY busy timeout\n",	__func__);
> > +		return -1;
> > +	}
> > +
> > +	writel(phy_addr << 16 | phy_reg << 21 | SMI_OP_R, &regs->smi);
> 
> Parentheses missing maybe ?
> 

:) okay..

> > +
> > +	/* now wait for the data to be valid */
> > +	if (armdfec_phy_timeout((u32)&regs->smi, SMI_R_VALID, TRUE)) {
> > +		val = readl(&regs->smi);
> > +		printf("Err (%s) PHY Read timeout, val=0x%x\n", __func__, val);
> > +		return -1;
> > +	}
> > +	val = readl(&regs->smi);
> > +	*value = val & 0xffff;
> > +
> > +	return 0;
> > +}
> > +
> > +static int smi_reg_write(const char *devname,
> > +	 u8 phy_addr, u8 phy_reg, u16 value)
> > +{
> > +	struct eth_device *dev = eth_get_dev_by_name(devname);
> > +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> > +	struct armdfec_reg *regs = darmdfec->regs;
> > +
> > +	if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {
> > +		clrsetbits_le32(&regs->phyadr, 0x1f, value & 0x1f);
> > +		return 0;
> > +	}
> > +
> > +	/* check parameters */
> > +	if (phy_addr > PHY_MASK) {
> > +		printf("Err..(%s) Invalid phy address\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +	if (phy_reg > PHY_MASK) {
> > +		printf("Err..(%s) Invalid register offset\n", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* wait for the SMI register to become available */
> > +	if (armdfec_phy_timeout((u32)&regs->smi, SMI_BUSY, FALSE)) {
> > +		printf("Error (%s) PHY busy timeout\n",	__func__);
> > +		return -1;
> > +	}
> > +
> > +	writel(phy_addr << 16 | phy_reg << 21 | SMI_OP_W | (value &
> 0xffff),
> > +			&regs->smi);
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Abort any transmit and receive operations and put DMA
> > + * in idle state. AT and AR bits are cleared upon entering
> > + * in IDLE state. So poll those bits to verify operation.
> > + */
> > +static void abortdma(struct eth_device *dev)
> > +{
> > +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> > +	struct armdfec_reg *regs = darmdfec->regs;
> > +	int delay;
> > +	int maxretries = 40;
> > +
> > +	do {
> > +		writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
> > +		udelay(100);
> > +
> > +		delay = 10;
> > +		while ((readl(&regs->sdma_cmd) &
> > +			(SDMA_CMD_AR | SDMA_CMD_AT))
> > +			&& delay-- > 0) {
> > +			udelay(10);
> > +		}
> > +	} while (maxretries-- > 0 && delay <= 0);
> 
> Didn't I comment on this one in V1?
> 

Yes you did... 

> > +
> > +	if (maxretries <= 0)
> > +		printf("%s : DMA Stuck\n", __func__);
> > +}
> 
> [...]
> 
> > +int armada100_fec_register()
> > +{
> > +	struct armdfec_device *darmdfec;
> > +	struct eth_device *dev;
> > +	int phy_adr;
> > +
> > +	darmdfec = malloc(sizeof(struct armdfec_device));
> > +	if (!darmdfec)
> > +		goto error;
> > +
> > +	memset(darmdfec, 0, sizeof(struct armdfec_device));
> > +
> > +	darmdfec->htpr = memalign(8, HASH_ADDR_TABLE_SIZE);
> > +	if (!darmdfec->htpr)
> > +		goto error;
> > +
> > +	darmdfec->p_rxdesc = (struct rx_desc *) memalign(PKTALIGN,
> > +			ARMDFEC_RXQ_DESC_ALIGNED_SIZE * RINGSZ + 1);
> > +
> > +	if (!darmdfec->p_rxdesc)
> > +		goto error;
> > +
> > +	darmdfec->p_rxbuf = (u8 *) memalign(PKTALIGN,
> > +			RINGSZ * PKTSIZE_ALIGN + 1);
> 
> Is the cast needed ? You should review your casts, most of them can be
> solved by 
> using proper type!
> 
Thats a serious issue with me I guess :)

> > +	if (!darmdfec->p_rxbuf)
> > +		goto error;
> > +
> > +	darmdfec->p_aligned_txbuf = memalign(8, PKTSIZE_ALIGN);
> > +	if (!darmdfec->p_aligned_txbuf)
> > +		goto error;
> > +
> > +	darmdfec->p_txdesc = (struct tx_desc *)
> > +		memalign(PKTALIGN, sizeof(struct tx_desc) + 1);
> > +	if (!darmdfec->p_txdesc)
> > +		goto error;
> > +
> > +	dev = &darmdfec->dev;
> > +	/* Assign ARMADA100 Fast Ethernet Controller Base Address */
> > +	darmdfec->regs = (void *) ARMD1_FEC_BASE;
> 
> Cheers!
> 
Thanks,
Ajay Bhargav

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

end of thread, other threads:[~2011-08-26  5:33 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-24 13:07 [U-Boot] [PATCH v2 1/3] net: Adds Fast Ethernet Controller driver for Armada100 Ajay Bhargav
2011-08-24 15:42 ` Mike Frysinger
2011-08-24 16:38   ` Marek Vasut
2011-08-25  5:24     ` Ajay Bhargav
2011-08-25 12:14       ` Marek Vasut
2011-08-25  5:10   ` Ajay Bhargav
2011-08-25 14:13     ` Mike Frysinger
2011-08-24 16:36 ` Marek Vasut
     [not found] <569776325.496.1314249408394.JavaMail.root@ahm.einfochips.com>
2011-08-25  5:21 ` Ajay Bhargav
2011-08-25 12:15   ` Marek Vasut
     [not found] <583672539.2717.1314270415380.JavaMail.root@ahm.einfochips.com>
2011-08-25 11:07 ` Ajay Bhargav
2011-08-25 12:19   ` Marek Vasut
2011-08-25 12:12     ` Ajay Bhargav
     [not found] <1826454834.6634.1314336622428.JavaMail.root@ahm.einfochips.com>
2011-08-26  5:33 ` Ajay Bhargav

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.