Re: [net-next PATCH v9 3/4] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

["Russell King (Oracle)" <linux@armlinux.org.uk>]

hi Christian,

On Thu, Dec 05, 2024 at 03:51:33PM +0100, Christian Marangi wrote:
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 2d10b4d6cfbb..6b6d0b7bae72 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -24,6 +24,15 @@ config NET_DSA_LOOP
>  	  This enables support for a fake mock-up switch chip which
>  	  exercises the DSA APIs.
>  
> +

Niggle - no need for the extra blank line.

> +static int an8855_set_mac_eee(struct dsa_switch *ds, int port,
> +			      struct ethtool_keee *eee)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	u32 reg;
> +	int ret;
> +
> +	if (eee->eee_enabled) {
> +		ret = regmap_read(priv->regmap, AN8855_PMCR_P(port), &reg);
> +		if (ret)
> +			return ret;
> +		/* Force enable EEE if force mode and LINK */
> +		if (reg & AN8855_PMCR_FORCE_MODE &&
> +		    reg & AN8855_PMCR_FORCE_LNK) {
> +			switch (reg & AN8855_PMCR_FORCE_SPEED) {
> +			case AN8855_PMCR_FORCE_SPEED_1000:
> +				reg |= AN8855_PMCR_FORCE_EEE1G;
> +				break;
> +			case AN8855_PMCR_FORCE_SPEED_100:
> +				reg |= AN8855_PMCR_FORCE_EEE100;
> +				break;
> +			default:
> +				break;
> +			}

Does this forcing force EEE to be enabled for the port, irrespective
of the negotiation?

> +			ret = regmap_write(priv->regmap, AN8855_PMCR_P(port), reg);
> +			if (ret)
> +				return ret;
> +		}
> +		ret = regmap_update_bits(priv->regmap, AN8855_PMEEECR_P(port),
> +					 AN8855_LPI_MODE_EN,
> +					 eee->tx_lpi_enabled ? AN8855_LPI_MODE_EN : 0);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_clear_bits(priv->regmap, AN8855_PMCR_P(port),
> +					AN8855_PMCR_FORCE_EEE1G |
> +					AN8855_PMCR_FORCE_EEE100);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_clear_bits(priv->regmap, AN8855_PMEEECR_P(port),
> +					AN8855_LPI_MODE_EN);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int an8855_get_mac_eee(struct dsa_switch *ds, int port,
> +			      struct ethtool_keee *eee)
> +{
> +	struct an8855_priv *priv = ds->priv;
> +	u32 reg;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, AN8855_PMEEECR_P(port), &reg);
> +	if (ret)
> +		return ret;
> +	eee->tx_lpi_enabled = reg & AN8855_LPI_MODE_EN;
> +
> +	ret = regmap_read(priv->regmap, AN8855_CKGCR, &reg);
> +	if (ret)
> +		return ret;
> +	/* Global LPI TXIDLE Threshold, default 60ms (unit 2us) */
> +	eee->tx_lpi_timer = FIELD_GET(AN8855_LPI_TXIDLE_THD_MASK, reg) / 500;

There is no point setting tx_lpi_enabled and tx_lpi_timer, as phylib
will immediately overwrite then in its phy_ethtool_get_eee() method.
In fact, there's no point to the DSA get_mac_eee() method.

> +
> +	ret = regmap_read(priv->regmap, AN8855_PMSR_P(port), &reg);
> +	if (ret)
> +		return ret;

What's the purpose of this read?

> +
> +	return 0;
> +}

Overall, I would suggest not implementing EEE just yet - I sent out a
22 patch RFC patch set last week which implements EEE support in
phylink, and I have another bunch of patches that clean up DSA that
I didn't send, which includes getting rid of the DSA get_mac_eee()
method.

Now that the bulk of the phylink in-band changes have been merged, my
plan is to work through getting the RFC patch set merged - I've sent
out the first four patches today for final review and merging.

I stated some questions in the RFC cover message that everyone ignored,
maybe you could look at that and give your views on the points I raised
there?

> +static struct phylink_pcs *
> +an8855_phylink_mac_select_pcs(struct phylink_config *config,
> +			      phy_interface_t interface)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct an8855_priv *priv = dp->ds->priv;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		return &priv->pcs;
> +	default:
> +		return NULL;
> +	}

Great, exactly what I like to see in mac_select_pcs()!

> +}
> +
> +static void
> +an8855_phylink_mac_config(struct phylink_config *config, unsigned int mode,
> +			  const struct phylink_link_state *state)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct dsa_switch *ds = dp->ds;
> +	struct an8855_priv *priv;
> +	int port = dp->index;
> +
> +	priv = ds->priv;
> +
> +	/* Nothing to configure for internal ports */
> +	if (port != 5)
> +		return;
> +
> +	regmap_update_bits(priv->regmap, AN8855_PMCR_P(port),
> +			   AN8855_PMCR_IFG_XMIT | AN8855_PMCR_MAC_MODE |
> +			   AN8855_PMCR_BACKOFF_EN | AN8855_PMCR_BACKPR_EN,
> +			   FIELD_PREP(AN8855_PMCR_IFG_XMIT, 0x1) |
> +			   AN8855_PMCR_MAC_MODE | AN8855_PMCR_BACKOFF_EN |
> +			   AN8855_PMCR_BACKPR_EN);
> +}

This looks much better, thanks.

> +
> +static void an8855_phylink_get_caps(struct dsa_switch *ds, int port,
> +				    struct phylink_config *config)
> +{
> +	switch (port) {
> +	case 0:
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:
> +		__set_bit(PHY_INTERFACE_MODE_GMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +		break;
> +	case 5:
> +		phy_interface_set_rgmii(config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			  config->supported_interfaces);
> +		break;
> +	}
> +
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;
> +}
> +
> +static void
> +an8855_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
> +			     phy_interface_t interface)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct an8855_priv *priv = dp->ds->priv;
> +
> +	/* With autoneg just disable TX/RX else also force link down */
> +	if (phylink_autoneg_inband(mode)) {
> +		regmap_clear_bits(priv->regmap, AN8855_PMCR_P(dp->index),
> +				  AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN);
> +	} else {
> +		regmap_update_bits(priv->regmap, AN8855_PMCR_P(dp->index),
> +				   AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN |
> +				   AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK,
> +				   AN8855_PMCR_FORCE_MODE);
> +	}
> +}
> +
> +static void
> +an8855_phylink_mac_link_up(struct phylink_config *config,
> +			   struct phy_device *phydev, unsigned int mode,
> +			   phy_interface_t interface, int speed, int duplex,
> +			   bool tx_pause, bool rx_pause)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct an8855_priv *priv = dp->ds->priv;
> +	int port = dp->index;
> +	u32 reg;
> +
> +	reg = regmap_read(priv->regmap, AN8855_PMCR_P(port), &reg);
> +	if (phylink_autoneg_inband(mode)) {
> +		reg &= ~AN8855_PMCR_FORCE_MODE;
> +	} else {
> +		reg |= AN8855_PMCR_FORCE_MODE | AN8855_PMCR_FORCE_LNK;
> +
> +		reg &= ~AN8855_PMCR_FORCE_SPEED;
> +		switch (speed) {
> +		case SPEED_10:
> +			reg |= AN8855_PMCR_FORCE_SPEED_10;
> +			break;
> +		case SPEED_100:
> +			reg |= AN8855_PMCR_FORCE_SPEED_100;
> +			break;
> +		case SPEED_1000:
> +			reg |= AN8855_PMCR_FORCE_SPEED_1000;
> +			break;
> +		case SPEED_2500:
> +			reg |= AN8855_PMCR_FORCE_SPEED_2500;
> +			break;
> +		case SPEED_5000:
> +			dev_err(priv->dev, "Missing support for 5G speed. Aborting...\n");
> +			return;
> +		}
> +
> +		reg &= ~AN8855_PMCR_FORCE_FDX;
> +		if (duplex == DUPLEX_FULL)
> +			reg |= AN8855_PMCR_FORCE_FDX;
> +
> +		reg &= ~AN8855_PMCR_RX_FC_EN;
> +		if (rx_pause || dsa_port_is_cpu(dp))
> +			reg |= AN8855_PMCR_RX_FC_EN;
> +
> +		reg &= ~AN8855_PMCR_TX_FC_EN;
> +		if (rx_pause || dsa_port_is_cpu(dp))
> +			reg |= AN8855_PMCR_TX_FC_EN;
> +
> +		/* Disable any EEE options */
> +		reg &= ~(AN8855_PMCR_FORCE_EEE5G | AN8855_PMCR_FORCE_EEE2P5G |
> +			 AN8855_PMCR_FORCE_EEE1G | AN8855_PMCR_FORCE_EEE100);
> +	}
> +
> +	reg |= AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN;
> +
> +	regmap_write(priv->regmap, AN8855_PMCR_P(port), reg);
> +}

All the above looks fine to me.

> +
> +static void an8855_pcs_get_state(struct phylink_pcs *pcs,
> +				 struct phylink_link_state *state)
> +{
> +	struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, AN8855_PMSR_P(AN8855_CPU_PORT), &val);
> +	if (ret < 0) {
> +		state->link = false;
> +		return;
> +	}
> +
> +	state->link = !!(val & AN8855_PMSR_LNK);
> +	state->an_complete = state->link;
> +	state->duplex = (val & AN8855_PMSR_DPX) ? DUPLEX_FULL :
> +						  DUPLEX_HALF;
> +
> +	switch (val & AN8855_PMSR_SPEED) {
> +	case AN8855_PMSR_SPEED_10:
> +		state->speed = SPEED_10;
> +		break;
> +	case AN8855_PMSR_SPEED_100:
> +		state->speed = SPEED_100;
> +		break;
> +	case AN8855_PMSR_SPEED_1000:
> +		state->speed = SPEED_1000;
> +		break;
> +	case AN8855_PMSR_SPEED_2500:
> +		state->speed = SPEED_2500;
> +		break;
> +	case AN8855_PMSR_SPEED_5000:
> +		dev_err(priv->dev, "Missing support for 5G speed. Setting Unknown.\n");
> +		fallthrough;
> +	default:
> +		state->speed = SPEED_UNKNOWN;
> +		break;
> +	}
> +
> +	if (val & AN8855_PMSR_RX_FC)
> +		state->pause |= MLO_PAUSE_RX;
> +	if (val & AN8855_PMSR_TX_FC)
> +		state->pause |= MLO_PAUSE_TX;
> +}
> +
> +static int an8855_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +			     phy_interface_t interface,
> +			     const unsigned long *advertising,
> +			     bool permit_pause_to_mac)
> +{
> +	struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs);
> +	u32 val;
> +	int ret;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +			dev_err(priv->dev, "in-band negotiation unsupported");
> +			return -EINVAL;
> +		}
> +		break;

Now that we have the phylink in-band changes merged, along with a new
PCS .pcs_inband_caps() method, please implement this method to indicate
that in-band is not supported in 2500BASE-X mode. Phylink will issue a
warning if it attempts to go against that (particularly when e.g. the
PHY demands in-band but the PCS doesn't support it - a case that likely
will never work in practice.)

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/*                   !!! WELCOME TO HELL !!!                   */
> +
> +	/* TX FIR - improve TX EYE */
> +	ret = regmap_update_bits(priv->regmap, AN8855_INTF_CTRL_10,
> +				 AN8855_RG_DA_QP_TX_FIR_C2_SEL |
> +				 AN8855_RG_DA_QP_TX_FIR_C2_FORCE |
> +				 AN8855_RG_DA_QP_TX_FIR_C1_SEL |
> +				 AN8855_RG_DA_QP_TX_FIR_C1_FORCE,
> +				 AN8855_RG_DA_QP_TX_FIR_C2_SEL |
> +				 FIELD_PREP(AN8855_RG_DA_QP_TX_FIR_C2_FORCE, 0x4) |
> +				 AN8855_RG_DA_QP_TX_FIR_C1_SEL |
> +				 FIELD_PREP(AN8855_RG_DA_QP_TX_FIR_C1_FORCE, 0x0));
> +	if (ret)
> +		return ret;
> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x0;
> +	else
> +		val = 0xd;
> +	ret = regmap_update_bits(priv->regmap, AN8855_INTF_CTRL_11,
> +				 AN8855_RG_DA_QP_TX_FIR_C0B_SEL |
> +				 AN8855_RG_DA_QP_TX_FIR_C0B_FORCE,
> +				 AN8855_RG_DA_QP_TX_FIR_C0B_SEL |
> +				 FIELD_PREP(AN8855_RG_DA_QP_TX_FIR_C0B_FORCE, val));
> +	if (ret)
> +		return ret;
> +
> +	/* RX CDR - improve RX Jitter Tolerance */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x5;
> +	else
> +		val = 0x6;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_BOT_LIM,
> +				 AN8855_RG_QP_CDR_LPF_KP_GAIN |
> +				 AN8855_RG_QP_CDR_LPF_KI_GAIN,
> +				 FIELD_PREP(AN8855_RG_QP_CDR_LPF_KP_GAIN, val) |
> +				 FIELD_PREP(AN8855_RG_QP_CDR_LPF_KI_GAIN, val));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x1;
> +	else
> +		val = 0x0;
> +	ret = regmap_update_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_1,
> +				 AN8855_RG_TPHY_SPEED,
> +				 FIELD_PREP(AN8855_RG_TPHY_SPEED, val));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - LPF */
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2,
> +				 AN8855_RG_DA_QP_PLL_RICO_SEL_INTF |
> +				 AN8855_RG_DA_QP_PLL_FBKSEL_INTF |
> +				 AN8855_RG_DA_QP_PLL_BR_INTF |
> +				 AN8855_RG_DA_QP_PLL_BPD_INTF |
> +				 AN8855_RG_DA_QP_PLL_BPA_INTF |
> +				 AN8855_RG_DA_QP_PLL_BC_INTF,
> +				 AN8855_RG_DA_QP_PLL_RICO_SEL_INTF |
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_FBKSEL_INTF, 0x0) |
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_BR_INTF, 0x3) |
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_BPD_INTF, 0x0) |
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_BPA_INTF, 0x5) |
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_BC_INTF, 0x1));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - ICO */
> +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_4,
> +			      AN8855_RG_DA_QP_PLL_ICOLP_EN_INTF);
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2,
> +				AN8855_RG_DA_QP_PLL_ICOIQ_EN_INTF);
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - CHP */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x6;
> +	else
> +		val = 0x4;
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2,
> +				 AN8855_RG_DA_QP_PLL_IR_INTF,
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_IR_INTF, val));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - PFD */
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2,
> +				 AN8855_RG_DA_QP_PLL_PFD_OFFSET_EN_INTRF |
> +				 AN8855_RG_DA_QP_PLL_PFD_OFFSET_INTF |
> +				 AN8855_RG_DA_QP_PLL_KBAND_PREDIV_INTF,
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_PFD_OFFSET_INTF, 0x1) |
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_KBAND_PREDIV_INTF, 0x1));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - POSTDIV */
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2,
> +				 AN8855_RG_DA_QP_PLL_POSTDIV_EN_INTF |
> +				 AN8855_RG_DA_QP_PLL_PHY_CK_EN_INTF |
> +				 AN8855_RG_DA_QP_PLL_PCK_SEL_INTF,
> +				 AN8855_RG_DA_QP_PLL_PCK_SEL_INTF);
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - SDM */
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_2,
> +				 AN8855_RG_DA_QP_PLL_SDM_HREN_INTF,
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_SDM_HREN_INTF, 0x0));
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CTRL_2,
> +				AN8855_RG_DA_QP_PLL_SDM_IFM_INTF);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_SS_LCPLL_PWCTL_SETTING_2,
> +				 AN8855_RG_NCPO_ANA_MSB,
> +				 FIELD_PREP(AN8855_RG_NCPO_ANA_MSB, 0x1));
> +	if (ret)
> +		return ret;
> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x7a000000;
> +	else
> +		val = 0x48000000;
> +	ret = regmap_write(priv->regmap, AN8855_SS_LCPLL_TDC_FLT_2,
> +			   FIELD_PREP(AN8855_RG_LCPLL_NCPO_VALUE, val));
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(priv->regmap, AN8855_SS_LCPLL_TDC_PCW_1,
> +			   FIELD_PREP(AN8855_RG_LCPLL_PON_HRDDS_PCW_NCPO_GPON, val));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(priv->regmap, AN8855_SS_LCPLL_TDC_FLT_5,
> +				AN8855_RG_LCPLL_NCPO_CHG);
> +	if (ret)
> +		return ret;
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CK_CTRL_0,
> +				AN8855_RG_DA_QP_PLL_SDM_DI_EN_INTF);
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - SS */
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_3,
> +				 AN8855_RG_DA_QP_PLL_SSC_DELTA_INTF,
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_SSC_DELTA_INTF, 0x0));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_4,
> +				 AN8855_RG_DA_QP_PLL_SSC_DIR_DLY_INTF,
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_SSC_DIR_DLY_INTF, 0x0));
> +	if (ret)
> +		return ret;
> +	ret = regmap_update_bits(priv->regmap, AN8855_PLL_CTRL_3,
> +				 AN8855_RG_DA_QP_PLL_SSC_PERIOD_INTF,
> +				 FIELD_PREP(AN8855_RG_DA_QP_PLL_SSC_PERIOD_INTF, 0x0));
> +	if (ret)
> +		return ret;
> +
> +	/* PLL - TDC */
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PLL_CK_CTRL_0,
> +				AN8855_RG_DA_QP_PLL_TDC_TXCK_SEL_INTF);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_QP_PLL_SDM_ORD,
> +			      AN8855_RG_QP_PLL_SSC_TRI_EN);
> +	if (ret)
> +		return ret;
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_QP_PLL_SDM_ORD,
> +			      AN8855_RG_QP_PLL_SSC_PHASE_INI);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_RX_DAC_EN,
> +				 AN8855_RG_QP_SIGDET_HF,
> +				 FIELD_PREP(AN8855_RG_QP_SIGDET_HF, 0x2));
> +	if (ret)
> +		return ret;
> +
> +	/* TCL Disable (only for Co-SIM) */
> +	ret = regmap_clear_bits(priv->regmap, AN8855_PON_RXFEDIG_CTRL_0,
> +				AN8855_RG_QP_EQ_RX500M_CK_SEL);
> +	if (ret)
> +		return ret;
> +
> +	/* TX Init */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x4;
> +	else
> +		val = 0x0;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_TX_MODE,
> +				 AN8855_RG_QP_TX_RESERVE |
> +				 AN8855_RG_QP_TX_MODE_16B_EN,
> +				 FIELD_PREP(AN8855_RG_QP_TX_RESERVE, val));
> +	if (ret)
> +		return ret;
> +
> +	/* RX Control/Init */
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_QP_RXAFE_RESERVE,
> +			      AN8855_RG_QP_CDR_PD_10B_EN);
> +	if (ret)
> +		return ret;
> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x1;
> +	else
> +		val = 0x2;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_MJV_LIM,
> +				 AN8855_RG_QP_CDR_LPF_RATIO,
> +				 FIELD_PREP(AN8855_RG_QP_CDR_LPF_RATIO, val));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_LPF_SETVALUE,
> +				 AN8855_RG_QP_CDR_PR_BUF_IN_SR |
> +				 AN8855_RG_QP_CDR_PR_BETA_SEL,
> +				 FIELD_PREP(AN8855_RG_QP_CDR_PR_BUF_IN_SR, 0x6) |
> +				 FIELD_PREP(AN8855_RG_QP_CDR_PR_BETA_SEL, 0x1));
> +	if (ret)
> +		return ret;
> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0xf;
> +	else
> +		val = 0xc;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_PR_CKREF_DIV1,
> +				 AN8855_RG_QP_CDR_PR_DAC_BAND,
> +				 FIELD_PREP(AN8855_RG_QP_CDR_PR_DAC_BAND, val));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE,
> +				 AN8855_RG_QP_CDR_PR_KBAND_PCIE_MODE |
> +				 AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE_MASK,
> +				 FIELD_PREP(AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE_MASK, 0x19));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_FORCE_IBANDLPF_R_OFF,
> +				 AN8855_RG_QP_CDR_PHYCK_SEL |
> +				 AN8855_RG_QP_CDR_PHYCK_RSTB |
> +				 AN8855_RG_QP_CDR_PHYCK_DIV,
> +				 FIELD_PREP(AN8855_RG_QP_CDR_PHYCK_SEL, 0x2) |
> +				 FIELD_PREP(AN8855_RG_QP_CDR_PHYCK_DIV, 0x21));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_QP_CDR_PR_KBAND_DIV_PCIE,
> +				AN8855_RG_QP_CDR_PR_XFICK_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RG_QP_CDR_PR_CKREF_DIV1,
> +				 AN8855_RG_QP_CDR_PR_KBAND_DIV,
> +				 FIELD_PREP(AN8855_RG_QP_CDR_PR_KBAND_DIV, 0x4));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_26,
> +				 AN8855_RG_QP_EQ_RETRAIN_ONLY_EN |
> +				 AN8855_RG_LINK_NE_EN |
> +				 AN8855_RG_LINK_ERRO_EN,
> +				 AN8855_RG_QP_EQ_RETRAIN_ONLY_EN |
> +				 AN8855_RG_LINK_ERRO_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_DLY_0,
> +				 AN8855_RG_QP_RX_SAOSC_EN_H_DLY |
> +				 AN8855_RG_QP_RX_PI_CAL_EN_H_DLY,
> +				 FIELD_PREP(AN8855_RG_QP_RX_SAOSC_EN_H_DLY, 0x3f) |
> +				 FIELD_PREP(AN8855_RG_QP_RX_PI_CAL_EN_H_DLY, 0x6f));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_42,
> +				 AN8855_RG_QP_EQ_EN_DLY,
> +				 FIELD_PREP(AN8855_RG_QP_EQ_EN_DLY, 0x150));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_2,
> +				 AN8855_RG_QP_RX_EQ_EN_H_DLY,
> +				 FIELD_PREP(AN8855_RG_QP_RX_EQ_EN_H_DLY, 0x150));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_PON_RXFEDIG_CTRL_9,
> +				 AN8855_RG_QP_EQ_LEQOSC_DLYCNT,
> +				 FIELD_PREP(AN8855_RG_QP_EQ_LEQOSC_DLYCNT, 0x1));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_8,
> +				 AN8855_RG_DA_QP_SAOSC_DONE_TIME |
> +				 AN8855_RG_DA_QP_LEQOS_EN_TIME,
> +				 FIELD_PREP(AN8855_RG_DA_QP_SAOSC_DONE_TIME, 0x200) |
> +				 FIELD_PREP(AN8855_RG_DA_QP_LEQOS_EN_TIME, 0xfff));
> +	if (ret)
> +		return ret;
> +
> +	/* Frequency meter */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +		val = 0x10;
> +	else
> +		val = 0x28;
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_5,
> +				 AN8855_RG_FREDET_CHK_CYCLE,
> +				 FIELD_PREP(AN8855_RG_FREDET_CHK_CYCLE, val));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_6,
> +				 AN8855_RG_FREDET_GOLDEN_CYCLE,
> +				 FIELD_PREP(AN8855_RG_FREDET_GOLDEN_CYCLE, 0x64));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, AN8855_RX_CTRL_7,
> +				 AN8855_RG_FREDET_TOLERATE_CYCLE,
> +				 FIELD_PREP(AN8855_RG_FREDET_TOLERATE_CYCLE, 0x2710));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(priv->regmap, AN8855_PLL_CTRL_0,
> +			      AN8855_RG_PHYA_AUTO_INIT);
> +	if (ret)
> +		return ret;
> +
> +	/* PCS Init */
> +	if (interface == PHY_INTERFACE_MODE_SGMII &&
> +	    neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED) {
> +		ret = regmap_clear_bits(priv->regmap, AN8855_QP_DIG_MODE_CTRL_0,
> +					AN8855_RG_SGMII_MODE | AN8855_RG_SGMII_AN_EN);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_clear_bits(priv->regmap, AN8855_RG_HSGMII_PCS_CTROL_1,
> +				AN8855_RG_TBI_10B_MODE);
> +	if (ret)
> +		return ret;
> +
> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +		/* Set AN Ability - Interrupt */
> +		ret = regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN_FORCE_CL37,
> +				      AN8855_RG_FORCE_AN_DONE);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(priv->regmap, AN8855_SGMII_REG_AN_13,
> +					 AN8855_SGMII_REMOTE_FAULT_DIS |
> +					 AN8855_SGMII_IF_MODE,
> +					 AN8855_SGMII_REMOTE_FAULT_DIS |
> +					 FIELD_PREP(AN8855_SGMII_IF_MODE, 0xb));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Rate Adaption - GMII path config. */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		ret = regmap_clear_bits(priv->regmap, AN8855_RATE_ADP_P0_CTRL_0,
> +					AN8855_RG_P0_DIS_MII_MODE);
> +		if (ret)
> +			return ret;
> +	} else {
> +		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +			ret = regmap_set_bits(priv->regmap, AN8855_MII_RA_AN_ENABLE,
> +					      AN8855_RG_P0_RA_AN_EN);
> +			if (ret)
> +				return ret;
> +		} else {
> +			ret = regmap_update_bits(priv->regmap, AN8855_RG_AN_SGMII_MODE_FORCE,
> +						 AN8855_RG_FORCE_CUR_SGMII_MODE |
> +						 AN8855_RG_FORCE_CUR_SGMII_SEL,
> +						 AN8855_RG_FORCE_CUR_SGMII_SEL);
> +			if (ret)
> +				return ret;
> +
> +			ret = regmap_clear_bits(priv->regmap, AN8855_RATE_ADP_P0_CTRL_0,
> +						AN8855_RG_P0_MII_RA_RX_EN |
> +						AN8855_RG_P0_MII_RA_TX_EN |
> +						AN8855_RG_P0_MII_RA_RX_MODE |
> +						AN8855_RG_P0_MII_RA_TX_MODE);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = regmap_set_bits(priv->regmap, AN8855_RATE_ADP_P0_CTRL_0,
> +				      AN8855_RG_P0_MII_MODE);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_set_bits(priv->regmap, AN8855_RG_RATE_ADAPT_CTRL_0,
> +			      AN8855_RG_RATE_ADAPT_RX_BYPASS |
> +			      AN8855_RG_RATE_ADAPT_TX_BYPASS |
> +			      AN8855_RG_RATE_ADAPT_RX_EN |
> +			      AN8855_RG_RATE_ADAPT_TX_EN);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable AN if not in autoneg */
> +	ret = regmap_update_bits(priv->regmap, AN8855_SGMII_REG_AN0, BMCR_ANENABLE,
> +				 neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED ? BMCR_ANENABLE :
> +									      0);
> +	if (ret)
> +		return ret;
> +
> +	if (interface == PHY_INTERFACE_MODE_SGMII &&
> +	    neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED) {
> +		ret = regmap_set_bits(priv->regmap, AN8855_PHY_RX_FORCE_CTRL_0,
> +				      AN8855_RG_FORCE_TXC_SEL);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Force Speed with fixed-link or 2500base-x as doesn't support aneg */
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX ||
> +	    neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +		if (interface == PHY_INTERFACE_MODE_2500BASEX)
> +			val = AN8855_RG_LINK_MODE_P0_SPEED_2500;
> +		else
> +			val = AN8855_RG_LINK_MODE_P0_SPEED_1000;
> +		ret = regmap_update_bits(priv->regmap, AN8855_SGMII_STS_CTRL_0,
> +					 AN8855_RG_LINK_MODE_P0 |
> +					 AN8855_RG_FORCE_SPD_MODE_P0,
> +					 val | AN8855_RG_FORCE_SPD_MODE_P0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* bypass flow control to MAC */
> +	ret = regmap_write(priv->regmap, AN8855_MSG_RX_LIK_STS_0,
> +			   AN8855_RG_DPX_STS_P3 | AN8855_RG_DPX_STS_P2 |
> +			   AN8855_RG_DPX_STS_P1 | AN8855_RG_TXFC_STS_P0 |
> +			   AN8855_RG_RXFC_STS_P0 | AN8855_RG_DPX_STS_P0);
> +	if (ret)
> +		return ret;
> +	ret = regmap_write(priv->regmap, AN8855_MSG_RX_LIK_STS_2,
> +			   AN8855_RG_RXFC_AN_BYPASS_P3 |
> +			   AN8855_RG_RXFC_AN_BYPASS_P2 |
> +			   AN8855_RG_RXFC_AN_BYPASS_P1 |
> +			   AN8855_RG_TXFC_AN_BYPASS_P3 |
> +			   AN8855_RG_TXFC_AN_BYPASS_P2 |
> +			   AN8855_RG_TXFC_AN_BYPASS_P1 |
> +			   AN8855_RG_DPX_AN_BYPASS_P3 |
> +			   AN8855_RG_DPX_AN_BYPASS_P2 |
> +			   AN8855_RG_DPX_AN_BYPASS_P1 |
> +			   AN8855_RG_DPX_AN_BYPASS_P0);
> +	if (ret)
> +		return ret;

Is the above disruptive to the link if it is executed when the link is
already up? Do you need to re-execute it even when "interface" hasn't
changed?

This gets called to possibly change the PCS advertisement in response
to ethtool -s, so ought not have any link disrupting effects if
nothing has changed.

> +
> +	return 0;
> +}
> +
> +static void an8855_pcs_an_restart(struct phylink_pcs *pcs)
> +{
> +	struct an8855_priv *priv = container_of(pcs, struct an8855_priv, pcs);
> +
> +	regmap_set_bits(priv->regmap, AN8855_SGMII_REG_AN0, BMCR_ANRESTART);

This won't get called for SGMII - Cisco SGMII is merely the PHY telling
the PCS/MAC what was negotiated at the media and how it's going to be
operating the SGMII link. AN restart is a media side thing, so that
would happen at the PHY in that case. I suggest this becomes an empty
no-op function.

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!