Re: [net-next PATCH v12 12/13] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

[Christian Marangi]

On Sun, Mar 09, 2025 at 05:57:20PM +0000, Russell King (Oracle) wrote:
> On Sun, Mar 09, 2025 at 06:26:57PM +0100, Christian Marangi wrote:
> > +static int an8855_port_enable(struct dsa_switch *ds, int port,
> > +			      struct phy_device *phy)
> > +{
> > +	struct an8855_priv *priv = ds->priv;
> > +
> > +	return regmap_set_bits(priv->regmap, AN8855_PMCR_P(port),
> > +			       AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN);
> 
> Shouldn't you wait for phylink to call your mac_link_up() method?
>

Did something change recently for this? I checked the pattern for other
driver and port enable normally just enable TX/RX traffic for the port.

Any hint for this?

> > +}
> > +
> > +static void an8855_port_disable(struct dsa_switch *ds, int port)
> > +{
> > +	struct an8855_priv *priv = ds->priv;
> > +	int ret;
> > +
> > +	ret = regmap_clear_bits(priv->regmap, AN8855_PMCR_P(port),
> > +				AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN);
> > +	if (ret)
> > +		dev_err(priv->ds->dev, "failed to disable port: %d\n", ret);
> 
> Doesn't the link get set down before this is called? IOW, doesn't
> phylink call your mac_link_down() method first?
> 
> ...
> 
> > +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->ds->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);
> 
> Why? Maybe consider implementing the phylink tx_lpi functions for EEE
> support.
> 

Will do, I disabled this as the EEE rework was being approved.

> > +	}
> > +
> > +	reg |= AN8855_PMCR_TX_EN | AN8855_PMCR_RX_EN;
> > +
> > +	regmap_write(priv->regmap, AN8855_PMCR_P(port), reg);
> > +}
> > +
> > +static unsigned int an8855_pcs_inband_caps(struct phylink_pcs *pcs,
> > +					   phy_interface_t interface)
> > +{
> > +	/* SGMII can be configured to use inband with AN result */
> > +	if (interface == PHY_INTERFACE_MODE_SGMII)
> > +		return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;
> > +
> > +	/* inband is not supported in 2500-baseX and must be disabled */
> > +	return  LINK_INBAND_DISABLE;
> 
> Spurious double space.
> 

Will drop.

> > +}
> > +
> > +static void an8855_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
> > +				 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->ds->dev, "Missing support for 5G speed. Setting Unknown.\n");
> > +		fallthrough;
> 
> Which is wrong now, we have SPEED_5000.
> 

Maybe the comments weren't so clear. The Switch doesn't support the
speed... Even if it does have bits, the switch doesn't support it. And
the 2500 speed is really only for the CPU port. The user port are only
gigabit.

> > +	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;
> > +
> > +	/*                   !!! WELCOME TO HELL !!!                   */
> > +
> [... hell ...]

Will drop :( It was an easter egg for the 300 lines to configure PCS.

> > +	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;
> > +
> > +	return 0;
> 
> Is this disruptive to the link if the link is up, and this is called
> (e.g. to change the advertisement rather than switch interface mode).
> If so, please do something about that - e.g. only doing the bulk of
> the configuration if the interface mode has changed.

Airoha confirmed this is not disruptive, applying these config doesn't
terminate or disrupt the link.

> 
> I guess, however, that as you're only using SGMII with in-band, it
> probably doesn't make much difference, but having similar behaviour
> in the various drivers helps with ongoing maintenance.

Do we have some driver that implement the logic of skipping the bulk of
configuration if the mode doesn't change?

Maybe we can introduce some kind of additional OP like .init to apply
the very initial configuration that are not related to the mode.
Or something like .setup?

-- 
	Ansuel