From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuiko.Oshino at microchip.com Date: Wed, 10 May 2017 15:25:26 +0000 Subject: [U-Boot] [PATCH] Add support for Microchip LAN75xx and LAN78xx In-Reply-To: References: <6AF1B4F019F8804DB17BB6F30B56F026BD37CF@CHN-SV-EXMX02.mchp-main.com> Message-ID: <6AF1B4F019F8804DB17BB6F30B56F026BF06EC@CHN-SV-EXMX02.mchp-main.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de From: Yuiko Oshino >-----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