Re: [PATCH] phy: aquantia: Configure SERDES mode by default

[Sean Anderson]

On 11/18/22 12:30, Vladimir Oltean wrote:
> On Fri, Nov 18, 2022 at 12:11:30PM -0500, Sean Anderson wrote:
>> >> - We can check all the registers to ensure we are actually going to rate
>> >>   adapt. If we aren't, we tell phylink we don't support it. This is the
>> >>   least risky, but we can end up not bringing up the link even in
>> >>   circumstances where we could if we configured things properly. And we
>> >>   generally know the right way to configure things.
>> > 
>> > Like when?
>> 
>> Well, like whenever the phy says "Please do XFI/2" or some other mode we
>> don't have a phy interface mode for. We will never be able to tell the MAC
>> "Please do XFI/2" (until we add an interface mode for it), so that's
>> obviously wrong.
> 
> Add an interface mode for it then...

> But note that I have absolutely no clue what XFI/2 is. Apparently
> Aquantia doesn't want NXP to know....

SERDES Mode [2:0]
  0 = XFI
  1 = Reserved
  2 = Reserved
  3 = SGMII
  4 = OCSGMII
  5 = Low Power
  6 = XFI/2 (i.e. XFI 5G)
  7 = XFI*2 (i.e. XFI 20G)

This is about it (aside from a mention in the PHY XS System Interface
Connection Status register). I assume it's over/underclocked XFI, much
like how 2500BASE-X is over/underclocked "SGMII."

I got my manual from Marvell's customer portal (okta). My document is
dated March 10, 2022.

>> >> - Add a configuration option (devicetree? ethtool?) on which option
>> >>   above to pick. This is probably what we will want to do in the long
>> >>   term, but I feel like we have enough information to determine the
>> >>   right thing to do most of the time (without needing manual
>> >>   intervention).
>> > 
>> > Not sure I see the need, when long-term there is no volunteer to make
>> > the Linux driver bring Aquantia PHYs to a known state regardless of
>> > vendor provisioning. Until then, there is just no reason to even attempt
>> > this.
>> 
>> I mean a config for option 1 vs 2 above.
> 
> How would this interact with Marek's proposal for phy-mode to be an
> array, and some middle entity (phylink?) selects the SERDES protocol and
> rate matching algorithm to use for each medium side link speed?
> https://patchwork.kernel.org/project/netdevbpf/cover/20211123164027.15618-1-kabel@xxxxxxxxxx/

Yeah, this is what I was referring to in the other thread [1]. In order to
implement this properly, we'd need to know what interfaces are supported,
electrically, by the board.

[1] https://lore.kernel.org/netdev/ea320070-a949-c737-22c4-14fd199fdc23@xxxxxxxx/

>> > Until you look at the procedure in the NXP SDK and see that things are a
>> > bit more complicated to get right, like put the PHY in low power mode,
>> > sleep for a while. I think a large part of that was determined experimentally,
>> > out of laziness to change PHY firmware on some riser cards more than anything.
>> > We still expect the production boards to have a good firmware, and Linux
>> > to read what that does and adapt accordingly.
>> 
>> Alas, if only Marvell put stuff like this in a manual... All I have is a spec
>> sheet and the register reference, and my company has an NDA...
> 
> Can't help with much more than providing this hint, sorry. All I can say
> is that SERDES protocol override from Linux is possible with care, at
> least on some systems. But it may be riddled with landmines.
> 
>> We aren't even using this phy on our board, so I am fine disabling rate adaptation
>> for funky firmwares.
> 
> Disabling rate adaptation is one thing. But there's also the unresolved
> XFI/2 issue?

I had in mind something like

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 47a76df36b74..18dfc09e80ef 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -109,6 +109,12 @@
  #define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE	0
  #define VEND1_GLOBAL_CFG_RATE_ADAPT_USX		1
  #define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE	2
+#define VEND1_GLOBAL_CFG_SERDES_MODE		GENMASK(2, 0)
+#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI	0
+#define VEND1_GLOBAL_CFG_SERDES_MODE_SGMII	3
+#define VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII	4
+#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G	6
+#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G	7

  #define VEND1_GLOBAL_RSVD_STAT1			0xc885
  #define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID	GENMASK(7, 4)
@@ -675,14 +681,69 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
  	return 0;
  }

+static int aqr107_global_config_serdes_speed(int global_config)
+{
+	switch (FIELD_GET(VEND1_GLOBAL_CFG_SERDES_MODE, global_config)) {
+	case VEND1_GLOBAL_CFG_SERDES_MODE_XFI:
+		return SPEED_10000;
+	case VEND1_GLOBAL_CFG_SERDES_MODE_SGMII:
+		return SPEED_1000;
+	case VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII:
+		return SPEED_2500;
+	case VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G:
+		return SPEED_5000;
+	case VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G:
+		return SPEED_20000;
+	default:
+		return SPEED_UNKNOWN;
+	}
+}
+
+static bool aqr107_rate_adapt_ok(struct phy_device *phydev, u16 reg, int speed)
+{
+	int val;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg);
+	if (val < 0) {
+		phydev_warn(phydev, "could not read register %x:%x (err = %d)\n",
+			    MDIO_MMD_VEND1, reg, val);
+		return false;
+	}
+
+	if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) !=
+		VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
+		return false;
+
+	if (aqr107_global_config_serdes_speed(val) != speed)
+		return false;
+
+	return true;
+}
+
  static int aqr107_get_rate_matching(struct phy_device *phydev,
  				    phy_interface_t iface)
  {
-	if (iface == PHY_INTERFACE_MODE_10GBASER ||
-	    iface == PHY_INTERFACE_MODE_2500BASEX ||
-	    iface == PHY_INTERFACE_MODE_NA)
+	int speed = phy_interface_max_speed(iface);
+
+	switch (speed) {
+	case SPEED_10000:
+		if (!aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_10G, speed) ||
+		    !aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_5G, speed))
+			return RATE_MATCH_NONE;
+		fallthrough;
+	case SPEED_2500:
+		if (!aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_2_5G, speed))
+			return RATE_MATCH_NONE;
+		fallthrough;
+	case SPEED_1000:
+		if (!aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_1G, speed) ||
+		    !aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_100M, speed) ||
+		    !aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_10M, speed))
+			return RATE_MATCH_NONE;
  		return RATE_MATCH_PAUSE;
-	return RATE_MATCH_NONE;
+	default:
+		return RATE_MATCH_NONE;
+	};
  }

  static int aqr107_suspend(struct phy_device *phydev)
-- 
2.35.1.1320.gc452695387.dirty