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

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@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!