[PATCH net-next v1 1/1] net: usb: asix: ax88772: suspend PHY on driver probe

[PATCH net-next v1 1/1] net: usb: asix: ax88772: suspend PHY on driver probe

[Oleksij Rempel]

After probe/bind sequence is the PHY in active state, even if interface
is stopped. As result, on some systems like Samsung Exynos5250 SoC based Arndale
board, the ASIX PHY will be able to negotiate the link but fail to
transmit the data.

To handle it, suspend the PHY on probe.

Fixes: e532a096be0e ("net: usb: asix: ax88772: add phylib support")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/net/usb/asix_devices.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index aec97b021a73..2c115216420a 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -701,6 +701,7 @@ static int ax88772_init_phy(struct usbnet *dev)
 		return ret;
 	}
 
+	phy_suspend(priv->phydev);
 	priv->phydev->mac_managed_pm = 1;
 
 	phy_attached_info(priv->phydev);
-- 
2.30.2

Re: [PATCH net-next v1 1/1] net: usb: asix: ax88772: suspend PHY on driver probe

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Tue, 29 Jun 2021 06:43:05 +0200 you wrote:
> After probe/bind sequence is the PHY in active state, even if interface
> is stopped. As result, on some systems like Samsung Exynos5250 SoC based Arndale
> board, the ASIX PHY will be able to negotiate the link but fail to
> transmit the data.
> 
> To handle it, suspend the PHY on probe.
> 
> [...]

Here is the summary with links:
  - [net-next,v1,1/1] net: usb: asix: ax88772: suspend PHY on driver probe
    https://git.kernel.org/netdev/net/c/a3609ac24c18

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v1 1/1] net: usb: asix: ax88772: suspend PHY on driver probe

[Florian Fainelli]

On 6/28/21 9:43 PM, Oleksij Rempel wrote:
> After probe/bind sequence is the PHY in active state, even if interface
> is stopped. As result, on some systems like Samsung Exynos5250 SoC based Arndale
> board, the ASIX PHY will be able to negotiate the link but fail to
> transmit the data.
> 
> To handle it, suspend the PHY on probe.

Very unusual, could not the PHY be attached/connected to a ndo_open()
time like what most drivers do?

> 
> Fixes: e532a096be0e ("net: usb: asix: ax88772: add phylib support")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/net/usb/asix_devices.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index aec97b021a73..2c115216420a 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -701,6 +701,7 @@ static int ax88772_init_phy(struct usbnet *dev)
>  		return ret;
>  	}
>  
> +	phy_suspend(priv->phydev);
>  	priv->phydev->mac_managed_pm = 1;
>  
>  	phy_attached_info(priv->phydev);
> 


-- 
Florian

Re: [PATCH net-next v1 1/1] net: usb: asix: ax88772: suspend PHY on driver probe

[Oleksij Rempel]

On Thu, Jul 01, 2021 at 12:01:04PM -0700, Florian Fainelli wrote:
> On 6/28/21 9:43 PM, Oleksij Rempel wrote:
> > After probe/bind sequence is the PHY in active state, even if interface
> > is stopped. As result, on some systems like Samsung Exynos5250 SoC based Arndale
> > board, the ASIX PHY will be able to negotiate the link but fail to
> > transmit the data.
> > 
> > To handle it, suspend the PHY on probe.
> 
> Very unusual, could not the PHY be attached/connected to a ndo_open()
> time like what most drivers do?

May be this can be done to.
But, should not the PHY be in the same state after phy_connect() and after
phy_stop()?

Currently, phy_connect() and phy_start() resume the PHY. The phy_stop()
is suspending it. Since the driver calls phy_connect(), phy_start() and
phy_stop(), the resume/suspend state is out of balance.
In case some one will add for example something like regulator_enable/disable
callbacks in to phydev->syspend/resume callbacks, this would never work.

So, the question is, should phy_connect() put the PHY back in to suspend
mode? Or should the MAC driver take care of this?

> > 
> > Fixes: e532a096be0e ("net: usb: asix: ax88772: add phylib support")
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  drivers/net/usb/asix_devices.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> > index aec97b021a73..2c115216420a 100644
> > --- a/drivers/net/usb/asix_devices.c
> > +++ b/drivers/net/usb/asix_devices.c
> > @@ -701,6 +701,7 @@ static int ax88772_init_phy(struct usbnet *dev)
> >  		return ret;
> >  	}
> >  
> > +	phy_suspend(priv->phydev);
> >  	priv->phydev->mac_managed_pm = 1;
> >  
> >  	phy_attached_info(priv->phydev);
> > 
> 
> 
> -- 
> Florian
> 

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 1/1] net: usb: asix: ax88772: suspend PHY on driver probe

[Heiner Kallweit]

On 02.07.2021 05:53, Oleksij Rempel wrote:
> On Thu, Jul 01, 2021 at 12:01:04PM -0700, Florian Fainelli wrote:
>> On 6/28/21 9:43 PM, Oleksij Rempel wrote:
>>> After probe/bind sequence is the PHY in active state, even if interface
>>> is stopped. As result, on some systems like Samsung Exynos5250 SoC based Arndale
>>> board, the ASIX PHY will be able to negotiate the link but fail to
>>> transmit the data.
>>>
>>> To handle it, suspend the PHY on probe.
>>
>> Very unusual, could not the PHY be attached/connected to a ndo_open()
>> time like what most drivers do?
> 
> May be this can be done to.
> But, should not the PHY be in the same state after phy_connect() and after
> phy_stop()?
> 
> Currently, phy_connect() and phy_start() resume the PHY. The phy_stop()
> is suspending it. Since the driver calls phy_connect(), phy_start() and
> phy_stop(), the resume/suspend state is out of balance.

At least currently there is no requirement that phy_resume()/phy_suspend()
calls have to be balanced. Drivers must be able to deal with this.
However phy_suspend() checks the state of phydev->suspended and skips the
suspend callback if set.

> In case some one will add for example something like regulator_enable/disable
> callbacks in to phydev->syspend/resume callbacks, this would never work.
> 
> So, the question is, should phy_connect() put the PHY back in to suspend
> mode? Or should the MAC driver take care of this?
> 
>>>
>>> Fixes: e532a096be0e ("net: usb: asix: ax88772: add phylib support")
>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>  drivers/net/usb/asix_devices.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
>>> index aec97b021a73..2c115216420a 100644
>>> --- a/drivers/net/usb/asix_devices.c
>>> +++ b/drivers/net/usb/asix_devices.c
>>> @@ -701,6 +701,7 @@ static int ax88772_init_phy(struct usbnet *dev)
>>>  		return ret;
>>>  	}
>>>  
>>> +	phy_suspend(priv->phydev);
>>>  	priv->phydev->mac_managed_pm = 1;
>>>  
>>>  	phy_attached_info(priv->phydev);
>>>
>>
>>
>> -- 
>> Florian
>>
> 
> Regards,
> Oleksij
>