Re: [PATCH net-next v2 3/3] tsnep: Add TSN endpoint Ethernet MAC driver
flat view
From: Andrew Lunn <andrew@lunn.ch>
Date: 2021-08-31 21:55:23
Also in:
netdev
quoted hunk ↗ jump to hunk
+static int tsnep_ethtool_set_priv_flags(struct net_device *netdev, + u32 priv_flags) +{ + struct tsnep_adapter *adapter = netdev_priv(netdev); + int retval; + + if (priv_flags & ~TSNEP_PRIV_FLAGS) + return -EINVAL; + + if ((priv_flags & TSNEP_PRIV_FLAGS_LOOPBACK_100) && + (priv_flags & TSNEP_PRIV_FLAGS_LOOPBACK_1000)) + return -EINVAL; + + if ((priv_flags & TSNEP_PRIV_FLAGS_LOOPBACK_100) && + adapter->loopback != SPEED_100) { + if (adapter->loopback != SPEED_UNKNOWN) + retval = phy_loopback(adapter->phydev, false); + else + retval = 0; + + if (!retval) { + adapter->phydev->speed = SPEED_100; + adapter->phydev->duplex = DUPLEX_FULL; + retval = phy_loopback(adapter->phydev, true);
This is a pretty unusual use of private flags, changing loopback at runtime. ethtool --test generally does that. What is your use case which requires loopback in normal operation, not during testing?
quoted hunk ↗ jump to hunk
+static irqreturn_t tsnep_irq(int irq, void *arg) +{ + struct tsnep_adapter *adapter = arg; + u32 active = ioread32(adapter->addr + ECM_INT_ACTIVE); + + /* acknowledge interrupt */ + if (active != 0) + iowrite32(active, adapter->addr + ECM_INT_ACKNOWLEDGE); + + /* handle management data interrupt */ + if ((active & ECM_INT_MD) != 0) { + adapter->md_active = false; + wake_up_interruptible(&adapter->md_wait); + } + + /* handle link interrupt */ + if ((active & ECM_INT_LINK) != 0) { + if (adapter->netdev->phydev) { + struct phy_device *phydev = adapter->netdev->phydev; + u32 status = ioread32(adapter->addr + ECM_STATUS); + int link = (status & ECM_NO_LINK) ? 0 : 1; + u32 speed = status & ECM_SPEED_MASK;
How does PHY link and speed get into this MAC register? Is the MAC polling the PHY over the MDIO bus? Is the PHY internal to the MAC and it has backdoor access to the PHY status?
quoted hunk ↗ jump to hunk
+static int tsnep_mdiobus_read(struct mii_bus *bus, int addr, int regnum) +{ + struct tsnep_adapter *adapter = bus->priv; + u32 md; + int retval; + + if (regnum & MII_ADDR_C45) + return -EOPNOTSUPP; + + /* management data frame without preamble */ + md = ECM_MD_READ;
I know some PHYs are happy to work without a preamble. But as far as i know, 802.3 c22 does not say it is optional. So this needs to be an opt-in feature, for when you know all the devices on the bus support it. We have a standard DT property for this. See mdio.yaml, suppress-preamble. Please look for this in the DT blob, and only suppress the pre-amble if it is present.
quoted hunk ↗ jump to hunk
+ md |= (regnum << ECM_MD_ADDR_SHIFT) & ECM_MD_ADDR_MASK; + md |= ECM_MD_PHY_ADDR_FLAG; + md |= (addr << ECM_MD_PHY_ADDR_SHIFT) & ECM_MD_PHY_ADDR_MASK; + adapter->md_active = true; + iowrite32(md, adapter->addr + ECM_MD_CONTROL); + retval = wait_event_interruptible(adapter->md_wait, + !adapter->md_active);
It is pretty normal to have some sort of timeout here. So maybe use wait_event_interruptible_timeout()?
quoted hunk ↗ jump to hunk
+static void tsnep_phy_link_status_change(struct net_device *netdev) +{ + phy_print_status(netdev->phydev); +}
There is normally something here, like telling the MAC what speed it should run at.
quoted hunk ↗ jump to hunk
+static int tsnep_phy_open(struct tsnep_adapter *adapter) +{ + struct phy_device *phydev; + struct ethtool_eee ethtool_eee; + int retval; + + retval = phy_connect_direct(adapter->netdev, adapter->phydev, + tsnep_phy_link_status_change, + adapter->phy_mode); + if (retval) + return retval; + phydev = adapter->netdev->phydev; + + /* MAC supports only 100Mbps|1000Mbps full duplex + * SPE (Single Pair Ethernet) is also an option but not implemented yet + */ + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT); + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT); + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT); + phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT); + + /* disable EEE autoneg, EEE not supported by TSNEP */ + memset(ðtool_eee, 0, sizeof(ethtool_eee)); + phy_ethtool_set_eee(adapter->phydev, ðtool_eee); + + adapter->phydev->irq = PHY_MAC_INTERRUPT; + phy_start(adapter->phydev); + phy_start_aneg(adapter->phydev);
No need to call phy_start_aneg().
quoted hunk ↗ jump to hunk
+static int tsnep_phy_init(struct tsnep_adapter *adapter) +{ + struct device_node *dn; + int retval; + + retval = of_get_phy_mode(adapter->pdev->dev.of_node, + &adapter->phy_mode); + if (retval) + adapter->phy_mode = PHY_INTERFACE_MODE_GMII; + + dn = of_parse_phandle(adapter->pdev->dev.of_node, "phy-handle", 0); + adapter->phydev = of_phy_find_device(dn); + of_node_put(dn); + if (!adapter->phydev && adapter->mdiobus) + adapter->phydev = phy_find_first(adapter->mdiobus);
Do you actually need phy_find_first()? It is better to have it in DT.
quoted hunk ↗ jump to hunk
+ if (!adapter->phydev) + return -EIO; + + return 0; +} + +static int tsnep_probe(struct platform_device *pdev) +{
...
quoted hunk ↗ jump to hunk
+ dev_info(&adapter->pdev->dev, "device version %d.%02d\n", version, + revision); + if (adapter->gate_control) + dev_info(&adapter->pdev->dev, "gate control detected\n"); + + return 0; + + unregister_netdev(adapter->netdev);
How do you get here? Is gcc is warning about unreachable code?
quoted hunk ↗ jump to hunk
+register_failed: + tsnep_tc_cleanup(adapter); +tc_init_failed: + tsnep_ptp_cleanup(adapter); +ptp_init_failed: +phy_init_failed: + if (adapter->mdiobus) + mdiobus_unregister(adapter->mdiobus); +mdio_init_failed: +mac_init_failed: + return retval; +}
Andrew