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 <g...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
