From: Yuiko Oshino <yuiko.osh...@microchip.com> >-----Original Message----- >From: U-Boot [mailto:u-boot-boun...@lists.denx.de] On Behalf Of >yuiko.osh...@microchip.com >Sent: Wednesday, May 10, 2017 11:25 AM >To: joe.hershber...@gmail.com >Cc: ma...@denx.de; u-boot@lists.denx.de >Subject: Re: [U-Boot] [PATCH] Add support for Microchip LAN75xx and >LAN78xx > >From: Yuiko Oshino <yuiko.osh...@microchip.com> >>-----Original Message----- >>From: Joe Hershberger [mailto:joe.hershber...@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@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
