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

[Grant Likely <grant.likely@secretlab.ca>]

On Thu, Feb 11, 2010 at 1:52 PM, John Linn <john.linn@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@7 {
>                compatible = "marvell,88e1111";
>                reg = <7>;
>        } ;
> }
>
> Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala@xilinx.com>
> Signed-off-by: John Linn <john.linn@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.

Cheers,
g.


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