[PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization

[PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization

[Sebastian Hesselbarth]

Ethernet PHYs consume a significant amount of power when link is detected.
Especially, for embedded systems it can be easily 20-40% of total system
power. Now, currently most likely all ethernet drivers leave PHYs powered
on, even if the device is taken down. Also, some stupid boot loaders power
on all PHYs available.

This RFC deals with saving power consumed by ethernet PHYs, that have no
corresponding ethernet device driver or are attached to ethernet devices
which are taken down by user request, i.e. ifconfig ethN down. Ports with
no link, i.e. cable removed, are already quite good at power saving due to
PHY internal link detection.

I did some power measurements for 4 different boards with 3 different
drivers (all ARM) and reduction in power dissipation is usually ~500mW
per port. Guruplug savings are a bit high, but while re-testing v3.12
it just crashed because of heat issues - a known problem with this board
so don't take the absolute values too serious.

Now, the reason why I send this as a RFC and not a real patch set is,
that I started with mv643xx_eth which is an "evolutionary piece of code"
and phy support isn't that well done. When touching mvneta/cpsw, I saw
that phy_suspend/resume should possibly better be hidden in phy_start
and phy_stop, respectively. Also, suspending unused PHYs has the potential
to break stuff and I only had some ARM boards available to test.

Anyway, it would be great to get some general feed-back for the current
approach and possible improvements.

The individual patches can be summarized as follows:
Patch 1 adds genphy suspend/resume callbacks to Marvell PHYs.
Patch 2 introduces phy_resume and phy_suspend helpers to ease calling
the corresponding PHY driver callbacks, if available.
Patch 3 calls above helpers to automatically resume PHYs on
phy_attach and suspend them on phy_detach.
Patch 4 adds a late_initcall to suspend unused PHYs that have not been
taken by any driver. This is what we already have for unused clocks in
the common clock framework.
Patch 5, 6, and 7 add phy_resume/suspend to the three individual drivers
on their open/stop callbacks. This suspends the PHY as soon as the ethernet
device is taken down by ifconfig ethN down.

Below are the results of recent linux/master against this RFC. All links
are 1Gbps except Beaglebone Black wich is 100Mbps only.

A branch with the RFC applied can also be found at:
https://github.com/shesselba/linux-dove.git topic/ethphy-power-rfc-v1

Sebastian

(a) SolidRun CuBox, Marvell Dove, Marvell 88E1310, mv643xx_eth
                          v3.12     +RFC diff/abs diff/rel
1 port, link, up        2115 mW  2116 mW  +  1 mW   + 0.0%
1 port, link, down      2104 mW  1572 mW  -532 mW   -25.3%
1 port, no link, up     1600 mW  1614 mW  + 14 mW   + 0.9%
1 port, no link, down   1604 mW  1577 mW  - 27 mW   - 1.7%

(b) Beaglebone Black, TI AM3359, SMSC LAN8710/8720, cpsw (100Mbps)
                          v3.12     +RFC diff/abs diff/rel
1 port, link, up        1247 mW  1248 mW  +  1 mW   + 0.1%
1 port, link, down      1247 mW   808 mW  -439 mW   -35.2%
1 port, no link, up      817 mW   817 mW  +  0 mW   + 0.0%
1 port, no link, down    808 mW   809 mW  +  1 mW   + 0.1%

(c) Globalscale Guruplug, Marvell Kirkwood, Marvell 88E1121, mv643xx_eth
                          v3.12     +RFC diff/abs diff/rel
2 ports, link, up       6611 mW  6587 mW -  24 mW   - 0.4%
2 ports, link, down     6625 mW  3881 mW -2744 mW   -41.4%
2 ports, no link, up    4072 mW  4002 mW -  70 mW   - 1.7%
2 ports, no link, down  4085 mW  3875 mW - 210 mW   - 5.1%
1 port, link, up
1 port, link, down      6622 mW  5193 mW -1429 mW   -21.6%
1 port, link, up
1 port, no link, down   5276 mW  5193 mW -  83 mW   - 1.6%
1 port, no link, up
1 port, no link, down   4029 mW  3941 mW -  88 mW   - 2.2%

(d) Globalscale Mirabox, Marvell Armada 370, Marvell 88E1510, mvneta
                          v3.12     +RFC diff/abs diff/rel
2 ports, link, up       5218 mW  5177 mW  - 41 mW   - 0.8%
2 ports, link, down     5216 mW  4225 mW  -991 mW   -19.0%
2 ports, no link, up    4352 mW  4320 mW  - 32 mW   - 0.7%
2 ports, no link, down  4352 mW  4228 mW  -124 mW   - 2.8%
1 port, link, up
1 port, link, down      5218 mW  4718 mW  -500 mW   - 9.6%
1 port, link, up
1 port, no link, down   4842 mW  4722 mW  -120 mW   - 2.5%
1 port, no link, up
1 port, no link, down   4356 mW  4290 mW  - 66 mW   - 1.5%

Sebastian Hesselbarth (7):
  net: phy: marvell: provide genphy suspend/resume
  net: phy: provide phy_resume/phy_suspend helpers
  net: phy: resume/suspend PHYs on attach/detach
  net: phy: suspend unused PHYs on mdio_bus in late_initcall
  net: mv643xx_eth: resume/suspend PHY on port start/stop
  net: mvneta: resume/suspend PHY on port start/stop
  net: cpsw: resume/suspend PHY on port start/stop

 drivers/net/ethernet/marvell/mv643xx_eth.c |    7 +++++++
 drivers/net/ethernet/marvell/mvneta.c      |    2 ++
 drivers/net/ethernet/ti/cpsw.c             |    2 ++
 drivers/net/phy/marvell.c                  |   22 ++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c                 |   27 +++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c               |   22 ++++++++++++++++++++++
 include/linux/phy.h                        |    2 ++
 7 files changed, 84 insertions(+), 0 deletions(-)

---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
-- 
1.7.2.5

[PATCH RFC v1 1/7] net: phy: marvell: provide genphy suspend/resume

[Sebastian Hesselbarth]

Marvell PHYs support generic PHY suspend/resume, so provide those
callbacks to all marvell specific drivers.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/marvell.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 2e3c778e..bd37e45 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -894,6 +894,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -907,6 +909,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -920,6 +924,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &marvell_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -933,6 +939,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = {.owner = THIS_MODULE,},
 	},
 	{
@@ -946,6 +954,8 @@ static struct phy_driver marvell_drivers[] = {
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -961,6 +971,8 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.get_wol = &m88e1318_get_wol,
 		.set_wol = &m88e1318_set_wol,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -974,6 +986,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -987,6 +1001,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1000,6 +1016,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1013,6 +1031,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1026,6 +1046,8 @@ static struct phy_driver marvell_drivers[] = {
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 };
-- 
1.7.2.5

[PATCH RFC v1 2/7] net: phy: provide phy_resume/phy_suspend helpers

[Sebastian Hesselbarth]

This adds helper functions to resume and suspend a given phy_device
by calling the corresponding driver callbacks if available.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy_device.c |   19 +++++++++++++++++++
 include/linux/phy.h          |    2 ++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74630e9..2442895 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -625,6 +625,25 @@ void phy_detach(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_detach);
 
+int phy_suspend(struct phy_device *phydev)
+{
+	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+
+	if (phydrv->suspend)
+		return phydrv->suspend(phydev);
+	return 0;
+}
+EXPORT_SYMBOL(phy_suspend);
+
+int phy_resume(struct phy_device *phydev)
+{
+	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+
+	if (phydrv->resume)
+		return phydrv->resume(phydev);
+	return 0;
+}
+EXPORT_SYMBOL(phy_resume);
 
 /* Generic PHY support and helper functions */
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 64ab823..ed0d6d8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -552,6 +552,8 @@ void phy_detach(struct phy_device *phydev);
 void phy_start(struct phy_device *phydev);
 void phy_stop(struct phy_device *phydev);
 int phy_start_aneg(struct phy_device *phydev);
+int phy_suspend(struct phy_device *phydev);
+int phy_resume(struct phy_device *phydev);
 
 int phy_stop_interrupts(struct phy_device *phydev);
 
-- 
1.7.2.5

[PATCH RFC v1 3/7] net: phy: resume/suspend PHYs on attach/detach

[Sebastian Hesselbarth]

This ensures PHYs are resumed on attach and suspended on detach.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy_device.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2442895..a421fc6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -570,6 +570,8 @@ static int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (err)
 		phy_detach(phydev);
 
+	phy_resume(phydev);
+
 	return err;
 }
 
@@ -615,6 +617,7 @@ void phy_detach(struct phy_device *phydev)
 {
 	phydev->attached_dev->phydev = NULL;
 	phydev->attached_dev = NULL;
+	phy_suspend(phydev);
 
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
-- 
1.7.2.5

[PATCH RFC v1 4/7] net: phy: suspend unused PHYs on mdio_bus in late_initcall

[Sebastian Hesselbarth]

Since phy_attach ensures PHYs are resumed, we can now suspend all
PHYs that have no attached netdev after initcalls.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/mdio_bus.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 5617876..10eba58 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -320,6 +320,33 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
 		(phydev->phy_id & phydrv->phy_id_mask));
 }
 
+static int mdio_bus_suspend_unused(struct device *busdev, void *data)
+{
+	struct mii_bus *bus = to_mii_bus(busdev);
+	struct phy_device *phydev;
+	struct phy_driver *phydrv;
+	int i;
+
+	for (i = 0; i < PHY_MAX_ADDR; i++) {
+		if (!bus->phy_map[i])
+			continue;
+
+		phydev = to_phy_device(&bus->phy_map[i]->dev);
+		phydrv = to_phy_driver(phydev->dev.driver);
+		if (!phydev->attached_dev && phydrv && phydrv->suspend)
+			phy_suspend(phydev);
+	}
+
+	return 0;
+}
+
+static int mdio_bus_class_suspend_unused(void)
+{
+	return class_for_each_device(&mdio_bus_class, NULL, NULL,
+				     mdio_bus_suspend_unused);
+}
+late_initcall_sync(mdio_bus_class_suspend_unused);
+
 #ifdef CONFIG_PM
 
 static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
-- 
1.7.2.5

[PATCH RFC v1 5/7] net: mv643xx_eth: resume/suspend PHY on port start/stop

[Sebastian Hesselbarth]

Network PHYs consume a noticable amount of power. This adds phy_resume
on port start and phy_suspend on port stop to save this power if the
port is down anyway. While at it, also properly start/stop the phy.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/marvell/mv643xx_eth.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 00cd36e..55805b2 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2097,6 +2097,8 @@ static void port_start(struct mv643xx_eth_private *mp)
 
 		mv643xx_eth_get_settings(mp->dev, &cmd);
 		phy_reset(mp);
+		phy_resume(mp->phy);
+		phy_start(mp->phy);
 		mv643xx_eth_set_settings(mp->dev, &cmd);
 	}
 
@@ -2306,6 +2308,11 @@ static int mv643xx_eth_stop(struct net_device *dev)
 	for (i = 0; i < mp->txq_count; i++)
 		txq_deinit(mp->txq + i);
 
+	if (mp->phy) {
+		phy_stop(mp->phy);
+		phy_suspend(mp->phy);
+	}
+
 	return 0;
 }
 
-- 
1.7.2.5

[PATCH RFC v1 6/7] net: mvneta: resume/suspend PHY on port start/stop

[Sebastian Hesselbarth]

Network PHYs consume a noticable amount of power. This adds phy_resume
on start_dev and phy_suspend on stop_dev to save this power if the
port is down anyway.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/marvell/mvneta.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index b8e232b..8688e91 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2194,6 +2194,7 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 	mvreg_write(pp, MVNETA_INTR_NEW_MASK,
 		    MVNETA_RX_INTR_MASK(rxq_number));
 
+	phy_resume(pp->phy_dev);
 	phy_start(pp->phy_dev);
 	netif_tx_start_all_queues(pp->dev);
 }
@@ -2201,6 +2202,7 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 static void mvneta_stop_dev(struct mvneta_port *pp)
 {
 	phy_stop(pp->phy_dev);
+	phy_suspend(pp->phy_dev);
 
 	napi_disable(&pp->napi);
 
-- 
1.7.2.5

[PATCH RFC v1 7/7] net: cpsw: resume/suspend PHY on port start/stop

[Sebastian Hesselbarth]

Network PHYs consume a noticable amount of power. This adds phy_resume
on slave_open and phy_suspend on slave_stop to save this power if the
port is down anyway.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/ti/cpsw.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 90d41d2..f1dc54f 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1013,6 +1013,7 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
 	} else {
 		dev_info(priv->dev, "phy found : id is : 0x%x\n",
 			 slave->phy->phy_id);
+		phy_resume(slave->phy);
 		phy_start(slave->phy);
 
 		/* Configure GMII_SEL register */
@@ -1081,6 +1082,7 @@ static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
 	if (!slave->phy)
 		return;
 	phy_stop(slave->phy);
+	phy_suspend(slave->phy);
 	phy_disconnect(slave->phy);
 	slave->phy = NULL;
 }
-- 
1.7.2.5

Re: [PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Wed, 20 Nov 2013 21:21:46 +0100

> Ethernet PHYs consume a significant amount of power when link is detected.
> Especially, for embedded systems it can be easily 20-40% of total system
> power. Now, currently most likely all ethernet drivers leave PHYs powered
> on, even if the device is taken down. Also, some stupid boot loaders power
> on all PHYs available.
> 
> This RFC deals with saving power consumed by ethernet PHYs, that have no
> corresponding ethernet device driver or are attached to ethernet devices
> which are taken down by user request, i.e. ifconfig ethN down. Ports with
> no link, i.e. cable removed, are already quite good at power saving due to
> PHY internal link detection.

The idea is sound and the goal is of course valuable, but it brings up
a chronically reoccurring issue as of late.

You cannot reset the PHY or take it down without somehow retaining the
settings the PHY had when you bring it back up.

If I ifdown/ifup a device, my ethtool link configuration better be
retained.

This means the PHY layer must have a way to reprogram the device when
it is brought back up, with whatever settings the software state
things are there.

Re: [PATCH RFC v1 2/7] net: phy: provide phy_resume/phy_suspend helpers

[Florian Fainelli]

2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> This adds helper functions to resume and suspend a given phy_device
> by calling the corresponding driver callbacks if available.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

Looks good, thanks Sebastian:

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
> Cc: David S. Miller <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/net/phy/phy_device.c |   19 +++++++++++++++++++
>  include/linux/phy.h          |    2 ++
>  2 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 74630e9..2442895 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -625,6 +625,25 @@ void phy_detach(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(phy_detach);
>
> +int phy_suspend(struct phy_device *phydev)
> +{
> +       struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
> +
> +       if (phydrv->suspend)
> +               return phydrv->suspend(phydev);
> +       return 0;
> +}
> +EXPORT_SYMBOL(phy_suspend);
> +
> +int phy_resume(struct phy_device *phydev)
> +{
> +       struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
> +
> +       if (phydrv->resume)
> +               return phydrv->resume(phydev);
> +       return 0;
> +}
> +EXPORT_SYMBOL(phy_resume);
>
>  /* Generic PHY support and helper functions */
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 64ab823..ed0d6d8 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -552,6 +552,8 @@ void phy_detach(struct phy_device *phydev);
>  void phy_start(struct phy_device *phydev);
>  void phy_stop(struct phy_device *phydev);
>  int phy_start_aneg(struct phy_device *phydev);
> +int phy_suspend(struct phy_device *phydev);
> +int phy_resume(struct phy_device *phydev);
>
>  int phy_stop_interrupts(struct phy_device *phydev);
>
> --
> 1.7.2.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Florian

Re: [PATCH RFC v1 7/7] net: cpsw: resume/suspend PHY on port start/stop

[Florian Fainelli]

2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> Network PHYs consume a noticable amount of power. This adds phy_resume
> on slave_open and phy_suspend on slave_stop to save this power if the
> port is down anyway.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Cc: David S. Miller <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/net/ethernet/ti/cpsw.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 90d41d2..f1dc54f 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -1013,6 +1013,7 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
>         } else {
>                 dev_info(priv->dev, "phy found : id is : 0x%x\n",
>                          slave->phy->phy_id);
> +               phy_resume(slave->phy);
>                 phy_start(slave->phy);

Cannot phy_start() figure this out for us based on the internal PHY
device state machine?

I would imagine that you would want to call phy_suspend/resume from an
Ethernet driver's PM suspend/resume callbacks to make sure that the
PHY also enters a low power mode, but I do not want to have to
remember that I need to call phy_resume before phy_start for instance.
As of today we have PHY_HALTED/PHY_RESUMING, and I think we certainly
need at least a PHY_SUSPENDED state to help us with that.

>
>                 /* Configure GMII_SEL register */
> @@ -1081,6 +1082,7 @@ static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
>         if (!slave->phy)
>                 return;
>         phy_stop(slave->phy);
> +       phy_suspend(slave->phy);

Same here, why is not that hidden in phy_stop? If there is any fear
that this breaks setups where WoL is used or such, we could add a new
argument to phy_connect() and friends which says whether it is okay to
auto-suspend the PHY upon PHY_stop

>         phy_disconnect(slave->phy);
>         slave->phy = NULL;
>  }
> --
> 1.7.2.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Florian

Re: [PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization

[Sebastian Hesselbarth]

On 11/20/2013 09:36 PM, David Miller wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Date: Wed, 20 Nov 2013 21:21:46 +0100
>
>> Ethernet PHYs consume a significant amount of power when link is detected.
>> Especially, for embedded systems it can be easily 20-40% of total system
>> power. Now, currently most likely all ethernet drivers leave PHYs powered
>> on, even if the device is taken down. Also, some stupid boot loaders power
>> on all PHYs available.
>>
>> This RFC deals with saving power consumed by ethernet PHYs, that have no
>> corresponding ethernet device driver or are attached to ethernet devices
>> which are taken down by user request, i.e. ifconfig ethN down. Ports with
>> no link, i.e. cable removed, are already quite good at power saving due to
>> PHY internal link detection.
>
> The idea is sound and the goal is of course valuable, but it brings up
> a chronically reoccurring issue as of late.
>
> You cannot reset the PHY or take it down without somehow retaining the
> settings the PHY had when you bring it back up.

Right, as far as I understand BMCR powerdown, i.e. what is called in
genphy_suspend/resume, powers down the PHY but _does_ retain PHY config.
It is not resetting the device.

I haven't checked a lot of datasheets but [1] notes that "registers will
preserve their configuration". Even if we have PHYs that do not preserve
it, they should have a device specific callback for suspend/resume that
takes care of preserving it.

[1] http://www.ti.com/lit/an/snoa463a/snoa463a.pdf

> If I ifdown/ifup a device, my ethtool link configuration better be
> retained.
>
> This means the PHY layer must have a way to reprogram the device when
> it is brought back up, with whatever settings the software state
> things are there.
>

Re: [PATCH RFC v1 7/7] net: cpsw: resume/suspend PHY on port start/stop

[Sebastian Hesselbarth]

On 11/20/2013 09:48 PM, Florian Fainelli wrote:
> 2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
>> Network PHYs consume a noticable amount of power. This adds phy_resume
>> on slave_open and phy_suspend on slave_stop to save this power if the
>> port is down anyway.
>>
>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> ---
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   drivers/net/ethernet/ti/cpsw.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> index 90d41d2..f1dc54f 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -1013,6 +1013,7 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
>>          } else {
>>                  dev_info(priv->dev, "phy found : id is : 0x%x\n",
>>                           slave->phy->phy_id);
>> +               phy_resume(slave->phy);
>>                  phy_start(slave->phy);
>
> Cannot phy_start() figure this out for us based on the internal PHY
> device state machine?

Yeah, as I said in the cover letter, I started with mv643xx_eth which
is not using phy_start/stop as it probably should be. As soon as I
looked at mvneta/cpsw, I also came to the conclusion that
phy_suspend/resume should be hidden in phy_start/stop.

> I would imagine that you would want to call phy_suspend/resume from an
> Ethernet driver's PM suspend/resume callbacks to make sure that the
> PHY also enters a low power mode, but I do not want to have to
> remember that I need to call phy_resume before phy_start for instance.
> As of today we have PHY_HALTED/PHY_RESUMING, and I think we certainly
> need at least a PHY_SUSPENDED state to help us with that.
>
>>
>>                  /* Configure GMII_SEL register */
>> @@ -1081,6 +1082,7 @@ static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
>>          if (!slave->phy)
>>                  return;
>>          phy_stop(slave->phy);
>> +       phy_suspend(slave->phy);
>
> Same here, why is not that hidden in phy_stop? If there is any fear
> that this breaks setups where WoL is used or such, we could add a new
> argument to phy_connect() and friends which says whether it is okay to
> auto-suspend the PHY upon PHY_stop
>
>>          phy_disconnect(slave->phy);
>>          slave->phy = NULL;
>>   }
>> --
>> 1.7.2.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>

Re: [PATCH RFC v1 4/7] net: phy: suspend unused PHYs on mdio_bus in late_initcall

[Florian Fainelli]

2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> Since phy_attach ensures PHYs are resumed, we can now suspend all
> PHYs that have no attached netdev after initcalls.

I do like the idea, but I think you might want to make sure that the
MDIO bus suspend policy was set to "auto" (which is the default afair)
not to expose unexpected behavior.

>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Cc: David S. Miller <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/net/phy/mdio_bus.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 5617876..10eba58 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -320,6 +320,33 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
>                 (phydev->phy_id & phydrv->phy_id_mask));
>  }
>
> +static int mdio_bus_suspend_unused(struct device *busdev, void *data)
> +{
> +       struct mii_bus *bus = to_mii_bus(busdev);
> +       struct phy_device *phydev;
> +       struct phy_driver *phydrv;
> +       int i;
> +
> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
> +               if (!bus->phy_map[i])
> +                       continue;
> +
> +               phydev = to_phy_device(&bus->phy_map[i]->dev);
> +               phydrv = to_phy_driver(phydev->dev.driver);
> +               if (!phydev->attached_dev && phydrv && phydrv->suspend)
> +                       phy_suspend(phydev);
> +       }
> +
> +       return 0;

You might want to reuse mdio_bus_phy_may_suspend() here to have a
central place checking for phydev->attached_dev and phydrv->suspend
just in case we need to add more callbacks in the future or implicit
PHY state machine hooks. That might also take care of my concern
expressed above.

> +}
> +
> +static int mdio_bus_class_suspend_unused(void)
> +{
> +       return class_for_each_device(&mdio_bus_class, NULL, NULL,
> +                                    mdio_bus_suspend_unused);
> +}
> +late_initcall_sync(mdio_bus_class_suspend_unused);
> +
>  #ifdef CONFIG_PM
>
>  static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
> --
> 1.7.2.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Florian

Re: [PATCH RFC v1 4/7] net: phy: suspend unused PHYs on mdio_bus in late_initcall

[Sebastian Hesselbarth]

On 11/20/2013 09:58 PM, Florian Fainelli wrote:
> 2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
>> Since phy_attach ensures PHYs are resumed, we can now suspend all
>> PHYs that have no attached netdev after initcalls.
>
> I do like the idea, but I think you might want to make sure that the
> MDIO bus suspend policy was set to "auto" (which is the default afair)
> not to expose unexpected behavior.

Ok, TBH I haven't looked through all of phy internals. If we are fine
with the overall approach, I could use some guidance in the individual
patches and policies.

>>
>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> ---
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   drivers/net/phy/mdio_bus.c |   27 +++++++++++++++++++++++++++
>>   1 files changed, 27 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index 5617876..10eba58 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>> @@ -320,6 +320,33 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
>>                  (phydev->phy_id & phydrv->phy_id_mask));
>>   }
>>
>> +static int mdio_bus_suspend_unused(struct device *busdev, void *data)
>> +{
>> +       struct mii_bus *bus = to_mii_bus(busdev);
>> +       struct phy_device *phydev;
>> +       struct phy_driver *phydrv;
>> +       int i;
>> +
>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
>> +               if (!bus->phy_map[i])
>> +                       continue;
>> +
>> +               phydev = to_phy_device(&bus->phy_map[i]->dev);
>> +               phydrv = to_phy_driver(phydev->dev.driver);
>> +               if (!phydev->attached_dev && phydrv && phydrv->suspend)
>> +                       phy_suspend(phydev);
>> +       }
>> +
>> +       return 0;
>
> You might want to reuse mdio_bus_phy_may_suspend() here to have a
> central place checking for phydev->attached_dev and phydrv->suspend
> just in case we need to add more callbacks in the future or implicit
> PHY state machine hooks. That might also take care of my concern
> expressed above.

Unfortunately, mdio_bus_phy_may_suspend() doesn't help here. It will
correctly tell that unconnected PHYs may_suspend but also that PHYs
connected to PM aware drivers may_suspend.

I tried it and it will also suspend the PHY taken by mv643xx_eth.

But actually, as phy_suspend/resume check for phydrv->suspend
themselves, we can just call it on !phydev->attached_dev and remove
the additional checks.

>> +}
>> +
>> +static int mdio_bus_class_suspend_unused(void)
>> +{
>> +       return class_for_each_device(&mdio_bus_class, NULL, NULL,
>> +                                    mdio_bus_suspend_unused);
>> +}
>> +late_initcall_sync(mdio_bus_class_suspend_unused);
>> +
>>   #ifdef CONFIG_PM
>>
>>   static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
>> --
>> 1.7.2.5
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>

Re: [PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization

[Florian Fainelli]

2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> On 11/20/2013 09:36 PM, David Miller wrote:
>>
>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Date: Wed, 20 Nov 2013 21:21:46 +0100
>>
>>> Ethernet PHYs consume a significant amount of power when link is
>>> detected.
>>> Especially, for embedded systems it can be easily 20-40% of total system
>>> power. Now, currently most likely all ethernet drivers leave PHYs powered
>>> on, even if the device is taken down. Also, some stupid boot loaders
>>> power
>>> on all PHYs available.
>>>
>>> This RFC deals with saving power consumed by ethernet PHYs, that have no
>>> corresponding ethernet device driver or are attached to ethernet devices
>>> which are taken down by user request, i.e. ifconfig ethN down. Ports with
>>> no link, i.e. cable removed, are already quite good at power saving due
>>> to
>>> PHY internal link detection.
>>
>>
>> The idea is sound and the goal is of course valuable, but it brings up
>> a chronically reoccurring issue as of late.
>>
>> You cannot reset the PHY or take it down without somehow retaining the
>> settings the PHY had when you bring it back up.
>
>
> Right, as far as I understand BMCR powerdown, i.e. what is called in
> genphy_suspend/resume, powers down the PHY but _does_ retain PHY config.
> It is not resetting the device.

Right that's also my understanding of how BMCR powerdown works. That
said, I am relatively sure that we can find PHY devices for which this
is not true.

As for the PHY state machine, I think we need a new state
PHY_SUSPENDED  and upon calling phy_start() we make sure that we treat
PHY_SUSPENDED just like we treat PHY_HALTED today since that would
make sure that the PHY parameters are applied correctly upon resume
(interrupt configuration, autoneg and and such).

There is still some discussion on how we should deal with
auto-suspending the PHY when phy_stop() is called, and how does that
differ from the PHY_HALTED state? So this also raises the question of
whether PHY_HALTED is really different from PHY_SUSPENDED. The only
difference with your patches would be that we have put the PHY into a
low-power mode.

>
> I haven't checked a lot of datasheets but [1] notes that "registers will
> preserve their configuration". Even if we have PHYs that do not preserve
> it, they should have a device specific callback for suspend/resume that
> takes care of preserving it.

Right, but the PHY driver should only take care of restoring "state
less" PHY context, while the PHY state machine has to restore a "state
aware" PHY device context. So for quirky PHY chips, or those having
advanced power management features, we definitively need the two to be
helping each other.

>
> [1] http://www.ti.com/lit/an/snoa463a/snoa463a.pdf
>
>
>> If I ifdown/ifup a device, my ethtool link configuration better be
>> retained.
>>
>> This means the PHY layer must have a way to reprogram the device when
>> it is brought back up, with whatever settings the software state
>> things are there.
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Florian

Re: [PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization

[Sebastian Hesselbarth]

On 11/20/2013 10:10 PM, Florian Fainelli wrote:
> 2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
>> On 11/20/2013 09:36 PM, David Miller wrote:
>>>
>>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> Date: Wed, 20 Nov 2013 21:21:46 +0100
>>>
>>>> Ethernet PHYs consume a significant amount of power when link is
>>>> detected.
>>>> Especially, for embedded systems it can be easily 20-40% of total system
>>>> power. Now, currently most likely all ethernet drivers leave PHYs powered
>>>> on, even if the device is taken down. Also, some stupid boot loaders
>>>> power
>>>> on all PHYs available.
>>>>
>>>> This RFC deals with saving power consumed by ethernet PHYs, that have no
>>>> corresponding ethernet device driver or are attached to ethernet devices
>>>> which are taken down by user request, i.e. ifconfig ethN down. Ports with
>>>> no link, i.e. cable removed, are already quite good at power saving due
>>>> to
>>>> PHY internal link detection.
>>>
>>>
>>> The idea is sound and the goal is of course valuable, but it brings up
>>> a chronically reoccurring issue as of late.
>>>
>>> You cannot reset the PHY or take it down without somehow retaining the
>>> settings the PHY had when you bring it back up.
>>
>>
>> Right, as far as I understand BMCR powerdown, i.e. what is called in
>> genphy_suspend/resume, powers down the PHY but _does_ retain PHY config.
>> It is not resetting the device.
>
> Right that's also my understanding of how BMCR powerdown works. That
> said, I am relatively sure that we can find PHY devices for which this
> is not true.

No doubt.

> As for the PHY state machine, I think we need a new state
> PHY_SUSPENDED  and upon calling phy_start() we make sure that we treat
> PHY_SUSPENDED just like we treat PHY_HALTED today since that would
> make sure that the PHY parameters are applied correctly upon resume
> (interrupt configuration, autoneg and and such).
>
> There is still some discussion on how we should deal with
> auto-suspending the PHY when phy_stop() is called, and how does that
> differ from the PHY_HALTED state? So this also raises the question of
> whether PHY_HALTED is really different from PHY_SUSPENDED. The only
> difference with your patches would be that we have put the PHY into a
> low-power mode.

Well, I haven't thought about WoL and stuff and how the driver should
leave the PHY for that to work. Suspend support isn't really spread
among ARM SoCs :P

But if you can run some tests on non-ARM platforms you might already
been testing with, I am sure we can work it out. Maybe, we just have
PHY_SUSPENDED and deal with it differently if any regressions pop up?

Also, suspend_unused and auto-suspend on phy_stop can be disabled by
default for some kernel versions until we have enough coverage?

>> I haven't checked a lot of datasheets but [1] notes that "registers will
>> preserve their configuration". Even if we have PHYs that do not preserve
>> it, they should have a device specific callback for suspend/resume that
>> takes care of preserving it.
>
> Right, but the PHY driver should only take care of restoring "state
> less" PHY context, while the PHY state machine has to restore a "state
> aware" PHY device context. So for quirky PHY chips, or those having
> advanced power management features, we definitively need the two to be
> helping each other.

I see it was a good idea to send the RFC early. The savings are
impressive but I was already quite sure that it has the potential to
break (at least) the quirky PHYs.

Sebastian

>>
>> [1] http://www.ti.com/lit/an/snoa463a/snoa463a.pdf
>>
>>
>>> If I ifdown/ifup a device, my ethtool link configuration better be
>>> retained.
>>>
>>> This means the PHY layer must have a way to reprogram the device when
>>> it is brought back up, with whatever settings the software state
>>> things are there.
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>

Re: [PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Wed, 20 Nov 2013 21:54:19 +0100

> Right, as far as I understand BMCR powerdown, i.e. what is called in
> genphy_suspend/resume, powers down the PHY but _does_ retain PHY
> config.
> It is not resetting the device.
> 
> I haven't checked a lot of datasheets but [1] notes that "registers
> will
> preserve their configuration". Even if we have PHYs that do not
> preserve
> it, they should have a device specific callback for suspend/resume
> that
> takes care of preserving it.
> 
> [1] http://www.ti.com/lit/an/snoa463a/snoa463a.pdf

Thanks, that addresses my concerns.

Re: [PATCH RFC v1 4/7] net: phy: suspend unused PHYs on mdio_bus in late_initcall

[Florian Fainelli]

2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> On 11/20/2013 09:58 PM, Florian Fainelli wrote:
>>
>> 2013/11/20 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
>>>
>>> Since phy_attach ensures PHYs are resumed, we can now suspend all
>>> PHYs that have no attached netdev after initcalls.
>>
>>
>> I do like the idea, but I think you might want to make sure that the
>> MDIO bus suspend policy was set to "auto" (which is the default afair)
>> not to expose unexpected behavior.
>
>
> Ok, TBH I haven't looked through all of phy internals. If we are fine
> with the overall approach, I could use some guidance in the individual
> patches and policies.
>
>
>>>
>>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> ---
>>> Cc: David S. Miller <davem@davemloft.net>
>>> Cc: netdev@vger.kernel.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>>   drivers/net/phy/mdio_bus.c |   27 +++++++++++++++++++++++++++
>>>   1 files changed, 27 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>>> index 5617876..10eba58 100644
>>> --- a/drivers/net/phy/mdio_bus.c
>>> +++ b/drivers/net/phy/mdio_bus.c
>>> @@ -320,6 +320,33 @@ static int mdio_bus_match(struct device *dev, struct
>>> device_driver *drv)
>>>                  (phydev->phy_id & phydrv->phy_id_mask));
>>>   }
>>>
>>> +static int mdio_bus_suspend_unused(struct device *busdev, void *data)
>>> +{
>>> +       struct mii_bus *bus = to_mii_bus(busdev);
>>> +       struct phy_device *phydev;
>>> +       struct phy_driver *phydrv;
>>> +       int i;
>>> +
>>> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
>>> +               if (!bus->phy_map[i])
>>> +                       continue;
>>> +
>>> +               phydev = to_phy_device(&bus->phy_map[i]->dev);
>>> +               phydrv = to_phy_driver(phydev->dev.driver);
>>> +               if (!phydev->attached_dev && phydrv && phydrv->suspend)
>>> +                       phy_suspend(phydev);
>>> +       }
>>> +
>>> +       return 0;
>>
>>
>> You might want to reuse mdio_bus_phy_may_suspend() here to have a
>> central place checking for phydev->attached_dev and phydrv->suspend
>> just in case we need to add more callbacks in the future or implicit
>> PHY state machine hooks. That might also take care of my concern
>> expressed above.
>
>
> Unfortunately, mdio_bus_phy_may_suspend() doesn't help here. It will
> correctly tell that unconnected PHYs may_suspend but also that PHYs
> connected to PM aware drivers may_suspend.

I agree for the first part which is why I was suggesting it, we are
sure we will return early because of the if (!netdev) branch.

>
> I tried it and it will also suspend the PHY taken by mv643xx_eth.
>
> But actually, as phy_suspend/resume check for phydrv->suspend
> themselves, we can just call it on !phydev->attached_dev and remove
> the additional checks.

Sure, sounds good.
-- 
Florian

Re: [PATCH RFC v1 7/7] net: cpsw: resume/suspend PHY on port start/stop

[Mugunthan V N]

On Thursday 21 November 2013 01:51 AM, Sebastian Hesselbarth wrote:
> Network PHYs consume a noticable amount of power. This adds phy_resume
> on slave_open and phy_suspend on slave_stop to save this power if the
> port is down anyway.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Cc: David S. Miller <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/net/ethernet/ti/cpsw.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 90d41d2..f1dc54f 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -1013,6 +1013,7 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
>  	} else {
>  		dev_info(priv->dev, "phy found : id is : 0x%x\n",
>  			 slave->phy->phy_id);
> +		phy_resume(slave->phy);
>  		phy_start(slave->phy);
>  
>  		/* Configure GMII_SEL register */
> @@ -1081,6 +1082,7 @@ static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
>  	if (!slave->phy)
>  		return;
>  	phy_stop(slave->phy);
> +	phy_suspend(slave->phy);
>  	phy_disconnect(slave->phy);
>  	slave->phy = NULL;
>  }

Can these be called from phy_start and phy_stop itself so that it
reduces the effort of patching all drivers and also applies to all
etherent drivers.

Regards
Mugunthan V N

Re: [PATCH RFC v1 7/7] net: cpsw: resume/suspend PHY on port start/stop

[Sebastian Hesselbarth]

On 11/21/2013 09:35 AM, Mugunthan V N wrote:
> On Thursday 21 November 2013 01:51 AM, Sebastian Hesselbarth wrote:
>> Network PHYs consume a noticable amount of power. This adds phy_resume
>> on slave_open and phy_suspend on slave_stop to save this power if the
>> port is down anyway.
>>
>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> ---
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   drivers/net/ethernet/ti/cpsw.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> index 90d41d2..f1dc54f 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -1013,6 +1013,7 @@ static void cpsw_slave_open(struct cpsw_slave *slave, struct cpsw_priv *priv)
>>   	} else {
>>   		dev_info(priv->dev, "phy found : id is : 0x%x\n",
>>   			 slave->phy->phy_id);
>> +		phy_resume(slave->phy);
>>   		phy_start(slave->phy);
>>
>>   		/* Configure GMII_SEL register */
>> @@ -1081,6 +1082,7 @@ static void cpsw_slave_stop(struct cpsw_slave *slave, struct cpsw_priv *priv)
>>   	if (!slave->phy)
>>   		return;
>>   	phy_stop(slave->phy);
>> +	phy_suspend(slave->phy);
>>   	phy_disconnect(slave->phy);
>>   	slave->phy = NULL;
>>   }
>
> Can these be called from phy_start and phy_stop itself so that it
> reduces the effort of patching all drivers and also applies to all
> etherent drivers.

Mugunthan,

Florian already mentioned that and I was already guessing that it will
be better hidden within the PHY state machine, too.

My current idea is to have a new state, that Florian also mentioned,
with the following behavior: On phy_stop the PHY will drop down to
suspend if (a) the PHY driver provides a suspend callback and (b) if
there is nothing enabled that requires the PHY to remain powered, e.g.
WoL.

Anyway, I'll have to dig into PHY state machine internals for that
first.

Sebastian

[PATCH RFCv2 0/6] net: phy: Ethernet PHY powerdown optimization

[Sebastian Hesselbarth]

This is v2 of an RFC sent earlier [1] to reduce power consumption of network
PHYs with link that are either unused or the corresponding netdev is down.

In contrast to RFCv1, this now integrates phy_suspend/phy_resume transparent
to the netdev drivers. Also, phy_suspend now only suspends the PHY if WOL is
disabled. Moreover, the phy state machine calls phy_suspend on entering
HALTED state.

Again, a branch with RFCv2 applied to v3.13-rc2 can also be found at
https://github.com/shesselba/linux-dove.git topic/ethphy-power-rfc-v2

[1] http://lwn.net/Articles/574426/

Sebastian Hesselbarth (6):
  net: mv643xx_eth: properly start/stop phy device
  net: phy: marvell: provide genphy suspend/resume
  net: phy: provide phy_resume/phy_suspend helpers
  net: phy: resume/suspend PHYs on attach/detach
  net: phy: suspend unused PHYs on mdio_bus in late_initcall
  net: phy: suspend phydev when going to HALTED

 drivers/net/ethernet/marvell/mv643xx_eth.c |    4 +++-
 drivers/net/phy/marvell.c                  |   22 ++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c                 |   25 +++++++++++++++++++++++++
 drivers/net/phy/phy.c                      |    6 +++++-
 drivers/net/phy/phy_device.c               |   27 +++++++++++++++++++++++++++
 include/linux/phy.h                        |    2 ++
 6 files changed, 84 insertions(+), 2 deletions(-)

---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
-- 
1.7.2.5

[PATCH RFCv2 1/6] net: mv643xx_eth: properly start/stop phy device

[Sebastian Hesselbarth]

When using phydev, it should be phy_start/phy_stop'ed properly. This
driver doesn't do that, so add the corresponding calls to port_start/
stop respectively.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
RFCv1->RFCv2:
- initial, derived from netdev specific patches
  (Suggested by Mugunthan and Florian)

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/marvell/mv643xx_eth.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 61088a6..3aa706f 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2098,6 +2098,7 @@ static void port_start(struct mv643xx_eth_private *mp)
 		mv643xx_eth_get_settings(mp->dev, &cmd);
 		phy_reset(mp);
 		mv643xx_eth_set_settings(mp->dev, &cmd);
+		phy_start(mp->phy);
 	}
 
 	/*
@@ -2293,7 +2294,8 @@ static int mv643xx_eth_stop(struct net_device *dev)
 	del_timer_sync(&mp->rx_oom);
 
 	netif_carrier_off(dev);
-
+	if (mp->phy)
+		phy_stop(mp->phy);
 	free_irq(dev->irq, dev);
 
 	port_reset(mp);
-- 
1.7.2.5

[PATCH RFCv2 2/6] net: phy: marvell: provide genphy suspend/resume

[Sebastian Hesselbarth]

Marvell PHYs support generic PHY suspend/resume, so provide those
callbacks to all marvell specific drivers.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
RFCv1->RFCv2:
- none

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/marvell.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 2e3c778e..bd37e45 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -894,6 +894,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -907,6 +909,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -920,6 +924,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &marvell_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -933,6 +939,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = {.owner = THIS_MODULE,},
 	},
 	{
@@ -946,6 +954,8 @@ static struct phy_driver marvell_drivers[] = {
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -961,6 +971,8 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.get_wol = &m88e1318_get_wol,
 		.set_wol = &m88e1318_set_wol,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -974,6 +986,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -987,6 +1001,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1000,6 +1016,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1013,6 +1031,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1026,6 +1046,8 @@ static struct phy_driver marvell_drivers[] = {
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 };
-- 
1.7.2.5

[PATCH RFCv2 3/6] net: phy: provide phy_resume/phy_suspend helpers

[Sebastian Hesselbarth]

This adds helper functions to resume and suspend a given phy_device
by calling the corresponding driver callbacks if available.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
RFCv1->RFCv2:
- only suspend if WOL is not enabled
- remove EXPORT_SYMBOL for phy_suspend/resume

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy_device.c |   24 ++++++++++++++++++++++++
 include/linux/phy.h          |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d6447b3..6f0e9e4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -625,6 +625,30 @@ void phy_detach(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_detach);
 
+int phy_suspend(struct phy_device *phydev)
+{
+	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+	struct ethtool_wolinfo wol;
+
+	/* If the device has WOL enabled, we cannot suspend the PHY */
+	wol.cmd = ETHTOOL_GWOL;
+	phy_ethtool_get_wol(phydev, &wol);
+	if (wol.wolopts)
+		return -EBUSY;
+
+	if (phydrv->suspend)
+		return phydrv->suspend(phydev);
+	return 0;
+}
+
+int phy_resume(struct phy_device *phydev)
+{
+	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+
+	if (phydrv->resume)
+		return phydrv->resume(phydev);
+	return 0;
+}
 
 /* Generic PHY support and helper functions */
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 48a4dc3..1070ee3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -538,6 +538,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
 int phy_init_hw(struct phy_device *phydev);
+int phy_suspend(struct phy_device *phydev);
+int phy_resume(struct phy_device *phydev);
 struct phy_device * phy_attach(struct net_device *dev,
 		const char *bus_id, phy_interface_t interface);
 struct phy_device *phy_find_first(struct mii_bus *bus);
-- 
1.7.2.5

[PATCH RFCv2 4/6] net: phy: resume/suspend PHYs on attach/detach

[Sebastian Hesselbarth]

This ensures PHYs are resumed on attach and suspended on detach.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
RFCv1->RFCv2:
- none

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy_device.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6f0e9e4..3903f44 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -570,6 +570,8 @@ static int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (err)
 		phy_detach(phydev);
 
+	phy_resume(phydev);
+
 	return err;
 }
 
@@ -615,6 +617,7 @@ void phy_detach(struct phy_device *phydev)
 {
 	phydev->attached_dev->phydev = NULL;
 	phydev->attached_dev = NULL;
+	phy_suspend(phydev);
 
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
-- 
1.7.2.5

[PATCH RFCv2 5/6] net: phy: suspend unused PHYs on mdio_bus in late_initcall

[Sebastian Hesselbarth]

Since phy_attach ensures PHYs are resumed, we can now suspend all
PHYs that have no attached netdev after initcalls.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
RFCv1->RFCv2:
- only check for phydev->attached_dev and let phy_suspend decide on
  performing suspend or not (Suggested by Florian)

@Florian: You suggested to 'make sure that the MDIO bus suspend policy
was set to "auto"'. I wasn't able to find any clue how to check that,
so I ignored that for now.

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/mdio_bus.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 5617876..f533d17 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -320,6 +320,31 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
 		(phydev->phy_id & phydrv->phy_id_mask));
 }
 
+static int mdio_bus_suspend_unused(struct device *busdev, void *data)
+{
+	struct mii_bus *bus = to_mii_bus(busdev);
+	struct phy_device *phydev;
+	int i;
+
+	for (i = 0; i < PHY_MAX_ADDR; i++) {
+		if (!bus->phy_map[i])
+			continue;
+
+		phydev = to_phy_device(&bus->phy_map[i]->dev);
+		if (!phydev->attached_dev)
+			phy_suspend(phydev);
+	}
+
+	return 0;
+}
+
+static int mdio_bus_class_suspend_unused(void)
+{
+	return class_for_each_device(&mdio_bus_class, NULL, NULL,
+				     mdio_bus_suspend_unused);
+}
+late_initcall_sync(mdio_bus_class_suspend_unused);
+
 #ifdef CONFIG_PM
 
 static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
-- 
1.7.2.5

[PATCH RFCv2 6/6] net: phy: suspend phydev when going to HALTED

[Sebastian Hesselbarth]

When phydev is going to HALTED state, we can try to suspend it to
safe more power. phy_suspend helper will check if PHY can be suspended,
so just call it when entering HALTED state.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
RFCv1->RFCv2:
- initial, integrate phy_suspend to phy state machine
  (Suggested by Mugunthan and Florian)

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 36c6994..6429942 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -740,7 +740,7 @@ void phy_state_machine(struct work_struct *work)
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct phy_device *phydev =
 			container_of(dwork, struct phy_device, state_queue);
-	int needs_aneg = 0;
+	int needs_aneg = 0, do_suspend = 0;
 	int err = 0;
 
 	mutex_lock(&phydev->lock);
@@ -855,6 +855,7 @@ void phy_state_machine(struct work_struct *work)
 				phydev->link = 0;
 				netif_carrier_off(phydev->attached_dev);
 				phydev->adjust_link(phydev->attached_dev);
+				do_suspend = 1;
 			}
 			break;
 		case PHY_RESUMING:
@@ -913,6 +914,9 @@ void phy_state_machine(struct work_struct *work)
 	if (needs_aneg)
 		err = phy_start_aneg(phydev);
 
+	if (do_suspend)
+		phy_suspend(phydev);
+
 	if (err < 0)
 		phy_error(phydev);
 
-- 
1.7.2.5

Re: [PATCH RFCv2 5/6] net: phy: suspend unused PHYs on mdio_bus in late_initcall

[Sergei Shtylyov]

Hello.

On 12/04/2013 06:44 PM, Sebastian Hesselbarth wrote:

> Since phy_attach ensures PHYs are resumed, we can now suspend all
> PHYs that have no attached netdev after initcalls.

> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Changelog:
> RFCv1->RFCv2:
> - only check for phydev->attached_dev and let phy_suspend decide on
>    performing suspend or not (Suggested by Florian)

> @Florian: You suggested to 'make sure that the MDIO bus suspend policy
> was set to "auto"'. I wasn't able to find any clue how to check that,
> so I ignored that for now.

> Cc: David Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   drivers/net/phy/mdio_bus.c |   25 +++++++++++++++++++++++++
>   1 files changed, 25 insertions(+), 0 deletions(-)

> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 5617876..f533d17 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -320,6 +320,31 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
>   		(phydev->phy_id & phydrv->phy_id_mask));
>   }
>
> +static int mdio_bus_suspend_unused(struct device *busdev, void *data)
> +{
> +	struct mii_bus *bus = to_mii_bus(busdev);
> +	struct phy_device *phydev;
> +	int i;
> +
> +	for (i = 0; i < PHY_MAX_ADDR; i++) {
> +		if (!bus->phy_map[i])
> +			continue;
> +
> +		phydev = to_phy_device(&bus->phy_map[i]->dev);

    Why so complex? 'bus->phy_map[i]' already gives you 'phydev'.

> +		if (!phydev->attached_dev)
> +			phy_suspend(phydev);
> +	}
> +
> +	return 0;
> +}

WBR, Sergei

Re: [PATCH RFCv2 0/6] net: phy: Ethernet PHY powerdown optimization

[Mugunthan V N]

On Wednesday 04 December 2013 09:14 PM, Sebastian Hesselbarth wrote:
> This is v2 of an RFC sent earlier [1] to reduce power consumption of network
> PHYs with link that are either unused or the corresponding netdev is down.
>
> In contrast to RFCv1, this now integrates phy_suspend/phy_resume transparent
> to the netdev drivers. Also, phy_suspend now only suspends the PHY if WOL is
> disabled. Moreover, the phy state machine calls phy_suspend on entering
> HALTED state.
>
> Again, a branch with RFCv2 applied to v3.13-rc2 can also be found at
> https://github.com/shesselba/linux-dove.git topic/ethphy-power-rfc-v2
>
> [1] http://lwn.net/Articles/574426/
>
> Sebastian Hesselbarth (6):
>   net: mv643xx_eth: properly start/stop phy device
>   net: phy: marvell: provide genphy suspend/resume
>   net: phy: provide phy_resume/phy_suspend helpers
>   net: phy: resume/suspend PHYs on attach/detach
>   net: phy: suspend unused PHYs on mdio_bus in late_initcall
>   net: phy: suspend phydev when going to HALTED
>
>  drivers/net/ethernet/marvell/mv643xx_eth.c |    4 +++-
>  drivers/net/phy/marvell.c                  |   22 ++++++++++++++++++++++
>  drivers/net/phy/mdio_bus.c                 |   25 +++++++++++++++++++++++++
>  drivers/net/phy/phy.c                      |    6 +++++-
>  drivers/net/phy/phy_device.c               |   27 +++++++++++++++++++++++++++
>  include/linux/phy.h                        |    2 ++
>  6 files changed, 84 insertions(+), 2 deletions(-)
>
> ---
> Cc: David Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
Apart form Sergei's comment the patch series looks good to me.

Acked-by: Mugunthan V N <mugunthanvnm@ti.com>

Regards
Mugunthan V N

Re: [PATCH RFCv2 5/6] net: phy: suspend unused PHYs on mdio_bus in late_initcall

[Florian Fainelli]

2013/12/4 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> Since phy_attach ensures PHYs are resumed, we can now suspend all
> PHYs that have no attached netdev after initcalls.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Changelog:
> RFCv1->RFCv2:
> - only check for phydev->attached_dev and let phy_suspend decide on
>   performing suspend or not (Suggested by Florian)
>
> @Florian: You suggested to 'make sure that the MDIO bus suspend policy
> was set to "auto"'. I wasn't able to find any clue how to check that,
> so I ignored that for now.

I was looking at the sysfs knob which controls the auto suspend
policy, but I am not sure myself how to query that from the kernel, so
let's just forget about it.

>
> Cc: David Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/net/phy/mdio_bus.c |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 5617876..f533d17 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -320,6 +320,31 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
>                 (phydev->phy_id & phydrv->phy_id_mask));
>  }
>
> +static int mdio_bus_suspend_unused(struct device *busdev, void *data)
> +{
> +       struct mii_bus *bus = to_mii_bus(busdev);
> +       struct phy_device *phydev;
> +       int i;
> +
> +       for (i = 0; i < PHY_MAX_ADDR; i++) {
> +               if (!bus->phy_map[i])
> +                       continue;
> +
> +               phydev = to_phy_device(&bus->phy_map[i]->dev);
> +               if (!phydev->attached_dev)
> +                       phy_suspend(phydev);
> +       }
> +
> +       return 0;
> +}
> +
> +static int mdio_bus_class_suspend_unused(void)
> +{
> +       return class_for_each_device(&mdio_bus_class, NULL, NULL,
> +                                    mdio_bus_suspend_unused);
> +}
> +late_initcall_sync(mdio_bus_class_suspend_unused);
> +
>  #ifdef CONFIG_PM
>
>  static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
> --
> 1.7.2.5
>



-- 
Florian

Re: [PATCH RFCv2 3/6] net: phy: provide phy_resume/phy_suspend helpers

[Florian Fainelli]

2013/12/4 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> This adds helper functions to resume and suspend a given phy_device
> by calling the corresponding driver callbacks if available.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Changelog:
> RFCv1->RFCv2:
> - only suspend if WOL is not enabled
> - remove EXPORT_SYMBOL for phy_suspend/resume
>
> Cc: David Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/net/phy/phy_device.c |   24 ++++++++++++++++++++++++
>  include/linux/phy.h          |    2 ++
>  2 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d6447b3..6f0e9e4 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -625,6 +625,30 @@ void phy_detach(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(phy_detach);
>
> +int phy_suspend(struct phy_device *phydev)
> +{
> +       struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
> +       struct ethtool_wolinfo wol;
> +
> +       /* If the device has WOL enabled, we cannot suspend the PHY */
> +       wol.cmd = ETHTOOL_GWOL;
> +       phy_ethtool_get_wol(phydev, &wol);
> +       if (wol.wolopts)
> +               return -EBUSY;

I wonder if we should not set a flag in the phy device structure which
says "someone called my phydrv->set_wol() callback, this succeeded and
WoL is now enabled for me", rather than using this potentially racy
call? This would have to be modified in phy_ethtool_set_wol() for
instance?

This is kind of theoretical though as I can't picture a case where
this sequence of events would happen in real-life.
--
Florian

Re: [PATCH RFCv2 0/6] net: phy: Ethernet PHY powerdown optimization

[Florian Fainelli]

2013/12/4 Mugunthan V N <mugunthanvnm@ti.com>:
> On Wednesday 04 December 2013 09:14 PM, Sebastian Hesselbarth wrote:
>> This is v2 of an RFC sent earlier [1] to reduce power consumption of network
>> PHYs with link that are either unused or the corresponding netdev is down.
>>
>> In contrast to RFCv1, this now integrates phy_suspend/phy_resume transparent
>> to the netdev drivers. Also, phy_suspend now only suspends the PHY if WOL is
>> disabled. Moreover, the phy state machine calls phy_suspend on entering
>> HALTED state.
>>
>> Again, a branch with RFCv2 applied to v3.13-rc2 can also be found at
>> https://github.com/shesselba/linux-dove.git topic/ethphy-power-rfc-v2
>>
>> [1] http://lwn.net/Articles/574426/
>>
>> Sebastian Hesselbarth (6):
>>   net: mv643xx_eth: properly start/stop phy device
>>   net: phy: marvell: provide genphy suspend/resume
>>   net: phy: provide phy_resume/phy_suspend helpers
>>   net: phy: resume/suspend PHYs on attach/detach
>>   net: phy: suspend unused PHYs on mdio_bus in late_initcall
>>   net: phy: suspend phydev when going to HALTED
>>
>>  drivers/net/ethernet/marvell/mv643xx_eth.c |    4 +++-
>>  drivers/net/phy/marvell.c                  |   22 ++++++++++++++++++++++
>>  drivers/net/phy/mdio_bus.c                 |   25 +++++++++++++++++++++++++
>>  drivers/net/phy/phy.c                      |    6 +++++-
>>  drivers/net/phy/phy_device.c               |   27 +++++++++++++++++++++++++++
>>  include/linux/phy.h                        |    2 ++
>>  6 files changed, 84 insertions(+), 2 deletions(-)
>>
>> ---
>> Cc: David Miller <davem@davemloft.net>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Mugunthan V N <mugunthanvnm@ti.com>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
> Apart form Sergei's comment the patch series looks good to me.
>
> Acked-by: Mugunthan V N <mugunthanvnm@ti.com>

Looks good to me as well:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks Sebastian!
-- 
Florian

[PATCH v1 0/6] net: phy: Ethernet PHY powerdown optimization

[Sebastian Hesselbarth]

This is real v1 based on two RFCs sent earlier [1][2] to reduce power
consumption of network PHYs with link that are either unused or the
corresponding netdev is down.

Last RFC got Acked-by/Reviewed-by from Mugunthan and Florian, so it
was time to elevate the RFC to a real patch set. Compared to RFCv2
there is a simplification of phydev reference in patch 5, which was
suggested by Sergei Shtylyov.

Again, a branch with v1 applied to v3.13-rc2 can also be found at
https://github.com/shesselba/linux-dove.git topic/ethphy-power-v1

[1] http://lwn.net/Articles/574426/
[2] http://www.spinics.net/lists/netdev/msg259575.html

Sebastian Hesselbarth (6):
  net: mv643xx_eth: properly start/stop phy device
  net: phy: marvell: provide genphy suspend/resume
  net: phy: provide phy_resume/phy_suspend helpers
  net: phy: resume/suspend PHYs on attach/detach
  net: phy: suspend unused PHYs on mdio_bus in late_initcall
  net: phy: suspend phydev when going to HALTED

 drivers/net/ethernet/marvell/mv643xx_eth.c |  4 +++-
 drivers/net/phy/marvell.c                  | 22 ++++++++++++++++++++++
 drivers/net/phy/mdio_bus.c                 | 23 +++++++++++++++++++++++
 drivers/net/phy/phy.c                      |  6 +++++-
 drivers/net/phy/phy_device.c               | 27 +++++++++++++++++++++++++++
 include/linux/phy.h                        |  2 ++
 6 files changed, 82 insertions(+), 2 deletions(-)

---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
-- 
1.8.4.rc3

[PATCH v1 1/6] net: mv643xx_eth: properly start/stop phy device

[Sebastian Hesselbarth]

When using phydev, it should be phy_start/phy_stop'ed properly. This
driver doesn't do that, so add the corresponding calls to port_start/
stop respectively.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 61088a6..3aa706f 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2098,6 +2098,7 @@ static void port_start(struct mv643xx_eth_private *mp)
 		mv643xx_eth_get_settings(mp->dev, &cmd);
 		phy_reset(mp);
 		mv643xx_eth_set_settings(mp->dev, &cmd);
+		phy_start(mp->phy);
 	}
 
 	/*
@@ -2293,7 +2294,8 @@ static int mv643xx_eth_stop(struct net_device *dev)
 	del_timer_sync(&mp->rx_oom);
 
 	netif_carrier_off(dev);
-
+	if (mp->phy)
+		phy_stop(mp->phy);
 	free_irq(dev->irq, dev);
 
 	port_reset(mp);
-- 
1.8.4.rc3

[PATCH v1 2/6] net: phy: marvell: provide genphy suspend/resume

[Sebastian Hesselbarth]

Marvell PHYs support generic PHY suspend/resume, so provide those
callbacks to all marvell specific drivers.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/marvell.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 2e3c778e..bd37e45 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -894,6 +894,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -907,6 +909,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -920,6 +924,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &marvell_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -933,6 +939,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = {.owner = THIS_MODULE,},
 	},
 	{
@@ -946,6 +954,8 @@ static struct phy_driver marvell_drivers[] = {
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -961,6 +971,8 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.get_wol = &m88e1318_get_wol,
 		.set_wol = &m88e1318_set_wol,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -974,6 +986,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -987,6 +1001,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1000,6 +1016,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1013,6 +1031,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1026,6 +1046,8 @@ static struct phy_driver marvell_drivers[] = {
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 };
-- 
1.8.4.rc3

[PATCH v1 3/6] net: phy: provide phy_resume/phy_suspend helpers

[Sebastian Hesselbarth]

This adds helper functions to resume and suspend a given phy_device
by calling the corresponding driver callbacks if available.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy_device.c | 24 ++++++++++++++++++++++++
 include/linux/phy.h          |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d6447b3..6f0e9e4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -625,6 +625,30 @@ void phy_detach(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_detach);
 
+int phy_suspend(struct phy_device *phydev)
+{
+	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+	struct ethtool_wolinfo wol;
+
+	/* If the device has WOL enabled, we cannot suspend the PHY */
+	wol.cmd = ETHTOOL_GWOL;
+	phy_ethtool_get_wol(phydev, &wol);
+	if (wol.wolopts)
+		return -EBUSY;
+
+	if (phydrv->suspend)
+		return phydrv->suspend(phydev);
+	return 0;
+}
+
+int phy_resume(struct phy_device *phydev)
+{
+	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+
+	if (phydrv->resume)
+		return phydrv->resume(phydev);
+	return 0;
+}
 
 /* Generic PHY support and helper functions */
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 48a4dc3..1070ee3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -538,6 +538,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
 int phy_init_hw(struct phy_device *phydev);
+int phy_suspend(struct phy_device *phydev);
+int phy_resume(struct phy_device *phydev);
 struct phy_device * phy_attach(struct net_device *dev,
 		const char *bus_id, phy_interface_t interface);
 struct phy_device *phy_find_first(struct mii_bus *bus);
-- 
1.8.4.rc3

[PATCH v1 4/6] net: phy: resume/suspend PHYs on attach/detach

[Sebastian Hesselbarth]

This ensures PHYs are resumed on attach and suspended on detach.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy_device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6f0e9e4..3903f44 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -570,6 +570,8 @@ static int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (err)
 		phy_detach(phydev);
 
+	phy_resume(phydev);
+
 	return err;
 }
 
@@ -615,6 +617,7 @@ void phy_detach(struct phy_device *phydev)
 {
 	phydev->attached_dev->phydev = NULL;
 	phydev->attached_dev = NULL;
+	phy_suspend(phydev);
 
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
-- 
1.8.4.rc3

[PATCH v1 5/6] net: phy: suspend unused PHYs on mdio_bus in late_initcall

[Sebastian Hesselbarth]

Since phy_attach ensures PHYs are resumed, we can now suspend all
PHYs that have no attached netdev after initcalls.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changelog:
RFCv2->v1:
- simplify phydev reference (Suggested by Sergei Shtylyov)

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/mdio_bus.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 5617876..9ccf9c6 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -320,6 +320,29 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
 		(phydev->phy_id & phydrv->phy_id_mask));
 }
 
+static int mdio_bus_suspend_unused(struct device *busdev, void *data)
+{
+	struct mii_bus *bus = to_mii_bus(busdev);
+	int i;
+
+	for (i = 0; i < PHY_MAX_ADDR; i++) {
+		if (!bus->phy_map[i])
+			continue;
+
+		if (!bus->phy_map[i]->attached_dev)
+			phy_suspend(bus->phy_map[i]);
+	}
+
+	return 0;
+}
+
+static int mdio_bus_class_suspend_unused(void)
+{
+	return class_for_each_device(&mdio_bus_class, NULL, NULL,
+				     mdio_bus_suspend_unused);
+}
+late_initcall_sync(mdio_bus_class_suspend_unused);
+
 #ifdef CONFIG_PM
 
 static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
-- 
1.8.4.rc3

[PATCH v1 6/6] net: phy: suspend phydev when going to HALTED

[Sebastian Hesselbarth]

When phydev is going to HALTED state, we can try to suspend it to
safe more power. phy_suspend helper will check if PHY can be suspended,
so just call it when entering HALTED state.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 36c6994..6429942 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -740,7 +740,7 @@ void phy_state_machine(struct work_struct *work)
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct phy_device *phydev =
 			container_of(dwork, struct phy_device, state_queue);
-	int needs_aneg = 0;
+	int needs_aneg = 0, do_suspend = 0;
 	int err = 0;
 
 	mutex_lock(&phydev->lock);
@@ -855,6 +855,7 @@ void phy_state_machine(struct work_struct *work)
 				phydev->link = 0;
 				netif_carrier_off(phydev->attached_dev);
 				phydev->adjust_link(phydev->attached_dev);
+				do_suspend = 1;
 			}
 			break;
 		case PHY_RESUMING:
@@ -913,6 +914,9 @@ void phy_state_machine(struct work_struct *work)
 	if (needs_aneg)
 		err = phy_start_aneg(phydev);
 
+	if (do_suspend)
+		phy_suspend(phydev);
+
 	if (err < 0)
 		phy_error(phydev);
 
-- 
1.8.4.rc3

Re: [PATCH v1 1/6] net: mv643xx_eth: properly start/stop phy device

[David Miller]

This series looks good, applied to net-next, thanks.

Re: [PATCH v1 1/6] net: mv643xx_eth: properly start/stop phy device

[David Miller]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: Text/Plain; charset=iso-8859-7, Size: 938 bytes --]

From: David Miller <davem@davemloft.net>
Date: Tue, 10 Dec 2013 21:52:53 -0500 (EST)

> 
> This series looks good, applied to net-next, thanks.

Actually, I had to revert.

You cannot use late_initcall_sync() from code that is potentially
built modular, this caused my build to fail at the mdio_bus code.

drivers/net/phy/mdio_bus.c:344:1: warning: data definition has no type or storage class [enabled by default]
drivers/net/phy/mdio_bus.c:344:1: error: type defaults to ¡int¢ in declaration of ¡late_initcall_sync¢ [-Werror=implicit-int]
drivers/net/phy/mdio_bus.c:344:1: warning: parameter names (without types) in function declaration [enabled by default]
drivers/net/phy/mdio_bus.c:339:12: warning: ¡mdio_bus_class_suspend_unused¢ defined but not used [-Wunused-function]
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v1 1/6] net: mv643xx_eth: properly start/stop phy device

[Sebastian Hesselbarth]

On 12/11/2013 03:56 AM, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Tue, 10 Dec 2013 21:52:53 -0500 (EST)
>
>>
>> This series looks good, applied to net-next, thanks.
>
> Actually, I had to revert.
>
> You cannot use late_initcall_sync() from code that is potentially
> built modular, this caused my build to fail at the mdio_bus code.
>
> drivers/net/phy/mdio_bus.c:344:1: warning: data definition has no type or storage class [enabled by default]
> drivers/net/phy/mdio_bus.c:344:1: error: type defaults to ‘int’ in declaration of ‘late_initcall_sync’ [-Werror=implicit-int]
> drivers/net/phy/mdio_bus.c:344:1: warning: parameter names (without types) in function declaration [enabled by default]
> drivers/net/phy/mdio_bus.c:339:12: warning: ‘mdio_bus_class_suspend_unused’ defined but not used [-Wunused-function]
>

Hmm, I see. What about you drop patch 5 ("net: phy: suspend unused PHYs
on mdio_bus in late_initcall"), take the rest, and I'll have look at
suspending unused PHYs later?

Sebastian

Re: [PATCH v1 1/6] net: mv643xx_eth: properly start/stop phy device

[David Miller]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: Text/Plain; charset=iso-8859-7, Size: 1395 bytes --]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Wed, 11 Dec 2013 08:06:36 +0100

> On 12/11/2013 03:56 AM, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Tue, 10 Dec 2013 21:52:53 -0500 (EST)
>>
>>>
>>> This series looks good, applied to net-next, thanks.
>>
>> Actually, I had to revert.
>>
>> You cannot use late_initcall_sync() from code that is potentially
>> built modular, this caused my build to fail at the mdio_bus code.
>>
>> drivers/net/phy/mdio_bus.c:344:1: warning: data definition has no type
>> or storage class [enabled by default]
>> drivers/net/phy/mdio_bus.c:344:1: error: type defaults to ¡int¢ in
>> declaration of ¡late_initcall_sync¢ [-Werror=implicit-int]
>> drivers/net/phy/mdio_bus.c:344:1: warning: parameter names (without
>> types) in function declaration [enabled by default]
>> drivers/net/phy/mdio_bus.c:339:12: warning:
>> ¡mdio_bus_class_suspend_unused¢ defined but not used
>> [-Wunused-function]
>>
> 
> Hmm, I see. What about you drop patch 5 ("net: phy: suspend unused
> PHYs
> on mdio_bus in late_initcall"), take the rest, and I'll have look at
> suspending unused PHYs later?

What about you resubmit the series that you want me to apply?

Thanks.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH v2 0/5] net: phy: Ethernet PHY powerdown optimization

[Sebastian Hesselbarth]

This is v2 of the ethernet PHY power optimization patches to reduce
power consumption of network PHYs with link that are either unused or
the corresponding netdev is down.

Compared to the last version, this patch set drops a patch to disable
unused PHYs after late initcall, as it is not compatible with a modular
mdio bus [1]. I'll investigate different ways to have a modular mdio bus
driver get notified when driver loading is done.

Again, a branch with v2 applied to v3.13-rc2 can also be found at
https://github.com/shesselba/linux-dove.git topic/ethphy-power-v2

[1] http://www.spinics.net/lists/arm-kernel/msg293028.html

Sebastian Hesselbarth (5):
  net: mv643xx_eth: properly start/stop phy device
  net: phy: marvell: provide genphy suspend/resume
  net: phy: provide phy_resume/phy_suspend helpers
  net: phy: resume/suspend PHYs on attach/detach
  net: phy: suspend phydev when going to HALTED

 drivers/net/ethernet/marvell/mv643xx_eth.c |    4 +++-
 drivers/net/phy/marvell.c                  |   22 ++++++++++++++++++++++
 drivers/net/phy/phy.c                      |    6 +++++-
 drivers/net/phy/phy_device.c               |   27 +++++++++++++++++++++++++++
 include/linux/phy.h                        |    2 ++
 5 files changed, 59 insertions(+), 2 deletions(-)

---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
-- 
1.7.2.5

[PATCH v2 1/5] net: mv643xx_eth: properly start/stop phy device

[Sebastian Hesselbarth]

When using phydev, it should be phy_start/phy_stop'ed properly. This
driver doesn't do that, so add the corresponding calls to port_start/
stop respectively.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/ethernet/marvell/mv643xx_eth.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 61088a6..3aa706f 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2098,6 +2098,7 @@ static void port_start(struct mv643xx_eth_private *mp)
 		mv643xx_eth_get_settings(mp->dev, &cmd);
 		phy_reset(mp);
 		mv643xx_eth_set_settings(mp->dev, &cmd);
+		phy_start(mp->phy);
 	}
 
 	/*
@@ -2293,7 +2294,8 @@ static int mv643xx_eth_stop(struct net_device *dev)
 	del_timer_sync(&mp->rx_oom);
 
 	netif_carrier_off(dev);
-
+	if (mp->phy)
+		phy_stop(mp->phy);
 	free_irq(dev->irq, dev);
 
 	port_reset(mp);
-- 
1.7.2.5

[PATCH v2 2/5] net: phy: marvell: provide genphy suspend/resume

[Sebastian Hesselbarth]

Marvell PHYs support generic PHY suspend/resume, so provide those
callbacks to all marvell specific drivers.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/marvell.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 2e3c778e..bd37e45 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -894,6 +894,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -907,6 +909,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -920,6 +924,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &marvell_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -933,6 +939,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = {.owner = THIS_MODULE,},
 	},
 	{
@@ -946,6 +954,8 @@ static struct phy_driver marvell_drivers[] = {
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -961,6 +971,8 @@ static struct phy_driver marvell_drivers[] = {
 		.did_interrupt = &m88e1121_did_interrupt,
 		.get_wol = &m88e1318_get_wol,
 		.set_wol = &m88e1318_set_wol,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -974,6 +986,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -987,6 +1001,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1000,6 +1016,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1013,6 +1031,8 @@ static struct phy_driver marvell_drivers[] = {
 		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
@@ -1026,6 +1046,8 @@ static struct phy_driver marvell_drivers[] = {
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.did_interrupt = &m88e1121_did_interrupt,
+		.resume = &genphy_resume,
+		.suspend = &genphy_suspend,
 		.driver = { .owner = THIS_MODULE },
 	},
 };
-- 
1.7.2.5

[PATCH v2 3/5] net: phy: provide phy_resume/phy_suspend helpers

[Sebastian Hesselbarth]

This adds helper functions to resume and suspend a given phy_device
by calling the corresponding driver callbacks if available.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy_device.c |   24 ++++++++++++++++++++++++
 include/linux/phy.h          |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d6447b3..6f0e9e4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -625,6 +625,30 @@ void phy_detach(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_detach);
 
+int phy_suspend(struct phy_device *phydev)
+{
+	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+	struct ethtool_wolinfo wol;
+
+	/* If the device has WOL enabled, we cannot suspend the PHY */
+	wol.cmd = ETHTOOL_GWOL;
+	phy_ethtool_get_wol(phydev, &wol);
+	if (wol.wolopts)
+		return -EBUSY;
+
+	if (phydrv->suspend)
+		return phydrv->suspend(phydev);
+	return 0;
+}
+
+int phy_resume(struct phy_device *phydev)
+{
+	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+
+	if (phydrv->resume)
+		return phydrv->resume(phydev);
+	return 0;
+}
 
 /* Generic PHY support and helper functions */
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 48a4dc3..1070ee3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -538,6 +538,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
 int phy_init_hw(struct phy_device *phydev);
+int phy_suspend(struct phy_device *phydev);
+int phy_resume(struct phy_device *phydev);
 struct phy_device * phy_attach(struct net_device *dev,
 		const char *bus_id, phy_interface_t interface);
 struct phy_device *phy_find_first(struct mii_bus *bus);
-- 
1.7.2.5

[PATCH v2 4/5] net: phy: resume/suspend PHYs on attach/detach

[Sebastian Hesselbarth]

This ensures PHYs are resumed on attach and suspended on detach.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy_device.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6f0e9e4..3903f44 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -570,6 +570,8 @@ static int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (err)
 		phy_detach(phydev);
 
+	phy_resume(phydev);
+
 	return err;
 }
 
@@ -615,6 +617,7 @@ void phy_detach(struct phy_device *phydev)
 {
 	phydev->attached_dev->phydev = NULL;
 	phydev->attached_dev = NULL;
+	phy_suspend(phydev);
 
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
-- 
1.7.2.5

[PATCH v2 5/5] net: phy: suspend phydev when going to HALTED

[Sebastian Hesselbarth]

When phydev is going to HALTED state, we can try to suspend it to
safe more power. phy_suspend helper will check if PHY can be suspended,
so just call it when entering HALTED state.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/phy.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 36c6994..6429942 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -740,7 +740,7 @@ void phy_state_machine(struct work_struct *work)
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct phy_device *phydev =
 			container_of(dwork, struct phy_device, state_queue);
-	int needs_aneg = 0;
+	int needs_aneg = 0, do_suspend = 0;
 	int err = 0;
 
 	mutex_lock(&phydev->lock);
@@ -855,6 +855,7 @@ void phy_state_machine(struct work_struct *work)
 				phydev->link = 0;
 				netif_carrier_off(phydev->attached_dev);
 				phydev->adjust_link(phydev->attached_dev);
+				do_suspend = 1;
 			}
 			break;
 		case PHY_RESUMING:
@@ -913,6 +914,9 @@ void phy_state_machine(struct work_struct *work)
 	if (needs_aneg)
 		err = phy_start_aneg(phydev);
 
+	if (do_suspend)
+		phy_suspend(phydev);
+
 	if (err < 0)
 		phy_error(phydev);
 
-- 
1.7.2.5

Re: [PATCH v2 0/5] net: phy: Ethernet PHY powerdown optimization

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Fri, 13 Dec 2013 10:20:24 +0100

> This is v2 of the ethernet PHY power optimization patches to reduce
> power consumption of network PHYs with link that are either unused or
> the corresponding netdev is down.
> 
> Compared to the last version, this patch set drops a patch to disable
> unused PHYs after late initcall, as it is not compatible with a modular
> mdio bus [1]. I'll investigate different ways to have a modular mdio bus
> driver get notified when driver loading is done.
> 
> Again, a branch with v2 applied to v3.13-rc2 can also be found at
> https://github.com/shesselba/linux-dove.git topic/ethphy-power-v2
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg293028.html

Series applied, thanks.

Re: [PATCH v2 0/5] net: phy: Ethernet PHY powerdown optimization

[Sebastian Hesselbarth]

On 12/17/2013 08:43 PM, David Miller wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Date: Fri, 13 Dec 2013 10:20:24 +0100
>
>> This is v2 of the ethernet PHY power optimization patches to reduce
>> power consumption of network PHYs with link that are either unused or
>> the corresponding netdev is down.
>>
>> Compared to the last version, this patch set drops a patch to disable
>> unused PHYs after late initcall, as it is not compatible with a modular
>> mdio bus [1]. I'll investigate different ways to have a modular mdio bus
>> driver get notified when driver loading is done.
>>
>> Again, a branch with v2 applied to v3.13-rc2 can also be found at
>> https://github.com/shesselba/linux-dove.git topic/ethphy-power-v2
>>
>> [1] http://www.spinics.net/lists/arm-kernel/msg293028.html
>
> Series applied, thanks.
>

David, Mungunthan, Florian,

as expected the above patches create a Linux to bootloader dependency
that surfaces dumb bootloaders not initializing PHYs correctly.

Andrew has a Kirkwood based board that does not power-up and restart
auto-negotiation on the powered down PHY after a warm restart. While
this specific bootloader allows a soft-workaround by issuing the
required PHY writes before accessing the interface, others may not.

I think we should allow the user to soft-disable the automatic
power-down of PHYs, i.e. by exploiting a kernel parameter.

Do you have any preference for naming it? My call would be something
like libphy.suspend_halted = [0,1] with 1 being the default.

Sebastian

Re: [PATCH v2 0/5] net: phy: Ethernet PHY powerdown optimization

[Florian Fainelli]

Hi Sebastian,

2014-02-04 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>:
> On 12/17/2013 08:43 PM, David Miller wrote:
>>
>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Date: Fri, 13 Dec 2013 10:20:24 +0100
>>
>>> This is v2 of the ethernet PHY power optimization patches to reduce
>>> power consumption of network PHYs with link that are either unused or
>>> the corresponding netdev is down.
>>>
>>> Compared to the last version, this patch set drops a patch to disable
>>> unused PHYs after late initcall, as it is not compatible with a modular
>>> mdio bus [1]. I'll investigate different ways to have a modular mdio bus
>>> driver get notified when driver loading is done.
>>>
>>> Again, a branch with v2 applied to v3.13-rc2 can also be found at
>>> https://github.com/shesselba/linux-dove.git topic/ethphy-power-v2
>>>
>>> [1] http://www.spinics.net/lists/arm-kernel/msg293028.html
>>
>>
>> Series applied, thanks.
>>
>
> David, Mungunthan, Florian,
>
> as expected the above patches create a Linux to bootloader dependency
> that surfaces dumb bootloaders not initializing PHYs correctly.
>
> Andrew has a Kirkwood based board that does not power-up and restart
> auto-negotiation on the powered down PHY after a warm restart. While
> this specific bootloader allows a soft-workaround by issuing the
> required PHY writes before accessing the interface, others may not.
>
> I think we should allow the user to soft-disable the automatic
> power-down of PHYs, i.e. by exploiting a kernel parameter.
>
> Do you have any preference for naming it? My call would be something
> like libphy.suspend_halted = [0,1] with 1 being the default.

The name looks good to me, and it would avoid having to clear the
BMCR_PWRDOWN bit during the MDIO bus remove callback to workaround
such bootloaders.

BTW, it looks like we are omitting a 0.5 seconds delay after clearing
the BMCR_PWRDOWN bit:

"
22.2.4.1.5 Power Down
...
A PHY is not required to meet the RX_CLK and TX_CLK signal functional
requirements when either bit
0.11 or bit 0.10 is set to a logic one. A PHY shall meet the RX_CLK
and TX_CLK signal functional require-
ments defined in 22.2.2 within 0.5 s after both bit 0.11 and 0.10 are
cleared to zero."
-- 
Florian

Re: [PATCH v2 0/5] net: phy: Ethernet PHY powerdown optimization

[Ezequiel Garcia]

Hi Sebastian,

On Tue, Feb 04, 2014 at 08:38:22PM +0100, Sebastian Hesselbarth wrote:
> On 12/17/2013 08:43 PM, David Miller wrote:
> > From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> > Date: Fri, 13 Dec 2013 10:20:24 +0100
> >
> >> This is v2 of the ethernet PHY power optimization patches to reduce
> >> power consumption of network PHYs with link that are either unused or
> >> the corresponding netdev is down.
> >>
> >> Compared to the last version, this patch set drops a patch to disable
> >> unused PHYs after late initcall, as it is not compatible with a modular
> >> mdio bus [1]. I'll investigate different ways to have a modular mdio bus
> >> driver get notified when driver loading is done.
> >>
> >> Again, a branch with v2 applied to v3.13-rc2 can also be found at
> >> https://github.com/shesselba/linux-dove.git topic/ethphy-power-v2
> >>
> >> [1] http://www.spinics.net/lists/arm-kernel/msg293028.html
> >
> > Series applied, thanks.
> >
[..]
> 
> as expected the above patches create a Linux to bootloader dependency
> that surfaces dumb bootloaders not initializing PHYs correctly.
> 
> Andrew has a Kirkwood based board that does not power-up and restart
> auto-negotiation on the powered down PHY after a warm restart. While
> this specific bootloader allows a soft-workaround by issuing the
> required PHY writes before accessing the interface, others may not.
> 

I'm also having this same issue on Kirkwood USI Topkick, running
v3.14-rc1.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com