[U-Boot] [PATCH] Add support for Microchip LAN75xx and LAN78xx
Joe Hershberger
joe.hershberger at gmail.com
Thu May 25 16:43:29 UTC 2017
On Wed, May 10, 2017 at 10:25 AM, <Yuiko.Oshino at microchip.com> wrote:
> 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.
At least copy the real one.
>>> +{
>>> + 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.
The thought here is to do a minor refactor of the phy.c code such that
all this parsing of the bits can be shared code, while access to the
MDIO interface is specialized for your driver.
>>
>>> +
>>> +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.
OK.
>>
>>> + }
>>> + 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