From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ajay Bhargav <ajay.bhargav@einfochips.com>
Date: Tue, 30 Aug 2011 16:16:21 +0530 (IST)
Subject: [U-Boot] [PATCH v4 1/3] net: Adds Fast Ethernet Controller
 driver for Armada100
In-Reply-To: <261975966.7748.1314700867461.JavaMail.root@ahm.einfochips.com>
Message-ID: <364016227.7795.1314701181708.JavaMail.root@ahm.einfochips.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de


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

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

Yes.. sorry for mistake.

> [...]
> 
> > +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;
> }
> 
> ? :-)
> 

No.. if entry is valid, we should not modify it anyways. Deleting a valid entry might break the hash chain.

> > +		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 delete is requested for an address which is not present in chain, we should return immideatly. so && is fine there.

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

If I use dataptr directly, sending doesnt work... 

> > +	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.
> 
will do that..

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

Thanks,
Ajay Bhargav