Message ID | 1384978913-8052-8-git-send-email-sebastian.hesselbarth@gmail.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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/
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/ > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)