Re: [PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Russell King - ARM Linux admin]

On Wed, Jun 10, 2020 at 08:14:03PM +0100, Jonathan McDowell wrote:
> Update the driver to use the new PHYLINK callbacks, removing the
> legacy adjust_link callback.

Looks good, there's a couple of issues / questions

>  static void
> +qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> +			 const struct phylink_link_state *state)
>  {
>  	struct qca8k_priv *priv = ds->priv;
>  	u32 reg;
>  
> +	switch (port) {
...
> +	case 6: /* 2nd CPU port / external PHY */
> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +		    state->interface != PHY_INTERFACE_MODE_SGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
> +			return;
> +
> +		reg = QCA8K_REG_PORT6_PAD_CTRL;
> +		break;
...
> +	}
> +
> +	if (port != 6 && phylink_autoneg_inband(mode)) {
> +		dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
> +			__func__);
> +		return;
> +	}
> +
> +	switch (state->interface) {
...
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		/* Enable SGMII on the port */
> +		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> +		break;

Is inband mode configurable?  What if the link partner does/doesn't
send the configuration word?  How is the link state communicated to
the MAC?

> +static int
> +qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
> +			     struct phylink_link_state *state)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	u32 reg;
>  
> +	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
> +
> +	state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
> +	state->an_complete = state->link;
> +	state->an_enabled = !!(reg & QCA8K_PORT_STATUS_LINK_AUTO);
> +	state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
> +							   DUPLEX_HALF;
> +
> +	switch (reg & QCA8K_PORT_STATUS_SPEED) {
> +	case QCA8K_PORT_STATUS_SPEED_10:
> +		state->speed = SPEED_10;
> +		break;
> +	case QCA8K_PORT_STATUS_SPEED_100:
> +		state->speed = SPEED_100;
> +		break;
> +	case QCA8K_PORT_STATUS_SPEED_1000:
> +		state->speed = SPEED_1000;
> +		break;
> +	default:
> +		state->speed = SPEED_UNKNOWN;

Maybe also force the link down in this case, since the state is invalid?

Do you have access to the link partner's configuration word?  If you do,
you should use that to fill in state->lp_advertising.

> +		break;
> +	}
> +
> +	state->pause = MLO_PAUSE_NONE;
> +	if (reg & QCA8K_PORT_STATUS_RXFLOW)
> +		state->pause |= MLO_PAUSE_RX;
> +	if (reg & QCA8K_PORT_STATUS_TXFLOW)
> +		state->pause |= MLO_PAUSE_TX;
> +
> +	return 1;
> +}
> +
> +static void
> +qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
> +			    phy_interface_t interface)
> +{
> +	struct qca8k_priv *priv = ds->priv;
>  
>  	qca8k_port_set_status(priv, port, 0);

If operating in in-band mode, forcing the link down unconditionally
will prevent the link coming up if the SGMII/1000base-X block
automatically updates the MAC, and if this takes precedence.

When using in-band mode, you need to call dsa_port_phylink_mac_change()
to keep phylink updated with the link status.

Alternatively, phylink supports polling mode, but due to the layered
way DSA is written, DSA drivers don't have access to that as that is
in the DSA upper levels in net/dsa/slave.c (dsa_slave_phy_setup(),
it would be dp->pl_config.pcs_poll).

Apart from those points, I think it looks fine, thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 503kbps up