Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net-next v2 4/8] net: usb: asix: ax88772: add phylib
 support
To:     Oleksij Rempel <o.rempel@pengutronix.de>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>
Cc:     kernel@pengutronix.de, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, netdev@vger.kernel.org
From:   Marek Szyprowski <m.szyprowski@samsung.com>
Message-ID: <15e1bb24-7d67-9d45-54c1-c1c1a0fe444a@samsung.com>
Date:   Fri, 18 Jun 2021 10:39:12 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0)
        Gecko/20100101 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210607082727.26045-5-o.rempel@pengutronix.de>
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20210607082727.26045-1-o.rempel@pengutronix.de>
        <20210607082727.26045-5-o.rempel@pengutronix.de>
        <CGME20210618083914eucas1p240f88e7064a7bf15b68370b7506d24a9@eucas1p2.samsung.com>
Precedence: bulk

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>

I found one more issue with this patch. On one of my test boards 
(Samsung Exynos5250 SoC based Arndale) system fails to establish network 
connection just after starting the kernel when the driver is build-in.

--->8---
# dmesg | grep asix
[    2.761928] usbcore: registered new interface driver asix
[    5.003110] asix 1-3.2.4:1.0 (unnamed net_device) (uninitialized): 
invalid hw address, using random
[    6.065400] asix 1-3.2.4:1.0 eth0: register 'asix' at 
usb-12110000.usb-3.2.4, ASIX AX88772 USB 2.0 Ethernet, 7a:9b:9a:f2:94:8e
[   14.043868] asix 1-3.2.4:1.0 eth0: Link is Up - 100Mbps/Full - flow 
control off
# ping -c2  host
PING host (192.168.100.1) 56(84) bytes of data.
 From 192.168.100.20 icmp_seq=1 Destination Host Unreachable
 From 192.168.100.20 icmp_seq=2 Destination Host Unreachable

--- host ping statistics ---
2 packets transmitted, 0 received, +2 errors, 100% packet loss, time 59ms
--->8---

Calling ifup eth0 && ifdown eth0 fixes the network status:

--->8---
# ifdown eth0 && ifup eth0
[   60.474929] asix 1-3.2.4:1.0 eth0: Link is Down
[   60.623516] asix 1-3.2.4:1.0 eth0: Link is Down
[   62.774304] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
[   62.786354] asix 1-3.2.4:1.0 eth0: Link is Up - 100Mbps/Full - flow 
control off
# ping -c2 host
PING host (192.168.100.1) 56(84) bytes of data.
64 bytes from host (192.168.100.1): icmp_seq=1 ttl=64 time=1.25 ms
64 bytes from host (192.168.100.1): icmp_seq=2 ttl=64 time=0.853 ms

--- host ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 3ms
rtt min/avg/max/mdev = 0.853/1.053/1.254/0.203 ms
--->8---

When driver is loaded as a module (and without any other modules, so 
this is not a dependency issue), the connection is established properly 
just after the boot:

--->8---
# dmesg | grep asix
[   13.633284] asix 1-3.2.4:1.0 (unnamed net_device) (uninitialized): 
invalid hw address, using random
[   15.390350] asix 1-3.2.4:1.0 eth0: register 'asix' at 
usb-12110000.usb-3.2.4, ASIX AX88772 USB 2.0 Ethernet, 3a:51:11:08:aa:ea
[   15.414052] usbcore: registered new interface driver asix
[   15.832564] asix 1-3.2.4:1.0 eth0: Link is Down
[   18.053747] asix 1-3.2.4:1.0 eth0: Link is Up - 100Mbps/Full - flow 
control off
# ping -c2 host
PING host (192.168.100.1) 56(84) bytes of data.
64 bytes from host (192.168.100.1): icmp_seq=1 ttl=64 time=0.545 ms
64 bytes from host (192.168.100.1): icmp_seq=2 ttl=64 time=0.742 ms

--- host ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 3ms
rtt min/avg/max/mdev = 0.545/0.643/0.742/0.101 ms

--->8---

Let me know if I can make any other tests that would help fixing this issue.

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