| Message ID | E1nNGm6-00AOip-6r@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: sja1105: phylink updates | expand |
On Thu, Feb 24, 2022 at 04:15:26PM +0000, Russell King (Oracle) wrote: > Convert the PCS selection to use mac_select_pcs, which allows the PCS > to perform any validation it needs, and removes the need to set the PCS > in the mac_config() callback, delving into the higher DSA levels to do > so. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > drivers/net/dsa/sja1105/sja1105_main.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c > index e278bd86e3c6..b5c36f808df1 100644 > --- a/drivers/net/dsa/sja1105/sja1105_main.c > +++ b/drivers/net/dsa/sja1105/sja1105_main.c > @@ -1358,18 +1358,16 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, > return sja1105_clocking_setup_port(priv, port); > } > > -static void sja1105_mac_config(struct dsa_switch *ds, int port, > - unsigned int mode, > - const struct phylink_link_state *state) > +static struct phylink_pcs * > +sja1105_mac_select_pcs(struct dsa_switch *ds, int port, phy_interface_t iface) > { > - struct dsa_port *dp = dsa_to_port(ds, port); > struct sja1105_private *priv = ds->priv; > - struct dw_xpcs *xpcs; > - > - xpcs = priv->xpcs[port]; > + struct dw_xpcs *xpcs = priv->xpcs[port]; > > if (xpcs) > - phylink_set_pcs(dp->pl, &xpcs->pcs); > + return &xpcs->pcs; > + > + return NULL; > } > > static void sja1105_mac_link_down(struct dsa_switch *ds, int port, > @@ -3137,7 +3135,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = { > .port_max_mtu = sja1105_get_max_mtu, > .phylink_get_caps = sja1105_phylink_get_caps, > .phylink_validate = sja1105_phylink_validate, > - .phylink_mac_config = sja1105_mac_config, Deleting sja1105_mac_config() here is safe not because phylink_mac_config() stops calling pl->mac_ops->mac_config(), but because dsa_port_phylink_mac_config() first checks whether ds->ops->phylink_mac_config is implemented, and that is purely an artefact of providing a phylib-style ds->ops->adjust_link, right? Maybe it's worth mentioning. > + .phylink_mac_select_pcs = sja1105_mac_select_pcs, > .phylink_mac_link_up = sja1105_mac_link_up, > .phylink_mac_link_down = sja1105_mac_link_down, > .get_strings = sja1105_get_strings, > -- > 2.30.2 >
On Fri, Feb 25, 2022 at 12:39:13PM +0200, Vladimir Oltean wrote: > On Thu, Feb 24, 2022 at 04:15:26PM +0000, Russell King (Oracle) wrote: > > Convert the PCS selection to use mac_select_pcs, which allows the PCS > > to perform any validation it needs, and removes the need to set the PCS > > in the mac_config() callback, delving into the higher DSA levels to do > > so. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> Thanks. > > - .phylink_mac_config = sja1105_mac_config, > > Deleting sja1105_mac_config() here is safe not because > phylink_mac_config() stops calling pl->mac_ops->mac_config(), but > because dsa_port_phylink_mac_config() first checks whether > ds->ops->phylink_mac_config is implemented, and that is purely an > artefact of providing a phylib-style ds->ops->adjust_link, right? Yes and no. We already have a several DSA drivers that have NULL phylink_mac_config and that don't provide an adjust_link function. Even if adjust_link was eventually killed off, the test in dsa_port_phylink_mac_config() would still be necessary unless all these DSA drivers are updated with a stub function for it. Consequently, I view phylink_mac_config in DSA as entirely optional and that optionality is already very much a part of the DSA interface, even though that is not the case with the corresponding phylink_mac_ops .mac_config method. Moreover, this optionality is a common theme in DSA switch operations methods. > Maybe it's worth mentioning. Given that .phylink_mac_config is already established as being optional in DSA, does the addition of one more instance need to be explicitly mentioned?
On Fri, Feb 25, 2022 at 10:57:40AM +0000, Russell King (Oracle) wrote: > > > - .phylink_mac_config = sja1105_mac_config, > > > > Deleting sja1105_mac_config() here is safe not because > > phylink_mac_config() stops calling pl->mac_ops->mac_config(), but > > because dsa_port_phylink_mac_config() first checks whether > > ds->ops->phylink_mac_config is implemented, and that is purely an > > artefact of providing a phylib-style ds->ops->adjust_link, right? > > Yes and no. > > We already have a several DSA drivers that have NULL phylink_mac_config > and that don't provide an adjust_link function. Even if adjust_link was > eventually killed off, the test in dsa_port_phylink_mac_config() would > still be necessary unless all these DSA drivers are updated with a stub > function for it. > > Consequently, I view phylink_mac_config in DSA as entirely optional and > that optionality is already very much a part of the DSA interface, even > though that is not the case with the corresponding phylink_mac_ops > .mac_config method. > > Moreover, this optionality is a common theme in DSA switch operations > methods. > > > Maybe it's worth mentioning. > > Given that .phylink_mac_config is already established as being optional > in DSA, does the addition of one more instance need to be explicitly > mentioned? I am aware of that, I am just pointing out that this is an unintended side effect of the existence of adjust_link, and non-DSA phylink drivers still need the mac_config stub. When going back and forth between a DSA and a non-DSA driver, this is not obvious until you take into account the dsa_port_phylink_mac_config() trampoline. Anyway, don't mention it if you don't think it is necessary.
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index e278bd86e3c6..b5c36f808df1 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -1358,18 +1358,16 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, return sja1105_clocking_setup_port(priv, port); } -static void sja1105_mac_config(struct dsa_switch *ds, int port, - unsigned int mode, - const struct phylink_link_state *state) +static struct phylink_pcs * +sja1105_mac_select_pcs(struct dsa_switch *ds, int port, phy_interface_t iface) { - struct dsa_port *dp = dsa_to_port(ds, port); struct sja1105_private *priv = ds->priv; - struct dw_xpcs *xpcs; - - xpcs = priv->xpcs[port]; + struct dw_xpcs *xpcs = priv->xpcs[port]; if (xpcs) - phylink_set_pcs(dp->pl, &xpcs->pcs); + return &xpcs->pcs; + + return NULL; } static void sja1105_mac_link_down(struct dsa_switch *ds, int port, @@ -3137,7 +3135,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = { .port_max_mtu = sja1105_get_max_mtu, .phylink_get_caps = sja1105_phylink_get_caps, .phylink_validate = sja1105_phylink_validate, - .phylink_mac_config = sja1105_mac_config, + .phylink_mac_select_pcs = sja1105_mac_select_pcs, .phylink_mac_link_up = sja1105_mac_link_up, .phylink_mac_link_down = sja1105_mac_link_down, .get_strings = sja1105_get_strings,
Convert the PCS selection to use mac_select_pcs, which allows the PCS to perform any validation it needs, and removes the need to set the PCS in the mac_config() callback, delving into the higher DSA levels to do so. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/sja1105/sja1105_main.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)