Message-ID: <7186132a-7040-7131-396d-f1d6321e39d7@gmail.com>
Date:   Fri, 4 Nov 2022 08:40:56 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        UNGLinuxDriver@...rochip.com,
        Heiner Kallweit <hkallweit1@...il.com>,
        Sean Anderson <sean.anderson@...o.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method



On 11/4/2022 4:35 AM, Russell King (Oracle) wrote:
> On Fri, Nov 04, 2022 at 11:24:44AM +0000, Russell King (Oracle) wrote:
>> On Tue, Nov 01, 2022 at 01:48:06PM +0200, Vladimir Oltean wrote:
>>> As of now, all DSA drivers use phylink_generic_validate() and there is
>>> no known use case remaining for a driver-specific link mode validation
>>> procedure. As such, remove this DSA operation and let phylink determine
>>> what is supported based on config->mac_capabilities, which all drivers
>>> provide.
>>>
>>> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
>>> ---
>>> Not all DSA drivers provide config->mac_capabilities, for example
>>> mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
>>> those drivers on recent kernels and no one reported that they fail to
>>> establish a link, so I'm guessing that they work (somehow). But I must
>>> admit I don't understand why phylink_generic_validate() works when
>>> mac_capabilities=0. Anyway, these drivers did not provide a
>>> phylink_validate() method before and do not provide one now, so nothing
>>> changes for them.
>>
>> There is one remaining issue that needs to be properly addressed,
>> which is the bcm_sf2 driver, which is basically buggy. The recent
>> kernel build bot reports reminded me of this.
>>
>> I've tried talking to Florian about it, and didn't make much progress,
>> so I'm carrying a patch in my tree which at least makes what is
>> provided to phylink correct.

Might be worth submitting as RFC/RFT and then just hunt me down until 
you get what you want from me?

>>
>> See
>> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
>> and all the FIXME comments in there.
>>
>> This driver really needs to be fixed before we kill DSA's
>> phylink_validate method (although doing so doesn't change anything
>> in mainline, but will remove my reminder that bcm_sf2 is still
>> technically broken.)
> 
> Here's the corrected patch, along with a bit more commentry about the
> problems that I had kicking around in another commit.
> 
> 8<=====
> From: Russell King <rmk+kernel@...linux.org.uk>
> Subject: [PATCH] net: dsa: bcm_sf2: fix pause mode validation
> 
> The implementation appears not to appear to support pause modes on
> anything but RGMII, RGMII_TXID, MII and REVMII interface modes. Let
> phylink know that detail.
> 
> Moreover, RGMII_RXID and RGMII_ID appears to be unsupported.
> (This may not be correct; particularly see the FIXMEs in this patch.)
> 
> Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
> ---
>   drivers/net/dsa/bcm_sf2.c | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index 18a3847bd82b..6676971128d1 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -727,6 +727,10 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
>   		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
>   		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
>   		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
> +
> +		/* FIXME 1: Are RGMII_RXID and RGMII_ID actually supported?
> +		 * See FIXME 2 and FIXME 3 below.
> +		 */

They are supported, just not tested and still don't have hardware to 
test, I suppose you can include both modes for simplicity if they end up 
being broken, a fix would be submitted.

>   		phy_interface_set_rgmii(interfaces);
>   	}
>   
> @@ -734,6 +738,28 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
>   		MAC_10 | MAC_100 | MAC_1000;
>   }
>   
> +static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port,
> +				unsigned long *supported,
> +				struct phylink_link_state *state)
> +{
> +	u32 caps;
> +
> +	caps = dsa_to_port(ds, port)->pl_config.mac_capabilities;
> +
> +	/* Pause modes are only programmed for these modes - see FIXME 3.
> +	 * So, as pause modes are not configured for other modes, disable
> +	 * support for them. If FIXME 3 is updated to allow the other RGMII
> +	 * modes, these should be included here as well.
> +	 */
> +	if (!(state->interface == PHY_INTERFACE_MODE_RGMII ||
> +	      state->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> +	      state->interface == PHY_INTERFACE_MODE_MII ||
> +	      state->interface == PHY_INTERFACE_MODE_REVMII))
> +		caps &= ~(MAC_ASYM_PAUSE | MAC_SYM_PAUSE);

Can be programmed on all ports.

> +
> +	phylink_validate_mask_caps(supported, state, caps);
> +}
> +
>   static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
>   				  unsigned int mode,
>   				  const struct phylink_link_state *state)
> @@ -747,6 +773,11 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
>   		return;
>   
>   	switch (state->interface) {
> +	/* FIXME 2: Are RGMII_RXID and RGMII_ID actually supported? This
> +	 * switch statement means that the RGMII control register does not
> +	 * get programmed in these two modes, but surely it needs to at least
> +	 * set the port mode to EXT_GPHY?
> +	 */
>   	case PHY_INTERFACE_MODE_RGMII:
>   		id_mode_dis = 1;
>   		fallthrough;
> @@ -850,6 +881,10 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
>   		else
>   			offset = CORE_STS_OVERRIDE_GMIIP2_PORT(port);
>   
> +		/* FIXME 3: Are RGMII_RXID and RGMII_ID actually supported?
> +		 * Why are pause modes only supported for a couple of RGMII
> +		 * modes? Should this be using phy_interface_mode_is_rgmii()?
> +		 */
>   		if (interface == PHY_INTERFACE_MODE_RGMII ||
>   		    interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>   		    interface == PHY_INTERFACE_MODE_MII ||
> @@ -1207,6 +1242,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
>   	.get_ethtool_phy_stats	= b53_get_ethtool_phy_stats,
>   	.get_phy_flags		= bcm_sf2_sw_get_phy_flags,
>   	.phylink_get_caps	= bcm_sf2_sw_get_caps,
> +	.phylink_validate	= bcm_sf2_sw_validate,
>   	.phylink_mac_config	= bcm_sf2_sw_mac_config,
>   	.phylink_mac_link_down	= bcm_sf2_sw_mac_link_down,
>   	.phylink_mac_link_up	= bcm_sf2_sw_mac_link_up,

-- 
Florian

Powered by blists - more mailing lists