Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable RX HW timestamp only for PTP packets

[Chintan Vankar <c-vankar@ti.com>]

On 19/02/24 16:29, Roger Quadros wrote:
> Hi,
>
> On 15/02/2024 13:09, Chintan Vankar wrote:
>> The CPSW peripherals on J7AHP, J7VCL, J7AEP, J7ES, AM64 SoCs have
>> an errata i2401 "CPSW: Host Timestamps Cause CPSW Port to Lock up".
> Does this affect all platforms using am65-cpsw?

Yes. It affects all platforms using am65-cpsw. I will update a

commit message for the same.

> If this affects only some platforms then you need to restrict the
> workaround to those platforms only.
>
>> As a workaround, Disable timestamping on all RX packets and timestamp
>> only PTP packets.
> This does not exactly reflect what you are doing here.
>
> I think you need to clarify that you are disabling in-band RX timestamp
> mechanism on all packets and using out of band mechanism to get the
> RX timestamps only for certain type of packets.
>
>> Set Time Sync Receive bits in Time Sync control register so that
>> packets can be determined as a valid Ethernet Receive Event for time
>> synchronization.
>>
>> Update the RX filter configuration to indicate Hardware Timestamping
>> support only for PTP packets.
>>
>> Replace "am65_cpsw_rx_ts()" function with "am65_cpts_rx_timestamp()"
>> function which timestamps only PTP packets.
>>
>> Fixes: b1f66a5bee07 ("net: ethernet: ti: am65-cpsw-nuss: enable packet timestamping support")
>> Signed-off-by: Chintan Vankar <c-vankar@ti.com>
>> ---
>>   drivers/net/ethernet/ti/am65-cpsw-nuss.c | 51 +++++++++++-------------
>>   1 file changed, 24 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index 9d2f4ac783e4..ab843fb64b93 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -101,6 +101,12 @@
>>   #define AM65_CPSW_PN_TS_CTL_TX_HOST_TS_EN	BIT(11)
>>   #define AM65_CPSW_PN_TS_CTL_MSG_TYPE_EN_SHIFT	16
>>   
>> +#define AM65_CPSW_PN_TS_CTL_RX_ANX_F_EN		BIT(0)
>> +#define AM65_CPSW_PN_TS_CTL_RX_VLAN_LT1_EN	BIT(1)
>> +#define AM65_CPSW_PN_TS_CTL_RX_VLAN_LT2_EN	BIT(2)
>> +#define AM65_CPSW_PN_TS_CTL_RX_ANX_D_EN		BIT(3)
>> +#define AM65_CPSW_PN_TS_CTL_RX_ANX_E_EN		BIT(9)
>> +
>>   /* AM65_CPSW_PORTN_REG_TS_SEQ_LTYPE_REG register fields */
>>   #define AM65_CPSW_PN_TS_SEQ_ID_OFFSET_SHIFT	16
>>   
>> @@ -124,6 +130,11 @@
>>   	 AM65_CPSW_PN_TS_CTL_TX_ANX_E_EN |	\
>>   	 AM65_CPSW_PN_TS_CTL_TX_ANX_F_EN)
>>   
>> +#define AM65_CPSW_TS_RX_ANX_ALL_EN		\
>> +	(AM65_CPSW_PN_TS_CTL_RX_ANX_D_EN |	\
>> +	 AM65_CPSW_PN_TS_CTL_RX_ANX_E_EN |	\
>> +	 AM65_CPSW_PN_TS_CTL_RX_ANX_F_EN)
>> +
>>   #define AM65_CPSW_ALE_AGEOUT_DEFAULT	30
>>   /* Number of TX/RX descriptors */
>>   #define AM65_CPSW_MAX_TX_DESC	500
>> @@ -749,18 +760,6 @@ static int am65_cpsw_nuss_ndo_slave_open(struct net_device *ndev)
>>   	return ret;
>>   }
>>   
>> -static void am65_cpsw_nuss_rx_ts(struct sk_buff *skb, u32 *psdata)
>> -{
>> -	struct skb_shared_hwtstamps *ssh;
>> -	u64 ns;
>> -
>> -	ns = ((u64)psdata[1] << 32) | psdata[0];
>> -
>> -	ssh = skb_hwtstamps(skb);
>> -	memset(ssh, 0, sizeof(*ssh));
>> -	ssh->hwtstamp = ns_to_ktime(ns);
>> -}
>> -
>>   /* RX psdata[2] word format - checksum information */
>>   #define AM65_CPSW_RX_PSD_CSUM_ADD	GENMASK(15, 0)
>>   #define AM65_CPSW_RX_PSD_CSUM_ERR	BIT(16)
>> @@ -841,9 +840,6 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_common *common,
>>   	skb->dev = ndev;
>>   
>>   	psdata = cppi5_hdesc_get_psdata(desc_rx);
>> -	/* add RX timestamp */
>> -	if (port->rx_ts_enabled)
>> -		am65_cpsw_nuss_rx_ts(skb, psdata);
>>   	csum_info = psdata[2];
>>   	dev_dbg(dev, "%s rx csum_info:%#x\n", __func__, csum_info);
>>   
>> @@ -856,6 +852,9 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_common *common,
>>   		ndev_priv = netdev_priv(ndev);
>>   		am65_cpsw_nuss_set_offload_fwd_mark(skb, ndev_priv->offload_fwd_mark);
>>   		skb_put(skb, pkt_len);
>> +		skb_reset_mac_header(skb);
> Why do you need to add skb_reset_mac_header here?
>
>> +		if (port->rx_ts_enabled)
>> +			am65_cpts_rx_timestamp(common->cpts, skb);
> The timestamp should be added before you do skb_put().
>
>>   		skb->protocol = eth_type_trans(skb, ndev);
>>   		am65_cpsw_nuss_rx_csum(skb, csum_info);
>>   		napi_gro_receive(&common->napi_rx, skb);
>> @@ -1334,7 +1333,6 @@ static int am65_cpsw_nuss_ndo_slave_set_mac_address(struct net_device *ndev,
>>   static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
>>   				       struct ifreq *ifr)
>>   {
>> -	struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
>>   	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
>>   	u32 ts_ctrl, seq_id, ts_ctrl_ltype2, ts_vlan_ltype;
>>   	struct hwtstamp_config cfg;
>> @@ -1358,11 +1356,6 @@ static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
>>   	case HWTSTAMP_FILTER_NONE:
>>   		port->rx_ts_enabled = false;
>>   		break;
>> -	case HWTSTAMP_FILTER_ALL:
>> -	case HWTSTAMP_FILTER_SOME:
>> -	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
>> -	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
>> -	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> Can't PTP_V1 be supported?

No. As per TRM:

https://www.ti.com/lit/ug/spruim2h/spruim2h.pdf,

section 12.2.1.4.7 Common Platform Time Sync(CPTS)

only IEEE 1588-2008 which is PTP v2 is supported.

>
>>   	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
>>   	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
>>   	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
>> @@ -1372,10 +1365,13 @@ static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
>>   	case HWTSTAMP_FILTER_PTP_V2_EVENT:
>>   	case HWTSTAMP_FILTER_PTP_V2_SYNC:
>>   	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
>> -	case HWTSTAMP_FILTER_NTP_ALL:
>>   		port->rx_ts_enabled = true;
>> -		cfg.rx_filter = HWTSTAMP_FILTER_ALL;
>> +		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
>>   		break;
>> +	case HWTSTAMP_FILTER_ALL:
>> +	case HWTSTAMP_FILTER_SOME:
>> +	case HWTSTAMP_FILTER_NTP_ALL:
>> +		return -EOPNOTSUPP;
>>   	default:
>>   		return -ERANGE;
>>   	}
>> @@ -1405,6 +1401,10 @@ static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
>>   		ts_ctrl |= AM65_CPSW_TS_TX_ANX_ALL_EN |
>>   			   AM65_CPSW_PN_TS_CTL_TX_VLAN_LT1_EN;
>>   
>> +	if (port->rx_ts_enabled)
>> +		ts_ctrl |= AM65_CPSW_TS_RX_ANX_ALL_EN |
>> +			   AM65_CPSW_PN_TS_CTL_RX_VLAN_LT1_EN;
>> +
>>   	writel(seq_id, port->port_base + AM65_CPSW_PORTN_REG_TS_SEQ_LTYPE_REG);
>>   	writel(ts_vlan_ltype, port->port_base +
>>   	       AM65_CPSW_PORTN_REG_TS_VLAN_LTYPE_REG);
>> @@ -1412,9 +1412,6 @@ static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
>>   	       AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2);
>>   	writel(ts_ctrl, port->port_base + AM65_CPSW_PORTN_REG_TS_CTL);
>>   
>> -	/* en/dis RX timestamp */
>> -	am65_cpts_rx_enable(common->cpts, port->rx_ts_enabled);
>> -
>>   	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
>>   }
>>   
>> @@ -1431,7 +1428,7 @@ static int am65_cpsw_nuss_hwtstamp_get(struct net_device *ndev,
>>   	cfg.tx_type = port->tx_ts_enabled ?
>>   		      HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
>>   	cfg.rx_filter = port->rx_ts_enabled ?
>> -			HWTSTAMP_FILTER_ALL : HWTSTAMP_FILTER_NONE;
>> +			HWTSTAMP_FILTER_PTP_V2_EVENT : HWTSTAMP_FILTER_NONE;
>>   
>>   	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
>>   }