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