Re: [PATCH] net: phy: mscc: Add auto-negotiation feature to VSC8514

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

On Thu, Feb 13, 2025 at 03:51:50PM +0530, Chintan Vankar wrote:
> Add function vsc85xx_config_inband_aneg() in mscc_main.c to enable
> auto-negotiation. The function enables auto-negotiation by configuring
> the MAC SerDes PCS Control register of VSC8514.
> 
> Invoke the vsc85xx_config_inband_aneg() function from the
> vsc8514_config_init() function present in mscc_main.c to start the
> auto-negotiation process. This is required to get Ethernet working with
> the Quad port Ethernet Add-On card connected to J7 common processor board.

A few points:

1. please read the netdev FAQ:
   https://www.kernel.org/doc/html/v5.6/networking/netdev-FAQ.html
   specifically the first question. Please also note the delay
   requirement for resending patches.

2. Is this a fix? Does something not work at the moment?

3. Will always forcing the inband signalling to be enabled result in
   another existing user that doesn't provide the inband signalling
   now failing? Do you know for sure that this won't disrupt any other
   users of this PHY?

4. Maybe consider using phylink in the ethernet driver and populating
   the .inband_caps() and .config_inband() methods which will allow
   the inband signalling properties to be negotiated between the MAC's
   PCS and the PHY.

Other points inline below:

> +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
> +{
> +	u16 reg_val = 0;
> +	int rc;
> +
> +	if (enabled)
> +		reg_val = MSCC_PHY_SERDES_ANEG;
> +
> +	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
> +			      MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
> +			      reg_val);
> +
> +	return rc;

Why not simply:

	return phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
				MSCC_PHY_SERDES_PCS_CTRL,
				MSCC_PHY_SERDES_ANEG,
				reg_val);

?

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