Re: [RFC PATCH v2 1/8] mii: Add helpers for parsing SGMII auto-negotiation

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Wed, Dec 18, 2019 at 12:18:24AM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Typically a MAC PCS auto-configures itself after it receives the
> negotiated copper-side link settings from the PHY, but some MAC devices
> are more special and need manual interpretation of the SGMII AN result.
> 
> In other cases, the PCS exposes the entire tx_config_reg base page as it
> is transmitted on the wire during auto-negotiation, so it makes sense to
> be able to decode the equivalent lp_advertised bit mask from the raw u16
> (of course, "lp" considering the PCS to be the local PHY).
> 
> Therefore, add the bit definitions for the SGMII registers 4 and 5
> (local device ability, link partner ability), as well as a link_mode
> conversion helper that can be used to feed the AN results into
> phy_resolve_aneg_linkmode.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  include/linux/mii.h      | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/mii.h | 10 ++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/include/linux/mii.h b/include/linux/mii.h
> index 4ce8901a1af6..18c6208f56fc 100644
> --- a/include/linux/mii.h
> +++ b/include/linux/mii.h
> @@ -373,6 +373,56 @@ static inline u32 mii_lpa_to_ethtool_lpa_x(u32 lpa)
>  }
>  
>  /**
> + * mii_lpa_mod_linkmode_adv_sgmii
> + * @lp_advertising: pointer to destination link mode.
> + * @lpa: value of the MII_LPA register
> + *
> + * A small helper function that translates MII_LPA bits to
> + * linkmode advertisement settings for SGMII.
> + * Leaves other bits unchanged.
> + */
> +static inline void
> +mii_lpa_mod_linkmode_lpa_sgmii(unsigned long *lp_advertising, u32 lpa)
> +{
> +	u32 speed_duplex = lpa & LPA_SGMII_DPX_SPD_MASK;
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, lp_advertising,
> +			 speed_duplex == LPA_SGMII_1000HALF);
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, lp_advertising,
> +			 speed_duplex == LPA_SGMII_1000FULL);
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, lp_advertising,
> +			 speed_duplex == LPA_SGMII_100HALF);
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, lp_advertising,
> +			 speed_duplex == LPA_SGMII_100FULL);
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, lp_advertising,
> +			 speed_duplex == LPA_SGMII_10HALF);
> +
> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, lp_advertising,
> +			 speed_duplex == LPA_SGMII_10FULL);
> +}
> +
> +/**
> + * mii_lpa_to_linkmode_adv_sgmii
> + * @advertising: pointer to destination link mode.
> + * @lpa: value of the MII_LPA register
> + *
> + * A small helper function that translates MII_ADVERTISE bits
> + * to linkmode advertisement settings when in SGMII mode.
> + * Clears the old value of advertising.
> + */
> +static inline void mii_lpa_to_linkmode_lpa_sgmii(unsigned long *lp_advertising,
> +						 u32 lpa)
> +{
> +	linkmode_zero(lp_advertising);
> +
> +	mii_lpa_mod_linkmode_lpa_sgmii(lp_advertising, lpa);
> +}
> +
> +/**
>   * mii_adv_mod_linkmode_adv_t
>   * @advertising:pointer to destination link mode.
>   * @adv: value of the MII_ADVERTISE register
> diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
> index 51b48e4be1f2..dc3b5d635beb 100644
> --- a/include/uapi/linux/mii.h
> +++ b/include/uapi/linux/mii.h
> @@ -71,6 +71,7 @@
>  /* Advertisement control register. */
>  #define ADVERTISE_SLCT		0x001f	/* Selector bits               */
>  #define ADVERTISE_CSMA		0x0001	/* Only selector supported     */
> +#define ADVERTISE_SGMII		0x0001	/* Can do SGMII                */
>  #define ADVERTISE_10HALF	0x0020	/* Try for 10mbps half-duplex  */
>  #define ADVERTISE_1000XFULL	0x0020	/* Try for 1000BASE-X full-duplex */
>  #define ADVERTISE_10FULL	0x0040	/* Try for 10mbps full-duplex  */
> @@ -94,6 +95,7 @@
>  
>  /* Link partner ability register. */
>  #define LPA_SLCT		0x001f	/* Same as advertise selector  */
> +#define LPA_SGMII		0x0001	/* Can do SGMII                */
>  #define LPA_10HALF		0x0020	/* Can do 10mbps half-duplex   */
>  #define LPA_1000XFULL		0x0020	/* Can do 1000BASE-X full-duplex */
>  #define LPA_10FULL		0x0040	/* Can do 10mbps full-duplex   */
> @@ -104,11 +106,19 @@
>  #define LPA_1000XPAUSE_ASYM	0x0100	/* Can do 1000BASE-X pause asym*/
>  #define LPA_100BASE4		0x0200	/* Can do 100mbps 4k packets   */
>  #define LPA_PAUSE_CAP		0x0400	/* Can pause                   */
> +#define LPA_SGMII_DPX_SPD_MASK	0x1C00	/* SGMII duplex and speed bits */
> +#define LPA_SGMII_10HALF	0x0000	/* Can do SGMII 10mbps half-duplex */
> +#define LPA_SGMII_10FULL	0x1000	/* Can do SGMII 10mbps full-duplex */
> +#define LPA_SGMII_100HALF	0x0400	/* Can do SGMII 100mbps half-duplex */
> +#define LPA_SGMII_100FULL	0x1400	/* Can do SGMII 100mbps full-duplex */
>  #define LPA_PAUSE_ASYM		0x0800	/* Can pause asymetrically     */
> +#define LPA_SGMII_1000HALF	0x0800	/* Can do SGMII 1000mbps half-duplex */
> +#define LPA_SGMII_1000FULL	0x1800	/* Can do SGMII 1000mbps full-duplex */
>  #define LPA_RESV		0x1000	/* Unused...                   */
>  #define LPA_RFAULT		0x2000	/* Link partner faulted        */
>  #define LPA_LPACK		0x4000	/* Link partner acked us       */
>  #define LPA_NPAGE		0x8000	/* Next page bit               */
> +#define LPA_SGMII_LINK		0x8000	/* Link partner has link       */

I wonder whether mixing these definitions together is really such a
good idea, or whether separately grouping them would be better.

I already find the mixture of Clause 37 and Clause 22 definitions to
be a little difficult to spot which are which.

>  
>  #define LPA_DUPLEX		(LPA_10FULL | LPA_100FULL)
>  #define LPA_100			(LPA_100FULL | LPA_100HALF | LPA_100BASE4)
> -- 
> 2.7.4
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up