Re: [PATCH v1 net-next 4/6] net: dsa: ocelot: felix: add per-device-per-port quirks

[Colin Foster]

On Sun, Nov 21, 2021 at 05:13:25PM +0000, Vladimir Oltean wrote:
> On Fri, Nov 19, 2021 at 02:43:11PM -0800, Colin Foster wrote:
> > Initial Felix-driver products (VSC9959 and VSC9953) both had quirks
> > where the PCS was in charge of rate adaptation. In the case of the
> > VSC7512 there is a differnce in that some ports (ports 0-3) don't have
> > a PCS and others might have different quirks based on how they are
> > configured.
> > 
> > This adds a generic method by which any port can have any quirks that
> > are handled by each device's driver.
> > 
> > Signed-off-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/net/dsa/ocelot/felix.c           | 20 +++++++++++++++++---
> >  drivers/net/dsa/ocelot/felix.h           |  4 ++++
> >  drivers/net/dsa/ocelot/felix_vsc9959.c   |  1 +
> >  drivers/net/dsa/ocelot/seville_vsc9953.c |  1 +
> >  4 files changed, 23 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> > index 2a90a703162d..5be2baa83bd8 100644
> > --- a/drivers/net/dsa/ocelot/felix.c
> > +++ b/drivers/net/dsa/ocelot/felix.c
> > @@ -824,14 +824,25 @@ static void felix_phylink_mac_config(struct dsa_switch *ds, int port,
> >  		phylink_set_pcs(dp->pl, &felix->pcs[port]->pcs);
> >  }
> >  
> > +unsigned long felix_quirks_have_rate_adaptation(struct ocelot *ocelot,
> > +						int port)
> > +{
> > +	return FELIX_MAC_QUIRKS;
> > +}
> > +EXPORT_SYMBOL(felix_quirks_have_rate_adaptation);
> > +
> 
> I would prefer if you don't introduce an actual virtual function for
> this. An unsigned long bitmask constant per device family should be
> enough. Even if we end up in a situation where internal PHY ports have
> one set of quirks and SERDES ports another, I would rather keep all
> quirks in a global namespace from 0 to 31, or whatever. So the quirks
> can be per device, instead or per port, and they can still say "this
> device's internal PHY ports need this", or "this device's SERDES ports
> need that". Does that make sense?

That makes sense. I'll be able to get that into v2. Hopefully I'm not
too far from getting the additional ports working, which is when I'll
finish the PCS logic.

> 
> >  static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port,
> >  					unsigned int link_an_mode,
> >  					phy_interface_t interface)
> >  {
> >  	struct ocelot *ocelot = ds->priv;
> > +	unsigned long quirks;
> > +	struct felix *felix;
> >  
> > +	felix = ocelot_to_felix(ocelot);
> > +	quirks = felix->info->get_quirks_for_port(ocelot, port);
> >  	ocelot_phylink_mac_link_down(ocelot, port, link_an_mode, interface,
> > -				     FELIX_MAC_QUIRKS);
> > +				     quirks);
> >  }
> >  
> >  static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
> > @@ -842,11 +853,14 @@ static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port,
> >  				      bool tx_pause, bool rx_pause)
> >  {
> >  	struct ocelot *ocelot = ds->priv;
> > -	struct felix *felix = ocelot_to_felix(ocelot);
> > +	unsigned long quirks;
> > +	struct felix *felix;
> >  
> > +	felix = ocelot_to_felix(ocelot);
> > +	quirks = felix->info->get_quirks_for_port(ocelot, port);
> >  	ocelot_phylink_mac_link_up(ocelot, port, phydev, link_an_mode,
> >  				   interface, speed, duplex, tx_pause, rx_pause,
> > -				   FELIX_MAC_QUIRKS);
> > +				   quirks);
> >  
> >  	if (felix->info->port_sched_speed_set)
> >  		felix->info->port_sched_speed_set(ocelot, port, speed);
> > diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h
> > index 515bddc012c0..251463f7e882 100644
> > --- a/drivers/net/dsa/ocelot/felix.h
> > +++ b/drivers/net/dsa/ocelot/felix.h
> > @@ -52,6 +52,7 @@ struct felix_info {
> >  					u32 speed);
> >  	struct regmap *(*init_regmap)(struct ocelot *ocelot,
> >  				      struct resource *res);
> > +	unsigned long (*get_quirks_for_port)(struct ocelot *ocelot, int port);
> >  };
> >  
> >  extern const struct dsa_switch_ops felix_switch_ops;
> > @@ -72,4 +73,7 @@ struct felix {
> >  struct net_device *felix_port_to_netdev(struct ocelot *ocelot, int port);
> >  int felix_netdev_to_port(struct net_device *dev);
> >  
> > +unsigned long felix_quirks_have_rate_adaptation(struct ocelot *ocelot,
> > +						int port);
> > +
> >  #endif
> > diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
> > index 4ddec3325f61..7fc5cf28b7d9 100644
> > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> > @@ -2166,6 +2166,7 @@ static const struct felix_info felix_info_vsc9959 = {
> >  	.port_setup_tc		= vsc9959_port_setup_tc,
> >  	.port_sched_speed_set	= vsc9959_sched_speed_set,
> >  	.init_regmap		= ocelot_regmap_init,
> > +	.get_quirks_for_port	= felix_quirks_have_rate_adaptation,
> >  };
> >  
> >  static irqreturn_t felix_irq_handler(int irq, void *data)
> > diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
> > index ce30464371e2..c996fc45dc5e 100644
> > --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
> > +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
> > @@ -1188,6 +1188,7 @@ static const struct felix_info seville_info_vsc9953 = {
> >  	.phylink_validate	= vsc9953_phylink_validate,
> >  	.prevalidate_phy_mode	= vsc9953_prevalidate_phy_mode,
> >  	.init_regmap		= ocelot_regmap_init,
> > +	.get_quirks_for_port	= felix_quirks_have_rate_adaptation,
> >  };
> >  
> >  static int seville_probe(struct platform_device *pdev)
> > -- 
> > 2.25.1
> >