Re: [PATCH net-next v2 4/8] net: usb: asix: ax88772: add phylib support

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi Oleksij,

On 07.06.2021 10:27, Oleksij Rempel wrote:
> To be able to use ax88772 with external PHYs and use advantage of
> existing PHY drivers, we need to port at least ax88772 part of asix
> driver to the phylib framework.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

This patch landed recently in linux-next as commit e532a096be0e ("net: 
usb: asix: ax88772: add phylib support"). I found that it causes some 
warnings on boards with those devices, see the following log:

root@target:~# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Jun  9 08:16:41 2021
[  231.226579] PM: suspend entry (deep)
[  231.231697] Filesystems sync: 0.002 seconds
[  231.261761] Freezing user space processes ... (elapsed 0.002 seconds) 
done.
[  231.270526] OOM killer disabled.
[  231.273557] Freezing remaining freezable tasks ... (elapsed 0.002 
seconds) done.
[  231.282229] printk: Suspending console(s) (use no_console_suspend to 
debug)
...
[  231.710852] Disabling non-boot CPUs ...
...
[  231.901794] Enabling non-boot CPUs ...
...
[  232.225640] usb usb3: root hub lost power or was reset
[  232.225746] usb usb1: root hub lost power or was reset
[  232.225864] usb usb5: root hub lost power or was reset
[  232.226206] usb usb6: root hub lost power or was reset
[  232.226207] usb usb4: root hub lost power or was reset
[  232.297749] usb usb2: root hub lost power or was reset
[  232.343227] asix 3-1:1.0 eth0: Failed to write reg index 0x0000: -22
[  232.343293] asix 3-1:1.0 eth0: Failed to enable software MII access
[  232.344486] asix 3-1:1.0 eth0: Failed to read reg index 0x0000: -22
[  232.344512] asix 3-1:1.0 eth0: Failed to write reg index 0x0000: -22
[  232.344529] PM: dpm_run_callback(): mdio_bus_phy_resume+0x0/0x78 
returns -22
[  232.344554] Asix Electronics AX88772C usb-003:002:10: PM: failed to 
resume: error -22
[  232.563712] usb 1-1: reset high-speed USB device number 2 using 
exynos-ehci
[  232.757653] usb 3-1: reset high-speed USB device number 2 using xhci-hcd
[  233.730994] OOM killer enabled.
[  233.734122] Restarting tasks ... done.
[  233.754992] PM: suspend exit

real    0m11.546s
user    0m0.000s
sys     0m0.530s
root@target:~# sleep 2
root@target:~# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Jun  9 08:17:02 2021
[  241.959608] PM: suspend entry (deep)
[  241.963446] Filesystems sync: 0.001 seconds
[  241.978619] Freezing user space processes ... (elapsed 0.004 seconds) 
done.
[  241.989199] OOM killer disabled.
[  241.992215] Freezing remaining freezable tasks ... (elapsed 0.005 
seconds) done.
[  242.003979] printk: Suspending console(s) (use no_console_suspend to 
debug)
...
[  242.592030] Disabling non-boot CPUs ...
...
[  242.879721] Enabling non-boot CPUs ...
...
[  243.145870] usb usb3: root hub lost power or was reset
[  243.145910] usb usb4: root hub lost power or was reset
[  243.147084] usb usb5: root hub lost power or was reset
[  243.147157] usb usb6: root hub lost power or was reset
[  243.147298] usb usb1: root hub lost power or was reset
[  243.217137] usb usb2: root hub lost power or was reset
[  243.283807] asix 3-1:1.0 eth0: Failed to write reg index 0x0000: -22
[  243.284005] asix 3-1:1.0 eth0: Failed to enable software MII access
[  243.285526] asix 3-1:1.0 eth0: Failed to read reg index 0x0000: -22
[  243.285676] asix 3-1:1.0 eth0: Failed to read reg index 0x0004: -22
[  243.285769] ------------[ cut here ]------------
[  243.286011] WARNING: CPU: 2 PID: 2069 at drivers/net/phy/phy.c:916 
phy_error+0x28/0x68
[  243.286115] Modules linked in: cmac bnep mwifiex_sdio mwifiex 
sha256_generic libsha256 sha256_arm cfg80211 btmrvl_sdio btmrvl 
bluetooth s5p_mfc uvcvideo s5p_jpeg exynos_gsc v
[  243.287490] CPU: 2 PID: 2069 Comm: kworker/2:5 Not tainted 
5.13.0-rc5-next-20210608 #10443
[  243.287555] Hardware name: Samsung Exynos (Flattened Device Tree)
[  243.287609] Workqueue: events_power_efficient phy_state_machine
[  243.287716] [<c0111920>] (unwind_backtrace) from [<c010d0cc>] 
(show_stack+0x10/0x14)
[  243.287807] [<c010d0cc>] (show_stack) from [<c0b62360>] 
(dump_stack_lvl+0xa0/0xc0)
[  243.287882] [<c0b62360>] (dump_stack_lvl) from [<c0127960>] 
(__warn+0x118/0x11c)
[  243.287954] [<c0127960>] (__warn) from [<c0127a18>] 
(warn_slowpath_fmt+0xb4/0xbc)
[  243.288021] [<c0127a18>] (warn_slowpath_fmt) from [<c0734968>] 
(phy_error+0x28/0x68)
[  243.288094] [<c0734968>] (phy_error) from [<c0735d6c>] 
(phy_state_machine+0x218/0x278)
[  243.288173] [<c0735d6c>] (phy_state_machine) from [<c014ae08>] 
(process_one_work+0x30c/0x884)
[  243.288254] [<c014ae08>] (process_one_work) from [<c014b3d8>] 
(worker_thread+0x58/0x594)
[  243.288333] [<c014b3d8>] (worker_thread) from [<c0153944>] 
(kthread+0x160/0x1c0)
[  243.288408] [<c0153944>] (kthread) from [<c010011c>] 
(ret_from_fork+0x14/0x38)
[  243.288475] Exception stack(0xc4683fb0 to 0xc4683ff8)
[  243.288531] 3fa0:                                     00000000 
00000000 00000000 00000000
[  243.288587] 3fc0: 00000000 00000000 00000000 00000000 00000000 
00000000 00000000 00000000
[  243.288641] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[  243.288690] irq event stamp: 1611
[  243.288744] hardirqs last  enabled at (1619): [<c01a6ef0>] 
vprintk_emit+0x230/0x290
[  243.288830] hardirqs last disabled at (1626): [<c01a6f2c>] 
vprintk_emit+0x26c/0x290
[  243.288906] softirqs last  enabled at (1012): [<c0101768>] 
__do_softirq+0x500/0x63c
[  243.288978] softirqs last disabled at (1007): [<c01315b4>] 
irq_exit+0x214/0x220
[  243.289055] ---[ end trace eeacda95eb7db60a ]---
[  243.289345] asix 3-1:1.0 eth0: Failed to write reg index 0x0000: -22
[  243.289466] asix 3-1:1.0 eth0: Failed to write Medium Mode mode to 
0x0000: ffffffea
[  243.289540] asix 3-1:1.0 eth0: Link is Down
[  243.482809] usb 1-1: reset high-speed USB device number 2 using 
exynos-ehci
[  243.647251] usb 3-1: reset high-speed USB device number 2 using xhci-hcd
[  244.847161] OOM killer enabled.
[  244.850221] Restarting tasks ... done.
[  244.861372] PM: suspend exit

real    0m13.050s
user    0m0.000s
sys     0m1.152s
root@target:~#

It looks that some kind of system suspend/resume integration for phylib 
is not implemented.

> ---
>   drivers/net/usb/asix.h         |   9 +++
>   drivers/net/usb/asix_common.c  |  37 ++++++++++
>   drivers/net/usb/asix_devices.c | 120 +++++++++++++++++++++------------
>   drivers/net/usb/ax88172a.c     |  14 ----
>   4 files changed, 122 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
> index edb94efd265e..2122d302e643 100644
> --- a/drivers/net/usb/asix.h
> +++ b/drivers/net/usb/asix.h
> @@ -25,6 +25,7 @@
>   #include <linux/usb/usbnet.h>
>   #include <linux/slab.h>
>   #include <linux/if_vlan.h>
> +#include <linux/phy.h>
>   
>   #define DRIVER_VERSION "22-Dec-2011"
>   #define DRIVER_NAME "asix"
> @@ -178,6 +179,10 @@ struct asix_common_private {
>   	u16 presvd_phy_advertise;
>   	u16 presvd_phy_bmcr;
>   	struct asix_rx_fixup_info rx_fixup_info;
> +	struct mii_bus *mdio;
> +	struct phy_device *phydev;
> +	u16 phy_addr;
> +	char phy_name[20];
>   };
>   
>   extern const struct driver_info ax88172a_info;
> @@ -214,6 +219,7 @@ int asix_write_rx_ctl(struct usbnet *dev, u16 mode, int in_pm);
>   
>   u16 asix_read_medium_status(struct usbnet *dev, int in_pm);
>   int asix_write_medium_mode(struct usbnet *dev, u16 mode, int in_pm);
> +void asix_adjust_link(struct net_device *netdev);
>   
>   int asix_write_gpio(struct usbnet *dev, u16 value, int sleep, int in_pm);
>   
> @@ -222,6 +228,9 @@ void asix_set_multicast(struct net_device *net);
>   int asix_mdio_read(struct net_device *netdev, int phy_id, int loc);
>   void asix_mdio_write(struct net_device *netdev, int phy_id, int loc, int val);
>   
> +int asix_mdio_bus_read(struct mii_bus *bus, int phy_id, int regnum);
> +int asix_mdio_bus_write(struct mii_bus *bus, int phy_id, int regnum, u16 val);
> +
>   int asix_mdio_read_nopm(struct net_device *netdev, int phy_id, int loc);
>   void asix_mdio_write_nopm(struct net_device *netdev, int phy_id, int loc,
>   			  int val);
> diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
> index e1109f1a8dd5..085bc8281082 100644
> --- a/drivers/net/usb/asix_common.c
> +++ b/drivers/net/usb/asix_common.c
> @@ -384,6 +384,27 @@ int asix_write_medium_mode(struct usbnet *dev, u16 mode, int in_pm)
>   	return ret;
>   }
>   
> +/* set MAC link settings according to information from phylib */
> +void asix_adjust_link(struct net_device *netdev)
> +{
> +	struct phy_device *phydev = netdev->phydev;
> +	struct usbnet *dev = netdev_priv(netdev);
> +	u16 mode = 0;
> +
> +	if (phydev->link) {
> +		mode = AX88772_MEDIUM_DEFAULT;
> +
> +		if (phydev->duplex == DUPLEX_HALF)
> +			mode &= ~AX_MEDIUM_FD;
> +
> +		if (phydev->speed != SPEED_100)
> +			mode &= ~AX_MEDIUM_PS;
> +	}
> +
> +	asix_write_medium_mode(dev, mode, 0);
> +	phy_print_status(phydev);
> +}
> +
>   int asix_write_gpio(struct usbnet *dev, u16 value, int sleep, int in_pm)
>   {
>   	int ret;
> @@ -506,6 +527,22 @@ void asix_mdio_write(struct net_device *netdev, int phy_id, int loc, int val)
>   	mutex_unlock(&dev->phy_mutex);
>   }
>   
> +/* MDIO read and write wrappers for phylib */
> +int asix_mdio_bus_read(struct mii_bus *bus, int phy_id, int regnum)
> +{
> +	struct usbnet *priv = bus->priv;
> +
> +	return asix_mdio_read(priv->net, phy_id, regnum);
> +}
> +
> +int asix_mdio_bus_write(struct mii_bus *bus, int phy_id, int regnum, u16 val)
> +{
> +	struct usbnet *priv = bus->priv;
> +
> +	asix_mdio_write(priv->net, phy_id, regnum, val);
> +	return 0;
> +}
> +
>   int asix_mdio_read_nopm(struct net_device *netdev, int phy_id, int loc)
>   {
>   	struct usbnet *dev = netdev_priv(netdev);
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 00b6ac0570eb..e4cd85e38edd 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -285,7 +285,7 @@ static int ax88172_bind(struct usbnet *dev, struct usb_interface *intf)
>   
>   static const struct ethtool_ops ax88772_ethtool_ops = {
>   	.get_drvinfo		= asix_get_drvinfo,
> -	.get_link		= asix_get_link,
> +	.get_link		= usbnet_get_link,
>   	.get_msglevel		= usbnet_get_msglevel,
>   	.set_msglevel		= usbnet_set_msglevel,
>   	.get_wol		= asix_get_wol,
> @@ -293,37 +293,15 @@ static const struct ethtool_ops ax88772_ethtool_ops = {
>   	.get_eeprom_len		= asix_get_eeprom_len,
>   	.get_eeprom		= asix_get_eeprom,
>   	.set_eeprom		= asix_set_eeprom,
> -	.nway_reset		= usbnet_nway_reset,
> -	.get_link_ksettings	= usbnet_get_link_ksettings_mii,
> -	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
> +	.nway_reset		= phy_ethtool_nway_reset,
> +	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
> +	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
>   };
>   
> -static int ax88772_link_reset(struct usbnet *dev)
> -{
> -	u16 mode;
> -	struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET };
> -
> -	mii_check_media(&dev->mii, 1, 1);
> -	mii_ethtool_gset(&dev->mii, &ecmd);
> -	mode = AX88772_MEDIUM_DEFAULT;
> -
> -	if (ethtool_cmd_speed(&ecmd) != SPEED_100)
> -		mode &= ~AX_MEDIUM_PS;
> -
> -	if (ecmd.duplex != DUPLEX_FULL)
> -		mode &= ~AX_MEDIUM_FD;
> -
> -	netdev_dbg(dev->net, "ax88772_link_reset() speed: %u duplex: %d setting mode to 0x%04x\n",
> -		   ethtool_cmd_speed(&ecmd), ecmd.duplex, mode);
> -
> -	asix_write_medium_mode(dev, mode, 0);
> -
> -	return 0;
> -}
> -
>   static int ax88772_reset(struct usbnet *dev)
>   {
>   	struct asix_data *data = (struct asix_data *)&dev->data;
> +	struct asix_common_private *priv = dev->driver_priv;
>   	int ret;
>   
>   	/* Rewrite MAC address */
> @@ -342,6 +320,8 @@ static int ax88772_reset(struct usbnet *dev)
>   	if (ret < 0)
>   		goto out;
>   
> +	phy_start(priv->phydev);
> +
>   	return 0;
>   
>   out:
> @@ -586,7 +566,7 @@ static const struct net_device_ops ax88772_netdev_ops = {
>   	.ndo_get_stats64	= dev_get_tstats64,
>   	.ndo_set_mac_address 	= asix_set_mac_address,
>   	.ndo_validate_addr	= eth_validate_addr,
> -	.ndo_do_ioctl		= asix_ioctl,
> +	.ndo_do_ioctl		= phy_do_ioctl_running,
>   	.ndo_set_rx_mode        = asix_set_multicast,
>   };
>   
> @@ -677,12 +657,57 @@ static int asix_resume(struct usb_interface *intf)
>   	return usbnet_resume(intf);
>   }
>   
> +static int ax88772_init_mdio(struct usbnet *dev)
> +{
> +	struct asix_common_private *priv = dev->driver_priv;
> +
> +	priv->mdio = devm_mdiobus_alloc(&dev->udev->dev);
> +	if (!priv->mdio)
> +		return -ENOMEM;
> +
> +	priv->mdio->priv = dev;
> +	priv->mdio->read = &asix_mdio_bus_read;
> +	priv->mdio->write = &asix_mdio_bus_write;
> +	priv->mdio->name = "Asix MDIO Bus";
> +	/* mii bus name is usb-<usb bus number>-<usb device number> */
> +	snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "usb-%03d:%03d",
> +		 dev->udev->bus->busnum, dev->udev->devnum);
> +
> +	return devm_mdiobus_register(&dev->udev->dev, priv->mdio);
> +}
> +
> +static int ax88772_init_phy(struct usbnet *dev)
> +{
> +	struct asix_common_private *priv = dev->driver_priv;
> +	int ret;
> +
> +	priv->phy_addr = asix_read_phy_addr(dev, true);
> +	if (priv->phy_addr < 0)
> +		return priv->phy_addr;
> +
> +	snprintf(priv->phy_name, sizeof(priv->phy_name), PHY_ID_FMT,
> +		 priv->mdio->id, priv->phy_addr);
> +
> +	priv->phydev = phy_connect(dev->net, priv->phy_name, &asix_adjust_link,
> +				   PHY_INTERFACE_MODE_INTERNAL);
> +	if (IS_ERR(priv->phydev)) {
> +		netdev_err(dev->net, "Could not connect to PHY device %s\n",
> +			   priv->phy_name);
> +		ret = PTR_ERR(priv->phydev);
> +		return ret;
> +	}
> +
> +	phy_attached_info(priv->phydev);
> +
> +	return 0;
> +}
> +
>   static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
>   {
> -	int ret, i;
>   	u8 buf[ETH_ALEN] = {0}, chipcode = 0;
> -	u32 phyid;
>   	struct asix_common_private *priv;
> +	int ret, i;
> +	u32 phyid;
>   
>   	usbnet_get_endpoints(dev, intf);
>   
> @@ -714,17 +739,6 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
>   
>   	asix_set_netdev_dev_addr(dev, buf);
>   
> -	/* Initialize MII structure */
> -	dev->mii.dev = dev->net;
> -	dev->mii.mdio_read = asix_mdio_read;
> -	dev->mii.mdio_write = asix_mdio_write;
> -	dev->mii.phy_id_mask = 0x1f;
> -	dev->mii.reg_num_mask = 0x1f;
> -
> -	dev->mii.phy_id = asix_read_phy_addr(dev, true);
> -	if (dev->mii.phy_id < 0)
> -		return dev->mii.phy_id;
> -
>   	dev->net->netdev_ops = &ax88772_netdev_ops;
>   	dev->net->ethtool_ops = &ax88772_ethtool_ops;
>   	dev->net->needed_headroom = 4; /* cf asix_tx_fixup() */
> @@ -768,11 +782,31 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
>   		priv->suspend = ax88772_suspend;
>   	}
>   
> +	ret = ax88772_init_mdio(dev);
> +	if (ret)
> +		return ret;
> +
> +	return ax88772_init_phy(dev);
> +}
> +
> +static int ax88772_stop(struct usbnet *dev)
> +{
> +	struct asix_common_private *priv = dev->driver_priv;
> +
> +	/* On unplugged USB, we will get MDIO communication errors and the
> +	 * PHY will be set in to PHY_HALTED state.
> +	 */
> +	if (priv->phydev->state != PHY_HALTED)
> +		phy_stop(priv->phydev);
> +
>   	return 0;
>   }
>   
>   static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
>   {
> +	struct asix_common_private *priv = dev->driver_priv;
> +
> +	phy_disconnect(priv->phydev);
>   	asix_rx_fixup_common_free(dev->driver_priv);
>   }
>   
> @@ -1161,8 +1195,8 @@ static const struct driver_info ax88772_info = {
>   	.bind = ax88772_bind,
>   	.unbind = ax88772_unbind,
>   	.status = asix_status,
> -	.link_reset = ax88772_link_reset,
>   	.reset = ax88772_reset,
> +	.stop = ax88772_stop,
>   	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR | FLAG_MULTI_PACKET,
>   	.rx_fixup = asix_rx_fixup_common,
>   	.tx_fixup = asix_tx_fixup,
> @@ -1173,7 +1207,6 @@ static const struct driver_info ax88772b_info = {
>   	.bind = ax88772_bind,
>   	.unbind = ax88772_unbind,
>   	.status = asix_status,
> -	.link_reset = ax88772_link_reset,
>   	.reset = ax88772_reset,
>   	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
>   	         FLAG_MULTI_PACKET,
> @@ -1209,7 +1242,6 @@ static const struct driver_info hg20f9_info = {
>   	.bind = ax88772_bind,
>   	.unbind = ax88772_unbind,
>   	.status = asix_status,
> -	.link_reset = ax88772_link_reset,
>   	.reset = ax88772_reset,
>   	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR |
>   	         FLAG_MULTI_PACKET,
> diff --git a/drivers/net/usb/ax88172a.c b/drivers/net/usb/ax88172a.c
> index c8ca5187eece..2e2081346740 100644
> --- a/drivers/net/usb/ax88172a.c
> +++ b/drivers/net/usb/ax88172a.c
> @@ -25,20 +25,6 @@ struct ax88172a_private {
>   	struct asix_rx_fixup_info rx_fixup_info;
>   };
>   
> -/* MDIO read and write wrappers for phylib */
> -static int asix_mdio_bus_read(struct mii_bus *bus, int phy_id, int regnum)
> -{
> -	return asix_mdio_read(((struct usbnet *)bus->priv)->net, phy_id,
> -			      regnum);
> -}
> -
> -static int asix_mdio_bus_write(struct mii_bus *bus, int phy_id, int regnum,
> -			       u16 val)
> -{
> -	asix_mdio_write(((struct usbnet *)bus->priv)->net, phy_id, regnum, val);
> -	return 0;
> -}
> -
>   /* set MAC link settings according to information from phylib */
>   static void ax88172a_adjust_link(struct net_device *netdev)
>   {

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland