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@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 ");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