diff mbox series

[net-next,v5,4/5] net: ti: icssg-prueth: Enable HSR Tx duplication, Tx Tag and Rx Tag offload

Message ID 20240906111538.1259418-5-danishanwar@ti.com (mailing list archive)
State New, archived
Headers show
Series Introduce HSR offload support for ICSSG | expand

Commit Message

MD Danish Anwar Sept. 6, 2024, 11:15 a.m. UTC 
From: Ravi Gunasekaran <r-gunasekaran@ti.com>

The HSR stack allows to offload its Tx packet duplication functionality to
the hardware. Enable this offloading feature for ICSSG driver. Add support
to offload HSR Tx Tag Insertion and Rx Tag Removal and duplicate discard.

Inorder to enable hsr-tag-ins-offload, hsr-dup-offload must also be enabled
as these are tightly coupled in the firmware implementation.

Duplicate discard is done as part of RX tag removal and it is
done by the firmware. When driver sends the r30 command
ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware does RX tag removal as well as
duplicate discard.

Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_common.c | 18 ++++++++++---
 drivers/net/ethernet/ti/icssg/icssg_config.c |  4 ++-
 drivers/net/ethernet/ti/icssg/icssg_config.h |  2 ++
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 28 +++++++++++++++++++-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  3 +++
 5 files changed, 50 insertions(+), 5 deletions(-)

Comments

Roger Quadros Sept. 10, 2024, 5:38 p.m. UTC | #1 
On 06/09/2024 14:15, MD Danish Anwar wrote:
> From: Ravi Gunasekaran <r-gunasekaran@ti.com>
> 
> The HSR stack allows to offload its Tx packet duplication functionality to
> the hardware. Enable this offloading feature for ICSSG driver. Add support
> to offload HSR Tx Tag Insertion and Rx Tag Removal and duplicate discard.
> 
> Inorder to enable hsr-tag-ins-offload, hsr-dup-offload must also be enabled

"In order"

> as these are tightly coupled in the firmware implementation.
> 
> Duplicate discard is done as part of RX tag removal and it is
> done by the firmware. When driver sends the r30 command
> ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware does RX tag removal as well as
> duplicate discard.
> 
> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_common.c | 18 ++++++++++---
>  drivers/net/ethernet/ti/icssg/icssg_config.c |  4 ++-
>  drivers/net/ethernet/ti/icssg/icssg_config.h |  2 ++
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 28 +++++++++++++++++++-
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  3 +++
>  5 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index b9d8a93d1680..fdebeb2f84e0 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -660,14 +660,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>  {
>  	struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
>  	struct prueth_emac *emac = netdev_priv(ndev);
> +	struct prueth *prueth = emac->prueth;
>  	struct netdev_queue *netif_txq;
>  	struct prueth_tx_chn *tx_chn;
>  	dma_addr_t desc_dma, buf_dma;
> +	u32 pkt_len, dst_tag_id;
>  	int i, ret = 0, q_idx;
>  	bool in_tx_ts = 0;
>  	int tx_ts_cookie;
>  	void **swdata;
> -	u32 pkt_len;
>  	u32 *epib;
>  
>  	pkt_len = skb_headlen(skb);
> @@ -712,9 +713,20 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>  
>  	/* set dst tag to indicate internal qid at the firmware which is at
>  	 * bit8..bit15. bit0..bit7 indicates port num for directed
> -	 * packets in case of switch mode operation
> +	 * packets in case of switch mode operation and port num 0
> +	 * for undirected packets in case of HSR offload mode
>  	 */
> -	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
> +	dst_tag_id = emac->port_id | (q_idx << 8);
> +
> +	if (prueth->is_hsr_offload_mode &&
> +	    (ndev->features & NETIF_F_HW_HSR_DUP))
> +		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
> +
> +	if (prueth->is_hsr_offload_mode &&
> +	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
> +		epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
> +
> +	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
>  	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>  	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
>  	swdata = cppi5_hdesc_get_swdata(first_desc);
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
> index 7b2e6c192ff3..72ace151d8e9 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
> @@ -531,7 +531,9 @@ static const struct icssg_r30_cmd emac_r32_bitmask[] = {
>  	{{EMAC_NONE,  0xffff4000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx ENABLE*/
>  	{{EMAC_NONE,  0xbfff0000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx DISABLE*/
>  	{{0xffff0010,  EMAC_NONE, 0xffff0010, EMAC_NONE}},	/* VLAN AWARE*/
> -	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}}	/* VLAN UNWARE*/
> +	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}},	/* VLAN UNWARE*/
> +	{{0xffff2000, EMAC_NONE, EMAC_NONE, EMAC_NONE}},	/* HSR_RX_OFFLOAD_ENABLE */
> +	{{0xdfff0000, EMAC_NONE, EMAC_NONE, EMAC_NONE}}		/* HSR_RX_OFFLOAD_DISABLE */
>  };
>  
>  int icssg_set_port_state(struct prueth_emac *emac,
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
> index 1ac60283923b..92c2deaa3068 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
> @@ -80,6 +80,8 @@ enum icssg_port_state_cmd {
>  	ICSSG_EMAC_PORT_PREMPT_TX_DISABLE,
>  	ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE,
>  	ICSSG_EMAC_PORT_VLAN_AWARE_DISABLE,
> +	ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE,
> +	ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE,
>  	ICSSG_EMAC_PORT_MAX_COMMANDS
>  };
>  
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 676168d6fded..9af06454ba64 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -41,7 +41,10 @@
>  #define DEFAULT_PORT_MASK	1
>  #define DEFAULT_UNTAG_MASK	1
>  
> -#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
> +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
> +						 NETIF_F_HW_HSR_DUP | \
> +						 NETIF_F_HW_HSR_TAG_INS | \
> +						 NETIF_F_HW_HSR_TAG_RM)
>  
>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
> @@ -758,6 +761,21 @@ static void emac_change_hsr_feature(struct net_device *ndev,
>  	}
>  }
>  
> +static netdev_features_t emac_ndo_fix_features(struct net_device *ndev,
> +					       netdev_features_t features)
> +{
> +	/* In order to enable hsr tag insertion offload, hsr dup offload must
> +	 * also be enabled as these two are tightly coupled in firmware
> +	 * implementation.
> +	 */
> +	if (features & NETIF_F_HW_HSR_TAG_INS)
> +		features |= NETIF_F_HW_HSR_DUP;

What if only NETIF_F_HW_HSR_DUP was set? Don't you have to set NETIF_F_HW_HSR_TAG_INS as well?

> +	else
> +		features &= ~NETIF_F_HW_HSR_DUP;

what if NETIF_F_HW_HSR_DUP was still set?

I think you need to write a logic like follows.
	if both are already cleared in ndev->features and any one is set in features you set both in features.
	if both are already set in ndev->features and any one is cleared in features you clear both in features.

is this reasonable?

> +
> +	return features;
> +}
> +
>  static int emac_ndo_set_features(struct net_device *ndev,
>  				 netdev_features_t features)
>  {
> @@ -780,6 +798,7 @@ static const struct net_device_ops emac_netdev_ops = {
>  	.ndo_eth_ioctl = icssg_ndo_ioctl,
>  	.ndo_get_stats64 = icssg_ndo_get_stats64,
>  	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
> +	.ndo_fix_features = emac_ndo_fix_features,
>  	.ndo_set_features = emac_ndo_set_features,
>  };
>  
> @@ -1007,6 +1026,13 @@ static void icssg_change_mode(struct prueth *prueth)
>  
>  	for (mac = PRUETH_MAC0; mac < PRUETH_NUM_MACS; mac++) {
>  		emac = prueth->emac[mac];
> +		if (prueth->is_hsr_offload_mode) {
> +			if (emac->ndev->features & NETIF_F_HW_HSR_TAG_RM)
> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE);
> +			else
> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE);
> +		}
> +
>  		if (netif_running(emac->ndev)) {
>  			icssg_fdb_add_del(emac, eth_stp_addr, prueth->default_vlan,
>  					  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> index a4b025fae797..bba6da2e6bd8 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> @@ -59,6 +59,9 @@
>  
>  #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
>  
> +#define PRUETH_UNDIRECTED_PKT_DST_TAG	0
> +#define PRUETH_UNDIRECTED_PKT_TAG_INS	BIT(30)
> +
>  /* Firmware status codes */
>  #define ICSS_HS_FW_READY 0x55555555
>  #define ICSS_HS_FW_DEAD 0xDEAD0000	/* lower 16 bits contain error code */
Anwar, Md Danish Sept. 10, 2024, 5:52 p.m. UTC | #2 
Hi Roger,

On 9/10/2024 11:08 PM, Roger Quadros wrote:
> 
> 
> On 06/09/2024 14:15, MD Danish Anwar wrote:
>> From: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>
>> The HSR stack allows to offload its Tx packet duplication functionality to
>> the hardware. Enable this offloading feature for ICSSG driver. Add support
>> to offload HSR Tx Tag Insertion and Rx Tag Removal and duplicate discard.
>>
>> Inorder to enable hsr-tag-ins-offload, hsr-dup-offload must also be enabled
> 
> "In order"
> 
>> as these are tightly coupled in the firmware implementation.
>>
>> Duplicate discard is done as part of RX tag removal and it is
>> done by the firmware. When driver sends the r30 command
>> ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware does RX tag removal as well as
>> duplicate discard.
>>
>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  drivers/net/ethernet/ti/icssg/icssg_common.c | 18 ++++++++++---
>>  drivers/net/ethernet/ti/icssg/icssg_config.c |  4 ++-
>>  drivers/net/ethernet/ti/icssg/icssg_config.h |  2 ++
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 28 +++++++++++++++++++-
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  3 +++
>>  5 files changed, 50 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
>> index b9d8a93d1680..fdebeb2f84e0 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
>> @@ -660,14 +660,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>  {
>>  	struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
>>  	struct prueth_emac *emac = netdev_priv(ndev);
>> +	struct prueth *prueth = emac->prueth;
>>  	struct netdev_queue *netif_txq;
>>  	struct prueth_tx_chn *tx_chn;
>>  	dma_addr_t desc_dma, buf_dma;
>> +	u32 pkt_len, dst_tag_id;
>>  	int i, ret = 0, q_idx;
>>  	bool in_tx_ts = 0;
>>  	int tx_ts_cookie;
>>  	void **swdata;
>> -	u32 pkt_len;
>>  	u32 *epib;
>>  
>>  	pkt_len = skb_headlen(skb);
>> @@ -712,9 +713,20 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>  
>>  	/* set dst tag to indicate internal qid at the firmware which is at
>>  	 * bit8..bit15. bit0..bit7 indicates port num for directed
>> -	 * packets in case of switch mode operation
>> +	 * packets in case of switch mode operation and port num 0
>> +	 * for undirected packets in case of HSR offload mode
>>  	 */
>> -	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
>> +	dst_tag_id = emac->port_id | (q_idx << 8);
>> +
>> +	if (prueth->is_hsr_offload_mode &&
>> +	    (ndev->features & NETIF_F_HW_HSR_DUP))
>> +		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
>> +
>> +	if (prueth->is_hsr_offload_mode &&
>> +	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
>> +		epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
>> +
>> +	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
>>  	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>>  	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
>>  	swdata = cppi5_hdesc_get_swdata(first_desc);
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> index 7b2e6c192ff3..72ace151d8e9 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>> @@ -531,7 +531,9 @@ static const struct icssg_r30_cmd emac_r32_bitmask[] = {
>>  	{{EMAC_NONE,  0xffff4000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx ENABLE*/
>>  	{{EMAC_NONE,  0xbfff0000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx DISABLE*/
>>  	{{0xffff0010,  EMAC_NONE, 0xffff0010, EMAC_NONE}},	/* VLAN AWARE*/
>> -	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}}	/* VLAN UNWARE*/
>> +	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}},	/* VLAN UNWARE*/
>> +	{{0xffff2000, EMAC_NONE, EMAC_NONE, EMAC_NONE}},	/* HSR_RX_OFFLOAD_ENABLE */
>> +	{{0xdfff0000, EMAC_NONE, EMAC_NONE, EMAC_NONE}}		/* HSR_RX_OFFLOAD_DISABLE */
>>  };
>>  
>>  int icssg_set_port_state(struct prueth_emac *emac,
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
>> index 1ac60283923b..92c2deaa3068 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
>> @@ -80,6 +80,8 @@ enum icssg_port_state_cmd {
>>  	ICSSG_EMAC_PORT_PREMPT_TX_DISABLE,
>>  	ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE,
>>  	ICSSG_EMAC_PORT_VLAN_AWARE_DISABLE,
>> +	ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE,
>> +	ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE,
>>  	ICSSG_EMAC_PORT_MAX_COMMANDS
>>  };
>>  
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index 676168d6fded..9af06454ba64 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -41,7 +41,10 @@
>>  #define DEFAULT_PORT_MASK	1
>>  #define DEFAULT_UNTAG_MASK	1
>>  
>> -#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
>> +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
>> +						 NETIF_F_HW_HSR_DUP | \
>> +						 NETIF_F_HW_HSR_TAG_INS | \
>> +						 NETIF_F_HW_HSR_TAG_RM)
>>  
>>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>> @@ -758,6 +761,21 @@ static void emac_change_hsr_feature(struct net_device *ndev,
>>  	}
>>  }
>>  
>> +static netdev_features_t emac_ndo_fix_features(struct net_device *ndev,
>> +					       netdev_features_t features)
>> +{
>> +	/* In order to enable hsr tag insertion offload, hsr dup offload must
>> +	 * also be enabled as these two are tightly coupled in firmware
>> +	 * implementation.
>> +	 */
>> +	if (features & NETIF_F_HW_HSR_TAG_INS)
>> +		features |= NETIF_F_HW_HSR_DUP;
> 
> What if only NETIF_F_HW_HSR_DUP was set? Don't you have to set NETIF_F_HW_HSR_TAG_INS as well?
> 
>> +	else
>> +		features &= ~NETIF_F_HW_HSR_DUP;
> 
> what if NETIF_F_HW_HSR_DUP was still set?
> 
> I think you need to write a logic like follows.
> 	if both are already cleared in ndev->features and any one is set in features you set both in features.
> 	if both are already set in ndev->features and any one is cleared in features you clear both in features.
> 
> is this reasonable?
> 

Yes that does sound reasonable,
How does below code look to you.

	if (!(ndev->features & NETIF_F_HW_HSR_DUP) &&
	    !(ndev->features & NETIF_F_HW_HSR_TAG_INS))
		if ((features & NETIF_F_HW_HSR_DUP) ||
		    (features & NETIF_F_HW_HSR_TAG_INS)) {
			features |= NETIF_F_HW_HSR_DUP;
			features |= NETIF_F_HW_HSR_TAG_INS;
		}

	if ((ndev->features & NETIF_F_HW_HSR_DUP) &&
	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
		if (!(features & NETIF_F_HW_HSR_DUP) ||
		    !(features & NETIF_F_HW_HSR_TAG_INS)) {
			features &= ~NETIF_F_HW_HSR_DUP;
			features &= ~NETIF_F_HW_HSR_TAG_INS;
		}

>> +
>> +	return features;
>> +}
>> +
>>  static int emac_ndo_set_features(struct net_device *ndev,
>>  				 netdev_features_t features)
>>  {
>> @@ -780,6 +798,7 @@ static const struct net_device_ops emac_netdev_ops = {
>>  	.ndo_eth_ioctl = icssg_ndo_ioctl,
>>  	.ndo_get_stats64 = icssg_ndo_get_stats64,
>>  	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
>> +	.ndo_fix_features = emac_ndo_fix_features,
>>  	.ndo_set_features = emac_ndo_set_features,
>>  };
>>  
>> @@ -1007,6 +1026,13 @@ static void icssg_change_mode(struct prueth *prueth)
>>  
>>  	for (mac = PRUETH_MAC0; mac < PRUETH_NUM_MACS; mac++) {
>>  		emac = prueth->emac[mac];
>> +		if (prueth->is_hsr_offload_mode) {
>> +			if (emac->ndev->features & NETIF_F_HW_HSR_TAG_RM)
>> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE);
>> +			else
>> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE);
>> +		}
>> +
>>  		if (netif_running(emac->ndev)) {
>>  			icssg_fdb_add_del(emac, eth_stp_addr, prueth->default_vlan,
>>  					  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> index a4b025fae797..bba6da2e6bd8 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>> @@ -59,6 +59,9 @@
>>  
>>  #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
>>  
>> +#define PRUETH_UNDIRECTED_PKT_DST_TAG	0
>> +#define PRUETH_UNDIRECTED_PKT_TAG_INS	BIT(30)
>> +
>>  /* Firmware status codes */
>>  #define ICSS_HS_FW_READY 0x55555555
>>  #define ICSS_HS_FW_DEAD 0xDEAD0000	/* lower 16 bits contain error code */
>
Roger Quadros Sept. 10, 2024, 6:06 p.m. UTC | #3 
On 10/09/2024 20:52, Anwar, Md Danish wrote:
> Hi Roger,
> 
> On 9/10/2024 11:08 PM, Roger Quadros wrote:
>>
>>
>> On 06/09/2024 14:15, MD Danish Anwar wrote:
>>> From: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>>
>>> The HSR stack allows to offload its Tx packet duplication functionality to
>>> the hardware. Enable this offloading feature for ICSSG driver. Add support
>>> to offload HSR Tx Tag Insertion and Rx Tag Removal and duplicate discard.
>>>
>>> Inorder to enable hsr-tag-ins-offload, hsr-dup-offload must also be enabled
>>
>> "In order"
>>
>>> as these are tightly coupled in the firmware implementation.
>>>
>>> Duplicate discard is done as part of RX tag removal and it is
>>> done by the firmware. When driver sends the r30 command
>>> ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware does RX tag removal as well as
>>> duplicate discard.
>>>
>>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>> ---
>>>  drivers/net/ethernet/ti/icssg/icssg_common.c | 18 ++++++++++---
>>>  drivers/net/ethernet/ti/icssg/icssg_config.c |  4 ++-
>>>  drivers/net/ethernet/ti/icssg/icssg_config.h |  2 ++
>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 28 +++++++++++++++++++-
>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  3 +++
>>>  5 files changed, 50 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
>>> index b9d8a93d1680..fdebeb2f84e0 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
>>> @@ -660,14 +660,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>>  {
>>>  	struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
>>>  	struct prueth_emac *emac = netdev_priv(ndev);
>>> +	struct prueth *prueth = emac->prueth;
>>>  	struct netdev_queue *netif_txq;
>>>  	struct prueth_tx_chn *tx_chn;
>>>  	dma_addr_t desc_dma, buf_dma;
>>> +	u32 pkt_len, dst_tag_id;
>>>  	int i, ret = 0, q_idx;
>>>  	bool in_tx_ts = 0;
>>>  	int tx_ts_cookie;
>>>  	void **swdata;
>>> -	u32 pkt_len;
>>>  	u32 *epib;
>>>  
>>>  	pkt_len = skb_headlen(skb);
>>> @@ -712,9 +713,20 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>>  
>>>  	/* set dst tag to indicate internal qid at the firmware which is at
>>>  	 * bit8..bit15. bit0..bit7 indicates port num for directed
>>> -	 * packets in case of switch mode operation
>>> +	 * packets in case of switch mode operation and port num 0
>>> +	 * for undirected packets in case of HSR offload mode
>>>  	 */
>>> -	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
>>> +	dst_tag_id = emac->port_id | (q_idx << 8);
>>> +
>>> +	if (prueth->is_hsr_offload_mode &&
>>> +	    (ndev->features & NETIF_F_HW_HSR_DUP))
>>> +		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
>>> +
>>> +	if (prueth->is_hsr_offload_mode &&
>>> +	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
>>> +		epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
>>> +
>>> +	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
>>>  	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>>>  	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
>>>  	swdata = cppi5_hdesc_get_swdata(first_desc);
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>> index 7b2e6c192ff3..72ace151d8e9 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>> @@ -531,7 +531,9 @@ static const struct icssg_r30_cmd emac_r32_bitmask[] = {
>>>  	{{EMAC_NONE,  0xffff4000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx ENABLE*/
>>>  	{{EMAC_NONE,  0xbfff0000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx DISABLE*/
>>>  	{{0xffff0010,  EMAC_NONE, 0xffff0010, EMAC_NONE}},	/* VLAN AWARE*/
>>> -	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}}	/* VLAN UNWARE*/
>>> +	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}},	/* VLAN UNWARE*/
>>> +	{{0xffff2000, EMAC_NONE, EMAC_NONE, EMAC_NONE}},	/* HSR_RX_OFFLOAD_ENABLE */
>>> +	{{0xdfff0000, EMAC_NONE, EMAC_NONE, EMAC_NONE}}		/* HSR_RX_OFFLOAD_DISABLE */
>>>  };
>>>  
>>>  int icssg_set_port_state(struct prueth_emac *emac,
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>> index 1ac60283923b..92c2deaa3068 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>> @@ -80,6 +80,8 @@ enum icssg_port_state_cmd {
>>>  	ICSSG_EMAC_PORT_PREMPT_TX_DISABLE,
>>>  	ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE,
>>>  	ICSSG_EMAC_PORT_VLAN_AWARE_DISABLE,
>>> +	ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE,
>>> +	ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE,
>>>  	ICSSG_EMAC_PORT_MAX_COMMANDS
>>>  };
>>>  
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>> index 676168d6fded..9af06454ba64 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>> @@ -41,7 +41,10 @@
>>>  #define DEFAULT_PORT_MASK	1
>>>  #define DEFAULT_UNTAG_MASK	1
>>>  
>>> -#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
>>> +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
>>> +						 NETIF_F_HW_HSR_DUP | \
>>> +						 NETIF_F_HW_HSR_TAG_INS | \
>>> +						 NETIF_F_HW_HSR_TAG_RM)
>>>  
>>>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>>>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>>> @@ -758,6 +761,21 @@ static void emac_change_hsr_feature(struct net_device *ndev,
>>>  	}
>>>  }
>>>  
>>> +static netdev_features_t emac_ndo_fix_features(struct net_device *ndev,
>>> +					       netdev_features_t features)
>>> +{
>>> +	/* In order to enable hsr tag insertion offload, hsr dup offload must
>>> +	 * also be enabled as these two are tightly coupled in firmware
>>> +	 * implementation.
>>> +	 */
>>> +	if (features & NETIF_F_HW_HSR_TAG_INS)
>>> +		features |= NETIF_F_HW_HSR_DUP;
>>
>> What if only NETIF_F_HW_HSR_DUP was set? Don't you have to set NETIF_F_HW_HSR_TAG_INS as well?
>>
>>> +	else
>>> +		features &= ~NETIF_F_HW_HSR_DUP;
>>
>> what if NETIF_F_HW_HSR_DUP was still set?
>>
>> I think you need to write a logic like follows.
>> 	if both are already cleared in ndev->features and any one is set in features you set both in features.
>> 	if both are already set in ndev->features and any one is cleared in features you clear both in features.
>>
>> is this reasonable?
>>
> 
> Yes that does sound reasonable,
> How does below code look to you.
> 
> 	if (!(ndev->features & NETIF_F_HW_HSR_DUP) &&
> 	    !(ndev->features & NETIF_F_HW_HSR_TAG_INS))

While this is OK. it could also be
	if (!(ndev->features & (NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_TAG_INS)))
Your choice.

> 		if ((features & NETIF_F_HW_HSR_DUP) ||
> 		    (features & NETIF_F_HW_HSR_TAG_INS)) {
> 			features |= NETIF_F_HW_HSR_DUP;
> 			features |= NETIF_F_HW_HSR_TAG_INS;
how about one line?
			features |= NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_TAG_INS;
> 		}

then you can get rid of the braces.

> 
> 	if ((ndev->features & NETIF_F_HW_HSR_DUP) &&
> 	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
> 		if (!(features & NETIF_F_HW_HSR_DUP) ||
> 		    !(features & NETIF_F_HW_HSR_TAG_INS)) {
> 			features &= ~NETIF_F_HW_HSR_DUP;
> 			features &= ~NETIF_F_HW_HSR_TAG_INS;

			features &= ~(NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_TAG_INS);

> 		}
> 
>>> +
>>> +	return features;
>>> +}
>>> +
>>>  static int emac_ndo_set_features(struct net_device *ndev,
>>>  				 netdev_features_t features)
>>>  {
>>> @@ -780,6 +798,7 @@ static const struct net_device_ops emac_netdev_ops = {
>>>  	.ndo_eth_ioctl = icssg_ndo_ioctl,
>>>  	.ndo_get_stats64 = icssg_ndo_get_stats64,
>>>  	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
>>> +	.ndo_fix_features = emac_ndo_fix_features,
>>>  	.ndo_set_features = emac_ndo_set_features,
>>>  };
>>>  
>>> @@ -1007,6 +1026,13 @@ static void icssg_change_mode(struct prueth *prueth)
>>>  
>>>  	for (mac = PRUETH_MAC0; mac < PRUETH_NUM_MACS; mac++) {
>>>  		emac = prueth->emac[mac];
>>> +		if (prueth->is_hsr_offload_mode) {
>>> +			if (emac->ndev->features & NETIF_F_HW_HSR_TAG_RM)
>>> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE);
>>> +			else
>>> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE);
>>> +		}
>>> +
>>>  		if (netif_running(emac->ndev)) {
>>>  			icssg_fdb_add_del(emac, eth_stp_addr, prueth->default_vlan,
>>>  					  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>> index a4b025fae797..bba6da2e6bd8 100644
>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>> @@ -59,6 +59,9 @@
>>>  
>>>  #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
>>>  
>>> +#define PRUETH_UNDIRECTED_PKT_DST_TAG	0
>>> +#define PRUETH_UNDIRECTED_PKT_TAG_INS	BIT(30)
>>> +
>>>  /* Firmware status codes */
>>>  #define ICSS_HS_FW_READY 0x55555555
>>>  #define ICSS_HS_FW_DEAD 0xDEAD0000	/* lower 16 bits contain error code */
>>
>
Anwar, Md Danish Sept. 10, 2024, 6:27 p.m. UTC | #4 
On 9/10/2024 11:36 PM, Roger Quadros wrote:
> 
> 
> On 10/09/2024 20:52, Anwar, Md Danish wrote:
>> Hi Roger,
>>
>> On 9/10/2024 11:08 PM, Roger Quadros wrote:
>>>
>>>
>>> On 06/09/2024 14:15, MD Danish Anwar wrote:
>>>> From: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>>>
>>>> The HSR stack allows to offload its Tx packet duplication functionality to
>>>> the hardware. Enable this offloading feature for ICSSG driver. Add support
>>>> to offload HSR Tx Tag Insertion and Rx Tag Removal and duplicate discard.
>>>>
>>>> Inorder to enable hsr-tag-ins-offload, hsr-dup-offload must also be enabled
>>>
>>> "In order"
>>>
>>>> as these are tightly coupled in the firmware implementation.
>>>>
>>>> Duplicate discard is done as part of RX tag removal and it is
>>>> done by the firmware. When driver sends the r30 command
>>>> ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware does RX tag removal as well as
>>>> duplicate discard.
>>>>
>>>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>> ---
>>>>  drivers/net/ethernet/ti/icssg/icssg_common.c | 18 ++++++++++---
>>>>  drivers/net/ethernet/ti/icssg/icssg_config.c |  4 ++-
>>>>  drivers/net/ethernet/ti/icssg/icssg_config.h |  2 ++
>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 28 +++++++++++++++++++-
>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  3 +++
>>>>  5 files changed, 50 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
>>>> index b9d8a93d1680..fdebeb2f84e0 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
>>>> @@ -660,14 +660,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>>>  {
>>>>  	struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
>>>>  	struct prueth_emac *emac = netdev_priv(ndev);
>>>> +	struct prueth *prueth = emac->prueth;
>>>>  	struct netdev_queue *netif_txq;
>>>>  	struct prueth_tx_chn *tx_chn;
>>>>  	dma_addr_t desc_dma, buf_dma;
>>>> +	u32 pkt_len, dst_tag_id;
>>>>  	int i, ret = 0, q_idx;
>>>>  	bool in_tx_ts = 0;
>>>>  	int tx_ts_cookie;
>>>>  	void **swdata;
>>>> -	u32 pkt_len;
>>>>  	u32 *epib;
>>>>  
>>>>  	pkt_len = skb_headlen(skb);
>>>> @@ -712,9 +713,20 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>>>  
>>>>  	/* set dst tag to indicate internal qid at the firmware which is at
>>>>  	 * bit8..bit15. bit0..bit7 indicates port num for directed
>>>> -	 * packets in case of switch mode operation
>>>> +	 * packets in case of switch mode operation and port num 0
>>>> +	 * for undirected packets in case of HSR offload mode
>>>>  	 */
>>>> -	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
>>>> +	dst_tag_id = emac->port_id | (q_idx << 8);
>>>> +
>>>> +	if (prueth->is_hsr_offload_mode &&
>>>> +	    (ndev->features & NETIF_F_HW_HSR_DUP))
>>>> +		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
>>>> +
>>>> +	if (prueth->is_hsr_offload_mode &&
>>>> +	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
>>>> +		epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
>>>> +
>>>> +	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
>>>>  	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>>>>  	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
>>>>  	swdata = cppi5_hdesc_get_swdata(first_desc);
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>> index 7b2e6c192ff3..72ace151d8e9 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>> @@ -531,7 +531,9 @@ static const struct icssg_r30_cmd emac_r32_bitmask[] = {
>>>>  	{{EMAC_NONE,  0xffff4000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx ENABLE*/
>>>>  	{{EMAC_NONE,  0xbfff0000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx DISABLE*/
>>>>  	{{0xffff0010,  EMAC_NONE, 0xffff0010, EMAC_NONE}},	/* VLAN AWARE*/
>>>> -	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}}	/* VLAN UNWARE*/
>>>> +	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}},	/* VLAN UNWARE*/
>>>> +	{{0xffff2000, EMAC_NONE, EMAC_NONE, EMAC_NONE}},	/* HSR_RX_OFFLOAD_ENABLE */
>>>> +	{{0xdfff0000, EMAC_NONE, EMAC_NONE, EMAC_NONE}}		/* HSR_RX_OFFLOAD_DISABLE */
>>>>  };
>>>>  
>>>>  int icssg_set_port_state(struct prueth_emac *emac,
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>> index 1ac60283923b..92c2deaa3068 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>> @@ -80,6 +80,8 @@ enum icssg_port_state_cmd {
>>>>  	ICSSG_EMAC_PORT_PREMPT_TX_DISABLE,
>>>>  	ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE,
>>>>  	ICSSG_EMAC_PORT_VLAN_AWARE_DISABLE,
>>>> +	ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE,
>>>> +	ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE,
>>>>  	ICSSG_EMAC_PORT_MAX_COMMANDS
>>>>  };
>>>>  
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>> index 676168d6fded..9af06454ba64 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>> @@ -41,7 +41,10 @@
>>>>  #define DEFAULT_PORT_MASK	1
>>>>  #define DEFAULT_UNTAG_MASK	1
>>>>  
>>>> -#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
>>>> +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
>>>> +						 NETIF_F_HW_HSR_DUP | \
>>>> +						 NETIF_F_HW_HSR_TAG_INS | \
>>>> +						 NETIF_F_HW_HSR_TAG_RM)
>>>>  
>>>>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>>>>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>>>> @@ -758,6 +761,21 @@ static void emac_change_hsr_feature(struct net_device *ndev,
>>>>  	}
>>>>  }
>>>>  
>>>> +static netdev_features_t emac_ndo_fix_features(struct net_device *ndev,
>>>> +					       netdev_features_t features)
>>>> +{
>>>> +	/* In order to enable hsr tag insertion offload, hsr dup offload must
>>>> +	 * also be enabled as these two are tightly coupled in firmware
>>>> +	 * implementation.
>>>> +	 */
>>>> +	if (features & NETIF_F_HW_HSR_TAG_INS)
>>>> +		features |= NETIF_F_HW_HSR_DUP;
>>>
>>> What if only NETIF_F_HW_HSR_DUP was set? Don't you have to set NETIF_F_HW_HSR_TAG_INS as well?
>>>
>>>> +	else
>>>> +		features &= ~NETIF_F_HW_HSR_DUP;
>>>
>>> what if NETIF_F_HW_HSR_DUP was still set?
>>>
>>> I think you need to write a logic like follows.
>>> 	if both are already cleared in ndev->features and any one is set in features you set both in features.
>>> 	if both are already set in ndev->features and any one is cleared in features you clear both in features.
>>>
>>> is this reasonable?
>>>
>>
>> Yes that does sound reasonable,
>> How does below code look to you.
>>
>> 	if (!(ndev->features & NETIF_F_HW_HSR_DUP) &&
>> 	    !(ndev->features & NETIF_F_HW_HSR_TAG_INS))
> 
> While this is OK. it could also be
> 	if (!(ndev->features & (NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_TAG_INS)))
> Your choice.
> 
>> 		if ((features & NETIF_F_HW_HSR_DUP) ||
>> 		    (features & NETIF_F_HW_HSR_TAG_INS)) {
>> 			features |= NETIF_F_HW_HSR_DUP;
>> 			features |= NETIF_F_HW_HSR_TAG_INS;
> how about one line?
> 			features |= NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_TAG_INS;
>> 		}
> 
> then you can get rid of the braces.
> 

Yes I could do this but this make the line length go beyond 80
characters. That's why I thought of putting this in two lines.

>>
>> 	if ((ndev->features & NETIF_F_HW_HSR_DUP) &&
>> 	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
>> 		if (!(features & NETIF_F_HW_HSR_DUP) ||
>> 		    !(features & NETIF_F_HW_HSR_TAG_INS)) {
>> 			features &= ~NETIF_F_HW_HSR_DUP;
>> 			features &= ~NETIF_F_HW_HSR_TAG_INS;
> 
> 			features &= ~(NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_TAG_INS);
> 
>> 		}
>>
>>>> +
>>>> +	return features;
>>>> +}
>>>> +
>>>>  static int emac_ndo_set_features(struct net_device *ndev,
>>>>  				 netdev_features_t features)
>>>>  {
>>>> @@ -780,6 +798,7 @@ static const struct net_device_ops emac_netdev_ops = {
>>>>  	.ndo_eth_ioctl = icssg_ndo_ioctl,
>>>>  	.ndo_get_stats64 = icssg_ndo_get_stats64,
>>>>  	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
>>>> +	.ndo_fix_features = emac_ndo_fix_features,
>>>>  	.ndo_set_features = emac_ndo_set_features,
>>>>  };
>>>>  
>>>> @@ -1007,6 +1026,13 @@ static void icssg_change_mode(struct prueth *prueth)
>>>>  
>>>>  	for (mac = PRUETH_MAC0; mac < PRUETH_NUM_MACS; mac++) {
>>>>  		emac = prueth->emac[mac];
>>>> +		if (prueth->is_hsr_offload_mode) {
>>>> +			if (emac->ndev->features & NETIF_F_HW_HSR_TAG_RM)
>>>> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE);
>>>> +			else
>>>> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE);
>>>> +		}
>>>> +
>>>>  		if (netif_running(emac->ndev)) {
>>>>  			icssg_fdb_add_del(emac, eth_stp_addr, prueth->default_vlan,
>>>>  					  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>> index a4b025fae797..bba6da2e6bd8 100644
>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>> @@ -59,6 +59,9 @@
>>>>  
>>>>  #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
>>>>  
>>>> +#define PRUETH_UNDIRECTED_PKT_DST_TAG	0
>>>> +#define PRUETH_UNDIRECTED_PKT_TAG_INS	BIT(30)
>>>> +
>>>>  /* Firmware status codes */
>>>>  #define ICSS_HS_FW_READY 0x55555555
>>>>  #define ICSS_HS_FW_DEAD 0xDEAD0000	/* lower 16 bits contain error code */
>>>
>>
>
Roger Quadros Sept. 10, 2024, 7:15 p.m. UTC | #5 
On 10/09/2024 21:27, Anwar, Md Danish wrote:
> 
> 
> On 9/10/2024 11:36 PM, Roger Quadros wrote:
>>
>>
>> On 10/09/2024 20:52, Anwar, Md Danish wrote:
>>> Hi Roger,
>>>
>>> On 9/10/2024 11:08 PM, Roger Quadros wrote:
>>>>
>>>>
>>>> On 06/09/2024 14:15, MD Danish Anwar wrote:
>>>>> From: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>>>>
>>>>> The HSR stack allows to offload its Tx packet duplication functionality to
>>>>> the hardware. Enable this offloading feature for ICSSG driver. Add support
>>>>> to offload HSR Tx Tag Insertion and Rx Tag Removal and duplicate discard.
>>>>>
>>>>> Inorder to enable hsr-tag-ins-offload, hsr-dup-offload must also be enabled
>>>>
>>>> "In order"
>>>>
>>>>> as these are tightly coupled in the firmware implementation.
>>>>>
>>>>> Duplicate discard is done as part of RX tag removal and it is
>>>>> done by the firmware. When driver sends the r30 command
>>>>> ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE, firmware does RX tag removal as well as
>>>>> duplicate discard.
>>>>>
>>>>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>>>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>>>> ---
>>>>>  drivers/net/ethernet/ti/icssg/icssg_common.c | 18 ++++++++++---
>>>>>  drivers/net/ethernet/ti/icssg/icssg_config.c |  4 ++-
>>>>>  drivers/net/ethernet/ti/icssg/icssg_config.h |  2 ++
>>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 28 +++++++++++++++++++-
>>>>>  drivers/net/ethernet/ti/icssg/icssg_prueth.h |  3 +++
>>>>>  5 files changed, 50 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
>>>>> index b9d8a93d1680..fdebeb2f84e0 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
>>>>> @@ -660,14 +660,15 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>>>>  {
>>>>>  	struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
>>>>>  	struct prueth_emac *emac = netdev_priv(ndev);
>>>>> +	struct prueth *prueth = emac->prueth;
>>>>>  	struct netdev_queue *netif_txq;
>>>>>  	struct prueth_tx_chn *tx_chn;
>>>>>  	dma_addr_t desc_dma, buf_dma;
>>>>> +	u32 pkt_len, dst_tag_id;
>>>>>  	int i, ret = 0, q_idx;
>>>>>  	bool in_tx_ts = 0;
>>>>>  	int tx_ts_cookie;
>>>>>  	void **swdata;
>>>>> -	u32 pkt_len;
>>>>>  	u32 *epib;
>>>>>  
>>>>>  	pkt_len = skb_headlen(skb);
>>>>> @@ -712,9 +713,20 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
>>>>>  
>>>>>  	/* set dst tag to indicate internal qid at the firmware which is at
>>>>>  	 * bit8..bit15. bit0..bit7 indicates port num for directed
>>>>> -	 * packets in case of switch mode operation
>>>>> +	 * packets in case of switch mode operation and port num 0
>>>>> +	 * for undirected packets in case of HSR offload mode
>>>>>  	 */
>>>>> -	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
>>>>> +	dst_tag_id = emac->port_id | (q_idx << 8);
>>>>> +
>>>>> +	if (prueth->is_hsr_offload_mode &&
>>>>> +	    (ndev->features & NETIF_F_HW_HSR_DUP))
>>>>> +		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
>>>>> +
>>>>> +	if (prueth->is_hsr_offload_mode &&
>>>>> +	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
>>>>> +		epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
>>>>> +
>>>>> +	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
>>>>>  	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>>>>>  	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
>>>>>  	swdata = cppi5_hdesc_get_swdata(first_desc);
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>>> index 7b2e6c192ff3..72ace151d8e9 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
>>>>> @@ -531,7 +531,9 @@ static const struct icssg_r30_cmd emac_r32_bitmask[] = {
>>>>>  	{{EMAC_NONE,  0xffff4000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx ENABLE*/
>>>>>  	{{EMAC_NONE,  0xbfff0000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx DISABLE*/
>>>>>  	{{0xffff0010,  EMAC_NONE, 0xffff0010, EMAC_NONE}},	/* VLAN AWARE*/
>>>>> -	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}}	/* VLAN UNWARE*/
>>>>> +	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}},	/* VLAN UNWARE*/
>>>>> +	{{0xffff2000, EMAC_NONE, EMAC_NONE, EMAC_NONE}},	/* HSR_RX_OFFLOAD_ENABLE */
>>>>> +	{{0xdfff0000, EMAC_NONE, EMAC_NONE, EMAC_NONE}}		/* HSR_RX_OFFLOAD_DISABLE */
>>>>>  };
>>>>>  
>>>>>  int icssg_set_port_state(struct prueth_emac *emac,
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>>> index 1ac60283923b..92c2deaa3068 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
>>>>> @@ -80,6 +80,8 @@ enum icssg_port_state_cmd {
>>>>>  	ICSSG_EMAC_PORT_PREMPT_TX_DISABLE,
>>>>>  	ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE,
>>>>>  	ICSSG_EMAC_PORT_VLAN_AWARE_DISABLE,
>>>>> +	ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE,
>>>>> +	ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE,
>>>>>  	ICSSG_EMAC_PORT_MAX_COMMANDS
>>>>>  };
>>>>>  
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>>> index 676168d6fded..9af06454ba64 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>>>>> @@ -41,7 +41,10 @@
>>>>>  #define DEFAULT_PORT_MASK	1
>>>>>  #define DEFAULT_UNTAG_MASK	1
>>>>>  
>>>>> -#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
>>>>> +#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
>>>>> +						 NETIF_F_HW_HSR_DUP | \
>>>>> +						 NETIF_F_HW_HSR_TAG_INS | \
>>>>> +						 NETIF_F_HW_HSR_TAG_RM)
>>>>>  
>>>>>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>>>>>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>>>>> @@ -758,6 +761,21 @@ static void emac_change_hsr_feature(struct net_device *ndev,
>>>>>  	}
>>>>>  }
>>>>>  
>>>>> +static netdev_features_t emac_ndo_fix_features(struct net_device *ndev,
>>>>> +					       netdev_features_t features)
>>>>> +{
>>>>> +	/* In order to enable hsr tag insertion offload, hsr dup offload must
>>>>> +	 * also be enabled as these two are tightly coupled in firmware
>>>>> +	 * implementation.
>>>>> +	 */
>>>>> +	if (features & NETIF_F_HW_HSR_TAG_INS)
>>>>> +		features |= NETIF_F_HW_HSR_DUP;
>>>>
>>>> What if only NETIF_F_HW_HSR_DUP was set? Don't you have to set NETIF_F_HW_HSR_TAG_INS as well?
>>>>
>>>>> +	else
>>>>> +		features &= ~NETIF_F_HW_HSR_DUP;
>>>>
>>>> what if NETIF_F_HW_HSR_DUP was still set?
>>>>
>>>> I think you need to write a logic like follows.
>>>> 	if both are already cleared in ndev->features and any one is set in features you set both in features.
>>>> 	if both are already set in ndev->features and any one is cleared in features you clear both in features.
>>>>
>>>> is this reasonable?
>>>>
>>>
>>> Yes that does sound reasonable,
>>> How does below code look to you.
>>>
>>> 	if (!(ndev->features & NETIF_F_HW_HSR_DUP) &&
>>> 	    !(ndev->features & NETIF_F_HW_HSR_TAG_INS))
>>
>> While this is OK. it could also be
>> 	if (!(ndev->features & (NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_TAG_INS)))
>> Your choice.
>>
>>> 		if ((features & NETIF_F_HW_HSR_DUP) ||
>>> 		    (features & NETIF_F_HW_HSR_TAG_INS)) {
>>> 			features |= NETIF_F_HW_HSR_DUP;
>>> 			features |= NETIF_F_HW_HSR_TAG_INS;
>> how about one line?
>> 			features |= NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_TAG_INS;
>>> 		}
>>
>> then you can get rid of the braces.
>>
> 
> Yes I could do this but this make the line length go beyond 80
> characters. That's why I thought of putting this in two lines.

then you can split the line
e.g.
			features |= NETIF_F_HW_HSR_DUP |
				    NETIF_F_HW_HSR_TAG_INS;
> 
>>>
>>> 	if ((ndev->features & NETIF_F_HW_HSR_DUP) &&
>>> 	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
>>> 		if (!(features & NETIF_F_HW_HSR_DUP) ||
>>> 		    !(features & NETIF_F_HW_HSR_TAG_INS)) {
>>> 			features &= ~NETIF_F_HW_HSR_DUP;
>>> 			features &= ~NETIF_F_HW_HSR_TAG_INS;
>>
>> 			features &= ~(NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_TAG_INS);
>>
>>> 		}
>>>
>>>>> +
>>>>> +	return features;
>>>>> +}
>>>>> +
>>>>>  static int emac_ndo_set_features(struct net_device *ndev,
>>>>>  				 netdev_features_t features)
>>>>>  {
>>>>> @@ -780,6 +798,7 @@ static const struct net_device_ops emac_netdev_ops = {
>>>>>  	.ndo_eth_ioctl = icssg_ndo_ioctl,
>>>>>  	.ndo_get_stats64 = icssg_ndo_get_stats64,
>>>>>  	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
>>>>> +	.ndo_fix_features = emac_ndo_fix_features,
>>>>>  	.ndo_set_features = emac_ndo_set_features,
>>>>>  };
>>>>>  
>>>>> @@ -1007,6 +1026,13 @@ static void icssg_change_mode(struct prueth *prueth)
>>>>>  
>>>>>  	for (mac = PRUETH_MAC0; mac < PRUETH_NUM_MACS; mac++) {
>>>>>  		emac = prueth->emac[mac];
>>>>> +		if (prueth->is_hsr_offload_mode) {
>>>>> +			if (emac->ndev->features & NETIF_F_HW_HSR_TAG_RM)
>>>>> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE);
>>>>> +			else
>>>>> +				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE);
>>>>> +		}
>>>>> +
>>>>>  		if (netif_running(emac->ndev)) {
>>>>>  			icssg_fdb_add_del(emac, eth_stp_addr, prueth->default_vlan,
>>>>>  					  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
>>>>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>>> index a4b025fae797..bba6da2e6bd8 100644
>>>>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
>>>>> @@ -59,6 +59,9 @@
>>>>>  
>>>>>  #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
>>>>>  
>>>>> +#define PRUETH_UNDIRECTED_PKT_DST_TAG	0
>>>>> +#define PRUETH_UNDIRECTED_PKT_TAG_INS	BIT(30)
>>>>> +
>>>>>  /* Firmware status codes */
>>>>>  #define ICSS_HS_FW_READY 0x55555555
>>>>>  #define ICSS_HS_FW_DEAD 0xDEAD0000	/* lower 16 bits contain error code */
>>>>
>>>
>>
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
index b9d8a93d1680..fdebeb2f84e0 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_common.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
@@ -660,14 +660,15 @@  enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
 {
 	struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
 	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth *prueth = emac->prueth;
 	struct netdev_queue *netif_txq;
 	struct prueth_tx_chn *tx_chn;
 	dma_addr_t desc_dma, buf_dma;
+	u32 pkt_len, dst_tag_id;
 	int i, ret = 0, q_idx;
 	bool in_tx_ts = 0;
 	int tx_ts_cookie;
 	void **swdata;
-	u32 pkt_len;
 	u32 *epib;
 
 	pkt_len = skb_headlen(skb);
@@ -712,9 +713,20 @@  enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
 
 	/* set dst tag to indicate internal qid at the firmware which is at
 	 * bit8..bit15. bit0..bit7 indicates port num for directed
-	 * packets in case of switch mode operation
+	 * packets in case of switch mode operation and port num 0
+	 * for undirected packets in case of HSR offload mode
 	 */
-	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
+	dst_tag_id = emac->port_id | (q_idx << 8);
+
+	if (prueth->is_hsr_offload_mode &&
+	    (ndev->features & NETIF_F_HW_HSR_DUP))
+		dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
+
+	if (prueth->is_hsr_offload_mode &&
+	    (ndev->features & NETIF_F_HW_HSR_TAG_INS))
+		epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
+
+	cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
 	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
 	cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
 	swdata = cppi5_hdesc_get_swdata(first_desc);
diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
index 7b2e6c192ff3..72ace151d8e9 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
@@ -531,7 +531,9 @@  static const struct icssg_r30_cmd emac_r32_bitmask[] = {
 	{{EMAC_NONE,  0xffff4000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx ENABLE*/
 	{{EMAC_NONE,  0xbfff0000, EMAC_NONE, EMAC_NONE}},	/* Preemption on Tx DISABLE*/
 	{{0xffff0010,  EMAC_NONE, 0xffff0010, EMAC_NONE}},	/* VLAN AWARE*/
-	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}}	/* VLAN UNWARE*/
+	{{0xffef0000,  EMAC_NONE, 0xffef0000, EMAC_NONE}},	/* VLAN UNWARE*/
+	{{0xffff2000, EMAC_NONE, EMAC_NONE, EMAC_NONE}},	/* HSR_RX_OFFLOAD_ENABLE */
+	{{0xdfff0000, EMAC_NONE, EMAC_NONE, EMAC_NONE}}		/* HSR_RX_OFFLOAD_DISABLE */
 };
 
 int icssg_set_port_state(struct prueth_emac *emac,
diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.h b/drivers/net/ethernet/ti/icssg/icssg_config.h
index 1ac60283923b..92c2deaa3068 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.h
@@ -80,6 +80,8 @@  enum icssg_port_state_cmd {
 	ICSSG_EMAC_PORT_PREMPT_TX_DISABLE,
 	ICSSG_EMAC_PORT_VLAN_AWARE_ENABLE,
 	ICSSG_EMAC_PORT_VLAN_AWARE_DISABLE,
+	ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE,
+	ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE,
 	ICSSG_EMAC_PORT_MAX_COMMANDS
 };
 
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 676168d6fded..9af06454ba64 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -41,7 +41,10 @@ 
 #define DEFAULT_PORT_MASK	1
 #define DEFAULT_UNTAG_MASK	1
 
-#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	NETIF_F_HW_HSR_FWD
+#define NETIF_PRUETH_HSR_OFFLOAD_FEATURES	(NETIF_F_HW_HSR_FWD | \
+						 NETIF_F_HW_HSR_DUP | \
+						 NETIF_F_HW_HSR_TAG_INS | \
+						 NETIF_F_HW_HSR_TAG_RM)
 
 /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
 #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
@@ -758,6 +761,21 @@  static void emac_change_hsr_feature(struct net_device *ndev,
 	}
 }
 
+static netdev_features_t emac_ndo_fix_features(struct net_device *ndev,
+					       netdev_features_t features)
+{
+	/* In order to enable hsr tag insertion offload, hsr dup offload must
+	 * also be enabled as these two are tightly coupled in firmware
+	 * implementation.
+	 */
+	if (features & NETIF_F_HW_HSR_TAG_INS)
+		features |= NETIF_F_HW_HSR_DUP;
+	else
+		features &= ~NETIF_F_HW_HSR_DUP;
+
+	return features;
+}
+
 static int emac_ndo_set_features(struct net_device *ndev,
 				 netdev_features_t features)
 {
@@ -780,6 +798,7 @@  static const struct net_device_ops emac_netdev_ops = {
 	.ndo_eth_ioctl = icssg_ndo_ioctl,
 	.ndo_get_stats64 = icssg_ndo_get_stats64,
 	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
+	.ndo_fix_features = emac_ndo_fix_features,
 	.ndo_set_features = emac_ndo_set_features,
 };
 
@@ -1007,6 +1026,13 @@  static void icssg_change_mode(struct prueth *prueth)
 
 	for (mac = PRUETH_MAC0; mac < PRUETH_NUM_MACS; mac++) {
 		emac = prueth->emac[mac];
+		if (prueth->is_hsr_offload_mode) {
+			if (emac->ndev->features & NETIF_F_HW_HSR_TAG_RM)
+				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_ENABLE);
+			else
+				icssg_set_port_state(emac, ICSSG_EMAC_HSR_RX_OFFLOAD_DISABLE);
+		}
+
 		if (netif_running(emac->ndev)) {
 			icssg_fdb_add_del(emac, eth_stp_addr, prueth->default_vlan,
 					  ICSSG_FDB_ENTRY_P0_MEMBERSHIP |
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index a4b025fae797..bba6da2e6bd8 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -59,6 +59,9 @@ 
 
 #define IEP_DEFAULT_CYCLE_TIME_NS	1000000	/* 1 ms */
 
+#define PRUETH_UNDIRECTED_PKT_DST_TAG	0
+#define PRUETH_UNDIRECTED_PKT_TAG_INS	BIT(30)
+
 /* Firmware status codes */
 #define ICSS_HS_FW_READY 0x55555555
 #define ICSS_HS_FW_DEAD 0xDEAD0000	/* lower 16 bits contain error code */