[U-Boot] [PATCH] Add support for Microchip LAN75xx and LAN78xx

[Marek Vasut]

On 04/10/2017 09:23 PM, Yuiko.Oshino at microchip.com wrote:
> From: Yuiko Oshino <yuiko.oshino at microchip.com>
> 
> Add support for Microchip LAN7500, LAN7800 and 7850, USB to 10/100/1000 Ethernet Controllers
> 
> Signed-off-by: Yuiko Oshino <yuiko.oshino at microchip.com>
> Cc: Marek Vasut <marex at denx.de>

Hi!

mostly nit-picking below, thanks for the hard work!

> +/* MAC ADDRESS PERFECT FILTER For LAN75xx */
> +#define LAN75XX_ADDR_FILTX		0x300
> +#define LAN75XX_ADDR_FILTX_FB_VALID	BIT(31)
> +
> +#ifndef CONFIG_DM_ETH

I'd just make this depend on DM and scrap the non-DM part. It's not
worth the hassle .

> +/* local vars */
> +static int curr_eth_dev;	/* index for name of next device detected */
> +
> +/* local defines */
> +#define LAN75XX_BASE_NAME  "lan75xx"
> +#endif
> +
> +/*
> + * Lan75xx infrastructure commands
> + */


[...]

> diff --git a/drivers/usb/eth/lan78xx.c b/drivers/usb/eth/lan78xx.c
> new file mode 100644
> index 0000000..e450f2c
> --- /dev/null
> +++ b/drivers/usb/eth/lan78xx.c

[...]

> +#define LAN78XX_MAF_BASE		0x400
> +#define LAN78XX_MAF_HIX			0x00
> +#define LAN78XX_MAF_LOX			0x04
> +#define LAN78XX_MAF_HI_BEGIN		(LAN78XX_MAF_BASE + LAN78XX_MAF_HIX)
> +#define LAN78XX_MAF_LO_BEGIN		(LAN78XX_MAF_BASE + LAN78XX_MAF_LOX)
> +#define LAN78XX_MAF_HI(index)		(LAN78XX_MAF_BASE + (8 * (index)) + \
> +					(LAN78XX_MAF_HIX))
> +#define LAN78XX_MAF_LO(index)		(LAN78XX_MAF_BASE + (8 * (index)) + \
> +					(LAN78XX_MAF_LOX))

You don't need the extra parenthesis around LAN78XX_MAX_LOF (dtto for
MAF_HIX above).

> +#define LAN78XX_MAF_HI_VALID		BIT(31)


[...]

> +#ifndef CONFIG_DM_ETH

DTTO here, just dump the non-DM case :)

> +/* local vars */
> +static int curr_eth_dev;	/* index for name of next device detected */
> +
> +/* local defines */
> +#define LAN78XX_BASE_NAME "lan78xx"
> +#endif

[...]

> +static const struct lan7x_dongle lan78xx_dongles[] = {
> +	{0x0424, 0x7800},	/* LAN7800 USB Ethernet */
> +	{0x0424, 0x7850},	/* LAN7850 USB Ethernet */
> +	{0x0000, 0x0000}	/* END - Do not remove */
> +};

You can use ARRAY_SIZE() to save a few bytes maybe ?

> +/* Probe to see if a new device is actually an Microchip device */
> +int lan78xx_eth_probe(struct usb_device *dev, unsigned int ifnum,
> +		      struct ueth_data *ss)
> +{
> +	int i;
> +
> +	/* let's examine the device now */
> +
> +	for (i = 0; lan78xx_dongles[i].vendor != 0; i++) {
> +		if (dev->descriptor.idVendor == lan78xx_dongles[i].vendor &&
> +		    dev->descriptor.idProduct == lan78xx_dongles[i].product)
> +			/* Found a supported dongle */
> +			break;
> +	}
> +	if (lan78xx_dongles[i].vendor == 0)
> +		return 0;
> +
> +	/* At this point, we know we've got a live one */
> +	debug("\n\nUSB Ethernet device LAN78xx detected\n");
> +
> +	/*
> +	 * Note that this function needs to return 1
> +	 * for success
> +	 */
> +	return lan7x_eth_probe(dev, ifnum, ss);
> +}

[...]

> +int lan7x_update_flowcontrol(struct usb_device *udev,
> +			     struct ueth_data *dev,
> +			     uint32_t *flow, uint32_t *fct_flow)
> +{
> +	uint32_t lcladv, rmtadv, ctrl1000, stat1000;
> +	uint32_t advertising = 0, lp_advertising = 0, nego = 0;
> +	uint32_t duplex = 0;
> +	u8 cap = 0;

Shouldn't this be split into drivers/net/phy and be part of phylib ?
This code looks kinda familiar and similar to what I saw there ...

> +	lcladv = lan7x_mdio_read(udev, dev->phy_id, MII_ADVERTISE);
> +	advertising = lan7x_mii_get_an(lcladv);
> +
> +	rmtadv = lan7x_mdio_read(udev, dev->phy_id, MII_LPA);
> +	lp_advertising = lan7x_mii_get_an(rmtadv);
> +
> +	ctrl1000 = lan7x_mdio_read(udev, dev->phy_id, MII_CTRL1000);
> +	stat1000 = lan7x_mdio_read(udev, dev->phy_id, MII_STAT1000);
> +
> +	if (ctrl1000 & ADVERTISE_1000HALF)
> +		advertising |= ADVERTISED_1000baseT_Half;
> +
> +	if (ctrl1000 & ADVERTISE_1000FULL)
> +		advertising |= ADVERTISED_1000baseT_Full;
> +
> +	if (stat1000 & LPA_1000HALF)
> +		lp_advertising |= ADVERTISED_1000baseT_Half;
> +
> +	if (stat1000 & LPA_1000FULL)
> +		lp_advertising |= ADVERTISED_1000baseT_Full;
> +
> +	nego = advertising & lp_advertising;
> +
> +	debug("LAN7x linked at ");
> +
> +	if (nego & (ADVERTISED_1000baseT_Full | ADVERTISED_1000baseT_Half)) {
> +		debug("1000 ");
> +		duplex = !!(nego & ADVERTISED_1000baseT_Full);
> +
> +	} else if (nego & (ADVERTISED_100baseT_Full |
> +		   ADVERTISED_100baseT_Half)) {
> +		debug("100 ");
> +		duplex = !!(nego & ADVERTISED_100baseT_Full);
> +	} else {
> +		debug("10 ");
> +		duplex = !!(nego & ADVERTISED_10baseT_Full);
> +	}
> +
> +	if (duplex == DUPLEX_FULL)
> +		debug("full dup ");
> +	else
> +		debug("half dup ");
> +
> +	if (duplex == DUPLEX_FULL) {
> +		if (lcladv & rmtadv & ADVERTISE_PAUSE_CAP) {
> +			cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
> +		} else if (lcladv & rmtadv & ADVERTISE_PAUSE_ASYM) {
> +			if (lcladv & ADVERTISE_PAUSE_CAP)
> +				cap = FLOW_CTRL_RX;
> +			else if (rmtadv & LPA_PAUSE_CAP)
> +				cap = FLOW_CTRL_TX;
> +		}
> +		debug("TX Flow ");
> +		if (cap & FLOW_CTRL_TX) {
> +			*flow = (FLOW_CR_TX_FCEN | 0xFFFF);
> +			/* set fct_flow thresholds to 20% and 80% */
> +			*fct_flow = (((MAX_RX_FIFO_SIZE * 2) / (10 * 512))
> +					& 0x7FUL);

The outermost parenthesis are not needed :)

> +			*fct_flow <<= 8UL;
> +			*fct_flow |= (((MAX_RX_FIFO_SIZE * 8) / (10 * 512))
> +					& 0x7FUL);
> +			debug("EN ");
> +		} else {
> +			debug("DIS ");
> +		}
> +		debug("RX Flow ");
> +		if (cap & FLOW_CTRL_RX) {
> +			*flow |= FLOW_CR_RX_FCEN;
> +			debug("EN");
> +		} else {
> +			debug("DIS");
> +		}
> +	}
> +	debug("\n");
> +	return 0;
> +}

[...]

> +int lan7x_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;
> +
> +	iface = &dev->config.if_desc[ifnum];
> +	iface_desc = &dev->config.if_desc[ifnum].desc;
> +
> +	memset(ss, '\0', sizeof(struct ueth_data));
> +
> +	/* Initialize the ueth_data structure with some useful info */
> +	ss->ifnum = ifnum;
> +	ss->pusb_dev = dev;
> +	ss->subclass = iface_desc->bInterfaceSubClass;
> +	ss->protocol = iface_desc->bInterfaceProtocol;
> +
> +	/*
> +	 * We are expecting a minimum of 3 endpoints
> +	 * - in, out (bulk), and int.
> +	 * We will ignore any others.
> +	 */
> +	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
> +		/* is it an BULK endpoint? */
> +		if ((iface->ep_desc[i].bmAttributes &
> +			USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
> +			if (iface->ep_desc[i].bEndpointAddress & USB_DIR_IN)
> +				ss->ep_in =
> +					iface->ep_desc[i].bEndpointAddress &
> +					USB_ENDPOINT_NUMBER_MASK;
> +			else
> +				ss->ep_out =
> +					iface->ep_desc[i].bEndpointAddress &
> +					USB_ENDPOINT_NUMBER_MASK;
> +		}
> +
> +		/* is it an interrupt endpoint? */
> +		if ((iface->ep_desc[i].bmAttributes &
> +			USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) {
> +			ss->ep_int = iface->ep_desc[i].bEndpointAddress &
> +				USB_ENDPOINT_NUMBER_MASK;
> +			ss->irqinterval = iface->ep_desc[i].bInterval;
> +		}
> +	}
> +	debug("Endpoints In %d Out %d Int %d\n",
> +	      ss->ep_in, ss->ep_out, ss->ep_int);
> +
> +	/* Do some basic sanity checks, and bail if we find a problem */
> +	if (usb_set_interface(dev, iface_desc->bInterfaceNumber, 0) ||
> +	    !ss->ep_in || !ss->ep_out || !ss->ep_int) {
> +		debug("Problems with device\n");
> +		return 0;
> +	}
> +	dev->privptr = (void *)ss;
> +
> +#ifndef CONFIG_DM_ETH
> +	/* alloc driver private */
> +	ss->dev_priv = calloc(1, sizeof(struct lan7x_private));
> +	if (!ss->dev_priv)
> +		return 0;
> +#endif
> +
> +	return 1;

return 1 looks a bit weird, but maybe that's correct here .

> +}

[...]

> +/* Some extra defines */
> +#define LAN7X_INTERNAL_PHY_ID		1
> +
> +#define LAN7X_MAC_RX_MAX_SIZE(mtu) \
> +	((mtu) << 16)			/**< Max frame size */

Use just one asterisk in the comment please.

> +#define LAN7X_MAC_RX_MAX_SIZE_DEFAULT \
> +	LAN7X_MAC_RX_MAX_SIZE(ETH_FRAME_LEN + 4 /* VLAN */ + 4 /* CRC */)
> +
> +/* Timeouts */
> +#define USB_CTRL_SET_TIMEOUT_MS		5000
> +#define USB_CTRL_GET_TIMEOUT_MS		5000
> +#define USB_BULK_SEND_TIMEOUT_MS	5000
> +#define USB_BULK_RECV_TIMEOUT_MS	5000
> +#define TIMEOUT_RESOLUTION_MS		50
> +#define PHY_CONNECT_TIMEOUT_MS		5000
> +
> +#define RX_URB_SIZE	2048
> +
> +/* driver private */
> +struct lan7x_private {
> +#ifdef CONFIG_DM_ETH
> +	struct ueth_data ueth;
> +#endif
> +	int have_hwaddr;	/* 1 if we have a hardware MAC address */
> +	u32 chipid;		/* Chip or device ID */
> +};

[...]

-- 
Best regards,
Marek Vasut