From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lelv0142.ext.ti.com (lelv0142.ext.ti.com [198.47.23.249]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EA5C2216E3B; Mon, 9 Dec 2024 10:35:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.47.23.249 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733740522; cv=none; b=aQWn5Hh1A+snRDnwKrjeT9SPIXvKF6IIq91FyaNsKFK32/b+8G+4tljRMbdEmfnUN4wE0/bo3cwwmDiv/cLA6LlZh//xS30dmgl0P21Nq1WMRoqXw0fNxMgSXeaE/ywmev86N5JgD/2qb8dq+0vFPzybulWDmDvHQbpVH3BRxok= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733740522; c=relaxed/simple; bh=V5LBcTHLhKacE07FccmaYdNmNLE5eTgn9lJ+cRDgDdQ=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=Qz/uuk5lvlArK+Ec1JHkQZ7QgIMKt1ZRVfl+4G9frDkxcUBPE4floanzKGod/zZCQlp0uvk2FZWcOBfpipKvFY41oGG9uIYTdGBtSNpsR5XBwZ7MA5aypba9qBcJ/ALGliFG35pf0w7S2548WMNuj0iTds6+Hf2G8pxBweGzZPc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com; spf=pass smtp.mailfrom=ti.com; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b=ZIIrI962; arc=none smtp.client-ip=198.47.23.249 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ti.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b="ZIIrI962" Received: from fllv0035.itg.ti.com ([10.64.41.0]) by lelv0142.ext.ti.com (8.15.2/8.15.2) with ESMTP id 4B9AYknv090518; Mon, 9 Dec 2024 04:34:46 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1733740486; bh=IsPw+LQRUnCnanH7t9Kuob0H3qrAiI/xNlXeOV0nBHI=; h=Date:Subject:To:CC:References:From:In-Reply-To; b=ZIIrI962/DAEdDYaDYgVnnpsNxmI+8K+RXVCE7G589/gPvGKw17eYGqc/B+lVYs07 Lk4QWL1aqvEfRzIZpS5HfKKEV/NEo5wT0mG8cJmYln/1FTMSIuyE6ijTyisN5Ooc5r JBBzTU2urshLjGVefvakwLO1tW0QUk6jrxpVXzL8= Received: from DLEE113.ent.ti.com (dlee113.ent.ti.com [157.170.170.24]) by fllv0035.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 4B9AYkGF109951 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 9 Dec 2024 04:34:46 -0600 Received: from DLEE110.ent.ti.com (157.170.170.21) by DLEE113.ent.ti.com (157.170.170.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Mon, 9 Dec 2024 04:34:46 -0600 Received: from lelvsmtp6.itg.ti.com (10.180.75.249) by DLEE110.ent.ti.com (157.170.170.21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Mon, 9 Dec 2024 04:34:46 -0600 Received: from [10.24.69.13] (meghana-pc.dhcp.ti.com [10.24.69.13] (may be forged)) by lelvsmtp6.itg.ti.com (8.15.2/8.15.2) with ESMTP id 4B9AYfC2088003; Mon, 9 Dec 2024 04:34:41 -0600 Message-ID: <64621290-2488-474d-b2ed-597a1f4ac85f@ti.com> Date: Mon, 9 Dec 2024 16:04:40 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net v3 1/2] net: ti: icssg-prueth: Fix firmware load sequence. To: Roger Quadros , , , , , , , , , , , CC: , , , , References: <20241205082831.777868-1-m-malladi@ti.com> <20241205082831.777868-2-m-malladi@ti.com> Content-Language: en-US From: Meghana Malladi In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-C2ProcessedOrg: 333ef613-75bf-4e12-a4b1-8e3623f5dcea On 05/12/24 18:38, Roger Quadros wrote: > Hi, > > On 05/12/2024 10:28, Meghana Malladi wrote: >> From: MD Danish Anwar >> >> 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 resective 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. >> >> As there are interdependencies between SLICE0 and SLICE1 firmwares, >> fix this by running both PRU0 and PRU1 firmwares as long as at least 1 >> ICSSG interface is up. Add new flag in prueth struct to check if all >> firmwares are running. >> >> Use emacs_initialized as reference count to load the firmwares for the >> first and last interface up/down. Moving init_emac_mode and fw_offload_mode >> API outside of icssg_config to icssg_common_start API as they need >> to be called only once per firmware boot. >> >> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver") >> Signed-off-by: MD Danish Anwar >> Signed-off-by: Meghana Malladi >> --- >> >> Hi all, >> >> This patch is based on net-next tagged next-20241128. >> v2:https://lore.kernel.org/all/20241128122931.2494446-2-m-malladi@ti.com/ >> >> * Changes since v2 (v3-v2): >> - error handling in caller function of prueth_emac_common_start() >> - Use prus_running flag check before stopping the firmwares >> Both suggested by Roger Quadros >> >> drivers/net/ethernet/ti/icssg/icssg_config.c | 45 ++++-- >> drivers/net/ethernet/ti/icssg/icssg_config.h | 1 + >> drivers/net/ethernet/ti/icssg/icssg_prueth.c | 157 ++++++++++++------- >> drivers/net/ethernet/ti/icssg/icssg_prueth.h | 5 + >> 4 files changed, 140 insertions(+), 68 deletions(-) >> >> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c >> index 5d2491c2943a..342150756cf7 100644 >> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c >> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c >> @@ -397,7 +397,7 @@ static int prueth_emac_buffer_setup(struct prueth_emac *emac) >> return 0; >> } >> [ ... ] >> +static int prueth_emac_common_start(struct prueth *prueth) >> +{ >> + struct prueth_emac *emac; >> + int ret = 0; >> + int slice; >> + >> + if (!prueth->emac[ICSS_SLICE0] && !prueth->emac[ICSS_SLICE1]) >> + return -EINVAL; >> + >> + /* clear SMEM and MSMC settings for all slices */ >> + memset_io(prueth->msmcram.va, 0, prueth->msmcram.size); >> + memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS); >> + >> + icssg_class_default(prueth->miig_rt, ICSS_SLICE0, 0, false); >> + icssg_class_default(prueth->miig_rt, ICSS_SLICE1, 0, false); >> + >> + if (prueth->is_switch_mode || prueth->is_hsr_offload_mode) >> + icssg_init_fw_offload_mode(prueth); >> + else >> + icssg_init_emac_mode(prueth); >> + >> + for (slice = 0; slice < PRUETH_NUM_MACS; slice++) { >> + emac = prueth->emac[slice]; >> + if (emac) { >> + ret |= icssg_config(prueth, emac, slice); >> + if (ret) >> + return ret; >> + } >> + ret |= prueth_emac_start(prueth, slice); >> + } > > need newline? > Yes I will add it. >> + if (!ret) >> + prueth->prus_running = 1; >> + else >> + return ret; >> + >> + emac = prueth->emac[ICSS_SLICE0] ? prueth->emac[ICSS_SLICE0] : >> + prueth->emac[ICSS_SLICE1]; >> + ret = icss_iep_init(emac->iep, &prueth_iep_clockops, >> + emac, IEP_DEFAULT_CYCLE_TIME_NS); >> + if (ret) { >> + dev_err(prueth->dev, "Failed to initialize IEP module\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int prueth_emac_common_stop(struct prueth *prueth) >> +{ >> + struct prueth_emac *emac; >> + int slice; >> + >> + if (!prueth->emac[ICSS_SLICE0] && !prueth->emac[ICSS_SLICE1]) >> + return -EINVAL; >> + >> + icssg_class_disable(prueth->miig_rt, ICSS_SLICE0); >> + icssg_class_disable(prueth->miig_rt, ICSS_SLICE1); >> + >> + for (slice = 0; slice < PRUETH_NUM_MACS; slice++) { >> + if (prueth->prus_running) { >> + rproc_shutdown(prueth->txpru[slice]); >> + rproc_shutdown(prueth->rtu[slice]); >> + rproc_shutdown(prueth->pru[slice]); >> + } >> + } >> + prueth->prus_running = 0; >> + >> + emac = prueth->emac[ICSS_SLICE0] ? prueth->emac[ICSS_SLICE0] : >> + prueth->emac[ICSS_SLICE1]; >> + icss_iep_exit(emac->iep); > > if icss_iep_init() failed at prueth_emac_common_start(), we should not be > calling icss_iep_exit(). Maybe you need another flag for iep_init status? > Yes I have thought of it as well. In icss_iep_init() does lot of iep register configuration and in the end it enables iep by setting IEP_CNT_ENABLE bit and ptp_clock_register(). Whereas in icss_iep_exit() it checks for ptp_clock and pps/perout. And calls icss_iep_disable() which again clears IEP_CNT_ENABLE. So I see no harm in calling icss_iep_exit() even if icss_iep_init() as it overwrites the existing configuration only. But if you think this doesn't look good I can definitely add new flag for iep as well. But IMO I think this flag would be redundant, please correct me if I am wrong. So which one sounds better? > Is it better to call icss_iep_exit() at the top before icssg_class_disable()? > >> + >> + return 0; >> +} >> + >> /* called back by PHY layer if there is change in link state of hw port*/ >> static void emac_adjust_link(struct net_device *ndev) >> { >> @@ -369,12 +432,13 @@ static void prueth_iep_settime(void *clockops_data, u64 ns) >> { >> struct icssg_setclock_desc __iomem *sc_descp; >> struct prueth_emac *emac = clockops_data; >> + struct prueth *prueth = emac->prueth; >> struct icssg_setclock_desc sc_desc; >> u64 cyclecount; >> u32 cycletime; >> int timeout; >> >> - if (!emac->fw_running) >> + if (!prueth->prus_running) >> return; >> >> sc_descp = emac->prueth->shram.va + TIMESYNC_FW_WC_SETCLOCK_DESC_OFFSET; >> @@ -543,23 +607,17 @@ 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; >> int max_rx_flows; >> int rx_flow; >> >> - /* clear SMEM and MSMC settings for all slices */ >> - if (!prueth->emacs_initialized) { >> - memset_io(prueth->msmcram.va, 0, prueth->msmcram.size); >> - memset_io(prueth->shram.va, 0, ICSSG_CONFIG_OFFSET_SLICE1 * PRUETH_NUM_MACS); >> - } >> - >> /* set h/w MAC as user might have re-configured */ >> ether_addr_copy(emac->mac_addr, ndev->dev_addr); >> >> 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. */ >> @@ -597,18 +655,23 @@ 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) { >> + ret = prueth_emac_common_start(prueth); >> + if (ret) >> + goto stop; >> + } >> >> - icssg_mii_update_mtu(prueth->mii_rt, slice, ndev->max_mtu); >> + 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 (!prueth->emacs_initialized) { >> - ret = icss_iep_init(emac->iep, &prueth_iep_clockops, >> - emac, IEP_DEFAULT_CYCLE_TIME_NS); >> + 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); >> + >> ret = request_threaded_irq(emac->tx_ts_irq, NULL, prueth_tx_ts_irq, >> IRQF_ONESHOT, dev_name(dev), emac); >> if (ret) >> @@ -653,8 +716,7 @@ 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); >> -free_rx_irq: >> + prueth_emac_common_stop(prueth); >> free_irq(emac->rx_chns.irq[rx_flow], emac); >> cleanup_napi: >> prueth_ndev_del_tx_napi(emac, emac->tx_ch_num); >> @@ -689,8 +751,6 @@ 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 (emac->prueth->is_hsr_offload_mode) >> __dev_mc_unsync(ndev, icssg_prueth_hsr_del_mcast); >> else >> @@ -728,11 +788,9 @@ 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) >> - icss_iep_exit(emac->iep); >> - >> /* stop PRUs */ >> - prueth_emac_stop(emac); >> + if (prueth->emacs_initialized == 1) >> + prueth_emac_common_stop(prueth); >> >> free_irq(emac->tx_ts_irq, emac); >> >> @@ -1069,16 +1127,10 @@ static void prueth_emac_restart(struct prueth *prueth) >> icssg_set_port_state(emac1, ICSSG_EMAC_PORT_DISABLE); >> >> /* Stop both pru cores for both PRUeth ports*/ >> - prueth_emac_stop(emac0); >> - prueth->emacs_initialized--; >> - prueth_emac_stop(emac1); >> - prueth->emacs_initialized--; >> + prueth_emac_common_stop(prueth); >> >> /* Start both pru cores for both PRUeth ports */ >> - prueth_emac_start(prueth, emac0); >> - prueth->emacs_initialized++; >> - prueth_emac_start(prueth, emac1); >> - prueth->emacs_initialized++; >> + prueth_emac_common_start(prueth); > > But this can fail? You need to deal with failure condition appropriately. > I haven't added failure conditions for two reasons: - Existing code also didn't have any error checks - This func simply reloads a new firmware, given everything is already working with the old one. I can still handle error cases by changing this func to return int (currently it is void) and caller of the functions should print error and immediately return. Thoughts on this? >> >> /* Enable forwarding for both PRUeth ports */ >> icssg_set_port_state(emac0, ICSSG_EMAC_PORT_FORWARD); >> @@ -1413,13 +1465,10 @@ static int prueth_probe(struct platform_device *pdev) >> prueth->pa_stats = NULL; >> } >> >> - if (eth0_node) { >> + if (eth0_node || eth1_node) { >> ret = prueth_get_cores(prueth, ICSS_SLICE0, false); >> if (ret) >> goto put_cores; >> - } >> - >> - if (eth1_node) { >> ret = prueth_get_cores(prueth, ICSS_SLICE1, false); >> if (ret) >> goto put_cores; >> @@ -1618,14 +1667,12 @@ static int prueth_probe(struct platform_device *pdev) >> pruss_put(prueth->pruss); >> >> put_cores: >> - if (eth1_node) { >> - prueth_put_cores(prueth, ICSS_SLICE1); >> - of_node_put(eth1_node); >> - } >> - >> - if (eth0_node) { >> + if (eth0_node || eth1_node) { >> prueth_put_cores(prueth, ICSS_SLICE0); >> of_node_put(eth0_node); >> + >> + prueth_put_cores(prueth, ICSS_SLICE1); >> + of_node_put(eth1_node); >> } >> >> return ret; >> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h >> index f5c1d473e9f9..b30f2e9a73d8 100644 >> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h >> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h >> @@ -257,6 +257,7 @@ struct icssg_firmwares { >> * @is_switchmode_supported: indicates platform support for switch mode >> * @switch_id: ID for mapping switch ports to bridge >> * @default_vlan: Default VLAN for host >> + * @prus_running: flag to indicate if all pru cores are running >> */ >> struct prueth { >> struct device *dev; >> @@ -298,6 +299,7 @@ struct prueth { >> int default_vlan; >> /** @vtbl_lock: Lock for vtbl in shared memory */ >> spinlock_t vtbl_lock; >> + bool prus_running; > > I think you don't need fw_running flag anymore. Could you please remove it > from struct prueth_emac? > This flag is still being used by SR1, for which this patch doesn't apply. So I prefer not touching this flag for the sake of SR1. >> }; >> >> struct emac_tx_ts_response { >> @@ -361,6 +363,8 @@ int icssg_set_port_state(struct prueth_emac *emac, >> enum icssg_port_state_cmd state); >> void icssg_config_set_speed(struct prueth_emac *emac); >> void icssg_config_half_duplex(struct prueth_emac *emac); >> +void icssg_init_emac_mode(struct prueth *prueth); >> +void icssg_init_fw_offload_mode(struct prueth *prueth); >> >> /* Buffer queue helpers */ >> int icssg_queue_pop(struct prueth *prueth, u8 queue); >> @@ -377,6 +381,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) >> >