netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/6] net: macb: Add TAPRIO traffic scheduling support
@ 2025-07-22 15:41 Vineeth Karumanchi
  2025-07-22 15:41 ` [PATCH net-next 1/6] net: macb: Define ENST hardware registers for time-aware scheduling Vineeth Karumanchi
                   ` (5 more replies)
  0 siblings, 6 replies; 22+ messages in thread
From: Vineeth Karumanchi @ 2025-07-22 15:41 UTC (permalink / raw)
  To: nicolas.ferre, claudiu.beznea, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel, vineeth.karumanchi

Implement Time-Aware Traffic Scheduling (TAPRIO) offload support
for Cadence MACB/GEM ethernet controllers to enable IEEE 802.1Qbv
compliant time-sensitive networking (TSN) capabilities.

Key features implemented:
- Complete TAPRIO qdisc offload infrastructure with TC_SETUP_QDISC_TAPRIO
- Hardware-accelerated time-based gate control for multiple queues
- Enhanced Scheduled Traffic (ENST) register configuration and management
- Gate state scheduling with configurable start times, on/off intervals
- Support for cycle-time based traffic scheduling with validation
- Hardware capability detection via MACB_CAPS_QBV flag
- Robust error handling and parameter validation
- Queue-specific timing register programming
  (ENST_START_TIME, ENST_ON_TIME, ENST_OFF_TIME)

Changes include:
- Add macb_taprio_setup_replace() for TAPRIO configuration
- Add macb_taprio_destroy() for cleanup and reset
- Add macb_setup_tc() as TC offload entry point
- Enable NETIF_F_HW_TC feature for QBV-capable hardware
- Add ENST register offsets to queue configuration

The implementation validates timing constraints against hardware limits,
supports per-queue gate mask configuration, and provides comprehensive
logging for debugging and monitoring. Hardware registers are programmed
atomically with proper locking to ensure consistent state.

Tested on Xilinx Versal platforms with QBV-capable MACB controllers.

Vineeth Karumanchi (6):
  net: macb: Define ENST hardware registers for time-aware scheduling
  net: macb: Integrate ENST timing parameters and hardware unit
    conversion
  net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support
  net: macb: Implement TAPRIO DESTROY command offload for gate cleanup
  net: macb: Implement TAPRIO TC offload command interface
  net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support

 drivers/net/ethernet/cadence/macb.h      |  76 ++++++++
 drivers/net/ethernet/cadence/macb_main.c | 228 ++++++++++++++++++++++-
 2 files changed, 303 insertions(+), 1 deletion(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH net-next 1/6] net: macb: Define ENST hardware registers for time-aware scheduling
  2025-07-22 15:41 [PATCH net-next 0/6] net: macb: Add TAPRIO traffic scheduling support Vineeth Karumanchi
@ 2025-07-22 15:41 ` Vineeth Karumanchi
  2025-07-26 12:23   ` claudiu beznea (tuxon)
  2025-07-22 15:41 ` [PATCH net-next 2/6] net: macb: Integrate ENST timing parameters and hardware unit conversion Vineeth Karumanchi
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 22+ messages in thread
From: Vineeth Karumanchi @ 2025-07-22 15:41 UTC (permalink / raw)
  To: nicolas.ferre, claudiu.beznea, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel, vineeth.karumanchi

Add ENST (Enhanced Network Scheduling and Timing) register definitions
to support IEEE 802.1Qbv time-gated transmission.

Register architecture:
- Per-queue timing registers: ENST_START_TIME, ENST_ON_TIME, ENST_OFF_TIME
- Centralized control of the ENST_CONTROL register for enabling or
  disabling queue gates.
- Time intervals programmed in hardware byte units
- Hardware-level queue scheduling infrastructure.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb.h | 43 +++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index c9a5c8beb2fa..e456ac65d6c6 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -184,6 +184,13 @@
 #define GEM_DCFG8		0x029C /* Design Config 8 */
 #define GEM_DCFG10		0x02A4 /* Design Config 10 */
 #define GEM_DCFG12		0x02AC /* Design Config 12 */
+#define GEM_ENST_START_TIME_Q0	0x0800 /* ENST Q0 start time */
+#define GEM_ENST_START_TIME_Q1	0x0804 /* ENST Q1 start time */
+#define GEM_ENST_ON_TIME_Q0	0x0820 /* ENST Q0 on time */
+#define GEM_ENST_ON_TIME_Q1	0x0824 /* ENST Q1 on time */
+#define GEM_ENST_OFF_TIME_Q0	0x0840 /* ENST Q0 off time */
+#define GEM_ENST_OFF_TIME_Q1	0x0844 /* ENST Q1 off time */
+#define GEM_ENST_CONTROL	0x0880 /* ENST control register */
 #define GEM_USX_CONTROL		0x0A80 /* High speed PCS control register */
 #define GEM_USX_STATUS		0x0A88 /* High speed PCS status register */
 
@@ -221,6 +228,15 @@
 #define GEM_IDR(hw_q)		(0x0620 + ((hw_q) << 2))
 #define GEM_IMR(hw_q)		(0x0640 + ((hw_q) << 2))
 
+#define GEM_ENST_START_TIME(hw_q)	(0x0800 + ((hw_q) << 2))
+#define GEM_ENST_ON_TIME(hw_q)		(0x0820 + ((hw_q) << 2))
+#define GEM_ENST_OFF_TIME(hw_q)		(0x0840 + ((hw_q) << 2))
+
+/*  Bitfields in ENST_CONTROL. */
+#define GEM_ENST_DISABLE_QUEUE(hw_q)	BIT((hw_q) + 16) /* q0 disable is 16'b */
+#define GEM_ENST_DISABLE_QUEUE_OFFSET	16
+#define GEM_ENST_ENABLE_QUEUE(hw_q)		BIT(hw_q) /* q0 enable is 0'b */
+
 /* Bitfields in NCR */
 #define MACB_LB_OFFSET		0 /* reserved */
 #define MACB_LB_SIZE		1
@@ -554,6 +570,33 @@
 #define GEM_HIGH_SPEED_OFFSET			26
 #define GEM_HIGH_SPEED_SIZE			1
 
+/* Bitfields in ENST_START_TIME_Q0, Q1. */
+#define GEM_START_TIME_SEC_OFFSET		30
+#define GEM_START_TIME_SEC_SIZE			2
+#define GEM_START_TIME_NSEC_OFFSET		0
+#define GEM_START_TIME_NSEC_SIZE		30
+
+/* Bitfields in ENST_ON_TIME_Q0, Q1. */
+#define GEM_ON_TIME_OFFSET			0
+#define GEM_ON_TIME_SIZE			17
+
+/* Bitfields in ENST_OFF_TIME_Q0, Q1. */
+#define GEM_OFF_TIME_OFFSET			0
+#define GEM_OFF_TIME_SIZE			17
+
+/* Hardware ENST timing registers granularity */
+#define ENST_TIME_GRANULARITY_NS 8
+
+/* Bitfields in ENST_CONTROL. */
+#define GEM_DISABLE_Q1_OFFSET			17
+#define GEM_DISABLE_Q1_SIZE			1
+#define GEM_DISABLE_Q0_OFFSET			16
+#define GEM_DISABLE_Q0_SIZE			1
+#define GEM_ENABLE_Q1_OFFSET			1
+#define GEM_ENABLE_Q1_SIZE			1
+#define GEM_ENABLE_Q0_OFFSET			0
+#define GEM_ENABLE_Q0_SIZE			1
+
 /* Bitfields in USX_CONTROL. */
 #define GEM_USX_CTRL_SPEED_OFFSET		14
 #define GEM_USX_CTRL_SPEED_SIZE			3
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH net-next 2/6] net: macb: Integrate ENST timing parameters and hardware unit conversion
  2025-07-22 15:41 [PATCH net-next 0/6] net: macb: Add TAPRIO traffic scheduling support Vineeth Karumanchi
  2025-07-22 15:41 ` [PATCH net-next 1/6] net: macb: Define ENST hardware registers for time-aware scheduling Vineeth Karumanchi
@ 2025-07-22 15:41 ` Vineeth Karumanchi
  2025-07-26 12:24   ` claudiu beznea (tuxon)
  2025-07-22 15:41 ` [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support Vineeth Karumanchi
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 22+ messages in thread
From: Vineeth Karumanchi @ 2025-07-22 15:41 UTC (permalink / raw)
  To: nicolas.ferre, claudiu.beznea, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel, vineeth.karumanchi

Add Enhanced Network Scheduling and Timing (ENST) support to
queue infrastructure with speed-dependent timing calculations for
precise gate control.

Hardware timing unit conversion:
- Timing values programmed as hardware units based on link speed
- Conversion formula: time_bytes = time_ns / divisor
- Speed-specific divisors:
  * 1 Gbps:   divisor = 8
  * 100 Mbps: divisor = 80
  * 10 Mbps:  divisor = 800

Infrastructure changes:
- Extend macb_queue structure with ENST timing control registers
- Add queue_enst_configs structure for per-entry TC configuration storage
- Map ENST register offsets into existing queue management framework
- Define ENST_NS_TO_HW_UNITS() macro for automatic speed-based conversion

This enables hardware-native timing programming while abstracting the
speed-dependent conversions

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb.h      | 32 ++++++++++++++++++++++++
 drivers/net/ethernet/cadence/macb_main.c |  6 +++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index e456ac65d6c6..ef3995564c5c 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -857,6 +857,16 @@
 
 #define MACB_READ_NSR(bp)	macb_readl(bp, NSR)
 
+/* ENST macros*/
+#define ENST_NS_TO_HW_UNITS(ns, speed_mbps) \
+		DIV_ROUND_UP((ns) * (speed_mbps), (ENST_TIME_GRANULARITY_NS * 1000))
+
+#define ENST_MAX_HW_INTERVAL(speed_mbps) \
+		DIV_ROUND_UP(GENMASK(GEM_ON_TIME_SIZE - 1, 0) * ENST_TIME_GRANULARITY_NS * 1000,\
+		(speed_mbps))
+
+#define ENST_MAX_START_TIME_SEC GENMASK(GEM_START_TIME_SEC_SIZE - 1, 0)
+
 /* struct macb_dma_desc - Hardware DMA descriptor
  * @addr: DMA address of data buffer
  * @ctrl: Control and status bits
@@ -1262,6 +1272,11 @@ struct macb_queue {
 	unsigned int		RBQP;
 	unsigned int		RBQPH;
 
+	/* ENST register offsets for this queue */
+	unsigned int		ENST_START_TIME;
+	unsigned int		ENST_ON_TIME;
+	unsigned int		ENST_OFF_TIME;
+
 	/* Lock to protect tx_head and tx_tail */
 	spinlock_t		tx_ptr_lock;
 	unsigned int		tx_head, tx_tail;
@@ -1450,4 +1465,21 @@ struct macb_platform_data {
 	struct clk	*hclk;
 };
 
+/**
+ * struct queue_enst_configs - Configuration for Enhanced Scheduled Traffic (ENST) queue
+ * @queue_id:         Identifier for the queue
+ * @start_time_mask:  Bitmask representing the start time for the queue
+ * @on_time_bytes:    "on" time nsec expressed in bytes
+ * @off_time_bytes:   "off" time nsec expressed in bytes
+ *
+ * This structure holds the configuration parameters for an ENST queue,
+ * used to control time-based transmission scheduling in the MACB driver.
+ */
+struct queue_enst_configs {
+	u8 queue_id;
+	u32 start_time_mask;
+	u32 on_time_bytes;
+	u32 off_time_bytes;
+};
+
 #endif /* _MACB_H */
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ce95fad8cedd..ff87d3e1d8a0 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4305,6 +4305,9 @@ static int macb_init(struct platform_device *pdev)
 			queue->TBQP = GEM_TBQP(hw_q - 1);
 			queue->RBQP = GEM_RBQP(hw_q - 1);
 			queue->RBQS = GEM_RBQS(hw_q - 1);
+			queue->ENST_START_TIME = GEM_ENST_START_TIME(hw_q);
+			queue->ENST_ON_TIME = GEM_ENST_ON_TIME(hw_q);
+			queue->ENST_OFF_TIME = GEM_ENST_OFF_TIME(hw_q);
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
 				queue->TBQPH = GEM_TBQPH(hw_q - 1);
@@ -4319,6 +4322,9 @@ static int macb_init(struct platform_device *pdev)
 			queue->IMR  = MACB_IMR;
 			queue->TBQP = MACB_TBQP;
 			queue->RBQP = MACB_RBQP;
+			queue->ENST_START_TIME = GEM_ENST_START_TIME(0);
+			queue->ENST_ON_TIME = GEM_ENST_ON_TIME(0);
+			queue->ENST_OFF_TIME = GEM_ENST_OFF_TIME(0);
 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
 				queue->TBQPH = MACB_TBQPH;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support
  2025-07-22 15:41 [PATCH net-next 0/6] net: macb: Add TAPRIO traffic scheduling support Vineeth Karumanchi
  2025-07-22 15:41 ` [PATCH net-next 1/6] net: macb: Define ENST hardware registers for time-aware scheduling Vineeth Karumanchi
  2025-07-22 15:41 ` [PATCH net-next 2/6] net: macb: Integrate ENST timing parameters and hardware unit conversion Vineeth Karumanchi
@ 2025-07-22 15:41 ` Vineeth Karumanchi
  2025-07-23 10:02   ` kernel test robot
  2025-07-26 12:25   ` claudiu beznea (tuxon)
  2025-07-22 15:41 ` [PATCH net-next 4/6] net: macb: Implement TAPRIO DESTROY command offload for gate cleanup Vineeth Karumanchi
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 22+ messages in thread
From: Vineeth Karumanchi @ 2025-07-22 15:41 UTC (permalink / raw)
  To: nicolas.ferre, claudiu.beznea, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel, vineeth.karumanchi

Implement Time-Aware Traffic Scheduling (TAPRIO) hardware offload for
"tc qdisc replace" operations, enabling IEEE 802.1Qbv compliant gate
scheduling on Cadence MACB/GEM controllers.

Parameter validation checks performed:
- Queue count bounds checking (1 < queues <= MACB_MAX_QUEUES)
- TC entry limit validation against available hardware queues
- Base time non-negativity enforcement
- Speed-adaptive timing constraint verification
- Cycle time vs. total gate time consistency checks
- Single-queue gate mask enforcement per scheduling entry

Hardware programming sequence:
- GEM doesn't support changing register values if ENST is running,
  hence disable ENST before programming
- Atomic timing register configuration (START_TIME, ON_TIME, OFF_TIME)
- Enable the configured queues via ENST_CONTROL register

This implementation ensures deterministic gate scheduling while preventing
invalid configurations.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 155 +++++++++++++++++++++++
 1 file changed, 155 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ff87d3e1d8a0..4518b59168d5 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -36,6 +36,7 @@
 #include <linux/reset.h>
 #include <linux/firmware/xlnx-zynqmp.h>
 #include <linux/inetdevice.h>
+#include <net/pkt_sched.h>
 #include "macb.h"
 
 /* This structure is only used for MACB on SiFive FU540 devices */
@@ -4084,6 +4085,160 @@ static void macb_restore_features(struct macb *bp)
 	macb_set_rxflow_feature(bp, features);
 }
 
+static int macb_taprio_setup_replace(struct net_device *ndev,
+				     struct tc_taprio_qopt_offload *conf)
+{
+	u64 total_on_time = 0, start_time_sec = 0, start_time = conf->base_time;
+	struct queue_enst_configs  *enst_queue;
+	u32 configured_queues = 0, speed = 0;
+	struct tc_taprio_sched_entry *entry;
+	struct macb *bp = netdev_priv(ndev);
+	struct ethtool_link_ksettings kset;
+	struct macb_queue *queue;
+	unsigned long flags;
+	int err = 0, i;
+
+	/* Validate queue configuration */
+	if (bp->num_queues < 1 || bp->num_queues > MACB_MAX_QUEUES) {
+		netdev_err(ndev, "Invalid number of queues: %d\n", bp->num_queues);
+		return -EINVAL;
+	}
+
+	if (conf->num_entries > bp->num_queues) {
+		netdev_err(ndev, "Too many TAPRIO entries: %lu > %d queues\n",
+			   conf->num_entries, bp->num_queues);
+		return -EINVAL;
+	}
+
+	if (start_time < 0) {
+		netdev_err(ndev, "Invalid base_time: must be 0 or positive, got %lld\n",
+			   conf->base_time);
+		return -ERANGE;
+	}
+
+	/* Get the current link speed */
+	err = phylink_ethtool_ksettings_get(bp->phylink, &kset);
+	if (unlikely(err)) {
+		netdev_err(ndev, "Failed to get link settings: %d\n", err);
+		return err;
+	}
+
+	speed = kset.base.speed;
+	if (unlikely(speed <= 0)) {
+		netdev_err(ndev, "Invalid speed: %d\n", speed);
+		return -EINVAL;
+	}
+
+	enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), GFP_KERNEL);
+	if (!enst_queue)
+		return -ENOMEM;
+
+	/* Pre-validate all entries before making any hardware changes */
+	for (i = 0; i < conf->num_entries; i++) {
+		entry = &conf->entries[i];
+
+		if (entry->command != TC_TAPRIO_CMD_SET_GATES) {
+			netdev_err(ndev, "Entry %d: unsupported command %d\n",
+				   i, entry->command);
+			err = -EOPNOTSUPP;
+			goto cleanup;
+		}
+
+		/* Validate gate_mask: must be nonzero, single queue, and within range */
+		if (!is_power_of_2(entry->gate_mask)) {
+			netdev_err(ndev, "Entry %d: gate_mask 0x%x is not a power of 2 (only one queue per entry allowed)\n",
+				   i, entry->gate_mask);
+			err = -EINVAL;
+			goto cleanup;
+		}
+
+		/* gate_mask must not select queues outside the valid queue_mask */
+		if (entry->gate_mask & ~bp->queue_mask) {
+			netdev_err(ndev, "Entry %d: gate_mask 0x%x exceeds queue range (max_queues=%d)\n",
+				   i, entry->gate_mask, bp->num_queues);
+			err = -EINVAL;
+			goto cleanup;
+		}
+
+		/* Check for start time limits */
+		start_time_sec = div_u64(start_time, NSEC_PER_SEC);
+		if (start_time_sec > ENST_MAX_START_TIME_SEC) {
+			netdev_err(ndev, "Entry %d: Start time %llu s exceeds hardware limit\n",
+				   i, start_time_sec);
+			err = -ERANGE;
+			goto cleanup;
+		}
+
+		/* Check for on time limit*/
+		if (entry->interval > ENST_MAX_HW_INTERVAL(speed)) {
+			netdev_err(ndev, "Entry %d: interval %u ns exceeds hardware limit %lu ns\n",
+				   i, entry->interval, ENST_MAX_HW_INTERVAL(speed));
+			err = -ERANGE;
+			goto cleanup;
+		}
+
+		/* Check for off time limit*/
+		if ((conf->cycle_time - entry->interval) > ENST_MAX_HW_INTERVAL(speed)) {
+			netdev_err(ndev, "Entry %d: off_time %llu ns exceeds hardware limit %lu ns\n",
+				   i, conf->cycle_time - entry->interval,
+				   ENST_MAX_HW_INTERVAL(speed));
+			err = -ERANGE;
+			goto cleanup;
+		}
+
+		enst_queue[i].queue_id = order_base_2(entry->gate_mask);
+		enst_queue[i].start_time_mask =
+			(start_time_sec << GEM_START_TIME_SEC_OFFSET) |
+				  (start_time % NSEC_PER_SEC);
+		enst_queue[i].on_time_bytes =
+			ENST_NS_TO_HW_UNITS(entry->interval, speed);
+		enst_queue[i].off_time_bytes =
+			ENST_NS_TO_HW_UNITS(conf->cycle_time - entry->interval, speed);
+
+		configured_queues |= entry->gate_mask;
+		total_on_time += entry->interval;
+		start_time += entry->interval;
+	}
+
+	/* Check total interval doesn't exceed cycle time */
+	if (total_on_time > conf->cycle_time) {
+		netdev_err(ndev, "Total ON %llu ns exceeds cycle time %llu ns\n",
+			   total_on_time, conf->cycle_time);
+		err = -EINVAL;
+		goto cleanup;
+	}
+
+	netdev_dbg(ndev, "TAPRIO setup: %lu entries, base_time=%lld ns, cycle_time=%llu ns\n",
+		   conf->num_entries, conf->base_time, conf->cycle_time);
+
+	/* All validations passed - proceed with hardware configuration */
+	spin_lock_irqsave(&bp->lock, flags);
+
+	/* Disable ENST queues if running before configuring */
+	if (gem_readl(bp, ENST_CONTROL))
+		gem_writel(bp, ENST_CONTROL,
+			   GENMASK(bp->num_queues - 1, 0) << GEM_ENST_DISABLE_QUEUE_OFFSET);
+
+	for (i = 0; i < conf->num_entries; i++) {
+		queue = &bp->queues[enst_queue[i].queue_id];
+		/* Configure queue timing registers */
+		queue_writel(queue, ENST_START_TIME, enst_queue[i].start_time_mask);
+		queue_writel(queue, ENST_ON_TIME, enst_queue[i].on_time_bytes);
+		queue_writel(queue, ENST_OFF_TIME, enst_queue[i].off_time_bytes);
+	}
+
+	/* Enable ENST for all configured queues in one write */
+	gem_writel(bp, ENST_CONTROL, configured_queues);
+	spin_unlock_irqrestore(&bp->lock, flags);
+
+	netdev_info(ndev, "TAPRIO configuration completed successfully: %lu entries, %d queues configured\n",
+		    conf->num_entries, hweight32(configured_queues));
+
+cleanup:
+	kfree(enst_queue);
+	return err;
+}
+
 static const struct net_device_ops macb_netdev_ops = {
 	.ndo_open		= macb_open,
 	.ndo_stop		= macb_close,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH net-next 4/6] net: macb: Implement TAPRIO DESTROY command offload for gate cleanup
  2025-07-22 15:41 [PATCH net-next 0/6] net: macb: Add TAPRIO traffic scheduling support Vineeth Karumanchi
                   ` (2 preceding siblings ...)
  2025-07-22 15:41 ` [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support Vineeth Karumanchi
@ 2025-07-22 15:41 ` Vineeth Karumanchi
  2025-07-26 12:26   ` claudiu beznea (tuxon)
  2025-07-22 15:41 ` [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface Vineeth Karumanchi
  2025-07-22 15:41 ` [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support Vineeth Karumanchi
  5 siblings, 1 reply; 22+ messages in thread
From: Vineeth Karumanchi @ 2025-07-22 15:41 UTC (permalink / raw)
  To: nicolas.ferre, claudiu.beznea, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel, vineeth.karumanchi

Add hardware offload support for "tc qdisc destroy" operations to safely
remove IEEE 802.1Qbv time-gated scheduling configuration and restore
default queue behavior.

Cleanup sequence:
- Reset network device TC configuration state
- Disable Enhanced Network Scheduling and Timing for all queues
- Clear all ENST timing control registers (START_TIME, ON_TIME, OFF_TIME)
- Atomic register programming with proper synchronization

This ensures complete removal of time-aware scheduling state, returning
the controller to standard FIFO queue operation without residual timing
constraints

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 28 ++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 4518b59168d5..6b3eff28a842 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4239,6 +4239,34 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
 	return err;
 }
 
+static void macb_taprio_destroy(struct net_device *ndev)
+{
+	struct macb *bp = netdev_priv(ndev);
+	struct macb_queue *queue;
+	unsigned long flags;
+	u32 enst_disable_mask;
+	u8 i;
+
+	netdev_reset_tc(ndev);
+	enst_disable_mask = GENMASK(bp->num_queues - 1, 0) << GEM_ENST_DISABLE_QUEUE_OFFSET;
+	netdev_dbg(ndev, "TAPRIO destroy: disabling all gates\n");
+
+	spin_lock_irqsave(&bp->lock, flags);
+
+	/* Single disable command for all queues */
+	gem_writel(bp, ENST_CONTROL, enst_disable_mask);
+
+	/* Clear all queue ENST registers in batch */
+	for (i = 0; i < bp->num_queues; i++) {
+		queue = &bp->queues[i];
+		queue_writel(queue, ENST_START_TIME, 0);
+		queue_writel(queue, ENST_ON_TIME, 0);
+		queue_writel(queue, ENST_OFF_TIME, 0);
+	}
+
+	spin_unlock_irqrestore(&bp->lock, flags);
+}
+
 static const struct net_device_ops macb_netdev_ops = {
 	.ndo_open		= macb_open,
 	.ndo_stop		= macb_close,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface
  2025-07-22 15:41 [PATCH net-next 0/6] net: macb: Add TAPRIO traffic scheduling support Vineeth Karumanchi
                   ` (3 preceding siblings ...)
  2025-07-22 15:41 ` [PATCH net-next 4/6] net: macb: Implement TAPRIO DESTROY command offload for gate cleanup Vineeth Karumanchi
@ 2025-07-22 15:41 ` Vineeth Karumanchi
  2025-07-26 12:29   ` claudiu beznea (tuxon)
  2025-07-28 16:31   ` kernel test robot
  2025-07-22 15:41 ` [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support Vineeth Karumanchi
  5 siblings, 2 replies; 22+ messages in thread
From: Vineeth Karumanchi @ 2025-07-22 15:41 UTC (permalink / raw)
  To: nicolas.ferre, claudiu.beznea, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel, vineeth.karumanchi

Add Traffic Control offload infrastructure with command routing for
TAPRIO qdisc operations:

- macb_setup_taprio(): TAPRIO command dispatcher
- macb_setup_tc(): TC_SETUP_QDISC_TAPRIO entry point
- Support for REPLACE/DESTROY command mapping

Provides standardized TC interface for time-gated scheduling control.

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 33 ++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 6b3eff28a842..cc33491930e3 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4267,6 +4267,38 @@ static void macb_taprio_destroy(struct net_device *ndev)
 	spin_unlock_irqrestore(&bp->lock, flags);
 }
 
+static int macb_setup_taprio(struct net_device *ndev,
+			     struct tc_taprio_qopt_offload *taprio)
+{
+	int err = 0;
+
+	switch (taprio->cmd) {
+	case TAPRIO_CMD_REPLACE:
+		err = macb_taprio_setup_replace(ndev, taprio);
+		break;
+	case TAPRIO_CMD_DESTROY:
+		macb_taprio_destroy(ndev);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+	}
+
+	return err;
+}
+
+static int macb_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
+{
+	if (!dev || !type_data)
+		return -EINVAL;
+
+	switch (type) {
+	case TC_SETUP_QDISC_TAPRIO:
+		return macb_setup_taprio(dev, type_data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static const struct net_device_ops macb_netdev_ops = {
 	.ndo_open		= macb_open,
 	.ndo_stop		= macb_close,
@@ -4284,6 +4316,7 @@ static const struct net_device_ops macb_netdev_ops = {
 	.ndo_features_check	= macb_features_check,
 	.ndo_hwtstamp_set	= macb_hwtstamp_set,
 	.ndo_hwtstamp_get	= macb_hwtstamp_get,
+	.ndo_setup_tc		= macb_setup_tc,
 };
 
 /* Configure peripheral capabilities according to device tree
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support
  2025-07-22 15:41 [PATCH net-next 0/6] net: macb: Add TAPRIO traffic scheduling support Vineeth Karumanchi
                   ` (4 preceding siblings ...)
  2025-07-22 15:41 ` [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface Vineeth Karumanchi
@ 2025-07-22 15:41 ` Vineeth Karumanchi
  2025-07-23 19:05   ` Simon Horman
  2025-07-24  0:46   ` kernel test robot
  5 siblings, 2 replies; 22+ messages in thread
From: Vineeth Karumanchi @ 2025-07-22 15:41 UTC (permalink / raw)
  To: nicolas.ferre, claudiu.beznea, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel, vineeth.karumanchi

The "exclude_qbv" bit in designcfg_debug1 register varies between MACB/GEM
IP revisions, making direct register probing unreliable for
feature detection. A capability-based approach provides consistent
QBV support identification across the IP family

Platform support:
- Enable MACB_CAPS_QBV for Xilinx Versal platform configuration
- Foundation for QBV feature detection in TAPRIO implementation

Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb.h      | 1 +
 drivers/net/ethernet/cadence/macb_main.c | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index ef3995564c5c..4e8d5dcc814e 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -782,6 +782,7 @@
 #define MACB_CAPS_MIIONRGMII			0x00000200
 #define MACB_CAPS_NEED_TSUCLK			0x00000400
 #define MACB_CAPS_QUEUE_DISABLE			0x00000800
+#define MACB_CAPS_QBV				0x00001000
 #define MACB_CAPS_PCS				0x01000000
 #define MACB_CAPS_HIGH_SPEED			0x02000000
 #define MACB_CAPS_CLK_HW_CHG			0x04000000
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index cc33491930e3..98e56697661c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4601,6 +4601,10 @@ static int macb_init(struct platform_device *pdev)
 		dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
 	if (bp->caps & MACB_CAPS_SG_DISABLED)
 		dev->hw_features &= ~NETIF_F_SG;
+	/* Enable HW_TC if hardware supports QBV */
+	if (bp->caps & MACB_CAPS_QBV)
+		dev->hw_features |= NETIF_F_HW_TC;
+
 	dev->features = dev->hw_features;
 
 	/* Check RX Flow Filters support.
@@ -5345,7 +5349,7 @@ static const struct macb_config sama7g5_emac_config = {
 static const struct macb_config versal_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
 		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK |
-		MACB_CAPS_QUEUE_DISABLE,
+		MACB_CAPS_QUEUE_DISABLE, MACB_CAPS_QBV,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support
  2025-07-22 15:41 ` [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support Vineeth Karumanchi
@ 2025-07-23 10:02   ` kernel test robot
  2025-07-26 12:25   ` claudiu beznea (tuxon)
  1 sibling, 0 replies; 22+ messages in thread
From: kernel test robot @ 2025-07-23 10:02 UTC (permalink / raw)
  To: Vineeth Karumanchi, nicolas.ferre, claudiu.beznea, andrew+netdev,
	davem, edumazet, kuba, pabeni
  Cc: llvm, oe-kbuild-all, git, netdev, linux-kernel,
	vineeth.karumanchi

Hi Vineeth,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Vineeth-Karumanchi/net-macb-Define-ENST-hardware-registers-for-time-aware-scheduling/20250722-234618
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250722154111.1871292-4-vineeth.karumanchi%40amd.com
patch subject: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support
config: i386-randconfig-015-20250723 (https://download.01.org/0day-ci/archive/20250723/202507231739.LnkR6Gdd-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250723/202507231739.LnkR6Gdd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507231739.LnkR6Gdd-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/cadence/macb_main.c:4109:7: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
    4108 |                 netdev_err(ndev, "Too many TAPRIO entries: %lu > %d queues\n",
         |                                                            ~~~
         |                                                            %zu
    4109 |                            conf->num_entries, bp->num_queues);
         |                            ^~~~~~~~~~~~~~~~~
   drivers/net/ethernet/cadence/macb_main.c:4212:6: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
    4211 |         netdev_dbg(ndev, "TAPRIO setup: %lu entries, base_time=%lld ns, cycle_time=%llu ns\n",
         |                                         ~~~
         |                                         %zu
    4212 |                    conf->num_entries, conf->base_time, conf->cycle_time);
         |                    ^~~~~~~~~~~~~~~~~
   include/net/net_debug.h:66:46: note: expanded from macro 'netdev_dbg'
      66 |                 netdev_printk(KERN_DEBUG, __dev, format, ##args); \
         |                                                  ~~~~~~    ^~~~
   drivers/net/ethernet/cadence/macb_main.c:4235:7: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
    4234 |         netdev_info(ndev, "TAPRIO configuration completed successfully: %lu entries, %d queues configured\n",
         |                                                                         ~~~
         |                                                                         %zu
    4235 |                     conf->num_entries, hweight32(configured_queues));
         |                     ^~~~~~~~~~~~~~~~~
   drivers/net/ethernet/cadence/macb_main.c:4088:12: warning: unused function 'macb_taprio_setup_replace' [-Wunused-function]
    4088 | static int macb_taprio_setup_replace(struct net_device *ndev,
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~
   4 warnings generated.


vim +4109 drivers/net/ethernet/cadence/macb_main.c

  4087	
  4088	static int macb_taprio_setup_replace(struct net_device *ndev,
  4089					     struct tc_taprio_qopt_offload *conf)
  4090	{
  4091		u64 total_on_time = 0, start_time_sec = 0, start_time = conf->base_time;
  4092		struct queue_enst_configs  *enst_queue;
  4093		u32 configured_queues = 0, speed = 0;
  4094		struct tc_taprio_sched_entry *entry;
  4095		struct macb *bp = netdev_priv(ndev);
  4096		struct ethtool_link_ksettings kset;
  4097		struct macb_queue *queue;
  4098		unsigned long flags;
  4099		int err = 0, i;
  4100	
  4101		/* Validate queue configuration */
  4102		if (bp->num_queues < 1 || bp->num_queues > MACB_MAX_QUEUES) {
  4103			netdev_err(ndev, "Invalid number of queues: %d\n", bp->num_queues);
  4104			return -EINVAL;
  4105		}
  4106	
  4107		if (conf->num_entries > bp->num_queues) {
  4108			netdev_err(ndev, "Too many TAPRIO entries: %lu > %d queues\n",
> 4109				   conf->num_entries, bp->num_queues);
  4110			return -EINVAL;
  4111		}
  4112	
  4113		if (start_time < 0) {
  4114			netdev_err(ndev, "Invalid base_time: must be 0 or positive, got %lld\n",
  4115				   conf->base_time);
  4116			return -ERANGE;
  4117		}
  4118	
  4119		/* Get the current link speed */
  4120		err = phylink_ethtool_ksettings_get(bp->phylink, &kset);
  4121		if (unlikely(err)) {
  4122			netdev_err(ndev, "Failed to get link settings: %d\n", err);
  4123			return err;
  4124		}
  4125	
  4126		speed = kset.base.speed;
  4127		if (unlikely(speed <= 0)) {
  4128			netdev_err(ndev, "Invalid speed: %d\n", speed);
  4129			return -EINVAL;
  4130		}
  4131	
  4132		enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), GFP_KERNEL);
  4133		if (!enst_queue)
  4134			return -ENOMEM;
  4135	
  4136		/* Pre-validate all entries before making any hardware changes */
  4137		for (i = 0; i < conf->num_entries; i++) {
  4138			entry = &conf->entries[i];
  4139	
  4140			if (entry->command != TC_TAPRIO_CMD_SET_GATES) {
  4141				netdev_err(ndev, "Entry %d: unsupported command %d\n",
  4142					   i, entry->command);
  4143				err = -EOPNOTSUPP;
  4144				goto cleanup;
  4145			}
  4146	
  4147			/* Validate gate_mask: must be nonzero, single queue, and within range */
  4148			if (!is_power_of_2(entry->gate_mask)) {
  4149				netdev_err(ndev, "Entry %d: gate_mask 0x%x is not a power of 2 (only one queue per entry allowed)\n",
  4150					   i, entry->gate_mask);
  4151				err = -EINVAL;
  4152				goto cleanup;
  4153			}
  4154	
  4155			/* gate_mask must not select queues outside the valid queue_mask */
  4156			if (entry->gate_mask & ~bp->queue_mask) {
  4157				netdev_err(ndev, "Entry %d: gate_mask 0x%x exceeds queue range (max_queues=%d)\n",
  4158					   i, entry->gate_mask, bp->num_queues);
  4159				err = -EINVAL;
  4160				goto cleanup;
  4161			}
  4162	
  4163			/* Check for start time limits */
  4164			start_time_sec = div_u64(start_time, NSEC_PER_SEC);
  4165			if (start_time_sec > ENST_MAX_START_TIME_SEC) {
  4166				netdev_err(ndev, "Entry %d: Start time %llu s exceeds hardware limit\n",
  4167					   i, start_time_sec);
  4168				err = -ERANGE;
  4169				goto cleanup;
  4170			}
  4171	
  4172			/* Check for on time limit*/
  4173			if (entry->interval > ENST_MAX_HW_INTERVAL(speed)) {
  4174				netdev_err(ndev, "Entry %d: interval %u ns exceeds hardware limit %lu ns\n",
  4175					   i, entry->interval, ENST_MAX_HW_INTERVAL(speed));
  4176				err = -ERANGE;
  4177				goto cleanup;
  4178			}
  4179	
  4180			/* Check for off time limit*/
  4181			if ((conf->cycle_time - entry->interval) > ENST_MAX_HW_INTERVAL(speed)) {
  4182				netdev_err(ndev, "Entry %d: off_time %llu ns exceeds hardware limit %lu ns\n",
  4183					   i, conf->cycle_time - entry->interval,
  4184					   ENST_MAX_HW_INTERVAL(speed));
  4185				err = -ERANGE;
  4186				goto cleanup;
  4187			}
  4188	
  4189			enst_queue[i].queue_id = order_base_2(entry->gate_mask);
  4190			enst_queue[i].start_time_mask =
  4191				(start_time_sec << GEM_START_TIME_SEC_OFFSET) |
  4192					  (start_time % NSEC_PER_SEC);
  4193			enst_queue[i].on_time_bytes =
  4194				ENST_NS_TO_HW_UNITS(entry->interval, speed);
  4195			enst_queue[i].off_time_bytes =
  4196				ENST_NS_TO_HW_UNITS(conf->cycle_time - entry->interval, speed);
  4197	
  4198			configured_queues |= entry->gate_mask;
  4199			total_on_time += entry->interval;
  4200			start_time += entry->interval;
  4201		}
  4202	
  4203		/* Check total interval doesn't exceed cycle time */
  4204		if (total_on_time > conf->cycle_time) {
  4205			netdev_err(ndev, "Total ON %llu ns exceeds cycle time %llu ns\n",
  4206				   total_on_time, conf->cycle_time);
  4207			err = -EINVAL;
  4208			goto cleanup;
  4209		}
  4210	
  4211		netdev_dbg(ndev, "TAPRIO setup: %lu entries, base_time=%lld ns, cycle_time=%llu ns\n",
  4212			   conf->num_entries, conf->base_time, conf->cycle_time);
  4213	
  4214		/* All validations passed - proceed with hardware configuration */
  4215		spin_lock_irqsave(&bp->lock, flags);
  4216	
  4217		/* Disable ENST queues if running before configuring */
  4218		if (gem_readl(bp, ENST_CONTROL))
  4219			gem_writel(bp, ENST_CONTROL,
  4220				   GENMASK(bp->num_queues - 1, 0) << GEM_ENST_DISABLE_QUEUE_OFFSET);
  4221	
  4222		for (i = 0; i < conf->num_entries; i++) {
  4223			queue = &bp->queues[enst_queue[i].queue_id];
  4224			/* Configure queue timing registers */
  4225			queue_writel(queue, ENST_START_TIME, enst_queue[i].start_time_mask);
  4226			queue_writel(queue, ENST_ON_TIME, enst_queue[i].on_time_bytes);
  4227			queue_writel(queue, ENST_OFF_TIME, enst_queue[i].off_time_bytes);
  4228		}
  4229	
  4230		/* Enable ENST for all configured queues in one write */
  4231		gem_writel(bp, ENST_CONTROL, configured_queues);
  4232		spin_unlock_irqrestore(&bp->lock, flags);
  4233	
  4234		netdev_info(ndev, "TAPRIO configuration completed successfully: %lu entries, %d queues configured\n",
  4235			    conf->num_entries, hweight32(configured_queues));
  4236	
  4237	cleanup:
  4238		kfree(enst_queue);
  4239		return err;
  4240	}
  4241	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support
  2025-07-22 15:41 ` [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support Vineeth Karumanchi
@ 2025-07-23 19:05   ` Simon Horman
  2025-07-29  9:42     ` Karumanchi, Vineeth
  2025-07-24  0:46   ` kernel test robot
  1 sibling, 1 reply; 22+ messages in thread
From: Simon Horman @ 2025-07-23 19:05 UTC (permalink / raw)
  To: Vineeth Karumanchi
  Cc: nicolas.ferre, claudiu.beznea, andrew+netdev, davem, edumazet,
	kuba, pabeni, git, netdev, linux-kernel

On Tue, Jul 22, 2025 at 09:11:11PM +0530, Vineeth Karumanchi wrote:
> The "exclude_qbv" bit in designcfg_debug1 register varies between MACB/GEM
> IP revisions, making direct register probing unreliable for
> feature detection. A capability-based approach provides consistent
> QBV support identification across the IP family
> 
> Platform support:
> - Enable MACB_CAPS_QBV for Xilinx Versal platform configuration
> - Foundation for QBV feature detection in TAPRIO implementation
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>

...

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index cc33491930e3..98e56697661c 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4601,6 +4601,10 @@ static int macb_init(struct platform_device *pdev)
>  		dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
>  	if (bp->caps & MACB_CAPS_SG_DISABLED)
>  		dev->hw_features &= ~NETIF_F_SG;
> +	/* Enable HW_TC if hardware supports QBV */
> +	if (bp->caps & MACB_CAPS_QBV)
> +		dev->hw_features |= NETIF_F_HW_TC;
> +
>  	dev->features = dev->hw_features;
>  
>  	/* Check RX Flow Filters support.
> @@ -5345,7 +5349,7 @@ static const struct macb_config sama7g5_emac_config = {
>  static const struct macb_config versal_config = {
>  	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
>  		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK |
> -		MACB_CAPS_QUEUE_DISABLE,
> +		MACB_CAPS_QUEUE_DISABLE, MACB_CAPS_QBV,

Hi Vineeth,

TL;DR: I think you mean

		MACB_CAPS_QUEUE_DISABLE | MACB_CAPS_QBV,
		                       ^^^

I assume that the intention here is to set the MACB_CAPS_QBV bit of .caps.
However, because there is a comma rather than a pipe between
it and MACB_CAPS_QUEUE_DISABLE the effect is to leave .caps as
it was before, and set .dma_burst_length to MACB_CAPS_QBV.
.dma_burst_length is then overwritten on the following line.

Flagged by W=1 builds with Clang 20.1.8 and 15.1.0.

Please build your patches with W=1 and try to avoid adding warnings
it flags.

Also, while we are here, it would be nice to fix up the line wrapping so
the adjacent code is 80 columns wide or less, as is still preferred in
Networking code.

	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
		MACB_CAPS_NEED_TSUCLK | MACB_CAPS_QUEUE_DISABLE |
		MACB_CAPS_QBV,

>  	.dma_burst_length = 16,
>  	.clk_init = macb_clk_init,
>  	.init = init_reset_optional,
> -- 
> 2.34.1
> 
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support
  2025-07-22 15:41 ` [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support Vineeth Karumanchi
  2025-07-23 19:05   ` Simon Horman
@ 2025-07-24  0:46   ` kernel test robot
  1 sibling, 0 replies; 22+ messages in thread
From: kernel test robot @ 2025-07-24  0:46 UTC (permalink / raw)
  To: Vineeth Karumanchi, nicolas.ferre, claudiu.beznea, andrew+netdev,
	davem, edumazet, kuba, pabeni
  Cc: oe-kbuild-all, git, netdev, linux-kernel, vineeth.karumanchi

Hi Vineeth,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Vineeth-Karumanchi/net-macb-Define-ENST-hardware-registers-for-time-aware-scheduling/20250722-234618
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250722154111.1871292-7-vineeth.karumanchi%40amd.com
patch subject: [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support
config: parisc-randconfig-r131-20250724 (https://download.01.org/0day-ci/archive/20250724/202507240825.lVN6sSiB-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.3.0
reproduce: (https://download.01.org/0day-ci/archive/20250724/202507240825.lVN6sSiB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507240825.lVN6sSiB-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
   drivers/net/ethernet/cadence/macb_main.c:282:16: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [usertype] bottom @@     got restricted __le32 [usertype] @@
   drivers/net/ethernet/cadence/macb_main.c:282:16: sparse:     expected unsigned int [usertype] bottom
   drivers/net/ethernet/cadence/macb_main.c:282:16: sparse:     got restricted __le32 [usertype]
   drivers/net/ethernet/cadence/macb_main.c:284:13: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned short [usertype] top @@     got restricted __le16 [usertype] @@
   drivers/net/ethernet/cadence/macb_main.c:284:13: sparse:     expected unsigned short [usertype] top
   drivers/net/ethernet/cadence/macb_main.c:284:13: sparse:     got restricted __le16 [usertype]
   drivers/net/ethernet/cadence/macb_main.c:3645:39: sparse: sparse: restricted __be32 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3650:39: sparse: sparse: restricted __be32 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3655:40: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3655:69: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3680:20: sparse: sparse: restricted __be32 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3684:20: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [assigned] [usertype] w0 @@     got restricted __be32 [usertype] ip4src @@
   drivers/net/ethernet/cadence/macb_main.c:3684:20: sparse:     expected unsigned int [assigned] [usertype] w0
   drivers/net/ethernet/cadence/macb_main.c:3684:20: sparse:     got restricted __be32 [usertype] ip4src
   drivers/net/ethernet/cadence/macb_main.c:3694:20: sparse: sparse: restricted __be32 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3698:20: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [assigned] [usertype] w0 @@     got restricted __be32 [usertype] ip4dst @@
   drivers/net/ethernet/cadence/macb_main.c:3698:20: sparse:     expected unsigned int [assigned] [usertype] w0
   drivers/net/ethernet/cadence/macb_main.c:3698:20: sparse:     got restricted __be32 [usertype] ip4dst
   drivers/net/ethernet/cadence/macb_main.c:3708:21: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3708:50: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3714:30: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3715:30: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3722:36: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3723:38: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3726:38: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/cadence/macb_main.c:3762:9: sparse: sparse: cast from restricted __be32
   drivers/net/ethernet/cadence/macb_main.c:3762:9: sparse: sparse: cast from restricted __be32
   drivers/net/ethernet/cadence/macb_main.c:3816:25: sparse: sparse: cast from restricted __be32
   drivers/net/ethernet/cadence/macb_main.c:3816:25: sparse: sparse: cast from restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:5352:42: sparse: sparse: Initializer entry defined twice
   drivers/net/ethernet/cadence/macb_main.c:5353:10: sparse:   also defined here

vim +5352 drivers/net/ethernet/cadence/macb_main.c

  5348	
  5349	static const struct macb_config versal_config = {
  5350		.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
  5351			MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK |
> 5352			MACB_CAPS_QUEUE_DISABLE, MACB_CAPS_QBV,
  5353		.dma_burst_length = 16,
  5354		.clk_init = macb_clk_init,
  5355		.init = init_reset_optional,
  5356		.jumbo_max_len = 10240,
  5357		.usrio = &macb_default_usrio,
  5358	};
  5359	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 1/6] net: macb: Define ENST hardware registers for time-aware scheduling
  2025-07-22 15:41 ` [PATCH net-next 1/6] net: macb: Define ENST hardware registers for time-aware scheduling Vineeth Karumanchi
@ 2025-07-26 12:23   ` claudiu beznea (tuxon)
  0 siblings, 0 replies; 22+ messages in thread
From: claudiu beznea (tuxon) @ 2025-07-26 12:23 UTC (permalink / raw)
  To: Vineeth Karumanchi, nicolas.ferre, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel

Hi, Vineeth,

On 7/22/25 18:41, Vineeth Karumanchi wrote:
> Add ENST (Enhanced Network Scheduling and Timing) register definitions
> to support IEEE 802.1Qbv time-gated transmission.
> 
> Register architecture:
> - Per-queue timing registers: ENST_START_TIME, ENST_ON_TIME, ENST_OFF_TIME
> - Centralized control of the ENST_CONTROL register for enabling or
>    disabling queue gates.
> - Time intervals programmed in hardware byte units
> - Hardware-level queue scheduling infrastructure.
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>   drivers/net/ethernet/cadence/macb.h | 43 +++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index c9a5c8beb2fa..e456ac65d6c6 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -184,6 +184,13 @@
>   #define GEM_DCFG8		0x029C /* Design Config 8 */
>   #define GEM_DCFG10		0x02A4 /* Design Config 10 */
>   #define GEM_DCFG12		0x02AC /* Design Config 12 */
> +#define GEM_ENST_START_TIME_Q0	0x0800 /* ENST Q0 start time */
> +#define GEM_ENST_START_TIME_Q1	0x0804 /* ENST Q1 start time */
> +#define GEM_ENST_ON_TIME_Q0	0x0820 /* ENST Q0 on time */
> +#define GEM_ENST_ON_TIME_Q1	0x0824 /* ENST Q1 on time */
> +#define GEM_ENST_OFF_TIME_Q0	0x0840 /* ENST Q0 off time */
> +#define GEM_ENST_OFF_TIME_Q1	0x0844 /* ENST Q1 off time */
> +#define GEM_ENST_CONTROL	0x0880 /* ENST control register */
>   #define GEM_USX_CONTROL		0x0A80 /* High speed PCS control register */
>   #define GEM_USX_STATUS		0x0A88 /* High speed PCS status register */
>   
> @@ -221,6 +228,15 @@
>   #define GEM_IDR(hw_q)		(0x0620 + ((hw_q) << 2))
>   #define GEM_IMR(hw_q)		(0x0640 + ((hw_q) << 2))
>   
> +#define GEM_ENST_START_TIME(hw_q)	(0x0800 + ((hw_q) << 2))
> +#define GEM_ENST_ON_TIME(hw_q)		(0x0820 + ((hw_q) << 2))
> +#define GEM_ENST_OFF_TIME(hw_q)		(0x0840 + ((hw_q) << 2))
> +
> +/*  Bitfields in ENST_CONTROL. */
> +#define GEM_ENST_DISABLE_QUEUE(hw_q)	BIT((hw_q) + 16) /* q0 disable is 16'b */
> +#define GEM_ENST_DISABLE_QUEUE_OFFSET	16
> +#define GEM_ENST_ENABLE_QUEUE(hw_q)		BIT(hw_q) /* q0 enable is 0'b */

There is an extra tab here ------------^

> +
>   /* Bitfields in NCR */
>   #define MACB_LB_OFFSET		0 /* reserved */
>   #define MACB_LB_SIZE		1
> @@ -554,6 +570,33 @@
>   #define GEM_HIGH_SPEED_OFFSET			26
>   #define GEM_HIGH_SPEED_SIZE			1
>   
> +/* Bitfields in ENST_START_TIME_Q0, Q1. */
> +#define GEM_START_TIME_SEC_OFFSET		30
> +#define GEM_START_TIME_SEC_SIZE			2
> +#define GEM_START_TIME_NSEC_OFFSET		0
> +#define GEM_START_TIME_NSEC_SIZE		30
> +
> +/* Bitfields in ENST_ON_TIME_Q0, Q1. */
> +#define GEM_ON_TIME_OFFSET			0
> +#define GEM_ON_TIME_SIZE			17
> +
> +/* Bitfields in ENST_OFF_TIME_Q0, Q1. */
> +#define GEM_OFF_TIME_OFFSET			0
> +#define GEM_OFF_TIME_SIZE			17
> +
> +/* Hardware ENST timing registers granularity */
> +#define ENST_TIME_GRANULARITY_NS 8
> +
> +/* Bitfields in ENST_CONTROL. */
> +#define GEM_DISABLE_Q1_OFFSET			17
> +#define GEM_DISABLE_Q1_SIZE			1
> +#define GEM_DISABLE_Q0_OFFSET			16
> +#define GEM_DISABLE_Q0_SIZE			1
> +#define GEM_ENABLE_Q1_OFFSET			1
> +#define GEM_ENABLE_Q1_SIZE			1
> +#define GEM_ENABLE_Q0_OFFSET			0
> +#define GEM_ENABLE_Q0_SIZE			1
> +

Please introduce these defines along with the code that uses it. This way it is 
simpler to follow what is actually used.

Thank you,
Claudiu

>   /* Bitfields in USX_CONTROL. */
>   #define GEM_USX_CTRL_SPEED_OFFSET		14
>   #define GEM_USX_CTRL_SPEED_SIZE			3


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 2/6] net: macb: Integrate ENST timing parameters and hardware unit conversion
  2025-07-22 15:41 ` [PATCH net-next 2/6] net: macb: Integrate ENST timing parameters and hardware unit conversion Vineeth Karumanchi
@ 2025-07-26 12:24   ` claudiu beznea (tuxon)
  0 siblings, 0 replies; 22+ messages in thread
From: claudiu beznea (tuxon) @ 2025-07-26 12:24 UTC (permalink / raw)
  To: Vineeth Karumanchi, nicolas.ferre, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel

Hi, Vineeth,

On 7/22/25 18:41, Vineeth Karumanchi wrote:
> Add Enhanced Network Scheduling and Timing (ENST) support to
> queue infrastructure with speed-dependent timing calculations for
> precise gate control.
> 
> Hardware timing unit conversion:
> - Timing values programmed as hardware units based on link speed
> - Conversion formula: time_bytes = time_ns / divisor
> - Speed-specific divisors:
>    * 1 Gbps:   divisor = 8
>    * 100 Mbps: divisor = 80
>    * 10 Mbps:  divisor = 800
> 
> Infrastructure changes:
> - Extend macb_queue structure with ENST timing control registers
> - Add queue_enst_configs structure for per-entry TC configuration storage
> - Map ENST register offsets into existing queue management framework
> - Define ENST_NS_TO_HW_UNITS() macro for automatic speed-based conversion
> 
> This enables hardware-native timing programming while abstracting the
> speed-dependent conversions
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>   drivers/net/ethernet/cadence/macb.h      | 32 ++++++++++++++++++++++++
>   drivers/net/ethernet/cadence/macb_main.c |  6 +++++
>   2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index e456ac65d6c6..ef3995564c5c 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -857,6 +857,16 @@
>   
>   #define MACB_READ_NSR(bp)	macb_readl(bp, NSR)
>   
> +/* ENST macros*/
> +#define ENST_NS_TO_HW_UNITS(ns, speed_mbps) \
> +		DIV_ROUND_UP((ns) * (speed_mbps), (ENST_TIME_GRANULARITY_NS * 1000))
> +
> +#define ENST_MAX_HW_INTERVAL(speed_mbps) \
> +		DIV_ROUND_UP(GENMASK(GEM_ON_TIME_SIZE - 1, 0) * ENST_TIME_GRANULARITY_NS * 1000,\
> +		(speed_mbps))
> +
> +#define ENST_MAX_START_TIME_SEC GENMASK(GEM_START_TIME_SEC_SIZE - 1, 0)

These are not used in this patch.

> +
>   /* struct macb_dma_desc - Hardware DMA descriptor
>    * @addr: DMA address of data buffer
>    * @ctrl: Control and status bits
> @@ -1262,6 +1272,11 @@ struct macb_queue {
>   	unsigned int		RBQP;
>   	unsigned int		RBQPH;
>   
> +	/* ENST register offsets for this queue */
> +	unsigned int		ENST_START_TIME;
> +	unsigned int		ENST_ON_TIME;
> +	unsigned int		ENST_OFF_TIME;
> +
>   	/* Lock to protect tx_head and tx_tail */
>   	spinlock_t		tx_ptr_lock;
>   	unsigned int		tx_head, tx_tail;
> @@ -1450,4 +1465,21 @@ struct macb_platform_data {
>   	struct clk	*hclk;
>   };
>   
> +/**
> + * struct queue_enst_configs - Configuration for Enhanced Scheduled Traffic (ENST) queue
> + * @queue_id:         Identifier for the queue
> + * @start_time_mask:  Bitmask representing the start time for the queue
> + * @on_time_bytes:    "on" time nsec expressed in bytes
> + * @off_time_bytes:   "off" time nsec expressed in bytes
> + *
> + * This structure holds the configuration parameters for an ENST queue,
> + * used to control time-based transmission scheduling in the MACB driver.
> + */
> +struct queue_enst_configs {

s/queue_enst_config/macb_queue_enst_config

> +	u8 queue_id;

Could you please move this above to avoid any padding?

> +	u32 start_time_mask;
> +	u32 on_time_bytes;
> +	u32 off_time_bytes;
> +};
> +

This structure is not used in this patch. Can you please introduce it in the 
patch it is used?


>   #endif /* _MACB_H */
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ce95fad8cedd..ff87d3e1d8a0 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4305,6 +4305,9 @@ static int macb_init(struct platform_device *pdev)
>   			queue->TBQP = GEM_TBQP(hw_q - 1);
>   			queue->RBQP = GEM_RBQP(hw_q - 1);
>   			queue->RBQS = GEM_RBQS(hw_q - 1);
> +			queue->ENST_START_TIME = GEM_ENST_START_TIME(hw_q);
> +			queue->ENST_ON_TIME = GEM_ENST_ON_TIME(hw_q);
> +			queue->ENST_OFF_TIME = GEM_ENST_OFF_TIME(hw_q);

You can drop these lines here and move it outside of the if (hw_q) {} else {} block.

>   #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>   			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
>   				queue->TBQPH = GEM_TBQPH(hw_q - 1);
> @@ -4319,6 +4322,9 @@ static int macb_init(struct platform_device *pdev)
>   			queue->IMR  = MACB_IMR;
>   			queue->TBQP = MACB_TBQP;
>   			queue->RBQP = MACB_RBQP;
> +			queue->ENST_START_TIME = GEM_ENST_START_TIME(0);
> +			queue->ENST_ON_TIME = GEM_ENST_ON_TIME(0);
> +			queue->ENST_OFF_TIME = GEM_ENST_OFF_TIME(0);

With the above suggested change, these lines could be dropped.

On the other hand these lines are used in patch 3. So I would prefer to have 
them introduced there.

Thank you,
Claudiu

>   #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>   			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
>   				queue->TBQPH = MACB_TBQPH;


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support
  2025-07-22 15:41 ` [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support Vineeth Karumanchi
  2025-07-23 10:02   ` kernel test robot
@ 2025-07-26 12:25   ` claudiu beznea (tuxon)
  2025-07-26 15:32     ` Andrew Lunn
  2025-07-29  8:59     ` Karumanchi, Vineeth
  1 sibling, 2 replies; 22+ messages in thread
From: claudiu beznea (tuxon) @ 2025-07-26 12:25 UTC (permalink / raw)
  To: Vineeth Karumanchi, nicolas.ferre, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel



On 7/22/25 18:41, Vineeth Karumanchi wrote:
> Implement Time-Aware Traffic Scheduling (TAPRIO) hardware offload for
> "tc qdisc replace" operations, enabling IEEE 802.1Qbv compliant gate
> scheduling on Cadence MACB/GEM controllers.
> 
> Parameter validation checks performed:
> - Queue count bounds checking (1 < queues <= MACB_MAX_QUEUES)
> - TC entry limit validation against available hardware queues
> - Base time non-negativity enforcement
> - Speed-adaptive timing constraint verification
> - Cycle time vs. total gate time consistency checks
> - Single-queue gate mask enforcement per scheduling entry
> 
> Hardware programming sequence:
> - GEM doesn't support changing register values if ENST is running,
>    hence disable ENST before programming
> - Atomic timing register configuration (START_TIME, ON_TIME, OFF_TIME)
> - Enable the configured queues via ENST_CONTROL register
> 
> This implementation ensures deterministic gate scheduling while preventing
> invalid configurations.
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>   drivers/net/ethernet/cadence/macb_main.c | 155 +++++++++++++++++++++++
>   1 file changed, 155 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ff87d3e1d8a0..4518b59168d5 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -36,6 +36,7 @@
>   #include <linux/reset.h>
>   #include <linux/firmware/xlnx-zynqmp.h>
>   #include <linux/inetdevice.h>
> +#include <net/pkt_sched.h>
>   #include "macb.h"
>   
>   /* This structure is only used for MACB on SiFive FU540 devices */
> @@ -4084,6 +4085,160 @@ static void macb_restore_features(struct macb *bp)
>   	macb_set_rxflow_feature(bp, features);
>   }
>   
> +static int macb_taprio_setup_replace(struct net_device *ndev,
> +				     struct tc_taprio_qopt_offload *conf)

This function is unused in this patch. Nothing mentions it.

> +{
> +	u64 total_on_time = 0, start_time_sec = 0, start_time = conf->base_time;
> +	struct queue_enst_configs  *enst_queue;

Extra space here -----------------^

> +	u32 configured_queues = 0, speed = 0;
> +	struct tc_taprio_sched_entry *entry;
> +	struct macb *bp = netdev_priv(ndev);
> +	struct ethtool_link_ksettings kset;
> +	struct macb_queue *queue;
> +	unsigned long flags;
> +	int err = 0, i;

No need to initialize err with zero.
	
         size_t i;

as it is used along with conf->num_entries which has size_t type.

> +
> +	/* Validate queue configuration */
> +	if (bp->num_queues < 1 || bp->num_queues > MACB_MAX_QUEUES) {

Can this happen?

> +		netdev_err(ndev, "Invalid number of queues: %d\n", bp->num_queues);
> +		return -EINVAL;
> +	}
> +
> +	if (conf->num_entries > bp->num_queues) {
> +		netdev_err(ndev, "Too many TAPRIO entries: %lu > %d queues\n",
> +			   conf->num_entries, bp->num_queues);
> +		return -EINVAL;
> +	}
> +
> +	if (start_time < 0) {
> +		netdev_err(ndev, "Invalid base_time: must be 0 or positive, got %lld\n",
> +			   conf->base_time);
> +		return -ERANGE;
> +	}
> +
> +	/* Get the current link speed */
> +	err = phylink_ethtool_ksettings_get(bp->phylink, &kset);
> +	if (unlikely(err)) {
> +		netdev_err(ndev, "Failed to get link settings: %d\n", err);
> +		return err;
> +	}
> +
> +	speed = kset.base.speed;
> +	if (unlikely(speed <= 0)) {
> +		netdev_err(ndev, "Invalid speed: %d\n", speed);
> +		return -EINVAL;
> +	}
> +
> +	enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), GFP_KERNEL);

To simplify the error path you can use something like:

         struct queue_enst_configs *enst_queue __free(kfree) = kcalloc(...);

and drop the "goto cleanup" below.


> +	if (!enst_queue)
> +		return -ENOMEM;
> +
> +	/* Pre-validate all entries before making any hardware changes */
> +	for (i = 0; i < conf->num_entries; i++) {
> +		entry = &conf->entries[i];
> +
> +		if (entry->command != TC_TAPRIO_CMD_SET_GATES) {
> +			netdev_err(ndev, "Entry %d: unsupported command %d\n",
> +				   i, entry->command);
> +			err = -EOPNOTSUPP;
> +			goto cleanup;
> +		}
> +
> +		/* Validate gate_mask: must be nonzero, single queue, and within range */
> +		if (!is_power_of_2(entry->gate_mask)) {
> +			netdev_err(ndev, "Entry %d: gate_mask 0x%x is not a power of 2 (only one queue per entry allowed)\n",
> +				   i, entry->gate_mask);
> +			err = -EINVAL;
> +			goto cleanup;
> +		}
> +
> +		/* gate_mask must not select queues outside the valid queue_mask */
> +		if (entry->gate_mask & ~bp->queue_mask) {
> +			netdev_err(ndev, "Entry %d: gate_mask 0x%x exceeds queue range (max_queues=%d)\n",
> +				   i, entry->gate_mask, bp->num_queues);
> +			err = -EINVAL;
> +			goto cleanup;
> +		}
> +
> +		/* Check for start time limits */
> +		start_time_sec = div_u64(start_time, NSEC_PER_SEC);
> +		if (start_time_sec > ENST_MAX_START_TIME_SEC) {
> +			netdev_err(ndev, "Entry %d: Start time %llu s exceeds hardware limit\n",
> +				   i, start_time_sec);
> +			err = -ERANGE;
> +			goto cleanup;
> +		}
> +
> +		/* Check for on time limit*/

Missing one space here -------------------^

> +		if (entry->interval > ENST_MAX_HW_INTERVAL(speed)) {
> +			netdev_err(ndev, "Entry %d: interval %u ns exceeds hardware limit %lu ns\n",
> +				   i, entry->interval, ENST_MAX_HW_INTERVAL(speed));
> +			err = -ERANGE;
> +			goto cleanup;
> +		}
> +
> +		/* Check for off time limit*/
> +		if ((conf->cycle_time - entry->interval) > ENST_MAX_HW_INTERVAL(speed)) {
> +			netdev_err(ndev, "Entry %d: off_time %llu ns exceeds hardware limit %lu ns\n",
> +				   i, conf->cycle_time - entry->interval,
> +				   ENST_MAX_HW_INTERVAL(speed));
> +			err = -ERANGE;
> +			goto cleanup;
> +		}
> +
> +		enst_queue[i].queue_id = order_base_2(entry->gate_mask);
> +		enst_queue[i].start_time_mask =
> +			(start_time_sec << GEM_START_TIME_SEC_OFFSET) |
> +				  (start_time % NSEC_PER_SEC);
> +		enst_queue[i].on_time_bytes =
> +			ENST_NS_TO_HW_UNITS(entry->interval, speed);
> +		enst_queue[i].off_time_bytes =
> +			ENST_NS_TO_HW_UNITS(conf->cycle_time - entry->interval, speed);
> +
> +		configured_queues |= entry->gate_mask;
> +		total_on_time += entry->interval;
> +		start_time += entry->interval;
> +	}
> +
> +	/* Check total interval doesn't exceed cycle time */
> +	if (total_on_time > conf->cycle_time) {
> +		netdev_err(ndev, "Total ON %llu ns exceeds cycle time %llu ns\n",
> +			   total_on_time, conf->cycle_time);
> +		err = -EINVAL;
> +		goto cleanup;
> +	}
> +
> +	netdev_dbg(ndev, "TAPRIO setup: %lu entries, base_time=%lld ns, cycle_time=%llu ns\n",
> +		   conf->num_entries, conf->base_time, conf->cycle_time);
> +
> +	/* All validations passed - proceed with hardware configuration */
> +	spin_lock_irqsave(&bp->lock, flags);

You can use guard(spinlock_irqsave)(&bp->lock) or scoped_guard(spinlock_irqsave, 
&bp->lock)

> +
> +	/* Disable ENST queues if running before configuring */
> +	if (gem_readl(bp, ENST_CONTROL))

Is this read necessary?

> +		gem_writel(bp, ENST_CONTROL,
> +			   GENMASK(bp->num_queues - 1, 0) << GEM_ENST_DISABLE_QUEUE_OFFSET);

This could be replaced by GEM_BF(GENMASK(...), ENST_DISABLE_QUEUE) if you define 
GEM_ENST_DISABLE_QUEUE_SIZE along with GEM_ENST_DISABLE_QUEUE_OFFSET.

> +
> +	for (i = 0; i < conf->num_entries; i++) {
> +		queue = &bp->queues[enst_queue[i].queue_id];
> +		/* Configure queue timing registers */
> +		queue_writel(queue, ENST_START_TIME, enst_queue[i].start_time_mask);
> +		queue_writel(queue, ENST_ON_TIME, enst_queue[i].on_time_bytes);
> +		queue_writel(queue, ENST_OFF_TIME, enst_queue[i].off_time_bytes);
> +	}
> +
> +	/* Enable ENST for all configured queues in one write */
> +	gem_writel(bp, ENST_CONTROL, configured_queues);

Can this function be executed while other queues are configured? If so, would 
the configured_queues contains it (as well as conf)?

> +	spin_unlock_irqrestore(&bp->lock, flags);
> +
> +	netdev_info(ndev, "TAPRIO configuration completed successfully: %lu entries, %d queues configured\n",
> +		    conf->num_entries, hweight32(configured_queues));
> +
> +cleanup:
> +	kfree(enst_queue);

With the suggestions above, this could be dropped.

Thank you,
Claudiu

> +	return err;
> +}
> +
>   static const struct net_device_ops macb_netdev_ops = {
>   	.ndo_open		= macb_open,
>   	.ndo_stop		= macb_close,


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 4/6] net: macb: Implement TAPRIO DESTROY command offload for gate cleanup
  2025-07-22 15:41 ` [PATCH net-next 4/6] net: macb: Implement TAPRIO DESTROY command offload for gate cleanup Vineeth Karumanchi
@ 2025-07-26 12:26   ` claudiu beznea (tuxon)
  0 siblings, 0 replies; 22+ messages in thread
From: claudiu beznea (tuxon) @ 2025-07-26 12:26 UTC (permalink / raw)
  To: Vineeth Karumanchi, nicolas.ferre, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel



On 7/22/25 18:41, Vineeth Karumanchi wrote:
> Add hardware offload support for "tc qdisc destroy" operations to safely
> remove IEEE 802.1Qbv time-gated scheduling configuration and restore
> default queue behavior.
> 
> Cleanup sequence:
> - Reset network device TC configuration state
> - Disable Enhanced Network Scheduling and Timing for all queues
> - Clear all ENST timing control registers (START_TIME, ON_TIME, OFF_TIME)
> - Atomic register programming with proper synchronization
> 
> This ensures complete removal of time-aware scheduling state, returning
> the controller to standard FIFO queue operation without residual timing
> constraints
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>   drivers/net/ethernet/cadence/macb_main.c | 28 ++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 4518b59168d5..6b3eff28a842 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4239,6 +4239,34 @@ static int macb_taprio_setup_replace(struct net_device *ndev,
>   	return err;
>   }
>   
> +static void macb_taprio_destroy(struct net_device *ndev)

This function is unused in this patch. Nothing mentions it.

> +{
> +	struct macb *bp = netdev_priv(ndev);
> +	struct macb_queue *queue;
> +	unsigned long flags;
> +	u32 enst_disable_mask;
> +	u8 i;

unsigned int

> +
> +	netdev_reset_tc(ndev);
> +	enst_disable_mask = GENMASK(bp->num_queues - 1, 0) << GEM_ENST_DISABLE_QUEUE_OFFSET;

You can use GEM_BF(GENMASK(...), ENST_DISABLE_QUEUE) if you 
GEM_ENST_DISABLE_QUEUE_SIZE is defined

> +	netdev_dbg(ndev, "TAPRIO destroy: disabling all gates\n");
> +
> +	spin_lock_irqsave(&bp->lock, flags);

guard()

> +
> +	/* Single disable command for all queues */
> +	gem_writel(bp, ENST_CONTROL, enst_disable_mask);
> +
> +	/* Clear all queue ENST registers in batch */
> +	for (i = 0; i < bp->num_queues; i++) {

You can follow the pattern across macb_main.c and replace it with:

         for (unsigned int q = 0, queue = &bp->queues[q]; q < bp->num_queues; 
++q, ++queue)

> +		queue = &bp->queues[i];

And drop this line

Thank you,
Claudiu

> +		queue_writel(queue, ENST_START_TIME, 0);
> +		queue_writel(queue, ENST_ON_TIME, 0);
> +		queue_writel(queue, ENST_OFF_TIME, 0);
> +	}
> +
> +	spin_unlock_irqrestore(&bp->lock, flags);
> +}
> +
>   static const struct net_device_ops macb_netdev_ops = {
>   	.ndo_open		= macb_open,
>   	.ndo_stop		= macb_close,


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface
  2025-07-22 15:41 ` [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface Vineeth Karumanchi
@ 2025-07-26 12:29   ` claudiu beznea (tuxon)
  2025-07-29  9:38     ` Karumanchi, Vineeth
  2025-07-28 16:31   ` kernel test robot
  1 sibling, 1 reply; 22+ messages in thread
From: claudiu beznea (tuxon) @ 2025-07-26 12:29 UTC (permalink / raw)
  To: Vineeth Karumanchi, nicolas.ferre, andrew+netdev, davem, edumazet,
	kuba, pabeni
  Cc: git, netdev, linux-kernel



On 7/22/25 18:41, Vineeth Karumanchi wrote:
> Add Traffic Control offload infrastructure with command routing for
> TAPRIO qdisc operations:
> 
> - macb_setup_taprio(): TAPRIO command dispatcher
> - macb_setup_tc(): TC_SETUP_QDISC_TAPRIO entry point
> - Support for REPLACE/DESTROY command mapping
> 
> Provides standardized TC interface for time-gated scheduling control.
> 
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>   drivers/net/ethernet/cadence/macb_main.c | 33 ++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 6b3eff28a842..cc33491930e3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4267,6 +4267,38 @@ static void macb_taprio_destroy(struct net_device *ndev)
>   	spin_unlock_irqrestore(&bp->lock, flags);
>   }
>   
> +static int macb_setup_taprio(struct net_device *ndev,
> +			     struct tc_taprio_qopt_offload *taprio)
> +{
> +	int err = 0;
> +
> +	switch (taprio->cmd) {
> +	case TAPRIO_CMD_REPLACE:
> +		err = macb_taprio_setup_replace(ndev, taprio);
> +		break;
> +	case TAPRIO_CMD_DESTROY:
> +		macb_taprio_destroy(ndev);

macb_taprio_setup_replace() along with macb_taprio_destroy() touch HW registers. 
Could macb_setup_taprio() be called when the interface is runtime suspended?


> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +	}
> +
> +	return err;
> +}
> +
> +static int macb_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
> +{
> +	if (!dev || !type_data)
> +		return -EINVAL;
> +
> +	switch (type) {
> +	case TC_SETUP_QDISC_TAPRIO:
> +		return macb_setup_taprio(dev, type_data);

Same here.

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>   static const struct net_device_ops macb_netdev_ops = {
>   	.ndo_open		= macb_open,
>   	.ndo_stop		= macb_close,
> @@ -4284,6 +4316,7 @@ static const struct net_device_ops macb_netdev_ops = {
>   	.ndo_features_check	= macb_features_check,
>   	.ndo_hwtstamp_set	= macb_hwtstamp_set,
>   	.ndo_hwtstamp_get	= macb_hwtstamp_get,
> +	.ndo_setup_tc		= macb_setup_tc,

This patch (or parts of it) should be merged with the previous ones. Otherwise 
you introduce patches with code that is unused.

Thank you,
Claudiu

>   };
>   
>   /* Configure peripheral capabilities according to device tree


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support
  2025-07-26 12:25   ` claudiu beznea (tuxon)
@ 2025-07-26 15:32     ` Andrew Lunn
  2025-07-29  8:59     ` Karumanchi, Vineeth
  1 sibling, 0 replies; 22+ messages in thread
From: Andrew Lunn @ 2025-07-26 15:32 UTC (permalink / raw)
  To: claudiu beznea (tuxon)
  Cc: Vineeth Karumanchi, nicolas.ferre, andrew+netdev, davem, edumazet,
	kuba, pabeni, git, netdev, linux-kernel

> > +	enst_queue = kcalloc(conf->num_entries, sizeof(*enst_queue), GFP_KERNEL);
> 
> To simplify the error path you can use something like:
> 
>         struct queue_enst_configs *enst_queue __free(kfree) = kcalloc(...);
> 
> and drop the "goto cleanup" below.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

1.6.5. Using device-managed and cleanup.h constructs

Netdev remains skeptical about promises of all “auto-cleanup” APIs,
including even devm_ helpers, historically. They are not the preferred
style of implementation, merely an acceptable one.

Use of guard() is discouraged within any function longer than 20
lines, scoped_guard() is considered more readable. Using normal
lock/unlock is still (weakly) preferred.

Low level cleanup constructs (such as __free()) can be used when
building APIs and helpers, especially scoped iterators. However,
direct use of __free() within networking core and drivers is
discouraged. Similar guidance applies to declaring variables
mid-function.

> 
> You can use guard(spinlock_irqsave)(&bp->lock) or
> scoped_guard(spinlock_irqsave, &bp->lock)

scoped_guard() if anything.

> 
> > +
> > +	/* Disable ENST queues if running before configuring */
> > +	if (gem_readl(bp, ENST_CONTROL))
> 
> Is this read necessary?
> 
> > +		gem_writel(bp, ENST_CONTROL,
> > +			   GENMASK(bp->num_queues - 1, 0) << GEM_ENST_DISABLE_QUEUE_OFFSET);
> 
> This could be replaced by GEM_BF(GENMASK(...), ENST_DISABLE_QUEUE) if you
> define GEM_ENST_DISABLE_QUEUE_SIZE along with GEM_ENST_DISABLE_QUEUE_OFFSET.
> 
> > +
> > +	for (i = 0; i < conf->num_entries; i++) {
> > +		queue = &bp->queues[enst_queue[i].queue_id];
> > +		/* Configure queue timing registers */
> > +		queue_writel(queue, ENST_START_TIME, enst_queue[i].start_time_mask);
> > +		queue_writel(queue, ENST_ON_TIME, enst_queue[i].on_time_bytes);
> > +		queue_writel(queue, ENST_OFF_TIME, enst_queue[i].off_time_bytes);
> > +	}
> > +
> > +	/* Enable ENST for all configured queues in one write */
> > +	gem_writel(bp, ENST_CONTROL, configured_queues);
> 
> Can this function be executed while other queues are configured? If so,
> would the configured_queues contains it (as well as conf)?
> 
> > +	spin_unlock_irqrestore(&bp->lock, flags);
> > +
> > +	netdev_info(ndev, "TAPRIO configuration completed successfully: %lu entries, %d queues configured\n",
> > +		    conf->num_entries, hweight32(configured_queues));

guard() would put that netdev_info() print inside the guard. Do you
really want to be doing a print, inside a spinlock, with interrupts
potentially disabled? This is one of the reasons i don't like this
magical guard() construct, it is easy to forget how the magic works
and end up with sub optimal code. A scoped_guard() would avoid this
issue. You have to think about where you want to lock released in
order to place the } .

	Andrew

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface
  2025-07-22 15:41 ` [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface Vineeth Karumanchi
  2025-07-26 12:29   ` claudiu beznea (tuxon)
@ 2025-07-28 16:31   ` kernel test robot
  1 sibling, 0 replies; 22+ messages in thread
From: kernel test robot @ 2025-07-28 16:31 UTC (permalink / raw)
  To: Vineeth Karumanchi, nicolas.ferre, claudiu.beznea, andrew+netdev,
	davem, edumazet, kuba, pabeni
  Cc: oe-kbuild-all, git, netdev, linux-kernel, vineeth.karumanchi

Hi Vineeth,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Vineeth-Karumanchi/net-macb-Define-ENST-hardware-registers-for-time-aware-scheduling/20250722-234618
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250722154111.1871292-6-vineeth.karumanchi%40amd.com
patch subject: [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface
config: i386-randconfig-005-20250728 (https://download.01.org/0day-ci/archive/20250729/202507290045.ckQlBy8r-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250729/202507290045.ckQlBy8r-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507290045.ckQlBy8r-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__umoddi3" [drivers/net/ethernet/cadence/macb.ko] undefined!
>> ERROR: modpost: "__udivdi3" [drivers/net/ethernet/cadence/macb.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support
  2025-07-26 12:25   ` claudiu beznea (tuxon)
  2025-07-26 15:32     ` Andrew Lunn
@ 2025-07-29  8:59     ` Karumanchi, Vineeth
  2025-07-31  9:07       ` Claudiu Beznea
  1 sibling, 1 reply; 22+ messages in thread
From: Karumanchi, Vineeth @ 2025-07-29  8:59 UTC (permalink / raw)
  To: claudiu.beznea, vineeth.karumanchi, nicolas.ferre, andrew+netdev,
	davem, edumazet, kuba, pabeni
  Cc: git, netdev, linux-kernel

Hi Claudiu,

Thanks for the review.

On 7/26/2025 5:55 PM, claudiu beznea (tuxon) wrote:
> 
> 
> On 7/22/25 18:41, Vineeth Karumanchi wrote:
>> Implement Time-Aware Traffic Scheduling (TAPRIO) hardware offload for

<..>

> 
> as it is used along with conf->num_entries which has size_t type.
> 
>> +
>> +    /* Validate queue configuration */
>> +    if (bp->num_queues < 1 || bp->num_queues > MACB_MAX_QUEUES) {
> 
> Can this happen?

Yes, GEM in Zynq devices has single queue.

Currently, TAPRIO offload validation depends solely on the presence of 
.ndo_setup_tc. On Zynq-based devices, if the user configures the 
scheduler using tc replace, the operation fails at this point.

<...>

>> +    /* All validations passed - proceed with hardware configuration */
>> +    spin_lock_irqsave(&bp->lock, flags);
> 
> You can use guard(spinlock_irqsave)(&bp->lock) or 
> scoped_guard(spinlock_irqsave, &bp->lock)
>

ok, will leverage scoped_guard(spinlock_irqsave, &bp->lock).

>> +
>> +    /* Disable ENST queues if running before configuring */
>> +    if (gem_readl(bp, ENST_CONTROL))
> 
> Is this read necessary?
> 

Not necessary, I thought of disabling only if enabled.
But, will disable directly.

>> +        gem_writel(bp, ENST_CONTROL,
>> +               GENMASK(bp->num_queues - 1, 0) << 
>> GEM_ENST_DISABLE_QUEUE_OFFSET);
> 
> This could be replaced by GEM_BF(GENMASK(...), ENST_DISABLE_QUEUE) if 
> you define GEM_ENST_DISABLE_QUEUE_SIZE along with 
> GEM_ENST_DISABLE_QUEUE_OFFSET.
> 

I can leverage bp->queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET.
And remove GEM_ENST_ENABLE_QUEUE(hw_q) and GEM_ENST_DISABLE_QUEUE(hw_q) 
implementations.

>> +
>> +    for (i = 0; i < conf->num_entries; i++) {
>> +        queue = &bp->queues[enst_queue[i].queue_id];
>> +        /* Configure queue timing registers */
>> +        queue_writel(queue, ENST_START_TIME, 
>> enst_queue[i].start_time_mask);
>> +        queue_writel(queue, ENST_ON_TIME, enst_queue[i].on_time_bytes);
>> +        queue_writel(queue, ENST_OFF_TIME, 
>> enst_queue[i].off_time_bytes);
>> +    }
>> +
>> +    /* Enable ENST for all configured queues in one write */
>> +    gem_writel(bp, ENST_CONTROL, configured_queues);
> 
> Can this function be executed while other queues are configured? If so, 
> would the configured_queues contains it (as well as conf)?
> 

No, the tc add/replace command re-configures all queues, replacing any 
previous setup. Parameters such as START_TIME, ON_TIME, and CYCLE_TIME 
are recalculated based on the new configuration.

>> +    spin_unlock_irqrestore(&bp->lock, flags);
>> +
>> +    netdev_info(ndev, "TAPRIO configuration completed successfully: 
>> %lu entries, %d queues configured\n",
>> +            conf->num_entries, hweight32(configured_queues));
>> +
>> +cleanup:
>> +    kfree(enst_queue);
> 
> With the suggestions above, this could be dropped.
> 

ok.


> Thank you,
> Claudiu
> 
>> +    return err;
>> +}
>> +
>>   static const struct net_device_ops macb_netdev_ops = {
>>       .ndo_open        = macb_open,
>>       .ndo_stop        = macb_close,
> 

Thanks
-- 
🙏 vineeth


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface
  2025-07-26 12:29   ` claudiu beznea (tuxon)
@ 2025-07-29  9:38     ` Karumanchi, Vineeth
  2025-07-31  9:07       ` Claudiu Beznea
  0 siblings, 1 reply; 22+ messages in thread
From: Karumanchi, Vineeth @ 2025-07-29  9:38 UTC (permalink / raw)
  To: claudiu.beznea, vineeth.karumanchi, nicolas.ferre, andrew+netdev,
	davem, edumazet, kuba, pabeni
  Cc: git, netdev, linux-kernel

Hi Claudiu,


On 7/26/2025 5:59 PM, claudiu beznea (tuxon) wrote:
<...>
>> +    int err = 0;
>> +
>> +    switch (taprio->cmd) {
>> +    case TAPRIO_CMD_REPLACE:
>> +        err = macb_taprio_setup_replace(ndev, taprio);
>> +        break;
>> +    case TAPRIO_CMD_DESTROY:
>> +        macb_taprio_destroy(ndev);
> 
> macb_taprio_setup_replace() along with macb_taprio_destroy() touch HW 
> registers. Could macb_setup_taprio() be called when the interface is 
> runtime suspended?
> 
> 

Nice catch!

I will leverage pm_runtime_suspended(&pdev->dev) check before configuring.

>> +        break;
>> +    default:
>> +        err = -EOPNOTSUPP;
>> +    }
>> +
>> +    return err;
>> +}
>> +
>> +static int macb_setup_tc(struct net_device *dev, enum tc_setup_type 
>> type, void *type_data)
>> +{
>> +    if (!dev || !type_data)
>> +        return -EINVAL;
>> +
>> +    switch (type) {
>> +    case TC_SETUP_QDISC_TAPRIO:
>> +        return macb_setup_taprio(dev, type_data);
> 
> Same here.
> 
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +}
>> +
>>   static const struct net_device_ops macb_netdev_ops = {
>>       .ndo_open        = macb_open,
>>       .ndo_stop        = macb_close,
>> @@ -4284,6 +4316,7 @@ static const struct net_device_ops 
>> macb_netdev_ops = {
>>       .ndo_features_check    = macb_features_check,
>>       .ndo_hwtstamp_set    = macb_hwtstamp_set,
>>       .ndo_hwtstamp_get    = macb_hwtstamp_get,
>> +    .ndo_setup_tc        = macb_setup_tc,
> 
> This patch (or parts of it) should be merged with the previous ones. 
> Otherwise you introduce patches with code that is unused.
> 

Clubbing all comments on patch organization:
I see that patch series gets merged into 2 set only.

1/6 + 2/6 + 3/6 + 4/6 + 5/6 ==> 1/2
6/6 ==> 2/2

Please let me know your thoughts or suggestions.


Thanks
-- 
🙏 vineeth


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support
  2025-07-23 19:05   ` Simon Horman
@ 2025-07-29  9:42     ` Karumanchi, Vineeth
  0 siblings, 0 replies; 22+ messages in thread
From: Karumanchi, Vineeth @ 2025-07-29  9:42 UTC (permalink / raw)
  To: horms, vineeth.karumanchi
  Cc: nicolas.ferre, claudiu.beznea, andrew+netdev, davem, edumazet,
	kuba, pabeni, git, netdev, linux-kernel

Hi Simon,

On 7/24/2025 12:35 AM, Simon Horman wrote:

<...>

>> @@ -5345,7 +5349,7 @@ static const struct macb_config sama7g5_emac_config = {
>>   static const struct macb_config versal_config = {
>>   	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
>>   		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK |
>> -		MACB_CAPS_QUEUE_DISABLE,
>> +		MACB_CAPS_QUEUE_DISABLE, MACB_CAPS_QBV,
> Hi Vineeth,
> 
> TL;DR: I think you mean
> 
> 		MACB_CAPS_QUEUE_DISABLE | MACB_CAPS_QBV,
> 		                       ^^^
> 

Yes, since there's no strict validation for the presence of 
NETIF_F_HW_TC, the tc add/replace command succeeded. I've submitted an 
RFC patch to enforce stricter checks for NETIF_F_HW_TC within 
dev->hw_features

> I assume that the intention here is to set the MACB_CAPS_QBV bit of .caps.
> However, because there is a comma rather than a pipe between
> it and MACB_CAPS_QUEUE_DISABLE the effect is to leave .caps as
> it was before, and set .dma_burst_length to MACB_CAPS_QBV.
> .dma_burst_length is then overwritten on the following line.
> 
> Flagged by W=1 builds with Clang 20.1.8 and 15.1.0.
> 
> Please build your patches with W=1 and try to avoid adding warnings
> it flags.
> 
> Also, while we are here, it would be nice to fix up the line wrapping so
> the adjacent code is 80 columns wide or less, as is still preferred in
> Networking code.
> 
> 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
> 		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
> 		MACB_CAPS_NEED_TSUCLK | MACB_CAPS_QUEUE_DISABLE |
> 		MACB_CAPS_QBV,
> 

OK.

Thanks,
-- 
🙏 vineeth


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support
  2025-07-29  8:59     ` Karumanchi, Vineeth
@ 2025-07-31  9:07       ` Claudiu Beznea
  0 siblings, 0 replies; 22+ messages in thread
From: Claudiu Beznea @ 2025-07-31  9:07 UTC (permalink / raw)
  To: Karumanchi, Vineeth, vineeth.karumanchi, nicolas.ferre,
	andrew+netdev, davem, edumazet, kuba, pabeni
  Cc: git, netdev, linux-kernel



On 29.07.2025 11:59, Karumanchi, Vineeth wrote:
> Hi Claudiu,
> 
> Thanks for the review.
> 
> On 7/26/2025 5:55 PM, claudiu beznea (tuxon) wrote:
>>
>>
>> On 7/22/25 18:41, Vineeth Karumanchi wrote:
>>> Implement Time-Aware Traffic Scheduling (TAPRIO) hardware offload for
> 
> <..>
> 
>>
>> as it is used along with conf->num_entries which has size_t type.
>>
>>> +
>>> +    /* Validate queue configuration */
>>> +    if (bp->num_queues < 1 || bp->num_queues > MACB_MAX_QUEUES) {
>>
>> Can this happen?
> 
> Yes, GEM in Zynq devices has single queue.

I was asking as it looked to me that this validates the number of queues
the IP supports, which should have already been validated in probe.

> 
> Currently, TAPRIO offload validation depends solely on the presence
> of .ndo_setup_tc. On Zynq-based devices, if the user configures the
> scheduler using tc replace, the operation fails at this point.

I can't see how. That should translate into:

if (1 < 1 || 1 > 8)

which is in the end:

if (0)

Maybe it fails due to some other condition?

> 
> <...>
> 
>>> +    /* All validations passed - proceed with hardware configuration */
>>> +    spin_lock_irqsave(&bp->lock, flags);
>>
>> You can use guard(spinlock_irqsave)(&bp->lock) or
>> scoped_guard(spinlock_irqsave, &bp->lock)
>>
> 
> ok, will leverage scoped_guard(spinlock_irqsave, &bp->lock).
> 
>>> +
>>> +    /* Disable ENST queues if running before configuring */
>>> +    if (gem_readl(bp, ENST_CONTROL))
>>
>> Is this read necessary?
>>
> 
> Not necessary, I thought of disabling only if enabled.
> But, will disable directly.
> 
>>> +        gem_writel(bp, ENST_CONTROL,
>>> +               GENMASK(bp->num_queues - 1, 0) <<
>>> GEM_ENST_DISABLE_QUEUE_OFFSET);
>>
>> This could be replaced by GEM_BF(GENMASK(...), ENST_DISABLE_QUEUE) if you
>> define GEM_ENST_DISABLE_QUEUE_SIZE along with GEM_ENST_DISABLE_QUEUE_OFFSET.
>>
> 
> I can leverage bp->queue_mask << GEM_ENST_DISABLE_QUEUE_OFFSET.
> And remove GEM_ENST_ENABLE_QUEUE(hw_q) and GEM_ENST_DISABLE_QUEUE(hw_q)
> implementations.
> 
>>> +
>>> +    for (i = 0; i < conf->num_entries; i++) {
>>> +        queue = &bp->queues[enst_queue[i].queue_id];
>>> +        /* Configure queue timing registers */
>>> +        queue_writel(queue, ENST_START_TIME,
>>> enst_queue[i].start_time_mask);
>>> +        queue_writel(queue, ENST_ON_TIME, enst_queue[i].on_time_bytes);
>>> +        queue_writel(queue, ENST_OFF_TIME, enst_queue[i].off_time_bytes);
>>> +    }
>>> +
>>> +    /* Enable ENST for all configured queues in one write */
>>> +    gem_writel(bp, ENST_CONTROL, configured_queues);
>>
>> Can this function be executed while other queues are configured? If so,
>> would the configured_queues contains it (as well as conf)?
>>
> 
> No, the tc add/replace command re-configures all queues, replacing any
> previous setup. Parameters such as START_TIME, ON_TIME, and CYCLE_TIME are
> recalculated based on the new configuration.
> 
>>> +    spin_unlock_irqrestore(&bp->lock, flags);
>>> +
>>> +    netdev_info(ndev, "TAPRIO configuration completed successfully: %lu
>>> entries, %d queues configured\n",
>>> +            conf->num_entries, hweight32(configured_queues));
>>> +
>>> +cleanup:
>>> +    kfree(enst_queue);
>>
>> With the suggestions above, this could be dropped.
>>
> 
> ok.

Please check the documentation pointed by Andrew. With that, my suggestion
here should be dropped.

Thank you,
Claudiu

> 
> 
>> Thank you,
>> Claudiu
>>
>>> +    return err;
>>> +}
>>> +
>>>   static const struct net_device_ops macb_netdev_ops = {
>>>       .ndo_open        = macb_open,
>>>       .ndo_stop        = macb_close,
>>
> 
> Thanks


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface
  2025-07-29  9:38     ` Karumanchi, Vineeth
@ 2025-07-31  9:07       ` Claudiu Beznea
  0 siblings, 0 replies; 22+ messages in thread
From: Claudiu Beznea @ 2025-07-31  9:07 UTC (permalink / raw)
  To: Karumanchi, Vineeth, vineeth.karumanchi, nicolas.ferre,
	andrew+netdev, davem, edumazet, kuba, pabeni
  Cc: git, netdev, linux-kernel



On 29.07.2025 12:38, Karumanchi, Vineeth wrote:
> Hi Claudiu,
> 
> 
> On 7/26/2025 5:59 PM, claudiu beznea (tuxon) wrote:
> <...>
>>> +    int err = 0;
>>> +
>>> +    switch (taprio->cmd) {
>>> +    case TAPRIO_CMD_REPLACE:
>>> +        err = macb_taprio_setup_replace(ndev, taprio);
>>> +        break;
>>> +    case TAPRIO_CMD_DESTROY:
>>> +        macb_taprio_destroy(ndev);
>>
>> macb_taprio_setup_replace() along with macb_taprio_destroy() touch HW
>> registers. Could macb_setup_taprio() be called when the interface is
>> runtime suspended?
>>
>>
> 
> Nice catch!
> 
> I will leverage pm_runtime_suspended(&pdev->dev) check before configuring.
> 
>>> +        break;
>>> +    default:
>>> +        err = -EOPNOTSUPP;
>>> +    }
>>> +
>>> +    return err;
>>> +}
>>> +
>>> +static int macb_setup_tc(struct net_device *dev, enum tc_setup_type
>>> type, void *type_data)
>>> +{
>>> +    if (!dev || !type_data)
>>> +        return -EINVAL;
>>> +
>>> +    switch (type) {
>>> +    case TC_SETUP_QDISC_TAPRIO:
>>> +        return macb_setup_taprio(dev, type_data);
>>
>> Same here.
>>
>>> +    default:
>>> +        return -EOPNOTSUPP;
>>> +    }
>>> +}
>>> +
>>>   static const struct net_device_ops macb_netdev_ops = {
>>>       .ndo_open        = macb_open,
>>>       .ndo_stop        = macb_close,
>>> @@ -4284,6 +4316,7 @@ static const struct net_device_ops macb_netdev_ops
>>> = {
>>>       .ndo_features_check    = macb_features_check,
>>>       .ndo_hwtstamp_set    = macb_hwtstamp_set,
>>>       .ndo_hwtstamp_get    = macb_hwtstamp_get,
>>> +    .ndo_setup_tc        = macb_setup_tc,
>>
>> This patch (or parts of it) should be merged with the previous ones.
>> Otherwise you introduce patches with code that is unused.
>>
> 
> Clubbing all comments on patch organization:
> I see that patch series gets merged into 2 set only.
> 
> 1/6 + 2/6 + 3/6 + 4/6 + 5/6 ==> 1/2
> 6/6 ==> 2/2

That should be good.

Thank you,
Claudiu

> 
> Please let me know your thoughts or suggestions.
> 
> 
> Thanks


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2025-07-31  9:07 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-22 15:41 [PATCH net-next 0/6] net: macb: Add TAPRIO traffic scheduling support Vineeth Karumanchi
2025-07-22 15:41 ` [PATCH net-next 1/6] net: macb: Define ENST hardware registers for time-aware scheduling Vineeth Karumanchi
2025-07-26 12:23   ` claudiu beznea (tuxon)
2025-07-22 15:41 ` [PATCH net-next 2/6] net: macb: Integrate ENST timing parameters and hardware unit conversion Vineeth Karumanchi
2025-07-26 12:24   ` claudiu beznea (tuxon)
2025-07-22 15:41 ` [PATCH net-next 3/6] net: macb: Add IEEE 802.1Qbv TAPRIO REPLACE command offload support Vineeth Karumanchi
2025-07-23 10:02   ` kernel test robot
2025-07-26 12:25   ` claudiu beznea (tuxon)
2025-07-26 15:32     ` Andrew Lunn
2025-07-29  8:59     ` Karumanchi, Vineeth
2025-07-31  9:07       ` Claudiu Beznea
2025-07-22 15:41 ` [PATCH net-next 4/6] net: macb: Implement TAPRIO DESTROY command offload for gate cleanup Vineeth Karumanchi
2025-07-26 12:26   ` claudiu beznea (tuxon)
2025-07-22 15:41 ` [PATCH net-next 5/6] net: macb: Implement TAPRIO TC offload command interface Vineeth Karumanchi
2025-07-26 12:29   ` claudiu beznea (tuxon)
2025-07-29  9:38     ` Karumanchi, Vineeth
2025-07-31  9:07       ` Claudiu Beznea
2025-07-28 16:31   ` kernel test robot
2025-07-22 15:41 ` [PATCH net-next 6/6] net: macb: Add MACB_CAPS_QBV capability flag for IEEE 802.1Qbv support Vineeth Karumanchi
2025-07-23 19:05   ` Simon Horman
2025-07-29  9:42     ` Karumanchi, Vineeth
2025-07-24  0:46   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).