diff mbox series

[net-next,1/4] net: pcs: xpcs: add CL37 1000BASE-X AN support

Message ID 20220422073505.810084-2-boon.leong.ong@intel.com (mailing list archive)
State New, archived
Headers show
Series pcs-xpcs, stmmac: add 1000BASE-X AN for network switch | expand

Commit Message

Ong Boon Leong April 22, 2022, 7:35 a.m. UTC 
For CL37 1000BASE-X AN, DW xPCS does not support C22 method but offers
C45 vendor-specific MII MMD for programming.

We also add the ability to disable Autoneg (through ethtool for certain
network switch that supports 1000BASE-X (1000Mbps and Full-Duplex) but
not Autoneg capability.

Tested-by: Emilio Riva <emilio.riva@ericsson.com>
Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
---
 drivers/net/pcs/pcs-xpcs.c   | 174 ++++++++++++++++++++++++++++++++++-
 include/linux/pcs/pcs-xpcs.h |   3 +-
 2 files changed, 173 insertions(+), 4 deletions(-)

Comments

Russell King (Oracle) April 22, 2022, 8 a.m. UTC | #1 
Hi,

On Fri, Apr 22, 2022 at 03:35:02PM +0800, Ong Boon Leong wrote:
> @@ -774,6 +788,58 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
>  	return ret;
>  }
>  
> +static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs, unsigned int mode,
> +					  const unsigned long *advertising)
> +{
> +	int ret, mdio_ctrl;
> +
> +	/* For AN for 1000BASE-X mode, the settings are :-
> +	 * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable C37 AN in case
> +	 *    it is already enabled)
> +	 * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 00b (1000BASE-X C37)
> +	 * 3) SR_MII_AN_ADV Bit(6)[FD] = 1b (Full Duplex)
> +	 *    Note: Half Duplex is rarely used, so don't advertise.
> +	 * 4) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable C37 AN)

So if this function gets called to update the advertisement - even if
there is no actual change - we go through a AN-disable..AN-enable
dance and cause the link to re-negotiate. That doesn't sound like nice
behaviour.

> +	 */
> +	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
> +	if (mdio_ctrl < 0)
> +		return mdio_ctrl;
> +
> +	if (mdio_ctrl & AN_CL37_EN) {
> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
> +				 mdio_ctrl & ~AN_CL37_EN);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret &= ~DW_VR_MII_PCS_MODE_MASK;
> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, ret);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
> +	ret |= ADVERTISE_1000XFULL;
> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE, ret);

What if other bits are already set in the MII_ADVERTISE register?
Maybe consider using phylink_mii_c22_pcs_encode_advertisement()?

The pcs_config() method is also supposed to return either a negative
error, 0 for no advertisement change, or positive for an advertisement
change, in which case phylink will trigger a call to pcs_an_restart().

> +static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
> +					struct phylink_link_state *state)
> +{
> +	int lpa, adv;
> +	int ret;
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & AN_CL37_EN) {
> +		/* Reset link_state */
> +		state->link = false;
> +		state->speed = SPEED_UNKNOWN;
> +		state->duplex = DUPLEX_UNKNOWN;
> +		state->pause = 0;

Phylink guarantees that speed, duplex and pause are set to something
sensible - please remove these. The only one you probably need here
is state->link.

> +
> +		lpa = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_LPA);
> +		if (lpa < 0 || lpa & LPA_RFAULT)
> +			return false;

This function does not return a boolean. Returning "false" is the same
as returning 0, which means "no error" but an error has occurred.

> +
> +		adv = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
> +		if (adv < 0)
> +			return false;

Ditto.

> +
> +		if (lpa & ADVERTISE_1000XFULL &&
> +		    adv & ADVERTISE_1000XFULL) {
> +			state->speed = SPEED_1000;
> +			state->duplex = DUPLEX_FULL;
> +			state->link = true;
> +		}
> +
> +		/* Clear CL37 AN complete status */
> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +	} else {
> +		state->link = true;
> +		state->speed = SPEED_1000;
> +		state->duplex = DUPLEX_FULL;
> +		state->pause = 0;

If we're in AN-disabled mode, phylink will set state->speed and
state->duplex according to the user's parameters, so there should be no
need to do it here.

> @@ -994,9 +1143,21 @@ void xpcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
>  		return xpcs_config_usxgmii(xpcs, speed);
>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>  		return xpcs_link_up_sgmii(xpcs, mode, speed, duplex);
> +	if (interface == PHY_INTERFACE_MODE_1000BASEX)
> +		return xpcs_link_up_1000basex(xpcs, speed, duplex);
>  }
>  EXPORT_SYMBOL_GPL(xpcs_link_up);
>  
> +static void xpcs_an_restart(struct phylink_pcs *pcs)
> +{
> +	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
> +	int ret;
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1);
> +	ret |= BMCR_ANRESTART;
> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, ret);

If xpcs_read() returns an error, we try to write the error back to
the control register? Is that a good idea/

Thanks.
kernel test robot April 22, 2022, 5:35 p.m. UTC | #2 
Hi Ong,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Ong-Boon-Leong/pcs-xpcs-stmmac-add-1000BASE-X-AN-for-network-switch/20220422-154446
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9c8774e629a1950c24b44e3c8fb93d76fb644b49
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220423/202204230159.Tixm42oR-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 5bd87350a5ae429baf8f373cb226a57b62f87280)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/dc88c5b7c183eeaff9db0e88d7b0d1d7f73e830b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ong-Boon-Leong/pcs-xpcs-stmmac-add-1000BASE-X-AN-for-network-switch/20220422-154446
        git checkout dc88c5b7c183eeaff9db0e88d7b0d1d7f73e830b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/dsa/sja1105/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/dsa/sja1105/sja1105_main.c:2334:52: error: too few arguments to function call, expected 4, have 3
                   rc = xpcs_do_config(xpcs, priv->phy_mode[i], mode);
                        ~~~~~~~~~~~~~~                              ^
   include/linux/pcs/pcs-xpcs.h:33:5: note: 'xpcs_do_config' declared here
   int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
       ^
   1 error generated.


vim +2334 drivers/net/dsa/sja1105/sja1105_main.c

2eea1fa82f681b Vladimir Oltean 2019-11-12  2224  
6666cebc5e306f Vladimir Oltean 2019-05-02  2225  /* For situations where we need to change a setting at runtime that is only
6666cebc5e306f Vladimir Oltean 2019-05-02  2226   * available through the static configuration, resetting the switch in order
6666cebc5e306f Vladimir Oltean 2019-05-02  2227   * to upload the new static config is unavoidable. Back up the settings we
6666cebc5e306f Vladimir Oltean 2019-05-02  2228   * modify at runtime (currently only MAC) and restore them after uploading,
6666cebc5e306f Vladimir Oltean 2019-05-02  2229   * such that this operation is relatively seamless.
6666cebc5e306f Vladimir Oltean 2019-05-02  2230   */
2eea1fa82f681b Vladimir Oltean 2019-11-12  2231  int sja1105_static_config_reload(struct sja1105_private *priv,
2eea1fa82f681b Vladimir Oltean 2019-11-12  2232  				 enum sja1105_reset_reason reason)
6666cebc5e306f Vladimir Oltean 2019-05-02  2233  {
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2234  	struct ptp_system_timestamp ptp_sts_before;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2235  	struct ptp_system_timestamp ptp_sts_after;
82760d7f2ea638 Vladimir Oltean 2021-05-24  2236  	int speed_mbps[SJA1105_MAX_NUM_PORTS];
84db00f2c04338 Vladimir Oltean 2021-05-31  2237  	u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0};
6666cebc5e306f Vladimir Oltean 2019-05-02  2238  	struct sja1105_mac_config_entry *mac;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2239  	struct dsa_switch *ds = priv->ds;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2240  	s64 t1, t2, t3, t4;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2241  	s64 t12, t34;
6666cebc5e306f Vladimir Oltean 2019-05-02  2242  	int rc, i;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2243  	s64 now;
6666cebc5e306f Vladimir Oltean 2019-05-02  2244  
af580ae2dcb250 Vladimir Oltean 2019-11-09  2245  	mutex_lock(&priv->mgmt_lock);
af580ae2dcb250 Vladimir Oltean 2019-11-09  2246  
6666cebc5e306f Vladimir Oltean 2019-05-02  2247  	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
6666cebc5e306f Vladimir Oltean 2019-05-02  2248  
8400cff60b472c Vladimir Oltean 2019-06-08  2249  	/* Back up the dynamic link speed changed by sja1105_adjust_port_config
8400cff60b472c Vladimir Oltean 2019-06-08  2250  	 * in order to temporarily restore it to SJA1105_SPEED_AUTO - which the
8400cff60b472c Vladimir Oltean 2019-06-08  2251  	 * switch wants to see in the static config in order to allow us to
8400cff60b472c Vladimir Oltean 2019-06-08  2252  	 * change it through the dynamic interface later.
6666cebc5e306f Vladimir Oltean 2019-05-02  2253  	 */
542043e91df452 Vladimir Oltean 2021-05-24  2254  	for (i = 0; i < ds->num_ports; i++) {
3ad1d171548e85 Vladimir Oltean 2021-06-11  2255  		u32 reg_addr = mdiobus_c45_addr(MDIO_MMD_VEND2, MDIO_CTRL1);
3ad1d171548e85 Vladimir Oltean 2021-06-11  2256  
41fed17fdbe531 Vladimir Oltean 2021-05-31  2257  		speed_mbps[i] = sja1105_port_speed_to_ethtool(priv,
41fed17fdbe531 Vladimir Oltean 2021-05-31  2258  							      mac[i].speed);
41fed17fdbe531 Vladimir Oltean 2021-05-31  2259  		mac[i].speed = priv->info->port_speed[SJA1105_SPEED_AUTO];
6666cebc5e306f Vladimir Oltean 2019-05-02  2260  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2261  		if (priv->xpcs[i])
3ad1d171548e85 Vladimir Oltean 2021-06-11  2262  			bmcr[i] = mdiobus_read(priv->mdio_pcs, i, reg_addr);
84db00f2c04338 Vladimir Oltean 2021-05-31  2263  	}
ffe10e679cec9a Vladimir Oltean 2020-03-20  2264  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2265  	/* No PTP operations can run right now */
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2266  	mutex_lock(&priv->ptp_data.lock);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2267  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2268  	rc = __sja1105_ptp_gettimex(ds, &now, &ptp_sts_before);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2269  	if (rc < 0) {
61c77533b82ba8 Vladimir Oltean 2021-06-18  2270  		mutex_unlock(&priv->ptp_data.lock);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2271  		goto out;
61c77533b82ba8 Vladimir Oltean 2021-06-18  2272  	}
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2273  
6666cebc5e306f Vladimir Oltean 2019-05-02  2274  	/* Reset switch and send updated static configuration */
6666cebc5e306f Vladimir Oltean 2019-05-02  2275  	rc = sja1105_static_config_upload(priv);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2276  	if (rc < 0) {
61c77533b82ba8 Vladimir Oltean 2021-06-18  2277  		mutex_unlock(&priv->ptp_data.lock);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2278  		goto out;
61c77533b82ba8 Vladimir Oltean 2021-06-18  2279  	}
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2280  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2281  	rc = __sja1105_ptp_settime(ds, 0, &ptp_sts_after);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2282  	if (rc < 0) {
61c77533b82ba8 Vladimir Oltean 2021-06-18  2283  		mutex_unlock(&priv->ptp_data.lock);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2284  		goto out;
61c77533b82ba8 Vladimir Oltean 2021-06-18  2285  	}
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2286  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2287  	t1 = timespec64_to_ns(&ptp_sts_before.pre_ts);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2288  	t2 = timespec64_to_ns(&ptp_sts_before.post_ts);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2289  	t3 = timespec64_to_ns(&ptp_sts_after.pre_ts);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2290  	t4 = timespec64_to_ns(&ptp_sts_after.post_ts);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2291  	/* Mid point, corresponds to pre-reset PTPCLKVAL */
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2292  	t12 = t1 + (t2 - t1) / 2;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2293  	/* Mid point, corresponds to post-reset PTPCLKVAL, aka 0 */
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2294  	t34 = t3 + (t4 - t3) / 2;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2295  	/* Advance PTPCLKVAL by the time it took since its readout */
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2296  	now += (t34 - t12);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2297  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2298  	__sja1105_ptp_adjtime(ds, now);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2299  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2300  	mutex_unlock(&priv->ptp_data.lock);
6666cebc5e306f Vladimir Oltean 2019-05-02  2301  
2eea1fa82f681b Vladimir Oltean 2019-11-12  2302  	dev_info(priv->ds->dev,
2eea1fa82f681b Vladimir Oltean 2019-11-12  2303  		 "Reset switch and programmed static config. Reason: %s\n",
2eea1fa82f681b Vladimir Oltean 2019-11-12  2304  		 sja1105_reset_reasons[reason]);
2eea1fa82f681b Vladimir Oltean 2019-11-12  2305  
6666cebc5e306f Vladimir Oltean 2019-05-02  2306  	/* Configure the CGU (PLLs) for MII and RMII PHYs.
6666cebc5e306f Vladimir Oltean 2019-05-02  2307  	 * For these interfaces there is no dynamic configuration
6666cebc5e306f Vladimir Oltean 2019-05-02  2308  	 * needed, since PLLs have same settings at all speeds.
6666cebc5e306f Vladimir Oltean 2019-05-02  2309  	 */
cb5a82d2b9aaca Vladimir Oltean 2021-06-18  2310  	if (priv->info->clocking_setup) {
c50376783f23ff Vladimir Oltean 2021-05-24  2311  		rc = priv->info->clocking_setup(priv);
6666cebc5e306f Vladimir Oltean 2019-05-02  2312  		if (rc < 0)
6666cebc5e306f Vladimir Oltean 2019-05-02  2313  			goto out;
cb5a82d2b9aaca Vladimir Oltean 2021-06-18  2314  	}
6666cebc5e306f Vladimir Oltean 2019-05-02  2315  
542043e91df452 Vladimir Oltean 2021-05-24  2316  	for (i = 0; i < ds->num_ports; i++) {
3ad1d171548e85 Vladimir Oltean 2021-06-11  2317  		struct dw_xpcs *xpcs = priv->xpcs[i];
3ad1d171548e85 Vladimir Oltean 2021-06-11  2318  		unsigned int mode;
84db00f2c04338 Vladimir Oltean 2021-05-31  2319  
8400cff60b472c Vladimir Oltean 2019-06-08  2320  		rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);
6666cebc5e306f Vladimir Oltean 2019-05-02  2321  		if (rc < 0)
6666cebc5e306f Vladimir Oltean 2019-05-02  2322  			goto out;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2323  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2324  		if (!xpcs)
84db00f2c04338 Vladimir Oltean 2021-05-31  2325  			continue;
84db00f2c04338 Vladimir Oltean 2021-05-31  2326  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2327  		if (bmcr[i] & BMCR_ANENABLE)
3ad1d171548e85 Vladimir Oltean 2021-06-11  2328  			mode = MLO_AN_INBAND;
3ad1d171548e85 Vladimir Oltean 2021-06-11  2329  		else if (priv->fixed_link[i])
3ad1d171548e85 Vladimir Oltean 2021-06-11  2330  			mode = MLO_AN_FIXED;
3ad1d171548e85 Vladimir Oltean 2021-06-11  2331  		else
3ad1d171548e85 Vladimir Oltean 2021-06-11  2332  			mode = MLO_AN_PHY;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2333  
3ad1d171548e85 Vladimir Oltean 2021-06-11 @2334  		rc = xpcs_do_config(xpcs, priv->phy_mode[i], mode);
3ad1d171548e85 Vladimir Oltean 2021-06-11  2335  		if (rc < 0)
3ad1d171548e85 Vladimir Oltean 2021-06-11  2336  			goto out;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2337  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2338  		if (!phylink_autoneg_inband(mode)) {
ffe10e679cec9a Vladimir Oltean 2020-03-20  2339  			int speed = SPEED_UNKNOWN;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2340  
56b63466333b25 Vladimir Oltean 2021-06-11  2341  			if (priv->phy_mode[i] == PHY_INTERFACE_MODE_2500BASEX)
56b63466333b25 Vladimir Oltean 2021-06-11  2342  				speed = SPEED_2500;
56b63466333b25 Vladimir Oltean 2021-06-11  2343  			else if (bmcr[i] & BMCR_SPEED1000)
ffe10e679cec9a Vladimir Oltean 2020-03-20  2344  				speed = SPEED_1000;
84db00f2c04338 Vladimir Oltean 2021-05-31  2345  			else if (bmcr[i] & BMCR_SPEED100)
ffe10e679cec9a Vladimir Oltean 2020-03-20  2346  				speed = SPEED_100;
053d8ad10d585a Vladimir Oltean 2021-03-04  2347  			else
ffe10e679cec9a Vladimir Oltean 2020-03-20  2348  				speed = SPEED_10;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2349  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2350  			xpcs_link_up(&xpcs->pcs, mode, priv->phy_mode[i],
3ad1d171548e85 Vladimir Oltean 2021-06-11  2351  				     speed, DUPLEX_FULL);
ffe10e679cec9a Vladimir Oltean 2020-03-20  2352  		}
ffe10e679cec9a Vladimir Oltean 2020-03-20  2353  	}
4d7525085a9ba8 Vladimir Oltean 2020-05-28  2354  
4d7525085a9ba8 Vladimir Oltean 2020-05-28  2355  	rc = sja1105_reload_cbs(priv);
4d7525085a9ba8 Vladimir Oltean 2020-05-28  2356  	if (rc < 0)
4d7525085a9ba8 Vladimir Oltean 2020-05-28  2357  		goto out;
6666cebc5e306f Vladimir Oltean 2019-05-02  2358  out:
af580ae2dcb250 Vladimir Oltean 2019-11-09  2359  	mutex_unlock(&priv->mgmt_lock);
af580ae2dcb250 Vladimir Oltean 2019-11-09  2360  
6666cebc5e306f Vladimir Oltean 2019-05-02  2361  	return rc;
6666cebc5e306f Vladimir Oltean 2019-05-02  2362  }
6666cebc5e306f Vladimir Oltean 2019-05-02  2363
kernel test robot April 23, 2022, 3 a.m. UTC | #3 
Hi Ong,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Ong-Boon-Leong/pcs-xpcs-stmmac-add-1000BASE-X-AN-for-network-switch/20220422-154446
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 9c8774e629a1950c24b44e3c8fb93d76fb644b49
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220423/202204231033.DjMxbbXU-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/dc88c5b7c183eeaff9db0e88d7b0d1d7f73e830b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ong-Boon-Leong/pcs-xpcs-stmmac-add-1000BASE-X-AN-for-network-switch/20220422-154446
        git checkout dc88c5b7c183eeaff9db0e88d7b0d1d7f73e830b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/dsa/sja1105/sja1105_main.c: In function 'sja1105_static_config_reload':
>> drivers/net/dsa/sja1105/sja1105_main.c:2334:22: error: too few arguments to function 'xpcs_do_config'
    2334 |                 rc = xpcs_do_config(xpcs, priv->phy_mode[i], mode);
         |                      ^~~~~~~~~~~~~~
   In file included from drivers/net/dsa/sja1105/sja1105_main.c:19:
   include/linux/pcs/pcs-xpcs.h:33:5: note: declared here
      33 | int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
         |     ^~~~~~~~~~~~~~


vim +/xpcs_do_config +2334 drivers/net/dsa/sja1105/sja1105_main.c

2eea1fa82f681b Vladimir Oltean 2019-11-12  2224  
6666cebc5e306f Vladimir Oltean 2019-05-02  2225  /* For situations where we need to change a setting at runtime that is only
6666cebc5e306f Vladimir Oltean 2019-05-02  2226   * available through the static configuration, resetting the switch in order
6666cebc5e306f Vladimir Oltean 2019-05-02  2227   * to upload the new static config is unavoidable. Back up the settings we
6666cebc5e306f Vladimir Oltean 2019-05-02  2228   * modify at runtime (currently only MAC) and restore them after uploading,
6666cebc5e306f Vladimir Oltean 2019-05-02  2229   * such that this operation is relatively seamless.
6666cebc5e306f Vladimir Oltean 2019-05-02  2230   */
2eea1fa82f681b Vladimir Oltean 2019-11-12  2231  int sja1105_static_config_reload(struct sja1105_private *priv,
2eea1fa82f681b Vladimir Oltean 2019-11-12  2232  				 enum sja1105_reset_reason reason)
6666cebc5e306f Vladimir Oltean 2019-05-02  2233  {
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2234  	struct ptp_system_timestamp ptp_sts_before;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2235  	struct ptp_system_timestamp ptp_sts_after;
82760d7f2ea638 Vladimir Oltean 2021-05-24  2236  	int speed_mbps[SJA1105_MAX_NUM_PORTS];
84db00f2c04338 Vladimir Oltean 2021-05-31  2237  	u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0};
6666cebc5e306f Vladimir Oltean 2019-05-02  2238  	struct sja1105_mac_config_entry *mac;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2239  	struct dsa_switch *ds = priv->ds;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2240  	s64 t1, t2, t3, t4;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2241  	s64 t12, t34;
6666cebc5e306f Vladimir Oltean 2019-05-02  2242  	int rc, i;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2243  	s64 now;
6666cebc5e306f Vladimir Oltean 2019-05-02  2244  
af580ae2dcb250 Vladimir Oltean 2019-11-09  2245  	mutex_lock(&priv->mgmt_lock);
af580ae2dcb250 Vladimir Oltean 2019-11-09  2246  
6666cebc5e306f Vladimir Oltean 2019-05-02  2247  	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
6666cebc5e306f Vladimir Oltean 2019-05-02  2248  
8400cff60b472c Vladimir Oltean 2019-06-08  2249  	/* Back up the dynamic link speed changed by sja1105_adjust_port_config
8400cff60b472c Vladimir Oltean 2019-06-08  2250  	 * in order to temporarily restore it to SJA1105_SPEED_AUTO - which the
8400cff60b472c Vladimir Oltean 2019-06-08  2251  	 * switch wants to see in the static config in order to allow us to
8400cff60b472c Vladimir Oltean 2019-06-08  2252  	 * change it through the dynamic interface later.
6666cebc5e306f Vladimir Oltean 2019-05-02  2253  	 */
542043e91df452 Vladimir Oltean 2021-05-24  2254  	for (i = 0; i < ds->num_ports; i++) {
3ad1d171548e85 Vladimir Oltean 2021-06-11  2255  		u32 reg_addr = mdiobus_c45_addr(MDIO_MMD_VEND2, MDIO_CTRL1);
3ad1d171548e85 Vladimir Oltean 2021-06-11  2256  
41fed17fdbe531 Vladimir Oltean 2021-05-31  2257  		speed_mbps[i] = sja1105_port_speed_to_ethtool(priv,
41fed17fdbe531 Vladimir Oltean 2021-05-31  2258  							      mac[i].speed);
41fed17fdbe531 Vladimir Oltean 2021-05-31  2259  		mac[i].speed = priv->info->port_speed[SJA1105_SPEED_AUTO];
6666cebc5e306f Vladimir Oltean 2019-05-02  2260  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2261  		if (priv->xpcs[i])
3ad1d171548e85 Vladimir Oltean 2021-06-11  2262  			bmcr[i] = mdiobus_read(priv->mdio_pcs, i, reg_addr);
84db00f2c04338 Vladimir Oltean 2021-05-31  2263  	}
ffe10e679cec9a Vladimir Oltean 2020-03-20  2264  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2265  	/* No PTP operations can run right now */
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2266  	mutex_lock(&priv->ptp_data.lock);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2267  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2268  	rc = __sja1105_ptp_gettimex(ds, &now, &ptp_sts_before);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2269  	if (rc < 0) {
61c77533b82ba8 Vladimir Oltean 2021-06-18  2270  		mutex_unlock(&priv->ptp_data.lock);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2271  		goto out;
61c77533b82ba8 Vladimir Oltean 2021-06-18  2272  	}
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2273  
6666cebc5e306f Vladimir Oltean 2019-05-02  2274  	/* Reset switch and send updated static configuration */
6666cebc5e306f Vladimir Oltean 2019-05-02  2275  	rc = sja1105_static_config_upload(priv);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2276  	if (rc < 0) {
61c77533b82ba8 Vladimir Oltean 2021-06-18  2277  		mutex_unlock(&priv->ptp_data.lock);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2278  		goto out;
61c77533b82ba8 Vladimir Oltean 2021-06-18  2279  	}
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2280  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2281  	rc = __sja1105_ptp_settime(ds, 0, &ptp_sts_after);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2282  	if (rc < 0) {
61c77533b82ba8 Vladimir Oltean 2021-06-18  2283  		mutex_unlock(&priv->ptp_data.lock);
61c77533b82ba8 Vladimir Oltean 2021-06-18  2284  		goto out;
61c77533b82ba8 Vladimir Oltean 2021-06-18  2285  	}
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2286  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2287  	t1 = timespec64_to_ns(&ptp_sts_before.pre_ts);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2288  	t2 = timespec64_to_ns(&ptp_sts_before.post_ts);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2289  	t3 = timespec64_to_ns(&ptp_sts_after.pre_ts);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2290  	t4 = timespec64_to_ns(&ptp_sts_after.post_ts);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2291  	/* Mid point, corresponds to pre-reset PTPCLKVAL */
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2292  	t12 = t1 + (t2 - t1) / 2;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2293  	/* Mid point, corresponds to post-reset PTPCLKVAL, aka 0 */
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2294  	t34 = t3 + (t4 - t3) / 2;
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2295  	/* Advance PTPCLKVAL by the time it took since its readout */
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2296  	now += (t34 - t12);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2297  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2298  	__sja1105_ptp_adjtime(ds, now);
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2299  
6cf99c13ea07b5 Vladimir Oltean 2019-11-09  2300  	mutex_unlock(&priv->ptp_data.lock);
6666cebc5e306f Vladimir Oltean 2019-05-02  2301  
2eea1fa82f681b Vladimir Oltean 2019-11-12  2302  	dev_info(priv->ds->dev,
2eea1fa82f681b Vladimir Oltean 2019-11-12  2303  		 "Reset switch and programmed static config. Reason: %s\n",
2eea1fa82f681b Vladimir Oltean 2019-11-12  2304  		 sja1105_reset_reasons[reason]);
2eea1fa82f681b Vladimir Oltean 2019-11-12  2305  
6666cebc5e306f Vladimir Oltean 2019-05-02  2306  	/* Configure the CGU (PLLs) for MII and RMII PHYs.
6666cebc5e306f Vladimir Oltean 2019-05-02  2307  	 * For these interfaces there is no dynamic configuration
6666cebc5e306f Vladimir Oltean 2019-05-02  2308  	 * needed, since PLLs have same settings at all speeds.
6666cebc5e306f Vladimir Oltean 2019-05-02  2309  	 */
cb5a82d2b9aaca Vladimir Oltean 2021-06-18  2310  	if (priv->info->clocking_setup) {
c50376783f23ff Vladimir Oltean 2021-05-24  2311  		rc = priv->info->clocking_setup(priv);
6666cebc5e306f Vladimir Oltean 2019-05-02  2312  		if (rc < 0)
6666cebc5e306f Vladimir Oltean 2019-05-02  2313  			goto out;
cb5a82d2b9aaca Vladimir Oltean 2021-06-18  2314  	}
6666cebc5e306f Vladimir Oltean 2019-05-02  2315  
542043e91df452 Vladimir Oltean 2021-05-24  2316  	for (i = 0; i < ds->num_ports; i++) {
3ad1d171548e85 Vladimir Oltean 2021-06-11  2317  		struct dw_xpcs *xpcs = priv->xpcs[i];
3ad1d171548e85 Vladimir Oltean 2021-06-11  2318  		unsigned int mode;
84db00f2c04338 Vladimir Oltean 2021-05-31  2319  
8400cff60b472c Vladimir Oltean 2019-06-08  2320  		rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]);
6666cebc5e306f Vladimir Oltean 2019-05-02  2321  		if (rc < 0)
6666cebc5e306f Vladimir Oltean 2019-05-02  2322  			goto out;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2323  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2324  		if (!xpcs)
84db00f2c04338 Vladimir Oltean 2021-05-31  2325  			continue;
84db00f2c04338 Vladimir Oltean 2021-05-31  2326  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2327  		if (bmcr[i] & BMCR_ANENABLE)
3ad1d171548e85 Vladimir Oltean 2021-06-11  2328  			mode = MLO_AN_INBAND;
3ad1d171548e85 Vladimir Oltean 2021-06-11  2329  		else if (priv->fixed_link[i])
3ad1d171548e85 Vladimir Oltean 2021-06-11  2330  			mode = MLO_AN_FIXED;
3ad1d171548e85 Vladimir Oltean 2021-06-11  2331  		else
3ad1d171548e85 Vladimir Oltean 2021-06-11  2332  			mode = MLO_AN_PHY;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2333  
3ad1d171548e85 Vladimir Oltean 2021-06-11 @2334  		rc = xpcs_do_config(xpcs, priv->phy_mode[i], mode);
3ad1d171548e85 Vladimir Oltean 2021-06-11  2335  		if (rc < 0)
3ad1d171548e85 Vladimir Oltean 2021-06-11  2336  			goto out;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2337  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2338  		if (!phylink_autoneg_inband(mode)) {
ffe10e679cec9a Vladimir Oltean 2020-03-20  2339  			int speed = SPEED_UNKNOWN;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2340  
56b63466333b25 Vladimir Oltean 2021-06-11  2341  			if (priv->phy_mode[i] == PHY_INTERFACE_MODE_2500BASEX)
56b63466333b25 Vladimir Oltean 2021-06-11  2342  				speed = SPEED_2500;
56b63466333b25 Vladimir Oltean 2021-06-11  2343  			else if (bmcr[i] & BMCR_SPEED1000)
ffe10e679cec9a Vladimir Oltean 2020-03-20  2344  				speed = SPEED_1000;
84db00f2c04338 Vladimir Oltean 2021-05-31  2345  			else if (bmcr[i] & BMCR_SPEED100)
ffe10e679cec9a Vladimir Oltean 2020-03-20  2346  				speed = SPEED_100;
053d8ad10d585a Vladimir Oltean 2021-03-04  2347  			else
ffe10e679cec9a Vladimir Oltean 2020-03-20  2348  				speed = SPEED_10;
ffe10e679cec9a Vladimir Oltean 2020-03-20  2349  
3ad1d171548e85 Vladimir Oltean 2021-06-11  2350  			xpcs_link_up(&xpcs->pcs, mode, priv->phy_mode[i],
3ad1d171548e85 Vladimir Oltean 2021-06-11  2351  				     speed, DUPLEX_FULL);
ffe10e679cec9a Vladimir Oltean 2020-03-20  2352  		}
ffe10e679cec9a Vladimir Oltean 2020-03-20  2353  	}
4d7525085a9ba8 Vladimir Oltean 2020-05-28  2354  
4d7525085a9ba8 Vladimir Oltean 2020-05-28  2355  	rc = sja1105_reload_cbs(priv);
4d7525085a9ba8 Vladimir Oltean 2020-05-28  2356  	if (rc < 0)
4d7525085a9ba8 Vladimir Oltean 2020-05-28  2357  		goto out;
6666cebc5e306f Vladimir Oltean 2019-05-02  2358  out:
af580ae2dcb250 Vladimir Oltean 2019-11-09  2359  	mutex_unlock(&priv->mgmt_lock);
af580ae2dcb250 Vladimir Oltean 2019-11-09  2360  
6666cebc5e306f Vladimir Oltean 2019-05-02  2361  	return rc;
6666cebc5e306f Vladimir Oltean 2019-05-02  2362  }
6666cebc5e306f Vladimir Oltean 2019-05-02  2363
Ong Boon Leong April 25, 2022, 3:30 a.m. UTC | #4 
>On Fri, Apr 22, 2022 at 03:35:02PM +0800, Ong Boon Leong wrote:
>> @@ -774,6 +788,58 @@ static int xpcs_config_aneg_c37_sgmii(struct
>dw_xpcs *xpcs, unsigned int mode)
>>  	return ret;
>>  }
>>
>> +static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs, unsigned
>int mode,
>> +					  const unsigned long *advertising)
>> +{
>> +	int ret, mdio_ctrl;
>> +
>> +	/* For AN for 1000BASE-X mode, the settings are :-
>> +	 * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable C37 AN in
>case
>> +	 *    it is already enabled)
>> +	 * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 00b (1000BASE-X C37)
>> +	 * 3) SR_MII_AN_ADV Bit(6)[FD] = 1b (Full Duplex)
>> +	 *    Note: Half Duplex is rarely used, so don't advertise.
>> +	 * 4) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable C37 AN)
>
>So if this function gets called to update the advertisement - even if
>there is no actual change - we go through a AN-disable..AN-enable
>dance and cause the link to re-negotiate. That doesn't sound like nice
>behaviour.
Good feedback. Will look into this part. 

>
>> +	 */
>> +	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2,
>DW_VR_MII_MMD_CTRL);
>> +	if (mdio_ctrl < 0)
>> +		return mdio_ctrl;
>> +
>> +	if (mdio_ctrl & AN_CL37_EN) {
>> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2,
>DW_VR_MII_MMD_CTRL,
>> +				 mdio_ctrl & ~AN_CL37_EN);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret &= ~DW_VR_MII_PCS_MODE_MASK;
>> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL,
>ret);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
>> +	ret |= ADVERTISE_1000XFULL;
>> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE, ret);
>
>What if other bits are already set in the MII_ADVERTISE register?
>Maybe consider using phylink_mii_c22_pcs_encode_advertisement()?

The IP supports C45 MII MMD access and not C22. Your feedback does
make sense to strengthen the logics here. Thanks 

>
>The pcs_config() method is also supposed to return either a negative
>error, 0 for no advertisement change, or positive for an advertisement
>change, in which case phylink will trigger a call to pcs_an_restart().
>
>> +static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
>> +					struct phylink_link_state *state)
>> +{
>> +	int lpa, adv;
>> +	int ret;
>> +
>> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret & AN_CL37_EN) {
>> +		/* Reset link_state */
>> +		state->link = false;
>> +		state->speed = SPEED_UNKNOWN;
>> +		state->duplex = DUPLEX_UNKNOWN;
>> +		state->pause = 0;
>
>Phylink guarantees that speed, duplex and pause are set to something
>sensible - please remove these. The only one you probably need here
>is state->link.

Thanks for the input here. 

>
>> +
>> +		lpa = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_LPA);
>> +		if (lpa < 0 || lpa & LPA_RFAULT)
>> +			return false;
>
>This function does not return a boolean. Returning "false" is the same
>as returning 0, which means "no error" but an error has occurred.
Good catch. 

>
>> +
>> +		adv = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
>> +		if (adv < 0)
>> +			return false;
>
>Ditto.
>
>> +
>> +		if (lpa & ADVERTISE_1000XFULL &&
>> +		    adv & ADVERTISE_1000XFULL) {
>> +			state->speed = SPEED_1000;
>> +			state->duplex = DUPLEX_FULL;
>> +			state->link = true;
>> +		}
>> +
>> +		/* Clear CL37 AN complete status */
>> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2,
>DW_VR_MII_AN_INTR_STS, 0);
>> +	} else {
>> +		state->link = true;
>> +		state->speed = SPEED_1000;
>> +		state->duplex = DUPLEX_FULL;
>> +		state->pause = 0;
>
>If we're in AN-disabled mode, phylink will set state->speed and
>state->duplex according to the user's parameters, so there should be no
>need to do it here.
Thanks for the input. 

>
>> @@ -994,9 +1143,21 @@ void xpcs_link_up(struct phylink_pcs *pcs,
>unsigned int mode,
>>  		return xpcs_config_usxgmii(xpcs, speed);
>>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>>  		return xpcs_link_up_sgmii(xpcs, mode, speed, duplex);
>> +	if (interface == PHY_INTERFACE_MODE_1000BASEX)
>> +		return xpcs_link_up_1000basex(xpcs, speed, duplex);
>>  }
>>  EXPORT_SYMBOL_GPL(xpcs_link_up);
>>
>> +static void xpcs_an_restart(struct phylink_pcs *pcs)
>> +{
>> +	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
>> +	int ret;
>> +
>> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1);
>> +	ret |= BMCR_ANRESTART;
>> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, ret);
>
>If xpcs_read() returns an error, we try to write the error back to
>the control register? Is that a good idea/
Thanks! I will fix this.
diff mbox series

Patch

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 61418d4dc0c..7ba60944ba0 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -77,6 +77,14 @@  static const int xpcs_sgmii_features[] = {
 	__ETHTOOL_LINK_MODE_MASK_NBITS,
 };
 
+static const int xpcs_1000basex_features[] = {
+	ETHTOOL_LINK_MODE_Pause_BIT,
+	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+	ETHTOOL_LINK_MODE_Autoneg_BIT,
+	ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+	__ETHTOOL_LINK_MODE_MASK_NBITS,
+};
+
 static const int xpcs_2500basex_features[] = {
 	ETHTOOL_LINK_MODE_Pause_BIT,
 	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
@@ -102,6 +110,10 @@  static const phy_interface_t xpcs_sgmii_interfaces[] = {
 	PHY_INTERFACE_MODE_SGMII,
 };
 
+static const phy_interface_t xpcs_1000basex_interfaces[] = {
+	PHY_INTERFACE_MODE_1000BASEX,
+};
+
 static const phy_interface_t xpcs_2500basex_interfaces[] = {
 	PHY_INTERFACE_MODE_2500BASEX,
 	PHY_INTERFACE_MODE_MAX,
@@ -112,6 +124,7 @@  enum {
 	DW_XPCS_10GKR,
 	DW_XPCS_XLGMII,
 	DW_XPCS_SGMII,
+	DW_XPCS_1000BASEX,
 	DW_XPCS_2500BASEX,
 	DW_XPCS_INTERFACE_MAX,
 };
@@ -239,6 +252,7 @@  static int xpcs_soft_reset(struct dw_xpcs *xpcs,
 		break;
 	case DW_AN_C37_SGMII:
 	case DW_2500BASEX:
+	case DW_AN_C37_1000BASEX:
 		dev = MDIO_MMD_VEND2;
 		break;
 	default:
@@ -774,6 +788,58 @@  static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
 	return ret;
 }
 
+static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs, unsigned int mode,
+					  const unsigned long *advertising)
+{
+	int ret, mdio_ctrl;
+
+	/* For AN for 1000BASE-X mode, the settings are :-
+	 * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable C37 AN in case
+	 *    it is already enabled)
+	 * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 00b (1000BASE-X C37)
+	 * 3) SR_MII_AN_ADV Bit(6)[FD] = 1b (Full Duplex)
+	 *    Note: Half Duplex is rarely used, so don't advertise.
+	 * 4) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable C37 AN)
+	 */
+	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
+	if (mdio_ctrl < 0)
+		return mdio_ctrl;
+
+	if (mdio_ctrl & AN_CL37_EN) {
+		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
+				 mdio_ctrl & ~AN_CL37_EN);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL);
+	if (ret < 0)
+		return ret;
+
+	ret &= ~DW_VR_MII_PCS_MODE_MASK;
+	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, ret);
+	if (ret < 0)
+		return ret;
+
+	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
+	ret |= ADVERTISE_1000XFULL;
+	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE, ret);
+	if (ret < 0)
+		return ret;
+
+	/* Clear CL37 AN complete status */
+	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
+	if (ret < 0)
+		return ret;
+
+	if (phylink_autoneg_inband(mode) &&
+	    linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, advertising))
+		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
+				 mdio_ctrl | AN_CL37_EN);
+
+	return ret;
+}
+
 static int xpcs_config_2500basex(struct dw_xpcs *xpcs)
 {
 	int ret;
@@ -797,7 +863,7 @@  static int xpcs_config_2500basex(struct dw_xpcs *xpcs)
 }
 
 int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
-		   unsigned int mode)
+		   unsigned int mode, const unsigned long *advertising)
 {
 	const struct xpcs_compat *compat;
 	int ret;
@@ -819,6 +885,12 @@  int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 		if (ret)
 			return ret;
 		break;
+	case DW_AN_C37_1000BASEX:
+		ret = xpcs_config_aneg_c37_1000basex(xpcs, mode,
+						     advertising);
+		if (ret)
+			return ret;
+		break;
 	case DW_2500BASEX:
 		ret = xpcs_config_2500basex(xpcs);
 		if (ret)
@@ -845,7 +917,7 @@  static int xpcs_config(struct phylink_pcs *pcs, unsigned int mode,
 {
 	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
 
-	return xpcs_do_config(xpcs, interface, mode);
+	return xpcs_do_config(xpcs, interface, mode, advertising);
 }
 
 static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
@@ -866,7 +938,7 @@  static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
 
 		state->link = 0;
 
-		return xpcs_do_config(xpcs, state->interface, MLO_AN_INBAND);
+		return xpcs_do_config(xpcs, state->interface, MLO_AN_INBAND, NULL);
 	}
 
 	if (state->an_enabled && xpcs_aneg_done_c73(xpcs, state, compat)) {
@@ -923,6 +995,50 @@  static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
 	return 0;
 }
 
+static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
+					struct phylink_link_state *state)
+{
+	int lpa, adv;
+	int ret;
+
+	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
+	if (ret < 0)
+		return ret;
+
+	if (ret & AN_CL37_EN) {
+		/* Reset link_state */
+		state->link = false;
+		state->speed = SPEED_UNKNOWN;
+		state->duplex = DUPLEX_UNKNOWN;
+		state->pause = 0;
+
+		lpa = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_LPA);
+		if (lpa < 0 || lpa & LPA_RFAULT)
+			return false;
+
+		adv = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
+		if (adv < 0)
+			return false;
+
+		if (lpa & ADVERTISE_1000XFULL &&
+		    adv & ADVERTISE_1000XFULL) {
+			state->speed = SPEED_1000;
+			state->duplex = DUPLEX_FULL;
+			state->link = true;
+		}
+
+		/* Clear CL37 AN complete status */
+		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
+	} else {
+		state->link = true;
+		state->speed = SPEED_1000;
+		state->duplex = DUPLEX_FULL;
+		state->pause = 0;
+	}
+
+	return 0;
+}
+
 static void xpcs_get_state(struct phylink_pcs *pcs,
 			   struct phylink_link_state *state)
 {
@@ -950,6 +1066,13 @@  static void xpcs_get_state(struct phylink_pcs *pcs,
 			       ERR_PTR(ret));
 		}
 		break;
+	case DW_AN_C37_1000BASEX:
+		ret = xpcs_get_state_c37_1000basex(xpcs, state);
+		if (ret) {
+			pr_err("xpcs_get_state_c37_1000basex returned %pe\n",
+			       ERR_PTR(ret));
+		}
+		break;
 	default:
 		return;
 	}
@@ -985,6 +1108,32 @@  static void xpcs_link_up_sgmii(struct dw_xpcs *xpcs, unsigned int mode,
 		pr_err("%s: xpcs_write returned %pe\n", __func__, ERR_PTR(ret));
 }
 
+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));
+}
+
 void xpcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 		  phy_interface_t interface, int speed, int duplex)
 {
@@ -994,9 +1143,21 @@  void xpcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 		return xpcs_config_usxgmii(xpcs, speed);
 	if (interface == PHY_INTERFACE_MODE_SGMII)
 		return xpcs_link_up_sgmii(xpcs, mode, speed, duplex);
+	if (interface == PHY_INTERFACE_MODE_1000BASEX)
+		return xpcs_link_up_1000basex(xpcs, speed, duplex);
 }
 EXPORT_SYMBOL_GPL(xpcs_link_up);
 
+static void xpcs_an_restart(struct phylink_pcs *pcs)
+{
+	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
+	int ret;
+
+	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1);
+	ret |= BMCR_ANRESTART;
+	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, ret);
+}
+
 static u32 xpcs_get_id(struct dw_xpcs *xpcs)
 {
 	int ret;
@@ -1062,6 +1223,12 @@  static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = {
 		.num_interfaces = ARRAY_SIZE(xpcs_sgmii_interfaces),
 		.an_mode = DW_AN_C37_SGMII,
 	},
+	[DW_XPCS_1000BASEX] = {
+		.supported = xpcs_1000basex_features,
+		.interface = xpcs_1000basex_interfaces,
+		.num_interfaces = ARRAY_SIZE(xpcs_1000basex_interfaces),
+		.an_mode = DW_AN_C37_1000BASEX,
+	},
 	[DW_XPCS_2500BASEX] = {
 		.supported = xpcs_2500basex_features,
 		.interface = xpcs_2500basex_interfaces,
@@ -1117,6 +1284,7 @@  static const struct phylink_pcs_ops xpcs_phylink_ops = {
 	.pcs_validate = xpcs_validate,
 	.pcs_config = xpcs_config,
 	.pcs_get_state = xpcs_get_state,
+	.pcs_an_restart = xpcs_an_restart,
 	.pcs_link_up = xpcs_link_up,
 };
 
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 266eb26fb02..d2da1e0b4a9 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -17,6 +17,7 @@ 
 #define DW_AN_C73			1
 #define DW_AN_C37_SGMII			2
 #define DW_2500BASEX			3
+#define DW_AN_C37_1000BASEX		4
 
 struct xpcs_id;
 
@@ -30,7 +31,7 @@  int xpcs_get_an_mode(struct dw_xpcs *xpcs, phy_interface_t interface);
 void xpcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
 		  phy_interface_t interface, int speed, int duplex);
 int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
-		   unsigned int mode);
+		   unsigned int mode, const unsigned long *advertising);
 void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces);
 int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
 		    int enable);