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

[Wolfgang Denk]

Dear Ajay Bhargav,

In message <1310982108-26029-2-git-send-email-ajay.bhargav at 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The heart is not a logical organ.
	-- Dr. Janet Wallace, "The Deadly Years", stardate 3479.4