[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 at 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?
[...]