Re: [PATCH net-next 8/8] net: phy: marvell10g: select host interface configuration

On Thu, 18 Nov 2021 14:03:34 +0200
Vladimir Oltean [off-list ref] wrote:

Hello,

quoted

+static int mv3310_select_mactype(unsigned long *interfaces)
+{
+	if (test_bit(PHY_INTERFACE_MODE_USXGMII, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_USXGMII;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_RXAUI, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_RXAUI;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
+		 test_bit(PHY_INTERFACE_MODE_XAUI, interfaces))
+		return MV_V2_3310_PORT_CTRL_MACTYPE_XAUI;
+	else if (test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH;
+	else if (test_bit(PHY_INTERFACE_MODE_RXAUI, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_RXAUI_RATE_MATCH;
+	else if (test_bit(PHY_INTERFACE_MODE_XAUI, interfaces))
+		return MV_V2_3310_PORT_CTRL_MACTYPE_XAUI_RATE_MATCH;
+	else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces))
+		return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER;
+	else
+		return -1;
+}
+  

I would like to understand this heuristic better. Both its purpose and
its implementation.

It says:
(a) If the intersection between interface modes supported by the MAC and
    the PHY contains USXGMII, then use USXGMII as a MACTYPE
(b) Otherwise, if the intersection contains both 10GBaseR and SGMII, then
    use 10GBaseR as MACTYPE
(...)
(c) Otherwise, if the intersection contains just 10GBaseR (no SGMII), then
    use 10GBaseR with rate matching as MACTYPE
(...)
(d) Otherwise, if the intersection contains just SGMII (no 10GBaseR), then
    use 10GBaseR as MACTYPE (no rate matching).

First of all, what is MACTYPE exactly? And what is the purpose of
changing it? What would happen if this configuration remained fixed, as
it were?

I see there is no MACTYPE definition for SGMII. Why is that? How does
the PHY choose to use SGMII?

Why is item (d) correct - use 10GBaseR as MACTYPE if the intersection
only supports SGMII?

A breakdown per link speed might be helpful in understanding the
configurations being performed here.

Russell already explained this. I will put a better explanation into
the commit message in v2.

quoted

 static int mv2110_init_interface(struct phy_device *phydev, int mactype)
 {
 	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);

@@ -674,10 +764,16 @@ static int mv3310_config_init(struct phy_device *phydev)
 {
 	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
 	const struct mv3310_chip *chip = to_mv3310_chip(phydev);
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
 	int err, mactype;
 
-	/* Check that the PHY interface type is compatible */
-	if (!test_bit(phydev->interface, priv->supported_interfaces))
+	/* In case host didn't provide supported interfaces */
+	__set_bit(phydev->interface, phydev->host_interfaces);  

Shouldn't phylib populate phydev->host_interfaces with
phydev->interface, rather than requiring PHY drivers to do it?

I will look into this.

Marek