[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241108133044.GB4507@kernel.org>
Date: Fri, 8 Nov 2024 13:30:44 +0000
From: Simon Horman <horms@...nel.org>
To: Meghana Malladi <m-malladi@...com>
Cc: vigneshr@...com, m-karicheri2@...com, jan.kiszka@...mens.com,
javier.carrasco.cruz@...il.com, jacob.e.keller@...el.com,
diogo.ivo@...mens.com, pabeni@...hat.com, kuba@...nel.org,
edumazet@...gle.com, davem@...emloft.net, andrew+netdev@...n.ch,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, srk@...com,
Roger Quadros <rogerq@...nel.org>, danishanwar@...com
Subject: Re: [PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load
sequence.
On Wed, Nov 06, 2024 at 01:10:39PM +0530, Meghana Malladi wrote:
> From: MD Danish Anwar <danishanwar@...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@...com>
> Signed-off-by: Meghana Malladi <m-malladi@...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);
>
...
Powered by blists - more mailing lists