[PATCH 09/15] tg3: Allow WOL for phylib controlled Broadcom phys

[PATCH 09/15] tg3: Allow WOL for phylib controlled Broadcom phys

[Matt Carlson]

This patch allows WOL to be enabled for Broadcom phys under phylib
control.  The only exception is the AC131, which has a completely
different register set.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/tg3.c |   31 ++++++++++++++++++++++++-------
 drivers/net/tg3.h |   10 +++++++++-
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index e64721b..d0f314c 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -1963,7 +1963,7 @@ static int tg3_halt_cpu(struct tg3 *, u32);
 static int tg3_nvram_lock(struct tg3 *);
 static void tg3_nvram_unlock(struct tg3 *);
 
-static void tg3_power_down_phy(struct tg3 *tp)
+static void tg3_power_down_phy(struct tg3 *tp, bool do_low_power)
 {
 	u32 val;
 
@@ -1986,10 +1986,15 @@ static void tg3_power_down_phy(struct tg3 *tp)
 		tw32_f(GRC_MISC_CFG, val | GRC_MISC_CFG_EPHY_IDDQ);
 		udelay(40);
 		return;
-	} else if (!(tp->tg3_flags3 & TG3_FLG3_USE_PHYLIB)) {
+	} else if (do_low_power) {
 		tg3_writephy(tp, MII_TG3_EXT_CTRL,
 			     MII_TG3_EXT_CTRL_FORCE_LED_OFF);
-		tg3_writephy(tp, MII_TG3_AUX_CTRL, 0x01b2);
+
+		tg3_writephy(tp, MII_TG3_AUX_CTRL,
+			     MII_TG3_AUXCTL_SHDWSEL_PWRCTL |
+			     MII_TG3_AUXCTL_PCTL_100TX_LPWR |
+			     MII_TG3_AUXCTL_PCTL_SPR_ISOLATE |
+			     MII_TG3_AUXCTL_PCTL_VREG_11V);
 	}
 
 	/* The PHY should not be powered down on some chips because
@@ -2052,7 +2057,7 @@ static void __tg3_set_mac_addr(struct tg3 *tp, int skip_mac_1)
 static int tg3_set_power_state(struct tg3 *tp, pci_power_t state)
 {
 	u32 misc_host_ctrl;
-	bool device_should_wake;
+	bool device_should_wake, do_low_power;
 
 	/* Make sure register accesses (indirect or otherwise)
 	 * will function correctly.
@@ -2091,10 +2096,11 @@ static int tg3_set_power_state(struct tg3 *tp, pci_power_t state)
 			     (tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
 
 	if (tp->tg3_flags3 & TG3_FLG3_USE_PHYLIB) {
+		do_low_power = false;
 		if ((tp->tg3_flags3 & TG3_FLG3_PHY_CONNECTED) &&
 		    !tp->link_config.phy_is_low_power) {
 			struct phy_device *phydev;
-			u32 advertising;
+			u32 phyid, advertising;
 
 			phydev = tp->mdio_bus->phy_map[PHY_ADDR];
 
@@ -2124,8 +2130,19 @@ static int tg3_set_power_state(struct tg3 *tp, pci_power_t state)
 			phydev->advertising = advertising;
 
 			phy_start_aneg(phydev);
+
+			phyid = phydev->drv->phy_id & phydev->drv->phy_id_mask;
+			if (phyid != TG3_PHY_ID_BCMAC131) {
+				phyid &= TG3_PHY_OUI_MASK;
+				if (phyid == TG3_PHY_OUI_1 &&
+				    phyid == TG3_PHY_OUI_2 &&
+				    phyid == TG3_PHY_OUI_3)
+					do_low_power = true;
+			}
 		}
 	} else {
+		do_low_power = false;
+
 		if (tp->link_config.phy_is_low_power == 0) {
 			tp->link_config.phy_is_low_power = 1;
 			tp->link_config.orig_speed = tp->link_config.speed;
@@ -2169,7 +2186,7 @@ static int tg3_set_power_state(struct tg3 *tp, pci_power_t state)
 		u32 mac_mode;
 
 		if (!(tp->tg3_flags2 & TG3_FLG2_PHY_SERDES)) {
-			if (!(tp->tg3_flags3 & TG3_FLG3_USE_PHYLIB)) {
+			if (do_low_power) {
 				tg3_writephy(tp, MII_TG3_AUX_CTRL, 0x5a);
 				udelay(40);
 			}
@@ -2277,7 +2294,7 @@ static int tg3_set_power_state(struct tg3 *tp, pci_power_t state)
 	if (!(device_should_wake) &&
 	    !(tp->tg3_flags & TG3_FLAG_ENABLE_ASF) &&
 	    !(tp->tg3_flags3 & TG3_FLG3_ENABLE_APE))
-		tg3_power_down_phy(tp);
+		tg3_power_down_phy(tp, do_low_power);
 
 	tg3_frob_aux_power(tp);
 
diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index 417de07..d7ce3a0 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h
@@ -1795,6 +1795,11 @@
 
 #define MII_TG3_AUX_CTRL		0x18 /* auxilliary control register */
 
+#define MII_TG3_AUXCTL_PCTL_100TX_LPWR	0x0010
+#define MII_TG3_AUXCTL_PCTL_SPR_ISOLATE	0x0020
+#define MII_TG3_AUXCTL_PCTL_VREG_11V	0x0180
+#define MII_TG3_AUXCTL_SHDWSEL_PWRCTL	0x0002
+
 #define MII_TG3_AUXCTL_MISC_WREN	0x8000
 #define MII_TG3_AUXCTL_MISC_FORCE_AMDIX	0x0200
 #define MII_TG3_AUXCTL_MISC_RDSEL_MISC	0x7000
@@ -2590,7 +2595,10 @@ struct tg3 {
 #define PHY_REV_BCM5411_X0		0x1 /* Found on Netgear GA302T */
 #define TG3_PHY_ID_BCM50610		0x143bd60
 #define TG3_PHY_ID_BCMAC131		0x143bc70
-
+#define TG3_PHY_OUI_MASK		0xfffffc00
+#define TG3_PHY_OUI_1			0x00206000
+#define TG3_PHY_OUI_2			0x0143bc00
+#define TG3_PHY_OUI_3			0x03625c00
 
 	u32				led_ctrl;
 	u32				phy_otp;
-- 
1.5.6.4

Re: [PATCH 09/15] tg3: Allow WOL for phylib controlled Broadcom phys

[Andy Fleming]

On Mon, Nov 3, 2008 at 4:13 AM, Matt Carlson <mcarlson@broadcom.com> wrote:
> This patch allows WOL to be enabled for Broadcom phys under phylib
> control.  The only exception is the AC131, which has a completely
> different register set.
>
> Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> Signed-off-by: Michael Chan <mchan@broadcom.com>

Code to enable WOL sounds like the sort of thing we'd want in the PHY
driver itself, rather than a manual setup done from the NIC driver.
Clearly, we need support for WOL on both in order for it to work,
though, so I can see two solutions off the top of my head:

1) phy_connect() allows for passing in flags.  Maybe we should create
one for WOL, and have the config_init() functions for the Broadcom
PHYs check that, and set it up.

2) The appropriate code can register a phy_fixup, which will be
invoked whenever the PHY is initialized.  The fixup can be restricted
based on PHY ID and address.

It seems to me that WOL support is probably a common desire, so we
should either always enable it, or provide generic infrastructure for
enabling it, so that your driver doesn't need to know what PHY it is
connecting to.

Andy

Re: [PATCH 09/15] tg3: Allow WOL for phylib controlled Broadcom phys

[David Miller]

From: "Andy Fleming" <afleming@gmail.com>
Date: Mon, 3 Nov 2008 17:09:34 -0600

> On Mon, Nov 3, 2008 at 4:13 AM, Matt Carlson <mcarlson@broadcom.com> wrote:
> > This patch allows WOL to be enabled for Broadcom phys under phylib
> > control.  The only exception is the AC131, which has a completely
> > different register set.
> >
> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> 
> Code to enable WOL sounds like the sort of thing we'd want in the PHY
> driver itself, rather than a manual setup done from the NIC driver.
> Clearly, we need support for WOL on both in order for it to work,
> though, so I can see two solutions off the top of my head:
 ...

I've applied Matt's patch, but I hope that you two will continue
the dialogue about this issue.

Re: [PATCH 09/15] tg3: Allow WOL for phylib controlled Broadcom phys

[Matt Carlson]

On Mon, Nov 03, 2008 at 03:09:34PM -0800, Andy Fleming wrote:
> On Mon, Nov 3, 2008 at 4:13 AM, Matt Carlson <mcarlson@broadcom.com> wrote:
> > This patch allows WOL to be enabled for Broadcom phys under phylib
> > control.  The only exception is the AC131, which has a completely
> > different register set.
> >
> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> 
> Code to enable WOL sounds like the sort of thing we'd want in the PHY
> driver itself, rather than a manual setup done from the NIC driver.
> Clearly, we need support for WOL on both in order for it to work,
> though, so I can see two solutions off the top of my head:

Disregarding my diatribe below for a moment, I'd think you'd need both.
(1) informs the phylib how and to what state to bring the phy down,
(2) informs phylib how to bring the phy back to full power.

> 1) phy_connect() allows for passing in flags.  Maybe we should create
> one for WOL, and have the config_init() functions for the Broadcom
> PHYs check that, and set it up.

> 2) The appropriate code can register a phy_fixup, which will be
> invoked whenever the PHY is initialized.  The fixup can be restricted
> based on PHY ID and address.
> 
> It seems to me that WOL support is probably a common desire, so we
> should either always enable it, or provide generic infrastructure for
> enabling it, so that your driver doesn't need to know what PHY it is
> connecting to.

I suppose I should have thought out the wording of the patch description
more carefully.  The patch isn't enabling WOL.  Rather, it is allowing the
driver to bring the phy to a lower powered state.  This patch is mostly
about maximizing power savings.

WOL is mostly a function of the MAC.  Disregarding power budgets for the
moment, there is nothing the phy is required (or should be required) to
do to support WOL.  You should be able to configure the phy however you
wish and, as long as you have link, you should be able to wake the
machine.

To remove these phy writes from the tg3 driver then, phylib would have
to take on the burden of power management.  To bring the phy to a lower
powered state means that you also need a way to bring it back to full
power.  That implies taking complete control of the lifetime of the
phy, starting from phy powerup and ending at either configuring the phy
for a low power state or turning it off entirely.

I had toyed around with that idea for some time, but I just don't think
it is the proper thing to do.  There are two problems that I can see
with that approach.  The first problem is that we would need a way to
express exactly what state to leave the phy in when phy_disconnect() is
called.  There can be other agents, like management firmware, that
require the phy to be enabled for use even after the driver has
released the phy.  The process by which the driver would inform the
phylib this information would look very similar to the code sequence in
tg3_set_power_state().  In other words, we really aren't gaining
anything if the abstraction is just as verbose as the present API usage.
(Is it worth it to remove one or two phy specific writes from the
driver?)

The other problem is that there can be deeper interactions between the
phy and the MAC that are not all that apparent.  In some earlier tg3
devices, if you shutdown the phy, certain MAC registers would be
inaccessible or yeild bad values.  To solve this problem, the driver
needs finer grained control over the power state of the phy.  Of course
this is just one example off the top of my head and I can't speak
towards the similar types of problems that might exist in other
hardware.

Unfortunately, the devil is in the details.