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

[Vladimir Oltean]

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?

>  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
>