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

[Marek Vasut]

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.bhar...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot