Re: [PATCH 4/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

On Tue, May 10, 2022 at 08:29:10PM +0300, Vladimir Oltean wrote:
> On Mon, May 09, 2022 at 12:48:48AM +0200, Hauke Mehrtens wrote:
> > @@ -983,14 +1295,25 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, 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))
> > +	int ext_int = rtl8365mb_extint_port_map[port];
> > +
> > +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> > +				   MAC_10 | MAC_100 | MAC_1000FD;
> > +
> > +	if (dsa_is_user_port(ds, port)) {
> >  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> >  			  config->supported_interfaces);
> > -	else if (dsa_is_cpu_port(ds, port))
> > +	} else if (dsa_is_cpu_port(ds, port)) {
> 
> What does the quality of being a user port or a CPU port have to do with
> which interfaces are supported?

Right, I think this function was actually broken already in a few ways. The
switch will have ports with integrated PHYs, and ports with extension interfaces
like RGMII or SGMII etc. But which of those ports one uses as a CPU port, user
port, or (one day) DSA port, is of no concern to the switch. The supported
interface of a given port is a static property and simply a function of the port
number and switch model. All switch models in the family have between 1 and 2
ports with an extension interface.

Luiz introduced this map:

/* valid for all 6-port or less variants */
static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2, -1, -1};

... which I think is actually what we ought to test on. It can be improved, but
currently it is correct for all supported models.

So something like this would be correct:

static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
				       struct phylink_config *config)
{
	int ext_int = rtl8365mb_extint_port_map[port];
	if (ext_int == -1) {
		/* integrated PHY, set PHY_INTERFACE_MODE_INTERNAL etc. */
	} else {
		/* extension interface available, but here one should really
		 * check the model based on the chip ID/version, because it
		 * varies a lot
		 */
		if (model == RTL8365MB && ext_int == 1)
			/* RGMII */
		else if (model == RTL8367S && ext_int == 1)
			/* SGMII / HSGMII */
		else if (model == RTL8367S && ext_int == 2)
			/* RGMII */
		/* etc */
	}

	/* ... */
}

There are of course various ways to do this.

Hauke, do you follow what I mean?  Would you like me to prepare a patch for the
current supported models/interfaces and then you can rebase your changes on top
of that to indicate support for (H)SGMII? Or do you want to give it a try
yourself?

Kind regards,
Alvin
    

> 
> > +		if (ext_int == 1) {
> > +			__set_bit(PHY_INTERFACE_MODE_SGMII,
> > +				  config->supported_interfaces);
> > +			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > +				  config->supported_interfaces);
> > +			config->mac_capabilities |= MAC_2500FD;
> > +		}
> >  		phy_interface_set_rgmii(config->supported_interfaces);
> > +	}
> >  
> > -	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> > -				   MAC_10 | MAC_100 | MAC_1000FD;
> >  }
> >  
> >  static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,