Re: [PATCH] net: stmmac: dwmac-rk: keep PHY up for WoL

[Giuseppe CAVALLARO]

Hello Vincent

On 6/10/2016 1:00 AM, Vincent Palatin wrote:

On Wed, Jun 8, 2016 at 5:17 PM, Andrew Lunn  wrote:

On Wed, Jun 08, 2016 at 03:25:38PM -0700, Vincent Palatin wrote:

On Tue, Jun 7, 2016 at 12:23 AM, Giuseppe CAVALLARO
 wrote:

Hello

On 6/3/2016 7:29 PM, Vincent Palatin wrote:


Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake
us up.



I do not understand why you need that.
This is done inside the PHY layer and it is tested on our platforms
he idea is: If the parent wants to Wake the system then the PHY should
not power-down.


I'm not sure I understand :
you mean that this path is not called if WoL is enabled ?
[ currently stmmac_pltfr_suspend() is calling priv->plat->exit() which
is the rk_gmac_exit() code I'm modifying ]
or the RK driver code should not power down the phy in its exit() callback ?


Take a look at phy_suspend().


phy_suspend() sends (or not) the PowerDown command to the PHY through
the MDIO bus,  depending if WoL is disabled,
but most of my question still stands as far as I can tell :
I was trying to get a proper WoL support on the following setup :
  dwmac  (inside a RK3288 SoC) connected to RTL8211 PHY
The current upstream code for this case will call rk_gmac_exit() when
the MAC suspends (after the PHY has already suspended). Effectively
doing a phy_power_on(, false) which is calling regulator_disable() on
the LDO defined by the 'phy-supply' attribute.
So my reading is that the RK specific MAC code is turning off
unconditionally the PHY power regulator. Unless I'm mistaken, either
this code is incorrect for the WoL case or the naming 'phy-supply' is
misleading and should be the MAC supply.


ok now clear. And you are right. I can conclude that the patch is ok
for me. I just ask you to resend it elaborating a bit the subject and
surrounding the code with a comment.

I do not know your SoC but indeed, when doing WoL, some parts of the
MAC + PHY must be powered so IMO it is legal that you do not cut
the power by invoking regulator.

Peppe

Re: [PATCH] net: stmmac: dwmac-rk: keep PHY up for WoL

[Vincent Palatin]

On Wed, Jun 8, 2016 at 5:17 PM, Andrew Lunn  wrote:
> On Wed, Jun 08, 2016 at 03:25:38PM -0700, Vincent Palatin wrote:
>> On Tue, Jun 7, 2016 at 12:23 AM, Giuseppe CAVALLARO
>>  wrote:
>> > Hello
>> >
>> > On 6/3/2016 7:29 PM, Vincent Palatin wrote:
>> >>
>> >> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake
>> >> us up.
>> >>
>> >
>> > I do not understand why you need that.
>> > This is done inside the PHY layer and it is tested on our platforms
>> > he idea is: If the parent wants to Wake the system then the PHY should
>> > not power-down.
>>
>> I'm not sure I understand :
>> you mean that this path is not called if WoL is enabled ?
>> [ currently stmmac_pltfr_suspend() is calling priv->plat->exit() which
>> is the rk_gmac_exit() code I'm modifying ]
>> or the RK driver code should not power down the phy in its exit() callback ?
>
> Take a look at phy_suspend().

phy_suspend() sends (or not) the PowerDown command to the PHY through
the MDIO bus,  depending if WoL is disabled,
but most of my question still stands as far as I can tell :
I was trying to get a proper WoL support on the following setup :
  dwmac  (inside a RK3288 SoC) connected to RTL8211 PHY
The current upstream code for this case will call rk_gmac_exit() when
the MAC suspends (after the PHY has already suspended). Effectively
doing a phy_power_on(, false) which is calling regulator_disable() on
the LDO defined by the 'phy-supply' attribute.
So my reading is that the RK specific MAC code is turning off
unconditionally the PHY power regulator. Unless I'm mistaken, either
this code is incorrect for the WoL case or the naming 'phy-supply' is
misleading and should be the MAC supply.

-- 
Vincent

Re: [PATCH] net: stmmac: dwmac-rk: keep PHY up for WoL

[Andrew Lunn]

On Wed, Jun 08, 2016 at 03:25:38PM -0700, Vincent Palatin wrote:
> On Tue, Jun 7, 2016 at 12:23 AM, Giuseppe CAVALLARO
>  wrote:
> > Hello
> >
> > On 6/3/2016 7:29 PM, Vincent Palatin wrote:
> >>
> >> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake
> >> us up.
> >>
> >
> > I do not understand why you need that.
> > This is done inside the PHY layer and it is tested on our platforms
> > he idea is: If the parent wants to Wake the system then the PHY should
> > not power-down.
> 
> I'm not sure I understand :
> you mean that this path is not called if WoL is enabled ?
> [ currently stmmac_pltfr_suspend() is calling priv->plat->exit() which
> is the rk_gmac_exit() code I'm modifying ]
> or the RK driver code should not power down the phy in its exit() callback ?

Take a look at phy_suspend().

 Andrew

Re: [PATCH] net: stmmac: dwmac-rk: keep PHY up for WoL

[Vincent Palatin]

On Tue, Jun 7, 2016 at 12:23 AM, Giuseppe CAVALLARO
 wrote:
> Hello
>
> On 6/3/2016 7:29 PM, Vincent Palatin wrote:
>>
>> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake
>> us up.
>>
>
> I do not understand why you need that.
> This is done inside the PHY layer and it is tested on our platforms
> he idea is: If the parent wants to Wake the system then the PHY should
> not power-down.

I'm not sure I understand :
you mean that this path is not called if WoL is enabled ?
[ currently stmmac_pltfr_suspend() is calling priv->plat->exit() which
is the rk_gmac_exit() code I'm modifying ]
or the RK driver code should not power down the phy in its exit() callback ?


>> Signed-off-by: Vincent Palatin 
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> index 0cd3ecf..2e45e75 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> @@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev,
>> void *priv)
>> struct rk_priv_data *bsp_priv = priv;
>> int ret;
>>
>> +   /* Keep the PHY up if we use Wake-on-Lan. */
>> +   if (device_may_wakeup(>dev))
>> +   return 0;
>> +
>> ret = phy_power_on(bsp_priv, true);
>> if (ret)
>> return ret;
>> @@ -549,6 +553,10 @@ static void rk_gmac_exit(struct platform_device
>> *pdev, void *priv)
>>  {
>> struct rk_priv_data *gmac = priv;
>>
>> +   /* The PHY was up for Wake-on-Lan. */
>> +   if (device_may_wakeup(>dev))
>> +   return;
>> +
>> phy_power_on(gmac, false);
>> gmac_clk_enable(gmac, false);
>>  }
>>
>

Re: [PATCH] net: stmmac: dwmac-rk: keep PHY up for WoL

[Giuseppe CAVALLARO]

Hello

On 6/3/2016 7:29 PM, Vincent Palatin wrote:

Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake
us up.



I do not understand why you need that.
This is done inside the PHY layer and it is tested on our platforms
he idea is: If the parent wants to Wake the system then the PHY should
not power-down.

Peppe


Signed-off-by: Vincent Palatin 
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 0cd3ecf..2e45e75 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev, void 
*priv)
struct rk_priv_data *bsp_priv = priv;
int ret;

+   /* Keep the PHY up if we use Wake-on-Lan. */
+   if (device_may_wakeup(>dev))
+   return 0;
+
ret = phy_power_on(bsp_priv, true);
if (ret)
return ret;
@@ -549,6 +553,10 @@ static void rk_gmac_exit(struct platform_device *pdev, 
void *priv)
 {
struct rk_priv_data *gmac = priv;

+   /* The PHY was up for Wake-on-Lan. */
+   if (device_may_wakeup(>dev))
+   return;
+
phy_power_on(gmac, false);
gmac_clk_enable(gmac, false);
 }

Re: [PATCH] net: stmmac: dwmac-rk: keep PHY up for WoL

[Vincent Palatin]

On Mon, Jun 6, 2016 at 1:45 PM, Heiko Stübner  wrote:
> Hi,
>
> Am Freitag, 3. Juni 2016, 10:29:20 schrieb Vincent Palatin:
>> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake
>> us up.
>>
>> Signed-off-by: Vincent Palatin 
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 0cd3ecf..2e45e75
>> 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> @@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev,
>> void *priv) struct rk_priv_data *bsp_priv = priv;
>>   int ret;
>>
>> + /* Keep the PHY up if we use Wake-on-Lan. */
>> + if (device_may_wakeup(>dev))
>> + return 0;
>> +
>
> Hmm, this looks like it would also block the initial setup of clocks and phy?

Yes, that's bad. Doug told me so but I forget to CC him on the
previous submission.
I will do another version.

> platform_device + device struct are created before probe gets called, so
> something could set the wakeup flag before the driver initially probes?

The device tree 'wakeup' attribute likely does it.

-- 
Vincent

Re: [PATCH] net: stmmac: dwmac-rk: keep PHY up for WoL

[Heiko Stübner]

Hi,

Am Freitag, 3. Juni 2016, 10:29:20 schrieb Vincent Palatin:
> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake
> us up.
> 
> Signed-off-by: Vincent Palatin 
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 0cd3ecf..2e45e75
> 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev,
> void *priv) struct rk_priv_data *bsp_priv = priv;
>   int ret;
> 
> + /* Keep the PHY up if we use Wake-on-Lan. */
> + if (device_may_wakeup(>dev))
> + return 0;
> +

Hmm, this looks like it would also block the initial setup of clocks and phy?
platform_device + device struct are created before probe gets called, so 
something could set the wakeup flag before the driver initially probes?


Confused,
Heiko