[PATCH net-next 0/2] Add Frame Preemption MAC Merge support for ICSSG

[PATCH net-next 0/2] Add Frame Preemption MAC Merge support for ICSSG

[Meghana Malladi]

Intersperse Express Traffic (IET) Frame preemption (FPE) feature is
defined by IEEE 802.3 2018 and IEEE 802.1Q standards and is supported
by ICSSG firmware.

This series adds driver support for viewing / changing the MAC Merge
sublayer parameters and seeing the verification state machine's
current state via ethtool.

Driver configures the verify state machine in the firmware to check
the remote peer capability. If remote fails to respond to the verify
command, then FPE is disabled by firmware and TX FPE active status
is disabled.

Meghana Malladi (2):
  net: ti: icssg-prueth: Add Frame Preemption MAC Merge support
  net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

 drivers/net/ethernet/ti/Makefile              |   2 +-
 drivers/net/ethernet/ti/icssg/icssg_ethtool.c |  58 ++++
 drivers/net/ethernet/ti/icssg/icssg_prueth.c  |   9 +
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   4 +-
 drivers/net/ethernet/ti/icssg/icssg_qos.c     | 319 ++++++++++++++++++
 drivers/net/ethernet/ti/icssg/icssg_qos.h     |  48 +++
 drivers/net/ethernet/ti/icssg/icssg_stats.h   |   5 +
 .../net/ethernet/ti/icssg/icssg_switch_map.h  |   5 +
 8 files changed, 448 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.c
 create mode 100644 drivers/net/ethernet/ti/icssg/icssg_qos.h


base-commit: 8e7148b5602321be48614bcde048cbe1c738ce3e
-- 
2.43.0

[PATCH net-next 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[Meghana Malladi]

This patch adds utility functions to configure firmware to enable
IET FPE. The highest priority queue is marked as Express queue and
lower priority queues as pre-emptable, as the default configuration
which will be overwritten by the mqprio tc mask passed by tc qdisc.
Driver optionally allow configure the Verify state machine in the
firmware to check remote peer capability. If remote fails to respond
to Verify command, then FPE is disabled by firmware and TX FPE active
status is disabled.

This also adds the necessary hooks to enable IET/FPE feature in ICSSG
driver. IET/FPE gets configured when Link is up and gets disabled when link
goes down or device is stopped.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/Makefile             |   2 +-
 drivers/net/ethernet/ti/icssg/icssg_prueth.c |   9 +
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |   2 +
 drivers/net/ethernet/ti/icssg/icssg_qos.c    | 319 +++++++++++++++++++
 drivers/net/ethernet/ti/icssg/icssg_qos.h    |  48 +++
 5 files changed, 379 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/Makefile b/drivers/net/ethernet/ti/Makefile
index 93c0a4d0e33a..2f588663fdf0 100644
--- a/drivers/net/ethernet/ti/Makefile
+++ b/drivers/net/ethernet/ti/Makefile
@@ -35,7 +35,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 f65041662173..668177eba3f8 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -378,6 +378,12 @@ static void emac_adjust_link(struct net_device *ndev)
 		} else {
 			icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
 		}
+
+		if (emac->link) {
+			icssg_qos_link_up(ndev);
+		} else {
+			icssg_qos_link_down(ndev);
+		}
 	}
 
 	if (emac->link) {
@@ -967,6 +973,8 @@ static int emac_ndo_open(struct net_device *ndev)
 	if (ret)
 		goto destroy_rxq;
 
+	icssg_qos_init(ndev);
+
 	/* start PHY */
 	phy_start(ndev->phydev);
 
@@ -1421,6 +1429,7 @@ static const struct net_device_ops emac_netdev_ops = {
 	.ndo_hwtstamp_get = icssg_ndo_get_ts_config,
 	.ndo_hwtstamp_set = icssg_ndo_set_ts_config,
 	.ndo_xsk_wakeup = prueth_xsk_wakeup,
+	.ndo_setup_tc = icssg_qos_ndo_setup_tc,
 };
 
 static int prueth_netdev_init(struct prueth *prueth,
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 10eadd356650..7a586038adf8 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -44,6 +44,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)
@@ -255,6 +256,7 @@ struct prueth_emac {
 	struct bpf_prog *xdp_prog;
 	struct xdp_attachment_info xdpi;
 	int xsk_qid;
+	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..858268740dae
--- /dev/null
+++ b/drivers/net/ethernet/ti/icssg/icssg_qos.c
@@ -0,0 +1,319 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Texas Instruments ICSSG PRUETH QoS submodule
+ * Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#include "icssg_prueth.h"
+#include "icssg_switch_map.h"
+
+static int icssg_prueth_iet_fpe_enable(struct prueth_emac *emac);
+static void icssg_prueth_iet_fpe_disable(struct prueth_qos_iet *iet);
+static void icssg_qos_enable_ietfpe(struct work_struct *work);
+
+void icssg_qos_init(struct net_device *ndev)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+
+	if (!iet->fpe_configured)
+		return;
+
+	/* Init work queue for IET MAC verify process */
+	iet->emac = emac;
+	INIT_WORK(&iet->fpe_config_task, icssg_qos_enable_ietfpe);
+	init_completion(&iet->fpe_config_compl);
+
+	/* As worker may be sleeping, check this flag to abort
+	 * as soon as it comes of out of sleep and cancel the
+	 * fpe config task.
+	 */
+	atomic_set(&iet->cancel_fpe_config, 0);
+}
+
+static void icssg_iet_set_preempt_mask(struct prueth_emac *emac, u8 preemptible_tcs)
+{
+	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
+	struct prueth_qos_mqprio *p_mqprio = &emac->qos.mqprio;
+	struct tc_mqprio_qopt *qopt = &p_mqprio->mqprio.qopt;
+	u8 tc;
+	int i;
+
+	/* Configure highest queue as express. Set Bit 4 for FPE,
+	 * Reset for express
+	 */
+
+	/* first set all 8 queues as Preemptive */
+	for (i = 0; i < PRUETH_MAX_TX_QUEUES * PRUETH_NUM_MACS; i++)
+		writeb(BIT(4), config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
+
+	/* set highest priority channel queue as express as default configuration */
+	writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + emac->tx_ch_num - 1);
+
+	/* set up queue mask for FPE. 1 means express */
+	writeb(BIT(emac->tx_ch_num - 1), config + EXPRESS_PRE_EMPTIVE_Q_MASK);
+
+	/* Overwrite the express queue mapping based on the tc map set by the user */
+	for (tc = 0; tc < p_mqprio->mqprio.qopt.num_tc; tc++) {
+		/* check if the tc is express or not */
+		if (!(p_mqprio->preemptible_tcs & BIT(tc))) {
+			for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++) {
+				/* Set all the queues in this tc as express queues */
+				writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
+				writeb(BIT(i), config + EXPRESS_PRE_EMPTIVE_Q_MASK);
+			}
+		}
+		netdev_set_tc_queue(emac->ndev, tc, qopt->count[tc], qopt->offset[tc]);
+	}
+}
+
+static int prueth_mqprio_validate(struct net_device *ndev,
+				  struct tc_mqprio_qopt_offload *mqprio)
+{
+	int num_tc = mqprio->qopt.num_tc;
+	int queue_count = 0;
+	int i;
+
+	/* Always start tc-queue mapping from queue 0 */
+	if (mqprio->qopt.offset[0] != 0)
+		return -EINVAL;
+
+	/* Check for valid number of traffic classes */
+	if (num_tc < 1 || num_tc > PRUETH_MAX_TX_QUEUES)
+		return -EINVAL;
+
+	/* Only channel mode is supported */
+	if (mqprio->mode != TC_MQPRIO_MODE_CHANNEL) {
+		netdev_err(ndev, "Unsupported mode: %d\n", mqprio->mode);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < num_tc; i++) {
+		if (!mqprio->qopt.count[i]) {
+			netdev_err(ndev, "TC %d has zero size queue count: %d\n",
+				   i, mqprio->qopt.count[i]);
+			return -EINVAL;
+		}
+		if (mqprio->min_rate[i] || mqprio->max_rate[i]) {
+			netdev_err(ndev, "Min/Max tx rate is not supported\n");
+			return -EINVAL;
+		}
+		if (mqprio->qopt.offset[i] != queue_count) {
+			netdev_err(ndev, "Discontinuous queues config is not supported\n");
+			return -EINVAL;
+		}
+		queue_count += mqprio->qopt.count[i];
+	}
+
+	if (queue_count > PRUETH_MAX_TX_QUEUES) {
+		netdev_err(ndev, "Total queues %d exceed max %d\n",
+			   queue_count, PRUETH_MAX_TX_QUEUES);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+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_MQPRIO: {
+		struct tc_mqprio_caps *caps = base->caps;
+
+		caps->validate_queue_counts = true;
+		return 0;
+	}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int emac_tc_setup_mqprio(struct net_device *ndev, void *type_data)
+{
+	struct tc_mqprio_qopt_offload *mqprio = type_data;
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_mqprio *p_mqprio;
+	int ret;
+
+	if (mqprio->qopt.hw == TC_MQPRIO_HW_OFFLOAD_TCS)
+		return -EOPNOTSUPP;
+
+	if (!mqprio->qopt.num_tc) {
+		netdev_reset_tc(ndev);
+		p_mqprio->preemptible_tcs = 0;
+		return 0;
+	}
+
+	ret = prueth_mqprio_validate(ndev, mqprio);
+	if (ret)
+		return ret;
+
+	p_mqprio = &emac->qos.mqprio;
+	memcpy(&p_mqprio->mqprio, mqprio, sizeof(*mqprio));
+	netdev_set_num_tc(ndev, mqprio->qopt.num_tc);
+
+	return 0;
+}
+
+int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
+			   void *type_data)
+{
+	switch (type) {
+	case TC_QUERY_CAPS:
+		return emac_tc_query_caps(ndev, type_data);
+	case TC_SETUP_QDISC_MQPRIO:
+		return emac_tc_setup_mqprio(ndev, type_data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+EXPORT_SYMBOL_GPL(icssg_qos_ndo_setup_tc);
+
+void icssg_qos_link_up(struct net_device *ndev)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+
+	if (!iet->fpe_configured)
+		return;
+
+	icssg_prueth_iet_fpe_enable(emac);
+}
+
+void icssg_qos_link_down(struct net_device *ndev)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+
+	if (iet->fpe_configured)
+		icssg_prueth_iet_fpe_disable(iet);
+}
+
+static int icssg_config_ietfpe(struct prueth_qos_iet *iet, bool enable)
+{
+	void __iomem *config = iet->emac->dram.va + ICSSG_CONFIG_OFFSET;
+	struct prueth_qos_mqprio *p_mqprio =  &iet->emac->qos.mqprio;
+	int ret;
+	u8 val;
+
+	/* If FPE is to be enabled, first configure MAC Verify state
+	 * machine in firmware as firmware kicks the Verify process
+	 * as soon as ICSSG_EMAC_PORT_PREMPT_TX_ENABLE command is
+	 * received.
+	 */
+	if (enable && iet->mac_verify_configured) {
+		writeb(1, config + PRE_EMPTION_ENABLE_VERIFY);
+		writew(iet->tx_min_frag_size, config + PRE_EMPTION_ADD_FRAG_SIZE_LOCAL);
+		writel(iet->verify_time_ms, config + PRE_EMPTION_VERIFY_TIME);
+	}
+
+	/* Send command to enable FPE Tx side. Rx is always enabled */
+	ret = icssg_set_port_state(iet->emac,
+				   enable ? ICSSG_EMAC_PORT_PREMPT_TX_ENABLE :
+					    ICSSG_EMAC_PORT_PREMPT_TX_DISABLE);
+	if (ret) {
+		netdev_err(iet->emac->ndev, "TX preempt %s command failed\n",
+			   str_enable_disable(enable));
+		writeb(0, config + PRE_EMPTION_ENABLE_VERIFY);
+		return ret;
+	}
+
+	/* Update FPE Tx enable bit. Assume firmware use this bit
+	 * and enable PRE_EMPTION_ACTIVE_TX if everything looks
+	 * good at firmware
+	 */
+	writeb(enable ? 1 : 0, config + PRE_EMPTION_ENABLE_TX);
+
+	if (enable && iet->mac_verify_configured) {
+		ret = readb_poll_timeout(config + PRE_EMPTION_VERIFY_STATUS, val,
+					 (val == ICSSG_IETFPE_STATE_SUCCEEDED),
+					 USEC_PER_MSEC, 5 * USEC_PER_SEC);
+		if (ret) {
+			netdev_err(iet->emac->ndev,
+				   "timeout for MAC Verify: status %x\n",
+				   val);
+			return ret;
+		}
+	} else {
+		/* Give f/w some time to update PRE_EMPTION_ACTIVE_TX state */
+		usleep_range(100, 200);
+	}
+
+	if (enable) {
+		val = readb(config + PRE_EMPTION_ACTIVE_TX);
+		if (val != 1) {
+			netdev_err(iet->emac->ndev,
+				   "F/w fails to activate IET/FPE\n");
+			writeb(0, config + PRE_EMPTION_ENABLE_TX);
+			return -ENODEV;
+		}
+	} else {
+		return 0;
+	}
+
+	icssg_iet_set_preempt_mask(iet->emac, p_mqprio->preemptible_tcs);
+
+	iet->fpe_enabled = true;
+
+	return ret;
+}
+
+static void icssg_qos_enable_ietfpe(struct work_struct *work)
+{
+	struct prueth_qos_iet *iet =
+		container_of(work, struct prueth_qos_iet, fpe_config_task);
+	int ret;
+
+	/* Set the required flag and send a command to ICSSG firmware to
+	 * enable FPE and start MAC verify
+	 */
+	ret = icssg_config_ietfpe(iet, true);
+
+	/* if verify configured, poll for the status and complete.
+	 * Or just do completion
+	 */
+	if (!ret)
+		netdev_err(iet->emac->ndev, "IET FPE configured successfully\n");
+	else
+		netdev_err(iet->emac->ndev, "IET FPE config error\n");
+	complete(&iet->fpe_config_compl);
+}
+
+static void icssg_prueth_iet_fpe_disable(struct prueth_qos_iet *iet)
+{
+	int ret;
+
+	atomic_set(&iet->cancel_fpe_config, 1);
+	cancel_work_sync(&iet->fpe_config_task);
+	ret = icssg_config_ietfpe(iet, false);
+	if (!ret)
+		netdev_err(iet->emac->ndev, "IET FPE disabled successfully\n");
+	else
+		netdev_err(iet->emac->ndev, "IET FPE disable failed\n");
+}
+
+static int icssg_prueth_iet_fpe_enable(struct prueth_emac *emac)
+{
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	int ret;
+
+	/* Schedule MAC Verify and enable IET FPE if configured */
+	atomic_set(&iet->cancel_fpe_config, 0);
+	reinit_completion(&iet->fpe_config_compl);
+	schedule_work(&iet->fpe_config_task);
+	/* By trial, found it takes about 1.5s. So
+	 * wait for 10s
+	 */
+	ret = wait_for_completion_timeout(&iet->fpe_config_compl,
+					  msecs_to_jiffies(10000));
+	if (!ret) {
+		netdev_err(emac->ndev,
+			   "IET verify completion timeout\n");
+		/* cancel verify in progress */
+		atomic_set(&iet->cancel_fpe_config, 1);
+		cancel_work_sync(&iet->fpe_config_task);
+	}
+
+	return ret;
+}
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..3d3f42107dd7
--- /dev/null
+++ b/drivers/net/ethernet/ti/icssg/icssg_qos.h
@@ -0,0 +1,48 @@
+/* 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>
+
+struct prueth_qos_mqprio {
+	struct tc_mqprio_qopt_offload mqprio;
+	u8 preemptible_tcs;
+};
+
+struct prueth_qos_iet {
+	struct work_struct fpe_config_task;
+	struct completion fpe_config_compl;
+	struct prueth_emac *emac;
+	atomic_t cancel_fpe_config;
+	/* Set through priv flags to enable IET frame preemption */
+	bool fpe_configured;
+	/* Set through priv flags to enable IET MAC Verify state machine
+	 * in firmware
+	 */
+	bool mac_verify_configured;
+	/* Min TX fragment size, set via ethtool */
+	u32 tx_min_frag_size;
+	/* wait time between verification attempts in ms (according to clause
+	 * 30.14.1.6 aMACMergeVerifyTime), set via ethtool
+	 */
+	u32 verify_time_ms;
+	/* Set if IET FPE is active */
+	bool fpe_enabled;
+};
+
+struct prueth_qos {
+	struct prueth_qos_iet iet;
+	struct prueth_qos_mqprio mqprio;
+};
+
+void icssg_qos_init(struct net_device *ndev);
+void icssg_qos_link_up(struct net_device *ndev);
+void icssg_qos_link_down(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 */
-- 
2.43.0

Re: [PATCH net-next 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[Vadim Fedorenko]

On 07/01/2026 12:51, Meghana Malladi wrote:
> This patch adds utility functions to configure firmware to enable
> IET FPE. The highest priority queue is marked as Express queue and
> lower priority queues as pre-emptable, as the default configuration
> which will be overwritten by the mqprio tc mask passed by tc qdisc.
> Driver optionally allow configure the Verify state machine in the
> firmware to check remote peer capability. If remote fails to respond
> to Verify command, then FPE is disabled by firmware and TX FPE active
> status is disabled.
> 
> This also adds the necessary hooks to enable IET/FPE feature in ICSSG
> driver. IET/FPE gets configured when Link is up and gets disabled when link
> goes down or device is stopped.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> ---
>   drivers/net/ethernet/ti/Makefile             |   2 +-
>   drivers/net/ethernet/ti/icssg/icssg_prueth.c |   9 +
>   drivers/net/ethernet/ti/icssg/icssg_prueth.h |   2 +
>   drivers/net/ethernet/ti/icssg/icssg_qos.c    | 319 +++++++++++++++++++
>   drivers/net/ethernet/ti/icssg/icssg_qos.h    |  48 +++
>   5 files changed, 379 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/Makefile b/drivers/net/ethernet/ti/Makefile
> index 93c0a4d0e33a..2f588663fdf0 100644
> --- a/drivers/net/ethernet/ti/Makefile
> +++ b/drivers/net/ethernet/ti/Makefile
> @@ -35,7 +35,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 f65041662173..668177eba3f8 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -378,6 +378,12 @@ static void emac_adjust_link(struct net_device *ndev)
>   		} else {
>   			icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
>   		}
> +
> +		if (emac->link) {
> +			icssg_qos_link_up(ndev);
> +		} else {
> +			icssg_qos_link_down(ndev);
> +		}

I believe this chunk can be incorporated into if-statement right above

>   	}
>   
>   	if (emac->link) {

[...]

Re: [EXTERNAL] Re: [PATCH net-next 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[Meghana Malladi]

Hi Vadim,

On 1/7/26 22:02, Vadim Fedorenko wrote:
> On 07/01/2026 12: 51, Meghana Malladi wrote: > This patch adds utility 
> functions to configure firmware to enable > IET FPE. The highest 
> priority queue is marked as Express queue and > lower priority queues as 
> pre-emptable, as the default
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! 
> urdgN3dP92274CXOvJKDvwNZ1ZfDoJYHZ2okAgDbtjDN3kmhmKUq71GAYLnzoFwx2mpfBIORw9Qoq3FoueFsPK2L6imycg$>
> ZjQcmQRYFpfptBannerEnd
> 
> On 07/01/2026 12:51, Meghana Malladi wrote:
>> This patch adds utility functions to configure firmware to enable
>> IET FPE. The highest priority queue is marked as Express queue and
>> lower priority queues as pre-emptable, as the default configuration
>> which will be overwritten by the mqprio tc mask passed by tc qdisc.
>> Driver optionally allow configure the Verify state machine in the
>> firmware to check remote peer capability. If remote fails to respond
>> to Verify command, then FPE is disabled by firmware and TX FPE active
>> status is disabled.
>> 
>> This also adds the necessary hooks to enable IET/FPE feature in ICSSG
>> driver. IET/FPE gets configured when Link is up and gets disabled when link
>> goes down or device is stopped.
>> 
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>> ---
>>   drivers/net/ethernet/ti/Makefile             |   2 +-
>>   drivers/net/ethernet/ti/icssg/icssg_prueth.c |   9 +
>>   drivers/net/ethernet/ti/icssg/icssg_prueth.h |   2 +
>>   drivers/net/ethernet/ti/icssg/icssg_qos.c    | 319 +++++++++++++++++++
>>   drivers/net/ethernet/ti/icssg/icssg_qos.h    |  48 +++
>>   5 files changed, 379 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/Makefile b/drivers/net/ethernet/ti/Makefile
>> index 93c0a4d0e33a..2f588663fdf0 100644
>> --- a/drivers/net/ethernet/ti/Makefile
>> +++ b/drivers/net/ethernet/ti/Makefile
>> @@ -35,7 +35,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 f65041662173..668177eba3f8 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -378,6 +378,12 @@ static void emac_adjust_link(struct net_device *ndev)
>>   		} else {
>>   			icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
>>   		}
>> +
>> +		if (emac->link) {
>> +			icssg_qos_link_up(ndev);
>> +		} else {
>> +			icssg_qos_link_down(ndev);
>> +		}
> 
> I believe this chunk can be incorporated into if-statement right above
> 

Yeah sure. Will update this as part of v2. Thanks.

>>   	}
>>   
>>   	if (emac->link) {
> 
> [...]
> 

Re: [PATCH net-next 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[Vladimir Oltean]

Hi Meghana,

On Wed, Jan 07, 2026 at 06:21:10PM +0530, Meghana Malladi wrote:
> This patch adds utility functions to configure firmware to enable
> IET FPE. The highest priority queue is marked as Express queue and
> lower priority queues as pre-emptable, as the default configuration
> which will be overwritten by the mqprio tc mask passed by tc qdisc.
> Driver optionally allow configure the Verify state machine in the
> firmware to check remote peer capability. If remote fails to respond
> to Verify command, then FPE is disabled by firmware and TX FPE active

"If remote fails to respond to Verify command, then FPE is disabled by
firmware" -> please clarify that. There is also a question on the code
about this.

> status is disabled.
> 
> This also adds the necessary hooks to enable IET/FPE feature in ICSSG
> driver. IET/FPE gets configured when Link is up and gets disabled when link
> goes down or device is stopped.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> ---
>  drivers/net/ethernet/ti/Makefile             |   2 +-
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c |   9 +
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |   2 +
>  drivers/net/ethernet/ti/icssg/icssg_qos.c    | 319 +++++++++++++++++++
>  drivers/net/ethernet/ti/icssg/icssg_qos.h    |  48 +++
>  5 files changed, 379 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/Makefile b/drivers/net/ethernet/ti/Makefile
> index 93c0a4d0e33a..2f588663fdf0 100644
> --- a/drivers/net/ethernet/ti/Makefile
> +++ b/drivers/net/ethernet/ti/Makefile
> @@ -35,7 +35,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 f65041662173..668177eba3f8 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -378,6 +378,12 @@ static void emac_adjust_link(struct net_device *ndev)
>  		} else {
>  			icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
>  		}
> +
> +		if (emac->link) {
> +			icssg_qos_link_up(ndev);
> +		} else {
> +			icssg_qos_link_down(ndev);
> +		}
>  	}
>  
>  	if (emac->link) {
> @@ -967,6 +973,8 @@ static int emac_ndo_open(struct net_device *ndev)
>  	if (ret)
>  		goto destroy_rxq;
>  
> +	icssg_qos_init(ndev);
> +
>  	/* start PHY */
>  	phy_start(ndev->phydev);
>  
> @@ -1421,6 +1429,7 @@ static const struct net_device_ops emac_netdev_ops = {
>  	.ndo_hwtstamp_get = icssg_ndo_get_ts_config,
>  	.ndo_hwtstamp_set = icssg_ndo_set_ts_config,
>  	.ndo_xsk_wakeup = prueth_xsk_wakeup,
> +	.ndo_setup_tc = icssg_qos_ndo_setup_tc,
>  };
>  
>  static int prueth_netdev_init(struct prueth *prueth,
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index 10eadd356650..7a586038adf8 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -44,6 +44,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)
> @@ -255,6 +256,7 @@ struct prueth_emac {
>  	struct bpf_prog *xdp_prog;
>  	struct xdp_attachment_info xdpi;
>  	int xsk_qid;
> +	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..858268740dae
> --- /dev/null
> +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.c
> @@ -0,0 +1,319 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Texas Instruments ICSSG PRUETH QoS submodule
> + * Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +
> +#include "icssg_prueth.h"
> +#include "icssg_switch_map.h"
> +
> +static int icssg_prueth_iet_fpe_enable(struct prueth_emac *emac);
> +static void icssg_prueth_iet_fpe_disable(struct prueth_qos_iet *iet);
> +static void icssg_qos_enable_ietfpe(struct work_struct *work);

Please order functions such as to avoid forward declarations.

> +
> +void icssg_qos_init(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +
> +	if (!iet->fpe_configured)
> +		return;

Bug:

if you open (ip link set ... up) the interface first, emac_ndo_open() ->
icssg_qos_init() will run, and will find iet->fpe_configured false,
exiting early.

Then you run ethtool --set-mm ... tx-enabled on, which sets
iet->fpe_configured = true.

Then you insert a cable and icssg_qos_link_up() -> icssg_prueth_iet_fpe_enable()
runs, which calls schedule_work(&iet->fpe_config_task). But you've never
called INIT_WORK() on this structure, so the kernel will oops.

> +
> +	/* Init work queue for IET MAC verify process */
> +	iet->emac = emac;
> +	INIT_WORK(&iet->fpe_config_task, icssg_qos_enable_ietfpe);
> +	init_completion(&iet->fpe_config_compl);
> +
> +	/* As worker may be sleeping, check this flag to abort
> +	 * as soon as it comes of out of sleep and cancel the
> +	 * fpe config task.
> +	 */
> +	atomic_set(&iet->cancel_fpe_config, 0);
> +}
> +
> +static void icssg_iet_set_preempt_mask(struct prueth_emac *emac, u8 preemptible_tcs)
> +{
> +	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
> +	struct prueth_qos_mqprio *p_mqprio = &emac->qos.mqprio;
> +	struct tc_mqprio_qopt *qopt = &p_mqprio->mqprio.qopt;
> +	u8 tc;
> +	int i;
> +
> +	/* Configure highest queue as express. Set Bit 4 for FPE,
> +	 * Reset for express
> +	 */

I wasn't able to parse the code below, I just got stuck on this comment.
Why configure just the highest queue as express, rather than all queues
as express?

What is the current default behaviour in mainline, and does this
constitute a change of that behaviour?

> +
> +	/* first set all 8 queues as Preemptive */
> +	for (i = 0; i < PRUETH_MAX_TX_QUEUES * PRUETH_NUM_MACS; i++)

What is the purpose of PRUETH_NUM_MACS here? Is the FPE configuration of
PRUETH_MAC0 not independent of that of PRUETH_MAC1?

> +		writeb(BIT(4), config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
> +
> +	/* set highest priority channel queue as express as default configuration */
> +	writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + emac->tx_ch_num - 1);
> +
> +	/* set up queue mask for FPE. 1 means express */
> +	writeb(BIT(emac->tx_ch_num - 1), config + EXPRESS_PRE_EMPTIVE_Q_MASK);
> +
> +	/* Overwrite the express queue mapping based on the tc map set by the user */
> +	for (tc = 0; tc < p_mqprio->mqprio.qopt.num_tc; tc++) {
> +		/* check if the tc is express or not */
> +		if (!(p_mqprio->preemptible_tcs & BIT(tc))) {
> +			for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++) {
> +				/* Set all the queues in this tc as express queues */
> +				writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
> +				writeb(BIT(i), config + EXPRESS_PRE_EMPTIVE_Q_MASK);
> +			}
> +		}
> +		netdev_set_tc_queue(emac->ndev, tc, qopt->count[tc], qopt->offset[tc]);
> +	}
> +}
> +
> +static int prueth_mqprio_validate(struct net_device *ndev,
> +				  struct tc_mqprio_qopt_offload *mqprio)
> +{
> +	int num_tc = mqprio->qopt.num_tc;
> +	int queue_count = 0;
> +	int i;
> +
> +	/* Always start tc-queue mapping from queue 0 */
> +	if (mqprio->qopt.offset[0] != 0)
> +		return -EINVAL;
> +
> +	/* Check for valid number of traffic classes */
> +	if (num_tc < 1 || num_tc > PRUETH_MAX_TX_QUEUES)
> +		return -EINVAL;
> +
> +	/* Only channel mode is supported */
> +	if (mqprio->mode != TC_MQPRIO_MODE_CHANNEL) {
> +		netdev_err(ndev, "Unsupported mode: %d\n", mqprio->mode);

struct tc_mqprio_qopt_offload has an extack argument. Please use
NL_SET_EXT_MSG_MOD(extack) instead of netdev_err() here and below.

> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < num_tc; i++) {
> +		if (!mqprio->qopt.count[i]) {
> +			netdev_err(ndev, "TC %d has zero size queue count: %d\n",
> +				   i, mqprio->qopt.count[i]);
> +			return -EINVAL;
> +		}

You set caps->validate_queue_counts = true, which is laudable. But did
you also look at what mqprio_validate_queue_counts() does? Do you need
to duplicate it here?

> +		if (mqprio->min_rate[i] || mqprio->max_rate[i]) {
> +			netdev_err(ndev, "Min/Max tx rate is not supported\n");
> +			return -EINVAL;
> +		}
> +		if (mqprio->qopt.offset[i] != queue_count) {
> +			netdev_err(ndev, "Discontinuous queues config is not supported\n");
> +			return -EINVAL;
> +		}
> +		queue_count += mqprio->qopt.count[i];
> +	}
> +
> +	if (queue_count > PRUETH_MAX_TX_QUEUES) {
> +		netdev_err(ndev, "Total queues %d exceed max %d\n",
> +			   queue_count, PRUETH_MAX_TX_QUEUES);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +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_MQPRIO: {
> +		struct tc_mqprio_caps *caps = base->caps;
> +
> +		caps->validate_queue_counts = true;
> +		return 0;
> +	}
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int emac_tc_setup_mqprio(struct net_device *ndev, void *type_data)
> +{
> +	struct tc_mqprio_qopt_offload *mqprio = type_data;
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_mqprio *p_mqprio;
> +	int ret;
> +
> +	if (mqprio->qopt.hw == TC_MQPRIO_HW_OFFLOAD_TCS)
> +		return -EOPNOTSUPP;
> +
> +	if (!mqprio->qopt.num_tc) {
> +		netdev_reset_tc(ndev);
> +		p_mqprio->preemptible_tcs = 0;
> +		return 0;
> +	}
> +
> +	ret = prueth_mqprio_validate(ndev, mqprio);
> +	if (ret)
> +		return ret;
> +
> +	p_mqprio = &emac->qos.mqprio;
> +	memcpy(&p_mqprio->mqprio, mqprio, sizeof(*mqprio));
> +	netdev_set_num_tc(ndev, mqprio->qopt.num_tc);
> +
> +	return 0;
> +}
> +
> +int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
> +			   void *type_data)
> +{
> +	switch (type) {
> +	case TC_QUERY_CAPS:
> +		return emac_tc_query_caps(ndev, type_data);
> +	case TC_SETUP_QDISC_MQPRIO:
> +		return emac_tc_setup_mqprio(ndev, type_data);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(icssg_qos_ndo_setup_tc);
> +
> +void icssg_qos_link_up(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +
> +	if (!iet->fpe_configured)
> +		return;
> +
> +	icssg_prueth_iet_fpe_enable(emac);
> +}
> +
> +void icssg_qos_link_down(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +
> +	if (iet->fpe_configured)
> +		icssg_prueth_iet_fpe_disable(iet);
> +}
> +
> +static int icssg_config_ietfpe(struct prueth_qos_iet *iet, bool enable)
> +{
> +	void __iomem *config = iet->emac->dram.va + ICSSG_CONFIG_OFFSET;
> +	struct prueth_qos_mqprio *p_mqprio =  &iet->emac->qos.mqprio;
> +	int ret;
> +	u8 val;
> +
> +	/* If FPE is to be enabled, first configure MAC Verify state
> +	 * machine in firmware as firmware kicks the Verify process
> +	 * as soon as ICSSG_EMAC_PORT_PREMPT_TX_ENABLE command is
> +	 * received.
> +	 */
> +	if (enable && iet->mac_verify_configured) {
> +		writeb(1, config + PRE_EMPTION_ENABLE_VERIFY);

When mac_verify_configured transitions from true to false, who disables
the feature in firmware? Or do you have to reboot the board?

> +		writew(iet->tx_min_frag_size, config + PRE_EMPTION_ADD_FRAG_SIZE_LOCAL);
> +		writel(iet->verify_time_ms, config + PRE_EMPTION_VERIFY_TIME);
> +	}
> +
> +	/* Send command to enable FPE Tx side. Rx is always enabled */
> +	ret = icssg_set_port_state(iet->emac,
> +				   enable ? ICSSG_EMAC_PORT_PREMPT_TX_ENABLE :
> +					    ICSSG_EMAC_PORT_PREMPT_TX_DISABLE);
> +	if (ret) {
> +		netdev_err(iet->emac->ndev, "TX preempt %s command failed\n",
> +			   str_enable_disable(enable));
> +		writeb(0, config + PRE_EMPTION_ENABLE_VERIFY);
> +		return ret;
> +	}
> +
> +	/* Update FPE Tx enable bit. Assume firmware use this bit
> +	 * and enable PRE_EMPTION_ACTIVE_TX if everything looks
> +	 * good at firmware
> +	 */
> +	writeb(enable ? 1 : 0, config + PRE_EMPTION_ENABLE_TX);
> +
> +	if (enable && iet->mac_verify_configured) {
> +		ret = readb_poll_timeout(config + PRE_EMPTION_VERIFY_STATUS, val,
> +					 (val == ICSSG_IETFPE_STATE_SUCCEEDED),
> +					 USEC_PER_MSEC, 5 * USEC_PER_SEC);
> +		if (ret) {
> +			netdev_err(iet->emac->ndev,
> +				   "timeout for MAC Verify: status %x\n",
> +				   val);
> +			return ret;
> +		}
> +	} else {
> +		/* Give f/w some time to update PRE_EMPTION_ACTIVE_TX state */
> +		usleep_range(100, 200);
> +	}
> +
> +	if (enable) {
> +		val = readb(config + PRE_EMPTION_ACTIVE_TX);
> +		if (val != 1) {
> +			netdev_err(iet->emac->ndev,
> +				   "F/w fails to activate IET/FPE\n");
> +			writeb(0, config + PRE_EMPTION_ENABLE_TX);

Why do you write 0 to PRE_EMPTION_ENABLE_TX here? You previously said
that "FPE is disabled by firmware" which I suppose translates into the
same (incorrect) thing? Is it disabled by firmware, or by the driver, or
by both?

When FPE is enabled with verification, but the link partner does not
respond, the ENABLED status remains, but the ACTIVE status never
transitions to true. It is documented that preemptible traffic should
only be sent based on the ACTIVE status, not the ENABLED one. So, I
don't think this is necessary at all.

> +			return -ENODEV;
> +		}
> +	} else {
> +		return 0;
> +	}
> +
> +	icssg_iet_set_preempt_mask(iet->emac, p_mqprio->preemptible_tcs);
> +
> +	iet->fpe_enabled = true;

iet->fpe_enabled is set to true here and never to false. It tracks no
useful information, other than "was icssg_config_ietfpe() ever called
for this interface with enable=true?". You assign that result to
state->tx_enabled in emac_get_mm(), which is obviously wrong because
this variable does not track that information.  For example, if you call
icssg_config_ietfpe() with enable=false, FPE should be enabled, and
emac_get_mm() should reflect that.

> +
> +	return ret;
> +}
> +
> +static void icssg_qos_enable_ietfpe(struct work_struct *work)
> +{
> +	struct prueth_qos_iet *iet =
> +		container_of(work, struct prueth_qos_iet, fpe_config_task);
> +	int ret;
> +
> +	/* Set the required flag and send a command to ICSSG firmware to
> +	 * enable FPE and start MAC verify
> +	 */
> +	ret = icssg_config_ietfpe(iet, true);
> +
> +	/* if verify configured, poll for the status and complete.
> +	 * Or just do completion
> +	 */
> +	if (!ret)
> +		netdev_err(iet->emac->ndev, "IET FPE configured successfully\n");
> +	else
> +		netdev_err(iet->emac->ndev, "IET FPE config error\n");
> +	complete(&iet->fpe_config_compl);
> +}
> +
> +static void icssg_prueth_iet_fpe_disable(struct prueth_qos_iet *iet)
> +{
> +	int ret;
> +
> +	atomic_set(&iet->cancel_fpe_config, 1);
> +	cancel_work_sync(&iet->fpe_config_task);
> +	ret = icssg_config_ietfpe(iet, false);
> +	if (!ret)
> +		netdev_err(iet->emac->ndev, "IET FPE disabled successfully\n");
> +	else
> +		netdev_err(iet->emac->ndev, "IET FPE disable failed\n");
> +}
> +
> +static int icssg_prueth_iet_fpe_enable(struct prueth_emac *emac)
> +{
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	int ret;
> +
> +	/* Schedule MAC Verify and enable IET FPE if configured */
> +	atomic_set(&iet->cancel_fpe_config, 0);
> +	reinit_completion(&iet->fpe_config_compl);
> +	schedule_work(&iet->fpe_config_task);
> +	/* By trial, found it takes about 1.5s. So
> +	 * wait for 10s
> +	 */
> +	ret = wait_for_completion_timeout(&iet->fpe_config_compl,
> +					  msecs_to_jiffies(10000));

Why schedule async work (&iet->fpe_config_task) then immediately wait
for it to finish? Isn't that an extremely roundabout way of just running
the contents of the task directly?

Also, I think you are blocking the system_power_efficient_wq (which
phy_queue_state_machine() -> .. -> emac_adjust_link() runs on) for up to
10 seconds at a time, and nothing else can run while you are waiting for
the FPE verification completion.

Why exactly do you need to wait for icssg_prueth_iet_fpe_enable() to
finish here?

> +	if (!ret) {
> +		netdev_err(emac->ndev,
> +			   "IET verify completion timeout\n");
> +		/* cancel verify in progress */
> +		atomic_set(&iet->cancel_fpe_config, 1);
> +		cancel_work_sync(&iet->fpe_config_task);
> +	}
> +
> +	return ret;
> +}
> 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..3d3f42107dd7
> --- /dev/null
> +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.h
> @@ -0,0 +1,48 @@
> +/* 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>
> +
> +struct prueth_qos_mqprio {
> +	struct tc_mqprio_qopt_offload mqprio;
> +	u8 preemptible_tcs;
> +};
> +
> +struct prueth_qos_iet {
> +	struct work_struct fpe_config_task;
> +	struct completion fpe_config_compl;
> +	struct prueth_emac *emac;
> +	atomic_t cancel_fpe_config;

This variable has no reader.

> +	/* Set through priv flags to enable IET frame preemption */

These comments referencing priv flags might be obsolete. At least in the
mainline submission there is no netdev priv flag involved.

> +	bool fpe_configured;
> +	/* Set through priv flags to enable IET MAC Verify state machine
> +	 * in firmware
> +	 */
> +	bool mac_verify_configured;
> +	/* Min TX fragment size, set via ethtool */
> +	u32 tx_min_frag_size;
> +	/* wait time between verification attempts in ms (according to clause
> +	 * 30.14.1.6 aMACMergeVerifyTime), set via ethtool
> +	 */
> +	u32 verify_time_ms;
> +	/* Set if IET FPE is active */
> +	bool fpe_enabled;

"enabled" and "active" have quite distinct meanings if you read
Documentation/networking/ethtool-netlink.rst (ETHTOOL_A_MM_TX_ENABLED vs
ETHTOOL_A_MM_TX_ACTIVE) as well as the standard. The fact that this
comment explain that "fpe_enabled" tracks the "active" status makes
things clear as mud.

> +};
> +
> +struct prueth_qos {
> +	struct prueth_qos_iet iet;
> +	struct prueth_qos_mqprio mqprio;
> +};
> +
> +void icssg_qos_init(struct net_device *ndev);
> +void icssg_qos_link_up(struct net_device *ndev);
> +void icssg_qos_link_down(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 */
> -- 
> 2.43.0
>

Re: [PATCH net-next 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[Meghana Malladi]

Hi Vladimir,

On 1/12/26 23:20, Vladimir Oltean wrote:
> Hi Meghana,
> 
> On Wed, Jan 07, 2026 at 06:21:10PM +0530, Meghana Malladi wrote:
>> This patch adds utility functions to configure firmware to enable
>> IET FPE. The highest priority queue is marked as Express queue and
>> lower priority queues as pre-emptable, as the default configuration
>> which will be overwritten by the mqprio tc mask passed by tc qdisc.
>> Driver optionally allow configure the Verify state machine in the
>> firmware to check remote peer capability. If remote fails to respond
>> to Verify command, then FPE is disabled by firmware and TX FPE active
> 
> "If remote fails to respond to Verify command, then FPE is disabled by
> firmware" -> please clarify that. There is also a question on the code
> about this.

Sure, I will elaborate this more.

> 
>> status is disabled.
>>
>> This also adds the necessary hooks to enable IET/FPE feature in ICSSG
>> driver. IET/FPE gets configured when Link is up and gets disabled when link
>> goes down or device is stopped.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>> ---
>>   drivers/net/ethernet/ti/Makefile             |   2 +-
>>   drivers/net/ethernet/ti/icssg/icssg_prueth.c |   9 +
>>   drivers/net/ethernet/ti/icssg/icssg_prueth.h |   2 +
>>   drivers/net/ethernet/ti/icssg/icssg_qos.c    | 319 +++++++++++++++++++
>>   drivers/net/ethernet/ti/icssg/icssg_qos.h    |  48 +++
>>   5 files changed, 379 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/Makefile b/drivers/net/ethernet/ti/Makefile
>> index 93c0a4d0e33a..2f588663fdf0 100644
>> --- a/drivers/net/ethernet/ti/Makefile
>> +++ b/drivers/net/ethernet/ti/Makefile
>> @@ -35,7 +35,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 f65041662173..668177eba3f8 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -378,6 +378,12 @@ static void emac_adjust_link(struct net_device *ndev)
>>   		} else {
>>   			icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
>>   		}
>> +
>> +		if (emac->link) {
>> +			icssg_qos_link_up(ndev);
>> +		} else {
>> +			icssg_qos_link_down(ndev);
>> +		}
>>   	}
>>   
>>   	if (emac->link) {
>> @@ -967,6 +973,8 @@ static int emac_ndo_open(struct net_device *ndev)
>>   	if (ret)
>>   		goto destroy_rxq;
>>   
>> +	icssg_qos_init(ndev);
>> +
>>   	/* start PHY */
>>   	phy_start(ndev->phydev);
>>   
>> @@ -1421,6 +1429,7 @@ static const struct net_device_ops emac_netdev_ops = {
>>   	.ndo_hwtstamp_get = icssg_ndo_get_ts_config,
>>   	.ndo_hwtstamp_set = icssg_ndo_set_ts_config,
>>   	.ndo_xsk_wakeup = prueth_xsk_wakeup,
>> +	.ndo_setup_tc = icssg_qos_ndo_setup_tc,
>>   };
>>   
>>   static int prueth_netdev_init(struct prueth *prueth,
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> index 10eadd356650..7a586038adf8 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -44,6 +44,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)
>> @@ -255,6 +256,7 @@ struct prueth_emac {
>>   	struct bpf_prog *xdp_prog;
>>   	struct xdp_attachment_info xdpi;
>>   	int xsk_qid;
>> +	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..858268740dae
>> --- /dev/null
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.c
>> @@ -0,0 +1,319 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Texas Instruments ICSSG PRUETH QoS submodule
>> + * Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
>> + */
>> +
>> +#include "icssg_prueth.h"
>> +#include "icssg_switch_map.h"
>> +
>> +static int icssg_prueth_iet_fpe_enable(struct prueth_emac *emac);
>> +static void icssg_prueth_iet_fpe_disable(struct prueth_qos_iet *iet);
>> +static void icssg_qos_enable_ietfpe(struct work_struct *work);
> 
> Please order functions such as to avoid forward declarations.
> 

Sure, I will fix this.

>> +
>> +void icssg_qos_init(struct net_device *ndev)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth_qos_iet *iet = &emac->qos.iet;
>> +
>> +	if (!iet->fpe_configured)
>> +		return;
> 
> Bug:
> 
> if you open (ip link set ... up) the interface first, emac_ndo_open() ->
> icssg_qos_init() will run, and will find iet->fpe_configured false,
> exiting early.
> 
> Then you run ethtool --set-mm ... tx-enabled on, which sets
> iet->fpe_configured = true.
> 
> Then you insert a cable and icssg_qos_link_up() -> icssg_prueth_iet_fpe_enable()
> runs, which calls schedule_work(&iet->fpe_config_task). But you've never
> called INIT_WORK() on this structure, so the kernel will oops.
> 

Yes that's true. Thanks for catching it. I wasn't able to reproduce this 
because I always bring down my interface, call ethtool --set-mm and only 
then bring up the interface again. So during ndo_open() it always calls 
INIT_WORK(). I will fix this in v2.

>> +
>> +	/* Init work queue for IET MAC verify process */
>> +	iet->emac = emac;
>> +	INIT_WORK(&iet->fpe_config_task, icssg_qos_enable_ietfpe);
>> +	init_completion(&iet->fpe_config_compl);
>> +
>> +	/* As worker may be sleeping, check this flag to abort
>> +	 * as soon as it comes of out of sleep and cancel the
>> +	 * fpe config task.
>> +	 */
>> +	atomic_set(&iet->cancel_fpe_config, 0);
>> +}
>> +
>> +static void icssg_iet_set_preempt_mask(struct prueth_emac *emac, u8 preemptible_tcs)
>> +{
>> +	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
>> +	struct prueth_qos_mqprio *p_mqprio = &emac->qos.mqprio;
>> +	struct tc_mqprio_qopt *qopt = &p_mqprio->mqprio.qopt;
>> +	u8 tc;
>> +	int i;
>> +
>> +	/* Configure highest queue as express. Set Bit 4 for FPE,
>> +	 * Reset for express
>> +	 */
> 
> I wasn't able to parse the code below, I just got stuck on this comment.
> Why configure just the highest queue as express, rather than all queues
> as express?
> 
> What is the current default behaviour in mainline, and does this
> constitute a change of that behaviour?
> 

This code was the existing design in our TI kernel, on top of which I 
made some additional changes. Now that I think about it, this 
configuration is redundant. If user doesn't request anything all queues 
should be express, and that is the case with the all the queues 
out-of-box. So all I need to do will be to check what user is expecting 
and configure the queues accordingly.

>> +
>> +	/* first set all 8 queues as Preemptive */
>> +	for (i = 0; i < PRUETH_MAX_TX_QUEUES * PRUETH_NUM_MACS; i++)
> 
> What is the purpose of PRUETH_NUM_MACS here? Is the FPE configuration of
> PRUETH_MAC0 not independent of that of PRUETH_MAC1?

You are right. This function gets called per emac and should be 
independent of the other emacs. I will drop this as this is not required 
in the first place.

> 
>> +		writeb(BIT(4), config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
>> +
>> +	/* set highest priority channel queue as express as default configuration */
>> +	writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + emac->tx_ch_num - 1);
>> +
>> +	/* set up queue mask for FPE. 1 means express */
>> +	writeb(BIT(emac->tx_ch_num - 1), config + EXPRESS_PRE_EMPTIVE_Q_MASK);
>> +
>> +	/* Overwrite the express queue mapping based on the tc map set by the user */
>> +	for (tc = 0; tc < p_mqprio->mqprio.qopt.num_tc; tc++) {
>> +		/* check if the tc is express or not */
>> +		if (!(p_mqprio->preemptible_tcs & BIT(tc))) {
>> +			for (i = qopt->offset[tc]; i < qopt->offset[tc] + qopt->count[tc]; i++) {
>> +				/* Set all the queues in this tc as express queues */
>> +				writeb(0, config + EXPRESS_PRE_EMPTIVE_Q_MAP + i);
>> +				writeb(BIT(i), config + EXPRESS_PRE_EMPTIVE_Q_MASK);
>> +			}
>> +		}
>> +		netdev_set_tc_queue(emac->ndev, tc, qopt->count[tc], qopt->offset[tc]);
>> +	}
>> +}
>> +
>> +static int prueth_mqprio_validate(struct net_device *ndev,
>> +				  struct tc_mqprio_qopt_offload *mqprio)
>> +{
>> +	int num_tc = mqprio->qopt.num_tc;
>> +	int queue_count = 0;
>> +	int i;
>> +
>> +	/* Always start tc-queue mapping from queue 0 */
>> +	if (mqprio->qopt.offset[0] != 0)
>> +		return -EINVAL;
>> +
>> +	/* Check for valid number of traffic classes */
>> +	if (num_tc < 1 || num_tc > PRUETH_MAX_TX_QUEUES)
>> +		return -EINVAL;
>> +
>> +	/* Only channel mode is supported */
>> +	if (mqprio->mode != TC_MQPRIO_MODE_CHANNEL) {
>> +		netdev_err(ndev, "Unsupported mode: %d\n", mqprio->mode);
> 
> struct tc_mqprio_qopt_offload has an extack argument. Please use
> NL_SET_EXT_MSG_MOD(extack) instead of netdev_err() here and below.
> 

Sure, will update it as part of v2.

>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < num_tc; i++) {
>> +		if (!mqprio->qopt.count[i]) {
>> +			netdev_err(ndev, "TC %d has zero size queue count: %d\n",
>> +				   i, mqprio->qopt.count[i]);
>> +			return -EINVAL;
>> +		}
> 
> You set caps->validate_queue_counts = true, which is laudable. But did
> you also look at what mqprio_validate_queue_counts() does? Do you need
> to duplicate it here?

Honestly, I simply set this to true looking at other drivers, with no 
background on what this does, which I shouldn't have done :)
But now that I looked into mqprio_validate_queue_counts(), and I will 
remove the code inside prueth_mqprio_validate() whatever is getting 
duplicated.

> 
>> +		if (mqprio->min_rate[i] || mqprio->max_rate[i]) {
>> +			netdev_err(ndev, "Min/Max tx rate is not supported\n");
>> +			return -EINVAL;
>> +		}
>> +		if (mqprio->qopt.offset[i] != queue_count) {
>> +			netdev_err(ndev, "Discontinuous queues config is not supported\n");
>> +			return -EINVAL;
>> +		}
>> +		queue_count += mqprio->qopt.count[i];
>> +	}
>> +
>> +	if (queue_count > PRUETH_MAX_TX_QUEUES) {
>> +		netdev_err(ndev, "Total queues %d exceed max %d\n",
>> +			   queue_count, PRUETH_MAX_TX_QUEUES);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +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_MQPRIO: {
>> +		struct tc_mqprio_caps *caps = base->caps;
>> +
>> +		caps->validate_queue_counts = true;
>> +		return 0;
>> +	}
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int emac_tc_setup_mqprio(struct net_device *ndev, void *type_data)
>> +{
>> +	struct tc_mqprio_qopt_offload *mqprio = type_data;
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth_qos_mqprio *p_mqprio;
>> +	int ret;
>> +
>> +	if (mqprio->qopt.hw == TC_MQPRIO_HW_OFFLOAD_TCS)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (!mqprio->qopt.num_tc) {
>> +		netdev_reset_tc(ndev);
>> +		p_mqprio->preemptible_tcs = 0;
>> +		return 0;
>> +	}
>> +
>> +	ret = prueth_mqprio_validate(ndev, mqprio);
>> +	if (ret)
>> +		return ret;
>> +
>> +	p_mqprio = &emac->qos.mqprio;
>> +	memcpy(&p_mqprio->mqprio, mqprio, sizeof(*mqprio));
>> +	netdev_set_num_tc(ndev, mqprio->qopt.num_tc);
>> +
>> +	return 0;
>> +}
>> +
>> +int icssg_qos_ndo_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>> +			   void *type_data)
>> +{
>> +	switch (type) {
>> +	case TC_QUERY_CAPS:
>> +		return emac_tc_query_caps(ndev, type_data);
>> +	case TC_SETUP_QDISC_MQPRIO:
>> +		return emac_tc_setup_mqprio(ndev, type_data);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(icssg_qos_ndo_setup_tc);
>> +
>> +void icssg_qos_link_up(struct net_device *ndev)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth_qos_iet *iet = &emac->qos.iet;
>> +
>> +	if (!iet->fpe_configured)
>> +		return;
>> +
>> +	icssg_prueth_iet_fpe_enable(emac);
>> +}
>> +
>> +void icssg_qos_link_down(struct net_device *ndev)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth_qos_iet *iet = &emac->qos.iet;
>> +
>> +	if (iet->fpe_configured)
>> +		icssg_prueth_iet_fpe_disable(iet);
>> +}
>> +
>> +static int icssg_config_ietfpe(struct prueth_qos_iet *iet, bool enable)
>> +{
>> +	void __iomem *config = iet->emac->dram.va + ICSSG_CONFIG_OFFSET;
>> +	struct prueth_qos_mqprio *p_mqprio =  &iet->emac->qos.mqprio;
>> +	int ret;
>> +	u8 val;
>> +
>> +	/* If FPE is to be enabled, first configure MAC Verify state
>> +	 * machine in firmware as firmware kicks the Verify process
>> +	 * as soon as ICSSG_EMAC_PORT_PREMPT_TX_ENABLE command is
>> +	 * received.
>> +	 */
>> +	if (enable && iet->mac_verify_configured) {
>> +		writeb(1, config + PRE_EMPTION_ENABLE_VERIFY);
> 
> When mac_verify_configured transitions from true to false, who disables
> the feature in firmware? Or do you have to reboot the board?
> 

The driver doesn't need to explicitly disable PRE_EMPTION_ENABLE_VERIFY. 
The firmware handles the cleanup after successful mac verification.

>> +		writew(iet->tx_min_frag_size, config + PRE_EMPTION_ADD_FRAG_SIZE_LOCAL);
>> +		writel(iet->verify_time_ms, config + PRE_EMPTION_VERIFY_TIME);
>> +	}
>> +
>> +	/* Send command to enable FPE Tx side. Rx is always enabled */
>> +	ret = icssg_set_port_state(iet->emac,
>> +				   enable ? ICSSG_EMAC_PORT_PREMPT_TX_ENABLE :
>> +					    ICSSG_EMAC_PORT_PREMPT_TX_DISABLE);
>> +	if (ret) {
>> +		netdev_err(iet->emac->ndev, "TX preempt %s command failed\n",
>> +			   str_enable_disable(enable));
>> +		writeb(0, config + PRE_EMPTION_ENABLE_VERIFY);
>> +		return ret;
>> +	}
>> +
>> +	/* Update FPE Tx enable bit. Assume firmware use this bit
>> +	 * and enable PRE_EMPTION_ACTIVE_TX if everything looks
>> +	 * good at firmware
>> +	 */
>> +	writeb(enable ? 1 : 0, config + PRE_EMPTION_ENABLE_TX);
>> +
>> +	if (enable && iet->mac_verify_configured) {
>> +		ret = readb_poll_timeout(config + PRE_EMPTION_VERIFY_STATUS, val,
>> +					 (val == ICSSG_IETFPE_STATE_SUCCEEDED),
>> +					 USEC_PER_MSEC, 5 * USEC_PER_SEC);
>> +		if (ret) {
>> +			netdev_err(iet->emac->ndev,
>> +				   "timeout for MAC Verify: status %x\n",
>> +				   val);
>> +			return ret;
>> +		}
>> +	} else {
>> +		/* Give f/w some time to update PRE_EMPTION_ACTIVE_TX state */
>> +		usleep_range(100, 200);
>> +	}
>> +
>> +	if (enable) {
>> +		val = readb(config + PRE_EMPTION_ACTIVE_TX);
>> +		if (val != 1) {
>> +			netdev_err(iet->emac->ndev,
>> +				   "F/w fails to activate IET/FPE\n");
>> +			writeb(0, config + PRE_EMPTION_ENABLE_TX);
> 
> Why do you write 0 to PRE_EMPTION_ENABLE_TX here? You previously said
> that "FPE is disabled by firmware" which I suppose translates into the
> same (incorrect) thing? Is it disabled by firmware, or by the driver, or
> by both?
> 

Yes FPE is disabled by firmware if MAC verify fails. But let's say 
verify is successful but firmware couldn't transition ACTIVE to true by 
the time we are reading PRE_EMPTION_ACTIVE_TX, we will assume firmware 
failed to activate FPE, but firmware might be taking time and eventually 
activate FPE. But since timeout happened w.r.t driver we manually 
disable FPE from the driver so that driver and firmware are on the same 
page.

> When FPE is enabled with verification, but the link partner does not
> respond, the ENABLED status remains, but the ACTIVE status never
> transitions to true. It is documented that preemptible traffic should
> only be sent based on the ACTIVE status, not the ENABLED one. So, I
> don't think this is necessary at all.
> 
>> +			return -ENODEV;
>> +		}
>> +	} else {
>> +		return 0;
>> +	}
>> +
>> +	icssg_iet_set_preempt_mask(iet->emac, p_mqprio->preemptible_tcs);
>> +
>> +	iet->fpe_enabled = true;
> 
> iet->fpe_enabled is set to true here and never to false. It tracks no
> useful information, other than "was icssg_config_ietfpe() ever called
> for this interface with enable=true?". You assign that result to
> state->tx_enabled in emac_get_mm(), which is obviously wrong because
> this variable does not track that information.  For example, if you call
> icssg_config_ietfpe() with enable=false, FPE should be enabled, and
> emac_get_mm() should reflect that.
> 

Yeah I get your point, if fpe ever gets disabled no one tracking that 
ans returning incorrect status. Will fix it, thanks. I think there 
should be two flags fpe_enabled (which tracks if user has requested to 
enable/disable fpe) and fpe_active (which tracks if fpe is enabled by 
the driver and is operational)

>> +
>> +	return ret;
>> +}
>> +
>> +static void icssg_qos_enable_ietfpe(struct work_struct *work)
>> +{
>> +	struct prueth_qos_iet *iet =
>> +		container_of(work, struct prueth_qos_iet, fpe_config_task);
>> +	int ret;
>> +
>> +	/* Set the required flag and send a command to ICSSG firmware to
>> +	 * enable FPE and start MAC verify
>> +	 */
>> +	ret = icssg_config_ietfpe(iet, true);
>> +
>> +	/* if verify configured, poll for the status and complete.
>> +	 * Or just do completion
>> +	 */
>> +	if (!ret)
>> +		netdev_err(iet->emac->ndev, "IET FPE configured successfully\n");
>> +	else
>> +		netdev_err(iet->emac->ndev, "IET FPE config error\n");
>> +	complete(&iet->fpe_config_compl);
>> +}
>> +
>> +static void icssg_prueth_iet_fpe_disable(struct prueth_qos_iet *iet)
>> +{
>> +	int ret;
>> +
>> +	atomic_set(&iet->cancel_fpe_config, 1);
>> +	cancel_work_sync(&iet->fpe_config_task);
>> +	ret = icssg_config_ietfpe(iet, false);
>> +	if (!ret)
>> +		netdev_err(iet->emac->ndev, "IET FPE disabled successfully\n");
>> +	else
>> +		netdev_err(iet->emac->ndev, "IET FPE disable failed\n");
>> +}
>> +
>> +static int icssg_prueth_iet_fpe_enable(struct prueth_emac *emac)
>> +{
>> +	struct prueth_qos_iet *iet = &emac->qos.iet;
>> +	int ret;
>> +
>> +	/* Schedule MAC Verify and enable IET FPE if configured */
>> +	atomic_set(&iet->cancel_fpe_config, 0);
>> +	reinit_completion(&iet->fpe_config_compl);
>> +	schedule_work(&iet->fpe_config_task);
>> +	/* By trial, found it takes about 1.5s. So
>> +	 * wait for 10s
>> +	 */
>> +	ret = wait_for_completion_timeout(&iet->fpe_config_compl,
>> +					  msecs_to_jiffies(10000));
> 
> Why schedule async work (&iet->fpe_config_task) then immediately wait
> for it to finish? Isn't that an extremely roundabout way of just running
> the contents of the task directly?
> 
> Also, I think you are blocking the system_power_efficient_wq (which
> phy_queue_state_machine() -> .. -> emac_adjust_link() runs on) for up to
> 10 seconds at a time, and nothing else can run while you are waiting for
> the FPE verification completion.
> 
> Why exactly do you need to wait for icssg_prueth_iet_fpe_enable() to
> finish here?
> 

Ok, I wasn't aware that I am scheduling work onto 
system_power_efficient_wq (he same WQ used by phy_queue_state_machine -> 
emac_adjust_link), indirectly blocking other works queued in that WQ for 
10 secs, which I believe is a bad design.

I was waiting inside icssg_prueth_iet_fpe_enable() because whatever FPE 
configuration has been requested by the driver needs to be serviced by 
the firmware and will take sometime assuming firmware is alive and is 
able to take the requests from the driver.

I think better design would be to make it truly asynchronous (simply 
schedule the work, not wait and return immediately), since the caller 
doesn't strictly need the operation to finish.

>> +	if (!ret) {
>> +		netdev_err(emac->ndev,
>> +			   "IET verify completion timeout\n");
>> +		/* cancel verify in progress */
>> +		atomic_set(&iet->cancel_fpe_config, 1);
>> +		cancel_work_sync(&iet->fpe_config_task);
>> +	}
>> +
>> +	return ret;
>> +}
>> 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..3d3f42107dd7
>> --- /dev/null
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.h
>> @@ -0,0 +1,48 @@
>> +/* 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>
>> +
>> +struct prueth_qos_mqprio {
>> +	struct tc_mqprio_qopt_offload mqprio;
>> +	u8 preemptible_tcs;
>> +};
>> +
>> +struct prueth_qos_iet {
>> +	struct work_struct fpe_config_task;
>> +	struct completion fpe_config_compl;
>> +	struct prueth_emac *emac;
>> +	atomic_t cancel_fpe_config;
> 
> This variable has no reader.
> 

Yes, will fix it.

>> +	/* Set through priv flags to enable IET frame preemption */
> 
> These comments referencing priv flags might be obsolete. At least in the
> mainline submission there is no netdev priv flag involved.
> 

Ok, I will drop mentioning priv flags in the comments.

>> +	bool fpe_configured;
>> +	/* Set through priv flags to enable IET MAC Verify state machine
>> +	 * in firmware
>> +	 */
>> +	bool mac_verify_configured;
>> +	/* Min TX fragment size, set via ethtool */
>> +	u32 tx_min_frag_size;
>> +	/* wait time between verification attempts in ms (according to clause
>> +	 * 30.14.1.6 aMACMergeVerifyTime), set via ethtool
>> +	 */
>> +	u32 verify_time_ms;
>> +	/* Set if IET FPE is active */
>> +	bool fpe_enabled;
> 
> "enabled" and "active" have quite distinct meanings if you read
> Documentation/networking/ethtool-netlink.rst (ETHTOOL_A_MM_TX_ENABLED vs
> ETHTOOL_A_MM_TX_ACTIVE) as well as the standard. The fact that this
> comment explain that "fpe_enabled" tracks the "active" status makes
> things clear as mud.

Makes sense. I will update fpe_configured to fpe_enabled (IET is 
enabled, which can be inactive if verification failed) and fpe_enabled 
to fpe_active (IET is operationally enabled i.e., verification is either 
successful or disabled).

> 
>> +};
>> +
>> +struct prueth_qos {
>> +	struct prueth_qos_iet iet;
>> +	struct prueth_qos_mqprio mqprio;
>> +};
>> +
>> +void icssg_qos_init(struct net_device *ndev);
>> +void icssg_qos_link_up(struct net_device *ndev);
>> +void icssg_qos_link_down(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 */
>> -- 
>> 2.43.0
>>

Re: [PATCH net-next 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[Simon Horman]

On Wed, Jan 07, 2026 at 06:21:10PM +0530, Meghana Malladi wrote:
> This patch adds utility functions to configure firmware to enable
> IET FPE. The highest priority queue is marked as Express queue and
> lower priority queues as pre-emptable, as the default configuration
> which will be overwritten by the mqprio tc mask passed by tc qdisc.
> Driver optionally allow configure the Verify state machine in the
> firmware to check remote peer capability. If remote fails to respond
> to Verify command, then FPE is disabled by firmware and TX FPE active
> status is disabled.
> 
> This also adds the necessary hooks to enable IET/FPE feature in ICSSG
> driver. IET/FPE gets configured when Link is up and gets disabled when link
> goes down or device is stopped.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>

...

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

...

> +static int emac_tc_setup_mqprio(struct net_device *ndev, void *type_data)
> +{
> +	struct tc_mqprio_qopt_offload *mqprio = type_data;
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_mqprio *p_mqprio;
> +	int ret;
> +
> +	if (mqprio->qopt.hw == TC_MQPRIO_HW_OFFLOAD_TCS)
> +		return -EOPNOTSUPP;
> +
> +	if (!mqprio->qopt.num_tc) {
> +		netdev_reset_tc(ndev);
> +		p_mqprio->preemptible_tcs = 0;

Hi MD & Meghana,

p_mqprio is dereferenced here.
But it isn't initialised yet.

Flagged by Clang 21.1.7 W=1 build for arm64.

> +		return 0;
> +	}
> +
> +	ret = prueth_mqprio_validate(ndev, mqprio);
> +	if (ret)
> +		return ret;
> +
> +	p_mqprio = &emac->qos.mqprio;
> +	memcpy(&p_mqprio->mqprio, mqprio, sizeof(*mqprio));
> +	netdev_set_num_tc(ndev, mqprio->qopt.num_tc);
> +
> +	return 0;
> +}

...

Re: [PATCH net-next 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[Meghana Malladi]

Hi Simon,

On 1/15/26 20:27, Simon Horman wrote:
> On Wed, Jan 07, 2026 at 06:21:10PM +0530, Meghana Malladi wrote:
>> This patch adds utility functions to configure firmware to enable
>> IET FPE. The highest priority queue is marked as Express queue and
>> lower priority queues as pre-emptable, as the default configuration
>> which will be overwritten by the mqprio tc mask passed by tc qdisc.
>> Driver optionally allow configure the Verify state machine in the
>> firmware to check remote peer capability. If remote fails to respond
>> to Verify command, then FPE is disabled by firmware and TX FPE active
>> status is disabled.
>>
>> This also adds the necessary hooks to enable IET/FPE feature in ICSSG
>> driver. IET/FPE gets configured when Link is up and gets disabled when link
>> goes down or device is stopped.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> 
> ...
> 
>>   /* 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
> 
> ...
> 
>> +static int emac_tc_setup_mqprio(struct net_device *ndev, void *type_data)
>> +{
>> +	struct tc_mqprio_qopt_offload *mqprio = type_data;
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth_qos_mqprio *p_mqprio;
>> +	int ret;
>> +
>> +	if (mqprio->qopt.hw == TC_MQPRIO_HW_OFFLOAD_TCS)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (!mqprio->qopt.num_tc) {
>> +		netdev_reset_tc(ndev);
>> +		p_mqprio->preemptible_tcs = 0;
> 
> Hi MD & Meghana,
> 
> p_mqprio is dereferenced here.
> But it isn't initialised yet.
> 
> Flagged by Clang 21.1.7 W=1 build for arm64.
> 

Thanks for catching this.
Will include W=1 build test in my checklist before posting in upstream 
from next time :)

>> +		return 0;
>> +	}
>> +
>> +	ret = prueth_mqprio_validate(ndev, mqprio);
>> +	if (ret)
>> +		return ret;
>> +
>> +	p_mqprio = &emac->qos.mqprio;
>> +	memcpy(&p_mqprio->mqprio, mqprio, sizeof(*mqprio));
>> +	netdev_set_num_tc(ndev, mqprio->qopt.num_tc);
>> +
>> +	return 0;
>> +}
> 
> ...

[PATCH net-next 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

[Meghana Malladi]

Add ethtool ops .get_mm, .set_mm and .get_mm_stats to enable / disable
Mac merge frame preemption and dump Preemption related statistics for
ICSSG driver. Add pa stats registers for mac merge related statistics,
which can be dumped using the ethtool ops.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 58 +++++++++++++++++++
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  2 +-
 drivers/net/ethernet/ti/icssg/icssg_stats.h   |  5 ++
 .../net/ethernet/ti/icssg/icssg_switch_map.h  |  5 ++
 4 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
index b715af21d23a..ceca6d6ec0f4 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
@@ -294,6 +294,61 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
 	return 0;
 }
 
+static int emac_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+	void __iomem *config;
+
+	config = emac->dram.va + ICSSG_CONFIG_OFFSET;
+
+	state->tx_enabled = iet->fpe_enabled;
+	state->pmac_enabled = true;
+	state->verify_status = readb(config + PRE_EMPTION_VERIFY_STATUS);
+	state->tx_min_frag_size = iet->tx_min_frag_size;
+	state->rx_min_frag_size = 124;
+	state->tx_active = readb(config + PRE_EMPTION_ACTIVE_TX) ? true : false;
+	state->verify_enabled = readb(config + PRE_EMPTION_ENABLE_VERIFY) ? true : false;
+	state->verify_time = iet->verify_time_ms;
+
+	/* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
+	 * variable has a range between 1 and 128 ms inclusive. Limit to that.
+	 */
+	state->max_verify_time = 128;
+
+	return 0;
+}
+
+static int emac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
+		       struct netlink_ext_ack *extack)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth_qos_iet *iet = &emac->qos.iet;
+
+	if (!cfg->pmac_enabled)
+		netdev_err(ndev, "preemptible MAC is always enabled");
+
+	iet->verify_time_ms = cfg->verify_time;
+	iet->tx_min_frag_size = cfg->tx_min_frag_size;
+
+	iet->fpe_configured = cfg->tx_enabled;
+	iet->mac_verify_configured = cfg->verify_enabled;
+
+	return 0;
+}
+
+static void emac_get_mm_stats(struct net_device *ndev,
+			      struct ethtool_mm_stats *s)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+
+	s->MACMergeFrameAssOkCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_OK");
+	s->MACMergeFrameAssErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_ERR");
+	s->MACMergeFragCountRx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_RX");
+	s->MACMergeFragCountTx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_TX");
+	s->MACMergeFrameSmdErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_BAD_FRAG");
+}
+
 const struct ethtool_ops icssg_ethtool_ops = {
 	.get_drvinfo = emac_get_drvinfo,
 	.get_msglevel = emac_get_msglevel,
@@ -317,5 +372,8 @@ const struct ethtool_ops icssg_ethtool_ops = {
 	.set_eee = emac_set_eee,
 	.nway_reset = emac_nway_reset,
 	.get_rmon_stats = emac_get_rmon_stats,
+	.get_mm = emac_get_mm,
+	.set_mm = emac_set_mm,
+	.get_mm_stats = emac_get_mm_stats,
 };
 EXPORT_SYMBOL_GPL(icssg_ethtool_ops);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 7a586038adf8..9c31574cc7f6 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -58,7 +58,7 @@
 
 #define ICSSG_MAX_RFLOWS	8	/* per slice */
 
-#define ICSSG_NUM_PA_STATS	32
+#define ICSSG_NUM_PA_STATS	37
 #define ICSSG_NUM_MIIG_STATS	60
 /* Number of ICSSG related stats */
 #define ICSSG_NUM_STATS (ICSSG_NUM_MIIG_STATS + ICSSG_NUM_PA_STATS)
diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.h b/drivers/net/ethernet/ti/icssg/icssg_stats.h
index 5ec0b38e0c67..f35ae1b4f846 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_stats.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_stats.h
@@ -189,6 +189,11 @@ static const struct icssg_pa_stats icssg_all_pa_stats[] = {
 	ICSSG_PA_STATS(FW_INF_DROP_PRIOTAGGED),
 	ICSSG_PA_STATS(FW_INF_DROP_NOTAG),
 	ICSSG_PA_STATS(FW_INF_DROP_NOTMEMBER),
+	ICSSG_PA_STATS(FW_PREEMPT_BAD_FRAG),
+	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_ERR),
+	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_TX),
+	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_OK),
+	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_RX),
 	ICSSG_PA_STATS(FW_RX_EOF_SHORT_FRMERR),
 	ICSSG_PA_STATS(FW_RX_B0_DROP_EARLY_EOF),
 	ICSSG_PA_STATS(FW_TX_JUMBO_FRM_CUTOFF),
diff --git a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
index 7e053b8af3ec..855fd4ed0b3f 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
@@ -256,6 +256,11 @@
 #define FW_INF_DROP_PRIOTAGGED		0x0148
 #define FW_INF_DROP_NOTAG		0x0150
 #define FW_INF_DROP_NOTMEMBER		0x0158
+#define FW_PREEMPT_BAD_FRAG		0x0160
+#define FW_PREEMPT_ASSEMBLY_ERR		0x0168
+#define FW_PREEMPT_FRAG_CNT_TX		0x0170
+#define FW_PREEMPT_ASSEMBLY_OK		0x0178
+#define FW_PREEMPT_FRAG_CNT_RX		0x0180
 #define FW_RX_EOF_SHORT_FRMERR		0x0188
 #define FW_RX_B0_DROP_EARLY_EOF		0x0190
 #define FW_TX_JUMBO_FRM_CUTOFF		0x0198
-- 
2.43.0

Re: [PATCH net-next 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

[Vladimir Oltean]

Hi Meghana,

On Wed, Jan 07, 2026 at 06:21:11PM +0530, Meghana Malladi wrote:
> Add ethtool ops .get_mm, .set_mm and .get_mm_stats to enable / disable
> Mac merge frame preemption and dump Preemption related statistics for
> ICSSG driver. Add pa stats registers for mac merge related statistics,
> which can be dumped using the ethtool ops.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 58 +++++++++++++++++++
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  2 +-
>  drivers/net/ethernet/ti/icssg/icssg_stats.h   |  5 ++
>  .../net/ethernet/ti/icssg/icssg_switch_map.h  |  5 ++
>  4 files changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> index b715af21d23a..ceca6d6ec0f4 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> @@ -294,6 +294,61 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
>  	return 0;
>  }
>  
> +static int emac_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	void __iomem *config;
> +
> +	config = emac->dram.va + ICSSG_CONFIG_OFFSET;
> +
> +	state->tx_enabled = iet->fpe_enabled;

I would expect state->tx_enabled to be returned from
iet->fpe_configured, aka from the same variable in which tx_enabled was
saved in emac_set_mm(). In case it's not clear, ethtool saves state in
the device driver and expects that state to be later returned in the
get() callback.

> +	state->pmac_enabled = true;
> +	state->verify_status = readb(config + PRE_EMPTION_VERIFY_STATUS);
> +	state->tx_min_frag_size = iet->tx_min_frag_size;

What is the range of acceptable values for PRE_EMPTION_ADD_FRAG_SIZE_LOCAL?
Is it safe to accept this value with no sanitization, just the ethtool
netlink policy which limits it between 60 and 252? Even if that is the
case, please add a comment specifying what the valid range is.

> +	state->rx_min_frag_size = 124;

Please add a comment justifying this non-standard value.

> +	state->tx_active = readb(config + PRE_EMPTION_ACTIVE_TX) ? true : false;
> +	state->verify_enabled = readb(config + PRE_EMPTION_ENABLE_VERIFY) ? true : false;
> +	state->verify_time = iet->verify_time_ms;

Why are some values returned from firmware and others from driver memory?

> +
> +	/* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
> +	 * variable has a range between 1 and 128 ms inclusive. Limit to that.
> +	 */
> +	state->max_verify_time = 128;

ETHTOOL_MM_MAX_VERIFY_TIME_MS

> +
> +	return 0;
> +}
> +
> +static int emac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
> +		       struct netlink_ext_ack *extack)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +
> +	if (!cfg->pmac_enabled)
> +		netdev_err(ndev, "preemptible MAC is always enabled");

missing \n, OR use NL_SET_ERR_MSG_MOD(extack) which doesn't need \n.
Doing the latter is preferable, because the driver still accepts the
command while not modifying its internal state, and ethtool prints the
extack as warning if the return code was 0.
The catch is that openlldp sets pmac_enabled=false on exit, and that
would otherwise generate a noisy netdev_err() in your proposal (but
would be silent with the extack):
https://github.com/intel/openlldp/blob/master/lldp_8023.c#L443-L444

> +
> +	iet->verify_time_ms = cfg->verify_time;
> +	iet->tx_min_frag_size = cfg->tx_min_frag_size;
> +
> +	iet->fpe_configured = cfg->tx_enabled;
> +	iet->mac_verify_configured = cfg->verify_enabled;

Changes to the verification parameters should retrigger the verification
state machine, even if the link did not flap. Also, changes to the
ENABLED state should similarly be applied right away.

> +
> +	return 0;
> +}
> +
> +static void emac_get_mm_stats(struct net_device *ndev,
> +			      struct ethtool_mm_stats *s)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +
> +	s->MACMergeFrameAssOkCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_OK");
> +	s->MACMergeFrameAssErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_ERR");
> +	s->MACMergeFragCountRx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_RX");
> +	s->MACMergeFragCountTx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_TX");
> +	s->MACMergeFrameSmdErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_BAD_FRAG");
> +}
> +
>  const struct ethtool_ops icssg_ethtool_ops = {
>  	.get_drvinfo = emac_get_drvinfo,
>  	.get_msglevel = emac_get_msglevel,
> @@ -317,5 +372,8 @@ const struct ethtool_ops icssg_ethtool_ops = {
>  	.set_eee = emac_set_eee,
>  	.nway_reset = emac_nway_reset,
>  	.get_rmon_stats = emac_get_rmon_stats,
> +	.get_mm = emac_get_mm,
> +	.set_mm = emac_set_mm,
> +	.get_mm_stats = emac_get_mm_stats,
>  };
>  EXPORT_SYMBOL_GPL(icssg_ethtool_ops);
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index 7a586038adf8..9c31574cc7f6 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -58,7 +58,7 @@
>  
>  #define ICSSG_MAX_RFLOWS	8	/* per slice */
>  
> -#define ICSSG_NUM_PA_STATS	32
> +#define ICSSG_NUM_PA_STATS	37

Can't this be expressed as ARRAY_SIZE(icssg_all_pa_stats)? It is very
fragile to have to count and update this manually.

>  #define ICSSG_NUM_MIIG_STATS	60
>  /* Number of ICSSG related stats */
>  #define ICSSG_NUM_STATS (ICSSG_NUM_MIIG_STATS + ICSSG_NUM_PA_STATS)
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.h b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> index 5ec0b38e0c67..f35ae1b4f846 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> @@ -189,6 +189,11 @@ static const struct icssg_pa_stats icssg_all_pa_stats[] = {
>  	ICSSG_PA_STATS(FW_INF_DROP_PRIOTAGGED),
>  	ICSSG_PA_STATS(FW_INF_DROP_NOTAG),
>  	ICSSG_PA_STATS(FW_INF_DROP_NOTMEMBER),
> +	ICSSG_PA_STATS(FW_PREEMPT_BAD_FRAG),
> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_ERR),
> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_TX),
> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_OK),
> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_RX),
>  	ICSSG_PA_STATS(FW_RX_EOF_SHORT_FRMERR),
>  	ICSSG_PA_STATS(FW_RX_B0_DROP_EARLY_EOF),
>  	ICSSG_PA_STATS(FW_TX_JUMBO_FRM_CUTOFF),
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
> index 7e053b8af3ec..855fd4ed0b3f 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
> @@ -256,6 +256,11 @@
>  #define FW_INF_DROP_PRIOTAGGED		0x0148
>  #define FW_INF_DROP_NOTAG		0x0150
>  #define FW_INF_DROP_NOTMEMBER		0x0158
> +#define FW_PREEMPT_BAD_FRAG		0x0160
> +#define FW_PREEMPT_ASSEMBLY_ERR		0x0168
> +#define FW_PREEMPT_FRAG_CNT_TX		0x0170
> +#define FW_PREEMPT_ASSEMBLY_OK		0x0178
> +#define FW_PREEMPT_FRAG_CNT_RX		0x0180
>  #define FW_RX_EOF_SHORT_FRMERR		0x0188
>  #define FW_RX_B0_DROP_EARLY_EOF		0x0190
>  #define FW_TX_JUMBO_FRM_CUTOFF		0x0198
> -- 
> 2.43.0
>

Re: [PATCH net-next 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

[Malladi, Meghana]

Hi Vladimir,

On 1/12/2026 11:44 PM, Vladimir Oltean wrote:
> Hi Meghana,
> 
> On Wed, Jan 07, 2026 at 06:21:11PM +0530, Meghana Malladi wrote:
>> Add ethtool ops .get_mm, .set_mm and .get_mm_stats to enable / disable
>> Mac merge frame preemption and dump Preemption related statistics for
>> ICSSG driver. Add pa stats registers for mac merge related statistics,
>> which can be dumped using the ethtool ops.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>> ---
>>   drivers/net/ethernet/ti/icssg/icssg_ethtool.c | 58 +++++++++++++++++++
>>   drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  2 +-
>>   drivers/net/ethernet/ti/icssg/icssg_stats.h   |  5 ++
>>   .../net/ethernet/ti/icssg/icssg_switch_map.h  |  5 ++
>>   4 files changed, 69 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
>> index b715af21d23a..ceca6d6ec0f4 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
>> @@ -294,6 +294,61 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
>>   	return 0;
>>   }
>>   
>> +static int emac_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth_qos_iet *iet = &emac->qos.iet;
>> +	void __iomem *config;
>> +
>> +	config = emac->dram.va + ICSSG_CONFIG_OFFSET;
>> +
>> +	state->tx_enabled = iet->fpe_enabled;
> 
> I would expect state->tx_enabled to be returned from
> iet->fpe_configured, aka from the same variable in which tx_enabled was
> saved in emac_set_mm(). In case it's not clear, ethtool saves state in
> the device driver and expects that state to be later returned in the
> get() callback.

Ok got it, will fix it in v2. I am aware that ethtool saves state in the 
device driver which will be returned in the get() callback but didn't 
know that should be the case every time.

> 
>> +	state->pmac_enabled = true;
>> +	state->verify_status = readb(config + PRE_EMPTION_VERIFY_STATUS);
>> +	state->tx_min_frag_size = iet->tx_min_frag_size;
> 
> What is the range of acceptable values for PRE_EMPTION_ADD_FRAG_SIZE_LOCAL?
> Is it safe to accept this value with no sanitization, just the ethtool
> netlink policy which limits it between 60 and 252? Even if that is the
> case, please add a comment specifying what the valid range is.
> 

Sure, I will check and update in the comments.

>> +	state->rx_min_frag_size = 124;
> 
> Please add a comment justifying this non-standard value.
> 
>> +	state->tx_active = readb(config + PRE_EMPTION_ACTIVE_TX) ? true : false;
>> +	state->verify_enabled = readb(config + PRE_EMPTION_ENABLE_VERIFY) ? true : false;
>> +	state->verify_time = iet->verify_time_ms;
> 
> Why are some values returned from firmware and others from driver memory?

Sure, I will store all the values in the driver memory and return from 
them. But should that be the case all the time ?

> 
>> +
>> +	/* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
>> +	 * variable has a range between 1 and 128 ms inclusive. Limit to that.
>> +	 */
>> +	state->max_verify_time = 128;
> 
> ETHTOOL_MM_MAX_VERIFY_TIME_MS

Ok, will update it.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int emac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
>> +		       struct netlink_ext_ack *extack)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth_qos_iet *iet = &emac->qos.iet;
>> +
>> +	if (!cfg->pmac_enabled)
>> +		netdev_err(ndev, "preemptible MAC is always enabled");
> 
> missing \n, OR use NL_SET_ERR_MSG_MOD(extack) which doesn't need \n.
> Doing the latter is preferable, because the driver still accepts the
> command while not modifying its internal state, and ethtool prints the
> extack as warning if the return code was 0.
> The catch is that openlldp sets pmac_enabled=false on exit, and that
> would otherwise generate a noisy netdev_err() in your proposal (but
> would be silent with the extack):
> https://github.com/intel/openlldp/blob/master/lldp_8023.c#L443-L444
> 

Ok makes sense, thanks for pointing this out.

>> +
>> +	iet->verify_time_ms = cfg->verify_time;
>> +	iet->tx_min_frag_size = cfg->tx_min_frag_size;
>> +
>> +	iet->fpe_configured = cfg->tx_enabled;
>> +	iet->mac_verify_configured = cfg->verify_enabled;
> 
> Changes to the verification parameters should retrigger the verification
> state machine, even if the link did not flap. Also, changes to the
> ENABLED state should similarly be applied right away.

.set_mm() will return -EBUSY if the interfaces are up. So changes in 
verification parameters or ENABLED state can happen only when the 
interfaces are down. And when the interface goes up the IET task get 
scheduled re-triggering everything based on updated configurations.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static void emac_get_mm_stats(struct net_device *ndev,
>> +			      struct ethtool_mm_stats *s)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +
>> +	s->MACMergeFrameAssOkCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_OK");
>> +	s->MACMergeFrameAssErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_ERR");
>> +	s->MACMergeFragCountRx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_RX");
>> +	s->MACMergeFragCountTx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_TX");
>> +	s->MACMergeFrameSmdErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_BAD_FRAG");
>> +}
>> +
>>   const struct ethtool_ops icssg_ethtool_ops = {
>>   	.get_drvinfo = emac_get_drvinfo,
>>   	.get_msglevel = emac_get_msglevel,
>> @@ -317,5 +372,8 @@ const struct ethtool_ops icssg_ethtool_ops = {
>>   	.set_eee = emac_set_eee,
>>   	.nway_reset = emac_nway_reset,
>>   	.get_rmon_stats = emac_get_rmon_stats,
>> +	.get_mm = emac_get_mm,
>> +	.set_mm = emac_set_mm,
>> +	.get_mm_stats = emac_get_mm_stats,
>>   };
>>   EXPORT_SYMBOL_GPL(icssg_ethtool_ops);
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> index 7a586038adf8..9c31574cc7f6 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -58,7 +58,7 @@
>>   
>>   #define ICSSG_MAX_RFLOWS	8	/* per slice */
>>   
>> -#define ICSSG_NUM_PA_STATS	32
>> +#define ICSSG_NUM_PA_STATS	37
> 
> Can't this be expressed as ARRAY_SIZE(icssg_all_pa_stats)? It is very
> fragile to have to count and update this manually.
> 

Yes I think its high time we did this. Will make this change in the next 
version.

>>   #define ICSSG_NUM_MIIG_STATS	60
>>   /* Number of ICSSG related stats */
>>   #define ICSSG_NUM_STATS (ICSSG_NUM_MIIG_STATS + ICSSG_NUM_PA_STATS)
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.h b/drivers/net/ethernet/ti/icssg/icssg_stats.h
>> index 5ec0b38e0c67..f35ae1b4f846 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.h
>> @@ -189,6 +189,11 @@ static const struct icssg_pa_stats icssg_all_pa_stats[] = {
>>   	ICSSG_PA_STATS(FW_INF_DROP_PRIOTAGGED),
>>   	ICSSG_PA_STATS(FW_INF_DROP_NOTAG),
>>   	ICSSG_PA_STATS(FW_INF_DROP_NOTMEMBER),
>> +	ICSSG_PA_STATS(FW_PREEMPT_BAD_FRAG),
>> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_ERR),
>> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_TX),
>> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_OK),
>> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_RX),
>>   	ICSSG_PA_STATS(FW_RX_EOF_SHORT_FRMERR),
>>   	ICSSG_PA_STATS(FW_RX_B0_DROP_EARLY_EOF),
>>   	ICSSG_PA_STATS(FW_TX_JUMBO_FRM_CUTOFF),
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
>> index 7e053b8af3ec..855fd4ed0b3f 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_switch_map.h
>> @@ -256,6 +256,11 @@
>>   #define FW_INF_DROP_PRIOTAGGED		0x0148
>>   #define FW_INF_DROP_NOTAG		0x0150
>>   #define FW_INF_DROP_NOTMEMBER		0x0158
>> +#define FW_PREEMPT_BAD_FRAG		0x0160
>> +#define FW_PREEMPT_ASSEMBLY_ERR		0x0168
>> +#define FW_PREEMPT_FRAG_CNT_TX		0x0170
>> +#define FW_PREEMPT_ASSEMBLY_OK		0x0178
>> +#define FW_PREEMPT_FRAG_CNT_RX		0x0180
>>   #define FW_RX_EOF_SHORT_FRMERR		0x0188
>>   #define FW_RX_B0_DROP_EARLY_EOF		0x0190
>>   #define FW_TX_JUMBO_FRM_CUTOFF		0x0198
>> -- 
>> 2.43.0
>>

-- 
Thanks,
Meghana Malladi

Re: [PATCH net-next 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

[Vladimir Oltean]

On Wed, Jan 28, 2026 at 06:32:05PM +0530, Malladi, Meghana wrote:
> > > diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> > > index b715af21d23a..ceca6d6ec0f4 100644
> > > --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> > > +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> > > @@ -294,6 +294,61 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
> > >   	return 0;
> > >   }
> > > +static int emac_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
> > > +{
> > > +	struct prueth_emac *emac = netdev_priv(ndev);
> > > +	struct prueth_qos_iet *iet = &emac->qos.iet;
> > > +	void __iomem *config;
> > > +
> > > +	config = emac->dram.va + ICSSG_CONFIG_OFFSET;
> > > +
> > > +	state->tx_enabled = iet->fpe_enabled;
> > 
> > I would expect state->tx_enabled to be returned from
> > iet->fpe_configured, aka from the same variable in which tx_enabled was
> > saved in emac_set_mm(). In case it's not clear, ethtool saves state in
> > the device driver and expects that state to be later returned in the
> > get() callback.
> 
> Ok got it, will fix it in v2. I am aware that ethtool saves state in the
> device driver which will be returned in the get() callback but didn't know
> that should be the case every time.

Driver, hardware or firmware. But certainly the state returned in get()
must have some relationship with the state previously set in set().
iet->fpe_enabled has no relationship with state->tx_enabled; it
represents more or less the "tx_active" state.

> > > +	state->tx_active = readb(config + PRE_EMPTION_ACTIVE_TX) ? true : false;
> > > +	state->verify_enabled = readb(config + PRE_EMPTION_ENABLE_VERIFY) ? true : false;
> > > +	state->verify_time = iet->verify_time_ms;
> > 
> > Why are some values returned from firmware and others from driver memory?
> 
> Sure, I will store all the values in the driver memory and return from them.
> But should that be the case all the time ?

Not necessarily, this implementation is just very inconsistent overall
and that raised the question whether there's any reason behind it.

> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int emac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
> > > +		       struct netlink_ext_ack *extack)
> > > +{
> > > +	struct prueth_emac *emac = netdev_priv(ndev);
> > > +	struct prueth_qos_iet *iet = &emac->qos.iet;
> > > +
> > > +	if (!cfg->pmac_enabled)
> > > +		netdev_err(ndev, "preemptible MAC is always enabled");
> > 
> > missing \n, OR use NL_SET_ERR_MSG_MOD(extack) which doesn't need \n.
> > Doing the latter is preferable, because the driver still accepts the
> > command while not modifying its internal state, and ethtool prints the
> > extack as warning if the return code was 0.
> > The catch is that openlldp sets pmac_enabled=false on exit, and that
> > would otherwise generate a noisy netdev_err() in your proposal (but
> > would be silent with the extack):
> > https://github.com/intel/openlldp/blob/master/lldp_8023.c#L443-L444
> > 
> 
> Ok makes sense, thanks for pointing this out.
> 
> > > +
> > > +	iet->verify_time_ms = cfg->verify_time;
> > > +	iet->tx_min_frag_size = cfg->tx_min_frag_size;
> > > +
> > > +	iet->fpe_configured = cfg->tx_enabled;
> > > +	iet->mac_verify_configured = cfg->verify_enabled;
> > 
> > Changes to the verification parameters should retrigger the verification
> > state machine, even if the link did not flap. Also, changes to the
> > ENABLED state should similarly be applied right away.
> 
> .set_mm() will return -EBUSY if the interfaces are up.

So it won't work with the openlldp stack?
tools/testing/selftests/drivers/net/hw/ethtool_mm.sh won't pass either?
(see h1_create() and h2_create())

I think adding such limitation will put the icssg in a world of its own,
technically implementing the same API as other network controllers but
practically not usable with the same scripts and tools.

Re: [PATCH net-next 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

[Meghana Malladi]

Hi Vladimir,

On 1/28/26 18:44, Vladimir Oltean wrote:
> On Wed, Jan 28, 2026 at 06:32:05PM +0530, Malladi, Meghana wrote:
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
>>>> index b715af21d23a..ceca6d6ec0f4 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
>>>> @@ -294,6 +294,61 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
>>>>    	return 0;
>>>>    }
>>>> +static int emac_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
>>>> +{
>>>> +	struct prueth_emac *emac = netdev_priv(ndev);
>>>> +	struct prueth_qos_iet *iet = &emac->qos.iet;
>>>> +	void __iomem *config;
>>>> +
>>>> +	config = emac->dram.va + ICSSG_CONFIG_OFFSET;
>>>> +
>>>> +	state->tx_enabled = iet->fpe_enabled;
>>>
>>> I would expect state->tx_enabled to be returned from
>>> iet->fpe_configured, aka from the same variable in which tx_enabled was
>>> saved in emac_set_mm(). In case it's not clear, ethtool saves state in
>>> the device driver and expects that state to be later returned in the
>>> get() callback.
>>
>> Ok got it, will fix it in v2. I am aware that ethtool saves state in the
>> device driver which will be returned in the get() callback but didn't know
>> that should be the case every time.
> 
> Driver, hardware or firmware. But certainly the state returned in get()
> must have some relationship with the state previously set in set().
> iet->fpe_enabled has no relationship with state->tx_enabled; it
> represents more or less the "tx_active" state.
> 
>>>> +	state->tx_active = readb(config + PRE_EMPTION_ACTIVE_TX) ? true : false;
>>>> +	state->verify_enabled = readb(config + PRE_EMPTION_ENABLE_VERIFY) ? true : false;
>>>> +	state->verify_time = iet->verify_time_ms;
>>>
>>> Why are some values returned from firmware and others from driver memory?
>>
>> Sure, I will store all the values in the driver memory and return from them.
>> But should that be the case all the time ?
> 
> Not necessarily, this implementation is just very inconsistent overall
> and that raised the question whether there's any reason behind it.
> 
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int emac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
>>>> +		       struct netlink_ext_ack *extack)
>>>> +{
>>>> +	struct prueth_emac *emac = netdev_priv(ndev);
>>>> +	struct prueth_qos_iet *iet = &emac->qos.iet;
>>>> +
>>>> +	if (!cfg->pmac_enabled)
>>>> +		netdev_err(ndev, "preemptible MAC is always enabled");
>>>
>>> missing \n, OR use NL_SET_ERR_MSG_MOD(extack) which doesn't need \n.
>>> Doing the latter is preferable, because the driver still accepts the
>>> command while not modifying its internal state, and ethtool prints the
>>> extack as warning if the return code was 0.
>>> The catch is that openlldp sets pmac_enabled=false on exit, and that
>>> would otherwise generate a noisy netdev_err() in your proposal (but
>>> would be silent with the extack):
>>> https://github.com/intel/openlldp/blob/master/lldp_8023.c#L443-L444
>>>
>>
>> Ok makes sense, thanks for pointing this out.
>>
>>>> +
>>>> +	iet->verify_time_ms = cfg->verify_time;
>>>> +	iet->tx_min_frag_size = cfg->tx_min_frag_size;
>>>> +
>>>> +	iet->fpe_configured = cfg->tx_enabled;
>>>> +	iet->mac_verify_configured = cfg->verify_enabled;
>>>
>>> Changes to the verification parameters should retrigger the verification
>>> state machine, even if the link did not flap. Also, changes to the
>>> ENABLED state should similarly be applied right away.
>>
>> .set_mm() will return -EBUSY if the interfaces are up.
> 
> So it won't work with the openlldp stack?
> tools/testing/selftests/drivers/net/hw/ethtool_mm.sh won't pass either?
> (see h1_create() and h2_create())
> 
> I think adding such limitation will put the icssg in a world of its own,
> technically implementing the same API as other network controllers but
> practically not usable with the same scripts and tools.

Ok got it. Will fix this in v2. Thanks.