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
