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

[Simon Horman <horms@kernel.org>]

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

...