[U-Boot] [PATCH] Add support for Microchip LAN75xx and LAN78xx
Joe Hershberger
joe.hershberger at gmail.com
Tue May 30 16:54:13 UTC 2017
On Wed, May 24, 2017 at 10:14 AM, <Yuiko.Oshino at microchip.com> wrote:
> From: Yuiko Oshino <yuiko.oshino at microchip.com>
>
>>-----Original Message-----
>>From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of
>>Yuiko.Oshino at microchip.com
>>Sent: Wednesday, May 10, 2017 11:25 AM
>>To: joe.hershberger at gmail.com
>>Cc: marex at denx.de; u-boot at lists.denx.de
>>Subject: Re: [U-Boot] [PATCH] Add support for Microchip LAN75xx and
>>LAN78xx
>>
>>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!
>>
>>[...]
[...]
>>>> +
>>>> +/* 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
>>_______________________________________________
>>U-Boot mailing list
>>U-Boot at 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
More information about the U-Boot
mailing list