Re: [PATCH RFC net-next 06/10] net: dsa: sja1105: simplify static configuration reload

[Vladimir Oltean <olteanv@gmail.com>]

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
>