Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver
From: Andrew Lunn <andrew@lunn.ch>
Date: 2017-09-08 13:50:43
Also in:
linux-arm-kernel, lkml, netdev
quoted hunk ↗ jump to hunk
+static int ave_mdio_busywait(struct net_device *ndev) +{ + int ret = 1, loop = 100; + u32 mdiosr; + + /* wait until completion */ + while (1) { + mdiosr = ave_r32(ndev, AVE_MDIOSR); + if (!(mdiosr & AVE_MDIOSR_STS)) + break; + + usleep_range(10, 20); + if (!loop--) { + netdev_err(ndev, + "failed to read from MDIO (status:0x%08x)\n", + mdiosr); + ret = 0;
ETIMEDOUT would be better.
quoted hunk ↗ jump to hunk
+ break; + } + } + + return ret;
and then return 0 on success. That is the normal convention for return values. An error code, and 0.
quoted hunk ↗ jump to hunk
+static int ave_mdiobus_write(struct mii_bus *bus, + int phyid, int regnum, u16 val) +{ + struct net_device *ndev = bus->priv; + u32 mdioctl; + + /* write address */ + ave_w32(ndev, AVE_MDIOAR, (phyid << 8) | regnum); + + /* write data */ + ave_w32(ndev, AVE_MDIOWDR, val); + + /* write request */ + mdioctl = ave_r32(ndev, AVE_MDIOCTR); + ave_w32(ndev, AVE_MDIOCTR, mdioctl | AVE_MDIOCTR_WREQ); + + if (!ave_mdio_busywait(ndev)) { + netdev_err(ndev, "phy-%d reg-%x write failed\n", + phyid, regnum); + return -1;
If ave_mdio_busywait() returns ETIMEDOUT, you can just return it. Returning -1 is not good.
quoted hunk ↗ jump to hunk
+ } + + return 0; +} + +static irqreturn_t ave_interrupt(int irq, void *netdev) +{ + struct net_device *ndev = (struct net_device *)netdev; + struct ave_private *priv = netdev_priv(ndev); + u32 gimr_val, gisr_val; + + gimr_val = ave_irq_disable_all(ndev); + + /* get interrupt status */ + gisr_val = ave_r32(ndev, AVE_GISR); + + /* PHY */ + if (gisr_val & AVE_GI_PHY) { + ave_w32(ndev, AVE_GISR, AVE_GI_PHY); + if (priv->internal_phy_interrupt) + phy_mac_interrupt(ndev->phydev, ndev->phydev->link);
Humm. I don't think this is correct. You are supposed to give it the new link state, not the old. What does a PHY interrupt mean here?
quoted hunk ↗ jump to hunk
+static void ave_adjust_link(struct net_device *ndev) +{ + struct ave_private *priv = netdev_priv(ndev); + struct phy_device *phydev = ndev->phydev; + u32 val, txcr, rxcr, rxcr_org; + + /* set RGMII speed */ + val = ave_r32(ndev, AVE_TXCR); + val &= ~(AVE_TXCR_TXSPD_100 | AVE_TXCR_TXSPD_1G); + + if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII && + phydev->speed == SPEED_1000)
phy_interface_mode_is_rgmii(), so that you handle all the RGMII modes.
quoted hunk ↗ jump to hunk
+ val |= AVE_TXCR_TXSPD_1G; + else if (phydev->speed == SPEED_100) + val |= AVE_TXCR_TXSPD_100; + + ave_w32(ndev, AVE_TXCR, val); + + /* set RMII speed (100M/10M only) */ + if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {
Not so safe. It would be better to check for the modes you actually support.
quoted hunk ↗ jump to hunk
+ if (phydev->link) + netif_carrier_on(ndev); + else + netif_carrier_off(ndev);
I don't think you need this. The phylib should do it for you.
quoted hunk ↗ jump to hunk
+ + phy_print_status(phydev); +} + +static int ave_init(struct net_device *ndev) +{ + struct ave_private *priv = netdev_priv(ndev); + struct device *dev = ndev->dev.parent; + struct device_node *phy_node, *np = dev->of_node; + struct phy_device *phydev; + const void *mac_addr; + u32 supported; + + /* get mac address */ + mac_addr = of_get_mac_address(np); + if (mac_addr) + ether_addr_copy(ndev->dev_addr, mac_addr); + + /* if the mac address is invalid, use random mac address */ + if (!is_valid_ether_addr(ndev->dev_addr)) { + eth_hw_addr_random(ndev); + dev_warn(dev, "Using random MAC address: %pM\n", + ndev->dev_addr); + } + + /* attach PHY with MAC */ + phy_node = of_get_next_available_child(np, NULL);
??? Should this not be looking for a phy-handle property? Documentation/devicetree/binds/net/ethernet.txt: - phy-handle: phandle, specifies a reference to a node representing a PHY device; this property is described in the Devicetree Specification and so preferred;
quoted hunk ↗ jump to hunk
+ phydev = of_phy_connect(ndev, phy_node, + ave_adjust_link, 0, priv->phy_mode); + if (!phydev) { + dev_err(dev, "could not attach to PHY\n"); + return -ENODEV; + } + of_node_put(phy_node); + + priv->phydev = phydev; + phydev->autoneg = AUTONEG_ENABLE; + phydev->speed = 0; + phydev->duplex = 0;
And this should not be needed.
quoted hunk ↗ jump to hunk
+ + dev_info(dev, "connected to %s phy with id 0x%x\n", + phydev->drv->name, phydev->phy_id);
phy_attached_info()
+
+ if (priv->phy_mode != PHY_INTERFACE_MODE_RGMII) {Same comment as above.
quoted hunk ↗ jump to hunk
+ supported = phydev->supported; + phydev->supported &= ~PHY_GBIT_FEATURES; + phydev->supported |= supported & PHY_BASIC_FEATURES; + } + + /* PHY interrupt stop instruction is needed because the interrupt + * continues to assert. + */ + phy_stop_interrupts(phydev);
Could you explain this some more? It sounds like your interrupt controller is broken.
quoted hunk ↗ jump to hunk
+ + /* When PHY driver can't handle its interrupt directly, + * interrupt request always fails and polling method is used + * alternatively. In this case, the libphy says + * "libphy: uniphier-mdio: Can't get IRQ -1 (PHY)". + */ + phy_start_interrupts(phydev);
-1 is PHY_POLL. So calling phy_start_interrupts() is wrong. In fact, you should not be calling phy_start_interrupts() at all. No other Ethernet driver does.
quoted hunk ↗ jump to hunk
+ + phy_start_aneg(phydev); + + return 0; +} + +static void ave_uninit(struct net_device *ndev) +{ + struct ave_private *priv = netdev_priv(ndev); + + phy_stop_interrupts(priv->phydev);
And no other Ethernet driver calls phy_stop_interrupts either. Please take a look at this.
quoted hunk ↗ jump to hunk
+ phy_disconnect(priv->phydev); +} +
Andrew