Re: [net v2 PATCH] net: stmmac: Update CBS parameters when speed changes after linking up

[Andrew Lunn]

>  static void stmmac_configure_cbs(struct stmmac_priv *priv)
>  {
>  	u32 tx_queues_count = priv->plat->tx_queues_to_use;
>  	u32 mode_to_use;
>  	u32 queue;
> +	u32 ptr, speed_div;
> +	u64 value;
> +
> +	/* Port Transmit Rate and Speed Divider */
> +	switch (priv->speed) {
> +	case SPEED_10000:
> +		ptr = 32;
> +		speed_div = 10000000;
> +		break;
> +	case SPEED_5000:
> +		ptr = 32;
> +		speed_div = 5000000;
> +		break;
> +	case SPEED_2500:
> +		ptr = 8;
> +		speed_div = 2500000;
> +		break;
> +	case SPEED_1000:
> +		ptr = 8;
> +		speed_div = 1000000;
> +		break;
> +	case SPEED_100:
> +		ptr = 4;
> +		speed_div = 100000;
> +		break;
> +	default:

No SPEED_10 ?

> +		netdev_dbg(priv->dev, "link speed is not known\n");
> +	}
>  
>  	/* queue 0 is reserved for legacy traffic */
>  	for (queue = 1; queue < tx_queues_count; queue++) {
> @@ -3196,6 +3231,12 @@ static void stmmac_configure_cbs(struct stmmac_priv *priv)
>  		if (mode_to_use == MTL_QUEUE_DCB)
>  			continue;
>  
> +		value = div_s64(priv->old_idleslope[queue] * 1024ll * ptr, speed_div);
> +		priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);

Rather than masking off the top bits, shouldn't you be looking for
overflow? that indicates the configuration is not possible. You don't
have a good way to report the problem, since there is no user action
on link up, so you cannot return -EINVAL or -EOPNOTSUPP. So you
probably want to set the hardware as close as possible.

Also, what happens if the result of the div is 0? Does 0 have a
special meaning?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 222540b55480..d3526ad91aff 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -355,6 +355,9 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
>  	if (!priv->dma_cap.av)
>  		return -EOPNOTSUPP;
>  
> +	if (!netif_carrier_ok(priv->dev))
> +		return -ENETDOWN;
> +

Now that you are configuring the hardware on link up, does that
matter?

	Andrew