Re: [PATCH net-next v2 3/6] net: pcs: xpcs: add CL37 1000BASE-X AN support

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Ong Boon Leong <boon.leong.ong@intel.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Jose Abreu <Jose.Abreu@synopsys.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Paolo Abeni <pabeni@redhat.com>,
	"David S . Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Vladimir Oltean <olteanv@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Giuseppe Cavallaro <peppe.cavallaro@st.com>,
	netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Emilio Riva <emilio.riva@ericsson.com>
Subject: Re: [PATCH net-next v2 3/6] net: pcs: xpcs: add CL37 1000BASE-X AN support
Date: Fri, 10 Jun 2022 08:26:55 +0100	[thread overview]
Message-ID: <YqLyP6ezO3C9Fe4t@shell.armlinux.org.uk> (raw)
In-Reply-To: <20220610032941.113690-4-boon.leong.ong@intel.com>

On Fri, Jun 10, 2022 at 11:29:38AM +0800, Ong Boon Leong wrote:
> +static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
> +					struct phylink_link_state *state)
> +{
> +	int lpa, adv;
> +	int ret;
> +
> +	if (state->an_enabled) {
> +		/* Reset link state */
> +		state->link = false;
> +
> +		lpa = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_LPA);
> +		if (lpa < 0 || lpa & LPA_RFAULT)
> +			return lpa;
> +
> +		adv = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
> +		if (adv < 0)
> +			return adv;
> +
> +		if (lpa & ADVERTISE_1000XFULL &&
> +		    adv & ADVERTISE_1000XFULL) {
> +			state->link = true;
> +			state->speed = SPEED_1000;
> +			state->duplex = DUPLEX_FULL;
> +		}

phylink_mii_c22_pcs_decode_state() is your friend here, will implement
this correctly, and will set lp_advertising correctly as well.

> +
> +		/* Clear CL37 AN complete status */
> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +		if (ret < 0)
> +			return ret;

Why do you need to clear the interrupt status here? This function will
be called from a work queue sometime later after an interrupt has fired.
It will also be called at random times when userspace enquires what the
link parameters are, so clearing the interrupt here can result in lost
link changes.

> +static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, int speed,
> +				   int duplex)
> +{
> +	int val, ret;
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		val = BMCR_SPEED1000;
> +		break;
> +	case SPEED_100:
> +	case SPEED_10:
> +	default:
> +		pr_err("%s: speed = %d\n", __func__, speed);
> +		return;
> +	}
> +
> +	if (duplex == DUPLEX_FULL)
> +		val |= BMCR_FULLDPLX;
> +	else
> +		pr_err("%s: half duplex not supported\n", __func__);
> +
> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, val);
> +	if (ret)
> +		pr_err("%s: xpcs_write returned %pe\n", __func__, ERR_PTR(ret));

Does this need to be done even when AN is enabled?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel