[PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[freddy-knRN6Y/kmf1NUHwG+Fw1Kw]

From: Freddy Xin <freddy-knRN6Y/kmf1NUHwG+Fw1Kw@public.gmane.org>

This patch adds a workaroud to solve Suspend/Resume issue that AX88772B turns
off its Ethernet PHY power in the case that REMOTE_WAKEUP feature doesn't
be set when system suspend. In this case, the PHY power will not be turned
on again when system resume, so the HW reset must be added in the resume function.

Signed-off-by: Freddy Xin <freddy-knRN6Y/kmf1NUHwG+Fw1Kw@public.gmane.org>
Tested-by: Grant Grundler <grundler-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/net/usb/asix.h         |    1 +
 drivers/net/usb/asix_devices.c |   16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
index bdaa12d..0e975aa 100644
--- a/drivers/net/usb/asix.h
+++ b/drivers/net/usb/asix.h
@@ -48,6 +48,7 @@
 #define AX_CMD_SET_SW_MII		0x06
 #define AX_CMD_READ_MII_REG		0x07
 #define AX_CMD_WRITE_MII_REG		0x08
+#define AX_CMD_STATMNGSTS_REG		0x09
 #define AX_CMD_SET_HW_MII		0x0a
 #define AX_CMD_READ_EEPROM		0x0b
 #define AX_CMD_WRITE_EEPROM		0x0c
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 386a3df..3ac9958 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -845,6 +845,20 @@ static int ax88178_bind(struct usbnet *dev, struct usb_interface *intf)
 	return 0;
 }
 
+static int asix_resume(struct usb_interface *intf)
+{
+	struct usbnet *dev = usb_get_intfdata(intf);
+	u8 chipcode = 0;
+
+	asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG, 0, 0, 1, &chipcode);
+	chipcode &= 0x70;
+
+	if (dev->driver_info->reset && (chipcode == 0x20))
+		dev->driver_info->reset(dev);
+
+	return usbnet_resume(intf);
+}
+
 static const struct driver_info ax8817x_info = {
 	.description = "ASIX AX8817x USB 2.0 Ethernet",
 	.bind = ax88172_bind,
@@ -1101,7 +1115,7 @@ static struct usb_driver asix_driver = {
 	.id_table =	products,
 	.probe =	usbnet_probe,
 	.suspend =	usbnet_suspend,
-	.resume =	usbnet_resume,
+	.resume =	asix_resume,
 	.disconnect =	usbnet_disconnect,
 	.supports_autosuspend = 1,
 	.disable_hub_initiated_lpm = 1,
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[David Miller]

From: freddy@asix.com.tw
Date: Wed, 20 Nov 2013 10:11:36 +0800

> From: Freddy Xin <freddy@asix.com.tw>
> 
> This patch adds a workaroud to solve Suspend/Resume issue that AX88772B turns
> off its Ethernet PHY power in the case that REMOTE_WAKEUP feature doesn't
> be set when system suspend. In this case, the PHY power will not be turned
> on again when system resume, so the HW reset must be added in the resume function.
> 
> Signed-off-by: Freddy Xin <freddy@asix.com.tw>
> Tested-by: Grant Grundler <grundler@chromium.org>

Like many similar patches I've seen recently, you cannot do this.

Now the the configuration that software things the PHY has is no
longer accurate.

If you absolutely must reset the chip on resume, you have to reprogram
the PHY with whatever settings were existed at the time of the suspend.

Re: [PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[Grant Grundler]

On Tue, Nov 19, 2013 at 9:40 PM, David Miller <davem@davemloft.net> wrote:
> From: freddy@asix.com.tw
> Date: Wed, 20 Nov 2013 10:11:36 +0800
>
>> From: Freddy Xin <freddy@asix.com.tw>
>>
>> This patch adds a workaroud to solve Suspend/Resume issue that AX88772B turns
>> off its Ethernet PHY power in the case that REMOTE_WAKEUP feature doesn't
>> be set when system suspend. In this case, the PHY power will not be turned
>> on again when system resume, so the HW reset must be added in the resume function.
>>
>> Signed-off-by: Freddy Xin <freddy@asix.com.tw>
>> Tested-by: Grant Grundler <grundler@chromium.org>
>
> Like many similar patches I've seen recently, you cannot do this.
>
> Now the the configuration that software things the PHY has is no
> longer accurate.

Dave,
Sorry - I didn't think of that.  I agree the phy settings should be
reprograming in the phy.

The kernel state of the phy is not accurate in any case since the phy
was powered off and left powered off.
While this patch raises a new issue, can you apply this patch anyway
since in most cases (default settings) it improves "the user
experience" for most users?

> If you absolutely must reset the chip on resume, you have to reprogram
> the PHY with whatever settings were existed at the time of the suspend.

AFAICT, the reset is required.

Since you've seen so many of these patches that do NOT reprogram the
phy on reset, would you consider a patch that added "reprogram phy" to
usbnet_resume() code path?

thanks!
grant

Re: [PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[David Miller]

From: Grant Grundler <grundler@google.com>
Date: Wed, 20 Nov 2013 09:42:42 -0800

> While this patch raises a new issue, can you apply this patch anyway
> since in most cases (default settings) it improves "the user
> experience" for most users?

I'm not applying a patch which knowingly is not implemented correctly.

Re: [PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[Grant Grundler]

On Wed, Nov 20, 2013 at 10:54 AM, David Miller <davem@davemloft.net> wrote:
> From: Grant Grundler <grundler@google.com>
> Date: Wed, 20 Nov 2013 09:42:42 -0800
>
>> While this patch raises a new issue, can you apply this patch anyway
>> since in most cases (default settings) it improves "the user
>> experience" for most users?
>
> I'm not applying a patch which knowingly is not implemented correctly.

Ok - fair enough. I'll figure out what needs to be done to
save/restore phy values.

Seems like this should be part of usbnet_resume code based on whether
the driver provides mdio_write hook (most USBNET drivers do).

thanks,
grant

Re: [PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[Grant Grundler]

On Wed, Nov 20, 2013 at 11:32 AM, Grant Grundler <grundler@google.com> wrote:
> Seems like this should be part of usbnet_resume code based on whether
> the driver provides mdio_write hook (most USBNET drivers do).

Just to be clear: I don't think this is feasible for the now obvious
reason that not all device lose phy stat on resume. :/

Freddy sent me a patch that saves/restores MII_ADVERTISE and MII_BMCR
registers. Testing that now and he'll post if that all works out.

thanks,
grant

Re: [PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[Florian Fainelli]

2013/11/21 Grant Grundler <grundler@google.com>:
> On Wed, Nov 20, 2013 at 11:32 AM, Grant Grundler <grundler@google.com> wrote:
>> Seems like this should be part of usbnet_resume code based on whether
>> the driver provides mdio_write hook (most USBNET drivers do).
>
> Just to be clear: I don't think this is feasible for the now obvious
> reason that not all device lose phy stat on resume. :/
>
> Freddy sent me a patch that saves/restores MII_ADVERTISE and MII_BMCR
> registers. Testing that now and he'll post if that all works out.

That should be enough to ensure the PHY is put back into a consistent
state. So long as you are suspending/resuming from a configuration
where autoneg was enabled, I would not expect anything bad to happen.
If the link was forced, that is a different story.

One way to make sure this work properly without driver-specific code
is to include that functionality into the PHY library. As far as I can
see it the asix drivers do not use it but implement the old-style
mii_bus interface so that would be a first step.
-- 
Florian

Re: [PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[Grant Grundler]

On Thu, Nov 21, 2013 at 5:58 PM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
...
>> Freddy sent me a patch that saves/restores MII_ADVERTISE and MII_BMCR
>> registers. Testing that now and he'll post if that all works out.

Bad News: Freddy's patch didn't work and needs to be rework it.

> That should be enough to ensure the PHY is put back into a consistent
> state. So long as you are suspending/resuming from a configuration
> where autoneg was enabled, I would not expect anything bad to happen.

Well, I think davem NACK'd that approach (the original patch in this
thread did that).

> If the link was forced, that is a different story.
>
> One way to make sure this work properly without driver-specific code
> is to include that functionality into the PHY library. As far as I can
> see it the asix drivers do not use it but implement the old-style
> mii_bus interface so that would be a first step.

AFAICT, many USBNET drivers support mii_bus interface. Can you point
at a good example of a USBNET driver that is supporting PHY lib?

I also didn't see anything in usbnet_resume() code path that calls
into a phy lib. So it seems that some infrastructure still needs to
happen if that's considered a feasible.

thanks,
grant

> --
> Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS

[Florian Fainelli]

2013/11/21 Grant Grundler <grundler@google.com>:
> On Thu, Nov 21, 2013 at 5:58 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> ...
>>> Freddy sent me a patch that saves/restores MII_ADVERTISE and MII_BMCR
>>> registers. Testing that now and he'll post if that all works out.
>
> Bad News: Freddy's patch didn't work and needs to be rework it.
>
>> That should be enough to ensure the PHY is put back into a consistent
>> state. So long as you are suspending/resuming from a configuration
>> where autoneg was enabled, I would not expect anything bad to happen.
>
> Well, I think davem NACK'd that approach (the original patch in this
> thread did that).
>
>> If the link was forced, that is a different story.
>>
>> One way to make sure this work properly without driver-specific code
>> is to include that functionality into the PHY library. As far as I can
>> see it the asix drivers do not use it but implement the old-style
>> mii_bus interface so that would be a first step.
>
> AFAICT, many USBNET drivers support mii_bus interface. Can you point
> at a good example of a USBNET driver that is supporting PHY lib?

drivers/net/usb/ax88172a.c implements the PHY library calls.

>
> I also didn't see anything in usbnet_resume() code path that calls
> into a phy lib. So it seems that some infrastructure still needs to
> happen if that's considered a feasible.

There are still some pending patches to make sure that suspend/resume
are properly being taken care of when issuing phy_stop/phy_start, but
once that is done, you would not have to call anything in
usbnet_resume(), the driver's ndo_open() callback would call
phy_start() which would resume the PHY into the state it was before
suspending.
-- 
Florian