[PATCH net 1/2] net: ti: icssg-prueth: Fix firmware load sequence.
Roger Quadros
rogerq at kernel.org
Mon Nov 11 06:25:36 PST 2024
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 at 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 at ti.com>
>>> Signed-off-by: Meghana Malladi <m-malladi at 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
More information about the linux-arm-kernel
mailing list