[U-Boot] [PATCH v1 0/5] usb: eth: introduce Moschip MCS7830 driver

[U-Boot] [PATCH v1 0/5] usb: eth: introduce Moschip MCS7830 driver

[Gerhard Sittig]

this series
- adds a new USB ethernet driver for adapters that are based on the
  MCS7730/7830/7832 chips
- enables the driver for those boards which previously had support for
  "all other" USB ethernet adapters
- updates the README.usb documentation file to list all available
  drivers for USB ethernet adapters

development and tests were done on a taskit stamp9g20 with Delock and
Logilink adapters, running TFTP transfers of a file as large as RAM is

there are several checkpatch warnings
- about CamelCase for NetReceive() and USB related structure members,
  which cannot get fixed as the names are in the established API
- about a multiple assignment for the "found" flags in the USB endpoint
  search, which I consider acceptable

hopefully I got the Cc: list right, Marek as the USB custodian, Simon
for introducing the Asix driver in the past -- unfortunately I could not
determine a "network" custodian nor one outstanding usb-eth committer

Gerhard Sittig (5):
  usb: net: introduce support for Moschip USB ethernet
  arm: config: alpha-sort USB ethernet items for Asix and SMSC
  arm: config: enable Moschip USB ethernet support for several boards
  arm: config: enable USB ethernet for taskit stamp9g20
  usb: doc: update README.doc to list all USB ethernet options

 doc/README.usb                 |   13 +-
 drivers/usb/eth/Makefile       |    1 +
 drivers/usb/eth/mcs7830.c      |  677 ++++++++++++++++++++++++++++++++++++++++
 drivers/usb/eth/usb_ether.c    |    7 +
 include/configs/harmony.h      |    3 +-
 include/configs/m53evk.h       |    1 +
 include/configs/mx53loco.h     |    1 +
 include/configs/nitrogen6x.h   |    1 +
 include/configs/omap3_beagle.h |    3 +-
 include/configs/stamp9g20.h    |    5 +-
 include/usb_ether.h            |    8 +
 11 files changed, 715 insertions(+), 5 deletions(-)
 create mode 100644 drivers/usb/eth/mcs7830.c

-- 
1.7.10.4

[U-Boot] [PATCH v1 1/5] usb: net: introduce support for Moschip USB ethernet

[Gerhard Sittig]

introduce an 'mcs7830' driver for Moschip based USB ethernet adapters,
which was implemented based on the U-Boot Asix driver with additional
information gathered from the Moschip Linux driver

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
the driver was tested with a Logilink and a Delock product each, both
of which contain an MCS7830 chip rev C, by several times transferring
a file of 50MB size via TFTP (on a board with a total of 64MB RAM)

---
 drivers/usb/eth/Makefile    |    1 +
 drivers/usb/eth/mcs7830.c   |  677 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/eth/usb_ether.c |    7 +
 include/usb_ether.h         |    8 +
 4 files changed, 693 insertions(+)
 create mode 100644 drivers/usb/eth/mcs7830.c

diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
index 03f54749f720..94551c4c0c9a 100644
--- a/drivers/usb/eth/Makefile
+++ b/drivers/usb/eth/Makefile
@@ -8,4 +8,5 @@ obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
 ifdef CONFIG_USB_ETHER_ASIX
 obj-y += asix.o
 endif
+obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
 obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o
diff --git a/drivers/usb/eth/mcs7830.c b/drivers/usb/eth/mcs7830.c
new file mode 100644
index 000000000000..60cacc752578
--- /dev/null
+++ b/drivers/usb/eth/mcs7830.c
@@ -0,0 +1,677 @@
+/*
+ * Copyright (c) 2013 Gerhard Sittig <gsi@denx.de>
+ * based on the U-Boot Asix driver as well as information
+ * from the Linux Moschip driver
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/*
+ * MOSCHIP MCS7830 based (7730/7830/7832) USB 2.0 Ethernet Devices
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <linux/mii.h>
+#include <malloc.h>
+#include <usb.h>
+
+#include "usb_ether.h"
+
+/* global declarations {{{ */
+
+#define MCS7830_BASE_NAME	"mcs"
+
+#define USB_CTRL_SET_TIMEOUT	5000
+#define USB_CTRL_GET_TIMEOUT	5000
+#define USB_BULK_SEND_TIMEOUT	5000
+#define USB_BULK_RECV_TIMEOUT	5000
+#define PHY_CONNECT_TIMEOUT	5000	/* link status, connect timeout */
+#define PHY_CONNECT_TIMEOUT_RES	50	/* link status, resolution in msec */
+
+#define MCS7830_RX_URB_SIZE	2048
+
+#ifndef BIT
+#define BIT(n)	(1 << (n))
+#endif
+
+/* command opcodes */
+enum {
+	MCS7830_WR_BREQ			= 0x0d,
+	MCS7830_RD_BREQ			= 0x0e,
+};
+
+/* register offsets, field bitmasks and default values */
+enum {
+	REG_MULTICAST_HASH	= 0x00,
+	REG_PHY_DATA		= 0x0a,
+	REG_PHY_CMD1		= 0x0c,
+		PHY_CMD1_READ		= BIT(6),
+		PHY_CMD1_WRITE		= BIT(5),
+		PHY_CMD1_PHYADDR	= BIT(0),
+	REG_PHY_CMD2		= 0x0d,
+		PHY_CMD2_PEND_FLAG_BIT	= BIT(7),
+		PHY_CMD2_READY_FLAG_BIT	= BIT(6),
+	REG_CONFIG		= 0x0e,
+		CONFIG_CFG		= BIT(7),
+		CONFIG_SPEED100		= BIT(6),
+		CONFIG_FDX_ENABLE	= BIT(5),
+		CONFIG_RXENABLE		= BIT(4),
+		CONFIG_TXENABLE		= BIT(3),
+		CONFIG_SLEEPMODE	= BIT(2),
+		CONFIG_ALLMULTICAST	= BIT(1),
+		CONFIG_PROMISCUOUS	= BIT(0),
+	REG_ETHER_ADDR		= 0x0f,
+	REG_FRAME_DROP_COUNTER	= 0x15,
+	REG_PAUSE_THRESHOLD	= 0x16,
+		PAUSE_THRESHOLD_DEFAULT	= 0,
+};
+
+/* trailing status byte after RX frame */
+enum {
+	STAT_RX_FRAME_CORRECT	= BIT(5),
+	STAT_RX_LARGE_FRAME	= BIT(4),
+	STAT_RX_CRC_ERROR	= BIT(3),
+	STAT_RX_ALIGNMENT_ERROR	= BIT(2),
+	STAT_RX_LENGTH_ERROR	= BIT(1),
+	STAT_RX_SHORT_FRAME	= BIT(0),
+};
+
+struct mcs7830_private {
+	uint8_t config;
+	uint8_t mchash[8];
+};
+
+/* }}} global declarations */
+/* private helper routines {{{ */
+
+static int mcs7830_read_reg(struct ueth_data *dev, uint8_t idx,
+			    uint16_t size, void *data)
+{
+	int len;
+	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, size);
+
+	debug("%s() idx=0x%04X sz=%d\n", __func__, idx, size);
+
+	len = usb_control_msg(dev->pusb_dev,
+			      usb_rcvctrlpipe(dev->pusb_dev, 0),
+			      MCS7830_RD_BREQ,
+			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      0, idx, buf, size,
+			      USB_CTRL_GET_TIMEOUT);
+	if (len == size) {
+		memcpy(data, buf, size);
+		return 0;
+	}
+	debug("%s() len=%d != sz=%d\n", __func__, len, size);
+	return -1;
+}
+
+static int mcs7830_write_reg(struct ueth_data *dev, uint8_t idx,
+			     uint16_t size, void *data)
+{
+	int len;
+	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, size);
+
+	debug("%s() idx=0x%04X sz=%d\n", __func__, idx, size);
+
+	memcpy(buf, data, size);
+	len = usb_control_msg(dev->pusb_dev,
+			      usb_sndctrlpipe(dev->pusb_dev, 0),
+			      MCS7830_WR_BREQ,
+			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      0, idx, buf, size,
+			      USB_CTRL_SET_TIMEOUT);
+	if (len != size)
+		debug("%s() len=%d != sz=%d\n", __func__, len, size);
+	return (len == size) ? 0 : -1;
+}
+
+static int mcs7830_read_phy(struct ueth_data *dev, uint8_t index)
+{
+	int rc;
+	int retry;
+	uint8_t cmd[2];
+	uint16_t val;
+
+	/* send the PHY read request */
+	cmd[0] = PHY_CMD1_READ | PHY_CMD1_PHYADDR;
+	cmd[1] = PHY_CMD2_PEND_FLAG_BIT | (index & 0x1f);
+	rc = mcs7830_write_reg(dev, REG_PHY_CMD1, sizeof(cmd), cmd);
+	if (rc < 0)
+		return rc;
+
+	/* wait for the response to become available (usually < 1ms) */
+	retry = 10;
+	do {
+		rc = mcs7830_read_reg(dev, REG_PHY_CMD1, sizeof(cmd), cmd);
+		if (rc < 0)
+			return rc;
+		if (cmd[1] & PHY_CMD2_READY_FLAG_BIT)
+			break;
+		if (!retry)
+			return -EIO;
+		mdelay(1);
+	} while (--retry);
+
+	/* fetch the response data */
+	rc = mcs7830_read_reg(dev, REG_PHY_DATA, sizeof(val), &val);
+	if (rc < 0)
+		return rc;
+	rc = le16_to_cpu(val);
+	debug("%s(%s, %d) => 0x%04X\n", __func__, dev->eth_dev.name, index, rc);
+	return rc;
+}
+
+static int mcs7830_write_phy(struct ueth_data *dev, uint8_t index, uint16_t val)
+{
+	int rc;
+	int retry;
+	uint8_t cmd[2];
+
+	debug("%s(%s, %d, 0x%04X)\n", __func__, dev->eth_dev.name, index, val);
+
+	/* send the PHY data to get written */
+	val = cpu_to_le16(val);
+	rc = mcs7830_write_reg(dev, REG_PHY_DATA, sizeof(val), &val);
+	if (rc < 0)
+		return rc;
+
+	/* send the PHY write request */
+	cmd[0] = PHY_CMD1_WRITE | PHY_CMD1_PHYADDR;
+	cmd[1] = PHY_CMD2_PEND_FLAG_BIT | (index & 0x1f);
+	rc = mcs7830_write_reg(dev, REG_PHY_CMD1, sizeof(cmd), cmd);
+	if (rc < 0)
+		return rc;
+
+	/* wait for the command to get accepted */
+	retry = 10;
+	do {
+		rc = mcs7830_read_reg(dev, REG_PHY_CMD1, sizeof(cmd), cmd);
+		if (rc < 0)
+			return rc;
+		if (cmd[1] & PHY_CMD2_READY_FLAG_BIT)
+			break;
+		if (!retry)
+			return -EIO;
+		mdelay(1);
+	} while (--retry);
+
+	return 0;
+}
+
+static int mcs7830_read_config(struct eth_device *eth)
+{
+	struct ueth_data *dev;
+	int rc;
+	uint8_t cfg;
+
+	debug("%s()\n", __func__);
+	dev = eth->priv;
+
+	rc = mcs7830_read_reg(dev, REG_CONFIG, sizeof(cfg), &cfg);
+	if (rc < 0) {
+		debug("reading config from adapter failed\n");
+		return -1;
+	}
+
+	debug("%s() reg=0x%02X [ %s %s %s %s %s %s %s %s ]\n",
+	      __func__, cfg,
+	      (cfg & CONFIG_CFG) ? "cfg" : "-",
+	      (cfg & CONFIG_SPEED100) ? "100" : "-",
+	      (cfg & CONFIG_FDX_ENABLE) ? "fdx" : "-",
+	      (cfg & CONFIG_RXENABLE) ? "rx" : "-",
+	      (cfg & CONFIG_TXENABLE) ? "tx" : "-",
+	      (cfg & CONFIG_SLEEPMODE) ? "sleep" : "-",
+	      (cfg & CONFIG_ALLMULTICAST) ? "multi" : "-",
+	      (cfg & CONFIG_PROMISCUOUS) ? "promisc" : "-");
+	return cfg;
+}
+
+static int mcs7830_write_config(struct eth_device *eth)
+{
+	struct ueth_data *dev;
+	struct mcs7830_private *priv;
+	int rc;
+
+	debug("%s()\n", __func__);
+	dev = eth->priv;
+	priv = dev->dev_priv;
+
+	rc = mcs7830_write_reg(dev, REG_CONFIG,
+			       sizeof(priv->config), &priv->config);
+	if (rc < 0) {
+		debug("writing config to adapter failed\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+static int mcs7830_write_mchash(struct eth_device *eth)
+{
+	struct ueth_data *dev;
+	struct mcs7830_private *priv;
+	int rc;
+
+	debug("%s()\n", __func__);
+	dev = eth->priv;
+	priv = dev->dev_priv;
+
+	rc = mcs7830_write_reg(dev, REG_MULTICAST_HASH,
+			       sizeof(priv->mchash), &priv->mchash);
+	if (rc < 0) {
+		debug("writing multicast hash to adapter failed\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+static int mcs7830_set_autoneg(struct ueth_data *dev)
+{
+	int adv, flg;
+	int rc;
+
+	debug("%s()\n", __func__);
+
+	/*
+	 * algorithm taken from the Linux driver, which took it from
+	 * "the original mcs7830 version 1.4 driver":
+	 *
+	 * enable all media, reset BMCR, enable auto neg, restart
+	 * auto neg while keeping the enable auto neg flag set
+	 */
+
+	adv = ADVERTISE_PAUSE_CAP | ADVERTISE_ALL | ADVERTISE_CSMA;
+	rc = mcs7830_write_phy(dev, MII_ADVERTISE, adv);
+
+	flg = 0;
+	if (!rc)
+		rc = mcs7830_write_phy(dev, MII_BMCR, flg);
+
+	flg |= BMCR_ANENABLE;
+	if (!rc)
+		rc = mcs7830_write_phy(dev, MII_BMCR, flg);
+
+	flg |= BMCR_ANRESTART;
+	if (!rc)
+		rc = mcs7830_write_phy(dev, MII_BMCR, flg);
+
+	return rc;
+}
+
+static int mcs7830_get_rev(struct ueth_data *dev)
+{
+	uint8_t buf[2];
+	int rc;
+	int rev;
+
+	/* register 22 is readable in rev C and higher */
+	rc = mcs7830_read_reg(dev, REG_FRAME_DROP_COUNTER, sizeof(buf), buf);
+	if (rc < 0)
+		rev = 1;
+	else
+		rev = 2;
+	debug("%s() rc=%d, rev=%d\n", __func__, rc, rev);
+	return rev;
+}
+
+static int mcs7830_apply_fixup(struct ueth_data *dev)
+{
+	int rev;
+	int i;
+	uint8_t thr;
+
+	rev = mcs7830_get_rev(dev);
+	debug("%s() rev=%d\n", __func__, rev);
+
+	/*
+	 * rev C requires setting the pause threshold (the Linux driver
+	 * is inconsistent, the implementation does it for "rev C
+	 * exactly", the introductory comment says "rev C and above")
+	 */
+	if (rev == 2) {
+		debug("%s: applying rev C fixup\n", dev->eth_dev.name);
+		thr = PAUSE_THRESHOLD_DEFAULT;
+		for (i = 0; i < 2; i++) {
+			(void)mcs7830_write_reg(dev, REG_PAUSE_THRESHOLD,
+						sizeof(thr), &thr);
+			mdelay(1);
+		}
+	}
+
+	return 0;
+}
+
+static int mcs7830_basic_reset(struct ueth_data *dev)
+{
+	struct mcs7830_private *priv;
+	int rc;
+
+	debug("%s()\n", __func__);
+	priv = dev->dev_priv;
+
+	/*
+	 * comment from the respective Linux driver, which
+	 * unconditionally sets the ALLMULTICAST flag as well:
+	 * should not be needed, but does not work otherwise
+	 */
+	priv->config = CONFIG_TXENABLE;
+	priv->config |= CONFIG_ALLMULTICAST;
+
+	rc = mcs7830_set_autoneg(dev);
+	if (rc < 0) {
+		error("setting autoneg failed\n");
+		return rc;
+	}
+
+	rc = mcs7830_write_mchash(dev);
+	if (rc < 0) {
+		error("failed to set multicast hash\n");
+		return rc;
+	}
+
+	rc = mcs7830_write_config(dev);
+	if (rc < 0) {
+		error("failed to set configuration\n");
+		return rc;
+	}
+
+	rc = mcs7830_apply_fixup(dev);
+	if (rc < 0) {
+		error("fixup application failed\n");
+		return rc;
+	}
+
+	return 0;
+}
+
+static int mcs7830_read_mac(struct eth_device *eth)
+{
+	struct ueth_data *dev;
+	int ret;
+	uint8_t buf[ETH_ALEN];
+
+	debug("%s()\n", __func__);
+	dev = eth->priv;
+
+	ret = mcs7830_read_reg(dev, REG_ETHER_ADDR, ETH_ALEN, buf);
+	if (ret < 0) {
+		debug("reading MAC from adapter failed\n");
+		return -1;
+	}
+
+	memcpy(&eth->enetaddr[0], buf, ETH_ALEN);
+	return 0;
+}
+
+static int mcs7830_write_mac(struct eth_device *eth)
+{
+	struct ueth_data *dev;
+	int ret;
+
+	debug("%s()\n", __func__);
+	dev = eth->priv;
+
+	if (sizeof(eth->enetaddr) != ETH_ALEN)
+		return -1;
+	ret = mcs7830_write_reg(dev, REG_ETHER_ADDR, ETH_ALEN, eth->enetaddr);
+	if (ret < 0) {
+		debug("writing MAC to adapter failed\n");
+		return -1;
+	}
+	return 0;
+}
+
+/* }}} private helper routines */
+/* private callbacks init/send/recv/halt {{{ */
+
+static int mcs7830_init(struct eth_device *eth, bd_t *bd)
+{
+	struct ueth_data *dev;
+	int timeout;
+	int have_link;
+
+	debug("%s()\n", __func__);
+	dev = eth->priv;
+
+	timeout = 0;
+	do {
+		have_link = mcs7830_read_phy(dev, MII_BMSR) & BMSR_LSTATUS;
+		if (!have_link) {
+			if (!timeout)
+				puts("Waiting for ethernet connection ... ");
+			udelay(PHY_CONNECT_TIMEOUT_RES * 1000);
+			timeout += PHY_CONNECT_TIMEOUT_RES;
+		}
+	} while (!have_link && timeout < PHY_CONNECT_TIMEOUT);
+	if (have_link) {
+		if (timeout)
+			puts("done.\n");
+		return 0;
+	} else {
+		puts("unable to connect.\n");
+		return -1;
+	}
+}
+
+static int mcs7830_send(struct eth_device *eth, void *packet, int length)
+{
+	struct ueth_data *dev;
+	int rc;
+	int gotlen;
+	/* there is a status byte after the ethernet frame */
+	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, PKTSIZE + sizeof(uint8_t));
+
+	dev = eth->priv;
+
+	memcpy(buf, packet, length);
+	rc = usb_bulk_msg(dev->pusb_dev,
+			  usb_sndbulkpipe(dev->pusb_dev, dev->ep_out),
+			  &buf[0], length, &gotlen,
+			  USB_BULK_SEND_TIMEOUT);
+	debug("%s() TX want len %d, got len %d, rc %d\n",
+	      __func__, length, gotlen, rc);
+	return rc;
+}
+
+static int mcs7830_recv(struct eth_device *eth)
+{
+	struct ueth_data *dev;
+	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, MCS7830_RX_URB_SIZE);
+	int rc, wantlen, gotlen;
+	uint8_t sts;
+
+	debug("%s()\n", __func__);
+	dev = eth->priv;
+
+	/* fetch input data from the adapter */
+	wantlen = MCS7830_RX_URB_SIZE;
+	rc = usb_bulk_msg(dev->pusb_dev,
+			  usb_rcvbulkpipe(dev->pusb_dev, dev->ep_in),
+			  &buf[0], wantlen, &gotlen,
+			  USB_BULK_RECV_TIMEOUT);
+	debug("%s() RX want len %d, got len %d, rc %d\n",
+	      __func__, wantlen, gotlen, rc);
+	if (rc != 0) {
+		error("RX: failed to receive\n");
+		return -1;
+	}
+	if (gotlen > wantlen) {
+		error("RX: got too many bytes (%d)\n", gotlen);
+		return -1;
+	}
+
+	/*
+	 * the bulk message that we received from USB contains exactly
+	 * one ethernet frame and a trailing status byte
+	 */
+	if (gotlen < sizeof(sts))
+		return -1;
+	gotlen -= sizeof(sts);
+	sts = buf[gotlen];
+
+	if (sts == STAT_RX_FRAME_CORRECT) {
+		debug("%s() got a frame, len=%d\n", __func__, gotlen);
+		NetReceive(buf, gotlen);
+		return 0;
+	}
+
+	debug("RX: frame error (sts 0x%02X, %s %s %s %s %s)\n",
+	      sts,
+	      (sts & STAT_RX_LARGE_FRAME) ? "large" : "-",
+	      (sts & STAT_RX_LENGTH_ERROR) ?  "length" : "-",
+	      (sts & STAT_RX_SHORT_FRAME) ? "short" : "-",
+	      (sts & STAT_RX_CRC_ERROR) ? "crc" : "-",
+	      (sts & STAT_RX_ALIGNMENT_ERROR) ?  "align" : "-");
+	return -1;
+}
+
+static void mcs7830_halt(struct eth_device *eth)
+{
+	debug("%s()\n", __func__);
+}
+
+/* }}} init/send/recv/halt */
+/* public routines probe/getinfo {{{ */
+
+static int curr_eth_dev;	/* index for name of next device detected */
+
+void mcs7830_eth_before_probe(void)
+{
+	curr_eth_dev = 0;
+}
+
+static const struct mcs7830_dongle {
+	uint16_t vendor;
+	uint16_t product;
+} const mcs7830_dongles[] = {
+	{ 0x9710, 0x7832, },	/* Moschip 7832 */
+	{ 0x9710, 0x7830, },	/* Moschip 7830 */
+	{ 0x9710, 0x7730, },	/* Moschip 7730 */
+	{ 0x0df6, 0x0021, }, /* Sitecom LN 30 */
+	{ /* sentinel */ },
+};
+
+int mcs7830_eth_probe(struct usb_device *dev, unsigned int ifnum,
+		      struct ueth_data *ss)
+{
+	struct usb_interface *iface;
+	struct usb_interface_descriptor *iface_desc;
+	int i;
+	struct mcs7830_private *priv;
+	int ep_in_found, ep_out_found, ep_intr_found;
+
+	debug("%s()\n", __func__);
+
+	/* iterate the list of supported dongles */
+	iface = &dev->config.if_desc[ifnum];
+	iface_desc = &iface->desc;
+	for (i = 0; mcs7830_dongles[i].vendor; i++) {
+		if (dev->descriptor.idVendor == mcs7830_dongles[i].vendor &&
+		    dev->descriptor.idProduct == mcs7830_dongles[i].product)
+			break;
+	}
+	if (!mcs7830_dongles[i].vendor)
+		return 0;
+	debug("detected USB ethernet device: %04X:%04X\n",
+	      dev->descriptor.idVendor, dev->descriptor.idProduct);
+
+	/* fill in driver private data */
+	priv = calloc(1, sizeof(*priv));
+	if (!priv)
+		return 0;
+
+	/* fill in the ueth_data structure, attach private data */
+	memset(ss, 0, sizeof(*ss));
+	ss->ifnum = ifnum;
+	ss->pusb_dev = dev;
+	ss->subclass = iface_desc->bInterfaceSubClass;
+	ss->protocol = iface_desc->bInterfaceProtocol;
+	ss->dev_priv = priv;
+
+	/*
+	 * a minimum of three endpoints is expected: in (bulk),
+	 * out (bulk), and interrupt; ignore all others
+	 */
+	ep_in_found = ep_out_found = ep_intr_found = 0;
+	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
+		uint8_t eptype, epaddr;
+		bool is_input;
+
+		eptype = iface->ep_desc[i].bmAttributes;
+		eptype &= USB_ENDPOINT_XFERTYPE_MASK;
+
+		epaddr = iface->ep_desc[i].bEndpointAddress;
+		is_input = epaddr & USB_DIR_IN;
+		epaddr &= USB_ENDPOINT_NUMBER_MASK;
+
+		if (eptype == USB_ENDPOINT_XFER_BULK) {
+			if (is_input && !ep_in_found) {
+				ss->ep_in = epaddr;
+				ep_in_found++;
+			}
+			if (!is_input && !ep_out_found) {
+				ss->ep_out = epaddr;
+				ep_out_found++;
+			}
+		}
+
+		if (eptype == USB_ENDPOINT_XFER_INT) {
+			if (is_input && !ep_intr_found) {
+				ss->ep_int = epaddr;
+				ss->irqinterval = iface->ep_desc[i].bInterval;
+				ep_intr_found++;
+			}
+		}
+	}
+	debug("endpoints: in %d, out %d, intr %d\n",
+	      ss->ep_in, ss->ep_out, ss->ep_int);
+
+	/* apply basic sanity checks */
+	if (usb_set_interface(dev, iface_desc->bInterfaceNumber, 0) ||
+	    !ss->ep_in || !ss->ep_out || !ss->ep_int) {
+		debug("device probe incomplete\n");
+		return 0;
+	}
+
+	dev->privptr = ss;
+	return 1;
+}
+
+int mcs7830_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
+			 struct eth_device *eth)
+{
+	debug("%s()\n", __func__);
+	if (!eth) {
+		debug("%s: missing parameter.\n", __func__);
+		return 0;
+	}
+
+	snprintf(eth->name, sizeof(eth->name), "%s%d",
+		 MCS7830_BASE_NAME, curr_eth_dev++);
+	eth->init = mcs7830_init;
+	eth->send = mcs7830_send;
+	eth->recv = mcs7830_recv;
+	eth->halt = mcs7830_halt;
+	eth->write_hwaddr = mcs7830_write_mac;
+	eth->priv = ss;
+
+	if (mcs7830_basic_reset(ss))
+		return 0;
+
+	(void)mcs7830_read_config;
+#ifdef DEBUG
+	(void)mcs7830_read_config(eth);
+#endif
+
+	if (mcs7830_read_mac(eth))
+		return 0;
+	debug("MAC %pM\n", eth->enetaddr);
+
+	return 1;
+}
+
+/* }}} public routines */
+/* vim:set foldmethod=marker: */
diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
index 2c4126be3601..1dda54c2f116 100644
--- a/drivers/usb/eth/usb_ether.c
+++ b/drivers/usb/eth/usb_ether.c
@@ -30,6 +30,13 @@ static const struct usb_eth_prob_dev prob_dev[] = {
 		.get_info = asix_eth_get_info,
 	},
 #endif
+#ifdef CONFIG_USB_ETHER_MCS7830
+	{
+		.before_probe = mcs7830_eth_before_probe,
+		.probe = mcs7830_eth_probe,
+		.get_info = mcs7830_eth_get_info,
+	},
+#endif
 #ifdef CONFIG_USB_ETHER_SMSC95XX
 	{
 		.before_probe = smsc95xx_eth_before_probe,
diff --git a/include/usb_ether.h b/include/usb_ether.h
index 678c9dff2524..ddc11fb69560 100644
--- a/include/usb_ether.h
+++ b/include/usb_ether.h
@@ -51,6 +51,14 @@ int asix_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
 		      struct eth_device *eth);
 #endif
 
+#ifdef CONFIG_USB_ETHER_MCS7830
+void mcs7830_eth_before_probe(void);
+int mcs7830_eth_probe(struct usb_device *dev, unsigned int ifnum,
+		      struct ueth_data *ss);
+int mcs7830_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
+			 struct eth_device *eth);
+#endif
+
 #ifdef CONFIG_USB_ETHER_SMSC95XX
 void smsc95xx_eth_before_probe(void);
 int smsc95xx_eth_probe(struct usb_device *dev, unsigned int ifnum,
-- 
1.7.10.4

[U-Boot] [PATCH v1 2/5] arm: config: alpha-sort USB ethernet items for Asix and SMSC

[Gerhard Sittig]

adjust the harmony and omap3_beagle board configs to make
their CONFIG_USB_ETHER_* items appear in alphabetical order

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 include/configs/harmony.h      |    2 +-
 include/configs/omap3_beagle.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/configs/harmony.h b/include/configs/harmony.h
index d733be9cd5b6..fa66c665ec8c 100644
--- a/include/configs/harmony.h
+++ b/include/configs/harmony.h
@@ -61,8 +61,8 @@
 
 /* USB networking support */
 #define CONFIG_USB_HOST_ETHER
-#define CONFIG_USB_ETHER_SMSC95XX
 #define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_SMSC95XX
 
 /* General networking support */
 #define CONFIG_CMD_NET
diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h
index c58bc91a50c5..e01a6d9547f9 100644
--- a/include/configs/omap3_beagle.h
+++ b/include/configs/omap3_beagle.h
@@ -120,8 +120,8 @@
 
 #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3
 #define CONFIG_USB_HOST_ETHER
-#define CONFIG_USB_ETHER_SMSC95XX
 #define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_SMSC95XX
 
 /* GPIO banks */
 #define CONFIG_OMAP3_GPIO_5		/* GPIO128..159 is in GPIO bank 5 */
-- 
1.7.10.4

[U-Boot] [PATCH v1 3/5] arm: config: enable Moschip USB ethernet support for several boards

[Gerhard Sittig]

enable support for the Moschip USB ethernet adapter for those boards
which previously had support for "all other" USB ethernet adapters
(that's Asix _and_ SMSC) enabled -- which applies to harmony, m53evk,
mx53loco, nitrogen6x, omap3_beagle

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 include/configs/harmony.h      |    1 +
 include/configs/m53evk.h       |    1 +
 include/configs/mx53loco.h     |    1 +
 include/configs/nitrogen6x.h   |    1 +
 include/configs/omap3_beagle.h |    1 +
 5 files changed, 5 insertions(+)

diff --git a/include/configs/harmony.h b/include/configs/harmony.h
index fa66c665ec8c..34b43faeb079 100644
--- a/include/configs/harmony.h
+++ b/include/configs/harmony.h
@@ -62,6 +62,7 @@
 /* USB networking support */
 #define CONFIG_USB_HOST_ETHER
 #define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_MCS7830
 #define CONFIG_USB_ETHER_SMSC95XX
 
 /* General networking support */
diff --git a/include/configs/m53evk.h b/include/configs/m53evk.h
index a344af457392..a57ac166ba90 100644
--- a/include/configs/m53evk.h
+++ b/include/configs/m53evk.h
@@ -183,6 +183,7 @@
 #define CONFIG_USB_STORAGE
 #define CONFIG_USB_HOST_ETHER
 #define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_MCS7830
 #define CONFIG_USB_ETHER_SMSC95XX
 #define CONFIG_MXC_USB_PORT		1
 #define CONFIG_MXC_USB_PORTSC		(PORT_PTS_UTMI | PORT_PTS_PTW)
diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h
index ae43ea3c1f28..df1f377fdd6d 100644
--- a/include/configs/mx53loco.h
+++ b/include/configs/mx53loco.h
@@ -65,6 +65,7 @@
 #define CONFIG_USB_STORAGE
 #define CONFIG_USB_HOST_ETHER
 #define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_MCS7830
 #define CONFIG_USB_ETHER_SMSC95XX
 #define CONFIG_MXC_USB_PORT	1
 #define CONFIG_MXC_USB_PORTSC	(PORT_PTS_UTMI | PORT_PTS_PTW)
diff --git a/include/configs/nitrogen6x.h b/include/configs/nitrogen6x.h
index e44ec88f71b3..b412cc5cd9ca 100644
--- a/include/configs/nitrogen6x.h
+++ b/include/configs/nitrogen6x.h
@@ -115,6 +115,7 @@
 #define CONFIG_USB_STORAGE
 #define CONFIG_USB_HOST_ETHER
 #define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_MCS7830
 #define CONFIG_USB_ETHER_SMSC95XX
 #define CONFIG_USB_MAX_CONTROLLER_COUNT 2
 #define CONFIG_EHCI_HCD_INIT_AFTER_RESET	/* For OTG port */
diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h
index e01a6d9547f9..73eea304d19f 100644
--- a/include/configs/omap3_beagle.h
+++ b/include/configs/omap3_beagle.h
@@ -121,6 +121,7 @@
 #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3
 #define CONFIG_USB_HOST_ETHER
 #define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_MCS7830
 #define CONFIG_USB_ETHER_SMSC95XX
 
 /* GPIO banks */
-- 
1.7.10.4

[U-Boot] [PATCH v1 4/5] arm: config: enable USB ethernet for taskit stamp9g20

[Gerhard Sittig]

enabling CONFIG_MACB makes other locations in the stamp config file
enable network related commands (actually prevents disabling them)

enable USB ethernet support by activating generic support as well as
Asix and Moschip ethernet adapters

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 include/configs/stamp9g20.h |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/configs/stamp9g20.h b/include/configs/stamp9g20.h
index 51339b1496e6..01085dc5c114 100644
--- a/include/configs/stamp9g20.h
+++ b/include/configs/stamp9g20.h
@@ -140,7 +140,10 @@
  * can enable it here if your baseboard features ethernet.
  */
 
-/* #define CONFIG_MACB */
+#define CONFIG_MACB
+#define CONFIG_USB_HOST_ETHER
+#define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_MCS7830
 
 #ifdef CONFIG_MACB
 # define CONFIG_RMII			/* use reduced MII inteface */
-- 
1.7.10.4

[U-Boot] [PATCH v1 5/5] usb: doc: update README.doc to list all USB ethernet options

[Gerhard Sittig]

- extend the discussion of USB network related config options such that
  all available adapter drivers are listed, and that the 'usb' command
  for the interactive prompt and scripting becomes available
- suggest to *not* put individual IP configuration parameters into the
  exectuable, but instead to put them into external environment or fetch
  them from network

Signed-off-by: Gerhard Sittig <gsi@denx.de>
---
 doc/README.usb |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/doc/README.usb b/doc/README.usb
index 65fb2886d958..bc768a385450 100644
--- a/doc/README.usb
+++ b/doc/README.usb
@@ -127,8 +127,14 @@ To enable USB Host Ethernet in U-Boot, your platform must of course
 support USB with CONFIG_CMD_USB enabled and working. You will need to
 add some config settings to your board header file:
 
+#define CONFIG_CMD_USB		/* the 'usb' interactive command */
 #define CONFIG_USB_HOST_ETHER	/* Enable USB Ethernet adapters */
-#define CONFIG_USB_ETHER_ASIX	/* Asix, or whatever driver(s) you want */
+
+and one or more of the following for individual adapter hardware:
+
+#define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_MCS7830
+#define CONFIG_USB_ETHER_SMSC95XX
 
 As with built-in networking, you will also want to enable some network
 commands, for example:
@@ -148,7 +154,10 @@ settings should start you off:
 
 You can also set the default IP address of your board and the server
 as well as the default file to load when a 'bootp' command is issued.
-All of these can be obtained from the bootp server if not set.
+However note that encoding these individual network settings into a
+common exectuable is discouraged, as it leads to potential conflicts,
+and all the parameters can either get stored in the board's external
+environment, or get obtained from the bootp server if not set.
 
 #define CONFIG_IPADDR		10.0.0.2  (replace with your value)
 #define CONFIG_SERVERIP		10.0.0.1  (replace with your value)
-- 
1.7.10.4

[U-Boot] [PATCH v1 0/5] usb: eth: introduce Moschip MCS7830 driver

[Marek Vasut]

On Monday, February 17, 2014 at 12:01:06 AM, Gerhard Sittig wrote:
> this series
> - adds a new USB ethernet driver for adapters that are based on the
>   MCS7730/7830/7832 chips
> - enables the driver for those boards which previously had support for
>   "all other" USB ethernet adapters
> - updates the README.usb documentation file to list all available
>   drivers for USB ethernet adapters
> 
> development and tests were done on a taskit stamp9g20 with Delock and
> Logilink adapters, running TFTP transfers of a file as large as RAM is
> 
> there are several checkpatch warnings
> - about CamelCase for NetReceive() and USB related structure members,
>   which cannot get fixed as the names are in the established API
> - about a multiple assignment for the "found" flags in the USB endpoint
>   search, which I consider acceptable
> 
> hopefully I got the Cc: list right, Marek as the USB custodian, Simon
> for introducing the Asix driver in the past -- unfortunately I could not
> determine a "network" custodian nor one outstanding usb-eth committer

CCing Joe, see [1] ;-)

[1] http://www.denx.de/wiki/U-Boot/Custodians

Best regards,
Marek Vasut

[U-Boot] [PATCH v1 1/5] usb: net: introduce support for Moschip USB ethernet

[Marek Vasut]

On Monday, February 17, 2014 at 12:01:07 AM, Gerhard Sittig wrote:
> introduce an 'mcs7830' driver for Moschip based USB ethernet adapters,
> which was implemented based on the U-Boot Asix driver with additional
> information gathered from the Moschip Linux driver
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
> the driver was tested with a Logilink and a Delock product each, both
> of which contain an MCS7830 chip rev C, by several times transferring
> a file of 50MB size via TFTP (on a board with a total of 64MB RAM)

Did you also try tftpput ? ;-)

[...]

> +#define USB_CTRL_SET_TIMEOUT	5000
> +#define USB_CTRL_GET_TIMEOUT	5000
> +#define USB_BULK_SEND_TIMEOUT	5000
> +#define USB_BULK_RECV_TIMEOUT	5000
> +#define PHY_CONNECT_TIMEOUT	5000	/* link status, connect timeout */

Why don't we use just one TIMEOUT for all ?

> +#define PHY_CONNECT_TIMEOUT_RES	50	/* link status, resolution in 
msec */
> +
> +#define MCS7830_RX_URB_SIZE	2048
> +
> +#ifndef BIT
> +#define BIT(n)	(1 << (n))
> +#endif

This should probably be in include/common.h or so.

> +/* command opcodes */
> +enum {
> +	MCS7830_WR_BREQ			= 0x0d,
> +	MCS7830_RD_BREQ			= 0x0e,
> +};
> +
> +/* register offsets, field bitmasks and default values */
> +enum {
> +	REG_MULTICAST_HASH	= 0x00,
> +	REG_PHY_DATA		= 0x0a,
> +	REG_PHY_CMD1		= 0x0c,
> +		PHY_CMD1_READ		= BIT(6),
> +		PHY_CMD1_WRITE		= BIT(5),
> +		PHY_CMD1_PHYADDR	= BIT(0),
> +	REG_PHY_CMD2		= 0x0d,
> +		PHY_CMD2_PEND_FLAG_BIT	= BIT(7),
> +		PHY_CMD2_READY_FLAG_BIT	= BIT(6),
> +	REG_CONFIG		= 0x0e,
> +		CONFIG_CFG		= BIT(7),
> +		CONFIG_SPEED100		= BIT(6),
> +		CONFIG_FDX_ENABLE	= BIT(5),
> +		CONFIG_RXENABLE		= BIT(4),
> +		CONFIG_TXENABLE		= BIT(3),
> +		CONFIG_SLEEPMODE	= BIT(2),
> +		CONFIG_ALLMULTICAST	= BIT(1),
> +		CONFIG_PROMISCUOUS	= BIT(0),
> +	REG_ETHER_ADDR		= 0x0f,
> +	REG_FRAME_DROP_COUNTER	= 0x15,
> +	REG_PAUSE_THRESHOLD	= 0x16,
> +		PAUSE_THRESHOLD_DEFAULT	= 0,
> +};

Why don't you just use structure with padding as the rest of the U-Boot does ? 
Like so:

struct regs {
 u8 reg1;
 u8 pad1[n];
 u8 reg2;
...
};

> +
> +/* trailing status byte after RX frame */
> +enum {
> +	STAT_RX_FRAME_CORRECT	= BIT(5),
> +	STAT_RX_LARGE_FRAME	= BIT(4),
> +	STAT_RX_CRC_ERROR	= BIT(3),
> +	STAT_RX_ALIGNMENT_ERROR	= BIT(2),
> +	STAT_RX_LENGTH_ERROR	= BIT(1),
> +	STAT_RX_SHORT_FRAME	= BIT(0),
> +};

I am not quite fond of the enum for bits, but I don't care enough either .

> +struct mcs7830_private {
> +	uint8_t config;
> +	uint8_t mchash[8];
> +};
> +
> +/* }}} global declarations */
> +/* private helper routines {{{ */
> +
> +static int mcs7830_read_reg(struct ueth_data *dev, uint8_t idx,
> +			    uint16_t size, void *data)
> +{
> +	int len;
> +	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, size);
> +
> +	debug("%s() idx=0x%04X sz=%d\n", __func__, idx, size);
> +
> +	len = usb_control_msg(dev->pusb_dev,
> +			      usb_rcvctrlpipe(dev->pusb_dev, 0),
> +			      MCS7830_RD_BREQ,
> +			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +			      0, idx, buf, size,
> +			      USB_CTRL_GET_TIMEOUT);
> +	if (len == size) {
> +		memcpy(data, buf, size);
> +		return 0;
> +	}
> +	debug("%s() len=%d != sz=%d\n", __func__, len, size);
> +	return -1;

Use errno.h defines for return codes please.

> +}
> +
> +static int mcs7830_write_reg(struct ueth_data *dev, uint8_t idx,
> +			     uint16_t size, void *data)
> +{
> +	int len;
> +	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, size);
> +
> +	debug("%s() idx=0x%04X sz=%d\n", __func__, idx, size);
> +
> +	memcpy(buf, data, size);
> +	len = usb_control_msg(dev->pusb_dev,
> +			      usb_sndctrlpipe(dev->pusb_dev, 0),
> +			      MCS7830_WR_BREQ,
> +			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +			      0, idx, buf, size,
> +			      USB_CTRL_SET_TIMEOUT);
> +	if (len != size)
> +		debug("%s() len=%d != sz=%d\n", __func__, len, size);
> +	return (len == size) ? 0 : -1;

DTTO.

> +}
> +
> +static int mcs7830_read_phy(struct ueth_data *dev, uint8_t index)
> +{
> +	int rc;
> +	int retry;
> +	uint8_t cmd[2];
> +	uint16_t val;
> +
> +	/* send the PHY read request */
> +	cmd[0] = PHY_CMD1_READ | PHY_CMD1_PHYADDR;
> +	cmd[1] = PHY_CMD2_PEND_FLAG_BIT | (index & 0x1f);
> +	rc = mcs7830_write_reg(dev, REG_PHY_CMD1, sizeof(cmd), cmd);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* wait for the response to become available (usually < 1ms) */
> +	retry = 10;
> +	do {
> +		rc = mcs7830_read_reg(dev, REG_PHY_CMD1, sizeof(cmd), cmd);
> +		if (rc < 0)
> +			return rc;
> +		if (cmd[1] & PHY_CMD2_READY_FLAG_BIT)
> +			break;

Hm ... if retry==1 , then the following if (!retry) test will fail ...

> +		if (!retry)
> +			return -EIO;
> +		mdelay(1);
> +	} while (--retry);

... and the loop will exit here. Yet, (cmd[1] & PHY_CMD2_READY_FLAG_BIT) might 
still not be set . Right ?

> +	/* fetch the response data */
> +	rc = mcs7830_read_reg(dev, REG_PHY_DATA, sizeof(val), &val);
> +	if (rc < 0)
> +		return rc;
> +	rc = le16_to_cpu(val);
> +	debug("%s(%s, %d) => 0x%04X\n", __func__, dev->eth_dev.name, index, rc);
> +	return rc;
> +}

[...]

> +static int mcs7830_init(struct eth_device *eth, bd_t *bd)
> +{
> +	struct ueth_data *dev;
> +	int timeout;
> +	int have_link;
> +
> +	debug("%s()\n", __func__);
> +	dev = eth->priv;
> +
> +	timeout = 0;
> +	do {
> +		have_link = mcs7830_read_phy(dev, MII_BMSR) & BMSR_LSTATUS;
> +		if (!have_link) {
> +			if (!timeout)
> +				puts("Waiting for ethernet connection ... ");
> +			udelay(PHY_CONNECT_TIMEOUT_RES * 1000);
> +			timeout += PHY_CONNECT_TIMEOUT_RES;
> +		}
> +	} while (!have_link && timeout < PHY_CONNECT_TIMEOUT);
> +	if (have_link) {
> +		if (timeout)
> +			puts("done.\n");
> +		return 0;
> +	} else {
> +		puts("unable to connect.\n");
> +		return -1;
> +	}

Uh, this code is not quite clear to me ... can you not simplify this weird loop?
[...]

[U-Boot] [PATCH v1 1/5] usb: net: introduce support for Moschip USB ethernet

[Gerhard Sittig]

On Mon, Feb 17, 2014 at 02:40 +0100, Marek Vasut wrote:
> 
> On Monday, February 17, 2014 at 12:01:07 AM, Gerhard Sittig wrote:
> > introduce an 'mcs7830' driver for Moschip based USB ethernet adapters,
> > which was implemented based on the U-Boot Asix driver with additional
> > information gathered from the Moschip Linux driver
> 
> [...]
> 
> > +#define USB_CTRL_SET_TIMEOUT	5000
> > +#define USB_CTRL_GET_TIMEOUT	5000
> > +#define USB_BULK_SEND_TIMEOUT	5000
> > +#define USB_BULK_RECV_TIMEOUT	5000
> > +#define PHY_CONNECT_TIMEOUT	5000	/* link status, connect timeout */
> 
> Why don't we use just one TIMEOUT for all ?

copied from asix, might as well unify them, or at least reduce
them to USB ctrl and USB bulk and PHY connect (which translates
to setup/control, ethernet frames, and ethernet link status)

> > +#define PHY_CONNECT_TIMEOUT_RES	50	/* link status, resolution in 
> msec */
> > +
> > +#define MCS7830_RX_URB_SIZE	2048
> > +
> > +#ifndef BIT
> > +#define BIT(n)	(1 << (n))
> > +#endif
> 
> This should probably be in include/common.h or so.

when searching, I noticed that there were several local copies
and no common decl; the ifdef makes it work in the presence or
absence of a common decl

will look into introducing the common.h decl for BIT()

> > +/* command opcodes */
> > +enum {
> > +	MCS7830_WR_BREQ			= 0x0d,
> > +	MCS7830_RD_BREQ			= 0x0e,
> > +};
> > +
> > +/* register offsets, field bitmasks and default values */
> > +enum {
> > +	REG_MULTICAST_HASH	= 0x00,
> > +	REG_PHY_DATA		= 0x0a,
> > +	REG_PHY_CMD1		= 0x0c,
> > +		PHY_CMD1_READ		= BIT(6),
> > +		PHY_CMD1_WRITE		= BIT(5),
> > +		PHY_CMD1_PHYADDR	= BIT(0),
> > +	REG_PHY_CMD2		= 0x0d,
> > +		PHY_CMD2_PEND_FLAG_BIT	= BIT(7),
> > +		PHY_CMD2_READY_FLAG_BIT	= BIT(6),
> > +	REG_CONFIG		= 0x0e,
> > +		CONFIG_CFG		= BIT(7),
> > +		CONFIG_SPEED100		= BIT(6),
> > +		CONFIG_FDX_ENABLE	= BIT(5),
> > +		CONFIG_RXENABLE		= BIT(4),
> > +		CONFIG_TXENABLE		= BIT(3),
> > +		CONFIG_SLEEPMODE	= BIT(2),
> > +		CONFIG_ALLMULTICAST	= BIT(1),
> > +		CONFIG_PROMISCUOUS	= BIT(0),
> > +	REG_ETHER_ADDR		= 0x0f,
> > +	REG_FRAME_DROP_COUNTER	= 0x15,
> > +	REG_PAUSE_THRESHOLD	= 0x16,
> > +		PAUSE_THRESHOLD_DEFAULT	= 0,
> > +};
> 
> Why don't you just use structure with padding as the rest of the U-Boot does ? 
> Like so:
> 
> struct regs {
>  u8 reg1;
>  u8 pad1[n];
>  u8 reg2;
> ...
> };

does not apply here -- these are "adapter registers behind USB",
very much like "PHY registers behind MII communication"; the
driver won't pass addresses to I/O accessors, but will pass
register numbers as parameters to USB API calls

off list I learned that using CONFIG_* names here is a
BadIdea(TM), will adjust this, and may think about not using
BIT() depending on other feedback

> > +
> > +/* trailing status byte after RX frame */
> > +enum {
> > +	STAT_RX_FRAME_CORRECT	= BIT(5),
> > +	STAT_RX_LARGE_FRAME	= BIT(4),
> > +	STAT_RX_CRC_ERROR	= BIT(3),
> > +	STAT_RX_ALIGNMENT_ERROR	= BIT(2),
> > +	STAT_RX_LENGTH_ERROR	= BIT(1),
> > +	STAT_RX_SHORT_FRAME	= BIT(0),
> > +};
> 
> I am not quite fond of the enum for bits, but I don't care enough either .

OK, I will leave the call sites as they are (testing "val & MASK"
when checking individual bits), and prepare the decls without
BIT() but with 0x20 et al or their "1 << x" equivalent, and see
whether this looks better or@least more in line with existing
code

> > +
> > +static int mcs7830_read_reg(struct ueth_data *dev, uint8_t idx,
> > +			    uint16_t size, void *data)

BTW this is where "register index" numerical values are passed,
and get fed to the usb_control_msg() call below, no addresses
involved here

> > +{
> > +	int len;
> > +	ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, size);
> > +
> > +	debug("%s() idx=0x%04X sz=%d\n", __func__, idx, size);
> > +
> > +	len = usb_control_msg(dev->pusb_dev,
> > +			      usb_rcvctrlpipe(dev->pusb_dev, 0),
> > +			      MCS7830_RD_BREQ,
> > +			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> > +			      0, idx, buf, size,
> > +			      USB_CTRL_GET_TIMEOUT);
> > +	if (len == size) {
> > +		memcpy(data, buf, size);
> > +		return 0;
> > +	}
> > +	debug("%s() len=%d != sz=%d\n", __func__, len, size);
> > +	return -1;
> 
> Use errno.h defines for return codes please.

this was copied from asix, will see which errno code is most
appropriate (here and elsewhere)

> > +static int mcs7830_read_phy(struct ueth_data *dev, uint8_t index)
> > +{
> > +	int rc;
> > +	int retry;
> > +	uint8_t cmd[2];
> > +	uint16_t val;
> > +
> > +	/* send the PHY read request */
> > +	cmd[0] = PHY_CMD1_READ | PHY_CMD1_PHYADDR;
> > +	cmd[1] = PHY_CMD2_PEND_FLAG_BIT | (index & 0x1f);
> > +	rc = mcs7830_write_reg(dev, REG_PHY_CMD1, sizeof(cmd), cmd);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	/* wait for the response to become available (usually < 1ms) */
> > +	retry = 10;
> > +	do {
> > +		rc = mcs7830_read_reg(dev, REG_PHY_CMD1, sizeof(cmd), cmd);
> > +		if (rc < 0)
> > +			return rc;
> > +		if (cmd[1] & PHY_CMD2_READY_FLAG_BIT)
> > +			break;
> 
> Hm ... if retry==1 , then the following if (!retry) test will fail ...
> 
> > +		if (!retry)
> > +			return -EIO;
> > +		mdelay(1);
> > +	} while (--retry);
> 
> ... and the loop will exit here. Yet, (cmd[1] & PHY_CMD2_READY_FLAG_BIT) might 
> still not be set . Right ?

I knew I missed something :)  The intent was to iterate 11 times,
and thus to retry exactly 10 times after the initial attempt.
"retry--" should fix this, and should result in 11 attempts with 10
delays between them.  I.e. no early termination, and no useless
delay in the last iteration.

> > +static int mcs7830_init(struct eth_device *eth, bd_t *bd)
> > +{
> > +	struct ueth_data *dev;
> > +	int timeout;
> > +	int have_link;
> > +
> > +	debug("%s()\n", __func__);
> > +	dev = eth->priv;
> > +
> > +	timeout = 0;
> > +	do {
> > +		have_link = mcs7830_read_phy(dev, MII_BMSR) & BMSR_LSTATUS;
> > +		if (!have_link) {
> > +			if (!timeout)
> > +				puts("Waiting for ethernet connection ... ");
> > +			udelay(PHY_CONNECT_TIMEOUT_RES * 1000);
> > +			timeout += PHY_CONNECT_TIMEOUT_RES;
> > +		}
> > +	} while (!have_link && timeout < PHY_CONNECT_TIMEOUT);
> > +	if (have_link) {
> > +		if (timeout)
> > +			puts("done.\n");
> > +		return 0;
> > +	} else {
> > +		puts("unable to connect.\n");
> > +		return -1;
> > +	}
> 
> Uh, this code is not quite clear to me ... can you not simplify this weird loop?
> [...]

again, this was copied from asix; its core is checking for the
ethernet link status, what makes it look so weird is the "nice"
user presentation of whether the link already was established or
needed to get established first -- will see how I can rephrase it

thank you for reviewing!


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH v1 0/5] usb: eth: introduce Moschip MCS7830 driver

[Gerhard Sittig]

On Mon, Feb 17, 2014 at 02:25 +0100, Marek Vasut wrote:
> 
> On Monday, February 17, 2014 at 12:01:06 AM, Gerhard Sittig wrote:
> > [ ... ]
> > 
> > hopefully I got the Cc: list right, Marek as the USB custodian, Simon
> > for introducing the Asix driver in the past -- unfortunately I could not
> > determine a "network" custodian nor one outstanding usb-eth committer
> 
> CCing Joe, see [1] ;-)
> 
> [1] http://www.denx.de/wiki/U-Boot/Custodians

Thank you!  I hope Joe took no offense here. :)  And I will grab
board maintainership info from boards.cfg in v2 for the config
changes.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH v1 1/5] usb: net: introduce support for Moschip USB ethernet

[Marek Vasut]

On Monday, February 17, 2014 at 10:40:41 AM, Gerhard Sittig wrote:
[...]

> > Why don't you just use structure with padding as the rest of the U-Boot
> > does ? Like so:
> > 
> > struct regs {
> > 
> >  u8 reg1;
> >  u8 pad1[n];
> >  u8 reg2;
> > 
> > ...
> > };
> 
> does not apply here -- these are "adapter registers behind USB",
> very much like "PHY registers behind MII communication"; the
> driver won't pass addresses to I/O accessors, but will pass
> register numbers as parameters to USB API calls

You'd be passing offset of the register in the structure, right ?

[...]

> > Uh, this code is not quite clear to me ... can you not simplify this
> > weird loop? [...]
> 
> again, this was copied from asix; its core is checking for the
> ethernet link status, what makes it look so weird is the "nice"
> user presentation of whether the link already was established or
> needed to get established first -- will see how I can rephrase it
> 
> thank you for reviewing!

Thanks

Best regards,
Marek Vasut

[U-Boot] [PATCH v1 2/5] arm: config: alpha-sort USB ethernet items for Asix and SMSC

[Simon Glass]

On 16 February 2014 16:01, Gerhard Sittig <gsi@denx.de> wrote:
> adjust the harmony and omap3_beagle board configs to make
> their CONFIG_USB_ETHER_* items appear in alphabetical order
>
> Signed-off-by: Gerhard Sittig <gsi@denx.de>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v1 5/5] usb: doc: update README.doc to list all USB ethernet options

[Simon Glass]

On 16 February 2014 16:01, Gerhard Sittig <gsi@denx.de> wrote:
> - extend the discussion of USB network related config options such that
>   all available adapter drivers are listed, and that the 'usb' command
>   for the interactive prompt and scripting becomes available
> - suggest to *not* put individual IP configuration parameters into the
>   exectuable, but instead to put them into external environment or fetch
>   them from network
>
> Signed-off-by: Gerhard Sittig <gsi@denx.de>

Acked-by: Simon Glass <sjg@chromium.org>