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 {
> =A0 =A0 =A0 =A0#address-cells =3D <1>;
> =A0 =A0 =A0 =A0#size-cells =3D <0>;
> =A0 =A0 =A0 =A0phy0: phy@7 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "marvell,88e1111";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <7>;
> =A0 =A0 =A0 =A0} ;
> }
>
> 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.
> ---
> =A0drivers/net/Kconfig =A0 =A0 =A0 =A0 =A0 | =A0 =A01 +
> =A0drivers/net/xilinx_emaclite.c | =A0381 +++++++++++++++++++++++++++++++=
+++++-----
> =A02 files changed, 339 insertions(+), 43 deletions(-)
[...]
> =A0/**
> =A0* xemaclite_open - Open the network device
> =A0* @dev: =A0 =A0 =A0 Pointer to the network device
> =A0*
> =A0* This function sets the MAC address, requests an IRQ and enables inte=
rrupts
> =A0* for the Emaclite device and starts the Tx queue.
> + * It also connects to the phy device, if MDIO is included in Emaclite d=
evice.
> =A0*/
> =A0static int xemaclite_open(struct net_device *dev)
> =A0{
> @@ -656,14 +924,50 @@ static int xemaclite_open(struct net_device *dev)
> =A0 =A0 =A0 =A0/* Just to be safe, stop the device first */
> =A0 =A0 =A0 =A0xemaclite_disable_interrupts(lp);
>
> + =A0 =A0 =A0 if (lp->phy_node) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 bmcr;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lp->phy_dev =3D of_phy_connect(lp->ndev, lp=
->phy_node,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0xemaclite_adjust_link, 0,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0PHY_INTERFACE_MODE_MII);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!lp->phy_dev) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&lp->ndev->dev, "of=
_phy_connect() failed\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENODEV;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EmacLite doesn't support giga-bit speeds=
 */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lp->phy_dev->supported &=3D (PHY_BASIC_FEAT=
URES);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lp->phy_dev->advertising =3D lp->phy_dev->s=
upported;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Don't advertise 1000BASE-T Full/Half dup=
lex speeds */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 xemaclite_mdio_write(lp->mii_bus, lp->phy_d=
ev->addr,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
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.


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