On 04/10/2017 09:23 PM, yuiko.osh...@microchip.com wrote:
> From: Yuiko Oshino <yuiko.osh...@microchip.com>
>
> Add support for Microchip LAN7500, LAN7800 and 7850, USB to 10/100/1000
> Ethernet Controllers
>
> Signed-off-by: Yuiko Oshino <yuiko.osh...@microchip.com>
> Cc: Marek Vasut <ma...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
