From: Joe Hershberger <joe.hershberger@gmail.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH] Add support for Microchip LAN75xx and LAN78xx
Date: Thu, 25 May 2017 11:43:29 -0500 [thread overview]
Message-ID: <CANr=Z=bdexw_=wjythRLN2pNLjbm=Q8YSCaaJFO51vztHXb+zA@mail.gmail.com> (raw)
In-Reply-To: <6AF1B4F019F8804DB17BB6F30B56F026BF06EC@CHN-SV-EXMX02.mchp-main.com>
On Wed, May 10, 2017 at 10:25 AM, <Yuiko.Oshino@microchip.com> wrote:
> From: Yuiko Oshino <yuiko.oshino@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
prev parent reply other threads:[~2017-05-25 16:43 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-10 19:23 [U-Boot] [PATCH] Add support for Microchip LAN75xx and LAN78xx Yuiko.Oshino at microchip.com
2017-04-14 12:35 ` Marek Vasut
2017-04-18 15:58 ` Yuiko.Oshino at microchip.com
2017-04-18 16:47 ` Marek Vasut
2017-05-03 14:20 ` Yuiko.Oshino at microchip.com
2017-04-18 18:18 ` Simon Glass
2017-05-05 20:58 ` Joe Hershberger
2017-05-10 15:25 ` Yuiko.Oshino at microchip.com
2017-05-24 15:14 ` Yuiko.Oshino at microchip.com
2017-05-30 16:54 ` Joe Hershberger
2017-05-25 16:43 ` Joe Hershberger [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANr=Z=bdexw_=wjythRLN2pNLjbm=Q8YSCaaJFO51vztHXb+zA@mail.gmail.com' \
--to=joe.hershberger@gmail.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.