Re: [U-Boot] [PATCH 2/4] net: Adds Fast Ethernet Controller driver for Armada100

Dear Ajay Bhargav,

In message 1310982108-26029-2-git-send-email-ajay.bhargav@einfochips.com you wrote:

This patch adds support for Fast Ethernet Controller driver for Armada100 series.

...

• printf("\noffset: phy_adr, value: 0x%x\n", ARMDFEC_RD(regs->phyadr));
• printf("offset: smi, value: 0x%x\n", ARMDFEC_RD(regs->smi));

Please get rid of the ARMDFEC_RD() and ARMDFEC_WR() macros and use the standard I/O accessors directly.

+static u8 get_random_byte(u8 seed) +{

• udelay(seed);
• return (u8)(get_timer(0) % 100) + seed;

+}

You probably want to use "udelay(1000 * seed)" here - udelay() is in microseconds, but get_timer() is in milliseconds.

• /* check parameters */
• if (phy_addr > PHY_MASK) {

• printf("Err..(%s) Invalid phy address\n", __func__);
  

• return -EINVAL;
  

In such error messages the incorrect data (here: the content of phy_addr) is the most interesting thing - please include this. Please fix globally.

...

• if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {

• reg_data = ARMDFEC_RD(regs->phyadr);
  

• reg_data &= ~(0x1f);
  

• reg_data |= (value & 0x1f);
  

• ARMDFEC_WR(reg_data, regs->phyadr);
  

• return 0;
  

• }

Sequences like thesde should be rewritten using the standard I/O accessor macros, here clrsetbits_le32(). Please fix globally.

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

We don't allow for CamelCaps identifiers. Please fix globally.

+/* 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);
• ARMDFEC_WR((u32) darmdfec->htpr, regs->htpr);

Please always separate declarations and code by a blank line. Please fix globally.

...

• for (addr = 0; addr < 32; addr++) {

• if (miiphy_read(dev->name, addr, MII_BMSR, &mii_status)
  

• 	!= 0)
  

• 	/* 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;
  

Even if it's only comments, these are multiline statements which need braces. Please fix globally.

• 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;
  

• }
  

Coding style: Use braces in both branches. Please fix globally.

• /* 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.
  

•  */
  

Incorrect multiline comment style.

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

• return 0;

+}

+int armada100_fec_initialize() +{

• struct armdfec_device *darmdfec;
• struct eth_device *dev;
• int phy_adr;
• char *s = "ethaddr";
• darmdfec = malloc(sizeof(struct armdfec_device));
• if (!darmdfec)

• goto error1;
  

• memset(darmdfec, 0, sizeof(struct armdfec_device));
• darmdfec->htpr = memalign(8, HASH_ADDR_TABLE_SIZE);
• if (!darmdfec->htpr)

• goto error2;
  

• darmdfec->p_rxdesc =

• (struct rx_desc *) memalign(PKTALIGN,
  

• 			ARMDFEC_RXQ_DESC_ALIGNED_SIZE
  

• 			* RINGSZ + 1);
  

• if (!darmdfec->p_rxdesc)

• goto error3;
  

• darmdfec->p_rxbuf = (u8 *) memalign(PKTALIGN, RINGSZ

• 				* PKTSIZE_ALIGN + 1);
  

• if (!darmdfec->p_rxbuf)

• goto error4;
  

• darmdfec->p_aligned_txbuf = memalign(8, PKTSIZE_ALIGN);
• if (!darmdfec->p_aligned_txbuf)

• goto error5;
  

• darmdfec->p_txdesc = (struct tx_desc *)

• memalign(PKTALIGN, sizeof(struct tx_desc) + 1);
  

• if (!darmdfec->p_txdesc) {

• free(darmdfec->p_aligned_txbuf);
  

+error5:

• free(darmdfec->p_rxbuf);
  

+error4:

• free(darmdfec->p_rxdesc);
  

+error3:

• free(darmdfec->htpr);
  

+error2:

• free(darmdfec);
  

You could simplify the code and just use a common error entry point and always call free() for all entries - free(NULL) is defined to have no effect.

+/* Bit fields of a Hash Table Entry */ +enum hash_table_entry {

• hteValid = 1,
• hteSkip = 2,
• hteRD = 4,
• hteRDBit = 2

+};

No CamelCaps identifiers please - see above.

+struct tx_desc {

• volatile u32 cmd_sts; /* Command/status field */

WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

+struct rx_desc {

• volatile u32 cmd_sts; /* Descriptor command status */

WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

Best regards,

Wolfgang Denk