linux-mips
[Top] [All Lists]

Re: RFC: new WIP version of au1000_eth.c phylib conversion (was Re: RFC:

To: Andy Fleming <afleming@freescale.com>
Subject: Re: RFC: new WIP version of au1000_eth.c phylib conversion (was Re: RFC: au1000_etc.c phylib rewrite)
From: Herbert Valerio Riedel <hvr@gnu.org>
Date: Tue, 09 May 2006 04:04:23 +0200
Cc: Mark Schank <mschank@dcbnet.com>, ppopov@embeddedalley.com, sshtylyov@ru.mvista.com, linux-mips@linux-mips.org, jgarzik@pobox.com, netdev@vger.kernel.org, Ralf Baechle <ralf@linux-mips.org>, "Robin H. Johnson" <robbat2@gentoo.org>
In-reply-to: <6BF86C09-0732-4322-A43E-29705849886D@freescale.com>
Organization: Free Software Foundation
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <5.1.0.14.2.20060501144633.025e4e20@205.166.54.3> <1146510542.16643.10.camel@localhost.localdomain> <1146510542.16643.10.camel@localhost.localdomain> <5.1.0.14.2.20060501144633.025e4e20@205.166.54.3> <5.1.0.14.2.20060502095256.01fd4210@205.166.54.3> <1146674056.31241.18.camel@localhost.localdomain> <1146734223.31241.44.camel@localhost.localdomain> <6BF86C09-0732-4322-A43E-29705849886D@freescale.com>
Sender: linux-mips-bounce@linux-mips.org
jfyi, yet another more recent snapshot is attached to this email...

On Mon, 2006-05-08 at 17:24 -0500, Andy Fleming wrote:
> > I've tried to adapt the PHY detection code to allow for dynamic  
> > runtime configuration (with fallback to search for the 2nd MAC PHY on the 
> > 1st
> > MAC's MII bus), as well as selectable static PHY configuration through
> > Kconfig (e.g. for supporting PHYs w/o MII connection)
> 
> Comments inline, below:

> [snip]
> 
> Nothing to say so far, except seeing all those "-" signs was quite  
> the thrill, since one of the goals of the phylib was to lead to  
> reduced complexity.  That said, it looked like there were about a  
> dozen PHY-specific code blocks in there.  I saw you submit one PHY  
> driver.  Were there others in there that could be ported?

could well be, although I hope that most would work with the genphy
driver out of the box; but I'm depending on other people here anyway to
test it, since I don't have all kinds of evaluation boards available for
testing myself... :-)

I submitted the SMSC phy driver, because that's a piece of hardware I
had to play with...

> [snip]
> 
> > +static int mdiobus_read(struct mii_bus *bus, int phy_addr, int  
> > regnum)
> > {
> > -   int i, val;
> > +   struct net_device *const dev = bus->priv; /* beware: bus->phy_map 
> > [phy_addr].attached_dev == dev does _NOT_ hold always  */
> > +   enable_mac(dev, 0); /* make sure MAC associated with this mii_bus  
> > is enabled */
> > +   return mdio_read(dev, phy_addr, regnum);
> > +}
> 
> 
> Why is attached_dev not always correct?  I'm not sure if I'm not  
> understanding the hardware (I'm unfamiliar with this NIC), or if  
> you've misinterpreted the meaning of the attached_dev field.  It's  
> supposed to be a connection between the network device and a PHY,  
> mainly used for allowing the PHY to signal state changes back to the  
> ethernet device.  Is it actually the case that there is one MAC being  
> used for two PHYs at the same time? 

exactly; some au1xxxx based boards have so-called DualPHYs which provide
two PHYs, but with only one MII interface...

>  If so, how do you resolve which  
> PHY's state gets used at any given moment?

the attached_dev points to the net_device for which the PHY state is
relevant -- which might not be the net_device whose MII bus the PHY is
attached to...

it's just that the net_device stored in the mii_bus->priv field points
always to the "physical" net_device

and finally, each net_device has a pointer to its "logical" phy_dev

...does this answer your question? :-)

> The same question applies for the code in mdiobus_write()
> 
> [snip]

> > -static int mii_probe (struct net_device * dev)
> > +static int mdiobus_reset(struct mii_bus *bus)
> > {
> > -   struct au1000_private *aup = (struct au1000_private *) dev->priv;
> > -   int phy_addr;
> > -#ifdef CONFIG_MIPS_BOSPORUS
> > -   int phy_found=0;
> > -#endif
> > +   struct net_device *dev = bus->priv;
> > -   /* search for total of 32 possible mii phy addresses */
> > -   for (phy_addr = 0; phy_addr < 32; phy_addr++) {
> > -           u16 mii_status;
> > -           u16 phy_id0, phy_id1;
> > -           int i;
> > +   enable_mac(dev, 0); /* make sure MAC associated with this mii_bus  
> > is enabled */
> >
> 
> Do you need to call enable_mac() every time?  If it needs to be up,  
> wouldn't it be easier to make sure it's up during bus initialization?

it's actually a hack :-/ it's needed for the case where a PHY is
attached to a MAC other than the state-associated one... as MAC can be
powered down, when the eth is down, this allows to keep the MAC down if
it's not needed, and also make sure it get's enabled again, in case
something happened between mdiobus_reset and mdiobus_read/write 

> [snip]
> Might want to change that to be "...not 10/100..." or add a case for  
> 1000.

done...

> [snip]
> 
> > +   spin_unlock_irqrestore(&aup->lock, flags);
> > +   if (status_change) {
> > +           phy_print_status(phydev);
> > +   }
> > }
> 
> 
> Stylistic issue (I've seen it a couple times, at least):  don't use  
> "{" and "}" if your block only has one line.
> ie:
>       if (status_change)
>               phy_print_status(phydev);

you seem to have found one of the very few if(){single-line} cases I
introduced... (and that particular one was even a placeholder which got
bloated anyway :-))

regards,
hvr

Attachment: au1000eth_phylib_conversion.patch
Description: Text Data

Attachment: signature.asc
Description: This is a digitally signed message part

<Prev in Thread] Current Thread [Next in Thread>