Re: [PATCH net-next 4/4] net: macb: Add TSN CBS TC offload support

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4622 bytes --]

On Fri, Aug 07, 2026 at 03:20:12PM +0530, Vineeth Karumanchi wrote:
> +static int macb_cbs_get_queue_params(struct macb *bp, u8 queue_num,
> +				     u32 *enable_bit, bool *is_queue_a)
> +{
> +	/* Queue A is highest priority (num_queues - 1) */
> +	if (queue_num == bp->num_queues - 1) {
> +		*enable_bit = GEM_BIT(CBS_ENABLE_QUEUE_A);
> +		*is_queue_a = true;
> +		return 0;
> +	}
> +
> +	/* Queue B is second highest priority (num_queues - 2) */
> +	if (queue_num == bp->num_queues - 2) {
> +		*enable_bit = GEM_BIT(CBS_ENABLE_QUEUE_B);

What's the point of making enable_bit a parameter if everything you do
using it bounds a conditional section gated on is_queue_a?

> +		*is_queue_a = false;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int macb_cbs_add(struct net_device *ndev,
> +			struct tc_cbs_qopt_offload *qopt)
> +{
> +	u32 enable_bit, idleslope, speed_kbps, ctrl;
> +	struct macb *bp = netdev_priv(ndev);
> +	struct ethtool_link_ksettings kset;
> +	bool is_queue_a;
> +	int err;
> +
> +	err = macb_cbs_get_queue_params(bp, qopt->queue, &enable_bit, &is_queue_a);
> +	if (err) {
> +		netdev_err(ndev, "CBS: Queue %d not eligible (only top 2 queues support CBS)\n",
> +			   qopt->queue);
> +		return -EINVAL;
> +	}
> +
> +	/* idleslope is calibrated for the current link speed; CBS is not
> +	 * reprogrammed on link-speed changes, so it must be reconfigured
> +	 * if the link speed changes.
> +	 */
> +	phylink_ethtool_ksettings_get(bp->phylink, &kset);
> +
> +	if (!kset.base.speed || kset.base.speed == SPEED_UNKNOWN) {
> +		netdev_err(ndev, "CBS: Invalid link speed\n");
> +		return -EINVAL;
> +	}
> +
> +	speed_kbps = kset.base.speed * 1000;
> +
> +	if (qopt->idleslope <= 0 || (u32)qopt->idleslope > speed_kbps) {
> +		netdev_err(ndev, "CBS: invalid idleslope %d (must be 1..%u kbps)\n",
> +			   qopt->idleslope, speed_kbps);
> +		return -EINVAL;
> +	}
> +
> +	/* Calculate idleslope for hardware register:
> +	 * - High-speed GEM: scale to full 32-bit register range

> +	 * - Standard MACB: multiply by port transmit rate factor

I think this comment should probably mention that the register expects
bytes/sec in 1G mode and nibbles/sec in 10/100.

This generally looks sane to my naive eyes otherwise.

Thanks,
Conor.

> +	 */
> +	if (bp->caps & MACB_CAPS_HIGH_SPEED)
> +		idleslope = DIV_ROUND_UP_ULL((u64)qopt->idleslope * U32_MAX, speed_kbps);
> +	else
> +		idleslope = (u32)qopt->idleslope * (kset.base.speed >= 1000 ?
> +					       MACB_CBS_PORT_RATE_1G : MACB_CBS_PORT_RATE_10_100M);
> +
> +	scoped_guard(spinlock_irqsave, &bp->lock) {
> +		/* Disable CBS for the queue before updating idleslope */
> +		ctrl = gem_readl(bp, CBS_CONTROL) & ~enable_bit;
> +		gem_writel(bp, CBS_CONTROL, ctrl);
> +		/* Update idleslope for the queue */
> +		if (is_queue_a)
> +			gem_writel(bp, CBS_IDLESLOPE_Q_A, idleslope);
> +		else
> +			gem_writel(bp, CBS_IDLESLOPE_Q_B, idleslope);
> +
> +		/* Re-enable CBS for the queue with new idleslope */
> +		gem_writel(bp, CBS_CONTROL, ctrl | enable_bit);
> +	}
> +
> +	netdev_dbg(ndev, "CBS: Configured queue %d with idleslope 0x%x\n",
> +		   qopt->queue, idleslope);
> +
> +	return 0;
> +}
> +
> +static void macb_cbs_destroy(struct net_device *ndev, u8 queue_num)
> +{
> +	struct macb *bp = netdev_priv(ndev);
> +	bool is_queue_a;
> +	u32 enable_bit;
> +
> +	if (macb_cbs_get_queue_params(bp, queue_num, &enable_bit, &is_queue_a))
> +		return;
> +
> +	scoped_guard(spinlock_irqsave, &bp->lock) {
> +		gem_writel(bp, CBS_CONTROL, gem_readl(bp, CBS_CONTROL) & ~enable_bit);
> +		if (is_queue_a)
> +			gem_writel(bp, CBS_IDLESLOPE_Q_A, 0);
> +		else
> +			gem_writel(bp, CBS_IDLESLOPE_Q_B, 0);
> +	}
> +
> +	netdev_dbg(ndev, "CBS: Disabled queue %d\n", queue_num);
> +}
> +
> +static int macb_setup_cbs(struct net_device *ndev,
> +			  struct tc_cbs_qopt_offload *qopt)
> +{
> +	if (qopt->enable)
> +		return macb_cbs_add(ndev, qopt);
> +
> +	macb_cbs_destroy(ndev, qopt->queue);
> +	return 0;
> +}
> +
>  static int macb_setup_mqprio(struct net_device *ndev,
>  			     struct tc_mqprio_qopt_offload *mqprio)
>  {
> @@ -4594,6 +4713,8 @@ static int macb_setup_tc(struct net_device *dev, enum tc_setup_type type,
>  	switch (type) {
>  	case TC_SETUP_QDISC_MQPRIO:
>  		return macb_setup_mqprio(dev, type_data);
> +	case TC_SETUP_QDISC_CBS:
> +		return macb_setup_cbs(dev, type_data);
>  	case TC_SETUP_QDISC_TAPRIO:
>  		return macb_setup_taprio(dev, type_data);
>  	default:
> -- 
> 2.44.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]