On Wed, May 24, 2017 at 10:14 AM, Yuiko.Oshino@microchip.com wrote:
From: Yuiko Oshino yuiko.oshino@microchip.com
-----Original Message----- From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Yuiko.Oshino@microchip.com Sent: Wednesday, May 10, 2017 11:25 AM To: joe.hershberger@gmail.com Cc: marex@denx.de; u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH] Add support for Microchip LAN75xx and LAN78xx
From: Yuiko Oshino yuiko.oshino@microchip.com
-----Original Message----- From: Joe Hershberger [mailto:joe.hershberger@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 ");elsedebug("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;elsess->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@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