Re: [PATCH net-next v2 5/5] net: dsa: realtek: rtl8365mb: handle PHY interface modes correctly

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Jun 10, 2022 at 05:38:29PM +0200, Alvin Šipraga wrote:
> Finally, rtl8365mb_phylink_get_caps() is fixed up to return supported
> capabilities based on the external interface properties described above.
> This allows for ports with an external interface to be treated as DSA
> user ports, and for ports with an internal PHY to be treated as DSA CPU
> ports.

I've needed to read that a few times... and I'm still not sure. You seem
to be saying that:
- ports with an internal PHY (which presumably provide baseT connections?)
  are used as DSA CPU ports.
- ports with an external interface supporting a range of RGMII, SGMII and
  HSGMII interface modes are DSA user ports.

With Marvell switches, it's the other way around - the ports with an
internal PHY are normally DSA user ports. Other ports can be a user,
inter-switch or CPU port.

So, I'm slightly confused by your description.

In any ase, looking at the get_caps() (and only that):

> @@ -953,7 +1026,13 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  				       struct phylink_config *config)
>  {
> -	if (dsa_is_user_port(ds, port)) {
> +	const struct rtl8365mb_extint *extint =
> +		rtl8365mb_get_port_extint(ds->priv, port);
> +
> +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000FD;

MAC capabilities are constant across all interfaces - okay.

> +
> +	if (!extint) {
>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
>  
> @@ -962,12 +1041,16 @@ static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  		 */
>  		__set_bit(PHY_INTERFACE_MODE_GMII,
>  			  config->supported_interfaces);
> -	} else if (dsa_is_cpu_port(ds, port)) {
> -		phy_interface_set_rgmii(config->supported_interfaces);
> +		return;

Internal ports need to support phylib, so both internal and gmii
interface modes - okay.

>  	}
>  
> -	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> -				   MAC_10 | MAC_100 | MAC_1000FD;
> +	/* Populate according to the modes supported by _this driver_,
> +	 * not necessarily the modes supported by the hardware, some of
> +	 * which remain unimplemented.
> +	 */
> +
> +	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_RGMII)
> +		phy_interface_set_rgmii(config->supported_interfaces);

External ports that support RGMII get all RGMII modes - also okay.

So, for the get_cops() function, I'm fine with this new code, and for
this alone:

Acked-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

I haven't looked at the remainder of the changes.

Thanks.

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