Re: [RFC net-next PATCH 10/16] net: macb: Move PCS settings to PCS callbacks

Hi Russell,

On 10/5/21 6:06 AM, Russell King (Oracle) wrote:

On Mon, Oct 04, 2021 at 03:15:21PM -0400, Sean Anderson wrote:

quoted

+static void macb_pcs_get_state(struct phylink_pcs *pcs,
+			       struct phylink_link_state *state)
+{
+	struct macb *bp = pcs_to_macb(pcs);
+
+	if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
+		state->interface = PHY_INTERFACE_MODE_SGMII;
+	else
+		state->interface = PHY_INTERFACE_MODE_1000BASEX;

There is no requirement to set state->interface here. Phylink doesn't
cater for interface changes when reading the state. As documented,
phylink will set state->interface already before calling this function
to indicate what interface mode it is currently expecting from the
hardware.

Ok, so instead I should be doing something like

if (gem_readl(bp, NCFGR) & GEM_BIT(SGMIIEN))
	interface = PHY_INTERFACE_MODE_SGMII;
else
	interface = PHY_INTERFACE_MODE_1000BASEX;

if (interface != state->interface) {
	state->link = 0;
	return;
}

?

quoted

+static int macb_pcs_config_an(struct macb *bp, unsigned int mode,
+			      phy_interface_t interface,
+			      const unsigned long *advertising)
+{
+	bool changed = false;
+	u16 old, new;
+
+	old = gem_readl(bp, PCSANADV);
+	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising,
+						       old);
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, PCSANADV, new);
+	}
+
+	old = new = gem_readl(bp, PCSCNTRL);
+	if (mode == MLO_AN_INBAND)

Please use phylink_autoneg_inband(mode) here.

Ok.

quoted

+		new |= BMCR_ANENABLE;
+	else
+		new &= ~BMCR_ANENABLE;
+	if (old != new) {
+		changed = true;
+		gem_writel(bp, PCSCNTRL, new);
+	}

There has been the suggestion that we should allow in-band AN to be
disabled in 1000base-X if we're in in-band mode according to the
ethtool state.

This logic is taken from phylink_mii_c22_pcs_config. Maybe I should add
another _encode variant? I hadn't done this here because the logic was
only one if statement.

I have a patch that adds that.

Have you posted it?

quoted

+	return changed;
+}
+
+static int macb_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+			   phy_interface_t interface,
+			   const unsigned long *advertising,
+			   bool permit_pause_to_mac)
+{
+	bool changed = false;
+	struct macb *bp = pcs_to_macb(pcs);
+	u16 old, new;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bp->lock, flags);
+	old = new = gem_readl(bp, NCFGR);
+	if (interface == PHY_INTERFACE_MODE_SGMII) {
+		new |= GEM_BIT(SGMIIEN);
+	} else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
+		new &= ~GEM_BIT(SGMIIEN);
+	} else {
+		spin_lock_irqsave(&bp->lock, flags);
+		return -EOPNOTSUPP;

You can't actually abort at this point - phylink will print the error
and carry on regardless. The checking is all done via the validate()
callback and if that indicates the interface mode is acceptable, then
it should be accepted.

Ok, so where can the PCS NAK an interface? This is the only callback
which has a return code, so I assumed this was the correct place to say
"no, we don't support this." This is what lynx_pcs_config does as well.

quoted

 static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
 	.pcs_get_state = macb_usx_pcs_get_state,
 	.pcs_config = macb_usx_pcs_config,
-	.pcs_link_up = macb_usx_pcs_link_up,
 };

 static const struct phylink_pcs_ops macb_phylink_pcs_ops = {
 	.pcs_get_state = macb_pcs_get_state,
-	.pcs_an_restart = macb_pcs_an_restart,

You don't want to remove this.

Hm, I didn't realized I had removed this, so I add it back in the next
patch. I will merge that one into this one.

When operating in 1000BASE-X mode, it
will be called if a restart is required (e.g. macb_pcs_config()
returning positive, or an ethtool request.) You need to keep the empty
function.

Ok, perhaps I can add some sanity checks for this in pcs_register.

--Sean

That may also help the diff algorithm to produce a cleaner
patch too.