[U-Boot] [PATCH] Add support for Microchip LAN75xx and LAN78xx

Yuiko.Oshino at microchip.com Yuiko.Oshino at microchip.com
Wed May 24 15:14:50 UTC 2017


From: Yuiko Oshino <yuiko.oshino at microchip.com>

>-----Original Message-----
>From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of
>Yuiko.Oshino at microchip.com
>Sent: Wednesday, May 10, 2017 11:25 AM
>To: joe.hershberger at gmail.com
>Cc: marex at denx.de; u-boot at lists.denx.de
>Subject: Re: [U-Boot] [PATCH] Add support for Microchip LAN75xx and
>LAN78xx
>
>From: Yuiko Oshino <yuiko.oshino at microchip.com>
>>-----Original Message-----
>>From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
>>Sent: Friday, May 5, 2017 4:59 PM
>>To: Yuiko Oshino - C18177
>>Cc: u-boot; Marek Vasut
>>Subject: Re: [U-Boot] [PATCH] Add support for Microchip LAN75xx and
>>LAN78xx
>>
>>Hi Yuiko,
>
>Hi Joe!
>
>[...]
>
>>> +static int lan75xx_phy_gig_workaround(struct usb_device *udev,
>>> +                                     struct ueth_data *dev) {
>>> +       int ret = 0;
>>> +
>>> +       /* Only internal phy */
>>> +       /* Set the phy in Gig loopback */
>>> +       lan7x_mdio_write(udev, dev->phy_id, MII_BMCR,
>>> +                        (BMCR_LOOPBACK | BMCR_SPEED1000));
>>> +
>>> +       /* Wait for the link up */
>>> +       ret = lan7x_mdio_wait_for_bit(udev, "BMSR_LSTATUS",
>>> +                                     dev->phy_id, MII_BMSR, BMSR_LSTATUS,
>>> +                                     true, PHY_CONNECT_TIMEOUT_MS);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       /* phy reset */
>>> +       ret = lan7x_pmt_phy_reset(udev, dev);
>>> +       return ret;
>>
>>Just return lan7x_pmt_phy_reset(udev, dev);
>
>Sure thing.
>
>>
>>> +}
>>> +
>>> +static int lan75xx_update_flowcontrol(struct usb_device *udev,
>>> +                                     struct ueth_data *dev) {
>>> +       uint32_t flow = 0, fct_flow = 0;
>>> +       int ret;
>>> +
>>> +       ret = lan7x_update_flowcontrol(udev, dev, &flow, &fct_flow);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       ret = lan7x_write_reg(udev, FLOW, flow);
>>
>>       if (ret)
>>              return ret;
>>
>>> +       ret = lan7x_write_reg(udev, LAN75XX_FCT_FLOW, fct_flow);
>>> +       return ret;
>>
>>Return directly
>
>OK.
>
>[...]
>
>>> +
>>> +static int lan75xx_set_multicast(struct usb_device *udev) {
>>> +       int ret;
>>> +       u32 write_buf;
>>> +
>>> +       /* No multicast in u-boot */
>>
>>May want to... will enable IPv6 later.
>
>Yes, later.
>
>>
>>> +       write_buf = RFE_CTL_BCAST_EN | RFE_CTL_DA_PERFECT;
>>> +       ret = lan7x_write_reg(udev, LAN75XX_RFE_CTL, write_buf);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +/* starts the TX path */
>>> +static void lan75xx_start_tx_path(struct usb_device *udev) {
>>> +       u32 reg_val;
>>> +
>>> +       /* Enable Tx at MAC */
>>> +       reg_val = MAC_TX_TXEN;
>>
>>Why not just pass it into the function directly? Applies globally when
>>the assignment is a single mask.
>
>True. I will take care of them.
>
>>
>>> +       lan7x_write_reg(udev, MAC_TX, reg_val);
>>> +
>>> +       /* Enable Tx at SCSRs */
>>> +       reg_val = FCT_TX_CTL_EN;
>>> +       lan7x_write_reg(udev, LAN75XX_FCT_TX_CTL, reg_val); }
>>> +
>
>[...]
>
>>> +static int lan75xx_eth_probe(struct udevice *dev) {
>>> +       struct usb_device *udev = dev_get_parent_priv(dev);
>>> +       struct lan7x_private *priv = dev_get_priv(dev);
>>> +       struct ueth_data *ueth = &priv->ueth;
>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>> +
>>> +       /* Do a reset in order to get the MAC address from HW */
>>> +       if (lan75xx_basic_reset(udev, ueth, priv))
>>> +               return 0;
>>> +
>>> +       /* Get the MAC address */
>>> +       /*
>>> +        * We must set the eth->enetaddr from HW because the upper layer
>>> +        * will force to use the environmental var (usbethaddr) or random if
>>> +        * there is no valid MAC address in eth->enetaddr.
>>> +        */
>>> +       lan75xx_read_mac(pdata->enetaddr, udev);
>>> +       /* Do not return 0 for not finding MAC addr in HW */
>>> +
>>> +       return usb_ether_register(dev, ueth, RX_URB_SIZE); }
>>
>>I agree that these can all be squashed to remove non-DM support and
>>move all of the common functions up into these DM functions.
>>
>I will try to clean them.
>
>[...]
>
>>> +/*
>>> + * Lan78xx infrastructure commands
>>> + */
>>> +static int lan78xx_read_raw_otp(struct usb_device *udev, u32 offset,
>>> +                               u32 length, u8 *data) {
>>> +       int i;
>>> +       int ret;
>>> +       u32 buf;
>>> +
>>> +       ret = lan7x_read_reg(udev, LAN78XX_OTP_PWR_DN, &buf);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       if (buf & LAN78XX_OTP_PWR_DN_PWRDN_N) {
>>> +               /* clear it and wait to be cleared */
>>> +               ret = lan7x_write_reg(udev, LAN78XX_OTP_PWR_DN, 0);
>>
>>Either you don't care about the ret value, in which case why is there
>>one, or you are losing it by overwriting it on the next call. You
>>should probably be checking it after every assignment. Applies
>>globally.
>>
>
>True also. I will take care of them.
>
>>> +               ret = lan7x_wait_for_bit(udev,
>>"LAN78XX_OTP_PWR_DN_PWRDN_N",
>>> +                                        LAN78XX_OTP_PWR_DN,
>>> +                                        LAN78XX_OTP_PWR_DN_PWRDN_N,
>>> +                                        false, 1000);
>>> +               if (ret)
>>> +                       return ret;
>>> +       }
>>> +
>>> +       for (i = 0; i < length; i++) {
>>> +               ret = lan7x_write_reg(udev, LAN78XX_OTP_ADDR1,
>>> +                                     ((offset + i) >> 8) &
>>> +                                     LAN78XX_OTP_ADDR1_15_11);
>>> +               ret = lan7x_write_reg(udev, LAN78XX_OTP_ADDR2,
>>> +                                     ((offset + i) &
>>> + LAN78XX_OTP_ADDR2_10_3));
>>> +
>>> +               ret = lan7x_write_reg(udev, LAN78XX_OTP_FUNC_CMD,
>>> +                                     LAN78XX_OTP_FUNC_CMD_READ);
>>> +               ret = lan7x_write_reg(udev, LAN78XX_OTP_CMD_GO,
>>> +                                     LAN78XX_OTP_CMD_GO_GO);
>>> +
>>> +               if (ret)
>>> +                       return ret;
>>> +
>>> +               ret = lan7x_wait_for_bit(udev, "LAN78XX_OTP_STATUS_BUSY",
>>> +                                        LAN78XX_OTP_STATUS,
>>> +                                        LAN78XX_OTP_STATUS_BUSY,
>>> +                                        false, 1000);
>>> +               if (ret)
>>> +                       return ret;
>>> +
>>> +               ret = lan7x_read_reg(udev, LAN78XX_OTP_RD_DATA,
>>> + &buf);
>>> +
>>> +               data[i] = (u8)(buf & 0xFF);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>
>[...]
>
>>> +
>>> +/* Loop until the read is completed with timeout */ int
>>> +lan7x_wait_for_bit(struct usb_device *udev,
>>> +                      const char *prefix, const u32 index,
>>> +                      const u32 mask, const bool set,
>>> +                      unsigned int timeout_ms)
>>
>>Can you not use the generic one? include/wait_bit.h
>
>We need to use our own register read function as our device is an USB device.
>It looks like wait_bit.h does not support function pointer to register read,
>therefore we need our own.
>
>>
>>> +{
>>> +       u32 val;
>>> +
>>> +       while (--timeout_ms) {
>>> +               lan7x_read_reg(udev, index, &val);
>>> +
>>> +               if (!set)
>>> +                       val = ~val;
>>> +
>>> +               if ((val & mask) == mask)
>>> +                       return 0;
>>> +
>>> +               mdelay(1);
>>> +       }
>>> +
>>> +       debug("%s: Timeout (reg=0x%x mask=%08x wait_set=%i)\n",
>>> +             prefix, index, mask, set);
>>> +
>>> +       return -ETIMEDOUT;
>>> +}
>>> +
>>> +static int lan7x_phy_wait_not_busy(struct usb_device *udev) {
>>> +       return lan7x_wait_for_bit(udev, __func__,
>>> +                                 MII_ACC, MII_ACC_MII_BUSY,
>>> +                                 false, 100); }
>>> +
>>> +int lan7x_mdio_read(struct usb_device *udev, int phy_id, int idx) {
>>> +       u32 val, addr;
>>> +
>>> +       /* confirm MII not busy */
>>> +       if (lan7x_phy_wait_not_busy(udev)) {
>>> +               debug("MII is busy in %s\n", __func__);
>>> +               return -ETIMEDOUT;
>>> +       }
>>> +
>>> +       /* set the address, index & direction (read from PHY) */
>>> +       addr = (phy_id << 11) | (idx << 6) |
>>> +               MII_ACC_MII_READ | MII_ACC_MII_BUSY;
>>> +       lan7x_write_reg(udev, MII_ACC, addr);
>>> +
>>> +       if (lan7x_phy_wait_not_busy(udev)) {
>>> +               debug("Timed out reading MII reg %02X\n", idx);
>>> +               return -ETIMEDOUT;
>>> +       }
>>> +
>>> +       lan7x_read_reg(udev, MII_DATA, &val);
>>> +
>>> +       return (u16) (val & 0xFFFF);
>>> +}
>>> +
>>> +int lan7x_mdio_wait_for_bit(struct usb_device *udev,
>>> +                           const char *prefix,
>>> +                           int phy_id, const u32 index,
>>> +                           const u32 mask, const bool set,
>>> +                           unsigned int timeout_ms) {
>>> +       u32 val;
>>> +
>>> +       while (--timeout_ms) {
>>> +               val = lan7x_mdio_read(udev, phy_id, index);
>>> +
>>> +               if (!set)
>>> +                       val = ~val;
>>> +
>>> +               if ((val & mask) == mask)
>>> +                       return 0;
>>> +
>>> +               mdelay(1);
>>> +       }
>>> +
>>> +       debug("%s: Timeout (reg=0x%x mask=%08x wait_set=%i)\n",
>>> +             prefix, index, mask, set);
>>> +
>>> +       return -ETIMEDOUT;
>>> +}
>>> +
>
>[...]
>
>>> +static int lan7x_mii_get_an(uint32_t advertising_reg) {
>>> +       int advertising = 0;
>>> +
>>> +       if (advertising_reg & LPA_LPACK)
>>> +               advertising |= ADVERTISED_Autoneg;
>>> +       if (advertising_reg & ADVERTISE_10HALF)
>>> +               advertising |= ADVERTISED_10baseT_Half;
>>> +       if (advertising_reg & ADVERTISE_10FULL)
>>> +               advertising |= ADVERTISED_10baseT_Full;
>>> +       if (advertising_reg & ADVERTISE_100HALF)
>>> +               advertising |= ADVERTISED_100baseT_Half;
>>> +       if (advertising_reg & ADVERTISE_100FULL)
>>> +               advertising |= ADVERTISED_100baseT_Full;
>>> +
>>> +       return advertising;
>>> +}
>>> +
>>> +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;
>>> +
>>> +       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);
>>> +                       *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;
>>> +}
>>
>>I see where Marek is coming from wrt thisall being in phylib already.
>>I guess you always have a fixed phy internal, so there's no need of the
>>flexibility of phylib. Maybe there's at least opportunity to
>>consolidate subroutines even if not using phylib the normal way.
>
>I am a bit confused what you say. Do you mean that I can keep the current
>code as is in this area?
>Please confirm?
>The access to PHY also needs our own register read/write through USB, so
>using the phylib is more complicated.
>
>>
>>> +
>>> +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;
>>
>>Seems this should return an error.
>
>The caller probe_valid_drivers() in usb_ether.c expects 0 for skipping a bad
>device.
>
>>
>>> +       }
>>> +       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;
>>> +}
>>> +
>
>Thank you.
>Yuiko
>_______________________________________________
>U-Boot mailing list
>U-Boot at lists.denx.de
>https://lists.denx.de/listinfo/u-boot

Joe,
Please reply so that I can re-submit the patch.
Thank you in advance.
Best regards,
Yuiko


More information about the U-Boot mailing list