Message-ID: <9514d904-04bd-4ff8-8fbd-a4491702352b@kernel.org>
Date: Tue, 12 Nov 2024 10:29:38 +0200
From: Roger Quadros <rogerq@...nel.org>
To: "Anwar, Md Danish" <a0501179@...com>, Meghana Malladi <m-malladi@...com>,
 vigneshr@...com, m-karicheri2@...com, jan.kiszka@...mens.com,
 javier.carrasco.cruz@...il.com, jacob.e.keller@...el.com, horms@...nel.org,
 diogo.ivo@...mens.com, pabeni@...hat.com, kuba@...nel.org,
 edumazet@...gle.com, davem@...emloft.net, andrew+netdev@...n.ch
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, srk@...com, danishanwar@...com
Subject: Re: [PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load
 sequence.



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@...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@...com>
>>>>> Signed-off-by: Meghana Malladi <m-malladi@...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

Powered by blists - more mailing lists