Skip to contentSkip to conversation

net: Adds Fast Ethernet Controller driver for Armada100

PATCH Superseded Ajay Bhargav 6 participants 21 messages Aug 22, 2011 – Aug 24, 2011

Subsystems (2)
Network drivers Arm (32-bit)

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Ajay Bhargav

author
#0 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

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

Signed-off-by Ajay Bhargav <[email protected]>
---
 arch/arm/include/asm/arch-armada100/armada100.h |    1 +
 drivers/net/Makefile                            |    1 +
 drivers/net/armada100_fec.c                     |  802 +++++++++++++++++++++++
 drivers/net/armada100_fec.h                     |  225 +++++++
 include/netdev.h                                |    1 +
 5 files changed, 1030 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/armada100_fec.c
 create mode 100644 drivers/net/armada100_fec.h
arch/arm/include/asm/arch-armada100/armada100.h +1 -0
index d5d125a..3d567eb 100644
@@ -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
drivers/net/Makefile +1 -0
index 819b197..34b4322 100644
@@ -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
drivers/net/armada100_fec.c new file +802 -0

802 lines not on this page · raw message

drivers/net/armada100_fec.h new file +225 -0

225 lines not on this page · raw message

include/netdev.h +1 -0
index 6f0a971..e7b9298 100644
@@ -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_initialize(void);
 
 /* Boards with PCI network controllers can call this from their board_eth_init()
  * function to initialize whatever's on board.
Signature · 1 line
1.7.0.4
#1 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 99 lines · Ajay Bhargav

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

Signed-off-by Ajay Bhargav <[email protected]>
---
 arch/arm/include/asm/arch-armada100/armada100.h |    1 +
 drivers/net/Makefile                            |    1 +
 drivers/net/armada100_fec.c                     |  802
+++++++++++++++++++++++ drivers/net/armada100_fec.h                     | 
225 +++++++
 include/netdev.h                                |    1 +
 5 files changed, 1030 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/armada100_fec.c
 create mode 100644 drivers/net/armada100_fec.h
(quoted diff) +0 -0

b/arch/arm/include/asm/arch-armada100/armada100.h index d5d125a..3d567eb 100644

arch/arm/include/asm/arch-armada100/armada100.h +1 -0
@@ -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
drivers/net/Makefile +1 -0
index 819b197..34b4322 100644
@@ -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
drivers/net/armada100_fec.c new file +53 -0
new file mode 100644
index 0000000..67fd73d
@@ -0,0 +1,802 @@
+/*
+ * (C) Copyright 2011
+ * eInfochips Ltd. <www.einfochips.com>
+ * Written-by: Ajay Bhargav <[email protected]>
+ *
+ * (C) Copyright 2010
+ * Marvell Semiconductor <www.marvell.com>
+ * Contributor: Mahavir Jain <[email protected]>
+ *
+ * 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 ETH_DUMP_REGS
+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)

drivers/net/armada100_fec.c line 63

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+		printf("offset: 0x%x, value: 0x%x\n",
+			i, readl(ARMD1_FEC_BASE + i));
+	return 0;
+}
+#endif
+
+static u8 get_random_byte(u8 seed)
+{
+	udelay(seed * 1000);
+	return (u8)(read_timer());

Parens not needed, anyway, don't we have any better "random()" here ?

Quoted 11 lines
drivers/net/armada100_fec.c new file +11 -0
+}
+
+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;
+	int i = 0;
+

drivers/net/armada100_fec.c line 84

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+	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);

Something more descriptive won't be a bad thing here and all around the place ... also if you want, you can use __LINE__ and __FILE__ though I think that's overkill. Even __func__ is.

drivers/net/armada100_fec.c line 94

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+		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 */
+	for (i = 0; (val = readl(&regs->smi)) & SMI_BUSY; i++) {

Parens not needed.

drivers/net/armada100_fec.c line 102

Quoted 8 lines
drivers/net/armada100_fec.c new file +8 -0
+
+		if (i == PHY_WAIT_ITERATIONS) {
+			printf("Error (%s) PHY busy timeout\n",
+				__func__);
+			return -1;
+		}
+		udelay(PHY_WAIT_MICRO_SECONDS);
+	}

Ok, maybe a timeout() function won't hurt with something like ...

int a100_timeout(uint32_t reg, uint32_t mask, uint32_t timeout)

{
	while (--timeout)
		if (readl(reg) & mask)
			break;

	return !timeout;
}

then call

if (a100_timeout(&regs->smi, SMI_R_VALID, timeout)) {
	printf("FEC died a horrible death in pain!\n");
	return -EDEATH;
}
Quoted 5 lines
drivers/net/armada100_fec.c new file +5 -0
+
+	writel(((phy_addr << 16) | (phy_reg << 21) | SMI_OP_R), &regs->smi);
+
+	/* now wait for the data to be valid */
+	for (i = 0; !((val = readl(&regs->smi)) & SMI_R_VALID); i++) {

drivers/net/armada100_fec.c line 117

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+		if (i == PHY_WAIT_ITERATIONS) {
+			printf("Error (%s) PHY Read timeout, val=0x%x\n",
+				__func__, val);
+			return -1;
+		}
+		udelay(PHY_WAIT_MICRO_SECONDS);
+	}
+	*value = val & 0xffff;
+	debug("%s:(adr 0x%x, off 0x%x) value= %04x\n", __func__,
+		phy_addr, phy_reg, *value);

Maybe if you want such a verbose debug, you should define variadic macro to avoid writing this ...

Quoted 5 lines
drivers/net/armada100_fec.c new file +5 -0
+
+	return 0;
+}
+
+static int smi_reg_write(const char *devname,

drivers/net/armada100_fec.c line 132

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+	 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 reg_data;
+	int i;
+
+	if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {
+		clrsetbits_le32(&regs->phyadr, 0x1f, (value & 0x1f));

This is good, but too many parens, please fix globally :)

Quoted 13 lines
drivers/net/armada100_fec.c new file +13 -0
+		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;
+	}

drivers/net/armada100_fec.c line 155

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+
+	/* wait for the SMI register to become available */
+	for (i = 0; readl(&regs->smi) & SMI_BUSY; i++) {
+		if (i == PHY_WAIT_ITERATIONS) {
+			printf("Error (%s) PHY busy timeout\n",
+			       __func__);
+			return -1;
+		}
+		udelay(PHY_WAIT_MICRO_SECONDS);
+	}

a100_timeout(), see above

drivers/net/armada100_fec.c line 158

Quoted 3 lines
drivers/net/armada100_fec.c new file +3 -0
+
+	writel(((phy_addr << 16) | (phy_reg << 21) | SMI_OP_W |
+		(value & 0xffff)), &regs->smi);

Wheee, parenthesis, parenthesis and even more parenthesis :-)

To make it clear, the readl() writel() macros are safe so you don't need to put them around the arguments ;-)

drivers/net/armada100_fec.c line 160

Quoted 2 lines
drivers/net/armada100_fec.c new file +2 -0
+	debug("%s:(adr 0x%x, off 0x%x) value= %04x\n", __func__,
+		phy_addr, phy_reg, value);

debug, see above.

Quoted 16 lines
drivers/net/armada100_fec.c new file +16 -0
+	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 {

drivers/net/armada100_fec.c line 186

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+		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);

I think you can write this in a bit more readable way ;-)

drivers/net/armada100_fec.c line 194

Quoted 8 lines
drivers/net/armada100_fec.c new file +8 -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);

Wow, the card is insane ?

Quoted 46 lines
drivers/net/armada100_fec.c new file +46 -0
+}
+
+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)

drivers/net/armada100_fec.c line 250

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+		+ ((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;

Put parens about the second argument of OR

Quoted 124 lines
drivers/net/armada100_fec.c new file +124 -0
+	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 at 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];

drivers/net/armada100_fec.c line 384

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+	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);

Do you need the cast ?

Quoted 38 lines
drivers/net/armada100_fec.c new file +38 -0
+}
+
+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.

drivers/net/armada100_fec.c line 432

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+ */
+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)

Just use a temporary variable here to avoid breaking the line.

Quoted 143 lines
drivers/net/armada100_fec.c new file +143 -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)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+	u32 val;
+
+	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 ETH_DUMP_REGS
+	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 int armdfec_halt(struct eth_device *dev)
+{
+	struct armdfec_device *darmdfec = to_darmdfec(dev);
+	struct armdfec_reg *regs = darmdfec->regs;
+	u32 val;
+	/* 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);
+

drivers/net/armada100_fec.c line 585

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+	return 0;
+}
+
+static int armdfec_send(struct eth_device *dev, 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;

Cast not needed ?

drivers/net/armada100_fec.c line 586

Quoted 1 line
drivers/net/armada100_fec.c new file +1 -0
+	int retry = 10000;

#define this retry count and reuse throughout this file. DTTO with timeout

Quoted 54 lines
drivers/net/armada100_fec.c new file +54 -0
+	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;
+

drivers/net/armada100_fec.c line 650

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -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);

Timeout function needed.

Quoted 37 lines
drivers/net/armada100_fec.c new file +37 -0
+
+	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));

drivers/net/armada100_fec.c line 697

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+	}
+	/*
+	 * 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((unsigned int) p_rxdesc_curr->nxtdesc_p,
+			&(darmdfec->p_rxdesc_curr));

If you cast pointers for writel() call, do it at least consistenly ;-)

drivers/net/armada100_fec.c line 707

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+
+	return 0;
+}
+
+int armada100_fec_initialize()
+{
+	struct armdfec_device *darmdfec;
+	struct eth_device *dev;
+	int phy_adr;
+	char *s = "ethaddr";

constify this.

Quoted 45 lines
drivers/net/armada100_fec.c new file +45 -0
+
+	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");
+
+	while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
+		/* Generate Private MAC addr if not set */
+		dev->enetaddr[0] = 0x00;
+		dev->enetaddr[1] = 0x50;
+		dev->enetaddr[2] = 0x43;
+#if defined(CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION)

drivers/net/armada100_fec.c line 762

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+		/* Generate fixed lower MAC half */
+		dev->enetaddr[3] = 0x11;
+		dev->enetaddr[4] = 0x22;
+		dev->enetaddr[5] = 0x33;
+#else
+		/* Generate random lower MAC half */
+		dev->enetaddr[3] = get_random_byte((u8)read_timer());
+		dev->enetaddr[4] = get_random_byte(dev->enetaddr[3]);
+		dev->enetaddr[5] = get_random_byte(dev->enetaddr[4]);
+#endif

Won't uboot handle this ... or aren't there some mechanisms to generate random MAC that can be reused?

drivers/net/armada100_fec.c line 769

Quoted 7 lines
drivers/net/armada100_fec.c new file +7 -0
+		eth_setenv_enetaddr(s, dev->enetaddr);
+	}
+
+	dev->init = (void *) armdfec_init;
+	dev->halt = (void *) armdfec_halt;
+	dev->send = (void *) armdfec_send;
+	dev->recv = (void *) armdfec_recv;

Why those casts ... besides they are wrong.

drivers/net/armada100_fec.c line 771

Quoted 2 lines
drivers/net/armada100_fec.c new file +2 -0
+
+	eth_register(dev);

Newline won't hurt here.

Quoted 230 lines
drivers/net/armada100_fec.c new file +31 -0
+#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 at 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;
+}
drivers/net/armada100_fec.h new file +59 -0
new file mode 100644
index 0000000..dcac964
@@ -0,0 +1,225 @@
+/*
+ * (C) Copyright 2011
+ * eInfochips Ltd. <www.einfochips.com>
+ * Written-by: Ajay Bhargav <[email protected]>
+ *
+ * (C) Copyright 2010
+ * Marvell Semiconductor <www.marvell.com>
+ * Contributor: Mahavir Jain <[email protected]>
+ *
+ * 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__
+
+#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

drivers/net/armada100_fec.h new file +2 -0
+
+#define PHY_WAIT_ITERATIONS     1000	/* 1000 iterations * 10uS = 10mS max

*/ +#define PHY_WAIT_MICRO_SECONDS 10

drivers/net/armada100_fec.h new file +130 -0
+
+#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;

drivers/net/armada100_fec.h line 203

Quoted 10 lines
drivers/net/armada100_fec.h new file +10 -0
+	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];

Use reserved maybe ?

Quoted 33 lines
drivers/net/armada100_fec.h new file +22 -0
+	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__ */
include/netdev.h +1 -0
index 6f0a971..e7b9298 100644
@@ -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_initialize(void);
 
 /* Boards with PCI network controllers can call this from their
Quoted 1 line

board_eth_init() * function to initialize whatever's on board.

Cheers!

author
#2 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 11 lines · ----- "Marek Vasut
Quoted 9 lines · Ajay Bhargav

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

Signed-off-by Ajay Bhargav <[email protected]>
---
 arch/arm/include/asm/arch-armada100/armada100.h |    1 +
 drivers/net/Makefile                            |    1 +
 drivers/net/armada100_fec.c                     |  802

+++++++++++++++++++++++ drivers/net/armada100_fec.h

|

Quoted 9 lines
Quoted 6 lines

225 +++++++

 include/netdev.h                                |    1 +
 5 files changed, 1030 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/armada100_fec.c
 create mode 100644 drivers/net/armada100_fec.h

Cheers!

Hi Marek,

Thank you so much for the detailed comments :) really helpful.

Regards,
Ajay Bhargav

author
#3 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

In reply to Marek Vasut
Quoted 17 lines · ----- "Marek Vasut
Quoted 9 lines · Ajay Bhargav

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

Signed-off-by Ajay Bhargav <[email protected]>
---
 arch/arm/include/asm/arch-armada100/armada100.h |    1 +
 drivers/net/Makefile                            |    1 +
 drivers/net/armada100_fec.c                     |  802

+++++++++++++++++++++++ drivers/net/armada100_fec.h

|

Quoted 6 lines

225 +++++++

 include/netdev.h                                |    1 +
 5 files changed, 1030 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/armada100_fec.c
 create mode 100644 drivers/net/armada100_fec.h

[...]

Quoted 7 lines

drivers/net/armada100_fec.c line 63

Quoted 4 lines
drivers/net/armada100_fec.c new file +4 -0
+static u8 get_random_byte(u8 seed)
+{
+	udelay(seed * 1000);
+	return (u8)(read_timer());

Parens not needed, anyway, don't we have any better "random()" here ?

I could not find random() function anywhere in the u-boot source.

[...]

Quoted 10 lines

drivers/net/armada100_fec.c line 762

Quoted 5 lines
drivers/net/armada100_fec.c new file +5 -0
+		/* Generate random lower MAC half */
+		dev->enetaddr[3] = get_random_byte((u8)read_timer());
+		dev->enetaddr[4] = get_random_byte(dev->enetaddr[3]);
+		dev->enetaddr[5] = get_random_byte(dev->enetaddr[4]);
+#endif

Won't uboot handle this ... or aren't there some mechanisms to generate random
MAC that can be reused?

There is a program given in tools directory for generating ethernet mac "gen_eth_addr.c" but nothing as such in u-boot source as far as i checked.

Please let me know if I missed something to check.

Thanks & Regards,
Ajay Bhargav

#4 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 44 lines · Ajay Bhargav
Quoted 19 lines · ----- "Marek Vasut
Quoted 18 lines · Ajay Bhargav

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

Signed-off-by Ajay Bhargav <[email protected]>

---

 arch/arm/include/asm/arch-armada100/armada100.h |    1 +
 drivers/net/Makefile                            |    1 +
 drivers/net/armada100_fec.c                     |  802

+++++++++++++++++++++++ drivers/net/armada100_fec.h

225 +++++++

 include/netdev.h                                |    1 +
 5 files changed, 1030 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/armada100_fec.c
 create mode 100644 drivers/net/armada100_fec.h

[...]

Quoted 6 lines

drivers/net/armada100_fec.c line 63

Quoted 4 lines
drivers/net/armada100_fec.c new file +4 -0
+static u8 get_random_byte(u8 seed)
+{
+	udelay(seed * 1000);
+	return (u8)(read_timer());

Parens not needed, anyway, don't we have any better "random()" here ?

I could not find random() function anywhere in the u-boot source.

[...]

Quoted 9 lines

drivers/net/armada100_fec.c line 762

Quoted 5 lines
drivers/net/armada100_fec.c new file +5 -0
+		/* Generate random lower MAC half */
+		dev->enetaddr[3] = get_random_byte((u8)read_timer());
+		dev->enetaddr[4] = get_random_byte(dev->enetaddr[3]);
+		dev->enetaddr[5] = get_random_byte(dev->enetaddr[4]);
+#endif

Won't uboot handle this ... or aren't there some mechanisms to generate random
MAC that can be reused?

Quoted 4 lines

There is a program given in tools directory for generating ethernet mac "gen_eth_addr.c" but nothing as such in u-boot source as far as i checked.

Please let me know if I missed something to check.

Hi,

I think someone NAKed this whole part with MAC randomisation and he was right about it ... you're supposed to configure the MAC properly yourself. Cheers

Quoted 3 lines

Thanks & Regards,
Ajay Bhargav

author
#5 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 51 lines · ----- "Marek Vasut
Quoted 21 lines · Ajay Bhargav
Quoted 11 lines · ----- "Marek Vasut
Quoted 10 lines · Ajay Bhargav

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

Signed-off-by Ajay Bhargav <[email protected]>

---

 arch/arm/include/asm/arch-armada100/armada100.h |    1 +
 drivers/net/Makefile                            |    1 +
 drivers/net/armada100_fec.c                     |  802
Quoted 8 lines
Quoted 8 lines

+++++++++++++++++++++++ drivers/net/armada100_fec.h

225 +++++++

 include/netdev.h                                |    1 +
 5 files changed, 1030 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/armada100_fec.c
 create mode 100644 drivers/net/armada100_fec.h
Quoted 8 lines

[...]

Quoted 6 lines

drivers/net/armada100_fec.c line 63

Quoted 4 lines
drivers/net/armada100_fec.c new file +4 -0
+static u8 get_random_byte(u8 seed)
+{
+	udelay(seed * 1000);
+	return (u8)(read_timer());

Parens not needed, anyway, don't we have any better "random()"

here ?

Quoted 15 lines

I could not find random() function anywhere in the u-boot source.

[...]

Quoted 9 lines

drivers/net/armada100_fec.c line 762

Quoted 5 lines
drivers/net/armada100_fec.c new file +5 -0
+		/* Generate random lower MAC half */
+		dev->enetaddr[3] = get_random_byte((u8)read_timer());
+		dev->enetaddr[4] = get_random_byte(dev->enetaddr[3]);
+		dev->enetaddr[5] = get_random_byte(dev->enetaddr[4]);
+#endif

Won't uboot handle this ... or aren't there some mechanisms to generate random
MAC that can be reused?

Quoted 1 line

There is a program given in tools directory for generating ethernet

mac

Quoted 1 line

"gen_eth_addr.c" but nothing as such in u-boot source as far as i

checked.

Quoted 1 line
Quoted 12 lines
Quoted 1 line

Please let me know if I missed something to check.

Hi,

I think someone NAKed this whole part with MAC randomisation and he was right
about it ... you're supposed to configure the MAC properly yourself. Cheers

Quoted 3 lines

Thanks & Regards,
Ajay Bhargav

MAC randomization is done in U-Boot by many already. I am confused... just like i was when working with GPIO :) Is it something which has changed lately?

Regards,
Ajay Bhargav

#6 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Dear Ajay Bhargav,

Quoted 3 lines · In message <[email protected]>…
Quoted 3 lines

I think someone NAKed this whole part with MAC randomisation and he was right about it ... you're supposed to configure the MAC properly yourself.

...

Quoted 1 line

MAC randomization is done in U-Boot by many already. I am

U-Boot is big and contains lots of poor code. And many people have a strange skill of always picking poor examples only when copying code. So it always makes sense to double-check.

Quoted 2 lines

confused... just like i was when working with GPIO :) Is it something which has changed lately?

No, this has not changed lately.

Just re-read Mike's message:
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/106117

Best regards,

Wolfgang Denk

Signature · 5 lines
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
It seems intuitively obvious to me, which  means  that  it  might  be
wrong.                                                 -- Chris Torek
author
#7 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 8 lines · ----- "Wolfgang Denk

Dear Ajay Bhargav,

In message <[email protected]> you

Quoted 2 lines · wrote:
Quoted 2 lines

I think someone NAKed this whole part with MAC randomisation and

he was right

Quoted 9 lines
Quoted 1 line
Quoted 1 line

about it ... you're supposed to configure the MAC properly

yourself.
...

Quoted 1 line

MAC randomization is done in U-Boot by many already. I am

U-Boot is big and contains lots of poor code. And many people have a strange skill of always picking poor examples only when copying code. So it always makes sense to double-check.

I agree :)

Regards,
Ajay Bhargav

author
#8 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

In reply to Wolfgang Denk
Quoted 23 lines · ----- "Wolfgang Denk

Dear Ajay Bhargav,

In message <[email protected]> you

Quoted 2 lines · wrote:
Quoted 2 lines

I think someone NAKed this whole part with MAC randomisation and

he was right

Quoted 1 line
Quoted 1 line

about it ... you're supposed to configure the MAC properly

yourself.
...

Quoted 1 line

MAC randomization is done in U-Boot by many already. I am

U-Boot is big and contains lots of poor code. And many people have a strange skill of always picking poor examples only when copying code. So it always makes sense to double-check.

Quoted 1 line

confused... just like i was when working with GPIO :) Is it

something

Quoted 1 line

which has changed lately?

No, this has not changed lately.

Quoted 16 lines

Just re-read Mike's message:
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/106117

Best regards,

Wolfgang Denk

Signature · 5 lines
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
It seems intuitively obvious to me, which  means  that  it  might  be
wrong.                                                 -- Chris Torek

Dear Wolfgang,

ok I just wanted to clear myself.. I do not have a dedicated hardware storage in my ethernet controller so I will just look into environment variable ethaddr, if its set I will just copy it to driver layer and if it is not set, I let the user set it. He/She may use the tools provided with U-Boot to generate a random MAC or by any mean get a MAC and store it in env which is gonna be one time unless env gets corrupt or cleared by user. Which means no MAC generation in code...

Please correct me if my understanding wrong...

Thanks & Regards,
Ajay Bhargav

#9 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Dear Ajay Bhargav,

Quoted 10 lines · In message <[email protected]>…

ok I just wanted to clear myself.. I do not have a dedicated hardware storage in my ethernet controller so I will just look into environment variable ethaddr, if its set I will just copy it to driver layer and if it is not set, I let the user set it. He/She may use the tools provided with U-Boot to generate a random MAC or by any mean get a MAC and store it in env which is gonna be one time unless env gets corrupt or cleared by user. Which means no MAC generation in code...

Please correct me if my understanding wrong...

This is correct, except for the nitpick that nobody should ever use a truly random MAC address - if no valid MAC address is available, one should at least make sure to pick one of the locally administered Ethernet address pool (2nd LSB in the most significant byte of the address must be set).

Best regards,

Wolfgang Denk

Signature · 4 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"If anything can go wrong, it will."                   - Edsel Murphy
#10 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 8 lines · Wolfgang Denk
Quoted 8 lines

ok I just wanted to clear myself.. I do not have a dedicated hardware storage in my ethernet controller so I will just look into environment variable ethaddr, if its set I will just copy it to driver layer and if it is not set, I let the user set it. He/She may use the tools provided with U-Boot to generate a random MAC or by any mean get a MAC and store it in env which is gonna be one time unless env gets corrupt or cleared by user. Which means no MAC generation in code...

Quoted 7 lines
Quoted 1 line

Please correct me if my understanding wrong...

This is correct, except for the nitpick that nobody should ever use a truly random MAC address - if no valid MAC address is available, one should at least make sure to pick one of the locally administered Ethernet address pool (2nd LSB in the most significant byte of the address must be set).

It is even more annoying that the Linux network driver for Armada 100 (pxa168_eth.c) does the same thing (random MAC) which made it impossible to properly assign IPs or even have the same in u-boot and Linux.

Since the pxa168 ethernet seems to (correct me if I am wrong) store all MACs, including its own, in a buffer allocated in main memory, it is not possible to set it in u-boot and then read it out in Linux. So, I introduced "pxa168_mac" as Linux command line argument to pass ethaddr to pxa168_eth. Or does anybody have a better solution?

Regards,
Ralph Metzler

#11 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 22 lines · On Tuesday, August 23, 2011 11:30:38 Ralph Metzler wrote:
Quoted 8 lines · Wolfgang Denk
Quoted 8 lines

ok I just wanted to clear myself.. I do not have a dedicated hardware storage in my ethernet controller so I will just look into environment variable ethaddr, if its set I will just copy it to driver layer and if it is not set, I let the user set it. He/She may use the tools provided with U-Boot to generate a random MAC or by any mean get a MAC and store it in env which is gonna be one time unless env gets corrupt or cleared by user. Which means no MAC generation in code...

Quoted 7 lines
Quoted 1 line

Please correct me if my understanding wrong...

This is correct, except for the nitpick that nobody should ever use a truly random MAC address - if no valid MAC address is available, one should at least make sure to pick one of the locally administered Ethernet address pool (2nd LSB in the most significant byte of the address must be set).

It is even more annoying that the Linux network driver for Armada 100 (pxa168_eth.c) does the same thing (random MAC) which made it impossible to properly assign IPs or even have the same in u-boot and Linux.

Quoted 5 lines

Since the pxa168 ethernet seems to (correct me if I am wrong) store all MACs, including its own, in a buffer allocated in main memory, it is not possible to set it in u-boot and then read it out in Linux. So, I introduced "pxa168_mac" as Linux command line argument to pass ethaddr to pxa168_eth. Or does anybody have a better solution?

the latest u-boot tree should call write_hwaddr when there is a valid mac available (i.e. "ethaddr" is in the env). then linux should be able to say "hey, the mac is already set to a valid one" and leave it be.

if linux always blows it away, you can certainly use the tried & true method:
read the u-boot env from userspace (fw_printenv) and then manually set it yourself with ifconfig hw class <mac>.
-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/20110823/ee717637/attachment.pgp

#12 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

In reply to Ralph Metzler

Dear Ralph Metzler,

Quoted 3 lines · In message <[email protected]> you wrote:

So, I introduced "pxa168_mac" as Linux command line argument to pass ethaddr to pxa168_eth. Or does anybody have a better solution?

Add device tree support for your board, and pass the MAC address in the device tree.

Best regards,

Wolfgang Denk

Signature · 5 lines
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 typical page layout program is nothing more  than  an  electronic
light table for cutting and pasting documents.
author
#13 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

In reply to Wolfgang Denk
Quoted 28 lines · ----- "Wolfgang Denk

Dear Ajay Bhargav,

In message <[email protected]> you

Quoted 2 lines · wrote:

ok I just wanted to clear myself.. I do not have a dedicated

hardware

Quoted 1 line

storage in my ethernet controller so I will just look into

environment

Quoted 1 line

variable ethaddr, if its set I will just copy it to driver layer

and

Quoted 1 line

if it is not set, I let the user set it. He/She may use the tools

provided

Quoted 1 line

with U-Boot to generate a random MAC or by any mean get a MAC and

store it

Quoted 1 line

in env which is gonna be one time unless env gets corrupt or cleared

by user.

Quoted 3 lines

Which means no MAC generation in code...

Please correct me if my understanding wrong...

This is correct, except for the nitpick that nobody should ever use a truly random MAC address - if no valid MAC address is available, one should at least make sure to pick one of the locally administered Ethernet address pool (2nd LSB in the most significant byte of the address must be set).

Quoted 4 lines

Best regards,

Wolfgang Denk

Dear Wolfgang,

Thank you so much for clarification. I tested u-boot on board with no random MAC and its working perfectly once user set it. Thanks again...

Regards,
Ajay Bhargav

#14 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

In reply to Ajay Bhargav

drivers/net/armada100_fec.c line 384

Quoted 1 line · On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
drivers/net/armada100_fec.c new file +1 -0
+	writel((u32) darmdfec->htpr, &regs->htpr);

do you really need to cast it yourself ? seems to show up a lot in this file.

drivers/net/armada100_fec.c line 533

Quoted 3 lines
drivers/net/armada100_fec.c new file +3 -0
+#ifdef ETH_DUMP_REGS
+	eth_dump_regs(dev);
+#endif

use #ifdef DEBUG

drivers/net/armada100_fec.c line 629

Quoted 3 lines
drivers/net/armada100_fec.c new file +2 -0
+	while (cmd_sts & BUF_OWNED_BY_DMA) {
...
+	};

no semi-colon needed

drivers/net/armada100_fec.c line 742

Quoted 4 lines
drivers/net/armada100_fec.c new file +3 -0
+int armada100_fec_initialize()
+{
...
+	darmdfec->regs = (void *) ARMD1_FEC_BASE;

make the reg base a parameter to armada100_fec_initialize()

drivers/net/armada100_fec.c line 709

Quoted 1 line
drivers/net/armada100_fec.c new file +1 -0
+	darmdfec = malloc(sizeof(struct armdfec_device));

sizeof(*darmdfec)

Quoted 8 lines
drivers/net/armada100_fec.c new file +8 -0
+	while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
+		/* Generate Private MAC addr if not set */
+		dev->enetaddr[0] = 0x00;
+		dev->enetaddr[1] = 0x50;
+		dev->enetaddr[2] = 0x43;
+#if defined(CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION)
+		/* Generate fixed lower MAC half */
+		dev->enetaddr[3] = 0x11;

drivers/net/armada100_fec.c line 764

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+		dev->enetaddr[4] = 0x22;
+		dev->enetaddr[5] = 0x33;
+#else
+		/* Generate random lower MAC half */
+		dev->enetaddr[3] = get_random_byte((u8)read_timer());
+		dev->enetaddr[4] = get_random_byte(dev->enetaddr[3]);
+		dev->enetaddr[5] = get_random_byte(dev->enetaddr[4]);
+#endif
+		eth_setenv_enetaddr(s, dev->enetaddr);
+	}

NAK on this whole thing. initialize dev->write_hwaddr and that is the only thing you should do. the higher eth layers will take care of calling that as necessary.

drivers/net/armada100_fec.c line 769

Quoted 4 lines
drivers/net/armada100_fec.c new file +4 -0
+	dev->init = (void *) armdfec_init;
+	dev->halt = (void *) armdfec_halt;
+	dev->send = (void *) armdfec_send;
+	dev->recv = (void *) armdfec_recv;

drop the (void*) casts. either you dont need them, or your funcs here are wrong and need fixing.

Quoted 5 lines
drivers/net/armada100_fec.c new file +5 -0
+#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 */

drivers/net/armada100_fec.c line 789

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+	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/20110822/06a8031c/attachment.pgp

#15 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 6 lines · Mike Frysinger

drivers/net/armada100_fec.c line 384

Quoted 1 line · On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
drivers/net/armada100_fec.c new file +1 -0
+	writel((u32) darmdfec->htpr, &regs->htpr);

do you really need to cast it yourself ? seems to show up a lot in this file.

[...]

Quoted 5 lines

make the reg base a parameter to armada100_fec_initialize()

drivers/net/armada100_fec.c line 709

Quoted 1 line
drivers/net/armada100_fec.c new file +1 -0
+	darmdfec = malloc(sizeof(struct armdfec_device));

sizeof(*darmdfec)

Why are you against sizeof(struct ...) ?

Quoted 15 lines
Quoted 15 lines
drivers/net/armada100_fec.c new file +14 -0
+	while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
+		/* Generate Private MAC addr if not set */
+		dev->enetaddr[0] = 0x00;
+		dev->enetaddr[1] = 0x50;
+		dev->enetaddr[2] = 0x43;
+#if defined(CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION)
+		/* Generate fixed lower MAC half */
+		dev->enetaddr[3] = 0x11;
+		dev->enetaddr[4] = 0x22;
+		dev->enetaddr[5] = 0x33;
+#else
+		/* Generate random lower MAC half */
+		dev->enetaddr[3] = get_random_byte((u8)read_timer());
+		dev->enetaddr[4] = get_random_byte(dev->enetaddr[3]);
Quoted 8 lines

drivers/net/armada100_fec.c line 764

Quoted 4 lines
drivers/net/armada100_fec.c new file +4 -0
+		dev->enetaddr[5] = get_random_byte(dev->enetaddr[4]);
+#endif
+		eth_setenv_enetaddr(s, dev->enetaddr);
+	}

NAK on this whole thing. initialize dev->write_hwaddr and that is the only thing you should do. the higher eth layers will take care of calling that as necessary.

ACK on the comment here.

#16 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 7 lines · On Monday, August 22, 2011 12:07:26 Marek Vasut wrote:
Quoted 4 lines · Mike Frysinger

drivers/net/armada100_fec.c line 709

Quoted 1 line · On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
drivers/net/armada100_fec.c new file +1 -0
+	darmdfec = malloc(sizeof(struct armdfec_device));

sizeof(*darmdfec)

Why are you against sizeof(struct ...) ?

imo, it holds up against bit rot easier and the code is clearer
-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/20110822/02ac261d/attachment.pgp

#17 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 10 lines · Mike Frysinger
Quoted 7 lines · On Monday, August 22, 2011 12:07:26 Marek Vasut wrote:
Quoted 4 lines · Mike Frysinger

drivers/net/armada100_fec.c line 709

Quoted 1 line · On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
drivers/net/armada100_fec.c new file +1 -0
+	darmdfec = malloc(sizeof(struct armdfec_device));

sizeof(*darmdfec)

Why are you against sizeof(struct ...) ?

imo, it holds up against bit rot easier and the code is clearer

OTOH, you don't have to look up what type of ptr it is if you use struct ...

Cheers

Quoted 1 line

-mike

#18 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 8 lines · On Monday, August 22, 2011 12:45:06 Marek Vasut wrote:
Quoted 7 lines · Mike Frysinger
Quoted 6 lines · On Monday, August 22, 2011 12:07:26 Marek Vasut wrote:
Quoted 4 lines · Mike Frysinger

drivers/net/armada100_fec.c line 709

Quoted 1 line · On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
drivers/net/armada100_fec.c new file +1 -0
+	darmdfec = malloc(sizeof(struct armdfec_device));

sizeof(*darmdfec)

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Why are you against sizeof(struct ...) ?

imo, it holds up against bit rot easier and the code is clearer

OTOH, you don't have to look up what type of ptr it is if you use struct
...

i dont see how that's generally relevant. if you want to know what the type of a variable is, you look for its definition. i dont go looking for alloc related funcs on the off chance that someone explicitly wrote out the type. -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/20110822/eeda6474/attachment.pgp

#19 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

Quoted 15 lines · Mike Frysinger

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 10 lines · On Monday, August 22, 2011 12:45:06 Marek Vasut wrote:
Quoted 9 lines · Mike Frysinger
Quoted 7 lines · On Monday, August 22, 2011 12:07:26 Marek Vasut wrote:
Quoted 4 lines · Mike Frysinger

drivers/net/armada100_fec.c line 709

Quoted 1 line · On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
drivers/net/armada100_fec.c new file +1 -0
+	darmdfec = malloc(sizeof(struct armdfec_device));

sizeof(*darmdfec)

Why are you against sizeof(struct ...) ?

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

imo, it holds up against bit rot easier and the code is clearer

OTOH, you don't have to look up what type of ptr it is if you use struct
...

i dont see how that's generally relevant. if you want to know what the type of a variable is, you look for its definition. i dont go looking for alloc related funcs on the off chance that someone explicitly wrote out the type.

There was a big thread on this on lkml long ago.

http://kerneltrap.org/node/5688

I prefer the inference of sizeof(*p) and think sizeof(struct blah) is a violation of DRY, but it's not a huge issue to me and I can go either way.

I've been called out by Wolfgang for using sizeof(*p) in some code but not in other code. It would be nice if there were some consistent recommendation for new code here, documented in the U-boot coding style wiki.

Jason

author
#20 lore raw

PATCH 1/3 net: Adds Fast Ethernet Controller driver for Armada100

In reply to Mike Frysinger
Quoted 75 lines · ----- "Mike Frysinger

drivers/net/armada100_fec.c line 384

Quoted 1 line · On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
drivers/net/armada100_fec.c new file +1 -0
+	writel((u32) darmdfec->htpr, &regs->htpr);

do you really need to cast it yourself ? seems to show up a lot in this file.

drivers/net/armada100_fec.c line 533

Quoted 3 lines
drivers/net/armada100_fec.c new file +3 -0
+#ifdef ETH_DUMP_REGS
+	eth_dump_regs(dev);
+#endif

use #ifdef DEBUG

drivers/net/armada100_fec.c line 629

Quoted 3 lines
drivers/net/armada100_fec.c new file +2 -0
+	while (cmd_sts & BUF_OWNED_BY_DMA) {
...
+	};

no semi-colon needed

drivers/net/armada100_fec.c line 742

Quoted 4 lines
drivers/net/armada100_fec.c new file +3 -0
+int armada100_fec_initialize()
+{
...
+	darmdfec->regs = (void *) ARMD1_FEC_BASE;

make the reg base a parameter to armada100_fec_initialize()

drivers/net/armada100_fec.c line 709

Quoted 1 line
drivers/net/armada100_fec.c new file +1 -0
+	darmdfec = malloc(sizeof(struct armdfec_device));

sizeof(*darmdfec)

Quoted 8 lines
drivers/net/armada100_fec.c new file +8 -0
+	while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
+		/* Generate Private MAC addr if not set */
+		dev->enetaddr[0] = 0x00;
+		dev->enetaddr[1] = 0x50;
+		dev->enetaddr[2] = 0x43;
+#if defined(CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION)
+		/* Generate fixed lower MAC half */
+		dev->enetaddr[3] = 0x11;

drivers/net/armada100_fec.c line 764

Quoted 10 lines
drivers/net/armada100_fec.c new file +10 -0
+		dev->enetaddr[4] = 0x22;
+		dev->enetaddr[5] = 0x33;
+#else
+		/* Generate random lower MAC half */
+		dev->enetaddr[3] = get_random_byte((u8)read_timer());
+		dev->enetaddr[4] = get_random_byte(dev->enetaddr[3]);
+		dev->enetaddr[5] = get_random_byte(dev->enetaddr[4]);
+#endif
+		eth_setenv_enetaddr(s, dev->enetaddr);
+	}

NAK on this whole thing. initialize dev->write_hwaddr and that is the only
thing you should do. the higher eth layers will take care of calling that as
necessary.

drivers/net/armada100_fec.c line 769

Quoted 4 lines
drivers/net/armada100_fec.c new file +4 -0
+	dev->init = (void *) armdfec_init;
+	dev->halt = (void *) armdfec_halt;
+	dev->send = (void *) armdfec_send;
+	dev->recv = (void *) armdfec_recv;

drop the (void*) casts. either you dont need them, or your funcs here are
wrong and need fixing.

Quoted 11 lines
drivers/net/armada100_fec.c new file +11 -0
+#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);
Quoted 8 lines

drivers/net/armada100_fec.c line 789

Quoted 4 lines
drivers/net/armada100_fec.c new file +4 -0
+		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

Hi Mike,

Thank you for comments.. I will make the required changes.

Regards,
Ajay Bhargav