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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Vineeth,

On Wed Sep 9, 2026 at 4:20 PM CEST, Vineeth Karumanchi wrote:
> Add Credit-Based Shaper (CBS/IEEE 802.1Qav) TC offload support for
> time-sensitive networking on GEM hardware. CBS is restricted to the
> two highest-priority queues: Queue A (num_queues - 1) and Queue B
> (num_queues - 2), matching hardware capability.
>
> Validate that idleslope is positive and does not exceed the link
> speed, preventing negative values from bypassing the bounds check
> due to signed-to-unsigned promotion.
>
> The idle slope register value is computed differently based on hardware
> variant:
>
>   High-speed GEM: scale idleslope linearly to the full 32-bit register
>   range relative to link speed.
>
>   Standard MACB: convert the kbps idleslope into the register's native
>   unit, which depends on the interface width:
>     - 1G (8-bit GMII):    bytes/sec,   scale kbps by 1000/8 (125)
>     - 10/100M (4-bit MII): nibbles/sec, scale kbps by 1000/4 (250)
>
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
> Changes in v2:
> - macb_cbs_get_queue_params() now returns the idleslope register offset
>   (u32 *idleslope_reg) instead of a bool flag, and the idleslope is
>   programmed via bp->macb_reg_writel(), dropping the per-queue if/else
>   that open-coded gem_writel(CBS_IDLESLOPE_Q_A/Q_B).
> - Expanded the idleslope kbps-to-hardware-unit conversion comment.
> - Zero-initialize kset in macb_cbs_add() so an unpopulated link speed
>   reads as 0 and is rejected; reorder locals to keep the declarations
>   in reverse-christmas-tree order.
> - Rebased on net-next, which renamed the struct net_device pointer to
>   "netdev" (was "dev"/"ndev").
>
>  drivers/net/ethernet/cadence/macb.h      |   9 ++
>  drivers/net/ethernet/cadence/macb_main.c | 116 +++++++++++++++++++++++
>  2 files changed, 125 insertions(+)
>
[...]
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 67150ff03066..00c1c619dea8 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> +static int macb_cbs_add(struct net_device *netdev,
> +			struct tc_cbs_qopt_offload *qopt)
> +{
[...]
> +	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 */
> +		bp->macb_reg_writel(bp, idleslope_reg, idleslope);
> +		/* Re-enable CBS for the queue with new idleslope */
> +		gem_writel(bp, CBS_CONTROL, ctrl | enable_bit);
> +	}
> +
> +	netdev_dbg(netdev, "CBS: Configured queue %d with idleslope 0x%x\n",
> +		   qopt->queue, idleslope);
> +
> +	return 0;
> +}
> +
> +static void macb_cbs_destroy(struct net_device *netdev, u8 queue_num)
> +{
> +	struct macb *bp = netdev_priv(netdev);
> +	u32 enable_bit, idleslope_reg;
> +
> +	if (macb_cbs_get_queue_params(bp, queue_num, &enable_bit, &idleslope_reg))
> +		return;
> +
> +	scoped_guard(spinlock_irqsave, &bp->lock) {
> +		gem_writel(bp, CBS_CONTROL, gem_readl(bp, CBS_CONTROL) & ~enable_bit);
> +		bp->macb_reg_writel(bp, idleslope_reg, 0);
> +	}
> +
> +	netdev_dbg(netdev, "CBS: Disabled queue %d\n", queue_num);
> +}

So we write cbs_control/0x04BC only on ndo_setup_tc(TC_SETUP_QDISC_CBS)
callback. As Sashiko pointed how I think that causes an issue with HW
resets & suspend:
 - what if configured then link down then link up?
 - what if configured before link up?

Also when not using CBS we hope tx_sched_ctrl/0x0580 is at its reset
value (0b00, fixed priority) for all queues. I would expect an
unconditional writel tx_sched_ctrl/0x0580 in macb_init_hw(). This
solves the down/up issue, the speed change and also ensures we don't
inherit CBS or DWRR from the boot stages (super unlikely though).

Also, it might be simpler to always use tx_sched_ctrl/0x0580 rather than
sometimes tx_sched_ctrl/0x0580 and sometimes cbs_control/0x04BC, which
are aliases.

What do you think?

[...]

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com