| Message ID | E1ssjcz-005Ns9-D5@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: pcs: xpcs: cleanups batch 1 | expand |
On Mon, Sep 23, 2024 at 03:01:25PM +0100, Russell King (Oracle) wrote: > The static configuration reload saves the port speed in the static > configuration tables by first converting it from the internal > respresentation to the SPEED_xxx ethtool representation, and then > converts it back to restore the setting. This is because > sja1105_adjust_port_config() takes the speed as SPEED_xxx. > > However, this is unnecessarily complex. If we split > sja1105_adjust_port_config() up, we can simply save and restore the > mac[port].speed member in the static configuration tables. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- Thanks for the patch, and the idea is good. > drivers/net/dsa/sja1105/sja1105_main.c | 63 +++++++++++++------------- > 1 file changed, 32 insertions(+), 31 deletions(-) > > diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c > index bc7e50dcb57c..b95c64b7e705 100644 > --- a/drivers/net/dsa/sja1105/sja1105_main.c > +++ b/drivers/net/dsa/sja1105/sja1105_main.c > @@ -1257,29 +1257,11 @@ static int sja1105_parse_dt(struct sja1105_private *priv) > return rc; > } > > -/* Convert link speed from SJA1105 to ethtool encoding */ > -static int sja1105_port_speed_to_ethtool(struct sja1105_private *priv, > - u64 speed) > -{ > - if (speed == priv->info->port_speed[SJA1105_SPEED_10MBPS]) > - return SPEED_10; > - if (speed == priv->info->port_speed[SJA1105_SPEED_100MBPS]) > - return SPEED_100; > - if (speed == priv->info->port_speed[SJA1105_SPEED_1000MBPS]) > - return SPEED_1000; > - if (speed == priv->info->port_speed[SJA1105_SPEED_2500MBPS]) > - return SPEED_2500; > - return SPEED_UNKNOWN; > -} > - > -/* Set link speed in the MAC configuration for a specific port. */ > -static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, > - int speed_mbps) > +static int sja1105_set_port_speed(struct sja1105_private *priv, int port, > + int speed_mbps) > { > struct sja1105_mac_config_entry *mac; > - struct device *dev = priv->ds->dev; I think if you could keep this line in the new sja1105_set_port_speed() function.. > u64 speed; > - int rc; > > /* On P/Q/R/S, one can read from the device via the MAC reconfiguration > * tables. On E/T, MAC reconfig tables are not readable, only writable. > @@ -1313,7 +1295,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, > speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS]; > break; > default: > - dev_err(dev, "Invalid speed %iMbps\n", speed_mbps); > + dev_err(priv->ds->dev, "Invalid speed %iMbps\n", speed_mbps); you could also get rid of this unnecessary line change. > return -EINVAL; > } There are 2 more changes which I believe should be made in sja1105_set_port_speed(): - since it isn't called from mac_config() anymore but from mac_link_up() (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN - we can trust that phylink will not call mac_link_up() with a speed outside what we provided in mac_capabilities, so we can remove the -EINVAL "default" speed_mbps case, and make this method return void, as it can never truly cause an error But I believe these are incremental changes which should be done after this patch. I've made a note of them and will create 2 patches on top when I have the spare time. > > @@ -1325,11 +1307,29 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, > * we need to configure the PCS only (if even that). > */ > if (priv->phy_mode[port] == PHY_INTERFACE_MODE_SGMII) > - mac[port].speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS]; > + speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS]; > else if (priv->phy_mode[port] == PHY_INTERFACE_MODE_2500BASEX) > - mac[port].speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS]; > - else > - mac[port].speed = speed; > + speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS]; > + > + mac[port].speed = speed; > + > + return 0; > +} > + > +/* Set link speed in the MAC configuration for a specific port. */ Could this comment state this instead? "Write the MAC Configuration Table entry and, if necessary, the CGU settings, after a link speed change for this port." > +static int sja1105_set_port_config(struct sja1105_private *priv, int port) > +{ > + struct sja1105_mac_config_entry *mac; > + struct device *dev = priv->ds->dev; > + int rc; > + > + /* On P/Q/R/S, one can read from the device via the MAC reconfiguration > + * tables. On E/T, MAC reconfig tables are not readable, only writable. > + * We have to *know* what the MAC looks like. For the sake of keeping > + * the code common, we'll use the static configuration tables as a > + * reasonable approximation for both E/T and P/Q/R/S. > + */ > + mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries; > > /* Write to the dynamic reconfiguration tables */ > rc = sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port, > @@ -1390,7 +1390,8 @@ static void sja1105_mac_link_up(struct phylink_config *config, > struct sja1105_private *priv = dp->ds->priv; > int port = dp->index; > > - sja1105_adjust_port_config(priv, port, speed); > + if (!sja1105_set_port_speed(priv, port, speed)) > + sja1105_set_port_config(priv, port); > > sja1105_inhibit_tx(priv, BIT(port), false); > } > @@ -2293,7 +2294,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv, > { > struct ptp_system_timestamp ptp_sts_before; > struct ptp_system_timestamp ptp_sts_after; > - int speed_mbps[SJA1105_MAX_NUM_PORTS]; > + u64 mac_speed[SJA1105_MAX_NUM_PORTS]; Could you move this line lower to preserve the ordering by decreasing line length? > u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0}; > struct sja1105_mac_config_entry *mac; > struct dsa_switch *ds = priv->ds; > @@ -2307,14 +2308,13 @@ int sja1105_static_config_reload(struct sja1105_private *priv, > > mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries; > > - /* Back up the dynamic link speed changed by sja1105_adjust_port_config > + /* Back up the dynamic link speed changed by sja1105_set_port_speed Could you please put () after the function name? I know the original code did not have it, but my coding style has changed in the past 5 years. > * in order to temporarily restore it to SJA1105_SPEED_AUTO - which the > * switch wants to see in the static config in order to allow us to > * change it through the dynamic interface later. > */ > for (i = 0; i < ds->num_ports; i++) { > - speed_mbps[i] = sja1105_port_speed_to_ethtool(priv, > - mac[i].speed); > + mac_speed[i] = mac[i].speed; > mac[i].speed = priv->info->port_speed[SJA1105_SPEED_AUTO]; > > if (priv->xpcs[i]) > @@ -2377,7 +2377,8 @@ int sja1105_static_config_reload(struct sja1105_private *priv, > struct dw_xpcs *xpcs = priv->xpcs[i]; > unsigned int neg_mode; > > - rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]); > + mac[i].speed = mac_speed[i]; > + rc = sja1105_set_port_config(priv, i); > if (rc < 0) > goto out; > > -- > 2.30.2 >
On Wed, Sep 25, 2024 at 04:15:17PM +0300, Vladimir Oltean wrote: > On Mon, Sep 23, 2024 at 03:01:25PM +0100, Russell King (Oracle) wrote: > > +static int sja1105_set_port_speed(struct sja1105_private *priv, int port, > > + int speed_mbps) > > { > > struct sja1105_mac_config_entry *mac; > > - struct device *dev = priv->ds->dev; > > I think if you could keep this line in the new sja1105_set_port_speed() > function.. > > > u64 speed; > > - int rc; > > > > /* On P/Q/R/S, one can read from the device via the MAC reconfiguration > > * tables. On E/T, MAC reconfig tables are not readable, only writable. > > @@ -1313,7 +1295,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, > > speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS]; > > break; > > default: > > - dev_err(dev, "Invalid speed %iMbps\n", speed_mbps); > > + dev_err(priv->ds->dev, "Invalid speed %iMbps\n", speed_mbps); > > you could also get rid of this unnecessary line change. Yes, maybe I'll move it to another patch, but the reason I made the change is that I don't see much point to the local variable existing for just one user (there were multiple users prior to this patch.) However... > > return -EINVAL; > > } > > There are 2 more changes which I believe should be made in sja1105_set_port_speed(): > - since it isn't called from mac_config() anymore but from mac_link_up() > (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN > - we can trust that phylink will not call mac_link_up() with a speed > outside what we provided in mac_capabilities, so we can remove the > -EINVAL "default" speed_mbps case, and make this method return void, > as it can never truly cause an error > > But I believe these are incremental changes which should be done after > this patch. I've made a note of them and will create 2 patches on top > when I have the spare time. ... if we were to make those changes prior to this patch, then the dev_err() will no longer be there and thus this becomes a non-issue. So I'd suggest a patch prior to this one to make the changes you state here, thus eliminating the need for this hunk in this patch. > > +/* Set link speed in the MAC configuration for a specific port. */ > > Could this comment state this instead? "Write the MAC Configuration > Table entry and, if necessary, the CGU settings, after a link speed > change for this port." Done. > > @@ -2293,7 +2294,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv, > > { > > struct ptp_system_timestamp ptp_sts_before; > > struct ptp_system_timestamp ptp_sts_after; > > - int speed_mbps[SJA1105_MAX_NUM_PORTS]; > > + u64 mac_speed[SJA1105_MAX_NUM_PORTS]; > > Could you move this line lower to preserve the ordering by decreasing line length? > > > u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0}; Probably didn't notice that due to not having full clear sight for the screen yet (a few more weeks to go before that happens!) Thanks for spotting that. > > - /* Back up the dynamic link speed changed by sja1105_adjust_port_config > > + /* Back up the dynamic link speed changed by sja1105_set_port_speed > > Could you please put () after the function name? I know the original > code did not have it, but my coding style has changed in the past 5 years. Done. Thanks!
On Wed, Sep 25, 2024 at 08:38:23PM +0100, Russell King (Oracle) wrote: > > There are 2 more changes which I believe should be made in sja1105_set_port_speed(): > > - since it isn't called from mac_config() anymore but from mac_link_up() > > (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN > > - we can trust that phylink will not call mac_link_up() with a speed > > outside what we provided in mac_capabilities, so we can remove the > > -EINVAL "default" speed_mbps case, and make this method return void, > > as it can never truly cause an error > > > > But I believe these are incremental changes which should be done after > > this patch. I've made a note of them and will create 2 patches on top > > when I have the spare time. > > ... if we were to make those changes prior to this patch, then the > dev_err() will no longer be there and thus this becomes a non-issue. > So I'd suggest a patch prior to this one to make the changes you state > here, thus eliminating the need for this hunk in this patch. That sounds good. Are you suggesting you will write up such a patch for v2?
On Thu, Sep 26, 2024 at 12:16:13AM +0300, Vladimir Oltean wrote: > On Wed, Sep 25, 2024 at 08:38:23PM +0100, Russell King (Oracle) wrote: > > > There are 2 more changes which I believe should be made in sja1105_set_port_speed(): > > > - since it isn't called from mac_config() anymore but from mac_link_up() > > > (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN > > > - we can trust that phylink will not call mac_link_up() with a speed > > > outside what we provided in mac_capabilities, so we can remove the > > > -EINVAL "default" speed_mbps case, and make this method return void, > > > as it can never truly cause an error > > > > > > But I believe these are incremental changes which should be done after > > > this patch. I've made a note of them and will create 2 patches on top > > > when I have the spare time. > > > > ... if we were to make those changes prior to this patch, then the > > dev_err() will no longer be there and thus this becomes a non-issue. > > So I'd suggest a patch prior to this one to make the changes you state > > here, thus eliminating the need for this hunk in this patch. > > That sounds good. Are you suggesting you will write up such a patch for v2? Actually, the three patches become interdependent. Let's say we want to eliminate SPEED_UNKNOWN. Prior to my patch in this sub-thread, we have this: speed_mbps[i] = sja1105_port_speed_to_ethtool(priv, mac[i].speed); ... rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]); sja1105_port_speed_to_ethtool() can return SPEED_UNKNOWN if mac[i].speed is not one of the four encodings. If we can't guarantee that it is one of the four encodings, then SPEED_UNKNOWN may be passed into sja1105_adjust_port_config(). Similarly, as for the default case, we can't simply delete that, because that'll leave "speed" uninitialised and we'll get a build warning without my changes. We could change the default case to simply: default: return 0; but that just looks perverse. So, I think rather than trying to do your suggestion before my patch, my patch needs to stand as it currently is, and then your suggestion must happen after it - otherwise we end up introducing more complexity or weirdness. Hmm?
On Thu, Sep 26, 2024 at 01:02:35PM +0100, Russell King (Oracle) wrote: > On Thu, Sep 26, 2024 at 12:16:13AM +0300, Vladimir Oltean wrote: > > On Wed, Sep 25, 2024 at 08:38:23PM +0100, Russell King (Oracle) wrote: > > > > There are 2 more changes which I believe should be made in sja1105_set_port_speed(): > > > > - since it isn't called from mac_config() anymore but from mac_link_up() > > > > (change which happened quite a while ago), it mustn't handle SPEED_UNKNOWN > > > > - we can trust that phylink will not call mac_link_up() with a speed > > > > outside what we provided in mac_capabilities, so we can remove the > > > > -EINVAL "default" speed_mbps case, and make this method return void, > > > > as it can never truly cause an error > > > > > > > > But I believe these are incremental changes which should be done after > > > > this patch. I've made a note of them and will create 2 patches on top > > > > when I have the spare time. > > > > > > ... if we were to make those changes prior to this patch, then the > > > dev_err() will no longer be there and thus this becomes a non-issue. > > > So I'd suggest a patch prior to this one to make the changes you state > > > here, thus eliminating the need for this hunk in this patch. > > > > That sounds good. Are you suggesting you will write up such a patch for v2? > > Actually, the three patches become interdependent. > > Let's say we want to eliminate SPEED_UNKNOWN. Prior to my patch in this > sub-thread, we have this: > > speed_mbps[i] = sja1105_port_speed_to_ethtool(priv, > mac[i].speed); > ... > rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]); > > sja1105_port_speed_to_ethtool() can return SPEED_UNKNOWN if > mac[i].speed is not one of the four encodings. If we can't guarantee > that it is one of the four encodings, then SPEED_UNKNOWN may be > passed into sja1105_adjust_port_config(). > > Similarly, as for the default case, we can't simply delete that, > because that'll leave "speed" uninitialised and we'll get a build > warning without my changes. We could change the default case to > simply: > > default: > return 0; > > but that just looks perverse. > > So, I think rather than trying to do your suggestion before my patch, > my patch needs to stand as it currently is, and then your suggestion > must happen after it - otherwise we end up introducing more complexity > or weirdness. > > Hmm? Ok, if we're back to my original proposal, I'm implicitly okay with that.
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index bc7e50dcb57c..b95c64b7e705 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -1257,29 +1257,11 @@ static int sja1105_parse_dt(struct sja1105_private *priv) return rc; } -/* Convert link speed from SJA1105 to ethtool encoding */ -static int sja1105_port_speed_to_ethtool(struct sja1105_private *priv, - u64 speed) -{ - if (speed == priv->info->port_speed[SJA1105_SPEED_10MBPS]) - return SPEED_10; - if (speed == priv->info->port_speed[SJA1105_SPEED_100MBPS]) - return SPEED_100; - if (speed == priv->info->port_speed[SJA1105_SPEED_1000MBPS]) - return SPEED_1000; - if (speed == priv->info->port_speed[SJA1105_SPEED_2500MBPS]) - return SPEED_2500; - return SPEED_UNKNOWN; -} - -/* Set link speed in the MAC configuration for a specific port. */ -static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, - int speed_mbps) +static int sja1105_set_port_speed(struct sja1105_private *priv, int port, + int speed_mbps) { struct sja1105_mac_config_entry *mac; - struct device *dev = priv->ds->dev; u64 speed; - int rc; /* On P/Q/R/S, one can read from the device via the MAC reconfiguration * tables. On E/T, MAC reconfig tables are not readable, only writable. @@ -1313,7 +1295,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS]; break; default: - dev_err(dev, "Invalid speed %iMbps\n", speed_mbps); + dev_err(priv->ds->dev, "Invalid speed %iMbps\n", speed_mbps); return -EINVAL; } @@ -1325,11 +1307,29 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, * we need to configure the PCS only (if even that). */ if (priv->phy_mode[port] == PHY_INTERFACE_MODE_SGMII) - mac[port].speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS]; + speed = priv->info->port_speed[SJA1105_SPEED_1000MBPS]; else if (priv->phy_mode[port] == PHY_INTERFACE_MODE_2500BASEX) - mac[port].speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS]; - else - mac[port].speed = speed; + speed = priv->info->port_speed[SJA1105_SPEED_2500MBPS]; + + mac[port].speed = speed; + + return 0; +} + +/* Set link speed in the MAC configuration for a specific port. */ +static int sja1105_set_port_config(struct sja1105_private *priv, int port) +{ + struct sja1105_mac_config_entry *mac; + struct device *dev = priv->ds->dev; + int rc; + + /* On P/Q/R/S, one can read from the device via the MAC reconfiguration + * tables. On E/T, MAC reconfig tables are not readable, only writable. + * We have to *know* what the MAC looks like. For the sake of keeping + * the code common, we'll use the static configuration tables as a + * reasonable approximation for both E/T and P/Q/R/S. + */ + mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries; /* Write to the dynamic reconfiguration tables */ rc = sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port, @@ -1390,7 +1390,8 @@ static void sja1105_mac_link_up(struct phylink_config *config, struct sja1105_private *priv = dp->ds->priv; int port = dp->index; - sja1105_adjust_port_config(priv, port, speed); + if (!sja1105_set_port_speed(priv, port, speed)) + sja1105_set_port_config(priv, port); sja1105_inhibit_tx(priv, BIT(port), false); } @@ -2293,7 +2294,7 @@ int sja1105_static_config_reload(struct sja1105_private *priv, { struct ptp_system_timestamp ptp_sts_before; struct ptp_system_timestamp ptp_sts_after; - int speed_mbps[SJA1105_MAX_NUM_PORTS]; + u64 mac_speed[SJA1105_MAX_NUM_PORTS]; u16 bmcr[SJA1105_MAX_NUM_PORTS] = {0}; struct sja1105_mac_config_entry *mac; struct dsa_switch *ds = priv->ds; @@ -2307,14 +2308,13 @@ int sja1105_static_config_reload(struct sja1105_private *priv, mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries; - /* Back up the dynamic link speed changed by sja1105_adjust_port_config + /* Back up the dynamic link speed changed by sja1105_set_port_speed * in order to temporarily restore it to SJA1105_SPEED_AUTO - which the * switch wants to see in the static config in order to allow us to * change it through the dynamic interface later. */ for (i = 0; i < ds->num_ports; i++) { - speed_mbps[i] = sja1105_port_speed_to_ethtool(priv, - mac[i].speed); + mac_speed[i] = mac[i].speed; mac[i].speed = priv->info->port_speed[SJA1105_SPEED_AUTO]; if (priv->xpcs[i]) @@ -2377,7 +2377,8 @@ int sja1105_static_config_reload(struct sja1105_private *priv, struct dw_xpcs *xpcs = priv->xpcs[i]; unsigned int neg_mode; - rc = sja1105_adjust_port_config(priv, i, speed_mbps[i]); + mac[i].speed = mac_speed[i]; + rc = sja1105_set_port_config(priv, i); if (rc < 0) goto out;
The static configuration reload saves the port speed in the static configuration tables by first converting it from the internal respresentation to the SPEED_xxx ethtool representation, and then converts it back to restore the setting. This is because sja1105_adjust_port_config() takes the speed as SPEED_xxx. However, this is unnecessarily complex. If we split sja1105_adjust_port_config() up, we can simply save and restore the mac[port].speed member in the static configuration tables. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/sja1105/sja1105_main.c | 63 +++++++++++++------------- 1 file changed, 32 insertions(+), 31 deletions(-)