Message ID | 1225751626.6190@xw6200 |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;