Re: [PATCH net-next v4 6/8] net: pcs: Add 10GBASE-R mode for Synopsys Designware XPCS

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

On Sat, Apr 22, 2023 at 12:56:19PM +0800, Jiawen Wu wrote:
> Add basic support for XPCS using 10GBASE-R interface. This mode will
> be extended to use interrupt, so set pcs.poll false. And avoid soft
> reset so that the device using this mode is in the default configuration.
> 
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  drivers/net/pcs/pcs-xpcs.c   | 56 ++++++++++++++++++++++++++++++++++++
>  include/linux/pcs/pcs-xpcs.h |  1 +
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 539cd43eae8d..9ddaceda1fe9 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -64,6 +64,16 @@ static const int xpcs_xlgmii_features[] = {
>  	__ETHTOOL_LINK_MODE_MASK_NBITS,
>  };
>  
> +static const int xpcs_10gbaser_features[] = {
> +	ETHTOOL_LINK_MODE_Pause_BIT,
> +	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +	ETHTOOL_LINK_MODE_10000baseSR_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseLR_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT,
> +	ETHTOOL_LINK_MODE_10000baseER_Full_BIT,
> +	__ETHTOOL_LINK_MODE_MASK_NBITS,
> +};
> +
>  static const int xpcs_sgmii_features[] = {
>  	ETHTOOL_LINK_MODE_Pause_BIT,
>  	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> @@ -106,6 +116,10 @@ static const phy_interface_t xpcs_xlgmii_interfaces[] = {
>  	PHY_INTERFACE_MODE_XLGMII,
>  };
>  
> +static const phy_interface_t xpcs_10gbaser_interfaces[] = {
> +	PHY_INTERFACE_MODE_10GBASER,
> +};
> +
>  static const phy_interface_t xpcs_sgmii_interfaces[] = {
>  	PHY_INTERFACE_MODE_SGMII,
>  };
> @@ -123,6 +137,7 @@ enum {
>  	DW_XPCS_USXGMII,
>  	DW_XPCS_10GKR,
>  	DW_XPCS_XLGMII,
> +	DW_XPCS_10GBASER,
>  	DW_XPCS_SGMII,
>  	DW_XPCS_1000BASEX,
>  	DW_XPCS_2500BASEX,
> @@ -246,6 +261,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs,
>  
>  	switch (compat->an_mode) {
>  	case DW_AN_C73:
> +	case DW_10GBASER:
>  		dev = MDIO_MMD_PCS;
>  		break;
>  	case DW_AN_C37_SGMII:
> @@ -872,6 +888,8 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
>  		return -ENODEV;
>  
>  	switch (compat->an_mode) {
> +	case DW_10GBASER:
> +		break;
>  	case DW_AN_C73:
>  		if (phylink_autoneg_inband(mode)) {
>  			ret = xpcs_config_aneg_c73(xpcs, compat);
> @@ -919,6 +937,27 @@ static int xpcs_config(struct phylink_pcs *pcs, unsigned int mode,
>  	return xpcs_do_config(xpcs, interface, mode, advertising);
>  }
>  
> +static int xpcs_get_state_10gbaser(struct dw_xpcs *xpcs,
> +				   struct phylink_link_state *state)
> +{
> +	int ret;
> +
> +	state->link = false;
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & MDIO_STAT1_LSTATUS) {
> +		state->link = true;
> +		state->pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
> +		state->duplex = DUPLEX_FULL;
> +		state->speed = SPEED_10000;
> +	}
> +
> +	return 0;

This looks to me to be an almost duplicate of
phylink_mii_c45_pcs_get_state().

> +}
> +
>  static int xpcs_get_state_c73(struct dw_xpcs *xpcs,
>  			      struct phylink_link_state *state,
>  			      const struct xpcs_compat *compat)
> @@ -1033,6 +1072,14 @@ static void xpcs_get_state(struct phylink_pcs *pcs,
>  		return;
>  
>  	switch (compat->an_mode) {
> +	case DW_10GBASER:
> +		ret = xpcs_get_state_10gbaser(xpcs, state);

This could then be simply:

		phylink_mii_c45_pcs_get_state(xpcs->mdiodev, state);

> +		if (ret) {
> +			pr_err("xpcs_get_state_10gbaser returned %pe\n",
> +			       ERR_PTR(ret));

Please avoid printing errors like this if we can't read the state. If we
become unable to read the state, then this message will flood the log at
the polling rate (if polling is enabled.)

I know the other cases here do, but they shouldn't.

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