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

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(-)

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