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

[Ajay Bhargav <ajay.bhargav@einfochips.com>]

----- "Mike Frysinger" <vapier@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