* [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).