[U-Boot] [PATCH v4 1/3] net: Adds Fast Ethernet Controller driver for Armada100

[Marek Vasut <marek.vasut@gmail.com>]

On Tuesday, August 30, 2011 07:44:40 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>

[...]

> +static int smi_reg_read(const char *devname, u8 phy_addr, u8 phy_reg,
> +			u16 *value)
> +{
> +	struct eth_device *dev = eth_get_dev_by_name(devname);
> +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> +	struct armdfec_reg *regs = darmdfec->regs;
> +	u32 val, reg_data;
> +
> +	if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {
> +		reg_data = readl(&regs->phyadr);

You use "reg_data" here and "val" below ... can't you use just one variable?

> +		*value = reg_data & 0x1f;
> +		return 0;
> +	}
> +
> +	/* check parameters */
> +	if (phy_addr > PHY_MASK) {
> +		printf("ARMD100 FEC: (%s) Invalid phy address: 0x%X\n",
> +				__func__, phy_addr);
> +		return -EINVAL;
> +	}
> +	if (phy_reg > PHY_MASK) {
> +		printf("ARMD100 FEC: (%s) Invalid register offset: 0x%X\n",
> +				__func__, phy_reg);
> +		return -EINVAL;
> +	}
> +
> +	/* wait for the SMI register to become available */
> +	if (armdfec_phy_timeout(&regs->smi, SMI_BUSY, FALSE)) {
> +		printf("ARMD100 FEC: (%s) PHY busy timeout\n",	__func__);
> +		return -1;
> +	}
> +
> +	writel((phy_addr << 16) | (phy_reg << 21) | SMI_OP_R, &regs->smi);
> +
> +	/* now wait for the data to be valid */
> +	if (armdfec_phy_timeout(&regs->smi, SMI_R_VALID, TRUE)) {
> +		val = readl(&regs->smi);
> +		printf("ARMD100 FEC: (%s) PHY Read timeout, val=0x%x\n",
> +				__func__, val);
> +		return -1;
> +	}
> +	val = readl(&regs->smi);
> +	*value = val & 0xffff;
> +
> +	return 0;
> +}

[...]

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

What about 

if (!(entry->lo & HTEVALID))
	break;

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;

Now thinking of it, are you sure about this condition ? Shouldn't the first && be 
|| instead ? And there should be parenthesis around that ?

> +
> +	if (i == HOP_NUMBER) {
> +		if (!del) {
> +			printf("ARMD100 FEC: (%s) table section is full\n",
> +					__func__);
> +			return -ENOSPC;
> +		} else {
> +			return 0;
> +		}
> +	}

[...]

> +
> +	/* 64 should work but does not -- dhcp packets NEVER get transmitted. */

What's this about ?

> +	if ((mtu > MAX_PKT_SIZE) || (mtu < 64))
> +		return -EINVAL;

[...]

> +static int armdfec_send(struct eth_device *dev, volatile void *dataptr,
> +		    int datasize)
> +{
> +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> +	struct armdfec_reg *regs = darmdfec->regs;
> +	struct tx_desc *p_txdesc = darmdfec->p_txdesc;
> +	void *p = (void *) dataptr;

Is this needed?

> +	int retry = PHY_WAIT_ITERATIONS * PHY_WAIT_MICRO_SECONDS;
> +	u32 cmd_sts;
> +
> +	/* Copy buffer if it's misaligned */
> +	if ((u32) dataptr & 0x07) {

Space after typecast. Please fix globally.

> +		if (datasize > PKTSIZE_ALIGN) {
> +			printf("ARMD100 FEC: Non-aligned data too large (%d)\n",
> +					datasize);
> +			return -1;
> +		}
> +		memcpy(darmdfec->p_aligned_txbuf, p, datasize);
> +		p = darmdfec->p_aligned_txbuf;
> +	}

The rest is really good !

Thanks!

Cheers