[U-Boot] [PATCH] Add support for Microchip LAN75xx and LAN78xx
Yuiko.Oshino at microchip.com
Yuiko.Oshino at microchip.com
Wed May 10 15:25:26 UTC 2017
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
More information about the U-Boot
mailing list