From: Yuiko Oshino <yuiko.osh...@microchip.com> > -----Original Message----- > From: Marek Vasut [mailto:ma...@denx.de] > Sent: Friday, April 14, 2017 8:35 AM > To: Yuiko Oshino - C18177; u-boot@lists.denx.de > Subject: Re: [PATCH] Add support for Microchip LAN75xx and LAN78xx > > 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!
Marek! Thank you for your hard work too! > > > +/* 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 . Okay, please let me confirm that I can delete all #ifndef CONFIG_DM_ETH stuff and remove non-DM code? Do I still need to keep #ifdef CONFIG_DM_ETH to avoid build errors? > > > +/* 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). Thank you. Will remove the extras. > > > +#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 ? Okay, I just followed what all other USB to Ether drivers do. But I can remove the "Do not remove" entry to save you a few and use the ARRAY_SIZE. > > > +/* 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 ... All I need is to get the auto negotiated duplex mode so that I can set the flow control. I am going to use our device specific status registers instead of the standard registers to get the duplex status to simplify the situation. > > > + 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 :) Thank you again! > > > + *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 . Yes, the caller needs to see 1 for a successful probe. > > > +} > > [...] > > > +/* 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. Thank you. > > > +#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 Best regards, Yuiko _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot