[PATCH net 0/2] IEP clock module bug fixes

[PATCH net 0/2] IEP clock module bug fixes

[Meghana Malladi]

Hi All,
This series has some bug fixes for IEP module needed by PPS and
timesync operations.

Patch 1/2 fixes firmware load sequence to ensure IEP module
is always running when either of the ethernet interfaces is up.

Patch 2/2 fixes distorted PPS signal when the ethernet interfaces
are brough down and up. This patch also fixes enabling PPS signal
after bringing the interface up, without disabling PPS.

MD Danish Anwar (1):
  net: ti: icssg-prueth: Fix firmware load sequence.

Meghana Malladi (1):
  net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during
    iep_init

 drivers/net/ethernet/ti/icssg/icss_iep.c     | 10 ++++
 drivers/net/ethernet/ti/icssg/icssg_config.c | 28 ++++++++++
 drivers/net/ethernet/ti/icssg/icssg_config.h |  1 +
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 58 ++++++++++++++++----
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  1 +
 5 files changed, 87 insertions(+), 11 deletions(-)


base-commit: 73840ca5ef361f143b89edd5368a1aa8c2979241
-- 
2.25.1

[PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load sequence.

[Meghana Malladi]

From: MD Danish Anwar <danishanwar@ti.com>

Timesync related operations are ran in PRU0 cores for both ICSSG SLICE0
and SLICE1. Currently whenever any ICSSG interface comes up we load the
respective firmwares to PRU cores and whenever interface goes down, we
stop the respective cores. Due to this, when SLICE0 goes down while
SLICE1 is still active, PRU0 firmwares are unloaded and PRU0 core is
stopped. This results in clock jump for SLICE1 interface as the timesync
related operations are no longer running.

Fix this by running both PRU0 and PRU1 firmwares as long as at least 1
ICSSG interface is up.

rx_flow_id is updated before firmware is loaded. Once firmware is loaded,
it reads the flow_id and uses it for rx. emac_fdb_flow_id_updated() is
used to let firmware know that the flow_id has been updated and to use the
latest rx_flow_id.

Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_config.c | 28 ++++++++++
 drivers/net/ethernet/ti/icssg/icssg_config.h |  1 +
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 58 ++++++++++++++++----
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  1 +
 4 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
index 5d2491c2943a..f1f0c8659e2d 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
@@ -786,3 +786,31 @@ void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port)
 		writel(pvid, prueth->shram.va + EMAC_ICSSG_SWITCH_PORT0_DEFAULT_VLAN_OFFSET);
 }
 EXPORT_SYMBOL_GPL(icssg_set_pvid);
+
+int emac_fdb_flow_id_updated(struct prueth_emac *emac)
+{
+	struct mgmt_cmd_rsp fdb_cmd_rsp = { 0 };
+	int slice = prueth_emac_slice(emac);
+	struct mgmt_cmd fdb_cmd = { 0 };
+	int ret = 0;
+
+	fdb_cmd.header = ICSSG_FW_MGMT_CMD_HEADER;
+	fdb_cmd.type   = ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW;
+	fdb_cmd.seqnum = ++(emac->prueth->icssg_hwcmdseq);
+	fdb_cmd.param  = 0;
+
+	fdb_cmd.param |= (slice << 4);
+	fdb_cmd.cmd_args[0] = 0;
+
+	ret = icssg_send_fdb_msg(emac, &fdb_cmd, &fdb_cmd_rsp);
+
+	if (ret)
+		return ret;
+
+	WARN_ON(fdb_cmd.seqnum != fdb_cmd_rsp.seqnum);
+	if (fdb_cmd_rsp.status == 1)
+		return 0;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(emac_fdb_flow_id_updated);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
index 92c2deaa3068..c884e9fa099e 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
@@ -55,6 +55,7 @@ struct icssg_rxq_ctx {
 #define ICSSG_FW_MGMT_FDB_CMD_TYPE	0x03
 #define ICSSG_FW_MGMT_CMD_TYPE		0x04
 #define ICSSG_FW_MGMT_PKT		0x80000000
+#define ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW	0x05
 
 struct icssg_r30_cmd {
 	u32 cmd[4];
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 0556910938fa..9df67539285b 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -534,6 +534,7 @@ static int emac_ndo_open(struct net_device *ndev)
 {
 	struct prueth_emac *emac = netdev_priv(ndev);
 	int ret, i, num_data_chn = emac->tx_ch_num;
+	struct icssg_flow_cfg __iomem *flow_cfg;
 	struct prueth *prueth = emac->prueth;
 	int slice = prueth_emac_slice(emac);
 	struct device *dev = prueth->dev;
@@ -549,8 +550,12 @@ static int emac_ndo_open(struct net_device *ndev)
 	/* set h/w MAC as user might have re-configured */
 	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
 
+	if (!prueth->emacs_initialized) {
+		icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
+		icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
+	}
+
 	icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
-	icssg_class_default(prueth->miig_rt, slice, 0, false);
 	icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
 
 	/* Notify the stack of the actual queue counts. */
@@ -588,10 +593,31 @@ static int emac_ndo_open(struct net_device *ndev)
 		goto cleanup_napi;
 	}
 
-	/* reset and start PRU firmware */
-	ret = prueth_emac_start(prueth, emac);
-	if (ret)
-		goto free_rx_irq;
+	if (!prueth->emacs_initialized) {
+		if (prueth->emac[ICSS_SLICE0]) {
+			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE0]);
+			if (ret) {
+				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE0);
+				goto free_rx_irq;
+			}
+		}
+		if (prueth->emac[ICSS_SLICE1]) {
+			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE1]);
+			if (ret) {
+				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE1);
+				goto halt_slice0_prus;
+			}
+		}
+	}
+
+	flow_cfg = emac->dram.va + ICSSG_CONFIG_OFFSET + PSI_L_REGULAR_FLOW_ID_BASE_OFFSET;
+	writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);
+	ret = emac_fdb_flow_id_updated(emac);
+
+	if (ret) {
+		netdev_err(ndev, "Failed to update Rx Flow ID %d", ret);
+		goto stop;
+	}
 
 	icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
 
@@ -644,7 +670,11 @@ static int emac_ndo_open(struct net_device *ndev)
 free_tx_ts_irq:
 	free_irq(emac->tx_ts_irq, emac);
 stop:
-	prueth_emac_stop(emac);
+	if (prueth->emac[ICSS_SLICE1])
+		prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
+halt_slice0_prus:
+	if (prueth->emac[ICSS_SLICE0])
+		prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
 free_rx_irq:
 	free_irq(emac->rx_chns.irq[rx_flow], emac);
 cleanup_napi:
@@ -680,7 +710,10 @@ static int emac_ndo_stop(struct net_device *ndev)
 	if (ndev->phydev)
 		phy_stop(ndev->phydev);
 
-	icssg_class_disable(prueth->miig_rt, prueth_emac_slice(emac));
+	if (prueth->emacs_initialized == 1) {
+		icssg_class_disable(prueth->miig_rt, ICSS_SLICE0);
+		icssg_class_disable(prueth->miig_rt, ICSS_SLICE1);
+	}
 
 	if (emac->prueth->is_hsr_offload_mode)
 		__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
@@ -719,11 +752,14 @@ static int emac_ndo_stop(struct net_device *ndev)
 	/* Destroying the queued work in ndo_stop() */
 	cancel_delayed_work_sync(&emac->stats_work);
 
-	if (prueth->emacs_initialized == 1)
+	if (prueth->emacs_initialized == 1) {
 		icss_iep_exit(emac->iep);
-
-	/* stop PRUs */
-	prueth_emac_stop(emac);
+		/* stop PRUs */
+		if (prueth->emac[ICSS_SLICE0])
+			prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
+		if (prueth->emac[ICSS_SLICE1])
+			prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
+	}
 
 	free_irq(emac->tx_ts_irq, emac);
 
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 8722bb4a268a..c4f5f0349ae7 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -365,6 +365,7 @@ void icssg_vtbl_modify(struct prueth_emac *emac, u8 vid, u8 port_mask,
 		       u8 untag_mask, bool add);
 u16 icssg_get_pvid(struct prueth_emac *emac);
 void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port);
+int emac_fdb_flow_id_updated(struct prueth_emac *emac);
 #define prueth_napi_to_tx_chn(pnapi) \
 	container_of(pnapi, struct prueth_tx_chn, napi_tx)
 
-- 
2.25.1

[PATCH net 2/2] net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init

[Meghana Malladi]

When ICSSG interfaces are brought down and brought up again, the
pru cores are shut down and booted again, flushing out all the memories
and start again in a clean state. Hence it is expected that the
IEP_CMP_CFG register needs to be flushed during iep_init() to ensure
that the existing residual configuration doesn't cause any unusual
behavior. If the register is not cleared, existing IEP_CMP_CFG set for
CMP1 will result in SYNC0_OUT signal based on the SYNC_OUT register values.

After bringing the interface up, calling PPS enable doesn't work as
the driver believes PPS is already enabled, (iep->pps_enabled is not
cleared during interface bring down) and driver  will just return true
even though there is no signal. Fix this by setting the iep->pps_enable
and iep->perout_enable flags to false during the link down.

Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
Signed-off-by: Meghana Malladi <m-malladi@ti.com>
---
 drivers/net/ethernet/ti/icssg/icss_iep.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index 5d6d1cf78e93..03abc25ced12 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -195,6 +195,12 @@ static void icss_iep_enable_shadow_mode(struct icss_iep *iep)
 
 	icss_iep_disable(iep);
 
+	/* clear compare config */
+	for (cmp = IEP_MIN_CMP; cmp < IEP_MAX_CMP; cmp++) {
+		regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
+				   IEP_CMP_CFG_CMP_EN(cmp), 0);
+	}
+
 	/* disable shadow mode */
 	regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
 			   IEP_CMP_CFG_SHADOW_EN, 0);
@@ -778,6 +784,10 @@ int icss_iep_exit(struct icss_iep *iep)
 		ptp_clock_unregister(iep->ptp_clock);
 		iep->ptp_clock = NULL;
 	}
+
+	iep->pps_enabled = false;
+	iep->perout_enabled = false;
+
 	icss_iep_disable(iep);
 
 	return 0;
-- 
2.25.1

Re: [PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load sequence.

[Simon Horman]

On Wed, Nov 06, 2024 at 01:10:39PM +0530, Meghana Malladi wrote:
> From: MD Danish Anwar <danishanwar@ti.com>
> 
> Timesync related operations are ran in PRU0 cores for both ICSSG SLICE0
> and SLICE1. Currently whenever any ICSSG interface comes up we load the
> respective firmwares to PRU cores and whenever interface goes down, we
> stop the respective cores. Due to this, when SLICE0 goes down while
> SLICE1 is still active, PRU0 firmwares are unloaded and PRU0 core is
> stopped. This results in clock jump for SLICE1 interface as the timesync
> related operations are no longer running.
> 
> Fix this by running both PRU0 and PRU1 firmwares as long as at least 1
> ICSSG interface is up.
> 
> rx_flow_id is updated before firmware is loaded. Once firmware is loaded,
> it reads the flow_id and uses it for rx. emac_fdb_flow_id_updated() is
> used to let firmware know that the flow_id has been updated and to use the
> latest rx_flow_id.
> 
> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>

...

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h

...

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 0556910938fa..9df67539285b 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -534,6 +534,7 @@ static int emac_ndo_open(struct net_device *ndev)
>  {
>  	struct prueth_emac *emac = netdev_priv(ndev);
>  	int ret, i, num_data_chn = emac->tx_ch_num;
> +	struct icssg_flow_cfg __iomem *flow_cfg;
>  	struct prueth *prueth = emac->prueth;
>  	int slice = prueth_emac_slice(emac);
>  	struct device *dev = prueth->dev;
> @@ -549,8 +550,12 @@ static int emac_ndo_open(struct net_device *ndev)
>  	/* set h/w MAC as user might have re-configured */
>  	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
>  
> +	if (!prueth->emacs_initialized) {
> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
> +	}
> +
>  	icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
> -	icssg_class_default(prueth->miig_rt, slice, 0, false);
>  	icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>  
>  	/* Notify the stack of the actual queue counts. */
> @@ -588,10 +593,31 @@ static int emac_ndo_open(struct net_device *ndev)
>  		goto cleanup_napi;
>  	}
>  
> -	/* reset and start PRU firmware */
> -	ret = prueth_emac_start(prueth, emac);
> -	if (ret)
> -		goto free_rx_irq;
> +	if (!prueth->emacs_initialized) {
> +		if (prueth->emac[ICSS_SLICE0]) {
> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE0]);

I wonder if it is worth simplifying this by having prueth_emac_start()
check if it's 2nd parameter is NULL. Likewise for prueth_emac_stop().

> +			if (ret) {
> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE0);
> +				goto free_rx_irq;
> +			}
> +		}
> +		if (prueth->emac[ICSS_SLICE1]) {
> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE1]);
> +			if (ret) {
> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE1);
> +				goto halt_slice0_prus;
> +			}
> +		}
> +	}
> +
> +	flow_cfg = emac->dram.va + ICSSG_CONFIG_OFFSET + PSI_L_REGULAR_FLOW_ID_BASE_OFFSET;
> +	writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);
> +	ret = emac_fdb_flow_id_updated(emac);
> +
> +	if (ret) {
> +		netdev_err(ndev, "Failed to update Rx Flow ID %d", ret);
> +		goto stop;

Branching to stop may result in calls to prueth_emac_stop() which does not
seem symmetrical with the condition on prueth->emacs_initialized for calls
to prueth_emac_start() above.

If so, is this intended?

> +	}
>  
>  	icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>  

...

Re: [PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load sequence.

[Meghana Malladi]

On 08/11/24 19:00, Simon Horman wrote:
> On Wed, Nov 06, 2024 at 01:10:39PM +0530, Meghana Malladi wrote:
>> From: MD Danish Anwar <danishanwar@ti.com>
>>
>> Timesync related operations are ran in PRU0 cores for both ICSSG SLICE0
>> and SLICE1. Currently whenever any ICSSG interface comes up we load the
>> respective firmwares to PRU cores and whenever interface goes down, we
>> stop the respective cores. Due to this, when SLICE0 goes down while
>> SLICE1 is still active, PRU0 firmwares are unloaded and PRU0 core is
>> stopped. This results in clock jump for SLICE1 interface as the timesync
>> related operations are no longer running.
>>
>> Fix this by running both PRU0 and PRU1 firmwares as long as at least 1
>> ICSSG interface is up.
>>
>> rx_flow_id is updated before firmware is loaded. Once firmware is loaded,
>> it reads the flow_id and uses it for rx. emac_fdb_flow_id_updated() is
>> used to let firmware know that the flow_id has been updated and to use the
>> latest rx_flow_id.
>>
>> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index 0556910938fa..9df67539285b 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -534,6 +534,7 @@ static int emac_ndo_open(struct net_device *ndev)
>>   {
>>   	struct prueth_emac *emac = netdev_priv(ndev);
>>   	int ret, i, num_data_chn = emac->tx_ch_num;
>> +	struct icssg_flow_cfg __iomem *flow_cfg;
>>   	struct prueth *prueth = emac->prueth;
>>   	int slice = prueth_emac_slice(emac);
>>   	struct device *dev = prueth->dev;
>> @@ -549,8 +550,12 @@ static int emac_ndo_open(struct net_device *ndev)
>>   	/* set h/w MAC as user might have re-configured */
>>   	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
>>   
>> +	if (!prueth->emacs_initialized) {
>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
>> +	}
>> +
>>   	icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>> -	icssg_class_default(prueth->miig_rt, slice, 0, false);
>>   	icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>>   
>>   	/* Notify the stack of the actual queue counts. */
>> @@ -588,10 +593,31 @@ static int emac_ndo_open(struct net_device *ndev)
>>   		goto cleanup_napi;
>>   	}
>>   
>> -	/* reset and start PRU firmware */
>> -	ret = prueth_emac_start(prueth, emac);
>> -	if (ret)
>> -		goto free_rx_irq;
>> +	if (!prueth->emacs_initialized) {
>> +		if (prueth->emac[ICSS_SLICE0]) {
>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE0]);
> 
> I wonder if it is worth simplifying this by having prueth_emac_start()
> check if it's 2nd parameter is NULL. Likewise for prueth_emac_stop().
> 
Yes right, I will update this.
>> +			if (ret) {
>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE0);
>> +				goto free_rx_irq;
>> +			}
>> +		}
>> +		if (prueth->emac[ICSS_SLICE1]) {
>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE1]);
>> +			if (ret) {
>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE1);
>> +				goto halt_slice0_prus;
>> +			}
>> +		}
>> +	}
>> +
>> +	flow_cfg = emac->dram.va + ICSSG_CONFIG_OFFSET + PSI_L_REGULAR_FLOW_ID_BASE_OFFSET;
>> +	writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);
>> +	ret = emac_fdb_flow_id_updated(emac);
>> +
>> +	if (ret) {
>> +		netdev_err(ndev, "Failed to update Rx Flow ID %d", ret);
>> +		goto stop;
> 
> Branching to stop may result in calls to prueth_emac_stop() which does not
> seem symmetrical with the condition on prueth->emacs_initialized for calls
> to prueth_emac_start() above.
> 
> If so, is this intended?
> 
Yes this is intended. This change is made to run all the cores during 
init and in case of any failure, stop all the cores. And even if one 
interface is up, all the cores should run.
>> +	}
>>   
>>   	icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>>   
> 
> ...

Re: [PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load sequence.

[Roger Quadros]

Hi,

On 06/11/2024 09:40, Meghana Malladi wrote:
> From: MD Danish Anwar <danishanwar@ti.com>
> 
> Timesync related operations are ran in PRU0 cores for both ICSSG SLICE0
> and SLICE1. Currently whenever any ICSSG interface comes up we load the
> respective firmwares to PRU cores and whenever interface goes down, we
> stop the respective cores. Due to this, when SLICE0 goes down while
> SLICE1 is still active, PRU0 firmwares are unloaded and PRU0 core is
> stopped. This results in clock jump for SLICE1 interface as the timesync
> related operations are no longer running.
> 
> Fix this by running both PRU0 and PRU1 firmwares as long as at least 1
> ICSSG interface is up.
> 
> rx_flow_id is updated before firmware is loaded. Once firmware is loaded,
> it reads the flow_id and uses it for rx. emac_fdb_flow_id_updated() is
> used to let firmware know that the flow_id has been updated and to use the
> latest rx_flow_id.

is rx_flow_id releated to timesync releated issue that this patch is fixing?
If not please split it into separate patch and mention what functionality
it is fixing.

> 
> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_config.c | 28 ++++++++++
>  drivers/net/ethernet/ti/icssg/icssg_config.h |  1 +
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 58 ++++++++++++++++----
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  1 +
>  4 files changed, 77 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
> index 5d2491c2943a..f1f0c8659e2d 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
> @@ -786,3 +786,31 @@ void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port)
>  		writel(pvid, prueth->shram.va + EMAC_ICSSG_SWITCH_PORT0_DEFAULT_VLAN_OFFSET);
>  }
>  EXPORT_SYMBOL_GPL(icssg_set_pvid);
> +
> +int emac_fdb_flow_id_updated(struct prueth_emac *emac)
> +{
> +	struct mgmt_cmd_rsp fdb_cmd_rsp = { 0 };
> +	int slice = prueth_emac_slice(emac);
> +	struct mgmt_cmd fdb_cmd = { 0 };
> +	int ret = 0;
> +
> +	fdb_cmd.header = ICSSG_FW_MGMT_CMD_HEADER;
> +	fdb_cmd.type   = ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW;
> +	fdb_cmd.seqnum = ++(emac->prueth->icssg_hwcmdseq);
> +	fdb_cmd.param  = 0;
> +
> +	fdb_cmd.param |= (slice << 4);
> +	fdb_cmd.cmd_args[0] = 0;
> +
> +	ret = icssg_send_fdb_msg(emac, &fdb_cmd, &fdb_cmd_rsp);
> +
> +	if (ret)
> +		return ret;
> +
> +	WARN_ON(fdb_cmd.seqnum != fdb_cmd_rsp.seqnum);
> +	if (fdb_cmd_rsp.status == 1)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(emac_fdb_flow_id_updated);
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
> index 92c2deaa3068..c884e9fa099e 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
> @@ -55,6 +55,7 @@ struct icssg_rxq_ctx {
>  #define ICSSG_FW_MGMT_FDB_CMD_TYPE	0x03
>  #define ICSSG_FW_MGMT_CMD_TYPE		0x04
>  #define ICSSG_FW_MGMT_PKT		0x80000000
> +#define ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW	0x05
>  
>  struct icssg_r30_cmd {
>  	u32 cmd[4];
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 0556910938fa..9df67539285b 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -534,6 +534,7 @@ static int emac_ndo_open(struct net_device *ndev)
>  {
>  	struct prueth_emac *emac = netdev_priv(ndev);
>  	int ret, i, num_data_chn = emac->tx_ch_num;
> +	struct icssg_flow_cfg __iomem *flow_cfg;
>  	struct prueth *prueth = emac->prueth;
>  	int slice = prueth_emac_slice(emac);
>  	struct device *dev = prueth->dev;
> @@ -549,8 +550,12 @@ static int emac_ndo_open(struct net_device *ndev)
>  	/* set h/w MAC as user might have re-configured */
>  	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
>  
> +	if (!prueth->emacs_initialized) {
> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
> +	}
> +
>  	icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
> -	icssg_class_default(prueth->miig_rt, slice, 0, false);
>  	icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>  
>  	/* Notify the stack of the actual queue counts. */
> @@ -588,10 +593,31 @@ static int emac_ndo_open(struct net_device *ndev)
>  		goto cleanup_napi;
>  	}
>  
> -	/* reset and start PRU firmware */
> -	ret = prueth_emac_start(prueth, emac);
> -	if (ret)
> -		goto free_rx_irq;
> +	if (!prueth->emacs_initialized) {
> +		if (prueth->emac[ICSS_SLICE0]) {
> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE0]);
> +			if (ret) {
> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE0);
> +				goto free_rx_irq;
> +			}
> +		}
> +		if (prueth->emac[ICSS_SLICE1]) {
> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE1]);
> +			if (ret) {
> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE1);
> +				goto halt_slice0_prus;
> +			}

Did I understand right: SLICE0 needs to be always running if any of the
interface is up but SLICE0 doesn't need to be running if only SLICE0
interface is running.

If yes then you need to update the patch so SLICE1 is not always running.

> +		}
> +	}
> +
> +	flow_cfg = emac->dram.va + ICSSG_CONFIG_OFFSET + PSI_L_REGULAR_FLOW_ID_BASE_OFFSET;
> +	writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);
> +	ret = emac_fdb_flow_id_updated(emac);
> +
> +	if (ret) {
> +		netdev_err(ndev, "Failed to update Rx Flow ID %d", ret);
> +		goto stop;
> +	}
>  
>  	icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>  
> @@ -644,7 +670,11 @@ static int emac_ndo_open(struct net_device *ndev)
>  free_tx_ts_irq:
>  	free_irq(emac->tx_ts_irq, emac);
>  stop:
> -	prueth_emac_stop(emac);
> +	if (prueth->emac[ICSS_SLICE1])
> +		prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
> +halt_slice0_prus:
> +	if (prueth->emac[ICSS_SLICE0])
> +		prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
>  free_rx_irq:
>  	free_irq(emac->rx_chns.irq[rx_flow], emac);
>  cleanup_napi:
> @@ -680,7 +710,10 @@ static int emac_ndo_stop(struct net_device *ndev)
>  	if (ndev->phydev)
>  		phy_stop(ndev->phydev);
>  
> -	icssg_class_disable(prueth->miig_rt, prueth_emac_slice(emac));
> +	if (prueth->emacs_initialized == 1) {
> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE0);
> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE1);
> +	}
>  
>  	if (emac->prueth->is_hsr_offload_mode)
>  		__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
> @@ -719,11 +752,14 @@ static int emac_ndo_stop(struct net_device *ndev)
>  	/* Destroying the queued work in ndo_stop() */
>  	cancel_delayed_work_sync(&emac->stats_work);
>  
> -	if (prueth->emacs_initialized == 1)
> +	if (prueth->emacs_initialized == 1) {
>  		icss_iep_exit(emac->iep);
> -
> -	/* stop PRUs */
> -	prueth_emac_stop(emac);
> +		/* stop PRUs */
> +		if (prueth->emac[ICSS_SLICE0])
> +			prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
> +		if (prueth->emac[ICSS_SLICE1])
> +			prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
> +	}
>  
>  	free_irq(emac->tx_ts_irq, emac);
>  
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index 8722bb4a268a..c4f5f0349ae7 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -365,6 +365,7 @@ void icssg_vtbl_modify(struct prueth_emac *emac, u8 vid, u8 port_mask,
>  		       u8 untag_mask, bool add);
>  u16 icssg_get_pvid(struct prueth_emac *emac);
>  void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port);
> +int emac_fdb_flow_id_updated(struct prueth_emac *emac);
>  #define prueth_napi_to_tx_chn(pnapi) \
>  	container_of(pnapi, struct prueth_tx_chn, napi_tx)
>  

-- 
cheers,
-roger

Re: [PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load sequence.

[Anwar, Md Danish]

On 11/11/2024 6:33 PM, Roger Quadros wrote:
> Hi,
> 
> On 06/11/2024 09:40, Meghana Malladi wrote:
>> From: MD Danish Anwar <danishanwar@ti.com>
>>
>> Timesync related operations are ran in PRU0 cores for both ICSSG SLICE0
>> and SLICE1. Currently whenever any ICSSG interface comes up we load the
>> respective firmwares to PRU cores and whenever interface goes down, we
>> stop the respective cores. Due to this, when SLICE0 goes down while
>> SLICE1 is still active, PRU0 firmwares are unloaded and PRU0 core is
>> stopped. This results in clock jump for SLICE1 interface as the timesync
>> related operations are no longer running.
>>
>> Fix this by running both PRU0 and PRU1 firmwares as long as at least 1
>> ICSSG interface is up.
>>
>> rx_flow_id is updated before firmware is loaded. Once firmware is loaded,
>> it reads the flow_id and uses it for rx. emac_fdb_flow_id_updated() is
>> used to let firmware know that the flow_id has been updated and to use the
>> latest rx_flow_id.
> 
> is rx_flow_id releated to timesync releated issue that this patch is fixing?
> If not please split it into separate patch and mention what functionality
> it is fixing.
> 

Roger, rx_flow_id is not related to timesync. However loading both
SLICE0 and SLICE1 firmware together results in wrong rx_flow_id used by
firmware for the interface that is brought later.

When eth0 (SLICE0) is brought up, it's flow_id is obtained from dma and
written to SMEM. Slice0 and Slice1 both firmwares are loaded. Firmware
reads the flow_id from SMEM and uses it for RX.

Second interface eth1 (SLICE1) comes up, it's flow id is obtained from
dma and written to SMEM. However firmware doesn't read the flow ID
again. It only reads it once when loaded and uses that through out. The
flow id for this interface remains 0 and that results in RX being broken.

To fix this, emac_fdb_flow_id_updated() is added which will let firmware
know that we have updated the flow_id and use the latest one.

This not related to timesync instead related to the fix of timesync
issue. Breaking this into separate patch will result in RX (ICSSG) being
broken at the former patch.

In order to avoid feature breakage at the former patch, the change
related to flow_id update is kept in the same patch.

If you think having the feature broken is OK, the patch can be splitted.
However IMO, these chanegs should be together in one patch.

>>
>> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>> ---
>>  drivers/net/ethernet/ti/icssg/icssg_config.c | 28 ++++++++++
>>  drivers/net/ethernet/ti/icssg/icssg_config.h |  1 +
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 58 ++++++++++++++++----
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  1 +
>>  4 files changed, 77 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> index 5d2491c2943a..f1f0c8659e2d 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> @@ -786,3 +786,31 @@ void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port)
>>  		writel(pvid, prueth->shram.va + EMAC_ICSSG_SWITCH_PORT0_DEFAULT_VLAN_OFFSET);
>>  }
>>  EXPORT_SYMBOL_GPL(icssg_set_pvid);
>> +
>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac)
>> +{
>> +	struct mgmt_cmd_rsp fdb_cmd_rsp = { 0 };
>> +	int slice = prueth_emac_slice(emac);
>> +	struct mgmt_cmd fdb_cmd = { 0 };
>> +	int ret = 0;
>> +
>> +	fdb_cmd.header = ICSSG_FW_MGMT_CMD_HEADER;
>> +	fdb_cmd.type   = ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW;
>> +	fdb_cmd.seqnum = ++(emac->prueth->icssg_hwcmdseq);
>> +	fdb_cmd.param  = 0;
>> +
>> +	fdb_cmd.param |= (slice << 4);
>> +	fdb_cmd.cmd_args[0] = 0;
>> +
>> +	ret = icssg_send_fdb_msg(emac, &fdb_cmd, &fdb_cmd_rsp);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	WARN_ON(fdb_cmd.seqnum != fdb_cmd_rsp.seqnum);
>> +	if (fdb_cmd_rsp.status == 1)
>> +		return 0;
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(emac_fdb_flow_id_updated);
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
>> index 92c2deaa3068..c884e9fa099e 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
>> @@ -55,6 +55,7 @@ struct icssg_rxq_ctx {
>>  #define ICSSG_FW_MGMT_FDB_CMD_TYPE	0x03
>>  #define ICSSG_FW_MGMT_CMD_TYPE		0x04
>>  #define ICSSG_FW_MGMT_PKT		0x80000000
>> +#define ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW	0x05
>>  
>>  struct icssg_r30_cmd {
>>  	u32 cmd[4];
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index 0556910938fa..9df67539285b 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -534,6 +534,7 @@ static int emac_ndo_open(struct net_device *ndev)
>>  {
>>  	struct prueth_emac *emac = netdev_priv(ndev);
>>  	int ret, i, num_data_chn = emac->tx_ch_num;
>> +	struct icssg_flow_cfg __iomem *flow_cfg;
>>  	struct prueth *prueth = emac->prueth;
>>  	int slice = prueth_emac_slice(emac);
>>  	struct device *dev = prueth->dev;
>> @@ -549,8 +550,12 @@ static int emac_ndo_open(struct net_device *ndev)
>>  	/* set h/w MAC as user might have re-configured */
>>  	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
>>  
>> +	if (!prueth->emacs_initialized) {
>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
>> +	}
>> +
>>  	icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>> -	icssg_class_default(prueth->miig_rt, slice, 0, false);
>>  	icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>>  
>>  	/* Notify the stack of the actual queue counts. */
>> @@ -588,10 +593,31 @@ static int emac_ndo_open(struct net_device *ndev)
>>  		goto cleanup_napi;
>>  	}
>>  
>> -	/* reset and start PRU firmware */
>> -	ret = prueth_emac_start(prueth, emac);
>> -	if (ret)
>> -		goto free_rx_irq;
>> +	if (!prueth->emacs_initialized) {
>> +		if (prueth->emac[ICSS_SLICE0]) {
>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE0]);
>> +			if (ret) {
>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE0);
>> +				goto free_rx_irq;
>> +			}
>> +		}
>> +		if (prueth->emac[ICSS_SLICE1]) {
>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE1]);
>> +			if (ret) {
>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE1);
>> +				goto halt_slice0_prus;
>> +			}
> 
> Did I understand right: SLICE0 needs to be always running if any of the
> interface is up but SLICE0 doesn't need to be running if only SLICE0

I think you meant `but SLICE1 doesn't need to be running if only SLICE0`

> interface is running.
> 
> If yes then you need to update the patch so SLICE1 is not always running.
> 

For Timesync - YES. Only slice0 is needed to be always running. However
these both firmwares have some more inter dependencies, timesync is just
one of them. As a result, firmware team has recommended to keep both
Slice0 and Slice1 firmware running as long as at least one ICSSG
interface is up. Stop both firmware only if no ICSSG interface is up.

I think the commit message can be modified to state that the dependecy
is not only SLICE0 but on SLICE1 as well and they both need to be running.

>> +		}
>> +	}
>> +
>> +	flow_cfg = emac->dram.va + ICSSG_CONFIG_OFFSET + PSI_L_REGULAR_FLOW_ID_BASE_OFFSET;
>> +	writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);
>> +	ret = emac_fdb_flow_id_updated(emac);
>> +
>> +	if (ret) {
>> +		netdev_err(ndev, "Failed to update Rx Flow ID %d", ret);
>> +		goto stop;
>> +	}
>>  
>>  	icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>>  
>> @@ -644,7 +670,11 @@ static int emac_ndo_open(struct net_device *ndev)
>>  free_tx_ts_irq:
>>  	free_irq(emac->tx_ts_irq, emac);
>>  stop:
>> -	prueth_emac_stop(emac);
>> +	if (prueth->emac[ICSS_SLICE1])
>> +		prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
>> +halt_slice0_prus:
>> +	if (prueth->emac[ICSS_SLICE0])
>> +		prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
>>  free_rx_irq:
>>  	free_irq(emac->rx_chns.irq[rx_flow], emac);
>>  cleanup_napi:
>> @@ -680,7 +710,10 @@ static int emac_ndo_stop(struct net_device *ndev)
>>  	if (ndev->phydev)
>>  		phy_stop(ndev->phydev);
>>  
>> -	icssg_class_disable(prueth->miig_rt, prueth_emac_slice(emac));
>> +	if (prueth->emacs_initialized == 1) {
>> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE0);
>> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE1);
>> +	}
>>  
>>  	if (emac->prueth->is_hsr_offload_mode)
>>  		__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
>> @@ -719,11 +752,14 @@ static int emac_ndo_stop(struct net_device *ndev)
>>  	/* Destroying the queued work in ndo_stop() */
>>  	cancel_delayed_work_sync(&emac->stats_work);
>>  
>> -	if (prueth->emacs_initialized == 1)
>> +	if (prueth->emacs_initialized == 1) {
>>  		icss_iep_exit(emac->iep);
>> -
>> -	/* stop PRUs */
>> -	prueth_emac_stop(emac);
>> +		/* stop PRUs */
>> +		if (prueth->emac[ICSS_SLICE0])
>> +			prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
>> +		if (prueth->emac[ICSS_SLICE1])
>> +			prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
>> +	}
>>  
>>  	free_irq(emac->tx_ts_irq, emac);
>>  
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> index 8722bb4a268a..c4f5f0349ae7 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -365,6 +365,7 @@ void icssg_vtbl_modify(struct prueth_emac *emac, u8 vid, u8 port_mask,
>>  		       u8 untag_mask, bool add);
>>  u16 icssg_get_pvid(struct prueth_emac *emac);
>>  void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port);
>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac);
>>  #define prueth_napi_to_tx_chn(pnapi) \
>>  	container_of(pnapi, struct prueth_tx_chn, napi_tx)
>>  
> 

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net 2/2] net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init

[Roger Quadros]

Hi,

On 06/11/2024 09:40, Meghana Malladi wrote:
> When ICSSG interfaces are brought down and brought up again, the
> pru cores are shut down and booted again, flushing out all the memories
> and start again in a clean state. Hence it is expected that the
> IEP_CMP_CFG register needs to be flushed during iep_init() to ensure
> that the existing residual configuration doesn't cause any unusual
> behavior. If the register is not cleared, existing IEP_CMP_CFG set for
> CMP1 will result in SYNC0_OUT signal based on the SYNC_OUT register values.
> 
> After bringing the interface up, calling PPS enable doesn't work as
> the driver believes PPS is already enabled, (iep->pps_enabled is not
> cleared during interface bring down) and driver  will just return true
> even though there is no signal. Fix this by setting the iep->pps_enable
> and iep->perout_enable flags to false during the link down.
> 
> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icss_iep.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> index 5d6d1cf78e93..03abc25ced12 100644
> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
> @@ -195,6 +195,12 @@ static void icss_iep_enable_shadow_mode(struct icss_iep *iep)
>  
>  	icss_iep_disable(iep);
>  
> +	/* clear compare config */
> +	for (cmp = IEP_MIN_CMP; cmp < IEP_MAX_CMP; cmp++) {
> +		regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
> +				   IEP_CMP_CFG_CMP_EN(cmp), 0);
> +	}
> +

A bit later we are clearing compare status. Can clearing CMP be done in same for loop?

>  	/* disable shadow mode */
>  	regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
>  			   IEP_CMP_CFG_SHADOW_EN, 0);
> @@ -778,6 +784,10 @@ int icss_iep_exit(struct icss_iep *iep)
>  		ptp_clock_unregister(iep->ptp_clock);
>  		iep->ptp_clock = NULL;
>  	}
> +
> +	iep->pps_enabled = false;
> +	iep->perout_enabled = false;
> +

But how do you keep things in sync with user space?
User might have enabled PPS or PEROUT and then put SLICE0 interface down.
Then if SLICE0 is brought up should PPS/PEROUT keep working like before?
We did call ptp_clock_unregister() so it should unregister the PPS as well.
What I'm not sure is if it calls the ptp->enable() hook to disable the PPS/PEROUT.

If yes then that should take care of the flags as well.

If not then you need to call the relevant hooks explicitly but just after
ptp_clock_unregister().
e.g.
	if (iep->pps_enabled)
		icss_iep_pps_enable(iep, false);
	else if (iep->perout_enabled)
		icss_iep_perout_enable(iep, NULL, false);

But this means that user has to again setup PPS/PEROUT.	

>  	icss_iep_disable(iep);
>  
>  	return 0;

-- 
cheers,
-roger

Re: [PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load sequence.

[Roger Quadros]

On 11/11/2024 15:35, Anwar, Md Danish wrote:
> 
> 
> On 11/11/2024 6:33 PM, Roger Quadros wrote:
>> Hi,
>>
>> On 06/11/2024 09:40, Meghana Malladi wrote:
>>> From: MD Danish Anwar <danishanwar@ti.com>
>>>
>>> Timesync related operations are ran in PRU0 cores for both ICSSG SLICE0
>>> and SLICE1. Currently whenever any ICSSG interface comes up we load the
>>> respective firmwares to PRU cores and whenever interface goes down, we
>>> stop the respective cores. Due to this, when SLICE0 goes down while
>>> SLICE1 is still active, PRU0 firmwares are unloaded and PRU0 core is
>>> stopped. This results in clock jump for SLICE1 interface as the timesync
>>> related operations are no longer running.
>>>
>>> Fix this by running both PRU0 and PRU1 firmwares as long as at least 1
>>> ICSSG interface is up.
>>>
>>> rx_flow_id is updated before firmware is loaded. Once firmware is loaded,
>>> it reads the flow_id and uses it for rx. emac_fdb_flow_id_updated() is
>>> used to let firmware know that the flow_id has been updated and to use the
>>> latest rx_flow_id.
>>
>> is rx_flow_id releated to timesync releated issue that this patch is fixing?
>> If not please split it into separate patch and mention what functionality
>> it is fixing.
>>
> 
> Roger, rx_flow_id is not related to timesync. However loading both
> SLICE0 and SLICE1 firmware together results in wrong rx_flow_id used by
> firmware for the interface that is brought later.
> 
> When eth0 (SLICE0) is brought up, it's flow_id is obtained from dma and
> written to SMEM. Slice0 and Slice1 both firmwares are loaded. Firmware
> reads the flow_id from SMEM and uses it for RX.
> 
> Second interface eth1 (SLICE1) comes up, it's flow id is obtained from
> dma and written to SMEM. However firmware doesn't read the flow ID
> again. It only reads it once when loaded and uses that through out. The
> flow id for this interface remains 0 and that results in RX being broken.
> 
> To fix this, emac_fdb_flow_id_updated() is added which will let firmware
> know that we have updated the flow_id and use the latest one.
> 
> This not related to timesync instead related to the fix of timesync
> issue. Breaking this into separate patch will result in RX (ICSSG) being
> broken at the former patch.
> 
> In order to avoid feature breakage at the former patch, the change
> related to flow_id update is kept in the same patch.

But setting the Flow ID should be taken care by icssg_config() which is
done in prueth_emac_start()?

is it really OK to provide wrong flow id to the Firmware or even start
the second PRU core without DMA/IRQ resources allocated to it?

I will suggest to introduce emac_ndo_common_open() and emac_ndo_common_stop()
and move the common code there i.e. allocating/freeing resources for both
cores and starting/stopping both cores.

> 
> If you think having the feature broken is OK, the patch can be splitted.
> However IMO, these chanegs should be together in one patch.
> 
>>>
>>> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>>> ---
>>>  drivers/net/ethernet/ti/icssg/icssg_config.c | 28 ++++++++++
>>>  drivers/net/ethernet/ti/icssg/icssg_config.h |  1 +
>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 58 ++++++++++++++++----
>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  1 +
>>>  4 files changed, 77 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>> index 5d2491c2943a..f1f0c8659e2d 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>> @@ -786,3 +786,31 @@ void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port)
>>>  		writel(pvid, prueth->shram.va + EMAC_ICSSG_SWITCH_PORT0_DEFAULT_VLAN_OFFSET);
>>>  }
>>>  EXPORT_SYMBOL_GPL(icssg_set_pvid);
>>> +
>>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac)
>>> +{
>>> +	struct mgmt_cmd_rsp fdb_cmd_rsp = { 0 };
>>> +	int slice = prueth_emac_slice(emac);
>>> +	struct mgmt_cmd fdb_cmd = { 0 };
>>> +	int ret = 0;
>>> +
>>> +	fdb_cmd.header = ICSSG_FW_MGMT_CMD_HEADER;
>>> +	fdb_cmd.type   = ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW;
>>> +	fdb_cmd.seqnum = ++(emac->prueth->icssg_hwcmdseq);
>>> +	fdb_cmd.param  = 0;
>>> +
>>> +	fdb_cmd.param |= (slice << 4);
>>> +	fdb_cmd.cmd_args[0] = 0;
>>> +
>>> +	ret = icssg_send_fdb_msg(emac, &fdb_cmd, &fdb_cmd_rsp);
>>> +
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	WARN_ON(fdb_cmd.seqnum != fdb_cmd_rsp.seqnum);
>>> +	if (fdb_cmd_rsp.status == 1)
>>> +		return 0;
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +EXPORT_SYMBOL_GPL(emac_fdb_flow_id_updated);
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>> index 92c2deaa3068..c884e9fa099e 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>> @@ -55,6 +55,7 @@ struct icssg_rxq_ctx {
>>>  #define ICSSG_FW_MGMT_FDB_CMD_TYPE	0x03
>>>  #define ICSSG_FW_MGMT_CMD_TYPE		0x04
>>>  #define ICSSG_FW_MGMT_PKT		0x80000000
>>> +#define ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW	0x05
>>>  
>>>  struct icssg_r30_cmd {
>>>  	u32 cmd[4];
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>> index 0556910938fa..9df67539285b 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>> @@ -534,6 +534,7 @@ static int emac_ndo_open(struct net_device *ndev)
>>>  {
>>>  	struct prueth_emac *emac = netdev_priv(ndev);
>>>  	int ret, i, num_data_chn = emac->tx_ch_num;
>>> +	struct icssg_flow_cfg __iomem *flow_cfg;
>>>  	struct prueth *prueth = emac->prueth;
>>>  	int slice = prueth_emac_slice(emac);
>>>  	struct device *dev = prueth->dev;
>>> @@ -549,8 +550,12 @@ static int emac_ndo_open(struct net_device *ndev)
>>>  	/* set h/w MAC as user might have re-configured */
>>>  	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
>>>  
>>> +	if (!prueth->emacs_initialized) {
>>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
>>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
>>> +	}
>>> +
>>>  	icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>>> -	icssg_class_default(prueth->miig_rt, slice, 0, false);
>>>  	icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>>>  
>>>  	/* Notify the stack of the actual queue counts. */
>>> @@ -588,10 +593,31 @@ static int emac_ndo_open(struct net_device *ndev)
>>>  		goto cleanup_napi;
>>>  	}
>>>  
>>> -	/* reset and start PRU firmware */
>>> -	ret = prueth_emac_start(prueth, emac);
>>> -	if (ret)
>>> -		goto free_rx_irq;
>>> +	if (!prueth->emacs_initialized) {
>>> +		if (prueth->emac[ICSS_SLICE0]) {
>>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE0]);
>>> +			if (ret) {
>>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE0);
>>> +				goto free_rx_irq;
>>> +			}
>>> +		}
>>> +		if (prueth->emac[ICSS_SLICE1]) {
>>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE1]);
>>> +			if (ret) {
>>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE1);
>>> +				goto halt_slice0_prus;
>>> +			}
>>
>> Did I understand right: SLICE0 needs to be always running if any of the
>> interface is up but SLICE0 doesn't need to be running if only SLICE0
> 
> I think you meant `but SLICE1 doesn't need to be running if only SLICE0`
> 
>> interface is running.
>>
>> If yes then you need to update the patch so SLICE1 is not always running.
>>
> 
> For Timesync - YES. Only slice0 is needed to be always running. However
> these both firmwares have some more inter dependencies, timesync is just
> one of them. As a result, firmware team has recommended to keep both
> Slice0 and Slice1 firmware running as long as at least one ICSSG
> interface is up. Stop both firmware only if no ICSSG interface is up.
> 
> I think the commit message can be modified to state that the dependecy
> is not only SLICE0 but on SLICE1 as well and they both need to be running.
> 

OK Please mention this in commit log.

>>> +		}
>>> +	}
>>> +
>>> +	flow_cfg = emac->dram.va + ICSSG_CONFIG_OFFSET + PSI_L_REGULAR_FLOW_ID_BASE_OFFSET;
>>> +	writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);
>>> +	ret = emac_fdb_flow_id_updated(emac);
>>> +
>>> +	if (ret) {
>>> +		netdev_err(ndev, "Failed to update Rx Flow ID %d", ret);
>>> +		goto stop;
>>> +	}
>>>  
>>>  	icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>>>  
>>> @@ -644,7 +670,11 @@ static int emac_ndo_open(struct net_device *ndev)
>>>  free_tx_ts_irq:
>>>  	free_irq(emac->tx_ts_irq, emac);
>>>  stop:
>>> -	prueth_emac_stop(emac);
>>> +	if (prueth->emac[ICSS_SLICE1])
>>> +		prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
>>> +halt_slice0_prus:
>>> +	if (prueth->emac[ICSS_SLICE0])
>>> +		prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
>>>  free_rx_irq:
>>>  	free_irq(emac->rx_chns.irq[rx_flow], emac);
>>>  cleanup_napi:
>>> @@ -680,7 +710,10 @@ static int emac_ndo_stop(struct net_device *ndev)
>>>  	if (ndev->phydev)
>>>  		phy_stop(ndev->phydev);
>>>  
>>> -	icssg_class_disable(prueth->miig_rt, prueth_emac_slice(emac));
>>> +	if (prueth->emacs_initialized == 1) {
>>> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE0);
>>> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE1);
>>> +	}
>>>  
>>>  	if (emac->prueth->is_hsr_offload_mode)
>>>  		__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
>>> @@ -719,11 +752,14 @@ static int emac_ndo_stop(struct net_device *ndev)
>>>  	/* Destroying the queued work in ndo_stop() */
>>>  	cancel_delayed_work_sync(&emac->stats_work);
>>>  
>>> -	if (prueth->emacs_initialized == 1)
>>> +	if (prueth->emacs_initialized == 1) {
>>>  		icss_iep_exit(emac->iep);
>>> -
>>> -	/* stop PRUs */
>>> -	prueth_emac_stop(emac);
>>> +		/* stop PRUs */
>>> +		if (prueth->emac[ICSS_SLICE0])
>>> +			prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
>>> +		if (prueth->emac[ICSS_SLICE1])
>>> +			prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
>>> +	}
>>>  
>>>  	free_irq(emac->tx_ts_irq, emac);
>>>  
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>> index 8722bb4a268a..c4f5f0349ae7 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>> @@ -365,6 +365,7 @@ void icssg_vtbl_modify(struct prueth_emac *emac, u8 vid, u8 port_mask,
>>>  		       u8 untag_mask, bool add);
>>>  u16 icssg_get_pvid(struct prueth_emac *emac);
>>>  void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port);
>>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac);
>>>  #define prueth_napi_to_tx_chn(pnapi) \
>>>  	container_of(pnapi, struct prueth_tx_chn, napi_tx)
>>>  
>>
> 

-- 
cheers,
-roger

Re: [PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load sequence.

[Anwar, Md Danish]

On 11/11/2024 7:55 PM, Roger Quadros wrote:
> 
> 
> On 11/11/2024 15:35, Anwar, Md Danish wrote:
>>
>>
>> On 11/11/2024 6:33 PM, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 06/11/2024 09:40, Meghana Malladi wrote:
>>>> From: MD Danish Anwar <danishanwar@ti.com>
>>>>
>>>> Timesync related operations are ran in PRU0 cores for both ICSSG SLICE0
>>>> and SLICE1. Currently whenever any ICSSG interface comes up we load the
>>>> respective firmwares to PRU cores and whenever interface goes down, we
>>>> stop the respective cores. Due to this, when SLICE0 goes down while
>>>> SLICE1 is still active, PRU0 firmwares are unloaded and PRU0 core is
>>>> stopped. This results in clock jump for SLICE1 interface as the timesync
>>>> related operations are no longer running.
>>>>
>>>> Fix this by running both PRU0 and PRU1 firmwares as long as at least 1
>>>> ICSSG interface is up.
>>>>
>>>> rx_flow_id is updated before firmware is loaded. Once firmware is loaded,
>>>> it reads the flow_id and uses it for rx. emac_fdb_flow_id_updated() is
>>>> used to let firmware know that the flow_id has been updated and to use the
>>>> latest rx_flow_id.
>>>
>>> is rx_flow_id releated to timesync releated issue that this patch is fixing?
>>> If not please split it into separate patch and mention what functionality
>>> it is fixing.
>>>
>>
>> Roger, rx_flow_id is not related to timesync. However loading both
>> SLICE0 and SLICE1 firmware together results in wrong rx_flow_id used by
>> firmware for the interface that is brought later.
>>
>> When eth0 (SLICE0) is brought up, it's flow_id is obtained from dma and
>> written to SMEM. Slice0 and Slice1 both firmwares are loaded. Firmware
>> reads the flow_id from SMEM and uses it for RX.
>>
>> Second interface eth1 (SLICE1) comes up, it's flow id is obtained from
>> dma and written to SMEM. However firmware doesn't read the flow ID
>> again. It only reads it once when loaded and uses that through out. The
>> flow id for this interface remains 0 and that results in RX being broken.
>>
>> To fix this, emac_fdb_flow_id_updated() is added which will let firmware
>> know that we have updated the flow_id and use the latest one.
>>
>> This not related to timesync instead related to the fix of timesync
>> issue. Breaking this into separate patch will result in RX (ICSSG) being
>> broken at the former patch.
>>
>> In order to avoid feature breakage at the former patch, the change
>> related to flow_id update is kept in the same patch.
> 
> But setting the Flow ID should be taken care by icssg_config() which is
> done in prueth_emac_start()?
> 

Correct. But prueth_emac_start() is called once for both the slices.
When eth0 comes up prueth_emac_start() will be called for both slice0
and slice1. When eth1 comes up prueth_emac_start() will not get called.

In icssg_config() we write
`writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);`

By default emac->rx_flow_id_base = 0, it is populated during
prueth_init_rx_chns() which is called per port / slice. So the second
port's actual flow_id will be discarded without the
emac_fdb_flow_id_updated() API.

> is it really OK to provide wrong flow id to the Firmware or even start
> the second PRU core without DMA/IRQ resources allocated to it?
> 

It is ok as firmware doesn't transmit any packets until driver issues
ICSSG_EMAC_PORT_FORWARD command which is done during link up / down in
emac_adjust_link() so it would be okay for firmware to have wrong flow
id as we only issue ICSSG_EMAC_PORT_FORWARD after link is detected and
flow id is updated.

> I will suggest to introduce emac_ndo_common_open() and emac_ndo_common_stop()
> and move the common code there i.e. allocating/freeing resources for both
> cores and starting/stopping both cores.
> 

I don't think that will help much. There are certain things that need to
be done only once for both ports where as certain things needs to be
done on a per port basis. In order for driver to work it has to follow a
proper sequence in which first few common APIs are called then
individual APIs and then again common APIs. I think trying to
consolidate all common within a function will result in this sequence
getting not followed properly.

The fairly simple way is to put all common code inside `if
(!prueth->emacs_initialized)` where as keep all the individual port code
without any condition.

>>
>> If you think having the feature broken is OK, the patch can be splitted.
>> However IMO, these chanegs should be together in one patch.
>>
>>>>
>>>> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>>>> ---
>>>>  drivers/net/ethernet/ti/icssg/icssg_config.c | 28 ++++++++++
>>>>  drivers/net/ethernet/ti/icssg/icssg_config.h |  1 +
>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 58 ++++++++++++++++----
>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  1 +
>>>>  4 files changed, 77 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>> index 5d2491c2943a..f1f0c8659e2d 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>> @@ -786,3 +786,31 @@ void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port)
>>>>  		writel(pvid, prueth->shram.va + EMAC_ICSSG_SWITCH_PORT0_DEFAULT_VLAN_OFFSET);
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(icssg_set_pvid);
>>>> +
>>>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac)
>>>> +{
>>>> +	struct mgmt_cmd_rsp fdb_cmd_rsp = { 0 };
>>>> +	int slice = prueth_emac_slice(emac);
>>>> +	struct mgmt_cmd fdb_cmd = { 0 };
>>>> +	int ret = 0;
>>>> +
>>>> +	fdb_cmd.header = ICSSG_FW_MGMT_CMD_HEADER;
>>>> +	fdb_cmd.type   = ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW;
>>>> +	fdb_cmd.seqnum = ++(emac->prueth->icssg_hwcmdseq);
>>>> +	fdb_cmd.param  = 0;
>>>> +
>>>> +	fdb_cmd.param |= (slice << 4);
>>>> +	fdb_cmd.cmd_args[0] = 0;
>>>> +
>>>> +	ret = icssg_send_fdb_msg(emac, &fdb_cmd, &fdb_cmd_rsp);
>>>> +
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	WARN_ON(fdb_cmd.seqnum != fdb_cmd_rsp.seqnum);
>>>> +	if (fdb_cmd_rsp.status == 1)
>>>> +		return 0;
>>>> +
>>>> +	return -EINVAL;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(emac_fdb_flow_id_updated);
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>> index 92c2deaa3068..c884e9fa099e 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>> @@ -55,6 +55,7 @@ struct icssg_rxq_ctx {
>>>>  #define ICSSG_FW_MGMT_FDB_CMD_TYPE	0x03
>>>>  #define ICSSG_FW_MGMT_CMD_TYPE		0x04
>>>>  #define ICSSG_FW_MGMT_PKT		0x80000000
>>>> +#define ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW	0x05
>>>>  
>>>>  struct icssg_r30_cmd {
>>>>  	u32 cmd[4];
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>> index 0556910938fa..9df67539285b 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>> @@ -534,6 +534,7 @@ static int emac_ndo_open(struct net_device *ndev)
>>>>  {
>>>>  	struct prueth_emac *emac = netdev_priv(ndev);
>>>>  	int ret, i, num_data_chn = emac->tx_ch_num;
>>>> +	struct icssg_flow_cfg __iomem *flow_cfg;
>>>>  	struct prueth *prueth = emac->prueth;
>>>>  	int slice = prueth_emac_slice(emac);
>>>>  	struct device *dev = prueth->dev;
>>>> @@ -549,8 +550,12 @@ static int emac_ndo_open(struct net_device *ndev)
>>>>  	/* set h/w MAC as user might have re-configured */
>>>>  	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
>>>>  
>>>> +	if (!prueth->emacs_initialized) {
>>>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
>>>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
>>>> +	}
>>>> +
>>>>  	icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>>>> -	icssg_class_default(prueth->miig_rt, slice, 0, false);
>>>>  	icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>>>>  
>>>>  	/* Notify the stack of the actual queue counts. */
>>>> @@ -588,10 +593,31 @@ static int emac_ndo_open(struct net_device *ndev)
>>>>  		goto cleanup_napi;
>>>>  	}
>>>>  
>>>> -	/* reset and start PRU firmware */
>>>> -	ret = prueth_emac_start(prueth, emac);
>>>> -	if (ret)
>>>> -		goto free_rx_irq;
>>>> +	if (!prueth->emacs_initialized) {
>>>> +		if (prueth->emac[ICSS_SLICE0]) {
>>>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE0]);
>>>> +			if (ret) {
>>>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE0);
>>>> +				goto free_rx_irq;
>>>> +			}
>>>> +		}
>>>> +		if (prueth->emac[ICSS_SLICE1]) {
>>>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE1]);
>>>> +			if (ret) {
>>>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE1);
>>>> +				goto halt_slice0_prus;
>>>> +			}
>>>
>>> Did I understand right: SLICE0 needs to be always running if any of the
>>> interface is up but SLICE0 doesn't need to be running if only SLICE0
>>
>> I think you meant `but SLICE1 doesn't need to be running if only SLICE0`
>>
>>> interface is running.
>>>
>>> If yes then you need to update the patch so SLICE1 is not always running.
>>>
>>
>> For Timesync - YES. Only slice0 is needed to be always running. However
>> these both firmwares have some more inter dependencies, timesync is just
>> one of them. As a result, firmware team has recommended to keep both
>> Slice0 and Slice1 firmware running as long as at least one ICSSG
>> interface is up. Stop both firmware only if no ICSSG interface is up.
>>
>> I think the commit message can be modified to state that the dependecy
>> is not only SLICE0 but on SLICE1 as well and they both need to be running.
>>
> 
> OK Please mention this in commit log.
> 
>>>> +		}
>>>> +	}
>>>> +
>>>> +	flow_cfg = emac->dram.va + ICSSG_CONFIG_OFFSET + PSI_L_REGULAR_FLOW_ID_BASE_OFFSET;
>>>> +	writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);
>>>> +	ret = emac_fdb_flow_id_updated(emac);
>>>> +
>>>> +	if (ret) {
>>>> +		netdev_err(ndev, "Failed to update Rx Flow ID %d", ret);
>>>> +		goto stop;
>>>> +	}
>>>>  
>>>>  	icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>>>>  
>>>> @@ -644,7 +670,11 @@ static int emac_ndo_open(struct net_device *ndev)
>>>>  free_tx_ts_irq:
>>>>  	free_irq(emac->tx_ts_irq, emac);
>>>>  stop:
>>>> -	prueth_emac_stop(emac);
>>>> +	if (prueth->emac[ICSS_SLICE1])
>>>> +		prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
>>>> +halt_slice0_prus:
>>>> +	if (prueth->emac[ICSS_SLICE0])
>>>> +		prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
>>>>  free_rx_irq:
>>>>  	free_irq(emac->rx_chns.irq[rx_flow], emac);
>>>>  cleanup_napi:
>>>> @@ -680,7 +710,10 @@ static int emac_ndo_stop(struct net_device *ndev)
>>>>  	if (ndev->phydev)
>>>>  		phy_stop(ndev->phydev);
>>>>  
>>>> -	icssg_class_disable(prueth->miig_rt, prueth_emac_slice(emac));
>>>> +	if (prueth->emacs_initialized == 1) {
>>>> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE0);
>>>> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE1);
>>>> +	}
>>>>  
>>>>  	if (emac->prueth->is_hsr_offload_mode)
>>>>  		__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
>>>> @@ -719,11 +752,14 @@ static int emac_ndo_stop(struct net_device *ndev)
>>>>  	/* Destroying the queued work in ndo_stop() */
>>>>  	cancel_delayed_work_sync(&emac->stats_work);
>>>>  
>>>> -	if (prueth->emacs_initialized == 1)
>>>> +	if (prueth->emacs_initialized == 1) {
>>>>  		icss_iep_exit(emac->iep);
>>>> -
>>>> -	/* stop PRUs */
>>>> -	prueth_emac_stop(emac);
>>>> +		/* stop PRUs */
>>>> +		if (prueth->emac[ICSS_SLICE0])
>>>> +			prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
>>>> +		if (prueth->emac[ICSS_SLICE1])
>>>> +			prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
>>>> +	}
>>>>  
>>>>  	free_irq(emac->tx_ts_irq, emac);
>>>>  
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>> index 8722bb4a268a..c4f5f0349ae7 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>> @@ -365,6 +365,7 @@ void icssg_vtbl_modify(struct prueth_emac *emac, u8 vid, u8 port_mask,
>>>>  		       u8 untag_mask, bool add);
>>>>  u16 icssg_get_pvid(struct prueth_emac *emac);
>>>>  void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port);
>>>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac);
>>>>  #define prueth_napi_to_tx_chn(pnapi) \
>>>>  	container_of(pnapi, struct prueth_tx_chn, napi_tx)
>>>>  
>>>
>>
> 

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load sequence.

[Roger Quadros]

On 11/11/2024 17:29, Anwar, Md Danish wrote:
> 
> 
> On 11/11/2024 7:55 PM, Roger Quadros wrote:
>>
>>
>> On 11/11/2024 15:35, Anwar, Md Danish wrote:
>>>
>>>
>>> On 11/11/2024 6:33 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 06/11/2024 09:40, Meghana Malladi wrote:
>>>>> From: MD Danish Anwar <danishanwar@ti.com>
>>>>>
>>>>> Timesync related operations are ran in PRU0 cores for both ICSSG SLICE0
>>>>> and SLICE1. Currently whenever any ICSSG interface comes up we load the
>>>>> respective firmwares to PRU cores and whenever interface goes down, we
>>>>> stop the respective cores. Due to this, when SLICE0 goes down while
>>>>> SLICE1 is still active, PRU0 firmwares are unloaded and PRU0 core is
>>>>> stopped. This results in clock jump for SLICE1 interface as the timesync
>>>>> related operations are no longer running.
>>>>>
>>>>> Fix this by running both PRU0 and PRU1 firmwares as long as at least 1
>>>>> ICSSG interface is up.
>>>>>
>>>>> rx_flow_id is updated before firmware is loaded. Once firmware is loaded,
>>>>> it reads the flow_id and uses it for rx. emac_fdb_flow_id_updated() is
>>>>> used to let firmware know that the flow_id has been updated and to use the
>>>>> latest rx_flow_id.
>>>>
>>>> is rx_flow_id releated to timesync releated issue that this patch is fixing?
>>>> If not please split it into separate patch and mention what functionality
>>>> it is fixing.
>>>>
>>>
>>> Roger, rx_flow_id is not related to timesync. However loading both
>>> SLICE0 and SLICE1 firmware together results in wrong rx_flow_id used by
>>> firmware for the interface that is brought later.
>>>
>>> When eth0 (SLICE0) is brought up, it's flow_id is obtained from dma and
>>> written to SMEM. Slice0 and Slice1 both firmwares are loaded. Firmware
>>> reads the flow_id from SMEM and uses it for RX.
>>>
>>> Second interface eth1 (SLICE1) comes up, it's flow id is obtained from
>>> dma and written to SMEM. However firmware doesn't read the flow ID
>>> again. It only reads it once when loaded and uses that through out. The
>>> flow id for this interface remains 0 and that results in RX being broken.
>>>
>>> To fix this, emac_fdb_flow_id_updated() is added which will let firmware
>>> know that we have updated the flow_id and use the latest one.
>>>
>>> This not related to timesync instead related to the fix of timesync
>>> issue. Breaking this into separate patch will result in RX (ICSSG) being
>>> broken at the former patch.
>>>
>>> In order to avoid feature breakage at the former patch, the change
>>> related to flow_id update is kept in the same patch.
>>
>> But setting the Flow ID should be taken care by icssg_config() which is
>> done in prueth_emac_start()?
>>
> 
> Correct. But prueth_emac_start() is called once for both the slices.
> When eth0 comes up prueth_emac_start() will be called for both slice0
> and slice1. When eth1 comes up prueth_emac_start() will not get called.
> 
> In icssg_config() we write
> `writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);`
> 
> By default emac->rx_flow_id_base = 0, it is populated during
> prueth_init_rx_chns() which is called per port / slice. So the second
> port's actual flow_id will be discarded without the
> emac_fdb_flow_id_updated() API.
> 
>> is it really OK to provide wrong flow id to the Firmware or even start
>> the second PRU core without DMA/IRQ resources allocated to it?
>>
> 
> It is ok as firmware doesn't transmit any packets until driver issues
> ICSSG_EMAC_PORT_FORWARD command which is done during link up / down in
> emac_adjust_link() so it would be okay for firmware to have wrong flow
> id as we only issue ICSSG_EMAC_PORT_FORWARD after link is detected and
> flow id is updated.
> 
>> I will suggest to introduce emac_ndo_common_open() and emac_ndo_common_stop()
>> and move the common code there i.e. allocating/freeing resources for both
>> cores and starting/stopping both cores.
>>
> 
> I don't think that will help much. There are certain things that need to
> be done only once for both ports where as certain things needs to be
> done on a per port basis. In order for driver to work it has to follow a
> proper sequence in which first few common APIs are called then
> individual APIs and then again common APIs. I think trying to
> consolidate all common within a function will result in this sequence
> getting not followed properly.
> 
> The fairly simple way is to put all common code inside `if
> (!prueth->emacs_initialized)` where as keep all the individual port code
> without any condition.

It is about code organisation. Please see am65-cpsw-nuss.c for example.
You will call the common_open() from individual port code if (!prueth->emac_initialized)
and vice versa common_stop() in port stop path.

> 
>>>
>>> If you think having the feature broken is OK, the patch can be splitted.
>>> However IMO, these chanegs should be together in one patch.
>>>
>>>>>
>>>>> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>>>>> ---
>>>>>  drivers/net/ethernet/ti/icssg/icssg_config.c | 28 ++++++++++
>>>>>  drivers/net/ethernet/ti/icssg/icssg_config.h |  1 +
>>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 58 ++++++++++++++++----
>>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  1 +
>>>>>  4 files changed, 77 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>>> index 5d2491c2943a..f1f0c8659e2d 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>>> @@ -786,3 +786,31 @@ void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port)
>>>>>  		writel(pvid, prueth->shram.va + EMAC_ICSSG_SWITCH_PORT0_DEFAULT_VLAN_OFFSET);
>>>>>  }
>>>>>  EXPORT_SYMBOL_GPL(icssg_set_pvid);
>>>>> +
>>>>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac)
>>>>> +{
>>>>> +	struct mgmt_cmd_rsp fdb_cmd_rsp = { 0 };
>>>>> +	int slice = prueth_emac_slice(emac);
>>>>> +	struct mgmt_cmd fdb_cmd = { 0 };
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	fdb_cmd.header = ICSSG_FW_MGMT_CMD_HEADER;
>>>>> +	fdb_cmd.type   = ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW;
>>>>> +	fdb_cmd.seqnum = ++(emac->prueth->icssg_hwcmdseq);
>>>>> +	fdb_cmd.param  = 0;
>>>>> +
>>>>> +	fdb_cmd.param |= (slice << 4);
>>>>> +	fdb_cmd.cmd_args[0] = 0;
>>>>> +
>>>>> +	ret = icssg_send_fdb_msg(emac, &fdb_cmd, &fdb_cmd_rsp);
>>>>> +
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	WARN_ON(fdb_cmd.seqnum != fdb_cmd_rsp.seqnum);
>>>>> +	if (fdb_cmd_rsp.status == 1)
>>>>> +		return 0;
>>>>> +
>>>>> +	return -EINVAL;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(emac_fdb_flow_id_updated);
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>>> index 92c2deaa3068..c884e9fa099e 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>>> @@ -55,6 +55,7 @@ struct icssg_rxq_ctx {
>>>>>  #define ICSSG_FW_MGMT_FDB_CMD_TYPE	0x03
>>>>>  #define ICSSG_FW_MGMT_CMD_TYPE		0x04
>>>>>  #define ICSSG_FW_MGMT_PKT		0x80000000
>>>>> +#define ICSSG_FW_MGMT_FDB_CMD_TYPE_RX_FLOW	0x05
>>>>>  
>>>>>  struct icssg_r30_cmd {
>>>>>  	u32 cmd[4];
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>>> index 0556910938fa..9df67539285b 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>>> @@ -534,6 +534,7 @@ static int emac_ndo_open(struct net_device *ndev)
>>>>>  {
>>>>>  	struct prueth_emac *emac = netdev_priv(ndev);
>>>>>  	int ret, i, num_data_chn = emac->tx_ch_num;
>>>>> +	struct icssg_flow_cfg __iomem *flow_cfg;
>>>>>  	struct prueth *prueth = emac->prueth;
>>>>>  	int slice = prueth_emac_slice(emac);
>>>>>  	struct device *dev = prueth->dev;
>>>>> @@ -549,8 +550,12 @@ static int emac_ndo_open(struct net_device *ndev)
>>>>>  	/* set h/w MAC as user might have re-configured */
>>>>>  	ether_addr_copy(emac->mac_addr, ndev->dev_addr);
>>>>>  
>>>>> +	if (!prueth->emacs_initialized) {
>>>>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false);
>>>>> +		icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false);
>>>>> +	}
>>>>> +
>>>>>  	icssg_class_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>>>>> -	icssg_class_default(prueth->miig_rt, slice, 0, false);
>>>>>  	icssg_ft1_set_mac_addr(prueth->miig_rt, slice, emac->mac_addr);
>>>>>  
>>>>>  	/* Notify the stack of the actual queue counts. */
>>>>> @@ -588,10 +593,31 @@ static int emac_ndo_open(struct net_device *ndev)
>>>>>  		goto cleanup_napi;
>>>>>  	}
>>>>>  
>>>>> -	/* reset and start PRU firmware */
>>>>> -	ret = prueth_emac_start(prueth, emac);
>>>>> -	if (ret)
>>>>> -		goto free_rx_irq;
>>>>> +	if (!prueth->emacs_initialized) {
>>>>> +		if (prueth->emac[ICSS_SLICE0]) {
>>>>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE0]);
>>>>> +			if (ret) {
>>>>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE0);
>>>>> +				goto free_rx_irq;
>>>>> +			}
>>>>> +		}
>>>>> +		if (prueth->emac[ICSS_SLICE1]) {
>>>>> +			ret = prueth_emac_start(prueth, prueth->emac[ICSS_SLICE1]);
>>>>> +			if (ret) {
>>>>> +				netdev_err(ndev, "unable to start fw for slice %d", ICSS_SLICE1);
>>>>> +				goto halt_slice0_prus;
>>>>> +			}
>>>>
>>>> Did I understand right: SLICE0 needs to be always running if any of the
>>>> interface is up but SLICE0 doesn't need to be running if only SLICE0
>>>
>>> I think you meant `but SLICE1 doesn't need to be running if only SLICE0`
>>>
>>>> interface is running.
>>>>
>>>> If yes then you need to update the patch so SLICE1 is not always running.
>>>>
>>>
>>> For Timesync - YES. Only slice0 is needed to be always running. However
>>> these both firmwares have some more inter dependencies, timesync is just
>>> one of them. As a result, firmware team has recommended to keep both
>>> Slice0 and Slice1 firmware running as long as at least one ICSSG
>>> interface is up. Stop both firmware only if no ICSSG interface is up.
>>>
>>> I think the commit message can be modified to state that the dependecy
>>> is not only SLICE0 but on SLICE1 as well and they both need to be running.
>>>
>>
>> OK Please mention this in commit log.
>>
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	flow_cfg = emac->dram.va + ICSSG_CONFIG_OFFSET + PSI_L_REGULAR_FLOW_ID_BASE_OFFSET;
>>>>> +	writew(emac->rx_flow_id_base, &flow_cfg->rx_base_flow);
>>>>> +	ret = emac_fdb_flow_id_updated(emac);
>>>>> +
>>>>> +	if (ret) {
>>>>> +		netdev_err(ndev, "Failed to update Rx Flow ID %d", ret);
>>>>> +		goto stop;
>>>>> +	}
>>>>>  
>>>>>  	icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu);
>>>>>  
>>>>> @@ -644,7 +670,11 @@ static int emac_ndo_open(struct net_device *ndev)
>>>>>  free_tx_ts_irq:
>>>>>  	free_irq(emac->tx_ts_irq, emac);
>>>>>  stop:
>>>>> -	prueth_emac_stop(emac);
>>>>> +	if (prueth->emac[ICSS_SLICE1])
>>>>> +		prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
>>>>> +halt_slice0_prus:
>>>>> +	if (prueth->emac[ICSS_SLICE0])
>>>>> +		prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
>>>>>  free_rx_irq:
>>>>>  	free_irq(emac->rx_chns.irq[rx_flow], emac);
>>>>>  cleanup_napi:
>>>>> @@ -680,7 +710,10 @@ static int emac_ndo_stop(struct net_device *ndev)
>>>>>  	if (ndev->phydev)
>>>>>  		phy_stop(ndev->phydev);
>>>>>  
>>>>> -	icssg_class_disable(prueth->miig_rt, prueth_emac_slice(emac));
>>>>> +	if (prueth->emacs_initialized == 1) {
>>>>> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE0);
>>>>> +		icssg_class_disable(prueth->miig_rt, ICSS_SLICE1);
>>>>> +	}
>>>>>  
>>>>>  	if (emac->prueth->is_hsr_offload_mode)
>>>>>  		__dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast);
>>>>> @@ -719,11 +752,14 @@ static int emac_ndo_stop(struct net_device *ndev)
>>>>>  	/* Destroying the queued work in ndo_stop() */
>>>>>  	cancel_delayed_work_sync(&emac->stats_work);
>>>>>  
>>>>> -	if (prueth->emacs_initialized == 1)
>>>>> +	if (prueth->emacs_initialized == 1) {
>>>>>  		icss_iep_exit(emac->iep);
>>>>> -
>>>>> -	/* stop PRUs */
>>>>> -	prueth_emac_stop(emac);
>>>>> +		/* stop PRUs */
>>>>> +		if (prueth->emac[ICSS_SLICE0])
>>>>> +			prueth_emac_stop(prueth->emac[ICSS_SLICE0]);
>>>>> +		if (prueth->emac[ICSS_SLICE1])
>>>>> +			prueth_emac_stop(prueth->emac[ICSS_SLICE1]);
>>>>> +	}
>>>>>  
>>>>>  	free_irq(emac->tx_ts_irq, emac);
>>>>>  
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>>> index 8722bb4a268a..c4f5f0349ae7 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>>> @@ -365,6 +365,7 @@ void icssg_vtbl_modify(struct prueth_emac *emac, u8 vid, u8 port_mask,
>>>>>  		       u8 untag_mask, bool add);
>>>>>  u16 icssg_get_pvid(struct prueth_emac *emac);
>>>>>  void icssg_set_pvid(struct prueth *prueth, u8 vid, u8 port);
>>>>> +int emac_fdb_flow_id_updated(struct prueth_emac *emac);
>>>>>  #define prueth_napi_to_tx_chn(pnapi) \
>>>>>  	container_of(pnapi, struct prueth_tx_chn, napi_tx)
>>>>>  
>>>>
>>>
>>
> 

-- 
cheers,
-roger

Re: [PATCH net 2/2] net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init

[Meghana Malladi]

On 11/11/24 19:23, Roger Quadros wrote:
> Hi,
> 
> On 06/11/2024 09:40, Meghana Malladi wrote:
>> When ICSSG interfaces are brought down and brought up again, the
>> pru cores are shut down and booted again, flushing out all the memories
>> and start again in a clean state. Hence it is expected that the
>> IEP_CMP_CFG register needs to be flushed during iep_init() to ensure
>> that the existing residual configuration doesn't cause any unusual
>> behavior. If the register is not cleared, existing IEP_CMP_CFG set for
>> CMP1 will result in SYNC0_OUT signal based on the SYNC_OUT register values.
>>
>> After bringing the interface up, calling PPS enable doesn't work as
>> the driver believes PPS is already enabled, (iep->pps_enabled is not
>> cleared during interface bring down) and driver  will just return true
>> even though there is no signal. Fix this by setting the iep->pps_enable
>> and iep->perout_enable flags to false during the link down.
>>
>> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>> ---
>>   drivers/net/ethernet/ti/icssg/icss_iep.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
>> index 5d6d1cf78e93..03abc25ced12 100644
>> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
>> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
>> @@ -195,6 +195,12 @@ static void icss_iep_enable_shadow_mode(struct icss_iep *iep)
>>   
>>   	icss_iep_disable(iep);
>>   
>> +	/* clear compare config */
>> +	for (cmp = IEP_MIN_CMP; cmp < IEP_MAX_CMP; cmp++) {
>> +		regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
>> +				   IEP_CMP_CFG_CMP_EN(cmp), 0);
>> +	}
>> +
> 
> A bit later we are clearing compare status. Can clearing CMP be done in same for loop?
> 

Yes it can be done in the same loop, I will update that.

>>   	/* disable shadow mode */
>>   	regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
>>   			   IEP_CMP_CFG_SHADOW_EN, 0);
>> @@ -778,6 +784,10 @@ int icss_iep_exit(struct icss_iep *iep)
>>   		ptp_clock_unregister(iep->ptp_clock);
>>   		iep->ptp_clock = NULL;
>>   	}
>> +
>> +	iep->pps_enabled = false;
>> +	iep->perout_enabled = false;
>> +
> 
> But how do you keep things in sync with user space?
> User might have enabled PPS or PEROUT and then put SLICE0 interface down.
> Then if SLICE0 is brought up should PPS/PEROUT keep working like before?

No, why? Because either both SLICE0 and SLICE1 run when atleast one 
interface is up and both SLICE0 and SLICE1 are stopped when both the 
interfaces are brought down. So when SLICE0 is brought down, SLICE1 is 
also brought down. Next time you bring an interface up, it is a fresh 
boot for both SLICE1 and SLICE0. In this case, just like how we register 
for ptp clock (this is handled by the driver in icss_iep_init(),
pps also needs to be enabled (this has to be done by the user).

> We did call ptp_clock_unregister() so it should unregister the PPS as well.
> What I'm not sure is if it calls the ptp->enable() hook to disable the PPS/PEROUT.
> 
> If yes then that should take care of the flags as well.
> 

No, ptp_clock_unregister() doesn't unregister PPS.

> If not then you need to call the relevant hooks explicitly but just after
> ptp_clock_unregister().
> e.g.
> 	if (iep->pps_enabled)
> 		icss_iep_pps_enable(iep, false);
> 	else if (iep->perout_enabled)
> 		icss_iep_perout_enable(iep, NULL, false);
> 

This doesn't work because if pps_enabled is already true, it goes to 
icss_iep_pps_enable(), but inside it checks if pps_enabled is true, if 
so it returns 0, without acutally enabling pps. Which is why we need to 
set pps_enable and perout_enable to false.

> But this means that user has to again setup PPS/PEROUT.	
> 

So yes, this is the expected behavior for user to setup PPS/PEROUT after 
bringing up an interface. To clarify when user needs to again setup PPS:

1. eth1 and eth2 are up, and one interface is brought down -> PPS/PEROUT 
will be working the same
2. No interface is up, and one interface is brought up -> PPS/PEROUT 
needs to be enabled

>>   	icss_iep_disable(iep);
>>   
>>   	return 0;
> 

Re: [PATCH net 2/2] net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init

[Roger Quadros]

On 12/11/2024 11:04, Meghana Malladi wrote:
> 
> On 11/11/24 19:23, Roger Quadros wrote:
>> Hi,
>>
>> On 06/11/2024 09:40, Meghana Malladi wrote:
>>> When ICSSG interfaces are brought down and brought up again, the
>>> pru cores are shut down and booted again, flushing out all the memories
>>> and start again in a clean state. Hence it is expected that the
>>> IEP_CMP_CFG register needs to be flushed during iep_init() to ensure
>>> that the existing residual configuration doesn't cause any unusual
>>> behavior. If the register is not cleared, existing IEP_CMP_CFG set for
>>> CMP1 will result in SYNC0_OUT signal based on the SYNC_OUT register values.
>>>
>>> After bringing the interface up, calling PPS enable doesn't work as
>>> the driver believes PPS is already enabled, (iep->pps_enabled is not
>>> cleared during interface bring down) and driver  will just return true
>>> even though there is no signal. Fix this by setting the iep->pps_enable
>>> and iep->perout_enable flags to false during the link down.
>>>
>>> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
>>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>>> ---
>>>   drivers/net/ethernet/ti/icssg/icss_iep.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
>>> index 5d6d1cf78e93..03abc25ced12 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
>>> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
>>> @@ -195,6 +195,12 @@ static void icss_iep_enable_shadow_mode(struct icss_iep *iep)
>>>         icss_iep_disable(iep);
>>>   +    /* clear compare config */
>>> +    for (cmp = IEP_MIN_CMP; cmp < IEP_MAX_CMP; cmp++) {
>>> +        regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
>>> +                   IEP_CMP_CFG_CMP_EN(cmp), 0);
>>> +    }
>>> +
>>
>> A bit later we are clearing compare status. Can clearing CMP be done in same for loop?
>>
> 
> Yes it can be done in the same loop, I will update that.
> 
>>>       /* disable shadow mode */
>>>       regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
>>>                  IEP_CMP_CFG_SHADOW_EN, 0);
>>> @@ -778,6 +784,10 @@ int icss_iep_exit(struct icss_iep *iep)
>>>           ptp_clock_unregister(iep->ptp_clock);
>>>           iep->ptp_clock = NULL;
>>>       }
>>> +
>>> +    iep->pps_enabled = false;
>>> +    iep->perout_enabled = false;
>>> +
>>
>> But how do you keep things in sync with user space?
>> User might have enabled PPS or PEROUT and then put SLICE0 interface down.
>> Then if SLICE0 is brought up should PPS/PEROUT keep working like before?
> 
> No, why? Because either both SLICE0 and SLICE1 run when atleast one interface is up and both SLICE0 and SLICE1 are stopped when both the interfaces are brought down. So when SLICE0 is brought down, SLICE1 is also brought down. Next time you bring an interface up, it is a fresh boot for both SLICE1 and SLICE0. In this case, just like how we register for ptp clock (this is handled by the driver in icss_iep_init(),
> pps also needs to be enabled (this has to be done by the user).

I just checked that PPS/PEROUT sysfs don't implement the show hook. So there
is nothing to be in sync with user space.

> 
>> We did call ptp_clock_unregister() so it should unregister the PPS as well.
>> What I'm not sure is if it calls the ptp->enable() hook to disable the PPS/PEROUT.
>>
>> If yes then that should take care of the flags as well.
>>
> 
> No, ptp_clock_unregister() doesn't unregister PPS.
> 
>> If not then you need to call the relevant hooks explicitly but just after
>> ptp_clock_unregister().
>> e.g.
>>     if (iep->pps_enabled)
>>         icss_iep_pps_enable(iep, false);
>>     else if (iep->perout_enabled)
>>         icss_iep_perout_enable(iep, NULL, false);
>>
> 
> This doesn't work because if pps_enabled is already true, it goes to icss_iep_pps_enable(), but inside it checks if pps_enabled is true, if so it returns 0, without acutally enabling pps. Which is why we need to set pps_enable and perout_enable to false.

Note that we are passing false in the last argument. i.e. we want to disable PPS/PEROUT.
I don't see why it won't work.

> 
>> But this means that user has to again setup PPS/PEROUT.   
>>
> 
> So yes, this is the expected behavior for user to setup PPS/PEROUT after bringing up an interface. To clarify when user needs to again setup PPS:
> 
> 1. eth1 and eth2 are up, and one interface is brought down -> PPS/PEROUT will be working the same
> 2. No interface is up, and one interface is brought up -> PPS/PEROUT needs to be enabled

OK.

> 
>>>       icss_iep_disable(iep);
>>>         return 0;
>>

-- 
cheers,
-roger

Re: [PATCH net 2/2] net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init

[Meghana Malladi]

On 12/11/24 18:47, Roger Quadros wrote:
> 
> 
> On 12/11/2024 11:04, Meghana Malladi wrote:
>>
>> On 11/11/24 19:23, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 06/11/2024 09:40, Meghana Malladi wrote:
>>>> When ICSSG interfaces are brought down and brought up again, the
>>>> pru cores are shut down and booted again, flushing out all the memories
>>>> and start again in a clean state. Hence it is expected that the
>>>> IEP_CMP_CFG register needs to be flushed during iep_init() to ensure
>>>> that the existing residual configuration doesn't cause any unusual
>>>> behavior. If the register is not cleared, existing IEP_CMP_CFG set for
>>>> CMP1 will result in SYNC0_OUT signal based on the SYNC_OUT register values.
>>>>
>>>> After bringing the interface up, calling PPS enable doesn't work as
>>>> the driver believes PPS is already enabled, (iep->pps_enabled is not
>>>> cleared during interface bring down) and driver  will just return true
>>>> even though there is no signal. Fix this by setting the iep->pps_enable
>>>> and iep->perout_enable flags to false during the link down.
>>>>
>>>> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
>>>> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
>>>> ---
>>>>    drivers/net/ethernet/ti/icssg/icss_iep.c | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
>>>> index 5d6d1cf78e93..03abc25ced12 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
>>>> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
>>>> @@ -195,6 +195,12 @@ static void icss_iep_enable_shadow_mode(struct icss_iep *iep)
>>>>          icss_iep_disable(iep);
>>>>    +    /* clear compare config */
>>>> +    for (cmp = IEP_MIN_CMP; cmp < IEP_MAX_CMP; cmp++) {
>>>> +        regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
>>>> +                   IEP_CMP_CFG_CMP_EN(cmp), 0);
>>>> +    }
>>>> +
>>>
>>> A bit later we are clearing compare status. Can clearing CMP be done in same for loop?
>>>
>>
>> Yes it can be done in the same loop, I will update that.
>>
>>>>        /* disable shadow mode */
>>>>        regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
>>>>                   IEP_CMP_CFG_SHADOW_EN, 0);
>>>> @@ -778,6 +784,10 @@ int icss_iep_exit(struct icss_iep *iep)
>>>>            ptp_clock_unregister(iep->ptp_clock);
>>>>            iep->ptp_clock = NULL;
>>>>        }
>>>> +
>>>> +    iep->pps_enabled = false;
>>>> +    iep->perout_enabled = false;
>>>> +
>>>
>>> But how do you keep things in sync with user space?
>>> User might have enabled PPS or PEROUT and then put SLICE0 interface down.
>>> Then if SLICE0 is brought up should PPS/PEROUT keep working like before?
>>
>> No, why? Because either both SLICE0 and SLICE1 run when atleast one interface is up and both SLICE0 and SLICE1 are stopped when both the interfaces are brought down. So when SLICE0 is brought down, SLICE1 is also brought down. Next time you bring an interface up, it is a fresh boot for both SLICE1 and SLICE0. In this case, just like how we register for ptp clock (this is handled by the driver in icss_iep_init(),
>> pps also needs to be enabled (this has to be done by the user).
> 
> I just checked that PPS/PEROUT sysfs don't implement the show hook. So there
> is nothing to be in sync with user space.
> 

I see, thanks for confirming this.

>>
>>> We did call ptp_clock_unregister() so it should unregister the PPS as well.
>>> What I'm not sure is if it calls the ptp->enable() hook to disable the PPS/PEROUT.
>>>
>>> If yes then that should take care of the flags as well.
>>>
>>
>> No, ptp_clock_unregister() doesn't unregister PPS.
>>
>>> If not then you need to call the relevant hooks explicitly but just after
>>> ptp_clock_unregister().
>>> e.g.
>>>      if (iep->pps_enabled)
>>>          icss_iep_pps_enable(iep, false);
>>>      else if (iep->perout_enabled)
>>>          icss_iep_perout_enable(iep, NULL, false);
>>>
>>
>> This doesn't work because if pps_enabled is already true, it goes to icss_iep_pps_enable(), but inside it checks if pps_enabled is true, if so it returns 0, without acutally enabling pps. Which is why we need to set pps_enable and perout_enable to false.
> 
> Note that we are passing false in the last argument. i.e. we want to disable PPS/PEROUT.
> I don't see why it won't work.
> 

I think I overlooked the false part, my bad. Setting pps_enable and 
perout_enable to false and calling relevant hooks - 
icss_iep_pps_enable()/icss_iep_perout_enable(). In both the cases the 
output behavior is same, but the later one is a cleaner approach. I will 
update it.

>>
>>> But this means that user has to again setup PPS/PEROUT.
>>>
>>
>> So yes, this is the expected behavior for user to setup PPS/PEROUT after bringing up an interface. To clarify when user needs to again setup PPS:
>>
>> 1. eth1 and eth2 are up, and one interface is brought down -> PPS/PEROUT will be working the same
>> 2. No interface is up, and one interface is brought up -> PPS/PEROUT needs to be enabled
> 
> OK.
> 
>>
>>>>        icss_iep_disable(iep);
>>>>          return 0;
>>>
>