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

[Ajay Bhargav]

----- "Mike Frysinger" <vap...@gentoo.org> wrote:

> On Monday, August 22, 2011 01:11:57 Ajay Bhargav wrote:
> > +   writel((u32) darmdfec->htpr, &regs->htpr);
> 
> do you really need to cast it yourself ?  seems to show up a lot in
> this file.
> 
> > +#ifdef ETH_DUMP_REGS
> > +   eth_dump_regs(dev);
> > +#endif
> 
> use #ifdef DEBUG
> 
> > +   while (cmd_sts & BUF_OWNED_BY_DMA) {
> > ...
> > +   };
> 
> no semi-colon needed
> 
> > +int armada100_fec_initialize()
> > +{
> > ...
> > +   darmdfec->regs = (void *) ARMD1_FEC_BASE;
> 
> make the reg base a parameter to armada100_fec_initialize()
> 
> > +   darmdfec = malloc(sizeof(struct armdfec_device));
> 
> sizeof(*darmdfec)
> 
> 
> > +   while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
> > +           /* Generate Private MAC addr if not set */
> > +           dev->enetaddr[0] = 0x00;
> > +           dev->enetaddr[1] = 0x50;
> > +           dev->enetaddr[2] = 0x43;
> > +#if defined(CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION)
> > +           /* Generate fixed lower MAC half */
> > +           dev->enetaddr[3] = 0x11;
> > +           dev->enetaddr[4] = 0x22;
> > +           dev->enetaddr[5] = 0x33;
> > +#else
> > +           /* Generate random lower MAC half */
> > +           dev->enetaddr[3] = get_random_byte((u8)read_timer());
> > +           dev->enetaddr[4] = get_random_byte(dev->enetaddr[3]);
> > +           dev->enetaddr[5] = get_random_byte(dev->enetaddr[4]);
> > +#endif
> > +           eth_setenv_enetaddr(s, dev->enetaddr);
> > +   }
> 
> NAK on this whole thing.  initialize dev->write_hwaddr and that is the
> only 
> thing you should do.  the higher eth layers will take care of calling
> that as 
> necessary.
> 
> > +   dev->init = (void *) armdfec_init;
> > +   dev->halt = (void *) armdfec_halt;
> > +   dev->send = (void *) armdfec_send;
> > +   dev->recv = (void *) armdfec_recv;
> 
> drop the (void*) casts.  either you dont need them, or your funcs here
> are 
> wrong and need fixing.
> 
> > +#if defined(CONFIG_PHY_BASE_ADR)
> > +   miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ,
> > +                (u16) CONFIG_PHY_BASE_ADR);
> > +#else
> > +   /* Search phy address from range 0-31 */
> > +   phy_adr = ethernet_phy_detect(dev);
> > +   if (phy_adr < 0) {
> > +           printf("Error: PHY not detected at address range 0-31\n");
> > +           return -1;
> > +   } else {
> > +           debug("PHY detected at addr %d\n", phy_adr);
> > +           miiphy_write(dev->name, PHY_ADR_REQ, PHY_ADR_REQ,
> > +                        (u16) phy_adr);
> > +   }
> > +#endif
> 
> this should be done in the armdfec_init() func, not the initialize
> func
> -mike

Hi Mike,

Thank you for comments.. I will make the required changes.

Regards,
Ajay Bhargav
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot