[PATCH net-next v2 0/7] Introduce HSR offload support for ICSSG

[PATCH net-next v2 0/7] 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 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.

MD Danish Anwar (5):
  net: ti: icssg-prueth: Enable IEP1
  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  |  10 +-
 drivers/net/ethernet/ti/icssg/icssg_config.h  |   2 +
 drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 187 ++++++++++++++++--
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   7 +
 8 files changed, 273 insertions(+), 95 deletions(-)


base-commit: 0a3e6939d4b33d68bce89477b9db01d68b744749
-- 
2.34.1

[PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[MD Danish Anwar]

IEP1 is needed by firmware to enable FDB learning and FDB ageing.
Always enable IEP1

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

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 53a3e44b99a2..613bd8de6eb8 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -1256,12 +1256,8 @@ static int prueth_probe(struct platform_device *pdev)
 		goto put_iep0;
 	}
 
-	if (prueth->pdata.quirk_10m_link_issue) {
-		/* Enable IEP1 for FW in 64bit mode as W/A for 10M FD link detect issue under TX
-		 * traffic.
-		 */
-		icss_iep_init_fw(prueth->iep1);
-	}
+	/* Enable IEP1 for FW as it's needed by FW for FDB Learning and FDB ageing */
+	icss_iep_init_fw(prueth->iep1);
 
 	/* setup netdev interfaces */
 	if (eth0_node) {
@@ -1366,8 +1362,7 @@ static int prueth_probe(struct platform_device *pdev)
 	}
 
 exit_iep:
-	if (prueth->pdata.quirk_10m_link_issue)
-		icss_iep_exit_fw(prueth->iep1);
+	icss_iep_exit_fw(prueth->iep1);
 	icss_iep_put(prueth->iep1);
 
 put_iep0:
@@ -1424,8 +1419,7 @@ static void prueth_remove(struct platform_device *pdev)
 		prueth_netdev_exit(prueth, eth_node);
 	}
 
-	if (prueth->pdata.quirk_10m_link_issue)
-		icss_iep_exit_fw(prueth->iep1);
+	icss_iep_exit_fw(prueth->iep1);
 
 	icss_iep_put(prueth->iep1);
 	icss_iep_put(prueth->iep0);
-- 
2.34.1

[PATCH net-next v2 2/7] 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>
---
 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 v2 3/7] 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>
---
 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 613bd8de6eb8..c93071e05c37 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 v2 4/7] 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.

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  |   6 +-
 drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 122 +++++++++++++++++-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h  |   4 +
 4 files changed, 127 insertions(+), 6 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..2f485318c940 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
@@ -455,7 +455,7 @@ 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)
+	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
 		icssg_init_switch_mode(prueth);
 	else
 		icssg_init_emac_mode(prueth);
@@ -472,7 +472,7 @@ 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)
+	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
 		icssg_config_mii_init_switch(emac);
 	else
 		icssg_config_mii_init(emac);
@@ -498,7 +498,7 @@ 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)
+	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
 		ret = prueth_switch_buffer_setup(emac);
 	else
 		ret = prueth_emac_buffer_setup(emac);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index c93071e05c37..142e267ff136 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	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;
+	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD;
+	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,63 @@ 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) {
+		dev_err(prueth->dev, "Switching from bridge to HSR mode not allowed\n");
+		return -EINVAL;
+	}
+
+	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) &&
+			    !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD)) {
+				dev_err(prueth->dev, "Enable HSR offload on both interfaces\n");
+				return -EINVAL;
+			}
+			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_err(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_info(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 +1141,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 +1152,26 @@ static int prueth_netdevice_event(struct notifier_block *unused,
 	case NETDEV_CHANGEUPPER:
 		info = ptr;
 
+		if ((ndev->features & NETIF_PRUETH_HSR_OFFLOAD) &&
+		    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_err(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 f678d656a3ed..40bc3912b6ae 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -239,6 +239,7 @@ 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
  * @prueth_netdevice_nb: netdevice notifier block
  * @prueth_switchdev_nb: switchdev notifier block
@@ -274,11 +275,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 v2 5/7] 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 142e267ff136..b32a2bff34dc 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	NETIF_F_HW_HSR_FWD
+#define NETIF_PRUETH_HSR_OFFLOAD	(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;
 
 	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 40bc3912b6ae..6cb1dce8b309 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -57,6 +57,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 v2 6/7] 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 | 38 +++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index b32a2bff34dc..521e9f914459 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
@@ -651,6 +681,7 @@ 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);
+	__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
 
 	atomic_set(&emac->tdown_cnt, emac->tx_ch_num);
 	/* ensure new tdown_cnt value is visible */
@@ -728,7 +759,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 v2 7/7] 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.

Signed-off-by: Ravi Gunasekaran <r-gunasekaran@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 2f485318c940..f061fa97a377 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 521e9f914459..582e72dd8f3f 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	(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)
@@ -1032,6 +1034,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 6cb1dce8b309..246f1e41c13a 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -58,6 +58,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 v2 3/7] net: ti: icssg-prueth: Stop hardcoding def_inc

[Dan Carpenter]

On Tue, Aug 13, 2024 at 01:12:29PM +0530, MD Danish Anwar wrote:
> 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>
> ---

Thanks for breaking this out into a separate patch and removing the unused
spinlock.  I don't necessarily feel qualified to review this patchset but I
didn't have any other issues with it.

regards,
dan carpenter

Re: [PATCH net-next v2 0/7] Introduce HSR offload support for ICSSG

[Andrew Lunn]

On Tue, Aug 13, 2024 at 01:12:26PM +0530, MD Danish Anwar wrote:
> Hi All,
> This series introduces HSR offload support for ICSSG driver. To support HSR
> offload to hardware, ICSSG HSR firmware is used.

Oh, no, not another firmware. How does this interact with using the
switch firmware and switchdev? I see in your examples you talk about
HSR to Dual EMAC, but what about HSR and Switchdev?

How many more different firmwares do you have?

	Andrew

Re: [PATCH net-next v2 4/7] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Andrew Lunn]

On Tue, Aug 13, 2024 at 01:12:30PM +0530, 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.

Maybe a dumb question, because i don't know HSR....

Can you have one interface in a HSR network, another interface in a
non-HSR network, and bridge packets between the two worlds? Do you
want the HSR firmware, the Switchdev firmware, or Dual EMAC and do the
bridge in software?

>  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..2f485318c940 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
> @@ -455,7 +455,7 @@ 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)
> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
>  		icssg_init_switch_mode(prueth);

Maybe icssg_init_switch_mode() needs renaming if it is used for more
than switch mode? There are other functions which might need
generalising.

> +#define NETIF_PRUETH_HSR_OFFLOAD	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;

Documentation/networking/netdev-features.rst

* hsr-fwd-offload

This should be set for devices which forward HSR (High-availability Seamless
Redundancy) frames from one port to another in hardware.

To me, this suggests if the flag is not set, you should keep in dual
EMACS or switchdev mode and perform HSR in software.

> +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;
> +	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD;

I would not add the _PRUETH_ alias. There is nothing _PRUETH_ specific
here, its just plain HSR offload.

> +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) {
> +		dev_err(prueth->dev, "Switching from bridge to HSR mode not allowed\n");
> +		return -EINVAL;

I think you want EOPNOTSUPP, so that it is performed in software, not
offloaded to hardware. And this is not an error condition, it is just
a limitation of your hardware/firmware.

> +	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) &&
> +			    !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD)) {
> +				dev_err(prueth->dev, "Enable HSR offload on both interfaces\n");
> +				return -EINVAL;

Again, EOPNOTSUPP, so it falls back to software, and no dev_err().

> +			}
> +			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_err(prueth->dev, "Enabling HSR offload mode\n");

This is not an error condition. dev_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_info(prueth->dev, "Enabling Dual EMAC mode\n");

dev_dbg().

> +	}
> +}
> +
>  /* netdev notifier */
>  static int prueth_netdevice_event(struct notifier_block *unused,
>  				  unsigned long event, void *ptr)
> @@ -1047,6 +1141,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 +1152,26 @@ static int prueth_netdevice_event(struct notifier_block *unused,
>  	case NETDEV_CHANGEUPPER:
>  		info = ptr;
>  
> +		if ((ndev->features & NETIF_PRUETH_HSR_OFFLOAD) &&
> +		    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_err(prueth->dev, "Both interfaces must be linked to same upper device\n");

dev_dbg()

	Andrew

Re: [PATCH net-next v2 5/7] net: ti: icssg-prueth: Enable HSR Tx Packet duplication offload

[Andrew Lunn]

> --- 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	NETIF_F_HW_HSR_FWD
> +#define NETIF_PRUETH_HSR_OFFLOAD	(NETIF_F_HW_HSR_FWD | \
> +					 NETIF_F_HW_HSR_DUP)

Ah! Now i see why you added the alias. This is O.K. then.

Maybe NETIF_PRUETH_HSR_OFFLOAD_FEATURES, although that is a bit long,
but it makes it clear it is a collection of features, not an alias for
one feature.


	Andrew

Re: [PATCH net-next v2 0/7] Introduce HSR offload support for ICSSG

[MD Danish Anwar]

Hi Andrew,

On 13/08/24 8:19 pm, Andrew Lunn wrote:
> On Tue, Aug 13, 2024 at 01:12:26PM +0530, MD Danish Anwar wrote:
>> Hi All,
>> This series introduces HSR offload support for ICSSG driver. To support HSR
>> offload to hardware, ICSSG HSR firmware is used.
> 
> Oh, no, not another firmware. How does this interact with using the
> switch firmware and switchdev? I see in your examples you talk about
> HSR to Dual EMAC, but what about HSR and Switchdev?
> 

HSR to Switch mode or switch mode to HSR is not supported by the firmware.

Only dual EMAC to Switch , dual EMAC to HSR, switch to dual EMAC and HSR
to dual EMAC is supported.

Software HSR, software Switch / bridging can be done only with dual EMAC
firmware.

To summarize,
Dual EMAC firmware - Supports normal Ethernet operations, Can do
software bridging, software HSR
Switch Firmware - Can do bridging in hardware. For software bridging
this firmware is not needed, DUAL EMAC firmware will be used.
HSR firmware - Can do HSR offloading in hardware. For software offload
this firmware is not needed, dual EMAC firmware will be used for that.

By default the firmware is Dual EMAC firmware. Firmware will only be
changed when offloading in hardware is needed.

> How many more different firmwares do you have?
> 

We have these 3 firmwares only for ICSSG.

> 	Andrew

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v2 4/7] net: ti: icssg-prueth: Add support for HSR frame forward offload

[MD Danish Anwar]

On 13/08/24 8:47 pm, Andrew Lunn wrote:
> On Tue, Aug 13, 2024 at 01:12:30PM +0530, 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.
> 
> Maybe a dumb question, because i don't know HSR....
> 
> Can you have one interface in a HSR network, another interface in a
> non-HSR network, and bridge packets between the two worlds? Do you
> want the HSR firmware, the Switchdev firmware, or Dual EMAC and do the
> bridge in software?
> 

As far as I know, when adding an hsr interface we need to specify both
the slave interfaces

` ip link add name hsr0 type hsr slave1 eth1 slave2 eth2 supervision 45
version 1`

HSR is only enabled when both the ports are added to hsr interface. If
hsr-fwd-offload is set, the firmware will be changed to HSR otherwise
Dual EMAC firmware will keep running and hsr forwarding will happen in
software.

>>  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..2f485318c940 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> @@ -455,7 +455,7 @@ 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)
>> +	if (prueth->is_switch_mode || prueth->is_hsr_offload_mode)
>>  		icssg_init_switch_mode(prueth);
> 
> Maybe icssg_init_switch_mode() needs renaming if it is used for more
> than switch mode? There are other functions which might need
> generalising.
> 

Yes, the icssg_init_ and many other APIs are common for switch and hsr.
They can be renamed to indicate that as well.

How does icssg_init_switch_or_hsr_mode() sound?

>> +#define NETIF_PRUETH_HSR_OFFLOAD	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;
> 
> Documentation/networking/netdev-features.rst
> 
> * hsr-fwd-offload
> 
> This should be set for devices which forward HSR (High-availability Seamless
> Redundancy) frames from one port to another in hardware.
> 
> To me, this suggests if the flag is not set, you should keep in dual
> EMACS or switchdev mode and perform HSR in software.


Correct. This is the expected behavior. If the flag is not set we remain
in dual EMAC firmware and do HSR in software. Please see
prueth_hsr_port_link() for detail on this.

> 
>> +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;
>> +	netdev_features_t hsr_feature_wanted = features & NETIF_PRUETH_HSR_OFFLOAD;
> 
> I would not add the _PRUETH_ alias. There is nothing _PRUETH_ specific
> here, its just plain HSR offload.
> 

I see your query in this is resolved by
https://lore.kernel.org/all/985e10e4-49df-46d8-b9c2-d385dab569a9@lunn.ch/

>> +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) {
>> +		dev_err(prueth->dev, "Switching from bridge to HSR mode not allowed\n");
>> +		return -EINVAL;
> 
> I think you want EOPNOTSUPP, so that it is performed in software, not
> offloaded to hardware. And this is not an error condition, it is just
> a limitation of your hardware/firmware.
> 

Sure.

>> +	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) &&
>> +			    !(emac1->ndev->features & NETIF_PRUETH_HSR_OFFLOAD)) {
>> +				dev_err(prueth->dev, "Enable HSR offload on both interfaces\n");
>> +				return -EINVAL;
> 
> Again, EOPNOTSUPP, so it falls back to software, and no dev_err().

sure I will change this to EOPNOTSUPP and remove the print.

> 
>> +			}
>> +			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_err(prueth->dev, "Enabling HSR offload mode\n");
> 
> This is not an error condition. dev_dbg().

Sure.

> 
>> +		}
>> +	}
>> +
>> +	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_info(prueth->dev, "Enabling Dual EMAC mode\n");
> 
> dev_dbg().

Sure.

> 
>> +	}
>> +}
>> +
>>  /* netdev notifier */
>>  static int prueth_netdevice_event(struct notifier_block *unused,
>>  				  unsigned long event, void *ptr)
>> @@ -1047,6 +1141,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 +1152,26 @@ static int prueth_netdevice_event(struct notifier_block *unused,
>>  	case NETDEV_CHANGEUPPER:
>>  		info = ptr;
>>  
>> +		if ((ndev->features & NETIF_PRUETH_HSR_OFFLOAD) &&
>> +		    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_err(prueth->dev, "Both interfaces must be linked to same upper device\n");
> 
> dev_dbg()

Sure. I will do these changes and send out a new version. Please let me
know if any other change is needed.

> 
> 	Andrew

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v2 5/7] net: ti: icssg-prueth: Enable HSR Tx Packet duplication offload

[MD Danish Anwar]

On 13/08/24 8:53 pm, Andrew Lunn wrote:
>> --- 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	NETIF_F_HW_HSR_FWD
>> +#define NETIF_PRUETH_HSR_OFFLOAD	(NETIF_F_HW_HSR_FWD | \
>> +					 NETIF_F_HW_HSR_DUP)
> 
> Ah! Now i see why you added the alias. This is O.K. then.
> 
> Maybe NETIF_PRUETH_HSR_OFFLOAD_FEATURES, although that is a bit long,
> but it makes it clear it is a collection of features, not an alias for
> one feature.
> 

Sure. I will change this. Thanks for reviewing this series.

> 
> 	Andrew

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v2 4/7] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Andrew Lunn]

> Yes, the icssg_init_ and many other APIs are common for switch and hsr.
> They can be renamed to indicate that as well.
> 
> How does icssg_init_switch_or_hsr_mode() sound?

I would say it is too long. And when you add the next thing, say
bonding, will it become icssg_init_switch_or_hsr_or_bond_mode()?

Maybe name the function after what it actually does, not why you call
it.

> >>  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;
> > 
> > Documentation/networking/netdev-features.rst
> > 
> > * hsr-fwd-offload
> > 
> > This should be set for devices which forward HSR (High-availability Seamless
> > Redundancy) frames from one port to another in hardware.
> > 
> > To me, this suggests if the flag is not set, you should keep in dual
> > EMACS or switchdev mode and perform HSR in software.
> 
> 
> Correct. This is the expected behavior. If the flag is not set we remain
> in dual EMAC firmware and do HSR in software. Please see
> prueth_hsr_port_link() for detail on this.

O.K.

	Andrew

Re: [PATCH net-next v2 0/7] Introduce HSR offload support for ICSSG

[Andrew Lunn]

> We have these 3 firmwares only for ICSSG.

O.K. But i also hope you have learned from this and the next
generation of the hardware with have more RAM for firmware, so you
only need one firmware image for everything.

     Andrew

Re: [PATCH net-next v2 4/7] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Anwar, Md Danish]

On 8/14/2024 7:32 PM, Andrew Lunn wrote:
>> Yes, the icssg_init_ and many other APIs are common for switch and hsr.
>> They can be renamed to indicate that as well.
>>
>> How does icssg_init_switch_or_hsr_mode() sound?
> 
> I would say it is too long. And when you add the next thing, say
> bonding, will it become icssg_init_switch_or_hsr_or_bond_mode()?
> 
> Maybe name the function after what it actually does, not why you call
> it.
> 

Sure Andrew, I will try to come up with a proper name for these APIs.

>>>>  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;
>>>
>>> Documentation/networking/netdev-features.rst
>>>
>>> * hsr-fwd-offload
>>>
>>> This should be set for devices which forward HSR (High-availability Seamless
>>> Redundancy) frames from one port to another in hardware.
>>>
>>> To me, this suggests if the flag is not set, you should keep in dual
>>> EMACS or switchdev mode and perform HSR in software.
>>
>>
>> Correct. This is the expected behavior. If the flag is not set we remain
>> in dual EMAC firmware and do HSR in software. Please see
>> prueth_hsr_port_link() for detail on this.
> 
> O.K.
> 
> 	Andrew

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v2 0/7] Introduce HSR offload support for ICSSG

[Anwar, Md Danish]

On 8/14/2024 7:34 PM, Andrew Lunn wrote:
>> We have these 3 firmwares only for ICSSG.
> 
> O.K. But i also hope you have learned from this and the next
> generation of the hardware with have more RAM for firmware, so you
> only need one firmware image for everything.
> 

Yes Andrew, The goal will always be to have a firmware which can do all
these instead of multiple firmwares in future. Thanks for all the
reviews and sugestions.

>      Andrew

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v2 4/7] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Simon Horman]

On Tue, Aug 13, 2024 at 01:12:30PM +0530, MD Danish Anwar wrote:

...

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index f678d656a3ed..40bc3912b6ae 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -239,6 +239,7 @@ 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
>   * @prueth_netdevice_nb: netdevice notifier block
>   * @prueth_switchdev_nb: switchdev notifier block

I think you also need to add Kernel doc entries for @hsr_members and
@is_hsr_offload_mode.

Flagged by W=1 builds and ./scripts/kernel-doc -none

> @@ -274,11 +275,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
> 

Re: [PATCH net-next v2 4/7] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Anwar, Md Danish]

On 8/15/2024 8:44 PM, Simon Horman wrote:
> On Tue, Aug 13, 2024 at 01:12:30PM +0530, MD Danish Anwar wrote:
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> index f678d656a3ed..40bc3912b6ae 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -239,6 +239,7 @@ 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
>>   * @prueth_netdevice_nb: netdevice notifier block
>>   * @prueth_switchdev_nb: switchdev notifier block
> 
> I think you also need to add Kernel doc entries for @hsr_members and
> @is_hsr_offload_mode.
> 
> Flagged by W=1 builds and ./scripts/kernel-doc -none

Sure Simon, I'll do that.

> 
>> @@ -274,11 +275,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
>>

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v2 4/7] net: ti: icssg-prueth: Add support for HSR frame forward offload

[Anwar, Md Danish]

On 8/14/2024 8:24 PM, Anwar, Md Danish wrote:
> 
> 
> On 8/14/2024 7:32 PM, Andrew Lunn wrote:
>>> Yes, the icssg_init_ and many other APIs are common for switch and hsr.
>>> They can be renamed to indicate that as well.
>>>
>>> How does icssg_init_switch_or_hsr_mode() sound?
>>
>> I would say it is too long. And when you add the next thing, say
>> bonding, will it become icssg_init_switch_or_hsr_or_bond_mode()?
>>
>> Maybe name the function after what it actually does, not why you call
>> it.
>>

Andrew, the functions are actually doing some configurations different
than EMAC mode which are needed for offloading the frames for switch as
well as HSR mode. icssg_init_emac_mode() doesn't do any configuration
related to offloading. Perhaps naming these APIs to
icssg_init_fw_offload_mode() will be a better fit?

Other Similar APIs can also be changed to use _fw_offload instead of
_switch. How does this sound?


> Sure Andrew, I will try to come up with a proper name for these APIs.
> 
>>>>>  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;
>>>>
>>>> Documentation/networking/netdev-features.rst
>>>>
>>>> * hsr-fwd-offload
>>>>
>>>> This should be set for devices which forward HSR (High-availability Seamless
>>>> Redundancy) frames from one port to another in hardware.
>>>>
>>>> To me, this suggests if the flag is not set, you should keep in dual
>>>> EMACS or switchdev mode and perform HSR in software.
>>>
>>>
>>> Correct. This is the expected behavior. If the flag is not set we remain
>>> in dual EMAC firmware and do HSR in software. Please see
>>> prueth_hsr_port_link() for detail on this.
>>
>> O.K.
>>
>> 	Andrew
> 

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[Roger Quadros]

Hi,

On 13/08/2024 10:42, MD Danish Anwar wrote:
> IEP1 is needed by firmware to enable FDB learning and FDB ageing.

Required by which firmware?

Does dual-emac firmware need this?

> Always enable IEP1
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 53a3e44b99a2..613bd8de6eb8 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -1256,12 +1256,8 @@ static int prueth_probe(struct platform_device *pdev)
>  		goto put_iep0;
>  	}
>  
> -	if (prueth->pdata.quirk_10m_link_issue) {
> -		/* Enable IEP1 for FW in 64bit mode as W/A for 10M FD link detect issue under TX
> -		 * traffic.
> -		 */
> -		icss_iep_init_fw(prueth->iep1);
> -	}
> +	/* Enable IEP1 for FW as it's needed by FW for FDB Learning and FDB ageing */
> +	icss_iep_init_fw(prueth->iep1);
>  
>  	/* setup netdev interfaces */
>  	if (eth0_node) {
> @@ -1366,8 +1362,7 @@ static int prueth_probe(struct platform_device *pdev)
>  	}
>  
>  exit_iep:
> -	if (prueth->pdata.quirk_10m_link_issue)
> -		icss_iep_exit_fw(prueth->iep1);
> +	icss_iep_exit_fw(prueth->iep1);
>  	icss_iep_put(prueth->iep1);
>  
>  put_iep0:
> @@ -1424,8 +1419,7 @@ static void prueth_remove(struct platform_device *pdev)
>  		prueth_netdev_exit(prueth, eth_node);
>  	}
>  
> -	if (prueth->pdata.quirk_10m_link_issue)
> -		icss_iep_exit_fw(prueth->iep1);
> +	icss_iep_exit_fw(prueth->iep1);
>  
>  	icss_iep_put(prueth->iep1);
>  	icss_iep_put(prueth->iep0);

-- 
cheers,
-roger

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[Anwar, Md Danish]

Hi Roger,

On 8/21/2024 4:57 PM, Roger Quadros wrote:
> Hi,
> 
> On 13/08/2024 10:42, MD Danish Anwar wrote:
>> IEP1 is needed by firmware to enable FDB learning and FDB ageing.
> 
> Required by which firmware?
> 

IEP1 is needed by all ICSSG firmwares (Dual EMAC / Switch / HSR)

> Does dual-emac firmware need this?
> 

Yes, Dual EMAC firmware needs IEP1 to enabled.

>> Always enable IEP1
>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 14 ++++----------
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index 53a3e44b99a2..613bd8de6eb8 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -1256,12 +1256,8 @@ static int prueth_probe(struct platform_device *pdev)
>>  		goto put_iep0;
>>  	}
>>  
>> -	if (prueth->pdata.quirk_10m_link_issue) {
>> -		/* Enable IEP1 for FW in 64bit mode as W/A for 10M FD link detect issue under TX
>> -		 * traffic.
>> -		 */
>> -		icss_iep_init_fw(prueth->iep1);
>> -	}
>> +	/* Enable IEP1 for FW as it's needed by FW for FDB Learning and FDB ageing */
>> +	icss_iep_init_fw(prueth->iep1);
>>  
>>  	/* setup netdev interfaces */
>>  	if (eth0_node) {
>> @@ -1366,8 +1362,7 @@ static int prueth_probe(struct platform_device *pdev)
>>  	}
>>  
>>  exit_iep:
>> -	if (prueth->pdata.quirk_10m_link_issue)
>> -		icss_iep_exit_fw(prueth->iep1);
>> +	icss_iep_exit_fw(prueth->iep1);
>>  	icss_iep_put(prueth->iep1);
>>  
>>  put_iep0:
>> @@ -1424,8 +1419,7 @@ static void prueth_remove(struct platform_device *pdev)
>>  		prueth_netdev_exit(prueth, eth_node);
>>  	}
>>  
>> -	if (prueth->pdata.quirk_10m_link_issue)
>> -		icss_iep_exit_fw(prueth->iep1);
>> +	icss_iep_exit_fw(prueth->iep1);
>>  
>>  	icss_iep_put(prueth->iep1);
>>  	icss_iep_put(prueth->iep0);
> 

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[Roger Quadros]

On 21/08/2024 14:33, Anwar, Md Danish wrote:
> Hi Roger,
> 
> On 8/21/2024 4:57 PM, Roger Quadros wrote:
>> Hi,
>>
>> On 13/08/2024 10:42, MD Danish Anwar wrote:
>>> IEP1 is needed by firmware to enable FDB learning and FDB ageing.
>>
>> Required by which firmware?
>>
> 
> IEP1 is needed by all ICSSG firmwares (Dual EMAC / Switch / HSR)
> 
>> Does dual-emac firmware need this?
>>
> 
> Yes, Dual EMAC firmware needs IEP1 to enabled.

Then this need to be a bug fix?
What is the impact if IEP1 is not enabled for dual emac.

> 
>>> Always enable IEP1
>>>
>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>> ---

-- 
cheers,
-roger

Re: [PATCH net-next v2 2/7] net: ti: icss-iep: Move icss_iep structure

[Roger Quadros]

On 13/08/2024 10:42, MD Danish Anwar wrote:
> 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>

Re: [PATCH net-next v2 3/7] net: ti: icssg-prueth: Stop hardcoding def_inc

[Roger Quadros]

On 13/08/2024 10:42, MD Danish Anwar wrote:
> 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>

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

[Roger Quadros]

On 13/08/2024 10:42, MD Danish Anwar wrote:
> 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 | 38 +++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index b32a2bff34dc..521e9f914459 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
> @@ -651,6 +681,7 @@ 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);

Above unsync call will already remove all MC addresses so nothing
is left to unsync in the below unsync call.
> +	__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);

Do you have to use an if/else like you do while calling __dev_mc_sync?

>  
>  	atomic_set(&emac->tdown_cnt, emac->tx_ch_num);
>  	/* ensure new tdown_cnt value is visible */
> @@ -728,7 +759,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);
>  }
>  
>  /**

-- 
cheers,
-roger

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

[Roger Quadros]

On 13/08/2024 10:42, 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.

I can see code for Tx Tag insertion and RX tag removal.
Where are you doing duplicate discard in this patch?

> 
> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@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 2f485318c940..f061fa97a377 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 521e9f914459..582e72dd8f3f 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	(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)
> @@ -1032,6 +1034,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 6cb1dce8b309..246f1e41c13a 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -58,6 +58,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

-- 
cheers,
-roger

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[MD Danish Anwar]

On 21/08/24 5:23 pm, Roger Quadros wrote:
> 
> 
> On 21/08/2024 14:33, Anwar, Md Danish wrote:
>> Hi Roger,
>>
>> On 8/21/2024 4:57 PM, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 13/08/2024 10:42, MD Danish Anwar wrote:
>>>> IEP1 is needed by firmware to enable FDB learning and FDB ageing.
>>>
>>> Required by which firmware?
>>>
>>
>> IEP1 is needed by all ICSSG firmwares (Dual EMAC / Switch / HSR)
>>
>>> Does dual-emac firmware need this?
>>>
>>
>> Yes, Dual EMAC firmware needs IEP1 to enabled.
> 
> Then this need to be a bug fix?

Correct, this is in fact a bug. But IEP1 is also needed by HSR firmware
so I thought of keeping this patch with HSR series. As HSR will be
completely broken if IEP1 is not enabled.

I didn't want to post two patches one as bug fix to net and one part of
HSR to net-next thus I thought of keeping this patch in this series only.

> What is the impact if IEP1 is not enabled for dual emac.
> 

Without IEP1 enabled, Crash is seen on AM64x 10M link when connecting /
disconnecting multiple times. On AM65x IEP1 was always enabled because
`prueth->pdata.quirk_10m_link_issue` was true. FDB learning and FDB
ageing will also get impacted if IEP1 is not enabled.

>>
>>>> Always enable IEP1
>>>>
>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>> ---
> 

-- 
Thanks and Regards,
Danish

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

[MD Danish Anwar]

On 21/08/24 5:40 pm, Roger Quadros wrote:
> 
> 
> On 13/08/2024 10:42, MD Danish Anwar wrote:
>> 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 | 38 +++++++++++++++++++-
>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index b32a2bff34dc..521e9f914459 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
>> @@ -651,6 +681,7 @@ 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);
> 
> Above unsync call will already remove all MC addresses so nothing
> is left to unsync in the below unsync call.
>> +	__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
> 
> Do you have to use an if/else like you do while calling __dev_mc_sync?
> 

Yes Roger, we will need and if/else here and remove MC addresses based
on the current mode.

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);

I will make this change and update the series.

>>  
>>  	atomic_set(&emac->tdown_cnt, emac->tx_ch_num);
>>  	/* ensure new tdown_cnt value is visible */
>> @@ -728,7 +759,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);
>>  }
>>  
>>  /**
> 

-- 
Thanks and Regards,
Danish

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

[MD Danish Anwar]

On 21/08/24 5:45 pm, Roger Quadros wrote:
> 
> 
> On 13/08/2024 10:42, 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.
> 
> I can see code for Tx Tag insertion and RX tag removal.
> Where are you doing duplicate discard in this patch?
> 

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

Maybe I can modify the commit message to stated that duplicate discard
is done as part of rx tag removal?

>>
>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@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 2f485318c940..f061fa97a377 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 521e9f914459..582e72dd8f3f 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	(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)
>> @@ -1032,6 +1034,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);

Duplicate discard is done here ^^^^

>> +			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 6cb1dce8b309..246f1e41c13a 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -58,6 +58,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
> 

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[Roger Quadros]

On 22/08/2024 08:52, MD Danish Anwar wrote:
> 
> 
> On 21/08/24 5:23 pm, Roger Quadros wrote:
>>
>>
>> On 21/08/2024 14:33, Anwar, Md Danish wrote:
>>> Hi Roger,
>>>
>>> On 8/21/2024 4:57 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 13/08/2024 10:42, MD Danish Anwar wrote:
>>>>> IEP1 is needed by firmware to enable FDB learning and FDB ageing.
>>>>
>>>> Required by which firmware?
>>>>
>>>
>>> IEP1 is needed by all ICSSG firmwares (Dual EMAC / Switch / HSR)
>>>
>>>> Does dual-emac firmware need this?
>>>>
>>>
>>> Yes, Dual EMAC firmware needs IEP1 to enabled.
>>
>> Then this need to be a bug fix?
> 
> Correct, this is in fact a bug. But IEP1 is also needed by HSR firmware
> so I thought of keeping this patch with HSR series. As HSR will be
> completely broken if IEP1 is not enabled.
> 
> I didn't want to post two patches one as bug fix to net and one part of
> HSR to net-next thus I thought of keeping this patch in this series only.

Bug fixes need to be posted earlier as they can get accepted sooner and
even back-ported to stable. You also need to add the Fixes tag.

> 
>> What is the impact if IEP1 is not enabled for dual emac.
>>
> 
> Without IEP1 enabled, Crash is seen on AM64x 10M link when connecting /
> disconnecting multiple times. On AM65x IEP1 was always enabled because

In that ase you need to enable quirk_10m_link_issue for AM64x platform.
I understand that IEP1 is not required for 100M/1G.

> `prueth->pdata.quirk_10m_link_issue` was true. FDB learning and FDB
> ageing will also get impacted if IEP1 is not enabled.

Is FDB learning and ageing involved in dual Emac mode?

> 
>>>
>>>>> Always enable IEP1
>>>>>
>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>> ---
>>
> 

-- 
cheers,
-roger

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

[Roger Quadros]

On 22/08/2024 11:03, MD Danish Anwar wrote:
> 
> 
> On 21/08/24 5:45 pm, Roger Quadros wrote:
>>
>>
>> On 13/08/2024 10:42, 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.
>>
>> I can see code for Tx Tag insertion and RX tag removal.
>> Where are you doing duplicate discard in this patch?
>>
> 
> Roger, duplicate discard is done as part of RX tag removal and it is
> done by firmware.
> When driver sends the command ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware
> does RX tag removal as well as duplicate discard.
> 
> Maybe I can modify the commit message to stated that duplicate discard
> is done as part of rx tag removal?

Yes please, that will help. Thanks!

> 
>>>
>>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@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 2f485318c940..f061fa97a377 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 521e9f914459..582e72dd8f3f 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	(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)
>>> @@ -1032,6 +1034,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);
> 
> Duplicate discard is done here ^^^^

Got it.

> 
>>> +			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 6cb1dce8b309..246f1e41c13a 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>> @@ -58,6 +58,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
>>
> 

-- 
cheers,
-roger

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[Dan Carpenter]

On Thu, Aug 22, 2024 at 11:22:44AM +0530, MD Danish Anwar wrote:
> 
> 
> On 21/08/24 5:23 pm, Roger Quadros wrote:
> > 
> > 
> > On 21/08/2024 14:33, Anwar, Md Danish wrote:
> >> Hi Roger,
> >>
> >> On 8/21/2024 4:57 PM, Roger Quadros wrote:
> >>> Hi,
> >>>
> >>> On 13/08/2024 10:42, MD Danish Anwar wrote:
> >>>> IEP1 is needed by firmware to enable FDB learning and FDB ageing.
> >>>
> >>> Required by which firmware?
> >>>
> >>
> >> IEP1 is needed by all ICSSG firmwares (Dual EMAC / Switch / HSR)
> >>
> >>> Does dual-emac firmware need this?
> >>>
> >>
> >> Yes, Dual EMAC firmware needs IEP1 to enabled.
> > 
> > Then this need to be a bug fix?
> 
> Correct, this is in fact a bug. But IEP1 is also needed by HSR firmware
> so I thought of keeping this patch with HSR series. As HSR will be
> completely broken if IEP1 is not enabled.
> 
> I didn't want to post two patches one as bug fix to net and one part of
> HSR to net-next thus I thought of keeping this patch in this series only.
> 
> > What is the impact if IEP1 is not enabled for dual emac.
> > 
> 
> Without IEP1 enabled, Crash is seen on AM64x 10M link when connecting /
> disconnecting multiple times. On AM65x IEP1 was always enabled because
> `prueth->pdata.quirk_10m_link_issue` was true. FDB learning and FDB
> ageing will also get impacted if IEP1 is not enabled.
> 

Please could you add the information mentioned in this email into the commit
message?

regards,
dan carpenter

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[Anwar, Md Danish]

On 8/22/2024 4:57 PM, Roger Quadros wrote:
> 
> 
> On 22/08/2024 08:52, MD Danish Anwar wrote:
>>
>>
>> On 21/08/24 5:23 pm, Roger Quadros wrote:
>>>
>>>
>>> On 21/08/2024 14:33, Anwar, Md Danish wrote:
>>>> Hi Roger,
>>>>
>>>> On 8/21/2024 4:57 PM, Roger Quadros wrote:
>>>>> Hi,
>>>>>
>>>>> On 13/08/2024 10:42, MD Danish Anwar wrote:
>>>>>> IEP1 is needed by firmware to enable FDB learning and FDB ageing.
>>>>>
>>>>> Required by which firmware?
>>>>>
>>>>
>>>> IEP1 is needed by all ICSSG firmwares (Dual EMAC / Switch / HSR)
>>>>
>>>>> Does dual-emac firmware need this?
>>>>>
>>>>
>>>> Yes, Dual EMAC firmware needs IEP1 to enabled.
>>>
>>> Then this need to be a bug fix?
>>
>> Correct, this is in fact a bug. But IEP1 is also needed by HSR firmware
>> so I thought of keeping this patch with HSR series. As HSR will be
>> completely broken if IEP1 is not enabled.
>>
>> I didn't want to post two patches one as bug fix to net and one part of
>> HSR to net-next thus I thought of keeping this patch in this series only.
> 
> Bug fixes need to be posted earlier as they can get accepted sooner and
> even back-ported to stable. You also need to add the Fixes tag.
> 

Yes I understand that. The problem was I will need to send the patch
twice once as bug fix to net/main, once as part of this series to
net-next/main and drop it from this series once the patch gets merged to
net and is synced to net-next. Since I cannot post this series without
this patch as the HSR feature will get broken.

Or I need to post this to net/main and wait for it to be part of
net-next and then only I can re-post this series. So to avoid all these
I thought of only posting it as part of this series. This is not a major
bug and it will be okay from feature perspective if this bug gets merged
via net-next.

What do you suggest now? Should I post the bug fix to net/main and post
this seires without this patch or wait for the bug fix to sync then only
post this series?

>>
>>> What is the impact if IEP1 is not enabled for dual emac.
>>>
>>
>> Without IEP1 enabled, Crash is seen on AM64x 10M link when connecting /
>> disconnecting multiple times. On AM65x IEP1 was always enabled because
> 
> In that ase you need to enable quirk_10m_link_issue for AM64x platform.

Correct. But since for all ICSSG supported platform quirk_10m_link_issue
 needs to be enabled. Which in turn will enable IEP1. I think it's
better enable IEP1 directly without any condition.

IEP1 is also needed for Switch and HSR firmwares so I thought directly
enabling it instead of enabling it inside the if check
`prueth->pdata.quirk_10m_link_issue` would be better idea.

What do you suggest here? Will setting `quirk_10m_link_issue` as true
for AM64x will be a better approach or always enabling IEP1 without any
if check will be better approach for this?

> I understand that IEP1 is not required for 100M/1G.
> 
>> `prueth->pdata.quirk_10m_link_issue` was true. FDB learning and FDB
>> ageing will also get impacted if IEP1 is not enabled.
> 
> Is FDB learning and ageing involved in dual Emac mode?
> 

Yes FDB learning and ageing is involved in dual EMAC mode as well.

>>
>>>>
>>>>>> Always enable IEP1
>>>>>>
>>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>>> ---
>>>
>>
> 

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[Anwar, Md Danish]

On 8/22/2024 5:02 PM, Dan Carpenter wrote:
> On Thu, Aug 22, 2024 at 11:22:44AM +0530, MD Danish Anwar wrote:
>>
>>
>> On 21/08/24 5:23 pm, Roger Quadros wrote:
>>>
>>>
>>> On 21/08/2024 14:33, Anwar, Md Danish wrote:
>>>> Hi Roger,
>>>>
>>>> On 8/21/2024 4:57 PM, Roger Quadros wrote:
>>>>> Hi,
>>>>>
>>>>> On 13/08/2024 10:42, MD Danish Anwar wrote:
>>>>>> IEP1 is needed by firmware to enable FDB learning and FDB ageing.
>>>>>
>>>>> Required by which firmware?
>>>>>
>>>>
>>>> IEP1 is needed by all ICSSG firmwares (Dual EMAC / Switch / HSR)
>>>>
>>>>> Does dual-emac firmware need this?
>>>>>
>>>>
>>>> Yes, Dual EMAC firmware needs IEP1 to enabled.
>>>
>>> Then this need to be a bug fix?
>>
>> Correct, this is in fact a bug. But IEP1 is also needed by HSR firmware
>> so I thought of keeping this patch with HSR series. As HSR will be
>> completely broken if IEP1 is not enabled.
>>
>> I didn't want to post two patches one as bug fix to net and one part of
>> HSR to net-next thus I thought of keeping this patch in this series only.
>>
>>> What is the impact if IEP1 is not enabled for dual emac.
>>>
>>
>> Without IEP1 enabled, Crash is seen on AM64x 10M link when connecting /
>> disconnecting multiple times. On AM65x IEP1 was always enabled because
>> `prueth->pdata.quirk_10m_link_issue` was true. FDB learning and FDB
>> ageing will also get impacted if IEP1 is not enabled.
>>
> 
> Please could you add the information mentioned in this email into the commit
> message?

Sure Dan.

> 
> regards,
> dan carpenter
> 

-- 
Thanks and Regards,
Md Danish Anwar

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

[Anwar, Md Danish]

On 8/22/2024 4:58 PM, Roger Quadros wrote:
> 
> 
> On 22/08/2024 11:03, MD Danish Anwar wrote:
>>
>>
>> On 21/08/24 5:45 pm, Roger Quadros wrote:
>>>
>>>
>>> On 13/08/2024 10:42, 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.
>>>
>>> I can see code for Tx Tag insertion and RX tag removal.
>>> Where are you doing duplicate discard in this patch?
>>>
>>
>> Roger, duplicate discard is done as part of RX tag removal and it is
>> done by firmware.
>> When driver sends the command ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware
>> does RX tag removal as well as duplicate discard.
>>
>> Maybe I can modify the commit message to stated that duplicate discard
>> is done as part of rx tag removal?
> 
> Yes please, that will help. Thanks!
> 

Sure Roger, will do.

>>
>>>>
>>>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@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 2f485318c940..f061fa97a377 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 521e9f914459..582e72dd8f3f 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	(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)
>>>> @@ -1032,6 +1034,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);
>>
>> Duplicate discard is done here ^^^^
> 
> Got it.
> 
>>
>>>> +			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 6cb1dce8b309..246f1e41c13a 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>> @@ -58,6 +58,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
>>>
>>
> 

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[Roger Quadros]

Hi,

On 22/08/2024 15:12, Anwar, Md Danish wrote:
> 
> 
> On 8/22/2024 4:57 PM, Roger Quadros wrote:
>>
>>
>> On 22/08/2024 08:52, MD Danish Anwar wrote:
>>>
>>>
>>> On 21/08/24 5:23 pm, Roger Quadros wrote:
>>>>
>>>>
>>>> On 21/08/2024 14:33, Anwar, Md Danish wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On 8/21/2024 4:57 PM, Roger Quadros wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 13/08/2024 10:42, MD Danish Anwar wrote:
>>>>>>> IEP1 is needed by firmware to enable FDB learning and FDB ageing.
>>>>>>
>>>>>> Required by which firmware?
>>>>>>
>>>>>
>>>>> IEP1 is needed by all ICSSG firmwares (Dual EMAC / Switch / HSR)
>>>>>
>>>>>> Does dual-emac firmware need this?
>>>>>>
>>>>>
>>>>> Yes, Dual EMAC firmware needs IEP1 to enabled.
>>>>
>>>> Then this need to be a bug fix?
>>>
>>> Correct, this is in fact a bug. But IEP1 is also needed by HSR firmware
>>> so I thought of keeping this patch with HSR series. As HSR will be
>>> completely broken if IEP1 is not enabled.
>>>
>>> I didn't want to post two patches one as bug fix to net and one part of
>>> HSR to net-next thus I thought of keeping this patch in this series only.
>>
>> Bug fixes need to be posted earlier as they can get accepted sooner and
>> even back-ported to stable. You also need to add the Fixes tag.
>>
> 
> Yes I understand that. The problem was I will need to send the patch
> twice once as bug fix to net/main, once as part of this series to
> net-next/main and drop it from this series once the patch gets merged to
> net and is synced to net-next. Since I cannot post this series without
> this patch as the HSR feature will get broken.

HSR feature is not yet there so nothing will be broken. You can mention
in cover letter that a separate patch is required for functionality.

> 
> Or I need to post this to net/main and wait for it to be part of
> net-next and then only I can re-post this series. So to avoid all these
> I thought of only posting it as part of this series. This is not a major
> bug and it will be okay from feature perspective if this bug gets merged
> via net-next.
> 

If there is no build dependency I don't see why you need to wait.

> What do you suggest now? Should I post the bug fix to net/main and post
> this seires without this patch or wait for the bug fix to sync then only
> post this series?
> 

please see below.

>>>
>>>> What is the impact if IEP1 is not enabled for dual emac.
>>>>
>>>
>>> Without IEP1 enabled, Crash is seen on AM64x 10M link when connecting /
>>> disconnecting multiple times. On AM65x IEP1 was always enabled because
>>
>> In that ase you need to enable quirk_10m_link_issue for AM64x platform.
> 
> Correct. But since for all ICSSG supported platform quirk_10m_link_issue
>  needs to be enabled. Which in turn will enable IEP1. I think it's
> better enable IEP1 directly without any condition.
> 
> IEP1 is also needed for Switch and HSR firmwares so I thought directly
> enabling it instead of enabling it inside the if check
> `prueth->pdata.quirk_10m_link_issue` would be better idea.
> 
> What do you suggest here? Will setting `quirk_10m_link_issue` as true
> for AM64x will be a better approach or always enabling IEP1 without any
> if check will be better approach for this?

I would suggest that you first send a bug fix patch for AM64x which sets
quirk_10m_link_issue for AM64x. This should make it into the next -rc
and eventually stable.

Then in your HSR series, you can decide if you want to conditionally
enable it for HSR/switch mode or permanently enable it regardless.

> 
>> I understand that IEP1 is not required for 100M/1G.
>>
>>> `prueth->pdata.quirk_10m_link_issue` was true. FDB learning and FDB
>>> ageing will also get impacted if IEP1 is not enabled.
>>
>> Is FDB learning and ageing involved in dual Emac mode?
>>
> 
> Yes FDB learning and ageing is involved in dual EMAC mode as well.
> 
>>>
>>>>>
>>>>>>> Always enable IEP1
>>>>>>>
>>>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>>>> ---
>>>>
>>>
>>
> 

-- 
cheers,
-roger

Re: [PATCH net-next v2 1/7] net: ti: icssg-prueth: Enable IEP1

[MD Danish Anwar]

On 23/08/24 5:00 pm, Roger Quadros wrote:
> Hi,
> 
> On 22/08/2024 15:12, Anwar, Md Danish wrote:
>>
>>
>> On 8/22/2024 4:57 PM, Roger Quadros wrote:
>>>
>>>
>>> On 22/08/2024 08:52, MD Danish Anwar wrote:
>>>>
>>>>
>>>> On 21/08/24 5:23 pm, Roger Quadros wrote:
>>>>>
>>>>>
>>>>> On 21/08/2024 14:33, Anwar, Md Danish wrote:
>>>>>> Hi Roger,
>>>>>>
>>>>>> On 8/21/2024 4:57 PM, Roger Quadros wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 13/08/2024 10:42, MD Danish Anwar wrote:
>>>>>>>> IEP1 is needed by firmware to enable FDB learning and FDB ageing.
>>>>>>>
>>>>>>> Required by which firmware?
>>>>>>>
>>>>>>
>>>>>> IEP1 is needed by all ICSSG firmwares (Dual EMAC / Switch / HSR)
>>>>>>
>>>>>>> Does dual-emac firmware need this?
>>>>>>>
>>>>>>
>>>>>> Yes, Dual EMAC firmware needs IEP1 to enabled.
>>>>>
>>>>> Then this need to be a bug fix?
>>>>
>>>> Correct, this is in fact a bug. But IEP1 is also needed by HSR firmware
>>>> so I thought of keeping this patch with HSR series. As HSR will be
>>>> completely broken if IEP1 is not enabled.
>>>>
>>>> I didn't want to post two patches one as bug fix to net and one part of
>>>> HSR to net-next thus I thought of keeping this patch in this series only.
>>>
>>> Bug fixes need to be posted earlier as they can get accepted sooner and
>>> even back-ported to stable. You also need to add the Fixes tag.
>>>
>>
>> Yes I understand that. The problem was I will need to send the patch
>> twice once as bug fix to net/main, once as part of this series to
>> net-next/main and drop it from this series once the patch gets merged to
>> net and is synced to net-next. Since I cannot post this series without
>> this patch as the HSR feature will get broken.
> 
> HSR feature is not yet there so nothing will be broken. You can mention
> in cover letter that a separate patch is required for functionality.
> 
>>
>> Or I need to post this to net/main and wait for it to be part of
>> net-next and then only I can re-post this series. So to avoid all these
>> I thought of only posting it as part of this series. This is not a major
>> bug and it will be okay from feature perspective if this bug gets merged
>> via net-next.
>>
> 
> If there is no build dependency I don't see why you need to wait.
> 
>> What do you suggest now? Should I post the bug fix to net/main and post
>> this seires without this patch or wait for the bug fix to sync then only
>> post this series?
>>
> 
> please see below.
> 
>>>>
>>>>> What is the impact if IEP1 is not enabled for dual emac.
>>>>>
>>>>
>>>> Without IEP1 enabled, Crash is seen on AM64x 10M link when connecting /
>>>> disconnecting multiple times. On AM65x IEP1 was always enabled because
>>>
>>> In that ase you need to enable quirk_10m_link_issue for AM64x platform.
>>
>> Correct. But since for all ICSSG supported platform quirk_10m_link_issue
>>  needs to be enabled. Which in turn will enable IEP1. I think it's
>> better enable IEP1 directly without any condition.
>>
>> IEP1 is also needed for Switch and HSR firmwares so I thought directly
>> enabling it instead of enabling it inside the if check
>> `prueth->pdata.quirk_10m_link_issue` would be better idea.
>>
>> What do you suggest here? Will setting `quirk_10m_link_issue` as true
>> for AM64x will be a better approach or always enabling IEP1 without any
>> if check will be better approach for this?
> 
> I would suggest that you first send a bug fix patch for AM64x which sets
> quirk_10m_link_issue for AM64x. This should make it into the next -rc
> and eventually stable.
> 

OK Roger, I will send out the fix patch.

> Then in your HSR series, you can decide if you want to conditionally
> enable it for HSR/switch mode or permanently enable it regardless.
> 
>>
>>> I understand that IEP1 is not required for 100M/1G.
>>>
>>>> `prueth->pdata.quirk_10m_link_issue` was true. FDB learning and FDB
>>>> ageing will also get impacted if IEP1 is not enabled.
>>>
>>> Is FDB learning and ageing involved in dual Emac mode?
>>>
>>
>> Yes FDB learning and ageing is involved in dual EMAC mode as well.
>>
>>>>
>>>>>>
>>>>>>>> Always enable IEP1
>>>>>>>>
>>>>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>>>>> ---
>>>>>
>>>>
>>>
>>
> 

-- 
Thanks and Regards,
Danish