Re: [RFC v8 net-next 15/16] net: dsa: felix: add phylink_get_caps capability

[Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>]

On Mon, May 09, 2022 at 05:30:30PM +0000, Vladimir Oltean wrote:
> On Mon, May 09, 2022 at 05:23:32PM -0700, Colin Foster wrote:
> > > > @@ -982,15 +982,23 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
> > > >  				   struct phylink_config *config)
> > > >  {
> > > >  	struct ocelot *ocelot = ds->priv;
> > > > +	struct felix *felix;
> > > >  
> > > > -	/* This driver does not make use of the speed, duplex, pause or the
> > > > -	 * advertisement in its mac_config, so it is safe to mark this driver
> > > > -	 * as non-legacy.
> > > > -	 */
> > > > -	config->legacy_pre_march2020 = false;
> > > > +	felix = ocelot_to_felix(ocelot);
> > > > +
> > > > +	if (felix->info->phylink_get_caps) {
> > > > +		felix->info->phylink_get_caps(ocelot, port, config);
> > > > +	} else {
> > > >  
> > > > -	__set_bit(ocelot->ports[port]->phy_mode,
> > > > -		  config->supported_interfaces);
> > > > +		/* This driver does not make use of the speed, duplex, pause or
> > > > +		 * the advertisement in its mac_config, so it is safe to mark
> > > > +		 * this driver as non-legacy.
> > > > +		 */
> > > > +		config->legacy_pre_march2020 = false;
> > > 
> > > I don't think you mean to set legacy_pre_march2020 to true only
> > > felix->info->phylink_get_caps is absent, do you?
> > > 
> > > Also, I'm thinking maybe we could provide an implementation of this
> > > function for all switches, not just for vsc7512.
> > 
> > I had assumed these last two patches might spark more discussion, which
> > is why I kept them separate (specifically the last patch).
> > 
> > With this, are you simply suggesting to take everything that is
> > currently in felix_phylink_get_caps and doing it in the felix / seville
> > implementations? This is because the default condition is no longer the
> > "only" condition. Sounds easy enough.
> 
> No, not everything, just the way in which config->supported_interfaces
> is populated. We have different PCS implementations, so it's likely that
> the procedures to retrieve the valid SERDES protocols (when changing
> them will be supported) are different.
> 
> But in fact I seriously doubt that the current way in which supported_interfaces
> gets populated is limiting you from doing anything right now, precisely
> because you don't have any code that supports changing the phy-mode.

Hmm... So the main reason I needed get_caps was because
phylink_generic_validate looks at mac_capabilities. I know I'll have to
deal with supported_interfaces once I finally get the other four ports
working, but for now the main purpose of this patch is to allow me to
populate the mac_capabilities entry for phylink_generic_validate.

Aside from ensuring legacy_pre_march2020 = false, I feel like the rest
of the patch makes sense.

> 
> Also, it's unlikely (I'd say impossible) for one driver to be
> unconverted to the post-March-2020 requirements and the other not to be.
> The simple reason is that they share the same mac_config implementation.
> So it's perfectly ok to keep "config->legacy_pre_march2020 = false"
> right where it is.
> 
> So I'd say it's up to you, but I'd drop this patch right now.