[PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[MD Danish Anwar]

From: Roger Quadros <rogerq@ti.com>

The Time-Aware Shaper (TAS) is a key feature of the Enhanced Scheduled
Traffic (EST) mechanism defined in IEEE 802.1Q-2018. This patch adds TAS
support for the ICSSG driver by interacting with the ICSSG firmware to
manage gate control lists, cycle times, and other TAS parameters.

The firmware maintains active and shadow lists. The driver updates the
operating list using API `tas_update_oper_list()` which,
- Updates firmware list pointers via `tas_update_fw_list_pointers`.
- Writes gate masks, window end times, and clears unused entries in the
  shadow list.
- Updates gate close times and Max SDU values for each queue.
- Triggers list changes using `tas_set_trigger_list_change`, which
  - Computes cycle count (base-time % cycle-time) and extend (base-time %
    cycle-time)
  - Writes cycle time, cycle count, and extend values to firmware memory.
  - base-time being in past or base-time not being a multiple of
    cycle-time is taken care by the firmware. Driver just writes these
    variable for firmware and firmware takes care of the scheduling.
  - If base-time is not a multiple of cycle-time, the value of extend
    (base-time % cycle-time) is used by the firmware to extend the last
    cycle.
  - Sets `config_change` and `config_pending` flags to notify firmware of
    the new shadow list and its readiness for activation.
  - Sends the `ICSSG_EMAC_PORT_TAS_TRIGGER` r30 command to ask firmware to
    swap active and shadow lists.
- Waits for the firmware to clear the `config_change` flag before
  completing the update and returning successfully.

This implementation ensures seamless TAS functionality by offloading
scheduling complexities to the firmware.

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
v9 - v10:
There has been significant changes since v9. I have tried to address all
the comments given by Vladimir Oltean <vladimir.oltean@nxp.com> on v9
*) Made the driver depend on NET_SCH_TAPRIO || NET_SCH_TAPRIO=n for TAS
*) Used MACRO for max sdu size instead of magic number
*) Kept `tas->state = state` outside of the switch case in `tas_set_state`
*) Implemented TC_QUERY_CAPS case in `icssg_qos_ndo_setup_tc`
*) Calling `tas_update_fw_list_pointers` only once in
   `tas_update_oper_list` as the second call as unnecessary.
*) Moved the check for TAS_MAX_CYCLE_TIME to beginning of
   `emac_taprio_replace`
*) Added `__packed` to structures in `icssg_qos.h`
*) Modified implementation of `tas_set_trigger_list_change` to handle
   cases where base-time isn't a multiple of cycle-time. For this a new
   variable extend has to be calculated as base-time % cycle-time. This
   variable is used by firmware to extend the last cycle.
*) The API prueth_iep_gettime() and prueth_iep_settime() also needs to be
   adjusted according to the cycle time extension. These changes are also
   taken care in this patch.

v9 https://lore.kernel.org/all/20240531044512.981587-3-danishanwar@ti.com/

 drivers/net/ethernet/ti/Kconfig               |   1 +
 drivers/net/ethernet/ti/Makefile              |   2 +-
 drivers/net/ethernet/ti/icssg/icssg_prueth.c  |   7 +
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   2 +
 drivers/net/ethernet/ti/icssg/icssg_qos.c     | 310 ++++++++++++++++++
 drivers/net/ethernet/ti/icssg/icssg_qos.h     | 112 +++++++
 .../net/ethernet/ti/icssg/icssg_switch_map.h  |   6 +
 7 files changed, 439 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.c
 create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.h

diff --git a/drivers/net/ethernet/ti/Kconfig b/drivers/net/ethernet/ti/Kconfig
index a07c910c497a..a69a2220b20e 100644
--- a/drivers/net/ethernet/ti/Kconfig
+++ b/drivers/net/ethernet/ti/Kconfig
@@ -192,6 +192,7 @@ config TI_ICSSG_PRUETH
 	depends on NET_SWITCHDEV
 	depends on ARCH_K3 && OF && TI_K3_UDMA_GLUE_LAYER
 	depends on PTP_1588_CLOCK_OPTIONAL
+	depends on NET_SCH_TAPRIO || NET_SCH_TAPRIO=n
 	help
 	  Support dual Gigabit Ethernet ports over the ICSSG PRU Subsystem.
 	  This subsystem is available starting with the AM65 platform.
diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
index cbcf44806924..d0ff793a8639 100644
--- a/drivers/net/ethernet/ti/Makefile
+++ b/drivers/net/ethernet/ti/Makefile
@@ -32,7 +32,7 @@ ti-am65-cpsw-nuss-$(CONFIG_TI_K3_AM65_CPSW_SWITCHDEV) += am65-cpsw-switchdev.o
 obj-$(CONFIG_TI_K3_AM65_CPTS) += am65-cpts.o
 
 obj-$(CONFIG_TI_ICSSG_PRUETH) += icssg-prueth.o icssg.o
-icssg-prueth-y := icssg/icssg_prueth.o icssg/icssg_switchdev.o
+icssg-prueth-y := icssg/icssg_prueth.o icssg/icssg_switchdev.o icssg/icssg_qos.o
 
 obj-$(CONFIG_TI_ICSSG_PRUETH_SR1) += icssg-prueth-sr1.o icssg.o
 icssg-prueth-sr1-y := icssg/icssg_prueth_sr1.o
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 443f90fa6557..a6f3e0f797a6 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -453,6 +453,7 @@ static u64 prueth_iep_gettime(void *clockops_data, struct ptp_system_timestamp *
 
 	ts = ((u64)hi_rollover_count) << 23 | iepcount_hi;
 	ts = ts * (u64)IEP_DEFAULT_CYCLE_TIME_NS + iepcount_lo;
+	ts += readl(prueth->shram.va + TIMESYNC_CYCLE_EXTN_TIME);
 
 	return ts;
 }
@@ -491,6 +492,9 @@ static void prueth_iep_settime(void *clockops_data, u64 ns)
 		usleep_range(500, 1000);
 	}
 
+	/* Clear the Cycle extension adjustments */
+	writel(0, emac->dram.va + TIMESYNC_CYCLE_EXTN_TIME);
+
 	dev_err(emac->prueth->dev, "settime timeout\n");
 }
 
@@ -1160,6 +1164,7 @@ static const struct net_device_ops emac_netdev_ops = {
 	.ndo_vlan_rx_kill_vid = emac_ndo_vlan_rx_del_vid,
 	.ndo_bpf = emac_ndo_bpf,
 	.ndo_xdp_xmit = emac_xdp_xmit,
+	.ndo_setup_tc = icssg_qos_ndo_setup_tc,
 };
 
 static int prueth_netdev_init(struct prueth *prueth,
@@ -1297,6 +1302,8 @@ static int prueth_netdev_init(struct prueth *prueth,
 		      HRTIMER_MODE_REL_PINNED);
 	prueth->emac[mac] = emac;
 
+	icssg_qos_tas_init(ndev);
+
 	return 0;
 
 free:
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 23c465f1ce7f..b88d6ea527fa 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -41,6 +41,7 @@
 #include "icssg_config.h"
 #include "icss_iep.h"
 #include "icssg_switch_map.h"
+#include "icssg_qos.h"
 
 #define PRUETH_MAX_MTU          (2000 - ETH_HLEN - ETH_FCS_LEN)
 #define PRUETH_MIN_PKT_SIZE     (VLAN_ETH_ZLEN)
@@ -240,6 +241,7 @@ struct prueth_emac {
 	struct netdev_hw_addr_list vlan_mcast_list[MAX_VLAN_ID];
 	struct bpf_prog *xdp_prog;
 	struct xdp_attachment_info xdpi;
+	struct prueth_qos qos;
 };
 
 /* The buf includes headroom compatible with both skb and xdpf */
diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.c b/drivers/net/ethernet/ti/icssg/icssg_qos.c
new file mode 100644
index 000000000000..b42c896675d3
--- /dev/null
+++ b/drivers/net/ethernet/ti/icssg/icssg_qos.c
@@ -0,0 +1,310 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Texas Instruments ICSSG PRUETH QoS submodule
+ * Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#include <linux/printk.h>
+#include "icssg_prueth.h"
+#include "icssg_switch_map.h"
+
+static void tas_update_fw_list_pointers(struct prueth_emac *emac)
+{
+	struct tas_config *tas = &emac->qos.tas.config;
+
+	if ((readb(tas->active_list)) == TAS_LIST0) {
+		tas->fw_active_list = emac->dram.va + TAS_GATE_MASK_LIST0;
+		tas->fw_shadow_list = emac->dram.va + TAS_GATE_MASK_LIST1;
+	} else {
+		tas->fw_active_list = emac->dram.va + TAS_GATE_MASK_LIST1;
+		tas->fw_shadow_list = emac->dram.va + TAS_GATE_MASK_LIST0;
+	}
+}
+
+static void tas_update_maxsdu_table(struct prueth_emac *emac)
+{
+	struct tas_config *tas = &emac->qos.tas.config;
+	u16 __iomem *max_sdu_tbl_ptr;
+	u8 gate_idx;
+
+	max_sdu_tbl_ptr = emac->dram.va + TAS_QUEUE_MAX_SDU_LIST;
+
+	/* In the tc-taprio UAPI, a max-sdu value of 0 is special and means "no
+	 * maxSDU limit for this TX queue". The firmware doesn't treat max-sdu
+	 * value of 0 specially. As a result the driver needs to change the
+	 * max-sdu value of 0 to PRUETH_MAX_MTU so that the firmware can treat
+	 * it accordingly.
+	 */
+	for (gate_idx = 0; gate_idx < TAS_MAX_NUM_QUEUES; gate_idx++) {
+		if (!tas->max_sdu_table.max_sdu[gate_idx])
+			tas->max_sdu_table.max_sdu[gate_idx] = PRUETH_MAX_MTU;
+		writew(tas->max_sdu_table.max_sdu[gate_idx], &max_sdu_tbl_ptr[gate_idx]);
+	}
+}
+
+static void tas_reset(struct prueth_emac *emac)
+{
+	struct tas_config *tas = &emac->qos.tas.config;
+	int i;
+
+	for (i = 0; i < TAS_MAX_NUM_QUEUES; i++)
+		tas->max_sdu_table.max_sdu[i] = PRUETH_MAX_MTU;
+
+	tas_update_maxsdu_table(emac);
+
+	memset_io(tas->fw_active_list, 0, sizeof(*tas->fw_active_list));
+	memset_io(tas->fw_shadow_list, 0, sizeof(*tas->fw_shadow_list));
+}
+
+static int tas_set_state(struct prueth_emac *emac, enum tas_state state)
+{
+	struct tas_config *tas = &emac->qos.tas.config;
+	int ret;
+
+	if (tas->state == state)
+		return 0;
+
+	switch (state) {
+	case TAS_STATE_RESET:
+		tas_reset(emac);
+		ret = icssg_set_port_state(emac, ICSSG_EMAC_PORT_TAS_RESET);
+		break;
+	case TAS_STATE_ENABLE:
+		ret = icssg_set_port_state(emac, ICSSG_EMAC_PORT_TAS_ENABLE);
+		break;
+	case TAS_STATE_DISABLE:
+		ret = icssg_set_port_state(emac, ICSSG_EMAC_PORT_TAS_DISABLE);
+		break;
+	}
+
+	if (!ret)
+		tas->state = state;
+
+	return ret;
+}
+
+static int tas_set_trigger_list_change(struct prueth_emac *emac)
+{
+	struct tc_taprio_qopt_offload *admin_list = emac->qos.tas.taprio_admin;
+	struct tas_config *tas = &emac->qos.tas.config;
+	u32 change_cycle_count;
+	u32 cycle_time;
+	u64 base_time;
+	u32 extend;
+
+	/* IEP clock has a hardware errata due to which it wraps around exactly
+	 * once every taprio cycle. To compensate for that, adjust cycle time
+	 * by the wrap around time which is stored in emac->iep->def_inc
+	 */
+	cycle_time = admin_list->cycle_time - emac->iep->def_inc;
+	base_time = admin_list->base_time;
+
+	change_cycle_count = base_time / cycle_time;
+	extend = base_time % cycle_time;
+
+	writel(cycle_time, emac->dram.va + TAS_ADMIN_CYCLE_TIME);
+	writel(change_cycle_count, emac->dram.va + TAS_CONFIG_CHANGE_CYCLE_COUNT);
+	writeb(admin_list->num_entries, emac->dram.va + TAS_ADMIN_LIST_LENGTH);
+	writel(extend, emac->dram.va + TAS_CONFIG_CYCLE_EXTEND);
+
+	/* config_change cleared by f/w to ack reception of new shadow list */
+	writeb(1, &tas->config_list->config_change);
+	/* config_pending cleared by f/w when new shadow list is copied to active list */
+	writeb(1, &tas->config_list->config_pending);
+
+	return icssg_set_port_state(emac, ICSSG_EMAC_PORT_TAS_TRIGGER);
+}
+
+static int tas_update_oper_list(struct prueth_emac *emac)
+{
+	struct tc_taprio_qopt_offload *admin_list = emac->qos.tas.taprio_admin;
+	struct tas_config *tas = &emac->qos.tas.config;
+	u32 tas_acc_gate_close_time = 0;
+	u8 idx, gate_idx, val;
+	int ret;
+
+	tas_update_fw_list_pointers(emac);
+
+	for (idx = 0; idx < admin_list->num_entries; idx++) {
+		writeb(admin_list->entries[idx].gate_mask,
+		       &tas->fw_shadow_list->gate_mask_list[idx]);
+		tas_acc_gate_close_time += admin_list->entries[idx].interval;
+
+		/* extend last entry till end of cycle time */
+		if (idx == admin_list->num_entries - 1)
+			writel(admin_list->cycle_time,
+			       &tas->fw_shadow_list->win_end_time_list[idx]);
+		else
+			writel(tas_acc_gate_close_time,
+			       &tas->fw_shadow_list->win_end_time_list[idx]);
+	}
+
+	/* clear remaining entries */
+	for (idx = admin_list->num_entries; idx < TAS_MAX_CMD_LISTS; idx++) {
+		writeb(0, &tas->fw_shadow_list->gate_mask_list[idx]);
+		writel(0, &tas->fw_shadow_list->win_end_time_list[idx]);
+	}
+
+	/* update the Array of gate close time for each queue in each window */
+	for (idx = 0 ; idx < admin_list->num_entries; idx++) {
+		/* On Linux, only PRUETH_MAX_TX_QUEUES are supported per port */
+		for (gate_idx = 0; gate_idx < PRUETH_MAX_TX_QUEUES; gate_idx++) {
+			u8 gate_mask_list_idx = readb(&tas->fw_shadow_list->gate_mask_list[idx]);
+			u32 gate_close_time = 0;
+
+			if (gate_mask_list_idx & BIT(gate_idx))
+				gate_close_time = readl(&tas->fw_shadow_list->win_end_time_list[idx]);
+
+			writel(gate_close_time,
+			       &tas->fw_shadow_list->gate_close_time_list[idx][gate_idx]);
+		}
+	}
+
+	/* Update the maxsdu table for firmware */
+	tas_update_maxsdu_table(emac);
+
+	/* tell f/w to swap active & shadow list */
+	ret = tas_set_trigger_list_change(emac);
+	if (ret) {
+		netdev_err(emac->ndev, "failed to swap f/w config list: %d\n", ret);
+		return ret;
+	}
+
+	/* Wait for completion */
+	ret = readb_poll_timeout(&tas->config_list->config_change, val, !val,
+				 USEC_PER_MSEC, 10 * USEC_PER_MSEC);
+	if (ret) {
+		netdev_err(emac->ndev, "TAS list change completion time out\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int emac_taprio_replace(struct net_device *ndev,
+			       struct tc_taprio_qopt_offload *taprio)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	int ret;
+
+	if (taprio->cycle_time_extension) {
+		NL_SET_ERR_MSG_MOD(taprio->extack, "Cycle time extension not supported");
+		return -EOPNOTSUPP;
+	}
+
+	if (taprio->cycle_time > TAS_MAX_CYCLE_TIME) {
+		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "cycle_time %llu is more than max supported cycle_time",
+				       taprio->cycle_time);
+		return -EINVAL;
+	}
+
+	if (taprio->cycle_time < TAS_MIN_CYCLE_TIME) {
+		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "cycle_time %llu is less than min supported cycle_time %d",
+				       taprio->cycle_time, TAS_MIN_CYCLE_TIME);
+		return -EINVAL;
+	}
+
+	if (taprio->num_entries > TAS_MAX_CMD_LISTS) {
+		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "num_entries %lu is more than max supported entries %d",
+				       taprio->num_entries, TAS_MAX_CMD_LISTS);
+		return -EINVAL;
+	}
+
+	if (emac->qos.tas.taprio_admin)
+		taprio_offload_free(emac->qos.tas.taprio_admin);
+
+	emac->qos.tas.taprio_admin = taprio_offload_get(taprio);
+	ret = tas_update_oper_list(emac);
+	if (ret)
+		goto clear_taprio;
+
+	ret = tas_set_state(emac, TAS_STATE_ENABLE);
+	if (ret)
+		goto clear_taprio;
+
+	return 0;
+
+clear_taprio:
+	emac->qos.tas.taprio_admin = NULL;
+	taprio_offload_free(taprio);
+
+	return ret;
+}
+
+static int emac_taprio_destroy(struct net_device *ndev,
+			       struct tc_taprio_qopt_offload *taprio)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	int ret;
+
+	ret = tas_set_state(emac, TAS_STATE_DISABLE);
+	if (ret)
+		return ret;
+
+	return tas_set_state(emac, TAS_STATE_RESET);
+}
+
+static int emac_setup_taprio(struct net_device *ndev, void *type_data)
+{
+	struct tc_taprio_qopt_offload *taprio = type_data;
+	int ret;
+
+	switch (taprio->cmd) {
+	case TAPRIO_CMD_REPLACE:
+		ret = emac_taprio_replace(ndev, taprio);
+		break;
+	case TAPRIO_CMD_DESTROY:
+		ret = emac_taprio_destroy(ndev, taprio);
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
+static int emac_tc_query_caps(struct net_device *ndev, void *type_data)
+{
+	struct tc_query_caps_base *base = type_data;
+
+	switch (base->type) {
+	case TC_SETUP_QDISC_TAPRIO: {
+		struct tc_taprio_caps *caps = base->caps;
+
+		caps->gate_mask_per_txq = true;
+
+		return 0;
+	}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
+			   void *type_data)
+{
+	switch (type) {
+	case TC_SETUP_QDISC_TAPRIO:
+		return emac_setup_taprio(ndev, type_data);
+	case TC_QUERY_CAPS:
+		return emac_tc_query_caps(ndev, type_data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+EXPORT_SYMBOL_GPL(icssg_qos_ndo_setup_tc);
+
+void icssg_qos_tas_init(struct net_device *ndev)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct tas_config *tas;
+
+	tas = &emac->qos.tas.config;
+
+	tas->config_list = emac->dram.va + TAS_CONFIG_CHANGE_TIME;
+	tas->active_list = emac->dram.va + TAS_ACTIVE_LIST_INDEX;
+
+	tas_update_fw_list_pointers(emac);
+
+	tas_set_state(emac, TAS_STATE_RESET);
+}
+EXPORT_SYMBOL_GPL(icssg_qos_tas_init);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.h b/drivers/net/ethernet/ti/icssg/icssg_qos.h
new file mode 100644
index 000000000000..60cd50c661b6
--- /dev/null
+++ b/drivers/net/ethernet/ti/icssg/icssg_qos.h
@@ -0,0 +1,112 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#ifndef __NET_TI_ICSSG_QOS_H
+#define __NET_TI_ICSSG_QOS_H
+
+#include <linux/atomic.h>
+#include <linux/netdevice.h>
+#include <net/pkt_sched.h>
+
+/* Maximum number of gate command entries in each list. */
+#define TAS_MAX_CMD_LISTS   (16)
+
+/* Maximum number of transmit queues supported by implementation */
+#define TAS_MAX_NUM_QUEUES  (8)
+
+/* Minimum cycle time supported by implementation (in ns) */
+#define TAS_MIN_CYCLE_TIME  (1000000)
+
+/* Minimum cycle time supported by implementation (in ns) */
+#define TAS_MAX_CYCLE_TIME  (4000000000)
+
+/* Minimum TAS window duration supported by implementation (in ns) */
+#define TAS_MIN_WINDOW_DURATION  (10000)
+
+/**
+ * enum tas_list_num - TAS list number
+ * @TAS_LIST0: TAS list number is 0
+ * @TAS_LIST1: TAS list number is 1
+ */
+enum tas_list_num {
+	TAS_LIST0 = 0,
+	TAS_LIST1 = 1
+};
+
+/**
+ * enum tas_state - State of TAS in firmware
+ * @TAS_STATE_DISABLE: TAS state machine is disabled.
+ * @TAS_STATE_ENABLE: TAS state machine is enabled.
+ * @TAS_STATE_RESET: TAS state machine is reset.
+ */
+enum tas_state {
+	TAS_STATE_DISABLE = 0,
+	TAS_STATE_ENABLE = 1,
+	TAS_STATE_RESET = 2,
+};
+
+/**
+ * struct tas_config_list - Config state machine variables
+ * @config_change_time: New list is copied at this time
+ * @config_change_error_counter: Incremented if admin->BaseTime < current time
+ *				 and TAS_enabled is true
+ * @config_pending: True if list update is pending
+ * @config_change: Set to true when application trigger updating of admin list
+ *		   to active list, cleared when configChangeTime is updated
+ */
+struct tas_config_list {
+	u64 config_change_time;
+	u32 config_change_error_counter;
+	u8 config_pending;
+	u8 config_change;
+} __packed;
+
+/* Max SDU table. See IEEE Std 802.1Q-2018 12.29.1.1 */
+struct tas_max_sdu_table {
+	u16 max_sdu[TAS_MAX_NUM_QUEUES];
+};
+
+/**
+ * struct tas_firmware_list - TAS List Structure based on firmware memory map
+ * @gate_mask_list: Window gate mask list
+ * @win_end_time_list: Window end time list
+ * @gate_close_time_list: Array of gate close time for each queue in each window
+ */
+struct tas_firmware_list {
+	u8 gate_mask_list[TAS_MAX_CMD_LISTS];
+	u32 win_end_time_list[TAS_MAX_CMD_LISTS];
+	u32 gate_close_time_list[TAS_MAX_CMD_LISTS][TAS_MAX_NUM_QUEUES];
+} __packed;
+
+/**
+ * struct tas_config - Main Time Aware Shaper Handle
+ * @state: TAS state
+ * @max_sdu_table: Max SDU table
+ * @config_list: Config change variables
+ * @active_list: Current operating list operating list
+ * @fw_active_list: Active List pointer, used by firmware
+ * @fw_shadow_list: Shadow List pointer, used by driver
+ */
+struct tas_config {
+	enum tas_state state;
+	struct tas_max_sdu_table max_sdu_table;
+	struct tas_config_list __iomem *config_list;
+	u8 __iomem *active_list;
+	struct tas_firmware_list __iomem *fw_active_list;
+	struct tas_firmware_list __iomem *fw_shadow_list;
+};
+
+struct prueth_qos_tas {
+	struct tc_taprio_qopt_offload *taprio_admin;
+	struct tas_config config;
+};
+
+struct prueth_qos {
+	struct prueth_qos_tas tas;
+};
+
+void icssg_qos_tas_init(struct net_device *ndev);
+int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
+			   void *type_data);
+#endif /* __NET_TI_ICSSG_QOS_H */
diff --git a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
index 490a9cc06fb0..e659a66bb7d7 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
@@ -46,6 +46,9 @@
 /* Same as P2_PORT_DF_VLAN_OFFSET */
 #define EMAC_ICSSG_SWITCH_PORT2_DEFAULT_VLAN_OFFSET        P2_PORT_DF_VLAN_OFFSET
 
+/* Time adjustment to be done due to Cycle extension */
+#define TIMESYNC_CYCLE_EXTN_TIME                           0x0028
+
 /* VLAN-FID Table offset. 4096 VIDs. 2B per VID = 8KB = 0x2000 */
 #define VLAN_STATIC_REG_TABLE_OFFSET                       0x0100
 
@@ -177,6 +180,9 @@
 /* Stores the table used for priority mapping. 1B per PCP/Queue */
 #define PORT_Q_PRIORITY_MAPPING_OFFSET                     0x003C
 
+/* Memory to store time value to be used for Cycle extension */
+#define TAS_CONFIG_CYCLE_EXTEND				   0x00A4
+
 /* Used to notify the FW of the current link speed */
 #define PORT_LINK_SPEED_OFFSET                             0x00A8
 

base-commit: 630cb33ccfcd04563598d0f0edd96c94ddf3352d
-- 
2.34.1

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[Vladimir Oltean]

On Fri, May 02, 2025 at 04:12:35PM +0530, MD Danish Anwar wrote:
> From: Roger Quadros <rogerq@ti.com>
> 
> The Time-Aware Shaper (TAS) is a key feature of the Enhanced Scheduled
> Traffic (EST) mechanism defined in IEEE 802.1Q-2018. This patch adds TAS
> support for the ICSSG driver by interacting with the ICSSG firmware to
> manage gate control lists, cycle times, and other TAS parameters.
> 
> The firmware maintains active and shadow lists. The driver updates the
> operating list using API `tas_update_oper_list()` which,
> - Updates firmware list pointers via `tas_update_fw_list_pointers`.
> - Writes gate masks, window end times, and clears unused entries in the
>   shadow list.
> - Updates gate close times and Max SDU values for each queue.
> - Triggers list changes using `tas_set_trigger_list_change`, which
>   - Computes cycle count (base-time % cycle-time) and extend (base-time %
>     cycle-time)
>   - Writes cycle time, cycle count, and extend values to firmware memory.
>   - base-time being in past or base-time not being a multiple of
>     cycle-time is taken care by the firmware. Driver just writes these
>     variable for firmware and firmware takes care of the scheduling.
>   - If base-time is not a multiple of cycle-time, the value of extend
>     (base-time % cycle-time) is used by the firmware to extend the last
>     cycle.
>   - Sets `config_change` and `config_pending` flags to notify firmware of
>     the new shadow list and its readiness for activation.
>   - Sends the `ICSSG_EMAC_PORT_TAS_TRIGGER` r30 command to ask firmware to
>     swap active and shadow lists.
> - Waits for the firmware to clear the `config_change` flag before
>   completing the update and returning successfully.
> 
> This implementation ensures seamless TAS functionality by offloading
> scheduling complexities to the firmware.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---

This is not a review comment - just wanted to mention that a week ago,
I've added a selftest for this feature at
tools/testing/selftests/net/forwarding/tc_taprio.sh, and I'm wondering
whether you have the necessary setup to give that a run. That would give
maintainers a bit more confidence when merging, that things work as
expected.

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[Vladimir Oltean]

It has been a long time since the last posting, everything has been
swapped out from my memory. Sorry if some comments are repeated.

On Fri, May 02, 2025 at 04:12:35PM +0530, MD Danish Anwar wrote:
> From: Roger Quadros <rogerq@ti.com>
> 
> The Time-Aware Shaper (TAS) is a key feature of the Enhanced Scheduled
> Traffic (EST) mechanism defined in IEEE 802.1Q-2018. This patch adds TAS
> support for the ICSSG driver by interacting with the ICSSG firmware to
> manage gate control lists, cycle times, and other TAS parameters.
> 
> The firmware maintains active and shadow lists. The driver updates the
> operating list using API `tas_update_oper_list()` which,
> - Updates firmware list pointers via `tas_update_fw_list_pointers`.
> - Writes gate masks, window end times, and clears unused entries in the
>   shadow list.
> - Updates gate close times and Max SDU values for each queue.
> - Triggers list changes using `tas_set_trigger_list_change`, which
>   - Computes cycle count (base-time % cycle-time) and extend (base-time %
>     cycle-time)

Please define the "cycle count" concept (local invention, not IEEE
standard). Also, cross-checking with the code, base-time % cycle-time is
incorrect here, that's not how you calculate it.

I'm afraid you also need to define the "extend" concept. It is not at
all clear what it does and how it does it. Does it have any relationship
with the CycleTimeExtension variables as documented by IEEE 802.1Q annex
Q.5 definitions?

A very compressed summary of the standard variable is this:
the CycleTimeExtension applies when:
- an Open schedule exists
- an Admin schedule is pending
- the AdminBaseTime is not an integer multiple of OperBaseTime + (N *
  OperCycleTime) - i.o.w. the admin schedule does not "line up" with the
  end of the oper schedule

The misalignment of the oper vs admin schedules might cause the very
last oper cycle to be truncated to an undesirably short value. The
OperCycleTimeExtension variable exists to prevent this, as such:

- If the length of the last oper cycle is < OperCycleTimeExtension,
  then this cycle does not execute at all. The gate states from the end
  of the next-to-last oper cycle remain in place (that cycle is extended)
  until the activation of the admin schedule at AdminBaseTime.

- If the length of the last oper cycle is >= OperCycleTimeExtension,
  this last cycle is left to execute until AdminBaseTime, and is
  potentially truncated during the switchover event (unless it perfectly
  lines up). Extension of the next-to-last oper cycle does not take
  place.

Is this the same functionality as the "extend" feature of the PRU
firmware - should I be reading the code and the commit message in this
key, in order to understand what it achieves?

>   - Writes cycle time, cycle count, and extend values to firmware memory.
>   - base-time being in past or base-time not being a multiple of
>     cycle-time is taken care by the firmware. Driver just writes these
>     variable for firmware and firmware takes care of the scheduling.

"base-time not being a multiple of cycle-time is taken care by the firmware":
To what extent is this true? You don't actually pass the base-time to
the firmware, so how would it know that it's not a multiple of cycle-time?

>   - If base-time is not a multiple of cycle-time, the value of extend
>     (base-time % cycle-time) is used by the firmware to extend the last
>     cycle.

I'm surprised to read this. Why does the firmware expect the base time
to be a multiple of the cycle time?

Also, I don't understand what the workaround achieves. If the "extend"
feature is similar to CycleTimeExtension, then it applies at the _end_
of the cycle. I.o.w. if you never change the cycle, it never applies.
How does that help address a problem which exists since the very first
cycle of the schedule (that it may be shifted relative to integer
multiples of the cycle time)?

And even assuming that a schedule change will take place - what's the
math that would suggest the "extend" feature does anything at all to
address the request to apply a phase-shifted schedule? The last cycle of
the oper schedule passes, the admin schedule becomes the new oper, and
then what? It still runs phase-aligned with its own cycle-time, but
misaligned with the user-provided base time, no?

The expectation is for all cycles to be shifted relative to N *
base-time, not just the first or last one. It doesn't "sound" like you
can achieve that using CycleTimeExtension (assuming that's what this
is), so better refuse those schedules which don't have the base-time you
need.

>   - Sets `config_change` and `config_pending` flags to notify firmware of
>     the new shadow list and its readiness for activation.
>   - Sends the `ICSSG_EMAC_PORT_TAS_TRIGGER` r30 command to ask firmware to
>     swap active and shadow lists.
> - Waits for the firmware to clear the `config_change` flag before
>   completing the update and returning successfully.
> 
> This implementation ensures seamless TAS functionality by offloading
> scheduling complexities to the firmware.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> v9 - v10:
> There has been significant changes since v9. I have tried to address all
> the comments given by Vladimir Oltean <vladimir.oltean@nxp.com> on v9
> *) Made the driver depend on NET_SCH_TAPRIO || NET_SCH_TAPRIO=n for TAS
> *) Used MACRO for max sdu size instead of magic number
> *) Kept `tas->state = state` outside of the switch case in `tas_set_state`
> *) Implemented TC_QUERY_CAPS case in `icssg_qos_ndo_setup_tc`
> *) Calling `tas_update_fw_list_pointers` only once in
>    `tas_update_oper_list` as the second call as unnecessary.
> *) Moved the check for TAS_MAX_CYCLE_TIME to beginning of
>    `emac_taprio_replace`
> *) Added `__packed` to structures in `icssg_qos.h`
> *) Modified implementation of `tas_set_trigger_list_change` to handle
>    cases where base-time isn't a multiple of cycle-time. For this a new
>    variable extend has to be calculated as base-time % cycle-time. This
>    variable is used by firmware to extend the last cycle.
> *) The API prueth_iep_gettime() and prueth_iep_settime() also needs to be
>    adjusted according to the cycle time extension. These changes are also
>    taken care in this patch.

Why? Given the explanation of CycleTimeExtension above, it makes no
sense to me why you would alter the gettime() and settime() values.

> 
> v9 https://lore.kernel.org/all/20240531044512.981587-3-danishanwar@ti.com/
> 
>  drivers/net/ethernet/ti/Kconfig               |   1 +
>  drivers/net/ethernet/ti/Makefile              |   2 +-
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  |   7 +
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   2 +
>  drivers/net/ethernet/ti/icssg/icssg_qos.c     | 310 ++++++++++++++++++
>  drivers/net/ethernet/ti/icssg/icssg_qos.h     | 112 +++++++
>  .../net/ethernet/ti/icssg/icssg_switch_map.h  |   6 +
>  7 files changed, 439 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.c
>  create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.h
> 
> +static int emac_taprio_replace(struct net_device *ndev,
> +			       struct tc_taprio_qopt_offload *taprio)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	int ret;
> +
> +	if (taprio->cycle_time_extension) {
> +		NL_SET_ERR_MSG_MOD(taprio->extack, "Cycle time extension not supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (taprio->cycle_time > TAS_MAX_CYCLE_TIME) {
> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "cycle_time %llu is more than max supported cycle_time",
> +				       taprio->cycle_time);

It would be better to also print here TAS_MAX_CYCLE_TIME, like TAS_MIN_CYCLE_TIME below.
Also, looping back a user-supplied value (taprio->cycle_time) is IMO not needed.

> +		return -EINVAL;
> +	}
> +
> +	if (taprio->cycle_time < TAS_MIN_CYCLE_TIME) {
> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "cycle_time %llu is less than min supported cycle_time %d",
> +				       taprio->cycle_time, TAS_MIN_CYCLE_TIME);
> +		return -EINVAL;
> +	}
> +
> +	if (taprio->num_entries > TAS_MAX_CMD_LISTS) {
> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "num_entries %lu is more than max supported entries %d",
> +				       taprio->num_entries, TAS_MAX_CMD_LISTS);
> +		return -EINVAL;
> +	}
> +
> +	if (emac->qos.tas.taprio_admin)
> +		taprio_offload_free(emac->qos.tas.taprio_admin);
> +
> +	emac->qos.tas.taprio_admin = taprio_offload_get(taprio);
> +	ret = tas_update_oper_list(emac);
> +	if (ret)
> +		goto clear_taprio;
> +
> +	ret = tas_set_state(emac, TAS_STATE_ENABLE);
> +	if (ret)
> +		goto clear_taprio;
> +
> +	return 0;
> +
> +clear_taprio:
> +	emac->qos.tas.taprio_admin = NULL;
> +	taprio_offload_free(taprio);
> +
> +	return ret;
> +}

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[MD Danish Anwar]

Hi Vladimir

On 06/05/25 7:47 pm, Vladimir Oltean wrote:
> On Fri, May 02, 2025 at 04:12:35PM +0530, MD Danish Anwar wrote:
>> From: Roger Quadros <rogerq@ti.com>
>>
>> The Time-Aware Shaper (TAS) is a key feature of the Enhanced Scheduled
>> Traffic (EST) mechanism defined in IEEE 802.1Q-2018. This patch adds TAS
>> support for the ICSSG driver by interacting with the ICSSG firmware to
>> manage gate control lists, cycle times, and other TAS parameters.
>>
>> The firmware maintains active and shadow lists. The driver updates the
>> operating list using API `tas_update_oper_list()` which,
>> - Updates firmware list pointers via `tas_update_fw_list_pointers`.
>> - Writes gate masks, window end times, and clears unused entries in the
>>   shadow list.
>> - Updates gate close times and Max SDU values for each queue.
>> - Triggers list changes using `tas_set_trigger_list_change`, which
>>   - Computes cycle count (base-time % cycle-time) and extend (base-time %
>>     cycle-time)
>>   - Writes cycle time, cycle count, and extend values to firmware memory.
>>   - base-time being in past or base-time not being a multiple of
>>     cycle-time is taken care by the firmware. Driver just writes these
>>     variable for firmware and firmware takes care of the scheduling.
>>   - If base-time is not a multiple of cycle-time, the value of extend
>>     (base-time % cycle-time) is used by the firmware to extend the last
>>     cycle.
>>   - Sets `config_change` and `config_pending` flags to notify firmware of
>>     the new shadow list and its readiness for activation.
>>   - Sends the `ICSSG_EMAC_PORT_TAS_TRIGGER` r30 command to ask firmware to
>>     swap active and shadow lists.
>> - Waits for the firmware to clear the `config_change` flag before
>>   completing the update and returning successfully.
>>
>> This implementation ensures seamless TAS functionality by offloading
>> scheduling complexities to the firmware.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> Reviewed-by: Simon Horman <horms@kernel.org>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
> 
> This is not a review comment - just wanted to mention that a week ago,
> I've added a selftest for this feature at
> tools/testing/selftests/net/forwarding/tc_taprio.sh, and I'm wondering
> whether you have the necessary setup to give that a run. That would give
> maintainers a bit more confidence when merging, that things work as
> expected.

I wasn't aware of this. I will check if I have the setup and try to test
it before posting future revisions.

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[MD Danish Anwar]

Hi Vladimir,

On 06/05/25 9:16 pm, Vladimir Oltean wrote:
> It has been a long time since the last posting, everything has been
> swapped out from my memory. Sorry if some comments are repeated.
> 

Yes. It has been almost a year since my last revision.

> On Fri, May 02, 2025 at 04:12:35PM +0530, MD Danish Anwar wrote:
>> From: Roger Quadros <rogerq@ti.com>
>>
>> The Time-Aware Shaper (TAS) is a key feature of the Enhanced Scheduled
>> Traffic (EST) mechanism defined in IEEE 802.1Q-2018. This patch adds TAS
>> support for the ICSSG driver by interacting with the ICSSG firmware to
>> manage gate control lists, cycle times, and other TAS parameters.
>>
>> The firmware maintains active and shadow lists. The driver updates the
>> operating list using API `tas_update_oper_list()` which,
>> - Updates firmware list pointers via `tas_update_fw_list_pointers`.
>> - Writes gate masks, window end times, and clears unused entries in the
>>   shadow list.
>> - Updates gate close times and Max SDU values for each queue.
>> - Triggers list changes using `tas_set_trigger_list_change`, which
>>   - Computes cycle count (base-time % cycle-time) and extend (base-time %
>>     cycle-time)
> 
> Please define the "cycle count" concept (local invention, not IEEE

cycle count here means number of cycles in the base-time.
If base-time is 1747291156846086012 and cycle-time is 1000000 (1ms) then
the cycle count is 1747291156846 where as extend will be 86012

> standard). Also, cross-checking with the code, base-time % cycle-time is
> incorrect here, that's not how you calculate it.

That's actually a typo. It should be

 - Computes cycle count (base-time / cycle-time) and extend (base-time %
   cycle-time)

> 
> I'm afraid you also need to define the "extend" concept. It is not at
> all clear what it does and how it does it. Does it have any relationship
> with the CycleTimeExtension variables as documented by IEEE 802.1Q annex
> Q.5 definitions?
> 
> A very compressed summary of the standard variable is this:
> the CycleTimeExtension applies when:
> - an Open schedule exists
> - an Admin schedule is pending
> - the AdminBaseTime is not an integer multiple of OperBaseTime + (N *
>   OperCycleTime) - i.o.w. the admin schedule does not "line up" with the
>   end of the oper schedule
> 
> The misalignment of the oper vs admin schedules might cause the very
> last oper cycle to be truncated to an undesirably short value. The
> OperCycleTimeExtension variable exists to prevent this, as such:
> 
> - If the length of the last oper cycle is < OperCycleTimeExtension,
>   then this cycle does not execute at all. The gate states from the end
>   of the next-to-last oper cycle remain in place (that cycle is extended)
>   until the activation of the admin schedule at AdminBaseTime.
> 
> - If the length of the last oper cycle is >= OperCycleTimeExtension,
>   this last cycle is left to execute until AdminBaseTime, and is
>   potentially truncated during the switchover event (unless it perfectly
>   lines up). Extension of the next-to-last oper cycle does not take
>   place.
> 
> Is this the same functionality as the "extend" feature of the PRU
> firmware - should I be reading the code and the commit message in this
> key, in order to understand what it achieves?


"extend" here is not same as `CycleTimeExtension`. The current firmware
implementation always extends the next-to-last cycle so that it aligns
with the new base-time.

Eg,
existing schedule, base-time 125ms cycle-time 1ms
New schedule, base-time 239.4ms cycle-time 1ms

Here the second-to-last cycle starts at 238ms and lasts for 1ms. The
Last cycle starts at 239ms and is only lasting for 0.4ms.

In this case, the existing schedule will continue till 238ms. After that
the next cycle will last for 1.4 ms instead of 1ms. And the new schedule
will happen at 239.4 ms.

The extend variable can be anything between 0 to 1ms in this case and
the second last cycle will be extended and the last cycle won't be
executed at all.

> 
>>   - Writes cycle time, cycle count, and extend values to firmware memory.
>>   - base-time being in past or base-time not being a multiple of
>>     cycle-time is taken care by the firmware. Driver just writes these
>>     variable for firmware and firmware takes care of the scheduling.
> 
> "base-time not being a multiple of cycle-time is taken care by the firmware":
> To what extent is this true? You don't actually pass the base-time to
> the firmware, so how would it know that it's not a multiple of cycle-time?
> 

We pass cycle-count and extend. If extend is zero, it implies base-time
is multiple of cycle-time. This way firmware knows whether base-time is
multiple of cycle-time or not.

>>   - If base-time is not a multiple of cycle-time, the value of extend
>>     (base-time % cycle-time) is used by the firmware to extend the last
>>     cycle.
> 
> I'm surprised to read this. Why does the firmware expect the base time
> to be a multiple of the cycle time?
> 

Earlier the limitation was that firmware can only start schedules at
multiple of cycle-times. If a base-time is not multiple of cycle-time
then the schedule is started at next nearest multiple of cycle-time from
the base-time. But now we have fix that, and schedule can be started at
any time. No need for base-time to be multiple of cycle-time.

> Also, I don't understand what the workaround achieves. If the "extend"
> feature is similar to CycleTimeExtension, then it applies at the _end_
> of the cycle. I.o.w. if you never change the cycle, it never applies.
> How does that help address a problem which exists since the very first
> cycle of the schedule (that it may be shifted relative to integer
> multiples of the cycle time)?
> 
> And even assuming that a schedule change will take place - what's the
> math that would suggest the "extend" feature does anything at all to
> address the request to apply a phase-shifted schedule? The last cycle of
> the oper schedule passes, the admin schedule becomes the new oper, and
> then what? It still runs phase-aligned with its own cycle-time, but
> misaligned with the user-provided base time, no?
> 
> The expectation is for all cycles to be shifted relative to N *
> base-time, not just the first or last one. It doesn't "sound" like you
> can achieve that using CycleTimeExtension (assuming that's what this

Yes I understand that. All the cycles will be shifted not just the first
or the last one. Let me explain with example.

Let's assume the existing schedule is as below,
base-time 500ms cycle-time 1ms

The schedule will start at 500ms and keep going on. The cycles will
start at 500ms, 501ms, 502ms ...

Now let's say new requested schedule is having base-time as 1000.821 ms
and cycle-time as 1ms.

In this case the earlier schedule's second-to-last cycle will start at
999ms and end at 1000.821ms. The cycle gets extended by 0.821ms

It will look like this, 500ms, 501ms, 502ms ... 997ms, 998ms, 999ms,
1000.821ms.

Now our new schedule will start at 1000.821ms and continue with 1ms
cycle-time.

The cycles will go on as 1000.821ms, 1001.821ms, 1002.821ms ......

Now in future some other schedule comes up with base-time as 1525.486ms
then again the second last cycle of current schedule will extend.

So the cycles will be like 1000.821ms, 1001.821ms, 1002.821ms ...
1521.821ms, 1522.821ms, 1523.821ms, 1525.486ms. Here the second-to-last
cycle will last for 1.665ms (extended by 0.665ms) where as all other
cycles will be 1ms as requested by user.

Here all cycles are aligned with base-time (shifter by N*base-time).
Only the last cycle is extended depending upon the base-time of new
schedule.

> is), so better refuse those schedules which don't have the base-time you
> need.
> 

That's what our first approach was. If it's okay with you I can drop all
these changes and add below check in driver

if (taprio->base_time % taprio->cycle_time) {
	NL_SET_ERR_MSG_MOD(taprio->extack, "Base-time should be multiple of
cycle-time");
	return -EOPNOTSUPP;
}

>>   - Sets `config_change` and `config_pending` flags to notify firmware of
>>     the new shadow list and its readiness for activation.
>>   - Sends the `ICSSG_EMAC_PORT_TAS_TRIGGER` r30 command to ask firmware to
>>     swap active and shadow lists.
>> - Waits for the firmware to clear the `config_change` flag before
>>   completing the update and returning successfully.
>>
>> This implementation ensures seamless TAS functionality by offloading
>> scheduling complexities to the firmware.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> Reviewed-by: Simon Horman <horms@kernel.org>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
>> v9 - v10:
>> There has been significant changes since v9. I have tried to address all
>> the comments given by Vladimir Oltean <vladimir.oltean@nxp.com> on v9
>> *) Made the driver depend on NET_SCH_TAPRIO || NET_SCH_TAPRIO=n for TAS
>> *) Used MACRO for max sdu size instead of magic number
>> *) Kept `tas->state = state` outside of the switch case in `tas_set_state`
>> *) Implemented TC_QUERY_CAPS case in `icssg_qos_ndo_setup_tc`
>> *) Calling `tas_update_fw_list_pointers` only once in
>>    `tas_update_oper_list` as the second call as unnecessary.
>> *) Moved the check for TAS_MAX_CYCLE_TIME to beginning of
>>    `emac_taprio_replace`
>> *) Added `__packed` to structures in `icssg_qos.h`
>> *) Modified implementation of `tas_set_trigger_list_change` to handle
>>    cases where base-time isn't a multiple of cycle-time. For this a new
>>    variable extend has to be calculated as base-time % cycle-time. This
>>    variable is used by firmware to extend the last cycle.
>> *) The API prueth_iep_gettime() and prueth_iep_settime() also needs to be
>>    adjusted according to the cycle time extension. These changes are also
>>    taken care in this patch.
> 
> Why? Given the explanation of CycleTimeExtension above, it makes no
> sense to me why you would alter the gettime() and settime() values.
> 

The Firmware has two counters

counter0 counts the number of miliseconds in current time
counter1 counts the number of nanoseconds in the current ms.

Let's say the current time is 1747305807237749032 ns.
counter0 will read 1747305807237 counter1 will read 749032.

The current time = counter0* 1ms + counter1

For taprio scheduling also counter0 is used. Now let's say below is are
the cycles of a schedule

cycles   = 500ms 501ms 502ms ... 997ms, 998ms, 999ms, 1000.821ms
counter0 = 500   501   502   ... 997    998    999    1000
curr_time= 500*1, 501*1, 502*2...997*1, 998*1, 999*1, 1000*1

Here you see after the last cycle the time is 1000.821 however our above
formula will give us 1000 as the time since last cycle was extended.

To compensate this, whatever extension firmware applies need to be added
during current time calculation. Below is the code for that.

      ts += readl(prueth->shram.va + TIMESYNC_CYCLE_EXTN_TIME);

Now the current time becomes,
	counter0* 1ms + counter1 + EXTEND

This is why change to set/get_time() APIs are needed. This will not be
needed if we drop this extends implementation.

Let me know if above explanation makes sense and if I should continue
with this approach or drop the extend feature at all and just refuse the
schedules?

Thanks for the feedback.

>>
>> v9 https://lore.kernel.org/all/20240531044512.981587-3-danishanwar@ti.com/
>>
>>  drivers/net/ethernet/ti/Kconfig               |   1 +
>>  drivers/net/ethernet/ti/Makefile              |   2 +-
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  |   7 +
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   2 +
>>  drivers/net/ethernet/ti/icssg/icssg_qos.c     | 310 ++++++++++++++++++
>>  drivers/net/ethernet/ti/icssg/icssg_qos.h     | 112 +++++++
>>  .../net/ethernet/ti/icssg/icssg_switch_map.h  |   6 +
>>  7 files changed, 439 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.c
>>  create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.h
>>
>> +static int emac_taprio_replace(struct net_device *ndev,
>> +			       struct tc_taprio_qopt_offload *taprio)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	int ret;
>> +
>> +	if (taprio->cycle_time_extension) {
>> +		NL_SET_ERR_MSG_MOD(taprio->extack, "Cycle time extension not supported");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	if (taprio->cycle_time > TAS_MAX_CYCLE_TIME) {
>> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "cycle_time %llu is more than max supported cycle_time",
>> +				       taprio->cycle_time);
> 
> It would be better to also print here TAS_MAX_CYCLE_TIME, like TAS_MIN_CYCLE_TIME below.
> Also, looping back a user-supplied value (taprio->cycle_time) is IMO not needed.
> 

Sure

>> +		return -EINVAL;
>> +	}
>> +
>> +	if (taprio->cycle_time < TAS_MIN_CYCLE_TIME) {
>> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "cycle_time %llu is less than min supported cycle_time %d",
>> +				       taprio->cycle_time, TAS_MIN_CYCLE_TIME);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (taprio->num_entries > TAS_MAX_CMD_LISTS) {
>> +		NL_SET_ERR_MSG_FMT_MOD(taprio->extack, "num_entries %lu is more than max supported entries %d",
>> +				       taprio->num_entries, TAS_MAX_CMD_LISTS);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (emac->qos.tas.taprio_admin)
>> +		taprio_offload_free(emac->qos.tas.taprio_admin);
>> +
>> +	emac->qos.tas.taprio_admin = taprio_offload_get(taprio);
>> +	ret = tas_update_oper_list(emac);
>> +	if (ret)
>> +		goto clear_taprio;
>> +
>> +	ret = tas_set_state(emac, TAS_STATE_ENABLE);
>> +	if (ret)
>> +		goto clear_taprio;
>> +
>> +	return 0;
>> +
>> +clear_taprio:
>> +	emac->qos.tas.taprio_admin = NULL;
>> +	taprio_offload_free(taprio);
>> +
>> +	return ret;
>> +}

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[MD Danish Anwar]

Hi Vladimir,

On 15/05/25 4:24 pm, MD Danish Anwar wrote:
> Hi Vladimir,
> 
> On 06/05/25 9:16 pm, Vladimir Oltean wrote:
>> It has been a long time since the last posting, everything has been
>> swapped out from my memory. Sorry if some comments are repeated.
>>
> 
> Yes. It has been almost a year since my last revision.
> 
>> On Fri, May 02, 2025 at 04:12:35PM +0530, MD Danish Anwar wrote:
>>> From: Roger Quadros <rogerq@ti.com>
>>>
>>> The Time-Aware Shaper (TAS) is a key feature of the Enhanced Scheduled
>>> Traffic (EST) mechanism defined in IEEE 802.1Q-2018. This patch adds TAS
>>> support for the ICSSG driver by interacting with the ICSSG firmware to
>>> manage gate control lists, cycle times, and other TAS parameters.
>>>
>>> The firmware maintains active and shadow lists. The driver updates the
>>> operating list using API `tas_update_oper_list()` which,
>>> - Updates firmware list pointers via `tas_update_fw_list_pointers`.
>>> - Writes gate masks, window end times, and clears unused entries in the
>>>   shadow list.
>>> - Updates gate close times and Max SDU values for each queue.
>>> - Triggers list changes using `tas_set_trigger_list_change`, which
>>>   - Computes cycle count (base-time % cycle-time) and extend (base-time %
>>>     cycle-time)
>>
>> Please define the "cycle count" concept (local invention, not IEEE
> 
> cycle count here means number of cycles in the base-time.
> If base-time is 1747291156846086012 and cycle-time is 1000000 (1ms) then
> the cycle count is 1747291156846 where as extend will be 86012
> 
>> standard). Also, cross-checking with the code, base-time % cycle-time is
>> incorrect here, that's not how you calculate it.
> 
> That's actually a typo. It should be
> 
>  - Computes cycle count (base-time / cycle-time) and extend (base-time %
>    cycle-time)
> 
>>
>> I'm afraid you also need to define the "extend" concept. It is not at
>> all clear what it does and how it does it. Does it have any relationship
>> with the CycleTimeExtension variables as documented by IEEE 802.1Q annex
>> Q.5 definitions?
>>
>> A very compressed summary of the standard variable is this:
>> the CycleTimeExtension applies when:
>> - an Open schedule exists
>> - an Admin schedule is pending
>> - the AdminBaseTime is not an integer multiple of OperBaseTime + (N *
>>   OperCycleTime) - i.o.w. the admin schedule does not "line up" with the
>>   end of the oper schedule
>>
>> The misalignment of the oper vs admin schedules might cause the very
>> last oper cycle to be truncated to an undesirably short value. The
>> OperCycleTimeExtension variable exists to prevent this, as such:
>>
>> - If the length of the last oper cycle is < OperCycleTimeExtension,
>>   then this cycle does not execute at all. The gate states from the end
>>   of the next-to-last oper cycle remain in place (that cycle is extended)
>>   until the activation of the admin schedule at AdminBaseTime.
>>
>> - If the length of the last oper cycle is >= OperCycleTimeExtension,
>>   this last cycle is left to execute until AdminBaseTime, and is
>>   potentially truncated during the switchover event (unless it perfectly
>>   lines up). Extension of the next-to-last oper cycle does not take
>>   place.
>>
>> Is this the same functionality as the "extend" feature of the PRU
>> firmware - should I be reading the code and the commit message in this
>> key, in order to understand what it achieves?
> 
> 
> "extend" here is not same as `CycleTimeExtension`. The current firmware
> implementation always extends the next-to-last cycle so that it aligns
> with the new base-time.
> 
> Eg,
> existing schedule, base-time 125ms cycle-time 1ms
> New schedule, base-time 239.4ms cycle-time 1ms
> 
> Here the second-to-last cycle starts at 238ms and lasts for 1ms. The
> Last cycle starts at 239ms and is only lasting for 0.4ms.
> 
> In this case, the existing schedule will continue till 238ms. After that
> the next cycle will last for 1.4 ms instead of 1ms. And the new schedule
> will happen at 239.4 ms.
> 
> The extend variable can be anything between 0 to 1ms in this case and
> the second last cycle will be extended and the last cycle won't be
> executed at all.
> 
>>
>>>   - Writes cycle time, cycle count, and extend values to firmware memory.
>>>   - base-time being in past or base-time not being a multiple of
>>>     cycle-time is taken care by the firmware. Driver just writes these
>>>     variable for firmware and firmware takes care of the scheduling.
>>
>> "base-time not being a multiple of cycle-time is taken care by the firmware":
>> To what extent is this true? You don't actually pass the base-time to
>> the firmware, so how would it know that it's not a multiple of cycle-time?
>>
> 
> We pass cycle-count and extend. If extend is zero, it implies base-time
> is multiple of cycle-time. This way firmware knows whether base-time is
> multiple of cycle-time or not.
> 
>>>   - If base-time is not a multiple of cycle-time, the value of extend
>>>     (base-time % cycle-time) is used by the firmware to extend the last
>>>     cycle.
>>
>> I'm surprised to read this. Why does the firmware expect the base time
>> to be a multiple of the cycle time?
>>
> 
> Earlier the limitation was that firmware can only start schedules at
> multiple of cycle-times. If a base-time is not multiple of cycle-time
> then the schedule is started at next nearest multiple of cycle-time from
> the base-time. But now we have fix that, and schedule can be started at
> any time. No need for base-time to be multiple of cycle-time.
> 
>> Also, I don't understand what the workaround achieves. If the "extend"
>> feature is similar to CycleTimeExtension, then it applies at the _end_
>> of the cycle. I.o.w. if you never change the cycle, it never applies.
>> How does that help address a problem which exists since the very first
>> cycle of the schedule (that it may be shifted relative to integer
>> multiples of the cycle time)?
>>
>> And even assuming that a schedule change will take place - what's the
>> math that would suggest the "extend" feature does anything at all to
>> address the request to apply a phase-shifted schedule? The last cycle of
>> the oper schedule passes, the admin schedule becomes the new oper, and
>> then what? It still runs phase-aligned with its own cycle-time, but
>> misaligned with the user-provided base time, no?
>>
>> The expectation is for all cycles to be shifted relative to N *
>> base-time, not just the first or last one. It doesn't "sound" like you
>> can achieve that using CycleTimeExtension (assuming that's what this
> 
> Yes I understand that. All the cycles will be shifted not just the first
> or the last one. Let me explain with example.
> 
> Let's assume the existing schedule is as below,
> base-time 500ms cycle-time 1ms
> 
> The schedule will start at 500ms and keep going on. The cycles will
> start at 500ms, 501ms, 502ms ...
> 
> Now let's say new requested schedule is having base-time as 1000.821 ms
> and cycle-time as 1ms.
> 
> In this case the earlier schedule's second-to-last cycle will start at
> 999ms and end at 1000.821ms. The cycle gets extended by 0.821ms
> 
> It will look like this, 500ms, 501ms, 502ms ... 997ms, 998ms, 999ms,
> 1000.821ms.
> 
> Now our new schedule will start at 1000.821ms and continue with 1ms
> cycle-time.
> 
> The cycles will go on as 1000.821ms, 1001.821ms, 1002.821ms ......
> 
> Now in future some other schedule comes up with base-time as 1525.486ms
> then again the second last cycle of current schedule will extend.
> 
> So the cycles will be like 1000.821ms, 1001.821ms, 1002.821ms ...
> 1521.821ms, 1522.821ms, 1523.821ms, 1525.486ms. Here the second-to-last
> cycle will last for 1.665ms (extended by 0.665ms) where as all other
> cycles will be 1ms as requested by user.
> 
> Here all cycles are aligned with base-time (shifter by N*base-time).
> Only the last cycle is extended depending upon the base-time of new
> schedule.
> 
>> is), so better refuse those schedules which don't have the base-time you
>> need.
>>
> 
> That's what our first approach was. If it's okay with you I can drop all
> these changes and add below check in driver
> 
> if (taprio->base_time % taprio->cycle_time) {
> 	NL_SET_ERR_MSG_MOD(taprio->extack, "Base-time should be multiple of
> cycle-time");
> 	return -EOPNOTSUPP;
> }
> 
>>>   - Sets `config_change` and `config_pending` flags to notify firmware of
>>>     the new shadow list and its readiness for activation.
>>>   - Sends the `ICSSG_EMAC_PORT_TAS_TRIGGER` r30 command to ask firmware to
>>>     swap active and shadow lists.
>>> - Waits for the firmware to clear the `config_change` flag before
>>>   completing the update and returning successfully.
>>>
>>> This implementation ensures seamless TAS functionality by offloading
>>> scheduling complexities to the firmware.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>>> Reviewed-by: Simon Horman <horms@kernel.org>
>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>> ---
>>> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
>>> v9 - v10:
>>> There has been significant changes since v9. I have tried to address all
>>> the comments given by Vladimir Oltean <vladimir.oltean@nxp.com> on v9
>>> *) Made the driver depend on NET_SCH_TAPRIO || NET_SCH_TAPRIO=n for TAS
>>> *) Used MACRO for max sdu size instead of magic number
>>> *) Kept `tas->state = state` outside of the switch case in `tas_set_state`
>>> *) Implemented TC_QUERY_CAPS case in `icssg_qos_ndo_setup_tc`
>>> *) Calling `tas_update_fw_list_pointers` only once in
>>>    `tas_update_oper_list` as the second call as unnecessary.
>>> *) Moved the check for TAS_MAX_CYCLE_TIME to beginning of
>>>    `emac_taprio_replace`
>>> *) Added `__packed` to structures in `icssg_qos.h`
>>> *) Modified implementation of `tas_set_trigger_list_change` to handle
>>>    cases where base-time isn't a multiple of cycle-time. For this a new
>>>    variable extend has to be calculated as base-time % cycle-time. This
>>>    variable is used by firmware to extend the last cycle.
>>> *) The API prueth_iep_gettime() and prueth_iep_settime() also needs to be
>>>    adjusted according to the cycle time extension. These changes are also
>>>    taken care in this patch.
>>
>> Why? Given the explanation of CycleTimeExtension above, it makes no
>> sense to me why you would alter the gettime() and settime() values.
>>
> 
> The Firmware has two counters
> 
> counter0 counts the number of miliseconds in current time
> counter1 counts the number of nanoseconds in the current ms.
> 
> Let's say the current time is 1747305807237749032 ns.
> counter0 will read 1747305807237 counter1 will read 749032.
> 
> The current time = counter0* 1ms + counter1
> 
> For taprio scheduling also counter0 is used. Now let's say below is are
> the cycles of a schedule
> 
> cycles   = 500ms 501ms 502ms ... 997ms, 998ms, 999ms, 1000.821ms
> counter0 = 500   501   502   ... 997    998    999    1000
> curr_time= 500*1, 501*1, 502*2...997*1, 998*1, 999*1, 1000*1
> 
> Here you see after the last cycle the time is 1000.821 however our above
> formula will give us 1000 as the time since last cycle was extended.
> 
> To compensate this, whatever extension firmware applies need to be added
> during current time calculation. Below is the code for that.
> 
>       ts += readl(prueth->shram.va + TIMESYNC_CYCLE_EXTN_TIME);
> 
> Now the current time becomes,
> 	counter0* 1ms + counter1 + EXTEND
> 
> This is why change to set/get_time() APIs are needed. This will not be
> needed if we drop this extends implementation.
> 
> Let me know if above explanation makes sense and if I should continue
> with this approach or drop the extend feature at all and just refuse the
> schedules?
> 

I am not sure if you got the change to review my replies to your initial
comments. Let me know if I should continue with this approach or just
refuse the schedules that don't have the base time that we need.

> Thanks for the feedback.
> 


-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[Vladimir Oltean]

Hi Danish,

On Tue, Jun 10, 2025 at 01:13:38PM +0530, MD Danish Anwar wrote:
> >> Please define the "cycle count" concept (local invention, not IEEE
> > 
> > cycle count here means number of cycles in the base-time.
> > If base-time is 1747291156846086012 and cycle-time is 1000000 (1ms) then
> > the cycle count is 1747291156846 where as extend will be 86012
> > 
> >> standard). Also, cross-checking with the code, base-time % cycle-time is
> >> incorrect here, that's not how you calculate it.
> > 
> > That's actually a typo. It should be
> > 
> >  - Computes cycle count (base-time / cycle-time) and extend (base-time %
> >    cycle-time)
> > 
> >>
> >> I'm afraid you also need to define the "extend" concept. It is not at
> >> all clear what it does and how it does it. Does it have any relationship
> >> with the CycleTimeExtension variables as documented by IEEE 802.1Q annex
> >> Q.5 definitions?
> >>
> > "extend" here is not same as `CycleTimeExtension`. The current firmware
> > implementation always extends the next-to-last cycle so that it aligns
> > with the new base-time.
> > 
> > Eg,
> > existing schedule, base-time 125ms cycle-time 1ms
> > New schedule, base-time 239.4ms cycle-time 1ms
> > 
> > Here the second-to-last cycle starts at 238ms and lasts for 1ms. The
> > Last cycle starts at 239ms and is only lasting for 0.4ms.
> > 
> > In this case, the existing schedule will continue till 238ms. After that
> > the next cycle will last for 1.4 ms instead of 1ms. And the new schedule
> > will happen at 239.4 ms.
> > 
> > The extend variable can be anything between 0 to 1ms in this case and
> > the second last cycle will be extended and the last cycle won't be
> > executed at all.

Thanks for the explanation. It sounds like a custom spin on CycleTimeExtension.

In your example above, "extend", when specified as part of the "new" schedule,
applies to the "existing" schedule. Whereas CycleTimeExtension extends
the next-to-last cycle of the same schedule as the one it was applied to.

Questions based on the above:

1. If there is no "existing" schedule, what does the "extend" variable
   extend? The custom base-time mechanism has to work even for the first
   taprio schedule. (this is an unanswered pre-existing question)

2. Can you give me another (valid, i.e. confirmed working) example of
   extension, where the cycle-time of the existing schedule is different
   from the cycle-time of the new one? You calculate the extension of
   the next-to-last cycle of the existing schedule based on the cycle
   length of the new schedule. It is not obvious to me why that would be
   correct.

> >>>   - Writes cycle time, cycle count, and extend values to firmware memory.
> >>>   - base-time being in past or base-time not being a multiple of
> >>>     cycle-time is taken care by the firmware. Driver just writes these
> >>>     variable for firmware and firmware takes care of the scheduling.
> >>
> >> "base-time not being a multiple of cycle-time is taken care by the firmware":
> >> To what extent is this true? You don't actually pass the base-time to
> >> the firmware, so how would it know that it's not a multiple of cycle-time?
> >>
> > 
> > We pass cycle-count and extend. If extend is zero, it implies base-time
> > is multiple of cycle-time. This way firmware knows whether base-time is
> > multiple of cycle-time or not.
> > 
> >>>   - If base-time is not a multiple of cycle-time, the value of extend
> >>>     (base-time % cycle-time) is used by the firmware to extend the last
> >>>     cycle.
> >>
> >> I'm surprised to read this. Why does the firmware expect the base time
> >> to be a multiple of the cycle time?
> >>
> > 
> > Earlier the limitation was that firmware can only start schedules at
> > multiple of cycle-times. If a base-time is not multiple of cycle-time
> > then the schedule is started at next nearest multiple of cycle-time from
> > the base-time. But now we have fix that, and schedule can be started at
> > any time. No need for base-time to be multiple of cycle-time.
> > 
> >> Also, I don't understand what the workaround achieves. If the "extend"
> >> feature is similar to CycleTimeExtension, then it applies at the _end_
> >> of the cycle. I.o.w. if you never change the cycle, it never applies.
> >> How does that help address a problem which exists since the very first
> >> cycle of the schedule (that it may be shifted relative to integer
> >> multiples of the cycle time)?
> >>
> >> And even assuming that a schedule change will take place - what's the
> >> math that would suggest the "extend" feature does anything at all to
> >> address the request to apply a phase-shifted schedule? The last cycle of
> >> the oper schedule passes, the admin schedule becomes the new oper, and
> >> then what? It still runs phase-aligned with its own cycle-time, but
> >> misaligned with the user-provided base time, no?
> >>
> >> The expectation is for all cycles to be shifted relative to N *
> >> base-time, not just the first or last one. It doesn't "sound" like you
> >> can achieve that using CycleTimeExtension (assuming that's what this
> > 
> > Yes I understand that. All the cycles will be shifted not just the first
> > or the last one. Let me explain with example.
> > 
> > Let's assume the existing schedule is as below,
> > base-time 500ms cycle-time 1ms
> > 
> > The schedule will start at 500ms and keep going on. The cycles will
> > start at 500ms, 501ms, 502ms ...
> > 
> > Now let's say new requested schedule is having base-time as 1000.821 ms
> > and cycle-time as 1ms.
> > 
> > In this case the earlier schedule's second-to-last cycle will start at
> > 999ms and end at 1000.821ms. The cycle gets extended by 0.821ms
> > 
> > It will look like this, 500ms, 501ms, 502ms ... 997ms, 998ms, 999ms,
> > 1000.821ms.
> > 
> > Now our new schedule will start at 1000.821ms and continue with 1ms
> > cycle-time.
> > 
> > The cycles will go on as 1000.821ms, 1001.821ms, 1002.821ms ......
> > 
> > Now in future some other schedule comes up with base-time as 1525.486ms
> > then again the second last cycle of current schedule will extend.
> > 
> > So the cycles will be like 1000.821ms, 1001.821ms, 1002.821ms ...
> > 1521.821ms, 1522.821ms, 1523.821ms, 1525.486ms. Here the second-to-last
> > cycle will last for 1.665ms (extended by 0.665ms) where as all other
> > cycles will be 1ms as requested by user.
> > 
> > Here all cycles are aligned with base-time (shifter by N*base-time).
> > Only the last cycle is extended depending upon the base-time of new
> > schedule.
> > 
> >> is), so better refuse those schedules which don't have the base-time you
> >> need.
> >>
> > 
> > That's what our first approach was. If it's okay with you I can drop all
> > these changes and add below check in driver
> > 
> > if (taprio->base_time % taprio->cycle_time) {
> > 	NL_SET_ERR_MSG_MOD(taprio->extack, "Base-time should be multiple of
> > cycle-time");
> > 	return -EOPNOTSUPP;
> > }

I don't want to make a definitive statement on this just yet, I don't
fully understand what was implemented in the firmware and what was the
thinking.

> >>>   - Sets `config_change` and `config_pending` flags to notify firmware of
> >>>     the new shadow list and its readiness for activation.
> >>>   - Sends the `ICSSG_EMAC_PORT_TAS_TRIGGER` r30 command to ask firmware to
> >>>     swap active and shadow lists.
> >>> - Waits for the firmware to clear the `config_change` flag before
> >>>   completing the update and returning successfully.
> >>>
> >>> This implementation ensures seamless TAS functionality by offloading
> >>> scheduling complexities to the firmware.
> >>>
> >>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> >>> Reviewed-by: Simon Horman <horms@kernel.org>
> >>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> >>> ---
> >>> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> >>> v9 - v10:
> >>> There has been significant changes since v9. I have tried to address all
> >>> the comments given by Vladimir Oltean <vladimir.oltean@nxp.com> on v9
> >>> *) Made the driver depend on NET_SCH_TAPRIO || NET_SCH_TAPRIO=n for TAS
> >>> *) Used MACRO for max sdu size instead of magic number
> >>> *) Kept `tas->state = state` outside of the switch case in `tas_set_state`
> >>> *) Implemented TC_QUERY_CAPS case in `icssg_qos_ndo_setup_tc`
> >>> *) Calling `tas_update_fw_list_pointers` only once in
> >>>    `tas_update_oper_list` as the second call as unnecessary.
> >>> *) Moved the check for TAS_MAX_CYCLE_TIME to beginning of
> >>>    `emac_taprio_replace`
> >>> *) Added `__packed` to structures in `icssg_qos.h`
> >>> *) Modified implementation of `tas_set_trigger_list_change` to handle
> >>>    cases where base-time isn't a multiple of cycle-time. For this a new
> >>>    variable extend has to be calculated as base-time % cycle-time. This
> >>>    variable is used by firmware to extend the last cycle.
> >>> *) The API prueth_iep_gettime() and prueth_iep_settime() also needs to be
> >>>    adjusted according to the cycle time extension. These changes are also
> >>>    taken care in this patch.
> >>
> >> Why? Given the explanation of CycleTimeExtension above, it makes no
> >> sense to me why you would alter the gettime() and settime() values.
> >>
> > 
> > The Firmware has two counters
> > 
> > counter0 counts the number of miliseconds in current time
> > counter1 counts the number of nanoseconds in the current ms.
> > 
> > Let's say the current time is 1747305807237749032 ns.
> > counter0 will read 1747305807237 counter1 will read 749032.
> > 
> > The current time = counter0* 1ms + counter1
> > 
> > For taprio scheduling also counter0 is used.

"Used" in the sense that taprio needs to know the current time, correct?
But by that logic, taprio equally uses counter0 and counter1, no? For
example, for a cycle-time of 1.23 ms.

> > Now let's say below are the cycles of a schedule
> > 
> > cycles   = 500ms 501ms 502ms ... 997ms, 998ms, 999ms, 1000.821ms
> > counter0 = 500   501   502   ... 997    998    999    1000
> > curr_time= 500*1, 501*1, 502*2...997*1, 998*1, 999*1, 1000*1
> > 
> > Here you see after the last cycle the time is 1000.821 however our above
> > formula will give us 1000 as the time since last cycle was extended.

Wait a second. You compensate the time in prueth_iep_gettime(), which is
called, among other places, from icss_iep_ptp_gettimeex() (aka struct
ptp_clock_info :: gettimex64()).

I don't know about the other call paths, but ptp_clock_info :: gettimex64()
doesn't answer the question "what was the last time that a taprio cycle
ended at?" but rather "what time is it according this clock, now?"

I still fail to see why the taprio cycle extension would affect the
current time. Or does TIMESYNC_CYCLE_EXTN_TIME extend the length of the
millisecond?

> > To compensate this, whatever extension firmware applies need to be added
> > during current time calculation. Below is the code for that.
> > 
> >       ts += readl(prueth->shram.va + TIMESYNC_CYCLE_EXTN_TIME);
> > 
> > Now the current time becomes,
> > 	counter0* 1ms + counter1 + EXTEND
> > 
> > This is why change to set/get_time() APIs are needed. This will not be
> > needed if we drop this extends implementation.

What if the cycle that has to be extended has not arrived yet (is in the
future)? Why is the current time compensated in that case?

> > Let me know if above explanation makes sense and if I should continue
> > with this approach or drop the extend feature at all and just refuse the
> > schedules?
> > 
> 
> I am not sure if you got the change to review my replies to your initial
> comments. Let me know if I should continue with this approach or just
> refuse the schedules that don't have the base time that we need.
> 
> > Thanks for the feedback.
> > 
> 
> 
> -- 
> Thanks and Regards,
> Danish

As you can see, I still have trouble understanding the concepts proposed
by the firmware.

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[Vladimir Oltean]

On Thu, May 15, 2025 at 04:24:50PM +0530, MD Danish Anwar wrote:
> To compensate this, whatever extension firmware applies need to be added
> during current time calculation. Below is the code for that.
> 
>       ts += readl(prueth->shram.va + TIMESYNC_CYCLE_EXTN_TIME);
> 
> Now the current time becomes,
> 	counter0* 1ms + counter1 + EXTEND

What will the TIMESYNC_CYCLE_EXTN_TIME register read (and what is its
exact meaning)? Is its value derived from TAS_CONFIG_CYCLE_EXTEND? I'm
asking because the driver only writes TIMESYNC_CYCLE_EXTN_TIME to zero
(for a reason that isn't clear to me either).

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[MD Danish Anwar]

Hi Vladimir,

On 10/06/25 2:20 pm, Vladimir Oltean wrote:
> Hi Danish,
> 
> On Tue, Jun 10, 2025 at 01:13:38PM +0530, MD Danish Anwar wrote:
>>>> Please define the "cycle count" concept (local invention, not IEEE
>>>
>>> cycle count here means number of cycles in the base-time.
>>> If base-time is 1747291156846086012 and cycle-time is 1000000 (1ms) then
>>> the cycle count is 1747291156846 where as extend will be 86012
>>>
>>>> standard). Also, cross-checking with the code, base-time % cycle-time is
>>>> incorrect here, that's not how you calculate it.
>>>
>>> That's actually a typo. It should be
>>>
>>>  - Computes cycle count (base-time / cycle-time) and extend (base-time %
>>>    cycle-time)
>>>
>>>>
>>>> I'm afraid you also need to define the "extend" concept. It is not at
>>>> all clear what it does and how it does it. Does it have any relationship
>>>> with the CycleTimeExtension variables as documented by IEEE 802.1Q annex
>>>> Q.5 definitions?
>>>>
>>> "extend" here is not same as `CycleTimeExtension`. The current firmware
>>> implementation always extends the next-to-last cycle so that it aligns
>>> with the new base-time.
>>>
>>> Eg,
>>> existing schedule, base-time 125ms cycle-time 1ms
>>> New schedule, base-time 239.4ms cycle-time 1ms
>>>
>>> Here the second-to-last cycle starts at 238ms and lasts for 1ms. The
>>> Last cycle starts at 239ms and is only lasting for 0.4ms.
>>>
>>> In this case, the existing schedule will continue till 238ms. After that
>>> the next cycle will last for 1.4 ms instead of 1ms. And the new schedule
>>> will happen at 239.4 ms.
>>>
>>> The extend variable can be anything between 0 to 1ms in this case and
>>> the second last cycle will be extended and the last cycle won't be
>>> executed at all.
> 
> Thanks for the explanation. It sounds like a custom spin on CycleTimeExtension.
> 
> In your example above, "extend", when specified as part of the "new" schedule,
> applies to the "existing" schedule. Whereas CycleTimeExtension extends
> the next-to-last cycle of the same schedule as the one it was applied to.
> 
> Questions based on the above:
> 
> 1. If there is no "existing" schedule, what does the "extend" variable
>    extend? The custom base-time mechanism has to work even for the first
>    taprio schedule. (this is an unanswered pre-existing question)
> 

The firmware has a cycle-time of 1ms even if there is no schedule. Every
1ms, firmware updates a counter. The curr_time is calculated as
CounterValue * 1ms.

Even if there are no schedule, the default cycle-time will remain 1ms.
Let's say the first schedule has a base-time of 20.5ms then the default
cycle will be extended by 0.5 ms and then the schedule will apply. This
is the reason, the extend feature is also impacting get/set_time
calculations.

> 2. Can you give me another (valid, i.e. confirmed working) example of
>    extension, where the cycle-time of the existing schedule is different
>    from the cycle-time of the new one? You calculate the extension of
>    the next-to-last cycle of the existing schedule based on the cycle
>    length of the new schedule. It is not obvious to me why that would be
>    correct.
> 

You are correct. This implementation may fail when the operating
schedule and new schedule have different cycle-times. I checked with the
firmware team and they have a limitation / bug. They only support
cycle-time = 1ms. Any other cycle-time is not supported by the firmware.

Because of this, the current implementation works as AdminBaseTime %
OperCycleTime is same as AdminBaseTime % AdminCycleTime since
OperCycleTime and AdminCycleTime are always going to be same (1ms)



>>>>>   - Writes cycle time, cycle count, and extend values to firmware memory.
>>>>>   - base-time being in past or base-time not being a multiple of
>>>>>     cycle-time is taken care by the firmware. Driver just writes these
>>>>>     variable for firmware and firmware takes care of the scheduling.
>>>>
>>>> "base-time not being a multiple of cycle-time is taken care by the firmware":
>>>> To what extent is this true? You don't actually pass the base-time to
>>>> the firmware, so how would it know that it's not a multiple of cycle-time?
>>>>
>>>
>>> We pass cycle-count and extend. If extend is zero, it implies base-time
>>> is multiple of cycle-time. This way firmware knows whether base-time is
>>> multiple of cycle-time or not.
>>>
>>>>>   - If base-time is not a multiple of cycle-time, the value of extend
>>>>>     (base-time % cycle-time) is used by the firmware to extend the last
>>>>>     cycle.
>>>>
>>>> I'm surprised to read this. Why does the firmware expect the base time
>>>> to be a multiple of the cycle time?
>>>>
>>>
>>> Earlier the limitation was that firmware can only start schedules at
>>> multiple of cycle-times. If a base-time is not multiple of cycle-time
>>> then the schedule is started at next nearest multiple of cycle-time from
>>> the base-time. But now we have fix that, and schedule can be started at
>>> any time. No need for base-time to be multiple of cycle-time.
>>>
>>>> Also, I don't understand what the workaround achieves. If the "extend"
>>>> feature is similar to CycleTimeExtension, then it applies at the _end_
>>>> of the cycle. I.o.w. if you never change the cycle, it never applies.
>>>> How does that help address a problem which exists since the very first
>>>> cycle of the schedule (that it may be shifted relative to integer
>>>> multiples of the cycle time)?
>>>>
>>>> And even assuming that a schedule change will take place - what's the
>>>> math that would suggest the "extend" feature does anything at all to
>>>> address the request to apply a phase-shifted schedule? The last cycle of
>>>> the oper schedule passes, the admin schedule becomes the new oper, and
>>>> then what? It still runs phase-aligned with its own cycle-time, but
>>>> misaligned with the user-provided base time, no?
>>>>
>>>> The expectation is for all cycles to be shifted relative to N *
>>>> base-time, not just the first or last one. It doesn't "sound" like you
>>>> can achieve that using CycleTimeExtension (assuming that's what this
>>>
>>> Yes I understand that. All the cycles will be shifted not just the first
>>> or the last one. Let me explain with example.
>>>
>>> Let's assume the existing schedule is as below,
>>> base-time 500ms cycle-time 1ms
>>>
>>> The schedule will start at 500ms and keep going on. The cycles will
>>> start at 500ms, 501ms, 502ms ...
>>>
>>> Now let's say new requested schedule is having base-time as 1000.821 ms
>>> and cycle-time as 1ms.
>>>
>>> In this case the earlier schedule's second-to-last cycle will start at
>>> 999ms and end at 1000.821ms. The cycle gets extended by 0.821ms
>>>
>>> It will look like this, 500ms, 501ms, 502ms ... 997ms, 998ms, 999ms,
>>> 1000.821ms.
>>>
>>> Now our new schedule will start at 1000.821ms and continue with 1ms
>>> cycle-time.
>>>
>>> The cycles will go on as 1000.821ms, 1001.821ms, 1002.821ms ......
>>>
>>> Now in future some other schedule comes up with base-time as 1525.486ms
>>> then again the second last cycle of current schedule will extend.
>>>
>>> So the cycles will be like 1000.821ms, 1001.821ms, 1002.821ms ...
>>> 1521.821ms, 1522.821ms, 1523.821ms, 1525.486ms. Here the second-to-last
>>> cycle will last for 1.665ms (extended by 0.665ms) where as all other
>>> cycles will be 1ms as requested by user.
>>>
>>> Here all cycles are aligned with base-time (shifter by N*base-time).
>>> Only the last cycle is extended depending upon the base-time of new
>>> schedule.
>>>
>>>> is), so better refuse those schedules which don't have the base-time you
>>>> need.
>>>>
>>>
>>> That's what our first approach was. If it's okay with you I can drop all
>>> these changes and add below check in driver
>>>
>>> if (taprio->base_time % taprio->cycle_time) {
>>> 	NL_SET_ERR_MSG_MOD(taprio->extack, "Base-time should be multiple of
>>> cycle-time");
>>> 	return -EOPNOTSUPP;
>>> }
> 
> I don't want to make a definitive statement on this just yet, I don't
> fully understand what was implemented in the firmware and what was the
> thinking.
> 

The firmware always expects cycle-time of 1ms and base-time to multiple
of cycle-time i.e. base-time to be multiple of 1ms.

This way all the schedules will be aligned and that's what current
implementation is.

To add support for base-time that are not multiple of cycle-time we
added "extend". However that will also only work as long as cycle-time
is 1ms. cycle-time other than 1ms is not supported by firmware as of
now. This is something we discovered recently.

We have a check for TAS_MIN_CYCLE_TIME and TAS_MAX_CYCLE_TIME and they
are defined as,

/* Minimum cycle time supported by implementation (in ns) */
#define TAS_MIN_CYCLE_TIME  (1000000)

/* Minimum cycle time supported by implementation (in ns) */
#define TAS_MAX_CYCLE_TIME  (4000000000)

But it is wrong. As per current firmware implementation,
	TAS_MIN_CYCLE_TIME = TAS_MAX_CYCLE_TIME = 1ms


The ideal use case will be to support,
1. Different cycle times
2. Different base times which may or may not be multiple of cycle-times.

With the current implementation, we are able to support #2 however #1 is
still a limitation. Once support for #1 is added, the implementation
will need to be changed.

>>>>>   - Sets `config_change` and `config_pending` flags to notify firmware of
>>>>>     the new shadow list and its readiness for activation.
>>>>>   - Sends the `ICSSG_EMAC_PORT_TAS_TRIGGER` r30 command to ask firmware to
>>>>>     swap active and shadow lists.
>>>>> - Waits for the firmware to clear the `config_change` flag before
>>>>>   completing the update and returning successfully.
>>>>>
>>>>> This implementation ensures seamless TAS functionality by offloading
>>>>> scheduling complexities to the firmware.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>>> Reviewed-by: Simon Horman <horms@kernel.org>
>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>> ---
>>>>> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
>>>>> v9 - v10:
>>>>> There has been significant changes since v9. I have tried to address all
>>>>> the comments given by Vladimir Oltean <vladimir.oltean@nxp.com> on v9
>>>>> *) Made the driver depend on NET_SCH_TAPRIO || NET_SCH_TAPRIO=n for TAS
>>>>> *) Used MACRO for max sdu size instead of magic number
>>>>> *) Kept `tas->state = state` outside of the switch case in `tas_set_state`
>>>>> *) Implemented TC_QUERY_CAPS case in `icssg_qos_ndo_setup_tc`
>>>>> *) Calling `tas_update_fw_list_pointers` only once in
>>>>>    `tas_update_oper_list` as the second call as unnecessary.
>>>>> *) Moved the check for TAS_MAX_CYCLE_TIME to beginning of
>>>>>    `emac_taprio_replace`
>>>>> *) Added `__packed` to structures in `icssg_qos.h`
>>>>> *) Modified implementation of `tas_set_trigger_list_change` to handle
>>>>>    cases where base-time isn't a multiple of cycle-time. For this a new
>>>>>    variable extend has to be calculated as base-time % cycle-time. This
>>>>>    variable is used by firmware to extend the last cycle.
>>>>> *) The API prueth_iep_gettime() and prueth_iep_settime() also needs to be
>>>>>    adjusted according to the cycle time extension. These changes are also
>>>>>    taken care in this patch.
>>>>
>>>> Why? Given the explanation of CycleTimeExtension above, it makes no
>>>> sense to me why you would alter the gettime() and settime() values.
>>>>
>>>
>>> The Firmware has two counters
>>>
>>> counter0 counts the number of miliseconds in current time
>>> counter1 counts the number of nanoseconds in the current ms.
>>>
>>> Let's say the current time is 1747305807237749032 ns.
>>> counter0 will read 1747305807237 counter1 will read 749032.
>>>
>>> The current time = counter0* 1ms + counter1
>>>
>>> For taprio scheduling also counter0 is used.
> 
> "Used" in the sense that taprio needs to know the current time, correct?
> But by that logic, taprio equally uses counter0 and counter1, no? For
> example, for a cycle-time of 1.23 ms.
> 

Counter0 is updated after every cycle. Since the last cycle below is
1.821 ms, counter0 will be incremented by 1 after 1.821 ms and 0.821ms
will not be written to couter1. This is compensated by the extend
variable in get/set_time APIs.

>>> Now let's say below are the cycles of a schedule
>>>
>>> cycles   = 500ms 501ms 502ms ... 997ms, 998ms, 999ms, 1000.821ms
>>> counter0 = 500   501   502   ... 997    998    999    1000
>>> curr_time= 500*1, 501*1, 502*2...997*1, 998*1, 999*1, 1000*1
>>>
>>> Here you see after the last cycle the time is 1000.821 however our above
>>> formula will give us 1000 as the time since last cycle was extended.
> 
> Wait a second. You compensate the time in prueth_iep_gettime(), which is
> called, among other places, from icss_iep_ptp_gettimeex() (aka struct
> ptp_clock_info :: gettimex64()).
> 
> I don't know about the other call paths, but ptp_clock_info :: gettimex64()
> doesn't answer the question "what was the last time that a taprio cycle
> ended at?" but rather "what time is it according this clock, now?"
> 
> I still fail to see why the taprio cycle extension would affect the
> current time. Or does TIMESYNC_CYCLE_EXTN_TIME extend the length of the
> millisecond?
> 

As I explained earlier, firmware only has one counter that it increases
post every cycle. By default that is 1ms. If a cycle is extended to
1.5ms, the counter still only increases by 1 and the 0.5 ms doesn't get
written to counter1. This is handled by TIMESYNC_CYCLE_EXTN_TIME.

>>> To compensate this, whatever extension firmware applies need to be added
>>> during current time calculation. Below is the code for that.
>>>
>>>       ts += readl(prueth->shram.va + TIMESYNC_CYCLE_EXTN_TIME);
>>>
>>> Now the current time becomes,
>>> 	counter0* 1ms + counter1 + EXTEND
>>>
>>> This is why change to set/get_time() APIs are needed. This will not be
>>> needed if we drop this extends implementation.
> 
> What if the cycle that has to be extended has not arrived yet (is in the
> future)? Why is the current time compensated in that case?
> 

I don't think it will compensate for those. It will only compensate for
the cycles that have already been extended not for those that are yet to
be extended.

Firmware writes what ever extensions it has done till now at
TIMESYNC_CYCLE_EXTN_TIME. During get time we read that and add it to
current time.

>>> Let me know if above explanation makes sense and if I should continue
>>> with this approach or drop the extend feature at all and just refuse the
>>> schedules?
>>>
>>
>> I am not sure if you got the change to review my replies to your initial
>> comments. Let me know if I should continue with this approach or just
>> refuse the schedules that don't have the base time that we need.
>>
>>> Thanks for the feedback.
>>>
>>
>>
>> -- 
>> Thanks and Regards,
>> Danish
> 
> As you can see, I still have trouble understanding the concepts proposed
> by the firmware.

I understand that. I hope this makes it a bit more clear. Let me know
what needs to be done now.

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[Vladimir Oltean]

On Tue, Jun 10, 2025 at 04:14:29PM +0530, MD Danish Anwar wrote:
> > 1. If there is no "existing" schedule, what does the "extend" variable
> >    extend? The custom base-time mechanism has to work even for the first
> >    taprio schedule. (this is an unanswered pre-existing question)
> 
> The firmware has a cycle-time of 1ms even if there is no schedule. Every
> 1ms, firmware updates a counter. The curr_time is calculated as
> CounterValue * 1ms.
> 
> Even if there are no schedule, the default cycle-time will remain 1ms.
> Let's say the first schedule has a base-time of 20.5ms then the default
> cycle will be extended by 0.5 ms and then the schedule will apply. This
> is the reason, the extend feature is also impacting get/set_time
> calculations.

So what is an ICSSG IEP cycle, then? I think this is another case where
the firmware calls something X, taprio (and 802.1Q) also calls something
X, and yet, they are talking about different things.

A schedule (in taprio terms) is an array of gate states and the
respective time intervals for which those states apply. The schedule is
periodic, and a cycle is the period of time after which the schedule
repeats itself. By default, the cycle time (the duration of the cycle)
is equal to the sum of the gate intervals, but it can also be longer or
shorter (extended or truncated schedule).

Whereas in the case of ICSSG, based on a code search for
IEP_DEFAULT_CYCLE_TIME_NS, a cycle seems to be some elementary unit of
timekeeping, used by the firmware API to express other information in
terms of it, like packet timestamps and periodic output. Would that be a
correct description?

When you say that "even if there is no schedule, the default cycle time
will remain 1ms", you are talking about the cycle time in the ICSSG
sense, because in the taprio/802.1Q sense, it is absurd to talk about a
cycle time in absense of a schedule. And implicitly, you are saying that
when the firmware extends the next-to-last cycle in order for unaligned
base times to work, this alters that timekeeping unit. Like stretching
the ruler in order for a mouse and an elephant to measure the same.

I think it needs to be explicitly pointed out that the taprio schedule
is only supposed to affect packet scheduling at the egress of the port,
but taprio is only a reader of the timekeeping process (and PTP time)
and should not alter it. The timekeeping process should be independent
and the taprio portion of the firmware should keep track of its own
"cycles" which may not begin at the beginning of a timekeeping "cycle",
may not end at the end of one, and may span multiple timekeeping
"cycles", respectively.

What if you also have a Qci (tc-gate) schedule on the ingress of the
same port, and that is configured for a different cycle-time or a
different base-time (requiring a different "extend" value)? It will be
too inflexible to apply the restriction that the parameters have to be
the same.

> >>> That's what our first approach was. If it's okay with you I can drop all
> >>> these changes and add below check in driver
> >>>
> >>> if (taprio->base_time % taprio->cycle_time) {
> >>> 	NL_SET_ERR_MSG_MOD(taprio->extack, "Base-time should be multiple of cycle-time");
> >>> 	return -EOPNOTSUPP;
> >>> }
> > 
> > I don't want to make a definitive statement on this just yet, I don't
> > fully understand what was implemented in the firmware and what was the
> > thinking.
> > 
> 
> The firmware always expects cycle-time of 1ms and base-time to multiple
> of cycle-time i.e. base-time to be multiple of 1ms.
> 
> This way all the schedules will be aligned and that's what current
> implementation is.
> 
> To add support for base-time that are not multiple of cycle-time we
> added "extend". However that will also only work as long as cycle-time
> is 1ms. cycle-time other than 1ms is not supported by firmware as of
> now. This is something we discovered recently.
> 
> We have a check for TAS_MIN_CYCLE_TIME and TAS_MAX_CYCLE_TIME and they
> are defined as,
> 
> /* Minimum cycle time supported by implementation (in ns) */
> #define TAS_MIN_CYCLE_TIME  (1000000)
> 
> /* Minimum cycle time supported by implementation (in ns) */
> #define TAS_MAX_CYCLE_TIME  (4000000000)
> 
> But it is wrong. As per current firmware implementation,
> 	TAS_MIN_CYCLE_TIME = TAS_MAX_CYCLE_TIME = 1ms
> 
> 
> The ideal use case will be to support,
> 1. Different cycle times
> 2. Different base times which may or may not be multiple of cycle-times.
> 
> With the current implementation, we are able to support #2 however #1 is
> still a limitation. Once support for #1 is added, the implementation
> will need to be changed.

(...)

> > As you can see, I still have trouble understanding the concepts proposed
> > by the firmware.
> 
> I understand that. I hope this makes it a bit more clear. Let me know
> what needs to be done now.

Was the "extend" feature added to the ICSSG firmware as a result of the
previous taprio review feedback? Because if it was, it's unfortunate
that because of the lack of clarity of the firmware concepts, the way
this feature was implemented is essentially DOA and cannot be used.

I am not very positive that even if adding the extra restrictions
discovered here (cycle-time cannot be != IEP_DEFAULT_CYCLE_TIME_NS),
the implementation will work as expected. I am not sure that our image
of "as expected" is the same.

Given that these don't seem to be hardware limitations, but constraints
imposed by the ICSSG firmware, I guess my suggestion would be to start
with the selftest I mentioned earlier (which may need to be adapted),
and use it to get a better picture of the gaps. Then make a plan to fix
them in the firmware, and see what it takes. If it isn't going to be
sufficient to fix the bugs unless major API changes are introduced, then
maybe it doesn't make sense for Linux to support taprio offload on the
buggy firmware versions.

Or maybe it does (with the appropriate restrictions), but it would still
inspire more trust to see that the developer at least got some version
of the firmware to pass a selftest, and has a valid reference to follow.
Not going to lie, it doesn't look great that we discover during v10 that
taprio offload only works with a cycle time of 1 ms. The schedule is
network-dependent and user-customizable, and maybe the users didn't get
the memo that only 1 ms was tested :-/

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[MD Danish Anwar]

On 10/06/25 8:32 pm, Vladimir Oltean wrote:
> On Tue, Jun 10, 2025 at 04:14:29PM +0530, MD Danish Anwar wrote:
>>> 1. If there is no "existing" schedule, what does the "extend" variable
>>>    extend? The custom base-time mechanism has to work even for the first
>>>    taprio schedule. (this is an unanswered pre-existing question)
>>
>> The firmware has a cycle-time of 1ms even if there is no schedule. Every
>> 1ms, firmware updates a counter. The curr_time is calculated as
>> CounterValue * 1ms.
>>
>> Even if there are no schedule, the default cycle-time will remain 1ms.
>> Let's say the first schedule has a base-time of 20.5ms then the default
>> cycle will be extended by 0.5 ms and then the schedule will apply. This
>> is the reason, the extend feature is also impacting get/set_time
>> calculations.
> 
> So what is an ICSSG IEP cycle, then? I think this is another case where
> the firmware calls something X, taprio (and 802.1Q) also calls something
> X, and yet, they are talking about different things.
> 
> A schedule (in taprio terms) is an array of gate states and the
> respective time intervals for which those states apply. The schedule is
> periodic, and a cycle is the period of time after which the schedule
> repeats itself. By default, the cycle time (the duration of the cycle)
> is equal to the sum of the gate intervals, but it can also be longer or
> shorter (extended or truncated schedule).
> 

Yes I understand that.

> Whereas in the case of ICSSG, based on a code search for
> IEP_DEFAULT_CYCLE_TIME_NS, a cycle seems to be some elementary unit of
> timekeeping, used by the firmware API to express other information in
> terms of it, like packet timestamps and periodic output. Would that be a
> correct description?
> 
> When you say that "even if there is no schedule, the default cycle time
> will remain 1ms", you are talking about the cycle time in the ICSSG
> sense, because in the taprio/802.1Q sense, it is absurd to talk about a

Yes I am. By default cycle-time, I meant IEP_DEFAULT_CYCLE_TIME_NS.

> cycle time in absense of a schedule. And implicitly, you are saying that
> when the firmware extends the next-to-last cycle in order for unaligned
> base times to work, this alters that timekeeping unit. Like stretching

Yes. This is exactly what's happening. The IEP cycle and schedule cycle
are related to each other in firmware implementation. I am not sure how,
but that's what the firmware team confirmed with me. Due to this, a
different cycle-time in scheduling impacts the IEP cycles and
get/set_time APIs.

> the ruler in order for a mouse and an elephant to measure the same.
> 
> I think it needs to be explicitly pointed out that the taprio schedule
> is only supposed to affect packet scheduling at the egress of the port,
> but taprio is only a reader of the timekeeping process (and PTP time)
> and should not alter it. The timekeeping process should be independent
> and the taprio portion of the firmware should keep track of its own
> "cycles" which may not begin at the beginning of a timekeeping "cycle",
> may not end at the end of one, and may span multiple timekeeping
> "cycles", respectively.
> 

I understand that. But there are some dependency in firmware which
results into taprio schedule impacting the IEP cycle.

> What if you also have a Qci (tc-gate) schedule on the ingress of the
> same port, and that is configured for a different cycle-time or a
> different base-time (requiring a different "extend" value)? It will be
> too inflexible to apply the restriction that the parameters have to be
> the same.
> 
>>>>> That's what our first approach was. If it's okay with you I can drop all
>>>>> these changes and add below check in driver
>>>>>
>>>>> if (taprio->base_time % taprio->cycle_time) {
>>>>> 	NL_SET_ERR_MSG_MOD(taprio->extack, "Base-time should be multiple of cycle-time");
>>>>> 	return -EOPNOTSUPP;
>>>>> }
>>>
>>> I don't want to make a definitive statement on this just yet, I don't
>>> fully understand what was implemented in the firmware and what was the
>>> thinking.
>>>
>>
>> The firmware always expects cycle-time of 1ms and base-time to multiple
>> of cycle-time i.e. base-time to be multiple of 1ms.
>>
>> This way all the schedules will be aligned and that's what current
>> implementation is.
>>
>> To add support for base-time that are not multiple of cycle-time we
>> added "extend". However that will also only work as long as cycle-time
>> is 1ms. cycle-time other than 1ms is not supported by firmware as of
>> now. This is something we discovered recently.
>>
>> We have a check for TAS_MIN_CYCLE_TIME and TAS_MAX_CYCLE_TIME and they
>> are defined as,
>>
>> /* Minimum cycle time supported by implementation (in ns) */
>> #define TAS_MIN_CYCLE_TIME  (1000000)
>>
>> /* Minimum cycle time supported by implementation (in ns) */
>> #define TAS_MAX_CYCLE_TIME  (4000000000)
>>
>> But it is wrong. As per current firmware implementation,
>> 	TAS_MIN_CYCLE_TIME = TAS_MAX_CYCLE_TIME = 1ms
>>
>>
>> The ideal use case will be to support,
>> 1. Different cycle times
>> 2. Different base times which may or may not be multiple of cycle-times.
>>
>> With the current implementation, we are able to support #2 however #1 is
>> still a limitation. Once support for #1 is added, the implementation
>> will need to be changed.
> 
> (...)
> 
>>> As you can see, I still have trouble understanding the concepts proposed
>>> by the firmware.
>>
>> I understand that. I hope this makes it a bit more clear. Let me know
>> what needs to be done now.
> 
> Was the "extend" feature added to the ICSSG firmware as a result of the
> previous taprio review feedback? Because if it was, it's unfortunate
> that because of the lack of clarity of the firmware concepts, the way
> this feature was implemented is essentially DOA and cannot be used.
> 

Yes, the extend feature was added based on the feedback on v9.

> I am not very positive that even if adding the extra restrictions
> discovered here (cycle-time cannot be != IEP_DEFAULT_CYCLE_TIME_NS),
> the implementation will work as expected. I am not sure that our image
> of "as expected" is the same.
> 
> Given that these don't seem to be hardware limitations, but constraints
> imposed by the ICSSG firmware, I guess my suggestion would be to start
> with the selftest I mentioned earlier (which may need to be adapted),

Yes I am working on running the selftest on ICSSG driver however there
are some setup issues that I am encountering. I will try to test this
using the selftest.

> and use it to get a better picture of the gaps. Then make a plan to fix
> them in the firmware, and see what it takes. If it isn't going to be
> sufficient to fix the bugs unless major API changes are introduced, then
> maybe it doesn't make sense for Linux to support taprio offload on the
> buggy firmware versions.
> 
> Or maybe it does (with the appropriate restrictions), but it would still
> inspire more trust to see that the developer at least got some version
> of the firmware to pass a selftest, and has a valid reference to follow.

Sure. I think we can go back to v9 implementation (no extend feature)
and add two additional restrictions in the driver.

1. Cycle-time needs to be 1ms
2. Base-time needs to be Multiple of 1ms

With these two restrictions we can have the basic taprio support. Once
the firmware is fixed and has support for both the above cases, I will
modify the driver as needed.

I know firmware is very buggy as of now. But we can still start the
driver integration and fix these bugs with time.

I will try to test the implementation with these two limitations using
the selftest and share the logs if it's okay with you to go ahead with
these limitations.

> Not going to lie, it doesn't look great that we discover during v10 that
> taprio offload only works with a cycle time of 1 ms. The schedule is

I understand that. Even I got to know about this limitation after my
last response to v10
(https://lore.kernel.org/all/5e928ff0-e75b-4618-b84c-609138598801@ti.com/)

> network-dependent and user-customizable, and maybe the users didn't get
> the memo that only 1 ms was tested :-/

Let me know if it'll be okay to go ahead with the two limitations
mentioned above for now (with selftest done).

If it's okay, I will try to send v11 with testing with selftest done as
well. Thanks for the continuous feedback.

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[Vladimir Oltean]

On Wed, Jun 11, 2025 at 03:10:35PM +0530, MD Danish Anwar wrote:
> > I am not very positive that even if adding the extra restrictions
> > discovered here (cycle-time cannot be != IEP_DEFAULT_CYCLE_TIME_NS),
> > the implementation will work as expected. I am not sure that our image
> > of "as expected" is the same.
> > 
> > Given that these don't seem to be hardware limitations, but constraints
> > imposed by the ICSSG firmware, I guess my suggestion would be to start
> > with the selftest I mentioned earlier (which may need to be adapted),
> 
> Yes I am working on running the selftest on ICSSG driver however there
> are some setup issues that I am encountering. I will try to test this
> using the selftest.
> 
> > and use it to get a better picture of the gaps. Then make a plan to fix
> > them in the firmware, and see what it takes. If it isn't going to be
> > sufficient to fix the bugs unless major API changes are introduced, then
> > maybe it doesn't make sense for Linux to support taprio offload on the
> > buggy firmware versions.
> > 
> > Or maybe it does (with the appropriate restrictions), but it would still
> > inspire more trust to see that the developer at least got some version
> > of the firmware to pass a selftest, and has a valid reference to follow.
> 
> Sure. I think we can go back to v9 implementation (no extend feature)
> and add two additional restrictions in the driver.
> 
> 1. Cycle-time needs to be 1ms
> 2. Base-time needs to be Multiple of 1ms
> 
> With these two restrictions we can have the basic taprio support. Once
> the firmware is fixed and has support for both the above cases, I will
> modify the driver as needed.
> 
> I know firmware is very buggy as of now. But we can still start the
> driver integration and fix these bugs with time.
> 
> I will try to test the implementation with these two limitations using
> the selftest and share the logs if it's okay with you to go ahead with
> these limitations.
> 
> > Not going to lie, it doesn't look great that we discover during v10 that
> > taprio offload only works with a cycle time of 1 ms. The schedule is
> 
> I understand that. Even I got to know about this limitation after my
> last response to v10
> (https://lore.kernel.org/all/5e928ff0-e75b-4618-b84c-609138598801@ti.com/)
> 
> > network-dependent and user-customizable, and maybe the users didn't get
> > the memo that only 1 ms was tested :-/
> 
> Let me know if it'll be okay to go ahead with the two limitations
> mentioned above for now (with selftest done).
> 
> If it's okay, I will try to send v11 with testing with selftest done as
> well. Thanks for the continuous feedback.

I don't want to gate your upstreaming efforts, but a new version with
just these extra restrictions, and no concrete plan to lift them, will
be seen with scepticism from reviewers. You can alleviate some of that
by showing results from a selftest.

The existing selftest uses a 2 ms schedule and a 10 ms schedule. Neither
of those is supported by your current proposal. You can modify the
schedules to be compatible with your current firmware, and the selftest
may pass that way, but I will not be in favor of accepting that change
upstream, because the cycle time is something that needs to be highly
adaptive to the network requirements.

So to summarize, you can try to move forward with a restricted version
of this feature, but you will have to be very transparent as to what
works and what are the next steps, as well as give assurance that you
intend to keep supporting the current firmware and its API when an
improved firmware will become available that lifts these restrictions.

Re: [PATCH net-next v10] net: ti: icssg-prueth: add TAPRIO offload support

[MD Danish Anwar]

On 12/06/25 8:40 pm, Vladimir Oltean wrote:
> On Wed, Jun 11, 2025 at 03:10:35PM +0530, MD Danish Anwar wrote:
>>> I am not very positive that even if adding the extra restrictions
>>> discovered here (cycle-time cannot be != IEP_DEFAULT_CYCLE_TIME_NS),
>>> the implementation will work as expected. I am not sure that our image
>>> of "as expected" is the same.
>>>
>>> Given that these don't seem to be hardware limitations, but constraints
>>> imposed by the ICSSG firmware, I guess my suggestion would be to start
>>> with the selftest I mentioned earlier (which may need to be adapted),
>>
>> Yes I am working on running the selftest on ICSSG driver however there
>> are some setup issues that I am encountering. I will try to test this
>> using the selftest.
>>
>>> and use it to get a better picture of the gaps. Then make a plan to fix
>>> them in the firmware, and see what it takes. If it isn't going to be
>>> sufficient to fix the bugs unless major API changes are introduced, then
>>> maybe it doesn't make sense for Linux to support taprio offload on the
>>> buggy firmware versions.
>>>
>>> Or maybe it does (with the appropriate restrictions), but it would still
>>> inspire more trust to see that the developer at least got some version
>>> of the firmware to pass a selftest, and has a valid reference to follow.
>>
>> Sure. I think we can go back to v9 implementation (no extend feature)
>> and add two additional restrictions in the driver.
>>
>> 1. Cycle-time needs to be 1ms
>> 2. Base-time needs to be Multiple of 1ms
>>
>> With these two restrictions we can have the basic taprio support. Once
>> the firmware is fixed and has support for both the above cases, I will
>> modify the driver as needed.
>>
>> I know firmware is very buggy as of now. But we can still start the
>> driver integration and fix these bugs with time.
>>
>> I will try to test the implementation with these two limitations using
>> the selftest and share the logs if it's okay with you to go ahead with
>> these limitations.
>>
>>> Not going to lie, it doesn't look great that we discover during v10 that
>>> taprio offload only works with a cycle time of 1 ms. The schedule is
>>
>> I understand that. Even I got to know about this limitation after my
>> last response to v10
>> (https://lore.kernel.org/all/5e928ff0-e75b-4618-b84c-609138598801@ti.com/)
>>
>>> network-dependent and user-customizable, and maybe the users didn't get
>>> the memo that only 1 ms was tested :-/
>>
>> Let me know if it'll be okay to go ahead with the two limitations
>> mentioned above for now (with selftest done).
>>
>> If it's okay, I will try to send v11 with testing with selftest done as
>> well. Thanks for the continuous feedback.
> 
> I don't want to gate your upstreaming efforts, but a new version with
> just these extra restrictions, and no concrete plan to lift them, will
> be seen with scepticism from reviewers. You can alleviate some of that
> by showing results from a selftest.

We will make a complete plan for fixing these restrictions and I will
update the community.

> 
> The existing selftest uses a 2 ms schedule and a 10 ms schedule. Neither
> of those is supported by your current proposal. You can modify the
> schedules to be compatible with your current firmware, and the selftest
> may pass that way, but I will not be in favor of accepting that change
> upstream, because the cycle time is something that needs to be highly
> adaptive to the network requirements.
> 

I can tweak the script to use 1ms cycle time. I tried to run the script
however I am not able to run the script due to multiple package
dependencies which I am currently trying to resolve.

I checked your commit [1] where you have introduced the tc_taprio.sh
script. You have mentioned,
	"This is specifically intended for NICs with an offloaded data path
(switchdev/DSA) and requires taprio 'flags 2'. Also, $h1 and $h2 must
support hardware timestamping, and $h1 tc-etf offload, for isochron to
work."

My NIC does support offloaded data path (switchdev), taprio 'flags 2'
and hardware timestamping. However we don't support tc-etf offload.

Will it be possible to use this script without the support of tc-etf
offload?

> So to summarize, you can try to move forward with a restricted version
> of this feature, but you will have to be very transparent as to what
> works and what are the next steps, as well as give assurance that you
> intend to keep supporting the current firmware and its API when an
> improved firmware will become available that lifts these restrictions.

We are working on a plan to get full taprio support and I will update
the community accordingly. I want the driver to get upstreamed with
whatever support we have right now and add things later on. If I am able
to verify the `tc_taprio.sh` script then I would share results from
there. If I am not able to verify the script, I will at least try to
mention what we are supporting now and what are the limitations and when
do we plan on addressing those limitations.

[1]
https://lore.kernel.org/all/20250426144859.3128352-5-vladimir.oltean@nxp.com/#r

-- 
Thanks and Regards,
Danish