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

Reply via email to