----- "Marek Vasut" <[email protected]> wrote:
> 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 <[email protected]>
>
> [...]
>
> > +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(®s->phyadr);
>
> You use "reg_data" here and "val" below ... can't you use just one
> variable?
>
Yes.. sorry for mistake.
> [...]
>
> > +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;
> }
>
> ? :-)
>
No.. if entry is valid, we should not modify it anyways. Deleting a valid entry
might break the hash chain.
> > + 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 delete is requested for an address which is not present in chain, we should
return immideatly. so && is fine there.
> > +
> > + 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?
>
If I use dataptr directly, sending doesnt work...
> > + 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.
>
will do that..
> > + 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
>
Thanks,
Ajay Bhargav
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
