[PATCH net-next v3 0/6] Introduce HSR offload support for ICSSG

[PATCH net-next v3 0/6] Introduce HSR offload support for ICSSG

[MD Danish Anwar]

Hi All,
This series introduces HSR offload support for ICSSG driver. To support HSR
offload to hardware, ICSSG HSR firmware is used.

This series introduces,
1. HSR frame offload support for ICSSG driver.
2. HSR Tx Packet duplication offload
3. HSR Tx Tag and Rx Tag offload
4. Multicast filtering support in HSR offload mode.
5. Dependencies related to IEP.

HSR Test Setup:
--------------

     ___________           ___________           ___________
    |           | Link AB |           | Link BC |           |
  __|   AM64*   |_________|   AM64    |_________|   AM64*   |___
 |  | Station A |         | Station B |         | Station C |   |
 |  |___________|         |___________|         |___________|   |
 |                                                              |
 |______________________________________________________________|
                            Link CA
 *Could be any device that supports two ethernet interfaces.

Steps to switch to HSR frame forward offload mode:
-------------------------------------------------
Example assuming eth1, eth2 ports of ICSSG1 on AM64-EVM

  1) Enable HSR offload for both interfaces
      ethtool -K eth1 hsr-fwd-offload on
      ethtool -K eth1 hsr-dup-offload on
      ethtool -K eth1 hsr-tag-ins-offload on
      ethtool -K eth1 hsr-tag-rm-offload on

      ethtool -K eth2 hsr-fwd-offload on
      ethtool -K eth2 hsr-dup-offload on
      ethtool -K eth2 hsr-tag-ins-offload on
      ethtool -K eth2 hsr-tag-rm-offload on

  2) Create HSR interface and add slave interfaces to it
      ip link add name hsr0 type hsr slave1 eth1 slave2 eth2 \
    supervision 45 version 1

  3) Add IP address to the HSR interface
      ip addr add <IP_ADDR>/24 dev hsr0

  4) Bring up the HSR interface
      ip link set hsr0 up

Switching back to Dual EMAC mode:
--------------------------------
  1) Delete HSR interface
      ip link delete hsr0

  2) Disable HSR port-to-port offloading mode, packet duplication
      ethtool -K eth1 hsr-fwd-offload off
      ethtool -K eth1 hsr-dup-offload off
      ethtool -K eth1 hsr-tag-ins-offload off
      ethtool -K eth1 hsr-tag-rm-offload off

      ethtool -K eth2 hsr-fwd-offload off
      ethtool -K eth2 hsr-dup-offload off
      ethtool -K eth2 hsr-tag-ins-offload off
      ethtool -K eth2 hsr-tag-rm-offload off

Testing the port-to-port frame forward offload feature:
-----------------------------------------------------
  1) Connect the LAN cables as shown in the test setup.
  2) Configure Station A and Station C in HSR non-offload mode.
  3) Configure Station B is HSR offload mode.
  4) Since HSR is a redundancy protocol, disconnect cable "Link CA",
     to ensure frames from Station A reach Station C only through
     Station B.
  5) Run iperf3 Server on Station C and client on station A.
  7) Check the CPU usage on Station B.

CPU usage report on Station B using mpstat when running UDP iperf3:
-------------------------------------------------------------------

  1) Non-Offload case
  -------------------
  CPU  %usr  %nice  %sys %iowait  %irq  %soft  %steal  %guest   %idle
  all  0.00   0.00  0.50    0.00  3.52  29.15    0.00    0.00   66.83
    0  0.00   0.00  0.00    0.00  7.00  58.00    0.00    0.00   35.00
    1  0.00   0.00  0.99    0.00  0.99   0.00    0.00    0.00   98.02

  2) Offload case
  ---------------
  CPU  %usr  %nice  %sys %iowait  %irq  %soft  %steal  %guest   %idle
  all  0.00   0.00  0.00    0.00  0.50   0.00    0.00    0.00   99.50
    0  0.00   0.00  0.99    0.00  0.00   0.00    0.00    0.00   99.01
    1  0.00   0.00  0.00    0.00  0.00   0.00    0.00    0.00  100.00

Note:
1) At the very least, hsr-fwd-offload must be enabled.
   Without offloading the port-to-port offload, other
   HSR offloads cannot be enabled.

2) Inorder to enable hsr-tag-ins-offload, hsr-dup-offload
   must also be enabled as these are tightly coupled in
   the firmware implementation.

Changes from v2 to v3:
*) Renamed APIs common to switch and hsr modes with suffix _fw_offload.
*) Returning EOPNOTSUPP in prueth_hsr_port_link() as suggested by
   Andrew Lunn <andrew@lunn.ch>
*) Dropped unneccassary dev_err prints and changed dev_err to dev_dbg
   where applicable.
*) Renamed NETIF_PRUETH_HSR_OFFLOAD to NETIF_PRUETH_HSR_OFFLOAD_FEATURES
   to make it clear it is a collection of features, not an alias for one
   feature.
*) Added Kernel doc entries for @hsr_members and @is_hsr_offload_mode as
   suggested by Simon Horman <horms@kernel.org>
*) Dropped patch [1] as it is no longer needed in this series. It is
   already merged to net/main by commit [2].
*) Collected Reviewed-by tag from Roger Quadros <rogerq@kernel.org> for
   PATCH 1/6 and PATCH 2/6.
*) Added if check for current mode before calling __dev_mc_unsync as
   suggested by Roger Quadros <rogerq@kernel.org>
*) Updated commit message of PATCH 6/6 to describe handling of duplicate
   discard in the driver.

Changes from v1 to v2:
*) Modified patch 2/7 to only contain code movement as suggested by
   Dan Carpenter <dan.carpenter@linaro.org>
*) Added patch 3/7 by splitting it from 2/6 as the patch is not part of
   code movement done in patch 2/7.
*) Rebased on latest net-next/main.

v1: https://lore.kernel.org/all/20240808110800.1281716-1-danishanwar@ti.com/
v2: https://lore.kernel.org/all/20240813074233.2473876-1-danishanwar@ti.com

[1] https://lore.kernel.org/all/20240813074233.2473876-2-danishanwar@ti.com/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=e846be0fba85  

MD Danish Anwar (4):
  net: ti: icss-iep: Move icss_iep structure
  net: ti: icssg-prueth: Stop hardcoding def_inc
  net: ti: icssg-prueth: Add support for HSR frame forward offload
  net: ti: icssg-prueth: Add multicast filtering support in HSR mode

Ravi Gunasekaran (2):
  net: ti: icssg-prueth: Enable HSR Tx Packet duplication offload
  net: ti: icssg-prueth: Enable HSR Tx Tag and Rx Tag offload

 drivers/net/ethernet/ti/icssg/icss_iep.c      |  72 --------
 drivers/net/ethernet/ti/icssg/icss_iep.h      |  73 +++++++-
 .../net/ethernet/ti/icssg/icssg_classifier.c  |   1 +
 drivers/net/ethernet/ti/icssg/icssg_common.c  |  16 +-
 drivers/net/ethernet/ti/icssg/icssg_config.c  |  22 ++-
 drivers/net/ethernet/ti/icssg/icssg_config.h  |   2 +
 drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 172 +++++++++++++++++-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   9 +
 8 files changed, 275 insertions(+), 92 deletions(-)


base-commit: e5899b60f52a7591cfc2a2dec3e83710975117d7
-- 
2.34.1

[PATCH net-next v3 1/6] net: ti: icss-iep: Move icss_iep structure

[MD Danish Anwar]

Move icss_iep structure definition and to icss_iep.h file so that the
structure members can be used / accessed by all icssg driver files.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/icssg/icss_iep.c | 72 -----------------------
 drivers/net/ethernet/ti/icssg/icss_iep.h | 73 +++++++++++++++++++++++-
 2 files changed, 72 insertions(+), 73 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index 75c294ce6fb6..5d6d1cf78e93 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -53,78 +53,6 @@
 #define IEP_CAP_CFG_CAPNR_1ST_EVENT_EN(n)	BIT(LATCH_INDEX(n))
 #define IEP_CAP_CFG_CAP_ASYNC_EN(n)		BIT(LATCH_INDEX(n) + 10)
 
-enum {
-	ICSS_IEP_GLOBAL_CFG_REG,
-	ICSS_IEP_GLOBAL_STATUS_REG,
-	ICSS_IEP_COMPEN_REG,
-	ICSS_IEP_SLOW_COMPEN_REG,
-	ICSS_IEP_COUNT_REG0,
-	ICSS_IEP_COUNT_REG1,
-	ICSS_IEP_CAPTURE_CFG_REG,
-	ICSS_IEP_CAPTURE_STAT_REG,
-
-	ICSS_IEP_CAP6_RISE_REG0,
-	ICSS_IEP_CAP6_RISE_REG1,
-
-	ICSS_IEP_CAP7_RISE_REG0,
-	ICSS_IEP_CAP7_RISE_REG1,
-
-	ICSS_IEP_CMP_CFG_REG,
-	ICSS_IEP_CMP_STAT_REG,
-	ICSS_IEP_CMP0_REG0,
-	ICSS_IEP_CMP0_REG1,
-	ICSS_IEP_CMP1_REG0,
-	ICSS_IEP_CMP1_REG1,
-
-	ICSS_IEP_CMP8_REG0,
-	ICSS_IEP_CMP8_REG1,
-	ICSS_IEP_SYNC_CTRL_REG,
-	ICSS_IEP_SYNC0_STAT_REG,
-	ICSS_IEP_SYNC1_STAT_REG,
-	ICSS_IEP_SYNC_PWIDTH_REG,
-	ICSS_IEP_SYNC0_PERIOD_REG,
-	ICSS_IEP_SYNC1_DELAY_REG,
-	ICSS_IEP_SYNC_START_REG,
-	ICSS_IEP_MAX_REGS,
-};
-
-/**
- * struct icss_iep_plat_data - Plat data to handle SoC variants
- * @config: Regmap configuration data
- * @reg_offs: register offsets to capture offset differences across SoCs
- * @flags: Flags to represent IEP properties
- */
-struct icss_iep_plat_data {
-	const struct regmap_config *config;
-	u32 reg_offs[ICSS_IEP_MAX_REGS];
-	u32 flags;
-};
-
-struct icss_iep {
-	struct device *dev;
-	void __iomem *base;
-	const struct icss_iep_plat_data *plat_data;
-	struct regmap *map;
-	struct device_node *client_np;
-	unsigned long refclk_freq;
-	int clk_tick_time;	/* one refclk tick time in ns */
-	struct ptp_clock_info ptp_info;
-	struct ptp_clock *ptp_clock;
-	struct mutex ptp_clk_mutex;	/* PHC access serializer */
-	u32 def_inc;
-	s16 slow_cmp_inc;
-	u32 slow_cmp_count;
-	const struct icss_iep_clockops *ops;
-	void *clockops_data;
-	u32 cycle_time_ns;
-	u32 perout_enabled;
-	bool pps_enabled;
-	int cap_cmp_irq;
-	u64 period;
-	u32 latch_enable;
-	struct work_struct work;
-};
-
 /**
  * icss_iep_get_count_hi() - Get the upper 32 bit IEP counter
  * @iep: Pointer to structure representing IEP.
diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.h b/drivers/net/ethernet/ti/icssg/icss_iep.h
index 803a4b714893..0bdca0155abd 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.h
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.h
@@ -12,7 +12,78 @@
 #include <linux/ptp_clock_kernel.h>
 #include <linux/regmap.h>
 
-struct icss_iep;
+enum {
+	ICSS_IEP_GLOBAL_CFG_REG,
+	ICSS_IEP_GLOBAL_STATUS_REG,
+	ICSS_IEP_COMPEN_REG,
+	ICSS_IEP_SLOW_COMPEN_REG,
+	ICSS_IEP_COUNT_REG0,
+	ICSS_IEP_COUNT_REG1,
+	ICSS_IEP_CAPTURE_CFG_REG,
+	ICSS_IEP_CAPTURE_STAT_REG,
+
+	ICSS_IEP_CAP6_RISE_REG0,
+	ICSS_IEP_CAP6_RISE_REG1,
+
+	ICSS_IEP_CAP7_RISE_REG0,
+	ICSS_IEP_CAP7_RISE_REG1,
+
+	ICSS_IEP_CMP_CFG_REG,
+	ICSS_IEP_CMP_STAT_REG,
+	ICSS_IEP_CMP0_REG0,
+	ICSS_IEP_CMP0_REG1,
+	ICSS_IEP_CMP1_REG0,
+	ICSS_IEP_CMP1_REG1,
+
+	ICSS_IEP_CMP8_REG0,
+	ICSS_IEP_CMP8_REG1,
+	ICSS_IEP_SYNC_CTRL_REG,
+	ICSS_IEP_SYNC0_STAT_REG,
+	ICSS_IEP_SYNC1_STAT_REG,
+	ICSS_IEP_SYNC_PWIDTH_REG,
+	ICSS_IEP_SYNC0_PERIOD_REG,
+	ICSS_IEP_SYNC1_DELAY_REG,
+	ICSS_IEP_SYNC_START_REG,
+	ICSS_IEP_MAX_REGS,
+};
+
+/**
+ * struct icss_iep_plat_data - Plat data to handle SoC variants
+ * @config: Regmap configuration data
+ * @reg_offs: register offsets to capture offset differences across SoCs
+ * @flags: Flags to represent IEP properties
+ */
+struct icss_iep_plat_data {
+	const struct regmap_config *config;
+	u32 reg_offs[ICSS_IEP_MAX_REGS];
+	u32 flags;
+};
+
+struct icss_iep {
+	struct device *dev;
+	void __iomem *base;
+	const struct icss_iep_plat_data *plat_data;
+	struct regmap *map;
+	struct device_node *client_np;
+	unsigned long refclk_freq;
+	int clk_tick_time;	/* one refclk tick time in ns */
+	struct ptp_clock_info ptp_info;
+	struct ptp_clock *ptp_clock;
+	struct mutex ptp_clk_mutex;	/* PHC access serializer */
+	u32 def_inc;
+	s16 slow_cmp_inc;
+	u32 slow_cmp_count;
+	const struct icss_iep_clockops *ops;
+	void *clockops_data;
+	u32 cycle_time_ns;
+	u32 perout_enabled;
+	bool pps_enabled;
+	int cap_cmp_irq;
+	u64 period;
+	u32 latch_enable;
+	struct work_struct work;
+};
+
 extern const struct icss_iep_clockops prueth_iep_clockops;
 
 /* Firmware specific clock operations */
-- 
2.34.1

[PATCH net-next v3 2/6] net: ti: icssg-prueth: Stop hardcoding def_inc

[MD Danish Anwar]

The def_inc is stored in icss_iep structure. Currently default increment
(ns per clock tick) is hardcoded to 4 (Clock frequency being 250 MHz).
Change this to use the iep->def_inc variable as the iep structure is now
accessible to the driver files.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Reviewed-by: Roger Quadros <rogerq@kernel.org>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index f623a0f603fc..641e54849762 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -365,7 +365,8 @@ static void prueth_iep_settime(void *clockops_data, u64 ns)
 	sc_desc.cyclecounter0_set = cyclecount & GENMASK(31, 0);
 	sc_desc.cyclecounter1_set = (cyclecount & GENMASK(63, 32)) >> 32;
 	sc_desc.iepcount_set = ns % cycletime;
-	sc_desc.CMP0_current = cycletime - 4; //Count from 0 to (cycle time)-4
+	/* Count from 0 to (cycle time) - emac->iep->def_inc */
+	sc_desc.CMP0_current = cycletime - emac->iep->def_inc;
 
 	memcpy_toio(sc_descp, &sc_desc, sizeof(sc_desc));
 
-- 
2.34.1

[PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[MD Danish Anwar]

Add support for offloading HSR port-to-port frame forward to hardware.
When the slave interfaces are added to the HSR interface, the PRU cores
will be stopped and ICSSG HSR firmwares will be loaded to them.

Similarly, when HSR interface is deleted, the PRU cores will be stopped
and dual EMAC firmware will be loaded to them.

This commit also renames some APIs that are common between switch and
hsr mode with '_fw_offload' suffix.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 .../net/ethernet/ti/icssg/icssg_classifier.c  |   1 +
 drivers/net/ethernet/ti/icssg/icssg_config.c  |  18 +--
 drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 117 +++++++++++++++++-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   6 +
 4 files changed, 130 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_classifier.c b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
index 9ec504d976d6..833ca86d0b71 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_classifier.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
@@ -290,6 +290,7 @@ void icssg_class_set_host_mac_addr(struct regmap *miig_rt, const u8 *mac)
 		     mac[2] << 16 | mac[3] << 24));
 	regmap_write(miig_rt, MAC_INTERFACE_1, (u32)(mac[4] | mac[5] << 8));
 }
+EXPORT_SYMBOL_GPL(icssg_class_set_host_mac_addr);
 
 void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac)
 {
diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
index dae52a83a378..7b2e6c192ff3 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
@@ -107,7 +107,7 @@ static const struct map hwq_map[2][ICSSG_NUM_OTHER_QUEUES] = {
 	},
 };
 
-static void icssg_config_mii_init_switch(struct prueth_emac *emac)
+static void icssg_config_mii_init_fw_offload(struct prueth_emac *emac)
 {
 	struct prueth *prueth = emac->prueth;
 	int mii = prueth_emac_slice(emac);
@@ -278,7 +278,7 @@ static int emac_r30_is_done(struct prueth_emac *emac)
 	return 1;
 }
 
-static int prueth_switch_buffer_setup(struct prueth_emac *emac)
+static int prueth_fw_offload_buffer_setup(struct prueth_emac *emac)
 {
 	struct icssg_buffer_pool_cfg __iomem *bpool_cfg;
 	struct icssg_rxq_ctx __iomem *rxq_ctx;
@@ -424,7 +424,7 @@ static void icssg_init_emac_mode(struct prueth *prueth)
 	icssg_class_set_host_mac_addr(prueth->miig_rt, mac);
 }
 
-static void icssg_init_switch_mode(struct prueth *prueth)
+static void icssg_init_fw_offload_mode(struct prueth *prueth)
 {
 	u32 addr = prueth->shram.pa + EMAC_ICSSG_SWITCH_DEFAULT_VLAN_TABLE_OFFSET;
 	int i;
@@ -455,8 +455,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
 	struct icssg_flow_cfg __iomem *flow_cfg;
 	int ret;
 
-	if (prueth->is_switch_mode)
-		icssg_init_switch_mode(prueth);
+	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
+		icssg_init_fw_offload_mode(prueth);
 	else
 		icssg_init_emac_mode(prueth);
 
@@ -472,8 +472,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
 	regmap_update_bits(prueth->miig_rt, ICSSG_CFG_OFFSET,
 			   ICSSG_CFG_DEFAULT, ICSSG_CFG_DEFAULT);
 	icssg_miig_set_interface_mode(prueth->miig_rt, slice, emac->phy_if);
-	if (prueth->is_switch_mode)
-		icssg_config_mii_init_switch(emac);
+	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
+		icssg_config_mii_init_fw_offload(emac);
 	else
 		icssg_config_mii_init(emac);
 	icssg_config_ipg(emac);
@@ -498,8 +498,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
 	writeb(0, config + SPL_PKT_DEFAULT_PRIORITY);
 	writeb(0, config + QUEUE_NUM_UNTAGGED);
 
-	if (prueth->is_switch_mode)
-		ret = prueth_switch_buffer_setup(emac);
+	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
+		ret = prueth_fw_offload_buffer_setup(emac);
 	else
 		ret = prueth_emac_buffer_setup(emac);
 	if (ret)
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 641e54849762..f4fd346fe6f5 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -13,6 +13,7 @@
 #include <linux/dma/ti-cppi5.h>
 #include <linux/etherdevice.h>
 #include <linux/genalloc.h>
+#include <linux/if_hsr.h>
 #include <linux/if_vlan.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -40,6 +41,8 @@
 #define DEFAULT_PORT_MASK	1
 #define DEFAULT_UNTAG_MASK	1
 
+#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
+
 /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
 #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
 
@@ -118,6 +121,19 @@ static irqreturn_t prueth_tx_ts_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static struct icssg_firmwares icssg_hsr_firmwares[] = {
+	{
+		.pru = "ti-pruss/am65x-sr2-pru0-pruhsr-fw.elf",
+		.rtu = "ti-pruss/am65x-sr2-rtu0-pruhsr-fw.elf",
+		.txpru = "ti-pruss/am65x-sr2-txpru0-pruhsr-fw.elf",
+	},
+	{
+		.pru = "ti-pruss/am65x-sr2-pru1-pruhsr-fw.elf",
+		.rtu = "ti-pruss/am65x-sr2-rtu1-pruhsr-fw.elf",
+		.txpru = "ti-pruss/am65x-sr2-txpru1-pruhsr-fw.elf",
+	}
+};
+
 static struct icssg_firmwares icssg_switch_firmwares[] = {
 	{
 		.pru = "ti-pruss/am65x-sr2-pru0-prusw-fw.elf",
@@ -152,6 +168,8 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
 
 	if (prueth->is_switch_mode)
 		firmwares = icssg_switch_firmwares;
+	else if (prueth->is_hsr_offload_mode)
+		firmwares = icssg_hsr_firmwares;
 	else
 		firmwares = icssg_emac_firmwares;
 
@@ -726,6 +744,19 @@ static void emac_ndo_set_rx_mode(struct net_device *ndev)
 	queue_work(emac->cmd_wq, &emac->rx_mode_work);
 }
 
+static int emac_ndo_set_features(struct net_device *ndev,
+				 netdev_features_t features)
+{
+	netdev_features_t hsr_feature_present = ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
+	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
+	bool hsr_change_request = ((hsr_feature_wanted ^ hsr_feature_present) != 0);
+
+	if (hsr_change_request)
+		ndev->features = features;
+
+	return 0;
+}
+
 static const struct net_device_ops emac_netdev_ops = {
 	.ndo_open = emac_ndo_open,
 	.ndo_stop = emac_ndo_stop,
@@ -737,6 +768,7 @@ static const struct net_device_ops emac_netdev_ops = {
 	.ndo_eth_ioctl = icssg_ndo_ioctl,
 	.ndo_get_stats64 = icssg_ndo_get_stats64,
 	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
+	.ndo_set_features = emac_ndo_set_features,
 };
 
 static int prueth_netdev_init(struct prueth *prueth,
@@ -865,6 +897,7 @@ static int prueth_netdev_init(struct prueth *prueth,
 	ndev->ethtool_ops = &icssg_ethtool_ops;
 	ndev->hw_features = NETIF_F_SG;
 	ndev->features = ndev->hw_features;
+	ndev->hw_features |= NETIF_F_HW_HSR_FWD;
 
 	netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll);
 	hrtimer_init(&emac->rx_hrtimer, CLOCK_MONOTONIC,
@@ -953,7 +986,7 @@ static void prueth_emac_restart(struct prueth *prueth)
 	netif_device_attach(emac1->ndev);
 }
 
-static void icssg_enable_switch_mode(struct prueth *prueth)
+static void icssg_change_mode(struct prueth *prueth)
 {
 	struct prueth_emac *emac;
 	int mac;
@@ -973,8 +1006,12 @@ static void icssg_enable_switch_mode(struct prueth *prueth)
 					  BIT(emac->port_id) | DEFAULT_PORT_MASK,
 					  BIT(emac->port_id) | DEFAULT_UNTAG_MASK,
 					  true);
+			if (prueth->is_hsr_offload_mode)
+				icssg_vtbl_modify(emac, DEFAULT_VID, DEFAULT_PORT_MASK,
+						  DEFAULT_UNTAG_MASK, true);
 			icssg_set_pvid(prueth, emac->port_vlan, emac->port_id);
-			icssg_set_port_state(emac, ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE);
+			if (prueth->is_switch_mode)
+				icssg_set_port_state(emac, ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE);
 		}
 	}
 }
@@ -1012,7 +1049,7 @@ static int prueth_netdevice_port_link(struct net_device *ndev,
 			prueth->is_switch_mode = true;
 			prueth->default_vlan = 1;
 			emac->port_vlan = prueth->default_vlan;
-			icssg_enable_switch_mode(prueth);
+			icssg_change_mode(prueth);
 		}
 	}
 
@@ -1040,6 +1077,59 @@ static void prueth_netdevice_port_unlink(struct net_device *ndev)
 		prueth->hw_bridge_dev = NULL;
 }
 
+static int prueth_hsr_port_link(struct net_device *ndev)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth *prueth = emac->prueth;
+	struct prueth_emac *emac0;
+	struct prueth_emac *emac1;
+
+	emac0 = prueth->emac[PRUETH_MAC0];
+	emac1 = prueth->emac[PRUETH_MAC1];
+
+	if (prueth->is_switch_mode)
+		return -EOPNOTSUPP;
+
+	prueth->hsr_members |= BIT(emac->port_id);
+	if (!prueth->is_switch_mode && !prueth->is_hsr_offload_mode) {
+		if (prueth->hsr_members & BIT(PRUETH_PORT_MII0) &&
+		    prueth->hsr_members & BIT(PRUETH_PORT_MII1)) {
+			if (!(emac0->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES) &&
+			    !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES))
+				return -EOPNOTSUPP;
+			prueth->is_hsr_offload_mode = true;
+			prueth->default_vlan = 1;
+			emac0->port_vlan = prueth->default_vlan;
+			emac1->port_vlan = prueth->default_vlan;
+			icssg_change_mode(prueth);
+			dev_dbg(prueth->dev, "Enabling HSR offload mode\n");
+		}
+	}
+
+	return 0;
+}
+
+static void prueth_hsr_port_unlink(struct net_device *ndev)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth *prueth = emac->prueth;
+	struct prueth_emac *emac0;
+	struct prueth_emac *emac1;
+
+	emac0 = prueth->emac[PRUETH_MAC0];
+	emac1 = prueth->emac[PRUETH_MAC1];
+
+	prueth->hsr_members &= ~BIT(emac->port_id);
+	if (prueth->is_hsr_offload_mode) {
+		prueth->is_hsr_offload_mode = false;
+		emac0->port_vlan = 0;
+		emac1->port_vlan = 0;
+		prueth->hsr_dev = NULL;
+		prueth_emac_restart(prueth);
+		dev_dbg(prueth->dev, "Enabling Dual EMAC mode\n");
+	}
+}
+
 /* netdev notifier */
 static int prueth_netdevice_event(struct notifier_block *unused,
 				  unsigned long event, void *ptr)
@@ -1047,6 +1137,8 @@ static int prueth_netdevice_event(struct notifier_block *unused,
 	struct netlink_ext_ack *extack = netdev_notifier_info_to_extack(ptr);
 	struct net_device *ndev = netdev_notifier_info_to_dev(ptr);
 	struct netdev_notifier_changeupper_info *info;
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth *prueth = emac->prueth;
 	int ret = NOTIFY_DONE;
 
 	if (ndev->netdev_ops != &emac_netdev_ops)
@@ -1056,6 +1148,25 @@ static int prueth_netdevice_event(struct notifier_block *unused,
 	case NETDEV_CHANGEUPPER:
 		info = ptr;
 
+		if ((ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES) &&
+		    is_hsr_master(info->upper_dev)) {
+			if (info->linking) {
+				if (!prueth->hsr_dev) {
+					prueth->hsr_dev = info->upper_dev;
+					icssg_class_set_host_mac_addr(prueth->miig_rt,
+								      prueth->hsr_dev->dev_addr);
+				} else {
+					if (prueth->hsr_dev != info->upper_dev) {
+						dev_dbg(prueth->dev, "Both interfaces must be linked to same upper device\n");
+						return -EOPNOTSUPP;
+					}
+				}
+				prueth_hsr_port_link(ndev);
+			} else {
+				prueth_hsr_port_unlink(ndev);
+			}
+		}
+
 		if (netif_is_bridge_master(info->upper_dev)) {
 			if (info->linking)
 				ret = prueth_netdevice_port_link(ndev, info->upper_dev, extack);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 786bd1ba34ab..a4b025fae797 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -243,11 +243,14 @@ struct icssg_firmwares {
  * @iep1: pointer to IEP1 device
  * @vlan_tbl: VLAN-FID table pointer
  * @hw_bridge_dev: pointer to HW bridge net device
+ * @hsr_dev: pointer to the HSR net device
  * @br_members: bitmask of bridge member ports
+ * @hsr_members: bitmask of hsr member ports
  * @prueth_netdevice_nb: netdevice notifier block
  * @prueth_switchdev_nb: switchdev notifier block
  * @prueth_switchdev_bl_nb: switchdev blocking notifier block
  * @is_switch_mode: flag to indicate if device is in Switch mode
+ * @is_hsr_offload_mode: flag to indicate if device is in hsr offload mode
  * @is_switchmode_supported: indicates platform support for switch mode
  * @switch_id: ID for mapping switch ports to bridge
  * @default_vlan: Default VLAN for host
@@ -279,11 +282,14 @@ struct prueth {
 	struct prueth_vlan_tbl *vlan_tbl;
 
 	struct net_device *hw_bridge_dev;
+	struct net_device *hsr_dev;
 	u8 br_members;
+	u8 hsr_members;
 	struct notifier_block prueth_netdevice_nb;
 	struct notifier_block prueth_switchdev_nb;
 	struct notifier_block prueth_switchdev_bl_nb;
 	bool is_switch_mode;
+	bool is_hsr_offload_mode;
 	bool is_switchmode_supported;
 	unsigned char switch_id[MAX_PHYS_ITEM_ID_LEN];
 	int default_vlan;
-- 
2.34.1

[PATCH net-next v3 4/6] net: ti: icssg-prueth: Enable HSR Tx Packet duplication offload

[MD Danish Anwar]

From: Ravi Gunasekaran <r-gunasekaran@ti.com>

The HSR stack allows to offload its Tx packet duplication functionality to
the hardware. Enable this offloading feature for ICSSG driver

Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_common.c | 13 ++++++++++---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c |  5 +++--
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 ++
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index b9d8a93d1680..2d6d8648f5a9 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_common.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
@@ -660,14 +660,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
 {
 	struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
 	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth *prueth = emac->prueth;
 	struct netdev_queue *netif_txq;
 	struct prueth_tx_chn *tx_chn;
 	dma_addr_t desc_dma, buf_dma;
+	u32 pkt_len, dst_tag_id;
 	int i, ret = 0, q_idx;
 	bool in_tx_ts = 0;
 	int tx_ts_cookie;
 	void **swdata;
-	u32 pkt_len;
 	u32 *epib;
 
 	pkt_len = skb_headlen(skb);
@@ -712,9 +713,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
 
 	/* set dst tag to indicate internal qid at the firmware which is at
 	 * bit8..bit15. bit0..bit7 indicates port num for directed
-	 * packets in case of switch mode operation
+	 * packets in case of switch mode operation and port num 0
+	 * for undirected packets in case of HSR offload mode
 	 */
-	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
+	dst_tag_id = emac->port_id | (q_idx << 8);
+
+	if (prueth->is_hsr_offload_mode && (ndev->features & NETIF_F_HW_HSR_DUP))
+		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
+
+	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
 	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
 	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
 	swdata = cppi5_hdesc_get_swdata(first_desc);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index f4fd346fe6f5..b60efe7bd7a7 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -41,7 +41,8 @@
 #define DEFAULT_PORT_MASK	1
 #define DEFAULT_UNTAG_MASK	1
 
-#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
+#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
+						 NETIF_F_HW_HSR_DUP)
 
 /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
 #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
@@ -897,7 +898,7 @@ static int prueth_netdev_init(struct prueth *prueth,
 	ndev->ethtool_ops = &icssg_ethtool_ops;
 	ndev->hw_features = NETIF_F_SG;
 	ndev->features = ndev->hw_features;
-	ndev->hw_features |= NETIF_F_HW_HSR_FWD;
+	ndev->hw_features |= NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
 
 	netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll);
 	hrtimer_init(&emac->rx_hrtimer, CLOCK_MONOTONIC,
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index a4b025fae797..e110a5f92684 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -59,6 +59,8 @@
 
 #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
 
+#define PRUETH_UNDIRECTED_PKT_DST_TAG	0
+
 /* Firmware status codes */
 #define ICSS_HS_FW_READY 0x55555555
 #define ICSS_HS_FW_DEAD 0xDEAD0000	/* lower 16 bits contain error code */
-- 
2.34.1

[PATCH net-next v3 5/6] net: ti: icssg-prueth: Add multicast filtering support in HSR mode

[MD Danish Anwar]

Add support for multicast filtering in HSR mode

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 42 +++++++++++++++++++-
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index b60efe7bd7a7..ecc342bcc1b5 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -490,6 +490,36 @@ static int icssg_prueth_del_mcast(struct net_device *ndev, const u8 *addr)
 	return 0;
 }
 
+static int icssg_prueth_hsr_add_mcast(struct net_device *ndev, const u8 *addr)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth *prueth = emac->prueth;
+
+	icssg_fdb_add_del(emac, addr, prueth->default_vlan,
+			  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
+			  ICSSG_FDB_ENTRY_P1_MEMBERSHIP |
+			  ICSSG_FDB_ENTRY_P2_MEMBERSHIP |
+			  ICSSG_FDB_ENTRY_BLOCK, true);
+
+	icssg_vtbl_modify(emac, emac->port_vlan, BIT(emac->port_id),
+			  BIT(emac->port_id), true);
+	return 0;
+}
+
+static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth *prueth = emac->prueth;
+
+	icssg_fdb_add_del(emac, addr, prueth->default_vlan,
+			  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
+			  ICSSG_FDB_ENTRY_P1_MEMBERSHIP |
+			  ICSSG_FDB_ENTRY_P2_MEMBERSHIP |
+			  ICSSG_FDB_ENTRY_BLOCK, false);
+
+	return 0;
+}
+
 /**
  * emac_ndo_open - EMAC device open
  * @ndev: network adapter device
@@ -650,7 +680,10 @@ static int emac_ndo_stop(struct net_device *ndev)
 
 	icssg_class_disable(prueth->miig_rt, prueth_emac_slice(emac));
 
-	__dev_mc_unsync(ndev, icssg_prueth_del_mcast);
+	if (emac->prueth->is_hsr_offload_mode)
+		__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
+	else
+		__dev_mc_unsync(ndev, icssg_prueth_del_mcast);
 
 	atomic_set(&emac->tdown_cnt, emac->tx_ch_num);
 	/* ensure new tdown_cnt value is visible */
@@ -728,7 +761,12 @@ static void emac_ndo_set_rx_mode_work(struct work_struct *work)
 		return;
 	}
 
-	__dev_mc_sync(ndev, icssg_prueth_add_mcast, icssg_prueth_del_mcast);
+	if (emac->prueth->is_hsr_offload_mode)
+		__dev_mc_sync(ndev, icssg_prueth_hsr_add_mcast,
+			      icssg_prueth_hsr_del_mcast);
+	else
+		__dev_mc_sync(ndev, icssg_prueth_add_mcast,
+			      icssg_prueth_del_mcast);
 }
 
 /**
-- 
2.34.1

[PATCH net-next v3 6/6] net: ti: icssg-prueth: Enable HSR Tx Tag and Rx Tag offload

[MD Danish Anwar]

From: Ravi Gunasekaran <r-gunasekaran@ti.com>

Add support to offload HSR Tx Tag Insertion and Rx Tag Removal
and duplicate discard.

Duplicate discard is done as part of RX tag removal and it is
done by the firmware. When driver sends the r30 command
ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware does RX tag removal as well as
duplicate discard.

Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_common.c |  3 +++
 drivers/net/ethernet/ti/icssg/icssg_config.c |  4 +++-
 drivers/net/ethernet/ti/icssg/icssg_config.h |  2 ++
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 11 ++++++++++-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  1 +
 5 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index 2d6d8648f5a9..4eae4f9250c0 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_common.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
@@ -721,6 +721,9 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
 	if (prueth->is_hsr_offload_mode && (ndev->features & NETIF_F_HW_HSR_DUP))
 		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
 
+	if (prueth->is_hsr_offload_mode && (ndev->features & NETIF_F_HW_HSR_TAG_INS))
+		epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
+
 	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
 	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
 	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
index 7b2e6c192ff3..72ace151d8e9 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
@@ -531,7 +531,9 @@ static const struct icssg_r30_cmd emac_r32_bitmask[] = {
 	{{EMAC_NONE,  0xffff4000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx ENABLE*/
 	{{EMAC_NONE,  0xbfff0000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx DISABLE*/
 	{{0xffff0010,  EMAC_NONE, 0xffff0010, EMAC_NONE}},	/* VLAN AWARE*/
-	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}}	/* VLAN UNWARE*/
+	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}},	/* VLAN UNWARE*/
+	{{0xffff2000, EMAC_NONE, EMAC_NONE, EMAC_NONE}},	/* HSR_RX_OFFLOAD_ENABLE */
+	{{0xdfff0000, EMAC_NONE, EMAC_NONE, EMAC_NONE}}		/* HSR_RX_OFFLOAD_DISABLE */
 };
 
 int icssg_set_port_state(struct prueth_emac *emac,
diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
index 1ac60283923b..92c2deaa3068 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
@@ -80,6 +80,8 @@ enum icssg_port_state_cmd {
 	ICSSG_EMAC_PORT_PREMPT_TX_DISABLE,
 	ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE,
 	ICSSG_EMAC_PORT_VLAN_AWARE_DISABLE,
+	ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE,
+	ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE,
 	ICSSG_EMAC_PORT_MAX_COMMANDS
 };
 
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index ecc342bcc1b5..c671cf9813f0 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -42,7 +42,9 @@
 #define DEFAULT_UNTAG_MASK	1
 
 #define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
-						 NETIF_F_HW_HSR_DUP)
+						 NETIF_F_HW_HSR_DUP | \
+						 NETIF_F_HW_HSR_TAG_INS | \
+						 NETIF_F_HW_HSR_TAG_RM)
 
 /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
 #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
@@ -1034,6 +1036,13 @@ static void icssg_change_mode(struct prueth *prueth)
 
 	for (mac = PRUETH_MAC0; mac < PRUETH_NUM_MACS; mac++) {
 		emac = prueth->emac[mac];
+		if (prueth->is_hsr_offload_mode) {
+			if (emac->ndev->features & NETIF_F_HW_HSR_TAG_RM)
+				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE);
+			else
+				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE);
+		}
+
 		if (netif_running(emac->ndev)) {
 			icssg_fdb_add_del(emac, eth_stp_addr, prueth->default_vlan,
 					  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index e110a5f92684..bba6da2e6bd8 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -60,6 +60,7 @@
 #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
 
 #define PRUETH_UNDIRECTED_PKT_DST_TAG	0
+#define PRUETH_UNDIRECTED_PKT_TAG_INS	BIT(30)
 
 /* Firmware status codes */
 #define ICSS_HS_FW_READY 0x55555555
-- 
2.34.1

Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Roger Quadros]

On 28/08/2024 12:18, MD Danish Anwar wrote:
> Add support for offloading HSR port-to-port frame forward to hardware.
> When the slave interfaces are added to the HSR interface, the PRU cores
> will be stopped and ICSSG HSR firmwares will be loaded to them.
> 
> Similarly, when HSR interface is deleted, the PRU cores will be stopped
> and dual EMAC firmware will be loaded to them.

And what happens if we first started with switch mode and then switched to HSR mode?
Is this case possible and if yes should it revert to the last used mode
instead of forcing to dual EMAC mode?

> 
> This commit also renames some APIs that are common between switch and
> hsr mode with '_fw_offload' suffix.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  .../net/ethernet/ti/icssg/icssg_classifier.c  |   1 +
>  drivers/net/ethernet/ti/icssg/icssg_config.c  |  18 +--
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 117 +++++++++++++++++-
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   6 +
>  4 files changed, 130 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_classifier.c b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
> index 9ec504d976d6..833ca86d0b71 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_classifier.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
> @@ -290,6 +290,7 @@ void icssg_class_set_host_mac_addr(struct regmap *miig_rt, const u8 *mac)
>  		     mac[2] << 16 | mac[3] << 24));
>  	regmap_write(miig_rt, MAC_INTERFACE_1, (u32)(mac[4] | mac[5] << 8));
>  }
> +EXPORT_SYMBOL_GPL(icssg_class_set_host_mac_addr);
>  
>  void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac)
>  {
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
> index dae52a83a378..7b2e6c192ff3 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
> @@ -107,7 +107,7 @@ static const struct map hwq_map[2][ICSSG_NUM_OTHER_QUEUES] = {
>  	},
>  };
>  
> -static void icssg_config_mii_init_switch(struct prueth_emac *emac)
> +static void icssg_config_mii_init_fw_offload(struct prueth_emac *emac)
>  {
>  	struct prueth *prueth = emac->prueth;
>  	int mii = prueth_emac_slice(emac);
> @@ -278,7 +278,7 @@ static int emac_r30_is_done(struct prueth_emac *emac)
>  	return 1;
>  }
>  
> -static int prueth_switch_buffer_setup(struct prueth_emac *emac)
> +static int prueth_fw_offload_buffer_setup(struct prueth_emac *emac)
>  {
>  	struct icssg_buffer_pool_cfg __iomem *bpool_cfg;
>  	struct icssg_rxq_ctx __iomem *rxq_ctx;
> @@ -424,7 +424,7 @@ static void icssg_init_emac_mode(struct prueth *prueth)
>  	icssg_class_set_host_mac_addr(prueth->miig_rt, mac);
>  }
>  
> -static void icssg_init_switch_mode(struct prueth *prueth)
> +static void icssg_init_fw_offload_mode(struct prueth *prueth)
>  {
>  	u32 addr = prueth->shram.pa + EMAC_ICSSG_SWITCH_DEFAULT_VLAN_TABLE_OFFSET;
>  	int i;
> @@ -455,8 +455,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
>  	struct icssg_flow_cfg __iomem *flow_cfg;
>  	int ret;
>  
> -	if (prueth->is_switch_mode)
> -		icssg_init_switch_mode(prueth);
> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
> +		icssg_init_fw_offload_mode(prueth);
>  	else
>  		icssg_init_emac_mode(prueth);
>  
> @@ -472,8 +472,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
>  	regmap_update_bits(prueth->miig_rt, ICSSG_CFG_OFFSET,
>  			   ICSSG_CFG_DEFAULT, ICSSG_CFG_DEFAULT);
>  	icssg_miig_set_interface_mode(prueth->miig_rt, slice, emac->phy_if);
> -	if (prueth->is_switch_mode)
> -		icssg_config_mii_init_switch(emac);
> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
> +		icssg_config_mii_init_fw_offload(emac);
>  	else
>  		icssg_config_mii_init(emac);
>  	icssg_config_ipg(emac);
> @@ -498,8 +498,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
>  	writeb(0, config + SPL_PKT_DEFAULT_PRIORITY);
>  	writeb(0, config + QUEUE_NUM_UNTAGGED);
>  
> -	if (prueth->is_switch_mode)
> -		ret = prueth_switch_buffer_setup(emac);
> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
> +		ret = prueth_fw_offload_buffer_setup(emac);
>  	else
>  		ret = prueth_emac_buffer_setup(emac);
>  	if (ret)
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 641e54849762..f4fd346fe6f5 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -13,6 +13,7 @@
>  #include <linux/dma/ti-cppi5.h>
>  #include <linux/etherdevice.h>
>  #include <linux/genalloc.h>
> +#include <linux/if_hsr.h>
>  #include <linux/if_vlan.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -40,6 +41,8 @@
>  #define DEFAULT_PORT_MASK	1
>  #define DEFAULT_UNTAG_MASK	1
>  
> +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
> +
>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>  
> @@ -118,6 +121,19 @@ static irqreturn_t prueth_tx_ts_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static struct icssg_firmwares icssg_hsr_firmwares[] = {
> +	{
> +		.pru = "ti-pruss/am65x-sr2-pru0-pruhsr-fw.elf",
> +		.rtu = "ti-pruss/am65x-sr2-rtu0-pruhsr-fw.elf",
> +		.txpru = "ti-pruss/am65x-sr2-txpru0-pruhsr-fw.elf",
> +	},
> +	{
> +		.pru = "ti-pruss/am65x-sr2-pru1-pruhsr-fw.elf",
> +		.rtu = "ti-pruss/am65x-sr2-rtu1-pruhsr-fw.elf",
> +		.txpru = "ti-pruss/am65x-sr2-txpru1-pruhsr-fw.elf",
> +	}
> +};
> +
>  static struct icssg_firmwares icssg_switch_firmwares[] = {
>  	{
>  		.pru = "ti-pruss/am65x-sr2-pru0-prusw-fw.elf",
> @@ -152,6 +168,8 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>  
>  	if (prueth->is_switch_mode)
>  		firmwares = icssg_switch_firmwares;
> +	else if (prueth->is_hsr_offload_mode)
> +		firmwares = icssg_hsr_firmwares;
>  	else
>  		firmwares = icssg_emac_firmwares;
>  
> @@ -726,6 +744,19 @@ static void emac_ndo_set_rx_mode(struct net_device *ndev)
>  	queue_work(emac->cmd_wq, &emac->rx_mode_work);
>  }
>  
> +static int emac_ndo_set_features(struct net_device *ndev,
> +				 netdev_features_t features)
> +{
> +	netdev_features_t hsr_feature_present = ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
> +	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
> +	bool hsr_change_request = ((hsr_feature_wanted ^ hsr_feature_present) != 0);

This is quite hard to read for me.
why not just do this instead?

	netdev_features_t changed = netdev->features ^ feattures;

Then check and ack on individual features that you want to act upon.

	if (changed & NETIF_F_HW_HSR_FWD) {
		if (features & NETIF_F_HW_HSR_FWD)
			/* enable HSR FWD feature */
		else
			/* disable HSR FWD feature */
	}

	if (changed) {
		ndev->features = features;
		return 1;
	}

From include/linux/netdevice.h

 * int (*ndo_set_features)(struct net_device *dev, netdev_features_t features);
 *	Called to update device configuration to new features. Passed
 *	feature set might be less than what was returned by ndo_fix_features()).
 *	Must return >0 or -errno if it changed dev->features itself.

Can you please check that if we are not in dual emac mode then we should
error out if any HSR feature is requested to be set.

> +
> +	if (hsr_change_request)
> +		ndev->features = features;
> +
> +	return 0;


You may also want to check out

 * netdev_features_t (*ndo_fix_features)(struct net_device *dev,
 *		netdev_features_t features);
 *	Adjusts the requested feature flags according to device-specific
 *	constraints, and returns the resulting flags. Must not modify
 *	the device state.

As you mentioned there are some contstraints on what HSR features can be
enabled individually.
"2) Inorder to enable hsr-tag-ins-offload, hsr-dup-offload
   must also be enabled as these are tightly coupled in
   the firmware implementation."
You could do this check there by setting/clearing both features in tandem
if either one was set/cleared.

> +}
> +
>  static const struct net_device_ops emac_netdev_ops = {
>  	.ndo_open = emac_ndo_open,
>  	.ndo_stop = emac_ndo_stop,
> @@ -737,6 +768,7 @@ static const struct net_device_ops emac_netdev_ops = {
>  	.ndo_eth_ioctl = icssg_ndo_ioctl,
>  	.ndo_get_stats64 = icssg_ndo_get_stats64,
>  	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
> +	.ndo_set_features = emac_ndo_set_features,
>  };
>  
>  static int prueth_netdev_init(struct prueth *prueth,
> @@ -865,6 +897,7 @@ static int prueth_netdev_init(struct prueth *prueth,
>  	ndev->ethtool_ops = &icssg_ethtool_ops;
>  	ndev->hw_features = NETIF_F_SG;
>  	ndev->features = ndev->hw_features;
> +	ndev->hw_features |= NETIF_F_HW_HSR_FWD;
>  
>  	netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll);
>  	hrtimer_init(&emac->rx_hrtimer, CLOCK_MONOTONIC,
> @@ -953,7 +986,7 @@ static void prueth_emac_restart(struct prueth *prueth)
>  	netif_device_attach(emac1->ndev);
>  }
>  
> -static void icssg_enable_switch_mode(struct prueth *prueth)
> +static void icssg_change_mode(struct prueth *prueth)
>  {
>  	struct prueth_emac *emac;
>  	int mac;
> @@ -973,8 +1006,12 @@ static void icssg_enable_switch_mode(struct prueth *prueth)
>  					  BIT(emac->port_id) | DEFAULT_PORT_MASK,
>  					  BIT(emac->port_id) | DEFAULT_UNTAG_MASK,
>  					  true);
> +			if (prueth->is_hsr_offload_mode)
> +				icssg_vtbl_modify(emac, DEFAULT_VID, DEFAULT_PORT_MASK,
> +						  DEFAULT_UNTAG_MASK, true);
>  			icssg_set_pvid(prueth, emac->port_vlan, emac->port_id);
> -			icssg_set_port_state(emac, ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE);
> +			if (prueth->is_switch_mode)
> +				icssg_set_port_state(emac, ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE);
>  		}
>  	}
>  }
> @@ -1012,7 +1049,7 @@ static int prueth_netdevice_port_link(struct net_device *ndev,
>  			prueth->is_switch_mode = true;
>  			prueth->default_vlan = 1;
>  			emac->port_vlan = prueth->default_vlan;
> -			icssg_enable_switch_mode(prueth);
> +			icssg_change_mode(prueth);
>  		}
>  	}
>  
> @@ -1040,6 +1077,59 @@ static void prueth_netdevice_port_unlink(struct net_device *ndev)
>  		prueth->hw_bridge_dev = NULL;
>  }
>  
> +static int prueth_hsr_port_link(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth *prueth = emac->prueth;
> +	struct prueth_emac *emac0;
> +	struct prueth_emac *emac1;
> +
> +	emac0 = prueth->emac[PRUETH_MAC0];
> +	emac1 = prueth->emac[PRUETH_MAC1];
> +
> +	if (prueth->is_switch_mode)
> +		return -EOPNOTSUPP;
> +
> +	prueth->hsr_members |= BIT(emac->port_id);
> +	if (!prueth->is_switch_mode && !prueth->is_hsr_offload_mode) {

you already checked that is_switch_mode is not set earlier. No need to check again.

> +		if (prueth->hsr_members & BIT(PRUETH_PORT_MII0) &&
> +		    prueth->hsr_members & BIT(PRUETH_PORT_MII1)) {
> +			if (!(emac0->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES) &&
> +			    !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES))
> +				return -EOPNOTSUPP;
> +			prueth->is_hsr_offload_mode = true;
> +			prueth->default_vlan = 1;
> +			emac0->port_vlan = prueth->default_vlan;
> +			emac1->port_vlan = prueth->default_vlan;
> +			icssg_change_mode(prueth);
> +			dev_dbg(prueth->dev, "Enabling HSR offload mode\n");
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void prueth_hsr_port_unlink(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth *prueth = emac->prueth;
> +	struct prueth_emac *emac0;
> +	struct prueth_emac *emac1;
> +
> +	emac0 = prueth->emac[PRUETH_MAC0];
> +	emac1 = prueth->emac[PRUETH_MAC1];
> +
> +	prueth->hsr_members &= ~BIT(emac->port_id);
> +	if (prueth->is_hsr_offload_mode) {
> +		prueth->is_hsr_offload_mode = false;
> +		emac0->port_vlan = 0;
> +		emac1->port_vlan = 0;
> +		prueth->hsr_dev = NULL;
> +		prueth_emac_restart(prueth);
> +		dev_dbg(prueth->dev, "Enabling Dual EMAC mode\n");
> +	}
> +}
> +
>  /* netdev notifier */
>  static int prueth_netdevice_event(struct notifier_block *unused,
>  				  unsigned long event, void *ptr)
> @@ -1047,6 +1137,8 @@ static int prueth_netdevice_event(struct notifier_block *unused,
>  	struct netlink_ext_ack *extack = netdev_notifier_info_to_extack(ptr);
>  	struct net_device *ndev = netdev_notifier_info_to_dev(ptr);
>  	struct netdev_notifier_changeupper_info *info;
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth *prueth = emac->prueth;
>  	int ret = NOTIFY_DONE;
>  
>  	if (ndev->netdev_ops != &emac_netdev_ops)
> @@ -1056,6 +1148,25 @@ static int prueth_netdevice_event(struct notifier_block *unused,
>  	case NETDEV_CHANGEUPPER:
>  		info = ptr;
>  
> +		if ((ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES) &&
> +		    is_hsr_master(info->upper_dev)) {
> +			if (info->linking) {
> +				if (!prueth->hsr_dev) {
> +					prueth->hsr_dev = info->upper_dev;
> +					icssg_class_set_host_mac_addr(prueth->miig_rt,
> +								      prueth->hsr_dev->dev_addr);
> +				} else {
> +					if (prueth->hsr_dev != info->upper_dev) {
> +						dev_dbg(prueth->dev, "Both interfaces must be linked to same upper device\n");
> +						return -EOPNOTSUPP;
> +					}
> +				}
> +				prueth_hsr_port_link(ndev);
> +			} else {
> +				prueth_hsr_port_unlink(ndev);
> +			}
> +		}
> +
>  		if (netif_is_bridge_master(info->upper_dev)) {
>  			if (info->linking)
>  				ret = prueth_netdevice_port_link(ndev, info->upper_dev, extack);
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index 786bd1ba34ab..a4b025fae797 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -243,11 +243,14 @@ struct icssg_firmwares {
>   * @iep1: pointer to IEP1 device
>   * @vlan_tbl: VLAN-FID table pointer
>   * @hw_bridge_dev: pointer to HW bridge net device
> + * @hsr_dev: pointer to the HSR net device
>   * @br_members: bitmask of bridge member ports
> + * @hsr_members: bitmask of hsr member ports
>   * @prueth_netdevice_nb: netdevice notifier block
>   * @prueth_switchdev_nb: switchdev notifier block
>   * @prueth_switchdev_bl_nb: switchdev blocking notifier block
>   * @is_switch_mode: flag to indicate if device is in Switch mode
> + * @is_hsr_offload_mode: flag to indicate if device is in hsr offload mode
>   * @is_switchmode_supported: indicates platform support for switch mode
>   * @switch_id: ID for mapping switch ports to bridge
>   * @default_vlan: Default VLAN for host
> @@ -279,11 +282,14 @@ struct prueth {
>  	struct prueth_vlan_tbl *vlan_tbl;
>  
>  	struct net_device *hw_bridge_dev;
> +	struct net_device *hsr_dev;
>  	u8 br_members;
> +	u8 hsr_members;
>  	struct notifier_block prueth_netdevice_nb;
>  	struct notifier_block prueth_switchdev_nb;
>  	struct notifier_block prueth_switchdev_bl_nb;
>  	bool is_switch_mode;
> +	bool is_hsr_offload_mode;
>  	bool is_switchmode_supported;
>  	unsigned char switch_id[MAX_PHYS_ITEM_ID_LEN];
>  	int default_vlan;

-- 
cheers,
-roger

Re: [PATCH net-next v3 4/6] net: ti: icssg-prueth: Enable HSR Tx Packet duplication offload

[Roger Quadros]

On 28/08/2024 12:18, MD Danish Anwar wrote:
> From: Ravi Gunasekaran <r-gunasekaran@ti.com>
> 
> The HSR stack allows to offload its Tx packet duplication functionality to
> the hardware. Enable this offloading feature for ICSSG driver
> 
> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_common.c | 13 ++++++++++---
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c |  5 +++--
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 ++
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index b9d8a93d1680..2d6d8648f5a9 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -660,14 +660,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>  {
>  	struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
>  	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth *prueth = emac->prueth;
>  	struct netdev_queue *netif_txq;
>  	struct prueth_tx_chn *tx_chn;
>  	dma_addr_t desc_dma, buf_dma;
> +	u32 pkt_len, dst_tag_id;
>  	int i, ret = 0, q_idx;
>  	bool in_tx_ts = 0;
>  	int tx_ts_cookie;
>  	void **swdata;
> -	u32 pkt_len;
>  	u32 *epib;
>  
>  	pkt_len = skb_headlen(skb);
> @@ -712,9 +713,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>  
>  	/* set dst tag to indicate internal qid at the firmware which is at
>  	 * bit8..bit15. bit0..bit7 indicates port num for directed
> -	 * packets in case of switch mode operation
> +	 * packets in case of switch mode operation and port num 0
> +	 * for undirected packets in case of HSR offload mode
>  	 */
> -	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
> +	dst_tag_id = emac->port_id | (q_idx << 8);
> +
> +	if (prueth->is_hsr_offload_mode && (ndev->features & NETIF_F_HW_HSR_DUP))
> +		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
> +
> +	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
>  	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>  	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
>  	swdata = cppi5_hdesc_get_swdata(first_desc);
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index f4fd346fe6f5..b60efe7bd7a7 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -41,7 +41,8 @@
>  #define DEFAULT_PORT_MASK	1
>  #define DEFAULT_UNTAG_MASK	1
>  
> -#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
> +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
> +						 NETIF_F_HW_HSR_DUP)

You mentioned that these 2 features can't be enabled individually.

So better to squash this with previous patch and use ndo_fix_features() to make sure both
are set or cleared together.

>  
>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
> @@ -897,7 +898,7 @@ static int prueth_netdev_init(struct prueth *prueth,
>  	ndev->ethtool_ops = &icssg_ethtool_ops;
>  	ndev->hw_features = NETIF_F_SG;
>  	ndev->features = ndev->hw_features;
> -	ndev->hw_features |= NETIF_F_HW_HSR_FWD;
> +	ndev->hw_features |= NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
>  
>  	netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll);
>  	hrtimer_init(&emac->rx_hrtimer, CLOCK_MONOTONIC,
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index a4b025fae797..e110a5f92684 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -59,6 +59,8 @@
>  
>  #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
>  
> +#define PRUETH_UNDIRECTED_PKT_DST_TAG	0
> +
>  /* Firmware status codes */
>  #define ICSS_HS_FW_READY 0x55555555
>  #define ICSS_HS_FW_DEAD 0xDEAD0000	/* lower 16 bits contain error code */

-- 
cheers,
-roger

Re: [PATCH net-next v3 5/6] net: ti: icssg-prueth: Add multicast filtering support in HSR mode

[Roger Quadros]

On 28/08/2024 12:19, MD Danish Anwar wrote:
> Add support for multicast filtering in HSR mode
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH net-next v3 6/6] net: ti: icssg-prueth: Enable HSR Tx Tag and Rx Tag offload

[Roger Quadros]

On 28/08/2024 12:19, MD Danish Anwar wrote:
> From: Ravi Gunasekaran <r-gunasekaran@ti.com>
> 
> Add support to offload HSR Tx Tag Insertion and Rx Tag Removal
> and duplicate discard.
> 
> Duplicate discard is done as part of RX tag removal and it is
> done by the firmware. When driver sends the r30 command
> ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware does RX tag removal as well as
> duplicate discard.
> 
> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Andrew Lunn]

On Fri, Aug 30, 2024 at 04:27:34PM +0300, Roger Quadros wrote:
> 
> 
> On 28/08/2024 12:18, MD Danish Anwar wrote:
> > Add support for offloading HSR port-to-port frame forward to hardware.
> > When the slave interfaces are added to the HSR interface, the PRU cores
> > will be stopped and ICSSG HSR firmwares will be loaded to them.
> > 
> > Similarly, when HSR interface is deleted, the PRU cores will be stopped
> > and dual EMAC firmware will be loaded to them.
> 
> And what happens if we first started with switch mode and then switched to HSR mode?
> Is this case possible and if yes should it revert to the last used mode
> instead of forcing to dual EMAC mode?
> 
> > 
> > This commit also renames some APIs that are common between switch and
> > hsr mode with '_fw_offload' suffix.
> > 
> > Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> > ---
> >  .../net/ethernet/ti/icssg/icssg_classifier.c  |   1 +
> >  drivers/net/ethernet/ti/icssg/icssg_config.c  |  18 +--
> >  drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 117 +++++++++++++++++-
> >  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   6 +
> >  4 files changed, 130 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/icssg/icssg_classifier.c b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
> > index 9ec504d976d6..833ca86d0b71 100644
> > --- a/drivers/net/ethernet/ti/icssg/icssg_classifier.c
> > +++ b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
> > @@ -290,6 +290,7 @@ void icssg_class_set_host_mac_addr(struct regmap *miig_rt, const u8 *mac)
> >  		     mac[2] << 16 | mac[3] << 24));
> >  	regmap_write(miig_rt, MAC_INTERFACE_1, (u32)(mac[4] | mac[5] << 8));
> >  }
> > +EXPORT_SYMBOL_GPL(icssg_class_set_host_mac_addr);
> >  
> >  void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac)
> >  {
> > diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
> > index dae52a83a378..7b2e6c192ff3 100644
> > --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
> > +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
> > @@ -107,7 +107,7 @@ static const struct map hwq_map[2][ICSSG_NUM_OTHER_QUEUES] = {
> >  	},
> >  };
> >  
> > -static void icssg_config_mii_init_switch(struct prueth_emac *emac)
> > +static void icssg_config_mii_init_fw_offload(struct prueth_emac *emac)
> >  {
> >  	struct prueth *prueth = emac->prueth;
> >  	int mii = prueth_emac_slice(emac);
> > @@ -278,7 +278,7 @@ static int emac_r30_is_done(struct prueth_emac *emac)
> >  	return 1;
> >  }
> >  
> > -static int prueth_switch_buffer_setup(struct prueth_emac *emac)
> > +static int prueth_fw_offload_buffer_setup(struct prueth_emac *emac)
> >  {
> >  	struct icssg_buffer_pool_cfg __iomem *bpool_cfg;
> >  	struct icssg_rxq_ctx __iomem *rxq_ctx;
> > @@ -424,7 +424,7 @@ static void icssg_init_emac_mode(struct prueth *prueth)
> >  	icssg_class_set_host_mac_addr(prueth->miig_rt, mac);
> >  }
> >  
> > -static void icssg_init_switch_mode(struct prueth *prueth)
> > +static void icssg_init_fw_offload_mode(struct prueth *prueth)
> >  {
> >  	u32 addr = prueth->shram.pa + EMAC_ICSSG_SWITCH_DEFAULT_VLAN_TABLE_OFFSET;
> >  	int i;
> > @@ -455,8 +455,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
> >  	struct icssg_flow_cfg __iomem *flow_cfg;
> >  	int ret;
> >  
> > -	if (prueth->is_switch_mode)
> > -		icssg_init_switch_mode(prueth);
> > +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
> > +		icssg_init_fw_offload_mode(prueth);
> >  	else
> >  		icssg_init_emac_mode(prueth);
> >  
> > @@ -472,8 +472,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
> >  	regmap_update_bits(prueth->miig_rt, ICSSG_CFG_OFFSET,
> >  			   ICSSG_CFG_DEFAULT, ICSSG_CFG_DEFAULT);
> >  	icssg_miig_set_interface_mode(prueth->miig_rt, slice, emac->phy_if);
> > -	if (prueth->is_switch_mode)
> > -		icssg_config_mii_init_switch(emac);
> > +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
> > +		icssg_config_mii_init_fw_offload(emac);
> >  	else
> >  		icssg_config_mii_init(emac);
> >  	icssg_config_ipg(emac);
> > @@ -498,8 +498,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
> >  	writeb(0, config + SPL_PKT_DEFAULT_PRIORITY);
> >  	writeb(0, config + QUEUE_NUM_UNTAGGED);
> >  
> > -	if (prueth->is_switch_mode)
> > -		ret = prueth_switch_buffer_setup(emac);
> > +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
> > +		ret = prueth_fw_offload_buffer_setup(emac);
> >  	else
> >  		ret = prueth_emac_buffer_setup(emac);
> >  	if (ret)
> > diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> > index 641e54849762..f4fd346fe6f5 100644
> > --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> > +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/dma/ti-cppi5.h>
> >  #include <linux/etherdevice.h>
> >  #include <linux/genalloc.h>
> > +#include <linux/if_hsr.h>
> >  #include <linux/if_vlan.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/kernel.h>
> > @@ -40,6 +41,8 @@
> >  #define DEFAULT_PORT_MASK	1
> >  #define DEFAULT_UNTAG_MASK	1
> >  
> > +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
> > +
> >  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
> >  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
> >  
> > @@ -118,6 +121,19 @@ static irqreturn_t prueth_tx_ts_irq(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static struct icssg_firmwares icssg_hsr_firmwares[] = {
> > +	{
> > +		.pru = "ti-pruss/am65x-sr2-pru0-pruhsr-fw.elf",
> > +		.rtu = "ti-pruss/am65x-sr2-rtu0-pruhsr-fw.elf",
> > +		.txpru = "ti-pruss/am65x-sr2-txpru0-pruhsr-fw.elf",
> > +	},
> > +	{
> > +		.pru = "ti-pruss/am65x-sr2-pru1-pruhsr-fw.elf",
> > +		.rtu = "ti-pruss/am65x-sr2-rtu1-pruhsr-fw.elf",
> > +		.txpru = "ti-pruss/am65x-sr2-txpru1-pruhsr-fw.elf",
> > +	}
> > +};
> > +
> >  static struct icssg_firmwares icssg_switch_firmwares[] = {
> >  	{
> >  		.pru = "ti-pruss/am65x-sr2-pru0-prusw-fw.elf",
> > @@ -152,6 +168,8 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
> >  
> >  	if (prueth->is_switch_mode)
> >  		firmwares = icssg_switch_firmwares;
> > +	else if (prueth->is_hsr_offload_mode)
> > +		firmwares = icssg_hsr_firmwares;
> >  	else
> >  		firmwares = icssg_emac_firmwares;
> >  
> > @@ -726,6 +744,19 @@ static void emac_ndo_set_rx_mode(struct net_device *ndev)
> >  	queue_work(emac->cmd_wq, &emac->rx_mode_work);
> >  }
> >  
> > +static int emac_ndo_set_features(struct net_device *ndev,
> > +				 netdev_features_t features)
> > +{
> > +	netdev_features_t hsr_feature_present = ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
> > +	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
> > +	bool hsr_change_request = ((hsr_feature_wanted ^ hsr_feature_present) != 0);
> 
> This is quite hard to read for me.
> why not just do this instead?
> 
> 	netdev_features_t changed = netdev->features ^ feattures;
> 
> Then check and ack on individual features that you want to act upon.
> 
> 	if (changed & NETIF_F_HW_HSR_FWD) {
> 		if (features & NETIF_F_HW_HSR_FWD)
> 			/* enable HSR FWD feature */
> 		else
> 			/* disable HSR FWD feature */
> 	}
> 
> 	if (changed) {
> 		ndev->features = features;
> 		return 1;
> 	}
> 
> >From include/linux/netdevice.h
> 
>  * int (*ndo_set_features)(struct net_device *dev, netdev_features_t features);
>  *	Called to update device configuration to new features. Passed
>  *	feature set might be less than what was returned by ndo_fix_features()).
>  *	Must return >0 or -errno if it changed dev->features itself.
> 
> Can you please check that if we are not in dual emac mode then we should
> error out if any HSR feature is requested to be set.

This is where all the shenanigans with firmware makes things complex.

One of these options say 'If the interface is used for HSR, offload it
to hardware if possible'. You should be able to set this flag
anytime. It only has any effect when an interface is put into HSR
mode, or if it is already in HSR mode. Hence, the firmware running
right now should not matter.

I suspect the same is true for many of these flags.

> As you mentioned there are some contstraints on what HSR features can be
> enabled individually.
> "2) Inorder to enable hsr-tag-ins-offload, hsr-dup-offload
>    must also be enabled as these are tightly coupled in
>    the firmware implementation."
> You could do this check there by setting/clearing both features in tandem
> if either one was set/cleared.

Software HSR should always work. Offloading is generally thought as
accelerating this, if the hardware supports the current
configuration. When offloading, if the hardware cannot support the
current configuration, in general it should return -EOPNOTSUPP, and
the software will keep doing the work.

It is not particularly friendly, more of a documentation issue, but
the user needs to set the options the correct way for offload to
work. Otherwise it keeps chugging along in software. I would not
expect to see any error messages when offload is not possible.

	Andrew

Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Alexander Lobakin]

From: Md Danish Anwar <danishanwar@ti.com>
Date: Wed, 28 Aug 2024 14:48:58 +0530

> Add support for offloading HSR port-to-port frame forward to hardware.
> When the slave interfaces are added to the HSR interface, the PRU cores
> will be stopped and ICSSG HSR firmwares will be loaded to them.
> 
> Similarly, when HSR interface is deleted, the PRU cores will be stopped
> and dual EMAC firmware will be loaded to them.
> 
> This commit also renames some APIs that are common between switch and
> hsr mode with '_fw_offload' suffix.

[...]

> @@ -726,6 +744,19 @@ static void emac_ndo_set_rx_mode(struct net_device *ndev)
>  	queue_work(emac->cmd_wq, &emac->rx_mode_work);
>  }
>  
> +static int emac_ndo_set_features(struct net_device *ndev,
> +				 netdev_features_t features)
> +{
> +	netdev_features_t hsr_feature_present = ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;

Maybe you could give this definition and/or this variable shorter names,
so that you won't cross 80 cols?

> +	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;

(same)

> +	bool hsr_change_request = ((hsr_feature_wanted ^ hsr_feature_present) != 0);

You don't need to compare with zero. Just = a ^ b. Type `bool` takes
care of this.

> +
> +	if (hsr_change_request)
> +		ndev->features = features;

Does it mean you reject any feature change except HSR?

> +
> +	return 0;
> +}
> +
>  static const struct net_device_ops emac_netdev_ops = {
>  	.ndo_open = emac_ndo_open,
>  	.ndo_stop = emac_ndo_stop,
> @@ -737,6 +768,7 @@ static const struct net_device_ops emac_netdev_ops = {
>  	.ndo_eth_ioctl = icssg_ndo_ioctl,
>  	.ndo_get_stats64 = icssg_ndo_get_stats64,
>  	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
> +	.ndo_set_features = emac_ndo_set_features,
>  };
>  
>  static int prueth_netdev_init(struct prueth *prueth,
> @@ -865,6 +897,7 @@ static int prueth_netdev_init(struct prueth *prueth,
>  	ndev->ethtool_ops = &icssg_ethtool_ops;
>  	ndev->hw_features = NETIF_F_SG;
>  	ndev->features = ndev->hw_features;
> +	ndev->hw_features |= NETIF_F_HW_HSR_FWD;

Why not HSR_OFFLOAD right away, so that you wouldn't need to replace
this line with the mentioned def a commit later?

>  
>  	netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll);
>  	hrtimer_init(&emac->rx_hrtimer, CLOCK_MONOTONIC,

[...]

> +	prueth->hsr_members |= BIT(emac->port_id);
> +	if (!prueth->is_switch_mode && !prueth->is_hsr_offload_mode) {
> +		if (prueth->hsr_members & BIT(PRUETH_PORT_MII0) &&
> +		    prueth->hsr_members & BIT(PRUETH_PORT_MII1)) {
> +			if (!(emac0->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES) &&
> +			    !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES))
> +				return -EOPNOTSUPP;
> +			prueth->is_hsr_offload_mode = true;
> +			prueth->default_vlan = 1;
> +			emac0->port_vlan = prueth->default_vlan;
> +			emac1->port_vlan = prueth->default_vlan;
> +			icssg_change_mode(prueth);
> +			dev_dbg(prueth->dev, "Enabling HSR offload mode\n");

netdev_dbg()?

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void prueth_hsr_port_unlink(struct net_device *ndev)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth *prueth = emac->prueth;
> +	struct prueth_emac *emac0;
> +	struct prueth_emac *emac1;
> +
> +	emac0 = prueth->emac[PRUETH_MAC0];
> +	emac1 = prueth->emac[PRUETH_MAC1];
> +
> +	prueth->hsr_members &= ~BIT(emac->port_id);
> +	if (prueth->is_hsr_offload_mode) {
> +		prueth->is_hsr_offload_mode = false;
> +		emac0->port_vlan = 0;
> +		emac1->port_vlan = 0;
> +		prueth->hsr_dev = NULL;
> +		prueth_emac_restart(prueth);
> +		dev_dbg(prueth->dev, "Enabling Dual EMAC mode\n");

(same here and in all the places below)

> +	}
> +}

Thanks,
Olek

Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Anwar, Md Danish]

Hi Roger,

On 8/30/2024 6:57 PM, Roger Quadros wrote:
> 
> 
> On 28/08/2024 12:18, MD Danish Anwar wrote:
>> Add support for offloading HSR port-to-port frame forward to hardware.
>> When the slave interfaces are added to the HSR interface, the PRU cores
>> will be stopped and ICSSG HSR firmwares will be loaded to them.
>>
>> Similarly, when HSR interface is deleted, the PRU cores will be stopped
>> and dual EMAC firmware will be loaded to them.
> 
> And what happens if we first started with switch mode and then switched to HSR mode?
> Is this case possible and if yes should it revert to the last used mode
> instead of forcing to dual EMAC mode?
> 

That is not supported by the firmware. In prueth_hsr_port_link()

+	if (prueth->is_switch_mode)
+		return -EOPNOTSUPP;

We will not change the prueth->mode or the firmware settings, driver
will return -EOPNOTSUPP and the offloading will happen in software as
suggested by Andrew L in v2 [1] . HW offloading is only supported from
dual EMAC mode to HSR.

Perhaps the commit message here is misleading, I will update the commit
message.

>>
>> This commit also renames some APIs that are common between switch and
>> hsr mode with '_fw_offload' suffix.
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  .../net/ethernet/ti/icssg/icssg_classifier.c  |   1 +
>>  drivers/net/ethernet/ti/icssg/icssg_config.c  |  18 +--
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 117 +++++++++++++++++-
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   6 +
>>  4 files changed, 130 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_classifier.c b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
>> index 9ec504d976d6..833ca86d0b71 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_classifier.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
>> @@ -290,6 +290,7 @@ void icssg_class_set_host_mac_addr(struct regmap *miig_rt, const u8 *mac)
>>  		     mac[2] << 16 | mac[3] << 24));
>>  	regmap_write(miig_rt, MAC_INTERFACE_1, (u32)(mac[4] | mac[5] << 8));
>>  }
>> +EXPORT_SYMBOL_GPL(icssg_class_set_host_mac_addr);
>>  
>>  void icssg_class_set_mac_addr(struct regmap *miig_rt, int slice, u8 *mac)
>>  {
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> index dae52a83a378..7b2e6c192ff3 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> @@ -107,7 +107,7 @@ static const struct map hwq_map[2][ICSSG_NUM_OTHER_QUEUES] = {
>>  	},
>>  };
>>  
>> -static void icssg_config_mii_init_switch(struct prueth_emac *emac)
>> +static void icssg_config_mii_init_fw_offload(struct prueth_emac *emac)
>>  {
>>  	struct prueth *prueth = emac->prueth;
>>  	int mii = prueth_emac_slice(emac);
>> @@ -278,7 +278,7 @@ static int emac_r30_is_done(struct prueth_emac *emac)
>>  	return 1;
>>  }
>>  
>> -static int prueth_switch_buffer_setup(struct prueth_emac *emac)
>> +static int prueth_fw_offload_buffer_setup(struct prueth_emac *emac)
>>  {
>>  	struct icssg_buffer_pool_cfg __iomem *bpool_cfg;
>>  	struct icssg_rxq_ctx __iomem *rxq_ctx;
>> @@ -424,7 +424,7 @@ static void icssg_init_emac_mode(struct prueth *prueth)
>>  	icssg_class_set_host_mac_addr(prueth->miig_rt, mac);
>>  }
>>  
>> -static void icssg_init_switch_mode(struct prueth *prueth)
>> +static void icssg_init_fw_offload_mode(struct prueth *prueth)
>>  {
>>  	u32 addr = prueth->shram.pa + EMAC_ICSSG_SWITCH_DEFAULT_VLAN_TABLE_OFFSET;
>>  	int i;
>> @@ -455,8 +455,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
>>  	struct icssg_flow_cfg __iomem *flow_cfg;
>>  	int ret;
>>  
>> -	if (prueth->is_switch_mode)
>> -		icssg_init_switch_mode(prueth);
>> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
>> +		icssg_init_fw_offload_mode(prueth);
>>  	else
>>  		icssg_init_emac_mode(prueth);
>>  
>> @@ -472,8 +472,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
>>  	regmap_update_bits(prueth->miig_rt, ICSSG_CFG_OFFSET,
>>  			   ICSSG_CFG_DEFAULT, ICSSG_CFG_DEFAULT);
>>  	icssg_miig_set_interface_mode(prueth->miig_rt, slice, emac->phy_if);
>> -	if (prueth->is_switch_mode)
>> -		icssg_config_mii_init_switch(emac);
>> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
>> +		icssg_config_mii_init_fw_offload(emac);
>>  	else
>>  		icssg_config_mii_init(emac);
>>  	icssg_config_ipg(emac);
>> @@ -498,8 +498,8 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac, int slice)
>>  	writeb(0, config + SPL_PKT_DEFAULT_PRIORITY);
>>  	writeb(0, config + QUEUE_NUM_UNTAGGED);
>>  
>> -	if (prueth->is_switch_mode)
>> -		ret = prueth_switch_buffer_setup(emac);
>> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
>> +		ret = prueth_fw_offload_buffer_setup(emac);
>>  	else
>>  		ret = prueth_emac_buffer_setup(emac);
>>  	if (ret)
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index 641e54849762..f4fd346fe6f5 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/dma/ti-cppi5.h>
>>  #include <linux/etherdevice.h>
>>  #include <linux/genalloc.h>
>> +#include <linux/if_hsr.h>
>>  #include <linux/if_vlan.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/kernel.h>
>> @@ -40,6 +41,8 @@
>>  #define DEFAULT_PORT_MASK	1
>>  #define DEFAULT_UNTAG_MASK	1
>>  
>> +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
>> +
>>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>>  
>> @@ -118,6 +121,19 @@ static irqreturn_t prueth_tx_ts_irq(int irq, void *dev_id)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static struct icssg_firmwares icssg_hsr_firmwares[] = {
>> +	{
>> +		.pru = "ti-pruss/am65x-sr2-pru0-pruhsr-fw.elf",
>> +		.rtu = "ti-pruss/am65x-sr2-rtu0-pruhsr-fw.elf",
>> +		.txpru = "ti-pruss/am65x-sr2-txpru0-pruhsr-fw.elf",
>> +	},
>> +	{
>> +		.pru = "ti-pruss/am65x-sr2-pru1-pruhsr-fw.elf",
>> +		.rtu = "ti-pruss/am65x-sr2-rtu1-pruhsr-fw.elf",
>> +		.txpru = "ti-pruss/am65x-sr2-txpru1-pruhsr-fw.elf",
>> +	}
>> +};
>> +
>>  static struct icssg_firmwares icssg_switch_firmwares[] = {
>>  	{
>>  		.pru = "ti-pruss/am65x-sr2-pru0-prusw-fw.elf",
>> @@ -152,6 +168,8 @@ static int prueth_emac_start(struct prueth *prueth, struct prueth_emac *emac)
>>  
>>  	if (prueth->is_switch_mode)
>>  		firmwares = icssg_switch_firmwares;
>> +	else if (prueth->is_hsr_offload_mode)
>> +		firmwares = icssg_hsr_firmwares;
>>  	else
>>  		firmwares = icssg_emac_firmwares;
>>  
>> @@ -726,6 +744,19 @@ static void emac_ndo_set_rx_mode(struct net_device *ndev)
>>  	queue_work(emac->cmd_wq, &emac->rx_mode_work);
>>  }
>>  
>> +static int emac_ndo_set_features(struct net_device *ndev,
>> +				 netdev_features_t features)
>> +{
>> +	netdev_features_t hsr_feature_present = ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
>> +	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
>> +	bool hsr_change_request = ((hsr_feature_wanted ^ hsr_feature_present) != 0);
> 
> This is quite hard to read for me.
> why not just do this instead?
> 
> 	netdev_features_t changed = netdev->features ^ feattures;
> 
> Then check and ack on individual features that you want to act upon.
> 

Sure Roger, I will modfiy this function and implement it as you have
suggested below.

> 	if (changed & NETIF_F_HW_HSR_FWD) {
> 		if (features & NETIF_F_HW_HSR_FWD)
> 			/* enable HSR FWD feature */
> 		else
> 			/* disable HSR FWD feature */
> 	}
> 

Roger, there is no action needed here. In this function we should just
set / unset features related to HSR in ndev->features. We don't need to
do any action in if / else. Based on which HSR features are set / unset,
the different APIs will perform different actions. Perhaps we can do
something like below,

	if (changed & NETIF_F_HW_HSR_FWD)
		if (features & NETIF_F_HW_HSR_FWD)
			ndev->features |= NETIF_F_HW_HSR_FWD;
		else
			ndev->features &= ~NETIF_F_HW_HSR_FWD;

We can do this for all HSR related features. This will also elimnate
below if check and we can just `return changed`.

> 	if (changed) {
> 		ndev->features = features;
> 		return 1;
> 	}
> 
> From include/linux/netdevice.h
> 
>  * int (*ndo_set_features)(struct net_device *dev, netdev_features_t features);
>  *	Called to update device configuration to new features. Passed
>  *	feature set might be less than what was returned by ndo_fix_features()).
>  *	Must return >0 or -errno if it changed dev->features itself.
> 
> Can you please check that if we are not in dual emac mode then we should
> error out if any HSR feature is requested to be set.
> 

Yes, the ndev->features should only be changed when we are in dual EMAC
mode / HSR offload mode. If we are in swicth mode these features
shouldn't be changed as hardware offloading is not supported in switch mode.

>> +
>> +	if (hsr_change_request)
>> +		ndev->features = features;
>> +
>> +	return 0;
> 
> 
> You may also want to check out
> 
>  * netdev_features_t (*ndo_fix_features)(struct net_device *dev,
>  *		netdev_features_t features);
>  *	Adjusts the requested feature flags according to device-specific
>  *	constraints, and returns the resulting flags. Must not modify
>  *	the device state.
> 
> As you mentioned there are some contstraints on what HSR features can be
> enabled individually.
> "2) Inorder to enable hsr-tag-ins-offload, hsr-dup-offload
>    must also be enabled as these are tightly coupled in
>    the firmware implementation."
> You could do this check there by setting/clearing both features in tandem
> if either one was set/cleared.
> 

Sure I will look into that.

>> +}
>> +
>>  static const struct net_device_ops emac_netdev_ops = {
>>  	.ndo_open = emac_ndo_open,
>>  	.ndo_stop = emac_ndo_stop,
>> @@ -737,6 +768,7 @@ static const struct net_device_ops emac_netdev_ops = {
>>  	.ndo_eth_ioctl = icssg_ndo_ioctl,
>>  	.ndo_get_stats64 = icssg_ndo_get_stats64,
>>  	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
>> +	.ndo_set_features = emac_ndo_set_features,
>>  };
>>  
>>  static int prueth_netdev_init(struct prueth *prueth,
>> @@ -865,6 +897,7 @@ static int prueth_netdev_init(struct prueth *prueth,
>>  	ndev->ethtool_ops = &icssg_ethtool_ops;
>>  	ndev->hw_features = NETIF_F_SG;
>>  	ndev->features = ndev->hw_features;
>> +	ndev->hw_features |= NETIF_F_HW_HSR_FWD;
>>  
>>  	netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll);
>>  	hrtimer_init(&emac->rx_hrtimer, CLOCK_MONOTONIC,
>> @@ -953,7 +986,7 @@ static void prueth_emac_restart(struct prueth *prueth)
>>  	netif_device_attach(emac1->ndev);
>>  }
>>  
>> -static void icssg_enable_switch_mode(struct prueth *prueth)
>> +static void icssg_change_mode(struct prueth *prueth)
>>  {
>>  	struct prueth_emac *emac;
>>  	int mac;
>> @@ -973,8 +1006,12 @@ static void icssg_enable_switch_mode(struct prueth *prueth)
>>  					  BIT(emac->port_id) | DEFAULT_PORT_MASK,
>>  					  BIT(emac->port_id) | DEFAULT_UNTAG_MASK,
>>  					  true);
>> +			if (prueth->is_hsr_offload_mode)
>> +				icssg_vtbl_modify(emac, DEFAULT_VID, DEFAULT_PORT_MASK,
>> +						  DEFAULT_UNTAG_MASK, true);
>>  			icssg_set_pvid(prueth, emac->port_vlan, emac->port_id);
>> -			icssg_set_port_state(emac, ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE);
>> +			if (prueth->is_switch_mode)
>> +				icssg_set_port_state(emac, ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE);
>>  		}
>>  	}
>>  }
>> @@ -1012,7 +1049,7 @@ static int prueth_netdevice_port_link(struct net_device *ndev,
>>  			prueth->is_switch_mode = true;
>>  			prueth->default_vlan = 1;
>>  			emac->port_vlan = prueth->default_vlan;
>> -			icssg_enable_switch_mode(prueth);
>> +			icssg_change_mode(prueth);
>>  		}
>>  	}
>>  
>> @@ -1040,6 +1077,59 @@ static void prueth_netdevice_port_unlink(struct net_device *ndev)
>>  		prueth->hw_bridge_dev = NULL;
>>  }
>>  
>> +static int prueth_hsr_port_link(struct net_device *ndev)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth *prueth = emac->prueth;
>> +	struct prueth_emac *emac0;
>> +	struct prueth_emac *emac1;
>> +
>> +	emac0 = prueth->emac[PRUETH_MAC0];
>> +	emac1 = prueth->emac[PRUETH_MAC1];
>> +
>> +	if (prueth->is_switch_mode)
>> +		return -EOPNOTSUPP;
>> +
>> +	prueth->hsr_members |= BIT(emac->port_id);
>> +	if (!prueth->is_switch_mode && !prueth->is_hsr_offload_mode) {
> 
> you already checked that is_switch_mode is not set earlier. No need to check again.
> 

Sure. Will remove this.

>> +		if (prueth->hsr_members & BIT(PRUETH_PORT_MII0) &&
>> +		    prueth->hsr_members & BIT(PRUETH_PORT_MII1)) {
>> +			if (!(emac0->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES) &&
>> +			    !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES))
>> +				return -EOPNOTSUPP;
>> +			prueth->is_hsr_offload_mode = true;
>> +			prueth->default_vlan = 1;
>> +			emac0->port_vlan = prueth->default_vlan;
>> +			emac1->port_vlan = prueth->default_vlan;
>> +			icssg_change_mode(prueth);
>> +			dev_dbg(prueth->dev, "Enabling HSR offload mode\n");
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void prueth_hsr_port_unlink(struct net_device *ndev)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth *prueth = emac->prueth;
>> +	struct prueth_emac *emac0;
>> +	struct prueth_emac *emac1;
>> +
>> +	emac0 = prueth->emac[PRUETH_MAC0];
>> +	emac1 = prueth->emac[PRUETH_MAC1];
>> +
>> +	prueth->hsr_members &= ~BIT(emac->port_id);
>> +	if (prueth->is_hsr_offload_mode) {
>> +		prueth->is_hsr_offload_mode = false;
>> +		emac0->port_vlan = 0;
>> +		emac1->port_vlan = 0;
>> +		prueth->hsr_dev = NULL;
>> +		prueth_emac_restart(prueth);
>> +		dev_dbg(prueth->dev, "Enabling Dual EMAC mode\n");
>> +	}
>> +}
>> +

[...]

[1]
https://lore.kernel.org/all/082f81fc-c9ad-40d7-8172-440765350b48@lunn.ch/

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v3 4/6] net: ti: icssg-prueth: Enable HSR Tx Packet duplication offload

[Anwar, Md Danish]

On 8/30/2024 7:00 PM, Roger Quadros wrote:
> 
> 
> On 28/08/2024 12:18, MD Danish Anwar wrote:
>> From: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>
>> The HSR stack allows to offload its Tx packet duplication functionality to
>> the hardware. Enable this offloading feature for ICSSG driver
>>
>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  drivers/net/ethernet/ti/icssg/icssg_common.c | 13 ++++++++++---
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c |  5 +++--
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 ++
>>  3 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
>> index b9d8a93d1680..2d6d8648f5a9 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
>> @@ -660,14 +660,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>  {
>>  	struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
>>  	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth *prueth = emac->prueth;
>>  	struct netdev_queue *netif_txq;
>>  	struct prueth_tx_chn *tx_chn;
>>  	dma_addr_t desc_dma, buf_dma;
>> +	u32 pkt_len, dst_tag_id;
>>  	int i, ret = 0, q_idx;
>>  	bool in_tx_ts = 0;
>>  	int tx_ts_cookie;
>>  	void **swdata;
>> -	u32 pkt_len;
>>  	u32 *epib;
>>  
>>  	pkt_len = skb_headlen(skb);
>> @@ -712,9 +713,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>  
>>  	/* set dst tag to indicate internal qid at the firmware which is at
>>  	 * bit8..bit15. bit0..bit7 indicates port num for directed
>> -	 * packets in case of switch mode operation
>> +	 * packets in case of switch mode operation and port num 0
>> +	 * for undirected packets in case of HSR offload mode
>>  	 */
>> -	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
>> +	dst_tag_id = emac->port_id | (q_idx << 8);
>> +
>> +	if (prueth->is_hsr_offload_mode && (ndev->features & NETIF_F_HW_HSR_DUP))
>> +		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
>> +
>> +	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
>>  	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>>  	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
>>  	swdata = cppi5_hdesc_get_swdata(first_desc);
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index f4fd346fe6f5..b60efe7bd7a7 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -41,7 +41,8 @@
>>  #define DEFAULT_PORT_MASK	1
>>  #define DEFAULT_UNTAG_MASK	1
>>  
>> -#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
>> +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
>> +						 NETIF_F_HW_HSR_DUP)
> 
> You mentioned that these 2 features can't be enabled individually.
> 

Not these two but NETIF_F_HW_HSR_TAG_INS and NETIF_F_HW_HSR_DUP needs to
be enabled together. NETIF_F_HW_HSR_TAG_INS is added in patch 6/6.

2) Inorder to enable hsr-tag-ins-offload, hsr-dup-offload
   must also be enabled as these are tightly coupled in
   the firmware implementation.


> So better to squash this with previous patch and use ndo_fix_features() to make sure both
> are set or cleared together.
> 

I will squash patch 6/6 with this patch so that both are added in the
same patch. I will use ndo_fix_features() to make sure they re set /
unset together. I will add this also in this patch. Let me know if this
sounds good to you.

>>  
>>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>> @@ -897,7 +898,7 @@ static int prueth_netdev_init(struct prueth *prueth,
>>  	ndev->ethtool_ops = &icssg_ethtool_ops;
>>  	ndev->hw_features = NETIF_F_SG;
>>  	ndev->features = ndev->hw_features;
>> -	ndev->hw_features |= NETIF_F_HW_HSR_FWD;
>> +	ndev->hw_features |= NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
>>  
>>  	netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll);
>>  	hrtimer_init(&emac->rx_hrtimer, CLOCK_MONOTONIC,
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> index a4b025fae797..e110a5f92684 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -59,6 +59,8 @@
>>  
>>  #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
>>  
>> +#define PRUETH_UNDIRECTED_PKT_DST_TAG	0
>> +
>>  /* Firmware status codes */
>>  #define ICSS_HS_FW_READY 0x55555555
>>  #define ICSS_HS_FW_DEAD 0xDEAD0000	/* lower 16 bits contain error code */
> 

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Anwar, Md Danish]

On 8/30/2024 7:30 PM, Andrew Lunn wrote:
> On Fri, Aug 30, 2024 at 04:27:34PM +0300, Roger Quadros wrote:
>>
>>
>> On 28/08/2024 12:18, MD Danish Anwar wrote:
>>> Add support for offloading HSR port-to-port frame forward to hardware.
>>> When the slave interfaces are added to the HSR interface, the PRU cores
>>> will be stopped and ICSSG HSR firmwares will be loaded to them.
>>>
>>> Similarly, when HSR interface is deleted, the PRU cores will be stopped
>>> and dual EMAC firmware will be loaded to them.
>>
>> And what happens if we first started with switch mode and then switched to HSR mode?
>> Is this case possible and if yes should it revert to the last used mode
>> instead of forcing to dual EMAC mode?
>>
>>>
>>> This commit also renames some APIs that are common between switch and
>>> hsr mode with '_fw_offload' suffix.
>>>
>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>> ---

[...]

>>
>> Can you please check that if we are not in dual emac mode then we should
>> error out if any HSR feature is requested to be set.
> 
> This is where all the shenanigans with firmware makes things complex.
> 
> One of these options say 'If the interface is used for HSR, offload it
> to hardware if possible'. You should be able to set this flag
> anytime. It only has any effect when an interface is put into HSR
> mode, or if it is already in HSR mode. Hence, the firmware running
> right now should not matter.
> 
> I suspect the same is true for many of these flags.
> 

Andrew that is correct. Ideally the current running firmware should not
matter. But the firmware team only recommends dual EMAC -> HSR offload
and vise versa transitions. They suspect some configuration issues when
switch -> HSR transition happen. That is why they have recommended not
to do HSR hw offloading from switch mode. We can however keep doing SW
offload as suggested by you in v2.

>> As you mentioned there are some contstraints on what HSR features can be
>> enabled individually.
>> "2) Inorder to enable hsr-tag-ins-offload, hsr-dup-offload
>>    must also be enabled as these are tightly coupled in
>>    the firmware implementation."
>> You could do this check there by setting/clearing both features in tandem
>> if either one was set/cleared.
> 
> Software HSR should always work. Offloading is generally thought as
> accelerating this, if the hardware supports the current
> configuration. When offloading, if the hardware cannot support the
> current configuration, in general it should return -EOPNOTSUPP, and
> the software will keep doing the work.
> 
> It is not particularly friendly, more of a documentation issue, but
> the user needs to set the options the correct way for offload to
> work. Otherwise it keeps chugging along in software. I would not
> expect to see any error messages when offload is not possible.
> 

Yes, and I have already added this in this series based on your feedback
on v2.

I have one question though, in emac_ndo_set_features() should I change
these HSR related features irrespective of the current mode?

AFAIK, if NETIF_F_HW_HSR_FWD is set, the forwarding is offloaded to HW.
If NETIF_F_HW_HSR_FWD is not set the forwarding is not offloaded to HW
and is done in SW.

So, I don't see any need to enable this features if we are currently in
switch mode. Let me know what do you think. Should I still enable this
feature irrespective of current mode and later handle this in
prueth_hsr_port_link / unlink()?

> 	Andrew

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Andrew Lunn]

> Yes, and I have already added this in this series based on your feedback
> on v2.
> 
> I have one question though, in emac_ndo_set_features() should I change
> these HSR related features irrespective of the current mode?
> 
> AFAIK, if NETIF_F_HW_HSR_FWD is set, the forwarding is offloaded to HW.
> If NETIF_F_HW_HSR_FWD is not set the forwarding is not offloaded to HW
> and is done in SW.
> 
> So, I don't see any need to enable this features if we are currently in
> switch mode. Let me know what do you think. Should I still enable this
> feature irrespective of current mode and later handle this in
> prueth_hsr_port_link / unlink()?

The user should not need to know about the different firmwares. So i
would allow NETIF_F_HW_HSR_FWD at any time.

The exception would be, if you look at all the other drivers which
implement HSR offload, if they all return an error if the offloading
cannot be enabled, then you should do the same.

	Andrew

Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[MD Danish Anwar]

On 02/09/24 6:32 pm, Andrew Lunn wrote:
>> Yes, and I have already added this in this series based on your feedback
>> on v2.
>>
>> I have one question though, in emac_ndo_set_features() should I change
>> these HSR related features irrespective of the current mode?
>>
>> AFAIK, if NETIF_F_HW_HSR_FWD is set, the forwarding is offloaded to HW.
>> If NETIF_F_HW_HSR_FWD is not set the forwarding is not offloaded to HW
>> and is done in SW.
>>
>> So, I don't see any need to enable this features if we are currently in
>> switch mode. Let me know what do you think. Should I still enable this
>> feature irrespective of current mode and later handle this in
>> prueth_hsr_port_link / unlink()?
> 
> The user should not need to know about the different firmwares. So i
> would allow NETIF_F_HW_HSR_FWD at any time.
> 
> The exception would be, if you look at all the other drivers which
> implement HSR offload, if they all return an error if the offloading
> cannot be enabled, then you should do the same.
> 

Andrew, I looked at xrs700x dsa and microchip ksz dsa driver as an
example. The drivers return -EOPNOTSUPP whenever offloading is not
possible in the xrs700x_hsr_join / ksz_hsr_join api. I think the same
should be okay for ICSSG driver as well. ICSSG drivers equivalent API is
prueth_hsr_port_link() where I will return -EOPNOTSUPP whenever
offloading is not possible.

So in the .ndo_set_features() I will not add any check so that the user
wouldn't know about the current firmware and whether offloading is
supported or not. In the prueth_hsr_port_link() based on firmware
limitation we will error out.

Andrew, Roger, Let me know if this sounds good.

> 	Andrew

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

[MD Danish Anwar]

Hi Alexander

On 30/08/24 7:37 pm, Alexander Lobakin wrote:
> From: Md Danish Anwar <danishanwar@ti.com>
> Date: Wed, 28 Aug 2024 14:48:58 +0530
> 
>> Add support for offloading HSR port-to-port frame forward to hardware.
>> When the slave interfaces are added to the HSR interface, the PRU cores
>> will be stopped and ICSSG HSR firmwares will be loaded to them.
>>
>> Similarly, when HSR interface is deleted, the PRU cores will be stopped
>> and dual EMAC firmware will be loaded to them.
>>
>> This commit also renames some APIs that are common between switch and
>> hsr mode with '_fw_offload' suffix.
> 
> [...]
> 
>> @@ -726,6 +744,19 @@ static void emac_ndo_set_rx_mode(struct net_device *ndev)
>>  	queue_work(emac->cmd_wq, &emac->rx_mode_work);
>>  }
>>  
>> +static int emac_ndo_set_features(struct net_device *ndev,
>> +				 netdev_features_t features)
>> +{
>> +	netdev_features_t hsr_feature_present = ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
> 
> Maybe you could give this definition and/or this variable shorter names,
> so that you won't cross 80 cols?
> 

I will use Roger's feedback here [1] and modify this API. This will
contain this line within 80 cols.

I will  try to containig line length within 80 cols wherever possible.
There are however some instances where line length of 80 cols is not
possible. There the line length will exceed.

>> +	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
> 
> (same)
> 
>> +	bool hsr_change_request = ((hsr_feature_wanted ^ hsr_feature_present) != 0);
> 
> You don't need to compare with zero. Just = a ^ b. Type `bool` takes
> care of this.
> 

Sure.

>> +
>> +	if (hsr_change_request)
>> +		ndev->features = features;
> 
> Does it mean you reject any feature change except HSR?
> 

Currently only HSR features are supported. So we will modify
ndev->features only for HSR features request. For any other feature
request ndev->features will not be modified.

>> +
>> +	return 0;
>> +}
>> +
>>  static const struct net_device_ops emac_netdev_ops = {
>>  	.ndo_open = emac_ndo_open,
>>  	.ndo_stop = emac_ndo_stop,
>> @@ -737,6 +768,7 @@ static const struct net_device_ops emac_netdev_ops = {
>>  	.ndo_eth_ioctl = icssg_ndo_ioctl,
>>  	.ndo_get_stats64 = icssg_ndo_get_stats64,
>>  	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
>> +	.ndo_set_features = emac_ndo_set_features,
>>  };
>>  
>>  static int prueth_netdev_init(struct prueth *prueth,
>> @@ -865,6 +897,7 @@ static int prueth_netdev_init(struct prueth *prueth,
>>  	ndev->ethtool_ops = &icssg_ethtool_ops;
>>  	ndev->hw_features = NETIF_F_SG;
>>  	ndev->features = ndev->hw_features;
>> +	ndev->hw_features |= NETIF_F_HW_HSR_FWD;
> 
> Why not HSR_OFFLOAD right away, so that you wouldn't need to replace
> this line with the mentioned def a commit later?
> 

Sure Will do that.

>>  
>>  	netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll);
>>  	hrtimer_init(&emac->rx_hrtimer, CLOCK_MONOTONIC,
> 
> [...]
> 
>> +	prueth->hsr_members |= BIT(emac->port_id);
>> +	if (!prueth->is_switch_mode && !prueth->is_hsr_offload_mode) {
>> +		if (prueth->hsr_members & BIT(PRUETH_PORT_MII0) &&
>> +		    prueth->hsr_members & BIT(PRUETH_PORT_MII1)) {
>> +			if (!(emac0->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES) &&
>> +			    !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD_FEATURES))
>> +				return -EOPNOTSUPP;
>> +			prueth->is_hsr_offload_mode = true;
>> +			prueth->default_vlan = 1;
>> +			emac0->port_vlan = prueth->default_vlan;
>> +			emac1->port_vlan = prueth->default_vlan;
>> +			icssg_change_mode(prueth);
>> +			dev_dbg(prueth->dev, "Enabling HSR offload mode\n");
> 
> netdev_dbg()?
> 
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void prueth_hsr_port_unlink(struct net_device *ndev)
>> +{
>> +	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth *prueth = emac->prueth;
>> +	struct prueth_emac *emac0;
>> +	struct prueth_emac *emac1;
>> +
>> +	emac0 = prueth->emac[PRUETH_MAC0];
>> +	emac1 = prueth->emac[PRUETH_MAC1];
>> +
>> +	prueth->hsr_members &= ~BIT(emac->port_id);
>> +	if (prueth->is_hsr_offload_mode) {
>> +		prueth->is_hsr_offload_mode = false;
>> +		emac0->port_vlan = 0;
>> +		emac1->port_vlan = 0;
>> +		prueth->hsr_dev = NULL;
>> +		prueth_emac_restart(prueth);
>> +		dev_dbg(prueth->dev, "Enabling Dual EMAC mode\n");
> 
> (same here and in all the places below)
> 

Sure will use netdev_dbg instead of dev_dbg.

>> +	}
>> +}
> 
> Thanks,
> Olek


[1]
https://lore.kernel.org/all/22f5442b-62e6-42d0-8bf8-163d2c4ea4bd@kernel.org/

-- 
Thanks and Regards,
Danish