Re: [RFC PATCH v2 net-next 08/15] net: phylink: allow PCS to handle C73 autoneg for phy-mode = "internal"

[Vladimir Oltean]

Hi Russell,

On Sat, Sep 23, 2023 at 04:48:57PM +0300, Vladimir Oltean wrote:
> Some phylink and phylib based systems might want to operate on backplane
> media types ("K" in the name), and thus, picking a phy_interface_t for
> them becomes a challenge.
> 
> phy_interface_t is a description of the connection between the MAC and
> the PHY, or if a MAC-side PCS is present, the connection between that
> and the next link segment (which can be remote).
> 
> A MAC-side PCS is so far considered to be a PCS handling link modes with
> optional C37 autoneg. But C73 autoneg (for backplanes and SFP28 modules)
> is not at the same level in the OSI layering, so that existing model may
> or may not apply.
> 
> (a) If we say that the PCS is MAC-side for C73 modes as well, the
>     implication seems to be that the phy-mode should be one of
>     PHY_INTERFACE_MODE_10GBASEKR, PHY_INTERFACE_MODE_1000BASEKX, etc.
>     Similar to PHY_INTERFACE_MODE_1000BASEX which imitates the link mode
>     ETHTOOL_LINK_MODE_1000baseX_Full_BIT.
> 
> (b) If we say that the PCS is not MAC-side, but rather that the
>     phylink_pcs represents an entire non-phylib backplane PHY which may
>     negotiate one of many link modes (like a copper phylib PHY), then
>     the phy-mode should probably be one of PHY_INTERFACE_MODE_XGMII,
>     XLGMII etc. Or rather, because there is no MII pinout per se and the
>     backplane PHY / phylink_pcs is internal, we can also use
>     PHY_INTERFACE_MODE_INTERNAL.
> 
> The trouble with (a), in my opinion, is that if we let the phy_interface_t
> follow the link mode like in the case of Base-X fiber modes, we have to
> consider the fact that C73 PHYs can advertise multiple link modes, so
> the phy_interface_t selection will be arbitrary, and any phy_interface_t
> selection will have to leave in the "supported" and "advertised" masks
> of link modes all the other backplane modes. This may be hard to justify.
> 
> That is the reasoning based on which I selected this phy-mode to
> describe the setup in Layerscape SoCs which have integrated backplane
> autoneg support. The changes in phylink permit the managed =
> "in-band-status" fwnode property to be extended for C73 autoneg, which
> is then controllable through ethtool. With phy-mode = "internal" in an
> in-band autoneg mode, we advertise all backplane link modes. The list is
> not exhaustive and may be extended in the future.
> 
> Link: https://lore.kernel.org/netdev/ZOXlpkbcAZ4okric@xxxxxxxxxxxxxxxxxxxxx/
> Link: https://lore.kernel.org/netdev/ZGIkGmyL8yL1q1zp@xxxxxxxxxxxxxxxxxxxxx/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> ---
> v1->v2: patch is new
> 
>  drivers/net/phy/phylink.c | 19 ++++++++++++++++++-
>  include/linux/phylink.h   |  1 +
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 548130d77302..88ace7e203c3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -972,6 +972,21 @@ static int phylink_parse_mode(struct phylink *pl,
>  			phylink_set(pl->supported, 100000baseDR2_Full);
>  			break;
>  
> +		case PHY_INTERFACE_MODE_INTERNAL:
> +			phylink_set(pl->supported, 1000baseKX_Full);
> +			phylink_set(pl->supported, 10000baseKX4_Full);
> +			phylink_set(pl->supported, 10000baseKR_Full);
> +			phylink_set(pl->supported, 25000baseCR_Full);
> +			phylink_set(pl->supported, 25000baseKR_Full);
> +			phylink_set(pl->supported, 25000baseCR_S_Full);
> +			phylink_set(pl->supported, 25000baseKR_S_Full);
> +			phylink_set(pl->supported, 40000baseKR4_Full);
> +			phylink_set(pl->supported, 50000baseKR2_Full);
> +			phylink_set(pl->supported, 50000baseKR_Full);
> +			phylink_set(pl->supported, 100000baseKR4_Full);
> +			phylink_set(pl->supported, 100000baseKR2_Full);
> +			break;
> +
>  		default:
>  			phylink_err(pl,
>  				    "incorrect link mode %s for in-band status\n",
> @@ -1109,7 +1124,9 @@ static void phylink_mac_config(struct phylink *pl,
>  
>  static bool phylink_pcs_handles_an(phy_interface_t iface, unsigned int mode)
>  {
> -	return phy_interface_mode_is_8023z(iface) && phylink_autoneg_inband(mode);
> +	return (phy_interface_mode_is_8023z(iface) ||
> +		iface == PHY_INTERFACE_MODE_INTERNAL) &&
> +	       phylink_autoneg_inband(mode);
>  }
>  
>  static void phylink_pcs_an_restart(struct phylink *pl)
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 2b886ea654bb..7e8e26001587 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -141,6 +141,7 @@ static inline unsigned int phylink_pcs_neg_mode(unsigned int mode,
>  
>  	case PHY_INTERFACE_MODE_1000BASEX:
>  	case PHY_INTERFACE_MODE_2500BASEX:
> +	case PHY_INTERFACE_MODE_INTERNAL:
>  		/* 1000base-X is designed for use media-side for Fibre
>  		 * connections, and thus the Autoneg bit needs to be
>  		 * taken into account. We also do this for 2500base-X
> -- 
> 2.34.1
>

What do you think about this change?