[U-Boot] [PATCH v1 1/5] usb: net: introduce support for Moschip USB ethernet
Gerhard Sittig
gsi at denx.de
Mon Feb 17 10:40:41 CET 2014
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
--
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
More information about the U-Boot
mailing list