Re: [PATCH net-next 01/15] net: phylink: add PCS negotiation mode

From: Vladimir Oltean <olteanv@gmail.com>
To: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Alexander Couzens <lynxis@fe80.eu>,
	AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	Cc:  Claudiu Beznea <claudiu.beznea@microchip.com>,
	Daniel Golle <daniel@makrotopia.org>,
	Daniel Machon <daniel.machon@microchip.com>,
	"David S. Miller" <davem@davemloft.net>,
	DENG Qingfang <dqfext@gmail.com>,
	Eric Dumazet <edumazet@google.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Horatiu Vultur <horatiu.vultur@microchip.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Jose Abreu <Jose.Abreu@synopsys.com>,
	Landen Chao <Landen.Chao@mediatek.com>,
	Lars Povlsen <lars.povlsen@microchip.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Madalin Bucur <madalin.bucur@nxp.com>,
	Marcin Wojtas <mw@semihalf.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Michal Simek <michal.simek@amd.com>,
	netdev@vger.kernel.org,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	Paolo Abeni <pabeni@redhat.com>,
	Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>,
	Sean Anderson <sean.anderson@seco.com>,
	Sean Wang <sean.wang@mediatek.com>,
	Steen Hegelund <Steen.Hegelund@microchip.com>,
	Taras Chornyi <taras.chornyi@plvision.eu>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	UNGLinuxDriver@microchip.com
Subject: Re: [PATCH net-next 01/15] net: phylink: add PCS negotiation mode
Date: Tue, 20 Jun 2023 14:34:29 +0300	[thread overview]
Message-ID: <20230620113429.mc4p4y6mny5cm4ih@skbuf> (raw)
In-Reply-To: <E1qA8De-00EaFA-Ht@rmk-PC.armlinux.org.uk>

On Fri, Jun 16, 2023 at 01:06:22PM +0100, Russell King (Oracle) wrote:
> PCS have to work out whether they should enable PCS negotiation by
> looking at the "mode" and "interface" arguments, and the Autoneg bit
> in the advertising mask.
> 
> This leads to some complex logic, so lets pull that out into phylink
> and instead pass a "neg_mode" argument to the PCS configuration and
> link up methods, instead of the "mode" argument.
> 
> In order to transition drivers, add a "neg_mode" flag to the phylink
> PCS structure to PCS can indicate whether they want to be passed the
> neg_mode or the old mode argument.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 0cf07d7d11b8..2b322d7fa51a 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -21,6 +21,24 @@ enum {
>  	MLO_AN_FIXED,	/* Fixed-link mode */
>  	MLO_AN_INBAND,	/* In-band protocol */
>  
> +	/* PCS "negotiation" mode.
> +	 *  PHYLINK_PCS_NEG_NONE - protocol has no inband capability
> +	 *  PHYLINK_PCS_NEG_OUTBAND - some out of band or fixed link setting

Would OUTBAND be more clearly named as FORCED maybe?

> +	 *  PHYLINK_PCS_NEG_INBAND_DISABLED - inband mode disabled, e.g.
> +	 *				      1000base-X with autoneg off
> +	 *  PHYLINK_PCS_NEG_INBAND_ENABLED - inband mode enabled
> +	 * Additionally, this can be tested using bitmasks:
> +	 *  PHYLINK_PCS_NEG_INBAND - inband mode selected
> +	 *  PHYLINK_PCS_NEG_ENABLED - negotiation mode enabled
> +	 */
> +	PHYLINK_PCS_NEG_NONE = 0,
> +	PHYLINK_PCS_NEG_ENABLED = BIT(4),

Why do we start the enum values from BIT(4)? What are we colliding with,
in the range of lower values?

> +	PHYLINK_PCS_NEG_OUTBAND = BIT(5),
> +	PHYLINK_PCS_NEG_INBAND = BIT(6),
> +	PHYLINK_PCS_NEG_INBAND_DISABLED = PHYLINK_PCS_NEG_INBAND,
> +	PHYLINK_PCS_NEG_INBAND_ENABLED = PHYLINK_PCS_NEG_INBAND |
> +					 PHYLINK_PCS_NEG_ENABLED,
> +
>  	/* MAC_SYM_PAUSE and MAC_ASYM_PAUSE are used when configuring our
>  	 * autonegotiation advertisement. They correspond to the PAUSE and
>  	 * ASM_DIR bits defined by 802.3, respectively.
> @@ -79,6 +97,70 @@ static inline bool phylink_autoneg_inband(unsigned int mode)
>  	return mode == MLO_AN_INBAND;
>  }
>  
> +/**
> + * phylink_pcs_neg_mode() - helper to determine PCS inband mode

I think you are naming it "neg_mode" rather than "aneg_mode" because
with OUTBAND/NONE, there's nothing "automatic" about the negotiation.
However, "neg" rather than "aneg" sounds more like a shorthand form of
"negation" or "negative". Would you oppose renaming it to "aneg_mode"?