Thread (36 messages) 36 messages, 5 authors, 2021-07-29

Re: [PATCH net-next 4/5] tsnep: Add TSN endpoint Ethernet MAC driver

From: Andrew Lunn <andrew@lunn.ch>
Date: 2021-07-26 21:15:28
Also in: netdev

+static int tsnep_mdiobus_read(struct mii_bus *bus, int addr, int regnum)
+{
+	struct tsnep_adapter *adapter = bus->priv;
+	u16 data;
+	int retval;
+
+	if (adapter->loopback)
+		return 0;
+
+	retval = tsnep_read_md(adapter, addr, regnum, &data);
+	if (retval != 0)
+		return retval;
It appears your MDIO bus can only do C22. Please add a test for C45 and return -EOPNOTSUPP.
+static void tsnep_phy_link_status_change(struct net_device *netdev)
+{
+	struct tsnep_adapter *adapter = netdev_priv(netdev);
+	struct phy_device *phydev = netdev->phydev;
+
+	if (adapter->loopback)
+		return;
+
+	if (adapter->gmii2rgmii) {
+		u16 val;
+
+		if (phydev->link && phydev->speed == 1000)
+			val = BMCR_SPEED1000;
+		else
+			val = BMCR_SPEED100;
+		tsnep_write_md(adapter, ECM_GMII2RGMII_ADDR,
+			       ECM_GMII2RGMII_BMCR, val);
+	}
I _think_ this is wrong. They way the PHYs are chained means you
should not need to do this, the xgmiitorgmii_read_status() does it.
Maybe you have the chaining setup wrong?
+static int tsnep_phy_open(struct tsnep_adapter *adapter)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask);
+	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 -EIO;
phy_connect_direct() returns an error code. Use it, rather than
changing it to something else. This applies everywhere. You must have
a good reason to change error codes, and then it is wise to put a
comment why you change it.
+
+	/* MAC supports only 100Mbps|1000Mbps full duplex
+	 * SPE (Single Pair Ethernet) is also an option but not implemented yet
+	 */
+	linkmode_zero(mask);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, mask);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, mask);
+	linkmode_and(mask, adapter->phydev->supported, mask);
+	linkmode_copy(adapter->phydev->supported, mask);
+	linkmode_copy(adapter->phydev->advertising, mask);
You should not be accessing the phydev directly. Use
phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT),
etc.
+static int tsnep_phy_init(struct tsnep_adapter *adapter)
+{
+	struct device_node *dn;
+	u16 val;
+	u32 id;
+	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);
+	if (!adapter->phydev)
+		adapter->phydev = phy_find_first(adapter->mdiobus);
+	if (!adapter->phydev)
+		return -EIO;
+
+	/* detect optional GMII2RGMII */
+	retval = tsnep_read_md(adapter, ECM_GMII2RGMII_ADDR, MII_PHYSID1, &val);
+	if (retval)
+		return retval;
+	id = val << 16;
+	retval = tsnep_read_md(adapter, ECM_GMII2RGMII_ADDR, MII_PHYSID2, &val);
+	if (retval)
+		return retval;
+	id |= val;
+	if (id == 0)
+		adapter->gmii2rgmii = true;
This is where i think GMII2RGMII goes wrong. MAC phy-handle should
point to the GMII2RGMII device in DT. The GMII2RGMII should have a
phy-handle which points to the PHY.
+	/* reset PHY */
+	retval = tsnep_write_md(adapter, adapter->phydev->mdio.addr, MII_BMCR,
+				BMCR_RESET);
+	if (retval)
+		return retval;
+
+	/* reset GMII2RGMII */
+	if (adapter->gmii2rgmii) {
+		retval = tsnep_write_md(adapter, ECM_GMII2RGMII_ADDR,
+					ECM_GMII2RGMII_BMCR, BMCR_RESET);
+		if (retval)
+			return retval;
+		retval = tsnep_write_md(adapter, ECM_GMII2RGMII_ADDR,
+					ECM_GMII2RGMII_BMCR, BMCR_SPEED100);
+		if (retval)
+			return retval;
+	}
The PHY driver is in control of the PHY, not the MAC. Please remove.

    Andrew
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help