[PATCH] [V4] net: emaclite: adding MDIO and phy lib support

[John Linn]

> -----Original Message-----
> From: glikely at secretlab.ca [mailto:glikely at secretlab.ca] On Behalf Of Grant Likely
> Sent: Thursday, February 11, 2010 2:16 PM
> To: John Linn
> Cc: netdev at vger.kernel.org; linuxppc-dev at ozlabs.org; jgarzik at pobox.com; jwboyer at linux.vnet.ibm.com;
> john.williams at petalogix.com; Sadanand Mutyala
> Subject: Re: [PATCH] [V4] net: emaclite: adding MDIO and phy lib support
> 
> On Thu, Feb 11, 2010 at 1:52 PM, John Linn <john.linn at xilinx.com> wrote:
> > These changes add MDIO and phy lib support to the driver as the
> > IP core now supports the MDIO bus.
> >
> > The MDIO bus and phy are added as a child to the emaclite in the device
> > tree as illustrated below.
> >
> > mdio {
> >        #address-cells = <1>;
> >        #size-cells = <0>;
> >        phy0: phy at 7 {
> >                compatible = "marvell,88e1111";
> >                reg = <7>;
> >        } ;
> > }
> >
> > Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala at xilinx.com>
> > Signed-off-by: John Linn <john.linn at xilinx.com>
> >
> > ---
> >
> > V2 - updated it for Grant's comments, except I couldn't find any tabs
> > converted to white space issue, let's see if V2 has it also
> >
> > V3 - updated it for Grant's comments, aded mutex release when a timeout
> > happens, and added Grant's acked by.
> >
> > V4 - removed the mutex as I realized the higher layer mdio calls already
> > use a mutex and there are no internal calls to the driver except in open.
> > Didn't integrate John W comments since releasing mutex wasn't needed.
> > ---
> >  drivers/net/Kconfig           |    1 +
> >  drivers/net/xilinx_emaclite.c |  381 ++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 339 insertions(+), 43 deletions(-)
> [...]
> >  /**
> >  * xemaclite_open - Open the network device
> >  * @dev:       Pointer to the network device
> >  *
> >  * This function sets the MAC address, requests an IRQ and enables interrupts
> >  * for the Emaclite device and starts the Tx queue.
> > + * It also connects to the phy device, if MDIO is included in Emaclite device.
> >  */
> >  static int xemaclite_open(struct net_device *dev)
> >  {
> > @@ -656,14 +924,50 @@ static int xemaclite_open(struct net_device *dev)
> >        /* Just to be safe, stop the device first */
> >        xemaclite_disable_interrupts(lp);
> >
> > +       if (lp->phy_node) {
> > +               u32 bmcr;
> > +
> > +               lp->phy_dev = of_phy_connect(lp->ndev, lp->phy_node,
> > +                                            xemaclite_adjust_link, 0,
> > +                                            PHY_INTERFACE_MODE_MII);
> > +               if (!lp->phy_dev) {
> > +                       dev_err(&lp->ndev->dev, "of_phy_connect() failed\n");
> > +                       return -ENODEV;
> > +               }
> > +
> > +               /* EmacLite doesn't support giga-bit speeds */
> > +               lp->phy_dev->supported &= (PHY_BASIC_FEATURES);
> > +               lp->phy_dev->advertising = lp->phy_dev->supported;
> > +
> > +               /* Don't advertise 1000BASE-T Full/Half duplex speeds */
> > +               xemaclite_mdio_write(lp->mii_bus, lp->phy_dev->addr,
> > +                                    MII_CTRL1000, 0x00);
> 
> Wait!  No.  This is wrong.  Sorry, I was all ready to ack this patch,
> but I just realized what was bothering me (and I should have clued
> into it earlier).  The driver cannot assume that the PHY is on an
> xemaclite MDIO bus.  It might be on a GPIO driven MDIO bus.  Or
> something else.  Calling xemaclite_mdio_{write,read} is not the right
> thing to do here.
> 
> Instead, the driver should call the phy library function
> phy_write(lp->phy_dev, MII_CTRL1000, 0);  The phylib code will route
> it to the correct mdio bus.  It also takes care of grabbing the mutex
> lock.
> 
> Make this fix, and then I can ack the driver.
> 

Ok, no worries, one more spin, I'm getting pretty good at spinning it.

Will make those changes to the calls in open, retest and respin.

Thanks,
John

> Cheers,
> g.
> 
> 
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.