[U-Boot,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 <ajay.bhargav@einfochips.com>
---
 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

On Monday, August 22, 2011 07:11:57 AM Ajay Bhargav wrote:
> This patch adds support for Fast Ethernet Controller driver for
> Armada100 series.
> 
> Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> ---
>  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
> 
> 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..67fd73d
> --- /dev/null
> +++ b/drivers/net/armada100_fec.c
> @@ -0,0 +1,802 @@
> +/*
> + * (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 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)
> +		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 ?

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

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

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

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

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

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

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

> +	debug("%s:(adr 0x%x, off 0x%x) value= %04x\n", __func__,
> +		phy_addr, phy_reg, value);

debug, see above.

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

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


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

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

Put parens about the second argument of OR

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

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

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

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

> +	int retry = 10000;

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

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

Timeout function needed.

> +
> +	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((unsigned int) p_rxdesc_curr->nxtdesc_p,
> +			&(darmdfec->p_rxdesc_curr));

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

> +
> +	return 0;
> +}
> +
> +int armada100_fec_initialize()
> +{
> +	struct armdfec_device *darmdfec;
> +	struct eth_device *dev;
> +	int phy_adr;
> +	char *s = "ethaddr";

constify this.

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

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

> +
> +	eth_register(dev);

Newline won't hurt here.

> +#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;
> +}
> diff --git a/drivers/net/armada100_fec.h b/drivers/net/armada100_fec.h
> new file mode 100644
> index 0000000..dcac964
> --- /dev/null
> +++ b/drivers/net/armada100_fec.h
> @@ -0,0 +1,225 @@
> +/*
> + * (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__
> +
> +#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];

Use reserved maybe ?

> +	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..e7b9298 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_initialize(void);
> 
>  /* Boards with PCI network controllers can call this from their
> board_eth_init() * function to initialize whatever's on board.


Cheers!

On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
> +	writel((u32) darmdfec->htpr, &regs->htpr);

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

> +#ifdef ETH_DUMP_REGS
> +	eth_dump_regs(dev);
> +#endif

use #ifdef DEBUG

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

> +	darmdfec = malloc(sizeof(struct armdfec_device));

sizeof(*darmdfec)


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

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

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

On Monday, August 22, 2011 06:02:26 PM Mike Frysinger wrote:
> On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
> > +	writel((u32) darmdfec->htpr, &regs->htpr);
> 
> do you really need to cast it yourself ?  seems to show up a lot in this
> file.
> 

[...]

> make the reg base a parameter to armada100_fec_initialize()
> 
> > +	darmdfec = malloc(sizeof(struct armdfec_device));
> 
> sizeof(*darmdfec)

Why are you against sizeof(struct ...) ?
> 
> > +	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]);
> > +		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.

On Monday, August 22, 2011 12:07:26 Marek Vasut wrote:
> On Monday, August 22, 2011 06:02:26 PM Mike Frysinger wrote:
> > On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
> > > +	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

On Monday, August 22, 2011 06:42:33 PM Mike Frysinger wrote:
> On Monday, August 22, 2011 12:07:26 Marek Vasut wrote:
> > On Monday, August 22, 2011 06:02:26 PM Mike Frysinger wrote:
> > > On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
> > > > +	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
> -mike

On Monday, August 22, 2011 12:45:06 Marek Vasut wrote:
> On Monday, August 22, 2011 06:42:33 PM Mike Frysinger wrote:
> > On Monday, August 22, 2011 12:07:26 Marek Vasut wrote:
> > > On Monday, August 22, 2011 06:02:26 PM Mike Frysinger wrote:
> > > > On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
> > > > > +	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
> ...

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

On Monday, August 22, 2011 12:11pm, "Mike Frysinger" <vapier@gentoo.org> said:

> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> On Monday, August 22, 2011 12:45:06 Marek Vasut wrote:
>> On Monday, August 22, 2011 06:42:33 PM Mike Frysinger wrote:
>> > On Monday, August 22, 2011 12:07:26 Marek Vasut wrote:
>> > > On Monday, August 22, 2011 06:02:26 PM Mike Frysinger wrote:
>> > > > On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
>> > > > > +	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
>> ...
> 
> 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

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

> On Monday, August 22, 2011 07:11:57 AM Ajay Bhargav wrote:
> > This patch adds support for Fast Ethernet Controller driver for
> > Armada100 series.
> > 
> > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > ---
> >  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
> > 
> 
> Cheers!
> 

Hi Marek,

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

Regards,
Ajay Bhargav

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

> On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
> > +	writel((u32) darmdfec->htpr, &regs->htpr);
> 
> do you really need to cast it yourself ?  seems to show up a lot in
> this file.
> 
> > +#ifdef ETH_DUMP_REGS
> > +	eth_dump_regs(dev);
> > +#endif
> 
> use #ifdef DEBUG
> 
> > +	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()
> 
> > +	darmdfec = malloc(sizeof(struct armdfec_device));
> 
> sizeof(*darmdfec)
> 
> 
> > +	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]);
> > +		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.
> 
> > +	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.
> 
> > +#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

Hi Mike,

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

Regards,
Ajay Bhargav

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

> On Monday, August 22, 2011 07:11:57 AM Ajay Bhargav wrote:
> > This patch adds support for Fast Ethernet Controller driver for
> > Armada100 series.
> > 
> > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > ---
> >  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
> > 
[...]
> > +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.

[...]

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

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

> On Tuesday, August 23, 2011 08:11:57 AM Ajay Bhargav wrote:
> > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > On Monday, August 22, 2011 07:11:57 AM Ajay Bhargav wrote:
> > > > This patch adds support for Fast Ethernet Controller driver for
> > > > Armada100 series.
> > > > 
> > > > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > > > ---
> > > > 
> > > >  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
> > 
> > [...]
> > 
> > > > +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.
> > 
> > [...]
> > 
> > > > +		/* 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.
> 
> 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
> > 
> > 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

On Tuesday, August 23, 2011 08:11:57 AM Ajay Bhargav wrote:
> ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > On Monday, August 22, 2011 07:11:57 AM Ajay Bhargav wrote:
> > > This patch adds support for Fast Ethernet Controller driver for
> > > Armada100 series.
> > > 
> > > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > > ---
> > > 
> > >  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
> 
> [...]
> 
> > > +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.
> 
> [...]
> 
> > > +		/* 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.

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
> 
> Thanks & Regards,
> Ajay Bhargav

Dear Ajay Bhargav,

In message <1058764603.122757.1314095393033.JavaMail.root@ahm.einfochips.com> you wrote:
> 
> > 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.
...
> 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.

> 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

----- "Wolfgang Denk" <wd@denx.de> wrote:

> Dear Ajay Bhargav,
> 
> In message
> <1058764603.122757.1314095393033.JavaMail.root@ahm.einfochips.com> you
> wrote:
> > 
> > > 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.
> ...
> > 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

----- "Wolfgang Denk" <wd@denx.de> wrote:

> Dear Ajay Bhargav,
> 
> In message
> <1058764603.122757.1314095393033.JavaMail.root@ahm.einfochips.com> you
> wrote:
> > 
> > > 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.
> ...
> > 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.
> 
> > 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
> 
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> 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

Dear Ajay Bhargav,

In message <1570238601.123783.1314104389736.JavaMail.root@ahm.einfochips.com> you wrote:
> 
> 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

Wolfgang Denk writes:
 > > 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).

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

On Tuesday, August 23, 2011 11:30:38 Ralph Metzler wrote:
> Wolfgang Denk writes:
>  > > 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).
> 
> 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?

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

Dear Ralph Metzler,

In message <20051.51102.404246.81248@morden.metzler> 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

----- "Wolfgang Denk" <wd@denx.de> wrote:

> Dear Ajay Bhargav,
> 
> In message
> <1570238601.123783.1314104389736.JavaMail.root@ahm.einfochips.com> you
> wrote:
> > 
> > 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
> 
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