[PATCH 3/3] mv643xx_eth: convert to phylib

On Mon, Sep 15, 2008 at 06:55:18PM -0500, Andy Fleming wrote:

> >@@ -1338,7 +1327,7 @@ static int mv643xx_eth_nway_reset(struct  
> >net_device *dev)
> >{
> >	struct mv643xx_eth_private *mp = netdev_priv(dev);
> >
> >-	return mii_nway_restart(&mp->mii);
> >+	return phy_nway_restart(mp->phy);
> 
> As mentioned in patch #2, I think you can use genphy_restart_aneg(),  
> here.

Right, that looks like that'll work.

> >static int mv643xx_eth_nway_reset_phyless(struct net_device *dev)
> >@@ -1350,7 +1339,7 @@ static u32 mv643xx_eth_get_link(struct  
> >net_device *dev)
> >{
> >	struct mv643xx_eth_private *mp = netdev_priv(dev);
> >
> >-	return mii_link_ok(&mp->mii);
> >+	return phy_link_ok(mp->phy);
> 
> You can just return phydev->link, unless you don't think phydev->link  
> will be up-to-date.  If so, I recommend using genphy_update_link().   
> I'm curious: you say the controller handles all the link management.   
> Is it possible to read that information out?  Since you are doing the  
> "do it yourself" method, it would simplify and speed up certain  
> options if you just did:
> 
> phydev->link = readl(somereg) & LINK_STATUS ? 1 : 0
> 
> You could do that whenever the controller tells you the link is down.

Yes, e.g. this works:

	static u32 mv643xx_eth_get_link(struct net_device *dev)
	{
		struct mv643xx_eth_private *mp = netdev_priv(dev);

		return !!(rdl(mp, PORT_STATUS(mp->port_num)) & LINK_UP);
	}

I wonder if I need to update ->link at all, considering that I don't
use it, and I don't call any phylib functionality that uses it.

> >+static void phy_init(struct mv643xx_eth_private *mp, int speed, int  
> >duplex)
> >{
> >	struct ethtool_cmd cmd;
> >-	int err;
> >
> >-	err = phy_detect(mp);
> >-	if (err) {
> >-		dev_printk(KERN_INFO, &mp->dev->dev,
> >-			   "no PHY detected at addr %d\n", mp->phy_addr);
> >-		return err;
> >-	}
> >	phy_reset(mp);
> >
> >-	mp->mii.phy_id = mp->phy_addr;
> >-	mp->mii.phy_id_mask = 0x3f;
> >-	mp->mii.reg_num_mask = 0x1f;
> >-	mp->mii.dev = mp->dev;
> >-	mp->mii.mdio_read = mv643xx_eth_mdio_read;
> >-	mp->mii.mdio_write = mv643xx_eth_mdio_write;
> >-
> >-	mp->mii.supports_gmii = mii_check_gmii_support(&mp->mii);
> >+	phy_attach(mp->dev, mp->phy->dev.bus_id, 0, 0);
> >
> >	memset(&cmd, 0, sizeof(cmd));
> >
> >	cmd.port = PORT_MII;
> >	cmd.transceiver = XCVR_INTERNAL;
> >-	cmd.phy_address = mp->phy_addr;
> >-	if (pd->speed == 0) {
> >+	cmd.phy_address = mp->phy->addr;
> >+	if (speed == 0) {
> >		cmd.autoneg = AUTONEG_ENABLE;
> >		cmd.speed = SPEED_100;
> >		cmd.advertising = ADVERTISED_10baseT_Half  |
> >				  ADVERTISED_10baseT_Full  |
> >				  ADVERTISED_100baseT_Half |
> >				  ADVERTISED_100baseT_Full;
> >-		if (mp->mii.supports_gmii)
> >+		if (phy_check_gmii_support(mp->phy))
> >			cmd.advertising |= ADVERTISED_1000baseT_Full;
> >	} else {
> >		cmd.autoneg = AUTONEG_DISABLE;
> >-		cmd.speed = pd->speed;
> >-		cmd.duplex = pd->duplex;
> >+		cmd.speed = speed;
> >+		cmd.duplex = duplex;
> >	}
> >
> >	mv643xx_eth_set_settings(mp->dev, &cmd);
> 
> This is a somewhat roundabout way to configure the PHY.   
> The ..._sset() function was intended to provide support for userspace  
> configuration of the PHY via the same interface ethtool uses.  There  
> are more direct ways to configure the PHY when you have a pointer to a  
> struct phy_device.

ACK.  I was just doing the same thing as the old code did -- which I
inherited from somewhere else.

> Look at the code for phy_ethtool_sset(), and replicate what it does.   
> That way you don't need to set up the struct ethtool_cmd.  The  
> definitions for the fields are identical, so it should just mean  
> replacing all the cmd.foo = bar lines with mp->phy->foo = bar.
> 
> And, actually, I believe you can reduce this even more, since you are  
> just doing the generic initialization from what I can tell.  You can  
> just set mp->phy->supported and ->advertising to be what your  
> controller supports (masked with what the connected PHY supports), and  
> then invoke phy_start_aneg(mp->phy);  phylib will configure the PHY  
> properly, and then return control to you.

I'll make this function:

	static void phy_init(struct mv643xx_eth_private *mp, int speed, int duplex)
	{
		struct phy_device *phy = mp->phy;

		phy_reset(mp);

		phy_attach(mp->dev, phy->dev.bus_id, 0, PHY_INTERFACE_MODE_GMII);

		phy->state = PHY_HALTED;

		if (speed == 0) {
			phy->autoneg = AUTONEG_ENABLE;
			phy->speed = 0;
			phy->duplex = 0;
			phy->advertising = phy->supported | ADVERTISED_Autoneg;
		} else {
			phy->autoneg = AUTONEG_DISABLE;
			phy->advertising = 0;
			phy->speed = speed;
			phy->duplex = duplex;
		}
		phy_start_aneg(phy);
	}

OK?  (PHY_HALTED because that seems to be the way to stop the state
machine from running at all.  Or should I call phy_stop() once?)

> You will, however, need to  
> figure out how best to read out the status for the PHY, and how to  
> know when autonegotiation is done.  I'm assuming that your controller  
> tracks that information, and that you can manually pull that  
> information from local register space (rather than the SMI bus).

I don't really need to access it all during normal operation -- the
controller just reads the link up/down and speed and duplex bits from
the PHY automatically and then configures itself accordingly.  The
only time I need to access this information is when someone asks for
it via ethtool (or if I want to print a link status message).  I.e. I
don't need any state machine, be it phylib's or my own.

New patch:

commit 61c35c0299272a4f8d8fb8d3dd59a9742fb34f35
Author: Lennert Buytenhek <buytenh@wantstofly.org>
Date:   Tue Aug 26 13:34:19 2008 +0200

    mv643xx_eth: convert to phylib

    Switch mv643xx_eth from using drivers/net/mii.c to using phylib.

    Since the mv643xx_eth hardware does all the link state handling and
    PHY polling, the driver will use phylib in the "Doing it all yourself"
    mode described in the phylib documentation.

    Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>

Message ID	20080918124428.GB9157@xi.wantstofly.org
State	Deferred, archived
Delegated to:	Jeff Garzik
Headers	show Return-Path: <netdev-owner@vger.kernel.org> X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 18962DDF5A for <patchwork-incoming@ozlabs.org>; Thu, 18 Sep 2008 22:44:38 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752418AbYIRMob (ORCPT <rfc822;patchwork-incoming@ozlabs.org>); Thu, 18 Sep 2008 08:44:31 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752628AbYIRMob (ORCPT <rfc822; netdev-outgoing>); Thu, 18 Sep 2008 08:44:31 -0400 Received: from xi.wantstofly.org ([83.160.184.112]:40916 "EHLO xi.wantstofly.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752533AbYIRMo3 (ORCPT <rfc822;netdev@vger.kernel.org>); Thu, 18 Sep 2008 08:44:29 -0400 Received: by xi.wantstofly.org (Postfix, from userid 500) id 19B837FAC2; Thu, 18 Sep 2008 14:44:28 +0200 (CEST) Date: Thu, 18 Sep 2008 14:44:28 +0200 From: Lennert Buytenhek <buytenh@wantstofly.org> To: Andy Fleming <afleming@freescale.com> Cc: netdev@vger.kernel.org Subject: Re: [PATCH 3/3] mv643xx_eth: convert to phylib Message-ID: <20080918124428.GB9157@xi.wantstofly.org> References: <1220448012-20347-1-git-send-email-buytenh@wantstofly.org> <1220448012-20347-4-git-send-email-buytenh@wantstofly.org> <A8E7D979-2540-456E-818C-A419D7BE4166@freescale.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <A8E7D979-2540-456E-818C-A419D7BE4166@freescale.com> User-Agent: Mutt/1.4.2.2i Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: <netdev.vger.kernel.org> X-Mailing-List: netdev@vger.kernel.org

[PATCH 3/3] mv643xx_eth: convert to phylib

Commit Message

Comments

Patch