Re: [PATCH net] net: dsa: realtek: rtl8365mb: fix GMII caps for ports with internal PHY

[Russell King (Oracle)]

On Mon, Jun 06, 2022 at 03:01:30PM +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
> 
> phylib defaults to GMII when no phy-mode or phy-connection-type property
> is specified in a DSA port node.
> 
> Commit a5dba0f207e5 ("net: dsa: rtl8365mb: add GMII as user port mode")
> introduced implicit support for GMII mode on ports with internal PHY to
> allow a PHY connection for device trees where the phy-mode is not
> explicitly set to "internal".
> 
> Commit 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> then broke this behaviour by discarding the usage of
> rtl8365mb_phy_mode_supported() - where this GMII support was indicated -
> while switching to the new .phylink_get_caps API.
> 
> With the new API, rtl8365mb_phy_mode_supported() is no longer needed.
> Remove it altogether and add back the GMII capability - this time to
> rtl8365mb_phylink_get_caps() - so that the above default behaviour works
> for ports with internal PHY again.

Oops - I guess this has been caused by the delay between my patch being
initially prepared, it sitting around in my tree for many months while
other patches get merged, and it eventually seeing the light of day.

Sorry about that.

> Fixes: 6ff6064605e9 ("net: dsa: realtek: convert to phylink_generic_validate()")
> Signed-off-by: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
> ---
> 
> Luiz, Russel:
> 
> Commit a5dba0f207e5 ought to have had a Fixes: tag I think, because it
> claims to have been fixing a regression in the net-next tree - is that
> right? I seem to have missed both referenced commits when they were
> posted and never hit this issue personally. I only found things now
> during some other refactoring and the test for GMII looked weird to me
> so I went and investigated.
> 
> Could you please help me identify that Fixes: tag? Just for my own
> understanding of what caused this added requirement for GMII on ports
> with internal PHY.

I have absolutely no idea. I don't think any "requirement" has ever been
added - phylib has always defaulted to GMII, so as the driver stood when
it was first submitted on Oct 18 2021, I don't see how it could have
worked, unless the DT it was being tested with specified a phy-mode of
"internal". As you were the one who submitted it, you would have a
better idea.

The only suggestion I have is to bisect to find out exactly what caused
the GMII vs INTERNAL issue to crop up.

> 
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 38 +++++++----------------------
>  1 file changed, 9 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 3bb42a9f236d..769f672e9128 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -955,35 +955,21 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  	return 0;
>  }
>  
> -static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
> -					 phy_interface_t interface)
> -{
> -	int ext_int;
> -
> -	ext_int = rtl8365mb_extint_port_map[port];
> -
> -	if (ext_int < 0 &&
> -	    (interface == PHY_INTERFACE_MODE_NA ||
> -	     interface == PHY_INTERFACE_MODE_INTERNAL ||
> -	     interface == PHY_INTERFACE_MODE_GMII))
> -		/* Internal PHY */
> -		return true;
> -	else if ((ext_int >= 1) &&
> -		 phy_interface_mode_is_rgmii(interface))
> -		/* Extension MAC */
> -		return true;
> -
> -	return false;
> -}
> -
>  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  				       struct phylink_config *config)
>  {
> -	if (dsa_is_user_port(ds, port))
> +	if (dsa_is_user_port(ds, port)) {

Given the updates to rtl8365mb_phy_mode_supported(), this misses out on
the check of rtl8365mb_extint_port_map[port] introduced in commit
6147631c079f ("net: dsa: realtek: rtl8365mb: allow non-cpu extint
ports").

>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
> -	else if (dsa_is_cpu_port(ds, port))
> +
> +		/* GMII is the default interface mode for phylib, so
> +		 * we have to support it for ports with integrated PHY.
> +		 */
> +		__set_bit(PHY_INTERFACE_MODE_GMII,
> +			  config->supported_interfaces);
> +	} else if (dsa_is_cpu_port(ds, port)) {

This test also needs to be updated.

Not sure what rtl8365mb_extint_port_map[port] == 0 is supposed to
signify - maybe port unusable? Looks that way to me.

>  		phy_interface_set_rgmii(config->supported_interfaces);
> +	}
>  
>  	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
>  				   MAC_10 | MAC_100 | MAC_1000FD;
> @@ -996,12 +982,6 @@ static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
>  	struct realtek_priv *priv = ds->priv;
>  	int ret;
>  
> -	if (!rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
> -		dev_err(priv->dev, "phy mode %s is unsupported on port %d\n",
> -			phy_modes(state->interface), port);
> -		return;
> -	}
> -
>  	if (mode != MLO_AN_PHY && mode != MLO_AN_FIXED) {
>  		dev_err(priv->dev,
>  			"port %d supports only conventional PHY or fixed-link\n",
> -- 
> 2.36.0
> 
> 

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