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

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