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

[Marek Vasut]

On Friday, August 26, 2011 08:36:51 AM Ajay Bhargav wrote:
> This patch adds support for Fast Ethernet Controller driver for
> Armada100 series.
> 
> Signed-off-by: Ajay Bhargav <ajay.bhargav at einfochips.com>

Hi, please don't forget to CC me next time ;-)

[...]

> +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);
> +		*value = (u16) (reg_data & 0x1f);

Do you need this cast?

> +		return 0;
> +	}
> +
> +	/* check parameters */
> +	if (phy_addr > PHY_MASK) {
> +		printf("Err..(%s) Invalid phy address: 0x%X\n",
> +				__func__, phy_addr);

Maybe a firm "A100 FEC: Invalid phy address (address = 0x%x)\n" would be better, 
fix that "Err.." and "Err  " globally please.

> +		return -EINVAL;
> +	}
> +	if (phy_reg > PHY_MASK) {
> +		printf("Err..(%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("Error (%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("Err (%s) PHY Read timeout, val=0x%x\n", __func__, val);
> +		return -1;
> +	}
> +	val = readl(&regs->smi);
> +	*value = val & 0xffff;
> +
> +	return 0;
> +}
> +
> +static int smi_reg_write(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;
> +
> +	if (phy_addr == PHY_ADR_REQ && phy_reg == PHY_ADR_REQ) {
> +		clrsetbits_le32(&regs->phyadr, 0x1f, value & 0x1f);
> +		return 0;
> +	}
> +
> +	/* check parameters */
> +	if (phy_addr > PHY_MASK) {
> +		printf("Err..(%s) Invalid phy address\n", __func__);
> +		return -EINVAL;
> +	}
> +	if (phy_reg > PHY_MASK) {
> +		printf("Err..(%s) Invalid register offset\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	/* wait for the SMI register to become available */
> +	if (armdfec_phy_timeout(&regs->smi, SMI_BUSY, FALSE)) {
> +		printf("Error (%s) PHY busy timeout\n",	__func__);
> +		return -1;
> +	}
> +
> +	writel((phy_addr << 16) | (phy_reg << 21) | SMI_OP_W | (value & 0xffff),
> +			&regs->smi);
> +	return 0;
> +}
> +
> +/*
> + * Abort any transmit and receive operations and put DMA
> + * in idle state. AT and AR bits are cleared upon entering
> + * in IDLE state. So poll those bits to verify operation.
> + */
> +static void abortdma(struct eth_device *dev)
> +{
> +	struct armdfec_device *darmdfec = to_darmdfec(dev);
> +	struct armdfec_reg *regs = darmdfec->regs;
> +	int delay;
> +	int maxretries = 40;
> +	u32 tmp;
> +

while (--maxretries) {

That way it won't get negative and you can check below for if (!maxretries)

> +	while (maxretries--) {
> +		writel(SDMA_CMD_AR | SDMA_CMD_AT, &regs->sdma_cmd);
> +		udelay(100);
> +
> +		delay = 10;
> +		while (delay--) {

This will go negative and the check below will be true for -1.Fix this to
while (--delay) {

> +			tmp = readl(&regs->sdma_cmd);
> +			if (!(tmp & (SDMA_CMD_AR | SDMA_CMD_AT)))
> +				break;
> +			udelay(10);
> +		}
> +		if (delay)
> +			break;
> +	}
> +
> +	if (maxretries <= 0)
> +		printf("%s : DMA Stuck\n", __func__);
> +}

See two comments above.

> +
> +static inline u32 nibble_swapping_32_bit(u32 x)
> +{
> +	return (((x) & 0xf0f0f0f0) >> 4) | (((x) & 0x0f0f0f0f) << 4);
> +}
> +
> +static inline u32 nibble_swapping_16_bit(u32 x)
> +{
> +	return (((x) & 0x0000f0f0) >> 4) | (((x) & 0x00000f0f) << 4);
> +}
> +
> +static inline u32 flip_4_bits(u32 x)
> +{
> +	return (((x) & 0x01) << 3) | (((x) & 0x002) << 1)
> +		| (((x) & 0x04) >> 1) | (((x) & 0x008) >> 3);
> +}
> +
> +/*
> + * This function will calculate the hash function of the address.
> + * depends on the hash mode and hash size.
> + * Inputs
> + * mach             - the 2 most significant bytes of the MAC address.
> + * macl             - the 4 least significant bytes of the MAC address.
> + * Outputs
> + * return the calculated entry.
> + */
> +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;
> +
> +	addrh = nibble_swapping_16_bit(mach);
> +	addrl = nibble_swapping_32_bit(macl);
> +
> +	addrhswapped = flip_4_bits(addrh & 0xf)
> +		+ ((flip_4_bits((addrh >> 4) & 0xf)) << 4)
> +		+ ((flip_4_bits((addrh >> 8) & 0xf)) << 8)
> +		+ ((flip_4_bits((addrh >> 12) & 0xf)) << 12);
> +
> +	addrlswapped = flip_4_bits(addrl & 0xf)
> +		+ ((flip_4_bits((addrl >> 4) & 0xf)) << 4)
> +		+ ((flip_4_bits((addrl >> 8) & 0xf)) << 8)
> +		+ ((flip_4_bits((addrl >> 12) & 0xf)) << 12)
> +		+ ((flip_4_bits((addrl >> 16) & 0xf)) << 16)
> +		+ ((flip_4_bits((addrl >> 20) & 0xf)) << 20)
> +		+ ((flip_4_bits((addrl >> 24) & 0xf)) << 24)
> +		+ ((flip_4_bits((addrl >> 28) & 0xf)) << 28);
> +
> +	addrh = addrhswapped;
> +	addrl = addrlswapped;
> +
> +	addr0 = (addrl >> 2) & 0x03f;
> +	addr1 = (addrl & 0x003) | (((addrl >> 8) & 0x7f) << 2);
> +	addr2 = (addrl >> 15) & 0x1ff;
> +	addr3 = ((addrl >> 24) & 0x0ff) | ((addrh & 1) << 8);
> +
> +	hashresult = (addr0 << 9) | (addr1 ^ addr2 ^ addr3);
> +	hashresult = hashresult & 0x07ff;
> +	return hashresult;
> +}
> +
> +/*
> + * This function will add an entry to the address table.
> + * depends on the hash mode and hash size that was initialized.
> + * Inputs
> + * mach - the 2 most significant bytes of the MAC address.
> + * macl - the 4 least significant bytes of the MAC address.
> + * skip - if 1, skip this address.
> + * rd   - the RD field in the address table.
> + * Outputs
> + * address table entry is added.
> + * 0 if success.
> + * -ENOSPC if table full
> + */
> +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;
> +	u8 *last;
> +
> +	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;
> +		}
> +		if (entry == start + 0x7ff)
> +			entry = start;
> +		else
> +			entry++;
> +	}
> +
> +	if (((entry->lo & 0xfffffff8) != (newlo & 0xfffffff8)) &&
> +		(entry->hi != newhi) && del)
> +		return 0;
> +
> +	if (i == HOP_NUMBER) {
> +		if (!del) {
> +			printf("%s: table section is full\n", __FILE__);

Unify the error reporting please.

It looks good, just a few nits

Cheers!