Re: [PATCH net-next 3/5] net: lan966x: add port module support

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

Hi,

On Wed, Nov 17, 2021 at 10:18:56AM +0100, Horatiu Vultur wrote:
> +static void lan966x_phylink_mac_link_state(struct phylink_config *config,
> +					   struct phylink_link_state *state)
> +{
> +}
> +
> +static void lan966x_phylink_mac_aneg_restart(struct phylink_config *config)
> +{
> +}

Since you always attach a PCS, it is not necessary to provide stubs
for these functions.

> +static int lan966x_pcs_config(struct phylink_pcs *pcs,
> +			      unsigned int mode,
> +			      phy_interface_t interface,
> +			      const unsigned long *advertising,
> +			      bool permit_pause_to_mac)
> +{
> +	struct lan966x_port *port = lan966x_pcs_to_port(pcs);
> +	struct lan966x_port_config config;
> +	int ret;
> +
> +	memset(&config, 0, sizeof(config));
> +
> +	config = port->config;
> +	config.portmode = interface;
> +	config.inband = phylink_autoneg_inband(mode);
> +	config.autoneg = phylink_test(advertising, Autoneg);
> +	if (phylink_test(advertising, Pause))
> +		config.pause_adv |= ADVERTISE_1000XPAUSE;
> +	if (phylink_test(advertising, Asym_Pause))
> +		config.pause_adv |= ADVERTISE_1000XPSE_ASYM;

There are patches around that add
phylink_mii_c22_pcs_encode_advertisement() which will create the C22
advertisement for you. It would be good to get that patch merged so
people can use it. That should also eliminate lan966x_get_aneg_word(),
although I notice you need to set ADVERTISE_LPACK as well (can you
check that please? Hardware should be managing that bit as it should
only be set once the hardware has received the link partner's
advertisement.)

> +static void decode_cl37_word(u16 lp_abil, uint16_t ld_abil,
> +			     struct lan966x_port_status *status)
> +{
> +	status->link = !(lp_abil & ADVERTISE_RFAULT) && status->link;
> +	status->an_complete = true;
> +	status->duplex = (ADVERTISE_1000XFULL & lp_abil) ?
> +		DUPLEX_FULL : DUPLEX_UNKNOWN;
> +
> +	if ((ld_abil & ADVERTISE_1000XPAUSE) &&
> +	    (lp_abil & ADVERTISE_1000XPAUSE)) {
> +		status->pause = MLO_PAUSE_RX | MLO_PAUSE_TX;
> +	} else if ((ld_abil & ADVERTISE_1000XPSE_ASYM) &&
> +		   (lp_abil & ADVERTISE_1000XPSE_ASYM)) {
> +		status->pause |= (lp_abil & ADVERTISE_1000XPAUSE) ?
> +			MLO_PAUSE_TX : 0;
> +		status->pause |= (ld_abil & ADVERTISE_1000XPAUSE) ?
> +			MLO_PAUSE_RX : 0;
> +	} else {
> +		status->pause = MLO_PAUSE_NONE;
> +	}
> +}

We already have phylink_decode_c37_word() which will decode this for
you, although it would need to be exported. Please re-use this code.

> +
> +static void decode_sgmii_word(u16 lp_abil, struct lan966x_port_status *status)
> +{
> +	status->an_complete = true;
> +	if (!(lp_abil & LPA_SGMII_LINK)) {
> +		status->link = false;
> +		return;
> +	}
> +
> +	switch (lp_abil & LPA_SGMII_SPD_MASK) {
> +	case LPA_SGMII_10:
> +		status->speed = SPEED_10;
> +		break;
> +	case LPA_SGMII_100:
> +		status->speed = SPEED_100;
> +		break;
> +	case LPA_SGMII_1000:
> +		status->speed = SPEED_1000;
> +		break;
> +	default:
> +		status->link = false;
> +		return;
> +	}
> +	if (lp_abil & LPA_SGMII_FULL_DUPLEX)
> +		status->duplex = DUPLEX_FULL;
> +	else
> +		status->duplex = DUPLEX_HALF;
> +}

The above mentioned function will also handle SGMII as well.

> +int lan966x_port_pcs_set(struct lan966x_port *port,
> +			 struct lan966x_port_config *config)
> +{
> +	struct lan966x *lan966x = port->lan966x;
> +	bool sgmii = false, inband_aneg = false;
> +	int err;
> +
> +	lan966x_port_link_down(port);
> +
> +	if (config->inband) {
> +		if (config->portmode == PHY_INTERFACE_MODE_SGMII ||
> +		    config->portmode == PHY_INTERFACE_MODE_QSGMII)
> +			inband_aneg = true; /* Cisco-SGMII in-band-aneg */
> +		else if (config->portmode == PHY_INTERFACE_MODE_1000BASEX &&
> +			 config->autoneg)
> +			inband_aneg = true; /* Clause-37 in-band-aneg */
> +
> +		if (config->speed > 0) {
> +			err = phy_set_speed(port->serdes, config->speed);
> +			if (err)
> +				return err;
> +		}
> +
> +	} else {
> +		sgmii = true; /* Phy is connnected to the MAC */

This looks weird. SGMII can be in-band as well (and technically is
in-band in its as-specified form.)

> +	}
> +
> +	/* Choose SGMII or 1000BaseX/2500BaseX PCS mode */
> +	lan_rmw(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_SET(sgmii),
> +		DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA,
> +		lan966x, DEV_PCS1G_MODE_CFG(port->chip_port));

With the code as you have it, what this means is if we specify
SGMII + in-band, we end up configuring the port to be in 1000BaseX
mode, so it's incapable of 10 and 100M speeds. This seems incorrect.

Thanks.

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