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