From: Yuiko Oshino yuiko.oshino@microchip.com
-----Original Message----- From: Marek Vasut [mailto:marex@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.Oshino@microchip.com wrote:
From: Yuiko Oshino yuiko.oshino@microchip.com
Add support for Microchip LAN7500, LAN7800 and 7850, USB to 10/100/1000 Ethernet Controllers
Signed-off-by: Yuiko Oshino yuiko.oshino@microchip.com Cc: Marek Vasut marex@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;elsess->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