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

["Karumanchi, Vineeth" <vineeth@amd.com>]

Hi Conor,

On 8/7/2026 11:02 PM, Conor Dooley wrote:
> 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?
> 

yes, agreed. This approach initially enabled optimization of the idle
slope update path. However I have found a better approach and will
include it in the next revision (v2).

>> +		*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.
> 

OK.

Thanks,
Vineeth

> 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
>>