[PATCH] please review mcs7830 (DeLOCK USB etherner) driver

[PATCH] please review mcs7830 (DeLOCK USB etherner) driver

[Arnd Bergmann]

I've been unfortunate to stumble over the MosChip 7830 USB2 10/100
Ethernet adapter. There was a driver for it provided on the manufacturer's
web site, but since I intend need to use the device in the future, I
decided to clean that driver up and get it into shape for inclusion.

The code below should fix the style issues that the original had, but
still has the same bug that it creates a storm of error interrupts
and transfers less than 10kb/s of data on average. I hope to
be able to fix that soon.

There was no copyright information or contact address in the 
vendor-provided driver, but it was obviously provided under
the terms of the GPL.

I copied over some code from the asix driver, maybe there is
potential to share the common code in the future.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Index: linux-cg/drivers/usb/net/mcs7830.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-cg/drivers/usb/net/mcs7830.c	2006-08-07 02:40:04.000000000 +0200
@@ -0,0 +1,514 @@
+/*
+ * MosChips MCS7830 based USB 2.0 Ethernet Devices
+ *
+ * based on usbnet.c, asix.c and the vendor provided mcs7830 driver
+ *
+ * Copyright (C) 2006 Arnd Bergmann <arnd@arndb.de>
+ * Copyright (C) 2003-2005 David Hollis <dhollis@davehollis.com>
+ * Copyright (C) 2005 Phil Chang <pchang23@sbcglobal.net>
+ * Copyright (c) 2002-2003 TiVo Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/crc32.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/init.h>
+#include <linux/mii.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/usb.h>
+
+#include "usbnet.h"
+
+/* requests */
+#define MCS7830_RD_BMREQ	(USB_DIR_IN  | USB_TYPE_VENDOR | USB_RECIP_DEVICE)
+#define MCS7830_WR_BMREQ	(USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE)
+#define MCS7830_RD_BREQ		0x0E
+#define MCS7830_WR_BREQ		0x0D
+
+#define MCS7830_CTRL_TIMEOUT	1000
+#define MCS7830_MAX_MCAST	64
+
+#define MCS7830_VENDOR_ID	0x9710
+#define MCS7830_PRODUCT_ID	0x7830
+
+/* HIF_REG_XX coressponding index value */
+enum {
+	HIF_REG_MULTICAST_HASH			= 0x00,
+	HIF_REG_PACKET_GAP1			= 0x08,
+	HIF_REG_PACKET_GAP2			= 0x09,
+	HIF_REG_PHY_DATA			= 0x0a,
+	HIF_REG_PHY_CMD1			= 0x0c,
+	   HIF_REG_PHY_CMD1_READ		= 0x40,
+	   HIF_REG_PHY_CMD1_WRITE		= 0x20,
+	   HIF_REG_PHY_CMD1_PHYADDR		= 0x01,
+	HIF_REG_PHY_CMD2			= 0x0d,
+	   HIF_REG_PHY_CMD2_PEND_FLAG_BIT	= 0x80,
+	   HIF_REG_PHY_CMD2_READY_FLAG_BIT	= 0x40,
+	HIF_REG_CONFIG				= 0x0e,
+	   HIF_REG_CONFIG_CFG			= 0x80,
+	   HIF_REG_CONFIG_SPEED100		= 0x40,
+	   HIF_REG_CONFIG_FULLDUPLEX_ENABLE	= 0x20,
+	   HIF_REG_CONFIG_RXENABLE		= 0x10,
+	   HIF_REG_CONFIG_TXENABLE		= 0x08,
+	   HIF_REG_CONFIG_SLEEPMODE		= 0x04,
+	   HIF_REG_CONFIG_ALLMULTICAST		= 0x02,
+	   HIF_REG_CONFIG_PROMISCIOUS		= 0x01,
+	HIF_REG_ETHERNET_ADDR			= 0x0f,
+	HIF_REG_22				= 0x15,
+	HIF_REG_PAUSE_THRESHOLD			= 0x16,
+	   HIF_REG_PAUSE_THRESHOLD_DEFAULT	= 0,
+};
+
+/* index for PHY registers */
+enum {
+	PHY_CONTROL_REG_INDEX			=  0,
+	PHY_STATUS_REG_INDEX			=  1,
+	PHY_IDENTIFICATION1_REG_INDEX		=  2,
+	PHY_IDENTIFICATION2_REG_INDEX		=  3,
+	PHY_AUTONEGADVT_REG_INDEX		=  4,
+	PHY_AUTONEGLINK_REG_INDEX		=  5,
+	PHY_AUTONEGEXP_REG_INDEX		=  6,
+	PHY_MIRROR_REG_INDEX			= 16,
+	PHY_INTERRUPTENABLE_REG_INDEX		= 17,
+	PHY_INTERRUPTSTATUS_REG_INDEX		= 18,
+	PHY_CONFIG_REG_INDEX			= 19,
+	PHY_CHIPSTATUS_REG_INDEX		= 20,
+};
+
+struct mcs7830_data {
+	u8 multi_filter[8];
+	u8 config;
+};
+
+static DEFINE_MUTEX(mcs7830_phy_mutex);
+
+static const char driver_name[] = "MOSCHIP usb-ethernet driver";
+
+static int mcs7830_get_reg(struct usbnet *dev, u16 index, u16 size, void *data)
+{
+	struct usb_device *xdev = dev->udev;
+	int ret;
+
+	ret = usb_control_msg(xdev, usb_rcvctrlpipe(xdev, 0), MCS7830_RD_BREQ,
+			      MCS7830_RD_BMREQ, 0x0000, index, data,
+			      size, msecs_to_jiffies(MCS7830_CTRL_TIMEOUT));
+	return ret;
+}
+
+static int mcs7830_set_reg(struct usbnet *dev, u16 index, u16 size, void *data)
+{
+	struct usb_device *xdev = dev->udev;
+	int ret;
+
+	ret = usb_control_msg(xdev, usb_sndctrlpipe(xdev, 0), MCS7830_WR_BREQ,
+			      MCS7830_WR_BMREQ, 0x0000, index, data,
+			      size, msecs_to_jiffies(MCS7830_CTRL_TIMEOUT));
+	return ret;
+}
+
+static void mcs7830_async_cmd_callback(struct urb *urb, struct pt_regs *regs)
+{
+	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
+
+	if (urb->status < 0)
+		printk(KERN_DEBUG "mcs7830_async_cmd_callback() failed with %d",
+			urb->status);
+
+	kfree(req);
+	usb_free_urb(urb);
+}
+
+static void mcs7830_set_reg_async(struct usbnet *dev, u16 index, u16 size, void *data)
+{
+	struct usb_ctrlrequest *req;
+	int ret;
+	struct urb *urb;
+
+	urb = usb_alloc_urb(0, GFP_ATOMIC);
+	if (!urb) {
+		dev_dbg(&dev->udev->dev, "Error allocating URB "
+				"in write_cmd_async!");
+		return;
+	}
+
+	req = kmalloc(sizeof *req, GFP_ATOMIC);
+	if (!req) {
+		dev_err(&dev->udev->dev, "Failed to allocate memory for "
+				"control request");
+		goto out;
+	}
+	req->bRequestType = MCS7830_WR_BMREQ;
+	req->bRequest = MCS7830_WR_BREQ;
+	req->wValue = 0;
+	req->wIndex = cpu_to_le16(index);
+	req->wLength = cpu_to_le16(size);
+
+	usb_fill_control_urb(urb, dev->udev,
+			     usb_sndctrlpipe(dev->udev, 0),
+			     (void *)req, data, size,
+			     mcs7830_async_cmd_callback, req);
+
+	if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
+		dev_err(&dev->udev->dev, "Error submitting the control "
+				"message: ret=%d", ret);
+		goto out;
+	}
+	return;
+out:
+	kfree(req);
+	usb_free_urb(urb);
+}
+
+static int mcs7830_get_address(struct usbnet *dev)
+{
+	int ret;
+	ret = mcs7830_get_reg(dev, HIF_REG_ETHERNET_ADDR, ETH_ALEN,
+				   dev->net->dev_addr);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int mcs7830_read_phy(struct usbnet *dev, u8 index)
+{
+	int ret;
+	int i;
+	__le16 val;
+
+	u8 cmd[2] = {
+		HIF_REG_PHY_CMD1_READ | HIF_REG_PHY_CMD1_PHYADDR,
+		HIF_REG_PHY_CMD2_PEND_FLAG_BIT | index,
+	};
+
+	mutex_lock(&mcs7830_phy_mutex);
+	/* write the MII command */
+	ret = mcs7830_set_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
+	if (ret < 0)
+		goto out;
+
+	/* wait for the data to become valid, should be within < 1ms */
+	for (i = 0; i < 10; i++) {
+		ret = mcs7830_get_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
+		if ((ret < 0) || (cmd[1] & HIF_REG_PHY_CMD2_READY_FLAG_BIT))
+			break;
+		ret = -EIO;
+		msleep(1);
+	}
+	if (ret < 0)
+		goto out;
+
+	/* read actual register contents */
+	ret = mcs7830_get_reg(dev, HIF_REG_PHY_DATA, 2, &val);
+	if (ret < 0)
+		goto out;
+	ret = le16_to_cpu(val);
+	dev_dbg(&dev->udev->dev, "read PHY reg %02x: %04x (%d tries)\n",
+		index, val, i);
+out:
+	mutex_unlock(&mcs7830_phy_mutex);
+	return ret;
+}
+
+static int mcs7830_write_phy(struct usbnet *dev, u8 index, u16 val)
+{
+	int ret;
+	int i;
+	__le16 le_val;
+
+	u8 cmd[2] = {
+		HIF_REG_PHY_CMD1_WRITE | HIF_REG_PHY_CMD1_PHYADDR,
+		HIF_REG_PHY_CMD2_PEND_FLAG_BIT | (index & 0x1F),
+	};
+
+	mutex_lock(&mcs7830_phy_mutex);
+
+	/* write the new register contents */
+	le_val = cpu_to_le16(val);
+	ret = mcs7830_set_reg(dev, HIF_REG_PHY_DATA, 2, &le_val);
+	if (ret < 0)
+		goto out;
+
+	/* write the MII command */
+	ret = mcs7830_set_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
+	if (ret < 0)
+		goto out;
+
+	/* wait for the command to be accepted by the PHY */
+	for (i = 0; i < 10; i++) {
+		ret = mcs7830_get_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
+		if ((ret < 0) || (cmd[1] & HIF_REG_PHY_CMD2_READY_FLAG_BIT))
+			break;
+		ret = -EIO;
+		msleep(1);
+	}
+	if (ret < 0)
+		goto out;
+
+	ret = 0;
+	dev_dbg(&dev->udev->dev, "write PHY reg %02x: %04x (%d tries)\n",
+		index, val, i);
+out:
+	mutex_unlock(&mcs7830_phy_mutex);
+	return ret;
+}
+
+/*
+ * This algorithm comes from the original mcs7830 version 1.4 driver,
+ * not sure if it is needed.
+ */
+static int mcs7830_set_autoneg(struct usbnet *dev, int ptrUserPhyMode)
+{
+	int ret;
+	/* Enable all media types */
+	ret = mcs7830_write_phy(dev, PHY_AUTONEGADVT_REG_INDEX, 0x05e1);
+	/* First Disable All */
+	if (!ret)
+		ret = mcs7830_write_phy(dev, PHY_CONTROL_REG_INDEX, 0x0000);
+	/* Enable Auto Neg */
+	if (!ret)
+		ret = mcs7830_write_phy(dev, PHY_CONTROL_REG_INDEX, 0x1000);
+	/* Restart Auto Neg (Keep the Enable Auto Neg Bit Set) */
+	if (!ret)
+		ret = mcs7830_write_phy(dev, PHY_CONTROL_REG_INDEX, 0x1200);
+	return ret < 0 ? : 0;
+}
+
+/*
+ * if we can read register 22, the chip revision is C
+ * or higher, and we need to set the pause threshold
+ */
+static void mcs7830_rev_C_fixup(struct usbnet *dev)
+{
+	u8 pause_threshold = HIF_REG_PAUSE_THRESHOLD_DEFAULT;
+	int retry;
+	u8 dummy[2];
+
+	for (retry = 0; retry < 2; retry ++) {
+		int ret;
+		ret = mcs7830_get_reg(dev, HIF_REG_22, 2, dummy);
+		if (ret > 0) {
+			dev_info(&dev->udev->dev, "applying rev.C fixup\n");
+			mcs7830_set_reg(dev, HIF_REG_PAUSE_THRESHOLD,
+					1, &pause_threshold);
+		}
+		msleep(1);
+	}
+}
+
+static int mcs7830_init_dev(struct usbnet *dev)
+{
+	int ret;
+	int retry;
+
+	/* Read MAC address from EEPROM */
+	ret = -EINVAL;
+	for (retry = 0; retry < 5 && ret; retry++)
+		ret = mcs7830_get_address(dev);
+	if (ret) {
+		dev_warn(&dev->udev->dev, "Cannot read MAC address\n");
+		goto out;
+	}
+
+	/* Set up PHY */
+	ret = mcs7830_set_autoneg(dev, 0);
+	if (ret) {
+		dev_info(&dev->udev->dev, "Cannot set autoneg\n");
+		goto out;
+	}
+
+	mcs7830_rev_C_fixup(dev);
+	ret = 0;
+out:
+	return ret;
+}
+
+static int mcs7830_mdio_read(struct net_device *netdev, int phy_id,
+			     int location)
+{
+	struct usbnet *dev = netdev->priv;
+	return mcs7830_read_phy(dev, location);
+}
+
+static void mcs7830_mdio_write(struct net_device *netdev, int phy_id,
+				int location, int val)
+{
+	struct usbnet *dev = netdev->priv;
+	mcs7830_write_phy(dev, location, val);
+}
+
+static int mcs7830_ioctl(struct net_device *net, struct ifreq *rq, int cmd)
+{
+	struct usbnet *dev = netdev_priv(net);
+	return generic_mii_ioctl(&dev->mii, if_mii(rq), cmd, NULL);
+}
+
+/* credits go to asix_set_multicast */
+static void mcs7830_set_multicast(struct net_device *net)
+{
+	struct usbnet *dev = netdev_priv(net);
+	struct mcs7830_data *data = (struct mcs7830_data *)&dev->data;
+
+	data->config = HIF_REG_CONFIG_TXENABLE;
+
+	/* this should not be needed, but it doesn't work otherwise */
+	data->config |= HIF_REG_CONFIG_ALLMULTICAST;
+
+	if (net->flags & IFF_PROMISC) {
+		data->config |= HIF_REG_CONFIG_PROMISCIOUS;
+	} else if (net->flags & IFF_ALLMULTI
+		   || net->mc_count > MCS7830_MAX_MCAST) {
+		data->config |= HIF_REG_CONFIG_ALLMULTICAST;
+	} else if (net->mc_count == 0) {
+		/* just broadcast and directed */
+	} else {
+		/* We use the 20 byte dev->data
+		 * for our 8 byte filter buffer
+		 * to avoid allocating memory that
+		 * is tricky to free later */
+		struct dev_mc_list *mc_list = net->mc_list;
+		u32 crc_bits;
+		int i;
+
+		memset(data->multi_filter, 0, sizeof data->multi_filter);
+
+		/* Build the multicast hash filter. */
+		for (i = 0; i < net->mc_count; i++) {
+			crc_bits = ether_crc(ETH_ALEN, mc_list->dmi_addr) >> 26;
+			data->multi_filter[crc_bits >> 3] |= 1 << (crc_bits & 7);
+			mc_list = mc_list->next;
+		}
+
+		mcs7830_set_reg_async(dev, HIF_REG_MULTICAST_HASH,
+				sizeof data->multi_filter,
+				data->multi_filter);
+	}
+
+	mcs7830_set_reg_async(dev, HIF_REG_CONFIG, 1, &data->config);
+}
+
+static int mcs7830_bind(struct usbnet *dev, struct usb_interface *udev)
+{
+	struct net_device *net = dev->net;
+	int ret;
+
+	ret = mcs7830_init_dev(dev);
+	if (ret)
+		goto out;
+
+	net->do_ioctl = mcs7830_ioctl;
+	net->set_multicast_list = mcs7830_set_multicast;
+	mcs7830_set_multicast(net);
+
+	dev->mii.mdio_read = mcs7830_mdio_read;
+	dev->mii.mdio_write = mcs7830_mdio_write;
+	dev->mii.dev = net;
+	dev->mii.phy_id_mask = 0x3f;
+	dev->mii.reg_num_mask = 0x1f;
+	dev->mii.phy_id = *((u8 *) net->dev_addr + 1);
+
+	dev->in = usb_rcvbulkpipe(dev->udev, 1);
+	dev->out = usb_sndbulkpipe(dev->udev, 2);
+out:
+	return ret;
+}
+
+static int mcs7830_check_connect(struct usbnet *dev)
+{
+	int ret;
+	ret = mcs7830_mdio_read(dev->net, dev->mii.phy_id, 1);
+	return !ret;
+}
+
+static void mcs7830_status(struct usbnet *dev, struct urb *urb)
+{
+	u16 *event;
+	int link;
+	int count, i;
+
+	if (urb->actual_length != 16) {
+		dev_info(&dev->udev->dev, "unexpected irq packet length %d\n",
+			 urb->actual_length);
+		return;
+	}
+
+	event = urb->transfer_buffer;
+
+	dev_dbg(&dev->udev->dev, "%04x %04x %04x %04x %04x %04x %04x %04x\n",
+		event[0], event[1], event[2], event[3],
+		event[4], event[5], event[6], event[7]);
+
+	/* count number of packets to receive */
+	for (count = 0, i = 0; i < 8; i++)
+		if (event[i] & 0x8000)
+			count++;
+	if (count)
+		dev_info(&dev->udev->dev, "received %d packets", count);
+
+	link = !!(event[0] & 0x2000);
+	if (netif_carrier_ok(dev->net) != link) {
+		if (link) {
+			netif_carrier_on(dev->net);
+			usbnet_defer_kevent(dev, EVENT_LINK_RESET);
+		} else
+			netif_carrier_off(dev->net);
+		dev_dbg(&dev->udev->dev, "link status is: %d", link);
+	}
+
+	return;
+}
+
+static const struct driver_info moschip_info = {
+	.description	= "MOSCHIP 7830 usb-NET adapter",
+	.bind		= mcs7830_bind,
+	.check_connect	= mcs7830_check_connect,
+	.status		= mcs7830_status,
+	.flags		= FLAG_ETHER,
+};
+
+static const struct usb_device_id products[] = {
+	{ USB_DEVICE(MCS7830_VENDOR_ID, MCS7830_PRODUCT_ID),
+	 .driver_info = (unsigned long) &moschip_info, },
+	{},
+};
+MODULE_DEVICE_TABLE(usb, products);
+
+static struct usb_driver mcs7830_driver = {
+	.name = driver_name,
+	.id_table = products,
+	.probe = usbnet_probe,
+	.disconnect = usbnet_disconnect,
+	.suspend = usbnet_suspend,
+	.resume = usbnet_resume,
+};
+
+static int __init mcs7830_init(void)
+{
+	return usb_register(&mcs7830_driver);
+}
+
+module_init(mcs7830_init);
+
+static void __exit mcs7830_exit(void)
+{
+	usb_deregister(&mcs7830_driver);
+}
+
+module_exit(mcs7830_exit);
+
+MODULE_DESCRIPTION("USB to network adapter MCS7830)");
+MODULE_LICENSE("GPL");
Index: linux-cg/drivers/usb/net/Kconfig
===================================================================
--- linux-cg.orig/drivers/usb/net/Kconfig	2006-08-07 02:40:31.000000000 +0200
+++ linux-cg/drivers/usb/net/Kconfig	2006-08-07 02:40:43.000000000 +0200
@@ -207,6 +207,14 @@
 	  Choose this option if you're using a host-to-host cable
 	  with one of these chips.
 
+config USB_NET_MCS7830
+	tristate "MosChip MCS7830 based Ethernet adapters"
+	depends on USB_USBNET
+	help
+	  Choose this option if you're using a 10/100 Ethernet USB2
+	  adapter based on the MosChip 7830 controller. This includes
+	  adapters marketed under the DeLOCK brand.
+
 config USB_NET_RNDIS_HOST
 	tristate "Host for RNDIS devices (EXPERIMENTAL)"
 	depends on USB_USBNET && EXPERIMENTAL
Index: linux-cg/drivers/usb/net/Makefile
===================================================================
--- linux-cg.orig/drivers/usb/net/Makefile	2006-08-07 02:40:31.000000000 +0200
+++ linux-cg/drivers/usb/net/Makefile	2006-08-07 02:40:43.000000000 +0200
@@ -14,6 +14,7 @@
 obj-$(CONFIG_USB_NET_RNDIS_HOST)	+= rndis_host.o
 obj-$(CONFIG_USB_NET_CDC_SUBSET)	+= cdc_subset.o
 obj-$(CONFIG_USB_NET_ZAURUS)	+= zaurus.o
+obj-$(CONFIG_USB_NET_MCS7830)	+= mcs7830.o
 obj-$(CONFIG_USB_USBNET)	+= usbnet.o
 
 ifeq ($(CONFIG_USB_DEBUG),y)

Re: [PATCH] please review mcs7830 (DeLOCK USB etherner) driver

[David Hollis]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

On Mon, 2006-08-07 at 15:00 +0200, Arnd Bergmann wrote:
> +
> +/* index for PHY registers */
> +enum {
> +	PHY_CONTROL_REG_INDEX			=  0,
> +	PHY_STATUS_REG_INDEX			=  1,
> +	PHY_IDENTIFICATION1_REG_INDEX		=  2,
> +	PHY_IDENTIFICATION2_REG_INDEX		=  3,
> +	PHY_AUTONEGADVT_REG_INDEX		=  4,
> +	PHY_AUTONEGLINK_REG_INDEX		=  5,
> +	PHY_AUTONEGEXP_REG_INDEX		=  6,

These values are dupes of the MII_xxxx constants from linux/mii.h.  It
would be clearer and more consistent to use those instead

> +	PHY_MIRROR_REG_INDEX			= 16,
> +	PHY_INTERRUPTENABLE_REG_INDEX		= 17,
> +	PHY_INTERRUPTSTATUS_REG_INDEX		= 18,
> +	PHY_CONFIG_REG_INDEX			= 19,
> +	PHY_CHIPSTATUS_REG_INDEX		= 20,
> +};

These values are device specific so you would want to define them here.
Following the MII_xxxxx naming convention may be helpful.

> +
> +static DEFINE_MUTEX(mcs7830_phy_mutex);
> +

Does this need to be global?  Isn't it really just to prevent
simultaneous access to the adapters PHY?  What if you have multiple
adapters installed?

> +
> +static int mcs7830_bind(struct usbnet *dev, struct usb_interface *udev)
> +{
> +	struct net_device *net = dev->net;
> +	int ret;
> +
> +	ret = mcs7830_init_dev(dev);
> +	if (ret)
> +		goto out;
> +
> +	net->do_ioctl = mcs7830_ioctl;
> +	net->set_multicast_list = mcs7830_set_multicast;
> +	mcs7830_set_multicast(net);
> +
> +	dev->mii.mdio_read = mcs7830_mdio_read;
> +	dev->mii.mdio_write = mcs7830_mdio_write;
> +	dev->mii.dev = net;
> +	dev->mii.phy_id_mask = 0x3f;
> +	dev->mii.reg_num_mask = 0x1f;
> +	dev->mii.phy_id = *((u8 *) net->dev_addr + 1);
> +
> +	dev->in = usb_rcvbulkpipe(dev->udev, 1);
> +	dev->out = usb_sndbulkpipe(dev->udev, 2);

Couldn't you use usbnet_getendpoints() here.  It will also pick up the
status pipe.

> +out:
> +	return ret;
> +}
> +
> +static int mcs7830_check_connect(struct usbnet *dev)
> +{
> +	int ret;
> +	ret = mcs7830_mdio_read(dev->net, dev->mii.phy_id, 1);

use MII_BMSR here instead of the magic value '1'.
> +	return !ret;
> +}
> +

-- 
David Hollis <dhollis@davehollis.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] please review mcs7830 (DeLOCK USB etherner) driver

[Arnd Bergmann]

On Monday 07 August 2006 16:54, David Hollis wrote:
> These values are dupes of the MII_xxxx constants from linux/mii.h.  It
> would be clearer and more consistent to use those instead

ok

> These values are device specific so you would want to define them here.
> Following the MII_xxxxx naming convention may be helpful.

ok

> > +
> > +static DEFINE_MUTEX(mcs7830_phy_mutex);
> > +
> 
> Does this need to be global?  Isn't it really just to prevent
> simultaneous access to the adapters PHY?  What if you have multiple
> adapters installed?

It's very rarely held, so I don't expect this to be a bottleneck,
even with a large number of adapters.

The implementation is slightly simpler this way, but I can move
the mutex to struct usbnet instead if you prefer.

> > +     dev->in = usb_rcvbulkpipe(dev->udev, 1);
> > +     dev->out = usb_sndbulkpipe(dev->udev, 2);
> 
> Couldn't you use usbnet_getendpoints() here.  It will also pick up the
> status pipe.

Yes. usbnet_getendpoints() didn't work at first, but I think
I found the problem with it now. I'll change that once I
have the interrupts sorted out.

> use MII_BMSR here instead of the magic value '1'.

ok

	Arnd <><

[PATCH] driver for mcs7830 (aka DeLOCK) USB ethernet adapter

[Arnd Bergmann]

This driver adds support for the DeLOCK USB ethernet adapter
and potentially others based on the MosChip MCS7830 chip.

It is based on the usbnet and asix drivers as well as the
original device driver provided by MosChip, which in turn
was based on the usbnet driver.

It has been tested successfully on an OHCI, but interestingly
there seems to be a problem with the mcs7830 when connected to
the ICH6/EHCI in my thinkpad: it keeps receiving lots of
broken packets in the RX interrupt. The problem goes away when
I'm using an active USB hub, so I assume it's not related to
the device driver, but rather to the hardware.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---

This version incorporates a few cleanups from myself an changes
based on comments from David Hollis. In particular, it now has

- an rx_fixup function that removes an out-of-band data byte
  from each received packet.
- got rid of the status function, which did not do the right thing
  and is not needed in this driver.
- has a working set_multicast function, although that one always
  needs to set allmulticast mode in order to get the chip to
  receive any frames.
- doesn't use a private mutex in its mii functions, that functionality
  is added in a separate patch to usbnet.

Please merge the driver in 2.6.19!

 drivers/usb/net/Kconfig   |    8
 drivers/usb/net/Makefile  |    1
 drivers/usb/net/mcs7830.c |  474 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 483 insertions(+)

Index: linux-cg/drivers/usb/net/mcs7830.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-cg/drivers/usb/net/mcs7830.c	2006-08-20 21:47:06.000000000 +0200
@@ -0,0 +1,474 @@
+/*
+ * MosChips MCS7830 based USB 2.0 Ethernet Devices
+ *
+ * based on usbnet.c, asix.c and the vendor provided mcs7830 driver
+ *
+ * Copyright (C) 2006 Arnd Bergmann <arnd@arndb.de>
+ * Copyright (C) 2003-2005 David Hollis <dhollis@davehollis.com>
+ * Copyright (C) 2005 Phil Chang <pchang23@sbcglobal.net>
+ * Copyright (c) 2002-2003 TiVo Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/crc32.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/init.h>
+#include <linux/mii.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/usb.h>
+
+#include "usbnet.h"
+
+/* requests */
+#define MCS7830_RD_BMREQ	(USB_DIR_IN  | USB_TYPE_VENDOR | USB_RECIP_DEVICE)
+#define MCS7830_WR_BMREQ	(USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE)
+#define MCS7830_RD_BREQ		0x0E
+#define MCS7830_WR_BREQ		0x0D
+
+#define MCS7830_CTRL_TIMEOUT	1000
+#define MCS7830_MAX_MCAST	64
+
+#define MCS7830_VENDOR_ID	0x9710
+#define MCS7830_PRODUCT_ID	0x7830
+
+/* HIF_REG_XX coressponding index value */
+enum {
+	HIF_REG_MULTICAST_HASH			= 0x00,
+	HIF_REG_PACKET_GAP1			= 0x08,
+	HIF_REG_PACKET_GAP2			= 0x09,
+	HIF_REG_PHY_DATA			= 0x0a,
+	HIF_REG_PHY_CMD1			= 0x0c,
+	   HIF_REG_PHY_CMD1_READ		= 0x40,
+	   HIF_REG_PHY_CMD1_WRITE		= 0x20,
+	   HIF_REG_PHY_CMD1_PHYADDR		= 0x01,
+	HIF_REG_PHY_CMD2			= 0x0d,
+	   HIF_REG_PHY_CMD2_PEND_FLAG_BIT	= 0x80,
+	   HIF_REG_PHY_CMD2_READY_FLAG_BIT	= 0x40,
+	HIF_REG_CONFIG				= 0x0e,
+	   HIF_REG_CONFIG_CFG			= 0x80,
+	   HIF_REG_CONFIG_SPEED100		= 0x40,
+	   HIF_REG_CONFIG_FULLDUPLEX_ENABLE	= 0x20,
+	   HIF_REG_CONFIG_RXENABLE		= 0x10,
+	   HIF_REG_CONFIG_TXENABLE		= 0x08,
+	   HIF_REG_CONFIG_SLEEPMODE		= 0x04,
+	   HIF_REG_CONFIG_ALLMULTICAST		= 0x02,
+	   HIF_REG_CONFIG_PROMISCIOUS		= 0x01,
+	HIF_REG_ETHERNET_ADDR			= 0x0f,
+	HIF_REG_22				= 0x15,
+	HIF_REG_PAUSE_THRESHOLD			= 0x16,
+	   HIF_REG_PAUSE_THRESHOLD_DEFAULT	= 0,
+};
+
+struct mcs7830_data {
+	u8 multi_filter[8];
+	u8 config;
+};
+
+static const char driver_name[] = "MOSCHIP usb-ethernet driver";
+
+static int mcs7830_get_reg(struct usbnet *dev, u16 index, u16 size, void *data)
+{
+	struct usb_device *xdev = dev->udev;
+	int ret;
+
+	ret = usb_control_msg(xdev, usb_rcvctrlpipe(xdev, 0), MCS7830_RD_BREQ,
+			      MCS7830_RD_BMREQ, 0x0000, index, data,
+			      size, msecs_to_jiffies(MCS7830_CTRL_TIMEOUT));
+	return ret;
+}
+
+static int mcs7830_set_reg(struct usbnet *dev, u16 index, u16 size, void *data)
+{
+	struct usb_device *xdev = dev->udev;
+	int ret;
+
+	ret = usb_control_msg(xdev, usb_sndctrlpipe(xdev, 0), MCS7830_WR_BREQ,
+			      MCS7830_WR_BMREQ, 0x0000, index, data,
+			      size, msecs_to_jiffies(MCS7830_CTRL_TIMEOUT));
+	return ret;
+}
+
+static void mcs7830_async_cmd_callback(struct urb *urb, struct pt_regs *regs)
+{
+	struct usb_ctrlrequest *req = (struct usb_ctrlrequest *)urb->context;
+
+	if (urb->status < 0)
+		printk(KERN_DEBUG "mcs7830_async_cmd_callback() failed with %d",
+			urb->status);
+
+	kfree(req);
+	usb_free_urb(urb);
+}
+
+static void mcs7830_set_reg_async(struct usbnet *dev, u16 index, u16 size, void *data)
+{
+	struct usb_ctrlrequest *req;
+	int ret;
+	struct urb *urb;
+
+	urb = usb_alloc_urb(0, GFP_ATOMIC);
+	if (!urb) {
+		dev_dbg(&dev->udev->dev, "Error allocating URB "
+				"in write_cmd_async!");
+		return;
+	}
+
+	req = kmalloc(sizeof *req, GFP_ATOMIC);
+	if (!req) {
+		dev_err(&dev->udev->dev, "Failed to allocate memory for "
+				"control request");
+		goto out;
+	}
+	req->bRequestType = MCS7830_WR_BMREQ;
+	req->bRequest = MCS7830_WR_BREQ;
+	req->wValue = 0;
+	req->wIndex = cpu_to_le16(index);
+	req->wLength = cpu_to_le16(size);
+
+	usb_fill_control_urb(urb, dev->udev,
+			     usb_sndctrlpipe(dev->udev, 0),
+			     (void *)req, data, size,
+			     mcs7830_async_cmd_callback, req);
+
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret < 0) {
+		dev_err(&dev->udev->dev, "Error submitting the control "
+				"message: ret=%d", ret);
+		goto out;
+	}
+	return;
+out:
+	kfree(req);
+	usb_free_urb(urb);
+}
+
+static int mcs7830_get_address(struct usbnet *dev)
+{
+	int ret;
+	ret = mcs7830_get_reg(dev, HIF_REG_ETHERNET_ADDR, ETH_ALEN,
+				   dev->net->dev_addr);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int mcs7830_read_phy(struct usbnet *dev, u8 index)
+{
+	int ret;
+	int i;
+	__le16 val;
+
+	u8 cmd[2] = {
+		HIF_REG_PHY_CMD1_READ | HIF_REG_PHY_CMD1_PHYADDR,
+		HIF_REG_PHY_CMD2_PEND_FLAG_BIT | index,
+	};
+
+	/* write the MII command */
+	ret = mcs7830_set_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
+	if (ret < 0)
+		goto out;
+
+	/* wait for the data to become valid, should be within < 1ms */
+	for (i = 0; i < 10; i++) {
+		ret = mcs7830_get_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
+		if ((ret < 0) || (cmd[1] & HIF_REG_PHY_CMD2_READY_FLAG_BIT))
+			break;
+		ret = -EIO;
+		msleep(1);
+	}
+	if (ret < 0)
+		goto out;
+
+	/* read actual register contents */
+	ret = mcs7830_get_reg(dev, HIF_REG_PHY_DATA, 2, &val);
+	if (ret < 0)
+		goto out;
+	ret = le16_to_cpu(val);
+	dev_dbg(&dev->udev->dev, "read PHY reg %02x: %04x (%d tries)\n",
+		index, val, i);
+out:
+	return ret;
+}
+
+static int mcs7830_write_phy(struct usbnet *dev, u8 index, u16 val)
+{
+	int ret;
+	int i;
+	__le16 le_val;
+
+	u8 cmd[2] = {
+		HIF_REG_PHY_CMD1_WRITE | HIF_REG_PHY_CMD1_PHYADDR,
+		HIF_REG_PHY_CMD2_PEND_FLAG_BIT | (index & 0x1F),
+	};
+
+	/* write the new register contents */
+	le_val = cpu_to_le16(val);
+	ret = mcs7830_set_reg(dev, HIF_REG_PHY_DATA, 2, &le_val);
+	if (ret < 0)
+		goto out;
+
+	/* write the MII command */
+	ret = mcs7830_set_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
+	if (ret < 0)
+		goto out;
+
+	/* wait for the command to be accepted by the PHY */
+	for (i = 0; i < 10; i++) {
+		ret = mcs7830_get_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
+		if ((ret < 0) || (cmd[1] & HIF_REG_PHY_CMD2_READY_FLAG_BIT))
+			break;
+		ret = -EIO;
+		msleep(1);
+	}
+	if (ret < 0)
+		goto out;
+
+	ret = 0;
+	dev_dbg(&dev->udev->dev, "write PHY reg %02x: %04x (%d tries)\n",
+		index, val, i);
+out:
+	return ret;
+}
+
+/*
+ * This algorithm comes from the original mcs7830 version 1.4 driver,
+ * not sure if it is needed.
+ */
+static int mcs7830_set_autoneg(struct usbnet *dev, int ptrUserPhyMode)
+{
+	int ret;
+	/* Enable all media types */
+	ret = mcs7830_write_phy(dev, MII_ADVERTISE, 0x05e1);
+	/* First Disable All */
+	if (!ret)
+		ret = mcs7830_write_phy(dev, MII_BMCR, 0x0000);
+	/* Enable Auto Neg */
+	if (!ret)
+		ret = mcs7830_write_phy(dev, MII_BMCR, 0x1000);
+	/* Restart Auto Neg (Keep the Enable Auto Neg Bit Set) */
+	if (!ret)
+		ret = mcs7830_write_phy(dev, MII_BMCR, 0x1200);
+	return ret < 0 ? : 0;
+}
+
+/*
+ * if we can read register 22, the chip revision is C
+ * or higher, and we need to set the pause threshold
+ */
+static void mcs7830_rev_C_fixup(struct usbnet *dev)
+{
+	u8 pause_threshold = HIF_REG_PAUSE_THRESHOLD_DEFAULT;
+	int retry;
+	u8 dummy[2];
+
+	for (retry = 0; retry < 2; retry ++) {
+		int ret;
+		ret = mcs7830_get_reg(dev, HIF_REG_22, 2, dummy);
+		if (ret > 0) {
+			dev_info(&dev->udev->dev, "applying rev.C fixup\n");
+			mcs7830_set_reg(dev, HIF_REG_PAUSE_THRESHOLD,
+					1, &pause_threshold);
+		}
+		msleep(1);
+	}
+}
+
+static int mcs7830_init_dev(struct usbnet *dev)
+{
+	int ret;
+	int retry;
+
+	/* Read MAC address from EEPROM */
+	ret = -EINVAL;
+	for (retry = 0; retry < 5 && ret; retry++)
+		ret = mcs7830_get_address(dev);
+	if (ret) {
+		dev_warn(&dev->udev->dev, "Cannot read MAC address\n");
+		goto out;
+	}
+
+	/* Set up PHY */
+	ret = mcs7830_set_autoneg(dev, 0);
+	if (ret) {
+		dev_info(&dev->udev->dev, "Cannot set autoneg\n");
+		goto out;
+	}
+
+	mcs7830_rev_C_fixup(dev);
+	ret = 0;
+out:
+	return ret;
+}
+
+static int mcs7830_mdio_read(struct net_device *netdev, int phy_id,
+			     int location)
+{
+	struct usbnet *dev = netdev->priv;
+	return mcs7830_read_phy(dev, location);
+}
+
+static void mcs7830_mdio_write(struct net_device *netdev, int phy_id,
+				int location, int val)
+{
+	struct usbnet *dev = netdev->priv;
+	mcs7830_write_phy(dev, location, val);
+}
+
+static int mcs7830_ioctl(struct net_device *net, struct ifreq *rq, int cmd)
+{
+	struct usbnet *dev = netdev_priv(net);
+	return generic_mii_ioctl(&dev->mii, if_mii(rq), cmd, NULL);
+}
+
+/* credits go to asix_set_multicast */
+static void mcs7830_set_multicast(struct net_device *net)
+{
+	struct usbnet *dev = netdev_priv(net);
+	struct mcs7830_data *data = (struct mcs7830_data *)&dev->data;
+
+	data->config = HIF_REG_CONFIG_TXENABLE;
+
+	/* this should not be needed, but it doesn't work otherwise */
+	data->config |= HIF_REG_CONFIG_ALLMULTICAST;
+
+	if (net->flags & IFF_PROMISC) {
+		data->config |= HIF_REG_CONFIG_PROMISCIOUS;
+	} else if (net->flags & IFF_ALLMULTI
+		   || net->mc_count > MCS7830_MAX_MCAST) {
+		data->config |= HIF_REG_CONFIG_ALLMULTICAST;
+	} else if (net->mc_count == 0) {
+		/* just broadcast and directed */
+	} else {
+		/* We use the 20 byte dev->data
+		 * for our 8 byte filter buffer
+		 * to avoid allocating memory that
+		 * is tricky to free later */
+		struct dev_mc_list *mc_list = net->mc_list;
+		u32 crc_bits;
+		int i;
+
+		memset(data->multi_filter, 0, sizeof data->multi_filter);
+
+		/* Build the multicast hash filter. */
+		for (i = 0; i < net->mc_count; i++) {
+			crc_bits = ether_crc(ETH_ALEN, mc_list->dmi_addr) >> 26;
+			data->multi_filter[crc_bits >> 3] |= 1 << (crc_bits & 7);
+			mc_list = mc_list->next;
+		}
+
+		mcs7830_set_reg_async(dev, HIF_REG_MULTICAST_HASH,
+				sizeof data->multi_filter,
+				data->multi_filter);
+	}
+
+	mcs7830_set_reg_async(dev, HIF_REG_CONFIG, 1, &data->config);
+}
+
+static int mcs7830_bind(struct usbnet *dev, struct usb_interface *udev)
+{
+	struct net_device *net = dev->net;
+	int ret;
+
+	ret = mcs7830_init_dev(dev);
+	if (ret)
+		goto out;
+
+	net->do_ioctl = mcs7830_ioctl;
+	net->set_multicast_list = mcs7830_set_multicast;
+	mcs7830_set_multicast(net);
+
+	dev->mii.mdio_read = mcs7830_mdio_read;
+	dev->mii.mdio_write = mcs7830_mdio_write;
+	dev->mii.dev = net;
+	dev->mii.phy_id_mask = 0x3f;
+	dev->mii.reg_num_mask = 0x1f;
+	dev->mii.phy_id = *((u8 *) net->dev_addr + 1);
+
+	ret = usbnet_get_endpoints(dev, udev);
+out:
+	return ret;
+}
+
+static int mcs7830_check_connect(struct usbnet *dev)
+{
+	int ret;
+	ret = mcs7830_mdio_read(dev->net, dev->mii.phy_id, MII_BMSR);
+	return !ret;
+}
+
+/* The chip always appends a status bytes that we need to strip */
+static int mcs7830_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
+{
+	u8 status;
+
+	if (skb->len == 0) {
+		dev_err(&dev->udev->dev, "unexpected empty rx frame\n");
+		return 0;
+	}
+
+	skb_trim(skb, skb->len - 1);
+	status = skb->data[skb->len];
+
+	if (status != 0x20)
+		dev_dbg(&dev->udev->dev, "rx fixup status %x\n", status);
+
+	return skb->len > 0;
+}
+
+static const struct driver_info moschip_info = {
+	.description	= "MOSCHIP 7830 usb-NET adapter",
+	.bind		= mcs7830_bind,
+	.check_connect	= mcs7830_check_connect,
+	.rx_fixup	= mcs7830_rx_fixup,
+	.flags		= FLAG_ETHER,
+	.in		= 1,
+	.out		= 2,
+};
+
+static const struct usb_device_id products[] = {
+	{
+		USB_DEVICE(MCS7830_VENDOR_ID, MCS7830_PRODUCT_ID),
+		.driver_info = (unsigned long) &moschip_info,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(usb, products);
+
+static struct usb_driver mcs7830_driver = {
+	.name = driver_name,
+	.id_table = products,
+	.probe = usbnet_probe,
+	.disconnect = usbnet_disconnect,
+	.suspend = usbnet_suspend,
+	.resume = usbnet_resume,
+};
+
+static int __init mcs7830_init(void)
+{
+	return usb_register(&mcs7830_driver);
+}
+module_init(mcs7830_init);
+
+static void __exit mcs7830_exit(void)
+{
+	usb_deregister(&mcs7830_driver);
+}
+module_exit(mcs7830_exit);
+
+MODULE_DESCRIPTION("USB to network adapter MCS7830)");
+MODULE_LICENSE("GPL");
Index: linux-cg/drivers/usb/net/Kconfig
===================================================================
--- linux-cg.orig/drivers/usb/net/Kconfig	2006-08-20 21:32:06.000000000 +0200
+++ linux-cg/drivers/usb/net/Kconfig	2006-08-20 21:32:08.000000000 +0200
@@ -207,6 +207,14 @@
 	  Choose this option if you're using a host-to-host cable
 	  with one of these chips.
 
+config USB_NET_MCS7830
+	tristate "MosChip MCS7830 based Ethernet adapters"
+	depends on USB_USBNET
+	help
+	  Choose this option if you're using a 10/100 Ethernet USB2
+	  adapter based on the MosChip 7830 controller. This includes
+	  adapters marketed under the DeLOCK brand.
+
 config USB_NET_RNDIS_HOST
 	tristate "Host for RNDIS devices (EXPERIMENTAL)"
 	depends on USB_USBNET && EXPERIMENTAL
Index: linux-cg/drivers/usb/net/Makefile
===================================================================
--- linux-cg.orig/drivers/usb/net/Makefile	2006-08-20 21:32:06.000000000 +0200
+++ linux-cg/drivers/usb/net/Makefile	2006-08-20 21:32:08.000000000 +0200
@@ -14,6 +14,7 @@
 obj-$(CONFIG_USB_NET_RNDIS_HOST)	+= rndis_host.o
 obj-$(CONFIG_USB_NET_CDC_SUBSET)	+= cdc_subset.o
 obj-$(CONFIG_USB_NET_ZAURUS)	+= zaurus.o
+obj-$(CONFIG_USB_NET_MCS7830)	+= mcs7830.o
 obj-$(CONFIG_USB_USBNET)	+= usbnet.o
 
 ifeq ($(CONFIG_USB_DEBUG),y)

[PATCH] usbnet: add a mutex around phy register access

[Arnd Bergmann]

When working on the mcs7830, I noticed the need for a mutex in its
mdio_read/mdio_write functions. A related problem seems to be present
in the asix driver in the respective functions.

This introduces a mutex in the common usbnet driver and uses it
from the two hardware specific drivers.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---

 drivers/usb/net/asix.c    |    4 ++++
 drivers/usb/net/mcs7830.c |    5 +++++
 drivers/usb/net/usbnet.c  |    1 +
 drivers/usb/net/usbnet.h  |    1 +
 4 files changed, 11 insertions(+)

Index: linux-cg/drivers/usb/net/asix.c
===================================================================
--- linux-cg.orig/drivers/usb/net/asix.c	2006-08-20 21:22:43.000000000 +0200
+++ linux-cg/drivers/usb/net/asix.c	2006-08-20 22:09:31.000000000 +0200
@@ -328,10 +328,12 @@
 	struct usbnet *dev = netdev_priv(netdev);
 	u16 res;
 
+	mutex_lock(&dev->phy_mutex);
 	asix_set_sw_mii(dev);
 	asix_read_cmd(dev, AX_CMD_READ_MII_REG, phy_id,
 				(__u16)loc, 2, (u16 *)&res);
 	asix_set_hw_mii(dev);
+	mutex_unlock(&dev->phy_mutex);
 
 	return res & 0xffff;
 }
@@ -348,10 +350,12 @@
 	struct usbnet *dev = netdev_priv(netdev);
 	u16 res = val;
 
+	mutex_lock(&dev->phy_mutex);
 	asix_set_sw_mii(dev);
 	asix_write_cmd(dev, AX_CMD_WRITE_MII_REG, phy_id,
 				(__u16)loc, 2, (u16 *)&res);
 	asix_set_hw_mii(dev);
+	mutex_unlock(&dev->phy_mutex);
 }
 
 /* same as above, but converts new value to le16 byte order before writing */
Index: linux-cg/drivers/usb/net/usbnet.c
===================================================================
--- linux-cg.orig/drivers/usb/net/usbnet.c	2006-08-20 21:22:43.000000000 +0200
+++ linux-cg/drivers/usb/net/usbnet.c	2006-08-20 22:09:31.000000000 +0200
@@ -1070,6 +1070,7 @@
 	dev->delay.function = usbnet_bh;
 	dev->delay.data = (unsigned long) dev;
 	init_timer (&dev->delay);
+	mutex_init (&dev->phy_mutex);
 
 	SET_MODULE_OWNER (net);
 	dev->net = net;
Index: linux-cg/drivers/usb/net/usbnet.h
===================================================================
--- linux-cg.orig/drivers/usb/net/usbnet.h	2006-08-20 21:22:43.000000000 +0200
+++ linux-cg/drivers/usb/net/usbnet.h	2006-08-20 22:09:31.000000000 +0200
@@ -30,6 +30,7 @@
 	struct usb_device	*udev;
 	struct driver_info	*driver_info;
 	wait_queue_head_t	*wait;
+	struct mutex		phy_mutex;
 
 	/* i/o info: pipes etc */
 	unsigned		in, out;
Index: linux-cg/drivers/usb/net/mcs7830.c
===================================================================
--- linux-cg.orig/drivers/usb/net/mcs7830.c	2006-08-20 21:47:06.000000000 +0200
+++ linux-cg/drivers/usb/net/mcs7830.c	2006-08-20 22:09:31.000000000 +0200
@@ -178,6 +178,7 @@
 		HIF_REG_PHY_CMD2_PEND_FLAG_BIT | index,
 	};
 
+	mutex_lock(&dev->phy_mutex);
 	/* write the MII command */
 	ret = mcs7830_set_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
 	if (ret < 0)
@@ -202,6 +203,7 @@
 	dev_dbg(&dev->udev->dev, "read PHY reg %02x: %04x (%d tries)\n",
 		index, val, i);
 out:
+	mutex_unlock(&dev->phy_mutex);
 	return ret;
 }
 
@@ -216,6 +218,8 @@
 		HIF_REG_PHY_CMD2_PEND_FLAG_BIT | (index & 0x1F),
 	};
 
+	mutex_lock(&dev->phy_mutex);
+
 	/* write the new register contents */
 	le_val = cpu_to_le16(val);
 	ret = mcs7830_set_reg(dev, HIF_REG_PHY_DATA, 2, &le_val);
@@ -242,6 +246,7 @@
 	dev_dbg(&dev->udev->dev, "write PHY reg %02x: %04x (%d tries)\n",
 		index, val, i);
 out:
+	mutex_unlock(&dev->phy_mutex);
 	return ret;
 }
 

Re: [PATCH] driver for mcs7830 (aka DeLOCK) USB ethernet adapter

[David Hollis]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On Sun, 2006-08-20 at 22:07 +0200, Arnd Bergmann wrote:

> +static int mcs7830_set_autoneg(struct usbnet *dev, int ptrUserPhyMode)
> +{
> +	int ret;
> +	/* Enable all media types */
> +	ret = mcs7830_write_phy(dev, MII_ADVERTISE, 0x05e1);
> +	/* First Disable All */
> +	if (!ret)
> +		ret = mcs7830_write_phy(dev, MII_BMCR, 0x0000);
> +	/* Enable Auto Neg */
> +	if (!ret)
> +		ret = mcs7830_write_phy(dev, MII_BMCR, 0x1000);
> +	/* Restart Auto Neg (Keep the Enable Auto Neg Bit Set) */
> +	if (!ret)
> +		ret = mcs7830_write_phy(dev, MII_BMCR, 0x1200);
> +	return ret < 0 ? : 0;
> +}

include/linux/mii.h also has defines for the flags for MII_ADVERTISE and
MII_BMCR:

So your 0x1200 can be 'BMCR_ANENABLE | BMCR_ANRESTART' for example.
Makes it easier to tell whats going on.

Other than that, it's looking pretty good.


-- 
David Hollis <dhollis@davehollis.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] mcs7830: clean up use of kernel constants

[Arnd Bergmann]

This use the MII register constants provided
by the kernel instead of hardcoding numerical
values in the driver.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---

Index: linux-cg/drivers/usb/net/mcs7830.c
===================================================================
--- linux-cg.orig/drivers/usb/net/mcs7830.c	2006-08-25 21:17:24.000000000 +0200
+++ linux-cg/drivers/usb/net/mcs7830.c	2006-08-25 21:23:35.000000000 +0200
@@ -35,8 +35,10 @@
 #include "usbnet.h"
 
 /* requests */
-#define MCS7830_RD_BMREQ	(USB_DIR_IN  | USB_TYPE_VENDOR | USB_RECIP_DEVICE)
-#define MCS7830_WR_BMREQ	(USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE)
+#define MCS7830_RD_BMREQ	(USB_DIR_IN  | USB_TYPE_VENDOR | \
+				 USB_RECIP_DEVICE)
+#define MCS7830_WR_BMREQ	(USB_DIR_OUT | USB_TYPE_VENDOR | \
+				 USB_RECIP_DEVICE)
 #define MCS7830_RD_BREQ		0x0E
 #define MCS7830_WR_BREQ		0x0D
 
@@ -46,6 +48,10 @@
 #define MCS7830_VENDOR_ID	0x9710
 #define MCS7830_PRODUCT_ID	0x7830
 
+#define MCS7830_MII_ADVERTISE	(ADVERTISE_PAUSE_CAP | ADVERTISE_100FULL | \
+				 ADVERTISE_100HALF | ADVERTISE_10FULL | \
+				 ADVERTISE_10HALF | ADVERTISE_CSMA)
+
 /* HIF_REG_XX coressponding index value */
 enum {
 	HIF_REG_MULTICAST_HASH			= 0x00,
@@ -258,16 +264,18 @@
 {
 	int ret;
 	/* Enable all media types */
-	ret = mcs7830_write_phy(dev, MII_ADVERTISE, 0x05e1);
-	/* First Disable All */
+	ret = mcs7830_write_phy(dev, MII_ADVERTISE, MCS7830_MII_ADVERTISE);
+
+	/* First reset BMCR */
 	if (!ret)
 		ret = mcs7830_write_phy(dev, MII_BMCR, 0x0000);
 	/* Enable Auto Neg */
 	if (!ret)
-		ret = mcs7830_write_phy(dev, MII_BMCR, 0x1000);
+		ret = mcs7830_write_phy(dev, MII_BMCR, BMCR_ANENABLE);
 	/* Restart Auto Neg (Keep the Enable Auto Neg Bit Set) */
 	if (!ret)
-		ret = mcs7830_write_phy(dev, MII_BMCR, 0x1200);
+		ret = mcs7830_write_phy(dev, MII_BMCR,
+				BMCR_ANENABLE | BMCR_ANRESTART	);
 	return ret < 0 ? : 0;
 }
 

[PATCH] mcs7830: fix reception of 1514 byte frames

[Arnd Bergmann]

The mcs7830 chip always appends a byte with status information
to an rx frame, so the URB needs to reserve an extra byte.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Index: linux-cg/drivers/usb/net/mcs7830.c
===================================================================
--- linux-cg.orig/drivers/usb/net/mcs7830.c	2006-08-25 21:23:35.000000000 +0200
+++ linux-cg/drivers/usb/net/mcs7830.c	2006-08-25 21:26:02.000000000 +0200
@@ -405,6 +405,9 @@
 	net->set_multicast_list = mcs7830_set_multicast;
 	mcs7830_set_multicast(net);
 
+	/* reserve space for the status byte on rx */
+	dev->rx_urb_size = ETH_FRAME_LEN + 1;
+
 	dev->mii.mdio_read = mcs7830_mdio_read;
 	dev->mii.mdio_write = mcs7830_mdio_write;
 	dev->mii.dev = net;

Re: [PATCH] mcs7830: clean up use of kernel constants

[David Hollis]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

On Sun, 2006-08-27 at 22:41 +0200, Arnd Bergmann wrote:
> This use the MII register constants provided
> by the kernel instead of hardcoding numerical
> values in the driver.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: David Hollis <dhollis@davehollis.com>

-- 
David Hollis <dhollis@davehollis.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [linux-usb-devel] [PATCH] mcs7830: fix reception of 1514 byte frames

[David Brownell]

On Sunday 27 August 2006 1:41 pm, Arnd Bergmann wrote:
> The mcs7830 chip always appends a byte with status information
> to an rx frame, so the URB needs to reserve an extra byte.

Shouldn't you add VLAN_HLEN too, in case 802.1q is in use?

I'm not entirely sure how that's expected to work myself...

Re: [linux-usb-devel] [PATCH] driver for mcs7830 (aka DeLOCK) USB ethernet adapter

[David Brownell]

On Sunday 20 August 2006 1:07 pm, Arnd Bergmann wrote:
> This driver adds support for the DeLOCK USB ethernet adapter
> and potentially others based on the MosChip MCS7830 chip.
> 
> It is based on the usbnet and asix drivers as well as the
> original device driver provided by MosChip, which in turn
> was based on the usbnet driver.
> 
> It has been tested successfully on an OHCI, but interestingly
> there seems to be a problem with the mcs7830 when connected to
> the ICH6/EHCI in my thinkpad: it keeps receiving lots of
> broken packets in the RX interrupt.

That is, the "status" polling which you disabled??  If so, please
update this comment ...

> The problem goes away when 
> I'm using an active USB hub, so I assume it's not related to
> the device driver, but rather to the hardware.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Looks basically OK to me, although I'd rather see the two patches
you posted on 27-August be merged into it before an upstream merge.
(To use normal MII constants, and handle max size frames.)


> ---
> 
> This version incorporates a few cleanups from myself an changes
> based on comments from David Hollis. 

He has more experience than I do with respect to these sorts of
real Ethernet adapters and usbnet.  :)

Speaking of which ... isnt this driver missing a hook to make
the MII stuff visible through ethtool?

- Dave

> In particular, it now has 
> 
> - an rx_fixup function that removes an out-of-band data byte
>   from each received packet.
> - got rid of the status function, which did not do the right thing
>   and is not needed in this driver.
> - has a working set_multicast function, although that one always
>   needs to set allmulticast mode in order to get the chip to
>   receive any frames.
> - doesn't use a private mutex in its mii functions, that functionality
>   is added in a separate patch to usbnet.
> 
> Please merge the driver in 2.6.19!
> 
>  drivers/usb/net/Kconfig   |    8
>  drivers/usb/net/Makefile  |    1
>  drivers/usb/net/mcs7830.c |  474 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 483 insertions(+)
> 

Re: [linux-usb-devel] [PATCH] driver for mcs7830 (aka DeLOCK) USB ethernet adapter

[Arnd Bergmann]

On Saturday 02 September 2006 12:38, David Brownell wrote:
> > 
> > It has been tested successfully on an OHCI, but interestingly
> > there seems to be a problem with the mcs7830 when connected to
> > the ICH6/EHCI in my thinkpad: it keeps receiving lots of
> > broken packets in the RX interrupt.
> 
> That is, the "status" polling which you disabled??  If so, please
> update this comment ...

No, the receive errors are independent from the status interrupt.
I have now got confirmation by another user that they also
happen on a different thinkpad when not using a USB hub, but
with a hub everything seems fine.

> > The problem goes away when 
> > I'm using an active USB hub, so I assume it's not related to
> > the device driver, but rather to the hardware.
> > 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> Looks basically OK to me, although I'd rather see the two patches
> you posted on 27-August be merged into it before an upstream merge.
> (To use normal MII constants, and handle max size frames.)

Ok, I can resend.

> > This version incorporates a few cleanups from myself an changes
> > based on comments from David Hollis. 
> 
> He has more experience than I do with respect to these sorts of
> real Ethernet adapters and usbnet.  :)
> 
> Speaking of which ... isnt this driver missing a hook to make
> the MII stuff visible through ethtool?

hmm, I wasn't aware that ethtool does this. I did check that mii-tool
works though.

Going through the ethtool operations, I think that it should be
possible to implement a few of them, including ETHTOOL_GREGS,
ETHTOOL_GEEPROM, ETHTOOL_SEEPROM, ETHTOOL_NWAY_RST and ETHTOOL_GPERMADDR.
Do you think these should be done?

	Arnd <><

-- 
VGER BF report: U 0.49989

Re: [linux-usb-devel] [PATCH] mcs7830: fix reception of 1514 byte frames

[Arnd Bergmann]

On Saturday 02 September 2006 12:33, David Brownell wrote:
> Shouldn't you add VLAN_HLEN too, in case 802.1q is in use?
> 
> I'm not entirely sure how that's expected to work myself...
> 
I don't know if I can expect that device to even do VLAN frames
correctly, the specs don't mention it at all.

There is however a mention of 'large frame whose length is >1518 Bytes',
so it might be good to allow larger frames anyway, though I can't find
any information about what maximum size is supported.

	Arnd <><

-- 
VGER BF report: U 0.5

Re: [linux-usb-devel] [PATCH] driver for mcs7830 (aka DeLOCK) USB ethernet adapter

[David Brownell]

> No, the receive errors are independent from the status interrupt.
> I have now got confirmation by another user that they also
> happen on a different thinkpad when not using a USB hub, but
> with a hub everything seems fine.

Odd.  Hardware issues, I guess ... I'd like to understand better
just why high speed peripherals are acting up on root hubs.  Maybe
it's flakey motherboards ...


> > Speaking of which ... isnt this driver missing a hook to make
> > the MII stuff visible through ethtool?
>
> hmm, I wasn't aware that ethtool does this. I did check that mii-tool
> works though.

Then my main concern is gone.


> Going through the ethtool operations, I think that it should be
> possible to implement a few of them, including ETHTOOL_GREGS,
> ETHTOOL_GEEPROM, ETHTOOL_SEEPROM, ETHTOOL_NWAY_RST and ETHTOOL_GPERMADDR.
> Do you think these should be done?

I've not got much use for those, but maybe the netdev folk would
care.  That's probably not enough to hold up any merge.

- Dave


-- 
VGER BF report: U 0.499998

Re: [linux-usb-devel] [PATCH] driver for mcs7830 (aka DeLOCK) USB ethernet adapter

[David Hollis]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

On Sun, 2006-09-03 at 23:40 -0700, David Brownell wrote:

> 
> > Going through the ethtool operations, I think that it should be
> > possible to implement a few of them, including ETHTOOL_GREGS,
> > ETHTOOL_GEEPROM, ETHTOOL_SEEPROM, ETHTOOL_NWAY_RST and ETHTOOL_GPERMADDR.
> > Do you think these should be done?
> 
> I've not got much use for those, but maybe the netdev folk would
> care.  That's probably not enough to hold up any merge.

Definitely not showstoppers, but nice touches just to ensure that all
things work as expected.  The ones of most importance as I see it are
the get/set_settings ones, which you can just pass-thru to the
mii_ethtool_Xset() calls and get_drvinfo().  All others are somewhat
optional and do vary from device to device.

-- 
David Hollis <dhollis@davehollis.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]